mirror of
https://github.com/openai/codex.git
synced 2026-11-13 14:46:14 +00:00
Show file destinations in TUI change approvals (#39285)
## What changed - Show a description and the destination paths affected by each file-change approval. - Include both source and target paths for moves, format cross-platform paths for display, and show `unavailable` when change details are missing. - Recover file changes from buffered events and thread snapshots so live and replayed approvals retain their destination details. ## Testing - Add coverage for active and replayed approvals, moved and cross-platform paths, and the missing-destination fallback. GitOrigin-RevId: 24f90b313fd8643f90cbead1504c788b140d7fed
This commit is contained in:
1 parent
ba37d0c45b
commit
e7f9fa9cd9
14 files changed
+411
-73
No files matched your search
@@ -0,0 +1,74 @@
|
||||
//! Render file-change approval descriptions and cross-platform destinations.
|
||||
|
||||
use super::approval_overlay::ApplyPatchApprovalRequest;
|
||||
use crate::diff_model::FileChange;
|
||||
use crate::render::renderable::Renderable;
|
||||
use codex_utils_path_uri::LegacyAppPathString;
|
||||
use ratatui::style::Stylize;
|
||||
use ratatui::text::Line;
|
||||
use ratatui::widgets::Paragraph;
|
||||
use ratatui::widgets::Wrap;
|
||||
|
||||
pub(super) fn build_header(request: &ApplyPatchApprovalRequest) -> Box<dyn Renderable> {
|
||||
let mut header = Vec::new();
|
||||
if let Some(thread_label) = &request.thread_label {
|
||||
header.push(Line::from(vec![
|
||||
"Thread: ".into(),
|
||||
thread_label.clone().bold(),
|
||||
]));
|
||||
}
|
||||
|
||||
if !header.is_empty() {
|
||||
header.push(Line::from(""));
|
||||
}
|
||||
|
||||
let description = request
|
||||
.reason
|
||||
.as_deref()
|
||||
.filter(|reason| !reason.is_empty())
|
||||
.unwrap_or("Apply proposed file edits");
|
||||
header.push(Line::from_iter([
|
||||
"Description: ".into(),
|
||||
description.to_string().italic(),
|
||||
]));
|
||||
|
||||
let mut destinations: Vec<_> = request
|
||||
.changes
|
||||
.iter()
|
||||
.flat_map(|(path, change)| {
|
||||
let move_destination = match change {
|
||||
FileChange::Update {
|
||||
move_path: Some(move_path),
|
||||
..
|
||||
} => Some(move_path),
|
||||
FileChange::Add { .. }
|
||||
| FileChange::Delete { .. }
|
||||
| FileChange::Update {
|
||||
move_path: None, ..
|
||||
} => None,
|
||||
};
|
||||
std::iter::once(path).chain(move_destination)
|
||||
})
|
||||
.map(|path| {
|
||||
LegacyAppPathString::from_path(path)
|
||||
.to_inferred_path_uri()
|
||||
.map(|path| path.inferred_native_path_string())
|
||||
.unwrap_or_else(|| request.cwd.join(path).display().to_string())
|
||||
})
|
||||
.collect();
|
||||
destinations.sort();
|
||||
destinations.dedup();
|
||||
if destinations.is_empty() {
|
||||
header.push(Line::from(vec![
|
||||
"Destination: ".into(),
|
||||
"unavailable".bold(),
|
||||
]));
|
||||
}
|
||||
for destination in destinations {
|
||||
header.push(Line::from_iter([
|
||||
"Destination: ".into(),
|
||||
destination.bold(),
|
||||
]));
|
||||
}
|
||||
Box::new(Paragraph::new(header).wrap(Wrap { trim: false }))
|
||||
}
|
||||
@@ -742,30 +742,7 @@ fn build_header(request: &ApprovalRequest) -> Box<dyn Renderable> {
|
||||
}
|
||||
Box::new(Paragraph::new(header).wrap(Wrap { trim: false }))
|
||||
}
|
||||
ApprovalRequest::ApplyPatch(request) => {
|
||||
let mut header: Vec<Box<dyn Renderable>> = Vec::new();
|
||||
if let Some(thread_label) = &request.thread_label {
|
||||
header.push(Box::new(Line::from(vec![
|
||||
"Thread: ".into(),
|
||||
thread_label.clone().bold(),
|
||||
])));
|
||||
}
|
||||
if let Some(reason) = &request.reason
|
||||
&& !reason.is_empty()
|
||||
{
|
||||
if !header.is_empty() {
|
||||
header.push(Box::new(Line::from("")));
|
||||
}
|
||||
header.push(Box::new(
|
||||
Paragraph::new(Line::from_iter([
|
||||
"Reason: ".into(),
|
||||
reason.clone().italic(),
|
||||
]))
|
||||
.wrap(Wrap { trim: false }),
|
||||
));
|
||||
}
|
||||
Box::new(ColumnRenderable::with(header))
|
||||
}
|
||||
ApprovalRequest::ApplyPatch(request) => super::apply_patch_header::build_header(request),
|
||||
ApprovalRequest::McpElicitation(request) => {
|
||||
let mut lines = Vec::new();
|
||||
if let Some(thread_label) = &request.thread_label {
|
||||
@@ -1124,6 +1101,8 @@ mod tests {
|
||||
use codex_protocol::models::FileSystemPermissions;
|
||||
use codex_protocol::models::NetworkPermissions;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use codex_utils_absolute_path::test_support::PathExt;
|
||||
use codex_utils_absolute_path::test_support::test_path_buf;
|
||||
use crossterm::event::KeyModifiers;
|
||||
use insta::assert_snapshot;
|
||||
use pretty_assertions::assert_eq;
|
||||
@@ -2128,12 +2107,30 @@ mod tests {
|
||||
content: "one\ntwo\nthree\n".to_string(),
|
||||
},
|
||||
);
|
||||
changes.insert(
|
||||
PathBuf::from("old.txt"),
|
||||
FileChange::Update {
|
||||
unified_diff: String::new(),
|
||||
move_path: Some(PathBuf::from("new.txt")),
|
||||
},
|
||||
);
|
||||
#[cfg(windows)]
|
||||
let (foreign_path, foreign_move_path) = ("/remote/old.txt", "/remote/new.txt");
|
||||
#[cfg(not(windows))]
|
||||
let (foreign_path, foreign_move_path) = (r"C:\workspace\old.txt", r"C:\workspace\new.txt");
|
||||
changes.insert(
|
||||
PathBuf::from(foreign_path),
|
||||
FileChange::Update {
|
||||
unified_diff: String::new(),
|
||||
move_path: Some(PathBuf::from(foreign_move_path)),
|
||||
},
|
||||
);
|
||||
let request = ApprovalRequest::ApplyPatch(ApplyPatchApprovalRequest {
|
||||
thread_id: ThreadId::new(),
|
||||
thread_label: Some("Banach [worker]".to_string()),
|
||||
id: "test".to_string(),
|
||||
reason: None,
|
||||
cwd: absolute_path("/tmp"),
|
||||
cwd: test_path_buf("/tmp").abs(),
|
||||
changes,
|
||||
});
|
||||
let keymap = crate::keymap::RuntimeKeymap::defaults();
|
||||
@@ -2146,10 +2143,36 @@ mod tests {
|
||||
);
|
||||
let rendered = render_overlay_lines(&view, /*width*/ 120);
|
||||
assert!(rendered.contains("Thread: Banach [worker]"));
|
||||
assert!(rendered.contains("Description: Apply proposed file edits"));
|
||||
for path in ["/tmp/bug1.txt", "/tmp/new.txt", "/tmp/old.txt"] {
|
||||
assert!(rendered.contains(&format!("Destination: {}", test_path_buf(path).display())));
|
||||
}
|
||||
for path in [foreign_path, foreign_move_path] {
|
||||
assert!(rendered.contains(&format!("Destination: {path}")));
|
||||
}
|
||||
assert!(rendered.contains("o to open thread"));
|
||||
assert!(!rendered.contains("$ apply_patch"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn apply_patch_prompt_without_changes_shows_unavailable_destination() {
|
||||
let (tx, _rx) = unbounded_channel::<AppEvent>();
|
||||
let tx = AppEventSender::new(tx);
|
||||
let request = ApprovalRequest::ApplyPatch(ApplyPatchApprovalRequest {
|
||||
thread_id: ThreadId::new(),
|
||||
thread_label: None,
|
||||
id: "test".to_string(),
|
||||
reason: None,
|
||||
cwd: absolute_path("/tmp"),
|
||||
changes: HashMap::new(),
|
||||
});
|
||||
let view = make_overlay(request, tx, Features::with_defaults());
|
||||
assert_snapshot!(
|
||||
"approval_overlay_patch_destination_unavailable",
|
||||
normalize_snapshot_paths(render_overlay_lines(&view, /*width*/ 120))
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn network_exec_prompt_title_includes_host() {
|
||||
let (tx, _rx) = unbounded_channel::<AppEvent>();
|
||||
|
||||
@@ -56,6 +56,7 @@ use std::time::Instant;
|
||||
|
||||
mod action_required_title;
|
||||
mod app_link_view;
|
||||
mod apply_patch_header;
|
||||
mod approval_overlay;
|
||||
mod mcp_server_elicitation;
|
||||
mod multi_select_picker;
|
||||
|
||||
+14
@@ -0,0 +1,14 @@
|
||||
---
|
||||
source: tui/src/bottom_pane/approval_overlay.rs
|
||||
expression: "normalize_snapshot_paths(render_overlay_lines(&view, 120))"
|
||||
---
|
||||
Would you like to make the following edits?
|
||||
|
||||
Description: Apply proposed file edits
|
||||
Destination: unavailable
|
||||
|
||||
› 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
|
||||
Reference in new issue
Block a user