From 90314e34a6235e245d0c40e23e9c50c9a82497fc Mon Sep 17 00:00:00 2001 From: ketan0 Date: Tue, 20 Oct 2026 12:32:55 -0700 Subject: [PATCH 1/5] Resolve branch and PR sources consistently for stack discovery Stack discovery finds a review's PR from its branch, walks ordinary GitHub PR base/head chains, and resolves branch and PR the same way for create and retarget. What it learns is stored on the review's existing origin record: two optional fields, pullRequestBaseBranch (the branch the PR merges into) and pullRequestDetached (the user removed the PR, so its branch must not find one again). Older stores and shared bundles parse unchanged. Agent-Session: 01a0d12e-51ca-7230-9991-0ce7a73018b2 Co-Authored-By: Claude Opus 5.5 Agent-Session: 01a0d12e-51ca-7230-9991-0ce7a73018b2 --- .../review-protocol/src/review-api-client.ts | 4 +- packages/review/src/github-pr-graph.test.ts | 47 +++ packages/review/src/github-pr-graph.ts | 97 +++++ packages/review/src/review-api/http.ts | 32 +- packages/review/src/review-api/local-data.ts | 108 +++++- packages/review/src/review-api/origin.ts | 22 +- .../src/review-api/pull-request.test.ts | 345 ++++++++++++++++++ .../review/src/review-api/review-api.test.ts | 1 + packages/review/src/review-api/store.ts | 246 ++++++++++--- packages/review/src/review-stack.test.ts | 96 +++++ packages/review/src/review-stack.ts | 99 ++++- packages/review/src/sharing/bundle.test.ts | 9 +- packages/review/src/sharing/import.ts | 2 + 13 files changed, 1014 insertions(+), 94 deletions(-) create mode 100644 packages/review/src/github-pr-graph.test.ts create mode 100644 packages/review/src/github-pr-graph.ts diff --git a/packages/review-protocol/src/review-api-client.ts b/packages/review-protocol/src/review-api-client.ts index 8e09da381..0bf2fad9c 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,8 @@ export interface ReviewApiSummary { pullRequestNumber?: number; pullRequestUrl?: string; pullRequestTitle?: string; + pullRequestBaseBranch?: string; + pullRequestDetached?: true; revision?: string; }; /** The PR's state on GitHub, when the host has read it. */ 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/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..ccbb38315 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"; @@ -1052,22 +1057,99 @@ 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 = branch.replace(/^origin\//, ""); + + 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 || origin.pullRequestDetached) 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 && + origin.pullRequestUrl.toLowerCase() !== found.url.toLowerCase() + ) + 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 named = + headRef && + headRef !== "HEAD" && + headRef !== "@" && + !/^(?:[0-9a-f]{40}|[0-9a-f]{64})$/i.test(headRef); + + if (!named && vcs?.kind !== "git") return undefined; + + const location: VcsArg[] = named + ? ["--git-dir", context.commonDir] + : ["-C", vcs!.rootPath]; 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 }, + named ? { rev: headRef! } : "HEAD", ]); const ref = stdout.trim(); @@ -1075,13 +1157,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 +2302,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..0d47135bb 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 clears a detach and 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,22 @@ export function setPullRequest( delete snapshot.origin.pullRequestUrl; delete snapshot.origin.pullRequestNumber; delete snapshot.origin.pullRequestTitle; + delete snapshot.origin.pullRequestBaseBranch; + delete snapshot.origin.pullRequestDetached; } return; } + const same = snapshot.origin?.pullRequestUrl === url; + const { pullRequestDetached: _detached, ...origin } = snapshot.origin ?? {}; snapshot.origin = { - ...snapshot.origin, + ...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..c0963a7df 100644 --- a/packages/review/src/review-api/pull-request.test.ts +++ b/packages/review/src/review-api/pull-request.test.ts @@ -201,6 +201,7 @@ describe("creating a review from a pull request URL alone", () => { }; let ghCalls: number; + let openPullRequests: object[]; const gitIn = (cwd: string) => @@ -261,6 +262,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 +280,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 +355,347 @@ 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 = [ + { + number: 7, + title: "Add widgets", + head: { ref: "widgets", repo: { full_name: "acme/widget" } }, + base: { ref: "lower-pr", repo: { full_name: "acme/widget" } }, + }, + ]; + 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 = [ + { + number: 7, + title: "Published later", + head: { ref: "widgets", repo: { full_name: "acme/widget" } }, + base: { ref: "main", repo: { full_name: "acme/widget" } }, + }, + ]; + 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); + + const detached = { + ...saved, + origin: { ...saved.origin, pullRequestDetached: true as const }, + }; + + expect(await local.data.discoverPullRequest(detached)).toEqual(detached); + } finally { + clock.mockRestore(); + } + }); + + it("uses a linked worktree's HEAD and retains explicit detachment on retarget", 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 = [ + { + number: 7, + title: "Widgets", + head: { ref: "widgets", repo: { full_name: "acme/widget" } }, + base: { ref: "main", repo: { full_name: "acme/widget" } }, + }, + ]; + 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", + ); + + await local.store.execute( + command({ + type: "set_target", + reviewId: created.reviewId, + target, + pullRequestUrl: null, + }), + ); + await local.store.execute( + command({ type: "set_target", reviewId: created.reviewId, target }), + ); + expect(local.store.read(created.reviewId).origin?.pullRequestDetached).toBe( + true, + ); + expect( + local.store.read(created.reviewId).origin?.pullRequestUrl, + ).toBeUndefined(); + }); + + 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 = [ + { + number: 7, + title: "Widgets", + head: { ref: "widgets", repo: { full_name: "acme/widget" } }, + base: { ref: "main", repo: { full_name: "acme/widget" } }, + }, + ]; + 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 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/review-api.test.ts b/packages/review/src/review-api/review-api.test.ts index 1bef96341..ec40fd3df 100644 --- a/packages/review/src/review-api/review-api.test.ts +++ b/packages/review/src/review-api/review-api.test.ts @@ -295,6 +295,7 @@ describe("snapshot authoring", () => { branch: "feature", baseRef: "main", revision: "legacy-revision", + pullRequestDetached: true, }); }); diff --git a/packages/review/src/review-api/store.ts b/packages/review/src/review-api/store.ts index 25060fbcd..07ba3d8c4 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,10 @@ 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 user removed the PR: its branch must not find one again. */ + pullRequestDetached?: true; /** The legacy review revision this version was imported from. */ revision?: string; } @@ -425,6 +436,10 @@ export interface ResolvedPullRequest { } 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 +489,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; @@ -1026,6 +1042,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 +1164,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 +1544,140 @@ 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, + ) { + 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."); - const fromPullRequest = await pullRequest; + 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.", + ); - // 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; + if (!op.title && op.target && author.kind !== "person") + throw new ReviewInputError("Supply a title."); + } - if (requestedTarget && !resolvedTarget) - throw new ReviewInputError("Review targets are unavailable."); + 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; + + if (requestedTarget && !resolvedTarget) + throw new ReviewInputError("Review targets are unavailable."); + + 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; + + const resolvedTitle = await pullRequestTitle; + + return { + op, + requestedTarget, + fromPullRequest, + resolvedTarget, + resolvedBranch, + discoveredPullRequest, + resolvedTitle, + }; + } + + private applyPreparedCommand( + prepared: Awaited>, + 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?.pullRequestDetached && + !review.origin?.pullRequestUrl, + ) + .reverse() + .map((review) => review.reviewId) + : []; + + const [found, ...others] = [...known, ...matching]; if (found) { const result = this.existingReview( @@ -1594,7 +1706,9 @@ export class ReviewStore { }); if (op.type === "lifecycle") - return this.commitBoardFacts(op.reviewId, author, { state: op.state }); + return this.commitBoardFacts(op.reviewId, author, { + state: op.state, + }); if (op.type === "attention") return this.commitBoardFacts(op.reviewId, author, { viewed: true }); @@ -1642,21 +1756,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 +1797,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 +1811,7 @@ export class ReviewStore { resolvedTarget!.target.repositoryId ? null : undefined), - await pullRequestTitle, + resolvedTitle, ); snapshot.staleSources = []; @@ -1780,6 +1883,39 @@ 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; + + // The user removed this branch's PR; discovery must not restore it. + const keepDetached = + sameBranch && previous?.origin?.pullRequestDetached; + + // Explicit PR input is resolved once for create and retarget alike. + if (op.pullRequestUrl === null) { + setPullRequest(snapshot, null); + snapshot.origin = { ...snapshot.origin, pullRequestDetached: true }; + } else if (op.pullRequestUrl) + setPullRequest( + snapshot, + op.pullRequestUrl, + fromPullRequest?.title, + fromPullRequest?.baseBranch, + ); + else if (branch && discoveredPullRequest && !keepDetached) + 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.`, @@ -3026,7 +3162,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 +3183,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..566f1eead 100644 --- a/packages/review/src/review-stack.test.ts +++ b/packages/review/src/review-stack.test.ts @@ -165,6 +165,102 @@ describe("resolveReviewStackLayers", () => { ).toEqual([]); }); + it("discovers an ordinary PR chain even when other PRs have no whiteboards", async () => { + 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 subject = { + reviewId: "middle", + pins: { repositoryId: "repo", base: "base", head: "head" }, + origin: { + branch: "b", + baseBranch: "main", + pullRequestUrl: "https://github.com/o/r/pull/20", + pullRequestNumber: 20, + pullRequestBaseBranch: "a", + }, + }; + + const layers = await resolveReviewStackLayers( + subject, + [{ ...subject, title: "Middle board", trashedAt: null }], + { runGitHubApi: run }, + ); + + expect( + layers.map((layer) => [layer.branch, layer.relation, layer.reviewUuid]), + ).toEqual([ + ["a", "earlier", null], + ["b", "current", "middle"], + ["c", "later", null], + ]); + }); + + 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..043f1e605 100644 --- a/packages/review/src/review-stack.ts +++ b/packages/review/src/review-stack.ts @@ -5,6 +5,8 @@ import type { } from "@dev.fast/review-protocol"; import { z } from "zod"; +import { readGitHubPullRequests } from "./github-pr-graph.js"; + const GitHubStacksSchema = z.array( z.object({ pull_requests: z.array( @@ -18,13 +20,19 @@ 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) => + review?.origin?.pullRequestBaseBranch ?? review?.origin?.baseBranch; + interface StackEntry { branch: string; pullRequestNumber: number | null; @@ -47,6 +55,16 @@ export async function resolveReviewStackLayers( options.runGitHubApi ?? defaultRunGitHubApi, ); + if (entries.length < 2) { + const discovered = await ordinaryGitHubStack( + subject, + reviews, + options.runGitHubApi ?? defaultRunGitHubApi, + ).catch(() => []); + + if (discovered.length > 1) entries = discovered; + } + if (!entries.length && options.repositoryPath) entries = await localStack(subject, reviews, options.repositoryPath); @@ -178,6 +196,74 @@ async function gitHubStack( })); } +/** Discover PR relationships first; whiteboards only annotate the result. */ +async function ordinaryGitHubStack( + subject: Subject, + reviews: readonly Candidate[], + run: RunGitHubApi, +): Promise { + const binding = subject.origin?.pullRequestUrl?.match( + /^https:\/\/([a-z0-9.-]+)\/([A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+)\/pull\/([1-9]\d*)$/, + ); + + if (!binding) return []; + const [, host, slug, number] = binding; + const graph = await readGitHubPullRequests(host!, slug!, run); + const current = graph.find((pr) => pr.number === Number(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 candidateUrl.toLowerCase() === pr.url.toLowerCase(); + + const sameRepository = subject.pins?.repositoryId + ? review.pins?.repositoryId === subject.pins.repositoryId + : Boolean(project && review.project?.id === project); + + return sameRepository && review.origin?.branch === pr.headBranch; + }), + })); +} + /** The chain of reviews through this one, linked by the base each names. */ async function localStack( subject: Subject, @@ -188,10 +274,11 @@ async function localStack( ?.project?.id; const current = 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; @@ -201,7 +288,9 @@ async function localStack( const latest = (field: "branch" | "baseBranch", branch: string) => { const matches = inGroup.filter( - (review) => review.origin?.[field] === branch, + (review) => + (field === "branch" ? review.origin?.branch : baseOf(review)) === + branch, ); return matches.findLast((review) => !review.trashedAt) ?? matches.at(-1); @@ -210,9 +299,9 @@ async function localStack( const branches = [current]; for ( - let base = subject.origin?.baseBranch; + let base = currentBase; base && name(base) !== (main && name(main.ref)) && !branches.includes(base); - base = latest("branch", base)?.origin?.baseBranch + base = baseOf(latest("branch", base)) ) branches.unshift(base); diff --git a/packages/review/src/sharing/bundle.test.ts b/packages/review/src/sharing/bundle.test.ts index 79cc0be34..7f9c449a4 100644 --- a/packages/review/src/sharing/bundle.test.ts +++ b/packages/review/src/sharing/bundle.test.ts @@ -143,7 +143,12 @@ 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", + pullRequestDetached: true, + }, }), resource: (...args) => fixture.store.resource(...args), }, @@ -152,6 +157,8 @@ it("imports a stacked board, whose origin names its base branch", async () => { expect(validateShareBundle(bundle).snapshot.origin).toEqual({ branch: "top", baseBranch: "bottom", + pullRequestBaseBranch: "main", + pullRequestDetached: true, }); } finally { await fixture.data.close(); diff --git a/packages/review/src/sharing/import.ts b/packages/review/src/sharing/import.ts index 3a651655d..53c1f7d26 100644 --- a/packages/review/src/sharing/import.ts +++ b/packages/review/src/sharing/import.ts @@ -89,6 +89,8 @@ export const sharedSnapshotSchema = z.strictObject({ pullRequestNumber: z.number().int().positive().optional(), pullRequestUrl: pullRequestUrl.optional(), pullRequestTitle: z.string().optional(), + pullRequestBaseBranch: z.string().optional(), + pullRequestDetached: z.literal(true).optional(), revision: z.string().optional(), }) .optional(), From 7f1d5dc726d3ef6eb389f399af74573ab2b45825 Mon Sep 17 00:00:00 2001 From: ketan0 Date: Thu, 22 Oct 2026 15:51:34 -0700 Subject: [PATCH 2/5] Tidy branch and PR discovery after review Boards whose PR the user removed no longer join a stack by branch name, and stored branch labels are compared without their legacy origin/ prefix everywhere stacks match boards. PR URLs are parsed and compared through pullRequestProvider and pullRequestKey instead of local regexes, the inferred stack walk is named inferredGitHubStack, and prepareCommand returns a named PreparedCommand. Co-Authored-By: Claude Opus 5.5 Agent-Session: 01a0d12e-51ca-7230-9991-0ce7a73018b2 --- packages/review/src/review-api/local-data.ts | 21 +++-- .../src/review-api/pull-request.test.ts | 44 +++------- packages/review/src/review-api/store.ts | 19 ++-- packages/review/src/review-stack.test.ts | 61 ++++++++++--- packages/review/src/review-stack.ts | 87 ++++++++++++------- 5 files changed, 140 insertions(+), 92 deletions(-) diff --git a/packages/review/src/review-api/local-data.ts b/packages/review/src/review-api/local-data.ts index ccbb38315..685a84ebb 100644 --- a/packages/review/src/review-api/local-data.ts +++ b/packages/review/src/review-api/local-data.ts @@ -84,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, @@ -1077,7 +1078,7 @@ export class LocalReviewData { const remote = parseGitRemote(context.originUrl); if (!remote) return undefined; - const name = branch.replace(/^origin\//, ""); + const name = sourceBranchName(branch); const matches = ( await readGitHubPullRequests( @@ -1105,7 +1106,7 @@ export class LocalReviewData { // A manually selected PR must not be replaced by a branch-name match. if ( origin.pullRequestUrl && - origin.pullRequestUrl.toLowerCase() !== found.url.toLowerCase() + pullRequestKey(origin.pullRequestUrl) !== pullRequestKey(found.url) ) return snapshot; @@ -1131,17 +1132,19 @@ export class LocalReviewData { const vcs = await this.vcs(pins.repositoryId); - const named = + const symbolicRef = headRef && headRef !== "HEAD" && headRef !== "@" && - !/^(?:[0-9a-f]{40}|[0-9a-f]{64})$/i.test(headRef); + !/^(?:[0-9a-f]{40}|[0-9a-f]{64})$/i.test(headRef) + ? headRef + : undefined; - if (!named && vcs?.kind !== "git") return undefined; + let location: VcsArg[]; - const location: VcsArg[] = named - ? ["--git-dir", context.commonDir] - : ["-C", vcs!.rootPath]; + 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", [ @@ -1149,7 +1152,7 @@ export class LocalReviewData { "rev-parse", "--symbolic-full-name", "--verify", - named ? { rev: headRef! } : "HEAD", + symbolicRef ? { rev: symbolicRef } : "HEAD", ]); const ref = stdout.trim(); diff --git a/packages/review/src/review-api/pull-request.test.ts b/packages/review/src/review-api/pull-request.test.ts index c0963a7df..80ea295fa 100644 --- a/packages/review/src/review-api/pull-request.test.ts +++ b/packages/review/src/review-api/pull-request.test.ts @@ -203,6 +203,14 @@ 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) => (...args: string[]) => @@ -394,14 +402,7 @@ describe("creating a review from a pull request URL alone", () => { 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 = [ - { - number: 7, - title: "Add widgets", - head: { ref: "widgets", repo: { full_name: "acme/widget" } }, - base: { ref: "lower-pr", repo: { full_name: "acme/widget" } }, - }, - ]; + openPullRequests = [widgetsPullRequest("Add widgets", "lower-pr")]; const registered = await local.data.register(checkout); const created = await local.store.execute( @@ -474,14 +475,7 @@ describe("creating a review from a pull request URL alone", () => { const saved = local.store.read(created.reviewId); expect(saved.origin?.pullRequestUrl).toBeUndefined(); - openPullRequests = [ - { - number: 7, - title: "Published later", - head: { ref: "widgets", repo: { full_name: "acme/widget" } }, - base: { ref: "main", repo: { full_name: "acme/widget" } }, - }, - ]; + openPullRequests = [widgetsPullRequest("Published later")]; const clock = vi.spyOn(Date, "now").mockReturnValue(Date.now() + 61_000); try { @@ -507,14 +501,7 @@ describe("creating a review from a pull request URL alone", () => { git("fetch", "origin", "refs/pull/7/head:refs/heads/widgets"); const linked = path.join(directory, "linked"); git("worktree", "add", "--quiet", linked, "widgets"); - openPullRequests = [ - { - number: 7, - title: "Widgets", - head: { ref: "widgets", repo: { full_name: "acme/widget" } }, - base: { ref: "main", repo: { full_name: "acme/widget" } }, - }, - ]; + openPullRequests = [widgetsPullRequest("Widgets")]; const registered = await local.data.register(linked); const target = { @@ -571,14 +558,7 @@ describe("creating a review from a pull request URL alone", () => { 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 = [ - { - number: 7, - title: "Widgets", - head: { ref: "widgets", repo: { full_name: "acme/widget" } }, - base: { ref: "main", repo: { full_name: "acme/widget" } }, - }, - ]; + openPullRequests = [widgetsPullRequest("Widgets")]; const registered = await local.data.register(checkout); const created = await local.store.execute( diff --git a/packages/review/src/review-api/store.ts b/packages/review/src/review-api/store.ts index 07ba3d8c4..19454b319 100644 --- a/packages/review/src/review-api/store.ts +++ b/packages/review/src/review-api/store.ts @@ -435,6 +435,17 @@ 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, @@ -1565,7 +1576,7 @@ export class ReviewStore { pullRequest: Promise | undefined, pullRequestTitle: Promise, author: Author, - ) { + ): Promise { const op = command.operation; if (op.type === "create") { @@ -1637,7 +1648,7 @@ export class ReviewStore { } private applyPreparedCommand( - prepared: Awaited>, + prepared: PreparedCommand, initial: { document: Block[]; origin: SnapshotOrigin } | undefined, author: Author, ) { @@ -1706,9 +1717,7 @@ export class ReviewStore { }); if (op.type === "lifecycle") - return this.commitBoardFacts(op.reviewId, author, { - state: op.state, - }); + return this.commitBoardFacts(op.reviewId, author, { state: op.state }); if (op.type === "attention") return this.commitBoardFacts(op.reviewId, author, { viewed: true }); diff --git a/packages/review/src/review-stack.test.ts b/packages/review/src/review-stack.test.ts index 566f1eead..b16bb4bee 100644 --- a/packages/review/src/review-stack.test.ts +++ b/packages/review/src/review-stack.test.ts @@ -165,7 +165,7 @@ describe("resolveReviewStackLayers", () => { ).toEqual([]); }); - it("discovers an ordinary PR chain even when other PRs have no whiteboards", async () => { + describe("an inferred PR chain", () => { const repo = { full_name: "o/r" }; const graph = [ @@ -198,9 +198,11 @@ describe("resolveReviewStackLayers", () => { const run: RunGitHubApi = async (_, endpoint) => endpoint.includes("/stacks?") ? "[]" : JSON.stringify(graph); + const pins = { repositoryId: "repo", base: "base", head: "head" }; + const subject = { reviewId: "middle", - pins: { repositoryId: "repo", base: "base", head: "head" }, + pins, origin: { branch: "b", baseBranch: "main", @@ -210,19 +212,50 @@ describe("resolveReviewStackLayers", () => { }, }; - const layers = await resolveReviewStackLayers( - subject, - [{ ...subject, title: "Middle board", trashedAt: null }], - { runGitHubApi: run }, - ); + 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], + ]); + }); - expect( - layers.map((layer) => [layer.branch, layer.relation, layer.reviewUuid]), - ).toEqual([ - ["a", "earlier", null], - ["b", "current", "middle"], - ["c", "later", null], - ]); + it("does not attach a board whose PR was removed by its branch", async () => { + expect( + await rows([ + branchBoard("detached", { branch: "a", pullRequestDetached: true }), + 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 () => { diff --git a/packages/review/src/review-stack.ts b/packages/review/src/review-stack.ts index 043f1e605..191314124 100644 --- a/packages/review/src/review-stack.ts +++ b/packages/review/src/review-stack.ts @@ -5,7 +5,14 @@ import type { } from "@dev.fast/review-protocol"; import { z } from "zod"; -import { readGitHubPullRequests } from "./github-pr-graph.js"; +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({ @@ -18,8 +25,6 @@ const GitHubStacksSchema = z.array( }), ); -export type RunGitHubApi = (host: string, endpoint: string) => Promise; - type Subject = Pick & { reviewId?: string; }; @@ -31,7 +36,16 @@ type Candidate = Pick< /** The branch a review sits on: its PR's target, else the base it names. */ const baseOf = (review?: Pick) => - review?.origin?.pullRequestBaseBranch ?? review?.origin?.baseBranch; + 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; @@ -56,7 +70,7 @@ export async function resolveReviewStackLayers( ); if (entries.length < 2) { - const discovered = await ordinaryGitHubStack( + const discovered = await inferredGitHubStack( subject, reviews, options.runGitHubApi ?? defaultRunGitHubApi, @@ -153,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 { @@ -196,20 +207,18 @@ async function gitHubStack( })); } -/** Discover PR relationships first; whiteboards only annotate the result. */ -async function ordinaryGitHubStack( +/** 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 = 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, slug, number] = binding; - const graph = await readGitHubPullRequests(host!, slug!, run); - const current = graph.find((pr) => pr.number === Number(number)); + const graph = await readGitHubPullRequests(binding.host, binding.slug, run); + const current = graph.find((pr) => pr.number === binding.number); if (!current) return []; const chain = [current]; @@ -253,13 +262,18 @@ async function ordinaryGitHubStack( const candidateUrl = review.origin?.pullRequestUrl; if (candidateUrl) - return candidateUrl.toLowerCase() === pr.url.toLowerCase(); + 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 && review.origin?.branch === pr.headBranch; + // A board whose PR was removed matches no PR by its branch. + return ( + sameRepository && + !review.origin?.pullRequestDetached && + sourceBranchName(review.origin?.branch) === pr.headBranch + ); }), })); } @@ -273,7 +287,7 @@ 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 []; @@ -282,15 +296,22 @@ async function localStack( ? await defaultBranch(repositoryPath).catch(() => null) : null; - const name = (ref: string) => ref.replace(/^origin\//, ""); + // A board whose PR was removed joins no stack by its branch, unless it is + // the one asking. + const inGroup = reviews.filter( + (review) => + review.project?.id === group && + (!review.origin?.pullRequestDetached || + review.reviewId === subject.reviewId), + ); - 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) => - (field === "branch" ? review.origin?.branch : baseOf(review)) === - branch, + (field === "branch" ? branchOf(review) : baseOf(review)) === branch, ); return matches.findLast((review) => !review.trashedAt) ?? matches.at(-1); @@ -300,15 +321,17 @@ async function localStack( for ( let base = currentBase; - base && name(base) !== (main && name(main.ref)) && !branches.includes(base); + 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); @@ -332,7 +355,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), })); } From 399793f83c55643a5cfaca63da89680ae096cef8 Mon Sep 17 00:00:00 2001 From: ketan0 Date: Thu, 22 Oct 2026 21:29:22 -0700 Subject: [PATCH 3/5] Drop the PR detachment flag Removing a PR from a board (pullRequestUrl: null) clears its PR fields as it does on main, without remembering that the removal was deliberate. Later branch discovery may find the PR again, which is acceptable; nothing in the agent tools or the canvas detaches a PR today. Co-Authored-By: Claude Opus 5.5 Agent-Session: 01a0d12e-51ca-7230-9991-0ce7a73018b2 --- .../review-protocol/src/review-api-client.ts | 1 - packages/review/src/review-api/local-data.ts | 2 +- packages/review/src/review-api/origin.ts | 8 +++--- .../src/review-api/pull-request.test.ts | 27 +------------------ .../review/src/review-api/review-api.test.ts | 1 - packages/review/src/review-api/store.ts | 15 +++-------- packages/review/src/review-stack.test.ts | 13 --------- packages/review/src/review-stack.ts | 11 +------- packages/review/src/sharing/bundle.test.ts | 2 -- packages/review/src/sharing/import.ts | 1 - 10 files changed, 9 insertions(+), 72 deletions(-) diff --git a/packages/review-protocol/src/review-api-client.ts b/packages/review-protocol/src/review-api-client.ts index 0bf2fad9c..bddcf586f 100644 --- a/packages/review-protocol/src/review-api-client.ts +++ b/packages/review-protocol/src/review-api-client.ts @@ -31,7 +31,6 @@ export interface ReviewApiSummary { pullRequestUrl?: string; pullRequestTitle?: string; pullRequestBaseBranch?: string; - pullRequestDetached?: true; revision?: string; }; /** The PR's state on GitHub, when the host has read it. */ diff --git a/packages/review/src/review-api/local-data.ts b/packages/review/src/review-api/local-data.ts index 685a84ebb..2e68080d0 100644 --- a/packages/review/src/review-api/local-data.ts +++ b/packages/review/src/review-api/local-data.ts @@ -1095,7 +1095,7 @@ export class LocalReviewData { async discoverPullRequest(snapshot: Snapshot): Promise { const { origin, pins } = snapshot; - if (!pins || !origin?.branch || origin.pullRequestDetached) return snapshot; + if (!pins || !origin?.branch) return snapshot; const found = await this.findPullRequest(pins, origin.branch).catch( () => undefined, diff --git a/packages/review/src/review-api/origin.ts b/packages/review/src/review-api/origin.ts index 0d47135bb..77cb1d685 100644 --- a/packages/review/src/review-api/origin.ts +++ b/packages/review/src/review-api/origin.ts @@ -21,8 +21,8 @@ export const sourceBranchName = (branch: string | undefined) => branch?.replace(/^origin\//, ""); /** Omission preserves identity; null removes the PR without changing import - * metadata. A new url clears a detach and keeps the title and PR base when the - * caller omits them for the same PR. */ + * 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, @@ -37,16 +37,14 @@ export function setPullRequest( delete snapshot.origin.pullRequestNumber; delete snapshot.origin.pullRequestTitle; delete snapshot.origin.pullRequestBaseBranch; - delete snapshot.origin.pullRequestDetached; } return; } const same = snapshot.origin?.pullRequestUrl === url; - const { pullRequestDetached: _detached, ...origin } = snapshot.origin ?? {}; snapshot.origin = { - ...origin, + ...snapshot.origin, pullRequestUrl: url, pullRequestNumber: Number(url.split("/").at(-1)), pullRequestTitle: diff --git a/packages/review/src/review-api/pull-request.test.ts b/packages/review/src/review-api/pull-request.test.ts index 80ea295fa..39daec4e7 100644 --- a/packages/review/src/review-api/pull-request.test.ts +++ b/packages/review/src/review-api/pull-request.test.ts @@ -484,19 +484,12 @@ describe("creating a review from a pull request URL alone", () => { expect(projected.origin?.pullRequestNumber).toBe(7); expect(projected.pins).toEqual(saved.pins); expect(local.store.read(created.reviewId)).toEqual(saved); - - const detached = { - ...saved, - origin: { ...saved.origin, pullRequestDetached: true as const }, - }; - - expect(await local.data.discoverPullRequest(detached)).toEqual(detached); } finally { clock.mockRestore(); } }); - it("uses a linked worktree's HEAD and retains explicit detachment on retarget", async () => { + 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"); @@ -535,24 +528,6 @@ describe("creating a review from a pull request URL alone", () => { expect(local.store.read(explicitHead.reviewId).origin?.branch).toBe( "widgets", ); - - await local.store.execute( - command({ - type: "set_target", - reviewId: created.reviewId, - target, - pullRequestUrl: null, - }), - ); - await local.store.execute( - command({ type: "set_target", reviewId: created.reviewId, target }), - ); - expect(local.store.read(created.reviewId).origin?.pullRequestDetached).toBe( - true, - ); - expect( - local.store.read(created.reviewId).origin?.pullRequestUrl, - ).toBeUndefined(); }); it("clears the former branch's PR identity when moving to a different branch", async () => { diff --git a/packages/review/src/review-api/review-api.test.ts b/packages/review/src/review-api/review-api.test.ts index ec40fd3df..1bef96341 100644 --- a/packages/review/src/review-api/review-api.test.ts +++ b/packages/review/src/review-api/review-api.test.ts @@ -295,7 +295,6 @@ describe("snapshot authoring", () => { branch: "feature", baseRef: "main", revision: "legacy-revision", - pullRequestDetached: true, }); }); diff --git a/packages/review/src/review-api/store.ts b/packages/review/src/review-api/store.ts index 19454b319..fa8683e6f 100644 --- a/packages/review/src/review-api/store.ts +++ b/packages/review/src/review-api/store.ts @@ -259,8 +259,6 @@ export interface SnapshotOrigin { pullRequestTitle?: string; /** The branch the PR merges into, which can differ from baseBranch. */ pullRequestBaseBranch?: string; - /** The user removed the PR: its branch must not find one again. */ - pullRequestDetached?: true; /** The legacy review revision this version was imported from. */ revision?: string; } @@ -1681,7 +1679,6 @@ export class ReviewStore { review.project?.id === project && sourceBranchName(review.origin?.branch) === sourceBranchName(branch) && - !review.origin?.pullRequestDetached && !review.origin?.pullRequestUrl, ) .reverse() @@ -1899,22 +1896,16 @@ export class ReviewStore { previous?.pins?.repositoryId === snapshot.pins.repositoryId && sourceBranchName(previous.origin?.branch) === branch; - // The user removed this branch's PR; discovery must not restore it. - const keepDetached = - sameBranch && previous?.origin?.pullRequestDetached; - // Explicit PR input is resolved once for create and retarget alike. - if (op.pullRequestUrl === null) { - setPullRequest(snapshot, null); - snapshot.origin = { ...snapshot.origin, pullRequestDetached: true }; - } else if (op.pullRequestUrl) + if (op.pullRequestUrl === null) setPullRequest(snapshot, null); + else if (op.pullRequestUrl) setPullRequest( snapshot, op.pullRequestUrl, fromPullRequest?.title, fromPullRequest?.baseBranch, ); - else if (branch && discoveredPullRequest && !keepDetached) + else if (branch && discoveredPullRequest) setPullRequest( snapshot, discoveredPullRequest.url, diff --git a/packages/review/src/review-stack.test.ts b/packages/review/src/review-stack.test.ts index b16bb4bee..4417891db 100644 --- a/packages/review/src/review-stack.test.ts +++ b/packages/review/src/review-stack.test.ts @@ -234,19 +234,6 @@ describe("resolveReviewStackLayers", () => { ]); }); - it("does not attach a board whose PR was removed by its branch", async () => { - expect( - await rows([ - branchBoard("detached", { branch: "a", pullRequestDetached: true }), - 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" })]), diff --git a/packages/review/src/review-stack.ts b/packages/review/src/review-stack.ts index 191314124..305f99d90 100644 --- a/packages/review/src/review-stack.ts +++ b/packages/review/src/review-stack.ts @@ -268,10 +268,8 @@ async function inferredGitHubStack( ? review.pins?.repositoryId === subject.pins.repositoryId : Boolean(project && review.project?.id === project); - // A board whose PR was removed matches no PR by its branch. return ( sameRepository && - !review.origin?.pullRequestDetached && sourceBranchName(review.origin?.branch) === pr.headBranch ); }), @@ -296,14 +294,7 @@ async function localStack( ? await defaultBranch(repositoryPath).catch(() => null) : null; - // A board whose PR was removed joins no stack by its branch, unless it is - // the one asking. - const inGroup = reviews.filter( - (review) => - review.project?.id === group && - (!review.origin?.pullRequestDetached || - review.reviewId === subject.reviewId), - ); + const inGroup = reviews.filter((review) => review.project?.id === group); const branchOf = (review?: Candidate) => sourceBranchName(review?.origin?.branch); diff --git a/packages/review/src/sharing/bundle.test.ts b/packages/review/src/sharing/bundle.test.ts index 7f9c449a4..23c7cff0d 100644 --- a/packages/review/src/sharing/bundle.test.ts +++ b/packages/review/src/sharing/bundle.test.ts @@ -147,7 +147,6 @@ it("imports a stacked board, whose origin names its base branch", async () => { branch: "top", baseBranch: "bottom", pullRequestBaseBranch: "main", - pullRequestDetached: true, }, }), resource: (...args) => fixture.store.resource(...args), @@ -158,7 +157,6 @@ it("imports a stacked board, whose origin names its base branch", async () => { branch: "top", baseBranch: "bottom", pullRequestBaseBranch: "main", - pullRequestDetached: true, }); } finally { await fixture.data.close(); diff --git a/packages/review/src/sharing/import.ts b/packages/review/src/sharing/import.ts index 53c1f7d26..1b145a4a7 100644 --- a/packages/review/src/sharing/import.ts +++ b/packages/review/src/sharing/import.ts @@ -90,7 +90,6 @@ export const sharedSnapshotSchema = z.strictObject({ pullRequestUrl: pullRequestUrl.optional(), pullRequestTitle: z.string().optional(), pullRequestBaseBranch: z.string().optional(), - pullRequestDetached: z.literal(true).optional(), revision: z.string().optional(), }) .optional(), From b36d33f8117fb811f40b6b4897d50d9ec8db3c6d Mon Sep 17 00:00:00 2001 From: ketan0 Date: Thu, 22 Oct 2026 23:03:18 -0700 Subject: [PATCH 4/5] Fix shutdown draining and repository-scoped PR reuse Agent-Session: 01a0d12e-51ca-7230-9991-0ce7a73018b2 --- .../src/review-api/board-writes.test.ts | 128 ++++++++++++++++++ .../src/review-api/pull-request.test.ts | 35 +++++ packages/review/src/review-api/store.ts | 27 +++- 3 files changed, 185 insertions(+), 5 deletions(-) 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/pull-request.test.ts b/packages/review/src/review-api/pull-request.test.ts index 39daec4e7..baf1f587d 100644 --- a/packages/review/src/review-api/pull-request.test.ts +++ b/packages/review/src/review-api/pull-request.test.ts @@ -594,6 +594,41 @@ describe("creating a review from a pull request URL alone", () => { 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); diff --git a/packages/review/src/review-api/store.ts b/packages/review/src/review-api/store.ts index fa8683e6f..605f18f53 100644 --- a/packages/review/src/review-api/store.ts +++ b/packages/review/src/review-api/store.ts @@ -1035,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(?,?,?)") @@ -1679,7 +1689,10 @@ export class ReviewStore { review.project?.id === project && sourceBranchName(review.origin?.branch) === sourceBranchName(branch) && - !review.origin?.pullRequestUrl, + !review.origin?.pullRequestUrl && + review.pins !== undefined && + repository !== undefined && + this.sameRepository(review.pins.repositoryId, repository), ) .reverse() .map((review) => review.reviewId) @@ -2146,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(() => {}); @@ -2379,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); @@ -2469,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); From a64a351cc84bb4ee364b75d8fe0525d1fd08c755 Mon Sep 17 00:00:00 2001 From: ketan0 Date: Thu, 22 Oct 2026 23:25:03 -0700 Subject: [PATCH 5/5] Label local review stacks by their branch bases Agent-Session: 01a0d12e-51ca-7230-9991-0ce7a73018b2 --- .../review-stack-selector.browser.test.tsx | 122 ++++++++++-------- .../review/app/src/review-stack-selector.tsx | 6 +- 2 files changed, 74 insertions(+), 54 deletions(-) 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