From 50c8d9aadabed144049496ea6ac3712fe5aa4d13 Mon Sep 17 00:00:00 2001 From: Roo Code Date: Sat, 25 Oct 2025 01:11:58 +0000 Subject: [PATCH] fix: keep pinned models fixed at top of scrollable list - 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 --- .../src/components/chat/ApiConfigSelector.tsx | 51 ++++---- .../chat/__tests__/ApiConfigSelector.spec.tsx | 110 ++++++++++++++++++ 2 files changed, 141 insertions(+), 20 deletions(-) diff --git a/webview-ui/src/components/chat/ApiConfigSelector.tsx b/webview-ui/src/components/chat/ApiConfigSelector.tsx index 786ad01dff6d..1019b00ee0c2 100644 --- a/webview-ui/src/components/chat/ApiConfigSelector.tsx +++ b/webview-ui/src/components/chat/ApiConfigSelector.tsx @@ -194,26 +194,37 @@ export const ApiConfigSelector = ({ )} {/* Config list */} -
- {filteredConfigs.length === 0 && searchValue ? ( -
- {t("common:ui.no_results")} -
- ) : ( -
- {/* Pinned configs */} - {pinnedConfigs.map((config) => renderConfigItem(config, true))} - - {/* Separator between pinned and unpinned */} - {pinnedConfigs.length > 0 && unpinnedConfigs.length > 0 && ( -
- )} - - {/* Unpinned configs */} - {unpinnedConfigs.map((config) => renderConfigItem(config, false))} -
- )} -
+ {filteredConfigs.length === 0 && searchValue ? ( +
{t("common:ui.no_results")}
+ ) : ( + <> + {/* Pinned configs - fixed at top, not scrollable */} + {pinnedConfigs.length > 0 && ( +
+ {pinnedConfigs.map((config) => renderConfigItem(config, true))} +
+ )} + + {/* Separator between pinned and unpinned */} + {pinnedConfigs.length > 0 && unpinnedConfigs.length > 0 && ( +
+ )} + + {/* Unpinned configs - scrollable */} + {unpinnedConfigs.length > 0 && ( +
0 + ? `calc(300px - ${pinnedConfigs.length * 36}px)` + : "300px", + }}> + {unpinnedConfigs.map((config) => renderConfigItem(config, false))} +
+ )} + + )} {/* Bottom bar with buttons on left and title on right */}
diff --git a/webview-ui/src/components/chat/__tests__/ApiConfigSelector.spec.tsx b/webview-ui/src/components/chat/__tests__/ApiConfigSelector.spec.tsx index 5b25b3bef46b..96f583615e6c 100644 --- a/webview-ui/src/components/chat/__tests__/ApiConfigSelector.spec.tsx +++ b/webview-ui/src/components/chat/__tests__/ApiConfigSelector.spec.tsx @@ -435,4 +435,114 @@ describe("ApiConfigSelector", () => { // Search value should be maintained expect(searchInput.value).toBe("Config") }) + + test("pinned configs remain fixed at top while unpinned configs scroll", () => { + // Create a list with many configs to test scrolling + const manyConfigs = Array.from({ length: 15 }, (_, i) => ({ + id: `config${i + 1}`, + name: `Config ${i + 1}`, + modelId: `model-${i + 1}`, + })) + + const props = { + ...defaultProps, + listApiConfigMeta: manyConfigs, + pinnedApiConfigs: { + config1: true, + config2: true, + config3: true, + }, + } + + render() + + const trigger = screen.getByTestId("dropdown-trigger") + fireEvent.click(trigger) + + const popoverContent = screen.getByTestId("popover-content") + + // Check for Config 1, 2, 3 being visible (pinned) - use getAllByText since there might be multiple + expect(screen.getAllByText("Config 1").length).toBeGreaterThan(0) + expect(screen.getAllByText("Config 2").length).toBeGreaterThan(0) + expect(screen.getAllByText("Config 3").length).toBeGreaterThan(0) + + // Find all containers with py-1 class + const configContainers = popoverContent.querySelectorAll(".py-1") + + // Should have 2 containers: one for pinned (non-scrollable) and one for unpinned (scrollable) + expect(configContainers.length).toBeGreaterThanOrEqual(1) + + // Find the non-scrollable container (pinned configs) + let pinnedContainer: Element | null = null + let unpinnedContainer: Element | null = null + + configContainers.forEach((container) => { + if (container.classList.contains("overflow-y-auto")) { + unpinnedContainer = container + } else { + pinnedContainer = container + } + }) + + // Verify pinned container exists and contains the pinned configs + if (pinnedContainer) { + const elements = (pinnedContainer as Element).querySelectorAll(".flex-shrink-0") + const pinnedConfigTexts = Array.from(elements) + .map((el) => (el as Element).textContent) + .filter((text) => text?.startsWith("Config")) + + expect(pinnedConfigTexts).toContain("Config 1") + expect(pinnedConfigTexts).toContain("Config 2") + expect(pinnedConfigTexts).toContain("Config 3") + } + + // Verify unpinned container exists and is scrollable + expect(unpinnedContainer).toBeInTheDocument() + if (unpinnedContainer) { + expect((unpinnedContainer as Element).classList.contains("overflow-y-auto")).toBe(true) + // Check that the unpinned container has the correct max-height + expect((unpinnedContainer as Element).getAttribute("style")).toContain("max-height") + } + + // Verify separator exists between pinned and unpinned + const separator = popoverContent.querySelector(".h-px") + expect(separator).toBeInTheDocument() + }) + + test("displays all configs in scrollable container when no configs are pinned", () => { + const manyConfigs = Array.from({ length: 10 }, (_, i) => ({ + id: `config${i + 1}`, + name: `Config ${i + 1}`, + modelId: `model-${i + 1}`, + })) + + const props = { + ...defaultProps, + listApiConfigMeta: manyConfigs, + pinnedApiConfigs: {}, // No pinned configs + } + + render() + + const trigger = screen.getByTestId("dropdown-trigger") + fireEvent.click(trigger) + + const popoverContent = screen.getByTestId("popover-content") + + // Should have only one scrollable container with all configs + const scrollableContainer = popoverContent.querySelector(".overflow-y-auto.py-1") + expect(scrollableContainer).toBeInTheDocument() + + // Check max-height is 300px when no pinned configs + expect(scrollableContainer?.getAttribute("style")).toContain("max-height") + expect(scrollableContainer?.getAttribute("style")).toContain("300px") + + // All configs should be in the scrollable container + const allConfigRows = scrollableContainer?.querySelectorAll(".group") + expect(allConfigRows?.length).toBe(10) + + // No separator should exist + const separator = popoverContent.querySelector(".h-px.bg-vscode-dropdown-foreground\\/10") + expect(separator).not.toBeInTheDocument() + }) })