diff --git a/apps/web/src/components/pullRequest/pullRequestFileOrder.logic.test.ts b/apps/web/src/components/pullRequest/pullRequestFileOrder.logic.test.ts index d3d5d958a49f..4e1e865cac94 100644 --- a/apps/web/src/components/pullRequest/pullRequestFileOrder.logic.test.ts +++ b/apps/web/src/components/pullRequest/pullRequestFileOrder.logic.test.ts @@ -1,6 +1,7 @@ import type { FileDiffMetadata } from "@pierre/diffs"; import { describe, expect, it } from "vite-plus/test"; +import { buildFileDiffRenderKey, getRenderablePatch } from "../../lib/diffRendering"; import { orderDiffFiles } from "./pullRequestFileOrder.logic"; /** Only the path and the patch's own lines matter here; the viewer fills the rest in. */ @@ -161,4 +162,47 @@ describe("orderDiffFiles", () => { ]; expect(order(files)).toEqual(order(files.toReversed())); }); + + it("keeps both blocks of a file that turns into a symlink", () => { + // Git writes the type change as a deletion and an addition of the same path. + const patch = [ + "diff --git a/src/b.ts b/src/b.ts", + "new file mode 100644", + "--- /dev/null", + "+++ b/src/b.ts", + "@@ -0,0 +1 @@", + "+export const b = 1;", + "diff --git a/AGENTS.md b/AGENTS.md", + "deleted file mode 100644", + "--- a/AGENTS.md", + "+++ /dev/null", + "@@ -1 +0,0 @@", + "-duplicated instructions", + "diff --git a/AGENTS.md b/AGENTS.md", + "new file mode 120000", + "--- /dev/null", + "+++ b/AGENTS.md", + "@@ -0,0 +1 @@", + "+CLAUDE.md", + ].join("\n"); + const parsed = getRenderablePatch(patch, "type-change-order"); + expect(parsed?.kind).toBe("files"); + if (parsed?.kind !== "files") return; + + const ordered = orderDiffFiles(parsed.files); + expect(ordered.map((entry) => `${entry.name}:${entry.type}`)).toEqual([ + "AGENTS.md:deleted", + "AGENTS.md:new", + "src/b.ts:new", + ]); + expect(new Set(ordered.map(buildFileDiffRenderKey)).size).toBe(ordered.length); + }); + + it("reads the imports of every block a path owns", () => { + const rewritten = [ + { ...file("src/a.ts"), deletionLines: ['import { b } from "./b";'] } as FileDiffMetadata, + file("src/a.ts", ["export const a = 1;"]), + ]; + expect(order([...rewritten, file("src/b.ts")])).toEqual(["src/b.ts", "src/a.ts", "src/a.ts"]); + }); }); diff --git a/apps/web/src/components/pullRequest/pullRequestFileOrder.logic.ts b/apps/web/src/components/pullRequest/pullRequestFileOrder.logic.ts index 1a0df9d9beda..a8b789abccd5 100644 --- a/apps/web/src/components/pullRequest/pullRequestFileOrder.logic.ts +++ b/apps/web/src/components/pullRequest/pullRequestFileOrder.logic.ts @@ -144,13 +144,19 @@ function testedBaseName(path: string): string { /** * Diff files in reading order: source in dependency order, then the tests that cover it, then - * whatever a tool wrote. + * whatever a tool wrote. A path can own several blocks: Git writes a file turning into a symlink + * as a deletion followed by an addition of the same path. They travel together, in patch order. */ export function orderDiffFiles( files: ReadonlyArray, ): ReadonlyArray { - const byPath = new Map(); - for (const file of files) byPath.set(resolveFileDiffPath(file), file); + const byPath = new Map>(); + for (const file of files) { + const path = resolveFileDiffPath(file); + const blocks = byPath.get(path); + if (blocks) blocks.push(file); + else byPath.set(path, [file]); + } const tiers = new Map(); for (const path of byPath.keys()) tiers.set(path, diffFileTier(path)); @@ -168,11 +174,10 @@ export function orderDiffFiles( const imports = new Map>(); for (const path of sourcePaths) { - const file = byPath.get(path)!; - imports.set( - path, - importedPaths(path, [...file.additionLines, ...file.deletionLines], byModulePath, byBaseName), - ); + const lines = byPath + .get(path)! + .flatMap((block) => [...block.additionLines, ...block.deletionLines]); + imports.set(path, importedPaths(path, lines, byModulePath, byBaseName)); } const orderedSource = orderByImports(sourcePaths, imports); @@ -193,5 +198,7 @@ export function orderDiffFiles( .filter((path) => tiers.get(path) === "generated") .sort((left, right) => left.localeCompare(right)); - return [...orderedSource, ...orderedTests, ...orderedGenerated].map((path) => byPath.get(path)!); + return [...orderedSource, ...orderedTests, ...orderedGenerated].flatMap((path) => + byPath.get(path)!, + ); }