-
Notifications
You must be signed in to change notification settings - Fork 2.3k
fix: keep pinned models fixed at top of scrollable list #8813
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -194,26 +194,37 @@ export const ApiConfigSelector = ({ | |||||
| )} | ||||||
|
|
||||||
| {/* Config list */} | ||||||
| <div className="max-h-[300px] overflow-y-auto"> | ||||||
| {filteredConfigs.length === 0 && searchValue ? ( | ||||||
| <div className="py-2 px-3 text-sm text-vscode-foreground/70"> | ||||||
| {t("common:ui.no_results")} | ||||||
| </div> | ||||||
| ) : ( | ||||||
| <div className="py-1"> | ||||||
| {/* Pinned configs */} | ||||||
| {pinnedConfigs.map((config) => renderConfigItem(config, true))} | ||||||
|
|
||||||
| {/* Separator between pinned and unpinned */} | ||||||
| {pinnedConfigs.length > 0 && unpinnedConfigs.length > 0 && ( | ||||||
| <div className="mx-1 my-1 h-px bg-vscode-dropdown-foreground/10" /> | ||||||
| )} | ||||||
|
|
||||||
| {/* Unpinned configs */} | ||||||
| {unpinnedConfigs.map((config) => renderConfigItem(config, false))} | ||||||
| </div> | ||||||
| )} | ||||||
| </div> | ||||||
| {filteredConfigs.length === 0 && searchValue ? ( | ||||||
| <div className="py-2 px-3 text-sm text-vscode-foreground/70">{t("common:ui.no_results")}</div> | ||||||
| ) : ( | ||||||
| <> | ||||||
| {/* Pinned configs - fixed at top, not scrollable */} | ||||||
| {pinnedConfigs.length > 0 && ( | ||||||
| <div className="py-1"> | ||||||
| {pinnedConfigs.map((config) => renderConfigItem(config, true))} | ||||||
| </div> | ||||||
| )} | ||||||
|
|
||||||
| {/* Separator between pinned and unpinned */} | ||||||
| {pinnedConfigs.length > 0 && unpinnedConfigs.length > 0 && ( | ||||||
| <div className="mx-1 my-1 h-px bg-vscode-dropdown-foreground/10" /> | ||||||
| )} | ||||||
|
|
||||||
| {/* Unpinned configs - scrollable */} | ||||||
| {unpinnedConfigs.length > 0 && ( | ||||||
| <div | ||||||
| className="overflow-y-auto py-1" | ||||||
| style={{ | ||||||
| maxHeight: | ||||||
| pinnedConfigs.length > 0 | ||||||
| ? `calc(300px - ${pinnedConfigs.length * 36}px)` | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The dynamic
Suggested change
This comment was generated because it violated a code review rule: irule_JHt4LrVpts9BYI6Q. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The maxHeight calculation can produce negative values when 9 or more configs are pinned. For example, with 9 pinned configs: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||||||
| : "300px", | ||||||
| }}> | ||||||
| {unpinnedConfigs.map((config) => renderConfigItem(config, false))} | ||||||
| </div> | ||||||
| )} | ||||||
| </> | ||||||
| )} | ||||||
|
|
||||||
| {/* Bottom bar with buttons on left and title on right */} | ||||||
| <div className="flex flex-row items-center justify-between px-2 py-2 border-t border-vscode-dropdown-border"> | ||||||
|
|
||||||
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 maxHeight calculation doesn't account for the separator element's total height. The separator has class
h-px(1px height) plusmy-1margins (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.