From 50c8d9aadabed144049496ea6ac3712fe5aa4d13 Mon Sep 17 00:00:00 2001 From: Roo Code Date: Sat, 25 Oct 2025 01:11:58 +0000 Subject: [PATCH 1/2] 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 786ad01dff6..1019b00ee0c 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 5b25b3bef46..96f583615e6 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() + }) }) From 93c1b6f22518e360f6d991ea47625c5fe8b5e78c Mon Sep 17 00:00:00 2001 From: daniel-lxs Date: Tue, 4 Nov 2025 16:05:30 -0500 Subject: [PATCH 2/2] fix: resolve sticky header visual artifact in ApiConfigSelector - 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 --- .../src/components/chat/ApiConfigSelector.tsx | 31 +++----- .../chat/__tests__/ApiConfigSelector.spec.tsx | 76 ++++++++----------- 2 files changed, 45 insertions(+), 62 deletions(-) diff --git a/webview-ui/src/components/chat/ApiConfigSelector.tsx b/webview-ui/src/components/chat/ApiConfigSelector.tsx index 1019b00ee0c..4396019a2d2 100644 --- a/webview-ui/src/components/chat/ApiConfigSelector.tsx +++ b/webview-ui/src/components/chat/ApiConfigSelector.tsx @@ -193,37 +193,30 @@ export const ApiConfigSelector = ({
)} - {/* Config list */} + {/* Config list - single scroll container */} {filteredConfigs.length === 0 && searchValue ? (
{t("common:ui.no_results")}
) : ( - <> - {/* Pinned configs - fixed at top, not scrollable */} +
+ {/* Pinned configs - sticky header */} {pinnedConfigs.length > 0 && ( -
+
0 && "border-b border-vscode-dropdown-foreground/10", + )} + aria-label="Pinned configurations"> {pinnedConfigs.map((config) => renderConfigItem(config, true))}
)} - {/* Separator between pinned and unpinned */} - {pinnedConfigs.length > 0 && unpinnedConfigs.length > 0 && ( -
- )} - - {/* Unpinned configs - scrollable */} + {/* Unpinned configs */} {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 96f583615e6..ff1b95f9499 100644 --- a/webview-ui/src/components/chat/__tests__/ApiConfigSelector.spec.tsx +++ b/webview-ui/src/components/chat/__tests__/ApiConfigSelector.spec.tsx @@ -461,32 +461,23 @@ describe("ApiConfigSelector", () => { const popoverContent = screen.getByTestId("popover-content") - // Check for Config 1, 2, 3 being visible (pinned) - use getAllByText since there might be multiple + // Should have a single scroll container with max-h-[300px] and overflow-y-auto + const scrollContainer = popoverContent.querySelector(".max-h-\\[300px\\].overflow-y-auto") + expect(scrollContainer).toBeInTheDocument() + + // Check for pinned configs sticky header + const pinnedStickyHeader = scrollContainer?.querySelector(".sticky.top-0.z-10.bg-vscode-dropdown-background") + expect(pinnedStickyHeader).toBeInTheDocument() + expect(pinnedStickyHeader).toHaveAttribute("aria-label", "Pinned configurations") + + // Check for Config 1, 2, 3 being visible in the sticky header (pinned) 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") + // Verify pinned container contains the pinned configs + if (pinnedStickyHeader) { + const elements = pinnedStickyHeader.querySelectorAll(".flex-shrink-0") const pinnedConfigTexts = Array.from(elements) .map((el) => (el as Element).textContent) .filter((text) => text?.startsWith("Config")) @@ -496,17 +487,12 @@ describe("ApiConfigSelector", () => { 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") - } + // Check for unpinned configs section + const unpinnedSection = scrollContainer?.querySelector('[aria-label="All configurations"]') + expect(unpinnedSection).toBeInTheDocument() - // Verify separator exists between pinned and unpinned - const separator = popoverContent.querySelector(".h-px") - expect(separator).toBeInTheDocument() + // Verify separator exists as border on pinned section when unpinned configs exist + expect(pinnedStickyHeader).toHaveClass("border-b") }) test("displays all configs in scrollable container when no configs are pinned", () => { @@ -529,20 +515,24 @@ describe("ApiConfigSelector", () => { 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() + // Should have a single scroll container with max-h-[300px] and overflow-y-auto + const scrollContainer = popoverContent.querySelector(".max-h-\\[300px\\].overflow-y-auto") + expect(scrollContainer).toBeInTheDocument() + + // No pinned section should exist when no configs are pinned + const pinnedSection = scrollContainer?.querySelector(".sticky.top-0") + expect(pinnedSection).not.toBeInTheDocument() - // Check max-height is 300px when no pinned configs - expect(scrollableContainer?.getAttribute("style")).toContain("max-height") - expect(scrollableContainer?.getAttribute("style")).toContain("300px") + // Should have unpinned configs section with all configs + const unpinnedSection = scrollContainer?.querySelector('[aria-label="All configurations"]') + expect(unpinnedSection).toBeInTheDocument() - // All configs should be in the scrollable container - const allConfigRows = scrollableContainer?.querySelectorAll(".group") + // All configs should be in the unpinned section + const allConfigRows = unpinnedSection?.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() + // No separator should exist when no pinned configs (no sticky header exists) + const stickyHeader = scrollContainer?.querySelector(".sticky.top-0") + expect(stickyHeader).not.toBeInTheDocument() }) })