-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: resolve scroll jumping issue on smaller screens #7028
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
- Increased message limit threshold from 500 to 750 messages - Only apply message limiting for very long conversations (>1000 messages) - Balanced viewport rendering (1500px top/bottom instead of 3000px/1000px) - Increased LRU cache capacity from 100 to 200 entries with longer TTL - Expanded viewport tracking window from 100 to 200 messages These changes prevent aggressive message filtering that was causing scroll position jumps while maintaining memory optimizations from PR #6697. Fixes #7026
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 a mirror - everything looks backwards but the bugs are still mine.
| const visibleMessages = useMemo(() => { | ||
| // Use a more conservative approach for message filtering to prevent jumping | ||
| // Only apply the 500 message limit for very long conversations | ||
| const currentMessageCount = modifiedMessages.length |
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.
These magic numbers (1000, 750, 200) could benefit from being defined as named constants with comments explaining why these specific values were chosen. Something like:
This would make the code more maintainable and the reasoning clearer.
| new LRUCache({ | ||
| max: 100, | ||
| ttl: 1000 * 60 * 5, | ||
| max: 200, |
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.
Doubling the cache size and TTL partially reverses the memory optimizations from PR #6697. While this helps with scroll stability, have we considered if there's a way to achieve the same result without increasing memory usage? Perhaps we could be smarter about which messages to cache based on scroll direction or user behavior patterns?
| key={task.ts} | ||
| className="scrollable grow overflow-y-scroll mb-1" | ||
| increaseViewportBy={{ top: 3_000, bottom: 1000 }} | ||
| increaseViewportBy={{ top: 1500, bottom: 1500 }} |
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 symmetric viewport values might not be optimal since users typically scroll up more than down when reviewing conversation history. The previous asymmetric values (3000px top, 1000px bottom) might have been intentional. Could we consider keeping more buffer at the top, perhaps 2000px top and 1000px bottom?
| const currentMessageCount = modifiedMessages.length | ||
| const startIndex = Math.max(0, currentMessageCount - 500) | ||
| const recentMessages = modifiedMessages.slice(startIndex) | ||
| const shouldLimitMessages = currentMessageCount > 1000 |
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 gradual scaling approach is good, but for medium-sized conversations (500-800 messages), we're now processing all messages which could impact performance. Consider implementing a more gradual scaling, perhaps:
Summary
This PR fixes the scroll jumping issue reported in #7026 that occurs when scrolling in the Roo window on smaller screens or laptops.
Problem
After PR #6697 fixed memory leaks by optimizing virtual scrolling, users experienced scroll position jumping when trying to scroll up to read previous messages, especially on smaller screens. The aggressive message filtering and viewport management was causing messages to be dynamically added/removed, disrupting the scroll position.
Solution
This fix takes a more conservative approach to message filtering while maintaining the memory optimizations:
Changes Made:
Testing
Related Issues
Impact
Users on smaller screens or laptops will now experience smooth, stable scrolling without position jumps while still benefiting from the memory optimizations that prevent performance degradation in long conversations.
Important
Fixes scroll jumping in
ChatView.tsxby adjusting message filtering and viewport settings for better performance on smaller screens.ChatView.tsxby adjusting message filtering and viewport settings.This description was created by
for 01ed9cc. You can customize this summary. It will automatically update as commits are pushed.