From f752c97f7d0be0cbef5cece327f52a16ad6fb1c6 Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Fri, 30 Oct 2026 07:52:49 +0000 Subject: [PATCH] Allow inline patch review during approval prompts (#58281) ## Why Collapsed patch details should remain accessible while an approval prompt is open, and reviewing them should not leave the transcript stuck behind new output after approval. ## What changed Allow clicking transcript disclosure controls while a modal prompt is open. When expanding details during patch approval interrupts following the latest output, resume following once that approval ends or advances to another patch. Preserve manual scrolling and text selection instead of automatically jumping to the latest output. ## Testing Extend patch approval coverage with an inline expansion snapshot and checks for queued approvals and subsequent output. Add a regression test that preserves manual scroll position when approval ends. GitOrigin-RevId: 249aa6bb2a84364a8cb111454fa5b6a3dce218b3 --- codex-rs/tui/src/app.rs | 2 + codex-rs/tui/src/app/owned_transcript.rs | 12 ++-- .../tui/src/app/tests/patch_approval_tests.rs | 69 +++++++++++++++++-- .../tui/src/bottom_pane/approval_overlay.rs | 7 ++ .../tui/src/bottom_pane/bottom_pane_view.rs | 5 ++ codex-rs/tui/src/bottom_pane/mod.rs | 7 ++ codex-rs/tui/src/chatwidget/interaction.rs | 5 ++ ...tests__patch_approval_expanded_inline.snap | 28 ++++++++ ...roval_tests__patch_approval_pager_top.snap | 17 ++--- codex-rs/tui/src/transcript_view.rs | 2 + .../tui/src/transcript_view/disclosure.rs | 11 +++ .../tui/src/transcript_view/footer_tests.rs | 44 ++++++++++++ codex-rs/tui/src/transcript_view/input.rs | 27 +++++--- .../tui/src/transcript_view/input_tests.rs | 3 +- 14 files changed, 214 insertions(+), 25 deletions(-) create mode 100644 codex-rs/tui/src/snapshots/codex_tui__app__tests__patch_approval_tests__patch_approval_expanded_inline.snap diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index c442f48793..43ebfd2454 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -867,6 +867,8 @@ impl App { app_server: &mut AppServerSession, event: TuiEvent, ) -> Result { + self.transcript_view + .sync_patch_approval(self.chat_widget.current_patch_approval()); tui.link_hover.observe(&event); self.refresh_link_hover(tui)?; self.invalidate_right_click_paste(&event); diff --git a/codex-rs/tui/src/app/owned_transcript.rs b/codex-rs/tui/src/app/owned_transcript.rs index 76432f6ec9..fa4c42782f 100644 --- a/codex-rs/tui/src/app/owned_transcript.rs +++ b/codex-rs/tui/src/app/owned_transcript.rs @@ -395,7 +395,7 @@ impl App { Ok(bottom_area) } - /// Keep modal input ownership while allowing selection and copying in the visible transcript. + /// Keep modal input ownership while allowing selection, copying, and disclosure in the visible transcript. pub(super) fn handle_owned_transcript_event( &mut self, tui: &mut tui::Tui, @@ -670,9 +670,13 @@ impl App { TuiEvent::Key(key) => self .transcript_view .handle_key(*key, &self.transcript_cells), - TuiEvent::Mouse(mouse) if modal_transcript_mouse => self - .transcript_view - .handle_selection_mouse(*mouse, &self.transcript_cells), + TuiEvent::Mouse(mouse) if modal_transcript_mouse => { + self.transcript_view.handle_selection_mouse( + *mouse, + &self.transcript_cells, + self.chat_widget.current_patch_approval(), + ) + } TuiEvent::Mouse(mouse) => self .transcript_view .handle_mouse(*mouse, &self.transcript_cells), diff --git a/codex-rs/tui/src/app/tests/patch_approval_tests.rs b/codex-rs/tui/src/app/tests/patch_approval_tests.rs index c5dee23417..283be96d30 100644 --- a/codex-rs/tui/src/app/tests/patch_approval_tests.rs +++ b/codex-rs/tui/src/app/tests/patch_approval_tests.rs @@ -2,8 +2,14 @@ use super::*; use crate::app::app_server_requests::AppServerRequestResolution; +use crate::app::owned_transcript::tests::buffer_text; +use crate::chatwidget::tests::helpers::normalize_snapshot_paths; +use crate::custom_terminal::test_support::last_rendered_buffer; use codex_app_server_protocol::FileChangeApprovalDecision; use codex_app_server_protocol::PatchApplyStatus; +use crossterm::event::MouseButton; +use crossterm::event::MouseEvent; +use crossterm::event::MouseEventKind; use pretty_assertions::assert_eq; use ratatui::layout::Rect; use tokio::sync::mpsc::UnboundedReceiver; @@ -16,7 +22,7 @@ fn changes() -> Vec { FileUpdateChange { path: "a-added.txt".to_string(), kind: PatchChangeKind::Add, - diff: "alpha\nbeta\n".to_string(), + diff: "alpha\nbeta\ngamma\ndelta\n".to_string(), }, FileUpdateChange { path: "m-updated.txt".to_string(), @@ -36,7 +42,7 @@ fn expected_changes() -> HashMap { ( PathBuf::from("a-added.txt"), FileChange::Add { - content: "alpha\nbeta\n".to_string(), + content: "alpha\nbeta\ngamma\ndelta\n".to_string(), }, ), ( @@ -170,6 +176,42 @@ async fn active_patch_approval_pager_preserves_changes_and_accepts_once() -> Res enqueue_pending_patch(&mut app, thread_id).await?; drain_pending_patch(&mut app, &mut tui).await?; + tui.set_owned_screen(/*owned*/ true)?; + while let Ok(event) = rx.try_recv() { + if let AppEvent::InsertHistoryCell(cell) = event { + app.insert_history_cell(&mut tui, cell); + } + } + let size = tui.terminal.size()?; + app.render_owned_transcript(&mut tui, size)?; + let collapsed = buffer_text(last_rendered_buffer(&tui.terminal)); + assert!(!collapsed.contains("+delta")); + let (column, row) = collapsed + .lines() + .enumerate() + .find_map(|(row, line)| line.find("+ Show details").map(|column| (column, row))) + .expect("visible patch disclosure"); + for kind in [ + MouseEventKind::Down(MouseButton::Left), + MouseEventKind::Up(MouseButton::Left), + ] { + app.handle_tui_event( + &mut tui, + &mut app_server, + TuiEvent::Mouse(MouseEvent { + kind, + column: column as u16, + row: row as u16, + modifiers: KeyModifiers::NONE, + }), + ) + .await?; + } + app.render_owned_transcript(&mut tui, size)?; + let expanded = buffer_text(last_rendered_buffer(&tui.terminal)); + insta::with_settings!({snapshot_path => "../../snapshots"}, { + insta::assert_snapshot!("patch_approval_expanded_inline", normalize_snapshot_paths(expanded)); + }); let request = open_patch(&mut app, &mut rx); assert_eq!(request.changes, expected_changes()); assert_eq!(request.id, ITEM_ID); @@ -208,15 +250,34 @@ async fn active_patch_approval_pager_preserves_changes_and_accepts_once() -> Res app.pending_app_server_requests .contains_server_request(&self::request(thread_id)) ); - assert_eq!(open_patch(&mut app, &mut rx).changes, expected_changes()); + let mut next_patch = open_patch(&mut app, &mut rx); + assert_eq!(next_patch.changes, expected_changes()); + next_patch.id = "next-patch".into(); app.chat_widget - .handle_key_event(KeyEvent::new(KeyCode::Char('y'), KeyModifiers::NONE)); + .push_approval_request(ApprovalRequest::ApplyPatch(next_patch)); + app.handle_tui_event( + &mut tui, + &mut app_server, + TuiEvent::Key(KeyEvent::new(KeyCode::Char('y'), KeyModifiers::NONE)), + ) + .await?; assert_decision( &mut app, &mut rx, thread_id, FileChangeApprovalDecision::Accept, ); + assert_eq!(open_patch(&mut app, &mut rx).id, "next-patch"); + let mut output = vec!["more output".into(); usize::from(size.height)]; + output.push("after approval".into()); + app.insert_history_cell( + &mut tui, + Box::new(crate::history_cell::PlainHistoryCell::new(output)), + ); + app.handle_tui_event(&mut tui, &mut app_server, TuiEvent::Draw) + .await?; + assert!(buffer_text(last_rendered_buffer(&tui.terminal)).contains("after approval")); + tui.set_owned_screen(/*owned*/ false)?; Ok(()) } diff --git a/codex-rs/tui/src/bottom_pane/approval_overlay.rs b/codex-rs/tui/src/bottom_pane/approval_overlay.rs index 0fee868894..46fd8ef491 100644 --- a/codex-rs/tui/src/bottom_pane/approval_overlay.rs +++ b/codex-rs/tui/src/bottom_pane/approval_overlay.rs @@ -617,6 +617,13 @@ impl BottomPaneView for ApprovalOverlay { self.done } + fn current_patch_approval(&self) -> Option<&ApplyPatchApprovalRequest> { + let ApprovalRequest::ApplyPatch(patch) = self.current_request.as_ref()? else { + return None; + }; + Some(patch) + } + fn try_consume_approval_request( &mut self, request: ApprovalRequest, diff --git a/codex-rs/tui/src/bottom_pane/bottom_pane_view.rs b/codex-rs/tui/src/bottom_pane/bottom_pane_view.rs index 57a66255c5..de5e68d71d 100644 --- a/codex-rs/tui/src/bottom_pane/bottom_pane_view.rs +++ b/codex-rs/tui/src/bottom_pane/bottom_pane_view.rs @@ -182,6 +182,11 @@ pub(crate) trait BottomPaneView: Renderable { false } + /// The patch whose inline review should end when this prompt is answered. + fn current_patch_approval(&self) -> Option<&super::ApplyPatchApprovalRequest> { + None + } + /// Try to handle approval request; return the original value if not /// consumed. fn try_consume_approval_request( diff --git a/codex-rs/tui/src/bottom_pane/mod.rs b/codex-rs/tui/src/bottom_pane/mod.rs index d38dd154ac..aef7c535b1 100644 --- a/codex-rs/tui/src/bottom_pane/mod.rs +++ b/codex-rs/tui/src/bottom_pane/mod.rs @@ -752,6 +752,13 @@ impl BottomPane { self.view_stack.last().map(std::convert::AsRef::as_ref) } + pub(crate) fn current_patch_approval(&self) -> Option<&ApplyPatchApprovalRequest> { + self.view_stack + .iter() + .rev() + .find_map(|view| view.current_patch_approval()) + } + fn push_view(&mut self, view: Box) { self.key_chord_reset_requested = true; self.view_stack.push(view); diff --git a/codex-rs/tui/src/chatwidget/interaction.rs b/codex-rs/tui/src/chatwidget/interaction.rs index 94f4fe228d..617253a793 100644 --- a/codex-rs/tui/src/chatwidget/interaction.rs +++ b/codex-rs/tui/src/chatwidget/interaction.rs @@ -15,6 +15,11 @@ pub(crate) enum KeyEventAction { } impl ChatWidget { + pub(crate) fn current_patch_approval(&self) -> Option<(ThreadId, &str)> { + let patch = self.bottom_pane.current_patch_approval()?; + Some((patch.thread_id, &patch.id)) + } + pub(crate) fn clear_composer_selection(&mut self) { self.bottom_pane.clear_composer_selection(); } diff --git a/codex-rs/tui/src/snapshots/codex_tui__app__tests__patch_approval_tests__patch_approval_expanded_inline.snap b/codex-rs/tui/src/snapshots/codex_tui__app__tests__patch_approval_tests__patch_approval_expanded_inline.snap new file mode 100644 index 0000000000..596fac2327 --- /dev/null +++ b/codex-rs/tui/src/snapshots/codex_tui__app__tests__patch_approval_tests__patch_approval_expanded_inline.snap @@ -0,0 +1,28 @@ +--- +source: tui/src/app/tests/patch_approval_tests.rs +expression: normalize_snapshot_paths(expanded) +--- +• Edited 3 files (+5 -2) + └ a-added.txt (+4 -0) + 1 +alpha + 2 +beta + 3 +gamma + 4 +delta + + └ m-updated.txt (+1 -1) + 1 -old + + + Would you like to make the following edits? + + Description: Apply proposed file edits + Destination: /tmp/project/a-added.txt + Destination: /tmp/project/m-updated.txt + Destination: /tmp/project/z-deleted.txt + + +› 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 diff --git a/codex-rs/tui/src/snapshots/codex_tui__app__tests__patch_approval_tests__patch_approval_pager_top.snap b/codex-rs/tui/src/snapshots/codex_tui__app__tests__patch_approval_tests__patch_approval_pager_top.snap index 5f07ffbae4..e32daebd60 100644 --- a/codex-rs/tui/src/snapshots/codex_tui__app__tests__patch_approval_tests__patch_approval_pager_top.snap +++ b/codex-rs/tui/src/snapshots/codex_tui__app__tests__patch_approval_tests__patch_approval_pager_top.snap @@ -1,18 +1,17 @@ --- source: tui/src/app/tests/patch_approval_tests.rs -assertion_line: 184 expression: "format!(\"{first:?}\")" --- Buffer { area: Rect { x: 0, y: 0, width: 48, height: 11 }, content: [ "/ P A T C H / / / / / / / / / / / / / / / / / / ", - "a-added.txt (+2 -0) ", + "a-added.txt (+4 -0) ", " ", " 1 +alpha ", " 2 +beta ", - " ", - "m-updated.txt (+1 -1) ", + " 3 +gamma ", + " 4 +delta ", "─────────────────────────────────────────── 0% ─", " ↑/↓ to scroll · pgup/pgdn to page · home/end to", " q close ", @@ -31,10 +30,12 @@ Buffer { x: 2, y: 4, fg: Reset, bg: Reset, underline: Reset, modifier: DIM, x: 4, y: 4, fg: Green, bg: Reset, underline: Reset, modifier: NONE, x: 9, y: 4, fg: Reset, bg: Reset, underline: Reset, modifier: NONE, - x: 15, y: 6, fg: Green, bg: Reset, underline: Reset, modifier: NONE, - x: 17, y: 6, fg: Reset, bg: Reset, underline: Reset, modifier: NONE, - x: 18, y: 6, fg: Red, bg: Reset, underline: Reset, modifier: NONE, - x: 20, y: 6, fg: Reset, bg: Reset, underline: Reset, modifier: NONE, + x: 2, y: 5, fg: Reset, bg: Reset, underline: Reset, modifier: DIM, + x: 4, y: 5, fg: Green, bg: Reset, underline: Reset, modifier: NONE, + x: 10, y: 5, fg: Reset, bg: Reset, underline: Reset, modifier: NONE, + x: 2, y: 6, fg: Reset, bg: Reset, underline: Reset, modifier: DIM, + x: 4, y: 6, fg: Green, bg: Reset, underline: Reset, modifier: NONE, + x: 10, y: 6, fg: Reset, bg: Reset, underline: Reset, modifier: NONE, x: 0, y: 7, fg: Reset, bg: Reset, underline: Reset, modifier: DIM, x: 1, y: 8, fg: Reset, bg: Reset, underline: Reset, modifier: BOLD, x: 4, y: 8, fg: Reset, bg: Reset, underline: Reset, modifier: DIM, diff --git a/codex-rs/tui/src/transcript_view.rs b/codex-rs/tui/src/transcript_view.rs index 0f895886a6..9911b8c439 100644 --- a/codex-rs/tui/src/transcript_view.rs +++ b/codex-rs/tui/src/transcript_view.rs @@ -458,6 +458,7 @@ impl TranscriptView { } pub(crate) fn jump_to_latest(&mut self) { + self.disclosure.resume_after_approval = None; self.cancel_search(); self.cancel_beginning(); self.position = Position::Latest; @@ -471,6 +472,7 @@ impl TranscriptView { pub(crate) fn scroll(&mut self, cells: &[Arc], rows: isize) { if rows != 0 { + self.disclosure.resume_after_approval = None; self.last_click = None; self.cancel_beginning(); } diff --git a/codex-rs/tui/src/transcript_view/disclosure.rs b/codex-rs/tui/src/transcript_view/disclosure.rs index d22138770d..5429eaa7c2 100644 --- a/codex-rs/tui/src/transcript_view/disclosure.rs +++ b/codex-rs/tui/src/transcript_view/disclosure.rs @@ -8,6 +8,7 @@ use crate::bottom_pane::TranscriptFooter; use crate::key_hint::KeyBindingListExt; use crate::keymap::ListAction; use crate::keymap::RuntimeKeymap; +use codex_protocol::ThreadId; use crossterm::event::KeyEvent; use ratatui::style::Modifier; use ratatui::style::Style; @@ -17,6 +18,7 @@ use super::*; pub(super) struct Disclosure { pub(super) expanded: HashSet, pub(super) focused: Option>, + pub(super) resume_after_approval: Option<(ThreadId, String)>, pub(super) live_ids: Vec, pub(super) keymap: RuntimeKeymap, } @@ -26,6 +28,7 @@ impl Default for Disclosure { Self { expanded: HashSet::new(), focused: None, + resume_after_approval: None, live_ids: Vec::new(), keymap: RuntimeKeymap::defaults(), } @@ -39,6 +42,14 @@ impl Disclosure { } impl TranscriptView { + pub(crate) fn sync_patch_approval(&mut self, current: Option<(ThreadId, &str)>) { + if let Some((thread_id, id)) = &self.disclosure.resume_after_approval + && current != Some((*thread_id, id.as_str())) + { + self.jump_to_latest(); + } + } + pub(crate) fn is_activity_focused(&self) -> bool { self.disclosure.focused.is_some() && !self.detailed && self.mode == HistoryRenderMode::Rich } diff --git a/codex-rs/tui/src/transcript_view/footer_tests.rs b/codex-rs/tui/src/transcript_view/footer_tests.rs index 309403b832..60167612cd 100644 --- a/codex-rs/tui/src/transcript_view/footer_tests.rs +++ b/codex-rs/tui/src/transcript_view/footer_tests.rs @@ -435,6 +435,50 @@ impl HistoryCell for ActivityCell { } } +#[test] +fn patch_disclosure_preserves_manual_scrolling() { + let cells: Vec> = vec![ + Arc::new(PlainHistoryCell::new(vec!["older output".into(); 20])), + Arc::new(ActivityCell::default()), + Arc::new(PlainHistoryCell::new(vec!["tail".into()])), + ]; + let area = Rect::new( + /*x*/ 0, /*y*/ 0, /*width*/ 32, /*height*/ 10, + ); + for scroll_before_click in [false, true] { + let mut view = TranscriptView::default(); + let mut buf = Buffer::empty(area); + view.render(area, &mut buf, &cells); + if scroll_before_click { + view.scroll(&cells, /*rows*/ -1); + view.render(area, &mut buf, &cells); + } + let (column, row) = crate::transcript_view::tests::text(&buf) + .lines() + .enumerate() + .find_map(|(row, line)| line.find("+ Show details").map(|column| (column, row))) + .expect("activity disclosure"); + view.handle_selection_mouse( + MouseEvent { + kind: MouseEventKind::Down(MouseButton::Left), + column: column as u16, + row: row as u16, + modifiers: KeyModifiers::NONE, + }, + &cells, + Some((codex_protocol::ThreadId::new(), "patch")), + ); + assert!(view.is_activity_focused()); + if !scroll_before_click { + view.scroll(&cells, /*rows*/ -1); + } + let position = view.position; + view.sync_patch_approval(/*current*/ None); + assert_eq!(view.position, position); + assert!(!view.is_following()); + } +} + #[test] fn activity_focus_keeps_controls_without_passive_hints() { let mut cells: Vec> = Vec::new(); diff --git a/codex-rs/tui/src/transcript_view/input.rs b/codex-rs/tui/src/transcript_view/input.rs index 1ce29ba3af..59183c42ac 100644 --- a/codex-rs/tui/src/transcript_view/input.rs +++ b/codex-rs/tui/src/transcript_view/input.rs @@ -7,6 +7,7 @@ //! Wheel input accumulates fractional rows and discards the remainder when direction reverses. use crate::key_hint::KeyBindingListExt; +use codex_protocol::ThreadId; use crossterm::event::KeyCode; use crossterm::event::KeyEvent; use crossterm::event::KeyEventKind; @@ -29,9 +30,9 @@ pub(crate) enum ViewAction { } #[derive(Clone, Copy, PartialEq, Eq)] -enum MouseMode { +enum MouseMode<'a> { Interactive, - Selection, + Selection(Option<(ThreadId, &'a str)>), } /// Transcript jumps shared with input routing so legacy terminals preserve their Alt modifier. @@ -251,16 +252,17 @@ impl TranscriptView { cells: &[Arc], ) -> Option { if self.select_mode.is_some() { - return self.handle_selection_mouse(event, cells); + return self.handle_selection_mouse(event, cells, /*patch_approval*/ None); } self.handle_mouse_with_mode(event, cells, MouseMode::Interactive) } - /// Handles selection and scrolling without activating transcript content. + /// Modals allow transcript disclosure, while select mode keeps clicks for text selection. pub(crate) fn handle_selection_mouse( &mut self, event: MouseEvent, cells: &[Arc], + patch_approval: Option<(ThreadId, &str)>, ) -> Option { let endpoint_changed = matches!( event.kind, @@ -272,12 +274,16 @@ impl TranscriptView { .selection .as_ref() .map(|selection| (selection.start, selection.end, selection.unit())); - let action = self.handle_mouse_with_mode(event, cells, MouseMode::Selection); + let action = + self.handle_mouse_with_mode(event, cells, MouseMode::Selection(patch_approval)); let after = self .selection .as_ref() .map(|selection| (selection.start, selection.end, selection.unit())); if endpoint_changed && before != after { + if self.has_selection_range() { + self.disclosure.resume_after_approval = None; + } self.adopt_select_mouse_range(); } action @@ -287,7 +293,7 @@ impl TranscriptView { &mut self, event: MouseEvent, cells: &[Arc], - mode: MouseMode, + mode: MouseMode<'_>, ) -> Option { if mode == MouseMode::Interactive && let Some(action) = self.handle_follow_control_mouse(event) @@ -498,12 +504,17 @@ impl TranscriptView { &mut self, event: MouseEvent, cells: &[Arc], - mode: MouseMode, + mode: MouseMode<'_>, ) -> Option { - if mode == MouseMode::Interactive + let was_following = self.is_following(); + if !self.is_select_mode() && event.modifiers.is_empty() && self.toggle_disclosure_at(cells, event.column, event.row) { + // Keep the patch anchored for inspection, then follow again when the prompt closes. + if was_following && let MouseMode::Selection(Some((thread_id, id))) = mode { + self.disclosure.resume_after_approval = Some((thread_id, id.to_owned())); + } return Some(ViewAction::Changed); } self.disclosure.focused = None; diff --git a/codex-rs/tui/src/transcript_view/input_tests.rs b/codex-rs/tui/src/transcript_view/input_tests.rs index 829240298e..802f45d962 100644 --- a/codex-rs/tui/src/transcript_view/input_tests.rs +++ b/codex-rs/tui/src/transcript_view/input_tests.rs @@ -319,7 +319,7 @@ fn selection_mouse_does_not_open_links() { ) }; assert!(matches!( - view.handle_selection_mouse(down, &cells), + view.handle_selection_mouse(down, &cells, /*patch_approval*/ None), Some(ViewAction::Changed) )); let area = view.area; @@ -331,6 +331,7 @@ fn selection_mouse_does_not_open_links() { ..down }, &cells, + /*patch_approval*/ None, ), Some(ViewAction::Changed) ));