Skip to content

fix: prevent incorrect merge of content-identical lines across non-monotonic crossings #374

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
glehmann merged 1 commit into from
Jul 30, 2026
Merged

fix: prevent incorrect merge of content-identical lines across non-monotonic crossings #374

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 82 additions & 1 deletion src/api/diff.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,48 @@ function buildInitialPairs(
return { pairOfDel, pairOfAdd };
}

function detectAndUnpairCrossings(
changes: _Change[],
pairOfDel: Int32Array,
pairOfAdd: Int32Array,
deleteIdxs: number[]
) {
const pairs: { delIdx: number; oldLN: number; newLN: number }[] = [];
for (const di of deleteIdxs) {
const ai = pairOfDel[di];
if (ai === UNPAIRED) continue;
const del = changes[di] as DeleteChange;
const add = changes[ai] as InsertChange;
pairs.push({ delIdx: di, oldLN: del.lineNumber, newLN: add.lineNumber });
}

pairs.sort((a, b) => a.newLN - b.newLN);

for (let i = 1; i < pairs.length; i++) {
if (pairs[i].oldLN < pairs[i - 1].oldLN) {
const d1 = Math.abs(pairs[i - 1].oldLN - pairs[i - 1].newLN);
const d2 = Math.abs(pairs[i].oldLN - pairs[i].newLN);
if (d1 >= d2) {
const di = pairs[i - 1].delIdx;
const ai = pairOfDel[di];
pairOfDel[di] = UNPAIRED;
pairOfAdd[ai] = UNPAIRED;
} else {
const di = pairs[i].delIdx;
const ai = pairOfDel[di];
pairOfDel[di] = UNPAIRED;
pairOfAdd[ai] = UNPAIRED;
}
return detectAndUnpairCrossings(
changes,
pairOfDel,
pairOfAdd,
deleteIdxs
);
}
}
}

function buildUnpairedDeletePrefix(changes: _Change[], pairOfDel: Int32Array) {
const n = changes.length;
const prefix = new Int32Array(n + 1);
Expand Down Expand Up @@ -490,7 +532,21 @@ function mergeModifiedLines(changes: _Change[], options: ParseOptions): Line[] {
deleteIdxs,
options
);
const unpairedDelPrefix = buildUnpairedDeletePrefix(changes, pairOfDel);

detectAndUnpairCrossings(changes, pairOfDel, pairOfAdd, deleteIdxs);

// Count unpaired deletes and inserts to detect complete permutations
// (e.g. rotation of identical lines) where crossings are expected.
let unpairedDelCount = 0;
for (const di of deleteIdxs) {
if (pairOfDel[di] === UNPAIRED) unpairedDelCount++;
}
let unpairedInsCount = 0;
for (const ai of insertIdxs) {
if (pairOfAdd[ai] === UNPAIRED) unpairedInsCount++;
}
const isCompletePermutation =
unpairedDelCount > 0 && unpairedDelCount === unpairedInsCount;

for (const di of deleteIdxs) {
if (pairOfDel[di] !== UNPAIRED) continue;
Expand All @@ -501,12 +557,37 @@ function mergeModifiedLines(changes: _Change[], options: ParseOptions): Line[] {
const add = changes[ai] as InsertChange;
if (del.content.trim() !== add.content.trim()) continue;
if (del.content.trim() === "") continue;

// Prevent re-pairing that would create non-monotonic crossings,
// unless this is a complete permutation (rotation) of identical lines.
if (!isCompletePermutation) {
const candOld = del.lineNumber;
const candNew = add.lineNumber;
let createsCrossing = false;
for (const ddi of deleteIdxs) {
if (ddi === di) continue;
const aai = pairOfDel[ddi];
if (aai === UNPAIRED) continue;
const d = changes[ddi] as DeleteChange;
const a = changes[aai] as InsertChange;
if (
(candOld < d.lineNumber && candNew > a.lineNumber) ||
(candOld > d.lineNumber && candNew < a.lineNumber)
) {
createsCrossing = true;
break;
}
}
if (createsCrossing) continue;
}

pairOfDel[di] = ai;
pairOfAdd[ai] = di;
break;
}
}

const unpairedDelPrefix = buildUnpairedDeletePrefix(changes, pairOfDel);
return emitLines(changes, pairOfDel, pairOfAdd, unpairedDelPrefix, options);
}

