fix(tui): store a copy of the selected model, not the caller's object - #1365
Conversation
`selectModel()` stored the given model object in the model store by reference. A Solid store keeps the first object set at a path and merges later sets into it, so when that first object was a conversation's recorded model from the sync store, opening a second conversation wrote its model into the first conversation's record, and returning to the first selected the second's model. Store a copy instead. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016x3nbZU5Vb6HnSTqE7vELm
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughModel selection now stores a copy of the selected model object. A shared test fixture supports tests that restore models across conversations and cycle through model selections. ChangesConversation model restoration
Estimated code review effort: 2 (Simple) | ~12 minutes Merge Risk: ⚪ Minimal · up to The model-selection change and test refactor appear ready to merge after normal checks; no actionable issue remains identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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. A rabbit checks the model store with care, Comment |
|
@codex review Please try to falsify these claims with a concrete sequence:
Please skip style nits. |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (3 files)
Previous Review Summary (commit f85c3be)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit f85c3be)Status: No Issues Found | Recommendation: Merge Files Reviewed (3 files)
Reviewed by gpt-sol-latest · Input: 0 · Output: 0 · Cached: 0 Review guidance: REVIEW.md from base branch |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Move the LocalProvider and SyncProvider mount, the model fixtures and `waitUntil` into `test/fixture/local-model.tsx`, so the cycling tests and the conversation-switching test use one copy. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016x3nbZU5Vb6HnSTqE7vELm
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
sahrizvi
left a comment
sahrizvi
left a comment
There was a problem hiding this comment.
LGTM. The root cause is right, and the copy sits in selectModel, which every selection path (picker, favorite cycle, session restore) goes through. I confirmed the new regression test fails with only src/context/local.tsx reverted: A's record reads back as altimate-free/altimate-base. It passes with the fix, and so does the stale-Zen suite.
A few optional follow-ups, none of them blocking:
- Strengthen the regression test (
test/context/model-store-aliasing.test.tsx:42-48). It checks A's record and the final selection, but not B's record, and not the selection right after opening B. Adding these would cover both sides:local.model.restoreSession(recordedB().model) expect(local.model.current()).toMatchObject(BASE) // ... expect({ ...recordedB().model }).toMatchObject(BASE)
restoreSessionreturns the caller's object (src/context/local.tsx:1096-1103). In the non-Zen branch,resolvedis the sync store's message record. This is not a bug today: the only caller (component/prompt/index.tsx:403) only reads fields. Returning{ providerID: model.providerID, modelID: model.modelID }would keep the API from handing out a store-owned object.- Nit:
model-store-aliasing.test.tsx:34repeats thelocal.model.readywait thatmount()already does.
2 participants
Issue for this PR
Closes #1364
Type of change
What does this PR do?
selectModel()inpackages/tui/src/context/local.tsxstored the model object it was given withsetModelStore("model", agent, model). A Solid store keeps the first object set at a path by reference, and later sets at that path merge their fields into that same object (updatePath→mergeStoreNode).Opening a conversation calls
restoreSession(msg.model), passing the last user message's recorded model, an object owned by the sync store. When that is the first model stored for the agent in the launch, the model store and the message record share one object. Opening another conversation then merges its model into the first conversation's record, so returning to the first conversation restores the second one's model.The fix stores a copy (
{ providerID, modelID }), so the model store never holds or writes into an object owned by another store. It's a three-line change.How did you verify your code works?
New test
test/context/model-store-aliasing.test.tsx. It mounts the realLocalProviderandSyncProviderand feeds two conversations' user messages throughmessage.updatedevents. It then callsrestoreSession()with each conversation's recorded model, as the prompt does, and returns to the first. The test asserts the first conversation's record and the selection on return. It fails onmain(the record reads back as the second conversation's model) and passes with this change.In the real TUI, from source, with two conversations recorded on different providers:
mainTUI suite: 339 pass.
dialog-scan-gate> "pressing y chooses scan" fails identically onmain. Typecheck and the marker/branding guards are clean.Screenshots / recordings
Text captures are in the table above; the change has no visual component.
Checklist
🤖 Generated with Claude Code
https://claude.ai/code/session_016x3nbZU5Vb6HnSTqE7vELm
Summary by cubic
Fixes cross-conversation model aliasing so returning to a previously opened conversation restores its own model instead of a later one's.
Bug Fixes
selectModel()now stores a copy of the model rather than the caller's object; Solid stores keep the first object set at a path by reference and merge later sets into it, so the old code wrote later selections into another conversation's recorded message.Refactors
Written for commit 68fcbd1. Summary will update on new commits.
Summary by CodeRabbit