diff --git a/src/main/github/client.test.ts b/src/main/github/client.test.ts index e6e647e63..ea832165d 100644 --- a/src/main/github/client.test.ts +++ b/src/main/github/client.test.ts @@ -50,10 +50,16 @@ vi.mock('./gh-utils', () => ({ gitExecFileAsync: gitExecFileAsyncMock, ghRepoExecOptions: ghRepoExecOptionsMock, githubRepoContext: githubRepoContextMock, - classifyGhError: (stderr: string) => - stderr.toLowerCase().includes('not found') || stderr.includes('HTTP 404') - ? { type: 'not_found', message: stderr } - : { type: 'unknown', message: stderr }, + classifyGhError: (stderr: string) => { + const lower = stderr.toLowerCase() + if (lower.includes('not found') || stderr.includes('HTTP 404')) { + return { type: 'not_found', message: stderr } + } + if (lower.includes('rate limit')) { + return { type: 'rate_limited', message: stderr } + } + return { type: 'unknown', message: stderr } + }, parseGitHubOwnerRepo: (remoteUrl: string) => { const match = remoteUrl.trim().match(/github\.com[:/]([^/]+)\/([^/]+?)(?:\.git)?$/) return match ? { owner: match[1], repo: match[2] } : null @@ -146,6 +152,302 @@ describe('getPRForBranch', () => { expect(pr?.mergeable).toBe('MERGEABLE') }) + it('prefers exact linked PR lookup when the repo identity is known', async () => { + getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) + gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: 'linked-head-oid\n', stderr: '' }) + ghExecFileAsyncMock.mockResolvedValueOnce({ + stdout: JSON.stringify({ + number: 99, + title: 'Linked PR', + state: 'OPEN', + url: 'https://github.com/acme/widgets/pull/99', + statusCheckRollup: [], + updatedAt: '2026-03-28T00:00:00Z', + isDraft: false, + mergeable: 'MERGEABLE', + baseRefName: 'main', + headRefName: 'someone/fix', + baseRefOid: 'base-oid', + headRefOid: 'linked-head-oid' + }) + }) + + const pr = await getPRForBranch('/repo-root', 'feature/local-worktree', 99) + + expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(1) + expect(gitExecFileAsyncMock).toHaveBeenCalledWith(['rev-parse', 'HEAD'], { + cwd: '/repo-root' + }) + expect(ghExecFileAsyncMock).toHaveBeenCalledWith( + [ + 'pr', + 'view', + '99', + '--repo', + 'acme/widgets', + '--json', + 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,baseRefName,headRefName,baseRefOid,headRefOid' + ], + { cwd: '/repo-root' } + ) + expect(pr).toMatchObject({ + number: 99, + title: 'Linked PR', + state: 'open', + headSha: 'linked-head-oid' + }) + }) + + it('uses branch discovery when exact linked PR metadata resolves to a different PR', async () => { + getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) + gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: 'current-worktree-head\n', stderr: '' }) + ghExecFileAsyncMock + .mockResolvedValueOnce({ + stdout: JSON.stringify({ + number: 99, + title: 'Stale linked PR', + state: 'OPEN', + url: 'https://github.com/acme/widgets/pull/99', + statusCheckRollup: [], + updatedAt: '2026-03-28T00:00:00Z', + isDraft: false, + mergeable: 'MERGEABLE', + baseRefName: 'main', + headRefName: 'someone/other-work', + baseRefOid: 'base-oid', + headRefOid: 'stale-linked-head' + }) + }) + .mockResolvedValueOnce({ + stdout: JSON.stringify([ + { + number: 42, + title: 'Branch PR', + 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: 'current-worktree-head' + } + ]) + }) + + const pr = await getPRForBranch('/repo-root', 'feature/test', 99) + + 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' + ], + { cwd: '/repo-root' } + ) + expect(pr?.number).toBe(42) + }) + + it('falls back to branch discovery when exact linked PR metadata is stale', async () => { + getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) + ghExecFileAsyncMock + .mockRejectedValueOnce(new Error('HTTP 404: Not Found')) + .mockResolvedValueOnce({ + stdout: JSON.stringify([ + { + number: 42, + title: 'Branch PR', + 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' + } + ]) + }) + + const pr = await getPRForBranch('/repo-root', 'feature/test', 99) + + expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( + 1, + [ + 'pr', + 'view', + '99', + '--repo', + 'acme/widgets', + '--json', + 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,baseRefName,headRefName,baseRefOid,headRefOid' + ], + { cwd: '/repo-root' } + ) + 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' + ], + { cwd: '/repo-root' } + ) + expect(pr?.number).toBe(42) + }) + + it('continues to branch discovery when exact linked PR REST fallback also misses', async () => { + getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) + ghExecFileAsyncMock + .mockRejectedValueOnce(new Error('GraphQL: could not resolve to PullRequest')) + .mockRejectedValueOnce(new Error('HTTP 404: Not Found')) + .mockResolvedValueOnce({ + stdout: JSON.stringify([ + { + number: 42, + title: 'Branch PR after stale linked miss', + 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' + } + ]) + }) + + const pr = await getPRForBranch('/repo-root', 'feature/test', 99) + + expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith(2, ['api', 'repos/acme/widgets/pulls/99'], { + cwd: '/repo-root' + }) + 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' + ], + { cwd: '/repo-root' } + ) + expect(pr?.number).toBe(42) + }) + + it('continues to branch discovery when exact linked PR REST fallback has an unclassified failure', async () => { + getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) + ghExecFileAsyncMock + .mockRejectedValueOnce(new Error('GraphQL: server exploded')) + .mockRejectedValueOnce(new Error('HTTP 500: server error')) + .mockResolvedValueOnce({ + stdout: JSON.stringify([ + { + number: 42, + title: 'Branch PR after exact lookup outage', + 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' + } + ]) + }) + + const pr = await getPRForBranch('/repo-root', 'feature/test', 99) + + expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith(2, ['api', 'repos/acme/widgets/pulls/99'], { + cwd: '/repo-root' + }) + 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' + ], + { 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 () => { + getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) + ghExecFileAsyncMock + .mockRejectedValueOnce(new Error('GraphQL: API rate limit already exceeded')) + .mockRejectedValueOnce(new Error('REST API rate limit already exceeded')) + + const pr = await getPRForBranch('/repo-root', 'feature/test', 99) + + expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( + 1, + [ + 'pr', + 'view', + '99', + '--repo', + 'acme/widgets', + '--json', + 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,baseRefName,headRefName,baseRefOid,headRefOid' + ], + { cwd: '/repo-root' } + ) + expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith(2, ['api', 'repos/acme/widgets/pulls/99'], { + cwd: '/repo-root' + }) + expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(2) + expect(pr).toBeNull() + }) + it('falls back to REST branch lookup when gh pr list is GraphQL rate limited', async () => { getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) ghExecFileAsyncMock @@ -354,8 +656,8 @@ describe('getPRForBranch', () => { it('falls back to REST number lookup when linked PR GraphQL lookup is rate limited', async () => { getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) + gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: 'linked-head-oid\n', stderr: '' }) ghExecFileAsyncMock - .mockResolvedValueOnce({ stdout: JSON.stringify([]) }) .mockRejectedValueOnce(new Error('GraphQL: API rate limit already exceeded')) .mockResolvedValueOnce({ stdout: JSON.stringify({ @@ -374,9 +676,23 @@ describe('getPRForBranch', () => { const pr = await getPRForBranch('/repo-root', 'feature/test', 99) - expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith(3, ['api', 'repos/acme/widgets/pulls/99'], { + expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( + 1, + [ + 'pr', + 'view', + '99', + '--repo', + 'acme/widgets', + '--json', + 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,baseRefName,headRefName,baseRefOid,headRefOid' + ], + { cwd: '/repo-root' } + ) + expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith(2, ['api', 'repos/acme/widgets/pulls/99'], { cwd: '/repo-root' }) + expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(2) expect(pr).toMatchObject({ number: 99, state: 'merged', diff --git a/src/main/github/client.ts b/src/main/github/client.ts index 531dfc8db..b02aa6fdf 100644 --- a/src/main/github/client.ts +++ b/src/main/github/client.ts @@ -1302,20 +1302,84 @@ async function getRestPRByNumber( return mapRestPullRequest(JSON.parse(stdout) as RestPullRequest) } +async function getPRByNumber( + ownerRepo: OwnerRepo, + number: number, + ghOptions: ReturnType +): Promise { + try { + const { stdout } = await ghExecFileAsync( + [ + 'pr', + 'view', + String(number), + '--repo', + `${ownerRepo.owner}/${ownerRepo.repo}`, + '--json', + PR_LOOKUP_JSON_FIELDS + ], + ghOptions + ) + return JSON.parse(stdout) as PullRequestLookupData + } catch (err) { + // Why: deleted or manually edited linked PR metadata should fall back to + // branch discovery; quota/auth/network failures get one cheaper REST exact lookup. + if (isNotFoundGhError(err)) { + return null + } + try { + return await getRestPRByNumber(ownerRepo, number, ghOptions) + } catch (restErr) { + if (isNotFoundGhError(restErr)) { + return null + } + if (!shouldStopAfterExactLookupError(restErr)) { + return null + } + throw restErr + } + } +} + +async function exactPRMatchesWorktreeHead( + repoPath: string, + branchName: string, + 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 + } +} + function isNotFoundGhError(err: unknown): boolean { const stderr = err instanceof Error ? err.message : String(err) return classifyGhError(stderr).type === 'not_found' } +function shouldStopAfterExactLookupError(err: unknown): boolean { + const stderr = err instanceof Error ? err.message : String(err) + const type = classifyGhError(stderr).type + return type === 'rate_limited' || type === 'permission_denied' || type === 'network_error' +} + /** * Get PR info for a given branch using gh CLI. * Returns null if gh is not installed, or no PR exists for the branch. * - * When `linkedPRNumber` is provided and the branch lookup yields nothing, - * falls back to looking up the PR by number. This handles "create from PR" - * worktrees, whose branch is a fresh local branch (not the PR's head ref) — - * the branch-keyed lookup misses, but the user still expects the linked PR - * to surface on the worktree card. + * When `linkedPRNumber` is provided and the repo identity is known, starts + * with a direct PR-number lookup. This handles "create from PR" worktrees, + * whose branch is a fresh local branch, and avoids spending a branch-list + * request before asking for the exact PR the worktree already stores. */ export async function getPRForBranch( repoPath: string, @@ -1332,11 +1396,22 @@ export async function getPRForBranch( try { const ownerRepo = await getOwnerRepo(repoPath, connectionId) let data: PullRequestLookupData | null = null + let exactLinkedData: PullRequestLookupData | 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 + } + } // During a rebase the worktree is in detached HEAD and branch is empty. // 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 (branchName) { + if (!data && branchName) { if (ownerRepo) { try { const { stdout } = await ghExecFileAsync( @@ -1375,33 +1450,24 @@ export async function getPRForBranch( } } - if (!data && typeof linkedPRNumber === 'number') { - const args = ownerRepo - ? [ - 'pr', - 'view', - String(linkedPRNumber), - '--repo', - `${ownerRepo.owner}/${ownerRepo.repo}`, - '--json', - PR_LOOKUP_JSON_FIELDS - ] - : ['pr', 'view', String(linkedPRNumber), '--json', PR_LOOKUP_JSON_FIELDS] + if (!data && !ownerRepo && typeof linkedPRNumber === 'number') { + const args = ['pr', 'view', String(linkedPRNumber), '--json', PR_LOOKUP_JSON_FIELDS] try { const { stdout } = await ghExecFileAsync(args, ghOptions) data = JSON.parse(stdout) - } catch (err) { + } catch { // Why: a stale linkedPRNumber (PR deleted, wrong repo, …) makes // `gh pr view ` reject. Treat that as the no-PR case so // callers see the historical `null` semantics instead of a thrown // error every poll cycle. - data = - ownerRepo && !isNotFoundGhError(err) - ? await getRestPRByNumber(ownerRepo, linkedPRNumber, ghOptions) - : null + data = null } } + if (!data && exactLinkedData) { + data = exactLinkedData + } + if (!data) { return null } diff --git a/src/renderer/src/components/right-sidebar/SourceControl.tsx b/src/renderer/src/components/right-sidebar/SourceControl.tsx index 9a3bc672e..df9261448 100644 --- a/src/renderer/src/components/right-sidebar/SourceControl.tsx +++ b/src/renderer/src/components/right-sidebar/SourceControl.tsx @@ -656,7 +656,8 @@ function SourceControlInner(): React.JSX.Element { void fetchHostedReviewForBranch(activeRepo.path, branchName, { repoId: activeRepo.id, linkedGitHubPR, - linkedGitLabMR + linkedGitLabMR, + staleWhileRevalidate: true }) }, [ activeRepo, diff --git a/src/renderer/src/components/sidebar/WorktreeCard.tsx b/src/renderer/src/components/sidebar/WorktreeCard.tsx index 2399cafd9..df108477b 100644 --- a/src/renderer/src/components/sidebar/WorktreeCard.tsx +++ b/src/renderer/src/components/sidebar/WorktreeCard.tsx @@ -211,7 +211,8 @@ const WorktreeCard = React.memo(function WorktreeCard({ fetchHostedReviewForBranch(repo.path, branch, { repoId: repo.id, linkedGitHubPR: worktree.linkedPR ?? null, - linkedGitLabMR: worktree.linkedGitLabMR ?? null + linkedGitLabMR: worktree.linkedGitLabMR ?? null, + staleWhileRevalidate: true }) } }, [ diff --git a/src/renderer/src/store/slices/hosted-review.test.ts b/src/renderer/src/store/slices/hosted-review.test.ts index b976ed0e8..1d601022b 100644 --- a/src/renderer/src/store/slices/hosted-review.test.ts +++ b/src/renderer/src/store/slices/hosted-review.test.ts @@ -1,7 +1,11 @@ -import { beforeEach, describe, expect, it, vi } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { create } from 'zustand' import type { AppState } from '../types' -import { createHostedReviewSlice, refreshHostedReviewCard } from './hosted-review' +import { + createHostedReviewSlice, + getHostedReviewCacheKey, + refreshHostedReviewCard +} from './hosted-review' import type { HostedReviewInfo } from '../../../../shared/hosted-review' const runtimeRpc = vi.hoisted(() => ({ @@ -52,6 +56,10 @@ describe('hosted review slice', () => { runtimeRpc.callRuntimeRpc.mockReset() }) + afterEach(() => { + vi.useRealTimers() + }) + it('fetches and caches branch review status through the common IPC surface', async () => { mockApi.hostedReview.forBranch.mockResolvedValueOnce(review) const store = makeStore() @@ -201,4 +209,79 @@ describe('hosted review slice', () => { await expect(firstLinkedFetch).resolves.toEqual(review) await expect(secondLinkedFetch).resolves.toEqual(review) }) + + it('serves stale hosted review metadata while revalidating in the background', async () => { + vi.useFakeTimers() + vi.setSystemTime(0) + const updatedReview: HostedReviewInfo = { + ...review, + title: 'Updated linked PR status', + status: 'failure', + updatedAt: '2026-05-10T00:01:01.000Z' + } + let resolveRefresh: (value: typeof updatedReview) => void = () => {} + const refresh = new Promise((resolve) => { + resolveRefresh = resolve + }) + mockApi.hostedReview.forBranch + .mockResolvedValueOnce(review) + .mockReturnValueOnce(refresh as Promise) + const store = makeStore() + + await expect( + store.getState().fetchHostedReviewForBranch('/repo', 'feature/pr', { + linkedGitHubPR: 42 + }) + ).resolves.toEqual(review) + vi.setSystemTime(60_001) + await expect( + store.getState().fetchHostedReviewForBranch('/repo', 'feature/pr', { + linkedGitHubPR: 42, + staleWhileRevalidate: true + }) + ).resolves.toEqual(review) + await expect( + store.getState().fetchHostedReviewForBranch('/repo', 'feature/pr', { + linkedGitHubPR: 42, + staleWhileRevalidate: true + }) + ).resolves.toEqual(review) + + expect(mockApi.hostedReview.forBranch).toHaveBeenCalledTimes(2) + const cacheKey = getHostedReviewCacheKey('/repo', 'feature/pr') + expect(store.getState().hostedReviewCache[cacheKey]?.data).toEqual(review) + + resolveRefresh(updatedReview) + await refresh + await Promise.resolve() + + expect(store.getState().hostedReviewCache[cacheKey]?.data).toEqual(updatedReview) + }) + + it('does not serve stale metadata when a stronger linked PR hint changes the lookup', async () => { + vi.useFakeTimers() + vi.setSystemTime(0) + const linkedReview: HostedReviewInfo = { + ...review, + provider: 'github', + number: 42, + title: 'Exact linked PR', + url: 'https://github.com/acme/orca/pull/42' + } + mockApi.hostedReview.forBranch.mockResolvedValueOnce(review).mockResolvedValueOnce(linkedReview) + const store = makeStore() + + await expect(store.getState().fetchHostedReviewForBranch('/repo', 'feature/pr')).resolves.toBe( + review + ) + vi.setSystemTime(60_001) + await expect( + store.getState().fetchHostedReviewForBranch('/repo', 'feature/pr', { + linkedGitHubPR: 42, + staleWhileRevalidate: true + }) + ).resolves.toEqual(linkedReview) + + expect(mockApi.hostedReview.forBranch).toHaveBeenCalledTimes(2) + }) }) diff --git a/src/renderer/src/store/slices/hosted-review.ts b/src/renderer/src/store/slices/hosted-review.ts index dc14ad06b..f2f21b0d5 100644 --- a/src/renderer/src/store/slices/hosted-review.ts +++ b/src/renderer/src/store/slices/hosted-review.ts @@ -11,7 +11,7 @@ import { callRuntimeRpc, getActiveRuntimeTarget } from '@/runtime/runtime-rpc-cl import type { AppState } from '../types' type CacheEntry = { data: T | null; fetchedAt: number; linkedReviewHintKey?: string } -type FetchOptions = { force?: boolean; repoId?: string } +type FetchOptions = { force?: boolean; repoId?: string; staleWhileRevalidate?: boolean } type LinkedReviewHints = { linkedGitHubPR?: number | null linkedGitLabMR?: number | null @@ -36,7 +36,7 @@ function isFresh(entry: CacheEntry | undefined): entry is CacheEntry { return entry !== undefined && Date.now() - entry.fetchedAt < CACHE_TTL_MS } -// Why: a branch-only null is weaker than a null after trying the persisted +// Why: a branch-keyed lookup can describe a different PR than the persisted // linked review number. Track that distinction without changing the cache key. function linkedReviewHintKey(options?: LinkedReviewHints): string { const hints = [ @@ -51,11 +51,11 @@ function linkedReviewHintKey(options?: LinkedReviewHints): string { .join('|') } -function shouldRefetchNullForLinkedHint( +function shouldRefetchForLinkedHint( cached: CacheEntry | undefined, hintKey: string ): boolean { - return cached?.data === null && hintKey !== '' && (cached.linkedReviewHintKey ?? '') !== hintKey + return cached !== undefined && hintKey !== '' && (cached.linkedReviewHintKey ?? '') !== hintKey } function canReuseInflightHint(inflightHintKey: string, nextHintKey: string): boolean { @@ -175,7 +175,7 @@ export const createHostedReviewSlice: StateCreator => { + const generation = (requestGenerations.get(cacheKey) ?? 0) + 1 + requestGenerations.set(cacheKey, generation) + const request = (async () => { + try { + const args = { + branch, + ...(options?.repoId !== undefined ? { repoId: options.repoId } : {}), + linkedGitHubPR: options?.linkedGitHubPR ?? null, + linkedGitLabMR: options?.linkedGitLabMR ?? null, + linkedBitbucketPR: options?.linkedBitbucketPR ?? null, + linkedGiteaPR: options?.linkedGiteaPR ?? null + } + const review = + target.kind === 'environment' + ? await callRuntimeRpc( + target, + 'hostedReview.forBranch', + { repo: options?.repoId ?? repoPath, repoPath, ...args }, + // Why: remote dev boxes can be slower at `git`/`gh` lookups + // than local desktop repos, especially on Windows filesystem + // paths. The main-process queue caps concurrency, so a longer + // timeout no longer risks a background socket stampede. + { timeoutMs: 30_000 } + ) + : await window.api.hostedReview.forBranch({ repoPath, ...args }) + if (requestGenerations.get(cacheKey) === generation) { + set((state) => ({ + hostedReviewCache: { + ...state.hostedReviewCache, + [cacheKey]: { data: review, fetchedAt: Date.now(), linkedReviewHintKey: hintKey } + } + })) + } + return review + } catch (error) { + console.error('Failed to fetch hosted review:', error) + if (requestGenerations.get(cacheKey) === generation) { + set((state) => ({ + hostedReviewCache: { + ...state.hostedReviewCache, + [cacheKey]: { data: null, fetchedAt: Date.now(), linkedReviewHintKey: hintKey } + } + })) + } + return null + } finally { + const activeRequest = inflightHostedReviewRequests.get(cacheKey) + if (activeRequest?.generation === generation) { + inflightHostedReviewRequests.delete(cacheKey) + } + } + })() + + inflightHostedReviewRequests.set(cacheKey, { + promise: request, + force: Boolean(options?.force), + generation, + linkedReviewHintKey: hintKey + }) + return request + } + + if ( + !options?.force && + !linkedRefetch && + options?.staleWhileRevalidate && + cached !== undefined && + cached.data !== null + ) { + // Why: sidebar PR metadata can stay visible while a quiet refresh updates + // it; don't block card rendering on a quota-bound GitHub round trip. + if (!inflightRequest || !inflightHasRequestedHint) { + void startRequest() + } + return cached.data + } + if (inflightRequest && (!options?.force || inflightRequest.force) && inflightHasRequestedHint) { return inflightRequest.promise } - const generation = (requestGenerations.get(cacheKey) ?? 0) + 1 - requestGenerations.set(cacheKey, generation) - - const request = (async () => { - try { - const args = { - branch, - ...(options?.repoId !== undefined ? { repoId: options.repoId } : {}), - linkedGitHubPR: options?.linkedGitHubPR ?? null, - linkedGitLabMR: options?.linkedGitLabMR ?? null, - linkedBitbucketPR: options?.linkedBitbucketPR ?? null, - linkedGiteaPR: options?.linkedGiteaPR ?? null - } - const review = - target.kind === 'environment' - ? await callRuntimeRpc( - target, - 'hostedReview.forBranch', - { repo: options?.repoId ?? repoPath, repoPath, ...args }, - // Why: remote dev boxes can be slower at `git`/`gh` lookups - // than local desktop repos, especially on Windows filesystem - // paths. The main-process queue caps concurrency, so a longer - // timeout no longer risks a background socket stampede. - { timeoutMs: 30_000 } - ) - : await window.api.hostedReview.forBranch({ repoPath, ...args }) - if (requestGenerations.get(cacheKey) === generation) { - set((state) => ({ - hostedReviewCache: { - ...state.hostedReviewCache, - [cacheKey]: { data: review, fetchedAt: Date.now(), linkedReviewHintKey: hintKey } - } - })) - } - return review - } catch (error) { - console.error('Failed to fetch hosted review:', error) - if (requestGenerations.get(cacheKey) === generation) { - set((state) => ({ - hostedReviewCache: { - ...state.hostedReviewCache, - [cacheKey]: { data: null, fetchedAt: Date.now(), linkedReviewHintKey: hintKey } - } - })) - } - return null - } finally { - const activeRequest = inflightHostedReviewRequests.get(cacheKey) - if (activeRequest?.generation === generation) { - inflightHostedReviewRequests.delete(cacheKey) - } - } - })() - - inflightHostedReviewRequests.set(cacheKey, { - promise: request, - force: Boolean(options?.force), - generation, - linkedReviewHintKey: hintKey - }) - return request + return startRequest() } })