agent/codex
2
0
Fork 0
mirror of https://github.com/pchuan98/codex.git synced 2026-07-01 00:31:56 +08:00
Code Issues Packages Projects Releases Wiki Activity

Skip disabled rows in selection menu numbering and default focus (#19170)

Browse Source 
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 <noreply@openai.com>
This commit is contained in:
canvrno-oai
2026-04-24 13:21:43 -07:00
committed by GitHub
Unverified
parent 687c5d9081
commit 7262c0c450
4 changed files with 174 additions and 30 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/tui/src/bottom_pane/list_selection_view.rs
+131 -26
View File
@@ -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<GenericDisplayRow> {
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<usize> {
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<usize> {
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<usize> {
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::<AppEvent>();
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::<AppEvent>();
codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__plugin_detail_error_popup.snap
+11
View File
@@ -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.
codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__realtime_microphone_picker_popup.snap
+4 -4
View File
@@ -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
codex-rs/tui/src/chatwidget/tests/popups_and_settings.rs
+28
View File
@@ -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;