From 5feb68da4f7b30f25520a34e51f6de3e57d39383 Mon Sep 17 00:00:00 2001 From: Mohamed Boudra Date: Mon, 2 Nov 2026 07:18:48 +0100 Subject: [PATCH] fix(workspaces): preserve pull requests across checkout changes Keep branch associations when the local commit changes and resolve transferred source identities before matching. Wait for the composer to accept input before queueing in browser tests. --- packages/app/e2e/support/helpers/composer.ts | 3 + .../workspace-git-service.primitive.test.ts | 53 +++++++++++++++++- .../src/server/workspace-git-service.ts | 24 ++++++-- .../server/src/services/gitea-service.test.ts | 56 +++++++++++++++++++ packages/server/src/services/gitea-service.ts | 33 ++++++----- 5 files changed, 149 insertions(+), 20 deletions(-) diff --git a/packages/app/e2e/support/helpers/composer.ts b/packages/app/e2e/support/helpers/composer.ts index bc0fef67f3..7069dfd6c7 100644 --- a/packages/app/e2e/support/helpers/composer.ts +++ b/packages/app/e2e/support/helpers/composer.ts @@ -84,6 +84,9 @@ export async function typeIntoFocusedComposer(page: Page, text: string): Promise } export async function sendDraftToQueue(page: Page): Promise { + await expect( + page.getByRole("button", { name: /^(Send and steer|Send and interrupt)$/ }), + ).toBeEnabled({ timeout: 30_000 }); await composerInput(page).press("Control+Enter"); } diff --git a/packages/server/src/server/workspace-git-service.primitive.test.ts b/packages/server/src/server/workspace-git-service.primitive.test.ts index dedeb2672d..87be7a6183 100644 --- a/packages/server/src/server/workspace-git-service.primitive.test.ts +++ b/packages/server/src/server/workspace-git-service.primitive.test.ts @@ -1604,7 +1604,7 @@ describe("WorkspaceGitServiceImpl primitive refresh entrypoint", () => { nowMs = 3_000; await service.refresh(REPO_CWD); - expect(workspaceRuntimeStore.get(REPO_CWD)?.forge.pullRequest).toBeNull(); + expect(workspaceRuntimeStore.get(REPO_CWD)?.forge.pullRequest?.title).toBe("Visible PR"); expect(forge.getCurrentPullRequestStatus).not.toHaveBeenCalled(); await vi.advanceTimersByTimeAsync(0); @@ -1619,6 +1619,57 @@ describe("WorkspaceGitServiceImpl primitive refresh entrypoint", () => { } }); + test("keeps the known open association when a checkout commit changes before forge refresh", async () => { + let headSha = "1111111111111111111111111111111111111111"; + const open = createCurrentPullRequestStatus({ + number: 2, + url: "https://forge-commit.test/acme/repo/pulls/2", + state: "open", + }); + const merged = createCurrentPullRequestStatus({ + number: 1, + url: "https://forge-commit.test/acme/repo/pulls/1", + state: "merged", + isMerged: true, + }); + const forge = { ...createGitHubServiceStub(), retainCurrentPullRequestStatusPoll: undefined }; + const unregister = defaultForgeRegistry.register("forge-commit-test", { + createService: () => forge, + matchesHost: (host) => host === "forge-commit.test", + }); + let first = true; + const service = createService({ + getCheckoutSnapshotFacts: vi.fn(async (cwd) => + createCheckoutFacts(cwd, { + currentBranch: "feature", + remoteUrl: "https://forge-commit.test/acme/repo.git", + pullRequestLookupTarget: { headRef: "feature", headSha }, + }), + ), + getCheckoutStatus: vi.fn(async (cwd) => + createCheckoutStatus(cwd, { + currentBranch: "feature", + remoteUrl: "https://forge-commit.test/acme/repo.git", + }), + ), + getPullRequestStatus: vi.fn(async (_cwd, _forge, options) => { + const result = first || options.previousPullRequest?.number === 2 ? open : merged; + first = false; + return { ...createPullRequestStatusResult(), status: result }; + }), + }); + try { + expect((await service.getSnapshot(REPO_CWD)).forge.pullRequest?.number).toBe(2); + headSha = "2222222222222222222222222222222222222222"; + const refreshed = await service.getSnapshot(REPO_CWD, { force: true, reason: "test" }); + expect(refreshed.git.headSha).toBe(headSha); + expect(refreshed.forge.pullRequest).toMatchObject({ number: 2, state: "open" }); + } finally { + service.dispose(); + unregister(); + } + }); + test("subscription cancels generic forge PR status self-heal polling after unsubscribe", async () => { const forge = { ...createGitHubServiceStub(), diff --git a/packages/server/src/server/workspace-git-service.ts b/packages/server/src/server/workspace-git-service.ts index 8452cac873..c23315355c 100644 --- a/packages/server/src/server/workspace-git-service.ts +++ b/packages/server/src/server/workspace-git-service.ts @@ -3565,7 +3565,7 @@ export class WorkspaceGitServiceImpl implements WorkspaceGitService { target.lastShellOutAtMs = now.getTime(); const cwd = target.cwd; - const previousForgePrStatusPollKey = this.getForgePrStatusPollKey(target); + const previousForgePrAssociationKey = this.getForgePrAssociationKey(target); const baseContext: CheckoutContext = { paseoHome: this.paseoHome, worktreesRoot: this.worktreesRoot, @@ -3616,7 +3616,7 @@ export class WorkspaceGitServiceImpl implements WorkspaceGitService { const loadedAtMs = this.deps.now().getTime(); target.latestGitLoadedAtMs = loadedAtMs; - if (previousForgePrStatusPollKey !== this.getForgePrStatusPollKey(target)) { + if (previousForgePrAssociationKey !== this.getForgePrAssociationKey(target)) { target.latestForge = buildForgeUnavailableSnapshot(); target.latestForgeLoadedAtMs = target.latestGitLoadedAtMs; } @@ -3682,7 +3682,7 @@ export class WorkspaceGitServiceImpl implements WorkspaceGitService { }; } - private getForgePrStatusPollKey(target: WorkspaceGitTarget): string | null { + private getForgePrAssociationKey(target: WorkspaceGitTarget): string | null { const git = target.latestGit; if (!git?.currentBranch || !git.remoteUrl) { return null; @@ -3698,7 +3698,7 @@ export class WorkspaceGitServiceImpl implements WorkspaceGitService { return null; } - return buildWorkspaceForgePrStatusPollKey({ + return buildWorkspaceForgePrAssociationKey({ forge: resolution.forge, remoteUrl: git.remoteUrl, target: pollTarget, @@ -4188,12 +4188,26 @@ function buildWorkspaceForgePrStatusPollKey({ forge: string; remoteUrl: string; target: WorkspaceForgePrStatusPollTarget; +}): string { + return JSON.stringify([ + buildWorkspaceForgePrAssociationKey({ forge, remoteUrl, target }), + target.headSha ?? null, + ]); +} + +function buildWorkspaceForgePrAssociationKey({ + forge, + remoteUrl, + target, +}: { + forge: string; + remoteUrl: string; + target: WorkspaceForgePrStatusPollTarget; }): string { return JSON.stringify([ forge, remoteUrl, target.headRef, - target.headSha ?? null, target.headRepositoryOwner ?? null, target.headRepositoryPath ?? null, target.headRepositoryHost ?? null, diff --git a/packages/server/src/services/gitea-service.test.ts b/packages/server/src/services/gitea-service.test.ts index c09598fd77..05cc027307 100644 --- a/packages/server/src/services/gitea-service.test.ts +++ b/packages/server/src/services/gitea-service.test.ts @@ -48,6 +48,8 @@ function makeService(responder: Responder, overrides: Partial { expect(calls.filter((args) => args[1] === "repos/example-user/sample-repo")).toHaveLength(2); }); + it.each(["open", "merged"] as const)("matches a transferred fork for an %s PR", async (state) => { + const original = currentPullRequestApi({ + number: 8, + state: state === "open" ? "open" : "closed", + headRef: "topic", + headSha: "1".repeat(40), + merged: state === "merged", + }); + const pr = { + ...original, + html_url: "https://gitea.example/base/repo/pulls/8", + head: { + ...original.head, + repo: { + ...original.head.repo, + id: 2, + full_name: "new-owner/repo", + owner: { login: "new-owner" }, + }, + }, + base: { ...original.base, repo: { id: 1, owner: { login: "base" } } }, + }; + const { service } = makeService( + (args) => { + if (args[0] === "pr" && args[1] === "list") + return ok( + JSON.stringify( + state === "open" + ? [{ ...OPEN_PR, index: "8", head: "new-owner:topic", url: pr.html_url }] + : [], + ), + ); + if (args[0] === "api" && args[1].endsWith("/pulls/8")) return ok(JSON.stringify(pr)); + if (args[0] === "api" && args[1] === "repos/old-owner/repo") + return ok(JSON.stringify({ full_name: "new-owner/repo" })); + if (args[0] === "api" && args[1].includes("pulls?state=all")) + return ok(JSON.stringify([pr])); + if (args[0] === "pr" && args[1] === "8") return ok(JSON.stringify(STATUS_PR_VIEW)); + if (args[0] === "api" && args[1].includes("/commits/")) + return ok(JSON.stringify({ state: "", statuses: [] })); + throw new Error(`unexpected call: ${args.join(" ")}`); + }, + { resolveRemoteUrl: async () => "https://gitea.example/base/repo.git" }, + ); + expect( + await service.getCurrentPullRequestStatus({ + cwd: "/repo", + headRef: "topic", + headRepositoryOwner: "old-owner", + headRepositoryPath: "old-owner/repo", + }), + ).toMatchObject({ number: 8, state }); + }); + it.each(["merged", "open"] as const)( "finds a recent %s candidate after 200 unrelated open requests", async (state) => { diff --git a/packages/server/src/services/gitea-service.ts b/packages/server/src/services/gitea-service.ts index de3a1f74c6..32879e3b9c 100644 --- a/packages/server/src/services/gitea-service.ts +++ b/packages/server/src/services/gitea-service.ts @@ -1682,6 +1682,15 @@ export function createGiteaService(options: CreateGiteaServiceOptions = {}): For canonicalRepositoryPaths.set(key, { path: result }); return result; }; + const matchesHead = async (item: GiteaPrListItem): Promise => { + if (matchesCurrentHeadRef(item, input.headRef, expectedHeadOwner)) return true; + if (stripHeadOwner(item.head) !== input.headRef) return false; + canonicalLookup ??= loadCanonicalPath(); + const canonicalPath = await canonicalLookup; + return canonicalPath + ? matchesCurrentHeadRef(item, input.headRef, canonicalPath.split("/")[0] ?? null) + : false; + }; const matchesRepository = async (item: GiteaCurrentPullRequestApi): Promise => { const headPath = item.head.repo?.full_name ?? @@ -1709,7 +1718,7 @@ export function createGiteaService(options: CreateGiteaServiceOptions = {}): For }; const open = await findOpenPullRequestForHeadRef( input, - expectedHeadOwner, + matchesHead, repoPath, matchesRepository, expectedPath, @@ -1720,7 +1729,7 @@ export function createGiteaService(options: CreateGiteaServiceOptions = {}): For // Gitea does not expose a source-branch filter for this endpoint. return resolveRecentPullRequest({ ...input, - expectedHeadOwner, + matchesHead, repoPath, matchesRepository, exhausted: open.exhausted, @@ -1731,7 +1740,7 @@ export function createGiteaService(options: CreateGiteaServiceOptions = {}): For cwd: string; headRef: string; previousPullRequest?: { number?: number; url: string; state: string } | null; - expectedHeadOwner: string | null; + matchesHead: (item: GiteaPrListItem) => Promise; repoPath: string; matchesRepository: (item: GiteaCurrentPullRequestApi) => Promise; exhausted: boolean; @@ -1740,7 +1749,7 @@ export function createGiteaService(options: CreateGiteaServiceOptions = {}): For try { recent = await findRecentPullRequestForHeadRef( input, - input.expectedHeadOwner, + input.matchesHead, input.repoPath, input.matchesRepository, ); @@ -1771,7 +1780,7 @@ export function createGiteaService(options: CreateGiteaServiceOptions = {}): For cwd: string; headRef: string; repoPath: string; - expectedHeadOwner: string | null; + matchesHead: (item: GiteaPrListItem) => Promise; matchesRepository: (item: GiteaCurrentPullRequestApi) => Promise; previousPullRequest?: { number?: number; url: string; state: string } | null; recent: GiteaPrListItem | null; @@ -1794,7 +1803,7 @@ export function createGiteaService(options: CreateGiteaServiceOptions = {}): For ); const candidate = currentPullRequestApiToListItem(current); return candidate.url === previous.url && - matchesCurrentHeadRef(candidate, input.headRef, input.expectedHeadOwner) && + (await input.matchesHead(candidate)) && (await input.matchesRepository(current)) ? candidate : null; @@ -1802,7 +1811,7 @@ export function createGiteaService(options: CreateGiteaServiceOptions = {}): For async function findRecentPullRequestForHeadRef( input: { cwd: string; headRef: string }, - expectedHeadOwner: string | null, + matchesHead: (item: GiteaPrListItem) => Promise, repoPath: string, matchesRepository: (item: GiteaCurrentPullRequestApi) => Promise, ): Promise { @@ -1820,11 +1829,7 @@ export function createGiteaService(options: CreateGiteaServiceOptions = {}): For const candidates: GiteaCurrentPullRequestApi[] = []; for (const item of items) { if ( - matchesCurrentHeadRef( - currentPullRequestApiToListItem(item), - input.headRef, - expectedHeadOwner, - ) && + (await matchesHead(currentPullRequestApiToListItem(item))) && (await matchesRepository(item)) ) candidates.push(item); @@ -1856,7 +1861,7 @@ export function createGiteaService(options: CreateGiteaServiceOptions = {}): For headSha?: string; force?: boolean; }, - expectedHeadOwner: string | null, + matchesHead: (item: GiteaPrListItem) => Promise, repoPath: string, matchesRepository: (item: GiteaCurrentPullRequestApi) => Promise, expectedPath: string, @@ -1871,7 +1876,7 @@ export function createGiteaService(options: CreateGiteaServiceOptions = {}): For page, }); for (const item of items) { - if (!matchesCurrentHeadRef(item, input.headRef, expectedHeadOwner)) continue; + if (!(await matchesHead(item))) continue; const identityKey = JSON.stringify([ input.cwd, repoPath,