From 173447fdd377b48c95597154725467f5bf3bf1c8 Mon Sep 17 00:00:00 2001 From: Roy Han Date: Mon, 2 Mar 2026 18:00:25 -0800 Subject: [PATCH] ui changes --- codex-rs/tmp/approval-smoke.txt | 1 + codex-rs/tui/src/app.rs | 152 +++++++++++++----- .../tui/src/bottom_pane/approval_overlay.rs | 2 +- codex-rs/tui/src/chatwidget.rs | 36 +++-- ...ply_patch_declined_accepting_revision.snap | 6 + codex-rs/tui/src/chatwidget/tests.rs | 26 +++ codex-rs/tui/src/history_cell.rs | 9 +- ...atch_with_notes_follow_up_user_prompt.snap | 7 + 8 files changed, 184 insertions(+), 55 deletions(-) create mode 100644 codex-rs/tmp/approval-smoke.txt create mode 100644 codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__apply_patch_declined_accepting_revision.snap create mode 100644 codex-rs/tui/src/snapshots/codex_tui__app__tests__reject_patch_with_notes_follow_up_user_prompt.snap diff --git a/codex-rs/tmp/approval-smoke.txt b/codex-rs/tmp/approval-smoke.txt new file mode 100644 index 0000000000..b023018cab --- /dev/null +++ b/codex-rs/tmp/approval-smoke.txt @@ -0,0 +1 @@ +bye diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index b37aa359e6..1a3542c729 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -1074,6 +1074,47 @@ impl App { submitted } + async fn reject_patch_approval_with_notes( + &mut self, + thread_id: ThreadId, + approval_id: String, + text: String, + ) { + let denied = self + .submit_op_to_thread( + thread_id, + Op::PatchApproval { + id: approval_id, + decision: ReviewDecision::Denied, + }, + ) + .await; + if !denied { + return; + } + + let steered = self + .submit_op_to_thread( + thread_id, + Op::UserInput { + items: vec![UserInput::Text { + text: text.clone(), + text_elements: Vec::new(), + }], + final_output_json_schema: None, + }, + ) + .await; + if steered && self.active_thread_id == Some(thread_id) { + self.chat_widget.render_submitted_user_message( + text, + Vec::new(), + Vec::new(), + Vec::new(), + ); + } + } + async fn refresh_pending_thread_approvals(&mut self) { let channels: Vec<(ThreadId, Arc>)> = self .thread_event_channels @@ -2167,29 +2208,8 @@ impl App { approval_id, text, } => { - let denied = self - .submit_op_to_thread( - thread_id, - Op::PatchApproval { - id: approval_id, - decision: ReviewDecision::Denied, - }, - ) + self.reject_patch_approval_with_notes(thread_id, approval_id, text) .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 @@ -3881,6 +3901,41 @@ mod tests { 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 + .transcript_cells + .iter() + .filter_map(|cell| { + cell.as_any() + .downcast_ref::() + .map(|cell| cell.message.clone()) + }) + .collect(); + assert_eq!( + user_messages, + vec!["i want the content to say bye instead.".to_string()] + ); + assert_snapshot!( + "reject_patch_with_notes_follow_up_user_prompt", + render_transcript_cells_for_snapshot(&app, 80) + ); + 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; @@ -4652,31 +4707,42 @@ mod tests { approval_id: &str, text: &str, ) { - let denied = app - .submit_op_to_thread( - thread_id, - Op::PatchApproval { - id: approval_id.to_string(), - decision: ReviewDecision::Denied, - }, - ) + app.reject_patch_approval_with_notes(thread_id, approval_id.to_string(), text.to_string()) .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 drain_insert_history_cells( + app: &mut App, + app_event_rx: &mut tokio::sync::mpsc::UnboundedReceiver, + ) { + while let Ok(event) = app_event_rx.try_recv() { + if let AppEvent::InsertHistoryCell(cell) = event { + app.transcript_cells.push(cell.into()); + } } } + fn render_transcript_cells_for_snapshot(app: &App, width: u16) -> String { + app.transcript_cells + .iter() + .enumerate() + .flat_map(|(idx, cell)| { + let mut lines = cell.display_lines(width); + if idx > 0 && !lines.is_empty() { + lines.insert(0, Line::from("")); + } + lines + }) + .map(|line| { + line.spans + .iter() + .map(|span| span.content.as_ref()) + .collect::() + }) + .collect::>() + .join("\n") + } + 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/bottom_pane/approval_overlay.rs b/codex-rs/tui/src/bottom_pane/approval_overlay.rs index 7a41a7af51..ce51b25832 100644 --- a/codex-rs/tui/src/bottom_pane/approval_overlay.rs +++ b/codex-rs/tui/src/bottom_pane/approval_overlay.rs @@ -379,7 +379,7 @@ impl ApprovalOverlay { 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() + "enter or tab to edit follow up | esc to interrupt".to_string() } } else if self.patch_selected_index() == Some(PATCH_REJECT_OPTION_INDEX) { "tab to follow up | esc to interrupt".to_string() diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 78e8fcc669..b887e1d97b 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -2571,10 +2571,14 @@ impl ChatWidget { &mut self, event: codex_protocol::protocol::PatchApplyEndEvent, ) { - // If the patch was successful, just let the "Edited" block stand. - // Otherwise, add a failure block. - if !event.success { - self.add_to_history(history_cell::new_patch_apply_failure(event.stderr)); + match event.status { + codex_protocol::protocol::PatchApplyStatus::Completed => {} + codex_protocol::protocol::PatchApplyStatus::Failed => { + self.add_to_history(history_cell::new_patch_apply_failure(event.stderr)); + } + codex_protocol::protocol::PatchApplyStatus::Declined => { + self.add_to_history(history_cell::new_patch_apply_declined()); + } } // Mark that actual work was done (patch applied) self.had_work_activity = true; @@ -4217,12 +4221,26 @@ impl ChatWidget { }); } - // Show replayable user content in conversation history. + let local_image_paths = local_images + .into_iter() + .map(|img| img.path) + .collect::>(); + self.render_submitted_user_message( + text, + text_elements, + local_image_paths, + remote_image_urls, + ); + } + + pub(crate) fn render_submitted_user_message( + &mut self, + text: String, + text_elements: Vec, + local_image_paths: Vec, + remote_image_urls: Vec, + ) { if !text.is_empty() { - let local_image_paths = local_images - .into_iter() - .map(|img| img.path) - .collect::>(); self.last_rendered_user_message_event = Some(Self::rendered_user_message_event_from_parts( text.clone(), diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__apply_patch_declined_accepting_revision.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__apply_patch_declined_accepting_revision.snap new file mode 100644 index 0000000000..2328a5a639 --- /dev/null +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__apply_patch_declined_accepting_revision.snap @@ -0,0 +1,6 @@ +--- +source: tui/src/chatwidget/tests.rs +assertion_line: 7580 +expression: rendered +--- +• Accepting revision diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index 91a65d4546..b048e4d521 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -7558,6 +7558,32 @@ async fn apply_patch_events_emit_history_cells() { ); } +#[tokio::test] +async fn apply_patch_declined_shows_accepting_revision_without_detail() { + let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(None).await; + + chat.handle_codex_event(Event { + id: "declined".into(), + msg: EventMsg::PatchApplyEnd(PatchApplyEndEvent { + call_id: "call-declined".into(), + turn_id: "turn-declined".into(), + stdout: String::new(), + stderr: "patch rejected by user".into(), + success: false, + changes: HashMap::new(), + status: CorePatchApplyStatus::Declined, + }), + }); + + let cells = drain_insert_history(&mut rx); + let rendered = lines_to_single_string(cells.last().expect("declined patch cell")); + assert_snapshot!("apply_patch_declined_accepting_revision", rendered); + assert!( + !rendered.contains("patch rejected by user"), + "declined patch row should not show the low-level rejection detail" + ); +} + #[tokio::test] async fn apply_patch_manual_approval_adjusts_header() { let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(None).await; diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index 14510c10ae..875b9cd47f 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -2162,8 +2162,7 @@ pub(crate) fn new_patch_event( pub(crate) fn new_patch_apply_failure(stderr: String) -> PlainHistoryCell { let mut lines: Vec> = Vec::new(); - // Failure title - lines.push(Line::from("✘ Failed to apply patch".magenta().bold())); + lines.push(vec!["• ".dim(), "Failed to apply patch".red().bold()].into()); if !stderr.trim().is_empty() { let output = output_lines( @@ -2185,6 +2184,12 @@ pub(crate) fn new_patch_apply_failure(stderr: String) -> PlainHistoryCell { PlainHistoryCell { lines } } +pub(crate) fn new_patch_apply_declined() -> PlainHistoryCell { + PlainHistoryCell { + lines: vec![vec!["• ".dim(), "Accepting revision".bold()].into()], + } +} + pub(crate) fn new_view_image_tool_call(path: PathBuf, cwd: &Path) -> PlainHistoryCell { let display_path = display_path_for(&path, cwd); diff --git a/codex-rs/tui/src/snapshots/codex_tui__app__tests__reject_patch_with_notes_follow_up_user_prompt.snap b/codex-rs/tui/src/snapshots/codex_tui__app__tests__reject_patch_with_notes_follow_up_user_prompt.snap new file mode 100644 index 0000000000..9f19529598 --- /dev/null +++ b/codex-rs/tui/src/snapshots/codex_tui__app__tests__reject_patch_with_notes_follow_up_user_prompt.snap @@ -0,0 +1,7 @@ +--- +source: tui/src/app.rs +assertion_line: 3932 +expression: "render_transcript_cells_for_snapshot(&app, 80)" +--- + +› i want the content to say bye instead.