From cd8f247fa105794df6e4741e04b17f44282e507c Mon Sep 17 00:00:00 2001 From: Hunter Bown Date: Fri, 8 May 2026 10:04:30 -0500 Subject: [PATCH] fix(tui): accept uppercase approval shortcuts ## Summary - accept uppercase Y/A/N/D approval shortcuts in addition to lowercase and numeric input - keep destructive approvals on the existing two-step confirmation path - leave #1199 open because the Windows ConHost rendering report is broader than shortcut casing ## Test plan - cargo test -p deepseek-tui benign_y_one_step_approves --locked - cargo test -p deepseek-tui benign_a_two_approves_for_session --locked - cargo test -p deepseek-tui benign_n_d_three_all_deny --locked - cargo test -p deepseek-tui destructive_y_first_press_stages_then_second_commits --locked - cargo test -p deepseek-tui destructive_a_first_press_stages_then_second_commits_session --locked - cargo test -p deepseek-tui destructive_deny_does_not_require_confirmation --locked - cargo fmt --all -- --check - git diff --check - GitHub CI: lint, version drift, ubuntu/macos/windows tests, npm wrapper smoke, GitGuardian --- crates/tui/src/tui/approval.rs | 166 +++++++++++++++++++-------------- 1 file changed, 97 insertions(+), 69 deletions(-) diff --git a/crates/tui/src/tui/approval.rs b/crates/tui/src/tui/approval.rs index a70fadd5..870e3bbe 100644 --- a/crates/tui/src/tui/approval.rs +++ b/crates/tui/src/tui/approval.rs @@ -636,15 +636,17 @@ impl ModalView for ApprovalView { KeyCode::Enter => self.commit_or_stage(self.current_option()), // Direct shortcuts; '1' / '2' map to the first two options // so a numeric pad still works for benign approve flows. - KeyCode::Char('y') | KeyCode::Char('1') => { + KeyCode::Char('y') | KeyCode::Char('Y') | KeyCode::Char('1') => { self.commit_or_stage(ApprovalOption::ApproveOnce) } - KeyCode::Char('a') | KeyCode::Char('2') => { + KeyCode::Char('a') | KeyCode::Char('A') | KeyCode::Char('2') => { self.commit_or_stage(ApprovalOption::ApproveAlways) } - KeyCode::Char('n') | KeyCode::Char('d') | KeyCode::Char('3') => { - self.commit_or_stage(ApprovalOption::Deny) - } + KeyCode::Char('n') + | KeyCode::Char('N') + | KeyCode::Char('d') + | KeyCode::Char('D') + | KeyCode::Char('3') => self.commit_or_stage(ApprovalOption::Deny), KeyCode::Char('v') | KeyCode::Char('V') => { self.pending_confirm = None; self.emit_params_pager() @@ -1173,15 +1175,20 @@ mod tests { #[test] fn benign_y_one_step_approves() { - let mut view = ApprovalView::new(benign_request()); - let action = view.handle_key(create_key_event(KeyCode::Char('y'))); - assert!(matches!( - action, - ViewAction::EmitAndClose(ViewEvent::ApprovalDecision { - decision: ReviewDecision::Approved, - .. - }) - )); + for code in [KeyCode::Char('y'), KeyCode::Char('Y')] { + let mut view = ApprovalView::new(benign_request()); + let action = view.handle_key(create_key_event(code)); + assert!( + matches!( + action, + ViewAction::EmitAndClose(ViewEvent::ApprovalDecision { + decision: ReviewDecision::Approved, + .. + }) + ), + "expected Approved for {code:?}" + ); + } } #[test] @@ -1212,30 +1219,31 @@ mod tests { #[test] fn benign_a_two_approves_for_session() { - let mut view = ApprovalView::new(benign_request()); - let action = view.handle_key(create_key_event(KeyCode::Char('a'))); - assert!(matches!( - action, - ViewAction::EmitAndClose(ViewEvent::ApprovalDecision { - decision: ReviewDecision::ApprovedForSession, - .. - }) - )); - - let mut view = ApprovalView::new(benign_request()); - let action = view.handle_key(create_key_event(KeyCode::Char('2'))); - assert!(matches!( - action, - ViewAction::EmitAndClose(ViewEvent::ApprovalDecision { - decision: ReviewDecision::ApprovedForSession, - .. - }) - )); + for code in [KeyCode::Char('a'), KeyCode::Char('A'), KeyCode::Char('2')] { + let mut view = ApprovalView::new(benign_request()); + let action = view.handle_key(create_key_event(code)); + assert!( + matches!( + action, + ViewAction::EmitAndClose(ViewEvent::ApprovalDecision { + decision: ReviewDecision::ApprovedForSession, + .. + }) + ), + "expected ApprovedForSession for {code:?}" + ); + } } #[test] fn benign_n_d_three_all_deny() { - for code in [KeyCode::Char('n'), KeyCode::Char('d'), KeyCode::Char('3')] { + for code in [ + KeyCode::Char('n'), + KeyCode::Char('N'), + KeyCode::Char('d'), + KeyCode::Char('D'), + KeyCode::Char('3'), + ] { let mut view = ApprovalView::new(benign_request()); let action = view.handle_key(create_key_event(code)); assert!( @@ -1343,22 +1351,27 @@ mod tests { #[test] fn destructive_y_first_press_stages_then_second_commits() { - let mut view = ApprovalView::new(destructive_request()); + for code in [KeyCode::Char('y'), KeyCode::Char('Y')] { + let mut view = ApprovalView::new(destructive_request()); - // First press stages — no decision emitted yet. - let action = view.handle_key(create_key_event(KeyCode::Char('y'))); - assert!(matches!(action, ViewAction::None)); - assert_eq!(view.pending_confirm(), Some(ApprovalOption::ApproveOnce)); + // First press stages — no decision emitted yet. + let action = view.handle_key(create_key_event(code)); + assert!(matches!(action, ViewAction::None)); + assert_eq!(view.pending_confirm(), Some(ApprovalOption::ApproveOnce)); - // Second press of the same key commits. - let action = view.handle_key(create_key_event(KeyCode::Char('y'))); - assert!(matches!( - action, - ViewAction::EmitAndClose(ViewEvent::ApprovalDecision { - decision: ReviewDecision::Approved, - .. - }) - )); + // Second press of the same key commits. + let action = view.handle_key(create_key_event(code)); + assert!( + matches!( + action, + ViewAction::EmitAndClose(ViewEvent::ApprovalDecision { + decision: ReviewDecision::Approved, + .. + }) + ), + "expected Approved for {code:?}" + ); + } } #[test] @@ -1408,20 +1421,25 @@ mod tests { #[test] fn destructive_a_first_press_stages_then_second_commits_session() { - let mut view = ApprovalView::new(destructive_request()); + for code in [KeyCode::Char('a'), KeyCode::Char('A')] { + let mut view = ApprovalView::new(destructive_request()); - let action = view.handle_key(create_key_event(KeyCode::Char('a'))); - assert!(matches!(action, ViewAction::None)); - assert_eq!(view.pending_confirm(), Some(ApprovalOption::ApproveAlways)); + let action = view.handle_key(create_key_event(code)); + assert!(matches!(action, ViewAction::None)); + assert_eq!(view.pending_confirm(), Some(ApprovalOption::ApproveAlways)); - let action = view.handle_key(create_key_event(KeyCode::Char('a'))); - assert!(matches!( - action, - ViewAction::EmitAndClose(ViewEvent::ApprovalDecision { - decision: ReviewDecision::ApprovedForSession, - .. - }) - )); + let action = view.handle_key(create_key_event(code)); + assert!( + matches!( + action, + ViewAction::EmitAndClose(ViewEvent::ApprovalDecision { + decision: ReviewDecision::ApprovedForSession, + .. + }) + ), + "expected ApprovedForSession for {code:?}" + ); + } } #[test] @@ -1442,15 +1460,25 @@ mod tests { #[test] fn destructive_deny_does_not_require_confirmation() { // Deny / Abort skip the two-key dance — the user is bailing. - let mut view = ApprovalView::new(destructive_request()); - let action = view.handle_key(create_key_event(KeyCode::Char('n'))); - assert!(matches!( - action, - ViewAction::EmitAndClose(ViewEvent::ApprovalDecision { - decision: ReviewDecision::Denied, - .. - }) - )); + for code in [ + KeyCode::Char('n'), + KeyCode::Char('N'), + KeyCode::Char('d'), + KeyCode::Char('D'), + ] { + let mut view = ApprovalView::new(destructive_request()); + let action = view.handle_key(create_key_event(code)); + assert!( + matches!( + action, + ViewAction::EmitAndClose(ViewEvent::ApprovalDecision { + decision: ReviewDecision::Denied, + .. + }) + ), + "expected Denied for {code:?}" + ); + } } #[test]