Skip to content

Conversation

@roomote
Copy link

@roomote roomote bot commented Oct 25, 2025

This PR attempts to address Issue #8812. Feedback and guidance are welcome.

Problem

When the model list is long enough to scroll, the pinned models at the top scroll away with the rest of the list, which defeats the purpose of pinning them for quick access.

Solution

  • Separated pinned and unpinned configs into different containers
  • Pinned configs now stay fixed at the top
  • Only unpinned configs are scrollable
  • Dynamic height calculation ensures optimal use of space

Changes

  • Modified ApiConfigSelector.tsx to render pinned items in a fixed container and unpinned items in a scrollable container
  • Added comprehensive tests to verify the fixed behavior
  • No breaking changes to existing functionality

Testing

  • All existing tests pass
  • Added two new test cases to verify:
    1. Pinned configs remain fixed at top while unpinned configs scroll
    2. All configs display in scrollable container when no configs are pinned

Screenshots:

Before
2025-11-04_13-35-46
After
2025-11-04_13-36-48

Fixes #8812


Important

Fixes pinned models scrolling issue by separating pinned and unpinned models in ApiConfigSelector.tsx, ensuring pinned models remain fixed.

  • Behavior:
    • Pinned models remain fixed at the top of the list in ApiConfigSelector.tsx.
    • Unpinned models are scrollable, separated from pinned models.
    • Dynamic height calculation for optimal space usage.
  • Testing:
    • Added tests in ApiConfigSelector.spec.tsx to verify pinned models remain fixed and unpinned models scroll.
    • Tests for scenarios with no pinned models and all models in a scrollable container.
  • Misc:
    • No breaking changes to existing functionality.

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

- Separated pinned and unpinned configs into different containers
- Pinned configs now stay fixed at the top
- Only unpinned configs are scrollable
- Added tests to verify the fixed behavior

Fixes #8812
@roomote roomote bot requested review from cte, jr and mrubens as code owners October 25, 2025 01:16
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. bug Something isn't working labels Oct 25, 2025
@roomote
Copy link
Author

roomote bot commented Oct 25, 2025

See this task on Roo Code Cloud

Review Summary

All previously identified issues have been resolved. The refactored implementation using CSS sticky positioning is much cleaner and more maintainable than the previous dynamic height calculation approach.

  • Fix negative maxHeight calculation when 9+ configs are pinned (line 220)
  • Replace hardcoded 36px item height with dynamic calculation (line 220)
  • Account for separator height in maxHeight calculation (lines 209-211)
Previous Reviews

Mention @roomote in a comment to trigger your PR Fixer agent and make changes to this pull request.

Copy link
Author

@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.

Review complete. I found 3 issues that should be addressed before this PR can be merged. Please see the inline comments for details.

@XiaoYingYo
Copy link

XiaoYingYo commented Oct 25, 2025

Success, looks good
@roomote

@hannesrudolph hannesrudolph added the Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. label Oct 25, 2025
@daniel-lxs daniel-lxs moved this from Triage to PR [Needs Prelim Review] in Roo Code Roadmap Oct 29, 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 Oct 29, 2025
- Changed sticky header background from bg-vscode-editorWidget-background to bg-vscode-dropdown-background to match popover container
- Moved separator logic into sticky container as conditional bottom border to prevent scroll artifacts
- Updated tests to match new separator structure
- All 21 tests passing
@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Nov 4, 2025
@daniel-lxs daniel-lxs moved this from PR [Needs Prelim Review] to PR [Needs Review] in Roo Code Roadmap Nov 4, 2025
@mrubens mrubens merged commit 200a7c8 into main Nov 5, 2025
15 checks passed
@mrubens mrubens deleted the fix/pinned-models-scrolling-issue-8812 branch November 5, 2025 03:01
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Nov 5, 2025
@github-project-automation github-project-automation bot moved this from PR [Needs Review] to Done in Roo Code Roadmap Nov 5, 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 lgtm This PR has been approved by a maintainer PR - Needs Review size:L This PR changes 100-499 lines, ignoring generated files.

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Pinned model options should not scroll with the model list

6 participants