diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index 1a3542c729..7efea158bc 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -3868,8 +3868,8 @@ mod tests { } #[tokio::test] - async fn reject_patch_with_notes_submits_denied_then_user_input() -> Result<()> { - let (mut app, _app_event_rx, mut op_rx) = make_test_app_with_channels().await; + async fn reject_patch_with_notes_submits_ops_and_renders_follow_up() -> Result<()> { + let (mut app, mut app_event_rx, mut op_rx) = make_test_app_with_channels().await; let thread_id = ThreadId::new(); app.active_thread_id = Some(thread_id); @@ -3877,7 +3877,7 @@ mod tests { &mut app, thread_id, "call-1", - "please split this into smaller patches", + "i want the content to say bye instead.", ) .await; @@ -3892,28 +3892,13 @@ mod tests { op_rx.try_recv(), Ok(Op::UserInput { items: vec![UserInput::Text { - text: "please split this into smaller patches".to_string(), + text: "i want the content to say bye instead.".to_string(), text_elements: Vec::new(), }], final_output_json_schema: None, }) ); - Ok(()) - } - #[tokio::test] - async fn reject_patch_with_notes_renders_follow_up_in_active_transcript() -> Result<()> { - let (mut app, mut app_event_rx, _op_rx) = make_test_app_with_channels().await; - let thread_id = ThreadId::new(); - app.active_thread_id = Some(thread_id); - - reject_patch_with_notes_for_test( - &mut app, - thread_id, - "call-2", - "i want the content to say bye instead.", - ) - .await; drain_insert_history_cells(&mut app, &mut app_event_rx); let user_messages: Vec = app diff --git a/codex-rs/tui/src/bottom_pane/approval_overlay.rs b/codex-rs/tui/src/bottom_pane/approval_overlay.rs index 9fb8c55ec0..08254f10ae 100644 --- a/codex-rs/tui/src/bottom_pane/approval_overlay.rs +++ b/codex-rs/tui/src/bottom_pane/approval_overlay.rs @@ -1853,7 +1853,7 @@ mod tests { } #[test] - fn patch_options_show_expected_shortcuts() { + fn patch_reject_shortcuts_open_notes_and_show_expected_labels() { let (tx, _rx) = unbounded_channel::(); let tx = AppEventSender::new(tx); let view = ApprovalOverlay::new(make_patch_request(), tx, Features::with_defaults()); @@ -1879,70 +1879,76 @@ mod tests { rendered.contains("esc to interrupt"), "patch modal should show interrupt hint under options: {rendered}" ); + + let assert_opens_notes = |events: &[KeyEvent]| { + let (tx, _rx) = unbounded_channel::(); + let tx = AppEventSender::new(tx); + let mut view = + ApprovalOverlay::new(make_patch_request(), tx, Features::with_defaults()); + for event in events { + view.handle_key_event(*event); + } + + let state = view.patch_state().expect("patch state"); + assert_eq!( + state.options_state.selected_idx, + Some(PATCH_REJECT_OPTION_INDEX) + ); + assert_eq!(state.focus, PatchFocus::Notes); + assert!(view.patch_notes_visible()); + assert!(!view.is_complete()); + }; + + assert_opens_notes(&[KeyEvent::from(KeyCode::Tab)]); + assert_opens_notes(&[KeyEvent::new(KeyCode::Char('n'), KeyModifiers::NONE)]); + assert_opens_notes(&[KeyEvent::new(KeyCode::Char('3'), KeyModifiers::NONE)]); + assert_opens_notes(&[ + KeyEvent::from(KeyCode::Down), + KeyEvent::from(KeyCode::Down), + KeyEvent::from(KeyCode::Enter), + ]); } #[test] - fn tab_opens_patch_notes() { - let (tx, _rx) = unbounded_channel::(); + fn patch_shortcuts_bind_expected_actions() { + let (tx, mut rx) = unbounded_channel::(); let tx = AppEventSender::new(tx); let mut view = ApprovalOverlay::new(make_patch_request(), tx, Features::with_defaults()); - view.handle_key_event(KeyEvent::from(KeyCode::Tab)); + view.handle_key_event(KeyEvent::new(KeyCode::Char('y'), KeyModifiers::NONE)); - let state = view.patch_state().expect("patch state"); - assert_eq!( - state.options_state.selected_idx, - Some(PATCH_REJECT_OPTION_INDEX) - ); - assert_eq!(state.focus, PatchFocus::Notes); - assert!(view.patch_notes_visible()); - } + let mut decision = None; + while let Ok(event) = rx.try_recv() { + if let AppEvent::SubmitThreadOp { + op: Op::PatchApproval { decision: d, .. }, + .. + } = event + { + decision = Some(d); + break; + } + } + assert_eq!(decision, Some(ReviewDecision::Approved)); - #[test] - fn enter_on_patch_option_three_opens_notes() { - let (tx, _rx) = unbounded_channel::(); + let (tx, mut rx) = unbounded_channel::(); let tx = AppEventSender::new(tx); let mut view = ApprovalOverlay::new(make_patch_request(), tx, Features::with_defaults()); - view.handle_key_event(KeyEvent::from(KeyCode::Down)); - view.handle_key_event(KeyEvent::from(KeyCode::Down)); - view.handle_key_event(KeyEvent::from(KeyCode::Enter)); + view.handle_key_event(KeyEvent::new(KeyCode::Char('a'), KeyModifiers::NONE)); - let state = view.patch_state().expect("patch state"); - assert_eq!( - state.options_state.selected_idx, - Some(PATCH_REJECT_OPTION_INDEX) - ); - assert_eq!(state.focus, PatchFocus::Notes); - assert!(!view.is_complete()); - } + let mut decision = None; + while let Ok(event) = rx.try_recv() { + if let AppEvent::SubmitThreadOp { + op: Op::PatchApproval { decision: d, .. }, + .. + } = event + { + decision = Some(d); + break; + } + } + assert_eq!(decision, Some(ReviewDecision::ApprovedForSession)); - #[test] - fn n_opens_patch_notes() { - let (tx, _rx) = unbounded_channel::(); - let tx = AppEventSender::new(tx); - let mut view = ApprovalOverlay::new(make_patch_request(), tx, Features::with_defaults()); - - view.handle_key_event(KeyEvent::new(KeyCode::Char('n'), KeyModifiers::NONE)); - - assert!(view.patch_focus_is_notes()); - assert_eq!(view.patch_selected_index(), Some(PATCH_REJECT_OPTION_INDEX)); - } - - #[test] - fn digit_three_opens_patch_notes() { - let (tx, _rx) = unbounded_channel::(); - let tx = AppEventSender::new(tx); - let mut view = ApprovalOverlay::new(make_patch_request(), tx, Features::with_defaults()); - - view.handle_key_event(KeyEvent::new(KeyCode::Char('3'), KeyModifiers::NONE)); - - assert!(view.patch_focus_is_notes()); - assert_eq!(view.patch_selected_index(), Some(PATCH_REJECT_OPTION_INDEX)); - } - - #[test] - fn ctrl_c_aborts_patch_even_when_notes_are_visible() { let (tx, mut rx) = unbounded_channel::(); let tx = AppEventSender::new(tx); let mut view = ApprovalOverlay::new(make_patch_request(), tx, Features::with_defaults()); @@ -1965,32 +1971,7 @@ mod tests { } #[test] - fn submitting_patch_notes_emits_reject_with_notes_event() { - let (tx, mut rx) = unbounded_channel::(); - let tx = AppEventSender::new(tx); - let mut view = ApprovalOverlay::new(make_patch_request(), tx, Features::with_defaults()); - view.handle_key_event(KeyEvent::from(KeyCode::Tab)); - view.patch_state_mut() - .expect("patch state") - .composer - .set_text_content("use smaller diffs".to_string(), Vec::new(), Vec::new()); - - view.handle_key_event(KeyEvent::from(KeyCode::Enter)); - - let event = rx.try_recv().expect("reject event"); - assert_eq!( - matches!( - event, - AppEvent::RejectPatchApprovalWithNotes { text, .. } - if text == "use smaller diffs" - ), - true - ); - assert!(view.is_complete()); - } - - #[test] - fn empty_patch_note_submit_does_not_emit_event() { + fn patch_notes_validate_before_submit_and_emit_event() { let (tx, mut rx) = unbounded_channel::(); let tx = AppEventSender::new(tx); let mut view = ApprovalOverlay::new(make_patch_request(), tx, Features::with_defaults()); @@ -2004,62 +1985,23 @@ mod tests { ); assert!(!view.is_complete()); assert!(view.patch_note_error_visible()); - } - #[test] - fn patch_yes_shortcuts_remain_unchanged() { - let (tx, mut rx) = unbounded_channel::(); - let tx = AppEventSender::new(tx); - let mut view = ApprovalOverlay::new(make_patch_request(), tx, Features::with_defaults()); - - view.handle_key_event(KeyEvent::new(KeyCode::Char('a'), KeyModifiers::NONE)); - - let mut decision = None; - while let Ok(event) = rx.try_recv() { - if let AppEvent::SubmitThreadOp { - op: Op::PatchApproval { decision: d, .. }, - .. - } = event - { - decision = Some(d); - break; - } - } - assert_eq!(decision, Some(ReviewDecision::ApprovedForSession)); - } - - #[test] - fn patch_notes_snapshot() { - let (tx, _rx) = unbounded_channel::(); - let tx = AppEventSender::new(tx); - let mut view = ApprovalOverlay::new(make_patch_request(), tx, Features::with_defaults()); - view.handle_key_event(KeyEvent::from(KeyCode::Tab)); view.patch_state_mut() .expect("patch state") .composer - .set_text_content( - "split the changes by file".to_string(), - Vec::new(), - Vec::new(), - ); + .set_text_content("use smaller diffs".to_string(), Vec::new(), Vec::new()); - assert_snapshot!( - "approval_overlay_patch_notes_visible", - render_overlay_lines(&view, 80) - ); - } - - #[test] - fn patch_reject_selected_snapshot() { - let (tx, _rx) = unbounded_channel::(); - let tx = AppEventSender::new(tx); - let mut view = ApprovalOverlay::new(make_patch_request(), tx, Features::with_defaults()); - view.handle_key_event(KeyEvent::from(KeyCode::Down)); - view.handle_key_event(KeyEvent::from(KeyCode::Down)); - - assert_snapshot!( - "approval_overlay_patch_reject_selected", - render_overlay_lines(&view, 80) + view.handle_key_event(KeyEvent::from(KeyCode::Enter)); + + let event = rx.try_recv().expect("reject event"); + assert!( + matches!( + event, + AppEvent::RejectPatchApprovalWithNotes { text, .. } + if text == "use smaller diffs" + ), + "expected reject-with-notes event" ); + assert!(view.is_complete()); } } diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__approval_overlay__tests__approval_overlay_patch_notes_visible.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__approval_overlay__tests__approval_overlay_patch_notes_visible.snap deleted file mode 100644 index 56a2b19506..0000000000 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__approval_overlay__tests__approval_overlay_patch_notes_visible.snap +++ /dev/null @@ -1,21 +0,0 @@ ---- -source: tui/src/bottom_pane/approval_overlay.rs -assertion_line: 1995 -expression: "render_overlay_lines(&view, 80)" ---- - - Would you like to make the following edits? - - Reason: review these edits - - README.md (+2 -0) - - 1 +hello - 2 +world - - 1. Yes, proceed (y) - 2. Yes, and don't ask again for these files (a) - › 3. No, and tell Codex what to do differently (tab) - enter to send | tab to go back | esc to interrupt - - › split the changes by file diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__approval_overlay__tests__approval_overlay_patch_reject_selected.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__approval_overlay__tests__approval_overlay_patch_reject_selected.snap deleted file mode 100644 index b90be3a204..0000000000 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__approval_overlay__tests__approval_overlay_patch_reject_selected.snap +++ /dev/null @@ -1,19 +0,0 @@ ---- -source: tui/src/bottom_pane/approval_overlay.rs -assertion_line: 2009 -expression: "render_overlay_lines(&view, 80)" ---- - - Would you like to make the following edits? - - Reason: review these edits - - README.md (+2 -0) - - 1 +hello - 2 +world - - 1. Yes, proceed (y) - 2. Yes, and don't ask again for these files (a) - › 3. No, and tell Codex what to do differently (tab) - tab to follow up | esc to interrupt diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index b048e4d521..fcf312031d 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -7730,7 +7730,7 @@ async fn apply_patch_approval_sends_op_with_call_id() { } #[tokio::test] -async fn apply_patch_reject_with_notes_emits_steering_event_without_interrupt_banner() { +async fn apply_patch_notes_choose_between_soft_reject_and_abort() { let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(None).await; let mut changes = HashMap::new(); changes.insert( @@ -7800,10 +7800,7 @@ async fn apply_patch_reject_with_notes_emits_steering_event_without_interrupt_ba !saw_interruption_banner, "soft reject should not show the interruption banner" ); -} -#[tokio::test] -async fn apply_patch_escape_from_notes_still_aborts() { let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(None).await; let mut changes = HashMap::new(); changes.insert(