diff --git a/site/src/pages/AgentsPage/components/DiffViewer/useParsedDiff.test.ts b/site/src/pages/AgentsPage/components/DiffViewer/useParsedDiff.test.ts new file mode 100644 index 0000000000..1dacb1bb0c --- /dev/null +++ b/site/src/pages/AgentsPage/components/DiffViewer/useParsedDiff.test.ts @@ -0,0 +1,88 @@ +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 index 437f42416d..77465e21d5 100644 --- a/site/src/pages/AgentsPage/components/DiffViewer/useParsedDiff.ts +++ b/site/src/pages/AgentsPage/components/DiffViewer/useParsedDiff.ts @@ -2,6 +2,35 @@ 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( @@ -11,9 +40,10 @@ export function useParsedDiff( return useMemo(() => { if (!diffString) return []; try { - return parsePatchFiles(diffString, cacheKeyPrefix).flatMap( + const files = parsePatchFiles(diffString, cacheKeyPrefix).flatMap( (p) => p.files, ); + return dedupeFilesByName(files); } catch (e) { console.error("Failed to parse diff:", e); return [];