diff --git a/packages/review-protocol/src/review-api-client.ts b/packages/review-protocol/src/review-api-client.ts index 8e09da381..bddcf586f 100644 --- a/packages/review-protocol/src/review-api-client.ts +++ b/packages/review-protocol/src/review-api-client.ts @@ -22,7 +22,7 @@ export interface ReviewApiSummary { createdAt: string; /** Timestamp of the earliest retained review version, when known. */ firstCreatedAt?: string; - /** Present on reviews imported from the legacy MDX store. */ + /** The branch and PR the review is of; see SnapshotOrigin. */ origin?: { branch?: string; baseRef?: string; @@ -30,6 +30,7 @@ export interface ReviewApiSummary { pullRequestNumber?: number; pullRequestUrl?: string; pullRequestTitle?: string; + pullRequestBaseBranch?: string; revision?: string; }; /** The PR's state on GitHub, when the host has read it. */ diff --git a/packages/review/app/src/review-stack-selector.browser.test.tsx b/packages/review/app/src/review-stack-selector.browser.test.tsx index 2197ac541..48b36e850 100644 --- a/packages/review/app/src/review-stack-selector.browser.test.tsx +++ b/packages/review/app/src/review-stack-selector.browser.test.tsx @@ -265,60 +265,80 @@ describe("ReviewStackSelector", () => { expect(container.textContent).toBe(""); }); - it("lists the stack newest first down to its base, the board on screen opening nothing", async () => { - const post = vi.fn(async () => ({ ok: true })); - const session = testReviewSession({}, { post }); + it.each([ + { kind: "GitHub", pullRequests: true, label: "from GitHub" }, + { kind: "local", pullRequests: false, label: "from branch bases" }, + ])( + "lists a $kind stack newest first down to its base, the board on screen opening nothing", + async ({ pullRequests, label }) => { + const post = vi.fn(async () => ({ + ok: true, + })); - session.review!.headBranch = "b"; - session.review!.stack = async () => ({ - base: "main", - layers: (["a", "b", "c"] as const).map((branch, index) => ({ - branch, - relation: (["earlier", "current", "later"] as const)[index]!, - pullRequestNumber: 10 + index, - pullRequestUrl: `https://github.com/o/r/pull/${10 + index}`, - base: null, - subject: null, - reviewUuid: `${index + 1}`.repeat(8) + "-1111-4111-8111-111111111111", - reviewTitle: `Review ${branch}`, - pullRequestState: index === 0 ? "merged" : "open", - })), - }); + const session = testReviewSession({}, { post }); - const container = document.createElement("div"); - document.body.append(container); - root = createRoot(container); - await act(async () => { - root?.render( - - - - - , + session.review!.headBranch = "b"; + session.review!.stack = async () => ({ + base: "main", + layers: (["a", "b", "c"] as const).map((branch, index) => ({ + branch, + relation: (["earlier", "current", "later"] as const)[index]!, + pullRequestNumber: pullRequests ? 10 + index : null, + pullRequestUrl: pullRequests + ? `https://github.com/o/r/pull/${10 + index}` + : null, + base: null, + subject: null, + reviewUuid: `${index + 1}`.repeat(8) + "-1111-4111-8111-111111111111", + reviewTitle: `Review ${branch}`, + pullRequestState: pullRequests + ? index === 0 + ? "merged" + : "open" + : undefined, + })), + }); + + const container = document.createElement("div"); + document.body.append(container); + root = createRoot(container); + await act(async () => { + root?.render( + + + + + , + ); + }); + + await vi.waitFor(() => + expect( + container.querySelector("button[aria-haspopup]")?.ariaLabel, + ).toBe("Stack, 2 of 3"), + ); + await openMenu(container); + expect(container.querySelector("[role=menu]")?.textContent).toContain( + label, ); - }); - await vi.waitFor(() => - expect(container.querySelector("button[aria-haspopup]")?.ariaLabel).toBe( - "Stack, 2 of 3", - ), - ); - await openMenu(container); + const rows = [...container.querySelectorAll("[role=menuitem]")]; - const rows = [...container.querySelectorAll("[role=menuitem]")]; - - expect(document.activeElement).toBe(rows[1]); - expect(rows.map((row) => row.textContent)).toEqual([ - expect.stringContaining("Review c"), - expect.stringContaining("Review b"), - expect.stringMatching(/Review a.*#10 merged/), - ]); - expect( - container.querySelector("[role=separator]")?.nextElementSibling - ?.textContent, - ).toContain("main"); - - await act(async () => (rows[1] as HTMLElement).click()); - expect(post).not.toHaveBeenCalled(); - }); + expect(document.activeElement).toBe(rows[1]); + expect(rows.map((row) => row.textContent)).toEqual([ + expect.stringContaining("Review c"), + expect.stringContaining("Review b"), + expect.stringMatching( + pullRequests ? /Review a.*#10 merged/ : /Review a/, + ), + ]); + expect( + container.querySelector("[role=separator]")?.nextElementSibling + ?.textContent, + ).toContain("main"); + + await act(async () => (rows[1] as HTMLElement).click()); + expect(post).not.toHaveBeenCalled(); + }, + ); }); diff --git a/packages/review/app/src/review-stack-selector.tsx b/packages/review/app/src/review-stack-selector.tsx index ee07dd050..2a8d514cf 100644 --- a/packages/review/app/src/review-stack-selector.tsx +++ b/packages/review/app/src/review-stack-selector.tsx @@ -222,9 +222,9 @@ export function StackPill({
Stack - {layers.some((layer) => layer.base) - ? "from branch bases" - : "from GitHub"}{" "} + {layers.some((layer) => layer.pullRequestUrl) + ? "from GitHub" + : "from branch bases"}{" "} · newest first
diff --git a/packages/review/src/github-pr-graph.test.ts b/packages/review/src/github-pr-graph.test.ts new file mode 100644 index 000000000..0fa5bd6ee --- /dev/null +++ b/packages/review/src/github-pr-graph.test.ts @@ -0,0 +1,47 @@ +import { describe, expect, it, vi } from "vitest"; + +import { type RunGitHubApi, readGitHubPullRequests } from "./github-pr-graph"; + +const repo = { full_name: "o/r" }; + +const record = (number: number) => ({ + number, + title: `PR ${number}`, + head: { ref: `feature-${number}`, repo }, + base: { ref: "main", repo }, +}); + +describe("repository PR facts", () => { + it("reads beyond the first page and coalesces concurrent consumers", async () => { + const run = vi.fn(async (_host: string, endpoint: string) => + JSON.stringify( + endpoint.endsWith("page=1") + ? Array.from({ length: 100 }, (_, index) => record(index + 1)) + : [record(101)], + ), + ); + + const [first, second] = await Promise.all([ + readGitHubPullRequests("github.com", "o/r", run), + readGitHubPullRequests("github.com", "o/r", run), + ]); + + expect(first).toHaveLength(101); + expect(second).toEqual(first); + expect(run).toHaveBeenCalledTimes(2); + }); + + it("allows a new read after a transient failure", async () => { + const run = vi + .fn() + .mockRejectedValueOnce(new Error("offline")) + .mockResolvedValueOnce(JSON.stringify([record(1)])); + + await expect( + readGitHubPullRequests("github.com", "o/r", run), + ).rejects.toThrow("offline"); + await expect( + readGitHubPullRequests("github.com", "o/r", run), + ).resolves.toMatchObject([{ number: 1 }]); + }); +}); diff --git a/packages/review/src/github-pr-graph.ts b/packages/review/src/github-pr-graph.ts new file mode 100644 index 000000000..d2d739ebe --- /dev/null +++ b/packages/review/src/github-pr-graph.ts @@ -0,0 +1,97 @@ +import { z } from "zod"; + +export type RunGitHubApi = (host: string, endpoint: string) => Promise; + +const repository = z.object({ full_name: z.string() }); + +const pullRequest = z.object({ + number: z.number().int().positive(), + title: z.string(), + draft: z.boolean().optional(), + head: z.object({ ref: z.string(), repo: repository.nullable() }), + base: z.object({ ref: z.string(), repo: repository }), +}); + +export interface GitHubPullRequest { + number: number; + url: string; + title: string; + headBranch: string; + baseBranch: string; + draft: boolean; +} + +// Repository facts are independent of which PRs have whiteboards. Share a +// short-lived, coalesced read between source resolution and stack discovery. +const caches = new WeakMap< + RunGitHubApi, + Map< + string, + { + expires: number; + read: Promise; + } + > +>(); + +export function readGitHubPullRequests( + host: string, + slug: string, + run: RunGitHubApi, +): Promise { + let cache = caches.get(run); + + if (!cache) caches.set(run, (cache = new Map())); + const key = `${host}/${slug}`.toLowerCase(); + const previous = cache.get(key); + + if (previous && previous.expires > Date.now()) return previous.read; + + const read = (async () => { + const result: GitHubPullRequest[] = []; + + for (let page = 1; page <= 10; page++) { + const records = z + .array(pullRequest) + .parse( + JSON.parse( + await run( + host, + `repos/${slug}/pulls?state=open&per_page=100&page=${page}`, + ), + ), + ); + + for (const pr of records) { + // A fork's identically named branch is not this repository's branch. + if ( + pr.head.repo?.full_name.toLowerCase() !== slug.toLowerCase() || + pr.base.repo.full_name.toLowerCase() !== slug.toLowerCase() + ) + continue; + result.push({ + number: pr.number, + url: `https://${host}/${slug}/pull/${pr.number}`, + title: pr.title, + headBranch: pr.head.ref, + baseBranch: pr.base.ref, + draft: pr.draft ?? false, + }); + } + + if (records.length < 100) return result; + } + + throw new Error( + "PR discovery exceeded 1000 open pull requests; refusing a partial stack.", + ); + })(); + + cache.set(key, { expires: Date.now() + 60_000, read }); + // Failures are optional context, but must not poison the next read. + void read.catch(() => { + if (cache.get(key)?.read === read) cache.delete(key); + }); + + return read; +} diff --git a/packages/review/src/review-api/board-writes.test.ts b/packages/review/src/review-api/board-writes.test.ts index 17244c17e..64617e4df 100644 --- a/packages/review/src/review-api/board-writes.test.ts +++ b/packages/review/src/review-api/board-writes.test.ts @@ -40,6 +40,75 @@ const agent: Author = { kind: "agent" }; const person: Author = { kind: "person" }; +it("drains accepted commands when closing and refuses new commands", async () => { + const database = path.join(directory, "closing.db"); + + const closing = new ReviewStore(database, { + validatePins: async () => {}, + validateSource: async () => {}, + validateResource: async () => {}, + }); + + const repositoryId = closing.registerRepository(directory).id; + + const { reviewId } = await closing.execute({ + operation: { + type: "create", + title: "Before", + target: { kind: "project", repositoryId }, + }, + }); + + const accepted = closing.execute({ + operation: { type: "rename", reviewId, title: "Saved before shutdown" }, + }); + + const closed = closing.close(); + + const refused = closing.execute({ + operation: { type: "rename", reviewId, title: "Too late" }, + }); + + const refusedEdit = closing.personEdit(reviewId, { + baseVersion: 0, + title: "Too late", + document: [], + }); + + const refusedDiscard = closing.discard(reviewId); + + const results = await Promise.allSettled([ + accepted, + closed, + refused, + refusedEdit, + refusedDiscard, + ]); + + expect(results[0].status).toBe("fulfilled"); + expect(results[1].status).toBe("fulfilled"); + + for (const result of results.slice(2)) + expect(result).toMatchObject({ + status: "rejected", + reason: new Error("Review store is closing."), + }); + + const saved = new DatabaseSync(database, { readOnly: true }); + + try { + const row = saved + .prepare( + "SELECT json_extract(snapshot,'$.title') AS title FROM versions WHERE review_id=? ORDER BY version DESC LIMIT 1", + ) + .get(reviewId)!; + + expect(row.title).toBe("Saved before shutdown"); + } finally { + saved.close(); + } +}); + const checkout = (name: string) => { const root = path.join(directory, name); @@ -57,6 +126,65 @@ const clone = (name: string, origin: string) => { return store.registerRepository(root).id; }; +it("reuses a branch board from another clone of the same origin", async () => { + const first = clone("first", "git@github.com:acme/widget.git"); + const second = clone("second", "https://github.com/acme/widget"); + + const reusing = new ReviewStore(path.join(directory, "reuse.db"), { + validatePins: async () => {}, + validateSource: async () => {}, + validateResource: async () => {}, + headBranch: async () => "widgets", + resolvePullRequest: async (_url, repository) => ({ + target: { + kind: "commits", + repositoryId: repository.id, + base: "a".repeat(40), + head: "b".repeat(40), + }, + pins: { + repositoryId: repository.id, + base: "a".repeat(40), + head: "b".repeat(40), + }, + title: "Add widgets", + branch: "widgets", + baseBranch: "main", + }), + }); + + try { + const original = reusing.registerRepository(store.repositoryPath(first)).id; + const other = reusing.registerRepository(store.repositoryPath(second)).id; + + const board = await reusing.execute({ + operation: { + type: "create", + title: "Before PR", + target: { + kind: "commits", + repositoryId: original, + base: "a".repeat(40), + head: "b".repeat(40), + }, + }, + }); + + const opened = await reusing.execute({ + operation: { + type: "create", + repositoryId: other, + pullRequestUrl: "https://github.com/acme/widget/pull/7", + }, + }); + + expect(opened).toMatchObject({ created: false, reviewId: board.reviewId }); + expect(reusing.read(board.reviewId).pins?.repositoryId).toBe(original); + } finally { + await reusing.close(); + } +}); + const create = async (repositoryId: string, title = "Plan") => ( await store.execute({ diff --git a/packages/review/src/review-api/http.ts b/packages/review/src/review-api/http.ts index ec4a9a799..54cd34143 100644 --- a/packages/review/src/review-api/http.ts +++ b/packages/review/src/review-api/http.ts @@ -1996,10 +1996,12 @@ export function createReviewApi( app.get("/:id/stack", async (context) => { const query = readQuerySchemas.get.parse(context.req.query()); const id = context.req.param("id"); - const snapshot = readReview(id, query.version); + let snapshot = readReview(id, query.version); if (isShared(id) || !snapshot.pins) return context.json({ layers: [] }); + if (data) snapshot = await data.discoverPullRequest(snapshot); + const repositoryPath = store .repositories() .find( @@ -2163,7 +2165,15 @@ export function createReviewApi( app.get("/:id", async (context) => { const query = readQuerySchemas.get.parse(context.req.query()); - const snapshot = { ...readReview(context.req.param("id"), query.version) }; + let snapshot = { ...readReview(context.req.param("id"), query.version) }; + + if ( + data && + query.full && + query.version === undefined && + !isShared(snapshot.reviewId) + ) + snapshot = await data.discoverPullRequest(snapshot); if (data && query.full) { try { @@ -2223,17 +2233,7 @@ export function createReviewApi( const request = withBoardRepository( await locateRepositories( - await pullRequestTargets( - body, - register, - (url, repositoryId) => { - if (!data) - throw new ReviewInputError("Repositories are unavailable."); - - return data.resolvePullRequest(url, { id: repositoryId }); - }, - (id) => store.read(id), - ), + await pullRequestTargets(body, register, (id) => store.read(id)), register, ), (id) => (store.has(id) ? store.read(id) : undefined), @@ -2433,10 +2433,6 @@ async function pullRequestTargets( // oxlint-disable-next-line anti-slop/no-unknown-parameters -- Request body boundary: commandSchema parses the result. body: unknown, register: (path: string) => Promise<{ id: string }>, - resolve: ( - url: string, - repositoryId: string, - ) => Promise<{ target: ReviewTarget }>, board: (id: string) => void, ) { const target = z.strictObject({ @@ -2480,7 +2476,7 @@ async function pullRequestTargets( ...located.data, operation: { ...operation, - target: (await resolve(url, repositoryId)).target, + repositoryId, pullRequestUrl: url, }, }; diff --git a/packages/review/src/review-api/local-data.ts b/packages/review/src/review-api/local-data.ts index c4a433f68..2e68080d0 100644 --- a/packages/review/src/review-api/local-data.ts +++ b/packages/review/src/review-api/local-data.ts @@ -20,6 +20,7 @@ import { gitCommonDir, listCommitRange, listTrackedFilesAtCommit, + parseGitRemote, readFileAtCommit, resolveRepoContext, runVcs, @@ -39,6 +40,10 @@ import type { } from "@parcel/watcher"; import { textIncludesQuote } from "@review/evidence.js"; import { isMissingFileError } from "@review/fs-utils.js"; +import { + type GitHubPullRequest, + readGitHubPullRequests, +} from "@review/github-pr-graph.js"; import { reviewManagedCheckoutRoot } from "@review/review-checkout-paths.js"; import { ensureReviewPinnedCheckout } from "@review/review-head-checkout.js"; import { StructuralComparisons } from "@review/server/structural-comparisons.js"; @@ -79,6 +84,7 @@ import { } from "./document.js"; import { decodeImage } from "./image-decode.js"; import { mapInputSchema } from "./map-input.js"; +import { pullRequestKey, sourceBranchName } from "./origin.js"; import { type PullRequestAddress, type PullRequestProvider, @@ -1052,22 +1058,101 @@ export class LocalReviewData { return snapshot.pins; } + /** Optional forge context; never changes the comparison's pins. */ + private readonly readGitHubApi = (host: string, endpoint: string) => + (this.options.pullRequests ?? defaultPullRequestDeps).run( + "gh", + ["api", "--hostname", { value: host }, { value: endpoint }], + { timeoutMs: 10_000 }, + ); + + async findPullRequest( + pins: Pins, + branch: string, + ): Promise { + const context = await resolveRepoContext( + this.store.repositoryPath(pins.repositoryId), + ); + + if (!context?.githubSlug || !context.originUrl) return undefined; + const remote = parseGitRemote(context.originUrl); + + if (!remote) return undefined; + const name = sourceBranchName(branch); + + const matches = ( + await readGitHubPullRequests( + remote.host, + context.githubSlug, + this.readGitHubApi, + ) + ).filter((pr) => pr.headBranch === name); + + return matches.length === 1 ? matches[0] : undefined; + } + + /** Enrich current display metadata without repinning or rewriting history. */ + async discoverPullRequest(snapshot: Snapshot): Promise { + const { origin, pins } = snapshot; + + if (!pins || !origin?.branch) return snapshot; + + const found = await this.findPullRequest(pins, origin.branch).catch( + () => undefined, + ); + + if (!found) return snapshot; + + // A manually selected PR must not be replaced by a branch-name match. + if ( + origin.pullRequestUrl && + pullRequestKey(origin.pullRequestUrl) !== pullRequestKey(found.url) + ) + return snapshot; + + return { + ...snapshot, + origin: { + ...origin, + pullRequestUrl: found.url, + pullRequestNumber: found.number, + pullRequestTitle: found.title, + pullRequestBaseBranch: found.baseBranch, + }, + }; + } + /** Capture a label only when its ref still resolves to these exact pins. */ async headBranch(pins: Pins, headRef?: string): Promise { + const context = await resolveRepoContext( + this.store.repositoryPath(pins.repositoryId), + ); + + if (!context) return undefined; + const vcs = await this.vcs(pins.repositoryId); - if (!vcs || vcs.kind !== "git") return undefined; + const symbolicRef = + headRef && + headRef !== "HEAD" && + headRef !== "@" && + !/^(?:[0-9a-f]{40}|[0-9a-f]{64})$/i.test(headRef) + ? headRef + : undefined; + + let location: VcsArg[]; + + if (symbolicRef) location = ["--git-dir", context.commonDir]; + else if (vcs?.kind === "git") location = ["-C", vcs.rootPath]; + else return undefined; try { const { stdout } = await runVcs("git", [ - "-C", - vcs.rootPath, + ...location, "rev-parse", "--symbolic-full-name", "--verify", - !headRef || /^(?:[0-9a-f]{40}|[0-9a-f]{64})$/i.test(headRef) - ? "HEAD" - : { rev: headRef }, + symbolicRef ? { rev: symbolicRef } : "HEAD", ]); const ref = stdout.trim(); @@ -1075,13 +1160,18 @@ export class LocalReviewData { if (!ref.startsWith("refs/heads/") && !ref.startsWith("refs/remotes/")) return undefined; - const resolved = await vcs.resolveRevision(ref); + const { stdout: commit } = await runVcs("git", [ + ...location, + "rev-parse", + { rev: ref }, + ]); - if (resolved?.commit !== pins.head) return undefined; + if (commit.trim() !== pins.head) return undefined; - return ref.replace(/^refs\/(heads|remotes)\//, ""); + return ref + .replace(/^refs\/remotes\/origin\//, "") + .replace(/^refs\/(heads|remotes)\//, ""); } catch { - // Optional provenance must not block authoring in a detached checkout. return undefined; } } @@ -2215,6 +2305,7 @@ export function openLocalReviewStore( resolvePullRequest: (url, repository) => data.resolvePullRequest(url, repository), pullRequestTitle: (url) => data.pullRequestTitle(url), + findPullRequest: (pins, branch) => data.findPullRequest(pins, branch), headBranch: (pins, headRef) => data.headBranch(pins, headRef), sourcePins: (snapshot) => data.sourcePins(snapshot), unavailableAnchors: (snapshot) => data.unavailableAnchors(snapshot), diff --git a/packages/review/src/review-api/origin.ts b/packages/review/src/review-api/origin.ts index 14f013ad5..77cb1d685 100644 --- a/packages/review/src/review-api/origin.ts +++ b/packages/review/src/review-api/origin.ts @@ -16,11 +16,18 @@ export function pullRequestKey(url: string): string { return url.toLowerCase(); } -/** Omission preserves identity; null detaches it without changing import metadata. */ +/** Older snapshots kept origin/ on remote branch labels. */ +export const sourceBranchName = (branch: string | undefined) => + branch?.replace(/^origin\//, ""); + +/** Omission preserves identity; null removes the PR without changing import + * metadata. A new url keeps the title and PR base when the caller omits them + * for the same PR. */ export function setPullRequest( snapshot: Pick, url: string | null | undefined, title?: string, + baseBranch?: string, ) { if (url === undefined) return; @@ -29,19 +36,20 @@ export function setPullRequest( delete snapshot.origin.pullRequestUrl; delete snapshot.origin.pullRequestNumber; delete snapshot.origin.pullRequestTitle; + delete snapshot.origin.pullRequestBaseBranch; } return; } + const same = snapshot.origin?.pullRequestUrl === url; snapshot.origin = { ...snapshot.origin, pullRequestUrl: url, pullRequestNumber: Number(url.split("/").at(-1)), pullRequestTitle: - title ?? - (snapshot.origin?.pullRequestUrl === url - ? snapshot.origin.pullRequestTitle - : undefined), + title ?? (same ? snapshot.origin?.pullRequestTitle : undefined), + pullRequestBaseBranch: + baseBranch ?? (same ? snapshot.origin?.pullRequestBaseBranch : undefined), }; } diff --git a/packages/review/src/review-api/pull-request.test.ts b/packages/review/src/review-api/pull-request.test.ts index 92b5ad691..baf1f587d 100644 --- a/packages/review/src/review-api/pull-request.test.ts +++ b/packages/review/src/review-api/pull-request.test.ts @@ -201,6 +201,15 @@ describe("creating a review from a pull request URL alone", () => { }; let ghCalls: number; + let openPullRequests: object[]; + + // The open PR #7 GitHub lists for the widgets branch. + const widgetsPullRequest = (title: string, baseBranch = "main") => ({ + number: 7, + title, + head: { ref: "widgets", repo: { full_name: "acme/widget" } }, + base: { ref: baseBranch, repo: { full_name: "acme/widget" } }, + }); const gitIn = (cwd: string) => @@ -261,6 +270,7 @@ describe("creating a review from a pull request URL alone", () => { isCrossRepository: true, }; ghCalls = 0; + openPullRequests = []; local = openLocalReviewStore(path.join(directory, "reviews.db"), { manageWorkspaces: false, pullRequests: { @@ -278,6 +288,8 @@ describe("creating a review from a pull request URL alone", () => { return defaultPullRequestDeps.run(file, args, options); ghCalls++; + if (args[0] === "api") return JSON.stringify(openPullRequests); + return JSON.stringify(pr); }, fetch: () => Promise.reject(new Error("no network in tests")), @@ -351,6 +363,329 @@ describe("creating a review from a pull request URL alone", () => { expect(userRefs(path.join(checkout, ".git"))).toBe(before); }); + it("keeps the same source identity when creating and retargeting by PR", async () => { + pr.isCrossRepository = false; + const created = await createFromUrl(); + const original = local.store.read(created.reviewId); + const api = createReviewApi(local.store, local.data); + + const response = await api.request("/commands", { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify( + command({ + type: "set_target", + reviewId: created.reviewId, + target: { + kind: "pullRequest", + repositoryPath: checkout, + pullRequest: 7, + }, + }), + ), + }); + + expect(response.status).toBe(200); + const retargeted = local.store.read(created.reviewId); + expect(retargeted.pins).toEqual(original.pins); + expect(retargeted.origin).toEqual(original.origin); + expect(retargeted.origin).toMatchObject({ + branch: "contributor", + baseBranch: "main", + pullRequestUrl: url, + }); + expect(local.store.read(created.reviewId, original.version).origin).toEqual( + original.origin, + ); + }); + + it("discovers a branch's PR without replacing its explicitly chosen comparison", async () => { + const git = gitIn(checkout); + git("fetch", "origin", "refs/pull/7/head:refs/heads/widgets"); + openPullRequests = [widgetsPullRequest("Add widgets", "lower-pr")]; + const registered = await local.data.register(checkout); + + const created = await local.store.execute( + command({ + type: "create", + title: "Explicit comparison", + target: { + kind: "commits", + repositoryId: registered.id, + head: "widgets", + base: "main", + }, + }), + ); + + const snapshot = local.store.read(created.reviewId); + expect(snapshot.pins).toMatchObject({ head: upstreamHead(), base: trunk }); + expect(snapshot.origin).toMatchObject({ + pullRequestUrl: url, + pullRequestNumber: 7, + pullRequestBaseBranch: "lower-pr", + branch: "widgets", + baseBranch: "main", + }); + + const lookup = vi + .spyOn(local.data, "findPullRequest") + .mockRejectedValue(new Error("offline")); + + try { + await local.store.execute( + command({ + type: "set_target", + reviewId: created.reviewId, + target: { + kind: "commits", + repositoryId: registered.id, + head: "widgets", + base: "main", + }, + }), + ); + expect(local.store.read(created.reviewId).origin).toMatchObject({ + pullRequestUrl: url, + pullRequestNumber: 7, + pullRequestBaseBranch: "lower-pr", + }); + } finally { + lookup.mockRestore(); + } + }); + + it("finds a PR published after a branch review without rewriting its saved pins or history", async () => { + const git = gitIn(checkout); + git("fetch", "origin", "refs/pull/7/head:refs/heads/widgets"); + const registered = await local.data.register(checkout); + + const created = await local.store.execute( + command({ + type: "create", + title: "Before publishing", + target: { + kind: "commits", + repositoryId: registered.id, + head: "widgets", + base: "main", + }, + }), + ); + + const saved = local.store.read(created.reviewId); + expect(saved.origin?.pullRequestUrl).toBeUndefined(); + openPullRequests = [widgetsPullRequest("Published later")]; + const clock = vi.spyOn(Date, "now").mockReturnValue(Date.now() + 61_000); + + try { + const projected = await local.data.discoverPullRequest(saved); + expect(projected.origin?.pullRequestUrl).toBe(url); + expect(projected.origin?.pullRequestNumber).toBe(7); + expect(projected.pins).toEqual(saved.pins); + expect(local.store.read(created.reviewId)).toEqual(saved); + } finally { + clock.mockRestore(); + } + }); + + it("uses a linked worktree's HEAD", async () => { + const git = gitIn(checkout); + git("fetch", "origin", "refs/pull/7/head:refs/heads/widgets"); + const linked = path.join(directory, "linked"); + git("worktree", "add", "--quiet", linked, "widgets"); + openPullRequests = [widgetsPullRequest("Widgets")]; + const registered = await local.data.register(linked); + + const target = { + kind: "worktree", + repositoryId: registered.id, + base: "main", + }; + + const created = await local.store.execute( + command({ type: "create", title: "Linked", target }), + ); + + expect(local.store.read(created.reviewId).origin).toMatchObject({ + branch: "widgets", + pullRequestUrl: url, + }); + + const explicitHead = await local.store.execute( + command({ + type: "create", + title: "Explicit HEAD", + target: { + kind: "commits", + repositoryId: registered.id, + head: "HEAD", + base: "main", + }, + }), + ); + + expect(local.store.read(explicitHead.reviewId).origin?.branch).toBe( + "widgets", + ); + }); + + it("clears the former branch's PR identity when moving to a different branch", async () => { + const git = gitIn(checkout); + git("fetch", "origin", "refs/pull/7/head:refs/heads/widgets"); + openPullRequests = [widgetsPullRequest("Widgets")]; + const registered = await local.data.register(checkout); + + const created = await local.store.execute( + command({ + type: "create", + title: "Widgets", + target: { + kind: "commits", + repositoryId: registered.id, + head: "widgets", + base: "main", + }, + }), + ); + + expect(local.store.read(created.reviewId).origin?.pullRequestUrl).toBe(url); + await local.store.execute( + command({ + type: "set_target", + reviewId: created.reviewId, + target: { + kind: "commits", + repositoryId: registered.id, + head: "main", + base: fork, + }, + }), + ); + const moved = local.store.read(created.reviewId); + expect(moved.origin?.branch).toBe("main"); + expect(moved.origin?.pullRequestUrl).toBeUndefined(); + }); + + it("reuses a branch board when its PR is published later", async () => { + const git = gitIn(checkout); + git("fetch", "origin", "refs/pull/7/head:refs/remotes/origin/widgets"); + const registered = await local.data.register(checkout); + + const created = await local.store.execute( + command({ + type: "create", + title: "Before PR", + target: { + kind: "commits", + repositoryId: registered.id, + head: "origin/widgets", + base: "main", + }, + }), + ); + + pr.isCrossRepository = false; + pr.headRefName = "widgets"; + expect(await createFromUrl()).toMatchObject({ + created: false, + reviewId: created.reviewId, + }); + expect(local.store.list()).toHaveLength(1); + }); + + it("does not reuse a different repository’s branch board filed in the same project", async () => { + const other = path.join(directory, "other"); + gitIn(directory)("clone", "-q", upstream, other); + pointAtUpstream( + path.join(other, ".git"), + "https://github.com/other/widget.git", + ); + gitIn(other)("fetch", "origin", "refs/pull/7/head:refs/heads/widgets"); + const registered = await local.data.register(checkout); + const candidate = await local.data.register(other); + + const created = await local.store.execute( + command({ + type: "create", + title: "Before PR", + projectId: local.store.projectOfRepository(registered.id), + target: { + kind: "commits", + repositoryId: candidate.id, + head: "widgets", + base: "main", + }, + }), + ); + + pr.isCrossRepository = false; + pr.headRefName = "widgets"; + const opened = await createFromUrl(); + expect(opened.created).toBe(true); + expect(opened.reviewId).not.toBe(created.reviewId); + expect(local.store.read(opened.reviewId).pins?.repositoryId).toBe( + registered.id, + ); + }); + + it("does not hold up unrelated saves while optional PR discovery waits", async () => { + const existing = await createFromUrl(); + const git = gitIn(checkout); + git("fetch", "origin", "refs/pull/7/head:refs/heads/widgets"); + const registered = await local.data.register(checkout); + let release!: () => void; + let started!: () => void; + + const waiting = new Promise((resolve) => { + release = resolve; + }); + + const entered = new Promise((resolve) => { + started = resolve; + }); + + const lookup = vi + .spyOn(local.data, "findPullRequest") + .mockImplementation(async () => { + started(); + await waiting; + + return undefined; + }); + + const pending = local.store.execute( + command({ + type: "create", + title: "Slow lookup", + target: { + kind: "commits", + repositoryId: registered.id, + head: "widgets", + base: "main", + }, + }), + ); + + try { + await entered; + await local.store.execute( + command({ + type: "rename", + reviewId: existing.reviewId, + title: "Save went through", + }), + ); + expect(local.store.read(existing.reviewId).title).toBe( + "Save went through", + ); + } finally { + release(); + await pending; + lookup.mockRestore(); + } + }); + it("returns the existing review for a repeat, comparing against the PR's current head", async () => { const created = await createFromUrl(); diff --git a/packages/review/src/review-api/store.ts b/packages/review/src/review-api/store.ts index 25060fbcd..605f18f53 100644 --- a/packages/review/src/review-api/store.ts +++ b/packages/review/src/review-api/store.ts @@ -12,6 +12,7 @@ import { boardIconKind, boardTitle, } from "@dev.fast/review-protocol"; +import type { GitHubPullRequest } from "@review/github-pr-graph.js"; import { sourceAnchors } from "@review/lens-selection.js"; import { migrateStoredLenses, @@ -76,7 +77,12 @@ import { summarizeEdit, } from "./document.js"; import { type BoardLinkPreview, boardLinkPreview } from "./link-preview.js"; -import { pullRequestKey, pullRequestUrl, setPullRequest } from "./origin.js"; +import { + pullRequestKey, + pullRequestUrl, + setPullRequest, + sourceBranchName, +} from "./origin.js"; import { upgradeStore } from "./store-migrations.js"; import { BOARD_REPOSITORY_ID_SQL, @@ -168,7 +174,8 @@ export const commandSchema = z.strictObject({ z.strictObject({ type: z.literal("set_target"), reviewId, - target: reviewTargetSchema, + target: reviewTargetSchema.optional(), + repositoryId: z.string().min(1).optional(), pullRequestUrl: pullRequestUrl.nullable().optional(), }), z.strictObject({ @@ -250,6 +257,8 @@ export interface SnapshotOrigin { pullRequestUrl?: string; /** As the forge reported it when the PR was attached. */ pullRequestTitle?: string; + /** The branch the PR merges into, which can differ from baseBranch. */ + pullRequestBaseBranch?: string; /** The legacy review revision this version was imported from. */ revision?: string; } @@ -424,7 +433,22 @@ export interface ResolvedPullRequest { baseBranch: string; } +/** A command with its network context resolved, ready for the write queue. */ +interface PreparedCommand { + op: z.infer["operation"]; + requestedTarget: ReviewTarget | undefined; + fromPullRequest: ResolvedPullRequest | undefined; + resolvedTarget: { target: ReviewTarget; pins?: Pins } | undefined; + resolvedBranch: string | undefined; + discoveredPullRequest: GitHubPullRequest | undefined; + resolvedTitle: string | undefined; +} + export interface ReviewProviders { + findPullRequest?( + pins: Pins, + branch: string, + ): Promise; headBranch?(pins: Pins, headRef?: string): Promise; projectSource?(snapshot: Snapshot, pins: Pins): Promise; /** Fetch a PR into the registered checkout the caller names. */ @@ -474,6 +498,7 @@ export class ReviewStore { readonly askHistory: AskHistory; private readonly db: DatabaseSync; private pending: Promise = Promise.resolve(); + private readonly preparing = new Set>(); private closing = false; private readonly liveSources = new Map(); private refreshTimer?: ReturnType; @@ -1010,6 +1035,16 @@ export class ReviewStore { return this.checkouts.group(root); } + private sameRepository(left: string, right: string): boolean { + if (left === right) return true; + const group = this.repositoryGroup(this.repositoryPath(left)); + + return ( + group !== undefined && + group.key === this.repositoryGroup(this.repositoryPath(right))?.key + ); + } + registerRepository(root: string) { this.db .prepare("INSERT OR IGNORE INTO repositories(id,path,name) VALUES(?,?,?)") @@ -1026,6 +1061,11 @@ export class ReviewStore { /** Forgets a checkout no board uses, after every write queued before it, * so a board being made with it keeps it. */ unregisterRepository(id: string): Promise { + if (this.preparing.size) + return Promise.allSettled([...this.preparing]).then(() => + this.unregisterRepository(id), + ); + const run = this.pending.then(async () => { this.db.exec("BEGIN IMMEDIATE"); @@ -1143,6 +1183,7 @@ export class ReviewStore { clearInterval(this.externalChanges); clearInterval(this.refreshTimer); clearInterval(this.purgeTimer); + await Promise.allSettled([...this.preparing]); await this.pending; this.listeners.clear(); this.catalogListeners.clear(); @@ -1522,50 +1563,142 @@ export class ReviewStore { ) : this.startPullRequestTitle(command); - return this.enqueue(async () => { - const op = command.operation; + const prepared = this.prepareCommand( + command, + pullRequest, + pullRequestTitle, + author, + ).then((source) => this.applyPreparedCommand(source, initial, author)); - if (op.type === "create") { - if (!op.target && !op.pullRequestUrl) - throw new ReviewInputError("Supply a target or a pullRequestUrl."); + this.preparing.add(prepared); + void prepared + .finally(() => this.preparing.delete(prepared)) + .catch(() => {}); - if (op.repositoryId && op.target) - throw new ReviewInputError( - "A checkout for the PR applies only to a create from pullRequestUrl alone; name it in the target instead.", - ); + return prepared; + } - if (!op.title && op.target && author.kind !== "person") - throw new ReviewInputError("Supply a title."); - } + /** Resolve optional network context before entering the write queue. */ + private async prepareCommand( + command: z.infer, + pullRequest: Promise | undefined, + pullRequestTitle: Promise, + author: Author, + ): Promise { + const op = command.operation; - const requestedTarget = - op.type === "create" || op.type === "set_target" - ? op.target - : undefined; + if (op.type === "create") { + if (!op.target && !op.pullRequestUrl) + throw new ReviewInputError("Supply a target or a pullRequestUrl."); + + if (op.repositoryId && op.target) + throw new ReviewInputError( + "A checkout for the PR applies only to a create from pullRequestUrl alone; name it in the target instead.", + ); + + if (!op.title && op.target && author.kind !== "person") + throw new ReviewInputError("Supply a title."); + } + + if (op.type === "set_target" && !op.target && !op.pullRequestUrl) + throw new ReviewInputError("Supply a target or a pullRequestUrl."); + + const requestedTarget = + op.type === "create" || op.type === "set_target" ? op.target : undefined; + + const fromPullRequest = await pullRequest; + + // No commits to resolve; repositoryPath throws if it is unregistered. + const resolvedTarget: { target: ReviewTarget; pins?: Pins } | undefined = + requestedTarget?.kind === "project" + ? (this.repositoryPath(requestedTarget.repositoryId), + { target: requestedTarget }) + : requestedTarget + ? await (this.providers.resolveTarget?.(requestedTarget) ?? + namedCommits(requestedTarget)) + : fromPullRequest; - const fromPullRequest = await pullRequest; + if (requestedTarget && !resolvedTarget) + throw new ReviewInputError("Review targets are unavailable."); - // No commits to resolve; repositoryPath throws if it is unregistered. - const resolvedTarget: { target: ReviewTarget; pins?: Pins } | undefined = - requestedTarget?.kind === "project" - ? (this.repositoryPath(requestedTarget.repositoryId), - { target: requestedTarget }) - : requestedTarget - ? await (this.providers.resolveTarget?.(requestedTarget) ?? - namedCommits(requestedTarget)) - : fromPullRequest; + const resolvedBranch = fromPullRequest + ? fromPullRequest.branch + : resolvedTarget?.pins && this.providers.headBranch + ? await this.providers.headBranch( + resolvedTarget.pins, + requestedTarget?.kind === "commits" + ? requestedTarget.head + : undefined, + ) + : undefined; + + const discoveredPullRequest = + resolvedTarget?.pins && + resolvedBranch && + (op.type === "create" || op.type === "set_target") && + op.pullRequestUrl === undefined + ? await this.providers + .findPullRequest?.(resolvedTarget.pins, resolvedBranch) + .catch(() => undefined) + : undefined; - if (requestedTarget && !resolvedTarget) - throw new ReviewInputError("Review targets are unavailable."); + const resolvedTitle = await pullRequestTitle; + return { + op, + requestedTarget, + fromPullRequest, + resolvedTarget, + resolvedBranch, + discoveredPullRequest, + resolvedTitle, + }; + } + + private applyPreparedCommand( + prepared: PreparedCommand, + initial: { document: Block[]; origin: SnapshotOrigin } | undefined, + author: Author, + ) { + const { + op, + requestedTarget, + fromPullRequest, + resolvedTarget, + resolvedBranch, + discoveredPullRequest, + resolvedTitle, + } = prepared; + + return this.enqueue(async () => { if ( op.type === "create" && op.pullRequestUrl && op.reuseExisting !== false ) { - const [found, ...others] = this.reviewsForPullRequest( - op.pullRequestUrl, - ); + const known = this.reviewsForPullRequest(op.pullRequestUrl); + const branch = fromPullRequest?.branch; + const repository = resolvedTarget?.target.repositoryId; + const project = repository && this.projectOfRepository(repository); + + const matching = + !known.length && branch && project + ? this.list() + .filter( + (review) => + review.project?.id === project && + sourceBranchName(review.origin?.branch) === + sourceBranchName(branch) && + !review.origin?.pullRequestUrl && + review.pins !== undefined && + repository !== undefined && + this.sameRepository(review.pins.repositoryId, repository), + ) + .reverse() + .map((review) => review.reviewId) + : []; + + const [found, ...others] = [...known, ...matching]; if (found) { const result = this.existingReview( @@ -1642,21 +1775,10 @@ export class ReviewStore { if ( (op.type === "create" || op.type === "set_target") && - this.providers.headBranch - ) { - const pins = resolvedTarget?.pins ?? snapshot.pins; - - if (pins) { - const headRef = - requestedTarget?.kind === "commits" - ? requestedTarget.head - : undefined; - - const branch = await this.providers.headBranch(pins, headRef); - - snapshot.origin = { ...snapshot.origin, branch }; - } - } + resolvedTarget?.pins && + (this.providers.headBranch || fromPullRequest) + ) + snapshot.origin = { ...snapshot.origin, branch: resolvedBranch }; if (!requestedTarget && fromPullRequest) snapshot.origin = { @@ -1694,7 +1816,7 @@ export class ReviewStore { assignFreshIds(block, (prefix) => `${prefix}-${++nextId}`); } - setPullRequest(snapshot, op.pullRequestUrl, await pullRequestTitle); + setPullRequest(snapshot, op.pullRequestUrl, resolvedTitle); break; case "rename": snapshot.title = op.title; @@ -1708,7 +1830,7 @@ export class ReviewStore { resolvedTarget!.target.repositoryId ? null : undefined), - await pullRequestTitle, + resolvedTitle, ); snapshot.staleSources = []; @@ -1780,6 +1902,33 @@ export class ReviewStore { } } + if ((op.type === "create" || op.type === "set_target") && snapshot.pins) { + const branch = sourceBranchName(snapshot.origin?.branch); + + const sameBranch = + previous?.pins?.repositoryId === snapshot.pins.repositoryId && + sourceBranchName(previous.origin?.branch) === branch; + + // Explicit PR input is resolved once for create and retarget alike. + if (op.pullRequestUrl === null) setPullRequest(snapshot, null); + else if (op.pullRequestUrl) + setPullRequest( + snapshot, + op.pullRequestUrl, + fromPullRequest?.title, + fromPullRequest?.baseBranch, + ); + else if (branch && discoveredPullRequest) + setPullRequest( + snapshot, + discoveredPullRequest.url, + discoveredPullRequest.title, + discoveredPullRequest.baseBranch, + ); + // An explicit binding survives an unavailable optional lookup. + else if (previous && !sameBranch) setPullRequest(snapshot, null); + } + const warnings = unpinnedWorktree ? [ `Its references now read at ${unpinnedWorktree.slice(0, 7)}, the checkout's last commit, which lacks any uncommitted lines they quoted. Check them.`, @@ -2010,11 +2159,10 @@ export class ReviewStore { .version, ); } - /** Runs one write after every write queued before it. */ + /** Runs an accepted write after every write queued before it. Admission + * checks belong at entry points: preparations accepted before close still + * need to enqueue their writes while close waits for them. */ private enqueue(work: () => Promise): Promise { - if (this.closing) - return Promise.reject(new Error("Review store is closing.")); - const run = this.pending.then(work); this.pending = run.catch(() => {}); @@ -2243,6 +2391,8 @@ export class ReviewStore { while (this.leased(reviewId) && Date.now() < deadline) await new Promise((resolve) => setTimeout(resolve, DISCARD_POLL_MS)); + if (this.closing) throw new Error("Review store is closing."); + return this.enqueue(async () => { if (!this.has(reviewId)) return { deleted: false }; const snapshot = this.read(reviewId); @@ -2333,6 +2483,9 @@ export class ReviewStore { personEdit(reviewId: string, input: unknown): Promise { const body = personEditSchema.parse(input); + if (this.closing) + return Promise.reject(new Error("Review store is closing.")); + return this.enqueue(async () => { const previous = this.read(reviewId); @@ -3026,7 +3179,11 @@ export class ReviewStore { ): Promise | undefined { const op = command.operation; - if (op.type !== "create" || op.target || !op.pullRequestUrl) + if ( + (op.type !== "create" && op.type !== "set_target") || + op.target || + !op.pullRequestUrl + ) return undefined; if (!this.providers.resolvePullRequest) @@ -3043,7 +3200,7 @@ export class ReviewStore { // A PR whose review is in Trash is refused before it is fetched. const [existing] = - op.reuseExisting === false + op.type !== "create" || op.reuseExisting === false ? [] : this.reviewsForPullRequest(op.pullRequestUrl); diff --git a/packages/review/src/review-stack.test.ts b/packages/review/src/review-stack.test.ts index 08ff0e852..4417891db 100644 --- a/packages/review/src/review-stack.test.ts +++ b/packages/review/src/review-stack.test.ts @@ -165,6 +165,122 @@ describe("resolveReviewStackLayers", () => { ).toEqual([]); }); + describe("an inferred PR chain", () => { + const repo = { full_name: "o/r" }; + + const graph = [ + { + number: 10, + title: "Lower", + head: { ref: "a", repo }, + base: { ref: "main", repo }, + }, + { + number: 20, + title: "Middle", + head: { ref: "b", repo }, + base: { ref: "a", repo }, + }, + { + number: 30, + title: "Upper", + head: { ref: "c", repo }, + base: { ref: "b", repo }, + }, + { + number: 40, + title: "Fork collision", + head: { ref: "a", repo: { full_name: "fork/r" } }, + base: { ref: "main", repo }, + }, + ]; + + const run: RunGitHubApi = async (_, endpoint) => + endpoint.includes("/stacks?") ? "[]" : JSON.stringify(graph); + + const pins = { repositoryId: "repo", base: "base", head: "head" }; + + const subject = { + reviewId: "middle", + pins, + origin: { + branch: "b", + baseBranch: "main", + pullRequestUrl: "https://github.com/o/r/pull/20", + pullRequestNumber: 20, + pullRequestBaseBranch: "a", + }, + }; + + const middle = { ...subject, title: "Middle board", trashedAt: null }; + + const branchBoard = ( + reviewId: string, + origin: NonNullable, + ) => ({ reviewId, title: reviewId, trashedAt: null, pins, origin }); + + const rows = async ( + reviews: Parameters[1], + ) => + ( + await resolveReviewStackLayers(subject, reviews, { runGitHubApi: run }) + ).map((layer) => [layer.branch, layer.relation, layer.reviewUuid]); + + it("discovers it even when other PRs have no whiteboards", async () => { + expect(await rows([middle])).toEqual([ + ["a", "earlier", null], + ["b", "current", "middle"], + ["c", "later", null], + ]); + }); + + it("attaches a board by a stored origin/ branch label", async () => { + expect( + await rows([middle, branchBoard("legacy", { branch: "origin/c" })]), + ).toEqual([ + ["a", "earlier", null], + ["b", "current", "middle"], + ["c", "later", "legacy"], + ]); + }); + }); + + it("does not pick an arbitrary continuation when multiple PRs depend on one branch", async () => { + const repo = { full_name: "o/r" }; + + const graph = [ + { + number: 10, + title: "Base", + head: { ref: "a", repo }, + base: { ref: "main", repo }, + }, + { + number: 20, + title: "Left", + head: { ref: "b", repo }, + base: { ref: "a", repo }, + }, + { + number: 30, + title: "Right", + head: { ref: "c", repo }, + base: { ref: "a", repo }, + }, + ]; + + const result = await resolveReviewStackLayers( + { origin: { pullRequestUrl: "https://github.com/o/r/pull/10" } }, + [], + { + runGitHubApi: async (_, endpoint) => + endpoint.includes("/stacks?") ? "[]" : JSON.stringify(graph), + }, + ); + + expect(result).toEqual([]); + }); + it("does not attach reviews of the same PR number in another repository", async () => { const result = await resolveReviewStackLayers( { origin: { pullRequestUrl: "https://github.com/o/r/pull/20" } }, diff --git a/packages/review/src/review-stack.ts b/packages/review/src/review-stack.ts index 56473a60f..305f99d90 100644 --- a/packages/review/src/review-stack.ts +++ b/packages/review/src/review-stack.ts @@ -5,6 +5,15 @@ import type { } from "@dev.fast/review-protocol"; import { z } from "zod"; +import { + type RunGitHubApi, + readGitHubPullRequests, +} from "./github-pr-graph.js"; +import { pullRequestKey, sourceBranchName } from "./review-api/origin.js"; +import { pullRequestProvider } from "./review-api/provider.js"; + +export type { RunGitHubApi }; + const GitHubStacksSchema = z.array( z.object({ pull_requests: z.array( @@ -16,15 +25,28 @@ const GitHubStacksSchema = z.array( }), ); -export type RunGitHubApi = (host: string, endpoint: string) => Promise; - -type Subject = Pick & { reviewId?: string }; +type Subject = Pick & { + reviewId?: string; +}; type Candidate = Pick< ReviewApiSummary, "reviewId" | "title" | "origin" | "pins" | "trashedAt" | "project" >; +/** The branch a review sits on: its PR's target, else the base it names. */ +const baseOf = (review?: Pick) => + sourceBranchName( + review?.origin?.pullRequestBaseBranch ?? review?.origin?.baseBranch, + ); + +/** A GitHub PR URL's host, owner/repo slug and number; else undefined. */ +function gitHubPullRequest(url: string | undefined) { + const resolution = url ? pullRequestProvider(url) : undefined; + + return resolution?.provider.id === "github" ? resolution.address : undefined; +} + interface StackEntry { branch: string; pullRequestNumber: number | null; @@ -47,6 +69,16 @@ export async function resolveReviewStackLayers( options.runGitHubApi ?? defaultRunGitHubApi, ); + if (entries.length < 2) { + const discovered = await inferredGitHubStack( + subject, + reviews, + options.runGitHubApi ?? defaultRunGitHubApi, + ).catch(() => []); + + if (discovered.length > 1) entries = discovered; + } + if (!entries.length && options.repositoryPath) entries = await localStack(subject, reviews, options.repositoryPath); @@ -135,21 +167,18 @@ async function gitHubStack( reviews: readonly Candidate[], runGitHubApi: RunGitHubApi, ): Promise { - const binding = subject.origin?.pullRequestUrl?.match( - /^https:\/\/([a-z0-9.-]+)\/([A-Za-z0-9_.-]+)\/([A-Za-z0-9_.-]+)\/pull\/([1-9]\d*)$/, - ); + const binding = gitHubPullRequest(subject.origin?.pullRequestUrl); if (!binding) return []; - const [, host, owner, repository, number] = binding; - const pullRequestNumber = Number(number); - const repoKey = `https://${host}/${owner}/${repository}`; + const { host, slug, number: pullRequestNumber } = binding; + const repoKey = `https://${host}/${slug}`; let stacks: GitHubStacks; try { stacks = await readGitHubStacks( runGitHubApi, - host!, - `${owner}/${repository}`, + host, + slug, pullRequestNumber, ); } catch { @@ -178,6 +207,75 @@ async function gitHubStack( })); } +/** A stack inferred from open PRs whose bases chain through this one's, for + * repositories without GitHub stacks. Whiteboards only annotate the result. */ +async function inferredGitHubStack( + subject: Subject, + reviews: readonly Candidate[], + run: RunGitHubApi, +): Promise { + const binding = gitHubPullRequest(subject.origin?.pullRequestUrl); + + if (!binding) return []; + const graph = await readGitHubPullRequests(binding.host, binding.slug, run); + const current = graph.find((pr) => pr.number === binding.number); + + if (!current) return []; + const chain = [current]; + const seen = new Set([current.headBranch]); + + const extend = (direction: "base" | "head") => { + let end = current; + + for (;;) { + const matches = graph.filter((pr) => + direction === "base" + ? pr.headBranch === end.baseBranch + : pr.baseBranch === end.headBranch, + ); + + if (matches.length !== 1 || seen.has(matches[0]!.headBranch)) return; + end = matches[0]!; + seen.add(end.headBranch); + + if (direction === "base") chain.unshift(end); + else chain.push(end); + } + }; + + extend("base"); + extend("head"); + + if (chain.length < 2) return []; + + const project = reviews.find((review) => review.reviewId === subject.reviewId) + ?.project?.id; + + return chain.map((pr) => ({ + branch: pr.headBranch, + pullRequestNumber: pr.number, + pullRequestUrl: pr.url, + base: null, + subject: pr.title, + current: pr.number === current.number, + reviews: reviews.filter((review) => { + const candidateUrl = review.origin?.pullRequestUrl; + + if (candidateUrl) + return pullRequestKey(candidateUrl) === pullRequestKey(pr.url); + + const sameRepository = subject.pins?.repositoryId + ? review.pins?.repositoryId === subject.pins.repositoryId + : Boolean(project && review.project?.id === project); + + return ( + sameRepository && + sourceBranchName(review.origin?.branch) === pr.headBranch + ); + }), + })); +} + /** The chain of reviews through this one, linked by the base each names. */ async function localStack( subject: Subject, @@ -187,21 +285,24 @@ async function localStack( const group = reviews.find((review) => review.reviewId === subject.reviewId) ?.project?.id; - const current = subject.origin?.branch; + const current = sourceBranchName(subject.origin?.branch); + const currentBase = baseOf(subject); if (!group || !current) return []; - const main = subject.origin?.baseBranch + const main = currentBase ? await defaultBranch(repositoryPath).catch(() => null) : null; - const name = (ref: string) => ref.replace(/^origin\//, ""); - const inGroup = reviews.filter((review) => review.project?.id === group); + const branchOf = (review?: Candidate) => + sourceBranchName(review?.origin?.branch); + const latest = (field: "branch" | "baseBranch", branch: string) => { const matches = inGroup.filter( - (review) => review.origin?.[field] === branch, + (review) => + (field === "branch" ? branchOf(review) : baseOf(review)) === branch, ); return matches.findLast((review) => !review.trashedAt) ?? matches.at(-1); @@ -210,16 +311,18 @@ async function localStack( const branches = [current]; for ( - let base = subject.origin?.baseBranch; - base && name(base) !== (main && name(main.ref)) && !branches.includes(base); - base = latest("branch", base)?.origin?.baseBranch + let base = currentBase; + base && + base !== (main && sourceBranchName(main.ref)) && + !branches.includes(base); + base = baseOf(latest("branch", base)) ) branches.unshift(base); for ( - let child = latest("baseBranch", current)?.origin?.branch; + let child = branchOf(latest("baseBranch", current)); child && !branches.includes(child); - child = latest("baseBranch", child)?.origin?.branch + child = branchOf(latest("baseBranch", child)) ) branches.push(child); @@ -243,7 +346,7 @@ async function localStack( base: (branch === bottom && start?.base) || null, subject: (branch === bottom && start?.subject) || null, current: branch === current, - reviews: inGroup.filter((review) => review.origin?.branch === branch), + reviews: inGroup.filter((review) => branchOf(review) === branch), })); } diff --git a/packages/review/src/sharing/bundle.test.ts b/packages/review/src/sharing/bundle.test.ts index 79cc0be34..23c7cff0d 100644 --- a/packages/review/src/sharing/bundle.test.ts +++ b/packages/review/src/sharing/bundle.test.ts @@ -143,7 +143,11 @@ it("imports a stacked board, whose origin names its base branch", async () => { store: { read: (...args) => ({ ...fixture.store.read(...args), - origin: { branch: "top", baseBranch: "bottom" }, + origin: { + branch: "top", + baseBranch: "bottom", + pullRequestBaseBranch: "main", + }, }), resource: (...args) => fixture.store.resource(...args), }, @@ -152,6 +156,7 @@ it("imports a stacked board, whose origin names its base branch", async () => { expect(validateShareBundle(bundle).snapshot.origin).toEqual({ branch: "top", baseBranch: "bottom", + pullRequestBaseBranch: "main", }); } finally { await fixture.data.close(); diff --git a/packages/review/src/sharing/import.ts b/packages/review/src/sharing/import.ts index 3a651655d..1b145a4a7 100644 --- a/packages/review/src/sharing/import.ts +++ b/packages/review/src/sharing/import.ts @@ -89,6 +89,7 @@ export const sharedSnapshotSchema = z.strictObject({ pullRequestNumber: z.number().int().positive().optional(), pullRequestUrl: pullRequestUrl.optional(), pullRequestTitle: z.string().optional(), + pullRequestBaseBranch: z.string().optional(), revision: z.string().optional(), }) .optional(),