From 7262c0c450f2705f84e54356519f1077b27ca293 Mon Sep 17 00:00:00 2001 From: canvrno-oai Date: Fri, 24 Apr 2026 13:21:43 -0700 Subject: [PATCH] Skip disabled rows in selection menu numbering and default focus (#19170) Selection menus in the TUI currently let disabled rows interfere with numbering and default focus. This makes mixed menus harder to read and can land selection on rows that are not actionable. This change updates the shared selection-menu behavior in list_selection_view so disabled rows are not selected when these views open, and prevents them from being numbered like selectable rows. - Disabled rows no longer receive numeric labels - Digit shortcuts map to enabled rows only - Default selection moves to the first enabled row in mixed menus - Updated affected snapshot - Added snapshot coverage for a plugin detail error popup - Added a focused unit test for shared selection-view behavior --------- Co-authored-by: Codex --- .../src/bottom_pane/list_selection_view.rs | 157 +++++++++++++++--- ...get__tests__plugin_detail_error_popup.snap | 11 ++ ...sts__realtime_microphone_picker_popup.snap | 8 +- .../chatwidget/tests/popups_and_settings.rs | 28 ++++ 4 files changed, 174 insertions(+), 30 deletions(-) create mode 100644 codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__plugin_detail_error_popup.snap diff --git a/codex-rs/tui/src/bottom_pane/list_selection_view.rs b/codex-rs/tui/src/bottom_pane/list_selection_view.rs index c4e7e1c70..4262e4fde 100644 --- a/codex-rs/tui/src/bottom_pane/list_selection_view.rs +++ b/codex-rs/tui/src/bottom_pane/list_selection_view.rs @@ -388,12 +388,21 @@ impl ListSelectionView { fn apply_filter(&mut self) { let previously_selected = self .selected_actual_idx() + .filter(|actual_idx| self.enabled_actual_idx(*actual_idx).is_some()) .or_else(|| { (!self.is_searchable) - .then(|| self.active_items().iter().position(|item| item.is_current)) + .then(|| { + self.active_items() + .iter() + .position(|item| item.is_current && Self::item_is_enabled(item)) + }) .flatten() }) - .or_else(|| self.initial_selected_idx.take()); + .or_else(|| { + self.initial_selected_idx + .take() + .filter(|actual_idx| self.enabled_actual_idx(*actual_idx).is_some()) + }); if self.is_searchable && !self.search_query.is_empty() { let query_lower = self.search_query.to_lowercase(); @@ -411,7 +420,7 @@ impl ListSelectionView { } let len = self.filtered_indices.len(); - self.state.selected_idx = self + let selected_visible_idx = self .state .selected_idx .and_then(|visible_idx| { @@ -425,7 +434,15 @@ impl ListSelectionView { .iter() .position(|idx| *idx == actual_idx) }) + }); + self.state.selected_idx = selected_visible_idx + .filter(|visible_idx| { + self.filtered_indices + .get(*visible_idx) + .and_then(|actual_idx| self.active_items().get(*actual_idx)) + .is_some_and(Self::item_is_enabled) }) + .or_else(|| self.first_enabled_visible_idx()) .or_else(|| (len > 0).then_some(0)); let visible = Self::max_visible_rows(len); @@ -441,6 +458,19 @@ impl ListSelectionView { } fn build_rows(&self) -> Vec { + let enabled_row_number_width = self + .filtered_indices + .iter() + .filter(|actual_idx| { + self.active_items() + .get(**actual_idx) + .is_some_and(Self::item_is_enabled) + }) + .count() + .max(1) + .to_string() + .len(); + let mut enabled_row_number = 0; self.filtered_indices .iter() .enumerate() @@ -458,14 +488,15 @@ impl ListSelectionView { }; let name_with_marker = format!("{name}{marker}"); let is_disabled = item.is_disabled || item.disabled_reason.is_some(); - let n = visible_idx + 1; let wrap_prefix = if self.is_searchable { // The number keys don't work when search is enabled (since we let the // numbers be used for the search query). format!("{prefix} ") } else if is_disabled { - format!("{prefix} {}", " ".repeat(n.to_string().len() + 2)) + format!("{prefix} {}", " ".repeat(enabled_row_number_width + 2)) } else { + enabled_row_number += 1; + let n = enabled_row_number; format!("{prefix} {n}. ") }; let wrap_prefix_width = UnicodeWidthStr::width(wrap_prefix.as_str()); @@ -524,24 +555,35 @@ impl ListSelectionView { fn select_first_enabled_row(&mut self) { let selected_visible_idx = self - .filtered_indices - .iter() - .position(|actual_idx| { - self.active_items() - .get(*actual_idx) - .is_some_and(|item| item.disabled_reason.is_none() && !item.is_disabled) - }) + .first_enabled_visible_idx() .or_else(|| (!self.filtered_indices.is_empty()).then_some(0)); self.state.selected_idx = selected_visible_idx; self.state.scroll_top = 0; } + fn first_enabled_visible_idx(&self) -> Option { + self.filtered_indices.iter().position(|actual_idx| { + self.active_items() + .get(*actual_idx) + .is_some_and(Self::item_is_enabled) + }) + } + + fn enabled_actual_idx(&self, actual_idx: usize) -> Option { + self.active_items() + .get(actual_idx) + .is_some_and(Self::item_is_enabled) + .then_some(actual_idx) + } + + fn item_is_enabled(item: &SelectionItem) -> bool { + item.disabled_reason.is_none() && !item.is_disabled + } + fn selected_item_has_toggle(&self) -> bool { self.selected_actual_idx() .and_then(|actual_idx| self.active_items().get(actual_idx)) - .is_some_and(|item| { - item.toggle.is_some() && item.disabled_reason.is_none() && !item.is_disabled - }) + .is_some_and(|item| item.toggle.is_some() && Self::item_is_enabled(item)) } fn selected_item_has_toggle_placeholder(&self) -> bool { @@ -550,11 +592,23 @@ impl ListSelectionView { .is_some_and(|item| { item.toggle.is_none() && item.toggle_placeholder.is_some() - && item.disabled_reason.is_none() - && !item.is_disabled + && Self::item_is_enabled(item) }) } + fn actual_idx_for_enabled_number(&self, number: usize) -> Option { + if number == 0 { + return None; + } + + self.active_items() + .iter() + .enumerate() + .filter(|(_, item)| Self::item_is_enabled(item)) + .nth(number - 1) + .map(|(idx, _)| idx) + } + fn toggle_selected(&mut self) { let Some(actual_idx) = self.selected_actual_idx() else { return; @@ -563,7 +617,7 @@ impl ListSelectionView { let Some(item) = self.active_items_mut().get_mut(actual_idx) else { return; }; - if item.is_disabled || item.disabled_reason.is_some() { + if !Self::item_is_enabled(item) { return; } let Some(toggle) = item.toggle.as_mut() else { @@ -845,8 +899,7 @@ impl BottomPaneView for ListSelectionView { if let Some(idx) = self.items.iter().position(|item| { item.display_shortcut .is_some_and(|shortcut| shortcut.is_press(key_event)) - && item.disabled_reason.is_none() - && !item.is_disabled + && Self::item_is_enabled(item) }) { self.state.selected_idx = Some(idx); self.accept(); @@ -855,12 +908,7 @@ impl BottomPaneView for ListSelectionView { if let Some(idx) = c .to_digit(10) .map(|d| d as usize) - .and_then(|d| d.checked_sub(1)) - && idx < self.active_items().len() - && self - .active_items() - .get(idx) - .is_some_and(|item| item.disabled_reason.is_none() && !item.is_disabled) + .and_then(|number| self.actual_idx_for_enabled_number(number)) { self.state.selected_idx = Some(idx); self.accept(); @@ -1839,6 +1887,63 @@ mod tests { ); } + #[test] + fn disabled_current_rows_skip_default_selection_and_number_shortcuts() { + let (tx_raw, _rx) = unbounded_channel::(); + let tx = AppEventSender::new(tx_raw); + let mut view = ListSelectionView::new( + SelectionViewParams { + items: vec![ + SelectionItem { + name: "Unavailable".to_string(), + description: Some("Not available right now.".to_string()), + is_current: true, + is_disabled: true, + ..Default::default() + }, + SelectionItem { + name: "Alpha".to_string(), + dismiss_on_select: true, + ..Default::default() + }, + SelectionItem { + name: "Busy".to_string(), + description: Some("Still disabled.".to_string()), + disabled_reason: Some("Try again later.".to_string()), + ..Default::default() + }, + SelectionItem { + name: "Beta".to_string(), + dismiss_on_select: true, + ..Default::default() + }, + ], + ..Default::default() + }, + tx, + ); + + assert_eq!(view.selected_actual_idx(), Some(1)); + + let rendered = render_lines_with_width(&view, /*width*/ 60); + assert!( + rendered.contains("› 1. Alpha"), + "expected first enabled row to be selected and numbered 1, got:\n{rendered}" + ); + assert!( + rendered.contains(" 2. Beta"), + "expected second enabled row to be numbered 2, got:\n{rendered}" + ); + assert!( + !rendered.contains("1. Unavailable") && !rendered.contains("3. Beta"), + "expected disabled rows to be skipped by numbering, got:\n{rendered}" + ); + + view.handle_key_event(KeyEvent::new(KeyCode::Char('2'), KeyModifiers::NONE)); + + assert_eq!(view.take_last_selected_index(), Some(3)); + } + #[test] fn wraps_long_option_without_overflowing_columns() { let (tx_raw, _rx) = unbounded_channel::(); diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__plugin_detail_error_popup.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__plugin_detail_error_popup.snap new file mode 100644 index 000000000..5305a9fc7 --- /dev/null +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__plugin_detail_error_popup.snap @@ -0,0 +1,11 @@ +--- +source: tui/src/chatwidget/tests.rs +expression: popup +--- + Plugins + Failed to load plugin details. + + Plugin detail unavailable Failed to load plugin details. +› 1. Back to plugins Return to the plugin list. + + Press esc to close. diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__realtime_microphone_picker_popup.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__realtime_microphone_picker_popup.snap index 00392bc9d..418fb5c9e 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__realtime_microphone_picker_popup.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__realtime_microphone_picker_popup.snap @@ -5,14 +5,14 @@ expression: popup Select Microphone Saved devices apply to realtime voice only. - 1. System default Use your operating system +› 1. System default Use your operating system default device. -› Unavailable: Studio Mic (current) (disabled) Configured device is not + Unavailable: Studio Mic (current) (disabled) Configured device is not currently available. (disabled: Reconnect the device or choose another one.) - 3. Built-in Mic - 4. USB Mic + 2. Built-in Mic + 3. USB Mic Press enter to confirm or esc to go back diff --git a/codex-rs/tui/src/chatwidget/tests/popups_and_settings.rs b/codex-rs/tui/src/chatwidget/tests/popups_and_settings.rs index 776741d05..a800c9e7e 100644 --- a/codex-rs/tui/src/chatwidget/tests/popups_and_settings.rs +++ b/codex-rs/tui/src/chatwidget/tests/popups_and_settings.rs @@ -247,6 +247,34 @@ async fn plugin_detail_popup_hides_disclosure_for_installed_plugins() { ); } +#[tokio::test] +async fn plugin_detail_error_popup_skips_disabled_row_numbering() { + let (mut chat, _rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await; + chat.set_feature_enabled(Feature::Plugins, /*enabled*/ true); + + let response = plugins_test_response(vec![plugins_test_curated_marketplace(vec![ + plugins_test_summary( + "plugin-figma", + "figma", + Some("Figma"), + Some("Design handoff."), + /*installed*/ false, + /*enabled*/ true, + PluginInstallPolicy::Available, + ), + ])]); + let cwd = chat.config.cwd.clone(); + chat.on_plugins_loaded(cwd.to_path_buf(), Ok(response)); + chat.add_plugins_output(); + chat.on_plugin_detail_loaded( + cwd.to_path_buf(), + Err("Failed to load plugin details.".to_string()), + ); + + let popup = render_bottom_popup(&chat, /*width*/ 100); + assert_chatwidget_snapshot!("plugin_detail_error_popup", popup); +} + #[tokio::test] async fn plugins_popup_refresh_preserves_selected_row_position() { let (mut chat, _rx, _op_rx) = make_chatwidget_manual(/*model_override*/ None).await;