From 2e42068266f31f6bbb250d4e34deb00e5dc92d3c Mon Sep 17 00:00:00 2001 From: Cestercian Date: Mon, 21 Sep 2026 09:11:25 +0000 Subject: [PATCH] fix(web): keep PR diffs open when a file becomes a directory Git can replace a file or symlink with a directory of the same name (and the reverse). Pierre's tree cannot represent both, so opening those diffs crashed the sidebar. Detect the prefix collision and show a flat file list instead of handing the colliding paths to the tree. Co-authored-by: Cestercian --- .../components/diffs/DiffFileTree.test.tsx | 124 ++++++++++++ .../web/src/components/diffs/DiffFileTree.tsx | 188 ++++++++++++++---- .../diffs/diffFileTree.logic.test.ts | 38 ++++ .../components/diffs/diffFileTree.logic.ts | 30 +++ 4 files changed, 341 insertions(+), 39 deletions(-) diff --git a/apps/web/src/components/diffs/DiffFileTree.test.tsx b/apps/web/src/components/diffs/DiffFileTree.test.tsx index 3a97c60254ab..1720f14281bc 100644 --- a/apps/web/src/components/diffs/DiffFileTree.test.tsx +++ b/apps/web/src/components/diffs/DiffFileTree.test.tsx @@ -191,3 +191,127 @@ describe("diff tree file activation", () => { expect(targets).toHaveLength(1); }); }); + +describe("diff tree file/directory prefix collisions", () => { + let renderer: ReactTestRenderer | undefined; + const targets: CodeViewScrollTarget[] = []; + const viewer = { + getInstance: () => viewer, + scrollTo: (target: CodeViewScrollTarget) => targets.push(target), + }; + + function Panel({ + files, + selectedPath = null, + revealRequestId = 0, + }: { + files: DiffFileTreeEntry[]; + selectedPath?: string | null; + revealRequestId?: number; + }) { + const reveal = useCodeViewFileReveal(viewer, "working-tree"); + return ( + reveal(`${path}\0${path}`)} + /> + ); + } + + async function mount(props: Parameters[0]) { + await act(async () => { + renderer = create(); + }); + } + + async function activateListed(path: string) { + const button = renderer!.root.find( + (node) => node.type === "button" && node.props["data-item-path"] === path, + ); + await act(async () => { + button.props.onClick(); + }); + } + + beforeEach(() => { + targets.length = 0; + vi.useFakeTimers(); + vi.stubGlobal("IS_REACT_ACT_ENVIRONMENT", true); + vi.stubGlobal("HTMLElement", TreeRow); + }); + + afterEach(async () => { + await act(async () => renderer?.unmount()); + renderer = undefined; + vi.runOnlyPendingTimers(); + vi.useRealTimers(); + vi.unstubAllGlobals(); + }); + + it("keeps every file selectable when a path is both a file and a directory", async () => { + await mount({ + files: [ + { path: "office", status: "deleted" }, + { path: "office/config.ts", status: "added" }, + ], + }); + await activateListed("office"); + await activateListed("office/config.ts"); + expect(targets.map((target) => ("id" in target ? target.id : null))).toEqual([ + "office\u0000office", + "office/config.ts\u0000office/config.ts", + ]); + }); + + it("keeps every file selectable for the reverse directory-to-file transition", async () => { + await mount({ + files: [ + { path: "office/config.ts", status: "deleted" }, + { path: "office", status: "added" }, + ], + }); + await activateListed("office/config.ts"); + await activateListed("office"); + expect(targets.map((target) => ("id" in target ? target.id : null))).toEqual([ + "office/config.ts\u0000office/config.ts", + "office\u0000office", + ]); + }); + + it("switches to the list when a later slice introduces a colliding descendant", async () => { + await mount({ files: [{ path: "src/a.ts", status: "modified" }] }); + await act(async () => { + renderer!.update( + , + ); + }); + await activateListed("office"); + await activateListed("office/config.ts"); + expect(targets.map((target) => ("id" in target ? target.id : null))).toEqual([ + "office\u0000office", + "office/config.ts\u0000office/config.ts", + ]); + }); + + it("keeps every colliding file selectable after a refresh of the same set", async () => { + const files: DiffFileTreeEntry[] = [ + { path: "office", status: "deleted" }, + { path: "office/config.ts", status: "added" }, + ]; + await mount({ files, selectedPath: "office" }); + await act(async () => { + renderer!.update( ({ ...file }))} selectedPath="office" />); + }); + await activateListed("office"); + expect(targets).toEqual([{ type: "item", id: "office\u0000office", align: "start" }]); + }); +}); diff --git a/apps/web/src/components/diffs/DiffFileTree.tsx b/apps/web/src/components/diffs/DiffFileTree.tsx index 0d200853bcb4..641a727309e2 100644 --- a/apps/web/src/components/diffs/DiffFileTree.tsx +++ b/apps/web/src/components/diffs/DiffFileTree.tsx @@ -14,6 +14,7 @@ import { Tooltip, TooltipPopup, TooltipTrigger } from "../ui/tooltip"; import { buildDiffFileTreeUpdates, collectDirectoryPaths, + diffFileTreeModelPaths, type DiffFileTreeEntry, } from "./diffFileTree.logic"; @@ -40,6 +41,9 @@ interface DiffFileTreeProps { /** * A directory tree of the files in a diff. Every directory starts open: a diff is a short list * compared to a workspace, and the reader came for the files, not the folders. + * + * Git can replace a file or symlink with a directory of the same name (and the reverse). Pierre + * cannot represent both, so those diffs are shown as a flat list instead of crashing the tree. */ export function DiffFileTree({ entries, @@ -53,10 +57,15 @@ export function DiffFileTree({ }: DiffFileTreeProps) { const { resolvedTheme } = useTheme(); const paths = useMemo(() => entries.map((entry) => entry.path), [entries]); - const directoryPaths = useMemo(() => collectDirectoryPaths(paths), [paths]); + const treePaths = useMemo(() => diffFileTreeModelPaths(paths), [paths]); + const hasPrefixCollision = treePaths !== paths; + const directoryPaths = useMemo(() => collectDirectoryPaths(treePaths), [treePaths]); const gitStatus = useMemo>( - () => entries.map((entry) => ({ path: entry.path, status: entry.status })), - [entries], + () => + hasPrefixCollision + ? [] + : entries.map((entry) => ({ path: entry.path, status: entry.status })), + [entries, hasPrefixCollision], ); const filePathsRef = useRef>(new Set(paths)); const onSelectFileRef = useRef(onSelectFile); @@ -91,16 +100,16 @@ export function DiffFileTree({ useEffect(() => { const mountedPaths = mountedPathsRef.current; - if (mountedPaths === paths) return; - mountedPathsRef.current = paths; + if (mountedPaths === treePaths) return; + mountedPathsRef.current = treePaths; if (mountedPaths === null) { - model.resetPaths(paths); + model.resetPaths(treePaths); } else { - const updates = buildDiffFileTreeUpdates(mountedPaths, paths); + const updates = buildDiffFileTreeUpdates(mountedPaths, treePaths); if (updates.length > 0) model.batch(updates); } model.setGitStatus(gitStatus); - }, [gitStatus, model, paths]); + }, [gitStatus, model, treePaths]); useEffect(() => { if (selectedPath === null) { @@ -109,6 +118,12 @@ export function DiffFileTree({ } // A path list that changes under an already-revealed file (a refresh, a later slice) must // not pull the tree back to it over whatever the reader has picked since. + if (!treePaths.includes(selectedPath)) { + // A file that left the diff, or a colliding set shown as a flat list, has to be revealed + // again when it is back in the tree. + handledRevealRef.current = null; + return; + } const item = model.getItem(selectedPath); if (item === null || item.isDirectory()) { // A file that left the diff has to be revealed again when it comes back. @@ -133,8 +148,7 @@ export function DiffFileTree({ queueMicrotask(() => { syncingSelectionRef.current = false; }); - // `paths` is a dependency so a file that arrives after it was asked for is still revealed. - }, [model, paths, revealRequestId, selectedPath]); + }, [model, revealRequestId, selectedPath, treePaths]); return (
@@ -174,36 +188,132 @@ export function DiffFileTree({ ) : null}
- { - if ( - event.defaultPrevented || - event.button !== 0 || - event.ctrlKey || - event.metaKey || - event.shiftKey || - event.altKey - ) { - return; - } - // Pierre does not emit a selection change for its sole selected row. - // Read selection before the row handles the click so new selections reveal only once. - const selected = model.getSelectedPaths(); - const path = selected.length === 1 ? selected[0] : undefined; - if (!path || !filePathsRef.current.has(path)) return; - const clickedSelectedRow = event.nativeEvent - .composedPath() - .some( - (node) => node instanceof HTMLElement && node.getAttribute("data-item-path") === path, - ); - if (clickedSelectedRow) onSelectFileRef.current(path); - }} - className="min-h-0 flex-1 overflow-hidden" - style={pierreTreeStyle(resolvedTheme)} - /> + {hasPrefixCollision ? ( + + ) : ( + { + if ( + event.defaultPrevented || + event.button !== 0 || + event.ctrlKey || + event.metaKey || + event.shiftKey || + event.altKey + ) { + return; + } + // Pierre does not emit a selection change for its sole selected row. + // Read selection before the row handles the click so new selections reveal only once. + const selected = model.getSelectedPaths(); + const path = selected.length === 1 ? selected[0] : undefined; + if (!path || !filePathsRef.current.has(path)) return; + const clickedSelectedRow = event.nativeEvent + .composedPath() + .some( + (node) => + node instanceof HTMLElement && node.getAttribute("data-item-path") === path, + ); + if (clickedSelectedRow) onSelectFileRef.current(path); + }} + className="min-h-0 flex-1 overflow-hidden" + style={pierreTreeStyle(resolvedTheme)} + /> + )} {footer} ); } + +function gitStatusMark(status: DiffFileTreeEntry["status"]): string { + switch (status) { + case "added": + return "A"; + case "deleted": + return "D"; + case "renamed": + return "R"; + default: + return "M"; + } +} + +function gitStatusMarkClassName(status: DiffFileTreeEntry["status"]): string { + switch (status) { + case "added": + return "text-diff-addition"; + case "deleted": + return "text-diff-deletion"; + default: + return "text-muted-foreground"; + } +} + +/** Flat fallback when git replaces a file/symlink with a directory of the same name, or the reverse. */ +function DiffFileCollisionList({ + entries, + selectedPath, + revealRequestId, + ariaLabel, + onSelectFile, +}: { + entries: ReadonlyArray; + selectedPath: string | null; + revealRequestId: number; + ariaLabel: string; + onSelectFile: (path: string) => void; +}) { + const selectedRef = useRef(null); + const handledRevealRef = useRef<{ path: string; revealRequestId: number } | null>(null); + useEffect(() => { + if (selectedPath === null) { + handledRevealRef.current = null; + return; + } + const handled = handledRevealRef.current; + if (handled?.path === selectedPath && handled.revealRequestId === revealRequestId) return; + handledRevealRef.current = { path: selectedPath, revealRequestId }; + selectedRef.current?.scrollIntoView?.({ block: "nearest" }); + }, [revealRequestId, selectedPath]); + + return ( +
    + {entries.map((entry) => { + const selected = entry.path === selectedPath; + return ( +
  • + +
    • + ); + })} +
+ ); +} diff --git a/apps/web/src/components/diffs/diffFileTree.logic.test.ts b/apps/web/src/components/diffs/diffFileTree.logic.test.ts index d8e24968dcea..1a355434cd3c 100644 --- a/apps/web/src/components/diffs/diffFileTree.logic.test.ts +++ b/apps/web/src/components/diffs/diffFileTree.logic.test.ts @@ -5,6 +5,8 @@ import { buildDiffFileTreeUpdates, collectDirectoryPaths, diffFileTreeEntries, + diffFileTreeModelPaths, + hasFileDirectoryPrefixCollision, } from "./diffFileTree.logic"; function file(type: FileDiffMetadata["type"], name: string, prevName = name): FileDiffMetadata { @@ -67,3 +69,39 @@ describe("buildDiffFileTreeUpdates", () => { expect(buildDiffFileTreeUpdates(["src/a.ts"], ["src/a.ts"])).toEqual([]); }); }); + +describe("hasFileDirectoryPrefixCollision", () => { + it("detects a file replaced by a directory of the same name", () => { + expect(hasFileDirectoryPrefixCollision(["office", "office/config.ts"])).toBe(true); + }); + + it("detects the reverse directory-to-file transition", () => { + expect(hasFileDirectoryPrefixCollision(["office/config.ts", "office"])).toBe(true); + }); + + it("detects a collision deeper than the first segment", () => { + expect(hasFileDirectoryPrefixCollision(["src/office", "src/office/config.ts"])).toBe(true); + }); + + it("does not treat a shared string prefix as a colliding directory", () => { + expect(hasFileDirectoryPrefixCollision(["office", "officer.ts"])).toBe(false); + }); + + it("does not collide when only the directory's descendants are present", () => { + expect(hasFileDirectoryPrefixCollision(["office/config.ts", "office/index.ts"])).toBe(false); + }); +}); + +describe("diffFileTreeModelPaths", () => { + it("hands ordinary diffs to Pierre unchanged", () => { + const paths = ["src/a.ts", "README.md"]; + expect(diffFileTreeModelPaths(paths)).toBe(paths); + }); + + it("withholds a colliding set so Pierre never builds both a file and its directory", () => { + expect(diffFileTreeModelPaths(["office", "office/config.ts"])).toEqual([]); + expect(diffFileTreeModelPaths(["office", "office/config.ts"])).toBe( + diffFileTreeModelPaths(["office/config.ts", "office"]), + ); + }); +}); diff --git a/apps/web/src/components/diffs/diffFileTree.logic.ts b/apps/web/src/components/diffs/diffFileTree.logic.ts index 4535ece8b143..c54cd3ca811c 100644 --- a/apps/web/src/components/diffs/diffFileTree.logic.ts +++ b/apps/web/src/components/diffs/diffFileTree.logic.ts @@ -30,6 +30,36 @@ export function diffFileTreeEntries( return files.map((file) => ({ path: resolveFileDiffPath(file), status: toGitStatus(file) })); } +const EMPTY_DIFF_FILE_TREE_PATHS: ReadonlyArray = []; + +/** + * Pierre's tree is a single filesystem: a path cannot be a file and a directory at once. + * Git can produce both when a file or symlink is replaced by a directory of the same name + * (delete `office`, add `office/config.ts`) or the reverse. + */ +export function hasFileDirectoryPrefixCollision(paths: ReadonlyArray): boolean { + if (paths.length < 2) return false; + const unique = [...new Set(paths)].toSorted(); + for (let index = 0; index < unique.length; index++) { + const path = unique[index]!; + const directoryPrefix = `${path}/`; + for (let next = index + 1; next < unique.length; next++) { + const other = unique[next]!; + if (!other.startsWith(path)) break; + if (other.startsWith(directoryPrefix)) return true; + } + } + return false; +} + +/** + * Paths Pierre can represent as a tree. A colliding set is withheld entirely so the sidebar + * can show a flat list instead of throwing while creating a directory that is already a file. + */ +export function diffFileTreeModelPaths(paths: ReadonlyArray): ReadonlyArray { + return hasFileDirectoryPrefixCollision(paths) ? EMPTY_DIFF_FILE_TREE_PATHS : paths; +} + /** * Every directory on the way to each file, registered with the trailing slash Pierre uses for * directory ids. Parents come before children so the tree can add them in order.