From c24437a2d9a8de1d1372351fca7587ed84e62467 Mon Sep 17 00:00:00 2001 From: Roy Han Date: Mon, 2 Mar 2026 16:21:36 -0800 Subject: [PATCH] initial draft --- codex-rs/tui/src/app.rs | 225 ++++- codex-rs/tui/src/app_event.rs | 7 + .../tui/src/bottom_pane/approval_overlay.rs | 844 +++++++++++++++++- ..._approval_overlay_patch_notes_visible.snap | 21 + ...pproval_overlay_patch_reject_selected.snap | 19 + ...atwidget__tests__approval_modal_patch.snap | 10 +- ...et__tests__approval_modal_patch_notes.snap | 22 + codex-rs/tui/src/chatwidget/tests.rs | 160 ++++ 8 files changed, 1283 insertions(+), 25 deletions(-) create mode 100644 codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__approval_overlay__tests__approval_overlay_patch_notes_visible.snap create mode 100644 codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__approval_overlay__tests__approval_overlay_patch_reject_selected.snap create mode 100644 codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_patch_notes.snap diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index 742a1c9778..b37aa359e6 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -72,11 +72,13 @@ use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::FinalOutput; use codex_protocol::protocol::ListSkillsResponseEvent; use codex_protocol::protocol::Op; +use codex_protocol::protocol::ReviewDecision; use codex_protocol::protocol::SandboxPolicy; use codex_protocol::protocol::SessionConfiguredEvent; use codex_protocol::protocol::SessionSource; use codex_protocol::protocol::SkillErrorInfo; use codex_protocol::protocol::TokenUsage; +use codex_protocol::user_input::UserInput; use codex_utils_absolute_path::AbsolutePathBuf; use color_eyre::eyre::Result; use color_eyre::eyre::WrapErr; @@ -1040,7 +1042,7 @@ impl App { } } - async fn submit_op_to_thread(&mut self, thread_id: ThreadId, op: Op) { + async fn submit_op_to_thread(&mut self, thread_id: ThreadId, op: Op) -> bool { let replay_state_op = ThreadEventStore::op_can_change_pending_replay_state(&op).then(|| op.clone()); let submitted = if self.active_thread_id == Some(thread_id) { @@ -1069,6 +1071,7 @@ impl App { self.note_thread_outbound_op(thread_id, op).await; self.refresh_pending_thread_approvals().await; } + submitted } async fn refresh_pending_thread_approvals(&mut self) { @@ -2157,7 +2160,36 @@ impl App { } } AppEvent::SubmitThreadOp { thread_id, op } => { - self.submit_op_to_thread(thread_id, op).await; + let _ = self.submit_op_to_thread(thread_id, op).await; + } + AppEvent::RejectPatchApprovalWithNotes { + thread_id, + approval_id, + text, + } => { + let denied = self + .submit_op_to_thread( + thread_id, + Op::PatchApproval { + id: approval_id, + decision: ReviewDecision::Denied, + }, + ) + .await; + if denied { + let _ = self + .submit_op_to_thread( + thread_id, + Op::UserInput { + items: vec![UserInput::Text { + text, + text_elements: Vec::new(), + }], + final_output_json_schema: None, + }, + ) + .await; + } } AppEvent::DiffResult(text) => { // Clear the in-progress state in the bottom pane @@ -3608,9 +3640,12 @@ mod tests { use codex_otel::OtelManager; use codex_protocol::ThreadId; use codex_protocol::openai_models::ModelAvailabilityNux; + use codex_protocol::protocol::ApplyPatchApprovalRequestEvent; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::Event; use codex_protocol::protocol::EventMsg; + use codex_protocol::protocol::FileChange; + use codex_protocol::protocol::ReviewDecision; use codex_protocol::protocol::SandboxPolicy; use codex_protocol::protocol::SessionConfiguredEvent; use codex_protocol::protocol::SessionSource; @@ -3812,6 +3847,161 @@ mod tests { panic!("expected approval action to submit a thread-scoped op"); } + #[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; + let thread_id = ThreadId::new(); + app.active_thread_id = Some(thread_id); + + reject_patch_with_notes_for_test( + &mut app, + thread_id, + "call-1", + "please split this into smaller patches", + ) + .await; + + assert_eq!( + op_rx.try_recv(), + Ok(Op::PatchApproval { + id: "call-1".to_string(), + decision: ReviewDecision::Denied, + }) + ); + assert_eq!( + op_rx.try_recv(), + Ok(Op::UserInput { + items: vec![UserInput::Text { + text: "please split this into smaller patches".to_string(), + text_elements: Vec::new(), + }], + final_output_json_schema: None, + }) + ); + Ok(()) + } + + #[tokio::test] + async fn reject_patch_with_notes_skips_steering_when_deny_fails() -> Result<()> { + let (mut app, mut app_event_rx, _op_rx) = make_test_app_with_channels().await; + + reject_patch_with_notes_for_test( + &mut app, + ThreadId::new(), + "call-missing", + "try again with fewer files", + ) + .await; + + let mut errors = Vec::new(); + while let Ok(event) = app_event_rx.try_recv() { + if let AppEvent::InsertHistoryCell(cell) = event { + let text = cell + .display_lines(120) + .into_iter() + .map(|line| { + line.spans + .into_iter() + .map(|span| span.content.into_owned()) + .collect::() + }) + .collect::>() + .join("\n"); + errors.push(text); + } + } + + assert_eq!(errors.len(), 1); + assert!(errors[0].contains("Failed to find thread")); + Ok(()) + } + + #[tokio::test] + async fn reject_patch_with_notes_routes_denial_to_originating_thread() -> Result<()> { + let (mut app, _app_event_rx, mut op_rx) = make_test_app_with_channels().await; + let active = app.server.start_thread(app.config.clone()).await?; + let target = app.server.start_thread(app.config.clone()).await?; + app.active_thread_id = Some(active.thread_id); + + let active_channel = ThreadEventChannel::new(THREAD_EVENT_CHANNEL_CAPACITY); + { + let mut store = active_channel.store.lock().await; + store.push_event(Event { + id: "active-approval".to_string(), + msg: EventMsg::ApplyPatchApprovalRequest(ApplyPatchApprovalRequestEvent { + call_id: "active-call".to_string(), + turn_id: "active-turn".to_string(), + changes: HashMap::from([( + PathBuf::from("active.rs"), + FileChange::Add { + content: "fn active() {}\n".to_string(), + }, + )]), + reason: None, + grant_root: None, + }), + }); + } + app.thread_event_channels + .insert(active.thread_id, active_channel); + + let target_channel = ThreadEventChannel::new(THREAD_EVENT_CHANNEL_CAPACITY); + { + let mut store = target_channel.store.lock().await; + store.push_event(Event { + id: "target-approval".to_string(), + msg: EventMsg::ApplyPatchApprovalRequest(ApplyPatchApprovalRequestEvent { + call_id: "target-call".to_string(), + turn_id: "target-turn".to_string(), + changes: HashMap::from([( + PathBuf::from("target.rs"), + FileChange::Add { + content: "fn target() {}\n".to_string(), + }, + )]), + reason: None, + grant_root: None, + }), + }); + } + app.thread_event_channels + .insert(target.thread_id, target_channel); + + reject_patch_with_notes_for_test( + &mut app, + target.thread_id, + "target-call", + "apply only the target file", + ) + .await; + + assert!( + op_rx.try_recv().is_err(), + "cross-thread steering should not route through the active thread sender" + ); + + let active_pending = app + .thread_event_channels + .get(&active.thread_id) + .expect("active channel") + .store + .lock() + .await + .has_pending_thread_approvals(); + let target_pending = app + .thread_event_channels + .get(&target.thread_id) + .expect("target channel") + .store + .lock() + .await + .has_pending_thread_approvals(); + + assert!(active_pending); + assert!(!target_pending); + Ok(()) + } + #[tokio::test] async fn routed_thread_event_does_not_recreate_channel_after_reset() -> Result<()> { let mut app = make_test_app().await; @@ -4456,6 +4646,37 @@ mod tests { ) } + async fn reject_patch_with_notes_for_test( + app: &mut App, + thread_id: ThreadId, + approval_id: &str, + text: &str, + ) { + let denied = app + .submit_op_to_thread( + thread_id, + Op::PatchApproval { + id: approval_id.to_string(), + decision: ReviewDecision::Denied, + }, + ) + .await; + if denied { + let _ = app + .submit_op_to_thread( + thread_id, + Op::UserInput { + items: vec![UserInput::Text { + text: text.to_string(), + text_elements: Vec::new(), + }], + final_output_json_schema: None, + }, + ) + .await; + } + } + fn test_otel_manager(config: &Config, model: &str) -> OtelManager { let model_info = codex_core::test_support::construct_model_info_offline(model, config); OtelManager::new( diff --git a/codex-rs/tui/src/app_event.rs b/codex-rs/tui/src/app_event.rs index e769764c91..45fa55e41b 100644 --- a/codex-rs/tui/src/app_event.rs +++ b/codex-rs/tui/src/app_event.rs @@ -79,6 +79,13 @@ pub(crate) enum AppEvent { op: codex_protocol::protocol::Op, }, + /// Reject a patch approval without interrupting the turn, then steer the same thread. + RejectPatchApprovalWithNotes { + thread_id: ThreadId, + approval_id: String, + text: String, + }, + /// Forward an event from a non-primary thread into the app-level thread router. ThreadEvent { thread_id: ThreadId, diff --git a/codex-rs/tui/src/bottom_pane/approval_overlay.rs b/codex-rs/tui/src/bottom_pane/approval_overlay.rs index a9ef2cd38a..9a6a2b9e1c 100644 --- a/codex-rs/tui/src/bottom_pane/approval_overlay.rs +++ b/codex-rs/tui/src/bottom_pane/approval_overlay.rs @@ -1,3 +1,4 @@ +use std::borrow::Cow; use std::collections::HashMap; use std::path::PathBuf; @@ -5,9 +6,19 @@ use crate::app_event::AppEvent; use crate::app_event_sender::AppEventSender; use crate::bottom_pane::BottomPaneView; use crate::bottom_pane::CancellationEvent; +use crate::bottom_pane::ChatComposer; +use crate::bottom_pane::ChatComposerConfig; use crate::bottom_pane::list_selection_view::ListSelectionView; use crate::bottom_pane::list_selection_view::SelectionItem; use crate::bottom_pane::list_selection_view::SelectionViewParams; +use crate::bottom_pane::scroll_state::ScrollState; +use crate::bottom_pane::selection_popup_common::GenericDisplayRow; +use crate::bottom_pane::selection_popup_common::measure_rows_height; +use crate::bottom_pane::selection_popup_common::menu_surface_inset; +use crate::bottom_pane::selection_popup_common::menu_surface_padding_height; +use crate::bottom_pane::selection_popup_common::render_menu_surface; +use crate::bottom_pane::selection_popup_common::render_rows; +use crate::bottom_pane::selection_popup_common::wrap_styled_line; use crate::diff_render::DiffSummary; use crate::exec_command::strip_bash_lc_and_escape; use crate::history_cell; @@ -36,7 +47,45 @@ use ratatui::style::Stylize; use ratatui::text::Line; use ratatui::text::Span; use ratatui::widgets::Paragraph; +use ratatui::widgets::Widget; use ratatui::widgets::Wrap; +use unicode_width::UnicodeWidthStr; + +const PATCH_REJECT_OPTION_INDEX: usize = 2; +const PATCH_NOTES_PLACEHOLDER: &str = "Tell Codex what to do differently"; +const PATCH_EMPTY_NOTES_MESSAGE: &str = "Add guidance before sending."; +const PATCH_MIN_OVERLAY_HEIGHT: u16 = 8; +const PATCH_MIN_COMPOSER_HEIGHT: u16 = 3; +const PATCH_MAX_COMPOSER_HEIGHT: u16 = 8; + +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +enum PatchFocus { + Options, + Notes, +} + +struct PatchOverlayState { + focus: PatchFocus, + options_state: ScrollState, + composer: ChatComposer, + notes_visible: bool, + note_submit_attempted: bool, +} + +fn line_to_owned(line: Line<'_>) -> Line<'static> { + Line { + style: line.style, + alignment: line.alignment, + spans: line + .spans + .into_iter() + .map(|span| Span { + style: span.style, + content: Cow::Owned(span.content.into_owned()), + }) + .collect(), + } +} /// Request coming from the agent that needs user approval. #[derive(Clone, Debug)] @@ -93,6 +142,7 @@ pub(crate) struct ApprovalOverlay { app_event_tx: AppEventSender, list: ListSelectionView, options: Vec, + patch_state: Option, current_complete: bool, done: bool, features: Features, @@ -106,6 +156,7 @@ impl ApprovalOverlay { app_event_tx: app_event_tx.clone(), list: ListSelectionView::new(Default::default(), app_event_tx), options: Vec::new(), + patch_state: None, current_complete: false, done: false, features, @@ -122,11 +173,35 @@ impl ApprovalOverlay { self.current_complete = false; let header = build_header(&request); let (options, params) = Self::build_options(&request, header, &self.features); + self.patch_state = + matches!(request, ApprovalRequest::ApplyPatch { .. }).then(|| self.new_patch_state()); self.current_request = Some(request); self.options = options; self.list = ListSelectionView::new(params, self.app_event_tx.clone()); } + fn new_patch_state(&self) -> PatchOverlayState { + let mut composer = ChatComposer::new_with_config( + true, + self.app_event_tx.clone(), + false, + PATCH_NOTES_PLACEHOLDER.to_string(), + false, + ChatComposerConfig::plain_text(), + ); + composer.set_footer_hint_override(Some(Vec::new())); + PatchOverlayState { + focus: PatchFocus::Options, + options_state: ScrollState { + selected_idx: Some(0), + ..Default::default() + }, + composer, + notes_visible: false, + note_submit_attempted: false, + } + } + fn build_options( request: &ApprovalRequest, header: Box, @@ -196,9 +271,13 @@ impl ApprovalOverlay { if self.current_complete { return; } - let Some(option) = self.options.get(actual_idx) else { + let Some(option) = self.options.get(actual_idx).cloned() else { return; }; + if matches!(option.decision, ApprovalDecision::PatchRejectWithNotes) { + self.open_patch_notes(); + return; + } if let Some(request) = self.current_request.as_ref() { match (request, &option.decision) { (ApprovalRequest::Exec { id, command, .. }, ApprovalDecision::Review(decision)) => { @@ -225,6 +304,165 @@ impl ApprovalOverlay { self.advance_queue(); } + fn patch_state(&self) -> Option<&PatchOverlayState> { + self.patch_state.as_ref() + } + + fn patch_state_mut(&mut self) -> Option<&mut PatchOverlayState> { + self.patch_state.as_mut() + } + + fn patch_selected_index(&self) -> Option { + self.patch_state() + .and_then(|state| state.options_state.selected_idx) + } + + fn patch_focus_is_notes(&self) -> bool { + self.patch_state() + .is_some_and(|state| matches!(state.focus, PatchFocus::Notes)) + } + + fn patch_note_text(&self) -> String { + self.patch_state() + .map(|state| state.composer.current_text_with_pending()) + .unwrap_or_default() + } + + fn patch_notes_visible(&self) -> bool { + let Some(state) = self.patch_state() else { + return false; + }; + state.options_state.selected_idx == Some(PATCH_REJECT_OPTION_INDEX) + && (state.notes_visible + || !state.composer.current_text_with_pending().trim().is_empty()) + } + + fn patch_note_error_visible(&self) -> bool { + self.patch_notes_visible() + && self + .patch_state() + .is_some_and(|state| state.note_submit_attempted) + && self.patch_note_text().trim().is_empty() + } + + fn patch_title_lines(&self, width: u16) -> Vec> { + let line = Line::from("Would you like to make the following edits?".bold()); + wrap_styled_line(&line, width.max(1)) + .into_iter() + .map(line_to_owned) + .collect() + } + + fn patch_option_rows(&self) -> Vec { + let selected_idx = self.patch_selected_index(); + self.options + .iter() + .enumerate() + .map(|(idx, option)| { + let prefix = if selected_idx == Some(idx) { + '›' + } else { + ' ' + }; + let prefix_label = format!("{prefix} {}. ", idx + 1); + GenericDisplayRow { + name: format!("{prefix_label}{}", option.label), + wrap_indent: Some(UnicodeWidthStr::width(prefix_label.as_str())), + ..Default::default() + } + }) + .collect() + } + + fn patch_hint_lines(&self, width: u16) -> Vec> { + let mut hint = if self.patch_notes_visible() { + if self.patch_focus_is_notes() { + "enter to send | tab to go back | esc to interrupt".to_string() + } else { + "enter or tab to edit rejection | esc to interrupt".to_string() + } + } else if self.patch_selected_index() == Some(PATCH_REJECT_OPTION_INDEX) { + "tab to explain rejection | esc to interrupt".to_string() + } else { + "esc to interrupt".to_string() + }; + if self + .current_request + .as_ref() + .and_then(ApprovalRequest::thread_label) + .is_some() + { + hint.push_str(" | o to open thread"); + } + let line = Line::from(hint).dim(); + wrap_styled_line(&line, width.max(1)) + .into_iter() + .map(line_to_owned) + .collect() + } + + fn patch_validation_lines(&self, width: u16) -> Vec> { + if !self.patch_note_error_visible() { + return Vec::new(); + } + let line = Line::from(PATCH_EMPTY_NOTES_MESSAGE).red(); + wrap_styled_line(&line, width.max(1)) + .into_iter() + .map(line_to_owned) + .collect() + } + + fn patch_notes_input_height(&self, width: u16) -> u16 { + self.patch_state() + .map(|state| { + state + .composer + .desired_height(width.max(1)) + .clamp(PATCH_MIN_COMPOSER_HEIGHT, PATCH_MAX_COMPOSER_HEIGHT) + }) + .unwrap_or(PATCH_MIN_COMPOSER_HEIGHT) + } + + fn patch_select_index(&mut self, idx: usize) { + let len = self.options.len(); + let idx = idx.min(len.saturating_sub(1)); + if let Some(state) = self.patch_state_mut() { + state.options_state.selected_idx = Some(idx); + if idx != PATCH_REJECT_OPTION_INDEX { + state.notes_visible = false; + } + state.note_submit_attempted = false; + } + } + + fn open_patch_notes(&mut self) { + if self.options.len() <= PATCH_REJECT_OPTION_INDEX { + return; + } + self.patch_select_index(PATCH_REJECT_OPTION_INDEX); + if let Some(state) = self.patch_state_mut() { + state.notes_visible = true; + state.focus = PatchFocus::Notes; + state.note_submit_attempted = false; + state.composer.move_cursor_to_end(); + } + } + + fn move_patch_selection(&mut self, move_down: bool) { + let options_len = self.options.len(); + if let Some(state) = self.patch_state_mut() { + if move_down { + state.options_state.move_down_wrap(options_len); + } else { + state.options_state.move_up_wrap(options_len); + } + if state.options_state.selected_idx != Some(PATCH_REJECT_OPTION_INDEX) { + state.notes_visible = false; + } + state.note_submit_attempted = false; + } + } + fn handle_exec_decision(&self, id: &str, command: &[String], decision: ReviewDecision) { let Some(request) = self.current_request.as_ref() else { return; @@ -292,7 +530,7 @@ impl ApprovalOverlay { } } - fn try_handle_shortcut(&mut self, key_event: &KeyEvent) -> bool { + fn try_handle_global_shortcut(&mut self, key_event: &KeyEvent) -> bool { match key_event { KeyEvent { kind: KeyEventKind::Press, @@ -325,25 +563,161 @@ impl ApprovalOverlay { false } } - e => { - if let Some(idx) = self - .options - .iter() - .position(|opt| opt.shortcuts().any(|s| s.is_press(*e))) - { - self.apply_selection(idx); - true - } else { - false + _ => false, + } + } + + fn try_handle_option_shortcut(&mut self, key_event: &KeyEvent) -> bool { + self.options + .iter() + .position(|opt| { + opt.shortcuts() + .any(|shortcut| shortcut.is_press(*key_event)) + }) + .map(|idx| { + self.apply_selection(idx); + }) + .is_some() + } + + fn handle_patch_key_event(&mut self, key_event: KeyEvent) { + if key_event.kind == KeyEventKind::Release { + return; + } + if self.done || self.current_complete { + return; + } + + if self.try_handle_global_shortcut(&key_event) { + return; + } + + match self.patch_state().map(|state| state.focus) { + Some(PatchFocus::Options) => match key_event { + KeyEvent { + code: KeyCode::Char('y'), + modifiers: KeyModifiers::NONE, + .. + } => self.apply_selection(0), + KeyEvent { + code: KeyCode::Char('a'), + modifiers: KeyModifiers::NONE, + .. + } => self.apply_selection(1), + KeyEvent { + code: KeyCode::Char('n'), + modifiers: KeyModifiers::NONE, + .. + } + | KeyEvent { + code: KeyCode::Tab, + modifiers: KeyModifiers::NONE, + .. + } => self.open_patch_notes(), + KeyEvent { + code: KeyCode::Char('1'), + modifiers: KeyModifiers::NONE, + .. + } => self.apply_selection(0), + KeyEvent { + code: KeyCode::Char('2'), + modifiers: KeyModifiers::NONE, + .. + } => self.apply_selection(1), + KeyEvent { + code: KeyCode::Char('3'), + modifiers: KeyModifiers::NONE, + .. + } => self.open_patch_notes(), + KeyEvent { + code: KeyCode::Up | KeyCode::Char('k'), + modifiers: KeyModifiers::NONE, + .. + } => self.move_patch_selection(false), + KeyEvent { + code: KeyCode::Down | KeyCode::Char('j'), + modifiers: KeyModifiers::NONE, + .. + } => self.move_patch_selection(true), + KeyEvent { + code: KeyCode::Enter, + modifiers: KeyModifiers::NONE, + .. + } => match self.patch_selected_index() { + Some(PATCH_REJECT_OPTION_INDEX) => self.open_patch_notes(), + Some(idx) => self.apply_selection(idx), + None => {} + }, + _ => {} + }, + Some(PatchFocus::Notes) => { + if matches!( + key_event, + KeyEvent { + code: KeyCode::Tab, + modifiers: KeyModifiers::NONE, + .. + } + ) { + if let Some(state) = self.patch_state_mut() { + state.focus = PatchFocus::Options; + state.note_submit_attempted = false; + } + return; + } + if matches!( + key_event, + KeyEvent { + code: KeyCode::Enter, + modifiers: KeyModifiers::NONE, + .. + } + ) { + let text = self.patch_note_text(); + if text.trim().is_empty() { + if let Some(state) = self.patch_state_mut() { + state.note_submit_attempted = true; + } + return; + } + if let Some(ApprovalRequest::ApplyPatch { thread_id, id, .. }) = + self.current_request.as_ref() + { + self.app_event_tx + .send(AppEvent::RejectPatchApprovalWithNotes { + thread_id: *thread_id, + approval_id: id.clone(), + text, + }); + self.current_complete = true; + self.advance_queue(); + } + return; + } + if let Some(state) = self.patch_state_mut() { + let _ = state.composer.handle_key_event(key_event); + if !state.composer.current_text_with_pending().trim().is_empty() { + state.note_submit_attempted = false; + } } } + None => {} } } } impl BottomPaneView for ApprovalOverlay { fn handle_key_event(&mut self, key_event: KeyEvent) { - if self.try_handle_shortcut(&key_event) { + if matches!( + self.current_request, + Some(ApprovalRequest::ApplyPatch { .. }) + ) { + self.handle_patch_key_event(key_event); + return; + } + if self.try_handle_global_shortcut(&key_event) + || self.try_handle_option_shortcut(&key_event) + { return; } self.list.handle_key_event(key_event); @@ -395,19 +769,235 @@ impl BottomPaneView for ApprovalOverlay { self.enqueue_request(request); None } + + fn handle_paste(&mut self, pasted: String) -> bool { + if pasted.is_empty() + || !matches!( + self.current_request, + Some(ApprovalRequest::ApplyPatch { .. }) + ) + { + return false; + } + self.open_patch_notes(); + self.patch_state_mut() + .map(|state| { + state.note_submit_attempted = false; + state.composer.handle_paste(pasted) + }) + .unwrap_or(false) + } + + fn flush_paste_burst_if_due(&mut self) -> bool { + self.patch_state_mut() + .map(|state| state.composer.flush_paste_burst_if_due()) + .unwrap_or(false) + } + + fn is_in_paste_burst(&self) -> bool { + self.patch_state() + .is_some_and(|state| state.composer.is_in_paste_burst()) + } } impl Renderable for ApprovalOverlay { fn desired_height(&self, width: u16) -> u16 { - self.list.desired_height(width) + let Some(request @ ApprovalRequest::ApplyPatch { .. }) = self.current_request.as_ref() + else { + return self.list.desired_height(width); + }; + + let outer = Rect::new(0, 0, width, u16::MAX); + let inner = menu_surface_inset(outer); + let inner_width = inner.width.max(1); + let title_height = self.patch_title_lines(inner_width).len() as u16; + let header = build_header(request); + let header_height = header.desired_height(inner_width); + let rows = self.patch_option_rows(); + let mut state = self + .patch_state() + .map(|patch_state| patch_state.options_state) + .unwrap_or_default(); + if state.selected_idx.is_none() { + state.selected_idx = Some(0); + } + let options_height = + measure_rows_height(&rows, &state, rows.len().max(1), inner_width.max(1)); + let hint_height = self.patch_hint_lines(inner_width).len() as u16; + let notes_height = if self.patch_notes_visible() { + self.patch_notes_input_height(inner_width) + } else { + 0 + }; + let validation_height = self.patch_validation_lines(inner_width).len() as u16; + let height = title_height + + 1 + + header_height + + 1 + + options_height + + hint_height + + notes_height + + validation_height + + menu_surface_padding_height(); + height.max(PATCH_MIN_OVERLAY_HEIGHT) } fn render(&self, area: Rect, buf: &mut Buffer) { - self.list.render(area, buf); + let Some(request @ ApprovalRequest::ApplyPatch { .. }) = self.current_request.as_ref() + else { + self.list.render(area, buf); + return; + }; + let content_area = render_menu_surface(area, buf); + if content_area.width == 0 || content_area.height == 0 { + return; + } + + let width = content_area.width.max(1); + let title_lines = self.patch_title_lines(width); + let header = build_header(request); + let header_height = header.desired_height(width); + let rows = self.patch_option_rows(); + let mut state = self + .patch_state() + .map(|patch_state| patch_state.options_state) + .unwrap_or_default(); + if state.selected_idx.is_none() { + state.selected_idx = Some(0); + } + let options_height = measure_rows_height(&rows, &state, rows.len().max(1), width.max(1)); + let hint_lines = self.patch_hint_lines(width); + let validation_lines = self.patch_validation_lines(width); + let validation_height = validation_lines.len() as u16; + let notes_height = if self.patch_notes_visible() { + self.patch_notes_input_height(width) + } else { + 0 + }; + + let mut cursor_y = content_area.y; + for line in title_lines { + Paragraph::new(line).render( + Rect { + x: content_area.x, + y: cursor_y, + width: content_area.width, + height: 1, + }, + buf, + ); + cursor_y = cursor_y.saturating_add(1); + } + cursor_y = cursor_y.saturating_add(1); + + header.render( + Rect { + x: content_area.x, + y: cursor_y, + width: content_area.width, + height: header_height.min( + content_area + .height + .saturating_sub(cursor_y - content_area.y), + ), + }, + buf, + ); + cursor_y = cursor_y.saturating_add(header_height).saturating_add(1); + + let options_area = Rect { + x: content_area.x, + y: cursor_y, + width: content_area.width, + height: options_height, + }; + render_rows( + options_area, + buf, + &rows, + &state, + rows.len().max(1), + "No options", + ); + cursor_y = cursor_y.saturating_add(options_height); + + for line in hint_lines { + Paragraph::new(line).render( + Rect { + x: content_area.x, + y: cursor_y, + width: content_area.width, + height: 1, + }, + buf, + ); + cursor_y = cursor_y.saturating_add(1); + } + + if self.patch_notes_visible() { + let notes_area = + Rect { + x: content_area.x, + y: cursor_y, + width: content_area.width, + height: notes_height.min(content_area.height.saturating_sub( + cursor_y.saturating_sub(content_area.y) + validation_height, + )), + }; + if let Some(state) = self.patch_state() { + state.composer.render(notes_area, buf); + } + cursor_y = cursor_y.saturating_add(notes_area.height); + } + + for line in validation_lines { + Paragraph::new(line).render( + Rect { + x: content_area.x, + y: cursor_y, + width: content_area.width, + height: 1, + }, + buf, + ); + cursor_y = cursor_y.saturating_add(1); + } } fn cursor_pos(&self, area: Rect) -> Option<(u16, u16)> { - self.list.cursor_pos(area) + if !self.patch_focus_is_notes() || !self.patch_notes_visible() { + return self.list.cursor_pos(area); + } + let content_area = menu_surface_inset(area); + if content_area.width == 0 || content_area.height == 0 { + return None; + } + let width = content_area.width.max(1); + let title_height = self.patch_title_lines(width).len() as u16; + let header_height = self + .current_request + .as_ref() + .map(build_header) + .map(|header| header.desired_height(width)) + .unwrap_or(0); + let rows = self.patch_option_rows(); + let mut state = self + .patch_state() + .map(|patch_state| patch_state.options_state) + .unwrap_or_default(); + if state.selected_idx.is_none() { + state.selected_idx = Some(0); + } + let options_height = measure_rows_height(&rows, &state, rows.len().max(1), width.max(1)); + let hint_height = self.patch_hint_lines(width).len() as u16; + let notes_area = Rect { + x: content_area.x, + y: content_area.y + title_height + 1 + header_height + 1 + options_height + hint_height, + width: content_area.width, + height: self.patch_notes_input_height(width), + }; + self.patch_state() + .and_then(|state| state.composer.cursor_pos(notes_area)) } } @@ -529,6 +1119,7 @@ fn build_header(request: &ApprovalRequest) -> Box { enum ApprovalDecision { Review(ReviewDecision), McpElicitation(ElicitationAction), + PatchRejectWithNotes, } #[derive(Clone)] @@ -683,8 +1274,8 @@ fn patch_options() -> Vec { }, ApprovalOption { label: "No, and tell Codex what to do differently".to_string(), - decision: ApprovalDecision::Review(ReviewDecision::Abort), - display_shortcut: Some(key_hint::plain(KeyCode::Esc)), + decision: ApprovalDecision::PatchRejectWithNotes, + display_shortcut: None, additional_shortcuts: vec![key_hint::plain(KeyCode::Char('n'))], }, ] @@ -770,6 +1361,24 @@ mod tests { } } + fn make_patch_request() -> ApprovalRequest { + let mut changes = HashMap::new(); + changes.insert( + PathBuf::from("README.md"), + FileChange::Add { + content: "hello\nworld\n".to_string(), + }, + ); + ApprovalRequest::ApplyPatch { + thread_id: ThreadId::new(), + thread_label: None, + id: "patch-test".to_string(), + reason: Some("review these edits".to_string()), + cwd: PathBuf::from("/tmp"), + changes, + } + } + #[test] fn ctrl_c_aborts_and_clears_queue() { let (tx, _rx) = unbounded_channel::(); @@ -1239,4 +1848,203 @@ mod tests { } assert_eq!(decision, Some(ReviewDecision::Approved)); } + + #[test] + fn patch_option_three_no_longer_shows_esc_shortcut() { + let (tx, _rx) = unbounded_channel::(); + let tx = AppEventSender::new(tx); + let view = ApprovalOverlay::new(make_patch_request(), tx, Features::with_defaults()); + let rendered = render_overlay_lines(&view, 80); + + assert!( + !rendered.contains("(esc)"), + "patch option should not show esc shortcut: {rendered}" + ); + assert!( + rendered.contains("esc to interrupt"), + "patch modal should show interrupt hint under options: {rendered}" + ); + } + + #[test] + fn tab_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::from(KeyCode::Tab)); + + 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()); + } + + #[test] + fn enter_on_patch_option_three_opens_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::from(KeyCode::Down)); + view.handle_key_event(KeyEvent::from(KeyCode::Down)); + view.handle_key_event(KeyEvent::from(KeyCode::Enter)); + + 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()); + } + + #[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()); + view.handle_key_event(KeyEvent::from(KeyCode::Tab)); + + assert_eq!(view.on_ctrl_c(), CancellationEvent::Handled); + + 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::Abort)); + } + + #[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() { + 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::from(KeyCode::Enter)); + + assert!( + rx.try_recv().is_err(), + "unexpected event for empty note submit" + ); + 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(), + ); + + 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) + ); + } } 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 new file mode 100644 index 0000000000..e0082e6cbd --- /dev/null +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__approval_overlay__tests__approval_overlay_patch_notes_visible.snap @@ -0,0 +1,21 @@ +--- +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 + 2. Yes, and don't ask again for these files + › 3. No, and tell Codex what to do differently + 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 new file mode 100644 index 0000000000..d3b32e85cf --- /dev/null +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__approval_overlay__tests__approval_overlay_patch_reject_selected.snap @@ -0,0 +1,19 @@ +--- +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 + 2. Yes, and don't ask again for these files + › 3. No, and tell Codex what to do differently + tab to explain rejection | esc to interrupt diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_patch.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_patch.snap index e394605dcc..c0f3f97456 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_patch.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_patch.snap @@ -1,5 +1,6 @@ --- source: tui/src/chatwidget/tests.rs +assertion_line: 6996 expression: terminal.backend().vt100().screen().contents() --- @@ -13,8 +14,7 @@ expression: terminal.backend().vt100().screen().contents() 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 (esc) - - Press enter to confirm or esc to cancel + › 1. Yes, proceed + 2. Yes, and don't ask again for these files + 3. No, and tell Codex what to do differently + esc to interrupt diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_patch_notes.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_patch_notes.snap new file mode 100644 index 0000000000..86c19b9948 --- /dev/null +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_patch_notes.snap @@ -0,0 +1,22 @@ +--- +source: tui/src/chatwidget/tests.rs +assertion_line: 7041 +expression: terminal.backend().vt100().screen().contents() +--- + + + Would you like to make the following edits? + + Reason: The model wants to apply changes + + README.md (+2 -0) + + 1 +hello + 2 +world + + 1. Yes, proceed + 2. Yes, and don't ask again for these files + › 3. No, and tell Codex what to do differently + enter to send | tab to go back | esc to interrupt + + › split the edits into smaller commits diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index d89ed4bf55..91a65d4546 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -7001,6 +7001,51 @@ async fn approval_modal_patch_snapshot() -> anyhow::Result<()> { Ok(()) } +#[tokio::test] +async fn approval_modal_patch_notes_snapshot() -> anyhow::Result<()> { + let (mut chat, _rx, _op_rx) = make_chatwidget_manual(None).await; + chat.config + .permissions + .approval_policy + .set(AskForApproval::OnRequest)?; + + let mut changes = HashMap::new(); + changes.insert( + PathBuf::from("README.md"), + FileChange::Add { + content: "hello\nworld\n".into(), + }, + ); + chat.handle_codex_event(Event { + id: "sub-approve-patch-notes".into(), + msg: EventMsg::ApplyPatchApprovalRequest(ApplyPatchApprovalRequestEvent { + call_id: "call-approve-patch-notes".into(), + turn_id: "turn-approve-patch-notes".into(), + changes, + reason: Some("The model wants to apply changes".into()), + grant_root: Some(PathBuf::from("/tmp")), + }), + }); + + chat.handle_key_event(KeyEvent::from(KeyCode::Tab)); + chat.bottom_pane + .handle_paste("split the edits into smaller commits".to_string()); + + let height = chat.desired_height(80); + let mut terminal = + ratatui::Terminal::new(VT100Backend::new(80, height)).expect("create terminal"); + terminal.set_viewport_area(Rect::new(0, 0, 80, height)); + terminal + .draw(|f| chat.render(f.area(), f.buffer_mut())) + .expect("draw patch approval notes modal"); + assert_snapshot!( + "approval_modal_patch_notes", + terminal.backend().vt100().screen().contents() + ); + + Ok(()) +} + #[tokio::test] async fn interrupt_restores_queued_messages_into_composer() { let (mut chat, mut rx, mut op_rx) = make_chatwidget_manual(None).await; @@ -7658,6 +7703,121 @@ async fn apply_patch_approval_sends_op_with_call_id() { assert!(found, "expected PatchApproval op to be sent"); } +#[tokio::test] +async fn apply_patch_reject_with_notes_emits_steering_event_without_interrupt_banner() { + let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(None).await; + let mut changes = HashMap::new(); + changes.insert( + PathBuf::from("file.rs"), + FileChange::Add { + content: "fn main(){}\n".into(), + }, + ); + chat.handle_codex_event(Event { + id: "sub-124".into(), + msg: EventMsg::ApplyPatchApprovalRequest(ApplyPatchApprovalRequestEvent { + call_id: "call-1000".into(), + turn_id: "turn-1000".into(), + changes, + reason: None, + grant_root: None, + }), + }); + + chat.handle_key_event(KeyEvent::from(KeyCode::Tab)); + chat.bottom_pane + .handle_paste("apply only the formatter changes".to_string()); + chat.handle_key_event(KeyEvent::from(KeyCode::Enter)); + + let mut saw_reject = false; + let mut saw_abort = false; + let mut saw_interruption_banner = false; + while let Ok(event) = rx.try_recv() { + match event { + AppEvent::RejectPatchApprovalWithNotes { text, .. } => { + assert_eq!(text, "apply only the formatter changes"); + saw_reject = true; + } + AppEvent::SubmitThreadOp { + op: + Op::PatchApproval { + decision: codex_protocol::protocol::ReviewDecision::Abort, + .. + }, + .. + } => { + saw_abort = true; + } + AppEvent::InsertHistoryCell(cell) => { + let rendered = cell + .display_lines(120) + .into_iter() + .map(|line| { + line.spans + .into_iter() + .map(|span| span.content.into_owned()) + .collect::() + }) + .collect::>() + .join("\n"); + if rendered.contains("Conversation interrupted") { + saw_interruption_banner = true; + } + } + _ => {} + } + } + + assert!(saw_reject, "expected reject-with-notes app event"); + assert!(!saw_abort, "soft reject should not abort the turn"); + assert!( + !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( + PathBuf::from("file.rs"), + FileChange::Add { + content: "fn main(){}\n".into(), + }, + ); + chat.handle_codex_event(Event { + id: "sub-125".into(), + msg: EventMsg::ApplyPatchApprovalRequest(ApplyPatchApprovalRequestEvent { + call_id: "call-1001".into(), + turn_id: "turn-1001".into(), + changes, + reason: None, + grant_root: None, + }), + }); + + chat.handle_key_event(KeyEvent::from(KeyCode::Tab)); + chat.bottom_pane + .handle_paste("this note should be discarded".to_string()); + chat.handle_key_event(KeyEvent::from(KeyCode::Esc)); + + let mut saw_abort = false; + while let Ok(event) = rx.try_recv() { + if let AppEvent::SubmitThreadOp { + op: Op::PatchApproval { decision, .. }, + .. + } = event + && decision == codex_protocol::protocol::ReviewDecision::Abort + { + saw_abort = true; + break; + } + } + + assert!(saw_abort, "esc should still abort the patch approval flow"); +} + #[tokio::test] async fn apply_patch_full_flow_integration_like() { let (mut chat, mut rx, mut op_rx) = make_chatwidget_manual(None).await;