From 45167e8c0dfc170fd0ecd1ead6cb31e92e7d441b Mon Sep 17 00:00:00 2001 From: Tim Reynolds Date: Sun, 13 Sep 2026 22:51:47 +0100 Subject: [PATCH] Centralize review snapshot comparison and fallback --- src/review/snapshot-loader.ts | 156 +++++++++-- src/server.ts | 150 +---------- test/review/public-snapshot.test.ts | 81 +----- test/review/snapshot-loader.test.ts | 403 ++++++++++++++++++++++++---- 4 files changed, 499 insertions(+), 291 deletions(-) diff --git a/src/review/snapshot-loader.ts b/src/review/snapshot-loader.ts index 78d3f0a..5e4b875 100644 --- a/src/review/snapshot-loader.ts +++ b/src/review/snapshot-loader.ts @@ -1,9 +1,14 @@ +import { + listFileChangesBetweenCommits, + type CommitCompareFileChange, + type CommitCompareOctokit, +} from "../github/commit-compare.js"; import type { ReviewPullRequestRef, ReviewSnapshotOptions } from "./store.js"; import { loadPublicReviewSnapshot, type PublicPullRequestSnapshotOctokit, } from "./public-snapshot.js"; -import type { ReviewSnapshot } from "./types.js"; +import type { ReviewFile, ReviewFileStatus, ReviewSnapshot } from "./types.js"; export type ReviewSnapshotStore = { loadSnapshot( @@ -14,11 +19,7 @@ export type ReviewSnapshotStore = { }; export type LoadReviewSnapshotInput = { - enhanceIndexedSnapshot?: ( - snapshot: ReviewSnapshot, - ref: ReviewPullRequestRef, - ) => Promise | ReviewSnapshot; - loadPublicSnapshot?: typeof loadPublicReviewSnapshot; + getComparisonOctokit(ref: ReviewPullRequestRef): Promise; options?: ReviewSnapshotOptions; publicOctokit: PublicPullRequestSnapshotOctokit; ref: ReviewPullRequestRef; @@ -35,19 +36,142 @@ export async function loadReviewSnapshot( input.options, ); if (indexedSnapshot !== undefined) { - return input.enhanceIndexedSnapshot === undefined - ? indexedSnapshot - : input.enhanceIndexedSnapshot(indexedSnapshot, input.ref); + return addGithubPatchsetComparison(indexedSnapshot, input); } - const publicSnapshot = await (input.loadPublicSnapshot ?? loadPublicReviewSnapshot)( - input.publicOctokit, - input.ref, - input.viewerLogin, + return loadPublicReviewSnapshot(input.publicOctokit, input.ref, input.viewerLogin); +} + +async function addGithubPatchsetComparison( + snapshot: ReviewSnapshot, + input: LoadReviewSnapshotInput, +): Promise { + const fromPatchset = snapshot.patchsets.find( + (patchset) => patchset.patchsetNumber === snapshot.comparison.fromPatchsetNumber, + ); + const toPatchset = snapshot.patchsets.find( + (patchset) => patchset.patchsetNumber === snapshot.comparison.toPatchsetNumber, ); - if (publicSnapshot !== undefined) { - return publicSnapshot; + if ( + fromPatchset === undefined || + toPatchset === undefined || + fromPatchset.patchsetNumber >= toPatchset.patchsetNumber + ) { + return snapshot; } - return undefined; + const ref = input.ref; + try { + const octokit = await input.getComparisonOctokit(ref); + const files = await listFileChangesBetweenCommits(octokit, { + base: fromPatchset.headSha, + head: toPatchset.headSha, + owner: ref.owner, + repo: ref.repo, + }); + + return buildComparedSnapshot(snapshot, files); + } catch (error) { + console.warn( + { + error: error instanceof Error ? error.message : "GitHub patchset comparison failed", + fromPatchsetNumber: fromPatchset.patchsetNumber, + pullNumber: ref.pullNumber, + repository: `${ref.owner}/${ref.repo}`, + toPatchsetNumber: toPatchset.patchsetNumber, + }, + "failed to load GitHub patchset comparison", + ); + + return { + ...snapshot, + capabilities: { + ...snapshot.capabilities, + limitations: appendUnique( + snapshot.capabilities.limitations, + "Exact GitHub commit comparison is unavailable; showing the indexed patchset snapshot.", + ), + }, + }; + } +} + +function buildComparedSnapshot( + snapshot: ReviewSnapshot, + fileChanges: CommitCompareFileChange[], +): ReviewSnapshot { + const files = fileChanges.map((file) => buildComparedReviewFile(snapshot.files, file)); + + return { + ...snapshot, + comparison: { + ...snapshot.comparison, + additions: files.reduce((total, file) => total + file.additions, 0), + deletions: files.reduce((total, file) => total + file.deletions, 0), + fileCount: files.length, + }, + files, + }; +} + +function buildComparedReviewFile( + indexedFiles: ReviewFile[], + file: CommitCompareFileChange, +): ReviewFile { + const indexedFile = + indexedFiles.find((candidate) => candidate.path === file.filename) ?? + indexedFiles.find((candidate) => candidate.path === file.previousFilename); + + return { + additions: file.additions, + deletions: file.deletions, + markState: indexedFile?.markState ?? "unreviewed", + markedAt: indexedFile?.markedAt, + markedPatchsetNumber: indexedFile?.markedPatchsetNumber, + patch: buildComparedFilePatch(file), + path: file.filename, + previousPath: file.previousFilename, + status: mapComparedFileStatus(file.status), + }; +} + +function buildComparedFilePatch(file: CommitCompareFileChange): string | undefined { + if (file.patch === undefined || file.patch.trim() === "") { + return undefined; + } + + if (file.patch.startsWith("diff --git")) { + return file.patch; + } + + const previousPath = file.previousFilename ?? file.filename; + const oldPath = file.status === "added" ? "/dev/null" : `a/${previousPath}`; + const newPath = file.status === "removed" ? "/dev/null" : `b/${file.filename}`; + + return [ + `diff --git a/${previousPath} b/${file.filename}`, + `--- ${oldPath}`, + `+++ ${newPath}`, + file.patch, + ].join("\n"); +} + +function mapComparedFileStatus(status: string): ReviewFileStatus { + switch (status) { + case "added": + return "added"; + case "removed": + return "deleted"; + case "renamed": + return "renamed"; + case "changed": + case "copied": + case "modified": + default: + return "modified"; + } +} + +function appendUnique(values: string[], value: string): string[] { + return values.includes(value) ? values : [...values, value]; } diff --git a/src/server.ts b/src/server.ts index 129ada3..568c314 100644 --- a/src/server.ts +++ b/src/server.ts @@ -23,8 +23,6 @@ import { resolveGithubReviewThread, serializeReviewThreadMarker, submitGithubPullRequestReview, - listFileChangesBetweenCommits, - type CommitCompareFileChange, type GithubPullRequestReviewEvent, type GithubReviewCommentMirror, } from "./github/index.js"; @@ -39,9 +37,6 @@ import { type CreateThreadRequest, type MarkReviewedRequest, type ReplyThreadRequest, - type ReviewFile, - type ReviewFileStatus, - type ReviewSnapshot, type SubmitReviewRequest, } from "./review/index.js"; @@ -314,7 +309,13 @@ async function handleReviewApiRequest( if (request.method === "GET" && route.action === undefined) { try { const snapshot = await loadReviewSnapshot({ - enhanceIndexedSnapshot: addGithubPatchsetComparison, + getComparisonOctokit: async (ref) => { + const installation = await app.octokit.request("GET /repos/{owner}/{repo}/installation", { + owner: ref.owner, + repo: ref.repo, + }); + return createInstallationOctokit(app, installation.data.id); + }, options: { fromPatchsetNumber: getOptionalPositiveInteger(url.searchParams.get("from")), toPatchsetNumber: getOptionalPositiveInteger(url.searchParams.get("to")), @@ -538,143 +539,6 @@ function createGithubReadOctokit(accessToken: string | undefined): Octokit { return accessToken === undefined ? new Octokit() : createUserOctokit(accessToken); } -async function addGithubPatchsetComparison( - snapshot: ReviewSnapshot, - ref: ReviewPullRequestApiRoute, -): Promise { - const fromPatchset = snapshot.patchsets.find( - (patchset) => patchset.patchsetNumber === snapshot.comparison.fromPatchsetNumber, - ); - const toPatchset = snapshot.patchsets.find( - (patchset) => patchset.patchsetNumber === snapshot.comparison.toPatchsetNumber, - ); - if ( - fromPatchset === undefined || - toPatchset === undefined || - fromPatchset.patchsetNumber >= toPatchset.patchsetNumber - ) { - return snapshot; - } - - try { - const installation = await app.octokit.request("GET /repos/{owner}/{repo}/installation", { - owner: ref.owner, - repo: ref.repo, - }); - const octokit = await createInstallationOctokit(app, installation.data.id); - const files = await listFileChangesBetweenCommits(octokit, { - base: fromPatchset.headSha, - head: toPatchset.headSha, - owner: ref.owner, - repo: ref.repo, - }); - - return buildComparedSnapshot(snapshot, files); - } catch (error) { - console.warn( - { - error: getErrorMessage(error, "GitHub patchset comparison failed"), - fromPatchsetNumber: fromPatchset.patchsetNumber, - pullNumber: ref.pullNumber, - repository: `${ref.owner}/${ref.repo}`, - toPatchsetNumber: toPatchset.patchsetNumber, - }, - "failed to load GitHub patchset comparison", - ); - - return { - ...snapshot, - capabilities: { - ...snapshot.capabilities, - limitations: appendUnique( - snapshot.capabilities.limitations, - "Exact GitHub commit comparison is unavailable; showing the indexed patchset snapshot.", - ), - }, - }; - } -} - -function buildComparedSnapshot( - snapshot: ReviewSnapshot, - fileChanges: CommitCompareFileChange[], -): ReviewSnapshot { - const files = fileChanges.map((file) => buildComparedReviewFile(snapshot.files, file)); - - return { - ...snapshot, - comparison: { - ...snapshot.comparison, - additions: files.reduce((total, file) => total + file.additions, 0), - deletions: files.reduce((total, file) => total + file.deletions, 0), - fileCount: files.length, - }, - files, - }; -} - -function buildComparedReviewFile( - indexedFiles: ReviewFile[], - file: CommitCompareFileChange, -): ReviewFile { - const indexedFile = - indexedFiles.find((candidate) => candidate.path === file.filename) ?? - indexedFiles.find((candidate) => candidate.path === file.previousFilename); - - return { - additions: file.additions, - deletions: file.deletions, - markState: indexedFile?.markState ?? "unreviewed", - markedAt: indexedFile?.markedAt, - markedPatchsetNumber: indexedFile?.markedPatchsetNumber, - patch: buildComparedFilePatch(file), - path: file.filename, - previousPath: file.previousFilename, - status: mapComparedFileStatus(file.status), - }; -} - -function buildComparedFilePatch(file: CommitCompareFileChange): string | undefined { - if (file.patch === undefined || file.patch.trim() === "") { - return undefined; - } - - if (file.patch.startsWith("diff --git")) { - return file.patch; - } - - const previousPath = file.previousFilename ?? file.filename; - const oldPath = file.status === "added" ? "/dev/null" : `a/${previousPath}`; - const newPath = file.status === "removed" ? "/dev/null" : `b/${file.filename}`; - - return [ - `diff --git a/${previousPath} b/${file.filename}`, - `--- ${oldPath}`, - `+++ ${newPath}`, - file.patch, - ].join("\n"); -} - -function mapComparedFileStatus(status: string): ReviewFileStatus { - switch (status) { - case "added": - return "added"; - case "removed": - return "deleted"; - case "renamed": - return "renamed"; - case "changed": - case "copied": - case "modified": - default: - return "modified"; - } -} - -function appendUnique(values: string[], value: string): string[] { - return values.includes(value) ? values : [...values, value]; -} - function parseReviewRoute(pathname: string): | { action?: diff --git a/test/review/public-snapshot.test.ts b/test/review/public-snapshot.test.ts index 86028e0..0e6d278 100644 --- a/test/review/public-snapshot.test.ts +++ b/test/review/public-snapshot.test.ts @@ -1,85 +1,8 @@ import { describe, expect, it } from "vitest"; -import { - getPublicThreadRootCommentId, - loadPublicReviewSnapshot, - type PublicPullRequestSnapshotOctokit, -} from "../../src/review/index.js"; - -describe("loadPublicReviewSnapshot", () => { - it("builds a current public PR review snapshot from GitHub REST data", async () => { - const octokit: PublicPullRequestSnapshotOctokit = { - request: async (route: string): Promise<{ data: TResponse }> => { - if (route === "GET /repos/{owner}/{repo}/pulls/{pull_number}") { - return { - data: { - base: { sha: "base-sha" }, - created_at: "2026-05-23T09:00:00.000Z", - head: { sha: "head-sha" }, - html_url: "https://github.com/acme/repo/pull/7", - merged_at: null, - state: "open", - title: "Improve review flow", - updated_at: "2026-05-23T10:00:00.000Z", - user: { login: "author" }, - } as TResponse, - }; - } - - if (route === "GET /repos/{owner}/{repo}/pulls/{pull_number}/files") { - return { - data: [ - { - additions: 2, - deletions: 1, - filename: "src/review.ts", - patch: - "@@ -1,2 +1,3 @@\n-export const durable = false;\n+export const durable = true;\n+export const publicPreview = true;", - status: "modified", - }, - ] as TResponse, - }; - } - - return { - data: [ - { - body: "Looks good.", - created_at: "2026-05-23T10:30:00.000Z", - diff_hunk: - "@@ -1,2 +1,3 @@\n-export const durable = false;\n+export const durable = true;\n+export const publicPreview = true;", - html_url: "https://github.com/acme/repo/pull/7#discussion_r1", - id: 1, - line: 1, - path: "src/review.ts", - side: "RIGHT", - user: { avatar_url: "https://example.com/a.png", login: "reviewer" }, - }, - ] as TResponse, - }; - }, - }; - - const snapshot = await loadPublicReviewSnapshot( - octokit, - { owner: "acme", pullNumber: 7, repo: "repo" }, - "alice", - ); - - expect(snapshot).toEqual( - expect.objectContaining({ - capabilities: expect.objectContaining({ mode: "public" }), - comparison: expect.objectContaining({ additions: 2, deletions: 1, fileCount: 1 }), - patchsets: [expect.objectContaining({ headSha: "head-sha", reconstructed: true })], - pullRequest: expect.objectContaining({ title: "Improve review flow" }), - viewer: { login: "alice" }, - }), - ); - expect(snapshot?.files[0]?.patch).toContain("diff --git"); - expect(snapshot?.threads[0]?.id).toBe("github-comment-1"); - expect(snapshot?.threads[0]?.comments[0]?.body).toBe("Looks good."); - }); +import { getPublicThreadRootCommentId } from "../../src/review/index.js"; +describe("getPublicThreadRootCommentId", () => { it("extracts GitHub comment ids from public thread ids", () => { expect(getPublicThreadRootCommentId("github-comment-123")).toBe(123); expect(getPublicThreadRootCommentId("thread-123")).toBeUndefined(); diff --git a/test/review/snapshot-loader.test.ts b/test/review/snapshot-loader.test.ts index 3a37c5b..e8f767c 100644 --- a/test/review/snapshot-loader.test.ts +++ b/test/review/snapshot-loader.test.ts @@ -1,86 +1,383 @@ -import { describe, expect, it, vi } from "vitest"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import type { CommitCompareOctokit } from "../../src/github/commit-compare.js"; import { loadReviewSnapshot, type LoadReviewSnapshotInput, type PublicPullRequestSnapshotOctokit, type ReviewSnapshot, - type ReviewSnapshotStore, } from "../../src/review/index.js"; +afterEach(() => vi.restoreAllMocks()); + describe("loadReviewSnapshot", () => { - const ref = { owner: "public-org", pullNumber: 17, repo: "public-repo" }; - const publicOctokit: PublicPullRequestSnapshotOctokit = { - request: async () => { - throw new Error("unexpected public Octokit request"); - }, - }; + it("compares selected patchsets and keeps review marks across a rename", async () => { + const indexedSnapshot = createComparedSnapshot(); + indexedSnapshot.files = [ + { + additions: 10, + deletions: 5, + markState: "current", + markedAt: "2026-05-23T13:05:00.000Z", + markedPatchsetNumber: 2, + path: "src/index.ts", + status: "modified", + }, + ]; - it("loads the indexed database snapshot first for an anonymous public route", async () => { - const indexedSnapshot = createSnapshot("indexed"); - const reviewStore: ReviewSnapshotStore = { - loadSnapshot: vi.fn(async () => indexedSnapshot), - }; - const loadPublicSnapshot = vi.fn(async () => createSnapshot("public")); - - const snapshot = await loadReviewSnapshot({ - loadPublicSnapshot, - options: { fromPatchsetNumber: 1, toPatchsetNumber: 2 }, - publicOctokit, - ref, - reviewStore, - viewerLogin: undefined, - }); + const snapshot = await loadReviewSnapshot( + createInput(indexedSnapshot, { + getComparisonOctokit: async () => ({ + rest: { + repos: { + compareCommitsWithBasehead: async (parameters) => { + if ( + parameters.basehead !== "head-sha...next-sha" || + parameters.owner !== "public-org" || + parameters.repo !== "public-repo" + ) { + throw new Error("wrong selected comparison"); + } + return { + data: { + files: [ + { + additions: 2, + deletions: 1, + filename: "src/main.ts", + previous_filename: "src/index.ts", + status: "renamed", + patch: "@@ -1 +1,2 @@\n-old\n+new\n+extra", + }, + ], + }, + }; + }, + }, + }, + }), + }), + ); - expect(snapshot).toBe(indexedSnapshot); - expect(reviewStore.loadSnapshot).toHaveBeenCalledWith(ref, undefined, { + expect(snapshot?.comparison).toEqual({ + additions: 2, + deletions: 1, + fileCount: 1, fromPatchsetNumber: 1, toPatchsetNumber: 2, }); - expect(loadPublicSnapshot).not.toHaveBeenCalled(); + expect(snapshot?.files).toEqual([ + { + additions: 2, + deletions: 1, + markState: "current", + markedAt: "2026-05-23T13:05:00.000Z", + markedPatchsetNumber: 2, + patch: + "diff --git a/src/index.ts b/src/main.ts\n--- a/src/index.ts\n+++ b/src/main.ts\n@@ -1 +1,2 @@\n-old\n+new\n+extra", + path: "src/main.ts", + previousPath: "src/index.ts", + status: "renamed", + }, + ]); + expect(indexedSnapshot.files[0]?.path).toBe("src/index.ts"); }); - it("falls back to the public GitHub snapshot when the database has no indexed PR", async () => { - const publicSnapshot = createSnapshot("public", "alice"); - const reviewStore: ReviewSnapshotStore = { - loadSnapshot: vi.fn(async () => undefined), + it("normalizes added, deleted and binary files while retaining marks by current path", async () => { + const indexedSnapshot = createComparedSnapshot(); + indexedSnapshot.files[0] = { + additions: 1, + deletions: 0, + markState: "stale", + markedAt: "2026-05-23T12:30:00.000Z", + markedPatchsetNumber: 1, + path: "src/index.ts", + status: "modified", }; - const loadPublicSnapshot = vi.fn(async () => publicSnapshot); + const snapshot = await loadReviewSnapshot( + createInput(indexedSnapshot, { + getComparisonOctokit: async () => + createComparisonOctokit([ + { filename: "new.ts", additions: 1, status: "added", patch: "@@ -0,0 +1 @@\n+new" }, + { filename: "old.ts", deletions: 1, status: "removed", patch: "@@ -1 +0,0 @@\n-old" }, + { filename: "image.png", status: "modified", patch: " " }, + { + filename: "src/index.ts", + status: "modified", + patch: "diff --git a/src/index.ts b/src/index.ts\nexisting patch", + }, + ]), + }), + ); + expect(snapshot?.files).toMatchObject([ + { path: "image.png", status: "modified", patch: undefined, markState: "unreviewed" }, + { + path: "new.ts", + additions: 1, + deletions: 0, + status: "added", + patch: "diff --git a/new.ts b/new.ts\n--- /dev/null\n+++ b/new.ts\n@@ -0,0 +1 @@\n+new", + }, + { + path: "old.ts", + additions: 0, + deletions: 1, + status: "deleted", + patch: "diff --git a/old.ts b/old.ts\n--- a/old.ts\n+++ /dev/null\n@@ -1 +0,0 @@\n-old", + }, + { + path: "src/index.ts", + markState: "stale", + markedAt: "2026-05-23T12:30:00.000Z", + markedPatchsetNumber: 1, + patch: "diff --git a/src/index.ts b/src/index.ts\nexisting patch", + }, + ]); + }); + + it("shows an empty comparison when the selected heads have no content changes", async () => { + const snapshot = await loadReviewSnapshot( + createInput(createComparedSnapshot(), { + getComparisonOctokit: async () => createComparisonOctokit([]), + }), + ); + + expect(snapshot?.files).toEqual([]); + expect(snapshot?.comparison).toEqual({ + additions: 0, + deletions: 0, + fileCount: 0, + fromPatchsetNumber: 1, + toPatchsetNumber: 2, + }); + expect(snapshot?.capabilities.limitations).toEqual([]); + }); + + it.each(["client lookup", "commit comparison"])( + "keeps indexed data and an explicit limitation when %s fails", + async (failure) => { + vi.spyOn(console, "warn").mockImplementation(() => {}); + const indexedSnapshot = createComparedSnapshot(); + indexedSnapshot.capabilities.limitations = ["Existing limitation"]; + const snapshot = await loadReviewSnapshot( + createInput(indexedSnapshot, { + getComparisonOctokit: async () => { + if (failure === "client lookup") throw new Error("installation unavailable"); + return { + rest: { + repos: { + compareCommitsWithBasehead: async () => { + throw new Error("comparison unavailable"); + }, + }, + }, + }; + }, + }), + ); + + expect(snapshot).toEqual({ + ...indexedSnapshot, + capabilities: { + mode: "indexed", + limitations: [ + "Existing limitation", + "Exact GitHub commit comparison is unavailable; showing the indexed patchset snapshot.", + ], + }, + }); + expect(indexedSnapshot.capabilities.limitations).toEqual(["Existing limitation"]); + }, + ); + + it.each([ + { fromPatchsetNumber: 1, toPatchsetNumber: 1 }, + { fromPatchsetNumber: 2, toPatchsetNumber: 1 }, + { fromPatchsetNumber: 1, toPatchsetNumber: 3 }, + ])("keeps the index when the selected patchsets cannot be compared: %j", async (selection) => { + const indexedSnapshot = createComparedSnapshot(); + indexedSnapshot.comparison = { ...indexedSnapshot.comparison, ...selection }; + const snapshot = await loadReviewSnapshot(createInput(indexedSnapshot)); + expect(snapshot).toEqual(indexedSnapshot); + }); - const snapshot = await loadReviewSnapshot({ - loadPublicSnapshot, - publicOctokit, - ref, - reviewStore, + it("loads the viewer's selected indexed snapshot before trying public GitHub", async () => { + const indexedSnapshot = createSnapshot("indexed", "alice"); + const input = createInput(undefined, { + options: { fromPatchsetNumber: 1, toPatchsetNumber: 1 }, + reviewStore: { + loadSnapshot: async (ref, viewer, options) => { + if ( + ref.owner === "public-org" && + ref.repo === "public-repo" && + ref.pullNumber === 17 && + viewer === "alice" && + options?.fromPatchsetNumber === 1 && + options.toPatchsetNumber === 1 + ) { + return indexedSnapshot; + } + throw new Error("unexpected viewer or patchset selection"); + }, + }, viewerLogin: "alice", }); - expect(snapshot).toBe(publicSnapshot); - expect(reviewStore.loadSnapshot).toHaveBeenCalledWith(ref, "alice", undefined); - expect(loadPublicSnapshot).toHaveBeenCalledWith(publicOctokit, ref, "alice"); + expect(await loadReviewSnapshot(input)).toEqual(indexedSnapshot); }); - it("propagates database failures instead of silently treating the DB as absent", async () => { - const reviewStore: ReviewSnapshotStore = { - loadSnapshot: vi.fn(async () => { - throw new Error("database unavailable"); + it("builds the public snapshot when the database has no indexed PR", async () => { + const snapshot = await loadReviewSnapshot( + createInput(undefined, { + publicOctokit: createPublicOctokit(), + viewerLogin: "alice", }), - }; - const loadPublicSnapshot = vi.fn(async () => createSnapshot("public")); + ); - await expect( - loadReviewSnapshot({ - loadPublicSnapshot, - publicOctokit, - ref, - reviewStore, + expect(snapshot).toMatchObject({ + capabilities: { mode: "public" }, + comparison: { additions: 2, deletions: 1, fileCount: 1 }, + patchsets: [{ headSha: "public-head-sha", reconstructed: true }], + pullRequest: { owner: "public-org", repo: "public-repo", number: 17, title: "Public PR" }, + viewer: { login: "alice" }, + }); + expect(snapshot?.capabilities.limitations).toContain( + "Review marks and attention changes require this pull request to be indexed by Clearance.", + ); + expect(snapshot?.files[0]?.patch).toBe( + "diff --git a/src/index.ts b/src/index.ts\n--- a/src/index.ts\n+++ b/src/index.ts\n@@ -1 +1,2 @@\n-old\n+new\n+extra", + ); + expect(snapshot?.threads[0]).toMatchObject({ + id: "github-comment-1", + comments: [{ body: "Looks good." }], + }); + }); + + it("returns absent only when both the index and GitHub cannot find the PR", async () => { + const snapshot = await loadReviewSnapshot( + createInput(undefined, { + publicOctokit: { + request: async () => { + throw { status: 404 }; + }, + }, }), + ); + expect(snapshot).toBeUndefined(); + }); + + it("propagates database failures instead of hiding them behind a public snapshot", async () => { + await expect( + loadReviewSnapshot( + createInput(undefined, { + publicOctokit: createPublicOctokit(), + reviewStore: { + loadSnapshot: async () => { + throw new Error("database unavailable"); + }, + }, + }), + ), ).rejects.toThrow("database unavailable"); - expect(loadPublicSnapshot).not.toHaveBeenCalled(); + }); + + it("propagates public GitHub failures other than not found", async () => { + await expect( + loadReviewSnapshot( + createInput(undefined, { + publicOctokit: { + request: async () => { + throw new Error("GitHub unavailable"); + }, + }, + }), + ), + ).rejects.toThrow("GitHub unavailable"); }); }); -type PublicSnapshotLoader = NonNullable; +type ComparisonFiles = NonNullable< + Awaited< + ReturnType + >["data"]["files"] +>; + +function createComparisonOctokit(files: ComparisonFiles): CommitCompareOctokit { + return { rest: { repos: { compareCommitsWithBasehead: async () => ({ data: { files } }) } } }; +} + +function createInput( + indexedSnapshot: ReviewSnapshot | undefined, + overrides: Partial = {}, +): LoadReviewSnapshotInput { + return { + getComparisonOctokit: async () => { + throw new Error("unexpected commit comparison"); + }, + publicOctokit: { + request: async () => { + throw new Error("unexpected public request"); + }, + }, + ref: { owner: "public-org", pullNumber: 17, repo: "public-repo" }, + reviewStore: { loadSnapshot: async () => indexedSnapshot }, + ...overrides, + }; +} + +function createComparedSnapshot(): ReviewSnapshot { + const snapshot = createSnapshot("indexed"); + snapshot.patchsets.push({ + createdAt: "2026-05-23T13:00:00.000Z", + eventType: "synchronize", + forcePush: false, + headSha: "next-sha", + patchsetNumber: 2, + reconstructed: false, + }); + snapshot.comparison.toPatchsetNumber = 2; + return snapshot; +} + +function createPublicOctokit(): PublicPullRequestSnapshotOctokit { + const responses: Record = { + "GET /repos/{owner}/{repo}/pulls/{pull_number}": { + base: { sha: "base-sha" }, + created_at: "2026-05-23T09:00:00.000Z", + head: { sha: "public-head-sha" }, + html_url: "https://github.com/public-org/public-repo/pull/17", + state: "open", + title: "Public PR", + user: { login: "author" }, + }, + "GET /repos/{owner}/{repo}/pulls/{pull_number}/files": [ + { + additions: 2, + deletions: 1, + filename: "src/index.ts", + patch: "@@ -1 +1,2 @@\n-old\n+new\n+extra", + status: "modified", + }, + ], + "GET /repos/{owner}/{repo}/pulls/{pull_number}/comments": [ + { + body: "Looks good.", + created_at: "2026-05-23T10:30:00.000Z", + id: 1, + line: 1, + path: "src/index.ts", + side: "RIGHT", + user: { login: "reviewer" }, + }, + ], + }; + return { + request: async (route: string): Promise<{ data: TResponse }> => { + if (!(route in responses)) throw new Error(`unexpected public request: ${route}`); + return { data: responses[route] as TResponse }; + }, + }; +} function createSnapshot( mode: ReviewSnapshot["capabilities"]["mode"],