From c7ddfd17d18aca4f120943e3f461ced7017c6e5f Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Sun, 17 May 2026 21:00:15 -0700 Subject: [PATCH] Improve git remote selection (#2207) --- src/main/github/client-pr-checks.test.ts | 23 ++ src/main/github/client.test.ts | 363 +++++++++++------- src/main/github/client.ts | 158 +++++--- src/main/github/gh-utils.test.ts | 53 +++ src/main/github/gh-utils.ts | 57 ++- .../work-item-details-file-viewed.test.ts | 9 +- src/main/github/work-item-details.ts | 4 +- src/main/ipc/github.ts | 49 ++- src/main/runtime/orca-runtime.ts | 17 +- src/main/runtime/rpc/methods/github.test.ts | 56 ++- src/main/runtime/rpc/methods/github.ts | 19 +- src/preload/api-types.ts | 5 + src/preload/index.ts | 4 + .../components/right-sidebar/ChecksPanel.tsx | 147 ++++--- .../components/right-sidebar/PRActions.tsx | 5 +- .../checks-panel-async-result-key.test.ts | 28 +- .../checks-panel-async-result-key.ts | 14 +- .../src/store/slices/github-checks.ts | 21 +- src/renderer/src/store/slices/github.test.ts | 162 +++++++- src/renderer/src/store/slices/github.ts | 59 ++- src/shared/types.ts | 6 +- 21 files changed, 969 insertions(+), 290 deletions(-) diff --git a/src/main/github/client-pr-checks.test.ts b/src/main/github/client-pr-checks.test.ts index 36ebd9459..31ba43346 100644 --- a/src/main/github/client-pr-checks.test.ts +++ b/src/main/github/client-pr-checks.test.ts @@ -174,4 +174,27 @@ describe('getPRChecks', () => { { cwd: '/repo-root', env: { ...process.env, GH_PROMPT_DISABLED: '1' } } ) }) + + it('uses explicit PR repo for check-runs and gh pr checks fallback', async () => { + getOwnerRepoMock.mockResolvedValueOnce({ owner: 'fork', repo: 'widgets' }) + ghExecFileAsyncMock + .mockRejectedValueOnce(new Error('gh: No commit found for SHA: stale-head (HTTP 422)')) + .mockResolvedValueOnce({ + stdout: JSON.stringify([{ name: 'lint', state: 'PASS', link: 'https://example.com/lint' }]) + }) + + await getPRChecks('/repo-root', 42, 'stale-head', { owner: 'acme', repo: 'widgets' }) + + expect(getOwnerRepoMock).not.toHaveBeenCalled() + expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( + 1, + ['api', '--cache', '60s', 'repos/acme/widgets/commits/stale-head/check-runs?per_page=100'], + { cwd: '/repo-root' } + ) + expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( + 2, + ['pr', 'checks', '42', '--json', 'name,state,link', '--repo', 'acme/widgets'], + { cwd: '/repo-root' } + ) + }) }) diff --git a/src/main/github/client.test.ts b/src/main/github/client.test.ts index ea832165d..4ea209b66 100644 --- a/src/main/github/client.test.ts +++ b/src/main/github/client.test.ts @@ -11,6 +11,7 @@ const { getOwnerRepoMock, getIssueOwnerRepoMock, getOwnerRepoForRemoteMock, + resolvePRRepositoryCandidatesMock, getRemoteUrlForRepoMock, gitExecFileAsyncMock, rateLimitGuardMock, @@ -25,6 +26,7 @@ const { getOwnerRepoMock: vi.fn(), getIssueOwnerRepoMock: vi.fn(), getOwnerRepoForRemoteMock: vi.fn(), + resolvePRRepositoryCandidatesMock: vi.fn(), getRemoteUrlForRepoMock: vi.fn(), gitExecFileAsyncMock: vi.fn(), rateLimitGuardMock: vi.fn<() => RateLimitGuardResult>(() => ({ blocked: false })), @@ -46,6 +48,7 @@ vi.mock('./gh-utils', () => ({ getOwnerRepo: getOwnerRepoMock, getIssueOwnerRepo: getIssueOwnerRepoMock, getOwnerRepoForRemote: getOwnerRepoForRemoteMock, + resolvePRRepositoryCandidates: resolvePRRepositoryCandidatesMock, getRemoteUrlForRepo: getRemoteUrlForRepoMock, gitExecFileAsync: gitExecFileAsyncMock, ghRepoExecOptions: ghRepoExecOptionsMock, @@ -82,7 +85,9 @@ import { getPRComments, getPRForBranch, getPullRequestPushTarget, + mergePR, resolveReviewThread, + updatePRTitle, _resetOwnerRepoCache } from './client' @@ -93,6 +98,11 @@ describe('getPRForBranch', () => { getOwnerRepoMock.mockReset() getIssueOwnerRepoMock.mockReset() getOwnerRepoForRemoteMock.mockReset() + resolvePRRepositoryCandidatesMock.mockReset() + resolvePRRepositoryCandidatesMock.mockImplementation(async (repoPath, connectionId) => { + const origin = await getOwnerRepoMock(repoPath, connectionId) + return { candidates: origin ? [origin] : [], headRepo: origin } + }) getRemoteUrlForRepoMock.mockReset() gitExecFileAsyncMock.mockReset() rateLimitGuardMock.mockReset() @@ -113,16 +123,13 @@ describe('getPRForBranch', () => { { number: 42, title: 'Fix PR discovery', - state: 'OPEN', - url: 'https://github.com/acme/widgets/pull/42', - statusCheckRollup: [], - updatedAt: '2026-03-28T00:00:00Z', - isDraft: false, - mergeable: 'MERGEABLE', - baseRefName: 'main', - headRefName: 'feature/test', - baseRefOid: 'base-oid', - headRefOid: 'head-oid' + state: 'open', + html_url: 'https://github.com/acme/widgets/pull/42', + updated_at: '2026-03-28T00:00:00Z', + draft: false, + mergeable: true, + base: { ref: 'main', sha: 'base-oid' }, + head: { ref: 'feature/test', sha: 'head-oid' } } ]) }) @@ -131,25 +138,110 @@ describe('getPRForBranch', () => { expect(getOwnerRepoMock).toHaveBeenCalledWith('/repo-root', undefined) expect(ghExecFileAsyncMock).toHaveBeenCalledWith( - [ - 'pr', - 'list', - '--repo', - 'acme/widgets', - '--head', - 'feature/test', - '--state', - 'all', - '--limit', - '1', - '--json', - 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,baseRefName,headRefName,baseRefOid,headRefOid' - ], + ['api', 'repos/acme/widgets/pulls?head=acme%3Afeature%2Ftest&state=all&per_page=1'], { cwd: '/repo-root' } ) expect(pr?.number).toBe(42) expect(pr?.state).toBe('open') expect(pr?.mergeable).toBe('MERGEABLE') + expect(pr?.prRepo).toEqual({ owner: 'acme', repo: 'widgets' }) + expect(pr?.headRepo).toEqual({ owner: 'acme', repo: 'widgets' }) + }) + + it('resolves fork PRs from the upstream PR repo with the origin head owner', async () => { + resolvePRRepositoryCandidatesMock.mockResolvedValueOnce({ + candidates: [ + { owner: 'stablyai', repo: 'orca' }, + { owner: 'fork', repo: 'orca' } + ], + headRepo: { owner: 'fork', repo: 'orca' } + }) + ghExecFileAsyncMock.mockResolvedValueOnce({ + stdout: JSON.stringify([ + { + number: 1738, + title: 'Fork PR', + state: 'open', + html_url: 'https://github.com/stablyai/orca/pull/1738', + updated_at: '2026-03-28T00:00:00Z', + draft: false, + mergeable_state: 'clean', + base: { ref: 'main', sha: 'base-oid' }, + head: { ref: 'feature/test', sha: 'head-oid' } + } + ]) + }) + + const pr = await getPRForBranch('/repo-root', 'feature/test') + + expect(ghExecFileAsyncMock).toHaveBeenCalledWith( + ['api', 'repos/stablyai/orca/pulls?head=fork%3Afeature%2Ftest&state=all&per_page=1'], + { cwd: '/repo-root' } + ) + expect(pr).toMatchObject({ + number: 1738, + prRepo: { owner: 'stablyai', repo: 'orca' }, + headRepo: { owner: 'fork', repo: 'orca' } + }) + }) + + it('looks up a linked PR number across PR repo candidates', async () => { + resolvePRRepositoryCandidatesMock.mockResolvedValueOnce({ + candidates: [ + { owner: 'stablyai', repo: 'orca' }, + { owner: 'fork', repo: 'orca' } + ], + headRepo: { owner: 'fork', repo: 'orca' } + }) + gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: 'linked-head-oid\n', stderr: '' }) + ghExecFileAsyncMock + .mockRejectedValueOnce(new Error('HTTP 404: Not Found')) + .mockResolvedValueOnce({ + stdout: JSON.stringify({ + number: 99, + title: 'Linked fork PR', + state: 'OPEN', + url: 'https://github.com/fork/orca/pull/99', + statusCheckRollup: [], + updatedAt: '2026-03-28T00:00:00Z', + isDraft: false, + mergeable: 'MERGEABLE', + baseRefName: 'main', + headRefName: 'feature/test', + baseRefOid: 'base-oid', + headRefOid: 'linked-head-oid' + }) + }) + + const pr = await getPRForBranch('/repo-root', 'feature/test', 99) + + expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( + 1, + [ + 'pr', + 'view', + '99', + '--repo', + 'stablyai/orca', + '--json', + 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,baseRefName,headRefName,baseRefOid,headRefOid' + ], + { cwd: '/repo-root' } + ) + expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( + 2, + [ + 'pr', + 'view', + '99', + '--repo', + 'fork/orca', + '--json', + 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,baseRefName,headRefName,baseRefOid,headRefOid' + ], + { cwd: '/repo-root' } + ) + expect(pr?.prRepo).toEqual({ owner: 'fork', repo: 'orca' }) }) it('prefers exact linked PR lookup when the repo identity is known', async () => { @@ -242,20 +334,7 @@ describe('getPRForBranch', () => { expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(2) expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( 2, - [ - 'pr', - 'list', - '--repo', - 'acme/widgets', - '--head', - 'feature/test', - '--state', - 'all', - '--limit', - '1', - '--json', - 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,baseRefName,headRefName,baseRefOid,headRefOid' - ], + ['api', 'repos/acme/widgets/pulls?head=acme%3Afeature%2Ftest&state=all&per_page=1'], { cwd: '/repo-root' } ) expect(pr?.number).toBe(42) @@ -301,20 +380,7 @@ describe('getPRForBranch', () => { ) expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( 2, - [ - 'pr', - 'list', - '--repo', - 'acme/widgets', - '--head', - 'feature/test', - '--state', - 'all', - '--limit', - '1', - '--json', - 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,baseRefName,headRefName,baseRefOid,headRefOid' - ], + ['api', 'repos/acme/widgets/pulls?head=acme%3Afeature%2Ftest&state=all&per_page=1'], { cwd: '/repo-root' } ) expect(pr?.number).toBe(42) @@ -351,20 +417,7 @@ describe('getPRForBranch', () => { }) expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( 3, - [ - 'pr', - 'list', - '--repo', - 'acme/widgets', - '--head', - 'feature/test', - '--state', - 'all', - '--limit', - '1', - '--json', - 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,baseRefName,headRefName,baseRefOid,headRefOid' - ], + ['api', 'repos/acme/widgets/pulls?head=acme%3Afeature%2Ftest&state=all&per_page=1'], { cwd: '/repo-root' } ) expect(pr?.number).toBe(42) @@ -401,26 +454,13 @@ describe('getPRForBranch', () => { }) expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( 3, - [ - 'pr', - 'list', - '--repo', - 'acme/widgets', - '--head', - 'feature/test', - '--state', - 'all', - '--limit', - '1', - '--json', - 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,baseRefName,headRefName,baseRefOid,headRefOid' - ], + ['api', 'repos/acme/widgets/pulls?head=acme%3Afeature%2Ftest&state=all&per_page=1'], { cwd: '/repo-root' } ) expect(pr?.number).toBe(42) }) - it('does not spend branch discovery calls when exact linked PR REST fallback is rate limited', async () => { + it('continues to branch discovery when exact linked PR REST fallback is rate limited', async () => { getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) ghExecFileAsyncMock .mockRejectedValueOnce(new Error('GraphQL: API rate limit already exceeded')) @@ -444,34 +484,37 @@ describe('getPRForBranch', () => { expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith(2, ['api', 'repos/acme/widgets/pulls/99'], { cwd: '/repo-root' }) - expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(2) + expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( + 3, + ['api', 'repos/acme/widgets/pulls?head=acme%3Afeature%2Ftest&state=all&per_page=1'], + { cwd: '/repo-root' } + ) + expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(3) expect(pr).toBeNull() }) - it('falls back to REST branch lookup when gh pr list is GraphQL rate limited', async () => { + it('uses REST branch lookup directly when origin head repo is known', async () => { getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) - ghExecFileAsyncMock - .mockRejectedValueOnce(new Error('GraphQL: API rate limit already exceeded')) - .mockResolvedValueOnce({ - stdout: JSON.stringify([ - { - number: 43, - title: 'REST branch lookup', - state: 'open', - html_url: 'https://github.com/acme/widgets/pull/43', - updated_at: '2026-03-28T00:00:00Z', - draft: false, - mergeable: true, - head: { ref: 'feature/test', sha: 'rest-head-oid' }, - base: { ref: 'main', sha: 'rest-base-oid' } - } - ]) - }) + ghExecFileAsyncMock.mockResolvedValueOnce({ + stdout: JSON.stringify([ + { + number: 43, + title: 'REST branch lookup', + state: 'open', + html_url: 'https://github.com/acme/widgets/pull/43', + updated_at: '2026-03-28T00:00:00Z', + draft: false, + mergeable: true, + head: { ref: 'feature/test', sha: 'rest-head-oid' }, + base: { ref: 'main', sha: 'rest-base-oid' } + } + ]) + }) const pr = await getPRForBranch('/repo-root', 'feature/test') expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( - 2, + 1, ['api', 'repos/acme/widgets/pulls?head=acme%3Afeature%2Ftest&state=all&per_page=1'], { cwd: '/repo-root' } ) @@ -529,16 +572,13 @@ describe('getPRForBranch', () => { { number: 42, title: 'Fix PR discovery', - state: 'OPEN', - url: 'https://github.com/acme/widgets/pull/42', - statusCheckRollup: [], - updatedAt: '2026-03-28T00:00:00Z', - isDraft: false, - mergeable: 'CONFLICTING', - baseRefName: 'main', - headRefName: 'feature/test', - baseRefOid: 'base-oid', - headRefOid: 'head-oid' + state: 'open', + html_url: 'https://github.com/acme/widgets/pull/42', + updated_at: '2026-03-28T00:00:00Z', + draft: false, + mergeable_state: 'dirty', + base: { ref: 'main', sha: 'base-oid' }, + head: { ref: 'feature/test', sha: 'head-oid' } } ]) }) @@ -566,16 +606,13 @@ describe('getPRForBranch', () => { { number: 42, title: 'Fix PR discovery', - state: 'OPEN', - url: 'https://github.com/acme/widgets/pull/42', - statusCheckRollup: [], - updatedAt: '2026-03-28T00:00:00Z', - isDraft: false, - mergeable: 'CONFLICTING', - baseRefName: 'main', - headRefName: 'feature/test', - baseRefOid: 'base-oid', - headRefOid: 'head-oid' + state: 'open', + html_url: 'https://github.com/acme/widgets/pull/42', + updated_at: '2026-03-28T00:00:00Z', + draft: false, + mergeable_state: 'dirty', + base: { ref: 'main', sha: 'base-oid' }, + head: { ref: 'feature/test', sha: 'head-oid' } } ]) }) @@ -600,16 +637,13 @@ describe('getPRForBranch', () => { { number: 42, title: 'Fix PR discovery', - state: 'OPEN', - url: 'https://github.com/acme/widgets/pull/42', - statusCheckRollup: [], - updatedAt: '2026-03-28T00:00:00Z', - isDraft: false, - mergeable: 'CONFLICTING', - baseRefName: 'main', - headRefName: 'feature/test', - baseRefOid: 'base-oid', - headRefOid: 'head-oid' + state: 'open', + html_url: 'https://github.com/acme/widgets/pull/42', + updated_at: '2026-03-28T00:00:00Z', + draft: false, + mergeable_state: 'dirty', + base: { ref: 'main', sha: 'base-oid' }, + head: { ref: 'feature/test', sha: 'head-oid' } } ]) }) @@ -768,6 +802,11 @@ describe('GitHub GraphQL rate-limit guard', () => { getOwnerRepoMock.mockReset() getIssueOwnerRepoMock.mockReset() getOwnerRepoForRemoteMock.mockReset() + resolvePRRepositoryCandidatesMock.mockReset() + resolvePRRepositoryCandidatesMock.mockImplementation(async (repoPath, connectionId) => { + const origin = await getOwnerRepoMock(repoPath, connectionId) + return { candidates: origin ? [origin] : [], headRepo: origin } + }) getRemoteUrlForRepoMock.mockReset() gitExecFileAsyncMock.mockReset() rateLimitGuardMock.mockReset() @@ -810,6 +849,68 @@ describe('GitHub GraphQL rate-limit guard', () => { expect(noteRateLimitSpendMock).not.toHaveBeenCalled() }) + it('uses explicit PR repo for comments when a fork PR is discovered', async () => { + rateLimitGuardMock.mockReturnValue({ + blocked: true, + remaining: 4, + limit: 5000, + resetAt: 1_800_000_000 + }) + ghExecFileAsyncMock + .mockResolvedValueOnce({ + stdout: JSON.stringify([ + { + id: 10, + user: { login: 'octo', avatar_url: 'https://avatar', type: 'User' }, + body: 'top-level', + created_at: '2026-04-01T00:00:00Z', + html_url: 'https://github.com/stablyai/orca/pull/7#issuecomment-10' + } + ]) + }) + .mockResolvedValueOnce({ stdout: '[]' }) + + await getPRComments('/repo-root', 7, { prRepo: { owner: 'stablyai', repo: 'orca' } }, undefined) + + expect(getOwnerRepoMock).not.toHaveBeenCalled() + expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( + 1, + ['api', '--cache', '60s', 'repos/stablyai/orca/issues/7/comments?per_page=100'], + { cwd: '/repo-root' } + ) + expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( + 2, + ['api', '--cache', '60s', 'repos/stablyai/orca/pulls/7/reviews?per_page=100'], + { cwd: '/repo-root' } + ) + }) + + it('uses explicit PR repo for merge and title mutations', async () => { + ghExecFileAsyncMock.mockResolvedValue({ stdout: '', stderr: '' }) + + await expect( + mergePR('/repo-root', 7, 'squash', undefined, { owner: 'stablyai', repo: 'orca' }) + ).resolves.toEqual({ ok: true }) + await expect( + updatePRTitle('/repo-root', 7, 'New title', undefined, { owner: 'stablyai', repo: 'orca' }) + ).resolves.toBe(true) + + expect(getOwnerRepoMock).not.toHaveBeenCalled() + expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( + 1, + ['pr', 'merge', '7', '--squash', '--repo', 'stablyai/orca'], + expect.objectContaining({ + cwd: '/repo-root', + env: expect.objectContaining({ GH_PROMPT_DISABLED: '1' }) + }) + ) + expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( + 2, + ['pr', 'edit', '7', '--title', 'New title', '--repo', 'stablyai/orca'], + { cwd: '/repo-root' } + ) + }) + it('blocks review-thread resolve mutations before spawning gh when GraphQL is low', async () => { rateLimitGuardMock.mockReturnValue({ blocked: true, diff --git a/src/main/github/client.ts b/src/main/github/client.ts index 4490933b1..5c93a5052 100644 --- a/src/main/github/client.ts +++ b/src/main/github/client.ts @@ -37,6 +37,7 @@ import { getOwnerRepo, getIssueOwnerRepo, getOwnerRepoForRemote, + resolvePRRepositoryCandidates, resolveIssueSource, classifyGhError, classifyListIssuesError, @@ -1451,13 +1452,14 @@ function mapRestPullRequest(pr: RestPullRequest): PullRequestLookupData { } async function getRestPRForBranch( - ownerRepo: OwnerRepo, + prRepo: OwnerRepo, + headOwner: string, branchName: string, ghOptions: ReturnType ): Promise { - const head = encodeURIComponent(`${ownerRepo.owner}:${branchName}`) + const head = encodeURIComponent(`${headOwner}:${branchName}`) const { stdout } = await ghExecFileAsync( - ['api', `repos/${ownerRepo.owner}/${ownerRepo.repo}/pulls?head=${head}&state=all&per_page=1`], + ['api', `repos/${prRepo.owner}/${prRepo.repo}/pulls?head=${head}&state=all&per_page=1`], ghOptions ) const list = JSON.parse(stdout) as RestPullRequest[] @@ -1465,6 +1467,32 @@ async function getRestPRForBranch( return pr ? mapRestPullRequest(pr) : null } +async function getFallbackPRListForBranch( + prRepo: OwnerRepo, + branchName: string, + ghOptions: ReturnType +): Promise { + const { stdout } = await ghExecFileAsync( + [ + 'pr', + 'list', + '--repo', + `${prRepo.owner}/${prRepo.repo}`, + '--head', + branchName, + '--state', + 'all', + '--limit', + '1', + '--json', + PR_LOOKUP_JSON_FIELDS + ], + ghOptions + ) + const list = JSON.parse(stdout) as PullRequestLookupData[] + return list[0] ?? null +} + async function getRestPRByNumber( ownerRepo: OwnerRepo, number: number, @@ -1522,18 +1550,15 @@ async function exactPRMatchesWorktreeHead( data: PullRequestLookupData, connectionId?: string | null ): Promise { - if (!branchName || data.headRefName === branchName) { - return true - } - if (connectionId || !data.headRefOid) { - return false - } - try { - const { stdout } = await gitExecFileAsync(['rev-parse', 'HEAD'], { cwd: repoPath }) - return stdout.trim() === data.headRefOid - } catch { - return false + if (!connectionId && data.headRefOid) { + try { + const { stdout } = await gitExecFileAsync(['rev-parse', 'HEAD'], { cwd: repoPath }) + return stdout.trim() === data.headRefOid + } catch { + return false + } } + return !branchName || data.headRefName === branchName } function isNotFoundGhError(err: unknown): boolean { @@ -1564,22 +1589,39 @@ export async function getPRForBranch( ): Promise { // Strip refs/heads/ prefix if present const branchName = branch.replace(/^refs\/heads\//, '') + if (!branchName && typeof linkedPRNumber !== 'number') { + return null + } const context = githubRepoContext(repoPath, connectionId) const ghOptions = ghRepoExecOptions(context) await acquire() try { - const ownerRepo = await getOwnerRepo(repoPath, connectionId) + const { candidates, headRepo } = await resolvePRRepositoryCandidates(repoPath, connectionId) let data: PullRequestLookupData | null = null + let dataRepo: OwnerRepo | null = null let exactLinkedData: PullRequestLookupData | null = null + let exactLinkedRepo: OwnerRepo | null = null - if (ownerRepo && typeof linkedPRNumber === 'number') { - data = await getPRByNumber(ownerRepo, linkedPRNumber, ghOptions) - if (data && !(await exactPRMatchesWorktreeHead(repoPath, branchName, data, connectionId))) { - // Why: linked PR metadata is user-editable. If the stored number still - // resolves but no longer matches this worktree, let branch lookup correct it. - exactLinkedData = data - data = null + if (typeof linkedPRNumber === 'number') { + for (const candidate of candidates) { + try { + const linkedData = await getPRByNumber(candidate, linkedPRNumber, ghOptions) + if (!linkedData) { + continue + } + if (await exactPRMatchesWorktreeHead(repoPath, branchName, linkedData, connectionId)) { + data = linkedData + dataRepo = candidate + break + } + // Why: linked PR metadata is user-editable. If the stored number still + // resolves but no longer matches this worktree, let branch lookup correct it. + exactLinkedData ??= linkedData + exactLinkedRepo ??= candidate + } catch { + // Candidate probing is best-effort; another repo may own the PR. + } } } @@ -1587,33 +1629,28 @@ export async function getPRForBranch( // An empty --head filter causes gh to return an arbitrary PR — skip the // branch lookup and rely on the linkedPR fallback below if available. if (!data && branchName) { - if (ownerRepo) { - try { - const { stdout } = await ghExecFileAsync( - [ - 'pr', - 'list', - '--repo', - `${ownerRepo.owner}/${ownerRepo.repo}`, - '--head', - branchName, - '--state', - 'all', - '--limit', - '1', - '--json', - PR_LOOKUP_JSON_FIELDS - ], - ghOptions - ) - const list = JSON.parse(stdout) as PullRequestLookupData[] - data = list[0] ?? null - } catch (err) { - // Why: `gh pr list/view` uses GraphQL and can hit that quota while - // REST is still available. Falling back prevents a real PR from - // rendering as "No pull request found" during GraphQL outages. - if (!isNotFoundGhError(err)) { - data = await getRestPRForBranch(ownerRepo, branchName, ghOptions) + if (candidates.length > 0) { + for (const candidate of candidates) { + try { + data = headRepo + ? await getRestPRForBranch(candidate, headRepo.owner, branchName, ghOptions) + : await getFallbackPRListForBranch(candidate, branchName, ghOptions) + if (data) { + dataRepo = candidate + break + } + } catch (err) { + if (!headRepo && !isNotFoundGhError(err)) { + try { + data = await getRestPRForBranch(candidate, candidate.owner, branchName, ghOptions) + if (data) { + dataRepo = candidate + break + } + } catch { + // Continue to the next candidate below. + } + } } } } else { @@ -1625,7 +1662,7 @@ export async function getPRForBranch( } } - if (!data && !ownerRepo && typeof linkedPRNumber === 'number') { + if (!data && candidates.length === 0 && typeof linkedPRNumber === 'number') { const args = ['pr', 'view', String(linkedPRNumber), '--json', PR_LOOKUP_JSON_FIELDS] try { const { stdout } = await ghExecFileAsync(args, ghOptions) @@ -1641,6 +1678,7 @@ export async function getPRForBranch( if (!data && exactLinkedData) { data = exactLinkedData + dataRepo = exactLinkedRepo } if (!data) { @@ -1665,6 +1703,8 @@ export async function getPRForBranch( updatedAt: data.updatedAt, mergeable: (data.mergeable as PRMergeableState) ?? 'UNKNOWN', headSha: data.headRefOid, + prRepo: dataRepo ?? undefined, + headRepo: headRepo ?? undefined, conflictSummary } } catch { @@ -1683,11 +1723,12 @@ export async function getPRChecks( repoPath: string, prNumber: number, headSha?: string, + prRepo?: OwnerRepo | null, options?: { noCache?: boolean }, connectionId?: string | null ): Promise { const ghOptions = ghRepoExecOptions(githubRepoContext(repoPath, connectionId)) - const ownerRepo = await getOwnerRepo(repoPath, connectionId) + const ownerRepo = prRepo ?? (await getOwnerRepo(repoPath, connectionId)) const fallbackToPRChecks = async (): Promise => { const fallbackArgs = ['pr', 'checks', String(prNumber), '--json', 'name,state,link'] if (ownerRepo) { @@ -1784,6 +1825,7 @@ export async function rerunPRChecks( repoPath, prNumber, options.headSha, + ownerRepo, { noCache: true }, connectionId ) @@ -1908,11 +1950,11 @@ query($owner: String!, $repo: String!, $pr: Int!) { export async function getPRComments( repoPath: string, prNumber: number, - options?: { noCache?: boolean }, + options?: { noCache?: boolean; prRepo?: OwnerRepo | null }, connectionId?: string | null ): Promise { const ghOptions = ghRepoExecOptions(githubRepoContext(repoPath, connectionId)) - const ownerRepo = await getOwnerRepo(repoPath, connectionId) + const ownerRepo = options?.prRepo ?? (await getOwnerRepo(repoPath, connectionId)) await acquire() try { if (ownerRepo) { @@ -2345,10 +2387,11 @@ export async function mergePR( repoPath: string, prNumber: number, method: 'merge' | 'squash' | 'rebase' = 'squash', - connectionId?: string | null + connectionId?: string | null, + prRepo?: OwnerRepo | null ): Promise<{ ok: true } | { ok: false; error: string }> { const ghOptions = ghRepoExecOptions(githubRepoContext(repoPath, connectionId)) - const ownerRepo = await getOwnerRepo(repoPath, connectionId) + const ownerRepo = prRepo ?? (await getOwnerRepo(repoPath, connectionId)) await acquire() try { // Don't use --delete-branch: it tries to delete the local branch which @@ -2447,10 +2490,11 @@ export async function updatePRTitle( repoPath: string, prNumber: number, title: string, - connectionId?: string | null + connectionId?: string | null, + prRepo?: OwnerRepo | null ): Promise { const ghOptions = ghRepoExecOptions(githubRepoContext(repoPath, connectionId)) - const ownerRepo = await getOwnerRepo(repoPath, connectionId) + const ownerRepo = prRepo ?? (await getOwnerRepo(repoPath, connectionId)) await acquire() try { const args = ['pr', 'edit', String(prNumber), '--title', title] diff --git a/src/main/github/gh-utils.test.ts b/src/main/github/gh-utils.test.ts index 41ce4f465..54646ffbe 100644 --- a/src/main/github/gh-utils.test.ts +++ b/src/main/github/gh-utils.test.ts @@ -20,8 +20,10 @@ import { classifyListIssuesError, getIssueOwnerRepo, getOwnerRepo, + getOwnerRepoForRemote, parseGitHubRemoteIdentity, parseGitHubOwnerRepo, + resolvePRRepositoryCandidates, resolveIssueSource } from './gh-utils' @@ -136,6 +138,57 @@ describe('github owner/repo resolution', () => { await expect(getOwnerRepo('/repo')).resolves.toEqual({ owner: 'local', repo: 'orca' }) await expect(getOwnerRepo('/repo', 'ssh-1')).resolves.toEqual({ owner: 'remote', repo: 'orca' }) }) + + it('resolves PR candidates as upstream then origin and de-dupes matching slugs', async () => { + gitExecFileAsyncMock + .mockResolvedValueOnce({ stdout: 'git@github.com:Acme/Orca.git\n' }) + .mockResolvedValueOnce({ stdout: 'git@github.com:acme/orca.git\n' }) + + await expect(resolvePRRepositoryCandidates('/repo')).resolves.toEqual({ + candidates: [{ owner: 'Acme', repo: 'Orca' }], + headRepo: { owner: 'acme', repo: 'orca' } + }) + }) + + it('ignores non-GitHub upstream while keeping origin as the head repo', async () => { + gitExecFileAsyncMock + .mockResolvedValueOnce({ stdout: 'git@example.com:Acme/Orca.git\n' }) + .mockResolvedValueOnce({ stdout: 'git@github.com:fork/orca.git\n' }) + + await expect(resolvePRRepositoryCandidates('/repo')).resolves.toEqual({ + candidates: [{ owner: 'fork', repo: 'orca' }], + headRepo: { owner: 'fork', repo: 'orca' } + }) + }) + + it('expires cached remote owner/repo entries after the TTL', async () => { + vi.useFakeTimers() + try { + gitExecFileAsyncMock + .mockResolvedValueOnce({ stdout: 'git@github.com:old/orca.git\n' }) + .mockResolvedValueOnce({ stdout: 'git@github.com:new/orca.git\n' }) + + await expect(getOwnerRepoForRemote('/repo', 'origin')).resolves.toEqual({ + owner: 'old', + repo: 'orca' + }) + await expect(getOwnerRepoForRemote('/repo', 'origin')).resolves.toEqual({ + owner: 'old', + repo: 'orca' + }) + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(1) + + await vi.advanceTimersByTimeAsync(30_001) + + await expect(getOwnerRepoForRemote('/repo', 'origin')).resolves.toEqual({ + owner: 'new', + repo: 'orca' + }) + expect(gitExecFileAsyncMock).toHaveBeenCalledTimes(2) + } finally { + vi.useRealTimers() + } + }) }) describe('resolveIssueSource', () => { diff --git a/src/main/github/gh-utils.ts b/src/main/github/gh-utils.ts index bcff859ad..1105f9006 100644 --- a/src/main/github/gh-utils.ts +++ b/src/main/github/gh-utils.ts @@ -137,7 +137,14 @@ export function ghRepoExecOptions(context: GitHubRepoContext): { return context.connectionId ? {} : { cwd: context.repoPath } } -const ownerRepoCache = new Map() +const OWNER_REPO_CACHE_TTL_MS = 30_000 + +type OwnerRepoCacheEntry = { + value: OwnerRepo | null + expiresAt: number +} + +const ownerRepoCache = new Map() /** @internal — exposed for tests only */ export function _resetOwnerRepoCache(): void { @@ -190,20 +197,27 @@ export async function getOwnerRepoForRemote( ): Promise { const context = githubRepoContext(repoPath, connectionId) const cacheKey = `${context.connectionId ?? 'local'}\0${context.repoPath}\0${remoteName}` - if (ownerRepoCache.has(cacheKey)) { - return ownerRepoCache.get(cacheKey)! + const cached = ownerRepoCache.get(cacheKey) + if (cached && cached.expiresAt > Date.now()) { + return cached.value + } + if (cached) { + ownerRepoCache.delete(cacheKey) } try { const remoteUrl = await getRemoteUrlForRepo(context, remoteName) const result = remoteUrl ? parseGitHubOwnerRepo(remoteUrl) : null if (result) { - ownerRepoCache.set(cacheKey, result) + ownerRepoCache.set(cacheKey, { + value: result, + expiresAt: Date.now() + OWNER_REPO_CACHE_TTL_MS + }) return result } } catch { // ignore — non-GitHub remote or no remote } - ownerRepoCache.set(cacheKey, null) + ownerRepoCache.set(cacheKey, { value: null, expiresAt: Date.now() + OWNER_REPO_CACHE_TTL_MS }) return null } @@ -225,6 +239,39 @@ export async function getIssueOwnerRepo( return getOwnerRepoForRemote(repoPath, 'origin', connectionId) } +export type PRRepositoryCandidates = { + candidates: OwnerRepo[] + headRepo: OwnerRepo | null +} + +function ownerRepoKey(ownerRepo: OwnerRepo): string { + return `${ownerRepo.owner.toLowerCase()}/${ownerRepo.repo.toLowerCase()}` +} + +export async function resolvePRRepositoryCandidates( + repoPath: string, + connectionId?: string | null +): Promise { + const upstream = await getOwnerRepoForRemote(repoPath, 'upstream', connectionId) + const origin = await getOwnerRepoForRemote(repoPath, 'origin', connectionId) + const seen = new Set() + const candidates: OwnerRepo[] = [] + + for (const candidate of [upstream, origin]) { + if (!candidate) { + continue + } + const key = ownerRepoKey(candidate) + if (seen.has(key)) { + continue + } + seen.add(key) + candidates.push(candidate) + } + + return { candidates, headRepo: origin } +} + export type ResolvedIssueSource = { source: OwnerRepo | null /** True when the user preferred `upstream` but the upstream remote is no diff --git a/src/main/github/work-item-details-file-viewed.test.ts b/src/main/github/work-item-details-file-viewed.test.ts index 0341899f0..f77ccc92e 100644 --- a/src/main/github/work-item-details-file-viewed.test.ts +++ b/src/main/github/work-item-details-file-viewed.test.ts @@ -158,6 +158,13 @@ describe('getWorkItemDetails PR file viewed state', () => { ['src/viewed.ts', 'VIEWED'], ['src/changed.ts', 'DISMISSED'] ]) - expect(getPRChecksMock).toHaveBeenCalledWith('/repo-root', 42, 'head-sha', undefined, undefined) + expect(getPRChecksMock).toHaveBeenCalledWith( + '/repo-root', + 42, + 'head-sha', + null, + undefined, + undefined + ) }) }) diff --git a/src/main/github/work-item-details.ts b/src/main/github/work-item-details.ts index 42e8624d1..0de4add61 100644 --- a/src/main/github/work-item-details.ts +++ b/src/main/github/work-item-details.ts @@ -741,8 +741,8 @@ export async function getWorkItemDetails( const [mentionParticipants, checks] = await Promise.all([ getMentionParticipants(repoPath, item, comments, participants, connectionId), shas?.headSha - ? getPRChecks(repoPath, item.number, shas.headSha, undefined, connectionId) - : getPRChecks(repoPath, item.number, undefined, undefined, connectionId) + ? getPRChecks(repoPath, item.number, shas.headSha, null, undefined, connectionId) + : getPRChecks(repoPath, item.number, undefined, null, undefined, connectionId) ]) return { diff --git a/src/main/ipc/github.ts b/src/main/ipc/github.ts index 23fba6f78..c526a4c49 100644 --- a/src/main/ipc/github.ts +++ b/src/main/ipc/github.ts @@ -4,7 +4,12 @@ reviewable as one surface. Splitting by feature area would risk drifting validation/gate conventions across handler files. */ import { ipcMain, webContents } from 'electron' import { resolve } from 'path' -import type { Repo, GitHubIssueUpdate, GitHubPullRequestStateUpdate } from '../../shared/types' +import type { + Repo, + GitHubIssueUpdate, + GitHubOwnerRepo, + GitHubPullRequestStateUpdate +} from '../../shared/types' import type { Store } from '../persistence' import type { StatsCollector } from '../stats/collector' import { @@ -278,6 +283,7 @@ export function registerGitHubHandlers(store: Store, stats: StatsCollector): voi repoPath: string prNumber: number headSha?: string + prRepo?: GitHubOwnerRepo | null noCache?: boolean } ) => { @@ -286,6 +292,7 @@ export function registerGitHubHandlers(store: Store, stats: StatsCollector): voi repo.path, args.prNumber, args.headSha, + args.prRepo ?? null, { noCache: args.noCache }, @@ -296,12 +303,20 @@ export function registerGitHubHandlers(store: Store, stats: StatsCollector): voi ipcMain.handle( 'gh:prComments', - (_event, args: { repoPath: string; prNumber: number; noCache?: boolean }) => { + ( + _event, + args: { + repoPath: string + prNumber: number + prRepo?: GitHubOwnerRepo | null + noCache?: boolean + } + ) => { const repo = assertRegisteredRepo(args, store) return getPRComments( repo.path, args.prNumber, - { noCache: args.noCache }, + { noCache: args.noCache, prRepo: args.prRepo ?? null }, repoConnectionId(repo) ) } @@ -478,9 +493,18 @@ export function registerGitHubHandlers(store: Store, stats: StatsCollector): voi ipcMain.handle( 'gh:updatePRTitle', - async (event, args: { repoPath: string; prNumber: number; title: string }) => { + async ( + event, + args: { repoPath: string; prNumber: number; title: string; prRepo?: GitHubOwnerRepo | null } + ) => { const repo = assertRegisteredRepo(args, store) - const ok = await updatePRTitle(repo.path, args.prNumber, args.title, repoConnectionId(repo)) + const ok = await updatePRTitle( + repo.path, + args.prNumber, + args.title, + repoConnectionId(repo), + args.prRepo ?? null + ) if (ok) { broadcastWorkItemMutated( { repoPath: repo.path, repoId: repo.id, type: 'pr', number: args.prNumber }, @@ -495,10 +519,21 @@ export function registerGitHubHandlers(store: Store, stats: StatsCollector): voi 'gh:mergePR', async ( event, - args: { repoPath: string; prNumber: number; method?: 'merge' | 'squash' | 'rebase' } + args: { + repoPath: string + prNumber: number + method?: 'merge' | 'squash' | 'rebase' + prRepo?: GitHubOwnerRepo | null + } ) => { const repo = assertRegisteredRepo(args, store) - const result = await mergePR(repo.path, args.prNumber, args.method, repoConnectionId(repo)) + const result = await mergePR( + repo.path, + args.prNumber, + args.method, + repoConnectionId(repo), + args.prRepo ?? null + ) if (result.ok) { broadcastWorkItemMutated( { repoPath: repo.path, repoId: repo.id, type: 'pr', number: args.prNumber }, diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index bd16cdf10..f9f50b828 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -19,6 +19,7 @@ import type { CreateWorktreeResult, GitPushTarget, GitWorktreeInfo, + GitHubOwnerRepo, GlobalSettings, Repo, StatsSummary, @@ -4711,11 +4712,12 @@ export class OrcaRuntimeService { repoSelector: string, prNumber: number, headSha?: string, + prRepo?: GitHubOwnerRepo | null, options?: { noCache?: boolean } ): Promise>> { const repo = await this.resolveRepoSelector(repoSelector) this.assertHostIntegrationRepoIsLocal(repo, 'repo_pr_checks') - return getPRChecks(repo.path, prNumber, headSha, options) + return getPRChecks(repo.path, prNumber, headSha, prRepo ?? null, options) } async rerunRepoPRChecks( @@ -4731,11 +4733,12 @@ export class OrcaRuntimeService { async getRepoPRComments( repoSelector: string, prNumber: number, + prRepo?: GitHubOwnerRepo | null, options?: { noCache?: boolean } ): Promise>> { const repo = await this.resolveRepoSelector(repoSelector) this.assertHostIntegrationRepoIsLocal(repo, 'repo_pr_comments') - return getPRComments(repo.path, prNumber, options) + return getPRComments(repo.path, prNumber, { ...options, prRepo: prRepo ?? null }) } async getRepoPRFileContents( @@ -4780,21 +4783,23 @@ export class OrcaRuntimeService { async updateRepoPRTitle( repoSelector: string, prNumber: number, - title: string + title: string, + prRepo?: GitHubOwnerRepo | null ): Promise>> { const repo = await this.resolveRepoSelector(repoSelector) this.assertHostIntegrationRepoIsLocal(repo, 'repo_pr_title') - return updatePRTitle(repo.path, prNumber, title) + return updatePRTitle(repo.path, prNumber, title, undefined, prRepo ?? null) } async mergeRepoPR( repoSelector: string, prNumber: number, - method?: 'merge' | 'squash' | 'rebase' + method?: 'merge' | 'squash' | 'rebase', + prRepo?: GitHubOwnerRepo | null ): Promise>> { const repo = await this.resolveRepoSelector(repoSelector) this.assertHostIntegrationRepoIsLocal(repo, 'repo_pr_merge') - return mergePR(repo.path, prNumber, method) + return mergePR(repo.path, prNumber, method, undefined, prRepo ?? null) } async updateRepoPRState( diff --git a/src/main/runtime/rpc/methods/github.test.ts b/src/main/runtime/rpc/methods/github.test.ts index 50ff2038c..94d7c1ca9 100644 --- a/src/main/runtime/rpc/methods/github.test.ts +++ b/src/main/runtime/rpc/methods/github.test.ts @@ -156,13 +156,47 @@ describe('github RPC methods', () => { repo: 'repo-1', prNumber: 7, headSha: 'abc123', + prRepo: { owner: 'acme', repo: 'widgets' }, noCache: true }) ) - expect(runtime.getRepoPRChecks).toHaveBeenCalledWith('repo-1', 7, 'abc123', { - noCache: true - }) + expect(runtime.getRepoPRChecks).toHaveBeenCalledWith( + 'repo-1', + 7, + 'abc123', + { owner: 'acme', repo: 'widgets' }, + { + noCache: true + } + ) + expect(response).toMatchObject({ ok: true, result: [] }) + }) + + it('fetches PR comments on the runtime server with explicit PR repo', async () => { + const runtime = { + getRuntimeId: () => 'test-runtime', + getRepoPRComments: vi.fn().mockResolvedValue([]) + } as unknown as OrcaRuntimeService + const dispatcher = new RpcDispatcher({ runtime, methods: GITHUB_METHODS }) + + const response = await dispatcher.dispatch( + makeRequest('github.prComments', { + repo: 'repo-1', + prNumber: 7, + prRepo: { owner: 'acme', repo: 'widgets' }, + noCache: true + }) + ) + + expect(runtime.getRepoPRComments).toHaveBeenCalledWith( + 'repo-1', + 7, + { owner: 'acme', repo: 'widgets' }, + { + noCache: true + } + ) expect(response).toMatchObject({ ok: true, result: [] }) }) @@ -254,11 +288,15 @@ describe('github RPC methods', () => { makeRequest('github.updatePRTitle', { repo: 'repo-1', prNumber: 7, - title: 'New title' + title: 'New title', + prRepo: { owner: 'acme', repo: 'widgets' } }) ) - expect(runtime.updateRepoPRTitle).toHaveBeenCalledWith('repo-1', 7, 'New title') + expect(runtime.updateRepoPRTitle).toHaveBeenCalledWith('repo-1', 7, 'New title', { + owner: 'acme', + repo: 'widgets' + }) expect(response).toMatchObject({ ok: true, result: true }) }) @@ -273,11 +311,15 @@ describe('github RPC methods', () => { makeRequest('github.mergePR', { repo: 'repo-1', prNumber: 7, - method: 'squash' + method: 'squash', + prRepo: { owner: 'acme', repo: 'widgets' } }) ) - expect(runtime.mergeRepoPR).toHaveBeenCalledWith('repo-1', 7, 'squash') + expect(runtime.mergeRepoPR).toHaveBeenCalledWith('repo-1', 7, 'squash', { + owner: 'acme', + repo: 'widgets' + }) expect(response).toMatchObject({ ok: true, result: { ok: true } }) }) diff --git a/src/main/runtime/rpc/methods/github.ts b/src/main/runtime/rpc/methods/github.ts index 60e6203e1..77fbe27da 100644 --- a/src/main/runtime/rpc/methods/github.ts +++ b/src/main/runtime/rpc/methods/github.ts @@ -55,7 +55,8 @@ const Issue = RepoSelector.extend({ const PullRequest = RepoSelector.extend({ prNumber: z.number().int().positive(), - noCache: z.boolean().optional() + noCache: z.boolean().optional(), + prRepo: SlugRepo.nullable().optional() }) const PullRequestChecks = PullRequest.extend({ @@ -89,12 +90,14 @@ const ReviewThread = RepoSelector.extend({ const UpdatePrTitle = RepoSelector.extend({ prNumber: z.number().int().positive(), - title: requiredString('Missing title') + title: requiredString('Missing title'), + prRepo: SlugRepo.nullable().optional() }) const MergePr = RepoSelector.extend({ prNumber: z.number().int().positive(), - method: z.enum(['merge', 'squash', 'rebase']).optional() + method: z.enum(['merge', 'squash', 'rebase']).optional(), + prRepo: SlugRepo.nullable().optional() }) const UpdatePrState = RepoSelector.extend({ @@ -308,7 +311,7 @@ export const GITHUB_METHODS: RpcMethod[] = [ name: 'github.prChecks', params: PullRequestChecks, handler: async (params, { runtime }) => - runtime.getRepoPRChecks(params.repo, params.prNumber, params.headSha, { + runtime.getRepoPRChecks(params.repo, params.prNumber, params.headSha, params.prRepo ?? null, { noCache: params.noCache }) }), @@ -325,7 +328,9 @@ export const GITHUB_METHODS: RpcMethod[] = [ name: 'github.prComments', params: PullRequest, handler: async (params, { runtime }) => - runtime.getRepoPRComments(params.repo, params.prNumber, { noCache: params.noCache }) + runtime.getRepoPRComments(params.repo, params.prNumber, params.prRepo ?? null, { + noCache: params.noCache + }) }), defineMethod({ name: 'github.prFileContents', @@ -360,13 +365,13 @@ export const GITHUB_METHODS: RpcMethod[] = [ name: 'github.updatePRTitle', params: UpdatePrTitle, handler: async (params, { runtime }) => - runtime.updateRepoPRTitle(params.repo, params.prNumber, params.title) + runtime.updateRepoPRTitle(params.repo, params.prNumber, params.title, params.prRepo ?? null) }), defineMethod({ name: 'github.mergePR', params: MergePr, handler: async (params, { runtime }) => - runtime.mergeRepoPR(params.repo, params.prNumber, params.method) + runtime.mergeRepoPR(params.repo, params.prNumber, params.method, params.prRepo ?? null) }), defineMethod({ name: 'github.updatePRState', diff --git a/src/preload/api-types.ts b/src/preload/api-types.ts index c74761e25..302996b30 100644 --- a/src/preload/api-types.ts +++ b/src/preload/api-types.ts @@ -38,6 +38,7 @@ import type { GitHubPRFileContents, GitHubPRReviewCommentInput, GitHubCommentResult, + GitHubOwnerRepo, GitHubWorkItem, GitHubWorkItemDetails, GitHubViewer, @@ -798,6 +799,7 @@ export type PreloadApi = { repoId?: string prNumber: number headSha?: string + prRepo?: GitHubOwnerRepo | null noCache?: boolean }) => Promise rerunPRChecks: (args: { @@ -811,6 +813,7 @@ export type PreloadApi = { repoPath: string repoId?: string prNumber: number + prRepo?: GitHubOwnerRepo | null noCache?: boolean }) => Promise resolveReviewThread: (args: { @@ -832,12 +835,14 @@ export type PreloadApi = { repoId?: string prNumber: number title: string + prRepo?: GitHubOwnerRepo | null }) => Promise mergePR: (args: { repoPath: string repoId?: string prNumber: number method?: 'merge' | 'squash' | 'rebase' + prRepo?: GitHubOwnerRepo | null }) => Promise<{ ok: true } | { ok: false; error: string }> updatePRState: (args: { repoPath: string diff --git a/src/preload/index.ts b/src/preload/index.ts index 9b243c3d7..a6abd7ef6 100644 --- a/src/preload/index.ts +++ b/src/preload/index.ts @@ -814,6 +814,7 @@ const api = { repoId?: string prNumber: number headSha?: string + prRepo?: { owner: string; repo: string } | null noCache?: boolean }): Promise => ipcRenderer.invoke('gh:prChecks', args), @@ -830,6 +831,7 @@ const api = { repoPath: string repoId?: string prNumber: number + prRepo?: { owner: string; repo: string } | null noCache?: boolean }): Promise => ipcRenderer.invoke('gh:prComments', args), @@ -854,6 +856,7 @@ const api = { repoId?: string prNumber: number title: string + prRepo?: { owner: string; repo: string } | null }): Promise => ipcRenderer.invoke('gh:updatePRTitle', args), mergePR: (args: { @@ -861,6 +864,7 @@ const api = { repoId?: string prNumber: number method?: 'merge' | 'squash' | 'rebase' + prRepo?: { owner: string; repo: string } | null }): Promise<{ ok: true } | { ok: false; error: string }> => ipcRenderer.invoke('gh:mergePR', args), diff --git a/src/renderer/src/components/right-sidebar/ChecksPanel.tsx b/src/renderer/src/components/right-sidebar/ChecksPanel.tsx index 3071e5d27..6d2da15c6 100644 --- a/src/renderer/src/components/right-sidebar/ChecksPanel.tsx +++ b/src/renderer/src/components/right-sidebar/ChecksPanel.tsx @@ -3,6 +3,7 @@ merge actions, and conflict state in one component to keep the data flow straigh import React, { useCallback, useEffect, useRef, useState } from 'react' import { LoaderCircle, ExternalLink, RefreshCw, Check, X, Pencil } from 'lucide-react' import { useAppStore } from '@/store' +import { prChecksCacheSuffix, prCommentsCacheSuffix } from '@/store/slices/github' import { useActiveWorktree, useRepoById } from '@/store/selectors' import { cn } from '@/lib/utils' import { Button } from '@/components/ui/button' @@ -124,8 +125,10 @@ export default function ChecksPanel(): React.JSX.Element { const prFetchedAt = useAppStore((s) => prCacheKey ? s.prCache[prCacheKey]?.fetchedAt : undefined ) - const checksCacheKey = repo && prNumber ? `${repo.id}::pr-checks::${prNumber}` : '' - const commentsCacheKey = repo && prNumber ? `${repo.id}::pr-comments::${prNumber}` : '' + const checksCacheKey = + repo && prNumber ? `${repo.id}::${prChecksCacheSuffix(prNumber, pr?.prRepo)}` : '' + const commentsCacheKey = + repo && prNumber ? `${repo.id}::${prCommentsCacheSuffix(prNumber, pr?.prRepo)}` : '' const checksFetchedAt = useAppStore((s) => checksCacheKey ? s.checksCache[checksCacheKey]?.fetchedAt : undefined ) @@ -139,7 +142,8 @@ export default function ChecksPanel(): React.JSX.Element { const linkedPR = activeWorktree?.linkedPR ?? null const linkedGitLabMR = activeWorktree?.linkedGitLabMR ?? null const activeWorktreePath = activeWorktree?.path ?? null - const stateRequestKey = repo && branch ? checksPanelAsyncResultKey(repo.id, branch, prNumber) : '' + const stateRequestKey = + repo && branch ? checksPanelAsyncResultKey(repo.id, branch, prNumber, pr?.prRepo) : '' asyncResultKeyRef.current = stateRequestKey const isCurrentAsyncResult = useCallback( @@ -241,11 +245,18 @@ export default function ChecksPanel(): React.JSX.Element { } setChecksLoading(true) try { - const requestKey = checksPanelAsyncResultKey(repo.id, branch, targetPRNumber) - const result = await fetchPRChecks(repo.path, targetPRNumber, branch, pr?.headSha, { - force, - repoId: repo.id - }) + const requestKey = checksPanelAsyncResultKey(repo.id, branch, targetPRNumber, pr?.prRepo) + const result = await fetchPRChecks( + repo.path, + targetPRNumber, + branch, + pr?.headSha, + pr?.prRepo, + { + force, + repoId: repo.id + } + ) if (!isCurrentAsyncResult(requestKey)) { return } @@ -260,18 +271,26 @@ export default function ChecksPanel(): React.JSX.Element { : 30_000 prevChecksRef.current = signature } catch (err) { - if (!isCurrentAsyncResult(checksPanelAsyncResultKey(repo.id, branch, targetPRNumber))) { + if ( + !isCurrentAsyncResult( + checksPanelAsyncResultKey(repo.id, branch, targetPRNumber, pr?.prRepo) + ) + ) { return } console.warn('Failed to fetch PR checks:', err) setChecks([]) } finally { - if (isCurrentAsyncResult(checksPanelAsyncResultKey(repo.id, branch, targetPRNumber))) { + if ( + isCurrentAsyncResult( + checksPanelAsyncResultKey(repo.id, branch, targetPRNumber, pr?.prRepo) + ) + ) { setChecksLoading(false) } } }, - [repo, prNumber, branch, pr?.headSha, fetchPRChecks, isCurrentAsyncResult] + [repo, prNumber, branch, pr?.headSha, pr?.prRepo, fetchPRChecks, isCurrentAsyncResult] ) // Fetch checks on mount + poll with exponential backoff @@ -312,33 +331,51 @@ export default function ChecksPanel(): React.JSX.Element { const fetchComments = useCallback( async ({ force = false, - prNumberOverride - }: { force?: boolean; prNumberOverride?: number | null } = {}) => { + prNumberOverride, + prRepoOverride + }: { + force?: boolean + prNumberOverride?: number | null + prRepoOverride?: PRInfo['prRepo'] | null + } = {}) => { const targetPRNumber = prNumberOverride ?? prNumber + const targetPRRepo = prRepoOverride ?? pr?.prRepo if (!repo || !targetPRNumber) { return } setCommentsLoading(true) try { - const requestKey = checksPanelAsyncResultKey(repo.id, branch, targetPRNumber) - const result = await fetchPRComments(repo.path, targetPRNumber, { force, repoId: repo.id }) + const requestKey = checksPanelAsyncResultKey(repo.id, branch, targetPRNumber, targetPRRepo) + const result = await fetchPRComments(repo.path, targetPRNumber, { + force, + repoId: repo.id, + prRepo: targetPRRepo + }) if (!isCurrentAsyncResult(requestKey)) { return } setComments(result) } catch (err) { - if (!isCurrentAsyncResult(checksPanelAsyncResultKey(repo.id, branch, targetPRNumber))) { + if ( + !isCurrentAsyncResult( + checksPanelAsyncResultKey(repo.id, branch, targetPRNumber, targetPRRepo) + ) + ) { return } console.warn('Failed to fetch PR comments:', err) setComments([]) } finally { - if (isCurrentAsyncResult(checksPanelAsyncResultKey(repo.id, branch, targetPRNumber))) { + if ( + isCurrentAsyncResult( + checksPanelAsyncResultKey(repo.id, branch, targetPRNumber, targetPRRepo) + ) + ) { setCommentsLoading(false) } } }, - [repo, prNumber, fetchPRComments, branch, isCurrentAsyncResult] + [repo, prNumber, pr?.prRepo, fetchPRComments, branch, isCurrentAsyncResult] ) useEffect(() => { @@ -350,7 +387,7 @@ export default function ChecksPanel(): React.JSX.Element { // state after the user switches worktrees, showing the wrong PR's comments. let cancelled = false setCommentsLoading(true) - void fetchPRComments(repo.path, prNumber, { repoId: repo.id }).then( + void fetchPRComments(repo.path, prNumber, { repoId: repo.id, prRepo: pr?.prRepo }).then( (result) => { if (!cancelled) { setComments(result) @@ -367,13 +404,13 @@ export default function ChecksPanel(): React.JSX.Element { return () => { cancelled = true } - }, [repo, prNumber, isPanelVisible, fetchPRComments]) + }, [repo, prNumber, pr?.prRepo, isPanelVisible, fetchPRComments]) const handleRefresh = useCallback(async () => { if (!repo || !branch) { return } - const initialRequestKey = checksPanelAsyncResultKey(repo.id, branch, prNumber) + const initialRequestKey = checksPanelAsyncResultKey(repo.id, branch, prNumber, pr?.prRepo) let activeRefreshKey = initialRequestKey setIsRefreshing(true) try { @@ -390,7 +427,12 @@ export default function ChecksPanel(): React.JSX.Element { linkedGitLabMR }) if (refreshedPR) { - const requestKey = checksPanelAsyncResultKey(repo.id, branch, refreshedPR.number) + const requestKey = checksPanelAsyncResultKey( + repo.id, + branch, + refreshedPR.number, + refreshedPR.prRepo + ) if (!isCurrentAsyncResult(initialRequestKey) && !isCurrentAsyncResult(requestKey)) { return } @@ -407,6 +449,7 @@ export default function ChecksPanel(): React.JSX.Element { refreshedPR.number, branch, refreshedPR.headSha, + refreshedPR.prRepo, { force: true, repoId: repo.id } ).then( (result) => { @@ -434,7 +477,8 @@ export default function ChecksPanel(): React.JSX.Element { setChecksLoading(true) const refreshedComments = fetchComments({ force: true, - prNumberOverride: refreshedPR.number + prNumberOverride: refreshedPR.number, + prRepoOverride: refreshedPR.prRepo }) await Promise.all([ refreshedChecks.finally(() => { @@ -457,6 +501,7 @@ export default function ChecksPanel(): React.JSX.Element { repo, branch, prNumber, + pr?.prRepo, linkedPR, linkedGitLabMR, fetchPRForBranch, @@ -533,7 +578,8 @@ export default function ChecksPanel(): React.JSX.Element { repoPath: repo.path, repoId: repo.id, prNumber: pr.number, - title: titleDraft.trim() + title: titleDraft.trim(), + prRepo: pr.prRepo ?? null }) if (ok) { // Re-fetch PR to get updated title @@ -566,20 +612,21 @@ export default function ChecksPanel(): React.JSX.Element { if (!repo || !prNumber) { return } - void resolveReviewThread(repo.path, prNumber, threadId, resolve, { repoId: repo.id }).then( - (ok) => { - if (ok) { - // Update local state to match the optimistic store update - setComments((prev) => - prev.map((c) => (c.threadId === threadId ? { ...c, isResolved: resolve } : c)) - ) - } else { - toast.error('Could not update review thread. Check the GitHub API budget.') - } + void resolveReviewThread(repo.path, prNumber, threadId, resolve, { + repoId: repo.id, + prRepo: pr?.prRepo + }).then((ok) => { + if (ok) { + // Update local state to match the optimistic store update + setComments((prev) => + prev.map((c) => (c.threadId === threadId ? { ...c, isResolved: resolve } : c)) + ) + } else { + toast.error('Could not update review thread. Check the GitHub API budget.') } - ) + }) }, - [repo, prNumber, resolveReviewThread] + [repo, prNumber, pr?.prRepo, resolveReviewThread] ) // Refresh PR (passed to PRActions) @@ -632,7 +679,7 @@ export default function ChecksPanel(): React.JSX.Element { if (!repo || !branch) { return } - const initialRequestKey = checksPanelAsyncResultKey(repo.id, branch, prNumber) + const initialRequestKey = checksPanelAsyncResultKey(repo.id, branch, prNumber, pr?.prRepo) setRightSidebarOpen(true) setRightSidebarTab('checks') try { @@ -649,23 +696,36 @@ export default function ChecksPanel(): React.JSX.Element { linkedGitLabMR }) if (refreshedPR) { - const requestKey = checksPanelAsyncResultKey(repo.id, branch, refreshedPR.number) + const requestKey = checksPanelAsyncResultKey( + repo.id, + branch, + refreshedPR.number, + refreshedPR.prRepo + ) if (!isCurrentAsyncResult(initialRequestKey) && !isCurrentAsyncResult(requestKey)) { return } asyncResultKeyRef.current = requestKey await Promise.all([ - fetchPRChecks(repo.path, refreshedPR.number, branch, refreshedPR.headSha, { - force: true, - repoId: repo.id - }).then((result) => { + fetchPRChecks( + repo.path, + refreshedPR.number, + branch, + refreshedPR.headSha, + refreshedPR.prRepo, + { + force: true, + repoId: repo.id + } + ).then((result) => { if (isCurrentAsyncResult(requestKey)) { setChecks(result) } }), fetchPRComments(repo.path, refreshedPR.number, { force: true, - repoId: repo.id + repoId: repo.id, + prRepo: refreshedPR.prRepo }).then((result) => { if (isCurrentAsyncResult(requestKey)) { setComments(result) @@ -686,6 +746,7 @@ export default function ChecksPanel(): React.JSX.Element { isCurrentAsyncResult, linkedGitLabMR, prNumber, + pr?.prRepo, repo, setRightSidebarOpen, setRightSidebarTab diff --git a/src/renderer/src/components/right-sidebar/PRActions.tsx b/src/renderer/src/components/right-sidebar/PRActions.tsx index 9314bc67e..474d59d88 100644 --- a/src/renderer/src/components/right-sidebar/PRActions.tsx +++ b/src/renderer/src/components/right-sidebar/PRActions.tsx @@ -46,7 +46,8 @@ export default function PRActions({ repoPath: repo.path, repoId: repo.id, prNumber: pr.number, - method + method, + prRepo: pr.prRepo ?? null }) if (!result.ok) { setMergeError(result.error) @@ -59,7 +60,7 @@ export default function PRActions({ setMerging(false) } }, - [repo.id, repo.path, pr.number, onRefreshPR] + [repo.id, repo.path, pr.number, pr.prRepo, onRefreshPR] ) useEffect(() => { diff --git a/src/renderer/src/components/right-sidebar/checks-panel-async-result-key.test.ts b/src/renderer/src/components/right-sidebar/checks-panel-async-result-key.test.ts index f6f615038..5efdfabad 100644 --- a/src/renderer/src/components/right-sidebar/checks-panel-async-result-key.test.ts +++ b/src/renderer/src/components/right-sidebar/checks-panel-async-result-key.test.ts @@ -7,15 +7,24 @@ import { describe('checksPanelAsyncResultKey', () => { it('builds a stable repo-scoped key', () => { expect(checksPanelAsyncResultKey('repo-id', 'feature/test', 12)).toBe( - 'repo-id::feature/test::12' + 'repo-id::feature/test::none::12' ) }) it('uses explicit none marker when PR is absent', () => { expect(checksPanelAsyncResultKey('repo-id', 'feature/test', null)).toBe( - 'repo-id::feature/test::none' + 'repo-id::feature/test::none::none' ) }) + + it('normalizes PR repo identity', () => { + expect( + checksPanelAsyncResultKey('repo-id', 'feature/test', 12, { + owner: 'Acme', + repo: 'Widgets' + }) + ).toBe('repo-id::feature/test::acme/widgets::12') + }) }) describe('shouldCommitChecksPanelAsyncResult', () => { @@ -27,4 +36,19 @@ describe('shouldCommitChecksPanelAsyncResult', () => { ) ).toBe(false) }) + + it('suppresses stale completions when the PR repo changes without a PR number change', () => { + expect( + shouldCommitChecksPanelAsyncResult( + checksPanelAsyncResultKey('repo-id', 'feature/test', 12, { + owner: 'upstream', + repo: 'orca' + }), + checksPanelAsyncResultKey('repo-id', 'feature/test', 12, { + owner: 'fork', + repo: 'orca' + }) + ) + ).toBe(false) + }) }) diff --git a/src/renderer/src/components/right-sidebar/checks-panel-async-result-key.ts b/src/renderer/src/components/right-sidebar/checks-panel-async-result-key.ts index bcc2e446a..fc395aae9 100644 --- a/src/renderer/src/components/right-sidebar/checks-panel-async-result-key.ts +++ b/src/renderer/src/components/right-sidebar/checks-panel-async-result-key.ts @@ -1,9 +1,19 @@ +import type { GitHubOwnerRepo } from '../../../../shared/types' + +function normalizedPRRepoIdentity(prRepo?: GitHubOwnerRepo | null): string { + if (!prRepo) { + return 'none' + } + return `${prRepo.owner.toLowerCase()}/${prRepo.repo.toLowerCase()}` +} + export function checksPanelAsyncResultKey( repoId: string, branch: string, - prNumber: number | null + prNumber: number | null, + prRepo?: GitHubOwnerRepo | null ): string { - return `${repoId}::${branch}::${prNumber ?? 'none'}` + return `${repoId}::${branch}::${normalizedPRRepoIdentity(prRepo)}::${prNumber ?? 'none'}` } export function shouldCommitChecksPanelAsyncResult( diff --git a/src/renderer/src/store/slices/github-checks.ts b/src/renderer/src/store/slices/github-checks.ts index dbfd16057..9dc1927cd 100644 --- a/src/renderer/src/store/slices/github-checks.ts +++ b/src/renderer/src/store/slices/github-checks.ts @@ -1,5 +1,5 @@ import type { AppState } from '../types' -import type { PRCheckDetail, CheckStatus } from '../../../../shared/types' +import type { PRCheckDetail, CheckStatus, GitHubOwnerRepo } from '../../../../shared/types' export function normalizeBranchName(branch: string): string { return branch.replace(/^refs\/heads\//, '') @@ -38,7 +38,8 @@ export function syncPRChecksStatus( repoPath: string, repoId: string | undefined, branch: string | undefined, - checks: PRCheckDetail[] + checks: PRCheckDetail[], + prRepo?: GitHubOwnerRepo | null ): Partial | null { const normalized = branch ? normalizeBranchName(branch) : '' if (!normalized) { @@ -50,6 +51,11 @@ export function syncPRChecksStatus( if (!prEntry?.data) { return null } + // Why: fork PR rediscovery can retarget the branch cache while an older + // checks request is still in flight; only the matching PR repo may update it. + if (!samePRRepo(prEntry.data.prRepo, prRepo)) { + return null + } const nextStatus = deriveCheckStatusFromChecks(checks) if (prEntry.data.checksStatus === nextStatus) { @@ -69,3 +75,14 @@ export function syncPRChecksStatus( } } } + +function normalizedPRRepo(repo?: GitHubOwnerRepo | null): string | null { + if (!repo) { + return null + } + return `${repo.owner.toLowerCase()}/${repo.repo.toLowerCase()}` +} + +function samePRRepo(left?: GitHubOwnerRepo | null, right?: GitHubOwnerRepo | null): boolean { + return normalizedPRRepo(left) === normalizedPRRepo(right) +} diff --git a/src/renderer/src/store/slices/github.test.ts b/src/renderer/src/store/slices/github.test.ts index bea24d1de..8b0dc76d2 100644 --- a/src/renderer/src/store/slices/github.test.ts +++ b/src/renderer/src/store/slices/github.test.ts @@ -20,6 +20,7 @@ const mockApi = { prForBranch: vi.fn().mockResolvedValue(null), issue: vi.fn().mockResolvedValue(null), prChecks: vi.fn().mockResolvedValue([]), + prComments: vi.fn().mockResolvedValue([]), listWorkItems: vi.fn(), getProjectViewTable: vi.fn() }, @@ -194,7 +195,9 @@ describe('createGitHubSlice.fetchPRChecks', () => { { name: 'lint', status: 'completed', conclusion: 'success', url: null } ]) - await store.getState().fetchPRChecks(repoPath, 12, branch, undefined, { force: true, repoId }) + await store + .getState() + .fetchPRChecks(repoPath, 12, branch, undefined, null, { force: true, repoId }) expect(store.getState().prCache[prCacheKey]?.data?.checksStatus).toBe('success') }) @@ -220,7 +223,9 @@ describe('createGitHubSlice.fetchPRChecks', () => { { name: 'integration', status: 'completed', conclusion: 'failure', url: null } ]) - await store.getState().fetchPRChecks(repoPath, 12, branch, undefined, { force: true, repoId }) + await store + .getState() + .fetchPRChecks(repoPath, 12, branch, undefined, null, { force: true, repoId }) expect(store.getState().prCache[prCacheKey]?.data?.checksStatus).toBe('failure') }) @@ -247,7 +252,7 @@ describe('createGitHubSlice.fetchPRChecks', () => { await store .getState() - .fetchPRChecks(repoPath, 12, `refs/heads/${branch}`, undefined, { force: true, repoId }) + .fetchPRChecks(repoPath, 12, `refs/heads/${branch}`, undefined, null, { force: true, repoId }) expect(store.getState().prCache[prCacheKey]?.data?.checksStatus).toBe('success') }) @@ -274,7 +279,9 @@ describe('createGitHubSlice.fetchPRChecks', () => { { name: 'build', status: 'completed', conclusion: 'success', url: null } ]) - await store.getState().fetchPRChecks(repoPath, 12, branch, undefined, { force: true, repoId }) + await store + .getState() + .fetchPRChecks(repoPath, 12, branch, undefined, null, { force: true, repoId }) await vi.advanceTimersByTimeAsync(1000) expect(mockApi.cache.setGitHub).toHaveBeenCalledWith({ @@ -310,7 +317,7 @@ describe('createGitHubSlice.fetchPRChecks', () => { } }) - await store.getState().fetchPRChecks(repoPath, 12, branch, undefined, { repoId }) + await store.getState().fetchPRChecks(repoPath, 12, branch, undefined, null, { repoId }) await vi.advanceTimersByTimeAsync(1000) expect(mockApi.gh.prChecks).not.toHaveBeenCalled() @@ -341,17 +348,109 @@ describe('createGitHubSlice.fetchPRChecks', () => { await store .getState() - .fetchPRChecks(repoPath, 12, branch, 'abc123head', { force: true, repoId }) + .fetchPRChecks(repoPath, 12, branch, 'abc123head', null, { force: true, repoId }) expect(mockApi.gh.prChecks).toHaveBeenCalledWith({ repoPath, repoId, prNumber: 12, headSha: 'abc123head', + prRepo: null, noCache: true }) }) + it('keys PR checks by normalized PR repo identity', async () => { + const store = createTestStore() + const repoPath = '/repo' + const repoId = 'repo-id' + const branch = 'feature/test' + + mockApi.gh.prChecks + .mockResolvedValueOnce([ + { name: 'upstream', status: 'completed', conclusion: 'success', url: null } + ]) + .mockResolvedValueOnce([ + { name: 'fork', status: 'completed', conclusion: 'failure', url: null } + ]) + + await store + .getState() + .fetchPRChecks( + repoPath, + 12, + branch, + 'head-a', + { owner: 'Acme', repo: 'Widgets' }, + { force: true, repoId } + ) + await store + .getState() + .fetchPRChecks( + repoPath, + 12, + branch, + 'head-b', + { owner: 'Fork', repo: 'Widgets' }, + { force: true, repoId } + ) + + expect( + store.getState().checksCache[`${repoId}::pr-checks::acme/widgets::12`]?.data?.[0].name + ).toBe('upstream') + expect( + store.getState().checksCache[`${repoId}::pr-checks::fork/widgets::12`]?.data?.[0].name + ).toBe('fork') + expect(mockApi.gh.prChecks).toHaveBeenNthCalledWith(1, { + repoPath, + repoId, + prNumber: 12, + headSha: 'head-a', + prRepo: { owner: 'Acme', repo: 'Widgets' }, + noCache: true + }) + }) + + it('does not sync stale checks into a PR cache entry for a different PR repo', async () => { + const store = createTestStore() + const repoPath = '/repo' + const repoId = 'repo-id' + const branch = 'feature/test' + const prCacheKey = `${repoId}::${branch}` + + store.setState({ + prCache: { + [prCacheKey]: { + data: makePR({ + checksStatus: 'pending', + prRepo: { owner: 'Fork', repo: 'Widgets' } + }), + fetchedAt: 1 + } + } + }) + + mockApi.gh.prChecks.mockResolvedValue([ + { name: 'build', status: 'completed', conclusion: 'success', url: null } + ]) + + await store + .getState() + .fetchPRChecks( + repoPath, + 12, + branch, + 'head-a', + { owner: 'Acme', repo: 'Widgets' }, + { force: true, repoId } + ) + + expect(store.getState().prCache[prCacheKey]?.data?.checksStatus).toBe('pending') + expect( + store.getState().checksCache[`${repoId}::pr-checks::acme/widgets::12`]?.data?.[0].name + ).toBe('build') + }) + it('updates repo-scoped PR cache entry instead of repoPath fallback key', async () => { const store = createTestStore() const repoPath = '/repo' @@ -371,13 +470,62 @@ describe('createGitHubSlice.fetchPRChecks', () => { { name: 'build', status: 'completed', conclusion: 'success', url: null } ]) - await store.getState().fetchPRChecks(repoPath, 12, branch, undefined, { force: true, repoId }) + await store + .getState() + .fetchPRChecks(repoPath, 12, branch, undefined, null, { force: true, repoId }) expect(store.getState().prCache[repoScopedKey]?.data?.checksStatus).toBe('success') expect(store.getState().prCache[pathScopedKey]?.data?.checksStatus).toBe('pending') }) }) +describe('createGitHubSlice.fetchPRComments', () => { + beforeEach(() => { + vi.clearAllMocks() + resetRemoteRuntimeMocks() + mockApi.gh.prComments.mockResolvedValue([]) + }) + + it('keys PR comments by normalized PR repo identity', async () => { + const store = createTestStore() + const repoPath = '/repo' + const repoId = 'repo-id' + + mockApi.gh.prComments + .mockResolvedValueOnce([ + { id: 1, author: 'upstream', authorAvatarUrl: '', body: '', createdAt: '', url: '' } + ]) + .mockResolvedValueOnce([ + { id: 2, author: 'fork', authorAvatarUrl: '', body: '', createdAt: '', url: '' } + ]) + + await store.getState().fetchPRComments(repoPath, 12, { + force: true, + repoId, + prRepo: { owner: 'Acme', repo: 'Widgets' } + }) + await store.getState().fetchPRComments(repoPath, 12, { + force: true, + repoId, + prRepo: { owner: 'Fork', repo: 'Widgets' } + }) + + expect( + store.getState().commentsCache[`${repoId}::pr-comments::acme/widgets::12`]?.data?.[0].author + ).toBe('upstream') + expect( + store.getState().commentsCache[`${repoId}::pr-comments::fork/widgets::12`]?.data?.[0].author + ).toBe('fork') + expect(mockApi.gh.prComments).toHaveBeenNthCalledWith(1, { + repoPath, + repoId, + prNumber: 12, + prRepo: { owner: 'Acme', repo: 'Widgets' }, + noCache: true + }) + }) +}) + describe('createGitHubSlice.fetchPRForBranch', () => { beforeEach(() => { vi.clearAllMocks() diff --git a/src/renderer/src/store/slices/github.ts b/src/renderer/src/store/slices/github.ts index cbfefed70..c9423825a 100644 --- a/src/renderer/src/store/slices/github.ts +++ b/src/renderer/src/store/slices/github.ts @@ -393,6 +393,24 @@ function evictRepoCacheEntries( return next ? { cache: next, evicted: true } : { cache, evicted: false } } +function normalizedRepoIdentity(repo: GitHubOwnerRepo): string { + return `${repo.owner.toLowerCase()}/${repo.repo.toLowerCase()}` +} + +export function prChecksCacheSuffix(prNumber: number, prRepo?: GitHubOwnerRepo | null): string { + if (!prRepo) { + return `pr-checks::${prNumber}` + } + return `pr-checks::${normalizedRepoIdentity(prRepo)}::${prNumber}` +} + +export function prCommentsCacheSuffix(prNumber: number, prRepo?: GitHubOwnerRepo | null): string { + if (!prRepo) { + return `pr-comments::${prNumber}` + } + return `pr-comments::${normalizedRepoIdentity(prRepo)}::${prNumber}` +} + // Why: 500 entries is generous enough that active developers will never hit it // during normal use, but prevents the cache from growing without bound across // many repos and branches over a long-running session. @@ -467,19 +485,20 @@ export type GitHubSlice = { prNumber: number, branch?: string, headSha?: string, + prRepo?: GitHubOwnerRepo | null, options?: RepoScopedFetchOptions ) => Promise fetchPRComments: ( repoPath: string, prNumber: number, - options?: RepoScopedFetchOptions + options?: RepoScopedFetchOptions & { prRepo?: GitHubOwnerRepo | null } ) => Promise resolveReviewThread: ( repoPath: string, prNumber: number, threadId: string, resolve: boolean, - options?: RepoScopedFetchOptions + options?: RepoScopedFetchOptions & { prRepo?: GitHubOwnerRepo | null } ) => Promise initGitHubCache: () => Promise refreshAllGitHub: () => void @@ -1397,13 +1416,27 @@ export const createGitHubSlice: StateCreator = (s return request }, - fetchPRChecks: async (repoPath, prNumber, branch, headSha, options): Promise => { + fetchPRChecks: async ( + repoPath, + prNumber, + branch, + headSha, + prRepo, + options + ): Promise => { const repoId = options?.repoId ?? get().repos?.find((repo) => repo.path === repoPath)?.id - const cacheKey = repoScopedCacheKey(repoPath, repoId, `pr-checks::${prNumber}`) + const cacheKey = repoScopedCacheKey(repoPath, repoId, prChecksCacheSuffix(prNumber, prRepo)) const cached = get().checksCache[cacheKey] if (!options?.force && isFresh(cached, CHECKS_CACHE_TTL)) { const cachedChecks = cached.data ?? [] - const prStatusUpdate = syncPRChecksStatus(get(), repoPath, repoId, branch, cachedChecks) + const prStatusUpdate = syncPRChecksStatus( + get(), + repoPath, + repoId, + branch, + cachedChecks, + prRepo + ) if (prStatusUpdate) { set(prStatusUpdate) debouncedSaveCache(get()) @@ -1423,6 +1456,7 @@ export const createGitHubSlice: StateCreator = (s repoId, prNumber, headSha, + prRepo: prRepo ?? null, noCache: options?.force })) as PRCheckDetail[] set((s) => { @@ -1430,7 +1464,7 @@ export const createGitHubSlice: StateCreator = (s checksCache: { ...s.checksCache, [cacheKey]: { data: checks, fetchedAt: Date.now() } } } - const prStatusUpdate = syncPRChecksStatus(s, repoPath, repoId, branch, checks) + const prStatusUpdate = syncPRChecksStatus(s, repoPath, repoId, branch, checks, prRepo) if (prStatusUpdate?.prCache) { nextState.prCache = prStatusUpdate.prCache } @@ -1453,7 +1487,11 @@ export const createGitHubSlice: StateCreator = (s fetchPRComments: async (repoPath, prNumber, options): Promise => { const repoId = options?.repoId ?? get().repos?.find((repo) => repo.path === repoPath)?.id - const cacheKey = repoScopedCacheKey(repoPath, repoId, `pr-comments::${prNumber}`) + const cacheKey = repoScopedCacheKey( + repoPath, + repoId, + prCommentsCacheSuffix(prNumber, options?.prRepo) + ) const cached = get().commentsCache[cacheKey] if (!options?.force && isFresh(cached)) { return cached.data ?? [] @@ -1470,6 +1508,7 @@ export const createGitHubSlice: StateCreator = (s repoPath, repoId, prNumber, + prRepo: options?.prRepo ?? null, noCache: options?.force })) as PRComment[] set((s) => ({ @@ -1493,7 +1532,11 @@ export const createGitHubSlice: StateCreator = (s resolveReviewThread: async (repoPath, prNumber, threadId, resolve, options) => { const repoId = options?.repoId ?? get().repos?.find((repo) => repo.path === repoPath)?.id - const cacheKey = repoScopedCacheKey(repoPath, repoId, `pr-comments::${prNumber}`) + const cacheKey = repoScopedCacheKey( + repoPath, + repoId, + prCommentsCacheSuffix(prNumber, options?.prRepo) + ) // Optimistic update: toggle isResolved on all comments in this thread immediately // so the UI feels instant. Reverts if the API call fails. diff --git a/src/shared/types.ts b/src/shared/types.ts index f09e6317c..343953cf7 100644 --- a/src/shared/types.ts +++ b/src/shared/types.ts @@ -585,6 +585,8 @@ export type PRConflictSummary = { files: string[] } +export type GitHubRepositoryIdentity = { owner: string; repo: string } + export type PRInfo = { number: number title: string @@ -597,6 +599,8 @@ export type PRInfo = { // Keeping the head SHA in cached PR metadata lets the checks panel poll the // correct commit without re-querying GitHub or guessing from local branch refs. headSha?: string + prRepo?: GitHubRepositoryIdentity + headRepo?: GitHubRepositoryIdentity conflictSummary?: PRConflictSummary } @@ -904,7 +908,7 @@ export type ClassifiedError = { // slices can reference the same structural type without importing from main. // Aliased as `OwnerRepo` in `src/main/github/gh-utils.ts` so main call sites // can continue using the short local name. -export type GitHubOwnerRepo = { owner: string; repo: string } +export type GitHubOwnerRepo = GitHubRepositoryIdentity // Why: GitLab-specific types live in `./gitlab-types` so they can grow // independently from the central types file (which is touched by every