fix(model-combobox): stop filter flash on close - #536
Conversation
📋 PR Overview
🔬 Coverage
|
🎨 Storybook previewUpdated for c45acc8 🧭 Story changesCompared to ✏️ Changed stories (5)
|
![darkmatter[bot]](https://avatars.githubusercontent.com/in/3663660?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean fix. Decoupling search (popover-internal filter text) from value (external controlled value) is the right call — handleSelect no longer touches the filter state, so the list doesn't re-filter during the close animation.
Verified the key scenarios hold:
- Re-sync on open:
handleOpenChange(true)→setSearch(value)ensures the search field starts from the current value each time the popover opens. - Trigger display: Reading
valuedirectly eliminates theuseEffectsync and its one-render delay — strictly better. - Custom option / Check icon: Both correctly switched to
search; sincehandleInputChangecalls bothsetSearchandonChange,value === searchstays valid during typing. - External
valuechange while closed: Trigger updates immediately;searchsyncs on next open. No stale UI visible. - External
valuechange while open:searchisn't clobbered mid-typing — better than the olduseEffectapproach.
No correctness, security, or quality concerns. Ship it.
|
Review posted on PR #536. Verdict: Approved. The fix cleanly decouples the popover-internal |
Juanpe Bolívar (arximboldi)
left a comment
Juanpe Bolívar (arximboldi)
left a comment
There was a problem hiding this comment.
Actually... this is something I wanted to change but in a different direction, it is tracked here: https://linear.app/darkmatterlabs/issue/ENG-567/empty-list-when-chosing-a-model-with-the-drop-down-a-second-time
In my opinion, the "search" string should not change when you choose a model. the current behavior means that if I want to choose a different model, I need to clear the search string every time, otherwise I only see the currently selected model as option.
The search string should probably only change when the user writes something to it. If anything, when selecting a model, perhaps it should be cleared (but no strong opinion here, an argument could be made to just keep it until the user types something else).
|
hmm i dont really have an opinion here and will defer to Juanpe Bolívar (@arximboldi) - in this scenario I usually will go look at shadcn or elements on my iphone since they iterate on this kind of stufff a lot. in this case they do something interesting though, they reset the input on blur unless you had something selected. so the rule is to restore your previous selection, which is empty the first time https://ui.shadcn.com/docs/components/base/combobox |
ba5bf05 to
eaf2dbf
Compare
Juanpe Bolívar (arximboldi)
left a comment
Juanpe Bolívar (arximboldi)
left a comment
There was a problem hiding this comment.
Much better, thank you! I also love that the "default" option is now an explicit item in the list, a missing thing that was bothering me already a little bit...
About the code: perhaps the core component could be extracted into a combobox component that is not "AI model" related. Since the code was already like this, I don't think you are required to merge this and we can have this as a follow up if necessary.
|
Juanpe Bolívar (@arximboldi) hah i actually gave the same suggestion via slack |
|
attempt: f9e8c1b #1 proposed — unused import AI_MODEL_PROVIDERS; remove it from ai-models-tab.tsx Diagnosis
Proposed fix (no push — dry run)File:
If this is attempt #2The fixer stops retrying this SHA after attempt #2. A human should apply the one-line removal above (or run — sent by Claude (ci_fixer) |
|
👀 CI is failing here (TypeScript, build) — on it. I'll post a diagnosis with a proposed fix; progress lands in the ci-fixer ledger comment. — sent by Claude (ci_fixer) |
Match Base UI combobox semantics: the field itself is the cmdk input (no search row in the popup), opening shows the unfiltered list with the selected model checked and highlighted, and typing only filters — values commit on selection. An explicit "Default: <model>" row commits the empty string, which means "use the provider default"; consumers pass the default model name. - keep the model list across opens (clear only on provider change): async item remount mid-click made cmdk re-highlight and re-focus, which WebKit answers by collapsing the caret to the start - freeze the filter query on select so the closing popover does not re-filter mid-animation - control cmdk highlight so opening highlights the current selection and Enter cannot commit a different model than the chosen one - restore native Home/End/arrow editing while the popover is closed; ArrowUp/Down open the list - cover the interaction matrix with component tests
Move the input-anchored combobox (search/query split, controlled highlight, key handling) to packages/ui as a reusable control; ModelCombobox keeps only provider model loading and maps its defaults onto the generic props. Generic interaction tests move to packages/ui; the unit project now includes packages/ui tests, which resurrected ui.test.tsx — its snapshots were stale and regenerated.
f9e8c1b to
c45acc8
Compare
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
3 participants
Summary
without.fix.mov
with.fix.mov
Test Plan
Docs