Skip to content

Commit b4acc69

Browse files
committed
fix(web): handle file/directory path collision when opening PR diff
Git can delete a file and add a directory of the same name in one diff. Pierre's tree throws if both paths are inserted. Give that file a distinct tree path and translate selection back to the diff path. Fixes #12887. Co-authored-by: maco <macodev00@users.noreply.github.com>
1 parent 2577baf commit b4acc69

4 files changed

Lines changed: 343 additions & 32 deletions

File tree

‎apps/web/src/components/diffs/DiffFileTree.test.tsx‎

Lines changed: 78 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vite-plus/test"
88
import type { FileDiffMetadata } from "@pierre/diffs";
99

1010
import { DiffFileTree, type DiffFileTreeEntry } from "./DiffFileTree";
11-
import { diffFileTreeEntries } from "./diffFileTree.logic";
11+
import { diffFileTreeEntries, diffFileTreeModel } from "./diffFileTree.logic";
1212
import { useCodeViewFileReveal } from "./useCodeViewFileReveal";
1313

1414
vi.mock("../../hooks/useTheme", () => ({ useTheme: () => ({ resolvedTheme: "dark" }) }));
@@ -209,6 +209,83 @@ describe("diff tree file activation", () => {
209209
expect(folder.isExpanded()).toBe(false);
210210
});
211211

