From 51c20c320bd6f11a34acdbbb9310a4ae411de714 Mon Sep 17 00:00:00 2001 From: pppp606 Date: Fri, 5 Dec 2025 15:53:22 +0900 Subject: [PATCH 1/3] feat(tui): add Ctrl+n/Ctrl+p navigation to selection popups Extends keyboard navigation support to approval mode, model selection, and rate limit switch popups, matching the behavior added in #1994 for slash commands and file selector. --- .../src/bottom_pane/list_selection_view.rs | 86 +++++++++++++++++++ 1 file changed, 86 insertions(+) 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 d294a472653..1cadad0ddce 100644 --- a/codex-rs/tui/src/bottom_pane/list_selection_view.rs +++ b/codex-rs/tui/src/bottom_pane/list_selection_view.rs @@ -266,10 +266,20 @@ impl BottomPaneView for ListSelectionView { match key_event { KeyEvent { code: KeyCode::Up, .. + } + | KeyEvent { + code: KeyCode::Char('p'), + modifiers: KeyModifiers::CONTROL, + .. } => self.move_up(), KeyEvent { code: KeyCode::Down, .. + } + | KeyEvent { + code: KeyCode::Char('n'), + modifiers: KeyModifiers::CONTROL, + .. } => self.move_down(), KeyEvent { code: KeyCode::Backspace, @@ -713,4 +723,80 @@ mod tests { render_lines_with_width(&view, 24) ); } + + #[test] + fn ctrl_n_moves_selection_down() { + let mut view = make_selection_view(None); + // Initial selection is on first item (Read Only which is_current) + let initial = render_lines(&view); + assert!( + initial.contains("› 1. Read Only"), + "expected first item selected initially" + ); + + // Press Ctrl+n to move down + view.handle_key_event(KeyEvent::new( + KeyCode::Char('n'), + KeyModifiers::CONTROL, + )); + let after_ctrl_n = render_lines(&view); + assert!( + after_ctrl_n.contains("› 2. Full Access"), + "expected second item selected after Ctrl+n" + ); + } + + #[test] + fn ctrl_p_moves_selection_up() { + let mut view = make_selection_view(None); + // Move down first so we can move up + view.handle_key_event(KeyEvent::new(KeyCode::Down, KeyModifiers::NONE)); + let after_down = render_lines(&view); + assert!( + after_down.contains("› 2. Full Access"), + "expected second item selected after Down" + ); + + // Press Ctrl+p to move up + view.handle_key_event(KeyEvent::new( + KeyCode::Char('p'), + KeyModifiers::CONTROL, + )); + let after_ctrl_p = render_lines(&view); + assert!( + after_ctrl_p.contains("› 1. Read Only"), + "expected first item selected after Ctrl+p" + ); + } + + #[test] + fn ctrl_n_and_ctrl_p_wrap_around() { + let mut view = make_selection_view(None); + // Move to second item + view.handle_key_event(KeyEvent::new( + KeyCode::Char('n'), + KeyModifiers::CONTROL, + )); + // Move past last item - should wrap to first + view.handle_key_event(KeyEvent::new( + KeyCode::Char('n'), + KeyModifiers::CONTROL, + )); + let wrapped_forward = render_lines(&view); + assert!( + wrapped_forward.contains("› 1. Read Only"), + "expected selection to wrap to first item" + ); + + // Move up from first item - should wrap to last + view.handle_key_event(KeyEvent::new( + KeyCode::Char('p'), + KeyModifiers::CONTROL, + )); + let wrapped_back = render_lines(&view); + assert!( + wrapped_back.contains("› 2. Full Access"), + "expected selection to wrap to last item" + ); + } } From a154d281040afffe99b59ab63812633c52afff9d Mon Sep 17 00:00:00 2001 From: pppp606 Date: Fri, 5 Dec 2025 17:10:03 +0900 Subject: [PATCH 2/3] fix(tui): handle C0 control chars for Ctrl+n/Ctrl+p in selection popups Some terminals send Control key chords as C0 control characters without reporting the CONTROL modifier. Add fallback handling for ^N (U+000E) and ^P (U+0010) to ensure consistent navigation across all terminal environments. Follows the same pattern used in textarea.rs (see #7530). --- .../src/bottom_pane/list_selection_view.rs | 45 ++++++++++++++++++- 1 file changed, 43 insertions(+), 2 deletions(-) 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 1cadad0ddce..c097cf607a8 100644 --- a/codex-rs/tui/src/bottom_pane/list_selection_view.rs +++ b/codex-rs/tui/src/bottom_pane/list_selection_view.rs @@ -264,6 +264,9 @@ impl ListSelectionView { impl BottomPaneView for ListSelectionView { fn handle_key_event(&mut self, key_event: KeyEvent) { match key_event { + // Some terminals (or configurations) send Control key chords as + // C0 control characters without reporting the CONTROL modifier. + // Handle fallbacks for Ctrl-P/N here so navigation works everywhere. KeyEvent { code: KeyCode::Up, .. } @@ -271,7 +274,12 @@ impl BottomPaneView for ListSelectionView { code: KeyCode::Char('p'), modifiers: KeyModifiers::CONTROL, .. - } => self.move_up(), + } + | KeyEvent { + code: KeyCode::Char('\u{0010}'), + modifiers: KeyModifiers::NONE, + .. + } /* ^P */ => self.move_up(), KeyEvent { code: KeyCode::Down, .. @@ -280,7 +288,12 @@ impl BottomPaneView for ListSelectionView { code: KeyCode::Char('n'), modifiers: KeyModifiers::CONTROL, .. - } => self.move_down(), + } + | KeyEvent { + code: KeyCode::Char('\u{000e}'), + modifiers: KeyModifiers::NONE, + .. + } /* ^N */ => self.move_down(), KeyEvent { code: KeyCode::Backspace, .. @@ -799,4 +812,32 @@ mod tests { "expected selection to wrap to last item" ); } + + #[test] + fn c0_control_chars_navigate_selection() { + let mut view = make_selection_view(None); + // Initial selection is on first item + let initial = render_lines(&view); + assert!( + initial.contains("› 1. Read Only"), + "expected first item selected initially" + ); + + // Simulate terminals that send C0 control chars without CONTROL modifier. + // ^N (U+000E) should move down + view.handle_key_event(KeyEvent::new(KeyCode::Char('\u{000e}'), KeyModifiers::NONE)); + let after_c0_n = render_lines(&view); + assert!( + after_c0_n.contains("› 2. Full Access"), + "expected second item selected after ^N (C0)" + ); + + // ^P (U+0010) should move up + view.handle_key_event(KeyEvent::new(KeyCode::Char('\u{0010}'), KeyModifiers::NONE)); + let after_c0_p = render_lines(&view); + assert!( + after_c0_p.contains("› 1. Read Only"), + "expected first item selected after ^P (C0)" + ); + } } From 02d80ff9edb06dfe0d729442c4e3564ba8a4807d Mon Sep 17 00:00:00 2001 From: pppp606 Date: Sat, 6 Dec 2025 21:08:32 +0900 Subject: [PATCH 3/3] refactor: remove Ctrl+n/Ctrl+p tests per review feedback Tests for this functionality are considered overkill. Removed to keep CI times in check. --- .../src/bottom_pane/list_selection_view.rs | 104 ------------------ 1 file changed, 104 deletions(-) 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 c097cf607a8..4265034b01a 100644 --- a/codex-rs/tui/src/bottom_pane/list_selection_view.rs +++ b/codex-rs/tui/src/bottom_pane/list_selection_view.rs @@ -736,108 +736,4 @@ mod tests { render_lines_with_width(&view, 24) ); } - - #[test] - fn ctrl_n_moves_selection_down() { - let mut view = make_selection_view(None); - // Initial selection is on first item (Read Only which is_current) - let initial = render_lines(&view); - assert!( - initial.contains("› 1. Read Only"), - "expected first item selected initially" - ); - - // Press Ctrl+n to move down - view.handle_key_event(KeyEvent::new( - KeyCode::Char('n'), - KeyModifiers::CONTROL, - )); - let after_ctrl_n = render_lines(&view); - assert!( - after_ctrl_n.contains("› 2. Full Access"), - "expected second item selected after Ctrl+n" - ); - } - - #[test] - fn ctrl_p_moves_selection_up() { - let mut view = make_selection_view(None); - // Move down first so we can move up - view.handle_key_event(KeyEvent::new(KeyCode::Down, KeyModifiers::NONE)); - let after_down = render_lines(&view); - assert!( - after_down.contains("› 2. Full Access"), - "expected second item selected after Down" - ); - - // Press Ctrl+p to move up - view.handle_key_event(KeyEvent::new( - KeyCode::Char('p'), - KeyModifiers::CONTROL, - )); - let after_ctrl_p = render_lines(&view); - assert!( - after_ctrl_p.contains("› 1. Read Only"), - "expected first item selected after Ctrl+p" - ); - } - - #[test] - fn ctrl_n_and_ctrl_p_wrap_around() { - let mut view = make_selection_view(None); - // Move to second item - view.handle_key_event(KeyEvent::new( - KeyCode::Char('n'), - KeyModifiers::CONTROL, - )); - // Move past last item - should wrap to first - view.handle_key_event(KeyEvent::new( - KeyCode::Char('n'), - KeyModifiers::CONTROL, - )); - let wrapped_forward = render_lines(&view); - assert!( - wrapped_forward.contains("› 1. Read Only"), - "expected selection to wrap to first item" - ); - - // Move up from first item - should wrap to last - view.handle_key_event(KeyEvent::new( - KeyCode::Char('p'), - KeyModifiers::CONTROL, - )); - let wrapped_back = render_lines(&view); - assert!( - wrapped_back.contains("› 2. Full Access"), - "expected selection to wrap to last item" - ); - } - - #[test] - fn c0_control_chars_navigate_selection() { - let mut view = make_selection_view(None); - // Initial selection is on first item - let initial = render_lines(&view); - assert!( - initial.contains("› 1. Read Only"), - "expected first item selected initially" - ); - - // Simulate terminals that send C0 control chars without CONTROL modifier. - // ^N (U+000E) should move down - view.handle_key_event(KeyEvent::new(KeyCode::Char('\u{000e}'), KeyModifiers::NONE)); - let after_c0_n = render_lines(&view); - assert!( - after_c0_n.contains("› 2. Full Access"), - "expected second item selected after ^N (C0)" - ); - - // ^P (U+0010) should move up - view.handle_key_event(KeyEvent::new(KeyCode::Char('\u{0010}'), KeyModifiers::NONE)); - let after_c0_p = render_lines(&view); - assert!( - after_c0_p.contains("› 1. Read Only"), - "expected first item selected after ^P (C0)" - ); - } }