Skip to content

fix: preserve scroll position when reading LLM output #7975

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

fix: preserve scroll position when reading LLM output #7975

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 27 additions & 8 deletions webview-ui/src/components/chat/ChatView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,7 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro
const prevExpandedRowsRef = useRef<Record<number, boolean>>()
const scrollContainerRef = useRef<HTMLDivElement>(null)
const disableAutoScrollRef = useRef(false)
const userHasScrolledUpRef = useRef(false)
Copy link
Contributor Author

@roomote roomote bot Sep 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

const [showScrollToBottom, setShowScrollToBottom] = useState(false)
const [isAtBottom, setIsAtBottom] = useState(false)
const lastTtsRef = useRef<string>("")
Expand Down Expand Up @@ -477,6 +478,9 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro
}
// Reset user response flag for new task
userRespondedRef.current = false
// Reset scroll state for new task
disableAutoScrollRef.current = false
userHasScrolledUpRef.current = false
Copy link
Contributor Author

@roomote roomote bot Sep 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}, [task?.ts])

useEffect(() => {
Expand Down Expand Up @@ -508,6 +512,7 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro

if (wasAnyRowExpandedByUser) {
disableAutoScrollRef.current = true
userHasScrolledUpRef.current = true
}
prevExpandedRowsRef.current = expandedRows // Store current state for next comparison
}, [expandedRows])
Expand Down Expand Up @@ -582,6 +587,7 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro
// setPrimaryButtonText(undefined)
// setSecondaryButtonText(undefined)
disableAutoScrollRef.current = false
userHasScrolledUpRef.current = false
}, [])

/**
Expand Down Expand Up @@ -1407,16 +1413,25 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro
}
}, [groupedMessages.length, scrollToBottomSmooth])

const handleWheel = useCallback((event: Event) => {
const wheelEvent = event as WheelEvent
const handleWheel = useCallback(
(event: Event) => {
const wheelEvent = event as WheelEvent

if (wheelEvent.deltaY && wheelEvent.deltaY < 0) {
if (scrollContainerRef.current?.contains(wheelEvent.target as Node)) {
Copy link
Contributor Author

@roomote roomote bot Sep 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// User scrolled up
disableAutoScrollRef.current = true
if (wheelEvent.deltaY < 0) {
// User scrolled up - disable auto-scroll and mark that user has scrolled up
disableAutoScrollRef.current = true
userHasScrolledUpRef.current = true
Copy link
Contributor Author

@roomote roomote bot Sep 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

} else if (wheelEvent.deltaY > 0 && isAtBottom) {
// User scrolled down while at bottom - re-enable auto-scroll
// This indicates intentional scrolling to bottom to resume following output
disableAutoScrollRef.current = false
userHasScrolledUpRef.current = false
}
}
}
}, [])
},
[isAtBottom],
)

useEvent("wheel", handleWheel, window, { passive: true }) // passive improves scrolling performance

Expand Down Expand Up @@ -1880,7 +1895,9 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro
itemContent={itemContent}
atBottomStateChange={(isAtBottom: boolean) => {
setIsAtBottom(isAtBottom)
if (isAtBottom) {
// Only re-enable auto-scroll if user hasn't manually scrolled up
// When user manually scrolls to bottom, the wheel handler will re-enable it
if (isAtBottom && !userHasScrolledUpRef.current) {
disableAutoScrollRef.current = false
}
setShowScrollToBottom(disableAutoScrollRef.current && !isAtBottom)
Expand All @@ -1905,7 +1922,9 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro
className="flex-[2]"
onClick={() => {
scrollToBottomSmooth()
// User explicitly clicked to scroll to bottom - re-enable auto-scroll
disableAutoScrollRef.current = false
userHasScrolledUpRef.current = false
}}>
<span className="codicon codicon-chevron-down"></span>
</VSCodeButton>
Expand Down
Loading