fix(tui): persist provider switches to config#2718
Conversation
|
Thanks @xyuai for taking the time to contribute. This repository is currently observing a maintainer-managed contribution gate in dry-run mode, so this pull request is staying open. When enforcement is enabled, pull requests from contributors who are not listed in Please read |
There was a problem hiding this comment.
Code Review
This pull request updates the provider switching logic in the TUI to persist the selected provider to the configuration file, handling potential errors and displaying a warning if persistence fails. It also adds a corresponding integration test to verify this behavior. The review feedback suggests simplifying the construction of the switch_summary string by using a single format! macro call and using the idiomatic '\n' character instead of char::from(10).
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| let mut switch_summary = format!( | ||
| "Provider switched: {} -> {}", | ||
| previous_provider.as_str(), | ||
| target.as_str(), | ||
| ); | ||
| switch_summary.push(char::from(10)); | ||
| switch_summary.push_str(&format!("Model: {} -> {}", previous_model, new_model)); | ||
| if let Some(ref warning) = persist_warning { | ||
| switch_summary.push(char::from(10)); | ||
| switch_summary.push_str(warning); | ||
| } |
There was a problem hiding this comment.
The string construction for switch_summary can be simplified and made more idiomatic. Instead of manually pushing newlines via char::from(10) and allocating a temporary string with format! for the model transition, you can combine the initial message and the model transition into a single format! call. Additionally, using '\n' is the standard, idiomatic way to append a newline character in Rust.
let mut switch_summary = format!(
"Provider switched: {} -> {}\nModel: {} -> {}",
previous_provider.as_str(),
target.as_str(),
previous_model,
new_model
);
if let Some(ref warning) = persist_warning {
switch_summary.push('\n');
switch_summary.push_str(warning);
}| switch_summary.push(char::from(10)); | ||
| switch_summary.push_str(&format!("Model: {} -> {}", previous_model, new_model)); | ||
| if let Some(ref warning) = persist_warning { | ||
| switch_summary.push(char::from(10)); |
There was a problem hiding this comment.
char::from(10) is a non-idiomatic way to express a newline in Rust and makes the intent harder to read at a glance. The idiomatic form is the ' ' literal, which the rest of the codebase already uses in format strings.
| switch_summary.push(char::from(10)); | |
| switch_summary.push_str(&format!("Model: {} -> {}", previous_model, new_model)); | |
| if let Some(ref warning) = persist_warning { | |
| switch_summary.push(char::from(10)); | |
| switch_summary.push('\n'); | |
| switch_summary.push_str(&format!("Model: {} -> {}", previous_model, new_model)); | |
| if let Some(ref warning) = persist_warning { | |
| switch_summary.push('\n'); |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| let mut switch_summary = format!( | ||
| "Provider switched: {} -> {}", | ||
| previous_provider.as_str(), | ||
| target.as_str(), | ||
| ); | ||
| switch_summary.push(char::from(10)); | ||
| switch_summary.push_str(&format!("Model: {} -> {}", previous_model, new_model)); |
There was a problem hiding this comment.
The adjacent model/thinking change messages (lines 5215–5257) consistently use the Unicode right arrow
→, but the new provider-switch message uses the ASCII two-character sequence ->. The inconsistency is visible to users who see both types of messages in the same session.
| let mut switch_summary = format!( | |
| "Provider switched: {} -> {}", | |
| previous_provider.as_str(), | |
| target.as_str(), | |
| ); | |
| switch_summary.push(char::from(10)); | |
| switch_summary.push_str(&format!("Model: {} -> {}", previous_model, new_model)); | |
| let mut switch_summary = format!( | |
| "Provider switched: {} → {}", | |
| previous_provider.as_str(), | |
| target.as_str(), | |
| ); | |
| switch_summary.push('\n'); | |
| switch_summary.push_str(&format!("Model: {} → {}", previous_model, new_model)); |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
Cherry-picked to codex/v0.8.53 in dba332e with endpoint display merged. |
Summary
/providerswitches back toconfig.tomlso restart behavior matches the active providerFixes #2663.
Greptile Summary
This PR fixes a stale-restart bug (#2663) where switching providers via
/providerin the TUI was not written back toconfig.toml, so the next launch would revert to the previously configured provider. The fix wraps bothpersist_root_string_key(config.toml write) andSettings::load/savein a single closure; on any error the switch still takes effect in memory but a visible warning is appended to the chat and status bar.ui.rs:switch_providernow always callscommands::persist_root_string_keyto updateconfig.tomland separately updatesSettings, surfacing a non-fatal warning instead of silently ignoring persistence failures.tests.rs: New regression testprovider_switch_persists_provider_to_config_for_restartcovers the Arcee → XiaomiMimo split-state path from provider switching: settings/config split can mix MiMo model with Arcee base URL #2663, asserting both the in-memoryconfigand the file reloaded from disk agree on the new provider.Confidence Score: 4/5
Safe to merge — the persistence logic is correct and failures are surfaced as a user-visible warning rather than crashing or silently dropping the switch.
The core fix is straightforward and well-tested: the new regression test verifies the on-disk config, in-memory state, and settings all agree after a switch. The only findings are cosmetic — non-idiomatic newline literals and an arrow character that diverges from the style used by adjacent messages.
No files require special attention; the two style issues are both confined to the new lines in ui.rs.
Important Files Changed
Sequence Diagram
sequenceDiagram participant User participant TUI as TUI (switch_provider) participant Engine participant ConfigTOML as config.toml participant Settings as settings.json User->>TUI: /provider xiaomi-mimo TUI->>TUI: "Update in-memory Config & App state" TUI->>Engine: Op::Shutdown TUI->>Engine: spawn_engine (new provider) TUI->>Engine: Op::SyncSession / Op::SetCompaction note over TUI,Settings: Persistence closure (errors become a warning, never block the switch) TUI->>ConfigTOML: persist_root_string_key("provider", "xiaomi-mimo") alt write succeeds ConfigTOML-->>TUI: Ok(path) TUI->>Settings: Settings::load() Settings-->>TUI: settings TUI->>TUI: "settings.default_provider = "xiaomi-mimo"" TUI->>Settings: settings.save() TUI->>User: "Provider switched: Arcee → XiaomiMimo" (no warning) else any step fails TUI->>User: "Provider switched … (not fully persisted)" endReviews (1): Last reviewed commit: "fix(tui): persist provider switches to c..." | Re-trigger Greptile