From ab0a9047f4559fd3af9458ec02b3e217fb1ca6bb Mon Sep 17 00:00:00 2001 From: murashit Date: Tue, 21 Jul 2026 19:48:42 +0900 Subject: [PATCH] refactor(diff): replace custom LCS with jsdiff --- package-lock.json | 10 +++ package.json | 1 + src/features/selection-rewrite/diff.ts | 66 +++++-------------- .../selection-rewrite/popover.dom.tsx | 21 +++--- src/shared/ui/diff-view.tsx | 48 ++++---------- .../selection-rewrite.test.ts | 42 ++++++------ tests/features/turn-diff/turn-diff.test.ts | 14 ++++ 7 files changed, 83 insertions(+), 119 deletions(-) diff --git a/package-lock.json b/package-lock.json index 29d75505..e1a25b62 100644 --- a/package-lock.json +++ b/package-lock.json @@ -11,6 +11,7 @@ "dependencies": { "@tanstack/query-core": "^5.101.2", "defuddle": "^0.19.1", + "diff": "^9.0.0", "mdast-util-from-markdown": "^2.0.3", "mdast-util-to-markdown": "^2.1.2", "micromark": "^4.0.2", @@ -5368,6 +5369,15 @@ "url": "https://github.com/sponsors/wooorm" } }, + "node_modules/diff": { + "version": "9.0.0", + "resolved": "https://registry.npmjs.org/diff/-/diff-9.0.0.tgz", + "integrity": "sha512-svtcdpS8CgJyqAjEQIXdb3OjhFVVYjzGAPO8WGCmRbrml64SPw/jJD4GoE98aR7r25A0XcgrK3F02yw9R/vhQw==", + "license": "BSD-3-Clause", + "engines": { + "node": ">=0.3.1" + } + }, "node_modules/diff-match-patch": { "version": "1.0.5", "resolved": "https://registry.npmjs.org/diff-match-patch/-/diff-match-patch-1.0.5.tgz", diff --git a/package.json b/package.json index 585f80e6..4b19be4c 100644 --- a/package.json +++ b/package.json @@ -66,6 +66,7 @@ "dependencies": { "@tanstack/query-core": "^5.101.2", "defuddle": "^0.19.1", + "diff": "^9.0.0", "mdast-util-from-markdown": "^2.0.3", "mdast-util-to-markdown": "^2.1.2", "micromark": "^4.0.2", diff --git a/src/features/selection-rewrite/diff.ts b/src/features/selection-rewrite/diff.ts index ae0d6e5e..3c03b507 100644 --- a/src/features/selection-rewrite/diff.ts +++ b/src/features/selection-rewrite/diff.ts @@ -1,20 +1,13 @@ -export function buildSelectionUnifiedDiff(filePath: string, originalText: string, replacementText: string): string { +import { diffArrays } from "diff"; + +export function buildSelectionDiffLines(originalText: string, replacementText: string): string[] { const originalLines = textLines(originalText); const replacementLines = textLines(replacementText); const changes = lineChanges(originalLines, replacementLines); - const oldCount = Math.max(originalLines.length, 1); - const newCount = Math.max(replacementLines.length, 1); - - return [ - `diff --git a/${filePath} b/${filePath}`, - `--- a/${filePath}`, - `+++ b/${filePath}`, - `@@ -1,${String(oldCount)} +1,${String(newCount)} @@`, - ...changes.map((change) => `${change.prefix}${change.text}`), - ].join("\n"); + return changes.map((change) => `${change.prefix}${change.text}`); } -const MAX_SELECTION_REWRITE_LCS_CELLS = 40_000; +const MAX_SELECTION_REWRITE_EDIT_LENGTH = 400; interface DiffLine { prefix: " " | "+" | "-"; @@ -29,31 +22,17 @@ function textLines(text: string): string[] { } function lineChanges(originalLines: string[], replacementLines: string[]): DiffLine[] { - if (originalLines.length * replacementLines.length > MAX_SELECTION_REWRITE_LCS_CELLS) { - return linearLineChanges(originalLines, replacementLines); - } + const arrayChanges = diffArrays(originalLines, replacementLines, { + maxEditLength: MAX_SELECTION_REWRITE_EDIT_LENGTH, + }); + if (!arrayChanges) return linearLineChanges(originalLines, replacementLines); - const lengths = lcsLengths(originalLines, replacementLines); - const changes: DiffLine[] = []; - let oldIndex = 0; - let newIndex = 0; - - while (oldIndex < originalLines.length || newIndex < replacementLines.length) { - if (oldIndex < originalLines.length && newIndex < replacementLines.length && originalLines[oldIndex] === replacementLines[newIndex]) { - changes.push({ prefix: " ", text: originalLines[oldIndex] ?? "" }); - oldIndex += 1; - newIndex += 1; - } else if ( - newIndex < replacementLines.length && - (oldIndex === originalLines.length || (lengths[oldIndex]?.[newIndex + 1] ?? 0) > (lengths[oldIndex + 1]?.[newIndex] ?? 0)) - ) { - changes.push({ prefix: "+", text: replacementLines[newIndex] ?? "" }); - newIndex += 1; - } else if (oldIndex < originalLines.length) { - changes.push({ prefix: "-", text: originalLines[oldIndex] ?? "" }); - oldIndex += 1; - } - } + const changes = arrayChanges.flatMap((change) => + change.value.map((text) => ({ + prefix: change.added ? "+" : change.removed ? "-" : " ", + text, + })), + ); return changes.length > 0 ? changes : [{ prefix: " ", text: "" }]; } @@ -96,18 +75,3 @@ function pushLines(changes: DiffLine[], prefix: DiffLine["prefix"], lines: strin changes.push({ prefix, text: lines[index] ?? "" }); } } - -function lcsLengths(left: string[], right: string[]): number[][] { - const rows = Array.from({ length: left.length + 1 }, () => Array.from({ length: right.length + 1 }, () => 0)); - for (let leftIndex = left.length - 1; leftIndex >= 0; leftIndex -= 1) { - for (let rightIndex = right.length - 1; rightIndex >= 0; rightIndex -= 1) { - const row = rows[leftIndex]; - if (row === undefined) continue; - row[rightIndex] = - left[leftIndex] === right[rightIndex] - ? (rows[leftIndex + 1]?.[rightIndex + 1] ?? 0) + 1 - : Math.max(rows[leftIndex + 1]?.[rightIndex] ?? 0, rows[leftIndex]?.[rightIndex + 1] ?? 0); - } - } - return rows; -} diff --git a/src/features/selection-rewrite/popover.dom.tsx b/src/features/selection-rewrite/popover.dom.tsx index 27c618f7..507ec53e 100644 --- a/src/features/selection-rewrite/popover.dom.tsx +++ b/src/features/selection-rewrite/popover.dom.tsx @@ -6,8 +6,8 @@ import { renderUiRoot, unmountUiRoot } from "../../shared/dom/preact-root.dom"; import { syncTextareaHeight } from "../../shared/dom/textarea-autogrow.measure"; import { textareaCursorAtVisualBoundary } from "../../shared/dom/textarea-caret.measure"; import { IconButton } from "../../shared/obsidian/components.obsidian"; -import { DiffLineList, unifiedDiffDisplayLines } from "../../shared/ui/diff-view"; -import { buildSelectionUnifiedDiff } from "./diff"; +import { DiffLineList } from "../../shared/ui/diff-view"; +import { buildSelectionDiffLines } from "./diff"; import { canApplySelectionRewrite, type SelectionRewriteInstructionHistoryDirection, @@ -201,7 +201,7 @@ export class SelectionRewritePopover { elements.applyButton = element; }} debugText={state.debugText} - diff={replacement === null ? null : buildSelectionUnifiedDiff(state.filePath, state.originalText, replacement)} + diffLines={replacement === null ? null : buildSelectionDiffLines(state.originalText, replacement)} generating={state.status === "generating"} hasInstruction={this.session.hasInstruction} hasReplacement={replacement !== null} @@ -306,7 +306,7 @@ function selectionRewriteInstructionCursorOnLogicalBoundary( interface SelectionRewritePopoverViewProps { applyButtonRef: (element: HTMLButtonElement | null) => void; debugText: string | null; - diff: string | null; + diffLines: readonly string[] | null; generating: boolean; hasInstruction: boolean; hasReplacement: boolean; @@ -324,7 +324,7 @@ interface SelectionRewritePopoverViewProps { function SelectionRewritePopoverView({ applyButtonRef, debugText, - diff, + diffLines, generating, hasInstruction, hasReplacement, @@ -365,7 +365,7 @@ function SelectionRewritePopoverView({ 
{streamPreview}
-
{diff ? : null}
+
{diffLines ? : null}
line.kind !== "file" && !line.text.startsWith("@@"))} - className="codex-panel-selection-rewrite__diff-body" - /> - ); +function SelectionRewriteDiff({ lines }: { lines: readonly string[] }): UiNode { + return ({ text }))} className="codex-panel-selection-rewrite__diff-body" />; } diff --git a/src/shared/ui/diff-view.tsx b/src/shared/ui/diff-view.tsx index 41c6f818..5114d5e4 100644 --- a/src/shared/ui/diff-view.tsx +++ b/src/shared/ui/diff-view.tsx @@ -1,7 +1,9 @@ +import { diffArrays } from "diff"; import type { ComponentChild as UiNode } from "preact"; const MAX_INLINE_DIFF_CHARS = 4000; const MAX_INLINE_DIFF_TOKENS = 500; +const MAX_INLINE_DIFF_EDIT_LENGTH = MAX_INLINE_DIFF_TOKENS; export interface DiffDisplayLine { text: string; @@ -229,29 +231,18 @@ function inlineDiff(removedText: string, addedText: string): InlineDiff | null { const addedTokens = segmentWords(addedText); if (removedTokens.length + addedTokens.length > MAX_INLINE_DIFF_TOKENS) return null; - const lengths = lcsLengths(removedTokens, addedTokens); + const changes = diffArrays(removedTokens, addedTokens, { + maxEditLength: MAX_INLINE_DIFF_EDIT_LENGTH, + }); + if (!changes) return null; + const removed: InlineDiffPart[] = []; const added: InlineDiffPart[] = []; - let removedIndex = 0; - let addedIndex = 0; - - while (removedIndex < removedTokens.length || addedIndex < addedTokens.length) { - const removedToken = removedTokens[removedIndex]; - const addedToken = addedTokens[addedIndex]; - if (removedToken !== undefined && addedToken !== undefined && removedToken === addedToken) { - removed.push({ text: removedToken, changed: false }); - added.push({ text: addedToken, changed: false }); - removedIndex += 1; - addedIndex += 1; - } else if ( - addedToken !== undefined && - (removedToken === undefined || (lengths[removedIndex]?.[addedIndex + 1] ?? 0) >= (lengths[removedIndex + 1]?.[addedIndex] ?? 0)) - ) { - added.push({ text: addedToken, changed: true }); - addedIndex += 1; - } else if (removedToken !== undefined) { - removed.push({ text: removedToken, changed: true }); - removedIndex += 1; + for (const change of changes) { + const changed = change.added || change.removed; + for (const text of change.value) { + if (!change.added) removed.push({ text, changed }); + if (!change.removed) added.push({ text, changed }); } } @@ -263,21 +254,6 @@ function segmentWords(text: string): string[] { return Array.from(segmenter.segment(text), (segment) => segment.segment); } -function lcsLengths(left: string[], right: string[]): number[][] { - const rows = Array.from({ length: left.length + 1 }, () => Array.from({ length: right.length + 1 }, () => 0)); - for (let leftIndex = left.length - 1; leftIndex >= 0; leftIndex -= 1) { - for (let rightIndex = right.length - 1; rightIndex >= 0; rightIndex -= 1) { - const row = rows[leftIndex]; - if (!row) continue; - row[rightIndex] = - left[leftIndex] === right[rightIndex] - ? (rows[leftIndex + 1]?.[rightIndex + 1] ?? 0) + 1 - : Math.max(rows[leftIndex + 1]?.[rightIndex] ?? 0, rows[leftIndex]?.[rightIndex + 1] ?? 0); - } - } - return rows; -} - function mergeInlineParts(parts: InlineDiffPart[]): InlineDiffPart[] { const merged: InlineDiffPart[] = []; for (const part of parts) { diff --git a/tests/features/selection-rewrite/selection-rewrite.test.ts b/tests/features/selection-rewrite/selection-rewrite.test.ts index 3722463b..397c699f 100644 --- a/tests/features/selection-rewrite/selection-rewrite.test.ts +++ b/tests/features/selection-rewrite/selection-rewrite.test.ts @@ -5,7 +5,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import type { TurnRecord } from "../../../src/app-server/protocol/turn"; import type { EphemeralStructuredTurnRunner } from "../../../src/app-server/services/ephemeral-structured-turn"; import { createAppServerSelectionRewriteTransport } from "../../../src/features/selection-rewrite/app-server-transport"; -import { buildSelectionUnifiedDiff } from "../../../src/features/selection-rewrite/diff"; +import { buildSelectionDiffLines } from "../../../src/features/selection-rewrite/diff"; import { canApplySelectionRewrite, type SelectionRewriteState } from "../../../src/features/selection-rewrite/model"; import { SelectionRewriteOutputError, selectionRewriteOutputParseResultFromText } from "../../../src/features/selection-rewrite/output"; import { SelectionRewritePopover } from "../../../src/features/selection-rewrite/popover.dom"; @@ -90,24 +90,17 @@ describe("selection rewrite prompt", () => { }); describe("selection rewrite diff", () => { - it("renders replacements in a selection-scoped unified diff", () => { - const diff = buildSelectionUnifiedDiff("Note.md", "alpha\nbeta", "alpha\ngamma"); - - expect(diff).toContain("diff --git a/Note.md b/Note.md"); - expect(diff).toContain("@@ -1,2 +1,2 @@"); - expect(diff).toContain(" alpha"); - expect(diff).toContain("-beta"); - expect(diff).toContain("+gamma"); + it("builds selection-scoped display lines", () => { + expect(buildSelectionDiffLines("alpha\nbeta", "alpha\ngamma")).toEqual([" alpha", "-beta", "+gamma"]); }); it("orders full replacement blocks as removals before additions", () => { - const diff = buildSelectionUnifiedDiff( - "Note.md", + const diff = buildSelectionDiffLines( "これはdiffのテストです。\n今日は元気です。\nとても元気です。", "これはdiffのてすとです。\nきょうはげんきです。\nとてもげんきです。", ); - expect(diff).toContain( + expect(diff.join("\n")).toContain( [ "-これはdiffのテストです。", "-今日は元気です。", @@ -119,7 +112,18 @@ describe("selection rewrite diff", () => { ); }); - it("bounds large selection diffs to a linear prefix and suffix comparison", () => { + it("keeps accurate context for large selections with a small edit distance", () => { + const commonBefore = Array.from({ length: 160 }, (_unused, index) => `common before ${String(index)}`); + const commonAfter = Array.from({ length: 160 }, (_unused, index) => `common after ${String(index)}`); + const originalText = ["same start", ...commonBefore, "old line", "shared middle", ...commonAfter, "same end"].join("\n"); + const replacementText = ["same start", ...commonBefore, "new line", "shared middle", ...commonAfter, "same end"].join("\n"); + + const diff = buildSelectionDiffLines(originalText, replacementText).join("\n"); + + expect(diff).toContain("\n-old line\n+new line\n shared middle\n"); + }); + + it("bounds high-edit-distance selection diffs with a linear prefix and suffix fallback", () => { const originalText = [ "same start", ...Array.from({ length: 160 }, (_unused, index) => `old before ${String(index)}`), @@ -135,23 +139,23 @@ describe("selection rewrite diff", () => { "same end", ].join("\n"); - const diff = buildSelectionUnifiedDiff("Note.md", originalText, replacementText); + const diff = buildSelectionDiffLines(originalText, replacementText).join("\n"); - expect(diff).toContain("\n same start\n"); + expect(diff.startsWith(" same start\n")).toBe(true); expect(diff).toContain("\n-old before 0\n"); expect(diff).toContain("\n-shared middle\n"); expect(diff).toContain("\n+shared middle\n"); - expect(diff).toContain("\n same end"); + expect(diff.endsWith("\n same end")).toBe(true); expect(diff).not.toContain("\n shared middle\n"); }); it("renders additions and deletions", () => { - expect(buildSelectionUnifiedDiff("Note.md", "", "added")).toContain("+added"); - expect(buildSelectionUnifiedDiff("Note.md", "removed", "")).toContain("-removed"); + expect(buildSelectionDiffLines("", "added")).toContain("+added"); + expect(buildSelectionDiffLines("removed", "")).toContain("-removed"); }); it("renders unchanged text as context", () => { - expect(buildSelectionUnifiedDiff("Note.md", "same", "same")).toContain(" same"); + expect(buildSelectionDiffLines("same", "same")).toContain(" same"); }); }); diff --git a/tests/features/turn-diff/turn-diff.test.ts b/tests/features/turn-diff/turn-diff.test.ts index da0f32fe..508655cf 100644 --- a/tests/features/turn-diff/turn-diff.test.ts +++ b/tests/features/turn-diff/turn-diff.test.ts @@ -82,6 +82,20 @@ describe("turn diff view decisions", () => { expect(parent.querySelector(".codex-panel-diff__word--added")?.textContent).toBe("犬"); }); + it("preserves whitespace changes from Intl.Segmenter tokenization", () => { + const parent = document.createElement("div"); + + renderTurnDiffView(parent, { + threadId: "thread", + turnId: "turn", + files: ["Note.md"], + diff: "diff --git a/Note.md b/Note.md\n@@\n-alpha beta\n+alpha beta", + }); + + expect(parent.querySelector(".codex-panel-diff__word--removed")?.textContent).toBe(" "); + expect(parent.querySelector(".codex-panel-diff__word--added")?.textContent).toBe(" "); + }); + it("pairs changed words by line inside multi-line replacement blocks", () => { const parent = document.createElement("div");