From ec20e84d80c5216234b39c4148febef86126fd8e Mon Sep 17 00:00:00 2001 From: aibrahim-oai Date: Wed, 6 Aug 2025 22:25:41 -0700 Subject: [PATCH] Change the UI of apply patch (#1907) image --------- Co-authored-by: Gabriel Peal --- codex-rs/Cargo.lock | 35 +++-- codex-rs/tui/Cargo.toml | 4 +- codex-rs/tui/src/chatwidget.rs | 8 +- codex-rs/tui/src/history_cell.rs | 212 ++++++++++++++++++++----------- 4 files changed, 168 insertions(+), 91 deletions(-) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 4e21baf7..aa5398dd 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -873,6 +873,7 @@ dependencies = [ "codex-ollama", "color-eyre", "crossterm", + "diffy", "image", "insta", "lazy_static", @@ -1255,6 +1256,15 @@ version = "0.4.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "6184e33543162437515c2e2b48714794e37845ec9851711914eec9d308f6ebe8" +[[package]] +name = "diffy" +version = "0.4.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b545b8c50194bdd008283985ab0b31dba153cfd5b3066a92770634fbc0d7d291" +dependencies = [ + "nu-ansi-term 0.50.1", +] + [[package]] name = "digest" version = "0.10.7" @@ -1493,7 +1503,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "778e2ac28f6c47af28e4907f13ffd1e1ddbd400980a9abd7c8df189bf578a5ad" dependencies = [ "libc", - "windows-sys 0.52.0", + "windows-sys 0.60.2", ] [[package]] @@ -1573,7 +1583,7 @@ checksum = "0ce92ff622d6dadf7349484f42c93271a0d49b7cc4d466a936405bacbe10aa78" dependencies = [ "cfg-if", "rustix 1.0.8", - "windows-sys 0.52.0", + "windows-sys 0.59.0", ] [[package]] @@ -2356,7 +2366,7 @@ checksum = "e04d7f318608d35d4b61ddd75cbdaee86b023ebe2bd5a66ee0915f0bf93095a9" dependencies = [ "hermit-abi", "libc", - "windows-sys 0.52.0", + "windows-sys 0.59.0", ] [[package]] @@ -2832,6 +2842,15 @@ dependencies = [ "winapi", ] +[[package]] +name = "nu-ansi-term" +version = "0.50.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d4a28e057d01f97e61255210fcff094d74ed0466038633e95017f5beb68e4399" +dependencies = [ + "windows-sys 0.52.0", +] + [[package]] name = "nucleo-matcher" version = "0.3.1" @@ -3740,7 +3759,7 @@ dependencies = [ "errno", "libc", "linux-raw-sys 0.4.15", - "windows-sys 0.52.0", + "windows-sys 0.59.0", ] [[package]] @@ -3753,7 +3772,7 @@ dependencies = [ "errno", "libc", "linux-raw-sys 0.9.4", - "windows-sys 0.52.0", + "windows-sys 0.60.2", ] [[package]] @@ -4519,7 +4538,7 @@ dependencies = [ "getrandom 0.3.3", "once_cell", "rustix 1.0.8", - "windows-sys 0.52.0", + "windows-sys 0.59.0", ] [[package]] @@ -4960,7 +4979,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e8189decb5ac0fa7bc8b96b7cb9b2701d60d48805aca84a238004d665fcc4008" dependencies = [ "matchers", - "nu-ansi-term", + "nu-ansi-term 0.46.0", "once_cell", "regex", "sharded-slab", @@ -5378,7 +5397,7 @@ version = "0.1.9" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "cf221c93e13a30d793f7645a0e7762c55d169dbb0a49671918a2319d289b10bb" dependencies = [ - "windows-sys 0.52.0", + "windows-sys 0.59.0", ] [[package]] diff --git a/codex-rs/tui/Cargo.toml b/codex-rs/tui/Cargo.toml index 49d843f0..719c6311 100644 --- a/codex-rs/tui/Cargo.toml +++ b/codex-rs/tui/Cargo.toml @@ -36,6 +36,7 @@ codex-login = { path = "../login" } codex-ollama = { path = "../ollama" } color-eyre = "0.6.3" crossterm = { version = "0.28.1", features = ["bracketed-paste"] } +diffy = "0.4.2" image = { version = "^0.25.6", default-features = false, features = ["jpeg"] } lazy_static = "1" mcp-types = { path = "../mcp-types" } @@ -72,10 +73,9 @@ unicode-width = "0.1" uuid = "1" - [dev-dependencies] +chrono = { version = "0.4", features = ["serde"] } insta = "1.43.1" pretty_assertions = "1" rand = "0.8" -chrono = { version = "0.4", features = ["serde"] } vt100 = "0.16.2" diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 50fa776e..9936c0ee 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -476,11 +476,9 @@ impl ChatWidget<'_> { )); } EventMsg::PatchApplyEnd(event) => { - self.add_to_history(HistoryCell::new_patch_apply_end( - event.stdout, - event.stderr, - event.success, - )); + if !event.success { + self.add_to_history(HistoryCell::new_patch_apply_failure(event.stderr)); + } } EventMsg::ExecCommandEnd(ExecCommandEndEvent { call_id, diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index 5caedf98..79a17f4a 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -40,6 +40,12 @@ pub(crate) struct CommandOutput { pub(crate) stderr: String, } +struct FileSummary { + display_path: String, + added: usize, + removed: usize, +} + pub(crate) enum PatchEventType { ApprovalRequest, ApplyBegin { auto_approved: bool }, @@ -599,12 +605,12 @@ impl HistoryCell { PatchEventType::ApprovalRequest => "proposed patch", PatchEventType::ApplyBegin { auto_approved: true, - } => "applying patch", + } => "✏️ Applying patch", PatchEventType::ApplyBegin { auto_approved: false, } => { let lines: Vec> = vec![ - Line::from("applying patch".magenta().bold()), + Line::from("✏️ Applying patch".magenta().bold()), Line::from(""), ]; return Self::PendingPatch { @@ -613,39 +619,12 @@ impl HistoryCell { } }; - let summary_lines = create_diff_summary(changes); + let summary_lines = create_diff_summary(title, changes); let mut lines: Vec> = Vec::new(); - // Header similar to the command formatter so patches are visually - // distinct while still fitting the overall colour scheme. - lines.push(Line::from(title.magenta().bold())); - for line in summary_lines { - if line.starts_with('+') { - lines.push(line.green().into()); - } else if line.starts_with('-') { - lines.push(line.red().into()); - } else if let Some(space_idx) = line.find(' ') { - let kind_owned = line[..space_idx].to_string(); - let rest_owned = line[space_idx + 1..].to_string(); - - let style_for = |fg: Color| Style::default().fg(fg).add_modifier(Modifier::BOLD); - - let styled_kind = match kind_owned.as_str() { - "A" => RtSpan::styled(kind_owned.clone(), style_for(Color::Green)), - "D" => RtSpan::styled(kind_owned.clone(), style_for(Color::Red)), - "M" => RtSpan::styled(kind_owned.clone(), style_for(Color::Yellow)), - "R" | "C" => RtSpan::styled(kind_owned.clone(), style_for(Color::Cyan)), - _ => RtSpan::raw(kind_owned.clone()), - }; - - let styled_line = - RtLine::from(vec![styled_kind, RtSpan::raw(" "), RtSpan::raw(rest_owned)]); - lines.push(styled_line); - } else { - lines.push(Line::from(line)); - } + lines.push(line); } lines.push(Line::from("")); @@ -655,44 +634,23 @@ impl HistoryCell { } } - pub(crate) fn new_patch_apply_end(stdout: String, stderr: String, success: bool) -> Self { + pub(crate) fn new_patch_apply_failure(stderr: String) -> Self { let mut lines: Vec> = Vec::new(); - let status = if success { - RtSpan::styled("patch applied", Style::default().fg(Color::Green)) - } else { - RtSpan::styled( - "patch failed", - Style::default().fg(Color::Red).add_modifier(Modifier::BOLD), - ) - }; - lines.push(RtLine::from(vec![ - "patch".magenta().bold(), - " ".into(), - status, - ])); + // Failure title + lines.push(Line::from("✘ Failed to apply patch".magenta().bold())); - let src = if success { - if stdout.trim().is_empty() { - &stderr - } else { - &stdout - } - } else if stderr.trim().is_empty() { - &stdout - } else { - &stderr - }; - - if !src.trim().is_empty() { - lines.push(Line::from("")); - let mut iter = src.lines(); - for raw in iter.by_ref().take(TOOL_CALL_MAX_LINES) { - lines.push(ansi_escape_line(raw).dim()); + if !stderr.trim().is_empty() { + let mut iter = stderr.lines(); + for (i, raw) in iter.by_ref().take(TOOL_CALL_MAX_LINES).enumerate() { + let prefix = if i == 0 { " ⎿ " } else { " " }; + let s = format!("{prefix}{raw}"); + lines.push(ansi_escape_line(&s).dim()); } let remaining = iter.count(); if remaining > 0 { - lines.push(Line::from(format!("... {remaining} additional lines")).dim()); + lines.push(Line::from("")); + lines.push(Line::from(format!("... +{remaining} lines")).dim()); } } @@ -712,36 +670,138 @@ impl WidgetRef for &HistoryCell { } } -fn create_diff_summary(changes: HashMap) -> Vec { - // Build a concise, human‑readable summary list similar to the - // `git status` short format so the user can reason about the - // patch without scrolling. - let mut summaries: Vec = Vec::new(); +fn create_diff_summary(title: &str, changes: HashMap) -> Vec> { + let mut files: Vec = Vec::new(); + + // Count additions/deletions from a unified diff body + let count_from_unified = |diff: &str| -> (usize, usize) { + if let Ok(patch) = diffy::Patch::from_str(diff) { + let mut adds = 0usize; + let mut dels = 0usize; + for hunk in patch.hunks() { + for line in hunk.lines() { + match line { + diffy::Line::Insert(_) => adds += 1, + diffy::Line::Delete(_) => dels += 1, + _ => {} + } + } + } + (adds, dels) + } else { + let mut adds = 0usize; + let mut dels = 0usize; + for l in diff.lines() { + if l.starts_with("+++") || l.starts_with("---") || l.starts_with("@@") { + continue; + } + match l.as_bytes().first() { + Some(b'+') => adds += 1, + Some(b'-') => dels += 1, + _ => {} + } + } + (adds, dels) + } + }; + for (path, change) in &changes { use codex_core::protocol::FileChange::*; match change { Add { content } => { let added = content.lines().count(); - summaries.push(format!("A {} (+{added})", path.display())); + files.push(FileSummary { + display_path: path.display().to_string(), + added, + removed: 0, + }); } Delete => { - summaries.push(format!("D {}", path.display())); + let removed = std::fs::read_to_string(path) + .ok() + .map(|s| s.lines().count()) + .unwrap_or(0); + files.push(FileSummary { + display_path: path.display().to_string(), + added: 0, + removed, + }); } Update { unified_diff, move_path, } => { - if let Some(new_path) = move_path { - summaries.push(format!("R {} → {}", path.display(), new_path.display(),)); + let (added, removed) = count_from_unified(unified_diff); + let display_path = if let Some(new_path) = move_path { + format!("{} → {}", path.display(), new_path.display()) } else { - summaries.push(format!("M {}", path.display(),)); - } - summaries.extend(unified_diff.lines().map(|s| s.to_string())); + path.display().to_string() + }; + files.push(FileSummary { + display_path, + added, + removed, + }); } } } - summaries + let file_count = files.len(); + let total_added: usize = files.iter().map(|f| f.added).sum(); + let total_removed: usize = files.iter().map(|f| f.removed).sum(); + let noun = if file_count == 1 { "file" } else { "files" }; + + let mut out: Vec> = Vec::new(); + + // Header + let mut header_spans: Vec> = Vec::new(); + header_spans.push(RtSpan::styled( + title.to_owned(), + Style::default() + .fg(Color::Magenta) + .add_modifier(Modifier::BOLD), + )); + header_spans.push(RtSpan::raw(" to ")); + header_spans.push(RtSpan::raw(format!("{file_count} {noun} "))); + header_spans.push(RtSpan::raw("(")); + header_spans.push(RtSpan::styled( + format!("+{total_added}"), + Style::default().fg(Color::Green), + )); + header_spans.push(RtSpan::raw(" ")); + header_spans.push(RtSpan::styled( + format!("-{total_removed}"), + Style::default().fg(Color::Red), + )); + header_spans.push(RtSpan::raw(")")); + out.push(RtLine::from(header_spans)); + + // Dimmed per-file lines with prefix + for (idx, f) in files.iter().enumerate() { + let mut spans: Vec> = Vec::new(); + spans.push(RtSpan::raw(f.display_path.clone())); + spans.push(RtSpan::raw(" (")); + spans.push(RtSpan::styled( + format!("+{}", f.added), + Style::default().fg(Color::Green), + )); + spans.push(RtSpan::raw(" ")); + spans.push(RtSpan::styled( + format!("-{}", f.removed), + Style::default().fg(Color::Red), + )); + spans.push(RtSpan::raw(")")); + + let mut line = RtLine::from(spans); + let prefix = if idx == 0 { " ⎿ " } else { " " }; + line.spans.insert(0, prefix.into()); + line.spans.iter_mut().for_each(|span| { + span.style = span.style.add_modifier(Modifier::DIM); + }); + out.push(line); + } + + out } fn format_mcp_invocation<'a>(invocation: McpInvocation) -> Line<'a> {