From fa9ad0227511266ea7563898a1853d54f400091e Mon Sep 17 00:00:00 2001 From: Ousama Ben Younes Date: Sat, 6 Jun 2026 23:12:48 +0000 Subject: [PATCH 1/3] fix(tui): list saved models from all providers in picker The /model picker only listed models for the active provider plus that provider's saved model, so a model saved under a different provider in config (e.g. moonshot kimi-k2.6 while active is deepseek) was invisible even though it resolves and runs. Append cross-provider saved models as a labelled tail after the active provider's list; selecting one switches provider on apply via the existing resolved_provider / build_event path. Unknown provider keys are skipped since they cannot be applied. (#2596) Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/tui/src/tui/model_picker.rs | 81 ++++++++++++++++++++++++++++-- 1 file changed, 78 insertions(+), 3 deletions(-) diff --git a/crates/tui/src/tui/model_picker.rs b/crates/tui/src/tui/model_picker.rs index e2a8f1a0..a55777d1 100644 --- a/crates/tui/src/tui/model_picker.rs +++ b/crates/tui/src/tui/model_picker.rs @@ -385,6 +385,36 @@ fn picker_model_rows_for_app(app: &App) -> Vec { ); } + // Surface models saved under *other* providers in config (#2596). The + // active provider's list comes first; cross-provider saved models follow as + // a clearly labelled tail so a custom model that has never been selected on + // the current provider is still reachable. Selecting one switches provider + // on apply via `resolved_provider` / `build_event`. Rows are sorted by + // provider key so ordering stays deterministic regardless of map iteration. + let mut other_provider_models: Vec<(&String, &String)> = app + .provider_models + .iter() + .filter(|(key, _)| ApiProvider::parse(key) != Some(app.api_provider)) + .collect(); + other_provider_models.sort_by(|(a, _), (b, _)| a.cmp(b)); + for (key, model) in other_provider_models { + let Some(provider) = ApiProvider::parse(key) else { + // Unknown provider key — we cannot switch to it, so skip rather + // than offer a row that would fail to apply. + continue; + }; + let model = model.trim(); + if model.is_empty() { + continue; + } + push_model_row( + &mut rows, + model.to_string(), + Some(provider), + format!("{} saved", provider.display_name()), + ); + } + rows } @@ -855,7 +885,9 @@ mod tests { } #[test] - fn picker_hides_saved_models_from_other_providers() { + fn picker_lists_saved_models_from_other_providers() { + // #2596: custom models saved under a non-active provider must be + // reachable from the picker, after the active provider's own models. let (mut app, _lock) = create_test_app(); app.api_provider = crate::config::ApiProvider::XiaomiMimo; app.model = "mimo-v2.5-pro".to_string(); @@ -868,10 +900,53 @@ mod tests { let view = ModelPickerView::new(&app); let model_ids = view.visible_model_ids(); + // Active provider's own model stays present (and ahead of the tail). assert!(model_ids.contains(&"mimo-v2.5-pro")); - assert!(!model_ids.contains(&"deepseek-v4-pro")); - assert!(!model_ids.contains(&"kimi-k2.6")); + // Cross-provider saved models are now visible. + assert!(model_ids.contains(&"deepseek-v4-pro")); + assert!(model_ids.contains(&"kimi-k2.6")); assert!(!view.show_custom_model_row); + + // Each cross-provider row carries its own provider so applying it + // switches CodeWhale to that provider (verified via build_event below). + let deepseek_row = view + .visible_model_rows() + .iter() + .find(|row| row.id == "deepseek-v4-pro") + .expect("deepseek-v4-pro row present"); + assert_eq!( + deepseek_row.provider, + Some(crate::config::ApiProvider::Deepseek) + ); + + // Active-provider model must appear before any cross-provider tail row. + let active_idx = model_ids + .iter() + .position(|id| *id == "mimo-v2.5-pro") + .expect("active model index"); + let cross_idx = model_ids + .iter() + .position(|id| *id == "kimi-k2.6") + .expect("cross-provider model index"); + assert!( + active_idx < cross_idx, + "active provider models should precede cross-provider tail" + ); + } + + #[test] + fn picker_skips_unknown_provider_saved_models() { + // A config key that maps to no known provider cannot be applied, so it + // must not produce a picker row (#2596). + let (mut app, _lock) = create_test_app(); + app.api_provider = crate::config::ApiProvider::XiaomiMimo; + app.model = "mimo-v2.5-pro".to_string(); + app.auto_model = false; + app.provider_models + .insert("totally-unknown".to_string(), "ghost-model".to_string()); + + let view = ModelPickerView::new(&app); + assert!(!view.visible_model_ids().contains(&"ghost-model")); } #[test] From db0ce5b966cfec2977b23cb906b41df16ed4765c Mon Sep 17 00:00:00 2001 From: Ousama Ben Younes Date: Sat, 6 Jun 2026 23:25:18 +0000 Subject: [PATCH 2/3] refactor(tui): parse provider keys once in picker row build MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Collapse the duplicated ApiProvider::parse call in picker_model_rows_for_app: the filter previously parsed each key to exclude the active provider, then the loop parsed it again via a let-else to skip unknown keys. Use a single filter_map that parses once, drops unknown keys and the active provider in one pass, and carries the parsed provider through to the loop. Pure readability change — unknown keys are still dropped and the active provider is still excluded, so picker rows are identical. Sort still keys on the provider string for deterministic ordering. Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/tui/src/tui/model_picker.rs | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/crates/tui/src/tui/model_picker.rs b/crates/tui/src/tui/model_picker.rs index a55777d1..5cfa748d 100644 --- a/crates/tui/src/tui/model_picker.rs +++ b/crates/tui/src/tui/model_picker.rs @@ -391,18 +391,19 @@ fn picker_model_rows_for_app(app: &App) -> Vec { // the current provider is still reachable. Selecting one switches provider // on apply via `resolved_provider` / `build_event`. Rows are sorted by // provider key so ordering stays deterministic regardless of map iteration. - let mut other_provider_models: Vec<(&String, &String)> = app + // Parse each provider key once: drop unknown keys (cannot be applied) and + // the active provider (already listed above) in a single pass. `key` is + // kept only to keep ordering deterministic via the sort below. + let mut other_provider_models: Vec<(&String, ApiProvider, &String)> = app .provider_models .iter() - .filter(|(key, _)| ApiProvider::parse(key) != Some(app.api_provider)) + .filter_map(|(key, model)| { + let provider = ApiProvider::parse(key)?; + (provider != app.api_provider).then_some((key, provider, model)) + }) .collect(); - other_provider_models.sort_by(|(a, _), (b, _)| a.cmp(b)); - for (key, model) in other_provider_models { - let Some(provider) = ApiProvider::parse(key) else { - // Unknown provider key — we cannot switch to it, so skip rather - // than offer a row that would fail to apply. - continue; - }; + other_provider_models.sort_by(|(a, ..), (b, ..)| a.cmp(b)); + for (_key, provider, model) in other_provider_models { let model = model.trim(); if model.is_empty() { continue; From 97f6e0b2e5f9ed12c4e08c63ec655e4753483588 Mon Sep 17 00:00:00 2001 From: Ousama Ben Younes Date: Sun, 7 Jun 2026 15:17:13 +0000 Subject: [PATCH 3/3] fix(tui): use sort_by_key to satisfy clippy::unnecessary_sort_by --- crates/tui/src/tui/model_picker.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/tui/src/tui/model_picker.rs b/crates/tui/src/tui/model_picker.rs index 5cfa748d..826fe49f 100644 --- a/crates/tui/src/tui/model_picker.rs +++ b/crates/tui/src/tui/model_picker.rs @@ -402,7 +402,7 @@ fn picker_model_rows_for_app(app: &App) -> Vec { (provider != app.api_provider).then_some((key, provider, model)) }) .collect(); - other_provider_models.sort_by(|(a, ..), (b, ..)| a.cmp(b)); + other_provider_models.sort_by_key(|(a, ..)| *a); for (_key, provider, model) in other_provider_models { let model = model.trim(); if model.is_empty() {