authorgravatar for 56691628+Mnigos@users.noreply.github.comIgor Makowski <56691628+Mnigos@users.noreply.github.com> 2026-09-18 13:42:17+02:00
committergravatar for noreply@github.comGitHub <noreply@github.com> 2026-09-18 17:12:17+05:30
log9ea9c3d5d2c444133e3ddff40eecf38737951589
tree15f9a4fd273799474545df63e245d425cf18264a
parent52e4b4429359904441039a286c61eaafe8474451
signature Signed by PGP key B5690EEEBB952194

fix(web): keep a file-to-symlink type change from crashing the diff view (#11075)

Co-authored-by: shivam <91240327+shivamhwp@users.noreply.github.com>

5 files changed, 87 insertions(+), 8 deletions(-)

apps/web/src/components/diffs/DiffFileTree.test.tsx+19
...@@ -5,7 +5,10 @@ import { act, type MouseEvent, type ReactNode } from "react";...@@ -5,7 +5,10 @@ import { act, type MouseEvent, type ReactNode } from "react";
5import { create, type ReactTestRenderer } from "react-test-renderer";5import { create, type ReactTestRenderer } from "react-test-renderer";
6import { afterEach, beforeEach, describe, expect, it, vi } from "vite-plus/test";6import { afterEach, beforeEach, describe, expect, it, vi } from "vite-plus/test";
77
8import type { FileDiffMetadata } from "@pierre/diffs";
9
8import { DiffFileTree, type DiffFileTreeEntry } from "./DiffFileTree";10import { DiffFileTree, type DiffFileTreeEntry } from "./DiffFileTree";
11import { diffFileTreeEntries } from "./diffFileTree.logic";
9import { useCodeViewFileReveal } from "./useCodeViewFileReveal";12import { useCodeViewFileReveal } from "./useCodeViewFileReveal";
1013
11vi.mock("../../hooks/useTheme", () => ({ useTheme: () => ({ resolvedTheme: "dark" }) }));14vi.mock("../../hooks/useTheme", () => ({ useTheme: () => ({ resolvedTheme: "dark" }) }));
...@@ -112,6 +115,22 @@ describe("diff tree file activation", () => {...@@ -112,6 +115,22 @@ describe("diff tree file activation", () => {
112 vi.unstubAllGlobals();115 vi.unstubAllGlobals();
113 });116 });
114117
118 it("mounts a file-to-symlink type change as one tree row", async () => {
119 // Git carries a type change as a deletion and an addition of the same path.
120 const typeChange = diffFileTreeEntries([
121 { type: "deleted", name: "AGENTS.md", prevName: "AGENTS.md" } as FileDiffMetadata,
122 { type: "new", name: "AGENTS.md", prevName: "AGENTS.md" } as FileDiffMetadata,
123 { type: "change", name: "CLAUDE.md", prevName: "CLAUDE.md" } as FileDiffMetadata,
124 ]);
125 await mount({ files: [...typeChange] });
126
127 const tree = model();
128 expect(tree.getItem("AGENTS.md")?.isDirectory()).toBe(false);
129 expect(tree.getItem("CLAUDE.md")?.isDirectory()).toBe(false);
130 await activate("AGENTS.md");
131 expect(targets).toEqual([{ type: "item", id: "AGENTS.md\u0000AGENTS.md", align: "start" }]);
132 });
133
115 it("reissues the reveal when the sole selected file is activated again", async () => {134 it("reissues the reveal when the sole selected file is activated again", async () => {
116 await mount();135 await mount();
117 await activate("02-short.ts");136 await activate("02-short.ts");
apps/web/src/components/diffs/diffFileTree.logic.test.ts+17
...@@ -34,6 +34,23 @@ describe("diffFileTreeEntries", () => {...@@ -34,6 +34,23 @@ describe("diffFileTreeEntries", () => {
34 });34 });
35});35});
3636
37describe("diffFileTreeEntries", () => {
38 it("folds a file-to-symlink type change into one modified entry", () => {
39 expect(
40 diffFileTreeEntries([
41 file("change", "CLAUDE.md"),
42 file("deleted", "AGENTS.md"),
43 file("new", "AGENTS.md"),
44 file("new", "docs/new.md"),
45 ]),
46 ).toEqual([
47 { path: "CLAUDE.md", status: "modified" },
48 { path: "AGENTS.md", status: "modified" },
49 { path: "docs/new.md", status: "added" },
50 ]);
51 });
52});
53
37describe("collectDirectoryPaths", () => {54describe("collectDirectoryPaths", () => {
38 it("lists every ancestor once, parents first, with Pierre's trailing slash", () => {55 it("lists every ancestor once, parents first, with Pierre's trailing slash", () => {
39 expect(collectDirectoryPaths(["apps/web/src/a.ts", "apps/web/b.ts", "README.md"])).toEqual([56 expect(collectDirectoryPaths(["apps/web/src/a.ts", "apps/web/b.ts", "README.md"])).toEqual([
apps/web/src/components/diffs/diffFileTree.logic.ts+13-2
...@@ -23,11 +23,22 @@ function toGitStatus(file: FileDiffMetadata): GitStatus {...@@ -23,11 +23,22 @@ function toGitStatus(file: FileDiffMetadata): GitStatus {
23 }23 }
24}24}
2525
26/** Maps parsed diff files to tree entries, keeping the diff's own order. */26/**
27 * Maps parsed diff files to tree entries, keeping the diff's own order. A path
28 * appears once: a type change (regular file to symlink) is a deletion plus an
29 * addition of the same path, and the tree shows the surviving file as modified.
30 */
27export function diffFileTreeEntries(31export function diffFileTreeEntries(
28 files: ReadonlyArray<FileDiffMetadata>,32 files: ReadonlyArray<FileDiffMetadata>,
29): ReadonlyArray<DiffFileTreeEntry> {33): ReadonlyArray<DiffFileTreeEntry> {
30 return files.map((file) => ({ path: resolveFileDiffPath(file), status: toGitStatus(file) }));34 const statusByPath = new Map<string, GitStatus>();
35 for (const file of files) {
36 const path = resolveFileDiffPath(file);
37 const status = toGitStatus(file);
38 const previous = statusByPath.get(path);
39 statusByPath.set(path, previous === undefined || previous === status ? status : "modified");
40 }
41 return [...statusByPath].map(([path, status]) => ({ path, status }));
31}42}
3243
33/**44/**
apps/web/src/lib/diffRendering.test.ts+32-5
...@@ -35,10 +35,10 @@ describe("buildPatchCacheKey", () => {...@@ -35,10 +35,10 @@ describe("buildPatchCacheKey", () => {
3535
36describe("getRenderablePatch", () => {36describe("getRenderablePatch", () => {
37 it.each([37 it.each([
38 ["a/example.ts", "a/example.ts"],38 ["a/example.ts", "a/example.ts", "change"],
39 ["b/example.ts", "b/example.ts"],39 ["b/example.ts", "b/example.ts", "change"],
40 ["a/before.ts", "b/after.ts"],40 ["a/before.ts", "b/after.ts", "rename-changed"],
41 ])("preserves repository paths from %s to %s", (previousPath, path) => {41 ])("preserves repository paths from %s to %s", (previousPath, path, type) => {
42 const parsed = getRenderablePatch(42 const parsed = getRenderablePatch(
43 [43 [
44 `diff --git a/${previousPath} b/${path}`,44 `diff --git a/${previousPath} b/${path}`,
...@@ -59,7 +59,7 @@ describe("getRenderablePatch", () => {...@@ -59,7 +59,7 @@ describe("getRenderablePatch", () => {
59 if (!file) return;59 if (!file) return;
60 expect(resolveFileDiffPath(file)).toBe(path);60 expect(resolveFileDiffPath(file)).toBe(path);
61 expect(resolveFileDiffPreviousPath(file)).toBe(previousPath);61 expect(resolveFileDiffPreviousPath(file)).toBe(previousPath);
62 expect(buildFileDiffIdentityKey(file)).toBe(`${previousPath}\0${path}`);62 expect(buildFileDiffIdentityKey(file)).toBe(`${previousPath}\0${path}\0${type}`);
63 });63 });
6464
65 it("compacts partial hunk render offsets for virtualized review diffs", () => {65 it("compacts partial hunk render offsets for virtualized review diffs", () => {
...@@ -137,6 +137,33 @@ describe("diff file reconciliation", () => {...@@ -137,6 +137,33 @@ describe("diff file reconciliation", () => {
137 expect(buildFileDiffRenderKey(file)).toBe(key);137 expect(buildFileDiffRenderKey(file)).toBe(key);
138 });138 });
139139
140 it("gives a type change its own identity per block", () => {
141 const patch = [
142 "diff --git a/AGENTS.md b/AGENTS.md",
143 "deleted file mode 100644",
144 "--- a/AGENTS.md",
145 "+++ /dev/null",
146 "@@ -1 +0,0 @@",
147 "-duplicated instructions",
148 "diff --git a/AGENTS.md b/AGENTS.md",
149 "new file mode 120000",
150 "--- /dev/null",
151 "+++ b/AGENTS.md",
152 "@@ -0,0 +1 @@",
153 "+CLAUDE.md",
154 ].join("\n");
155 const parsed = getRenderablePatch(patch, "type-change");
156 expect(parsed?.kind).toBe("files");
157 if (parsed?.kind !== "files") return;
158 const [deleted, added] = parsed.files;
159 expect(deleted?.type).toBe("deleted");
160 expect(added?.type).toBe("new");
161 if (!deleted || !added) return;
162
163 expect(buildFileDiffIdentityKey(deleted)).not.toBe(buildFileDiffIdentityKey(added));
164 expect(new Set(parsed.files.map(buildFileDiffIdentityKey)).size).toBe(parsed.files.length);
165 });
166
140 it("keeps identities stable and versions local to the changed file", () => {167 it("keeps identities stable and versions local to the changed file", () => {
141 const patch = (secondLine: string) =>168 const patch = (secondLine: string) =>
142 [169 [
apps/web/src/lib/diffRendering.ts+6-1
...@@ -166,8 +166,13 @@ export function resolveFileDiffPreviousPath(fileDiff: FileDiffMetadata): string...@@ -166,8 +166,13 @@ export function resolveFileDiffPreviousPath(fileDiff: FileDiffMetadata): string
166 return fileDiffPath(fileDiff.prevName ?? fileDiff.name ?? "");166 return fileDiffPath(fileDiff.prevName ?? fileDiff.name ?? "");
167}167}
168168
169/**
170 * Stable across re-renders of the same file, distinct for every block in a
171 * patch. A type change (regular file to symlink) arrives as a deletion and an
172 * addition of the same path, so the change type is part of the identity.
173 */
169export function buildFileDiffIdentityKey(fileDiff: FileDiffMetadata): string {174export function buildFileDiffIdentityKey(fileDiff: FileDiffMetadata): string {
170 return `${resolveFileDiffPreviousPath(fileDiff)}\u0000${resolveFileDiffPath(fileDiff)}`;175 return `${resolveFileDiffPreviousPath(fileDiff)}\u0000${resolveFileDiffPath(fileDiff)}\u0000${fileDiff.type}`;
171}176}
172177
173export function buildFileDiffRenderKey(fileDiff: FileDiffMetadata): string {178export function buildFileDiffRenderKey(fileDiff: FileDiffMetadata): string {