mirror of
https://github.com/coder/coder.git
synced 2026-09-22 05:05:20 +08:00
fix(site/src/pages/AgentsPage/components/DiffViewer): dedupe diff files to prevent CodeView duplicate id crash (#26597)
Viewing a git diff on `/agents` could crash the whole diff view with `CodeView.addItem: duplicate id "agent/x/agentmcp/api_internal_test.go"`. When a diff body lists the same post-image path in more than one `diff --git` section, `parsePatchFiles` returns one `FileDiffMetadata` per section. `DiffViewer` then maps each file to a `CodeView` item keyed by `file.name` (the file tree is likewise keyed by path), so the repeated path produced two items with the same id and `CodeView.addItem` threw, tearing down the entire view. Deduplicate the parsed files by path in `useParsedDiff`, keeping the first occurrence, so both the `CodeView` and the file tree always receive unique ids. `useParsedDiff` is the single source feeding both panels, so a malformed diff now degrades gracefully and logs one warning instead of crashing. <details> <summary>Root cause and verification</summary> Confirmed against `@pierre/diffs` `parsePatchFiles` that a single patch with two `diff --git` sections for one path yields `patchCount: 1`, `fileCount: 2` with both entries named `agent/x/agentmcp/api_internal_test.go`. That is the exact input that made `CodeView.addItem` throw. Dropping (rather than merging) duplicates matches the renderer's model, which can only show one item per id anyway. Verified on the branch: `pnpm test` (DiffViewer suite, including the new `dedupeFilesByName` tests), `tsc -p .`, and the full `pnpm lint` (biome, types, circular-deps, React Compiler, knip) all pass. </details> > Opened by Coder Agents on behalf of @kylecarbs.
This commit is contained in:
@@ -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([]);
|
||||
});
|
||||
});
|
||||
@@ -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<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(
|
||||
@@ -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 [];
|
||||
|
||||
Reference in New Issue
Block a user