mirror of
https://github.com/coder/coder.git
synced 2026-09-21 20:51:01 +08:00
fix: derive diff cache keys from patch content (#27987)
This commit is contained in:
+24
@@ -52,6 +52,15 @@
|
||||
"message": "React 19 no longer requires forwardRef. Use ref as a prop instead.",
|
||||
"importNames": ["forwardRef"]
|
||||
},
|
||||
"@pierre/diffs": {
|
||||
"message": "Parse diffs through parseDiffString so every rendered file carries a content-derived cache key (pierrecomputer/pierre#1052).",
|
||||
"importNames": [
|
||||
"parsePatchFiles",
|
||||
"processPatch",
|
||||
"processFile",
|
||||
"parseDiffFromFile"
|
||||
]
|
||||
},
|
||||
"@mui/material/Alert": "Use components/Alert/Alert instead.",
|
||||
"@mui/material/AlertTitle": "Use components/Alert/Alert instead.",
|
||||
// "@mui/material/Autocomplete": "Use shadcn/ui Combobox instead.",
|
||||
@@ -160,6 +169,21 @@
|
||||
}
|
||||
}
|
||||
}
|
||||
},
|
||||
{
|
||||
// parseDiff.ts is the single choke point allowed to import
|
||||
// the raw @pierre/diffs parsers.
|
||||
"includes": [
|
||||
"**/DiffViewer/parseDiff.ts",
|
||||
"**/DiffViewer/parseDiff.test.ts"
|
||||
],
|
||||
"linter": {
|
||||
"rules": {
|
||||
"style": {
|
||||
"noRestrictedImports": "off"
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
],
|
||||
"$schema": "./node_modules/@biomejs/biome/configuration_schema.json"
|
||||
|
||||
@@ -27,6 +27,7 @@ import {
|
||||
parseServerEditDiffText,
|
||||
parseServerEditResults,
|
||||
sanitizeExecuteModelIntent,
|
||||
stripNoNewline,
|
||||
stripSvnIndexHeaders,
|
||||
summarizeParsedCommands,
|
||||
} from "./utils";
|
||||
@@ -677,9 +678,9 @@ describe("parseEditFilesArgs", () => {
|
||||
expect(diff).not.toBeNull();
|
||||
});
|
||||
|
||||
// search uses required() (rejects "") while replace uses
|
||||
// defined() (allows ""). This asymmetry is intentional:
|
||||
// empty search is meaningless, empty replace is a deletion.
|
||||
// normalizeEdit drops edits with an empty search but keeps an
|
||||
// empty replace: an empty search is meaningless, an empty
|
||||
// replace is a deletion.
|
||||
it("rejects edits with empty-string search", () => {
|
||||
const args = {
|
||||
files: [
|
||||
@@ -804,6 +805,10 @@ describe("buildEditDiff", () => {
|
||||
{ search: "const y = 3;", replace: "const y = 4;" },
|
||||
]);
|
||||
expect(diff).not.toBeNull();
|
||||
// Guards the regression that dropped edits after the first.
|
||||
const added = diff!.additionLines.join("");
|
||||
expect(added).toContain("const x = 2;");
|
||||
expect(added).toContain("const y = 4;");
|
||||
});
|
||||
|
||||
it("does not emit console errors for multi-edit diffs", () => {
|
||||
@@ -853,6 +858,40 @@ describe("buildEditDiff", () => {
|
||||
expect(diff).not.toBeNull();
|
||||
});
|
||||
|
||||
it("does not count the no-newline pragma in hunk headers", () => {
|
||||
const diff = buildEditDiff("file.ts", [{ search: "old", replace: "new" }]);
|
||||
expect(diff).not.toBeNull();
|
||||
// One-line replacement: the header must be 1/1, not 2/2 with the
|
||||
// \ No newline pragma counted as a source line.
|
||||
expect(diff!.hunks[0].deletionCount).toBe(1);
|
||||
expect(diff!.hunks[0].additionCount).toBe(1);
|
||||
});
|
||||
|
||||
it("does not manufacture blank lines at edit seams", () => {
|
||||
// search ends in a newline, replace does not; a deletion edit
|
||||
// sits beside a normal one. Neither may inject a blank line.
|
||||
const diff = buildEditDiff("file.ts", [
|
||||
{ search: "old\n", replace: "new" },
|
||||
{ search: "a", replace: "" },
|
||||
{ search: "b", replace: "c" },
|
||||
]);
|
||||
expect(diff).not.toBeNull();
|
||||
expect(diff!.deletionLines).toEqual(["old\n", "a", "b"]);
|
||||
expect(diff!.additionLines).toEqual(["new", "c"]);
|
||||
});
|
||||
|
||||
it("never correlates lines across edits", () => {
|
||||
// One edit deletes a block, another inserts the same block;
|
||||
// each edit must render as its own change, not as shared context.
|
||||
const diff = buildEditDiff("file.ts", [
|
||||
{ search: "foo\nbar\nbaz", replace: "" },
|
||||
{ search: "ctx", replace: "ctx\nfoo\nbar\nbaz" },
|
||||
]);
|
||||
expect(diff).not.toBeNull();
|
||||
expect(diff!.deletionLines).toEqual(["foo\n", "bar\n", "baz", "ctx"]);
|
||||
expect(diff!.additionLines).toEqual(["ctx\n", "foo\n", "bar\n", "baz"]);
|
||||
});
|
||||
|
||||
it("handles replace with trailing newline (trailing empty popped)", () => {
|
||||
const diff = buildEditDiff("file.ts", [
|
||||
{ search: "old\n", replace: "new\n" },
|
||||
@@ -874,6 +913,34 @@ describe("buildEditDiff", () => {
|
||||
const hasContext = hunk.hunkContent.some((c) => c.type === "context");
|
||||
expect(hasContext).toBe(true);
|
||||
});
|
||||
|
||||
it("keys diffs by patch content, not by file name alone", () => {
|
||||
const first = buildEditDiff("file.ts", [{ search: "old", replace: "new" }]);
|
||||
const sameAgain = buildEditDiff("file.ts", [
|
||||
{ search: "old", replace: "new" },
|
||||
]);
|
||||
const different = buildEditDiff("file.ts", [
|
||||
{ search: "old", replace: "other" },
|
||||
]);
|
||||
expect(first?.cacheKey).toMatch(/^content-/);
|
||||
expect(first?.cacheKey).toBe(sameAgain?.cacheKey);
|
||||
expect(first?.cacheKey).not.toBe(different?.cacheKey);
|
||||
});
|
||||
|
||||
it("restamps the cache key when stripNoNewline clears the flags", () => {
|
||||
const diff = buildEditDiff("file.ts", [{ search: "old", replace: "new" }]);
|
||||
expect(diff).not.toBeNull();
|
||||
// Pin the flags so the test does not depend on jsdiff's
|
||||
// no-newline marker emission for this fixture.
|
||||
for (const hunk of diff!.hunks) {
|
||||
hunk.noEOFCRDeletions = true;
|
||||
hunk.noEOFCRAdditions = true;
|
||||
}
|
||||
|
||||
const stripped = stripNoNewline(diff!);
|
||||
|
||||
expect(stripped.cacheKey).not.toBe(diff!.cacheKey);
|
||||
});
|
||||
});
|
||||
|
||||
describe("stripSvnIndexHeaders", () => {
|
||||
|
||||
@@ -1,7 +1,7 @@
|
||||
import type { FileDiffMetadata } from "@pierre/diffs";
|
||||
import { parsePatchFiles } from "@pierre/diffs";
|
||||
import * as Diff from "diff";
|
||||
import * as Yup from "yup";
|
||||
import { parseDiffString, stampCacheKey } from "../../DiffViewer/parseDiff";
|
||||
import { asRecord, asString, isValid } from "../runtimeTypeUtils";
|
||||
|
||||
export type ToolStatus = "completed" | "error" | "running";
|
||||
@@ -348,7 +348,7 @@ export function stripNoNewline(fileDiff: FileDiffMetadata): FileDiffMetadata {
|
||||
(h) => h.noEOFCRDeletions || h.noEOFCRAdditions,
|
||||
);
|
||||
if (!needsStrip) return fileDiff;
|
||||
return {
|
||||
const stripped = {
|
||||
...fileDiff,
|
||||
hunks: fileDiff.hunks.map((h) => ({
|
||||
...h,
|
||||
@@ -356,6 +356,8 @@ export function stripNoNewline(fileDiff: FileDiffMetadata): FileDiffMetadata {
|
||||
noEOFCRAdditions: false,
|
||||
})),
|
||||
};
|
||||
stampCacheKey(stripped);
|
||||
return stripped;
|
||||
}
|
||||
|
||||
export function getFileViewerOptions(isDark: boolean) {
|
||||
@@ -467,9 +469,7 @@ export const getFileContentForViewer = (
|
||||
*/
|
||||
const parseSingleFileDiff = (raw: string): FileDiffMetadata | null => {
|
||||
if (!raw) return null;
|
||||
const parsed = parsePatchFiles(stripSvnIndexHeaders(raw));
|
||||
if (!parsed.length || !parsed[0].files.length) return null;
|
||||
return parsed[0].files[0];
|
||||
return parseDiffString(stripSvnIndexHeaders(raw))[0] ?? null;
|
||||
};
|
||||
|
||||
/**
|
||||
@@ -534,36 +534,67 @@ export const parseEditFilesArgs = (args: unknown): EditFilesFileEntry[] => {
|
||||
};
|
||||
|
||||
/**
|
||||
* Builds a synthetic unified diff from edit pairs (normalized to
|
||||
* search/replace) for a single file. Each edit becomes a separate
|
||||
* `Diff.createPatch` call; the patches are concatenated and
|
||||
* parsed into a single FileDiffMetadata.
|
||||
* Builds a synthetic unified diff from edit pairs for a single file.
|
||||
* Each edit is diffed against only its own snippet, so lines can never
|
||||
* correlate across edits.
|
||||
*/
|
||||
export const buildEditDiff = (
|
||||
path: string,
|
||||
edits: Array<{ search: string; replace: string }>,
|
||||
): FileDiffMetadata | null => {
|
||||
if (!edits.length) return null;
|
||||
const kept = edits.filter((edit) => edit.search);
|
||||
|
||||
// Strip leading slash so the a/ and b/ prefixes don't
|
||||
// produce a double-slash that confuses the diff parser.
|
||||
const diffPath = path.startsWith("/") ? path.slice(1) : path;
|
||||
|
||||
const patches: string[] = [];
|
||||
for (const edit of edits) {
|
||||
if (!edit.search) continue;
|
||||
patches.push(Diff.createPatch(diffPath, edit.search, edit.replace, "", ""));
|
||||
}
|
||||
if (!patches.length) {
|
||||
// All edits were skipped (empty search). Produce a
|
||||
// header-only patch so the parser still returns a file
|
||||
// entry with zero hunks.
|
||||
patches.push(`--- ${diffPath}\n+++ ${diffPath}\n`);
|
||||
if (!kept.length) {
|
||||
// An empty patch still parses to a file entry with zero hunks.
|
||||
return parseSingleFileDiff(Diff.createPatch(diffPath, "", "", "", ""));
|
||||
}
|
||||
|
||||
return parseSingleFileDiff(patches.join(""));
|
||||
let oldOffset = 0;
|
||||
let newOffset = 0;
|
||||
const hunks: Array<Diff.StructuredPatchHunk> = [];
|
||||
for (const edit of kept) {
|
||||
const structured = Diff.structuredPatch(
|
||||
diffPath,
|
||||
diffPath,
|
||||
edit.search,
|
||||
edit.replace,
|
||||
"",
|
||||
"",
|
||||
);
|
||||
for (const hunk of structured.hunks) {
|
||||
hunks.push({
|
||||
...hunk,
|
||||
oldStart: hunk.oldStart + oldOffset,
|
||||
newStart: hunk.newStart + newOffset,
|
||||
});
|
||||
}
|
||||
oldOffset += snippetLineCount(edit.search);
|
||||
newOffset += snippetLineCount(edit.replace);
|
||||
}
|
||||
|
||||
const lines = [`--- ${diffPath}`, `+++ ${diffPath}`];
|
||||
for (const hunk of hunks) {
|
||||
lines.push(
|
||||
`@@ -${hunk.oldStart},${hunk.oldLines} +${hunk.newStart},${hunk.newLines} @@`,
|
||||
);
|
||||
lines.push(...hunk.lines);
|
||||
}
|
||||
return parseSingleFileDiff(`${lines.join("\n")}\n`);
|
||||
};
|
||||
|
||||
// Lines a snippet contributes to the synthetic file: trailing newlines
|
||||
// terminate the last line rather than starting another, and an empty
|
||||
// snippet contributes none.
|
||||
const snippetLineCount = (snippet: string): number =>
|
||||
snippet === ""
|
||||
? 0
|
||||
: snippet.split("\n").length - (snippet.endsWith("\n") ? 1 : 0);
|
||||
|
||||
/**
|
||||
* Per-file result from the agent's FileEditResponse. `path` matches
|
||||
* the caller-supplied path (pre-symlink resolution). `diff` is a
|
||||
|
||||
@@ -1,9 +1,11 @@
|
||||
import type { DiffLineAnnotation, SelectedLineRange } from "@pierre/diffs";
|
||||
import { parsePatchFiles } from "@pierre/diffs";
|
||||
import { FileDiff } from "@pierre/diffs/react";
|
||||
import type { Meta, StoryObj } from "@storybook/react-vite";
|
||||
import { expect, fn, waitFor } from "storybook/test";
|
||||
import { type FC, useState } from "react";
|
||||
import { expect, fn, userEvent, waitFor, within } from "storybook/test";
|
||||
import type { DiffStyle } from "../DiffViewer/DiffViewer";
|
||||
import { DiffViewer } from "../DiffViewer/DiffViewer";
|
||||
import { parseDiffString } from "../DiffViewer/parseDiff";
|
||||
import { InlinePromptInput } from "../DiffViewer/RemoteDiffPanel";
|
||||
import { generateLargeDiff } from "./testHelpers";
|
||||
|
||||
@@ -32,7 +34,7 @@ const sampleDiff = [
|
||||
"+ return app;",
|
||||
" }",
|
||||
].join("\n");
|
||||
const parsedFiles = parsePatchFiles(sampleDiff).flatMap((p) => p.files);
|
||||
const parsedFiles = parseDiffString(sampleDiff);
|
||||
const firstFileName = parsedFiles[0]?.name ?? "";
|
||||
|
||||
const meta: Meta<typeof DiffViewer> = {
|
||||
@@ -118,7 +120,7 @@ const multiHunkDiff = [
|
||||
"+ metrics.record(\"server.start\");",
|
||||
" });",
|
||||
].join("\n");
|
||||
const multiHunkFiles = parsePatchFiles(multiHunkDiff).flatMap((p) => p.files);
|
||||
const multiHunkFiles = parseDiffString(multiHunkDiff);
|
||||
|
||||
export const WithMidFileSeparator: Story = {
|
||||
args: {
|
||||
@@ -169,7 +171,7 @@ const changeDiff = [
|
||||
" debug: false,",
|
||||
" };",
|
||||
].join("\n");
|
||||
const changeFiles = parsePatchFiles(changeDiff).flatMap((p) => p.files);
|
||||
const changeFiles = parseDiffString(changeDiff);
|
||||
const changeFileName = changeFiles[0]?.name ?? "";
|
||||
|
||||
// Regression test: in split view, selecting from one side to the
|
||||
@@ -262,9 +264,7 @@ const mismatchedLinesDiff = [
|
||||
" cleanup();",
|
||||
" }",
|
||||
].join("\n");
|
||||
const mismatchedFiles = parsePatchFiles(mismatchedLinesDiff).flatMap(
|
||||
(p) => p.files,
|
||||
);
|
||||
const mismatchedFiles = parseDiffString(mismatchedLinesDiff);
|
||||
const mismatchedFileName = mismatchedFiles[0]?.name ?? "";
|
||||
|
||||
// Cross-side selection where deletion line 509 maps to addition
|
||||
@@ -332,9 +332,7 @@ const backwardSelectionDiff = [
|
||||
" ",
|
||||
" export function main() {",
|
||||
].join("\n");
|
||||
const backwardFiles = parsePatchFiles(backwardSelectionDiff).flatMap(
|
||||
(p) => p.files,
|
||||
);
|
||||
const backwardFiles = parseDiffString(backwardSelectionDiff);
|
||||
const backwardFileName = backwardFiles[0]?.name ?? "";
|
||||
|
||||
// Backward selection: start=9 > end=5 on the same side.
|
||||
@@ -423,7 +421,7 @@ const renameDiff = [
|
||||
"+ return <div />;",
|
||||
" }",
|
||||
].join("\n");
|
||||
const renameFiles = parsePatchFiles(renameDiff).flatMap((p) => p.files);
|
||||
const renameFiles = parseDiffString(renameDiff);
|
||||
|
||||
export const RenameWithLongPaths: Story = {
|
||||
args: {
|
||||
@@ -433,9 +431,7 @@ export const RenameWithLongPaths: Story = {
|
||||
|
||||
export const LargeDiff: Story = {
|
||||
args: {
|
||||
parsedFiles: parsePatchFiles(generateLargeDiff(40, 60)).flatMap(
|
||||
(p) => p.files,
|
||||
),
|
||||
parsedFiles: parseDiffString(generateLargeDiff(40, 60)),
|
||||
isExpanded: true,
|
||||
},
|
||||
decorators: [
|
||||
@@ -454,3 +450,87 @@ export const LargeDiff: Story = {
|
||||
});
|
||||
},
|
||||
};
|
||||
|
||||
// In production, before content-derived keys, the second render could hit
|
||||
// the worker-pool AST cached for the first body and throw "deletionLine and
|
||||
// additionLine are null". The storybook worker timing cannot reproduce that
|
||||
// collision window, so this story smoke-tests the re-render path instead:
|
||||
// the second body renders and no error box appears.
|
||||
const reparseFirstBody = [
|
||||
"--- a/src/hot.ts",
|
||||
"+++ b/src/hot.ts",
|
||||
"@@ -1,2 +1,2 @@",
|
||||
" export const keep = true;",
|
||||
"-const v = 1;",
|
||||
"+const v = 2;",
|
||||
].join("\n");
|
||||
const reparseSecondBody = [
|
||||
"--- a/src/hot.ts",
|
||||
"+++ b/src/hot.ts",
|
||||
"@@ -1,2 +1,5 @@",
|
||||
" export const keep = true;",
|
||||
"-const v = 1;",
|
||||
"+const v = 3;",
|
||||
"+const a = 1;",
|
||||
"+const b = 2;",
|
||||
"+const c = 3;",
|
||||
].join("\n");
|
||||
|
||||
// FileDiff renders hunks synchronously enough for play tests; CodeView
|
||||
// virtualizes and never paints lines in this environment.
|
||||
const ReparseSamePath: FC = () => {
|
||||
const [body, setBody] = useState(reparseFirstBody);
|
||||
const file = parseDiffString(body)[0];
|
||||
return (
|
||||
<div style={{ height: 400, width: 600 }}>
|
||||
<button type="button" onClick={() => setBody(reparseSecondBody)}>
|
||||
next body
|
||||
</button>
|
||||
{file && (
|
||||
<FileDiff
|
||||
fileDiff={file}
|
||||
options={{
|
||||
diffStyle: "unified",
|
||||
theme: "github-dark-high-contrast",
|
||||
themeType: "dark",
|
||||
}}
|
||||
/>
|
||||
)}
|
||||
</div>
|
||||
);
|
||||
};
|
||||
|
||||
export const ReparseSamePathAfterEdit: StoryObj = {
|
||||
render: () => <ReparseSamePath />,
|
||||
play: async ({ canvasElement }) => {
|
||||
const canvas = within(canvasElement);
|
||||
const shadowText = () =>
|
||||
Array.from(canvasElement.querySelectorAll("diffs-container"))
|
||||
.map((host) => host.shadowRoot?.textContent ?? "")
|
||||
.join("\n");
|
||||
const expectRendered = (text: string) =>
|
||||
waitFor(
|
||||
() => {
|
||||
// Checked inside the wait so a crash surfaces as the
|
||||
// error-box assertion instead of a text-timeout.
|
||||
expectNoErrorBox();
|
||||
expect(shadowText().includes(text)).toBe(true);
|
||||
},
|
||||
{
|
||||
timeout: 5000,
|
||||
},
|
||||
);
|
||||
const expectNoErrorBox = () =>
|
||||
expect(
|
||||
Array.from(canvasElement.querySelectorAll("diffs-container")).some(
|
||||
(host) => host.shadowRoot?.querySelector("[data-error-message]"),
|
||||
),
|
||||
).toBe(false);
|
||||
|
||||
await expectRendered("const v = 2");
|
||||
|
||||
await userEvent.click(canvas.getByRole("button", { name: "next body" }));
|
||||
|
||||
await expectRendered("const v = 3");
|
||||
},
|
||||
};
|
||||
|
||||
@@ -3,7 +3,7 @@ import type { WorkspaceAgentRepoChanges } from "#/api/typesGenerated";
|
||||
import type { ChatMessageInputRef } from "../AgentChatInput";
|
||||
import { CommentableDiffViewer } from "../DiffViewer/CommentableDiffViewer";
|
||||
import type { DiffStyle } from "../DiffViewer/DiffViewer";
|
||||
import { useParsedDiff } from "../DiffViewer/useParsedDiff";
|
||||
import { parseDiffString } from "../DiffViewer/parseDiff";
|
||||
|
||||
interface LocalDiffPanelProps {
|
||||
repo: WorkspaceAgentRepoChanges;
|
||||
@@ -18,7 +18,7 @@ export const LocalDiffPanel: FC<LocalDiffPanelProps> = ({
|
||||
diffStyle,
|
||||
chatInputRef,
|
||||
}) => {
|
||||
const parsedFiles = useParsedDiff(repo.unified_diff);
|
||||
const parsedFiles = parseDiffString(repo.unified_diff);
|
||||
|
||||
return (
|
||||
<CommentableDiffViewer
|
||||
|
||||
@@ -25,8 +25,7 @@ import type { ChatMessageInputRef } from "../AgentChatInput";
|
||||
import { CommentableDiffViewer } from "../DiffViewer/CommentableDiffViewer";
|
||||
import { DiffStatBadge } from "../DiffViewer/DiffStats";
|
||||
import type { DiffStyle } from "../DiffViewer/DiffViewer";
|
||||
import { getDiffCacheKeyPrefix } from "../DiffViewer/diffCacheKey";
|
||||
import { useParsedDiff } from "../DiffViewer/useParsedDiff";
|
||||
import { parseDiffString } from "../DiffViewer/parseDiff";
|
||||
|
||||
export { InlinePromptInput } from "../DiffViewer/CommentableDiffViewer";
|
||||
|
||||
@@ -126,17 +125,8 @@ export const RemoteDiffPanel: FC<RemoteDiffPanelProps> = ({
|
||||
});
|
||||
|
||||
const diffContent = diffContentsQuery.data?.diff;
|
||||
const dataUpdatedAt = diffContentsQuery.dataUpdatedAt;
|
||||
|
||||
// The @pierre/diffs worker pool only keys cached highlighted
|
||||
// ASTs by `cacheKey`, so the key must change whenever the diff
|
||||
// query updates. React Query's `dataUpdatedAt` survives panel
|
||||
// remounts, which prevents stale cache hits from pairing a new
|
||||
// FileDiffMetadata with an older highlighted AST.
|
||||
const parsedFiles = useParsedDiff(
|
||||
diffContent,
|
||||
getDiffCacheKeyPrefix(`chat-${chatId}`, dataUpdatedAt),
|
||||
);
|
||||
const parsedFiles = parseDiffString(diffContent);
|
||||
|
||||
// ---------------------------------------------------------------
|
||||
// Scroll-to-file from chat input chip clicks
|
||||
|
||||
@@ -1,21 +1,21 @@
|
||||
import { getDiffCacheKeyPrefix } from "./diffCacheKey";
|
||||
import { getContentCacheKey } from "./diffCacheKey";
|
||||
|
||||
describe("getDiffCacheKeyPrefix", () => {
|
||||
it("returns the same key for the same scope and query update time", () => {
|
||||
expect(getDiffCacheKeyPrefix("chat-123", 101)).toBe(
|
||||
getDiffCacheKeyPrefix("chat-123", 101),
|
||||
describe("getContentCacheKey", () => {
|
||||
it("returns the same key for identical text", () => {
|
||||
expect(getContentCacheKey("--- a\n+++ a\n")).toBe(
|
||||
getContentCacheKey("--- a\n+++ a\n"),
|
||||
);
|
||||
});
|
||||
|
||||
it("changes when the query update time changes", () => {
|
||||
expect(getDiffCacheKeyPrefix("chat-123", 101)).not.toBe(
|
||||
getDiffCacheKeyPrefix("chat-123", 202),
|
||||
it("returns different keys for different text", () => {
|
||||
expect(getContentCacheKey("--- a\n+++ a\n-x\n+y\n")).not.toBe(
|
||||
getContentCacheKey("--- a\n+++ a\n-x\n+z\n"),
|
||||
);
|
||||
});
|
||||
|
||||
it("changes when the scope changes", () => {
|
||||
expect(getDiffCacheKeyPrefix("chat-123", 101)).not.toBe(
|
||||
getDiffCacheKeyPrefix("chat-456", 101),
|
||||
it("formats keys as content-<hex hash>-<hex length>", () => {
|
||||
expect(getContentCacheKey("anything")).toMatch(
|
||||
/^content-[0-9a-f]+-[0-9a-f]+$/,
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1,12 +1,17 @@
|
||||
/**
|
||||
* Build a stable worker-pool cache key prefix for `@pierre/diffs`.
|
||||
*
|
||||
* We use React Query's `dataUpdatedAt` as the invalidation token instead of a
|
||||
* component-local counter. That timestamp survives component remounts, so a
|
||||
* freshly fetched diff cannot accidentally reuse a highlighted AST cached for
|
||||
* an older diff body.
|
||||
* The @pierre/diffs worker pool keys cached ASTs by each file's `cacheKey`,
|
||||
* which defaults to the file name (or prevName:name for renames) when
|
||||
* unset; hash the parsed content so different diff bodies for the same
|
||||
* path land on distinct keys, stable across re-renders and remounts.
|
||||
*/
|
||||
export const getDiffCacheKeyPrefix = (
|
||||
prefix: string,
|
||||
dataUpdatedAt: number,
|
||||
): string => `${prefix}-${dataUpdatedAt}`;
|
||||
export const getContentCacheKey = (text: string): string => {
|
||||
// FNV-1a plus the text length. The key only needs to separate different
|
||||
// diff bodies while an older AST is cached, so a 32-bit checksum is
|
||||
// plenty; crypto.subtle is async and cannot run in the render path.
|
||||
let hash = 0x811c9dc5;
|
||||
for (let i = 0; i < text.length; i++) {
|
||||
hash ^= text.charCodeAt(i);
|
||||
hash = Math.imul(hash, 0x01000193);
|
||||
}
|
||||
return `content-${(hash >>> 0).toString(16)}-${text.length.toString(16)}`;
|
||||
};
|
||||
|
||||
@@ -1,10 +1,6 @@
|
||||
import { parsePatchFiles } from "@pierre/diffs";
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { extractDiffContent } from "../DiffViewer/CommentableDiffViewer";
|
||||
|
||||
function parse(diffStr: string) {
|
||||
return parsePatchFiles(diffStr).flatMap((p) => p.files);
|
||||
}
|
||||
import { parseDiffString as parse } from "../DiffViewer/parseDiff";
|
||||
|
||||
/** Filter blank strings that arise from trailing newlines in parsed lines. */
|
||||
function contentLines(text: string): string[] {
|
||||
|
||||
@@ -0,0 +1,202 @@
|
||||
import { parsePatchFiles } from "@pierre/diffs";
|
||||
import { afterEach, describe, expect, it, vi } from "vitest";
|
||||
import { dedupeFilesByName, parseDiffString, stampCacheKey } from "./parseDiff";
|
||||
|
||||
// Two `diff --git` sections for the same post-image path. `parsePatchFiles`
|
||||
// emits one FileDiffMetadata per section, so this is the exact shape that made
|
||||
// CodeView.addItem throw `duplicate id "agent/x/agentmcp/api_internal_test.go"`
|
||||
// in production.
|
||||
const duplicateFileDiff = [
|
||||
"diff --git a/agent/x/agentmcp/api_internal_test.go b/agent/x/agentmcp/api_internal_test.go",
|
||||
"index 1111111..2222222 100644",
|
||||
"--- a/agent/x/agentmcp/api_internal_test.go",
|
||||
"+++ b/agent/x/agentmcp/api_internal_test.go",
|
||||
"@@ -1,3 +1,3 @@",
|
||||
" package agentmcp",
|
||||
"-const a = 1",
|
||||
"+const a = 2",
|
||||
" const b = 3",
|
||||
"diff --git a/agent/x/agentmcp/api_internal_test.go b/agent/x/agentmcp/api_internal_test.go",
|
||||
"index 3333333..4444444 100644",
|
||||
"--- a/agent/x/agentmcp/api_internal_test.go",
|
||||
"+++ b/agent/x/agentmcp/api_internal_test.go",
|
||||
"@@ -10,3 +10,3 @@",
|
||||
" const c = 4",
|
||||
"-const d = 5",
|
||||
"+const d = 6",
|
||||
" const e = 7",
|
||||
].join("\n");
|
||||
|
||||
const uniqueFilesDiff = [
|
||||
"diff --git a/first.ts b/first.ts",
|
||||
"index 1111111..2222222 100644",
|
||||
"--- a/first.ts",
|
||||
"+++ b/first.ts",
|
||||
"@@ -1,1 +1,1 @@",
|
||||
"-const a = 1",
|
||||
"+const a = 2",
|
||||
"diff --git a/second.ts b/second.ts",
|
||||
"index 3333333..4444444 100644",
|
||||
"--- a/second.ts",
|
||||
"+++ b/second.ts",
|
||||
"@@ -1,1 +1,1 @@",
|
||||
"-const b = 1",
|
||||
"+const b = 2",
|
||||
].join("\n");
|
||||
|
||||
function parse(diffStr: string) {
|
||||
return parsePatchFiles(diffStr).flatMap((p) => p.files);
|
||||
}
|
||||
|
||||
describe("dedupeFilesByName", () => {
|
||||
afterEach(() => {
|
||||
vi.restoreAllMocks();
|
||||
});
|
||||
|
||||
it("collapses repeated post-image paths to the first occurrence", () => {
|
||||
const warn = vi.spyOn(console, "warn").mockImplementation(() => {});
|
||||
const files = parse(duplicateFileDiff);
|
||||
// Sanity check: the parser really does hand us the duplicate that
|
||||
// crashes CodeView, so the dedupe below is exercising a real case.
|
||||
expect(files).toHaveLength(2);
|
||||
|
||||
const deduped = dedupeFilesByName(files);
|
||||
|
||||
expect(deduped).toHaveLength(1);
|
||||
expect(deduped[0]).toBe(files[0]);
|
||||
// Mapping to CodeView item ids (id: file.name) now yields no collision.
|
||||
expect(deduped.map((f) => f.name)).toEqual([
|
||||
"agent/x/agentmcp/api_internal_test.go",
|
||||
]);
|
||||
expect(warn).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it("preserves order and every file when paths are unique", () => {
|
||||
const warn = vi.spyOn(console, "warn").mockImplementation(() => {});
|
||||
const files = parse(uniqueFilesDiff);
|
||||
|
||||
const deduped = dedupeFilesByName(files);
|
||||
|
||||
expect(deduped).toEqual(files);
|
||||
expect(deduped.map((f) => f.name)).toEqual(["first.ts", "second.ts"]);
|
||||
expect(warn).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("returns an empty array unchanged", () => {
|
||||
expect(dedupeFilesByName([])).toEqual([]);
|
||||
});
|
||||
});
|
||||
|
||||
describe("parseDiffString", () => {
|
||||
it("returns an empty array for empty input", () => {
|
||||
expect(parseDiffString(null)).toEqual([]);
|
||||
expect(parseDiffString(undefined)).toEqual([]);
|
||||
expect(parseDiffString("")).toEqual([]);
|
||||
});
|
||||
|
||||
it("keys each file by its own parsed content", () => {
|
||||
const files = parseDiffString(uniqueFilesDiff);
|
||||
|
||||
expect(files).toHaveLength(2);
|
||||
for (const file of files) {
|
||||
expect(file.cacheKey).toMatch(/^content-/);
|
||||
}
|
||||
expect(files[0].cacheKey).not.toBe(files[1].cacheKey);
|
||||
});
|
||||
|
||||
it("keys identical bodies under different names distinctly", () => {
|
||||
// Both sides name the same file, so prevName is unset and the file
|
||||
// name is the only differing render input between the two.
|
||||
const tsBody = [
|
||||
"--- a.ts",
|
||||
"+++ a.ts",
|
||||
"@@ -1,1 +1,1 @@",
|
||||
"-const x = 1;",
|
||||
"+const x = 2;",
|
||||
].join("\n");
|
||||
const pyBody = tsBody.replaceAll("a.ts", "b.py");
|
||||
|
||||
const ts = parseDiffString(tsBody);
|
||||
const py = parseDiffString(pyBody);
|
||||
|
||||
// The deletion-side language comes from the file name, so a .ts
|
||||
// body must not reuse a .py highlight entry.
|
||||
expect(ts[0].cacheKey).not.toBe(py[0].cacheKey);
|
||||
});
|
||||
|
||||
it("keys renamed diffs by their source path", () => {
|
||||
const renameBody = (prev: string) =>
|
||||
[
|
||||
`diff --git a/${prev} b/src/new.ts`,
|
||||
"similarity index 95%",
|
||||
`rename from ${prev}`,
|
||||
"rename to src/new.ts",
|
||||
`--- a/${prev}`,
|
||||
"+++ b/src/new.ts",
|
||||
"@@ -1,1 +1,1 @@",
|
||||
"-const x = 1;",
|
||||
"+const x = 2;",
|
||||
].join("\n");
|
||||
|
||||
const fromOld = parseDiffString(renameBody("old.ts"));
|
||||
const fromOther = parseDiffString(renameBody("other.ts"));
|
||||
|
||||
expect(fromOld[0].name).toBe(fromOther[0].name);
|
||||
expect(fromOld[0].prevName).toBe("old.ts");
|
||||
expect(fromOther[0].prevName).toBe("other.ts");
|
||||
expect(fromOld[0].cacheKey).not.toBe(fromOther[0].cacheKey);
|
||||
});
|
||||
|
||||
it("keeps an unchanged file's key stable when a sibling changes", () => {
|
||||
const changed = uniqueFilesDiff.replace("const b = 2", "const b = 3");
|
||||
|
||||
const before = parseDiffString(uniqueFilesDiff);
|
||||
const after = parseDiffString(changed);
|
||||
|
||||
expect(before[0].cacheKey).toBe(after[0].cacheKey);
|
||||
expect(before[1].cacheKey).not.toBe(after[1].cacheKey);
|
||||
});
|
||||
|
||||
it("gives the same path different keys for different bodies", () => {
|
||||
const warn = vi.spyOn(console, "warn").mockImplementation(() => {});
|
||||
try {
|
||||
// A later diff body for the same path must not reuse this AST.
|
||||
const first = parseDiffString(duplicateFileDiff);
|
||||
// parseDiffString collapses the duplicate path, so the
|
||||
// CodeView duplicate-id crash class stays closed.
|
||||
expect(first).toHaveLength(1);
|
||||
// Dedupe keeps the first section; parse the second body alone to
|
||||
// compare keys for one path with two bodies.
|
||||
const secondBody = duplicateFileDiff
|
||||
.split("diff --git")
|
||||
.slice(2)
|
||||
.map((s) => `diff --git${s}`)
|
||||
.join("");
|
||||
const second = parseDiffString(secondBody);
|
||||
|
||||
expect(first[0].name).toBe(second[0].name);
|
||||
expect(first[0].cacheKey).not.toBe(second[0].cacheKey);
|
||||
expect(warn).toHaveBeenCalledTimes(1);
|
||||
} finally {
|
||||
warn.mockRestore();
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe("stampCacheKey", () => {
|
||||
it("folds lang into the key", () => {
|
||||
const body = [
|
||||
"--- a.ts",
|
||||
"+++ a.ts",
|
||||
"@@ -1,1 +1,1 @@",
|
||||
"-const x = 1;",
|
||||
"+const x = 2;",
|
||||
].join("\n");
|
||||
const plain = parseDiffString(body);
|
||||
const langSet = parseDiffString(body);
|
||||
langSet[0].lang = "json";
|
||||
stampCacheKey(langSet[0]);
|
||||
|
||||
expect(plain[0].cacheKey).not.toBe(langSet[0].cacheKey);
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,63 @@
|
||||
import type { FileDiffMetadata } from "@pierre/diffs";
|
||||
import { parsePatchFiles } from "@pierre/diffs";
|
||||
import { getContentCacheKey } from "./diffCacheKey";
|
||||
|
||||
// CodeView throws on duplicate item ids, so repeated post-image paths
|
||||
// collapse to their first occurrence. Exported for tests.
|
||||
export function dedupeFilesByName(
|
||||
files: readonly FileDiffMetadata[],
|
||||
): FileDiffMetadata[] {
|
||||
const seen = new Set<string>();
|
||||
const unique: FileDiffMetadata[] = [];
|
||||
const duplicates: string[] = [];
|
||||
for (const file of files) {
|
||||
if (seen.has(file.name)) {
|
||||
duplicates.push(file.name);
|
||||
continue;
|
||||
}
|
||||
seen.add(file.name);
|
||||
unique.push(file);
|
||||
}
|
||||
if (duplicates.length > 0) {
|
||||
console.warn(
|
||||
`Diff lists duplicate file paths; showing the first occurrence of each: ${duplicates.join(", ")}`,
|
||||
);
|
||||
}
|
||||
return unique;
|
||||
}
|
||||
|
||||
/**
|
||||
* Parses a unified or git diff string into per-file metadata; empty input
|
||||
* yields []. Each file is keyed by a hash of its own render inputs so
|
||||
* unchanged files keep their highlight cache hits.
|
||||
*/
|
||||
export function parseDiffString(
|
||||
diffString: string | undefined | null,
|
||||
): FileDiffMetadata[] {
|
||||
if (!diffString) return [];
|
||||
const files = parsePatchFiles(diffString).flatMap((p) => p.files);
|
||||
for (const file of files) {
|
||||
stampCacheKey(file);
|
||||
}
|
||||
return dedupeFilesByName(files);
|
||||
}
|
||||
|
||||
/**
|
||||
* Stamps `file.cacheKey` with a hash of the inputs the worker reads when
|
||||
* highlighting; callers that mutate a parsed file must restamp it.
|
||||
*/
|
||||
export function stampCacheKey(file: FileDiffMetadata): void {
|
||||
file.cacheKey = getContentCacheKey(serializeRenderInputs(file));
|
||||
}
|
||||
|
||||
// Everything the renderer walks when building the highlighted AST: equal
|
||||
// serializations are equal render inputs.
|
||||
const serializeRenderInputs = (file: FileDiffMetadata): string =>
|
||||
JSON.stringify([
|
||||
file.name,
|
||||
file.prevName,
|
||||
file.lang,
|
||||
file.hunks,
|
||||
file.additionLines,
|
||||
file.deletionLines,
|
||||
]);
|
||||
@@ -1,88 +0,0 @@
|
||||
import { parsePatchFiles } from "@pierre/diffs";
|
||||
import { afterEach, describe, expect, it, vi } from "vitest";
|
||||
import { dedupeFilesByName } from "./useParsedDiff";
|
||||
|
||||
// Two `diff --git` sections for the same post-image path. `parsePatchFiles`
|
||||
// emits one FileDiffMetadata per section, so this is the exact shape that made
|
||||
// CodeView.addItem throw `duplicate id "agent/x/agentmcp/api_internal_test.go"`
|
||||
// in production.
|
||||
const duplicateFileDiff = [
|
||||
"diff --git a/agent/x/agentmcp/api_internal_test.go b/agent/x/agentmcp/api_internal_test.go",
|
||||
"index 1111111..2222222 100644",
|
||||
"--- a/agent/x/agentmcp/api_internal_test.go",
|
||||
"+++ b/agent/x/agentmcp/api_internal_test.go",
|
||||
"@@ -1,3 +1,3 @@",
|
||||
" package agentmcp",
|
||||
"-const a = 1",
|
||||
"+const a = 2",
|
||||
" const b = 3",
|
||||
"diff --git a/agent/x/agentmcp/api_internal_test.go b/agent/x/agentmcp/api_internal_test.go",
|
||||
"index 3333333..4444444 100644",
|
||||
"--- a/agent/x/agentmcp/api_internal_test.go",
|
||||
"+++ b/agent/x/agentmcp/api_internal_test.go",
|
||||
"@@ -10,3 +10,3 @@",
|
||||
" const c = 4",
|
||||
"-const d = 5",
|
||||
"+const d = 6",
|
||||
" const e = 7",
|
||||
].join("\n");
|
||||
|
||||
const uniqueFilesDiff = [
|
||||
"diff --git a/first.ts b/first.ts",
|
||||
"index 1111111..2222222 100644",
|
||||
"--- a/first.ts",
|
||||
"+++ b/first.ts",
|
||||
"@@ -1,1 +1,1 @@",
|
||||
"-const a = 1",
|
||||
"+const a = 2",
|
||||
"diff --git a/second.ts b/second.ts",
|
||||
"index 3333333..4444444 100644",
|
||||
"--- a/second.ts",
|
||||
"+++ b/second.ts",
|
||||
"@@ -1,1 +1,1 @@",
|
||||
"-const b = 1",
|
||||
"+const b = 2",
|
||||
].join("\n");
|
||||
|
||||
function parse(diffStr: string) {
|
||||
return parsePatchFiles(diffStr).flatMap((p) => p.files);
|
||||
}
|
||||
|
||||
describe("dedupeFilesByName", () => {
|
||||
afterEach(() => {
|
||||
vi.restoreAllMocks();
|
||||
});
|
||||
|
||||
it("collapses repeated post-image paths to the first occurrence", () => {
|
||||
const warn = vi.spyOn(console, "warn").mockImplementation(() => {});
|
||||
const files = parse(duplicateFileDiff);
|
||||
// Sanity check: the parser really does hand us the duplicate that
|
||||
// crashes CodeView, so the dedupe below is exercising a real case.
|
||||
expect(files).toHaveLength(2);
|
||||
|
||||
const deduped = dedupeFilesByName(files);
|
||||
|
||||
expect(deduped).toHaveLength(1);
|
||||
expect(deduped[0]).toBe(files[0]);
|
||||
// Mapping to CodeView item ids (id: file.name) now yields no collision.
|
||||
expect(deduped.map((f) => f.name)).toEqual([
|
||||
"agent/x/agentmcp/api_internal_test.go",
|
||||
]);
|
||||
expect(warn).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it("preserves order and every file when paths are unique", () => {
|
||||
const warn = vi.spyOn(console, "warn").mockImplementation(() => {});
|
||||
const files = parse(uniqueFilesDiff);
|
||||
|
||||
const deduped = dedupeFilesByName(files);
|
||||
|
||||
expect(deduped).toEqual(files);
|
||||
expect(deduped.map((f) => f.name)).toEqual(["first.ts", "second.ts"]);
|
||||
expect(warn).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("returns an empty array unchanged", () => {
|
||||
expect(dedupeFilesByName([])).toEqual([]);
|
||||
});
|
||||
});
|
||||
@@ -1,52 +0,0 @@
|
||||
import type { FileDiffMetadata } from "@pierre/diffs";
|
||||
import { parsePatchFiles } from "@pierre/diffs";
|
||||
import { useMemo } from "react";
|
||||
|
||||
// A single diff body can list the same post-image path more than once: the
|
||||
// server may concatenate several `git diff` outputs, or one patch may carry
|
||||
// multiple `diff --git` sections for the same file. Both the CodeView (which
|
||||
// keys items by file name) and the file tree (which keys rows by path) require
|
||||
// unique ids, and CodeView.addItem throws on a duplicate id, which tears down
|
||||
// the entire diff view. Collapse repeats to their first occurrence so a
|
||||
// malformed diff degrades gracefully instead of crashing. Exported for tests.
|
||||
export function dedupeFilesByName(
|
||||
files: readonly FileDiffMetadata[],
|
||||
): FileDiffMetadata[] {
|
||||
const seen = new Set<string>();
|
||||
const unique: FileDiffMetadata[] = [];
|
||||
const duplicates: string[] = [];
|
||||
for (const file of files) {
|
||||
if (seen.has(file.name)) {
|
||||
duplicates.push(file.name);
|
||||
continue;
|
||||
}
|
||||
seen.add(file.name);
|
||||
unique.push(file);
|
||||
}
|
||||
if (duplicates.length > 0) {
|
||||
console.warn(
|
||||
`Diff lists duplicate file paths; showing the first occurrence of each: ${duplicates.join(", ")}`,
|
||||
);
|
||||
}
|
||||
return unique;
|
||||
}
|
||||
|
||||
// Uses explicit useMemo despite the React Compiler scope because
|
||||
// parsePatchFiles is external to the compiler's static analysis.
|
||||
export function useParsedDiff(
|
||||
diffString: string | undefined | null,
|
||||
cacheKeyPrefix?: string,
|
||||
): FileDiffMetadata[] {
|
||||
return useMemo(() => {
|
||||
if (!diffString) return [];
|
||||
try {
|
||||
const files = parsePatchFiles(diffString, cacheKeyPrefix).flatMap(
|
||||
(p) => p.files,
|
||||
);
|
||||
return dedupeFilesByName(files);
|
||||
} catch (e) {
|
||||
console.error("Failed to parse diff:", e);
|
||||
return [];
|
||||
}
|
||||
}, [diffString, cacheKeyPrefix]);
|
||||
}
|
||||
Reference in New Issue
Block a user