fix: preserve provider-selected models#1649
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes provider/model state drift in the TUI so saved provider selections are reflected in runtime config and active-provider reselects do not reset the current model.
Changes:
- Syncs
App’s selected provider back into the mutable runtimeConfigduring TUI startup. - Preserves the active model when the provider picker reapplies the currently active provider.
- Adds regression tests and changelog notes for the v0.8.38 provider/model fix.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
crates/tui/src/tui/ui.rs |
Adds provider sync helper and provider-picker model override handling. |
crates/tui/src/tui/ui/tests.rs |
Adds regression coverage for startup provider sync and active-provider reselect behavior. |
CHANGELOG.md |
Documents the provider/model fix and contributor credit. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let previous_home = std::env::var_os("HOME"); | ||
| let previous_userprofile = std::env::var_os("USERPROFILE"); | ||
| // Safety: test-only environment mutation guarded by a global mutex. | ||
| unsafe { | ||
| std::env::set_var("HOME", tmp.path()); | ||
| std::env::set_var("USERPROFILE", tmp.path()); | ||
| } |
32c8065 to
2b157f0
Compare
There was a problem hiding this comment.
Code Review
This pull request ensures that provider-selected models persist through startup and provider picker re-selections by synchronizing the application state with the runtime configuration. It also updates the provider switching logic to preserve the current model when the active provider is re-selected and adds test infrastructure for environment isolation. Feedback suggests extending the synchronization function to include the model as well as the provider to maintain full consistency across all components relying on the configuration.
| fn sync_config_provider_from_app(config: &mut Config, app: &App) { | ||
| config.provider = Some(app.api_provider.as_str().to_string()); | ||
| } |
There was a problem hiding this comment.
While syncing the provider is critical for correctly resolving the API key and base URL, App::new also resolves the active model from Settings. For full consistency between the runtime Config and the App state, we should also sync the model back to the config object. This ensures that any component relying on the Config (like sub-agents or future background tasks) uses the model the user actually selected in a previous session.
| fn sync_config_provider_from_app(config: &mut Config, app: &App) { | |
| config.provider = Some(app.api_provider.as_str().to_string()); | |
| } | |
| fn sync_config_provider_from_app(config: &mut Config, app: &App) { | |
| config.provider = Some(app.api_provider.as_str().to_string()); | |
| config.default_text_model = Some(app.model.clone()); | |
| } |
2b157f0 to
8dda9a4
Compare
Summary
Fixes #1632.
Harvests the provider/model bug-fix portion of #1642 without the vision feature.
Harvests the provider-aware /model picker catalog portion of #1201.
Tests