From 8e612dfe9843fb796447a7bd8645adc09a51442e Mon Sep 17 00:00:00 2001 From: Brad Hallett <53977268+bradhallett@users.noreply.github.com> Date: Wed, 26 Aug 2026 00:59:37 -0400 Subject: [PATCH] Group model picker by provider for ACP route models MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ACP agents such as omp advertise the same display name under multiple providers. Type description on the select-option schema, keep it through the catalog, and group the desktop picker by route — headers only when two or more providers are present. --- .../.ladle/model-picker-query-provider.tsx | 56 ++- apps/app/.ladle/story-fixtures.ts | 63 +++ .../pickers/ModelReasoningPicker.stories.tsx | 29 ++ .../pickers/ModelReasoningPicker.test.tsx | 400 +++++++++++++++++- .../pickers/ModelReasoningPicker.tsx | 280 ++++++++++-- .../pickers/model-picker-option.test.ts | 348 +++++++++++++++ .../components/pickers/model-picker-option.ts | 220 ++++++++++ .../data/compose/execution-options.test.ts | 22 + .../src/bridge/model-catalog.test.ts | 32 ++ .../src/bridge/model-catalog.ts | 2 +- packages/provider-bridge-acp/src/wire.test.ts | 11 + packages/provider-bridge-acp/src/wire.ts | 1 + 12 files changed, 1396 insertions(+), 68 deletions(-) create mode 100644 apps/app/src/components/pickers/model-picker-option.test.ts diff --git a/apps/app/.ladle/model-picker-query-provider.tsx b/apps/app/.ladle/model-picker-query-provider.tsx index 7676b2a4f7..8c5260e45d 100644 --- a/apps/app/.ladle/model-picker-query-provider.tsx +++ b/apps/app/.ladle/model-picker-query-provider.tsx @@ -16,6 +16,8 @@ import { STORY_CLAUDE_REASONING, STORY_CODEX_MODELS, STORY_CODEX_REASONING, + STORY_OMP_MODELS, + STORY_OMP_PROVIDER_OPTION, STORY_PI_MODELS, STORY_PROVIDER_OPTIONS, STORY_SERVICE_TIER_SUPPORT, @@ -46,31 +48,33 @@ const STORY_COMPOSER_ACTIONS_BY_PROVIDER: Record< }, ], pi: [], + omp: [], }; -const STORY_PROVIDER_INFOS: ProviderInfo[] = STORY_PROVIDER_OPTIONS.map( - (provider) => ({ - id: provider.value, - pluginId: `provider-${provider.value}`, - displayName: provider.label, - logoUrl: null, - available: true, - maintenance: { health: true, usage: true, installation: true }, - composerActions: [ - ...(STORY_COMPOSER_ACTIONS_BY_PROVIDER[provider.value] ?? []), - ], - capabilities: { - supportsThreadArchive: true, - supportsThreadRename: true, - supportsServiceTier: STORY_SERVICE_TIER_SUPPORT[provider.value] ?? false, - supportsNativeUserQuestion: true, - supportsFork: true, - supportsSessionRewind: true, - modelCatalogScope: "workspace", - permissionModes: [...permissionModes], - }, - }), -); +const STORY_PROVIDER_INFOS: ProviderInfo[] = [ + ...STORY_PROVIDER_OPTIONS, + STORY_OMP_PROVIDER_OPTION, +].map((provider) => ({ + id: provider.value, + pluginId: `provider-${provider.value}`, + displayName: provider.label, + logoUrl: null, + available: true, + maintenance: { health: true, usage: true, installation: true }, + composerActions: [ + ...(STORY_COMPOSER_ACTIONS_BY_PROVIDER[provider.value] ?? []), + ], + capabilities: { + supportsThreadArchive: true, + supportsThreadRename: true, + supportsServiceTier: STORY_SERVICE_TIER_SUPPORT[provider.value] ?? false, + supportsNativeUserQuestion: true, + supportsFork: true, + supportsSessionRewind: true, + modelCatalogScope: "workspace", + permissionModes: [...permissionModes], + }, +})); function makeSupportedReasoningEfforts( reasoningOptions: readonly PickerOption[], @@ -162,6 +166,12 @@ function createStoryQueryClient(): QueryClient { reasoningOptions: STORY_CODEX_REASONING, }), ), + omp: makeExecutionOptions( + makeAvailableModels({ + models: STORY_OMP_MODELS, + reasoningOptions: STORY_CODEX_REASONING, + }), + ), }; for (const [providerId, executionOptions] of Object.entries( diff --git a/apps/app/.ladle/story-fixtures.ts b/apps/app/.ladle/story-fixtures.ts index 68258ebc56..a11ed87815 100644 --- a/apps/app/.ladle/story-fixtures.ts +++ b/apps/app/.ladle/story-fixtures.ts @@ -198,6 +198,69 @@ export const STORY_PI_MODELS: readonly ModelPickerOption[] = [ }, ]; +/** + * An omp-style ACP catalog (`provider acp-omp`): ids carry their route as a + * `/` prefix, the same display name exists under several routes + * (GLM-5.3 in cursor + zai), one provider repeats a name across distinct ids + * (the mistral-medium triple), and descriptions echo the full id — the shape + * issue #2062 found across omp's 279-model catalog. + */ +export const STORY_OMP_MODELS: readonly ModelPickerOption[] = [ + { + value: "cursor/claude-sonnet-4-5", + label: "Claude Sonnet 4.5", + description: "cursor/claude-sonnet-4-5", + }, + { + value: "cursor/glm-5.3", + label: "GLM-5.3", + description: "cursor/glm-5.3", + }, + { + value: "zai/glm-5.3", + label: "GLM-5.3", + description: "zai/glm-5.3", + }, + { + value: "zai/glm-5.2-air", + label: "GLM-5.2 Air", + description: "zai/glm-5.2-air", + }, + { + value: "commandcode/zai-org/GLM-5", + label: "GLM-5", + description: "commandcode/zai-org/GLM-5", + }, + { + value: "mistral/mistral-medium-latest", + label: "mistral-medium-latest", + description: "mistral/mistral-medium-latest", + }, + { + value: "mistral/mistral-medium-2506", + label: "mistral-medium-latest", + description: "mistral/mistral-medium-2506", + }, + { + value: "mistral/mistral-medium-latest-2501", + label: "mistral-medium-latest", + description: "mistral/mistral-medium-latest-2501", + }, + { + value: "mistral/mistral-large-latest", + label: "Mistral Large", + description: "mistral/mistral-large-latest", + }, +]; + +/** The omp provider tab for picker stories; kept out of + * STORY_PROVIDER_OPTIONS so other stories' tab strips stay as they were. */ +export const STORY_OMP_PROVIDER_OPTION: PickerOption = { + value: "omp", + label: "OMP", + icon: storyProviderIcon("omp", "Layers"), +}; + export const STORY_CODEX_REASONING: readonly PickerOption[] = [ { value: "low", label: "Low" }, { value: "medium", label: "Medium" }, diff --git a/apps/app/src/components/pickers/ModelReasoningPicker.stories.tsx b/apps/app/src/components/pickers/ModelReasoningPicker.stories.tsx index 85a6c2fa9d..e685feb341 100644 --- a/apps/app/src/components/pickers/ModelReasoningPicker.stories.tsx +++ b/apps/app/src/components/pickers/ModelReasoningPicker.stories.tsx @@ -10,6 +10,8 @@ import { STORY_CLAUDE_REASONING, STORY_CODEX_MODELS, STORY_CODEX_REASONING, + STORY_OMP_MODELS, + STORY_OMP_PROVIDER_OPTION, STORY_PI_MODELS, STORY_PROVIDER_OPTIONS, STORY_SERVICE_TIER_SUPPORT, @@ -68,6 +70,7 @@ const MODEL_OPTIONS_BY_PROVIDER_ID: Record< codex: STORY_CODEX_MODELS, "claude-code": STORY_CLAUDE_CODE_MODELS, pi: STORY_PI_MODELS, + omp: STORY_OMP_MODELS, }; const MORE_MODEL_OPTIONS_BY_PROVIDER_ID: Record< @@ -77,6 +80,7 @@ const MORE_MODEL_OPTIONS_BY_PROVIDER_ID: Record< codex: [], "claude-code": STORY_CLAUDE_CODE_MORE_MODELS, pi: [], + omp: [], }; const REASONING_OPTIONS_BY_PROVIDER_ID: Record< @@ -86,6 +90,7 @@ const REASONING_OPTIONS_BY_PROVIDER_ID: Record< codex: STORY_CODEX_REASONING, "claude-code": STORY_CLAUDE_REASONING, pi: STORY_CODEX_REASONING, + omp: STORY_CODEX_REASONING, }; export function Overview() { @@ -116,6 +121,12 @@ export function Overview() { > + + + { setSelectedProviderId(providerId); @@ -191,6 +203,23 @@ function ModelReasoningPickerInteractive() { ); } +function ModelReasoningPickerOpenGrouped() { + return ( + + ); +} + function ModelReasoningPickerOpenEmpty() { return ( [] = [ { value: "sonnet-in-codex", label: "Sonnet" }, ]; +// An omp-style ACP catalog: the route lives in the value prefix, display +// names collide across providers (GLM-5.3 in cursor + zai) and within one +// provider (the mistral-medium triple), and unambiguous names stay clean. +// 9 options > MODEL_SEARCH_MIN_OPTIONS, so the search box renders. +const ompStyleModels: readonly ModelPickerOption[] = [ + { value: "cursor/claude-sonnet-4-5", label: "Claude Sonnet 4.5" }, + { value: "cursor/glm-5.3", label: "GLM-5.3" }, + { value: "zai/glm-5.3", label: "GLM-5.3" }, + { value: "zai/glm-5.2-air", label: "GLM-5.2 Air" }, + { value: "commandcode/zai-org/GLM-5", label: "GLM-5" }, + { value: "mistral/mistral-medium-latest", label: "mistral-medium-latest" }, + { value: "mistral/mistral-medium-2506", label: "mistral-medium-latest" }, + { + value: "mistral/mistral-medium-latest-2501", + label: "mistral-medium-latest", + }, + { value: "mistral/mistral-large-latest", label: "Mistral Large" }, +]; + +const ompProviderOptions: readonly ProviderPickerOption[] = [ + { value: "omp", label: "OMP" }, +]; + +// A single-route keyed catalog: every id shares the `zai/` route, so the +// list renders headerless — yet keyed rows keep raw-id tooltips and a +// within-group label collision still qualifies. Six options clear the +// search-box threshold so rows carry option semantics. +const singleRouteZai: readonly ModelPickerOption[] = [ + { value: "zai/glm-5.3", label: "GLM-5.3" }, // remainder equals label → no qualifier + { value: "zai/glm-5.3-preview", label: "GLM-5.3" }, // qualifier = glm-5.3-preview + { value: "zai/glm-5.2-air", label: "GLM-5.2 Air" }, // unique → no qualifier + { value: "zai/glm-5.2", label: "GLM-5.2" }, + { value: "zai/glm-5.1", label: "GLM-5.1" }, + { value: "zai/glm-4.7", label: "GLM-4.7" }, +]; + const reasoningOptions: readonly PickerOption[] = [ { value: "medium", label: "Medium" }, { value: "high", label: "High" }, @@ -579,7 +618,7 @@ describe("ModelReasoningPicker", () => { expect(await screen.findByText("Opus 4.7")).not.toBeNull(); }); - it("keeps duplicate Pi models distinct by their nested provider", () => { + it("groups Pi's nested-provider models under labelled route groups", () => { const apiModel = "openai/gpt-5.3-codex-spark"; const subscriptionModel = "openai-codex/gpt-5.3-codex-spark"; const modelLabel = "GPT-5.3 Codex Spark"; @@ -598,20 +637,365 @@ describe("ModelReasoningPicker", () => { }); const trigger = screen.getByRole("button", { - name: "Provider, model and reasoning", + // The qualifier rides in the aria-label so screen readers announce + // which colliding model is committed. + name: "Provider, model and reasoning, OpenAI Codex", }); + // The duplicate label is ambiguous, so the trigger says which route is + // committed; the raw value rides along in the tooltip. + expect(trigger.textContent).toContain("OpenAI Codex"); expect(trigger.textContent).toContain(modelLabel); - expect(trigger.textContent).not.toContain("openai-codex"); + expect( + (trigger.querySelector("span[title]") as HTMLElement | null)?.title, + ).toContain(subscriptionModel); fireEvent.click(trigger); - expect(screen.getAllByText(modelLabel)).toHaveLength(3); - const apiQualifier = screen.getByText("openai"); - expect(screen.getByText("openai-codex")).not.toBeNull(); - - fireEvent.click(apiQualifier); + // Each nested provider heads its own labelled group, and the duplicate + // labels stay distinct rows committing distinct full values. + expect(screen.getByRole("group", { name: "OpenAI" })).not.toBeNull(); + expect(screen.getByRole("group", { name: "OpenAI Codex" })).not.toBeNull(); + const rows = screen.getAllByText(modelLabel); + expect(rows).toHaveLength(3); // trigger + one row per provider + fireEvent.click(rows[1]); expect(onModelChange).toHaveBeenCalledWith(apiModel); + + fireEvent.click(rows[2]); + expect(onModelChange).toHaveBeenCalledWith(subscriptionModel); + }); + + it("groups a multi-route catalog under labelled provider headers", () => { + const { onModelChange } = renderPicker({ + modelOptions: ompStyleModels, + modelValue: "zai/glm-5.3", + pickerProviderOptions: ompProviderOptions, + selectedProviderId: "omp", + }); + fireEvent.click( + screen.getByRole("button", { + name: "Provider, model and reasoning, Z.ai", + }), + ); + // Every route gets a labelled group; the top "Model" label stays. + expect(screen.getByText("Model")).not.toBeNull(); + expect(screen.getByRole("group", { name: "Cursor" })).not.toBeNull(); + expect(screen.getByRole("group", { name: "Z.ai" })).not.toBeNull(); + expect(screen.getByRole("group", { name: "CommandCode" })).not.toBeNull(); + expect(screen.getByRole("group", { name: "Mistral" })).not.toBeNull(); + + // Group headers pin just below the sticky "Model" label, never on top + // of it (the offset is that label's rendered height: pt + 1.25rem line + // + pb). + expect( + screen + .getAllByText("Z.ai") + .some((node) => + node.className.includes(MODEL_GROUP_HEADER_STICKY_TOP_DESKTOP), + ), + ).toBe(true); + + // Cross-group duplicates (GLM-5.3 under cursor + zai) carry no inline + // qualifier — the headers disambiguate them — and a hover reveals the + // full raw id. + expect(screen.getAllByText("GLM-5.3")).toHaveLength(3); // trigger + 2 rows + expect(screen.queryByText("glm-5.3")).toBeNull(); + expect(screen.getByTitle("zai/glm-5.3")).not.toBeNull(); + expect(screen.getByTitle("cursor/glm-5.3")).not.toBeNull(); + + // Clicking commits the full provider/modelId value. + fireEvent.click(screen.getAllByText("GLM-5.3")[1]); + expect(onModelChange).toHaveBeenCalledWith("cursor/glm-5.3"); + }); + + it("renders single-route lists flat, keeping the pre-grouping DOM", () => { + renderPicker({ modelOptions: manyCodexModels }); + + fireEvent.click( + screen.getByRole("button", { name: "Provider, model and reasoning" }), + ); + + // One route-less, collision-free list keeps the exact DOM structure it + // had before grouping existed: no group wrappers, no headers, no + // qualifiers, and tooltips keep their default label form. A single-route + // KEYED list also drops wrappers and headers, but keeps raw-id tooltips + // and within-group qualifiers (see the keyed test below). + expect(screen.getByTitle("5.5")).not.toBeNull(); + expect(screen.queryByText("gpt-5.5")).toBeNull(); + expect(screen.queryByRole("group")).toBeNull(); + }); + + it("renders a single-route keyed list headerless but keeps raw-id tooltips and within-group qualifiers", () => { + renderPicker({ modelOptions: singleRouteZai }); + + fireEvent.click( + screen.getByRole("button", { name: "Provider, model and reasoning" }), + ); + + // One route → no headers or wrappers, but keyed rows still carry the + // raw-value tooltip instead of the default label form… + expect(screen.queryByRole("group")).toBeNull(); + expect(screen.getByTitle("zai/glm-5.3")).not.toBeNull(); + expect(screen.getByTitle("zai/glm-5.3-preview")).not.toBeNull(); + // …unique rows stay clean… + expect( + screen.getByRole("option", { name: "GLM-5.2 Air" }).textContent, + ).toBe("GLM-5.2 Air"); + // …colliding rows fold the id remainder into their accessible name (the + // same concatenation contract as the mistral case)… + expect( + screen.getByRole("option", { name: "GLM-5.3glm-5.3-preview" }), + ).not.toBeNull(); + // …and the degenerate row whose remainder restates its label stays clean. + expect(screen.getByRole("option", { name: "GLM-5.3" }).textContent).toBe( + "GLM-5.3", + ); + }); + + it("qualifies only within-provider label collisions", () => { + renderPicker({ + modelOptions: ompStyleModels, + modelValue: "mistral/mistral-medium-2506", + pickerProviderOptions: ompProviderOptions, + selectedProviderId: "omp", + }); + + fireEvent.click( + screen.getByRole("button", { + name: "Provider, model and reasoning, Mistral · mistral-medium-2506", + }), + ); + + const listbox = screen.getByRole("listbox", { name: "Models" }); + // Three rows share the rendered label — the alias whose id tail restates + // the label stays clean rather than duplicating it, the other two carry + // the id-revealing qualifier, and the trigger's copy sits outside the + // list. + expect(within(listbox).getAllByText("mistral-medium-latest")).toHaveLength( + 3, + ); + + // Screen-reader names stay distinct: the accessible name folds in the + // qualifier text, so each colliding row is addressable by name alone — + // and the committed row (mistral-medium-2506) announces which one it is. + const aliasRow = within(listbox).getByRole("option", { + name: "mistral-medium-latest", + }); + expect(aliasRow.textContent).toBe("mistral-medium-latest"); + expect( + within(listbox).getByRole("option", { + name: "mistral-medium-latestmistral-medium-2506", + }).textContent, + ).toContain("mistral-medium-2506"); + expect( + within(listbox).getByRole("option", { + name: "mistral-medium-latestmistral-medium-latest-2501", + }).textContent, + ).toContain("mistral-medium-latest-2501"); + // …while unique rows under the same provider stay clean. + const largeRow = within(listbox) + .getByText("Mistral Large") + .closest("button")?.textContent; + expect(largeRow).toBe("Mistral Large"); + }); + + it("regroups search results and re-applies the header rule", () => { + renderPicker({ + modelOptions: ompStyleModels, + pickerProviderOptions: ompProviderOptions, + selectedProviderId: "omp", + }); + + fireEvent.click( + screen.getByRole("button", { name: "Provider, model and reasoning" }), + ); + const search = screen.getByPlaceholderText("Search models"); + + // "zai" matches the zai rows and the nested commandcode id, so two + // routes survive and both keep their headers; cursor and mistral go. + fireEvent.change(search, { target: { value: "zai" } }); + expect(screen.getByRole("group", { name: "Z.ai" })).not.toBeNull(); + expect(screen.getByRole("group", { name: "CommandCode" })).not.toBeNull(); + expect(screen.queryByRole("group", { name: "Cursor" })).toBeNull(); + expect(screen.queryByRole("group", { name: "Mistral" })).toBeNull(); + expect( + within(screen.getByRole("listbox", { name: "Models" })).queryByText( + "Claude Sonnet 4.5", + ), + ).toBeNull(); + + // The pretty provider name is searchable too. Narrowing to a single + // route hides the headers again (the ≥2-route rule), but rows stay. + fireEvent.change(search, { target: { value: "Z.ai" } }); + expect(screen.getByText("GLM-5.3")).not.toBeNull(); + expect(screen.queryByRole("group")).toBeNull(); + + // Narrow to one route and the headers disappear again. + fireEvent.change(search, { target: { value: "mistral-large" } }); + expect(screen.queryByRole("group")).toBeNull(); + expect(screen.getByText("Mistral Large")).not.toBeNull(); + expect(screen.queryByText("mistral-medium-latest")).toBeNull(); + }); + + it("keeps keyboard selection working across group wrappers", () => { + const { onModelChange } = renderPicker({ + modelOptions: ompStyleModels, + pickerProviderOptions: ompProviderOptions, + selectedProviderId: "omp", + }); + + fireEvent.click( + screen.getByRole("button", { name: "Provider, model and reasoning" }), + ); + const search = screen.getByPlaceholderText("Search models"); + fireEvent.change(search, { target: { value: "glm" } }); + + // Headers are not nav rows: the first arrow-down lands on the first + // surviving model (cursor's GLM-5.3), not on a header. + fireEvent.keyDown(search, { key: "ArrowDown" }); + expect(search.getAttribute("aria-activedescendant")).not.toBeNull(); + fireEvent.keyDown(search, { key: "Enter" }); + expect(onModelChange).toHaveBeenCalledWith("cursor/glm-5.3"); + }); + + it("cycles a grouped catalog in list order", () => { + const { onModelChange } = renderPicker({ + modelOptions: ompStyleModels, + modelValue: "zai/glm-5.3", + pickerProviderOptions: ompProviderOptions, + selectedProviderId: "omp", + }); + const target = screen.getByRole("button", { + name: "Provider, model and reasoning, Z.ai", + }); + + expect( + commandHandlers.get("modelPicker.cycleModelBackward")?.({ target }), + ).toBe(true); + expect(onModelChange).toHaveBeenCalledWith("cursor/glm-5.3"); + }); + + it("shows the distinguishing token on the trigger only when ambiguous", () => { + const ambiguous = renderPicker({ + modelOptions: ompStyleModels, + modelValue: "zai/glm-5.3", + pickerProviderOptions: ompProviderOptions, + selectedProviderId: "omp", + }); + const ambiguousTrigger = screen.getByRole("button", { + name: "Provider, model and reasoning, Z.ai", + }); + // Cross-group ambiguity surfaces the pretty route name… + expect(ambiguousTrigger.textContent).toContain("Z.ai"); + expect(ambiguousTrigger.textContent).not.toContain("Cursor"); + cleanup(); + + renderPicker({ + modelOptions: ompStyleModels, + modelValue: "zai/glm-5.2-air", + pickerProviderOptions: ompProviderOptions, + selectedProviderId: "omp", + onModelChange: ambiguous.onModelChange, + }); + // …and a unique label renders the trigger unchanged. + const uniqueTrigger = screen.getByRole("button", { + name: "Provider, model and reasoning", + }); + expect(uniqueTrigger.textContent).toContain("GLM-5.2 Air"); + expect(uniqueTrigger.textContent).not.toContain("Z.ai"); + expect(uniqueTrigger.textContent).not.toContain("zai"); + }); + + it("qualifies a within-group collision on the trigger with route name and id tail", () => { + renderPicker({ + modelOptions: ompStyleModels, + modelValue: "mistral/mistral-medium-2506", + pickerProviderOptions: ompProviderOptions, + selectedProviderId: "omp", + }); + + // The suffix alone must pick out the committed row among its + // same-provider siblings. + const trigger = screen.getByRole("button", { + name: "Provider, model and reasoning, Mistral · mistral-medium-2506", + }); + expect(trigger.textContent).toContain("Mistral · mistral-medium-2506"); + }); + + it("groups the more-models submenu under the same headers", () => { + const { onModelChange } = renderPicker({ + modelOptions: manyCodexModels, + moreModelOptions: ompStyleModels, + }); + + fireEvent.click( + screen.getByRole("button", { name: "Provider, model and reasoning" }), + ); + // The submenu is hover-driven: pointer-over opens it and marks its + // trigger as the menu's last-hovered item (a bare click without hover is + // immediately closed again by the submenu's blur-away effect). + fireEvent.pointerOver(screen.getByText("More models")); + + expect(screen.getByRole("group", { name: "Z.ai" })).not.toBeNull(); + expect(screen.getByTitle("zai/glm-5.3")).not.toBeNull(); + fireEvent.click(screen.getByTitle("zai/glm-5.3")); + expect(onModelChange).toHaveBeenCalledWith("zai/glm-5.3"); + }); + + it("groups the compact drawer's model list and keeps the more-models toggle in place", () => { + const frames: FrameRequestCallback[] = []; + vi.spyOn(window, "requestAnimationFrame").mockImplementation((callback) => { + frames.push(callback); + return frames.length; + }); + renderPicker({ + modelOptions: ompStyleModels, + moreModelOptions: [ + { value: "cursor/claude-haiku-4-5", label: "Claude Haiku 4.5" }, + { value: "mistral/codestral-latest", label: "Codestral" }, + ], + pickerProviderOptions: ompProviderOptions, + selectedProviderId: "omp", + compact: true, + }); + + // The drawer content mounts on the next animation frame. + fireEvent.click( + screen.getByRole("button", { name: "Provider, model and reasoning" }), + ); + act(() => frames.shift()?.(0)); + act(() => frames.shift()?.(16)); + // The drawer groups exactly like the desktop menu, and its headers pin + // just below the sticky "Model" label at the compact offset. + expect(screen.getByRole("group", { name: "Z.ai" })).not.toBeNull(); + expect(screen.getByRole("group", { name: "Cursor" })).not.toBeNull(); + expect(screen.getByText("Z.ai").className).toContain( + MODEL_GROUP_HEADER_STICKY_TOP_COMPACT, + ); + + // Expanding more models renders the new runs after the toggle, not + // hoisted above it into their provider's first group. + fireEvent.click(screen.getByText("More models")); + const groups = screen.getAllByRole("group"); + const labelledBy = groups.map((group) => + group.getAttribute("aria-labelledby"), + ); + const groupLabels = labelledBy.map( + (id) => document.getElementById(id ?? "")?.textContent, + ); + expect(groupLabels).toEqual([ + "Cursor", + "Z.ai", + "CommandCode", + "Mistral", + "Cursor", + "Mistral", + ]); + // Repeated routes get their own header element (block index in the id), + // so the aria-labelledby pointers never collide. + expect(new Set(labelledBy).size).toBe(labelledBy.length); + expect(screen.getByText("Claude Haiku 4.5")).not.toBeNull(); + expect(screen.getByText("Codestral")).not.toBeNull(); }); it("fuzzy-filters a long model list and selects the match by keyboard", () => { diff --git a/apps/app/src/components/pickers/ModelReasoningPicker.tsx b/apps/app/src/components/pickers/ModelReasoningPicker.tsx index c6d8cf0862..b51126a7f1 100644 --- a/apps/app/src/components/pickers/ModelReasoningPicker.tsx +++ b/apps/app/src/components/pickers/ModelReasoningPicker.tsx @@ -1,4 +1,5 @@ import { + Fragment, useCallback, useEffect, useId, @@ -54,7 +55,15 @@ import { OPTION_TRIGGER_CONTENT_CLASS_NAME, } from "@bb/shared-ui/option-display"; import { type PickerOption } from "./OptionPicker"; -import type { ModelPickerOption } from "./model-picker-option"; +import { + groupModelOptions, + hasMultipleRouteGroups, + modelRouteKey, + qualifyCollidingLabels, + routeProviderDisplayName, + selectedModelQualifier, + type ModelPickerOption, +} from "./model-picker-option"; import { formatModelLoadErrorText, ModelLoadErrorMessage, @@ -158,7 +167,16 @@ function modelSearchText( option: ModelPickerOption, brandPrefix: string | undefined, ): string { - return `${stripModelBrandPrefix(option.label, brandPrefix)} ${option.routeProviderId ?? ""} ${option.value}`; + // The raw route key comes from the value or routeProviderId; the pretty + // name additionally lets "Z.ai" or "OpenAI Codex" find their groups. + const routeKey = modelRouteKey(option); + return [ + stripModelBrandPrefix(option.label, brandPrefix), + routeKey !== null + ? `${routeKey} ${routeProviderDisplayName(routeKey)}` + : "", + option.value, + ].join(" "); } /** @@ -186,9 +204,10 @@ export function buildModelNavRows({ isSearching: boolean; showMoreModels: boolean; }): ModelNavRow[] { - const rows: ModelNavRow[] = modelOptions.map( - (option): ModelNavRow => ({ kind: "model", option }), - ); + const rows: ModelNavRow[] = modelOptions.map((option): ModelNavRow => ({ + kind: "model", + option, + })); if (moreModelOptions.length === 0) return rows; // While searching, flatten every match into one list so results otherwise @@ -213,6 +232,45 @@ export function buildModelNavRows({ return rows; } +/** + * The model list rendered as contiguous same-route runs, aligned 1:1 with the + * `navRows` sequence: every model keeps its nav index, and non-model rows + * (the compact "More models" toggle) break runs and render in place. Keeping + * DOM order identical to the keyboard-nav order is what lets headers wrap + * rows without touching arrow-key, Enter, or active-descendant math — and a + * catalog that interleaves routes simply yields one group per run instead of + * reordering rows out from under the highlight. Catalogs list each provider's + * models together, so a route normally yields exactly one group. + */ +type ModelListBlock = + | { kind: "toggle"; navIndex: number } + | { + kind: "group"; + routeKey: string | null; + entries: { navIndex: number; option: ModelPickerOption }[]; + }; + +function buildModelListBlocks( + navRows: readonly ModelNavRow[], +): ModelListBlock[] { + const blocks: ModelListBlock[] = []; + let currentGroup: Extract | null = null; + navRows.forEach((row, navIndex) => { + if (row.kind === "more-toggle") { + currentGroup = null; + blocks.push({ kind: "toggle", navIndex }); + return; + } + const routeKey = modelRouteKey(row.option); + if (currentGroup === null || currentGroup.routeKey !== routeKey) { + currentGroup = { kind: "group", routeKey, entries: [] }; + blocks.push(currentGroup); + } + currentGroup.entries.push({ navIndex, option: row.option }); + }); + return blocks; +} + interface ModelReasoningPickerProps { // Provider state providerRouting?: SystemProvidersQuery; @@ -411,6 +469,20 @@ export function ModelReasoningPicker({ const { base: triggerModelBase, tag: triggerModelTag } = splitModelLabelTag(triggerModelLabel); + // When the committed model's rendered label also belongs to another model + // in the active list, the trigger alone can't say which one is selected — + // show the distinguishing token (route key cross-group, value remainder + // within a group) so the committed state is unambiguous. + const triggerModelQualifier = useMemo( + () => + selectedModelQualifier( + [...modelOptions, ...moreModelOptions], + modelValue, + selectedProvider?.brandPrefix, + ), + [modelOptions, moreModelOptions, modelValue, selectedProvider?.brandPrefix], + ); + const selectedReasoningOption = reasoningOptions.find( (r) => r.value === reasoningValue, ); @@ -580,6 +652,31 @@ export function ModelReasoningPicker({ ], ); + // The model rows currently on screen (post-search), which is what headers + // and qualifiers are derived from — a narrowed query regroups the survivors + // and re-applies the ≥2-route header rule to just them. + const visibleModelOptions = useMemo( + () => navRows.flatMap((row) => (row.kind === "model" ? [row.option] : [])), + [navRows], + ); + const visibleModelQualifiers = useMemo( + () => qualifyCollidingLabels(visibleModelOptions, activeBrandPrefix), + [visibleModelOptions, activeBrandPrefix], + ); + const modelListBlocks = useMemo( + () => buildModelListBlocks(navRows), + [navRows], + ); + const showModelGroupHeaders = useMemo( + () => hasMultipleRouteGroups(groupModelOptions(visibleModelOptions)), + [visibleModelOptions], + ); + // Block index keeps ids unique when one route yields several runs (compact + // "More models" expansion renders a provider's models above and below the + // toggle); React keys already carry it. + const modelGroupHeaderId = (routeKey: string, blockIndex: number): string => + `${navId}-group-${routeKey}-${blockIndex}`; + // The active index clamped to the rows currently on screen. When the list // shrinks (e.g. the query narrows it) a now-out-of-range index simply reads as // "nothing highlighted" until the user arrows again — no reactive clamping @@ -888,12 +985,23 @@ export function ModelReasoningPicker({ ? "Loading models..." : selectedModelLoadFailed ? selectedModelLoadErrorText - : triggerModelLabel; + : triggerModelQualifier + ? `${triggerModelLabel} · ${modelValue}` + : triggerModelLabel; const triggerTitle = [ `${selectedProviderLabel}: ${triggerTitleModelLabel}`, triggerReasoningLabel ? ` · ${triggerReasoningLabel} reasoning` : "", showSelectedFastMode ? " (Fast mode)" : "", ].join(""); + const triggerBaseAriaLabel = toggleShortcut + ? `Provider, model and reasoning (${toggleShortcut.label})` + : "Provider, model and reasoning"; + // The qualifier rides in the aria-label (not just the visible suffix) so + // screen readers announce which of the colliding models is committed — an + // aria-label override would otherwise mute the visible text entirely. + const triggerAriaLabel = triggerModelQualifier + ? `${triggerBaseAriaLabel}, ${triggerModelQualifier}` + : triggerBaseAriaLabel; // The trigger renders identically whether interactive or disabled — the only // difference is the `disabled` button state and a dropped chevron — so fully // read-only surfaces show the same model label in the same position as their @@ -904,11 +1012,7 @@ export function ModelReasoningPicker({ type="button" variant="ghost" size="sm" - aria-label={ - toggleShortcut - ? `Provider, model and reasoning (${toggleShortcut.label})` - : "Provider, model and reasoning" - } + aria-label={triggerAriaLabel} aria-keyshortcuts={toggleShortcut?.ariaKeyshortcuts} disabled={disabled} className={cn( @@ -967,6 +1071,11 @@ export function ModelReasoningPicker({ {triggerModelTag} ) : null} + {triggerModelQualifier ? ( + + {triggerModelQualifier} + + ) : null} {triggerReasoningLabel ? ( ) : hasActiveModelOptions ? ( <> - {navRows.map((row, index) => { - const active = highlightedIndex === index; - const domId = optionDomId(index); - if (row.kind === "more-toggle") { + {modelListBlocks.map((block, blockIndex) => { + if (block.kind === "toggle") { return ( setShowMoreModels((current) => !current) @@ -1135,25 +1242,62 @@ export function ModelReasoningPicker({ /> ); } - const option = row.option; - return ( + // Headers only pay their way when the visible list + // spans ≥2 route providers. With one route (or none) + // there are no wrappers and no headers; rows keep the + // raw-value tooltip whenever they carry a route key, + // and within-group label collisions still qualify — + // the pre-grouping DOM is matched only by + // collision-free route-less lists. + const headerKey = + showModelGroupHeaders && block.routeKey !== null + ? block.routeKey + : null; + const rows = block.entries.map(({ navIndex, option }) => ( handleModelSelect(option.value)} /> + )); + if (headerKey === null) { + return ( + + {rows} + + ); + } + const headerId = modelGroupHeaderId(headerKey, blockIndex); + return ( +
+ {/* Group headers pin just below the sticky "Model" + label so the two never paint over each other. */} + + {routeProviderDisplayName(headerKey)} + + {rows} +
); })} {/* Desktop, not searching: the selected-only models live in a @@ -1278,15 +1422,41 @@ export function ModelReasoningPicker({ ); } +/** Sticky offset matching the "Model" MenuSectionLabel height (pt-2 + 1.25rem line + pb). */ +export const MODEL_GROUP_HEADER_STICKY_TOP_DESKTOP = "top-[33px]"; +export const MODEL_GROUP_HEADER_STICKY_TOP_COMPACT = "top-[34px]"; + // Mirrors DropdownMenuLabel spacing/typography while staying sticky in the // scrollable model list. -function MenuSectionLabel({ children }: { children: ReactNode }) { +function MenuSectionLabel({ + id, + pinBelowLabel = false, + children, +}: { + /** Optional anchor id so a group wrapper can label itself via + * `aria-labelledby`. */ + id?: string; + /** + * Sticky group headers pin just below the outer "Model" label instead of + * the scroll top, so the two sticky labels never paint over each other. + * The offsets are that label's rendered height per viewport mode + * (pt-2 + 1.25rem line height + pb). + */ + pinBelowLabel?: boolean; + children: ReactNode; +}) { const isCompactViewport = useIsCompactViewport(); return (
@@ -1386,8 +1556,18 @@ function MoreModelsSubmenu({ onSelect: (value: string) => void; }) { const { isLastHovered, hoverProps } = useMenuItemHover(); + const submenuId = useId(); const triggerRef = useRef(null); const contentRef = useRef(null); + // Same grouping contract as the main list: headers for ≥2 routes, inline + // qualifiers only for within-group label collisions, tooltips carrying the + // raw value. + const groups = useMemo(() => groupModelOptions(options), [options]); + const qualifiers = useMemo( + () => qualifyCollidingLabels(options, activeBrandPrefix), + [options, activeBrandPrefix], + ); + const showGroupHeaders = hasMultipleRouteGroups(groups); const focusFirstSubItem = useCallback(() => { window.setTimeout(() => { contentRef.current?.querySelector("button")?.focus(); @@ -1467,15 +1647,36 @@ function MoreModelsSubmenu({ }} > - {options.map((option) => ( - onSelect(option.value)} - /> - ))} + {groups.map((group, groupIndex) => { + const rows = group.options.map((option) => ( + onSelect(option.value)} + /> + )); + if (group.key === null || !showGroupHeaders) { + return ( + {rows} + ); + } + const headerId = `${submenuId}-group-${group.key}`; + return ( +
+ + {routeProviderDisplayName(group.key)} + + {rows} +
+ ); + })} @@ -1499,6 +1700,7 @@ function ResetBrowseStateOnContentUnmount({ function MenuRowButton({ label, qualifier, + title, selected, disabled = false, onClick, @@ -1510,6 +1712,12 @@ function MenuRowButton({ }: { label: string; qualifier?: string; + /** + * Overrides the default label(·qualifier) tooltip — model rows pass their + * raw value (`zai/glm-5.3`) so a hover always reveals the full id. + * Reasoning rows keep the default. + */ + title?: string; selected: boolean; disabled?: boolean; onClick: () => void; @@ -1548,7 +1756,7 @@ function MenuRowButton({ > {base} {tag ? ( diff --git a/apps/app/src/components/pickers/model-picker-option.test.ts b/apps/app/src/components/pickers/model-picker-option.test.ts new file mode 100644 index 0000000000..590bca1804 --- /dev/null +++ b/apps/app/src/components/pickers/model-picker-option.test.ts @@ -0,0 +1,348 @@ +import { describe, expect, it } from "vitest"; +import type { ModelPickerOption } from "./model-picker-option"; +import { + groupModelOptions, + hasMultipleRouteGroups, + modelRouteKey, + qualifyCollidingLabels, + ROUTE_PROVIDER_DISPLAY_NAMES, + routeProviderDisplayName, + selectedModelQualifier, +} from "./model-picker-option"; + +// The omp catalog shape: route lives in the value prefix, display names +// collide across (and within) providers. +function model({ + value, + label, + routeProviderId, +}: { + value: string; + label: string; + routeProviderId?: string; +}): ModelPickerOption { + return { + value, + label, + ...(routeProviderId === undefined ? {} : { routeProviderId }), + }; +} + +const glmDupAcrossProviders: readonly ModelPickerOption[] = [ + model({ value: "cursor/glm-5.3", label: "GLM-5.3" }), + model({ value: "zai/glm-5.3", label: "GLM-5.3" }), + model({ value: "zai/glm-5.2-air", label: "GLM-5.2 Air" }), +]; + +const mistralTripleCollision: readonly ModelPickerOption[] = [ + model({ + value: "mistral/mistral-medium-latest", + label: "mistral-medium-latest", + }), + model({ + value: "mistral/mistral-medium-2506", + label: "mistral-medium-latest", + }), + model({ + value: "mistral/mistral-medium-latest-2501", + label: "mistral-medium-latest", + }), + model({ value: "mistral/mistral-large-latest", label: "Mistral Large" }), +]; + +describe("modelRouteKey", () => { + it("derives the key from the value's route prefix", () => { + expect( + modelRouteKey(model({ value: "zai/glm-5.3", label: "GLM-5.3" })), + ).toBe("zai"); + }); + + it("keeps only the leading segment of nested route ids", () => { + expect( + modelRouteKey( + model({ value: "commandcode/zai-org/GLM-5", label: "GLM-5" }), + ), + ).toBe("commandcode"); + }); + + it("prefers a declared routeProviderId over the value prefix", () => { + expect( + modelRouteKey( + model({ + value: "cursor/glm-5.3", + label: "GLM-5.3", + routeProviderId: "zai", + }), + ), + ).toBe("zai"); + }); + + it("returns null for route-less ids and missing prefixes", () => { + expect( + modelRouteKey(model({ value: "gpt-5.5", label: "GPT-5.5" })), + ).toBeNull(); + expect(modelRouteKey(model({ value: "/glm", label: "GLM" }))).toBeNull(); + }); +}); + +describe("groupModelOptions", () => { + it("groups by key in first-appearance order, merging repeats", () => { + const groups = groupModelOptions([ + model({ value: "zai/glm-5.3", label: "GLM-5.3" }), + model({ value: "cursor/claude-sonnet-4-5", label: "Claude Sonnet 4.5" }), + model({ value: "zai/glm-5.2-air", label: "GLM-5.2 Air" }), + ]); + + expect(groups.map((group) => group.key)).toEqual(["zai", "cursor"]); + expect(groups[0].options.map((option) => option.value)).toEqual([ + "zai/glm-5.3", + "zai/glm-5.2-air", + ]); + }); + + it("collects route-less models into the leading, headerless group", () => { + const groups = groupModelOptions([ + model({ value: "gpt-5.5", label: "GPT-5.5" }), + model({ value: "zai/glm-5.3", label: "GLM-5.3" }), + model({ value: "gpt-5.4", label: "GPT-5.4" }), + ]); + + expect(groups.map((group) => group.key)).toEqual([null, "zai"]); + expect(groups[0].options.map((option) => option.value)).toEqual([ + "gpt-5.5", + "gpt-5.4", + ]); + }); + + it("yields a single headerless group for an all-route-less list", () => { + const groups = groupModelOptions([ + model({ value: "gpt-5.5", label: "GPT-5.5" }), + model({ value: "claude-sonnet-5", label: "Claude Sonnet 5" }), + ]); + + expect(groups).toHaveLength(1); + expect(groups[0].key).toBeNull(); + }); +}); + +describe("hasMultipleRouteGroups", () => { + it("is true only when ≥2 keyed groups are present", () => { + const single = groupModelOptions([ + model({ value: "zai/glm-5.3", label: "GLM-5.3" }), + model({ value: "zai/glm-5.2", label: "GLM-5.2" }), + model({ value: "gpt-5.5", label: "GPT-5.5" }), + ]); + expect(hasMultipleRouteGroups(single)).toBe(false); + + const multi = groupModelOptions(glmDupAcrossProviders); + expect(hasMultipleRouteGroups(multi)).toBe(true); + }); + + it("stays false for an all-route-less list", () => { + expect( + hasMultipleRouteGroups( + groupModelOptions([ + model({ value: "gpt-5.5", label: "GPT-5.5" }), + model({ value: "gpt-5.2", label: "GPT-5.2" }), + ]), + ), + ).toBe(false); + }); +}); + +describe("routeProviderDisplayName", () => { + it("maps every known route key to its human-readable name", () => { + expect(ROUTE_PROVIDER_DISPLAY_NAMES.get("cursor")).toBe("Cursor"); + expect(ROUTE_PROVIDER_DISPLAY_NAMES.get("zai")).toBe("Z.ai"); + expect(ROUTE_PROVIDER_DISPLAY_NAMES.get("mistral")).toBe("Mistral"); + expect(ROUTE_PROVIDER_DISPLAY_NAMES.get("google-antigravity")).toBe( + "Google Antigravity", + ); + expect(ROUTE_PROVIDER_DISPLAY_NAMES.get("xai-oauth")).toBe("xAI"); + expect(ROUTE_PROVIDER_DISPLAY_NAMES.get("alibaba-token-plan")).toBe( + "Alibaba", + ); + expect(ROUTE_PROVIDER_DISPLAY_NAMES.get("opencode-zen")).toBe( + "OpenCode Zen", + ); + expect(ROUTE_PROVIDER_DISPLAY_NAMES.get("openai-codex")).toBe( + "OpenAI Codex", + ); + expect(ROUTE_PROVIDER_DISPLAY_NAMES.get("commandcode")).toBe("CommandCode"); + }); + + it("prettifies unknown keys: hyphens become spaces, words capitalize", () => { + expect(routeProviderDisplayName("new-route-x")).toBe("New Route X"); + expect(routeProviderDisplayName("acme")).toBe("Acme"); + expect(routeProviderDisplayName("a--b")).toBe("A B"); + }); + + it("is display-only: grouping keys stay raw", () => { + const groups = groupModelOptions([ + model({ value: "zai/glm-5.3", label: "GLM-5.3" }), + model({ value: "commandcode/zai-org/GLM-5", label: "GLM-5" }), + ]); + + expect(groups.map((group) => group.key)).toEqual(["zai", "commandcode"]); + }); +}); + +describe("qualifyCollidingLabels", () => { + it("qualifies only within-group collisions, suppressing id-restating remainders", () => { + const qualifiers = qualifyCollidingLabels([ + ...glmDupAcrossProviders, + ...mistralTripleCollision, + ]); + + // Cross-group GLM-5.3 duplicates stay clean — headers disambiguate them. + expect(qualifiers.has("cursor/glm-5.3")).toBe(false); + expect(qualifiers.has("zai/glm-5.3")).toBe(false); + expect(qualifiers.has("zai/glm-5.2-air")).toBe(false); + + // The mistral triple collides inside one provider, so each row shows the + // part of its id the label hides — except the alias whose remainder + // merely restates the label: it stays clean. + expect(qualifiers.has("mistral/mistral-medium-latest")).toBe(false); + expect(qualifiers.get("mistral/mistral-medium-2506")).toBe( + "mistral-medium-2506", + ); + expect(qualifiers.get("mistral/mistral-medium-latest-2501")).toBe( + "mistral-medium-latest-2501", + ); + expect(qualifiers.has("mistral/mistral-large-latest")).toBe(false); + }); + + it("suppresses a remainder that restates the label case-insensitively", () => { + // The alias's id tail equals its rendered label modulo casing — the + // qualifier would render "GLM-5glm-5". + const qualifiers = qualifyCollidingLabels([ + model({ value: "zai/glm-5", label: "GLM-5" }), + model({ value: "zai/glm-5.5", label: "GLM-5" }), + ]); + + expect(qualifiers.has("zai/glm-5")).toBe(false); + expect(qualifiers.get("zai/glm-5.5")).toBe("glm-5.5"); + }); + + it("keeps nested route ids' remainder path intact", () => { + const qualifiers = qualifyCollidingLabels([ + model({ value: "commandcode/zai-org/GLM-5", label: "GLM-5" }), + model({ value: "commandcode/zai-org/GLM-5.5", label: "GLM-5" }), + ]); + + expect(qualifiers.get("commandcode/zai-org/GLM-5")).toBe("zai-org/GLM-5"); + expect(qualifiers.get("commandcode/zai-org/GLM-5.5")).toBe( + "zai-org/GLM-5.5", + ); + }); + + it("falls back to the full value when it carries no route prefix", () => { + const qualifiers = qualifyCollidingLabels([ + model({ value: "gpt-5.5", label: "GPT" }), + model({ value: "gpt-5.4", label: "GPT" }), + ]); + + expect(qualifiers.get("gpt-5.5")).toBe("gpt-5.5"); + expect(qualifiers.get("gpt-5.4")).toBe("gpt-5.4"); + }); + + it("compares brand-stripped rendered labels", () => { + const qualifiers = qualifyCollidingLabels( + [ + model({ value: "zai/Codex-5", label: "Zai Codex 5" }), + model({ value: "zai/codex-5-preview", label: "Codex 5" }), + ], + "Zai ", + ); + + // Both render "Codex 5" once the brand prefix is stripped. + expect(qualifiers.get("zai/Codex-5")).toBe("Codex-5"); + expect(qualifiers.get("zai/codex-5-preview")).toBe("codex-5-preview"); + }); + + it("treats trailing parenthetical tags as part of the rendered label", () => { + const qualifiers = qualifyCollidingLabels([ + model({ value: "zai/glm-5.3", label: "GLM-5.3 (1M)" }), + model({ value: "zai/glm-5.3-pro", label: "GLM-5.3" }), + ]); + + // The tag separates the rendered labels, so neither row qualifies. + expect(qualifiers.size).toBe(0); + }); +}); + +describe("selectedModelQualifier", () => { + it("uses the pretty route name for a cross-group collision", () => { + expect(selectedModelQualifier(glmDupAcrossProviders, "zai/glm-5.3")).toBe( + "Z.ai", + ); + expect( + selectedModelQualifier(glmDupAcrossProviders, "cursor/glm-5.3"), + ).toBe("Cursor"); + }); + + it("qualifies a within-group collision with route name and remainder", () => { + // The trigger stands alone, so the suffix alone must pick out the row: + // provider plus the id tail that separates it from its group siblings. + expect( + selectedModelQualifier( + mistralTripleCollision, + "mistral/mistral-medium-2506", + ), + ).toBe("Mistral · mistral-medium-2506"); + }); + + it("returns null while the label is unambiguous", () => { + expect( + selectedModelQualifier(glmDupAcrossProviders, "zai/glm-5.2-air"), + ).toBeNull(); + expect( + selectedModelQualifier( + mistralTripleCollision, + "mistral/mistral-large-latest", + ), + ).toBeNull(); + }); + + it("returns null when the value is not in the list", () => { + expect(selectedModelQualifier(glmDupAcrossProviders, "gone/1")).toBeNull(); + }); + + it("drops a remainder that restates the label, within-group only", () => { + // The alias's remainder equals its label, so nothing distinguishes it + // textually — the raw-value tooltip carries the id instead. + expect( + selectedModelQualifier( + mistralTripleCollision, + "mistral/mistral-medium-latest", + ), + ).toBeNull(); + }); + + it("falls back to the pretty route name for a degenerate remainder with a cross-group collision", () => { + // "GLM-5.3" collides in zai (selected alias's remainder restates its + // label) and in cursor; the degenerate suffix would render + // "GLM-5.3 · glm-5.3", so the route name alone carries the disambiguation. + const options: readonly ModelPickerOption[] = [ + model({ value: "cursor/glm-5.3", label: "GLM-5.3" }), + model({ value: "zai/glm-5.3", label: "GLM-5.3" }), + model({ value: "zai/glm-5.3-air", label: "GLM-5.3" }), + ]; + + expect(selectedModelQualifier(options, "zai/glm-5.3")).toBe("Z.ai"); + }); + + it("prefers the remainder when collisions span both scopes", () => { + // "GLM-5.3" exists twice in zai plus once in cursor; the route name + // alone cannot separate the two zai rows, so the remainder rides along. + const options: readonly ModelPickerOption[] = [ + model({ value: "cursor/glm-5.3", label: "GLM-5.3" }), + model({ value: "zai/glm-5.3", label: "GLM-5.3" }), + model({ value: "zai/glm-5.3-air", label: "GLM-5.3" }), + ]; + + expect(selectedModelQualifier(options, "zai/glm-5.3-air")).toBe( + "Z.ai · glm-5.3-air", + ); + }); +}); diff --git a/apps/app/src/components/pickers/model-picker-option.ts b/apps/app/src/components/pickers/model-picker-option.ts index c7f746f1f2..bf114ec077 100644 --- a/apps/app/src/components/pickers/model-picker-option.ts +++ b/apps/app/src/components/pickers/model-picker-option.ts @@ -1,6 +1,226 @@ import type { PickerOption } from "./OptionPicker"; +import { stripModelBrandPrefix } from "./model-brand-prefix"; /** A model option can expose a distinct runtime route beside its friendly name. */ export interface ModelPickerOption extends PickerOption { routeProviderId?: string; } + +/** A provider's slice of a model list: its route key plus the rows under it. */ +export interface ModelOptionGroup { + /** + * The route every option in this group is reached through, or null for + * models with no derivable route (they render headerless). + */ + key: string | null; + options: ModelPickerOption[]; +} + +/** + * The route provider a model row groups under: the daemon-declared + * `routeProviderId` when present, else the `/` prefix of the raw value + * (ACP catalogs such as omp encode the route there — + * `zai/glm-5.3` → `"zai"`, `commandcode/zai-org/GLM-5` → `"commandcode"`). + * Null when neither exists, i.e. for providers whose ids carry no route + * (Codex `gpt-5.5`, Claude Code `claude-sonnet-5`). + */ +export function modelRouteKey(option: ModelPickerOption): string | null { + if (option.routeProviderId !== undefined) { + return option.routeProviderId; + } + const separatorIndex = option.value.indexOf("/"); + return separatorIndex > 0 ? option.value.slice(0, separatorIndex) : null; +} + +/** + * Human-readable names for route keys the picker knows about. omp exposes no + * provider names anywhere (models --json, the TUI, the binary's strings, the + * ACP schema), so bb owns this mapping. Group identity stays the raw key — + * only display goes through `routeProviderDisplayName`. + */ +export const ROUTE_PROVIDER_DISPLAY_NAMES: ReadonlyMap = + new Map([ + ["cursor", "Cursor"], + ["openai", "OpenAI"], + ["zai", "Z.ai"], + ["mistral", "Mistral"], + ["google-antigravity", "Google Antigravity"], + ["xai-oauth", "xAI"], + ["alibaba-token-plan", "Alibaba"], + ["opencode-zen", "OpenCode Zen"], + ["openai-codex", "OpenAI Codex"], + ["commandcode", "CommandCode"], + ]); + +/** + * The display name for a route key: the known mapping when present, else + * hyphens become spaces and each word is capitalized (`new-route-x` → + * `New Route X`). + */ +export function routeProviderDisplayName(key: string): string { + const known = ROUTE_PROVIDER_DISPLAY_NAMES.get(key); + if (known !== undefined) { + return known; + } + return key + .split("-") + .filter((word) => word.length > 0) + .map((word) => word.charAt(0).toUpperCase() + word.slice(1)) + .join(" "); +} + +/** + * Groups a model list by route key. Keyed groups appear in first-appearance + * order and collect every option that shares their key; the null-key group + * (models with no derivable route) leads and renders headerless, so stray + * unparseable ids never scatter singleton rows through the list. + */ +export function groupModelOptions( + options: readonly ModelPickerOption[], +): ModelOptionGroup[] { + const keyedGroups: ModelOptionGroup[] = []; + const groupsByKey = new Map(); + const ungrouped: ModelPickerOption[] = []; + + for (const option of options) { + const key = modelRouteKey(option); + if (key === null) { + ungrouped.push(option); + continue; + } + let group = groupsByKey.get(key); + if (group === undefined) { + group = { key, options: [] }; + groupsByKey.set(key, group); + keyedGroups.push(group); + } + group.options.push(option); + } + + return ungrouped.length > 0 + ? [{ key: null, options: ungrouped }, ...keyedGroups] + : keyedGroups; +} + +/** + * Group headers pay their way only when the list spans ≥2 route providers: a + * lone provider's header would repeat context the trigger already shows. + * Single-route lists (Codex, Claude Code) therefore render flat. + */ +export function hasMultipleRouteGroups( + groups: readonly ModelOptionGroup[], +): boolean { + let keyedGroupCount = 0; + for (const group of groups) { + if (group.key !== null) { + keyedGroupCount += 1; + } + } + return keyedGroupCount >= 2; +} + +// Collision checks compare the label a row actually renders — the +// brand-stripped label, trailing tag included, so `Opus 4.8 (1M)` and +// `Opus 4.8` stay distinct. Two rows the user cannot tell apart are +// precisely the rows that need a qualifier. + +/** + * The distinguishing token for a model id inside its route group: the value + * minus its `/` prefix (nested ids keep the rest of their path — + * `commandcode/zai-org/GLM-5` → `zai-org/GLM-5`), or the whole value when it + * carries no such prefix. + */ +function routeRemainder(option: ModelPickerOption, key: string | null): string { + return key !== null && option.value.startsWith(`${key}/`) + ? option.value.slice(key.length + 1) + : option.value; +} + +/** + * Inline qualifiers for rows whose rendered label collides with another row + * *within the same route group* — the header disambiguates cross-group + * collisions, so those rows stay clean. A colliding row's qualifier is the + * value remainder (see `routeRemainder`), unless that remainder merely + * restates the label (see the degenerate check inside). Returns only the + * colliding values; an absent entry means "no qualifier". + */ +export function qualifyCollidingLabels( + options: readonly ModelPickerOption[], + brandPrefix?: string, +): ReadonlyMap { + const qualifiers = new Map(); + for (const group of groupModelOptions(options)) { + const labels = group.options.map((option) => + stripModelBrandPrefix(option.label, brandPrefix), + ); + group.options.forEach((option, index) => { + const collides = labels.some( + (label, otherIndex) => otherIndex !== index && label === labels[index], + ); + if (!collides) { + return; + } + // A remainder that merely restates the rendered label (aliases whose + // id tail equals their name — `mistral/mistral-medium-latest`) adds no + // information the row does not already show. Skip it; the raw-value + // tooltip still reveals the full id on hover. + const remainder = routeRemainder(option, group.key); + if (remainder.toLowerCase() === labels[index].toLowerCase()) { + return; + } + qualifiers.set(option.value, remainder); + }); + } + return qualifiers; +} + +/** + * The muted suffix the trigger shows when the selected model's rendered label + * is ambiguous across the whole active list — another model renders the same + * label. The trigger stands alone (no group headers around it), so the suffix + * must be self-sufficient: a cross-group collision surfaces the provider's + * display name (`Z.ai`); a within-group collision surfaces the display name + * plus the value remainder, which is what separates the rows under that + * header (`Mistral · mistral-medium-2506`). A remainder that merely restates + * the rendered label adds nothing and is dropped, falling back to the + * cross-group name when one exists. Null when the label is unambiguous + * (trigger unchanged). + */ +export function selectedModelQualifier( + options: readonly ModelPickerOption[], + value: string, + brandPrefix?: string, +): string | null { + const selected = options.find((option) => option.value === value); + if (selected === undefined) { + return null; + } + const label = stripModelBrandPrefix(selected.label, brandPrefix); + const key = modelRouteKey(selected); + let sameGroupCollision = false; + let crossGroupCollision = false; + for (const other of options) { + if (other.value === value) { + continue; + } + if (stripModelBrandPrefix(other.label, brandPrefix) !== label) { + continue; + } + if (modelRouteKey(other) === key) { + sameGroupCollision = true; + } else { + crossGroupCollision = true; + } + } + if (sameGroupCollision) { + const remainder = routeRemainder(selected, key); + if (remainder.toLowerCase() !== label.toLowerCase()) { + return key !== null + ? `${routeProviderDisplayName(key)} · ${remainder}` + : remainder; + } + } + return crossGroupCollision && key !== null + ? routeProviderDisplayName(key) + : null; +} diff --git a/apps/mobile/src/data/compose/execution-options.test.ts b/apps/mobile/src/data/compose/execution-options.test.ts index 61fe2f9456..75da7f5432 100644 --- a/apps/mobile/src/data/compose/execution-options.test.ts +++ b/apps/mobile/src/data/compose/execution-options.test.ts @@ -126,6 +126,28 @@ describe("resolveModelSelection", () => { expect(resolved.isRecovery).toBe(true); expect(resolved.options[0].routeProviderId).toBe("openrouter"); }); + + it("preserves the description so ACP route models show their provider in the subtitle", () => { + const acp = response({ + models: [ + model({ + model: "zai/glm-5.3", + displayName: "GLM 5.3", + description: "zai/glm-5.3", + }), + ], + }); + const resolved = resolveModelSelection({ + executionOptions: acp, + selectedModel: "zai/glm-5.3", + catalogVerified: true, + }); + expect(resolved.options[0]).toMatchObject({ + value: "zai/glm-5.3", + label: "GLM 5.3", + description: "zai/glm-5.3", + }); + }); }); describe("reasoning", () => { diff --git a/packages/provider-bridge-acp/src/bridge/model-catalog.test.ts b/packages/provider-bridge-acp/src/bridge/model-catalog.test.ts index ca7766f187..ce053647ed 100644 --- a/packages/provider-bridge-acp/src/bridge/model-catalog.test.ts +++ b/packages/provider-bridge-acp/src/bridge/model-catalog.test.ts @@ -461,6 +461,38 @@ describe("acp configOptions model catalog", () => { ]); }); + it("preserves select-option descriptions, defaulting missing ones to empty", () => { + const models = buildModelCatalogFromConfigOptions({ + id: "model", + name: "Model", + category: "model", + type: "select", + currentValue: "zai/glm-5.3", + options: [ + // omp sends description == value ("provider/modelId") so duplicate + // display names stay distinguishable by route provider. + { + value: "zai/glm-5.3", + name: "GLM 5.3", + description: "zai/glm-5.3", + }, + { value: "cursor/glm-5.3", name: "GLM 5.3" }, + // An explicit null normalizes to undefined at the wire schema. + { + value: "mistral/mistral-medium-latest", + name: "Mistral Medium", + description: undefined, + }, + ], + }); + + expect(models.map((model) => model.description)).toEqual([ + "zai/glm-5.3", + "", + "", + ]); + }); + it("finds and maps ACP thought_level config options", () => { const thoughtLevel = { id: "effort", diff --git a/packages/provider-bridge-acp/src/bridge/model-catalog.ts b/packages/provider-bridge-acp/src/bridge/model-catalog.ts index 565f530f20..43cc83b1e0 100644 --- a/packages/provider-bridge-acp/src/bridge/model-catalog.ts +++ b/packages/provider-bridge-acp/src/bridge/model-catalog.ts @@ -276,7 +276,7 @@ export function buildModelCatalogFromConfigOptions( id: option.value, model: option.value, displayName: option.name ?? option.value, - description: "", + description: option.description ?? "", supportedReasoningEfforts: reasoning.supportedReasoningEfforts, defaultReasoningEffort: reasoning.defaultReasoningEffort, isDefault, diff --git a/packages/provider-bridge-acp/src/wire.test.ts b/packages/provider-bridge-acp/src/wire.test.ts index 47122d132b..a0c56b630d 100644 --- a/packages/provider-bridge-acp/src/wire.test.ts +++ b/packages/provider-bridge-acp/src/wire.test.ts @@ -131,6 +131,11 @@ describe("acpSessionNewResultSchema", () => { name: "openai-codex/GPT-5.5", description: null, }, + { + value: "openai-codex/gpt-5.5-codex-max", + name: "openai-codex/GPT-5.5 Codex Max", + description: "openai-codex/gpt-5.5-codex-max", + }, ], }, { @@ -154,6 +159,12 @@ describe("acpSessionNewResultSchema", () => { expect(parsed.data.configOptions?.[0].options?.[0].name).toBe( "openai-codex/GPT-5.5", ); + expect( + parsed.data.configOptions?.[0].options?.[0].description, + ).toBeUndefined(); + expect(parsed.data.configOptions?.[0].options?.[1].description).toBe( + "openai-codex/gpt-5.5-codex-max", + ); expect(parsed.data.configOptions?.[1].category).toBeUndefined(); expect(parsed.data.configOptions?.[1].options?.[0].name).toBeUndefined(); }); diff --git a/packages/provider-bridge-acp/src/wire.ts b/packages/provider-bridge-acp/src/wire.ts index 2f05422dba..980440b2a1 100644 --- a/packages/provider-bridge-acp/src/wire.ts +++ b/packages/provider-bridge-acp/src/wire.ts @@ -323,6 +323,7 @@ const acpConfigOptionSelectOptionSchema = z .object({ value: z.string(), name: acpOptionalString, + description: acpOptionalString, }) .passthrough();