212+
it("keeps a file and its replacement directory selectable", async () => {
213+
const files: DiffFileTreeEntry[] = [
214+
{ path: "office", status: "deleted" },
215+
{ path: "office/config.ts", status: "added" },
216+
];
217+
const presented = diffFileTreeModel(files.map((file) => file.path));
218+
await mount({ files, selectedPath: "office" });
219+
expect(model().getSelectedPaths()).toEqual([presented.modelPath("office")]);
220+
await activate(presented.modelPath("office"));
221+
await activate(presented.modelPath("office/config.ts"));
222+
expect(targets.map((target) => ("id" in target ? target.id : null))).toEqual([
223+
"office\u0000office",
224+
"office/config.ts\u0000office/config.ts",
225+
]);
226+
targets.length = 0;
227+
await activate("office/");
228+
expect(targets).toEqual([]);
229+
});
230+
231+
it("keeps the reverse directory-to-file pair selectable", async () => {
232+
const files: DiffFileTreeEntry[] = [
233+
{ path: "office/config.ts", status: "deleted" },
234+
{ path: "office", status: "added" },
235+
];
236+
const presented = diffFileTreeModel(files.map((file) => file.path));
237+
await mount({ files });
238+
await activate(presented.modelPath("office/config.ts"));
239+
await activate(presented.modelPath("office"));
240+
expect(targets.map((target) => ("id" in target ? target.id : null))).toEqual([
241+
"office/config.ts\u0000office/config.ts",
242+
"office\u0000office",
243+
]);
244+
});
245+
246+
it("survives a later slice that turns an existing file into a directory prefix", async () => {
247+
await mount({ files: [{ path: "office", status: "deleted" }] });
248+
const files: DiffFileTreeEntry[] = [
249+
{ path: "office", status: "deleted" },
250+
{ path: "office/config.ts", status: "added" },
251+
];
252+
await act(async () => {
253+
renderer!.update(<Panel files={files} />);
254+
});
255+
const presented = diffFileTreeModel(files.map((file) => file.path));
256+
await activate(presented.modelPath("office"));
257+
await activate(presented.modelPath("office/config.ts"));
258+
expect(targets.map((target) => ("id" in target ? target.id : null))).toEqual([
259+
"office\u0000office",
260+
"office/config.ts\u0000office/config.ts",
261+
]);
262+
});
263+
264+
it("keeps both colliding files selectable after a refresh of the same set", async () => {
265+
const files: DiffFileTreeEntry[] = [
266+
{ path: "office", status: "deleted" },
267+
{ path: "office/config.ts", status: "added" },
268+
{ path: "src/features/route.ts", status: "modified" },
269+
];
270+
const presented = diffFileTreeModel(files.map((file) => file.path));
271+
await mount({ files });
272+
const folder = model().getItem("src/features/")!;
273+
if (!("collapse" in folder)) throw new Error("Expected the directory handle");
274+
await act(async () => folder.collapse());
275+
await act(async () => {
276+
renderer!.update(<Panel files={files.map((file) => ({ ...file }))} />);
277+
});
278+
const refreshed = model().getItem("src/features/")!;
279+
if (!("isExpanded" in refreshed)) throw new Error("Expected the directory handle");
280+
expect(refreshed.isExpanded()).toBe(false);
281+
await activate(presented.modelPath("office"));
282+
await activate(presented.modelPath("office/config.ts"));
283+
expect(targets.map((target) => ("id" in target ? target.id : null))).toEqual([
284+
"office\u0000office",
285+
"office/config.ts\u0000office/config.ts",
286+
]);
287+
});
288+
212289
it("does not echo controlled selection, but lets the reader activate it", async () => {
213290
await mount({ selectedPath: "02-short.ts" });
214291
expect(model().getSelectedPaths()).toEqual(["02-short.ts"]);

‎apps/web/src/components/diffs/DiffFileTree.tsx‎

Lines changed: 58 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import {
1616
buildDiffFileTreeUpdates,
1717
compareDiffFileTreeEntries,
1818
collectDirectoryPaths,
19+
diffFileTreeModel,
1920
diffFileTreePositions,
2021
type DiffFileTreeEntry,
2122
} from "./diffFileTree.logic";
@@ -45,8 +46,10 @@ export function DiffFileTree({
4546
}: DiffFileTreeProps) {
4647
const { resolvedTheme } = useTheme();
4748
const paths = useMemo(() => entries.map((entry) => entry.path), [entries]);
48-
const directoryPaths = useMemo(() => collectDirectoryPaths(paths), [paths]);
49-
const positions = useMemo(() => diffFileTreePositions(paths), [paths]);
49+
const presented = useMemo(() => diffFileTreeModel(paths), [paths]);
50+
const modelPaths = presented.paths;
51+
const directoryPaths = useMemo(() => collectDirectoryPaths(modelPaths), [modelPaths]);
52+
const positions = useMemo(() => diffFileTreePositions(modelPaths), [modelPaths]);
5053
const [ordering] = useState(() => {
5154
let currentPositions: ReadonlyMap<string, number> = new Map();
5255
return {
@@ -57,19 +60,29 @@ export function DiffFileTree({
5760
};
5861
});
5962
const gitStatus = useMemo<ReadonlyArray<GitStatusEntry>>(
60-
() => entries.map((entry) => ({ path: entry.path, status: entry.status })),
61-
[entries],
63+
() =>
64+
entries.map((entry, index) => ({
65+
path: modelPaths[index] ?? entry.path,
66+
status: entry.status,
67+
})),
68+
[entries, modelPaths],
6269
);
63-
const filePathsRef = useRef<ReadonlySet<string>>(new Set(paths));
70+
const filePathsRef = useRef<ReadonlySet<string>>(new Set(modelPaths));
6471
const onSelectFileRef = useRef(onSelectFile);
72+
const toSelectionPathRef = useRef(presented.selectionPath);
6573
const syncingSelectionRef = useRef(false);
66-
const handledRevealRef = useRef<{ path: string; revealRequestId: number } | null>(null);
74+
const handledRevealRef = useRef<{
75+
path: string;
76+
revealRequestId: number;
77+
modelPath: string;
78+
} | null>(null);
6779
const mountedPathsRef = useRef<ReadonlyArray<string> | null>(null);
6880

6981
useEffect(() => {
70-
filePathsRef.current = new Set(paths);
82+
filePathsRef.current = new Set(modelPaths);
7183
onSelectFileRef.current = onSelectFile;
72-
}, [onSelectFile, paths]);
84+
toSelectionPathRef.current = presented.selectionPath;
85+
}, [modelPaths, onSelectFile, presented.selectionPath]);
7386

7487
const { model } = useFileTree({
7588
density: "compact",
@@ -78,8 +91,13 @@ export function DiffFileTree({
7891
icons: T3_PIERRE_ICONS,
7992
onSelectionChange: (selectedPaths) => {
8093
if (syncingSelectionRef.current) return;
81-
const path = selectedPaths.at(-1)?.replace(/\/$/, "");
82-
if (path && filePathsRef.current.has(path)) onSelectFileRef.current(path);
94+
const raw = selectedPaths.at(-1);
95+
if (!raw) return;
96+
// Directory ids end in `/`. A file that shares that directory's name is stored under a
97+
// different model path, so stripping the slash must not select it.
98+
const modelPath = filePathsRef.current.has(raw) ? raw : raw.replace(/\/$/, "");
99+
if (!filePathsRef.current.has(modelPath)) return;
100+
onSelectFileRef.current(toSelectionPathRef.current(modelPath));
83101
},
84102
paths: [],
85103
search: false,
@@ -93,59 +111,67 @@ export function DiffFileTree({
93111
useEffect(() => {
94112
ordering.update(positions);
95113
const mountedPaths = mountedPathsRef.current;
96-
if (mountedPaths === paths) return;
97-
mountedPathsRef.current = paths;
114+
if (mountedPaths === modelPaths) return;
115+
mountedPathsRef.current = modelPaths;
98116
if (mountedPaths === null) {
99-
model.resetPaths(paths);
100-
} else if (mountedPaths.every((path, index) => paths[index] === path)) {
101-
const updates = buildDiffFileTreeUpdates(mountedPaths, paths);
117+
model.resetPaths(modelPaths);
118+
} else if (mountedPaths.every((path, index) => modelPaths[index] === path)) {
119+
const updates = buildDiffFileTreeUpdates(mountedPaths, modelPaths);
102120
if (updates.length > 0) model.batch(updates);
103121
} else {
104-
// A refreshed diff can change the rank of existing siblings. Mutations do not reorder
105-
// those rows, so rebuild while carrying the reader's folder expansion forward.
122+
// A refreshed diff can change the rank of existing siblings. A file that becomes a
123+
// directory prefix also changes its model path, which Pierre cannot rename in place.
124+
// Rebuild while carrying the reader's folder expansion forward.
106125
const collapsedDirectories = directoryPaths.filter((path) => {
107126
const directory = model.getItem(path);
108127
return directory !== null && "isExpanded" in directory && !directory.isExpanded();
109128
});
110-
model.resetPaths(paths);
129+
model.resetPaths(modelPaths);
111130
for (const path of collapsedDirectories) {
112131
const directory = model.getItem(path);
113132
if (directory !== null && "collapse" in directory) directory.collapse();
114133
}
115134
}
116135
model.setGitStatus(gitStatus);
117-
}, [directoryPaths, gitStatus, model, ordering, paths, positions]);
136+
}, [directoryPaths, gitStatus, model, modelPaths, ordering, positions]);
118137

119138
useEffect(() => {
120139
if (selectedPath === null) {
121140
handledRevealRef.current = null;
122141
return;
123142
}
124-
const item = model.getItem(selectedPath);
143+
const modelPath = presented.modelPath(selectedPath);
144+
const item = model.getItem(modelPath);
125145
if (item === null || item.isDirectory()) {
126146
handledRevealRef.current = null;
127147
return;
128148
}
129149
const handled = handledRevealRef.current;
130-
if (handled?.path === selectedPath && handled.revealRequestId === revealRequestId) return;
131-
handledRevealRef.current = { path: selectedPath, revealRequestId };
150+
if (
151+
handled?.path === selectedPath &&
152+
handled.revealRequestId === revealRequestId &&
153+
handled.modelPath === modelPath
154+
) {
155+
return;
156+
}
157+
handledRevealRef.current = { path: selectedPath, revealRequestId, modelPath };
132158
syncingSelectionRef.current = true;
133159
for (const path of model.getSelectedPaths()) {
134-
if (path !== selectedPath) model.getItem(path)?.deselect();
160+
if (path !== modelPath) model.getItem(path)?.deselect();
135161
}
136162
let ancestor = "";
137-
for (const segment of selectedPath.split("/").slice(0, -1)) {
163+
for (const segment of modelPath.split("/").slice(0, -1)) {
138164
ancestor += `${segment}/`;
139165
const directory = model.getItem(ancestor);
140166
if (directory !== null && "expand" in directory) directory.expand();
141167
}
142168
item.select();
143-
model.scrollToPath(selectedPath, { offset: "nearest" });
169+
model.scrollToPath(modelPath, { offset: "nearest" });
144170
queueMicrotask(() => {
145171
syncingSelectionRef.current = false;
146172
});
147-
// `paths` is a dependency so a file that arrives after it was asked for is still revealed.
148-
}, [model, paths, revealRequestId, selectedPath]);
173+
// `presented` is a dependency so a file that arrives after it was asked for is still revealed.
174+
}, [model, presented, revealRequestId, selectedPath]);
149175

150176
return (
151177
<div className={cn("flex min-h-0 flex-1 flex-col bg-background", className)}>
@@ -201,14 +227,16 @@ export function DiffFileTree({
201227
// Pierre does not emit a selection change for its sole selected row.
202228
// Read selection before the row handles the click so new selections reveal only once.
203229
const selected = model.getSelectedPaths();
204-
const path = selected.length === 1 ? selected[0] : undefined;
205-
if (!path || !filePathsRef.current.has(path)) return;
230+
const raw = selected.length === 1 ? selected[0] : undefined;
231+
if (!raw) return;
232+
const path = filePathsRef.current.has(raw) ? raw : raw.replace(/\/$/, "");
233+
if (!filePathsRef.current.has(path)) return;
206234
const clickedSelectedRow = event.nativeEvent
207235
.composedPath()
208236
.some(
209237
(node) => node instanceof HTMLElement && node.getAttribute("data-item-path") === path,
210238
);
211-
if (clickedSelectedRow) onSelectFileRef.current(path);
239+
if (clickedSelectedRow) onSelectFileRef.current(toSelectionPathRef.current(path));
212240
}}
213241
className="min-h-0 flex-1 overflow-hidden"
214242
style={pierreTreeStyle(resolvedTheme)}

‎apps/web/src/components/diffs/diffFileTree.logic.test.ts‎

Lines changed: 108 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,12 @@
11
import type { FileDiffMetadata } from "@pierre/diffs";
2-
import { preloadFileTree } from "@pierre/trees";
2+
import { FileTree, preloadFileTree } from "@pierre/trees";
33
import { describe, expect, it } from "vite-plus/test";
44

55
import {
66
buildDiffFileTreeUpdates,
77
compareDiffFileTreeEntries,
88
collectDirectoryPaths,
9+
diffFileTreeModel,
910
diffFileTreePositions,
1011
diffFileTreeEntries,
1112
} from "./diffFileTree.logic";
@@ -115,3 +116,109 @@ describe("buildDiffFileTreeUpdates", () => {
115116
expect(buildDiffFileTreeUpdates(["src/a.ts"], ["src/a.ts"])).toEqual([]);
116117
});
117118
});
119+
120+
describe("diffFileTreeModel", () => {
121+
it("returns ordinary paths unchanged", () => {
122+
const paths = ["src/a.ts", "README.md"];
123+
const presented = diffFileTreeModel(paths);
124+
expect(presented.paths).toBe(paths);
125+
expect(presented.selectionPath("src/a.ts")).toBe("src/a.ts");
126+
expect(presented.modelPath("src/a.ts")).toBe("src/a.ts");
127+
});
128+
129+
it("does not treat a shared string prefix as a directory collision", () => {
130+
const paths = ["office", "officer.ts"];
131+
expect(diffFileTreeModel(paths).paths).toBe(paths);
132+
});
133+
134+
it("does not rewrite a directory that only contains files", () => {
135+
const paths = ["office/config.ts", "office/index.ts"];
136+
expect(diffFileTreeModel(paths).paths).toBe(paths);
137+
});
138+
139+
it("keeps a file that is also a directory prefix selectable in Pierre", () => {
140+
const paths = ["office", "office/config.ts"];
141+
const presented = diffFileTreeModel(paths);
142+
expect(presented.paths).not.toBe(paths);
143+
expect(presented.selectionPath(presented.modelPath("office"))).toBe("office");
144+
expect(presented.selectionPath(presented.modelPath("office/config.ts"))).toBe(
145+
"office/config.ts",
146+
);
147+
expect(presented.modelPath("office/config.ts")).toBe("office/config.ts");
148+
expect(presented.modelPath("office")).not.toBe("office");
149+
150+
const tree = new FileTree({
151+
paths: presented.paths,
152+
initialExpansion: "open",
153+
flattenEmptyDirectories: true,
154+
});
155+
expect(tree.getItem(presented.modelPath("office"))?.isDirectory()).toBe(false);
156+
expect(tree.getItem("office/")?.isDirectory()).toBe(true);
157+
expect(tree.getItem("office/config.ts")?.isDirectory()).toBe(false);
158+
tree.cleanUp();
159+
});
160+
161+
it("keeps the reverse directory-to-file transition selectable", () => {
162+
const paths = ["office/config.ts", "office"];
163+
const presented = diffFileTreeModel(paths);
164+
expect(presented.paths[0]).toBe("office/config.ts");
165+
expect(presented.selectionPath(presented.modelPath("office"))).toBe("office");
166+
expect(() => {
167+
const tree = new FileTree({ paths: presented.paths, initialExpansion: "open" });
168+
tree.cleanUp();
169+
}).not.toThrow();
170+
});
171+
172+
it("rewrites a colliding file deeper than the first segment, and a chain of prefixes", () => {
173+
const nested = diffFileTreeModel(["src/office", "src/office/config.ts"]);
174+
expect(nested.modelPath("src/office")).not.toBe("src/office");
175+
expect(nested.modelPath("src/office/config.ts")).toBe("src/office/config.ts");
176+
expect(nested.selectionPath(nested.modelPath("src/office"))).toBe("src/office");
177+
178+
const chain = diffFileTreeModel(["a", "a/b", "a/b/c"]);
179+
expect(chain.modelPath("a")).not.toBe("a");
180+
expect(chain.modelPath("a/b")).not.toBe("a/b");
181+
expect(chain.modelPath("a/b/c")).toBe("a/b/c");
182+
const tree = new FileTree({ paths: chain.paths, initialExpansion: "open" });
183+
expect(tree.getItem(chain.modelPath("a"))?.isDirectory()).toBe(false);
184+
expect(tree.getItem(chain.modelPath("a/b"))?.isDirectory()).toBe(false);
185+
expect(tree.getItem("a/b/c")?.isDirectory()).toBe(false);
186+
tree.cleanUp();
187+
});
188+
189+
it("does not reuse a diff path that already ends with the file mark", () => {
190+
const marked = "office\u200b";
191+
const paths = ["office", marked, "office/config.ts"];
192+
const presented = diffFileTreeModel(paths);
193+
expect(new Set(presented.paths).size).toBe(paths.length);
194+
expect(presented.modelPath("office")).not.toBe(marked);
195+
expect(presented.selectionPath(presented.modelPath("office"))).toBe("office");
196+
expect(presented.selectionPath(presented.modelPath(marked))).toBe(marked);
197+
const tree = new FileTree({ paths: presented.paths, initialExpansion: "open" });
198+
expect(tree.getItem(presented.modelPath("office"))?.isDirectory()).toBe(false);
199+
expect(tree.getItem(presented.modelPath(marked))?.isDirectory()).toBe(false);
200+
expect(tree.getItem("office/config.ts")?.isDirectory()).toBe(false);
201+
tree.cleanUp();
202+
});
203+
204+
it("rejects the raw colliding list and accepts a later slice once paths are safe", () => {
205+
const tree = new FileTree({ paths: ["office"], initialExpansion: "open" });
206+
expect(() =>
207+
tree.batch(buildDiffFileTreeUpdates(["office"], ["office", "office/config.ts"])),
208+
).toThrow(/collides with an existing file/);
209+
210+
const appended = diffFileTreeModel(["src/a.ts", "office", "office/config.ts"]);
211+
const updates = buildDiffFileTreeUpdates(["src/a.ts"], appended.paths);
212+
const appending = new FileTree({ paths: ["src/a.ts"], initialExpansion: "open" });
213+
expect(() => appending.batch(updates)).not.toThrow();
214+
expect(appending.getItem(appended.modelPath("office"))?.isDirectory()).toBe(false);
215+
expect(appending.getItem("office/config.ts")?.isDirectory()).toBe(false);
216+
217+
const rewritten = diffFileTreeModel(["office", "office/config.ts"]);
218+
expect(() => tree.resetPaths(rewritten.paths)).not.toThrow();
219+
expect(tree.getItem(rewritten.modelPath("office"))?.isDirectory()).toBe(false);
220+
expect(tree.getItem("office/config.ts")?.isDirectory()).toBe(false);
221+
tree.cleanUp();
222+
appending.cleanUp();
223+
});
224+
});

0 commit comments

Comments
 (0)