From df278ec0795cda3af53bae17aae5108ddcfe2d69 Mon Sep 17 00:00:00 2001 From: Danielle Maywood Date: Tue, 11 Aug 2026 12:22:13 +0100 Subject: [PATCH] fix: derive diff cache keys from patch content (#27987) --- biome.jsonc | 24 +++ .../ChatElements/tools/utils.test.ts | 73 ++++++- .../components/ChatElements/tools/utils.ts | 71 ++++-- .../DiffViewer/DiffViewer.stories.tsx | 110 ++++++++-- .../components/DiffViewer/LocalDiffPanel.tsx | 4 +- .../components/DiffViewer/RemoteDiffPanel.tsx | 14 +- .../DiffViewer/diffCacheKey.test.ts | 22 +- .../components/DiffViewer/diffCacheKey.ts | 25 ++- .../DiffViewer/extractDiffContent.test.ts | 6 +- .../components/DiffViewer/parseDiff.test.ts | 202 ++++++++++++++++++ .../components/DiffViewer/parseDiff.ts | 63 ++++++ .../DiffViewer/useParsedDiff.test.ts | 88 -------- .../components/DiffViewer/useParsedDiff.ts | 52 ----- 13 files changed, 536 insertions(+), 218 deletions(-) create mode 100644 site/src/pages/AgentsPage/components/DiffViewer/parseDiff.test.ts create mode 100644 site/src/pages/AgentsPage/components/DiffViewer/parseDiff.ts delete mode 100644 site/src/pages/AgentsPage/components/DiffViewer/useParsedDiff.test.ts delete mode 100644 site/src/pages/AgentsPage/components/DiffViewer/useParsedDiff.ts diff --git a/biome.jsonc b/biome.jsonc index e7e351963d..b6d5604b87 100644 --- a/biome.jsonc +++ b/biome.jsonc @@ -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" diff --git a/site/src/pages/AgentsPage/components/ChatElements/tools/utils.test.ts b/site/src/pages/AgentsPage/components/ChatElements/tools/utils.test.ts index 441385982c..3a5ed1a472 100644 --- a/site/src/pages/AgentsPage/components/ChatElements/tools/utils.test.ts +++ b/site/src/pages/AgentsPage/components/ChatElements/tools/utils.test.ts @@ -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", () => { diff --git a/site/src/pages/AgentsPage/components/ChatElements/tools/utils.ts b/site/src/pages/AgentsPage/components/ChatElements/tools/utils.ts index f2500c0967..ea84f619b2 100644 --- a/site/src/pages/AgentsPage/components/ChatElements/tools/utils.ts +++ b/site/src/pages/AgentsPage/components/ChatElements/tools/utils.ts @@ -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 = []; + 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 diff --git a/site/src/pages/AgentsPage/components/DiffViewer/DiffViewer.stories.tsx b/site/src/pages/AgentsPage/components/DiffViewer/DiffViewer.stories.tsx index 08cc63ab53..f4601d3320 100644 --- a/site/src/pages/AgentsPage/components/DiffViewer/DiffViewer.stories.tsx +++ b/site/src/pages/AgentsPage/components/DiffViewer/DiffViewer.stories.tsx @@ -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 = { @@ -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
;", " }", ].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 ( +
+ + {file && ( + + )} +
+ ); +}; + +export const ReparseSamePathAfterEdit: StoryObj = { + render: () => , + 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"); + }, +}; diff --git a/site/src/pages/AgentsPage/components/DiffViewer/LocalDiffPanel.tsx b/site/src/pages/AgentsPage/components/DiffViewer/LocalDiffPanel.tsx index 1b3bed2dfd..b61676dd95 100644 --- a/site/src/pages/AgentsPage/components/DiffViewer/LocalDiffPanel.tsx +++ b/site/src/pages/AgentsPage/components/DiffViewer/LocalDiffPanel.tsx @@ -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 = ({ diffStyle, chatInputRef, }) => { - const parsedFiles = useParsedDiff(repo.unified_diff); + const parsedFiles = parseDiffString(repo.unified_diff); return ( = ({ }); 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 diff --git a/site/src/pages/AgentsPage/components/DiffViewer/diffCacheKey.test.ts b/site/src/pages/AgentsPage/components/DiffViewer/diffCacheKey.test.ts index 5797860289..8f40fca762 100644 --- a/site/src/pages/AgentsPage/components/DiffViewer/diffCacheKey.test.ts +++ b/site/src/pages/AgentsPage/components/DiffViewer/diffCacheKey.test.ts @@ -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--", () => { + expect(getContentCacheKey("anything")).toMatch( + /^content-[0-9a-f]+-[0-9a-f]+$/, ); }); }); diff --git a/site/src/pages/AgentsPage/components/DiffViewer/diffCacheKey.ts b/site/src/pages/AgentsPage/components/DiffViewer/diffCacheKey.ts index cb015da2d9..022d8ef4e9 100644 --- a/site/src/pages/AgentsPage/components/DiffViewer/diffCacheKey.ts +++ b/site/src/pages/AgentsPage/components/DiffViewer/diffCacheKey.ts @@ -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)}`; +}; diff --git a/site/src/pages/AgentsPage/components/DiffViewer/extractDiffContent.test.ts b/site/src/pages/AgentsPage/components/DiffViewer/extractDiffContent.test.ts index 074793e12e..9ad5c47da7 100644 --- a/site/src/pages/AgentsPage/components/DiffViewer/extractDiffContent.test.ts +++ b/site/src/pages/AgentsPage/components/DiffViewer/extractDiffContent.test.ts @@ -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[] { diff --git a/site/src/pages/AgentsPage/components/DiffViewer/parseDiff.test.ts b/site/src/pages/AgentsPage/components/DiffViewer/parseDiff.test.ts new file mode 100644 index 0000000000..79e75cb4a3 --- /dev/null +++ b/site/src/pages/AgentsPage/components/DiffViewer/parseDiff.test.ts @@ -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); + }); +}); diff --git a/site/src/pages/AgentsPage/components/DiffViewer/parseDiff.ts b/site/src/pages/AgentsPage/components/DiffViewer/parseDiff.ts new file mode 100644 index 0000000000..b7be3467d0 --- /dev/null +++ b/site/src/pages/AgentsPage/components/DiffViewer/parseDiff.ts @@ -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(); + 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, + ]); diff --git a/site/src/pages/AgentsPage/components/DiffViewer/useParsedDiff.test.ts b/site/src/pages/AgentsPage/components/DiffViewer/useParsedDiff.test.ts deleted file mode 100644 index 1dacb1bb0c..0000000000 --- a/site/src/pages/AgentsPage/components/DiffViewer/useParsedDiff.test.ts +++ /dev/null @@ -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([]); - }); -}); diff --git a/site/src/pages/AgentsPage/components/DiffViewer/useParsedDiff.ts b/site/src/pages/AgentsPage/components/DiffViewer/useParsedDiff.ts deleted file mode 100644 index 77465e21d5..0000000000 --- a/site/src/pages/AgentsPage/components/DiffViewer/useParsedDiff.ts +++ /dev/null @@ -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(); - 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]); -}