fix: keep pinned models fixed at top of scrollable list #8813
Conversation
- 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
dosubot
bot
added
size:L
Review SummaryI've reviewed the changes and identified 3 issues that should be addressed:
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
| style={{ | ||
| maxHeight: | ||
| pinnedConfigs.length > 0 | ||
| ? `calc(300px - ${pinnedConfigs.length * 36}px)` |
There was a problem hiding this comment.
The dynamic maxHeight calculation may yield a negative value if there are too many pinned configs. Consider adding a guard to prevent negative heights.
| ? `calc(300px - ${pinnedConfigs.length * 36}px)` | |
| ? `calc(300px - ${Math.max(0, pinnedConfigs.length * 36)}px)` |
This comment was generated because it violated a code review rule: irule_JHt4LrVpts9BYI6Q.
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
| style={{ | ||
| maxHeight: | ||
| pinnedConfigs.length > 0 | ||
| ? `calc(300px - ${pinnedConfigs.length * 36}px)` |
There was a problem hiding this comment.
The maxHeight calculation can produce negative values when 9 or more configs are pinned. For example, with 9 pinned configs: calc(300px - 324px) = -24px, which will hide the scrollable container entirely. This happens when users pin many configs or when a search query returns multiple pinned results. Consider adding a minimum value or dynamically calculating based on actual rendered heights.
| style={{ | ||
| maxHeight: | ||
| pinnedConfigs.length > 0 | ||
| ? `calc(300px - ${pinnedConfigs.length * 36}px)` |
There was a problem hiding this comment.
The hardcoded 36px assumption for each config item height is fragile. Item height can vary with different themes, font sizes, zoom levels, or accessibility settings. If the actual height differs, the scrollable area calculation will be incorrect, potentially causing either wasted space or unintended overflow. Consider using CSS flexbox with flex-basis or measuring actual rendered heights dynamically.
| {pinnedConfigs.length > 0 && unpinnedConfigs.length > 0 && ( | ||
| <div className="mx-1 my-1 h-px bg-vscode-dropdown-foreground/10" /> | ||
| )} |
There was a problem hiding this comment.
The maxHeight calculation doesn't account for the separator element's total height. The separator has class h-px (1px height) plus my-1 margins (0.25rem top and bottom, typically 4px each = 8px total), adding approximately 9px to the container. This causes the combined height of pinned configs + separator + unpinned container to exceed the intended 300px limit.
|
Success, looks good |
hannesrudolph
added
the
Issue/PR - Triage
3 participants
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
Changes
ApiConfigSelector.tsxto render pinned items in a fixed container and unpinned items in a scrollable containerTesting
Fixes #8812
Important
Fixes pinned models scrolling issue by separating pinned and unpinned models into fixed and scrollable containers in
ApiConfigSelector.tsx.ApiConfigSelector.tsx.ApiConfigSelector.spec.tsxto verify pinned models stay fixed and unpinned models scroll.This description was created by
for 50c8d9a. You can customize this summary. It will automatically update as commits are pushed.