Skip to content

Conversation

@hannesrudolph
Copy link
Collaborator

@hannesrudolph hannesrudolph commented Aug 6, 2025

Problem

PR #6697 fixed a memory leak by reducing the increaseViewportBy bottom value from Number.MAX_SAFE_INTEGER to 1000, but this caused scroll lock issues where the chat view doesn't always stay pinned to the bottom when it should.

Solution

This PR implements a dynamic approach that balances both memory efficiency and proper scroll behavior:

  • When scrolled to bottom: Uses a larger viewport buffer (10,000px) to ensure smooth scroll lock behavior
  • When scrolled up: Uses the smaller buffer (1,000px) to preserve memory efficiency
  • Added followOutput='smooth': Improves the scrolling animation when new content arrives

Technical Details

The key change is making the increaseViewportBy.bottom value dynamic based on the isAtBottom state:

increaseViewportBy={{ 
  top: 3_000, 
  // Use a dynamic bottom value: larger when at bottom to maintain scroll lock,
  // smaller when scrolled up to preserve memory efficiency
  bottom: isAtBottom ? 10_000 : 1000 
}}

This approach ensures:

  1. ✅ Memory efficiency is maintained when users scroll up to review messages
  2. ✅ Scroll stays locked to bottom when new messages arrive (when already at bottom)
  3. ✅ No unbounded memory growth from rendering all messages

Testing

  • All existing tests pass
  • Manual testing confirms scroll lock behavior is restored
  • Memory usage remains bounded

Fixes the scroll lock regression introduced in #6697 while preserving the memory leak fix.


Important

Dynamic viewport buffer adjustment in ChatView.tsx to fix scroll lock issues while maintaining memory efficiency.

  • Behavior:
    • Dynamic increaseViewportBy.bottom in ChatView.tsx based on isAtBottom state: 10,000px when at bottom, 1,000px when scrolled up.
    • Adds followOutput='smooth' for smoother scrolling when new content arrives.
  • State Management:
    • Introduces debouncedIsAtBottom state to prevent rapid viewport buffer changes.
    • Updates atBottomStateChange to manage scroll lock and visibility of scroll-to-bottom button.
  • Constants:
    • Defines VIEWPORT_BUFFER_AT_BOTTOM, VIEWPORT_BUFFER_SCROLLED_UP, and VIEWPORT_BUFFER_TOP for viewport buffer management.

This description was created by Ellipsis for d368697. You can customize this summary. It will automatically update as commits are pushed.

- Use dynamic increaseViewportBy bottom value based on scroll position
- When at bottom, use larger value (10,000px) to maintain scroll lock
- When scrolled up, use smaller value (1,000px) for memory efficiency
- Add followOutput='smooth' for better scroll behavior
- Fixes scroll lock issues introduced in PR #6697
Copilot AI review requested due to automatic review settings August 6, 2025 18:49
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes scroll lock behavior issues that were introduced when fixing a memory leak in PR #6697. The solution implements a dynamic viewport buffer approach that maintains both memory efficiency and proper scroll behavior.

  • Implements dynamic increaseViewportBy.bottom values based on scroll position
  • Adds smooth scrolling animation with followOutput='smooth'
  • Preserves memory efficiency while restoring scroll lock functionality

@dosubot dosubot bot added size:XS This PR changes 0-9 lines, ignoring generated files. bug Something isn't working labels Aug 6, 2025
Copy link
Contributor

@roomote roomote bot left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! I've reviewed the changes and have some suggestions for improvement. The dynamic viewport approach is clever and should effectively address the scroll lock issue while maintaining memory efficiency.

}}
atBottomThreshold={10}
initialTopMostItemIndex={groupedMessages.length - 1}
followOutput="smooth"
Copy link
Contributor

Choose a reason for hiding this comment

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

Good addition! The smooth scrolling should improve UX. Could you add a comment explaining how this relates to the scroll lock fix? This would help future maintainers understand the complete solution.

@hannesrudolph hannesrudolph added the Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. label Aug 6, 2025
- Extract magic numbers into named constants (VIEWPORT_BUFFER_AT_BOTTOM, VIEWPORT_BUFFER_SCROLLED_UP, VIEWPORT_BUFFER_TOP)
- Add detailed comments explaining how followOutput='smooth' relates to the scroll lock fix
- Implement debouncing (300ms) for isAtBottom state to prevent rapid viewport buffer changes
- Add hysteresis to avoid performance issues during rapid scrolling

This maintains the balance between memory efficiency and proper scroll lock behavior while addressing all reviewer concerns.
@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:XS This PR changes 0-9 lines, ignoring generated files. labels Aug 6, 2025
- Remove verbose comments and references to other PRs
- Keep comments concise and focused on what the code does
@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Aug 6, 2025
@hannesrudolph hannesrudolph moved this from Triage to PR [Needs Prelim Review] in Roo Code Roadmap Aug 6, 2025
@hannesrudolph hannesrudolph added PR - Needs Preliminary Review and removed Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. labels Aug 6, 2025
@daniel-lxs
Copy link
Member

Fixed by #6780

@daniel-lxs daniel-lxs closed this Aug 7, 2025
@github-project-automation github-project-automation bot moved this from PR [Needs Prelim Review] to Done in Roo Code Roadmap Aug 7, 2025
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Aug 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working PR - Needs Preliminary Review size:S This PR changes 10-29 lines, ignoring generated files.

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants