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 b535c4f45e97..ced3ccfa297d 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,37 @@ 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. A + // base-branch advance without a head commit moves `behindBy` instead, so it rides along in + // 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. + const fileContentsRevisionKey = useMemo( + () => + pullRequestFileContentsRevisionKey({ + commits: detail.commits, + commit, + behindBy: detail.behindBy, + }) ?? `updated:${detail.updatedAt}`, + [commit, detail.behindBy, 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..5fe6e7d4cc1a 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,100 @@ 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")); + }); + + 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", () => { expect( diff --git a/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts b/apps/web/src/components/pullRequest/pullRequestDetail.logic.ts index 733e314b770f..3d95d67c2cdd 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,56 @@ 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 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. + * + * 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 }>; + readonly commit: string | null; + readonly behindBy?: number | null | undefined; +}): 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); + 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. */ 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..633f942a3ea9 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,441 @@ 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 = "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("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("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(`src/file-${index}.ts`)); + } + // file-0 fell out; file-1 is still held. + 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("src/file-1.ts")); + expect(getDiffFileContents).toHaveBeenCalledTimes( + PULL_REQUEST_FILE_CONTENTS_CACHE_MAX_ENTRIES + 2, + ); + }); + + 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("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("src/a.ts"))).resolves.toMatchObject({ + newFile: { contents: "after@head-1\n" }, + }); + revision = { baseSha: "base-1", headSha: "head-2" }; + 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("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("src/a.ts"))).resolves.toMatchObject({ + oldFile: { contents: "before@base-1\n" }, + }); + revision = { baseSha: "base-2", headSha: "head-1" }; + 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); + }); + + 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("src/a.ts")); + const pendingB = load(prFileDiff("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("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("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("src/a.ts")); + shouldFail = true; + 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("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("src/a.ts")); + await load(prFileDiff("src/b.ts")); + await load(prFileDiff("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("src/a.ts")); + await load(prFileDiff("src/b.ts")); + await load(prFileDiff("src/a.ts")); + + 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("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("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("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("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). + 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("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("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..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,29 +107,162 @@ 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); } +/** + * 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 — + // 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; + readonly newContents: string; + readonly size: number; + } + const settled = 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 + // 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"]; + 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 => { + const key = keyOf(input); + const hit = takeSettled(key); + // 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); + // 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: { + ...source.reference, + ...(source.commit === null ? {} : { commit: source.commit }), + changeType: input.changeType, + oldPath: input.oldPath, + newPath: input.newPath, + }, + }); + if (result._tag !== "Success") { + throw squashAtomCommandFailure(result); + } + 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 { oldContents: value.oldContents, newContents: value.newContents, revision: served }; + })(); + inflight.set(key, { promise: pending, sequence }); + try { + return await pending; + } finally { + // 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); } 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;