Fix selected model mark if ACP doesn't return updated configOptions after a model change#149
Fix selected model mark if ACP doesn't return updated configOptions after a model change#149ro0gr wants to merge 1 commit intocarlos-algms:mainfrom
configOptions after a model change#149Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe pull request adds state synchronization to the model-switching callback handler in SessionManager. When a model change succeeds, the current model ID is now persisted to the legacy state structure, matching the existing pattern used for mode changes. Additionally, comprehensive test coverage is introduced to validate the model selector UI behavior, including verifying that model selections are properly marked, persisted, and restored across reopenings. Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip Migrating from UI to YAML configuration.Use the |
|
Oh.. seems I was too fast to publish this PR. Local testing has failed. I'll keep working on it |
Mirror the mode change handler pattern to ensure legacy_agent_models state is updated when the provider doesn't return updated configOptions. Also add a happy-path test for modern model change with configOptions
ro0gr
force-pushed
the
bugfix/selected-model
branch
from
c0774d3 to
667459e
Compare
There was a problem hiding this comment.
Pull request overview
This PR aims to fix the model picker’s selected-item marking so the currently selected model is correctly indicated after switching models, even when the provider doesn’t return updated configOptions.
Changes:
- Added SessionManager tests around model selection rendering/marking for both configOptions-backed and legacy model lists.
- Updated
SessionManager:_handle_model_changeto persist the selected legacy model id on successful model change.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lua/agentic/session_manager.lua | Updates legacy model selection state after a successful model change. |
| lua/agentic/session_manager.test.lua | Adds tests validating selected-model re-marking on reopen for configOptions and legacy model selectors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
carlos-algms
left a comment
carlos-algms
left a comment
There was a problem hiding this comment.
Thank you for the contribution.
It's nice and simple.
I just have this comment in the tests, lets address it so we can merge.
| end) | ||
| end) | ||
|
|
||
| describe("show_model_selector", function() |
There was a problem hiding this comment.
Can we move this test to the dedicated file?
So we can reduce its scope and avoid mocking too much.
Using opencode. The model picker always highlights the default model as the selected one.
TODO
The other UX issue I've noticed is that the selected model can be rendered somewhere in the middle of a long model list. It makes it difficult to figure out which model is currently selected. I'd keep the selected model on top of the list. Let me know, and I can work on it in a follow-up PR.