feat(tui): remember recent provider+model selections in /model picker - #568
Merged
kevincodex1 merged 6 commits intoJul 8, 2026
Merged
Conversation
The /model picker's "Recent" section only ever showed the currently active model, making it hard to switch back to a previously used provider+model pair once you've moved on to something else (especially when the same model name exists across multiple providers). Track a short automatic history of provider-qualified switches instead: newest first, capped at 5, deduped by provider+model pair (not model name alone), and selectable across providers like any other picker row. Closes Gitlawb#562 Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds provider-qualified recent model selection history to the /model picker, persisting a capped newest-first list to user config to make switching between provider+model pairs faster and avoid cross-provider model-id collisions.
Changes:
- Track and persist
preferences.recentModelsas{provider, model}pairs (newest-first, deduped, capped toconfig.MaxRecentModels). - Update the TUI model picker to render a real “Recent” history (active pinned first) and de-dup picker rows by provider+model (not model id alone).
- Add config + TUI tests covering normalization, persistence, and picker behavior.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/tui/picker.go | Builds Recent from provider+model history, adds provider-aware picker de-dup, and records/persists recent selections. |
| internal/tui/picker_test.go | Adds tests for recent-pair pinning/dedup/cap and provider-aware picker de-dup + persistence. |
| internal/tui/options.go | Threads resolved recent history into TUI options. |
| internal/tui/model.go | Stores/normalizes recentModels in TUI state. |
| internal/tui/command_center.go | Records outgoing+incoming provider/model pairs on successful switches. |
| internal/config/writer.go | Persists preferences.recentModels with normalization (order-preserving, dedup, cap). |
| internal/config/writer_test.go | Tests SetRecentModels ordering, dedup-by-pair, and cap behavior. |
| internal/config/types.go | Defines RecentModelEntry and MaxRecentModels, adds Preferences field. |
| internal/config/resolver.go | Normalizes recent models when merging user/command config. |
| internal/config/resolver_test.go | Ensures recentModels load from user config only (not project config). |
| internal/cli/app.go | Passes resolved recentModels into the interactive TUI options. |
| .gitignore | Ignores local Go cache directories used for local runs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
441
to
457
| func (m model) assembleModelPickerItems(recent []pickerItem, catalog []pickerItem) []pickerItem { | ||
| result := []pickerItem{} | ||
| seen := map[string]bool{} | ||
| all := append(append([]pickerItem{}, recent...), catalog...) | ||
| for _, item := range all { | ||
| if item.Value == "" || !m.favoriteModels[item.Value] || seen[item.Value] { | ||
| if item.Value == "" || !m.favoriteModels[item.Value] { | ||
| continue | ||
| } | ||
| key := pickerItemDedupKey(item) | ||
| if seen[key] { | ||
| continue | ||
| } | ||
| item.Group = "Favorites" | ||
| item.Favorite = true | ||
| result = append(result, item) | ||
| seen[item.Value] = true | ||
| seen[key] = true | ||
| } |
Comment on lines
458
to
470
| for _, item := range recent { | ||
| if item.Value == "" || seen[item.Value] { | ||
| if item.Value == "" { | ||
| continue | ||
| } | ||
| key := pickerItemDedupKey(item) | ||
| if seen[key] { | ||
| continue | ||
| } | ||
| item.Group = "Recent" | ||
| item.Favorite = m.favoriteModels[item.Value] | ||
| result = append(result, item) | ||
| seen[item.Value] = true | ||
| seen[key] = true | ||
| } |
Comment on lines
471
to
482
| for _, item := range catalog { | ||
| if item.Value == "" || seen[item.Value] { | ||
| if item.Value == "" { | ||
| continue | ||
| } | ||
| key := pickerItemDedupKey(item) | ||
| if seen[key] { | ||
| continue | ||
| } | ||
| item.Favorite = m.favoriteModels[item.Value] | ||
| result = append(result, item) | ||
| seen[item.Value] = true | ||
| seen[key] = true | ||
| } |
Comment on lines
+883
to
+886
| // or picker), so the "Recent" section reflects real switching history even | ||
| // across sessions. A no-op when modelID is blank or there is no user config | ||
| // path to persist to (in-memory history alone would not survive restart, and | ||
| // silently diverging from the persisted list would be confusing). |
WalkthroughAdds persisted provider-qualified recent model history, wires it into TUI startup, records ChangesRecent Models Feature
Estimated code review effort: 4 (Complex) | ~45 minutes Related issues: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant User
participant CommandCenter
participant TUIModel
participant ConfigWriter
participant ModelPicker
User->>CommandCenter: /model selection
CommandCenter->>TUIModel: recordRecentModels(old, new)
TUIModel->>ConfigWriter: SetRecentModels(user config, entries)
ConfigWriter->>ConfigWriter: NormalizeRecentModels(entries)
ConfigWriter-->>TUIModel: updated config
TUIModel->>ModelPicker: refresh recent rows
ModelPicker-->>User: provider-qualified Recent entries
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
internal/tui/picker_test.go (2)
680-688: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDon't silently discard the error from
switchProviderModel.
next, _, _ := m.switchProviderModel(...)ignores the (presumed) error return. If the switch itself fails, the test degrades to a confusingrecentModelsmismatch instead of a clear failure pointing at the actual root cause.✅ Suggested fix
- next, _, _ := m.switchProviderModel("ollama", "kimi-k2.7-code:cloud") + next, _, err := m.switchProviderModel("ollama", "kimi-k2.7-code:cloud") + if err != nil { + t.Fatalf("switchProviderModel() error = %v", err) + }Please confirm the actual signature of
switchProviderModelininternal/tui/picker.go(not included in this review's context) to adjust the fix if the discarded value isn't an error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/picker_test.go` around lines 680 - 688, The test is ignoring the return value from switchProviderModel, which can hide a real failure behind a later recentModels assertion. Update the picker_test.go case to capture and assert the actual return from m.switchProviderModel("ollama", "kimi-k2.7-code:cloud"), using the switchProviderModel symbol in internal/tui/picker.go to confirm whether the discarded value is an error or another result, and fail immediately if the switch operation does not succeed.
646-659: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winVerify persistence for the reorder/dedupe edge case too.
After the 3rd switch (re-selecting the oldest pair — the trickiest dedupe+reorder path), only
m.recentModelsis asserted; the persistedconfig.jsonisn't re-checked like it was after the 2nd switch (Lines 641-644). If in-memoryrecordRecentModelreordering ever diverges fromSetRecentModels/normalizeRecentModelsdisk-write semantics, this test would pass despite a real persistence bug on exactly the case most likely to expose it.✅ Suggested addition
if !reflect.DeepEqual(m.recentModels, want) { t.Fatalf("recentModels after re-selecting = %#v, want %#v", m.recentModels, want) } + persisted = readTUIConfigFixture(t, configPath) + if !reflect.DeepEqual(persisted.Preferences.RecentModels, want) { + t.Fatalf("persisted RecentModels after re-selecting = %#v, want %#v", persisted.Preferences.RecentModels, want) + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/picker_test.go` around lines 646 - 659, The reorder/dedupe scenario in picker_test.go only checks the in-memory recentModels after re-selecting the oldest pair, so it can miss a persistence mismatch. Update the test around the model Update flow for this third switch to also read back the persisted config state, using the same config helpers already used after the earlier switch, and assert the saved recent models match the expected reordered list. Keep the check aligned with recordRecentModel, SetRecentModels, and normalizeRecentModels so both memory and disk semantics are verified for this edge case.internal/config/resolver_test.go (1)
130-169: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for the "project sets recentModels, user doesn't" case.
The new test only validates that user config wins when both layers set
recentModels. Given the PR objective that recents must never be merged from project config, it's worth adding a case where only the project config setspreferences.recentModelsand the user config omits it entirely, assertingresolved.Preferences.RecentModelsstays empty. This would also pin down the merge-order concern raised onresolver.go(Line 226-228).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/config/resolver_test.go` around lines 130 - 169, Add a resolver test covering the case where only the project config defines preferences.recentModels and the user config omits it, and assert Resolve leaves resolved.Preferences.RecentModels empty. Extend the existing coverage in TestResolveLoadsRecentModelsFromUserConfigOnly or add a sibling test in resolver_test.go to verify Resolve/Preferences merge behavior ignores project recentModels when user config has none.internal/tui/command_center.go (1)
450-456: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winTwo sequential
recordRecentModelcalls persist to disk twice per switch.Both
handleModelCommandandswitchProviderModelrecord the outgoing pair, then the incoming pair, andrecordRecentModeldoes a full read-modify-write touserConfigPathon each call (internal/tui/picker.goLines 897-901). That's two synchronous disk writes for one logical "switch model" action, and if the first write fails, a spurious "recent model save error" transcript line gets appended before the second (successful) write silently supersedes it — confusing for a single user action.Consider batching both pairs into one
recordRecentModel(pairs ...config.RecentModelEntry)-style call that builds the combined entry list once and persists once.Also applies to: 536-540
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/command_center.go` around lines 450 - 456, The model switch path is persisting recent models twice for one user action because both handleModelCommand and switchProviderModel call recordRecentModel separately for the previous and next pairs. Update the recent-model flow around recordRecentModel so it accepts both pairs together (or otherwise batches them) and performs a single read-modify-write to userConfigPath, using the existing identifiers recordRecentModel, handleModelCommand, and switchProviderModel to locate the affected logic.internal/tui/picker.go (1)
905-930: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
normalizeRecentModelEntriesduplicatesconfig.normalizeRecentModelslogic.Both implement identical trim/dedupe(provider-lowercase +
\x00+ model)/cap-to-MaxRecentModelssemantics (seeinternal/config/writer.go'snormalizeRecentModels). The comment at Line 907-909 explicitly acknowledges this is a deliberate mirror "so options loaded outside of config.Resolve... get the same guarantees" — but two independently-maintained copies of the same normalization rule will silently drift if one is changed without the other (e.g. a future tweak to the dedupe key or cap logic).Consider exporting
config.NormalizeRecentModelsand calling it here instead of re-implementing it, guaranteeing the TUI and persisted-config views can never diverge.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/picker.go` around lines 905 - 930, The `normalizeRecentModelEntries` helper is duplicating the same recent-model normalization logic already implemented in `config.normalizeRecentModels`, which risks the TUI and config paths drifting apart. Replace the local trim/dedupe/cap behavior in `normalizeRecentModelEntries` with a shared normalization function from the config package, such as an exported `config.NormalizeRecentModels`, and use that in `picker.go` so both `normalizeRecentModelEntries` and the config writer stay consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/tui/picker.go`:
- Around line 429-484: Set OwnerProvider on the rows built by
registryModelPickerItem so pickerItemDedupKey can distinguish same-named models
from different providers instead of falling back to Value alone. Update the
registry catalog item construction to populate the owning provider consistently,
and verify assembleModelPickerItems still deduplicates Favorites/Recent/Catalog
correctly using the provider+model key.
---
Nitpick comments:
In `@internal/config/resolver_test.go`:
- Around line 130-169: Add a resolver test covering the case where only the
project config defines preferences.recentModels and the user config omits it,
and assert Resolve leaves resolved.Preferences.RecentModels empty. Extend the
existing coverage in TestResolveLoadsRecentModelsFromUserConfigOnly or add a
sibling test in resolver_test.go to verify Resolve/Preferences merge behavior
ignores project recentModels when user config has none.
In `@internal/tui/command_center.go`:
- Around line 450-456: The model switch path is persisting recent models twice
for one user action because both handleModelCommand and switchProviderModel call
recordRecentModel separately for the previous and next pairs. Update the
recent-model flow around recordRecentModel so it accepts both pairs together (or
otherwise batches them) and performs a single read-modify-write to
userConfigPath, using the existing identifiers recordRecentModel,
handleModelCommand, and switchProviderModel to locate the affected logic.
In `@internal/tui/picker_test.go`:
- Around line 680-688: The test is ignoring the return value from
switchProviderModel, which can hide a real failure behind a later recentModels
assertion. Update the picker_test.go case to capture and assert the actual
return from m.switchProviderModel("ollama", "kimi-k2.7-code:cloud"), using the
switchProviderModel symbol in internal/tui/picker.go to confirm whether the
discarded value is an error or another result, and fail immediately if the
switch operation does not succeed.
- Around line 646-659: The reorder/dedupe scenario in picker_test.go only checks
the in-memory recentModels after re-selecting the oldest pair, so it can miss a
persistence mismatch. Update the test around the model Update flow for this
third switch to also read back the persisted config state, using the same config
helpers already used after the earlier switch, and assert the saved recent
models match the expected reordered list. Keep the check aligned with
recordRecentModel, SetRecentModels, and normalizeRecentModels so both memory and
disk semantics are verified for this edge case.
In `@internal/tui/picker.go`:
- Around line 905-930: The `normalizeRecentModelEntries` helper is duplicating
the same recent-model normalization logic already implemented in
`config.normalizeRecentModels`, which risks the TUI and config paths drifting
apart. Replace the local trim/dedupe/cap behavior in
`normalizeRecentModelEntries` with a shared normalization function from the
config package, such as an exported `config.NormalizeRecentModels`, and use that
in `picker.go` so both `normalizeRecentModelEntries` and the config writer stay
consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 303ae06b-ccf2-4fad-9efe-9dcbabf9ca64
📒 Files selected for processing (12)
.gitignoreinternal/cli/app.gointernal/config/resolver.gointernal/config/resolver_test.gointernal/config/types.gointernal/config/writer.gointernal/config/writer_test.gointernal/tui/command_center.gointernal/tui/model.gointernal/tui/options.gointernal/tui/picker.gointernal/tui/picker_test.go
Favorites de-dup had switched to the provider+model key, letting the same favorited model ID surface once per provider. Revert to per-Value de-dup for Favorites, and skip any model ID already shown under Favorites from Recent and Catalog so favorited models don't reappear in a second group. Recent and Catalog still de-dup among themselves by provider+model so cross-provider rows remain distinct. Also correct the recordRecentModel doc comment: in-memory history is updated even without a user config path (only persistence is skipped), not a no-op. Addresses Copilot review comments on Gitlawb#568.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/tui/picker_test.go (1)
566-611: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCoverage gap: surviving Favorites row's
OwnerProviderisn't asserted.Per
assembleModelPickerItems, the dedupedFavoritesrow keeps whicheverprovider-a/provider-boccurrence comes first inrecent+catalogorder — hereprovider-afromrecent[0]. Since picking a Favorites row switches provider+model per this PR's objectives, that's worth pinning down explicitly rather than leaving it implicit.Also, the
otherRecentcheck (lines 593-601) is tautological:other-modelwas never added to therecentslice in this test, so this assertion can never fail regardless of dedup correctness.💡 Suggested tightening
if len(favorites) != 1 { t.Fatalf("expected exactly one Favorites row for shared-model, got %#v", favorites) } + if favorites[0].OwnerProvider != "provider-a" { + t.Fatalf("expected surviving favorite to keep first-seen provider, got %#v", favorites[0]) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/picker_test.go` around lines 566 - 611, The test in TestAssembleModelPickerItemsFavoritesDedupByModelIDAndHideFromOtherGroups is missing an assertion for the surviving Favorites row’s OwnerProvider, so pin down that the deduped shared-model entry keeps the expected provider (the first occurrence from recent/catalog order). Also remove or replace the tautological otherRecent check since other-model is never present in recent, and instead assert the relevant grouping/dedup behavior through assembleModelPickerItems and the Favorites row selection.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/tui/picker_test.go`:
- Around line 566-611: The test in
TestAssembleModelPickerItemsFavoritesDedupByModelIDAndHideFromOtherGroups is
missing an assertion for the surviving Favorites row’s OwnerProvider, so pin
down that the deduped shared-model entry keeps the expected provider (the first
occurrence from recent/catalog order). Also remove or replace the tautological
otherRecent check since other-model is never present in recent, and instead
assert the relevant grouping/dedup behavior through assembleModelPickerItems and
the Favorites row selection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 26321a29-d10f-4981-b8a9-235c020a01e6
📒 Files selected for processing (2)
internal/tui/picker.gointernal/tui/picker_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/tui/picker.go
…odel normalization - registryModelPickerItem now tags rows with OwnerProvider (the registry entry's canonical provider), fixing the plain-registry fallback path (no saved providers resolved any models) where two providers offering the same model id would collide in pickerItemDedupKey and silently drop a row. - Export config.NormalizeRecentModels and have internal/tui/picker.go's normalizeRecentModelEntries delegate to it instead of maintaining an independent copy of the same trim/dedupe/cap logic, so the TUI and persisted-config normalization can never drift apart. - Add regression tests for both. Addresses remaining CodeRabbit review comments on Gitlawb#568.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/tui/picker_test.go (1)
627-643: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTighten the assertion beyond row count.
The count-only check (
len(items) != 2) confirms rows survive dedup but doesn't confirm they retain the correctOwnerProviderper row. A dedup key bug that still yields 2 rows (e.g. swapped/duplicated provider) would slip through undetected.✅ Suggested stronger assertion
m := model{} items := m.assembleModelPickerItems(nil, catalog) if len(items) != 2 { t.Fatalf("expected both provider rows to survive de-dup, got %#v", items) } + owners := map[string]bool{} + for _, item := range items { + owners[item.OwnerProvider] = true + } + if !owners[string(modelregistry.ProviderAnthropic)] || !owners[string(modelregistry.ProviderOpenAI)] { + t.Fatalf("expected rows for both Anthropic and OpenAI, got %#v", items) + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/picker_test.go` around lines 627 - 643, The test in assembleModelPickerItems only checks that two rows remain, but it does not verify each row kept the correct OwnerProvider. Strengthen the assertion by checking the returned items individually from registryModelPickerItem/testModelEntry so one row is tied to ProviderAnthropic and the other to ProviderOpenAI, ensuring a dedup bug that swaps or duplicates providers is caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/tui/picker_test.go`:
- Around line 627-643: The test in assembleModelPickerItems only checks that two
rows remain, but it does not verify each row kept the correct OwnerProvider.
Strengthen the assertion by checking the returned items individually from
registryModelPickerItem/testModelEntry so one row is tied to ProviderAnthropic
and the other to ProviderOpenAI, ensuring a dedup bug that swaps or duplicates
providers is caught.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 72acf775-09e0-4bec-9613-18df5d1e560b
📒 Files selected for processing (4)
internal/config/resolver.gointernal/config/writer.gointernal/tui/picker.gointernal/tui/picker_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/config/resolver.go
- internal/config/writer.go
- internal/tui/picker.go
…ighten tests - Add recordRecentModels(pairs...), a batched form of recordRecentModel that normalizes and persists exactly once per switch instead of twice (outgoing + incoming pair each triggered their own read-modify-write to userConfigPath). handleModelCommand and switchProviderModel now call it once with both pairs. - picker_test.go: capture and assert switchProviderModel's status return in TestSwitchProviderModelRecordsRecentHistory instead of discarding it, so a failed switch fails at the actual point of failure. - picker_test.go: also assert the persisted config (not just in-memory recentModels) after the reorder/dedupe case in TestModelCommandRecordsAndPersistsRecentHistory. - picker_test.go: pin down the surviving Favorites row's OwnerProvider in TestAssembleModelPickerItemsFavoritesDedupByModelIDAndHideFromOtherGroups, and drop the tautological otherRecent assertion (other-model was never added to recent, so it could never fail). - resolver_test.go: add TestResolveIgnoresProjectOnlyRecentModels, covering the case where only project config sets preferences.recentModels and user config omits it — resolved value must stay empty. Addresses remaining CodeRabbit nitpick comments on Gitlawb#568.
…t-history keys - choosePicker: only treat a picker row as cross-provider when its OwnerProvider resolves to an actual saved provider. Registry-fallback catalog rows (tagged with a provider *kind* string) and stale "Recent" rows referencing a since-removed provider now fall back to an in-place model switch instead of erroring "unknown provider". - handleModelCommand/switchProviderModel: capture and record recent-history entries using m.providerName (the resolved value recentModelPairsForPicker pins the active row with) instead of the raw, possibly-empty provider profile Name, so a no-name provider profile can no longer produce a duplicate "Recent" row for the same switch. - newModelPicker: scope the registry-fallback's active-model exclusion by provider too, consistent with the provider-aware de-dup used elsewhere. - recentModelPairsForPicker: delegate to config.NormalizeRecentModels instead of reimplementing the same trim/dedup/cap logic. - Remove the unused recordRecentModel wrapper. - assembleModelPickerItems: build Favorites from recent/catalog directly instead of allocating a concatenated copy of both slices.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/tui/model.go (1)
3767-3780: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRouting logic correctly guards cross-provider switches; add a regression test for the new branch.
The owner-resolves check (
ownerIsSavedProvider) correctly prevents attempting a switch to a stale/unresolvable provider from a recent-history row, falling back to applying the model against the active provider instead — matching the intended fix for stale/registry-fallback rows. Usingstrings.EqualFoldfor the provider-name comparison is also a good defensive touch against casing mismatches.This is a behavior-changing branch on the model-switch path with no visible unit test covering it (e.g. selecting a picker item whose
OwnerProvideris non-empty but doesn't resolve viasavedProviderByName, or differs only by case from the active provider). Worth locking in with a table-driven test inmodel_test.gogiven this directly implements user-facing/modelrecents correctness.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/model.go` around lines 3767 - 3780, Add a regression test for the new pickerModel routing branch in model_test.go to cover the case where item.OwnerProvider is non-empty but savedProviderByName does not resolve it, and confirm handleModelCommand is used instead of switchProviderModel. Also cover the case where OwnerProvider matches providerName only by case to verify strings.EqualFold keeps the active-provider path. Use the pickerModel branch in internal/tui/model.go and the symbols savedProviderByName, switchProviderModel, and handleModelCommand to locate the behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/tui/model.go`:
- Around line 3767-3780: Add a regression test for the new pickerModel routing
branch in model_test.go to cover the case where item.OwnerProvider is non-empty
but savedProviderByName does not resolve it, and confirm handleModelCommand is
used instead of switchProviderModel. Also cover the case where OwnerProvider
matches providerName only by case to verify strings.EqualFold keeps the
active-provider path. Use the pickerModel branch in internal/tui/model.go and
the symbols savedProviderByName, switchProviderModel, and handleModelCommand to
locate the behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8c2ba901-cda7-4749-9c21-2662627b0df1
📒 Files selected for processing (3)
internal/tui/command_center.gointernal/tui/model.gointernal/tui/picker.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/tui/command_center.go
- internal/tui/picker.go
jatmn
approved these changes
Jul 7, 2026
jatmn
left a comment
jatmn
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for the update. I do not see any actionable code issues in the current patch.
GitHub still reports this PR as blocked, and the earlier Copilot change-request threads for internal/tui/picker.go are still unresolved in the review UI even though the current code appears to implement those requests. Please resolve the completed review conversations or otherwise clear the stale requested-review state before this is treated as ready to merge.
@Vasanthdev2004 LGTM
Vasanthdev2004
added a commit
that referenced
this pull request
Jul 8, 2026
#568 added a bool return to switchProviderModel (model, string, bool, tea.Cmd) and updated every call site except picker_test.go:771, which still assigned three values. That broke the internal/tui test build and turned Smoke red on main. Add the missing receiver here.
yanalialiuk
pushed a commit
to yanalialiuk/zero
that referenced
this pull request
Jul 8, 2026
…Gitlawb#568) * feat(tui): remember recent provider+model selections in /model picker The /model picker's "Recent" section only ever showed the currently active model, making it hard to switch back to a previously used provider+model pair once you've moved on to something else (especially when the same model name exists across multiple providers). Track a short automatic history of provider-qualified switches instead: newest first, capped at 5, deduped by provider+model pair (not model name alone), and selectable across providers like any other picker row. Closes Gitlawb#562 Co-authored-by: Cursor <cursoragent@cursor.com> * chore: gofmt fixes * fix(tui): restore favorites one-row-per-model semantics in /model picker Favorites de-dup had switched to the provider+model key, letting the same favorited model ID surface once per provider. Revert to per-Value de-dup for Favorites, and skip any model ID already shown under Favorites from Recent and Catalog so favorited models don't reappear in a second group. Recent and Catalog still de-dup among themselves by provider+model so cross-provider rows remain distinct. Also correct the recordRecentModel doc comment: in-memory history is updated even without a user config path (only persistence is skipped), not a no-op. Addresses Copilot review comments on Gitlawb#568. * fix(tui): set OwnerProvider on registry catalog rows; dedupe recent-model normalization - registryModelPickerItem now tags rows with OwnerProvider (the registry entry's canonical provider), fixing the plain-registry fallback path (no saved providers resolved any models) where two providers offering the same model id would collide in pickerItemDedupKey and silently drop a row. - Export config.NormalizeRecentModels and have internal/tui/picker.go's normalizeRecentModelEntries delegate to it instead of maintaining an independent copy of the same trim/dedupe/cap logic, so the TUI and persisted-config normalization can never drift apart. - Add regression tests for both. Addresses remaining CodeRabbit review comments on Gitlawb#568. * fix(tui): batch recent-model persistence into one write per switch; tighten tests - Add recordRecentModels(pairs...), a batched form of recordRecentModel that normalizes and persists exactly once per switch instead of twice (outgoing + incoming pair each triggered their own read-modify-write to userConfigPath). handleModelCommand and switchProviderModel now call it once with both pairs. - picker_test.go: capture and assert switchProviderModel's status return in TestSwitchProviderModelRecordsRecentHistory instead of discarding it, so a failed switch fails at the actual point of failure. - picker_test.go: also assert the persisted config (not just in-memory recentModels) after the reorder/dedupe case in TestModelCommandRecordsAndPersistsRecentHistory. - picker_test.go: pin down the surviving Favorites row's OwnerProvider in TestAssembleModelPickerItemsFavoritesDedupByModelIDAndHideFromOtherGroups, and drop the tautological otherRecent assertion (other-model was never added to recent, so it could never fail). - resolver_test.go: add TestResolveIgnoresProjectOnlyRecentModels, covering the case where only project config sets preferences.recentModels and user config omits it — resolved value must stay empty. Addresses remaining CodeRabbit nitpick comments on Gitlawb#568. * fix(tui): route unresolvable OwnerProvider rows in-place; align recent-history keys - choosePicker: only treat a picker row as cross-provider when its OwnerProvider resolves to an actual saved provider. Registry-fallback catalog rows (tagged with a provider *kind* string) and stale "Recent" rows referencing a since-removed provider now fall back to an in-place model switch instead of erroring "unknown provider". - handleModelCommand/switchProviderModel: capture and record recent-history entries using m.providerName (the resolved value recentModelPairsForPicker pins the active row with) instead of the raw, possibly-empty provider profile Name, so a no-name provider profile can no longer produce a duplicate "Recent" row for the same switch. - newModelPicker: scope the registry-fallback's active-model exclusion by provider too, consistent with the provider-aware de-dup used elsewhere. - recentModelPairsForPicker: delegate to config.NormalizeRecentModels instead of reimplementing the same trim/dedup/cap logic. - Remove the unused recordRecentModel wrapper. - assembleModelPickerItems: build Favorites from recent/catalog directly instead of allocating a concatenated copy of both slices. --------- Co-authored-by: Cursor <cursoragent@cursor.com>
yanalialiuk
pushed a commit
to yanalialiuk/zero
that referenced
this pull request
Jul 8, 2026
…itlawb#589) Gitlawb#568 added a bool return to switchProviderModel (model, string, bool, tea.Cmd) and updated every call site except picker_test.go:771, which still assigned three values. That broke the internal/tui test build and turned Smoke red on main. Add the missing receiver here.
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.
Summary
Closes #562.
The
/modelpicker's "Recent" section only ever showed the currently active model. This made it hard to switch back to a previously used provider+model pair once you'd moved on to something else — especially confusing when the same model name exists across multiple providers (the picker's cross-group de-dup keyed on model id alone, so two providers offering the same id would collide).This adds a short automatic history of provider-qualified selections:
config.MaxRecentModels){provider, model}), not bare model namespreferences.recentModels), never merged from project config — same posture asfavoriteModelsTest plan
go build ./...go vet ./...go test ./internal/config/...— new tests forSetRecentModelsnormalization (order preservation, dedup by provider+model pair, cap to 5), and a resolver test confirmingrecentModelsloads from user config only (not merged from project config)go test ./internal/tui/...— new tests:Recentsection pins the active model first and shows history past it; picker row de-dup keys on provider+model, not model id alone;/model <id>and cross-provider picker switches both record and persist history, moving a re-selected pair back to the frontgo test ./internal/cli/...TestProviderWizardAdvancesProviderAPIKeyAndModelStepsalso fails onmainat the base commit (rendering-width assertion), not touched by this change.Summary by CodeRabbit