feat(ENG-1096): model picker with search, provider grouping, and icons - #512
feat(ENG-1096): model picker with search, provider grouping, and icons#512hazemahmedx0 wants to merge 4 commits into
Conversation
- New ModelSelect (Base UI Combobox): search input pinned in the popup, models grouped under provider headers, checkmark on the current model, keyboard nav, and a No models found empty state - New ProviderIcon: monochrome currentColor marks (MindsHub, OpenAI, Anthropic, Google, xAI, Moonshot, Alibaba, DeepSeek, Mistral, Meta), neutral placeholder for makers without an svg yet (ENG-1112) - New lib/modelCatalog: maker inferred from alias + label until the backend ships an explicit maker field (ENG-1111); unknowns land in Other instead of a wrong group - Settings Agent Models rows and the composer model pill now share the same picker; the composer's hand-rolled model menu is gone - buildModelOptions tags the stale-pin and Other entries with pin so they render unheaded above/below the provider groups Refs ENG-1096
hazemahmedx0
left a comment
There was a problem hiding this comment.
Reviewed at b16af30f402735ca1d8ec67cf0387cce1068406b.
Good change. The split is right: Base UI owns filtering, keyboard nav and focus, modelCatalog owns the maker guess, and the component is just a skin over the existing Select trigger tokens. No new dependency, @base-ui/react 1.5.0 already ships combobox. I ran the four affected test files locally, 73 pass.
Verdict: approved. Everything below is non-blocking.
What I checked and it holds up
Re-selecting the model that is already selected. This was my main worry, because onValueChange={(item) => onValueChange?.(item ? item.value : '')} would write an empty model id if Base UI ever deselected on re-click. It does not. In single-select mode handleSelection always does setSelectedValue(itemValue), the deselect branch is multiple-select only. So the empty-string path is unreachable from a click and Settings cannot wipe planning_model by clicking the current row.
The Settings behaviours all still line up. resolveModelPickerValue hands back __stale__ or __custom__, buildModelOptions tags both with pin, items pulls pinned entries out of the maker groups, and makerKeyFor short-circuits pinned entries to the neutral mark so no provider gets guessed for "Other…" or the legacy row. Locked models keep disabled through to Combobox.Item and clicking one fires nothing (covered at ModelSelect.test.jsx:61). onOpenChange has the same (open) shape Select was giving it, so the wallet recheck and the TTL guard are untouched.
A value that is not in options still renders on the trigger via the synthesized entry, and isItemEqualToValue compares on .value, so the fresh object identity on every render does not cause spurious change events.
Accessibility is in decent shape. Combobox.Empty already renders role="status" aria-live="polite" aria-atomic, so "No models found" gets announced. The search input has a real label. Group labels are wired by Base UI.
modelCatalog is the part I would have expected to be flaky and it is not, mostly because the test drives the actual live catalog as a table.
Suggestions
-
Search filters away "Other…". The
filtercallback runs over pinned entries too, so on a BYOK provider the natural flow for a custom model is: open the picker, type the model id, get "No models found", and the only route to the free-text field is now hidden until you clear the search. Consider returningtruefromfilterfor anything carryingpin, or naming "Other…" in the empty state. Worth noting the current test at ModelSelect.test.jsx:95 locks the present behaviour in, since__custom__is inOPTIONSand the assertion is zero options. -
The composer call site has no coverage.
Composer.test.jsxsetshideModel: true, soModelSelectnever renders in that suite, and all five screenshots are Settings. The composer is also the only path that usesvariant="unstyled", which skipstriggerVariantsentirely. One render test that opens the pill and assertsonModelChangereceived the full model object rather than the id string would cover the contract, and a composer screenshot would cover the skin. -
modelshas no default in Composer.models.map(...)at Composer.jsx:1249 now runs on every render of the editable meta row, where before it was inside{openMenu === 'model' && ...}and only ran with the menu open. Every caller today passes an array, so nothing is broken, butmodels = []in the destructure is free. -
Stale comment in ProviderIcon.jsx. The header lists google as having no mark, but
MARKS.googleis right there. It also namesmuseas a maker, andmuseis not a key inMAKERSat all, it lands inother, which is also markless, so the placeholder is correct for the wrong reason. The PR description has this right, the file comment is the one to fix. -
Dead branch in the OpenAI pattern.
(^| )o\d(\b|-)cannot match on the-alternative, becausemodelMakerreplaces[_\-./]with spaces before testing. Harmless, just misleading to read.
… tidy - Pinned entries bypass the search filter so the Other… free-text escape hatch never disappears behind a no-match search - Composer model picker gets direct test coverage (pill label, grouped popup, full-model-object onModelChange contract) and a safe models default - ProviderIcon header comment matches reality (google mark shipped) - Drop a dead branch from the OpenAI o-series regex Refs ENG-1096
- Composer.jsx and its tests go back to the staging state; the new ModelSelect stays a reusable ui primitive but only the Settings Agent Models rows adopt it in this PR Refs ENG-1096
hazemahmedx0
left a comment
There was a problem hiding this comment.
Re-reviewed at 9c947f8a00a954dd15d3606d7524355f7cda9cbc. Follow-ups look good: pinned entries bypassing the filter is the right fix and the no-results test was correctly re-scoped to pin-free options, \bo\d\b is equivalent after the separator normalisation (and cleaner), and the ProviderIcon comment now matches the marks that are actually there.
Confirmed Composer.jsx and Composer.test.jsx are byte-identical to staging, so the PR is Settings only with ModelSelect left as a reusable primitive. 74 tests pass locally on the affected files. Still approved.
pnewsam
left a comment
There was a problem hiding this comment.
Two non-blocking architectural concerns to consider. The picker behavior itself looks sound, and I confirmed that model availability remains backend-driven; the questions are about the reusable UI boundary and keeping maker metadata backend-extensible.
…f ui/ - ModelSelect is now a thin domain component under components/, composing a new generic ui/Combobox that owns the Base UI wiring and styling - ProviderIcon moves from components/ui/ to components/ - it carries provider knowledge, not generic styling - groupModelOptions gives an unknown backend maker its own group (named by makerName or a capitalised key, placeholder icon) instead of lumping it under Other, so a new maker never needs a frontend release once ENG-1111 ships Refs ENG-1096
Rebuilds the Settings model picker so you can find a model quickly instead of scrolling a flat list. Linear: ENG-1096
Summary
ui/Combobox+ domaincomponents/ModelSelect(Base UI Combobox, "input inside popup" shape). Closed state shows the provider mark, the model name, and a caret. Open state pins a search box above a grouped, scrollable list with a checkmark on the current model. Base UI owns filtering, keyboard nav (arrows, Enter, Esc, typing lands in the search box), and focus, so we only ship the skin. The wiring and styling live in a genericui/Combobox(no model knowledge);ModelSelectcomposes it with the maker grouping, the pin-aware filter, and the provider mark on the trigger. It reuses theSelecttrigger styling so the two controls look identical when closed.ModelSelectkeeps aSelect-shaped API, so other surfaces can adopt it later with a one-line swap; nothing else changes in this PR.lib/modelCatalog): the maker comes from the alias and label, and unknown models land in an "Other" bucket instead of a wrong group. The backend has no maker field yet (ENG-1111 asks for one), and options that carry an explicitmakerare trusted verbatim, so the guessing self-retires when it ships. Amakerthe frontend doesn't recognize gets its own group (named by an optionalmakerName, placeholder icon) instead of falling into "Other", so new backend makers won't need a frontend release.ProviderIcon(components/, since it carries provider knowledge): monochrome marks rendered withcurrentColorso they follow the text color in both themes. MindsHub, OpenAI, Anthropic, Google, xAI, Moonshot, Alibaba, DeepSeek, Mistral, and Meta are covered; makers without an svg yet fall back to a neutral placeholder instead of a broken image (ENG-1112).pintag onbuildModelOptions.Screenshots
Light, open with groups and checkmark:
Typing "opus" filters every group and keeps the matching header:
No results:
Dark mode:
Testing
npm test(622 tests, including 27 new formodelCatalogand 12 forModelSelect),npm run typecheck,npm run check:cowork-purity, and the renderer build all pass.The optional hover detail card from the design is out of scope here; it needs a per-model metadata source first (see the ticket).
Glossary
sonnet,gpt-codex). The picker groups and searches on both the alias and its display label.latest:prefix). Shown as a disabled "re-select" row so picking a real model always fires a change.Refs ENG-1096