fix: compact model config selector (#1611)
This commit is contained in:
@@ -1,5 +1,5 @@
|
||||
import { cleanup, render, screen } from "@testing-library/react";
|
||||
import { afterEach, describe, expect, it } from "vitest";
|
||||
import { cleanup, fireEvent, render, screen, within } from "@testing-library/react";
|
||||
import { afterEach, describe, expect, it, vi } from "vitest";
|
||||
|
||||
import { ModelConfigSelector } from "@/components/model-config-selector";
|
||||
|
||||
@@ -8,6 +8,15 @@ afterEach(() => {
|
||||
});
|
||||
|
||||
describe("ModelConfigSelector", () => {
|
||||
const effortSectionTestId = "config-option-section-effort";
|
||||
const effortTriggerTestId = "config-option-trigger-effort";
|
||||
const modelSettingsButtonName = "Model settings";
|
||||
const makeModelOptions = (count: number) =>
|
||||
Array.from({ length: count }, (_, index) => ({
|
||||
id: `model-${index + 1}`,
|
||||
name: `Model ${index + 1}`,
|
||||
}));
|
||||
|
||||
it("passes custom trigger classes to the button", () => {
|
||||
render(
|
||||
<ModelConfigSelector
|
||||
@@ -18,8 +27,96 @@ describe("ModelConfigSelector", () => {
|
||||
/>,
|
||||
);
|
||||
|
||||
expect(screen.getByRole("button", { name: "Model settings" }).className).toContain(
|
||||
expect(screen.getByRole("button", { name: modelSettingsButtonName }).className).toContain(
|
||||
"max-w-[56vw]",
|
||||
);
|
||||
});
|
||||
|
||||
it("opens extra config options from compact sub-selector rows", () => {
|
||||
const onConfigChange = vi.fn();
|
||||
|
||||
render(
|
||||
<ModelConfigSelector
|
||||
modelOptions={[{ id: "sonnet", name: "Sonnet" }]}
|
||||
currentModel="sonnet"
|
||||
onModelChange={() => {}}
|
||||
onConfigChange={onConfigChange}
|
||||
configOptions={[
|
||||
{
|
||||
type: "select",
|
||||
id: "model",
|
||||
name: "Model",
|
||||
currentValue: "sonnet",
|
||||
category: "model",
|
||||
options: [{ value: "sonnet", name: "Sonnet" }],
|
||||
},
|
||||
{
|
||||
type: "select",
|
||||
id: "effort",
|
||||
name: "Effort",
|
||||
currentValue: "medium",
|
||||
options: [
|
||||
{ value: "low", name: "Low" },
|
||||
{ value: "medium", name: "Medium" },
|
||||
{ value: "high", name: "High" },
|
||||
],
|
||||
},
|
||||
]}
|
||||
/>,
|
||||
);
|
||||
|
||||
fireEvent.click(screen.getByRole("button", { name: modelSettingsButtonName }));
|
||||
|
||||
const effortTrigger = screen.getByTestId(effortTriggerTestId);
|
||||
expect(effortTrigger.textContent).toContain("Effort");
|
||||
expect(effortTrigger.textContent).toContain("Medium");
|
||||
expect(screen.queryByTestId(effortSectionTestId)).toBeNull();
|
||||
|
||||
fireEvent.click(effortTrigger);
|
||||
|
||||
const effortSection = screen.getByTestId(effortSectionTestId);
|
||||
expect(effortSection).not.toBeNull();
|
||||
const backButton = screen.getByRole("button", { name: /back to model settings from effort/i });
|
||||
expect(document.activeElement).toBe(backButton);
|
||||
|
||||
fireEvent.click(backButton);
|
||||
|
||||
expect(screen.queryByTestId(effortSectionTestId)).toBeNull();
|
||||
expect(document.activeElement).toBe(screen.getByTestId(effortTriggerTestId));
|
||||
|
||||
fireEvent.click(screen.getByTestId(effortTriggerTestId));
|
||||
const reopenedEffortSection = screen.getByTestId(effortSectionTestId);
|
||||
fireEvent.click(within(reopenedEffortSection).getByRole("button", { name: "High" }));
|
||||
|
||||
expect(onConfigChange).toHaveBeenCalledWith("effort", "high");
|
||||
expect(screen.queryByTestId(effortSectionTestId)).toBeNull();
|
||||
});
|
||||
|
||||
it("hides the model filter when there are five or fewer models", () => {
|
||||
render(
|
||||
<ModelConfigSelector
|
||||
modelOptions={makeModelOptions(5)}
|
||||
currentModel="model-1"
|
||||
onModelChange={() => {}}
|
||||
/>,
|
||||
);
|
||||
|
||||
fireEvent.click(screen.getByRole("button", { name: modelSettingsButtonName }));
|
||||
|
||||
expect(screen.queryByPlaceholderText("Filter models...")).toBeNull();
|
||||
});
|
||||
|
||||
it("shows the model filter when there are more than five models", () => {
|
||||
render(
|
||||
<ModelConfigSelector
|
||||
modelOptions={makeModelOptions(6)}
|
||||
currentModel="model-1"
|
||||
onModelChange={() => {}}
|
||||
/>,
|
||||
);
|
||||
|
||||
fireEvent.click(screen.getByRole("button", { name: modelSettingsButtonName }));
|
||||
|
||||
expect(screen.getByPlaceholderText("Filter models...")).not.toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1,7 +1,7 @@
|
||||
"use client";
|
||||
|
||||
import { memo, useState } from "react";
|
||||
import { IconCheck, IconChevronDown } from "@tabler/icons-react";
|
||||
import { memo, useEffect, useRef, useState } from "react";
|
||||
import { IconCheck, IconChevronDown, IconChevronLeft, IconChevronRight } from "@tabler/icons-react";
|
||||
|
||||
import { cn } from "@/lib/utils";
|
||||
import { Button } from "@kandev/ui/button";
|
||||
@@ -147,37 +147,177 @@ function ModelRow({
|
||||
);
|
||||
}
|
||||
|
||||
function ConfigOptionSection({
|
||||
function ConfigOptionTrigger({
|
||||
option,
|
||||
onSelect,
|
||||
triggerRef,
|
||||
}: {
|
||||
option: SelectConfigOption;
|
||||
onSelect: () => void;
|
||||
triggerRef?: (element: HTMLButtonElement | null) => void;
|
||||
}) {
|
||||
return (
|
||||
<button
|
||||
type="button"
|
||||
ref={triggerRef}
|
||||
data-testid={`config-option-trigger-${option.id}`}
|
||||
className="flex min-h-9 w-full cursor-pointer items-center justify-between gap-3 rounded-md px-2.5 py-2 text-left text-xs/relaxed hover:bg-muted focus-visible:ring-2 focus-visible:ring-ring/35 focus-visible:outline-none"
|
||||
onClick={onSelect}
|
||||
>
|
||||
<span className="font-medium">{option.name}</span>
|
||||
<span className="flex min-w-0 items-center gap-2 text-muted-foreground">
|
||||
<span className="truncate">{currentOptionName(option)}</span>
|
||||
<IconChevronRight className="h-3.5 w-3.5 shrink-0" />
|
||||
</span>
|
||||
</button>
|
||||
);
|
||||
}
|
||||
|
||||
function ConfigOptionSubSelector({
|
||||
option,
|
||||
onBack,
|
||||
onChange,
|
||||
}: {
|
||||
option: SelectConfigOption;
|
||||
onBack: () => void;
|
||||
onChange?: (configId: string, value: string) => void;
|
||||
}) {
|
||||
return (
|
||||
<div className="space-y-1.5" data-testid={`config-option-section-${option.id}`}>
|
||||
<div className="text-[0.6875rem] font-medium uppercase tracking-wide text-muted-foreground">
|
||||
{option.name}
|
||||
</div>
|
||||
<div className="grid grid-cols-2 gap-1">
|
||||
{option.options.map((item) => (
|
||||
<Button
|
||||
key={item.value}
|
||||
type="button"
|
||||
variant={item.value === option.currentValue ? "secondary" : "ghost"}
|
||||
size="sm"
|
||||
className="h-9 min-w-0 cursor-pointer justify-start px-2 text-left"
|
||||
disabled={!onChange}
|
||||
onClick={() => onChange?.(option.id, item.value)}
|
||||
>
|
||||
<span className="truncate">{item.name}</span>
|
||||
</Button>
|
||||
))}
|
||||
</div>
|
||||
<div className="flex min-h-0 flex-col gap-2">
|
||||
<button
|
||||
type="button"
|
||||
aria-label={`Back to model settings from ${option.name}`}
|
||||
autoFocus
|
||||
className="flex min-h-9 w-full cursor-pointer items-center gap-2 rounded-md px-2 text-left text-xs/relaxed hover:bg-muted focus-visible:ring-2 focus-visible:ring-ring/35 focus-visible:outline-none"
|
||||
onClick={onBack}
|
||||
>
|
||||
<IconChevronLeft className="h-3.5 w-3.5 shrink-0 text-muted-foreground" />
|
||||
<span className="truncate font-medium">{option.name}</span>
|
||||
</button>
|
||||
<ScrollArea
|
||||
className="max-h-[min(18rem,calc(100vh-8rem))] pr-2"
|
||||
data-testid={`config-option-section-${option.id}`}
|
||||
>
|
||||
<div className="space-y-1">
|
||||
{option.options.map((item) => (
|
||||
<Button
|
||||
key={item.value}
|
||||
type="button"
|
||||
variant={item.value === option.currentValue ? "secondary" : "ghost"}
|
||||
size="sm"
|
||||
className="h-9 w-full min-w-0 cursor-pointer justify-start px-2 text-left"
|
||||
disabled={!onChange}
|
||||
onClick={() => {
|
||||
onChange?.(option.id, item.value);
|
||||
onBack();
|
||||
}}
|
||||
>
|
||||
<span className="truncate">{item.name}</span>
|
||||
<IconCheck
|
||||
className={cn(
|
||||
"ml-auto h-4 w-4 shrink-0",
|
||||
item.value === option.currentValue ? "opacity-100" : "opacity-0",
|
||||
)}
|
||||
/>
|
||||
</Button>
|
||||
))}
|
||||
</div>
|
||||
</ScrollArea>
|
||||
</div>
|
||||
);
|
||||
}
|
||||
|
||||
type ModelConfigSelectorContentProps = {
|
||||
activeConfig: SelectConfigOption | undefined;
|
||||
modelOptions: ModelSelectorOption[];
|
||||
currentModelValue: string;
|
||||
extraConfigOptions: SelectConfigOption[];
|
||||
onModelSelect: (value: string) => void;
|
||||
onConfigSelect: (configId: string) => void;
|
||||
onConfigBack: () => void;
|
||||
onConfigChange?: (configId: string, value: string) => void;
|
||||
};
|
||||
|
||||
function ModelConfigSelectorContent({
|
||||
activeConfig,
|
||||
modelOptions,
|
||||
currentModelValue,
|
||||
extraConfigOptions,
|
||||
onModelSelect,
|
||||
onConfigSelect,
|
||||
onConfigBack,
|
||||
onConfigChange,
|
||||
}: ModelConfigSelectorContentProps) {
|
||||
const pendingFocusConfigId = useRef<string | null>(null);
|
||||
const triggerRefs = useRef<Record<string, HTMLButtonElement | null>>({});
|
||||
const showModelFilter = modelOptions.length > 5;
|
||||
|
||||
useEffect(() => {
|
||||
if (activeConfig) return;
|
||||
const configId = pendingFocusConfigId.current;
|
||||
if (!configId) return;
|
||||
pendingFocusConfigId.current = null;
|
||||
triggerRefs.current[configId]?.focus();
|
||||
}, [activeConfig]);
|
||||
|
||||
const returnToConfigTrigger = () => {
|
||||
if (activeConfig) {
|
||||
pendingFocusConfigId.current = activeConfig.id;
|
||||
}
|
||||
onConfigBack();
|
||||
};
|
||||
|
||||
if (activeConfig) {
|
||||
return (
|
||||
<ConfigOptionSubSelector
|
||||
option={activeConfig}
|
||||
onBack={returnToConfigTrigger}
|
||||
onChange={onConfigChange}
|
||||
/>
|
||||
);
|
||||
}
|
||||
|
||||
return (
|
||||
<>
|
||||
<Command>
|
||||
{showModelFilter && <CommandInput placeholder="Filter models..." className="h-8" />}
|
||||
<CommandList className="max-h-60">
|
||||
<CommandEmpty>No models found.</CommandEmpty>
|
||||
<CommandGroup heading="Model">
|
||||
{modelOptions.map((model) => (
|
||||
<ModelRow
|
||||
key={model.id}
|
||||
model={model}
|
||||
selected={model.id === currentModelValue}
|
||||
onSelect={onModelSelect}
|
||||
/>
|
||||
))}
|
||||
</CommandGroup>
|
||||
</CommandList>
|
||||
</Command>
|
||||
{extraConfigOptions.length > 0 && (
|
||||
<>
|
||||
<Separator />
|
||||
<ScrollArea className="max-h-40 pr-2">
|
||||
<div className="space-y-1">
|
||||
{extraConfigOptions.map((option) => (
|
||||
<ConfigOptionTrigger
|
||||
key={option.id}
|
||||
option={option}
|
||||
onSelect={() => onConfigSelect(option.id)}
|
||||
triggerRef={(element) => {
|
||||
triggerRefs.current[option.id] = element;
|
||||
}}
|
||||
/>
|
||||
))}
|
||||
</div>
|
||||
</ScrollArea>
|
||||
</>
|
||||
)}
|
||||
</>
|
||||
);
|
||||
}
|
||||
|
||||
export type ModelConfigSelectorProps = {
|
||||
modelOptions: ModelSelectorOption[];
|
||||
currentModel: string | null;
|
||||
@@ -206,9 +346,11 @@ export const ModelConfigSelector = memo(function ModelConfigSelector({
|
||||
triggerClassName: customTriggerClassName,
|
||||
}: ModelConfigSelectorProps) {
|
||||
const [open, setOpen] = useState(false);
|
||||
const [activeConfigId, setActiveConfigId] = useState<string | null>(null);
|
||||
const selectConfigOptions = usableConfigOptions(configOptions);
|
||||
const modelConfig = selectConfigOptions.find(isModelConfigOption);
|
||||
const extraConfigOptions = selectConfigOptions.filter((option) => !isModelConfigOption(option));
|
||||
const activeConfig = extraConfigOptions.find((option) => option.id === activeConfigId);
|
||||
const currentModelValue = modelConfig?.currentValue || currentModel || "";
|
||||
const label = resolveTriggerLabel(modelOptions, currentModel, modelConfig, configOptions);
|
||||
|
||||
@@ -225,9 +367,15 @@ export const ModelConfigSelector = memo(function ModelConfigSelector({
|
||||
variant === "compact"
|
||||
? "h-7 max-w-[min(18rem,70vw)] cursor-pointer gap-1 px-2 text-xs hover:bg-muted/40"
|
||||
: "w-full justify-between font-normal cursor-pointer";
|
||||
const onOpenChange = (nextOpen: boolean) => {
|
||||
setOpen(nextOpen);
|
||||
if (!nextOpen) {
|
||||
setActiveConfigId(null);
|
||||
}
|
||||
};
|
||||
|
||||
return (
|
||||
<Popover open={open} onOpenChange={setOpen}>
|
||||
<Popover open={open} onOpenChange={onOpenChange}>
|
||||
<PopoverTrigger asChild>
|
||||
<Button
|
||||
type="button"
|
||||
@@ -244,36 +392,18 @@ export const ModelConfigSelector = memo(function ModelConfigSelector({
|
||||
<PopoverContent
|
||||
align="end"
|
||||
side={popoverSide}
|
||||
className="w-[min(24rem,calc(100vw-1rem))] gap-2 p-2"
|
||||
className="w-[min(24rem,calc(100vw-1rem))] max-h-[min(32rem,calc(100vh-1rem))] gap-2 overflow-hidden p-2"
|
||||
>
|
||||
<Command>
|
||||
<CommandInput placeholder="Filter models..." className="h-8" />
|
||||
<CommandList className="max-h-60">
|
||||
<CommandEmpty>No models found.</CommandEmpty>
|
||||
<CommandGroup heading="Model">
|
||||
{modelOptions.map((model) => (
|
||||
<ModelRow
|
||||
key={model.id}
|
||||
model={model}
|
||||
selected={model.id === currentModelValue}
|
||||
onSelect={onModelSelect}
|
||||
/>
|
||||
))}
|
||||
</CommandGroup>
|
||||
</CommandList>
|
||||
</Command>
|
||||
{extraConfigOptions.length > 0 && (
|
||||
<>
|
||||
<Separator />
|
||||
<ScrollArea className="max-h-56 pr-2">
|
||||
<div className="space-y-3">
|
||||
{extraConfigOptions.map((option) => (
|
||||
<ConfigOptionSection key={option.id} option={option} onChange={onConfigChange} />
|
||||
))}
|
||||
</div>
|
||||
</ScrollArea>
|
||||
</>
|
||||
)}
|
||||
<ModelConfigSelectorContent
|
||||
activeConfig={activeConfig}
|
||||
modelOptions={modelOptions}
|
||||
currentModelValue={currentModelValue}
|
||||
extraConfigOptions={extraConfigOptions}
|
||||
onModelSelect={onModelSelect}
|
||||
onConfigSelect={setActiveConfigId}
|
||||
onConfigBack={() => setActiveConfigId(null)}
|
||||
onConfigChange={onConfigChange}
|
||||
/>
|
||||
</PopoverContent>
|
||||
</Popover>
|
||||
);
|
||||
|
||||
@@ -48,5 +48,10 @@ test.describe("Mobile chat model selector", () => {
|
||||
await expect(testPage.getByRole("option", { name: /Mock Smart/ })).toBeVisible({
|
||||
timeout: 5_000,
|
||||
});
|
||||
|
||||
const effortTrigger = testPage.getByTestId("config-option-trigger-effort");
|
||||
await expect(effortTrigger).toBeVisible();
|
||||
await effortTrigger.tap();
|
||||
await expect(testPage.getByTestId("config-option-section-effort")).toBeVisible();
|
||||
});
|
||||
});
|
||||
|
||||
@@ -198,13 +198,12 @@ test.describe("Chat model selector — persistence", () => {
|
||||
/**
|
||||
* Verifies the chat-input model selector's popover stays open after picking a
|
||||
* model when extra config options (reasoning effort, thought level, …) are
|
||||
* present on the same popover. Picking a model is rarely the last thing a user
|
||||
* wants to do when there are other knobs in the same surface — they typically
|
||||
* want to adjust effort too. Auto-closing on model select forces a second open.
|
||||
* present in the same selector. Picking a model is rarely the last thing a user
|
||||
* wants to do when there are other knobs available — they typically want to
|
||||
* adjust effort too. Auto-closing on model select forces a second open.
|
||||
*
|
||||
* mock-agent ships with model + effort config options, so the popover renders
|
||||
* the Effort section alongside the model list — exactly the layout this guard
|
||||
* protects.
|
||||
* an Effort row below the model list and opens the Effort picker from there.
|
||||
*/
|
||||
test.describe("Chat model selector — popover open/close behavior", () => {
|
||||
test("stays open after picking a model when extra config options exist", async ({
|
||||
@@ -234,12 +233,10 @@ test.describe("Chat model selector — popover open/close behavior", () => {
|
||||
await expect(trigger).toContainText("Mock Fast", { timeout: 15_000 });
|
||||
|
||||
await trigger.click();
|
||||
// The effort config section is the rendered marker that the popover
|
||||
// content is mounted — it sits below the model list inside the same
|
||||
// PopoverContent. Selecting by testid mirrors the suite-wide convention
|
||||
// (apps/web/e2e/README.md).
|
||||
const effortSection = testPage.getByTestId("config-option-section-effort");
|
||||
await expect(effortSection).toBeVisible({ timeout: 5_000 });
|
||||
// The effort config row is the rendered marker that the popover content is
|
||||
// mounted. It opens the nested effort selector inside the same popover.
|
||||
const effortTrigger = testPage.getByTestId("config-option-trigger-effort");
|
||||
await expect(effortTrigger).toBeVisible({ timeout: 5_000 });
|
||||
|
||||
await testPage.getByRole("option", { name: /Mock Smart/ }).click();
|
||||
|
||||
@@ -248,8 +245,9 @@ test.describe("Chat model selector — popover open/close behavior", () => {
|
||||
await expect(trigger).toContainText("Mock Smart", { timeout: 5_000 });
|
||||
|
||||
// Popover must still be open so the user can also pick an effort level
|
||||
// without re-opening. The effort section is rendered only when the
|
||||
// PopoverContent is mounted (Radix unmounts it on close).
|
||||
await expect(effortSection).toBeVisible();
|
||||
// without re-opening. The effort row is rendered only while PopoverContent
|
||||
// is mounted (Radix unmounts it on close).
|
||||
const updatedEffortTrigger = testPage.getByTestId("config-option-trigger-effort");
|
||||
await expect(updatedEffortTrigger).toBeVisible();
|
||||
});
|
||||
});
|
||||
|
||||
@@ -161,6 +161,11 @@ test.describe("Quick Chat", () => {
|
||||
await expect(trigger).toContainText("Mock Fast", { timeout: 15_000 });
|
||||
await trigger.click();
|
||||
|
||||
const effortTrigger = testPage.getByTestId("config-option-trigger-effort");
|
||||
await expect(effortTrigger).toBeVisible({
|
||||
timeout: 10_000,
|
||||
});
|
||||
await effortTrigger.click();
|
||||
await expect(testPage.getByTestId("config-option-section-effort")).toBeVisible({
|
||||
timeout: 10_000,
|
||||
});
|
||||
|
||||
@@ -134,7 +134,9 @@ test.describe("Agent profile — ACP-first", () => {
|
||||
await expect(selector).toContainText("High", { timeout: 10_000 });
|
||||
|
||||
await selector.click();
|
||||
await expect(testPage.getByText("Effort", { exact: true })).toBeVisible();
|
||||
const effortTrigger = testPage.getByTestId("config-option-trigger-effort");
|
||||
await expect(effortTrigger).toBeVisible();
|
||||
await effortTrigger.click();
|
||||
await testPage.getByRole("button", { name: "Low", exact: true }).click();
|
||||
await expect(selector).toContainText("Low");
|
||||
|
||||
|
||||
@@ -36,7 +36,9 @@ test.describe("Mobile agent profile config selector", () => {
|
||||
const selector = testPage.getByRole("button", { name: "Profile start model settings" });
|
||||
await expect(selector).toBeVisible({ timeout: 15_000 });
|
||||
await selector.click();
|
||||
await expect(testPage.getByText("Effort", { exact: true })).toBeVisible();
|
||||
const effortTrigger = testPage.getByTestId("config-option-trigger-effort");
|
||||
await expect(effortTrigger).toBeVisible();
|
||||
await effortTrigger.click();
|
||||
await testPage.getByRole("button", { name: "High", exact: true }).click();
|
||||
await expect(selector).toContainText("High");
|
||||
} finally {
|
||||
|
||||
@@ -288,9 +288,8 @@ test.describe("Utility Agents settings page", () => {
|
||||
await expect(suggestions.getByText("Mock Fast", { exact: true })).toBeVisible();
|
||||
await expect(suggestions.getByText("Mock Smart", { exact: true })).toBeVisible();
|
||||
|
||||
// Search input is part of the shared selector — filtering narrows the list.
|
||||
await testPage.getByPlaceholder("Filter models...").fill("smart");
|
||||
await expect(suggestions.getByText("Mock Fast", { exact: true })).toHaveCount(0);
|
||||
// Short model lists hide the shared selector's filter input.
|
||||
await expect(testPage.getByPlaceholder("Filter models...")).toHaveCount(0);
|
||||
|
||||
// Pick Mock Smart and verify the trigger reflects the selection.
|
||||
await suggestions.getByText("Mock Smart", { exact: true }).click();
|
||||
|
||||
Reference in New Issue
Block a user