From c86b1be3cdbe12307843bcc9e7a44c1904ddcdf1 Mon Sep 17 00:00:00 2001
From: Charlie Marsh <charliemarsh@openai.com>
Date: Sun, 19 Jul 2026 20:15:45 +0000
Subject: [PATCH] Avoid cloning file changes in TUI diff rendering (#34224)

## What changed

- Consume and sort `DiffSummary` entries directly when building renderables.
- Borrow paths and `FileChange` values in the shared row representation used by line-based summaries.
- Share line-count calculation between both rendering paths.

GitOrigin-RevId: b02668074def7529ff39445e9970a1fec209f02b
---
 codex-rs/tui/src/diff_render.rs | 61 ++++++++++++++++++---------------
 1 file changed, 33 insertions(+), 28 deletions(-)

diff --git a/codex-rs/tui/src/diff_render.rs b/codex-rs/tui/src/diff_render.rs
index 9972a3fa63..58410baf33 100644
--- a/codex-rs/tui/src/diff_render.rs
+++ b/codex-rs/tui/src/diff_render.rs
@@ -323,18 +323,21 @@ impl Renderable for FileChange {
 impl From<DiffSummary> for Box<dyn Renderable> {
     fn from(val: DiffSummary) -> Self {
         let mut rows: Vec<Box<dyn Renderable>> = vec![];
+        let mut changes: Vec<_> = val.changes.into_iter().collect();
+        changes.sort_by(|left, right| left.0.cmp(&right.0));
 
-        for (i, row) in collect_rows(&val.changes).into_iter().enumerate() {
+        for (i, (path, change)) in changes.into_iter().enumerate() {
             if i > 0 {
                 rows.push(Box::new(RtLine::from("")));
             }
-            let mut path = RtLine::from(display_path_for(&row.path, val.cwd.as_path()));
+            let (added, removed) = line_counts(&change);
+            let mut path = RtLine::from(display_path_for(&path, val.cwd.as_path()));
             path.push_span(" ");
-            path.extend(render_line_count_summary(row.added, row.removed));
+            path.extend(render_line_count_summary(added, removed));
             rows.push(Box::new(path));
             rows.push(Box::new(RtLine::from("")));
             rows.push(Box::new(InsetRenderable::new(
-                Box::new(row.change) as Box<dyn Renderable>,
+                Box::new(change) as Box<dyn Renderable>,
                 Insets::tlbr(
                     /*top*/ 0, /*left*/ 2, /*bottom*/ 0, /*right*/ 0,
                 ),
@@ -355,43 +358,45 @@ pub(crate) fn create_diff_summary(
 }
 
 // Shared row for per-file presentation
-#[derive(Clone)]
-struct Row {
-    #[allow(dead_code)]
-    path: PathBuf,
-    move_path: Option<PathBuf>,
+struct Row<'a> {
+    path: &'a Path,
+    move_path: Option<&'a Path>,
     added: usize,
     removed: usize,
-    change: FileChange,
+    change: &'a FileChange,
 }
 
-fn collect_rows(changes: &HashMap<PathBuf, FileChange>) -> Vec<Row> {
-    let mut rows: Vec<Row> = Vec::new();
+fn collect_rows(changes: &HashMap<PathBuf, FileChange>) -> Vec<Row<'_>> {
+    let mut rows = Vec::with_capacity(changes.len());
     for (path, change) in changes.iter() {
-        let (added, removed) = match change {
-            FileChange::Add { content } => (content.lines().count(), 0),
-            FileChange::Delete { content } => (0, content.lines().count()),
-            FileChange::Update { unified_diff, .. } => calculate_add_remove_from_diff(unified_diff),
-        };
+        let (added, removed) = line_counts(change);
         let move_path = match change {
             FileChange::Update {
                 move_path: Some(new),
                 ..
-            } => Some(new.clone()),
+            } => Some(new.as_path()),
             _ => None,
         };
         rows.push(Row {
-            path: path.clone(),
+            path: path.as_path(),
             move_path,
             added,
             removed,
-            change: change.clone(),
+            change,
         });
     }
-    rows.sort_by_key(|r| r.path.clone());
+    rows.sort_by(|left, right| left.path.cmp(right.path));
     rows
 }
 
+fn line_counts(change: &FileChange) -> (usize, usize) {
+    match change {
+        FileChange::Add { content } => (content.lines().count(), 0),
+        FileChange::Delete { content } => (0, content.lines().count()),
+        FileChange::Update { unified_diff, .. } => calculate_add_remove_from_diff(unified_diff),
+    }
+}
+
 fn render_line_count_summary(added: usize, removed: usize) -> Vec<RtSpan<'static>> {
     let mut spans = Vec::new();
     spans.push("(".into());
@@ -402,13 +407,13 @@ fn render_line_count_summary(added: usize, removed: usize) -> Vec<RtSpan<'static
     spans
 }
 
-fn render_changes_block(rows: Vec<Row>, wrap_cols: usize, cwd: &Path) -> Vec<RtLine<'static>> {
+fn render_changes_block(rows: Vec<Row<'_>>, wrap_cols: usize, cwd: &Path) -> Vec<RtLine<'static>> {
     let mut out: Vec<RtLine<'static>> = Vec::new();
 
-    let render_path = |row: &Row| -> Vec<RtSpan<'static>> {
+    let render_path = |row: &Row<'_>| -> Vec<RtSpan<'static>> {
         let mut spans = Vec::new();
-        spans.push(display_path_for(&row.path, cwd).into());
-        if let Some(move_path) = &row.move_path {
+        spans.push(display_path_for(row.path, cwd).into());
+        if let Some(move_path) = row.move_path {
             spans.push(format!(" → {}", display_path_for(move_path, cwd)).into());
         }
         spans
@@ -421,7 +426,7 @@ fn render_changes_block(rows: Vec<Row>, wrap_cols: usize, cwd: &Path) -> Vec<RtL
     let noun = if file_count == 1 { "file" } else { "files" };
     let mut header_spans: Vec<RtSpan<'static>> = vec!["• ".dim()];
     if let [row] = &rows[..] {
-        let verb = match &row.change {
+        let verb = match row.change {
             FileChange::Add { .. } => "Added",
             FileChange::Delete { .. } => "Deleted",
             _ => "Edited",
@@ -456,10 +461,10 @@ fn render_changes_block(rows: Vec<Row>, wrap_cols: usize, cwd: &Path) -> Vec<RtL
 
         // For renames, use the destination extension for highlighting — the
         // diff content reflects the new file, not the old one.
-        let lang_path = r.move_path.as_deref().unwrap_or(&r.path);
+        let lang_path = r.move_path.unwrap_or(r.path);
         let lang = detect_lang_for_path(lang_path);
         let mut lines = vec![];
-        render_change(&r.change, &mut lines, wrap_cols - 4, lang.as_deref());
+        render_change(r.change, &mut lines, wrap_cols - 4, lang.as_deref());
         out.extend(prefix_lines(lines, "    ".into(), "    ".into()));
     }