From efb54750ed31d304b41f8d3b27e3ea7794c98a02 Mon Sep 17 00:00:00 2001 From: Miguel Palhas Date: Sat, 5 Sep 2026 09:18:05 +0100 Subject: [PATCH] fix(maestro): render accurate diffs --- crates/maestro-tui/src/structured_pane.rs | 288 +++++++++++++++++++++- web/src/structuredDiff.test.ts | 50 +++- web/src/structuredDiff.ts | 82 +++--- web/src/structuredRender.test.ts | 11 + web/src/styles.css | 5 + 5 files changed, 398 insertions(+), 38 deletions(-) diff --git a/crates/maestro-tui/src/structured_pane.rs b/crates/maestro-tui/src/structured_pane.rs index b8f29bd..af73ecc 100644 --- a/crates/maestro-tui/src/structured_pane.rs +++ b/crates/maestro-tui/src/structured_pane.rs @@ -14,6 +14,7 @@ use crate::session_task::SessionState; use crate::structured_task::{offered_decisions, StructuredHandle, StructuredState}; const DIFF_PREVIEW_LINES: usize = 3; +const LCS_CELL_LIMIT: usize = 250_000; #[derive(Debug, Clone)] pub struct StructuredPane { @@ -322,8 +323,8 @@ fn tool_lines( }, }); - let content = tool_content_lines(row); if row.kind == TranscriptRowKind::ToolDiff && !expanded { + let content = tool_content_lines(row, Some(DIFF_PREVIEW_LINES + 1)); let shown = content.len().min(DIFF_PREVIEW_LINES); for text in content.iter().take(shown) { push_wrapped(out, row, " ", text.clone(), width, diff_tone(text), false); @@ -340,6 +341,7 @@ fn tool_lines( ); } } else if expanded { + let content = tool_content_lines(row, None); for text in content { push_wrapped(out, row, " ", text.clone(), width, diff_tone(&text), false); } @@ -448,7 +450,7 @@ fn tool_title(row: &TranscriptRow) -> (String, String) { (explicit.unwrap_or(fallback).to_string(), title.to_string()) } -fn tool_content_lines(row: &TranscriptRow) -> Vec { +fn tool_content_lines(row: &TranscriptRow, limit: Option) -> Vec { let mut out = Vec::new(); for block in row .data @@ -457,6 +459,9 @@ fn tool_content_lines(row: &TranscriptRow) -> Vec { .into_iter() .flatten() { + if limit.is_some_and(|limit| out.len() >= limit) { + break; + } match block.get("type").and_then(serde_json::Value::as_str) { Some("content") => { let nested = block.get("content").cloned().unwrap_or_default(); @@ -466,7 +471,8 @@ fn tool_content_lines(row: &TranscriptRow) -> Vec { data: serde_json::json!({"content": nested}), }; let text = content_text(&fake); - out.extend(text.lines().map(str::to_string)); + let remaining = limit.map_or(usize::MAX, |limit| limit.saturating_sub(out.len())); + out.extend(text.lines().take(remaining).map(str::to_string)); } Some("diff") => { let path = block @@ -474,11 +480,10 @@ fn tool_content_lines(row: &TranscriptRow) -> Vec { .and_then(serde_json::Value::as_str) .unwrap_or("file"); out.push(format!("{path}:")); - if let Some(old) = block.get("oldText").and_then(serde_json::Value::as_str) { - out.extend(old.lines().map(|line| format!("- {line}"))); - } + let old = block.get("oldText").and_then(serde_json::Value::as_str); if let Some(new) = block.get("newText").and_then(serde_json::Value::as_str) { - out.extend(new.lines().map(|line| format!("+ {line}"))); + let remaining = limit.map(|limit| limit.saturating_sub(out.len())); + out.extend(render_diff(old, new, remaining)); } } Some("terminal") => out.push("[embedded terminal]".into()), @@ -488,6 +493,212 @@ fn tool_content_lines(row: &TranscriptRow) -> Vec { out } +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +struct SourceLine<'a> { + text: &'a str, + terminated: bool, +} + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +enum DiffKind { + Add, + Delete, + Context, +} + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +struct DiffOp<'a> { + kind: DiffKind, + line: SourceLine<'a>, +} + +fn split_diff_lines(text: &str) -> Vec> { + text.split_inclusive('\n') + .map(|part| SourceLine { + text: part.strip_suffix('\n').unwrap_or(part), + terminated: part.ends_with('\n'), + }) + .collect() +} + +fn lcs_diff<'a>(old: &[SourceLine<'a>], new: &[SourceLine<'a>], limit: usize) -> Vec> { + if old.is_empty() { + return new + .iter() + .copied() + .map(|line| DiffOp { + kind: DiffKind::Add, + line, + }) + .take(limit) + .collect(); + } + if new.is_empty() { + return old + .iter() + .copied() + .map(|line| DiffOp { + kind: DiffKind::Delete, + line, + }) + .take(limit) + .collect(); + } + if old.len().saturating_mul(new.len()) > LCS_CELL_LIMIT { + return old + .iter() + .copied() + .map(|line| DiffOp { + kind: DiffKind::Delete, + line, + }) + .chain(new.iter().copied().map(|line| DiffOp { + kind: DiffKind::Add, + line, + })) + .take(limit) + .collect(); + } + + let columns = new.len() + 1; + let mut table = vec![0_u32; (old.len() + 1) * columns]; + for i in (0..old.len()).rev() { + for j in (0..new.len()).rev() { + table[i * columns + j] = if old[i] == new[j] { + table[(i + 1) * columns + j + 1] + 1 + } else { + table[(i + 1) * columns + j].max(table[i * columns + j + 1]) + }; + } + } + + let mut ops = Vec::with_capacity(old.len() + new.len()); + let (mut i, mut j) = (0, 0); + while i < old.len() && j < new.len() && ops.len() < limit { + if old[i] == new[j] { + ops.push(DiffOp { + kind: DiffKind::Context, + line: old[i], + }); + i += 1; + j += 1; + } else if table[(i + 1) * columns + j] >= table[i * columns + j + 1] { + ops.push(DiffOp { + kind: DiffKind::Delete, + line: old[i], + }); + i += 1; + } else { + ops.push(DiffOp { + kind: DiffKind::Add, + line: new[j], + }); + j += 1; + } + } + let remaining = limit.saturating_sub(ops.len()); + ops.extend( + old[i..] + .iter() + .copied() + .map(|line| DiffOp { + kind: DiffKind::Delete, + line, + }) + .take(remaining), + ); + let remaining = limit.saturating_sub(ops.len()); + ops.extend( + new[j..] + .iter() + .copied() + .map(|line| DiffOp { + kind: DiffKind::Add, + line, + }) + .take(remaining), + ); + ops +} + +fn diff_ops<'a>(old: &'a str, new: &'a str, limit: usize) -> Vec> { + let old = split_diff_lines(old); + let new = split_diff_lines(new); + let mut prefix = 0; + while prefix < old.len() && prefix < new.len() && old[prefix] == new[prefix] { + prefix += 1; + } + let mut suffix = 0; + while suffix < old.len() - prefix + && suffix < new.len() - prefix + && old[old.len() - 1 - suffix] == new[new.len() - 1 - suffix] + { + suffix += 1; + } + + let mut ops = old[..prefix] + .iter() + .copied() + .map(|line| DiffOp { + kind: DiffKind::Context, + line, + }) + .take(limit) + .collect::>(); + let remaining = limit.saturating_sub(ops.len()); + ops.extend(lcs_diff( + &old[prefix..old.len() - suffix], + &new[prefix..new.len() - suffix], + remaining, + )); + let remaining = limit.saturating_sub(ops.len()); + ops.extend( + old[old.len() - suffix..] + .iter() + .copied() + .map(|line| DiffOp { + kind: DiffKind::Context, + line, + }) + .take(remaining), + ); + ops +} + +fn render_diff(old: Option<&str>, new: &str, limit: Option) -> Vec { + let op_limit = limit.unwrap_or(usize::MAX); + let ops = old.map_or_else( + || { + split_diff_lines(new) + .into_iter() + .map(|line| DiffOp { + kind: DiffKind::Add, + line, + }) + .take(op_limit) + .collect() + }, + |old| diff_ops(old, new, op_limit), + ); + let mut rendered = Vec::with_capacity(ops.len()); + for op in ops { + let prefix = match op.kind { + DiffKind::Add => "+ ", + DiffKind::Delete => "- ", + DiffKind::Context => " ", + }; + rendered.push(format!("{prefix}{}", op.line.text)); + if op.kind != DiffKind::Context && !op.line.terminated { + rendered.push("\\ No newline at end of file".into()); + } + if limit.is_some_and(|limit| rendered.len() >= limit) { + rendered.truncate(limit.unwrap_or(0)); + break; + } + } + rendered +} + fn diff_tone(text: &str) -> Tone { if text.starts_with("+ ") { Tone::Active @@ -722,6 +933,69 @@ mod tests { assert!(lines.last().unwrap().text.contains("Enter to expand")); } + fn diff_content(old_text: Option<&str>, new_text: &str) -> Vec { + let old_text = old_text.map_or(serde_json::Value::Null, |text| json!(text)); + let row = row( + "d", + TranscriptRowKind::ToolDiff, + json!({"content":[{"type":"diff","path":"x","oldText":old_text,"newText":new_text}]}), + ); + tool_content_lines(&row, None) + } + + #[test] + fn structured_diff_preserves_unchanged_lines() { + assert_eq!(diff_content(Some("a\nb\n"), "a\nb\n"), ["x:", " a", " b"]); + } + + #[test] + fn structured_diff_handles_insert_delete_replace_and_empty_files() { + assert_eq!( + diff_content(Some("a\nc\n"), "a\nb\nc\n"), + ["x:", " a", "+ b", " c"] + ); + assert_eq!( + diff_content(Some("a\nb\nc\n"), "a\nc\n"), + ["x:", " a", "- b", " c"] + ); + assert_eq!( + diff_content(Some("a\nb\n"), "a\nc\n"), + ["x:", " a", "- b", "+ c"] + ); + assert_eq!(diff_content(Some(""), ""), ["x:"]); + assert_eq!(diff_content(None, "a\n"), ["x:", "+ a"]); + assert_eq!(diff_content(Some("a\n"), ""), ["x:", "- a"]); + } + + #[test] + fn structured_diff_marks_added_and_removed_final_newlines() { + assert_eq!( + diff_content(Some("a"), "a\n"), + ["x:", "- a", "\\ No newline at end of file", "+ a"] + ); + assert_eq!( + diff_content(Some("a\n"), "a"), + ["x:", "- a", "+ a", "\\ No newline at end of file"] + ); + } + + #[test] + fn structured_diff_bounds_pathological_changed_middles() { + let old_lines = (0..501) + .map(|index| format!("line{index}")) + .collect::>(); + let mut new_lines = old_lines[1..].to_vec(); + new_lines.push(old_lines[0].clone()); + let old = format!("{}\n", old_lines.join("\n")); + let new = format!("{}\n", new_lines.join("\n")); + let rendered = render_diff(Some(&old), &new, None); + + assert_eq!(rendered.len(), 1_002); + assert!(rendered[..501].iter().all(|line| line.starts_with("- "))); + assert!(rendered[501..].iter().all(|line| line.starts_with("+ "))); + assert_eq!(render_diff(Some(&old), &new, Some(4)), rendered[..4]); + } + #[test] fn plan_is_not_a_transcript_row_and_status_is_two_lines() { let mut state = StructuredState::default(); diff --git a/web/src/structuredDiff.test.ts b/web/src/structuredDiff.test.ts index 2a53480..04a640a 100644 --- a/web/src/structuredDiff.test.ts +++ b/web/src/structuredDiff.test.ts @@ -42,6 +42,42 @@ describe("computeDiffLines", () => { ]); }); + it("handles an insertion without changing surrounding lines", () => { + expect(texts(computeDiffLines("a\nc\n", "a\nb\nc\n"))).toEqual([ + ["ctx", "a"], + ["add", "b"], + ["ctx", "c"], + ]); + }); + + it("handles a deletion without changing surrounding lines", () => { + expect(texts(computeDiffLines("a\nb\nc\n", "a\nc\n"))).toEqual([ + ["ctx", "a"], + ["del", "b"], + ["ctx", "c"], + ]); + }); + + it("renders two empty files without invented lines", () => { + expect(computeDiffLines("", "")).toEqual([]); + }); + + it("shows when a final newline is added", () => { + expect(texts(computeDiffLines("a", "a\n"))).toEqual([ + ["del", "a"], + ["eof", "\\ No newline at end of file"], + ["add", "a"], + ]); + }); + + it("shows when a final newline is removed", () => { + expect(texts(computeDiffLines("a\n", "a"))).toEqual([ + ["del", "a"], + ["add", "a"], + ["eof", "\\ No newline at end of file"], + ]); + }); + it("collapses a long unchanged interior run, keeping 3 lines of context each side", () => { const common = Array.from({ length: 20 }, (_, i) => `line${i}`); const oldText = [...common.slice(0, 10), "OLD", ...common.slice(10)].join("\n"); @@ -85,10 +121,22 @@ describe("computeDiffLines", () => { const lines = computeDiffLines("a\nb", "a\nb\nc"); expect(texts(lines)).toEqual([ ["ctx", "a"], - ["ctx", "b"], + ["del", "b"], + ["eof", "\\ No newline at end of file"], + ["add", "b"], ["add", "c"], + ["eof", "\\ No newline at end of file"], ]); }); + + it("falls back to a bounded coarse diff for a pathological changed middle", () => { + const oldLines = Array.from({ length: 501 }, (_, index) => `line${index}`); + const newLines = [...oldLines.slice(1), oldLines[0]]; + const lines = computeDiffLines(`${oldLines.join("\n")}\n`, `${newLines.join("\n")}\n`); + expect(lines).toHaveLength(1_002); + expect(lines.slice(0, 501).every((line) => line.kind === "del")).toBe(true); + expect(lines.slice(501).every((line) => line.kind === "add")).toBe(true); + }); }); describe("diffStats", () => { diff --git a/web/src/structuredDiff.ts b/web/src/structuredDiff.ts index b681c24..2d92cc3 100644 --- a/web/src/structuredDiff.ts +++ b/web/src/structuredDiff.ts @@ -2,7 +2,7 @@ // hands over full `oldText`/`newText`, not a unified-diff patch, so the // coloured hunks the design calls for have to be computed here. -export type DiffLineKind = "add" | "del" | "ctx" | "hunk"; +export type DiffLineKind = "add" | "del" | "ctx" | "hunk" | "eof"; export interface DiffLine { kind: DiffLineKind; @@ -19,66 +19,82 @@ const CONTEXT = 3; * still correct, just not minimal. */ const LCS_CELL_LIMIT = 250_000; -function splitLines(text: string): string[] { - if (text === "") return []; - const body = text.endsWith("\n") ? text.slice(0, -1) : text; - return body.split("\n"); +interface SourceLine { + text: string; + terminated: boolean; } -type Op = { kind: "add" | "del" | "ctx"; text: string }; +function splitLines(text: string): SourceLine[] { + if (text === "") return []; + const parts = text.split("\n"); + const hasFinalNewline = text.endsWith("\n"); + if (hasFinalNewline) parts.pop(); + return parts.map((line, index) => ({ + text: line, + terminated: index < parts.length - 1 || hasFinalNewline, + })); +} + +type Op = { kind: "add" | "del" | "ctx"; line: SourceLine }; + +function sameLine(a: SourceLine, b: SourceLine): boolean { + return a.text === b.text && a.terminated === b.terminated; +} /** Longest-common-subsequence diff of two line arrays, already trimmed of any * shared prefix/suffix by the caller. */ -function lcsDiff(a: string[], b: string[]): Op[] { +function lcsDiff(a: SourceLine[], b: SourceLine[]): Op[] { const n = a.length; const m = b.length; - if (n === 0) return b.map((text) => ({ kind: "add", text })); - if (m === 0) return a.map((text) => ({ kind: "del", text })); + if (n === 0) return b.map((line) => ({ kind: "add", line })); + if (m === 0) return a.map((line) => ({ kind: "del", line })); if (n * m > LCS_CELL_LIMIT) { - return [...a.map((text) => ({ kind: "del" as const, text })), ...b.map((text) => ({ kind: "add" as const, text }))]; + return [...a.map((line) => ({ kind: "del" as const, line })), ...b.map((line) => ({ kind: "add" as const, line }))]; } // dp[i][j] = length of the LCS of a[i:] and b[j:]. - const dp: Uint32Array[] = Array.from({ length: n + 1 }, () => new Uint32Array(m + 1)); + const columns = m + 1; + const dp = new Uint32Array((n + 1) * columns); for (let i = n - 1; i >= 0; i--) { for (let j = m - 1; j >= 0; j--) { - dp[i][j] = - a[i] === b[j] ? dp[i + 1][j + 1] + 1 : Math.max(dp[i + 1][j], dp[i][j + 1]); + dp[i * columns + j] = sameLine(a[i], b[j]) + ? dp[(i + 1) * columns + j + 1] + 1 + : Math.max(dp[(i + 1) * columns + j], dp[i * columns + j + 1]); } } const ops: Op[] = []; let i = 0; let j = 0; while (i < n && j < m) { - if (a[i] === b[j]) { - ops.push({ kind: "ctx", text: a[i] }); + if (sameLine(a[i], b[j])) { + ops.push({ kind: "ctx", line: a[i] }); i++; j++; - } else if (dp[i + 1][j] >= dp[i][j + 1]) { - ops.push({ kind: "del", text: a[i] }); + } else if (dp[(i + 1) * columns + j] >= dp[i * columns + j + 1]) { + ops.push({ kind: "del", line: a[i] }); i++; } else { - ops.push({ kind: "add", text: b[j] }); + ops.push({ kind: "add", line: b[j] }); j++; } } - while (i < n) ops.push({ kind: "del", text: a[i++] }); - while (j < m) ops.push({ kind: "add", text: b[j++] }); + while (i < n) ops.push({ kind: "del", line: a[i++] }); + while (j < m) ops.push({ kind: "add", line: b[j++] }); return ops; } -function diffLines(a: string[], b: string[]): Op[] { +function diffLines(a: SourceLine[], b: SourceLine[]): Op[] { let pre = 0; - while (pre < a.length && pre < b.length && a[pre] === b[pre]) pre++; + while (pre < a.length && pre < b.length && sameLine(a[pre], b[pre])) pre++; let suf = 0; while ( suf < a.length - pre && suf < b.length - pre && - a[a.length - 1 - suf] === b[b.length - 1 - suf] + sameLine(a[a.length - 1 - suf], b[b.length - 1 - suf]) ) { suf++; } - const prefixCtx: Op[] = a.slice(0, pre).map((text) => ({ kind: "ctx", text })); - const suffixCtx: Op[] = a.slice(a.length - suf).map((text) => ({ kind: "ctx", text })); + const prefixCtx: Op[] = a.slice(0, pre).map((line) => ({ kind: "ctx", line })); + const suffixCtx: Op[] = a.slice(a.length - suf).map((line) => ({ kind: "ctx", line })); const middle = lcsDiff(a.slice(pre, a.length - suf), b.slice(pre, b.length - suf)); return [...prefixCtx, ...middle, ...suffixCtx]; } @@ -91,10 +107,16 @@ function diffLines(a: string[], b: string[]): Op[] { * (unless it is the *only* run, i.e. nothing changed at all). */ function hunk(ops: Op[]): DiffLine[] { const out: DiffLine[] = []; + const push = (op: Op) => { + out.push({ kind: op.kind, text: op.line.text }); + if (op.kind !== "ctx" && !op.line.terminated) { + out.push({ kind: "eof", text: "\\ No newline at end of file" }); + } + }; let i = 0; while (i < ops.length) { if (ops[i].kind !== "ctx") { - out.push(ops[i]); + push(ops[i]); i++; continue; } @@ -105,14 +127,14 @@ function hunk(ops: Op[]): DiffLine[] { const keepHead = wholeDiff || i > 0 ? CONTEXT : 0; const keepTail = wholeDiff || j < ops.length ? CONTEXT : 0; if (run.length <= keepHead + keepTail) { - out.push(...run); + run.forEach(push); } else { - if (keepHead) out.push(...run.slice(0, keepHead)); + if (keepHead) run.slice(0, keepHead).forEach(push); out.push({ kind: "hunk", text: `⋯ ${run.length - keepHead - keepTail} unchanged lines ⋯`, }); - if (keepTail) out.push(...run.slice(run.length - keepTail)); + if (keepTail) run.slice(run.length - keepTail).forEach(push); } i = j; } @@ -123,7 +145,7 @@ function hunk(ops: Op[]): DiffLine[] { * (a new file) renders every line of `newText` as added. */ export function computeDiffLines(oldText: string | null | undefined, newText: string): DiffLine[] { const newLines = splitLines(newText); - if (oldText == null) return newLines.map((text) => ({ kind: "add", text })); + if (oldText == null) return hunk(newLines.map((line) => ({ kind: "add", line }))); const ops = diffLines(splitLines(oldText), newLines); return hunk(ops); } diff --git a/web/src/structuredRender.test.ts b/web/src/structuredRender.test.ts index 1aa406b..1fb6c80 100644 --- a/web/src/structuredRender.test.ts +++ b/web/src/structuredRender.test.ts @@ -157,6 +157,17 @@ describe("buildTranscriptRow: tool diffs", () => { // stylesheet keys `overflow-x: auto` off, not a computed style. expect(el.querySelector(".st-diff")!.className).toBe("st-diff"); }); + + it("renders a visible marker for a removed final newline", () => { + const el = buildTranscriptRow( + row("tool_diff", { + title: "Edit(x.rs)", + content: [{ type: "diff", path: "/repo/x.rs", oldText: "same\n", newText: "same" }], + }), + ); + (el.querySelector(".st-tool-head") as HTMLButtonElement).click(); + expect(el.querySelector(".st-ln-eof")!.textContent).toBe("\\ No newline at end of file"); + }); }); describe("buildTranscriptRow: approval", () => { diff --git a/web/src/styles.css b/web/src/styles.css index 60ea4ed..f994df8 100644 --- a/web/src/styles.css +++ b/web/src/styles.css @@ -2509,6 +2509,11 @@ input.leaf-rename:focus { background: var(--surface-2); } +.st-ln-eof { + color: var(--faint); + font-style: italic; +} + /* the one lifted, card-like element on the surface: the only thing the operator owes a decision on */ .st-approval {