From 386e02cd46be08ff6c7d25e1d3ebe77a588ded4b Mon Sep 17 00:00:00 2001 From: Adamulek123 Date: Sat, 12 Sep 2026 13:28:29 +0200 Subject: [PATCH 1/5] perf(web): key PR file contents by commit set, memoize loader --- .../pullRequest/PullRequestCodeTab.tsx | 23 ++- .../pullRequestDetail.logic.test.ts | 64 +++++++ .../pullRequest/pullRequestDetail.logic.ts | 37 ++++ apps/web/src/lib/diffFileContents.test.ts | 168 +++++++++++++++++- apps/web/src/lib/diffFileContents.ts | 111 ++++++++++-- 5 files changed, 383 insertions(+), 20 deletions(-) diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx index b535c4f45e97..7562db1adc67 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx @@ -33,7 +33,11 @@ import { useLocalStorage } from "~/hooks/useLocalStorage"; import { useClientSettings, useUpdateClientSettings } from "~/hooks/useSettings"; import { useTheme } from "~/hooks/useTheme"; import { areAllDiffFilesCollapsed } from "~/lib/diffCollapse"; -import { pullRequestFindingKey, type PullRequestFinding } from "./pullRequestDetail.logic"; +import { + pullRequestFileContentsRevisionKey, + pullRequestFindingKey, + type PullRequestFinding, +} from "./pullRequestDetail.logic"; import { canEditPullRequestComment } from "./pullRequestEditing.logic"; import { orderDiffFiles } from "./pullRequestFileOrder.logic"; import { @@ -355,15 +359,28 @@ function PullRequestCodeTab({ reportFailure: false, }); const getDiffFileContents = useAtomCommand(pullRequestEnvironment.diffFileContents); + // Revision-scoped, not update-scoped: `updatedAt` moves on comments, labels, and reviews + // while the files stay identical, and carrying it into the cache key throws away every file + // Pierre holds plus the loader's own memo. The commit set only moves when the code does, so + // the loader — and its memo — survives metadata touches and is rebuilt on a push. Empty + // (activity not yet loaded) keeps the previous conservative key rather than sharing one + // across revisions that cannot be told apart. Preservation is unit-covered in + // pullRequestDetail.logic.test.ts rather than by a component render here. + const fileContentsRevisionKey = useMemo( + () => + pullRequestFileContentsRevisionKey({ commits: detail.commits, commit }) ?? + `updated:${detail.updatedAt}`, + [commit, detail.commits, detail.updatedAt], + ); const loadDiffFiles = useMemo( () => createPullRequestDiffFileContentsLoader(getDiffFileContents, { environmentId, reference, commit, - cacheKey: `pull-request:${referenceKey}:${detail.updatedAt}:${commit ?? "all"}`, + cacheKey: `pull-request:${referenceKey}:${fileContentsRevisionKey}`, }), - [commit, detail.updatedAt, environmentId, getDiffFileContents, reference, referenceKey], + [commit, environmentId, fileContentsRevisionKey, getDiffFileContents, reference, referenceKey], ); // What is offered is the intersection of two different questions: what this host can do at diff --git a/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts b/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts index 5e75851f08ad..2bcd0ee61dec 100644 --- a/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts +++ b/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts @@ -35,6 +35,7 @@ import { pullRequestActionMenuHasGroup, pullRequestActionNeedsHostRefresh, pullRequestCheckoutCommand, + pullRequestFileContentsRevisionKey, pullRequestFindingKey, pullRequestReviewOutcome, readableFailure, @@ -162,6 +163,69 @@ describe("pull request activity refresh", () => { ).toBe(false); }); }); + +describe("pull request file-contents revision", () => { + const commits = [{ oid: "aaa" }, { oid: "bbb" }]; + + it("keys one commit's own comparison by its oid alone", () => { + expect(pullRequestFileContentsRevisionKey({ commits, commit: "bbb" })).toBe("commit:bbb"); + // Independent of the whole-PR commit set: the commit itself is immutable. + expect(pullRequestFileContentsRevisionKey({ commits: [{ oid: "zzz" }], commit: "bbb" })).toBe( + "commit:bbb", + ); + }); + + it("holds still while only metadata moves", () => { + // Same commit set as after an unrelated updatedAt bump: identical key, so the loader and + // Pierre keep every expanded file instead of re-reading them. + expect(pullRequestFileContentsRevisionKey({ commits, commit: null })).toBe( + pullRequestFileContentsRevisionKey({ + commits: [{ oid: "bbb" }, { oid: "aaa" }], + commit: null, + }), + ); + }); + + it("busts on a push and on a same-length force-push", () => { + const before = pullRequestFileContentsRevisionKey({ commits, commit: null }); + expect( + pullRequestFileContentsRevisionKey({ commits: [...commits, { oid: "ccc" }], commit: null }), + ).not.toBe(before); + expect( + pullRequestFileContentsRevisionKey({ + commits: [{ oid: "aaa" }, { oid: "ccc" }], + commit: null, + }), + ).not.toBe(before); + }); + + it("tells apart oid sets that only differ by a separator boundary", () => { + expect( + pullRequestFileContentsRevisionKey({ commits: [{ oid: "ab" }, { oid: "c" }], commit: null }), + ).not.toBe( + pullRequestFileContentsRevisionKey({ commits: [{ oid: "a" }, { oid: "bc" }], commit: null }), + ); + }); + + it("reports unknown while the activity has not loaded", () => { + expect(pullRequestFileContentsRevisionKey({ commits: [], commit: null })).toBe(null); + expect(pullRequestFileContentsRevisionKey({ commits: [{ oid: "" }], commit: null })).toBe(null); + }); + + it("keeps the CodeTab cache key still across updatedAt bumps while the revision is known", () => { + // Mirrors PullRequestCodeTab's `revisionKey ?? updated:${updatedAt}`: the loader's memo + // survives comments/labels/reviews, and only the unknown-revision fallback tracks updatedAt. + const keyFor = (oids: ReadonlyArray, updatedAt: string) => + pullRequestFileContentsRevisionKey({ + commits: oids.map((oid) => ({ oid })), + commit: null, + }) ?? `updated:${updatedAt}`; + expect(keyFor(["aaa", "bbb"], "2026-08-13T13:00:00Z")).toBe( + keyFor(["aaa", "bbb"], "2026-08-13T13:01:00Z"), + ); + expect(keyFor([], "2026-08-13T13:00:00Z")).not.toBe(keyFor([], "2026-08-13T13:01:00Z")); + }); +}); describe("review thread comment pages", () => { it("appends new comments once and keeps refreshed base comments", () => { expect( diff --git a/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts b/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts index 733e314b770f..d8e1b57590bc 100644 --- a/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts +++ b/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts @@ -33,6 +33,7 @@ import { import { inferReviewCommentFenceLanguage, type ReviewCommentContext } from "~/reviewCommentContext"; import { reviewCommentContextId } from "~/lib/composerContextRecords"; import { removeInlineContextReference } from "~/lib/composerContextReferences"; +import { fnv1a32 } from "~/lib/diffRendering"; export const PULL_REQUEST_MERGE_METHOD_LABELS: Record = { merge: "Merge", @@ -147,6 +148,42 @@ export function shouldRefreshPullRequestActivity( ): boolean { return previous !== null && previous.key === next.key && previous.updatedAt !== next.updatedAt; } + +/** + * The revision a pull request's file contents belong to, for scoping the hunk-expansion cache. + * `updatedAt` moves on comments, labels, and reviews while the files stay identical, so keying + * the cache on it throws every expanded file away on each of those. The commit set only moves + * when the code does — a push, a force-push, a rebase — so it busts exactly when the contents + * may have changed. One commit's own comparison is immutable and keyed by its oid directly. + * + * Null where the revision cannot be known yet (activity not loaded): the caller falls back to + * `updatedAt` rather than sharing one key across revisions it cannot tell apart. + * + * A base-branch advance without a new head commit keeps the same key and may serve the + * previous base side until the commit set moves. An explicit refresh re-reads the patch but + * keeps expanded files: threading its token into this key would also bust on every metadata + * touch, which is the thrash revision-scoping exists to avoid. + */ +export function pullRequestFileContentsRevisionKey(input: { + readonly commits: ReadonlyArray<{ readonly oid: string }>; + readonly commit: string | null; +}): string | null { + if (input.commit !== null) return `commit:${input.commit}`; + const oids = input.commits + .map((commit) => commit.oid) + .filter((oid) => oid.length > 0) + .sort(); + if (oids.length === 0) return null; + // The whole set, not just the head: a force-push can keep the length while replacing every + // oid, and a faked committer date can hide a new head from a newest-by-date read. NUL joins + // so ["ab", "c"] and ["a", "bc"] hash differently. Two independent FNV-1a passes make a + // 64-bit fingerprint, since a collision here serves another revision's files as this one's. + const joined = oids.join("\u0000"); + const low = fnv1a32(joined); + const high = fnv1a32(joined, 0x9e3779b9, 0x85ebca6b); + return `commits:${oids.length}:${low.toString(36)}${high.toString(36)}`; +} + /** Appends fetched pages without replacing fresher comments already in the activity response. */ export function mergePullRequestThreadComments( base: ReadonlyArray, diff --git a/apps/web/src/lib/diffFileContents.test.ts b/apps/web/src/lib/diffFileContents.test.ts index 203ff37c8886..d0c71129ff01 100644 --- a/apps/web/src/lib/diffFileContents.test.ts +++ b/apps/web/src/lib/diffFileContents.test.ts @@ -1,10 +1,21 @@ import type { FileDiffMetadata } from "@pierre/diffs"; -import { EnvironmentId, type ReviewDiffFileContentsResult } from "@t3tools/contracts"; +import type { AtomCommandResult } from "@t3tools/client-runtime/state/runtime"; +import { + EnvironmentId, + ProjectId, + type PullRequestDiffFileContentsResult, + type ReviewDiffFileContentsResult, +} from "@t3tools/contracts"; import * as Cause from "effect/Cause"; import { AsyncResult } from "effect/unstable/reactivity"; import { describe, expect, it, vi } from "vite-plus/test"; -import { createGitDiffFileContentsLoader } from "./diffFileContents"; +import { + createGitDiffFileContentsLoader, + createPullRequestDiffFileContentsLoader, + PULL_REQUEST_FILE_CONTENTS_CACHE_MAX_BYTES, + PULL_REQUEST_FILE_CONTENTS_CACHE_MAX_ENTRIES, +} from "./diffFileContents"; const SOURCE = { environmentId: EnvironmentId.make("environment-1"), @@ -78,3 +89,156 @@ describe("createGitDiffFileContentsLoader", () => { await expect(load(fileDiff())).rejects.toBe(failure); }); }); + +const PR_SOURCE = { + environmentId: EnvironmentId.make("environment-1"), + reference: { + projectId: ProjectId.make("project-1"), + repository: "acme/web", + number: 7, + }, + commit: null, + cacheKey: "pull-request:project-1/acme/web#7:commits:2:abc", +}; + +function prFileDiff(name = "b/src/file.ts", type: FileDiffMetadata["type"] = "change") { + return { type, name } as FileDiffMetadata; +} + +describe("createPullRequestDiffFileContentsLoader", () => { + it("expands the same file twice for one request", async () => { + const getDiffFileContents = vi.fn(async () => + AsyncResult.success({ + oldContents: "before\n", + newContents: "after\n", + }), + ); + const load = createPullRequestDiffFileContentsLoader(getDiffFileContents, PR_SOURCE); + + const first = await load(prFileDiff()); + const second = await load(prFileDiff()); + + expect(second).toEqual(first); + expect(getDiffFileContents).toHaveBeenCalledTimes(1); + expect(getDiffFileContents).toHaveBeenCalledWith({ + environmentId: "environment-1", + input: { + projectId: "project-1", + repository: "acme/web", + number: 7, + changeType: "change", + oldPath: "src/file.ts", + newPath: "src/file.ts", + }, + }); + }); + + it("shares one request between concurrent expansions of the same file", async () => { + let release!: () => void; + const gate = new Promise((resolve) => { + release = resolve; + }); + const getDiffFileContents = vi.fn(async () => { + await gate; + return AsyncResult.success({ + oldContents: "before\n", + newContents: "after\n", + }); + }); + const load = createPullRequestDiffFileContentsLoader(getDiffFileContents, PR_SOURCE); + + const both = Promise.all([load(prFileDiff()), load(prFileDiff())]); + release(); + const [first, second] = await both; + + expect(second).toEqual(first); + expect(getDiffFileContents).toHaveBeenCalledTimes(1); + }); + + it("reads each file once and keeps change types apart", async () => { + const getDiffFileContents = vi.fn(async () => + AsyncResult.success({ + oldContents: "before\n", + newContents: "after\n", + }), + ); + const load = createPullRequestDiffFileContentsLoader(getDiffFileContents, PR_SOURCE); + + await load(prFileDiff("b/src/a.ts")); + await load(prFileDiff("b/src/b.ts")); + await load(prFileDiff("b/src/a.ts")); + // Same paths but another comparison: the old side of a deletion is another read. + await load(prFileDiff("b/src/a.ts", "deleted")); + + expect(getDiffFileContents).toHaveBeenCalledTimes(3); + }); + + it("does not pin a file to a transient failure", async () => { + const failure = new Error("host hiccup"); + const getDiffFileContents = vi.fn( + async (): Promise> => + AsyncResult.failure(Cause.fail(failure)), + ); + const load = createPullRequestDiffFileContentsLoader(getDiffFileContents, PR_SOURCE); + + await expect(load(prFileDiff())).rejects.toBe(failure); + getDiffFileContents.mockImplementation(async () => + AsyncResult.success({ + oldContents: "before\n", + newContents: "after\n", + }), + ); + + await expect(load(prFileDiff())).resolves.toMatchObject({ + newFile: { name: "src/file.ts", contents: "after\n" }, + }); + expect(getDiffFileContents).toHaveBeenCalledTimes(2); + }); + + it("evicts the least recently expanded file past the entry cap", async () => { + const getDiffFileContents = vi.fn(async () => + AsyncResult.success({ + oldContents: "before\n", + newContents: "after\n", + }), + ); + const load = createPullRequestDiffFileContentsLoader(getDiffFileContents, PR_SOURCE); + + for (let index = 0; index < PULL_REQUEST_FILE_CONTENTS_CACHE_MAX_ENTRIES + 1; index += 1) { + await load(prFileDiff(`b/src/file-${index}.ts`)); + } + // file-0 fell out; file-1 is still held. + await load(prFileDiff("b/src/file-1.ts")); + await load(prFileDiff("b/src/file-0.ts")); + + expect(getDiffFileContents).toHaveBeenCalledTimes( + PULL_REQUEST_FILE_CONTENTS_CACHE_MAX_ENTRIES + 2, + ); + // file-1 survived file-0's return only with recency: FIFO would have dropped file-1 to + // make room for file-0, so this re-read stays free on LRU and costs one RPC on FIFO. + await load(prFileDiff("b/src/file-1.ts")); + expect(getDiffFileContents).toHaveBeenCalledTimes( + PULL_REQUEST_FILE_CONTENTS_CACHE_MAX_ENTRIES + 2, + ); + }); + + it("evicts by total size before the entry cap fills", async () => { + // One shared side keeps the test cheap: the cap counts lengths, not allocations, and two + // entries at ~2/3 of the cap each already exceed it with only two files held (cap is 30). + const side = "x".repeat(Math.floor(PULL_REQUEST_FILE_CONTENTS_CACHE_MAX_BYTES / 3)); + const getDiffFileContents = vi.fn(async () => + AsyncResult.success({ + oldContents: side, + newContents: side, + }), + ); + const load = createPullRequestDiffFileContentsLoader(getDiffFileContents, PR_SOURCE); + + await load(prFileDiff("b/src/big-a.ts")); + await load(prFileDiff("b/src/big-b.ts")); + expect(getDiffFileContents).toHaveBeenCalledTimes(2); + // Two entries are far below the entry cap, so a miss here proves the size arm evicted. + await load(prFileDiff("b/src/big-a.ts")); + expect(getDiffFileContents).toHaveBeenCalledTimes(3); + }); +}); diff --git a/apps/web/src/lib/diffFileContents.ts b/apps/web/src/lib/diffFileContents.ts index 2dc4e5558bd5..04b6f43045d8 100644 --- a/apps/web/src/lib/diffFileContents.ts +++ b/apps/web/src/lib/diffFileContents.ts @@ -100,25 +100,106 @@ export function createGitDiffFileContentsLoader( }, source.cacheKey); } +/** + * Bounds for one pull-request file-contents loader's memo. Each entry holds up to two whole + * files (the host caps each at 1 MiB), so the size cap — not the entry count — is what keeps a + * long expanding session from growing without bound. Sizes are UTF-16 code units, not bytes; + * still a bound, just a conservative one for non-BMP text. Exported for tests. + */ +export const PULL_REQUEST_FILE_CONTENTS_CACHE_MAX_ENTRIES = 30; +export const PULL_REQUEST_FILE_CONTENTS_CACHE_MAX_BYTES = 10 * 1024 * 1024; + /** Loads host-backed PR files, which may name revisions this checkout has never fetched. */ export function createPullRequestDiffFileContentsLoader( getDiffFileContents: GetPullRequestDiffFileContents, source: PullRequestDiffFileContentsSource, ): FileDiffContentsLoader { - return createDiffFileContentsLoader(async ({ changeType, oldPath, newPath }) => { - const result = await getDiffFileContents({ - environmentId: source.environmentId, - input: { - ...source.reference, - ...(source.commit === null ? {} : { commit: source.commit }), - changeType, - oldPath, - newPath, - }, - }); - if (result._tag !== "Success") { - throw squashAtomCommandFailure(result); + // One hunk expansion costs a refs lookup plus up to two raw file reads on the host, so an + // expansion is memoized by what it reads: the comparison is fixed for the life of this loader + // (its cache key already carries the revision), and only the file identity varies per call. + // Concurrent expansions of the same file share one request rather than racing two. + // Failures are never kept: a transient host error must not pin a file to its error. + interface SettledEntry { + readonly oldContents: string; + readonly newContents: string; + readonly size: number; + } + const settled = new Map(); + const inflight = new Map>(); + let settledBytes = 0; + // NUL separates the fields: git paths never contain it, while spaces are legal in them. + const keyOf = (input: { + readonly changeType: PullRequestDiffFileContentsInput["changeType"]; + readonly oldPath: string; + readonly newPath: string; + }) => [input.changeType, input.oldPath, input.newPath].join("\u0000"); + const takeSettled = (key: string): SettledEntry | null => { + const hit = settled.get(key); + if (!hit) return null; + // Refresh recency so the eviction below drops the least recently expanded file. + settled.delete(key); + settled.set(key, hit); + return hit; + }; + const storeSettled = (key: string, value: SettledEntry) => { + const previous = settled.get(key); + if (previous !== undefined) { + settled.delete(key); + settledBytes -= previous.size; } - return result.value; - }, source.cacheKey); + while ( + settled.size > 0 && + (settled.size >= PULL_REQUEST_FILE_CONTENTS_CACHE_MAX_ENTRIES || + settledBytes + value.size > PULL_REQUEST_FILE_CONTENTS_CACHE_MAX_BYTES) + ) { + const oldest = settled.keys().next(); + if (oldest.done) break; + const removed = settled.get(oldest.value); + settled.delete(oldest.value); + settledBytes -= removed?.size ?? 0; + } + settled.set(key, value); + settledBytes += value.size; + }; + const load = async (input: { + readonly changeType: PullRequestDiffFileContentsInput["changeType"]; + readonly oldPath: string; + readonly newPath: string; + }): Promise<{ readonly oldContents: string; readonly newContents: string }> => { + const key = keyOf(input); + const hit = takeSettled(key); + if (hit) return hit; + const ongoing = inflight.get(key); + if (ongoing) return ongoing; + const pending = (async (): Promise => { + const result = await getDiffFileContents({ + environmentId: source.environmentId, + input: { + ...source.reference, + ...(source.commit === null ? {} : { commit: source.commit }), + changeType: input.changeType, + oldPath: input.oldPath, + newPath: input.newPath, + }, + }); + if (result._tag !== "Success") { + throw squashAtomCommandFailure(result); + } + return { + oldContents: result.value.oldContents, + newContents: result.value.newContents, + // UTF-16 units, not bytes (see above): bounded either way, no extra measuring cost. + size: result.value.oldContents.length + result.value.newContents.length, + }; + })(); + inflight.set(key, pending); + try { + const value = await pending; + storeSettled(key, value); + return value; + } finally { + inflight.delete(key); + } + }; + return createDiffFileContentsLoader(load, source.cacheKey); } From 7c60fe37896367cae3e98963412013b5fd61f55a Mon Sep 17 00:00:00 2001 From: Adamulek123 Date: Sat, 12 Sep 2026 15:39:26 +0200 Subject: [PATCH 2/5] perf(web): fold behindBy into PR file-contents revision key --- .../pullRequest/PullRequestCodeTab.tsx | 19 +++++++----- .../pullRequestDetail.logic.test.ts | 31 +++++++++++++++++++ .../pullRequest/pullRequestDetail.logic.ts | 12 ++++--- apps/web/src/lib/diffFileContents.test.ts | 27 ++++++++++++++++ 4 files changed, 78 insertions(+), 11 deletions(-) diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx index 7562db1adc67..6b39c4f2c0a0 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx @@ -362,15 +362,20 @@ function PullRequestCodeTab({ // Revision-scoped, not update-scoped: `updatedAt` moves on comments, labels, and reviews // while the files stay identical, and carrying it into the cache key throws away every file // Pierre holds plus the loader's own memo. The commit set only moves when the code does, so - // the loader — and its memo — survives metadata touches and is rebuilt on a push. Empty - // (activity not yet loaded) keeps the previous conservative key rather than sharing one - // across revisions that cannot be told apart. Preservation is unit-covered in - // pullRequestDetail.logic.test.ts rather than by a component render here. + // the loader — and its memo — survives metadata touches and is rebuilt on a push. A + // base-branch advance without a head commit moves `behindBy` instead, so it rides along in + // the key where the host counted it. Empty (activity not yet loaded) keeps the previous + // conservative key rather than sharing one across revisions that cannot be told apart. + // Preservation is unit-covered in pullRequestDetail.logic.test.ts rather than by a + // component render here. const fileContentsRevisionKey = useMemo( () => - pullRequestFileContentsRevisionKey({ commits: detail.commits, commit }) ?? - `updated:${detail.updatedAt}`, - [commit, detail.commits, detail.updatedAt], + pullRequestFileContentsRevisionKey({ + commits: detail.commits, + commit, + behindBy: detail.behindBy, + }) ?? `updated:${detail.updatedAt}`, + [commit, detail.behindBy, detail.commits, detail.updatedAt], ); const loadDiffFiles = useMemo( () => diff --git a/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts b/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts index 2bcd0ee61dec..5fe6e7d4cc1a 100644 --- a/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts +++ b/apps/web/src/components/pullRequest/pullRequestDetail.logic.test.ts @@ -225,6 +225,37 @@ describe("pull request file-contents revision", () => { ); expect(keyFor([], "2026-08-13T13:00:00Z")).not.toBe(keyFor([], "2026-08-13T13:01:00Z")); }); + + it("busts on a base-branch advance without a head commit, where the host counted it", () => { + const before = pullRequestFileContentsRevisionKey({ commits, commit: null, behindBy: 2 }); + const after = pullRequestFileContentsRevisionKey({ commits, commit: null, behindBy: 5 }); + expect(before).not.toBeNull(); + expect(after).not.toBe(before); + expect(after).toBe(`${before!.split(":behind")[0]}:behind5`); + }); + + it("keeps the old key shape where behindBy is absent", () => { + const without = pullRequestFileContentsRevisionKey({ commits, commit: null }); + expect(without).toMatch(/^commits:2:[0-9a-z]+$/); + expect(pullRequestFileContentsRevisionKey({ commits, commit: null, behindBy: undefined })).toBe( + without, + ); + expect(pullRequestFileContentsRevisionKey({ commits, commit: null, behindBy: null })).toBe( + without, + ); + }); + + it("keeps one commit's own comparison keyed by its oid alone, whatever the base did", () => { + expect(pullRequestFileContentsRevisionKey({ commits, commit: "bbb", behindBy: 5 })).toBe( + "commit:bbb", + ); + }); + + it("stays unknown while the activity has not loaded, even where the base count is known", () => { + expect( + pullRequestFileContentsRevisionKey({ commits: [], commit: null, behindBy: 5 }), + ).toBeNull(); + }); }); describe("review thread comment pages", () => { it("appends new comments once and keeps refreshed base comments", () => { diff --git a/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts b/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts index d8e1b57590bc..9f648e20dff8 100644 --- a/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts +++ b/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts @@ -159,14 +159,17 @@ export function shouldRefreshPullRequestActivity( * Null where the revision cannot be known yet (activity not loaded): the caller falls back to * `updatedAt` rather than sharing one key across revisions it cannot tell apart. * - * A base-branch advance without a new head commit keeps the same key and may serve the - * previous base side until the commit set moves. An explicit refresh re-reads the patch but - * keeps expanded files: threading its token into this key would also bust on every metadata + * A base-branch advance without a new head commit leaves the commit set alone but moves + * `behindBy`, so the count rides along as a `:behindN` suffix and busts the key where the + * host counted it. Absent (a host that cannot compare, or an older caller) keeps the + * commit-set key exactly as before. An explicit refresh re-reads the patch but keeps + * expanded files: threading its token into this key would also bust on every metadata * touch, which is the thrash revision-scoping exists to avoid. */ export function pullRequestFileContentsRevisionKey(input: { readonly commits: ReadonlyArray<{ readonly oid: string }>; readonly commit: string | null; + readonly behindBy?: number | null | undefined; }): string | null { if (input.commit !== null) return `commit:${input.commit}`; const oids = input.commits @@ -181,7 +184,8 @@ export function pullRequestFileContentsRevisionKey(input: { const joined = oids.join("\u0000"); const low = fnv1a32(joined); const high = fnv1a32(joined, 0x9e3779b9, 0x85ebca6b); - return `commits:${oids.length}:${low.toString(36)}${high.toString(36)}`; + const base = `commits:${oids.length}:${low.toString(36)}${high.toString(36)}`; + return input.behindBy == null ? base : `${base}:behind${input.behindBy}`; } /** Appends fetched pages without replacing fresher comments already in the activity response. */ diff --git a/apps/web/src/lib/diffFileContents.test.ts b/apps/web/src/lib/diffFileContents.test.ts index d0c71129ff01..3b0162e1e6eb 100644 --- a/apps/web/src/lib/diffFileContents.test.ts +++ b/apps/web/src/lib/diffFileContents.test.ts @@ -222,6 +222,33 @@ describe("createPullRequestDiffFileContentsLoader", () => { ); }); + it("carries the revision key into each file so a base advance busts the render cache", async () => { + const getDiffFileContents = vi.fn(async () => + AsyncResult.success({ + oldContents: "before\n", + newContents: "after\n", + }), + ); + // The `:behindN` suffix is folded into the revision key upstream (PullRequestCodeTab); + // the loader stays opaque to it, but the per-file keys Pierre hydrates against must + // differ when it moves, or the previous base side would be served as this one's. + const loadBehind = createPullRequestDiffFileContentsLoader(getDiffFileContents, { + ...PR_SOURCE, + cacheKey: `${PR_SOURCE.cacheKey}:behind5`, + }); + const loadAhead = createPullRequestDiffFileContentsLoader(getDiffFileContents, { + ...PR_SOURCE, + cacheKey: `${PR_SOURCE.cacheKey}:behind2`, + }); + + const behind = await loadBehind(prFileDiff()); + const ahead = await loadAhead(prFileDiff()); + + expect(behind.newFile?.cacheKey).toContain(":behind5:"); + expect(ahead.newFile?.cacheKey).toContain(":behind2:"); + expect(behind.newFile?.cacheKey).not.toBe(ahead.newFile?.cacheKey); + }); + it("evicts by total size before the entry cap fills", async () => { // One shared side keeps the test cheap: the cap counts lengths, not allocations, and two // entries at ~2/3 of the cap each already exceed it with only two files held (cap is 30). From 9033bdb88fa1e174e9d60c15814671eb67f7b304 Mon Sep 17 00:00:00 2001 From: Adamulek123 Date: Sun, 13 Sep 2026 23:51:03 +0200 Subject: [PATCH 3/5] fix(pr): echo served revisions in file contents and bust the memo on change The file-contents revision key rides the commit set plus behindBy, but both ride queries that can lag a refreshed diff, and a base replacement at the same behindBy count moves neither. The server now echoes the base/head revisions each file read actually served (optional contract fields; absent means unknown, root-commit new files omit baseSha), and the loader busts its memo when a read lands on new revisions, ordered by fetch creation so a late older read never steps the memo back. --- .../pullRequest/GitHubPullRequestCli.test.ts | 33 +++- .../src/pullRequest/GitHubPullRequestCli.ts | 10 +- .../pullRequest/GitLabPullRequestCli.test.ts | 45 ++++- .../src/pullRequest/GitLabPullRequestCli.ts | 9 +- .../src/pullRequest/PullRequestProvider.ts | 3 + .../pullRequest/PullRequestCodeTab.tsx | 6 +- .../pullRequest/pullRequestDetail.logic.ts | 10 ++ apps/web/src/lib/diffFileContents.test.ts | 168 ++++++++++++++++++ apps/web/src/lib/diffFileContents.ts | 41 ++++- packages/contracts/src/pullRequest.ts | 9 + 10 files changed, 325 insertions(+), 9 deletions(-) diff --git a/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts b/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts index fd5ab04e8a81..345b6437d355 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestCli.test.ts @@ -2560,12 +2560,43 @@ layer("GitHubPullRequestCli.layer", (it) => { newPath: "src/root.ts", }); - expect(contents).toEqual({ oldContents: "", newContents: "root contents\n" }); + expect(contents).toEqual({ + oldContents: "", + newContents: "root contents\n", + // No parent to echo: a root commit's new file leaves `baseSha` absent. + headSha: "a1b2c3d", + }); assert.strictEqual(mockedExecute.mock.calls.length, 2); expect(callAt(1).args.join(" ")).toContain("contents/src/root.ts?ref=a1b2c3d"); }), ); + it.effect("echoes the revisions the host actually read", () => + Effect.gen(function* () { + mockedExecute.mockReturnValueOnce(Effect.succeed(output("a1b2c3d\tb1c2d3e\n"))); + mockedExecute.mockReturnValueOnce(Effect.succeed(output("before\n"))); + mockedExecute.mockReturnValueOnce(Effect.succeed(output("after\n"))); + const cli = yield* GitHubPullRequestCli.GitHubPullRequestCli; + + const contents = yield* cli.getPullRequestDiffFileContents({ + cwd: "/w", + repository: "acme/web", + host: "github.com", + number: 7, + changeType: "change", + oldPath: "src/a.ts", + newPath: "src/a.ts", + }); + + expect(contents).toEqual({ + oldContents: "before\n", + newContents: "after\n", + baseSha: "a1b2c3d", + headSha: "b1c2d3e", + }); + }), + ); + it.effect("reports unusable diff revisions as a structured error", () => Effect.gen(function* () { mockedExecute.mockReturnValueOnce(Effect.succeed(output("not-a-sha\tstill-not-a-sha\n"))); diff --git a/apps/server/src/pullRequest/GitHubPullRequestCli.ts b/apps/server/src/pullRequest/GitHubPullRequestCli.ts index 723fcfc4d9b9..3aef0e744c8c 100644 --- a/apps/server/src/pullRequest/GitHubPullRequestCli.ts +++ b/apps/server/src/pullRequest/GitHubPullRequestCli.ts @@ -1517,7 +1517,15 @@ export const make = Effect.gen(function* () { ], { concurrency: 2 }, ); - return { oldContents, newContents }; + // Echo the revisions actually read so the caller can invalidate its memo when the + // comparison moved under it. A root commit's new file has no parent: `baseRef` is + // the empty jq field there, and the echo stays absent rather than naming nothing. + return { + oldContents, + newContents, + ...(baseRef.length > 0 ? { baseSha: baseRef } : {}), + headSha: headRef, + }; }); const readLegacyDetail = ( diff --git a/apps/server/src/pullRequest/GitLabPullRequestCli.test.ts b/apps/server/src/pullRequest/GitLabPullRequestCli.test.ts index daa85ee9eb8f..a34a67174901 100644 --- a/apps/server/src/pullRequest/GitLabPullRequestCli.test.ts +++ b/apps/server/src/pullRequest/GitLabPullRequestCli.test.ts @@ -670,11 +670,54 @@ layer("GitLabPullRequestCli.layer", (it) => { newPath: "src/first.ts", }); - expect(contents).toEqual({ oldContents: "", newContents: "first contents\n" }); + expect(contents).toEqual({ + oldContents: "", + newContents: "first contents\n", + // No parent to echo: a root commit's new file leaves `baseSha` absent. + headSha: "a1b2c3d", + }); expect(argsOfCall(1)[1]).toContain("raw?ref=a1b2c3d"); }), ); + it.effect("echoes the revisions the host actually read", () => + Effect.gen(function* () { + mockedExecute.mockReturnValueOnce( + Effect.succeed( + output( + // @effect-diagnostics-next-line preferSchemaOverJson:off + JSON.stringify({ + diff_refs: { + base_sha: "a1b2c3d", + head_sha: "b1c2d3e", + start_sha: "a1b2c3d", + }, + }), + ), + ), + ); + mockedExecute.mockReturnValueOnce(Effect.succeed(output("before\n"))); + mockedExecute.mockReturnValueOnce(Effect.succeed(output("after\n"))); + const cli = yield* GitLabPullRequestCli.GitLabPullRequestCli; + + const contents = yield* cli.getMergeRequestDiffFileContents({ + cwd: "/w", + repository: "acme/web", + number: 7, + changeType: "change", + oldPath: "src/a.ts", + newPath: "src/a.ts", + }); + + expect(contents).toEqual({ + oldContents: "before\n", + newContents: "after\n", + baseSha: "a1b2c3d", + headSha: "b1c2d3e", + }); + }), + ); + it.effect("reports an oversized diff file with its path and reason", () => Effect.gen(function* () { mockedExecute.mockReturnValueOnce( diff --git a/apps/server/src/pullRequest/GitLabPullRequestCli.ts b/apps/server/src/pullRequest/GitLabPullRequestCli.ts index 5a07bb1ae37c..3a07e762b0a0 100644 --- a/apps/server/src/pullRequest/GitLabPullRequestCli.ts +++ b/apps/server/src/pullRequest/GitLabPullRequestCli.ts @@ -1295,7 +1295,14 @@ export const make = Effect.gen(function* () { ], { concurrency: 2 }, ); - return { oldContents, newContents }; + // Echo the revisions actually read (see PullRequestDiffFileContentsResult). A root + // commit's new file has no parent (`baseSha === ""`), which stays absent. + return { + oldContents, + newContents, + ...(refs.baseSha.length > 0 ? { baseSha: refs.baseSha } : {}), + headSha: refs.headSha, + }; }), getProjectMergeCapabilities: (input) => diff --git a/apps/server/src/pullRequest/PullRequestProvider.ts b/apps/server/src/pullRequest/PullRequestProvider.ts index b4361c125c38..57b99148d62d 100644 --- a/apps/server/src/pullRequest/PullRequestProvider.ts +++ b/apps/server/src/pullRequest/PullRequestProvider.ts @@ -258,6 +258,9 @@ export interface ProviderDiffSlice { export interface ProviderDiffFileContents { readonly oldContents: string; readonly newContents: string; + /** Echoed read revisions (see PullRequestDiffFileContentsResult); absent means unknown. */ + readonly baseSha?: string | undefined; + readonly headSha?: string | undefined; } export interface ProviderFilesViewed { diff --git a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx index 6b39c4f2c0a0..ced3ccfa297d 100644 --- a/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx +++ b/apps/web/src/components/pullRequest/PullRequestCodeTab.tsx @@ -364,7 +364,11 @@ function PullRequestCodeTab({ // Pierre holds plus the loader's own memo. The commit set only moves when the code does, so // the loader — and its memo — survives metadata touches and is rebuilt on a push. A // base-branch advance without a head commit moves `behindBy` instead, so it rides along in - // the key where the host counted it. Empty (activity not yet loaded) keeps the previous + // the key where the host counted it. The key is the coarse gate: the loader also busts + // its memo when a served read echoes new revisions (same-count base replacement, or a + // refreshed diff outrunning lagging activity), so files expanded after the move read the + // code the host actually served. Already-displayed expansions re-render only when the + // key rebuilds the loader. Empty (activity not yet loaded) keeps the previous // conservative key rather than sharing one across revisions that cannot be told apart. // Preservation is unit-covered in pullRequestDetail.logic.test.ts rather than by a // component render here. diff --git a/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts b/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts index 9f648e20dff8..3d95d67c2cdd 100644 --- a/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts +++ b/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts @@ -165,6 +165,16 @@ export function shouldRefreshPullRequestActivity( * commit-set key exactly as before. An explicit refresh re-reads the patch but keeps * expanded files: threading its token into this key would also bust on every metadata * touch, which is the thrash revision-scoping exists to avoid. + * + * This key is the coarse gate, not the whole correctness story: a base replacement at + * the same `behindBy` count moves neither arm of it, and the commits behind it ride the + * activity query while the diff rides its own, so a refreshed diff can outrun a lagging + * activity read. The loader closes both windows from the other side — the server echoes + * the revisions each file read actually served, and the first read that lands on new + * revisions busts the loader memo. Residual: entries settled before the move are served + * until the next file read observes it; with no new read, only the key above protects. + * Already-expanded files on screen likewise re-render only when the key rebuilds the + * loader — the bust covers subsequently expanded files. */ export function pullRequestFileContentsRevisionKey(input: { readonly commits: ReadonlyArray<{ readonly oid: string }>; diff --git a/apps/web/src/lib/diffFileContents.test.ts b/apps/web/src/lib/diffFileContents.test.ts index 3b0162e1e6eb..cdd06114225a 100644 --- a/apps/web/src/lib/diffFileContents.test.ts +++ b/apps/web/src/lib/diffFileContents.test.ts @@ -249,6 +249,174 @@ describe("createPullRequestDiffFileContentsLoader", () => { expect(behind.newFile?.cacheKey).not.toBe(ahead.newFile?.cacheKey); }); + it("busts settled entries when a read echoes new revisions", async () => { + // The revision key rides lagging queries, so a push or base replacement can land + // without rebuilding this loader. The first read served from the new comparison + // busts entries settled under the old one; without the echo the old file would stand. + let revision = { baseSha: "base-1", headSha: "head-1" }; + const getDiffFileContents = vi.fn(async () => + AsyncResult.success({ + oldContents: `before@${revision.headSha}\n`, + newContents: `after@${revision.headSha}\n`, + ...revision, + }), + ); + const load = createPullRequestDiffFileContentsLoader(getDiffFileContents, PR_SOURCE); + + await expect(load(prFileDiff("b/src/a.ts"))).resolves.toMatchObject({ + newFile: { contents: "after@head-1\n" }, + }); + revision = { baseSha: "base-1", headSha: "head-2" }; + await expect(load(prFileDiff("b/src/b.ts"))).resolves.toMatchObject({ + newFile: { contents: "after@head-2\n" }, + }); + // Settled under head-1, busted by b.ts's read: a.ts walks again and serves head-2. + await expect(load(prFileDiff("b/src/a.ts"))).resolves.toMatchObject({ + newFile: { contents: "after@head-2\n" }, + }); + expect(getDiffFileContents).toHaveBeenCalledTimes(3); + }); + + it("busts on a base-only move at the same head", async () => { + // The motivating same-count case: the base is replaced without a new head commit, so + // the commit set (and any `:behindN` count the host kept) does not move. Only the + // echo sees it. + let revision = { baseSha: "base-1", headSha: "head-1" }; + const getDiffFileContents = vi.fn(async () => + AsyncResult.success({ + oldContents: `before@${revision.baseSha}\n`, + newContents: "after\n", + ...revision, + }), + ); + const load = createPullRequestDiffFileContentsLoader(getDiffFileContents, PR_SOURCE); + + await expect(load(prFileDiff("b/src/a.ts"))).resolves.toMatchObject({ + oldFile: { contents: "before@base-1\n" }, + }); + revision = { baseSha: "base-2", headSha: "head-1" }; + await load(prFileDiff("b/src/b.ts")); + await expect(load(prFileDiff("b/src/a.ts"))).resolves.toMatchObject({ + oldFile: { contents: "before@base-2\n" }, + }); + expect(getDiffFileContents).toHaveBeenCalledTimes(3); + }); + + it("keeps the newer revision when concurrent reads resolve out of order", async () => { + // A push lands mid-expansion: the older read resolves after the newer one. The older + // caller still gets what the host served it, but the memo stays on the newer + // comparison instead of stepping back. + const echoByPath = new Map([ + ["src/a.ts", { baseSha: "base-1", headSha: "head-1" }], + ["src/b.ts", { baseSha: "base-1", headSha: "head-2" }], + ]); + const release = new Map void>(); + // Only the opening round is held; refetches answer immediately with the live echo. + const deferred = new Set(["src/a.ts", "src/b.ts"]); + const getDiffFileContents = vi.fn( + (request: { + input: { newPath: string }; + }): Promise> => { + const echo = echoByPath.get(request.input.newPath) ?? { + baseSha: "base-1", + headSha: "head-2", + }; + const result = AsyncResult.success({ + oldContents: "before\n", + newContents: `after@${echo.headSha}\n`, + ...echo, + }); + if (!deferred.has(request.input.newPath)) return Promise.resolve(result); + deferred.delete(request.input.newPath); + return new Promise((resolve) => { + release.set(request.input.newPath, () => resolve(result)); + }); + }, + ); + const load = createPullRequestDiffFileContentsLoader(getDiffFileContents, PR_SOURCE); + + const pendingA = load(prFileDiff("b/src/a.ts")); + const pendingB = load(prFileDiff("b/src/b.ts")); + // Newer resolves first, older lands late. + release.get("src/b.ts")?.(); + await pendingB; + release.get("src/a.ts")?.(); + await expect(pendingA).resolves.toMatchObject({ + newFile: { contents: "after@head-1\n" }, + }); + // Newer stayed settled; the late older read was served without storing. + await expect(load(prFileDiff("b/src/b.ts"))).resolves.toMatchObject({ + newFile: { contents: "after@head-2\n" }, + }); + expect(getDiffFileContents).toHaveBeenCalledTimes(2); + // The world moved on: a refetch now serves the newer comparison and busts to it. + echoByPath.set("src/a.ts", { baseSha: "base-1", headSha: "head-2" }); + await expect(load(prFileDiff("b/src/a.ts"))).resolves.toMatchObject({ + newFile: { contents: "after@head-2\n" }, + }); + expect(getDiffFileContents).toHaveBeenCalledTimes(3); + }); + + it("leaves the established revision intact when a later read fails", async () => { + const failure = new Error("host hiccup"); + let shouldFail = false; + const getDiffFileContents = vi.fn(async () => + shouldFail + ? AsyncResult.failure(Cause.fail(failure)) + : AsyncResult.success({ + oldContents: "before\n", + newContents: "after\n", + baseSha: "base-1", + headSha: "head-1", + }), + ); + const load = createPullRequestDiffFileContentsLoader(getDiffFileContents, PR_SOURCE); + + await load(prFileDiff("b/src/a.ts")); + shouldFail = true; + await expect(load(prFileDiff("b/src/b.ts"))).rejects.toBe(failure); + shouldFail = false; + // The failed read touched neither the memo nor the revision: a.ts stays settled. + await load(prFileDiff("b/src/a.ts")); + expect(getDiffFileContents).toHaveBeenCalledTimes(2); + }); + + it("keeps the memo when the echoed revisions do not move", async () => { + const getDiffFileContents = vi.fn(async () => + AsyncResult.success({ + oldContents: "before\n", + newContents: "after\n", + baseSha: "base-1", + headSha: "head-1", + }), + ); + const load = createPullRequestDiffFileContentsLoader(getDiffFileContents, PR_SOURCE); + + await load(prFileDiff("b/src/a.ts")); + await load(prFileDiff("b/src/b.ts")); + await load(prFileDiff("b/src/a.ts")); + + expect(getDiffFileContents).toHaveBeenCalledTimes(2); + }); + + it("keeps legacy behavior where the server echoes no revisions", async () => { + // Older servers answer contents alone: nothing to compare, so nothing to bust on — + // the revision key upstream stays the only gate, exactly as before. + const getDiffFileContents = vi.fn(async () => + AsyncResult.success({ + oldContents: "before\n", + newContents: "after\n", + }), + ); + const load = createPullRequestDiffFileContentsLoader(getDiffFileContents, PR_SOURCE); + + await load(prFileDiff("b/src/a.ts")); + await load(prFileDiff("b/src/b.ts")); + await load(prFileDiff("b/src/a.ts")); + + expect(getDiffFileContents).toHaveBeenCalledTimes(2); + }); + it("evicts by total size before the entry cap fills", async () => { // One shared side keeps the test cheap: the cap counts lengths, not allocations, and two // entries at ~2/3 of the cap each already exceed it with only two files held (cap is 30). diff --git a/apps/web/src/lib/diffFileContents.ts b/apps/web/src/lib/diffFileContents.ts index 04b6f43045d8..4f5d5eaf6d31 100644 --- a/apps/web/src/lib/diffFileContents.ts +++ b/apps/web/src/lib/diffFileContents.ts @@ -127,6 +127,22 @@ export function createPullRequestDiffFileContentsLoader( const settled = new Map(); const inflight = new Map>(); let settledBytes = 0; + // The revisions the last-applied read actually served, echoed by the server. The + // loader's identity (its cache key) already carries the commit set plus `:behindN`, but + // both ride queries that can lag a refreshed diff — and a base replacement at the same + // `behindBy` count moves neither. The first read that lands on new revisions therefore + // busts the memo it shares with older reads: every entry in it names the old comparison. + // Absent (an older server, or a host that cannot report revisions) keeps the previous + // behavior — nothing to compare, so nothing to bust on. Ordering is by fetch creation, + // not by landing: an older read resolving after a newer one served its caller without + // storing, so the memo never steps back to the older comparison. + let servedRevision: string | null = null; + let servedSequence = -1; + let nextSequence = 0; + const revisionOf = (result: PullRequestDiffFileContentsResult): string | null => { + if (result.headSha === undefined) return null; + return `${result.baseSha ?? ""}@${result.headSha}`; + }; // NUL separates the fields: git paths never contain it, while spaces are legal in them. const keyOf = (input: { readonly changeType: PullRequestDiffFileContentsInput["changeType"]; @@ -172,6 +188,8 @@ export function createPullRequestDiffFileContentsLoader( const ongoing = inflight.get(key); if (ongoing) return ongoing; const pending = (async (): Promise => { + const sequence = nextSequence; + nextSequence += 1; const result = await getDiffFileContents({ environmentId: source.environmentId, input: { @@ -185,18 +203,33 @@ export function createPullRequestDiffFileContentsLoader( if (result._tag !== "Success") { throw squashAtomCommandFailure(result); } - return { + const value = { oldContents: result.value.oldContents, newContents: result.value.newContents, // UTF-16 units, not bytes (see above): bounded either way, no extra measuring cost. size: result.value.oldContents.length + result.value.newContents.length, }; + const served = revisionOf(result.value); + if (served === null) { + storeSettled(key, value); + } else if (sequence >= servedSequence) { + if (servedRevision !== null && served !== servedRevision) { + // The comparison moved under this loader (push/force-push/base replacement that + // the revision key did not catch): entries already settled name the old code. + settled.clear(); + settledBytes = 0; + } + servedRevision = served; + servedSequence = sequence; + storeSettled(key, value); + } + // Otherwise an older read landed after a newer revision was established: its caller + // still gets what the host served it, but the memo stays on the newer comparison. + return value; })(); inflight.set(key, pending); try { - const value = await pending; - storeSettled(key, value); - return value; + return await pending; } finally { inflight.delete(key); } diff --git a/packages/contracts/src/pullRequest.ts b/packages/contracts/src/pullRequest.ts index b146810a8aec..3d7c0f923c26 100644 --- a/packages/contracts/src/pullRequest.ts +++ b/packages/contracts/src/pullRequest.ts @@ -979,6 +979,15 @@ export type PullRequestDiffFileContentsInput = typeof PullRequestDiffFileContent export const PullRequestDiffFileContentsResult = Schema.Struct({ oldContents: Schema.String, newContents: Schema.String, + /** + * The revisions the host actually read, echoed so the caller can tell whether its + * cached comparison still names the same code. Optional so older servers (and hosts + * that cannot report revisions) keep answering contents alone: absent means "unknown", + * never "unchanged". A root commit's new file has no parent, so `baseSha` stays absent + * there rather than claiming an empty revision. + */ + baseSha: Schema.optional(TrimmedNonEmptyString), + headSha: Schema.optional(TrimmedNonEmptyString), }); export type PullRequestDiffFileContentsResult = typeof PullRequestDiffFileContentsResult.Type; From e21692ed3ba36f51014c228ffb8b8bef00f05121 Mon Sep 17 00:00:00 2001 From: Adamulek123 Date: Mon, 14 Sep 2026 00:15:39 +0200 Subject: [PATCH 4/5] fix(web): supersede obsolete in-flight file reads, key hydrated files by revision Two review findings on the echoed-revision memo: a new expansion joining an in-flight read that predates the established revision was served known-stale content (fresh reads now supersede it, with identity-checked inflight cleanup), and hydrated FileContents cacheKeys stayed constant across revisions so Pierre could reuse previous-comparison highlights (served revision now rides each file key; legacy echo-less shape unchanged). --- apps/web/src/lib/diffFileContents.test.ts | 90 +++++++++++++++++++++++ apps/web/src/lib/diffFileContents.ts | 66 ++++++++++++----- 2 files changed, 138 insertions(+), 18 deletions(-) diff --git a/apps/web/src/lib/diffFileContents.test.ts b/apps/web/src/lib/diffFileContents.test.ts index cdd06114225a..4f8c982f8733 100644 --- a/apps/web/src/lib/diffFileContents.test.ts +++ b/apps/web/src/lib/diffFileContents.test.ts @@ -417,6 +417,96 @@ describe("createPullRequestDiffFileContentsLoader", () => { expect(getDiffFileContents).toHaveBeenCalledTimes(2); }); + it("supersedes an in-flight read predated by an established revision", async () => { + // A new expansion joining file A's still-flying revision-1 read would be served + // known-stale content. Once file B establishes revision 2, the new request starts a + // fresh read instead of joining; the late revision-1 landing still serves its own + // caller without storing. + const echoByPath = new Map([ + ["src/a.ts", { baseSha: "base-1", headSha: "head-1" }], + ["src/b.ts", { baseSha: "base-1", headSha: "head-2" }], + ]); + const release = new Map void>(); + const deferred = new Set(["src/a.ts", "src/b.ts"]); + const getDiffFileContents = vi.fn( + (request: { + input: { newPath: string }; + }): Promise> => { + const echo = echoByPath.get(request.input.newPath) ?? { + baseSha: "base-1", + headSha: "head-2", + }; + const result = AsyncResult.success({ + oldContents: "before\n", + newContents: `after@${echo.headSha}\n`, + ...echo, + }); + if (!deferred.has(request.input.newPath)) return Promise.resolve(result); + deferred.delete(request.input.newPath); + return new Promise((resolve) => { + release.set(request.input.newPath, () => resolve(result)); + }); + }, + ); + const load = createPullRequestDiffFileContentsLoader(getDiffFileContents, PR_SOURCE); + + const stale = load(prFileDiff("b/src/a.ts")); + const establishing = load(prFileDiff("b/src/b.ts")); + release.get("src/b.ts")?.(); + await establishing; + // The world moved on before the replacement read: it serves the new comparison. + echoByPath.set("src/a.ts", { baseSha: "base-1", headSha: "head-2" }); + const replacement = load(prFileDiff("b/src/a.ts")); + expect(getDiffFileContents).toHaveBeenCalledTimes(3); + release.get("src/a.ts")?.(); + await expect(stale).resolves.toMatchObject({ + newFile: { contents: "after@head-1\n" }, + }); + await expect(replacement).resolves.toMatchObject({ + newFile: { contents: "after@head-2\n" }, + }); + // The replacement settled; the stale landing stored nothing. + await expect(load(prFileDiff("b/src/a.ts"))).resolves.toMatchObject({ + newFile: { contents: "after@head-2\n" }, + }); + expect(getDiffFileContents).toHaveBeenCalledTimes(3); + }); + + it("keys hydrated files by served revision, legacy shape without echo", async () => { + // Pierre treats FileContents.cacheKey as a revision identity for worker-pool caching + // and hydration reuse: the same file served from two comparisons must key + // differently, or highlights from the previous revision are reused for the new one. + let revision = { baseSha: "base-1", headSha: "head-1" }; + const getDiffFileContents = vi.fn(async () => + AsyncResult.success({ + oldContents: "before\n", + newContents: "after\n", + ...revision, + }), + ); + const load = createPullRequestDiffFileContentsLoader(getDiffFileContents, PR_SOURCE); + + const first = await load(prFileDiff()); + expect(first.newFile?.cacheKey).toBe(`${PR_SOURCE.cacheKey}:new:src/file.ts:base-1@head-1`); + expect(first.oldFile?.cacheKey).toBe(`${PR_SOURCE.cacheKey}:old:src/file.ts:base-1@head-1`); + revision = { baseSha: "base-1", headSha: "head-2" }; + const second = await load(prFileDiff("b/src/other.ts")); + expect(second.newFile?.cacheKey).toBe(`${PR_SOURCE.cacheKey}:new:src/other.ts:base-1@head-2`); + + // No echo (older server): exactly the historical shape, so existing highlights keep + // hitting. + const legacyContents = vi.fn(async () => + AsyncResult.success({ + oldContents: "before\n", + newContents: "after\n", + }), + ); + const legacy = createPullRequestDiffFileContentsLoader(legacyContents, PR_SOURCE); + const legacyFirst = await legacy(prFileDiff()); + expect(legacyFirst.newFile?.cacheKey).toBe(`${PR_SOURCE.cacheKey}:new:src/file.ts`); + expect(legacyFirst.oldFile?.cacheKey).toBe(`${PR_SOURCE.cacheKey}:old:src/file.ts`); + }); + it("evicts by total size before the entry cap fills", async () => { // One shared side keeps the test cheap: the cap counts lengths, not allocations, and two // entries at ~2/3 of the cap each already exceed it with only two files held (cap is 30). diff --git a/apps/web/src/lib/diffFileContents.ts b/apps/web/src/lib/diffFileContents.ts index 4f5d5eaf6d31..31f6f807af90 100644 --- a/apps/web/src/lib/diffFileContents.ts +++ b/apps/web/src/lib/diffFileContents.ts @@ -42,12 +42,19 @@ type GetPullRequestDiffFileContents = (request: { readonly input: PullRequestDiffFileContentsInput; }) => Promise>; +/** One file read plus the revision it was served from (null where the server echoes none). */ +interface LoadedFileContents { + readonly oldContents: string; + readonly newContents: string; + readonly revision: string | null; +} + function createDiffFileContentsLoader( load: (input: { readonly changeType: PullRequestDiffFileContentsInput["changeType"]; readonly oldPath: string; readonly newPath: string; - }) => Promise<{ readonly oldContents: string; readonly newContents: string }>, + }) => Promise, cacheKey: string, ): FileDiffContentsLoader { return async (fileDiff) => { @@ -55,11 +62,15 @@ function createDiffFileContentsLoader( const oldPath = fileDiff.prevName ? resolveFileDiffPath({ ...fileDiff, name: fileDiff.prevName }) : newPath; - const contents = await load({ changeType: fileDiff.type, oldPath, newPath }); + const loaded = await load({ changeType: fileDiff.type, oldPath, newPath }); + // Pierre treats FileContents.cacheKey as a revision identity for worker-pool caching + // and hydration reuse: it must move whenever the served revision does, or highlights + // from the previous comparison are reused for the new one. + const revisionSuffix = loaded.revision === null ? "" : `:${loaded.revision}`; const newFile = { name: newPath, - contents: contents.newContents, - cacheKey: `${cacheKey}:new:${newPath}`, + contents: loaded.newContents, + cacheKey: `${cacheKey}:new:${newPath}${revisionSuffix}`, }; if (fileDiff.type === "rename-pure") { return { oldFile: null, newFile }; @@ -67,8 +78,8 @@ function createDiffFileContentsLoader( return { oldFile: { name: oldPath, - contents: contents.oldContents, - cacheKey: `${cacheKey}:old:${oldPath}`, + contents: loaded.oldContents, + cacheKey: `${cacheKey}:old:${oldPath}${revisionSuffix}`, }, newFile, }; @@ -96,7 +107,12 @@ export function createGitDiffFileContentsLoader( if (result._tag !== "Success") { throw squashAtomCommandFailure(result); } - return result.value; + // The Git comparison names its own revisions; there is nothing to echo back. + return { + oldContents: result.value.oldContents, + newContents: result.value.newContents, + revision: null, + }; }, source.cacheKey); } @@ -117,7 +133,9 @@ export function createPullRequestDiffFileContentsLoader( // One hunk expansion costs a refs lookup plus up to two raw file reads on the host, so an // expansion is memoized by what it reads: the comparison is fixed for the life of this loader // (its cache key already carries the revision), and only the file identity varies per call. - // Concurrent expansions of the same file share one request rather than racing two. + // Concurrent expansions of the same file share one request rather than racing two — + // unless that request predates the established revision, in which case a fresh read + // supersedes it rather than joining known-stale content. // Failures are never kept: a transient host error must not pin a file to its error. interface SettledEntry { readonly oldContents: string; @@ -125,7 +143,7 @@ export function createPullRequestDiffFileContentsLoader( readonly size: number; } const settled = new Map(); - const inflight = new Map>(); + const inflight = new Map; sequence: number }>(); let settledBytes = 0; // The revisions the last-applied read actually served, echoed by the server. The // loader's identity (its cache key) already carries the commit set plus `:behindN`, but @@ -181,15 +199,26 @@ export function createPullRequestDiffFileContentsLoader( readonly changeType: PullRequestDiffFileContentsInput["changeType"]; readonly oldPath: string; readonly newPath: string; - }): Promise<{ readonly oldContents: string; readonly newContents: string }> => { + }): Promise => { const key = keyOf(input); const hit = takeSettled(key); - if (hit) return hit; + // Settled entries always name the established revision (a move busts the memo), so a + // hit carries it. + if (hit) { + return { + oldContents: hit.oldContents, + newContents: hit.newContents, + revision: servedRevision, + }; + } const ongoing = inflight.get(key); - if (ongoing) return ongoing; - const pending = (async (): Promise => { - const sequence = nextSequence; - nextSequence += 1; + // Concurrent expansions of the same file share one request rather than racing two — + // unless that request predates the established revision, in which case joining it + // would serve the new caller known-stale content and a fresh read replaces it. + if (ongoing && ongoing.sequence >= servedSequence) return ongoing.promise; + const sequence = nextSequence; + nextSequence += 1; + const pending = (async (): Promise => { const result = await getDiffFileContents({ environmentId: source.environmentId, input: { @@ -225,13 +254,14 @@ export function createPullRequestDiffFileContentsLoader( } // Otherwise an older read landed after a newer revision was established: its caller // still gets what the host served it, but the memo stays on the newer comparison. - return value; + return { oldContents: value.oldContents, newContents: value.newContents, revision: served }; })(); - inflight.set(key, pending); + inflight.set(key, { promise: pending, sequence }); try { return await pending; } finally { - inflight.delete(key); + // A superseding read may have replaced this entry while it was in flight. + if (inflight.get(key)?.promise === pending) inflight.delete(key); } }; return createDiffFileContentsLoader(load, source.cacheKey); From 78e999704d9430d9699d2fd5ae4fdfa6458376e4 Mon Sep 17 00:00:00 2001 From: Adamulek123 Date: Sat, 19 Sep 2026 00:29:15 +0200 Subject: [PATCH 5/5] test(web): match prefix-free diff paths in PR file-contents fixtures #10822 removed the a/ and b/ stripping from resolveFileDiffPath, so the b/-prefixed fixtures this loader suite used now name real files and are forwarded to the host verbatim. Use the prefix-free names the patch parser yields; the memo, revision and LRU behavior under test is unchanged. --- apps/web/src/lib/diffFileContents.test.ts | 72 +++++++++++------------ 1 file changed, 36 insertions(+), 36 deletions(-) diff --git a/apps/web/src/lib/diffFileContents.test.ts b/apps/web/src/lib/diffFileContents.test.ts index 4f8c982f8733..633f942a3ea9 100644 --- a/apps/web/src/lib/diffFileContents.test.ts +++ b/apps/web/src/lib/diffFileContents.test.ts @@ -101,7 +101,7 @@ const PR_SOURCE = { cacheKey: "pull-request:project-1/acme/web#7:commits:2:abc", }; -function prFileDiff(name = "b/src/file.ts", type: FileDiffMetadata["type"] = "change") { +function prFileDiff(name = "src/file.ts", type: FileDiffMetadata["type"] = "change") { return { type, name } as FileDiffMetadata; } @@ -164,11 +164,11 @@ describe("createPullRequestDiffFileContentsLoader", () => { ); const load = createPullRequestDiffFileContentsLoader(getDiffFileContents, PR_SOURCE); - await load(prFileDiff("b/src/a.ts")); - await load(prFileDiff("b/src/b.ts")); - await load(prFileDiff("b/src/a.ts")); + await load(prFileDiff("src/a.ts")); + await load(prFileDiff("src/b.ts")); + await load(prFileDiff("src/a.ts")); // Same paths but another comparison: the old side of a deletion is another read. - await load(prFileDiff("b/src/a.ts", "deleted")); + await load(prFileDiff("src/a.ts", "deleted")); expect(getDiffFileContents).toHaveBeenCalledTimes(3); }); @@ -205,18 +205,18 @@ describe("createPullRequestDiffFileContentsLoader", () => { const load = createPullRequestDiffFileContentsLoader(getDiffFileContents, PR_SOURCE); for (let index = 0; index < PULL_REQUEST_FILE_CONTENTS_CACHE_MAX_ENTRIES + 1; index += 1) { - await load(prFileDiff(`b/src/file-${index}.ts`)); + await load(prFileDiff(`src/file-${index}.ts`)); } // file-0 fell out; file-1 is still held. - await load(prFileDiff("b/src/file-1.ts")); - await load(prFileDiff("b/src/file-0.ts")); + await load(prFileDiff("src/file-1.ts")); + await load(prFileDiff("src/file-0.ts")); expect(getDiffFileContents).toHaveBeenCalledTimes( PULL_REQUEST_FILE_CONTENTS_CACHE_MAX_ENTRIES + 2, ); // file-1 survived file-0's return only with recency: FIFO would have dropped file-1 to // make room for file-0, so this re-read stays free on LRU and costs one RPC on FIFO. - await load(prFileDiff("b/src/file-1.ts")); + await load(prFileDiff("src/file-1.ts")); expect(getDiffFileContents).toHaveBeenCalledTimes( PULL_REQUEST_FILE_CONTENTS_CACHE_MAX_ENTRIES + 2, ); @@ -263,15 +263,15 @@ describe("createPullRequestDiffFileContentsLoader", () => { ); const load = createPullRequestDiffFileContentsLoader(getDiffFileContents, PR_SOURCE); - await expect(load(prFileDiff("b/src/a.ts"))).resolves.toMatchObject({ + await expect(load(prFileDiff("src/a.ts"))).resolves.toMatchObject({ newFile: { contents: "after@head-1\n" }, }); revision = { baseSha: "base-1", headSha: "head-2" }; - await expect(load(prFileDiff("b/src/b.ts"))).resolves.toMatchObject({ + await expect(load(prFileDiff("src/b.ts"))).resolves.toMatchObject({ newFile: { contents: "after@head-2\n" }, }); // Settled under head-1, busted by b.ts's read: a.ts walks again and serves head-2. - await expect(load(prFileDiff("b/src/a.ts"))).resolves.toMatchObject({ + await expect(load(prFileDiff("src/a.ts"))).resolves.toMatchObject({ newFile: { contents: "after@head-2\n" }, }); expect(getDiffFileContents).toHaveBeenCalledTimes(3); @@ -291,12 +291,12 @@ describe("createPullRequestDiffFileContentsLoader", () => { ); const load = createPullRequestDiffFileContentsLoader(getDiffFileContents, PR_SOURCE); - await expect(load(prFileDiff("b/src/a.ts"))).resolves.toMatchObject({ + await expect(load(prFileDiff("src/a.ts"))).resolves.toMatchObject({ oldFile: { contents: "before@base-1\n" }, }); revision = { baseSha: "base-2", headSha: "head-1" }; - await load(prFileDiff("b/src/b.ts")); - await expect(load(prFileDiff("b/src/a.ts"))).resolves.toMatchObject({ + await load(prFileDiff("src/b.ts")); + await expect(load(prFileDiff("src/a.ts"))).resolves.toMatchObject({ oldFile: { contents: "before@base-2\n" }, }); expect(getDiffFileContents).toHaveBeenCalledTimes(3); @@ -335,8 +335,8 @@ describe("createPullRequestDiffFileContentsLoader", () => { ); const load = createPullRequestDiffFileContentsLoader(getDiffFileContents, PR_SOURCE); - const pendingA = load(prFileDiff("b/src/a.ts")); - const pendingB = load(prFileDiff("b/src/b.ts")); + const pendingA = load(prFileDiff("src/a.ts")); + const pendingB = load(prFileDiff("src/b.ts")); // Newer resolves first, older lands late. release.get("src/b.ts")?.(); await pendingB; @@ -345,13 +345,13 @@ describe("createPullRequestDiffFileContentsLoader", () => { newFile: { contents: "after@head-1\n" }, }); // Newer stayed settled; the late older read was served without storing. - await expect(load(prFileDiff("b/src/b.ts"))).resolves.toMatchObject({ + await expect(load(prFileDiff("src/b.ts"))).resolves.toMatchObject({ newFile: { contents: "after@head-2\n" }, }); expect(getDiffFileContents).toHaveBeenCalledTimes(2); // The world moved on: a refetch now serves the newer comparison and busts to it. echoByPath.set("src/a.ts", { baseSha: "base-1", headSha: "head-2" }); - await expect(load(prFileDiff("b/src/a.ts"))).resolves.toMatchObject({ + await expect(load(prFileDiff("src/a.ts"))).resolves.toMatchObject({ newFile: { contents: "after@head-2\n" }, }); expect(getDiffFileContents).toHaveBeenCalledTimes(3); @@ -372,12 +372,12 @@ describe("createPullRequestDiffFileContentsLoader", () => { ); const load = createPullRequestDiffFileContentsLoader(getDiffFileContents, PR_SOURCE); - await load(prFileDiff("b/src/a.ts")); + await load(prFileDiff("src/a.ts")); shouldFail = true; - await expect(load(prFileDiff("b/src/b.ts"))).rejects.toBe(failure); + await expect(load(prFileDiff("src/b.ts"))).rejects.toBe(failure); shouldFail = false; // The failed read touched neither the memo nor the revision: a.ts stays settled. - await load(prFileDiff("b/src/a.ts")); + await load(prFileDiff("src/a.ts")); expect(getDiffFileContents).toHaveBeenCalledTimes(2); }); @@ -392,9 +392,9 @@ describe("createPullRequestDiffFileContentsLoader", () => { ); const load = createPullRequestDiffFileContentsLoader(getDiffFileContents, PR_SOURCE); - await load(prFileDiff("b/src/a.ts")); - await load(prFileDiff("b/src/b.ts")); - await load(prFileDiff("b/src/a.ts")); + await load(prFileDiff("src/a.ts")); + await load(prFileDiff("src/b.ts")); + await load(prFileDiff("src/a.ts")); expect(getDiffFileContents).toHaveBeenCalledTimes(2); }); @@ -410,9 +410,9 @@ describe("createPullRequestDiffFileContentsLoader", () => { ); const load = createPullRequestDiffFileContentsLoader(getDiffFileContents, PR_SOURCE); - await load(prFileDiff("b/src/a.ts")); - await load(prFileDiff("b/src/b.ts")); - await load(prFileDiff("b/src/a.ts")); + await load(prFileDiff("src/a.ts")); + await load(prFileDiff("src/b.ts")); + await load(prFileDiff("src/a.ts")); expect(getDiffFileContents).toHaveBeenCalledTimes(2); }); @@ -450,13 +450,13 @@ describe("createPullRequestDiffFileContentsLoader", () => { ); const load = createPullRequestDiffFileContentsLoader(getDiffFileContents, PR_SOURCE); - const stale = load(prFileDiff("b/src/a.ts")); - const establishing = load(prFileDiff("b/src/b.ts")); + const stale = load(prFileDiff("src/a.ts")); + const establishing = load(prFileDiff("src/b.ts")); release.get("src/b.ts")?.(); await establishing; // The world moved on before the replacement read: it serves the new comparison. echoByPath.set("src/a.ts", { baseSha: "base-1", headSha: "head-2" }); - const replacement = load(prFileDiff("b/src/a.ts")); + const replacement = load(prFileDiff("src/a.ts")); expect(getDiffFileContents).toHaveBeenCalledTimes(3); release.get("src/a.ts")?.(); await expect(stale).resolves.toMatchObject({ @@ -466,7 +466,7 @@ describe("createPullRequestDiffFileContentsLoader", () => { newFile: { contents: "after@head-2\n" }, }); // The replacement settled; the stale landing stored nothing. - await expect(load(prFileDiff("b/src/a.ts"))).resolves.toMatchObject({ + await expect(load(prFileDiff("src/a.ts"))).resolves.toMatchObject({ newFile: { contents: "after@head-2\n" }, }); expect(getDiffFileContents).toHaveBeenCalledTimes(3); @@ -490,7 +490,7 @@ describe("createPullRequestDiffFileContentsLoader", () => { expect(first.newFile?.cacheKey).toBe(`${PR_SOURCE.cacheKey}:new:src/file.ts:base-1@head-1`); expect(first.oldFile?.cacheKey).toBe(`${PR_SOURCE.cacheKey}:old:src/file.ts:base-1@head-1`); revision = { baseSha: "base-1", headSha: "head-2" }; - const second = await load(prFileDiff("b/src/other.ts")); + const second = await load(prFileDiff("src/other.ts")); expect(second.newFile?.cacheKey).toBe(`${PR_SOURCE.cacheKey}:new:src/other.ts:base-1@head-2`); // No echo (older server): exactly the historical shape, so existing highlights keep @@ -519,11 +519,11 @@ describe("createPullRequestDiffFileContentsLoader", () => { ); const load = createPullRequestDiffFileContentsLoader(getDiffFileContents, PR_SOURCE); - await load(prFileDiff("b/src/big-a.ts")); - await load(prFileDiff("b/src/big-b.ts")); + await load(prFileDiff("src/big-a.ts")); + await load(prFileDiff("src/big-b.ts")); expect(getDiffFileContents).toHaveBeenCalledTimes(2); // Two entries are far below the entry cap, so a miss here proves the size arm evicted. - await load(prFileDiff("b/src/big-a.ts")); + await load(prFileDiff("src/big-a.ts")); expect(getDiffFileContents).toHaveBeenCalledTimes(3); }); });