Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 31 additions & 20 deletions webview-ui/src/components/chat/ApiConfigSelector.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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" />
)}
Comment on lines +209 to +211
Copy link
Author

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


{/* Unpinned configs - scrollable */}
{unpinnedConfigs.length > 0 && (
<div
className="overflow-y-auto py-1"
style={{
maxHeight:
pinnedConfigs.length > 0
? `calc(300px - ${pinnedConfigs.length * 36}px)`
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dynamic maxHeight calculation may yield a negative value if there are too many pinned configs. Consider adding a guard to prevent negative heights.

Suggested change
? `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.

Copy link
Author

Choose a reason for hiding this comment

The 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: 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.

Copy link
Author

Choose a reason for hiding this comment

The 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 flex-basis or measuring actual rendered heights dynamically.

: "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">
Expand Down
110 changes: 110 additions & 0 deletions webview-ui/src/components/chat/__tests__/ApiConfigSelector.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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(<ApiConfigSelector {...props} />)

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(<ApiConfigSelector {...props} />)

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()
})
})