From f96f94db8f8b94c9e861caee6f92aa0be558a149 Mon Sep 17 00:00:00 2001 From: Miguel Palhas Date: Sat, 5 Sep 2026 09:27:37 +0100 Subject: [PATCH] fix(maestro): align structured diff hunks --- crates/maestro-tui/src/structured_pane.rs | 260 +++++++++++++++++----- web/src/structuredDiff.test.ts | 22 +- web/src/structuredDiff.ts | 2 +- 3 files changed, 230 insertions(+), 54 deletions(-) diff --git a/crates/maestro-tui/src/structured_pane.rs b/crates/maestro-tui/src/structured_pane.rs index af73ecc..26b7b28 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 DIFF_CONTEXT_LINES: usize = 3; const LCS_CELL_LIMIT: usize = 250_000; #[derive(Debug, Clone)] @@ -521,7 +522,7 @@ fn split_diff_lines(text: &str) -> Vec> { .collect() } -fn lcs_diff<'a>(old: &[SourceLine<'a>], new: &[SourceLine<'a>], limit: usize) -> Vec> { +fn lcs_diff<'a>(old: &[SourceLine<'a>], new: &[SourceLine<'a>]) -> Vec> { if old.is_empty() { return new .iter() @@ -530,7 +531,6 @@ fn lcs_diff<'a>(old: &[SourceLine<'a>], new: &[SourceLine<'a>], limit: usize) -> kind: DiffKind::Add, line, }) - .take(limit) .collect(); } if new.is_empty() { @@ -541,7 +541,6 @@ fn lcs_diff<'a>(old: &[SourceLine<'a>], new: &[SourceLine<'a>], limit: usize) -> kind: DiffKind::Delete, line, }) - .take(limit) .collect(); } if old.len().saturating_mul(new.len()) > LCS_CELL_LIMIT { @@ -556,7 +555,6 @@ fn lcs_diff<'a>(old: &[SourceLine<'a>], new: &[SourceLine<'a>], limit: usize) -> kind: DiffKind::Add, line, })) - .take(limit) .collect(); } @@ -574,7 +572,7 @@ fn lcs_diff<'a>(old: &[SourceLine<'a>], new: &[SourceLine<'a>], limit: usize) -> 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 { + while i < old.len() && j < new.len() { if old[i] == new[j] { ops.push(DiffOp { kind: DiffKind::Context, @@ -596,32 +594,18 @@ fn lcs_diff<'a>(old: &[SourceLine<'a>], new: &[SourceLine<'a>], limit: usize) -> 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.extend(old[i..].iter().copied().map(|line| DiffOp { + kind: DiffKind::Delete, + line, + })); + ops.extend(new[j..].iter().copied().map(|line| DiffOp { + kind: DiffKind::Add, + line, + })); ops } -fn diff_ops<'a>(old: &'a str, new: &'a str, limit: usize) -> Vec> { +fn diff_ops<'a>(old: &'a str, new: &'a str) -> Vec> { let old = split_diff_lines(old); let new = split_diff_lines(new); let mut prefix = 0; @@ -643,15 +627,11 @@ fn diff_ops<'a>(old: &'a str, new: &'a str, limit: usize) -> Vec> { 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() @@ -659,14 +639,71 @@ fn diff_ops<'a>(old: &'a str, new: &'a str, limit: usize) -> Vec> { .map(|line| DiffOp { kind: DiffKind::Context, line, - }) - .take(remaining), + }), ); ops } +fn render_diff_ops(ops: &[DiffOp<'_>]) -> Vec { + let mut rendered = Vec::with_capacity(ops.len()); + let mut index = 0; + while index < ops.len() { + if ops[index].kind != DiffKind::Context { + push_diff_op(&mut rendered, ops[index]); + index += 1; + continue; + } + + let mut end = index; + while end < ops.len() && ops[end].kind == DiffKind::Context { + end += 1; + } + let run = &ops[index..end]; + let whole_diff = index == 0 && end == ops.len(); + let keep_head = if whole_diff || index > 0 { + DIFF_CONTEXT_LINES + } else { + 0 + }; + let keep_tail = if whole_diff || end < ops.len() { + DIFF_CONTEXT_LINES + } else { + 0 + }; + if run.len() <= keep_head + keep_tail { + for op in run { + push_diff_op(&mut rendered, *op); + } + } else { + for op in run.iter().take(keep_head) { + push_diff_op(&mut rendered, *op); + } + rendered.push(format!( + "… {} unchanged lines …", + run.len() - keep_head - keep_tail + )); + for op in run.iter().skip(run.len() - keep_tail) { + push_diff_op(&mut rendered, *op); + } + } + index = end; + } + rendered +} + +fn push_diff_op(rendered: &mut Vec, op: DiffOp<'_>) { + let prefix = match op.kind { + DiffKind::Add => "+ ", + DiffKind::Delete => "- ", + DiffKind::Context => " ", + }; + rendered.push(format!("{prefix}{}", op.line.text)); + if !op.line.terminated { + rendered.push("\\ No newline at end of file".into()); + } +} + 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) @@ -675,26 +712,13 @@ fn render_diff(old: Option<&str>, new: &str, limit: Option) -> Vec "+ ", - 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; - } + let mut rendered = render_diff_ops(&ops); + if let Some(limit) = limit { + rendered.truncate(limit); } rendered } @@ -977,6 +1001,138 @@ mod tests { diff_content(Some("a\n"), "a"), ["x:", "- a", "+ a", "\\ No newline at end of file"] ); + assert_eq!( + diff_content(None, "a"), + ["x:", "+ a", "\\ No newline at end of file"] + ); + } + + #[test] + fn structured_diff_marks_unchanged_unterminated_final_context() { + assert_eq!( + diff_content(Some("a\nb\nc"), "a\nX\nc"), + [ + "x:", + " a", + "- b", + "+ X", + " c", + "\\ No newline at end of file" + ] + ); + } + + #[test] + fn structured_diff_collapses_long_context_around_changes() { + let common = (0..20) + .map(|index| format!("line{index}")) + .collect::>(); + let old = common[..10] + .iter() + .cloned() + .chain(["OLD".into()]) + .chain(common[10..].iter().cloned()) + .collect::>() + .join("\n"); + let new = common[..10] + .iter() + .cloned() + .chain(["NEW".into()]) + .chain(common[10..].iter().cloned()) + .collect::>() + .join("\n"); + + assert_eq!( + render_diff(Some(&old), &new, None), + [ + "… 7 unchanged lines …", + " line7", + " line8", + " line9", + "- OLD", + "+ NEW", + " line10", + " line11", + " line12", + "… 7 unchanged lines …", + ] + ); + } + + #[test] + fn structured_diff_collapses_only_long_interior_context() { + let middle = (0..10).map(|index| format!("c{index}")).collect::>(); + let old = ["lead0", "lead1", "OLD1"] + .into_iter() + .map(str::to_string) + .chain(middle.iter().cloned()) + .chain(["OLD2", "trail0", "trail1"].into_iter().map(str::to_string)) + .collect::>() + .join("\n"); + let new = ["lead0", "lead1", "NEW1"] + .into_iter() + .map(str::to_string) + .chain(middle.iter().cloned()) + .chain(["NEW2", "trail0", "trail1"].into_iter().map(str::to_string)) + .collect::>() + .join("\n"); + let rendered = render_diff(Some(&old), &new, None); + + assert_eq!( + rendered + .iter() + .filter(|line| line.contains("unchanged lines")) + .count(), + 1 + ); + assert_eq!(&rendered[..4], [" lead0", " lead1", "- OLD1", "+ NEW1"]); + assert_eq!( + &rendered[4..11], + [ + " c0", + " c1", + " c2", + "… 4 unchanged lines …", + " c7", + " c8", + " c9" + ] + ); + assert_eq!( + &rendered[11..], + [ + "- OLD2", + "+ NEW2", + " trail0", + " trail1", + "\\ No newline at end of file" + ] + ); + } + + #[test] + fn structured_diff_preview_remains_bounded_after_hunking() { + let common = (0..10) + .map(|index| format!("line{index}")) + .collect::>(); + let old = common + .iter() + .cloned() + .chain(["OLD".into()]) + .collect::>() + .join("\n"); + let new = common + .iter() + .cloned() + .chain(["NEW".into()]) + .collect::>() + .join("\n"); + let expanded = render_diff(Some(&old), &new, None); + + assert_eq!(render_diff(Some(&old), &new, Some(4)), expanded[..4]); + assert_eq!(expanded[0], "… 7 unchanged lines …"); + assert!(expanded.contains(&"- OLD".into())); + assert!(expanded.contains(&"+ NEW".into())); } #[test] diff --git a/web/src/structuredDiff.test.ts b/web/src/structuredDiff.test.ts index 04a640a..70c2ced 100644 --- a/web/src/structuredDiff.test.ts +++ b/web/src/structuredDiff.test.ts @@ -78,6 +78,25 @@ describe("computeDiffLines", () => { ]); }); + it("marks an unchanged unterminated final context line", () => { + expect(texts(computeDiffLines("a\nb\nc", "a\nX\nc"))).toEqual([ + ["ctx", "a"], + ["del", "b"], + ["add", "X"], + ["ctx", "c"], + ["eof", "\\ No newline at end of file"], + ]); + }); + + it("marks an unterminated new file only once", () => { + const lines = computeDiffLines(null, "a"); + expect(texts(lines)).toEqual([ + ["add", "a"], + ["eof", "\\ No newline at end of file"], + ]); + expect(lines.filter((line) => line.kind === "eof")).toHaveLength(1); + }); + 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"); @@ -111,9 +130,10 @@ describe("computeDiffLines", () => { ["ctx", "lead0"], ["ctx", "lead1"], ]); - expect(texts(lines).slice(-2)).toEqual([ + expect(texts(lines).slice(-3)).toEqual([ ["ctx", "trail0"], ["ctx", "trail1"], + ["eof", "\\ No newline at end of file"], ]); }); diff --git a/web/src/structuredDiff.ts b/web/src/structuredDiff.ts index 2d92cc3..c7eb8b0 100644 --- a/web/src/structuredDiff.ts +++ b/web/src/structuredDiff.ts @@ -109,7 +109,7 @@ 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) { + if (!op.line.terminated) { out.push({ kind: "eof", text: "\\ No newline at end of file" }); } };