From f63a60e426a67a3bc5019ba9348907f1ffcc9eb9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Sat, 22 Aug 2026 22:38:59 +0000 Subject: [PATCH] feat: prioritize selected picker options (#2935) --- apps/web/components/combobox.test.tsx | 81 ++++ apps/web/components/combobox.tsx | 15 +- .../model-config-selector-content.tsx | 310 ++++++++++++++ .../components/model-config-selector.test.tsx | 52 +++ apps/web/components/model-config-selector.tsx | 303 +------------- .../task-create-dialog-branch-options.tsx | 79 ++++ .../components/task-create-dialog-options.tsx | 2 +- .../task-create-dialog-pill.test.tsx | 38 +- .../components/task-create-dialog-pill.tsx | 391 ++++++++---------- .../task-create-dialog-remote-repo-chip.tsx | 4 +- ...ask-create-dialog-workspace-repo-chips.tsx | 9 +- .../task/branch-picker-list.test.tsx | 50 ++- .../components/task/branch-picker-list.tsx | 10 +- apps/web/components/task/mode-selector.tsx | 12 +- .../task/selector-trigger-class.test.tsx | 21 +- .../tests/chat/mobile-model-selector.spec.ts | 16 +- .../tests/chat/model-selector-error.spec.ts | 7 +- .../task/create-task-branch-selector.spec.ts | 7 +- apps/web/e2e/tests/task/create-task.spec.ts | 23 ++ .../mobile-launch-failure-recovery.spec.ts | 4 + .../web/hooks/use-pill-tooltip-suppression.ts | 83 ++++ apps/web/lib/utils/selector-options.test.ts | 59 +++ apps/web/lib/utils/selector-options.ts | 50 +++ .../selected-option-picker-prominence/plan.md | 171 ++++++++ .../task-01-selected-option-picker-ui.md | 83 ++++ .../task-02-selected-option-picker-e2e.md | 79 ++++ .../ui/selected-option-picker-prominence.md | 90 ++++ 27 files changed, 1502 insertions(+), 547 deletions(-) create mode 100644 apps/web/components/combobox.test.tsx create mode 100644 apps/web/components/model-config-selector-content.tsx create mode 100644 apps/web/components/task-create-dialog-branch-options.tsx create mode 100644 apps/web/hooks/use-pill-tooltip-suppression.ts create mode 100644 apps/web/lib/utils/selector-options.test.ts create mode 100644 apps/web/lib/utils/selector-options.ts create mode 100644 docs/plans/selected-option-picker-prominence/plan.md create mode 100644 docs/plans/selected-option-picker-prominence/task-01-selected-option-picker-ui.md create mode 100644 docs/plans/selected-option-picker-prominence/task-02-selected-option-picker-e2e.md create mode 100644 docs/specs/ui/selected-option-picker-prominence.md diff --git a/apps/web/components/combobox.test.tsx b/apps/web/components/combobox.test.tsx new file mode 100644 index 0000000000..2afdb79f5b --- /dev/null +++ b/apps/web/components/combobox.test.tsx @@ -0,0 +1,81 @@ +import { cleanup, fireEvent, render, screen } from "@testing-library/react"; +import { TooltipProvider } from "@kandev/ui/tooltip"; +import { afterEach, describe, expect, it, vi } from "vitest"; + +import { Combobox, type ComboboxOption } from "@/components/combobox"; + +afterEach(() => cleanup()); + +const options: ComboboxOption[] = [ + { value: "first", label: "First" }, + { value: "current", label: "Current" }, + { value: "last", label: "Last" }, +]; +const selectedAttribute = "data-selected"; + +describe("Combobox", () => { + it("puts the current option first with a persistent selected surface", () => { + render( + , + ); + + fireEvent.click(screen.getByRole("combobox", { name: "Choose an agent" })); + + const renderedOptions = screen.getAllByRole("option"); + expect(renderedOptions.map((option) => option.textContent?.trim())).toEqual([ + "Current", + "First", + "Last", + ]); + expect(renderedOptions[0].className).toContain("bg-card"); + expect(renderedOptions[0].className).toContain("border-primary/50"); + + const searchInput = document.querySelector("[cmdk-input]"); + expect(searchInput).not.toBeNull(); + expect(renderedOptions[0].getAttribute(selectedAttribute)).toBe("true"); + + fireEvent.keyDown(searchInput!, { key: "ArrowDown" }); + expect(renderedOptions[0].getAttribute(selectedAttribute)).toBe("false"); + expect(renderedOptions[1].getAttribute(selectedAttribute)).toBe("true"); + + fireEvent.keyDown(searchInput!, { key: "ArrowUp" }); + expect(renderedOptions[0].getAttribute(selectedAttribute)).toBe("true"); + }); + + it("keeps a disabled current option first and non-selectable", () => { + render( + + + , + ); + + fireEvent.click(screen.getByRole("combobox", { name: "Choose an agent" })); + + const renderedOptions = screen.getAllByRole("option"); + expect(renderedOptions.map((option) => option.textContent?.trim())).toEqual([ + "Unavailable current", + "Available", + ]); + expect(renderedOptions[0].getAttribute("aria-disabled")).toBe("true"); + expect(renderedOptions[0].className).toContain("bg-card"); + }); +}); diff --git a/apps/web/components/combobox.tsx b/apps/web/components/combobox.tsx index 609d4128d0..7d90519b4f 100644 --- a/apps/web/components/combobox.tsx +++ b/apps/web/components/combobox.tsx @@ -4,6 +4,7 @@ import { memo, useState } from "react"; import { IconCheck, IconChevronDown, IconLoader2 } from "@tabler/icons-react"; import { cn } from "@/lib/utils"; +import { prioritizeSelectedOption, selectorOptionClassName } from "@/lib/utils/selector-options"; import { Button } from "@kandev/ui/button"; import { Command, @@ -92,8 +93,13 @@ function OptionsList({ value: string; onSelect: (value: string) => void; }) { - const enabled = options.filter((o) => !o.disabled); - const disabled = options.filter((o) => o.disabled); + const orderedOptions = prioritizeSelectedOption(options, value, (option) => option.value); + const selected = orderedOptions.find((option) => option.value === value); + const remaining = selected + ? orderedOptions.filter((option) => option.value !== value) + : orderedOptions; + const enabled = remaining.filter((o) => !o.disabled); + const disabled = remaining.filter((o) => o.disabled); const renderItem = (option: ComboboxOption) => { const item = ( @@ -103,7 +109,7 @@ function OptionsList({ keywords={option.keywords ?? [option.label, option.description ?? ""]} onSelect={() => !option.disabled && onSelect(option.value)} disabled={option.disabled} - className={cn("relative pr-7", option.disabled && "opacity-40 cursor-not-allowed")} + className={selectorOptionClassName(option.value === value, option.disabled)} >
{option.renderLabel ? option.renderLabel() : option.label} @@ -133,7 +139,8 @@ function OptionsList({ return ( <> - {enabled.map(renderItem)} + {selected && {renderItem(selected)}} + {enabled.length > 0 && {enabled.map(renderItem)}} {disabled.length > 0 && ( <> diff --git a/apps/web/components/model-config-selector-content.tsx b/apps/web/components/model-config-selector-content.tsx new file mode 100644 index 0000000000..db0b066c97 --- /dev/null +++ b/apps/web/components/model-config-selector-content.tsx @@ -0,0 +1,310 @@ +"use client"; + +import { useEffect, useRef } from "react"; +import { useTranslation } from "react-i18next"; +import { IconCheck, IconChevronLeft, IconChevronRight } from "@tabler/icons-react"; + +import { cn } from "@/lib/utils"; +import * as selectorOptions from "@/lib/utils/selector-options"; +import type { ModelSelectorOption, SelectConfigOption } from "@/components/model-config-selector"; +import { Button } from "@kandev/ui/button"; +import { + Command, + CommandEmpty, + CommandGroup, + CommandInput, + CommandItem, + CommandList, +} from "@kandev/ui/command"; +import { ScrollArea } from "@kandev/ui/scroll-area"; +import { Separator } from "@kandev/ui/separator"; +import { Spinner } from "@kandev/ui/spinner"; +import { Tooltip, TooltipContent, TooltipTrigger } from "@kandev/ui/tooltip"; + +function currentOptionName(option: SelectConfigOption): string { + return ( + option.options.find((item) => item.value === option.currentValue)?.name ?? option.currentValue + ); +} + +function ModelRow({ + model, + selected, + loading, + onSelect, +}: { + model: ModelSelectorOption; + selected: boolean; + loading: boolean; + onSelect: (value: string) => void; +}) { + const item = ( + !model.disabled && onSelect(model.id)} + disabled={model.disabled} + data-testid={selected ? "model-config-selected-row" : undefined} + className={selectorOptions.selectorOptionClassName(selected, model.disabled)} + > +
+
+
{model.name}
+ {model.description && ( +
+ {model.description} +
+ )} +
+ {model.usageMultiplier && ( + {model.usageMultiplier} + )} +
+ {selected && loading ? ( + + ); + // cmdk's CommandItem swallows pointer events with no native tooltip slot; + // keep disabled items in a focusable wrapper so their unavailable reason is reachable. + if (model.disabled && model.disabledReason) { + return ( + + +
+ {item} +
+ + {model.disabledReason} + + ); + } + return item; +} + +function ConfigOptionTrigger({ + option, + onSelect, + triggerRef, +}: { + option: SelectConfigOption; + onSelect: () => void; + triggerRef?: (element: HTMLButtonElement | null) => void; +}) { + return ( + + ); +} + +function ConfigOptionSubSelector({ + option, + onBack, + onChange, +}: { + option: SelectConfigOption; + onBack: () => void; + onChange?: (configId: string, value: string) => void; +}) { + const { t } = useTranslation(); + const orderedOptions = selectorOptions.prioritizeValueOption(option.options, option.currentValue); + return ( +
+ +
+
+ {orderedOptions.map((item, index) => { + const descriptionId = item.description + ? `config-option-value-description-${option.id}-${index}` + : undefined; + return ( + + ); + })} +
+
+
+ ); +} + +export 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; + configOptionsLoading: boolean; +}; + +export function ModelConfigSelectorContent({ + activeConfig, + modelOptions, + currentModelValue, + extraConfigOptions, + onModelSelect, + onConfigSelect, + onConfigBack, + onConfigChange, + configOptionsLoading, +}: ModelConfigSelectorContentProps) { + const { t } = useTranslation(); + const pendingFocusConfigId = useRef(null); + const triggerRefs = useRef>({}); + const showModelFilter = modelOptions.length > 5; + const orderedModelOptions = selectorOptions.prioritizeIdOption(modelOptions, currentModelValue); + + 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 ( + + ); + } + + return ( + <> + + {showModelFilter && } + + {t("agents:noModelsFound")} + + {orderedModelOptions.map((model) => ( + + ))} + + + + {configOptionsLoading ? ( + <> + +
+
+ + ) : ( + extraConfigOptions.length > 0 && ( + <> + + +
+ {extraConfigOptions.map((option) => ( + onConfigSelect(option.id)} + triggerRef={(element) => { + triggerRefs.current[option.id] = element; + }} + /> + ))} +
+ + + ) + )} + + ); +} diff --git a/apps/web/components/model-config-selector.test.tsx b/apps/web/components/model-config-selector.test.tsx index 7c88169378..a181bd85d1 100644 --- a/apps/web/components/model-config-selector.test.tsx +++ b/apps/web/components/model-config-selector.test.tsx @@ -102,6 +102,58 @@ describe("ModelConfigSelector", () => { }); }); +describe("ModelConfigSelector selected options", () => { + it("puts the current model and nested config value first", () => { + render( + {}} + onConfigChange={() => {}} + configOptions={[ + { + 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 })); + expect(screen.getAllByRole("option").map((option) => option.textContent?.trim())).toEqual([ + "Model 2", + "Model 1", + "Model 3", + ]); + expect(screen.getByTestId("model-config-selected-row").className).toContain("bg-card"); + expect(screen.getByTestId("model-config-selected-row").className).toContain( + "border-primary/50", + ); + + fireEvent.click(screen.getByTestId(effortTriggerTestId)); + const effortSection = screen.getByTestId(effortSectionTestId); + expect( + within(effortSection) + .getAllByRole("button") + .map((button) => button.textContent?.trim()), + ).toEqual(["Medium", "Low", "High"]); + expect(within(effortSection).getByRole("button", { name: "Medium" }).className).toContain( + "bg-card", + ); + }); +}); + describe("ModelConfigSelector loading behavior", () => { it("keeps the picker open after selecting a model without existing dependent options", () => { const onModelChange = vi.fn(); diff --git a/apps/web/components/model-config-selector.tsx b/apps/web/components/model-config-selector.tsx index d88471cffe..dc3df3cc01 100644 --- a/apps/web/components/model-config-selector.tsx +++ b/apps/web/components/model-config-selector.tsx @@ -1,25 +1,15 @@ "use client"; -import { memo, useEffect, useRef, useState } from "react"; +import { memo, useState } from "react"; import { useTranslation } from "react-i18next"; import type { TFunction } from "i18next"; -import { IconCheck, IconChevronDown, IconChevronLeft, IconChevronRight } from "@tabler/icons-react"; +import { IconChevronDown } from "@tabler/icons-react"; import { cn } from "@/lib/utils"; import { settingsControlClassName } from "@/components/settings/settings-control"; +import { ModelConfigSelectorContent } from "@/components/model-config-selector-content"; import { Button } from "@kandev/ui/button"; -import { - Command, - CommandEmpty, - CommandGroup, - CommandInput, - CommandItem, - CommandList, -} from "@kandev/ui/command"; import { Popover, PopoverContent, PopoverTrigger } from "@kandev/ui/popover"; -import { ScrollArea } from "@kandev/ui/scroll-area"; -import { Separator } from "@kandev/ui/separator"; -import { Spinner } from "@kandev/ui/spinner"; import { Tooltip, TooltipContent, TooltipTrigger } from "@kandev/ui/tooltip"; export type ModelSelectorOption = { @@ -27,8 +17,7 @@ export type ModelSelectorOption = { name: string; description?: string; usageMultiplier?: string; - /** When true the model is unavailable ("gone") — rendered greyed out and - * not selectable, with `disabledReason` shown in a tooltip. */ + /** Unavailable models are dimmed and non-selectable with a reason tooltip. */ disabled?: boolean; disabledReason?: string; }; @@ -184,286 +173,6 @@ function triggerDetails( ); } -function ModelRow({ - model, - selected, - loading, - onSelect, -}: { - model: ModelSelectorOption; - selected: boolean; - loading: boolean; - onSelect: (value: string) => void; -}) { - const item = ( - !model.disabled && onSelect(model.id)} - disabled={model.disabled} - data-testid={selected ? "model-config-selected-row" : undefined} - className={cn("relative pr-7", model.disabled && "opacity-40 cursor-not-allowed")} - > -
-
-
{model.name}
- {model.description && ( -
- {model.description} -
- )} -
- {model.usageMultiplier && ( - {model.usageMultiplier} - )} -
- {selected && loading ? ( - - ); - // cmdk's CommandItem swallows pointer events with no native tooltip slot; - // keep disabled items in a focusable wrapper so their gone reason is reachable. - if (model.disabled && model.disabledReason) { - return ( - - -
- {item} -
- - {model.disabledReason} - - ); - } - return item; -} - -function ConfigOptionTrigger({ - option, - onSelect, - triggerRef, -}: { - option: SelectConfigOption; - onSelect: () => void; - triggerRef?: (element: HTMLButtonElement | null) => void; -}) { - return ( - - ); -} - -function ConfigOptionSubSelector({ - option, - onBack, - onChange, -}: { - option: SelectConfigOption; - onBack: () => void; - onChange?: (configId: string, value: string) => void; -}) { - const { t } = useTranslation(); - return ( -
- -
-
- {option.options.map((item, index) => { - const descriptionId = item.description - ? `config-option-value-description-${option.id}-${index}` - : undefined; - return ( - - ); - })} -
-
-
- ); -} - -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; - configOptionsLoading: boolean; -}; - -function ModelConfigSelectorContent({ - activeConfig, - modelOptions, - currentModelValue, - extraConfigOptions, - onModelSelect, - onConfigSelect, - onConfigBack, - onConfigChange, - configOptionsLoading, -}: ModelConfigSelectorContentProps) { - const { t } = useTranslation(); - const pendingFocusConfigId = useRef(null); - const triggerRefs = useRef>({}); - 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 ( - - ); - } - - return ( - <> - - {showModelFilter && } - - {t("agents:noModelsFound")} - - {modelOptions.map((model) => ( - - ))} - - - - {configOptionsLoading ? ( - <> - -
-
- - ) : ( - extraConfigOptions.length > 0 && ( - <> - - -
- {extraConfigOptions.map((option) => ( - onConfigSelect(option.id)} - triggerRef={(element) => { - triggerRefs.current[option.id] = element; - }} - /> - ))} -
- - - ) - )} - - ); -} - export type ModelConfigSelectorProps = { modelOptions: ModelSelectorOption[]; currentModel: string | null; @@ -592,9 +301,7 @@ export const ModelConfigSelector = memo(function ModelConfigSelector({ triggerSummary === "changed" ? triggerDetails(modelOptions, currentModel, modelConfig, extraConfigOptions, t) : undefined; - // The (fallback) marker is a live WS signal, not replayed on reload. When - // it is active, explain on the trigger that the note is transient so users - // are not surprised when the marker disappears after a refresh. + // The fallback marker is live-only, so explain when it is active. const triggerTitle = currentModelSuffix ? t("settings:fallbackNoteLive") : undefined; const hasExtraConfigOptions = extraConfigOptions.length > 0; diff --git a/apps/web/components/task-create-dialog-branch-options.tsx b/apps/web/components/task-create-dialog-branch-options.tsx new file mode 100644 index 0000000000..812d0513ea --- /dev/null +++ b/apps/web/components/task-create-dialog-branch-options.tsx @@ -0,0 +1,79 @@ +import { Badge } from "@kandev/ui/badge"; + +import type { Branch } from "@/lib/types/http"; +import { t } from "@/lib/i18n"; +import type { PillOption } from "./task-create-dialog-pill"; + +// Conventional default branches surfaced at the top of the dropdown when no +// search term is active. cmdk preserves option order on empty queries, so a +// stable sort here lifts main/master/develop above feature branches. +const PREFERRED_BRANCH_NAMES = ["main", "master", "develop"]; + +function branchPriority(b: Branch): number { + const idx = PREFERRED_BRANCH_NAMES.indexOf(b.name); + if (idx === -1) return PREFERRED_BRANCH_NAMES.length; + return idx; +} + +export function sortBranches(branches: Branch[]): Branch[] { + return [...branches].sort((a, b) => { + const pa = branchPriority(a); + const pb = branchPriority(b); + if (pa !== pb) return pa - pb; + // Within the same priority bucket, locals before remotes. This matches + // the auto-select preference (`main` over `origin/main`). + if (a.type !== b.type) return a.type === "local" ? -1 : 1; + return 0; + }); +} + +const BRANCH_SEGMENT_RE = /[/_.\-\s]+/; + +export function buildBranchKeywords(name: string, remote?: string): string[] { + const out = new Set(); + out.add(name); + const leafIdx = name.lastIndexOf("/"); + if (leafIdx >= 0) out.add(name.slice(leafIdx + 1)); + for (const seg of name.split(BRANCH_SEGMENT_RE)) { + if (seg) out.add(seg); + } + if (remote) out.add(remote); + return Array.from(out); +} + +export function branchToOption(b: Branch): PillOption { + // Remote branches keep their "origin/" prefix so they're distinguishable + // from local branches with the same short name (e.g. "main" vs "origin/main"). + // Without the prefix, the dropdown shows two indistinguishable rows. + const display = b.type === "remote" && b.remote ? `${b.remote}/${b.name}` : b.name; + // `||` (not `??`) so an empty-string `remote` falls back too. Provider-backed + // workspace repos (URL-added) list branches without a tracking remote, so the + // backend sends `remote: ""`; `??` would render an invisible empty badge. + const badge = b.type === "local" ? "local" : b.remote || "remote"; + return { + value: display, + label: display, + keywords: buildBranchKeywords(b.name, b.remote), + renderLabel: () => ( + + + {display} + + + {badge} + + + ), + }; +} + +export function computeBranchPlaceholder( + hasRepo: boolean, + loading: boolean, + optionCount: number, +): string { + if (!hasRepo) return "branch"; + if (loading) return "loading…"; + if (optionCount === 0) return t("task:noBranchesShort"); + return "branch"; +} diff --git a/apps/web/components/task-create-dialog-options.tsx b/apps/web/components/task-create-dialog-options.tsx index 74767c6ab1..7e9c0259c4 100644 --- a/apps/web/components/task-create-dialog-options.tsx +++ b/apps/web/components/task-create-dialog-options.tsx @@ -32,7 +32,7 @@ import { getExecutorIcon } from "@/lib/executor-icons"; import { AgentLogo } from "@/components/agent-logo"; import { getCapabilityWarning } from "@/lib/capability-warning"; import { useTouchDrawer } from "@/hooks/use-compact-task-chrome"; -import { buildBranchKeywords } from "./task-create-dialog-pill"; +import { buildBranchKeywords } from "./task-create-dialog-branch-options"; type OptionItem = { value: string; diff --git a/apps/web/components/task-create-dialog-pill.test.tsx b/apps/web/components/task-create-dialog-pill.test.tsx index 7169cbedb1..e0238df303 100644 --- a/apps/web/components/task-create-dialog-pill.test.tsx +++ b/apps/web/components/task-create-dialog-pill.test.tsx @@ -80,13 +80,12 @@ vi.mock("@kandev/ui/tooltip", async () => { }; }); +import { Pill, type PillOption } from "./task-create-dialog-pill"; import { - sortBranches, branchToOption, computeBranchPlaceholder, - Pill, - type PillOption, -} from "./task-create-dialog-pill"; + sortBranches, +} from "./task-create-dialog-branch-options"; const CREATE_REPOSITORY = "Create new repository"; @@ -319,6 +318,37 @@ describe("Pill tooltip", () => { }); describe("Pill popover", () => { + it("puts the current option first with a persistent selected surface", () => { + render( +