mirror of
https://github.com/openai/codex.git
synced 2026-10-30 19:18:49 +00:00
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
This commit is contained in:
@@ -867,6 +867,8 @@ impl App {
|
||||
app_server: &mut AppServerSession,
|
||||
event: TuiEvent,
|
||||
) -> Result<AppRunControl> {
|
||||
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);
|
||||
|
||||
@@ -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),
|
||||
|
||||
@@ -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> {
|
||||
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, FileChange> {
|
||||
(
|
||||
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(())
|
||||
}
|
||||
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -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<dyn BottomPaneView>) {
|
||||
self.key_chord_reset_requested = true;
|
||||
self.view_stack.push(view);
|
||||
|
||||
@@ -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();
|
||||
}
|
||||
|
||||
+28
@@ -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
|
||||
+9
-8
@@ -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,
|
||||
|
||||
@@ -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<dyn HistoryCell>], rows: isize) {
|
||||
if rows != 0 {
|
||||
self.disclosure.resume_after_approval = None;
|
||||
self.last_click = None;
|
||||
self.cancel_beginning();
|
||||
}
|
||||
|
||||
@@ -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<String>,
|
||||
pub(super) focused: Option<Vec<String>>,
|
||||
pub(super) resume_after_approval: Option<(ThreadId, String)>,
|
||||
pub(super) live_ids: Vec<String>,
|
||||
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
|
||||
}
|
||||
|
||||
@@ -435,6 +435,50 @@ impl HistoryCell for ActivityCell {
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn patch_disclosure_preserves_manual_scrolling() {
|
||||
let cells: Vec<Arc<dyn HistoryCell>> = 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<Arc<dyn HistoryCell>> = Vec::new();
|
||||
|
||||
@@ -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<dyn HistoryCell>],
|
||||
) -> Option<ViewAction> {
|
||||
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<dyn HistoryCell>],
|
||||
patch_approval: Option<(ThreadId, &str)>,
|
||||
) -> Option<ViewAction> {
|
||||
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<dyn HistoryCell>],
|
||||
mode: MouseMode,
|
||||
mode: MouseMode<'_>,
|
||||
) -> Option<ViewAction> {
|
||||
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<dyn HistoryCell>],
|
||||
mode: MouseMode,
|
||||
mode: MouseMode<'_>,
|
||||
) -> Option<ViewAction> {
|
||||
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;
|
||||
|
||||
@@ -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)
|
||||
));
|
||||
|
||||
Reference in New Issue
Block a user