fix: preserve scroll position when reading LLM output #7975
Conversation
- Added userHasScrolledUpRef to track when user manually scrolls up - Modified wheel event handler to detect scroll direction - Only re-enable auto-scroll when user explicitly scrolls down to bottom - Prevent auto-scroll from re-enabling when reaching bottom passively - Reset scroll state when starting new tasks Fixes #7974
dosubot
bot
added
size:M
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
| if (wheelEvent.deltaY < 0) { | ||
| // User scrolled up - disable auto-scroll and mark that user has scrolled up | ||
| disableAutoScrollRef.current = true | ||
| userHasScrolledUpRef.current = true |
There was a problem hiding this comment.
Is this intentional? We're using refs () that don't trigger re-renders, but checking them in callbacks that might use stale values. Could this create a race condition where (line 1900) reads an outdated value?
Maybe consider using state instead of refs, or ensure the callback always has the latest ref value?
| const prevExpandedRowsRef = useRef<Record<number, boolean>>() | ||
| const scrollContainerRef = useRef<HTMLDivElement>(null) | ||
| const disableAutoScrollRef = useRef(false) | ||
| const userHasScrolledUpRef = useRef(false) |
There was a problem hiding this comment.
The dual-state tracking system with both and works, but could we simplify this? They seem to track similar concepts - maybe a single state enum like would be clearer?
| userRespondedRef.current = false | ||
| // Reset scroll state for new task | ||
| disableAutoScrollRef.current = false | ||
| userHasScrolledUpRef.current = false |
There was a problem hiding this comment.
Good that we're resetting scroll state for new tasks! However, this critical UX feature would benefit from test coverage. Could we add tests verifying:
- Scroll state preservation when user scrolls up
- Auto-scroll resumption when scrolling to bottom
- Proper state reset on new tasks
| const wheelEvent = event as WheelEvent | ||
|
|
||
| if (wheelEvent.deltaY && wheelEvent.deltaY < 0) { | ||
| if (scrollContainerRef.current?.contains(wheelEvent.target as Node)) { |
There was a problem hiding this comment.
Nice implementation of the scroll direction detection! Consider adding a comment block here explaining the dual-state tracking system for future maintainers, as the interaction between and might not be immediately obvious.
hannesrudolph
added
the
Issue/PR - Triage
hannesrudolph
added
PR - Needs Preliminary Review
and removed
Issue/PR - Triage
github-project-automation
bot
moved this from PR [Needs Prelim Review]
to Done
in Roo Code Roadmap
This PR attempts to address Issue #7974 by improving the auto-scroll behavior in the chat view.
Problem
Users were unable to read earlier parts of LLM output because the window would automatically scroll to the bottom whenever new content arrived, interrupting their reading experience.
Solution
Implemented a dual-state tracking system that:
userHasScrolledUpRef)Changes
userHasScrolledUpRefto track manual scroll stateatBottomStateChangecallback to respect user scroll preferencesTesting
Fixes #7974
Feedback and guidance are welcome!
Important
Improves chat view scroll behavior in
ChatView.tsxby tracking user scroll actions to manage auto-scroll state.userHasScrolledUpRefinChatView.tsxto track manual scroll actions.handleWheelto detect scroll direction and adjustdisableAutoScrollRefanduserHasScrolledUpRef.atBottomStateChangeto respectuserHasScrolledUpRef.This description was created by
for a81dc52. You can customize this summary. It will automatically update as commits are pushed.