-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: improve auto-scroll behavior during API activities and streaming #6989
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Keep auto-scroll enabled during streaming unless user explicitly scrolls up - Prevent disabling auto-scroll when expanding rows during streaming - Improve scroll-to-bottom logic to handle API activities better - Fix issue where scrollbar jumps up during long conversations Fixes #6987
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing my own code is like debugging in production - technically possible but morally questionable.
| if (wheelEvent.deltaY && wheelEvent.deltaY < 0) { | ||
| if (scrollContainerRef.current?.contains(wheelEvent.target as Node)) { | ||
| // User scrolled up - but don't disable during streaming if we're near bottom | ||
| if (!isStreaming || !isAtBottom) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this intentional? The isAtBottom state might not be immediately updated when checking in the wheel handler, potentially causing a race condition. Consider using a ref or checking the scroll position directly to avoid stale state issues.
| disableAutoScrollRef.current = false | ||
| } | ||
| // During streaming, always keep auto-scroll enabled unless user explicitly scrolled | ||
| if (isStreaming && isAtBottom) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This second condition checking isStreaming && isAtBottom appears redundant since if isAtBottom is true, we just set disableAutoScrollRef.current = false on line 1886. Could we simplify this logic?
| disableAutoScrollRef.current = false | ||
| } | ||
| // During streaming, always keep auto-scroll enabled unless user explicitly scrolled | ||
| if (isStreaming && isAtBottom) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a comment here explaining why auto-scroll behaves differently during streaming. Something like:
| let timer: ReturnType<typeof setTimeout> | undefined | ||
| if (!disableAutoScrollRef.current) { | ||
| // Always scroll to bottom during streaming unless user explicitly scrolled up | ||
| if (!disableAutoScrollRef.current || isStreaming) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes to auto-scroll logic during streaming are significant. Have we considered adding test coverage for this new behavior? It would help ensure the fix works correctly across different scenarios.
| } | ||
|
|
||
| // Don't disable auto-scroll during streaming even if rows are expanded | ||
| if (wasAnyRowExpandedByUser && !isStreaming) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For future maintainability, consider extracting the auto-scroll logic into a custom hook like useAutoScroll. This would centralize the complex conditions and make the behavior easier to test and understand.
|
Closing this PR as the issue has been resolved in PR #7064 which removed the 500-message limit that was causing the scrollbar jumping. The fix has been merged to main. Let me know if you notice this issue again! |
Summary
This PR fixes the scrollbar auto-scroll issue where the conversation window would jump up during API activities and not stay at the bottom, especially in longer conversations.
Problem
Solution
Changes Made
Testing
Fixes #6987
Important
Fixes auto-scroll behavior in
ChatView.tsxto remain at the bottom during streaming and API activities unless user scrolls up.ChatView.tsx.useEffectto prevent disabling auto-scroll during streaming when rows are expanded.handleWheelto avoid disabling auto-scroll during streaming if near bottom.atBottomStateChangeto maintain auto-scroll during streaming.This description was created by
for 0c3a8ba. You can customize this summary. It will automatically update as commits are pushed.