Expand Down
48 changes: 23 additions & 25 deletions src/browser/components/pr-review.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1971,15 +1971,9 @@ const DiffViewer = memo(function DiffViewer({
const line = lines[i];

if (line.type === "normal") {
// Check if this is a merged modified line with inline word-diff segments
const hasWordDiff = line.content.some((s) => s.type !== "normal");
if (hasWordDiff) {
// LEFT side: keep non-insert segments (normal + delete),
// preserving syntax-highlighted HTML from the original segments.
const leftContent = line.content.filter((s) => s.type !== "insert");

// Recover leading whitespace that diffWords may have absorbed
// into an INSERT token (e.g. indentation).
const leftRaw = leftContent.map((s) => s.value).join("");
const rightRaw = line.content
.filter((s) => s.type !== "delete")
Expand All @@ -1999,27 +1993,16 @@ const DiffViewer = memo(function DiffViewer({
});
}

// RIGHT side: keep non-delete segments (normal + insert),
// preserving syntax-highlighted HTML.
const rightContent = line.content.filter(
(s) => s.type !== "delete"
);

pairs.push({
left: {
...line,
type: "delete",
content: leftContent,
},
right: {
...line,
type: "insert",
content: rightContent,
},
left: { ...line, type: "delete", content: leftContent },
right: { ...line, type: "insert", content: rightContent },
lineNum: line.oldLineNumber || line.newLineNumber,
});
} else {
// Context line - show on both sides
pairs.push({
left: line,
right: line,
Expand All @@ -2028,21 +2011,16 @@ const DiffViewer = memo(function DiffViewer({
}
i++;
} else if (line.type === "delete") {
// Collect consecutive deletes
const deletes: DiffLine[] = [];
while (i < lines.length && lines[i].type === "delete") {
deletes.push(lines[i]);
i++;
}

// Collect consecutive inserts that follow
const inserts: DiffLine[] = [];
while (i < lines.length && lines[i].type === "insert") {
inserts.push(lines[i]);
i++;
}

// Pair them up
const maxLen = Math.max(deletes.length, inserts.length);
for (let j = 0; j < maxLen; j++) {
const del = deletes[j] || null;
Expand All @@ -2054,7 +2032,6 @@ const DiffViewer = memo(function DiffViewer({
});
}
} else if (line.type === "insert") {
// Standalone insert (no preceding delete)
pairs.push({
left: null,
right: line,
Expand Down Expand Up @@ -2167,6 +2144,27 @@ const DiffViewer = memo(function DiffViewer({
} else {
const artifact = hunk.isRebaseArtifact;
if (viewMode === "split") {
// DEBUG: log all lines for every hunk
if (hunk.lines && hunk.lines.length > 0) {
const firstOld = hunk.lines.find(
(l: any) => l.oldLineNumber != null
)?.oldLineNumber;
const firstNew = hunk.lines.find(
(l: any) => l.newLineNumber != null
)?.newLineNumber;
console.log(
`DEBUG hunk lines (firstOld=${firstOld}, firstNew=${firstNew}, count=${hunk.lines.length}):`
);
hunk.lines.forEach((l: any, i: number) => {
const t = l.type;
const o = l.oldLineNumber ?? l.lineNumber ?? "?";
const n = l.newLineNumber ?? "?";
const v = l.content?.map((s: any) => s.value).join("") ?? "";
console.log(
` [${i}] ${t} old=${o} new=${n} content="${v.substring(0, 80)}"`
);
});
}
// Convert to split pairs
const pairs = convertToSplitPairs(hunk.lines);
for (const pair of pairs) {
Expand Down
Loading
Loading