Group model picker by provider for ACP route models - #2465
Open
bradhallett wants to merge 1 commit into
Open
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Human comments
What was wrong
buildModelCatalogFromConfigOptionshard-codeddescription: ""for every ACP model select option, andacpConfigOptionSelectOptionSchemadid not typedescription(it only survived via.passthrough()). Agents such as omp senddescription: "provider/modelId"and advertise the same display name under several routes (on this machine: 279 models across 9 providers, 37 duplicate display names). The desktop picker rendered only the display name, so those rows were identical in text, tooltip, and accessible name. Issue: #2062. Closed PR #2226 had the bridge fix plus inline qualifiers; this redo keeps the bridge hunks and replaces the desktop UX with provider-grouped headers.What changed
Bridge (
packages/provider-bridge-acp; the formerplugins/provider-acp/src/bridgepath is now a thin declaration layer):src/wire.ts:description: acpOptionalStringon the select-option schema.src/bridge/model-catalog.ts:description: option.description ?? "", mirroringbuildModelCatalogFromSessionModels.HOST_DAEMON_PROTOCOL_VERSIONbump:AvailableModel.descriptionis already a required string.Desktop (
apps/app):routeProviderId, else the first segment of a slashed id). StickyMenuSectionLabelheaders only when ≥2 routes are visible. Search that narrows to one route hides the headers (same rule).provider/). The trigger shows a distinguishing token only when the committed label is ambiguous acrossmodelOptions+moreModelOptions.span[title]with the full raw id; keyboard order matches DOM order (contiguous route runs, not merged).zai→ Z.ai,xai-oauth→ xAI, …) with a hyphen→title-case fallback. omp 18.0.5 does not expose provider display names onomp models --json; group keys and tooltips stay on raw ids.Mobile: no source change.
execution-options.test.tsasserts the ACP description survivesresolveModelSelection→modelToOptionso the existing subtitle path lights up from the bridge fix.Deviation from closed #2226: grouping + headers instead of always-on inline qualifiers. F6 (Home/End) and F10 (header weight) from the design critique were skipped; no sibling picker supports Home/End.
How you verified
Tests (fail before, pass after):
packages/provider-bridge-acp/src/bridge/model-catalog.test.ts— catalog keeps the per-option description.packages/provider-bridge-acp/src/wire.test.ts— schema returnsdescriptionand normalizesnull→undefined.apps/app/src/components/pickers/model-picker-option.test.ts— 25 helper tests: grouping, ≥2-route header gate, within-group qualifiers, trigger token, display-name map.apps/app/src/components/pickers/ModelReasoningPicker.test.tsx— grouped headers,role="group"/aria-labelledby, raw-id tooltips, single-route lists stay flat, sticky offsets, unique-trigger shape.apps/mobile/src/data/compose/execution-options.test.ts— ACP description survives into the mobile option subtitle.Mutation-kills (reverted after):
hasMultipleRouteGroups→return falsefails the grouped-catalog test;titlealwaysundefinedfailsgetByTitle("zai/glm-5.3");qualifyCollidingLabelsearlyreturn new Map()fails the within-provider qualifier test.Commands (after rebase onto
31d66d9, thenpnpm install --frozen-lockfile):pnpm exec turbo run typecheck lint test --filter=@bb/provider-bridge-acp --filter=@bb/app --filter=@bb/mobile— 11/11.@bb/app3445 passed (4 skipped),@bb/provider-bridge-acp289 passed.Manual: Ladle
pickers--model-reasoning-picker--overview, "open: acp grouped" — pretty headers (Cursor / Z.ai / CommandCode / Mistral), collision tails, full-id tooltips,role=group.Fixes #2062