From fb4069be527641fefc5a7d202092a151af8f6ad6 Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Tue, 19 May 2026 18:39:44 -0400 Subject: [PATCH] Improve GitHub PR status refresh freshness (#1716) Co-authored-by: Orca --- src/main/github/client-pr-checks.test.ts | 24 +- src/main/github/client.test.ts | 118 ++-- src/main/github/client.ts | 357 +++++++---- .../github/pr-refresh-coordinator.test.ts | 338 ++++++++++ src/main/github/pr-refresh-coordinator.ts | 576 ++++++++++++++++++ src/main/ipc/github.ts | 91 ++- src/preload/api-types.ts | 15 + src/preload/index.ts | 24 + src/renderer/src/App.tsx | 5 + .../components/right-sidebar/ChecksPanel.tsx | 151 ++++- .../right-sidebar/SourceControl.tsx | 22 +- .../src/components/sidebar/WorktreeList.tsx | 50 ++ src/renderer/src/hooks/useIpcEvents.ts | 13 +- src/renderer/src/store/slices/editor.ts | 16 + .../src/store/slices/github-checks.ts | 6 +- src/renderer/src/store/slices/github.test.ts | 402 +++++++++++- src/renderer/src/store/slices/github.ts | 411 +++++++++++-- src/renderer/src/store/slices/ssh.ts | 12 +- src/renderer/src/store/slices/worktrees.ts | 5 +- src/renderer/src/web/web-preload-api.ts | 14 + src/shared/types.ts | 82 +++ 21 files changed, 2449 insertions(+), 283 deletions(-) create mode 100644 src/main/github/pr-refresh-coordinator.test.ts create mode 100644 src/main/github/pr-refresh-coordinator.ts diff --git a/src/main/github/client-pr-checks.test.ts b/src/main/github/client-pr-checks.test.ts index 1c23e1ba6..928da2776 100644 --- a/src/main/github/client-pr-checks.test.ts +++ b/src/main/github/client-pr-checks.test.ts @@ -7,6 +7,7 @@ const { getIssueOwnerRepoMock, gitExecFileAsyncMock, extractExecErrorMock, + getRateLimitMock, rateLimitGuardMock, noteRateLimitSpendMock, acquireMock, @@ -27,6 +28,7 @@ const { } return { stderr: String(err), stdout: '' } }), + getRateLimitMock: vi.fn(), rateLimitGuardMock: vi.fn(() => ({ blocked: false })), noteRateLimitSpendMock: vi.fn(), acquireMock: vi.fn(), @@ -54,6 +56,7 @@ vi.mock('../git/runner', () => ({ })) vi.mock('./rate-limit', () => ({ + getRateLimit: getRateLimitMock, rateLimitGuard: rateLimitGuardMock, noteRateLimitSpend: noteRateLimitSpendMock })) @@ -68,6 +71,8 @@ describe('getPRChecks', () => { getIssueOwnerRepoMock.mockReset() gitExecFileAsyncMock.mockReset() extractExecErrorMock.mockClear() + getRateLimitMock.mockReset() + getRateLimitMock.mockResolvedValue({ resources: {} }) rateLimitGuardMock.mockReset() rateLimitGuardMock.mockReturnValue({ blocked: false }) noteRateLimitSpendMock.mockReset() @@ -157,7 +162,7 @@ describe('getPRChecks', () => { consoleWarnSpy.mockRestore() }) - it('keeps unexpected gh pr checks fallback failures inside the empty-checks contract', async () => { + it('throws unexpected gh pr checks fallback failures so callers preserve cache', async () => { const consoleWarnSpy = vi.spyOn(console, 'warn').mockImplementation(() => undefined) getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) ghExecFileAsyncMock @@ -169,12 +174,8 @@ describe('getPRChecks', () => { }) ) - const checks = await getPRChecks('/repo-root', 42, 'head-oid') - - expect(checks).toEqual([]) - expect(consoleWarnSpy).toHaveBeenCalledWith( - 'getPRChecks via head SHA failed, falling back to gh pr checks:', - expect.any(Error) + await expect(getPRChecks('/repo-root', 42, 'head-oid')).rejects.toThrow( + 'Command failed: gh pr checks 42' ) expect(consoleWarnSpy).toHaveBeenCalledWith('getPRChecks failed:', expect.any(Error)) consoleWarnSpy.mockRestore() @@ -252,4 +253,13 @@ describe('getPRChecks', () => { { cwd: '/repo-root' } ) }) + + it('throws when both check-runs and gh pr checks fail', async () => { + getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) + ghExecFileAsyncMock + .mockRejectedValueOnce(new Error('gh: No commit found for SHA: stale-head (HTTP 422)')) + .mockRejectedValueOnce(new Error('rate limited')) + + await expect(getPRChecks('/repo-root', 42, 'stale-head')).rejects.toThrow('rate limited') + }) }) diff --git a/src/main/github/client.test.ts b/src/main/github/client.test.ts index f278624ae..af2677f4e 100644 --- a/src/main/github/client.test.ts +++ b/src/main/github/client.test.ts @@ -14,6 +14,7 @@ const { resolvePRRepositoryCandidatesMock, getRemoteUrlForRepoMock, gitExecFileAsyncMock, + getRateLimitMock, rateLimitGuardMock, noteRateLimitSpendMock, ghRepoExecOptionsMock, @@ -29,6 +30,7 @@ const { resolvePRRepositoryCandidatesMock: vi.fn(), getRemoteUrlForRepoMock: vi.fn(), gitExecFileAsyncMock: vi.fn(), + getRateLimitMock: vi.fn(), rateLimitGuardMock: vi.fn<() => RateLimitGuardResult>(() => ({ blocked: false })), noteRateLimitSpendMock: vi.fn(), ghRepoExecOptionsMock: vi.fn((context) => @@ -77,6 +79,7 @@ vi.mock('../git/runner', () => ({ })) vi.mock('./rate-limit', () => ({ + getRateLimit: getRateLimitMock, rateLimitGuard: rateLimitGuardMock, noteRateLimitSpend: noteRateLimitSpendMock })) @@ -106,6 +109,8 @@ describe('getPRForBranch', () => { }) getRemoteUrlForRepoMock.mockReset() gitExecFileAsyncMock.mockReset() + getRateLimitMock.mockReset() + getRateLimitMock.mockResolvedValue({ resources: {} }) rateLimitGuardMock.mockReset() rateLimitGuardMock.mockReturnValue({ blocked: false }) noteRateLimitSpendMock.mockReset() @@ -268,9 +273,7 @@ describe('getPRForBranch', () => { const pr = await getPRForBranch('/repo-root', 'feature/local-worktree', 99) expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(1) - expect(gitExecFileAsyncMock).toHaveBeenCalledWith(['rev-parse', 'HEAD'], { - cwd: '/repo-root' - }) + expect(gitExecFileAsyncMock).not.toHaveBeenCalled() expect(ghExecFileAsyncMock).toHaveBeenCalledWith( [ 'pr', @@ -291,7 +294,7 @@ describe('getPRForBranch', () => { }) }) - it('uses branch discovery when exact linked PR metadata resolves to a different PR', async () => { + it('treats linked PR metadata as authoritative even when the branch head differs', async () => { getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: 'current-worktree-head\n', stderr: '' }) ghExecFileAsyncMock @@ -332,16 +335,11 @@ describe('getPRForBranch', () => { const pr = await getPRForBranch('/repo-root', 'feature/test', 99) - expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(2) - expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( - 2, - ['api', 'repos/acme/widgets/pulls?head=acme%3Afeature%2Ftest&state=all&per_page=1'], - { cwd: '/repo-root' } - ) - expect(pr?.number).toBe(42) + expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(1) + expect(pr?.number).toBe(99) }) - it('falls back to branch discovery when exact linked PR metadata is stale', async () => { + it('does not fall back to branch discovery when linked PR metadata is stale', async () => { getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) ghExecFileAsyncMock .mockRejectedValueOnce(new Error('HTTP 404: Not Found')) @@ -379,15 +377,11 @@ describe('getPRForBranch', () => { ], { cwd: '/repo-root' } ) - expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( - 2, - ['api', 'repos/acme/widgets/pulls?head=acme%3Afeature%2Ftest&state=all&per_page=1'], - { cwd: '/repo-root' } - ) - expect(pr?.number).toBe(42) + expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(1) + expect(pr).toBeNull() }) - it('continues to branch discovery when exact linked PR REST fallback also misses', async () => { + it('returns no PR when linked PR REST fallback also misses', async () => { getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) ghExecFileAsyncMock .mockRejectedValueOnce(new Error('GraphQL: could not resolve to PullRequest')) @@ -416,15 +410,11 @@ describe('getPRForBranch', () => { expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith(2, ['api', 'repos/acme/widgets/pulls/99'], { cwd: '/repo-root' }) - expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( - 3, - ['api', 'repos/acme/widgets/pulls?head=acme%3Afeature%2Ftest&state=all&per_page=1'], - { cwd: '/repo-root' } - ) - expect(pr?.number).toBe(42) + expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(2) + expect(pr).toBeNull() }) - it('continues to branch discovery when exact linked PR REST fallback has an unclassified failure', async () => { + it('returns no PR when linked PR REST fallback has an unclassified failure', async () => { getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) ghExecFileAsyncMock .mockRejectedValueOnce(new Error('GraphQL: server exploded')) @@ -453,15 +443,11 @@ describe('getPRForBranch', () => { expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith(2, ['api', 'repos/acme/widgets/pulls/99'], { cwd: '/repo-root' }) - expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( - 3, - ['api', 'repos/acme/widgets/pulls?head=acme%3Afeature%2Ftest&state=all&per_page=1'], - { cwd: '/repo-root' } - ) - expect(pr?.number).toBe(42) + expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(2) + expect(pr).toBeNull() }) - it('continues to branch discovery when exact linked PR REST fallback is rate limited', async () => { + it('does not continue to branch discovery when linked PR REST fallback is rate limited', async () => { getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) ghExecFileAsyncMock .mockRejectedValueOnce(new Error('GraphQL: API rate limit already exceeded')) @@ -485,12 +471,7 @@ describe('getPRForBranch', () => { expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith(2, ['api', 'repos/acme/widgets/pulls/99'], { cwd: '/repo-root' }) - 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(ghExecFileAsyncMock).toHaveBeenCalledTimes(2) expect(pr).toBeNull() }) @@ -530,6 +511,43 @@ describe('getPRForBranch', () => { }) }) + it('uses linked PR number as the source of truth when provided', async () => { + getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) + ghExecFileAsyncMock.mockResolvedValueOnce({ + stdout: JSON.stringify({ + number: 77, + title: 'Linked PR lookup', + state: 'OPEN', + url: 'https://github.com/acme/widgets/pull/77', + statusCheckRollup: [], + updatedAt: '2026-03-28T00:00:00Z', + isDraft: false, + mergeable: 'MERGEABLE', + baseRefName: 'main', + headRefName: 'contributor/original', + baseRefOid: 'base-oid', + headRefOid: 'head-oid' + }) + }) + + const pr = await getPRForBranch('/repo-root', 'refs/heads/local-created-from-pr', 77) + + expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(1) + expect(ghExecFileAsyncMock).toHaveBeenCalledWith( + [ + 'pr', + 'view', + '77', + '--repo', + 'acme/widgets', + '--json', + 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,baseRefName,headRefName,baseRefOid,headRefOid' + ], + { cwd: '/repo-root' } + ) + expect(pr?.number).toBe(77) + }) + it('falls back to gh pr view when the remote cannot be resolved to GitHub', async () => { getOwnerRepoMock.mockResolvedValueOnce(null) ghExecFileAsyncMock.mockResolvedValueOnce({ @@ -891,12 +909,10 @@ describe('GitHub GraphQL rate-limit guard', () => { }) it('skips PR review-thread GraphQL fetch while preserving REST comments', async () => { - rateLimitGuardMock.mockReturnValue({ - blocked: true, - remaining: 4, - limit: 5000, - resetAt: 1_800_000_000 - }) + rateLimitGuardMock.mockImplementation(((bucket: string) => + bucket === 'graphql' + ? { blocked: true, remaining: 4, limit: 5000, resetAt: 1_800_000_000 } + : { blocked: false }) as () => RateLimitGuardResult) getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) ghExecFileAsyncMock .mockResolvedValueOnce({ @@ -918,16 +934,14 @@ describe('GitHub GraphQL rate-limit guard', () => { expect(comments[0].body).toBe('top-level') expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(2) expect(ghExecFileAsyncMock.mock.calls.some((call) => call[0][1] === 'graphql')).toBe(false) - expect(noteRateLimitSpendMock).not.toHaveBeenCalled() + expect(noteRateLimitSpendMock).not.toHaveBeenCalledWith('graphql') }) 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 - }) + rateLimitGuardMock.mockImplementation(((bucket: string) => + bucket === 'graphql' + ? { blocked: true, remaining: 4, limit: 5000, resetAt: 1_800_000_000 } + : { blocked: false }) as () => RateLimitGuardResult) ghExecFileAsyncMock .mockResolvedValueOnce({ stdout: JSON.stringify([ diff --git a/src/main/github/client.ts b/src/main/github/client.ts index 4a4571aae..233cecf16 100644 --- a/src/main/github/client.ts +++ b/src/main/github/client.ts @@ -6,6 +6,7 @@ import type { IssueSourcePreference, ListWorkItemsResult, PRInfo, + PRRefreshOutcome, PRMergeableState, PRCheckDetail, PRCheckRunDetails, @@ -66,10 +67,88 @@ import { deriveCheckStatus } from './mappers' import { mapGraphQLReactionGroups, type GitHubGraphQLReactionGroup } from './comment-reactions' -import { noteRateLimitSpend, rateLimitGuard } from './rate-limit' +import { + getRateLimit, + noteRateLimitSpend, + rateLimitGuard, + type RateLimitBucketKind +} from './rate-limit' const ORCA_REPO = 'stablyai/orca' +async function assertRateLimitBudget(bucket: RateLimitBucketKind): Promise { + await getRateLimit() + const guard = rateLimitGuard(bucket) + if (guard.blocked) { + throw new Error( + `GitHub ${bucket} rate limit is low; retry after ${new Date(guard.resetAt * 1000).toLocaleTimeString()}` + ) + } +} + +function classifyPRRefreshError( + err: unknown +): Extract['errorType'] { + const message = err instanceof Error ? err.message : String(err) + const lower = message.toLowerCase() + if (lower.includes('rate limit')) { + return 'rate_limited' + } + if ( + lower.includes('timeout') || + lower.includes('no such host') || + lower.includes('network') || + lower.includes('could not resolve host') + ) { + return 'network' + } + if (lower.includes('http 403') || lower.includes('resource not accessible')) { + return 'permission' + } + if (lower.includes('http 404') || lower.includes('could not resolve to a repository')) { + return 'repo_unavailable' + } + return /auth|login|credential/i.test(message) ? 'auth' : 'unknown' +} + +function safePRRefreshErrorMessage( + errorType: Extract['errorType'] +): string { + switch (errorType) { + case 'rate_limited': + return 'GitHub rate limit is low. Try again after the limit resets.' + case 'auth': + return 'GitHub authentication is unavailable. Check your gh login.' + case 'network': + return 'GitHub is unreachable right now. Check your network and try again.' + case 'permission': + return 'GitHub did not allow access to this pull request.' + case 'repo_unavailable': + return 'The GitHub repository is unavailable or cannot be resolved.' + case 'gh_unavailable': + return 'GitHub CLI is unavailable.' + case 'unknown': + return 'GitHub pull request refresh failed.' + } +} + +function prRefreshUpstreamError( + err: unknown +): Extract { + const errorType = classifyPRRefreshError(err) + return { + kind: 'upstream-error', + errorType, + message: safePRRefreshErrorMessage(errorType), + fetchedAt: Date.now() + } +} + +function isNoPullRequestError(err: unknown): boolean { + const message = err instanceof Error ? err.message : String(err) + return /no pull requests? found|could not find.*pull request/i.test(message) +} + /** * Check if the authenticated user has starred the Orca repo. * Returns true if starred, false if not, null if unable to determine (gh unavailable). @@ -1586,23 +1665,6 @@ async function getPRByNumber( } } -async function exactPRMatchesWorktreeHead( - repoPath: string, - branchName: string, - data: PullRequestLookupData, - connectionId?: string | null -): Promise { - 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 { const stderr = err instanceof Error ? err.message : String(err) return classifyGhError(stderr).type === 'not_found' @@ -1611,17 +1673,17 @@ function isNotFoundGhError(err: unknown): boolean { 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' + return type !== 'not_found' } /** * 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 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. + * When `linkedPRNumber` is provided, it is the source of truth. This handles + * "create from PR" worktrees whose local branch differs from the PR head ref, + * and prevents a coalesced linked-PR refresh from fanning out an unrelated + * branch lookup result to sibling aliases. */ export async function getPRForBranch( repoPath: string, @@ -1629,10 +1691,20 @@ export async function getPRForBranch( linkedPRNumber?: number | null, connectionId?: string | null ): Promise { + const outcome = await getPRForBranchOutcome(repoPath, branch, linkedPRNumber, connectionId) + return outcome.kind === 'found' ? outcome.pr : null +} + +export async function getPRForBranchOutcome( + repoPath: string, + branch: string, + linkedPRNumber?: number | null, + connectionId?: string | null +): Promise { // Strip refs/heads/ prefix if present const branchName = branch.replace(/^refs\/heads\//, '') if (!branchName && typeof linkedPRNumber !== 'number') { - return null + return { kind: 'no-pr', fetchedAt: Date.now() } } const context = githubRepoContext(repoPath, connectionId) const ghOptions = ghRepoExecOptions(context) @@ -1642,8 +1714,6 @@ export async function getPRForBranch( 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 (typeof linkedPRNumber === 'number') { for (const candidate of candidates) { @@ -1652,25 +1722,36 @@ export async function getPRForBranch( if (!linkedData) { continue } - if (await exactPRMatchesWorktreeHead(repoPath, branchName, linkedData, connectionId)) { - data = linkedData - dataRepo = candidate - break + data = linkedData + dataRepo = candidate + break + } catch (err) { + if (shouldStopAfterExactLookupError(err)) { + throw err } - // 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. } } - } - // 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 (!data && branchName) { + if (!data && candidates.length === 0) { + const args = ['pr', 'view', String(linkedPRNumber), '--json', PR_LOOKUP_JSON_FIELDS] + try { + const { stdout } = await ghExecFileAsync(args, ghOptions) + data = JSON.parse(stdout) + } catch (err) { + if (!isNoPullRequestError(err)) { + return prRefreshUpstreamError(err) + } + // 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 = null + } + } + } else if (branchName) { + // During a rebase the worktree is in detached HEAD and branch is empty. + // An empty --head filter causes gh to return an arbitrary PR. if (candidates.length > 0) { for (const candidate of candidates) { try { @@ -1682,49 +1763,35 @@ export async function getPRForBranch( 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. + if (headRepo) { + throw err + } else { + data = await getRestPRForBranch(candidate, candidate.owner, branchName, ghOptions) + if (data) { + dataRepo = candidate + break } } } } } else { - const { stdout } = await ghExecFileAsync( - ['pr', 'view', branchName, '--json', PR_LOOKUP_JSON_FIELDS], - ghOptions - ) - data = JSON.parse(stdout) + try { + const { stdout } = await ghExecFileAsync( + ['pr', 'view', branchName, '--json', PR_LOOKUP_JSON_FIELDS], + ghOptions + ) + data = JSON.parse(stdout) + } catch (err) { + if (isNoPullRequestError(err)) { + data = null + } else { + throw err + } + } } } - - 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) - data = JSON.parse(stdout) - } 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 = null - } - } - - if (!data && exactLinkedData) { - data = exactLinkedData - dataRepo = exactLinkedRepo - } - if (!data) { - return null + return { kind: 'no-pr', fetchedAt: Date.now() } } const conflictSummary = @@ -1737,20 +1804,24 @@ export async function getPRForBranch( : undefined return { - number: data.number, - title: data.title, - state: mapPRState(data.state, data.isDraft), - url: data.url, - checksStatus: deriveCheckStatus(data.statusCheckRollup), - updatedAt: data.updatedAt, - mergeable: (data.mergeable as PRMergeableState) ?? 'UNKNOWN', - headSha: data.headRefOid, - prRepo: dataRepo ?? undefined, - headRepo: headRepo ?? undefined, - conflictSummary + kind: 'found', + fetchedAt: Date.now(), + pr: { + number: data.number, + title: data.title, + state: mapPRState(data.state, data.isDraft), + url: data.url, + checksStatus: deriveCheckStatus(data.statusCheckRollup), + updatedAt: data.updatedAt, + mergeable: (data.mergeable as PRMergeableState) ?? 'UNKNOWN', + headSha: data.headRefOid, + prRepo: dataRepo ?? undefined, + headRepo: headRepo ?? undefined, + conflictSummary + } } - } catch { - return null + } catch (err) { + return prRefreshUpstreamError(err) } finally { release() } @@ -1772,35 +1843,54 @@ export async function getPRChecks( const ghOptions = ghRepoExecOptions(githubRepoContext(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) { - fallbackArgs.push('--repo', `${ownerRepo.owner}/${ownerRepo.repo}`) - } - const { stdout } = await ghExecFileAsync(fallbackArgs, ghOptions).catch((err: unknown) => { - const { stderr } = extractExecError(err) - // Why: `gh pr checks` exits non-zero when a PR genuinely has no check - // runs yet. Treat that as an empty optional section, not a load failure. - if (stderr.toLowerCase().includes('no checks reported')) { - return { stdout: '[]', stderr } + // Why: the REST check-runs path spends core only. Guard GraphQL only when + // we actually fall back to `gh pr checks`, so a low GraphQL bucket does not + // block fresh REST check data. Keep this outside the gh lock because the + // guard may need its own `gh api rate_limit` call. + await assertRateLimitBudget('graphql') + await acquire() + try { + const fallbackArgs = ['pr', 'checks', String(prNumber), '--json', 'name,state,link'] + if (ownerRepo) { + fallbackArgs.push('--repo', `${ownerRepo.owner}/${ownerRepo.repo}`) } - throw err - }) - const data = JSON.parse(stdout) as { name: string; state: string; link: string }[] - return data.map((d) => ({ - name: d.name, - status: mapCheckStatus(d.state), - conclusion: mapCheckConclusion(d.state), - url: d.link || null, - workflowRunId: parseActionsRunId(d.link) - })) + const { stdout } = await ghExecFileAsync(fallbackArgs, ghOptions).catch((err: unknown) => { + const { stderr } = extractExecError(err) + // Why: `gh pr checks` exits non-zero when a PR genuinely has no check + // runs yet. Treat that as an empty optional section, not a load failure. + if (stderr.toLowerCase().includes('no checks reported')) { + return { stdout: '[]', stderr } + } + throw err + }) + noteRateLimitSpend('graphql') + const data = JSON.parse(stdout) as { name: string; state: string; link: string }[] + return data.map((d) => ({ + name: d.name, + status: mapCheckStatus(d.state), + conclusion: mapCheckConclusion(d.state), + url: d.link || null, + workflowRunId: parseActionsRunId(d.link) + })) + } finally { + release() + } } - await acquire() - try { - if (ownerRepo && headSha) { - // Why: --cache 60s saves rate-limit budget during polling, but when the - // user explicitly clicks refresh we must skip it so gh fetches fresh data. - const cacheArgs = options?.noCache ? [] : ['--cache', '60s'] + + if (ownerRepo && headSha) { + let canUseRestChecks = true + try { + await assertRateLimitBudget('core') + } catch (err) { + canUseRestChecks = false + console.warn('getPRChecks skipped REST check-runs, falling back to gh pr checks:', err) + } + if (canUseRestChecks) { + await acquire() try { + // Why: --cache 60s saves rate-limit budget during polling, but when the + // user explicitly clicks refresh we must skip it so gh fetches fresh data. + const cacheArgs = options?.noCache ? [] : ['--cache', '60s'] const { stdout } = await ghExecFileAsync( [ 'api', @@ -1809,6 +1899,7 @@ export async function getPRChecks( ], ghOptions ) + noteRateLimitSpend('core') const data = JSON.parse(stdout) as { check_runs: { id?: number @@ -1819,33 +1910,32 @@ export async function getPRChecks( details_url: string | null }[] } - if (data.check_runs.length === 0) { - const fallbackChecks = await fallbackToPRChecks() - return fallbackChecks + if (data.check_runs.length > 0) { + return data.check_runs.map((d) => ({ + name: d.name, + status: mapCheckRunRESTStatus(d.status), + conclusion: mapCheckRunRESTConclusion(d.status, d.conclusion), + url: d.details_url || d.html_url || null, + ...(typeof d.id === 'number' ? { checkRunId: d.id } : {}), + workflowRunId: parseActionsRunId(d.details_url || d.html_url || null) + })) } - return data.check_runs.map((d) => ({ - name: d.name, - status: mapCheckRunRESTStatus(d.status), - conclusion: mapCheckRunRESTConclusion(d.status, d.conclusion), - url: d.details_url || d.html_url || null, - ...(typeof d.id === 'number' ? { checkRunId: d.id } : {}), - workflowRunId: parseActionsRunId(d.details_url || d.html_url || null) - })) } catch (err) { // Why: a PR can outlive the cached head SHA after force-pushes or remote // rewrites. Falling back to `gh pr checks` keeps the panel populated // instead of rendering a false "no checks" state from a stale commit. console.warn('getPRChecks via head SHA failed, falling back to gh pr checks:', err) + } finally { + release() } } - // Fallback: no branch provided, empty check-runs, or non-GitHub remote. - const fallbackChecks = await fallbackToPRChecks() - return fallbackChecks + } + + try { + return await fallbackToPRChecks() } catch (err) { console.warn('getPRChecks failed:', err) - return [] - } finally { - release() + throw err } } @@ -2160,6 +2250,9 @@ export async function getPRComments( ): Promise { const ghOptions = ghRepoExecOptions(githubRepoContext(repoPath, connectionId)) const ownerRepo = options?.prRepo ?? (await getOwnerRepo(repoPath, connectionId)) + if (ownerRepo) { + await assertRateLimitBudget('core') + } await acquire() try { if (ownerRepo) { @@ -2209,6 +2302,7 @@ export async function getPRComments( ghOptions ) ]) + noteRateLimitSpend('core', 2) // Parse issue comments (REST) type RESTComment = { @@ -2360,6 +2454,7 @@ export async function getPRComments( ['pr', 'view', String(prNumber), '--json', 'comments'], ghOptions ) + noteRateLimitSpend('graphql') const data = JSON.parse(stdout) as { comments: { author: { login: string } diff --git a/src/main/github/pr-refresh-coordinator.test.ts b/src/main/github/pr-refresh-coordinator.test.ts new file mode 100644 index 000000000..726784bae --- /dev/null +++ b/src/main/github/pr-refresh-coordinator.test.ts @@ -0,0 +1,338 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type { GitHubPRRefreshCandidate, PRInfo } from '../../shared/types' + +const { sendMock, getAllWebContentsMock, getPRForBranchOutcomeMock, getRateLimitMock } = vi.hoisted( + () => ({ + sendMock: vi.fn(), + getAllWebContentsMock: vi.fn(), + getPRForBranchOutcomeMock: vi.fn(), + getRateLimitMock: vi.fn() + }) +) + +vi.mock('electron', () => ({ + webContents: { + getAllWebContents: getAllWebContentsMock + } +})) + +vi.mock('./client', () => ({ + getPRForBranchOutcome: getPRForBranchOutcomeMock +})) + +vi.mock('./rate-limit', () => ({ + getRateLimit: getRateLimitMock, + noteRateLimitSpend: vi.fn(), + rateLimitGuard: vi.fn(() => ({ blocked: false })) +})) + +function makeCandidate( + overrides: Partial = {} +): GitHubPRRefreshCandidate { + return { + cacheKey: '/repo::feature/test', + repoPath: '/repo', + branch: 'feature/test', + repoKind: 'git', + repoId: 'repo-1', + worktreeId: 'wt-1', + cachedFetchedAt: null, + ...overrides + } +} + +function makePR(overrides: Partial = {}): PRInfo { + return { + number: 12, + title: 'Test PR', + state: 'open', + url: 'https://github.com/acme/repo/pull/12', + checksStatus: 'pending', + updatedAt: '2026-05-12T00:00:00Z', + mergeable: 'UNKNOWN', + headSha: 'head-sha', + ...overrides + } +} + +function deferred(): { + promise: Promise + resolve: (value: T) => void + reject: (error: unknown) => void +} { + let resolve!: (value: T) => void + let reject!: (error: unknown) => void + const promise = new Promise((res, rej) => { + resolve = res + reject = rej + }) + return { promise, resolve, reject } +} + +describe('pr-refresh-coordinator', () => { + beforeEach(() => { + vi.resetModules() + vi.useFakeTimers() + vi.setSystemTime(1_000) + sendMock.mockReset() + getAllWebContentsMock.mockReset() + getPRForBranchOutcomeMock.mockReset() + getRateLimitMock.mockReset() + getAllWebContentsMock.mockReturnValue([ + { + id: 1, + isDestroyed: () => false, + send: sendMock + } + ]) + getRateLimitMock.mockResolvedValue({ ok: true }) + }) + + afterEach(() => { + vi.useRealTimers() + }) + + it('does not show visible background refreshes as queued', async () => { + const { reportVisiblePRRefreshCandidates } = await import('./pr-refresh-coordinator') + getPRForBranchOutcomeMock.mockResolvedValueOnce({ + kind: 'found', + pr: makePR({ checksStatus: 'pending' }), + fetchedAt: Date.now() + }) + + reportVisiblePRRefreshCandidates([makeCandidate()], 1, 1) + await vi.runOnlyPendingTimersAsync() + + const queuedEvents = sendMock.mock.calls + .map(([, event]) => event) + .filter((event) => event.status === 'queued') + + expect(queuedEvents).toHaveLength(0) + expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(1) + }) + + it('lets an active worktree refresh bypass a delayed visible follow-up', async () => { + const { enqueuePRRefresh, reportVisiblePRRefreshCandidates } = + await import('./pr-refresh-coordinator') + getPRForBranchOutcomeMock + .mockResolvedValueOnce({ + kind: 'found', + pr: makePR({ checksStatus: 'pending' }), + fetchedAt: Date.now() + }) + .mockResolvedValueOnce({ + kind: 'found', + pr: makePR({ checksStatus: 'success' }), + fetchedAt: Date.now() + }) + + const candidate = makeCandidate() + reportVisiblePRRefreshCandidates([candidate], 1, 1) + await vi.runOnlyPendingTimersAsync() + enqueuePRRefresh({ ...candidate, cachedFetchedAt: Date.now() }, 'active', 80, 1) + await vi.runOnlyPendingTimersAsync() + + const inFlightEvents = sendMock.mock.calls + .map(([, event]) => event) + .filter((event) => event.status === 'in-flight') + + expect(inFlightEvents.map((event) => event.reason)).toEqual(['visible', 'active']) + expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(2) + }) + + it('lets a repeated active refresh pull forward an equal-priority visible follow-up', async () => { + const { enqueuePRRefresh } = await import('./pr-refresh-coordinator') + getPRForBranchOutcomeMock + .mockResolvedValueOnce({ + kind: 'found', + pr: makePR({ checksStatus: 'success' }), + fetchedAt: Date.now() + }) + .mockResolvedValueOnce({ + kind: 'found', + pr: makePR({ checksStatus: 'success', state: 'merged' }), + fetchedAt: Date.now() + }) + + const candidate = makeCandidate() + enqueuePRRefresh(candidate, 'active', 80, 1) + await vi.runOnlyPendingTimersAsync() + + enqueuePRRefresh( + { + ...candidate, + cachedFetchedAt: Date.now(), + cachedChecksStatus: 'success' + }, + 'active', + 80, + 1 + ) + await vi.runOnlyPendingTimersAsync() + + const inFlightEvents = sendMock.mock.calls + .map(([, event]) => event) + .filter((event) => event.status === 'in-flight') + const queuedEvents = sendMock.mock.calls + .map(([, event]) => event) + .filter((event) => event.status === 'queued') + + expect(inFlightEvents.map((event) => event.reason)).toEqual(['active', 'active']) + expect(queuedEvents).toHaveLength(0) + expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(2) + }) + + it('preserves an active refresh queued while a visible refresh is in flight', async () => { + const { enqueuePRRefresh, reportVisiblePRRefreshCandidates } = + await import('./pr-refresh-coordinator') + const visibleOutcome = deferred<{ + kind: 'found' + pr: PRInfo + fetchedAt: number + }>() + getPRForBranchOutcomeMock.mockReturnValueOnce(visibleOutcome.promise).mockResolvedValueOnce({ + kind: 'found', + pr: makePR({ checksStatus: 'success', state: 'merged' }), + fetchedAt: Date.now() + }) + + const candidate = makeCandidate() + reportVisiblePRRefreshCandidates([candidate], 1, 1) + await vi.advanceTimersByTimeAsync(0) + + enqueuePRRefresh({ ...candidate, cachedFetchedAt: Date.now() }, 'active', 80, 1) + visibleOutcome.resolve({ + kind: 'found', + pr: makePR({ checksStatus: 'pending' }), + fetchedAt: Date.now() + }) + await vi.advanceTimersByTimeAsync(0) + + const inFlightEvents = sendMock.mock.calls + .map(([, event]) => event) + .filter((event) => event.status === 'in-flight') + + expect(inFlightEvents.map((event) => event.reason)).toEqual(['visible', 'active']) + expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(2) + }) + + it('cancels queued work when a later enqueue marks the candidate invalid', async () => { + const { enqueuePRRefresh, reportVisiblePRRefreshCandidates } = + await import('./pr-refresh-coordinator') + getPRForBranchOutcomeMock.mockResolvedValueOnce({ + kind: 'found', + pr: makePR({ checksStatus: 'success' }), + fetchedAt: Date.now() + }) + + const candidate = makeCandidate() + reportVisiblePRRefreshCandidates([candidate], 1, 1) + await vi.advanceTimersByTimeAsync(0) + + enqueuePRRefresh( + { ...candidate, isArchived: true, cachedFetchedAt: Date.now() }, + 'active', + 80, + 1 + ) + await vi.advanceTimersByTimeAsync(10 * 60_000) + + const skippedEvents = sendMock.mock.calls + .map(([, event]) => event) + .filter((event) => event.status === 'skipped') + + expect(skippedEvents.at(-1)?.skippedReason).toBe('archived') + expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(1) + }) + + it('does not cancel other aliases when one coalesced PR alias becomes invalid', async () => { + const { enqueuePRRefresh, reportVisiblePRRefreshCandidates } = + await import('./pr-refresh-coordinator') + getPRForBranchOutcomeMock + .mockResolvedValueOnce({ + kind: 'found', + pr: makePR({ checksStatus: 'success' }), + fetchedAt: Date.now() + }) + .mockResolvedValueOnce({ + kind: 'found', + pr: makePR({ checksStatus: 'success', state: 'merged' }), + fetchedAt: Date.now() + }) + + const first = makeCandidate({ + cacheKey: '/repo::feature/a', + branch: 'feature/a', + linkedPRNumber: 12, + worktreeId: 'wt-a' + }) + const second = makeCandidate({ + cacheKey: '/repo::feature/b', + branch: 'feature/b', + linkedPRNumber: 12, + worktreeId: 'wt-b' + }) + reportVisiblePRRefreshCandidates([first, second], 1, 1) + await vi.advanceTimersByTimeAsync(0) + + enqueuePRRefresh({ ...first, isArchived: true, cachedFetchedAt: Date.now() }, 'active', 80, 1) + enqueuePRRefresh({ ...second, cachedFetchedAt: Date.now() }, 'active', 80, 1) + await vi.advanceTimersByTimeAsync(0) + + const outcomeEvents = sendMock.mock.calls + .map(([, event]) => event) + .filter((event) => event.outcome) + + expect(outcomeEvents.at(-1)?.aliases.map((alias) => alias.cacheKey)).toEqual([ + '/repo::feature/b' + ]) + expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(2) + }) + + it('preserves coalesced aliases across visible follow-up refreshes', async () => { + const { reportVisiblePRRefreshCandidates } = await import('./pr-refresh-coordinator') + getPRForBranchOutcomeMock + .mockResolvedValueOnce({ + kind: 'found', + pr: makePR({ checksStatus: 'pending' }), + fetchedAt: Date.now() + }) + .mockResolvedValueOnce({ + kind: 'found', + pr: makePR({ checksStatus: 'success' }), + fetchedAt: Date.now() + }) + + reportVisiblePRRefreshCandidates( + [ + makeCandidate({ + cacheKey: '/repo::feature/a', + branch: 'feature/a', + linkedPRNumber: 12, + worktreeId: 'wt-a' + }), + makeCandidate({ + cacheKey: '/repo::feature/b', + branch: 'feature/b', + linkedPRNumber: 12, + worktreeId: 'wt-b' + }) + ], + 1, + 1 + ) + await vi.runOnlyPendingTimersAsync() + await vi.advanceTimersByTimeAsync(90_000) + + const outcomeEvents = sendMock.mock.calls + .map(([, event]) => event) + .filter((event) => event.outcome) + + expect(outcomeEvents).toHaveLength(2) + expect(outcomeEvents[1].aliases.map((alias) => alias.cacheKey).sort()).toEqual([ + '/repo::feature/a', + '/repo::feature/b' + ]) + expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(2) + }) +}) diff --git a/src/main/github/pr-refresh-coordinator.ts b/src/main/github/pr-refresh-coordinator.ts new file mode 100644 index 000000000..75f883067 --- /dev/null +++ b/src/main/github/pr-refresh-coordinator.ts @@ -0,0 +1,576 @@ +/* eslint-disable max-lines -- Why: the coordinator keeps queueing, pacing, and +renderer broadcast rules together so freshness and rate-limit invariants are +reviewable in one place. */ +import { webContents } from 'electron' +import type { + GitHubPRRefreshAlias, + GitHubPRRefreshCandidate, + GitHubPRRefreshEvent, + GitHubPRRefreshReason, + GitHubPRRefreshSkippedReason, + PRRefreshOutcome +} from '../../shared/types' +import { getPRForBranchOutcome } from './client' +import { getRateLimit, noteRateLimitSpend, rateLimitGuard } from './rate-limit' + +type QueueEntry = { + key: string + candidate: GitHubPRRefreshCandidate + aliases: Map + reason: GitHubPRRefreshReason + priority: number + dueAt: number + windowId?: number +} + +type PRRefreshOutcomeObserver = ( + candidate: GitHubPRRefreshCandidate, + outcome: PRRefreshOutcome +) => void + +const MIN_BACKGROUND_REFRESH_AGE_MS = 60_000 +const BACKGROUND_BUDGET_WINDOW_MS = 5 * 60_000 +const MIN_BACKGROUND_SPACING_MS = 10_000 +const BACKGROUND_BUDGET_MAX = 20 +const POST_PUSH_DELAY_MS = 2_500 +const BACKOFF_BASE_MS = 60_000 +const BACKOFF_MAX_MS = 15 * 60_000 + +let sequence = 0 +let draining = false +let drainTimer: ReturnType | null = null +const queue = new Map() +const backgroundStarts: number[] = [] +const errorBackoff = new Map() +let lastBackgroundStartAt = 0 +const visibleByWindow = new Map }>() +let outcomeObserver: PRRefreshOutcomeObserver | null = null + +export function setPRRefreshOutcomeObserver(observer: PRRefreshOutcomeObserver | null): void { + outcomeObserver = observer +} + +function nextSequence(): number { + sequence += 1 + return sequence +} + +function broadcast(event: Omit, sequenceOverride?: number): void { + const payload = { ...event, sequence: sequenceOverride ?? nextSequence() } as GitHubPRRefreshEvent + for (const wc of webContents.getAllWebContents()) { + if (!wc.isDestroyed()) { + wc.send('gh:prRefreshEvent', payload) + } + } +} + +function refreshKey(candidate: GitHubPRRefreshCandidate): string { + if (typeof candidate.linkedPRNumber === 'number') { + return `${candidate.repoPath}::pr::${candidate.linkedPRNumber}` + } + return `${candidate.repoPath}::branch::${candidate.branch}` +} + +function isVisibleKey(key: string): boolean { + const liveWindowIds = new Set( + webContents + .getAllWebContents() + .filter((wc) => !wc.isDestroyed()) + .map((wc) => wc.id) + ) + for (const windowId of Array.from(visibleByWindow.keys())) { + if (!liveWindowIds.has(windowId)) { + visibleByWindow.delete(windowId) + } + } + for (const visible of visibleByWindow.values()) { + if (visible.keys.has(key)) { + return true + } + } + return false +} + +function isManual(reason: GitHubPRRefreshReason): boolean { + return reason === 'manual' +} + +function bypassesFreshnessDelay(reason: GitHubPRRefreshReason): boolean { + return reason === 'manual' || reason === 'active' || reason === 'post-push' +} + +function isBackground(reason: GitHubPRRefreshReason): boolean { + return reason !== 'manual' +} + +function isBudgetedBackground(reason: GitHubPRRefreshReason): boolean { + return reason === 'visible' || reason === 'swr' +} + +function validateCandidate( + candidate: GitHubPRRefreshCandidate +): GitHubPRRefreshSkippedReason | null { + if (candidate.repoKind !== 'git') { + return 'not-git' + } + if (candidate.isBare) { + return 'bare' + } + if (candidate.isArchived) { + return 'archived' + } + if (candidate.connectionId && candidate.connectionState === 'disconnected') { + return 'disconnected' + } + if (!candidate.branch && typeof candidate.linkedPRNumber !== 'number') { + return 'fresh' + } + return null +} + +function shouldSkipFresh( + candidate: GitHubPRRefreshCandidate, + reason: GitHubPRRefreshReason +): boolean { + if (bypassesFreshnessDelay(reason) || candidate.cachedFetchedAt == null) { + return false + } + return Date.now() - candidate.cachedFetchedAt < refreshIntervalForCandidate(candidate) +} + +function shouldBroadcastQueued(reason: GitHubPRRefreshReason, dueAt: number): boolean { + if (isBudgetedBackground(reason)) { + return false + } + const delay = dueAt - Date.now() + if (delay <= 0) { + return false + } + return delay <= 5_000 +} + +function freshRetryAt(candidate: GitHubPRRefreshCandidate): number | null { + return candidate.cachedFetchedAt == null + ? null + : candidate.cachedFetchedAt + refreshIntervalForCandidate(candidate) +} + +function visibleCandidateAfterOutcome( + candidate: GitHubPRRefreshCandidate, + outcome: PRRefreshOutcome +): GitHubPRRefreshCandidate { + if (outcome.kind === 'upstream-error') { + return candidate + } + return { + ...candidate, + cachedFetchedAt: outcome.fetchedAt, + cachedHasPR: outcome.kind === 'found', + cachedPRState: outcome.kind === 'found' ? outcome.pr.state : null, + cachedChecksStatus: outcome.kind === 'found' ? outcome.pr.checksStatus : null + } +} + +function setVisibleFollowUp(entry: QueueEntry): void { + const existing = queue.get(entry.key) + if (!existing) { + queue.set(entry.key, entry) + return + } + + for (const alias of entry.aliases.values()) { + existing.aliases.set(alias.cacheKey, alias) + } + + // Why: a user activation can arrive while a background refresh is awaiting gh. + // The background follow-up must not overwrite that pending active/manual work. + if ( + bypassesFreshnessDelay(existing.reason) || + existing.priority > entry.priority || + existing.dueAt <= entry.dueAt + ) { + return + } + + queue.set(entry.key, { + ...entry, + aliases: existing.aliases + }) +} + +function removeQueuedAliasForInvalidCandidate(key: string, alias: GitHubPRRefreshAlias): void { + const existing = queue.get(key) + if (!existing) { + return + } + + existing.aliases.delete(alias.cacheKey) + const replacementAlias = existing.aliases.values().next().value + if (!replacementAlias) { + queue.delete(key) + errorBackoff.delete(key) + return + } + + if (existing.candidate.cacheKey === alias.cacheKey) { + existing.candidate = { + ...existing.candidate, + cacheKey: replacementAlias.cacheKey, + branch: replacementAlias.branch, + worktreeId: replacementAlias.worktreeId, + isArchived: false, + isBare: false + } + } +} + +function scheduleVisibleFollowUp( + key: string, + candidate: GitHubPRRefreshCandidate, + outcome: PRRefreshOutcome, + priority: number, + aliases: GitHubPRRefreshAlias[], + windowId?: number +): void { + if (!isVisibleKey(key)) { + return + } + if (outcome.kind === 'upstream-error') { + const failures = (errorBackoff.get(key)?.failures ?? 0) + 1 + const retryAt = + Date.now() + Math.min(BACKOFF_MAX_MS, BACKOFF_BASE_MS * 2 ** Math.min(failures - 1, 4)) + errorBackoff.set(key, { failures, retryAt }) + setVisibleFollowUp({ + key, + candidate, + aliases: new Map(aliases.map((alias) => [alias.cacheKey, alias])), + reason: 'visible', + priority, + dueAt: retryAt, + windowId + }) + // Why: this is a delayed retry, not active work; showing it as a spinner + // makes visible worktrees look stuck until the backoff expires. + scheduleDrain(retryAt - Date.now()) + return + } + errorBackoff.delete(key) + const followUpCandidate = visibleCandidateAfterOutcome(candidate, outcome) + const dueAt = freshRetryAt(followUpCandidate) ?? Date.now() + // Why: coalesced linked-PR refreshes may represent several local branches. + // Preserve every alias for the next visible follow-up so all cache entries + // keep receiving periodic updates. + setVisibleFollowUp({ + key, + candidate: followUpCandidate, + aliases: new Map(aliases.map((alias) => [alias.cacheKey, alias])), + reason: 'visible', + priority, + dueAt, + windowId + }) + scheduleDrain(Math.max(0, dueAt - Date.now())) +} + +function refreshIntervalForCandidate(candidate: GitHubPRRefreshCandidate): number { + if (candidate.cachedPRState === 'closed' || candidate.cachedPRState === 'merged') { + return 30 * 60_000 + } + if (candidate.cachedHasPR === false) { + return 15 * 60_000 + } + if (candidate.cachedChecksStatus === 'success') { + return 10 * 60_000 + } + if (candidate.cachedChecksStatus === 'failure') { + return 3 * 60_000 + } + if (candidate.cachedChecksStatus === 'pending') { + return 90_000 + } + return MIN_BACKGROUND_REFRESH_AGE_MS +} + +function backgroundRefreshBuckets(): ('core' | 'graphql')[] { + // Why: branch refreshes prefer REST but can still fall back to `gh pr list` + // when local head-owner metadata is unavailable. Guard both buckets until the + // client exposes an exact per-lookup cost plan. + return ['core', 'graphql'] +} + +function noteBackgroundStart(): void { + const now = Date.now() + lastBackgroundStartAt = now + backgroundStarts.push(now) + while (backgroundStarts.length > 0 && now - backgroundStarts[0] > BACKGROUND_BUDGET_WINDOW_MS) { + backgroundStarts.shift() + } +} + +function nextBudgetDelay(): number { + const now = Date.now() + while (backgroundStarts.length > 0 && now - backgroundStarts[0] > BACKGROUND_BUDGET_WINDOW_MS) { + backgroundStarts.shift() + } + const spacingDelay = + lastBackgroundStartAt > 0 + ? Math.max(0, MIN_BACKGROUND_SPACING_MS - (now - lastBackgroundStartAt)) + : 0 + const windowDelay = + backgroundStarts.length < BACKGROUND_BUDGET_MAX + ? 0 + : Math.max(1_000, BACKGROUND_BUDGET_WINDOW_MS - (now - backgroundStarts[0])) + return Math.max(spacingDelay, windowDelay) +} + +function scheduleDrain(delay = 0): void { + if (drainTimer) { + clearTimeout(drainTimer) + } + drainTimer = setTimeout(() => { + drainTimer = null + void drainQueue() + }, delay) +} + +function queuedEntriesByPriority(): QueueEntry[] { + const now = Date.now() + return Array.from(queue.values()).sort((a, b) => { + const aReady = a.dueAt <= now + const bReady = b.dueAt <= now + if (aReady && bReady) { + return b.priority - a.priority || a.dueAt - b.dueAt + } + if (aReady !== bReady) { + return aReady ? -1 : 1 + } + return a.dueAt - b.dueAt || b.priority - a.priority + }) +} + +async function drainQueue(): Promise { + if (draining) { + return + } + draining = true + try { + while (queue.size > 0) { + const next = queuedEntriesByPriority()[0] + const waitMs = next.dueAt - Date.now() + if (waitMs > 0) { + scheduleDrain(waitMs) + return + } + + const budgetDelay = isBudgetedBackground(next.reason) ? nextBudgetDelay() : 0 + if (budgetDelay > 0) { + scheduleDrain(budgetDelay) + return + } + + queue.delete(next.key) + const aliases = Array.from(next.aliases.values()) + const skippedReason = validateCandidate(next.candidate) + if (skippedReason) { + broadcast({ aliases, reason: next.reason, status: 'skipped', skippedReason }) + continue + } + if (next.reason === 'visible' && !isVisibleKey(next.key)) { + errorBackoff.delete(next.key) + broadcast({ aliases, reason: next.reason, status: 'skipped', skippedReason: 'fresh' }) + continue + } + const requestSequence = nextSequence() + broadcast({ aliases, reason: next.reason, status: 'in-flight' }, requestSequence) + + if (isBackground(next.reason)) { + const rateLimit = await getRateLimit() + if (!rateLimit.ok) { + const retryAt = Date.now() + 30_000 + queue.set(next.key, { ...next, dueAt: retryAt }) + broadcast({ + aliases, + reason: next.reason, + status: 'paused', + pausedUntil: retryAt, + skippedReason: 'rate-limit' + }) + scheduleDrain(30_000) + continue + } + const buckets = backgroundRefreshBuckets() + const blockedGuard = buckets + .map((bucket) => rateLimitGuard(bucket)) + .find((guard) => guard.blocked) + if (blockedGuard?.blocked) { + const retryAt = blockedGuard.resetAt * 1000 + queue.set(next.key, { ...next, dueAt: retryAt }) + broadcast({ + aliases, + reason: next.reason, + status: 'paused', + pausedUntil: retryAt, + skippedReason: 'rate-limit' + }) + scheduleDrain(Math.max(1_000, retryAt - Date.now())) + continue + } + if (isBudgetedBackground(next.reason)) { + noteBackgroundStart() + } + for (const bucket of buckets) { + noteRateLimitSpend(bucket) + } + } + + const outcome = await getPRForBranchOutcome( + next.candidate.repoPath, + next.candidate.branch, + next.candidate.linkedPRNumber ?? null, + next.candidate.connectionId ?? null + ) + outcomeObserver?.(next.candidate, outcome) + broadcast({ aliases, reason: next.reason, outcome }, requestSequence) + scheduleVisibleFollowUp( + next.key, + next.candidate, + outcome, + next.priority, + aliases, + next.windowId + ) + } + } finally { + draining = false + } +} + +export function enqueuePRRefresh( + candidate: GitHubPRRefreshCandidate, + reason: GitHubPRRefreshReason, + priority = 0, + windowId?: number +): void { + const alias: GitHubPRRefreshAlias = { + cacheKey: candidate.cacheKey, + repoId: candidate.repoId, + repoPath: candidate.repoPath, + branch: candidate.branch, + worktreeId: candidate.worktreeId + } + const key = refreshKey(candidate) + const skippedReason = validateCandidate(candidate) + if (skippedReason) { + removeQueuedAliasForInvalidCandidate(key, alias) + broadcast({ + aliases: [alias], + reason, + status: 'skipped', + skippedReason + }) + return + } + + const existing = queue.get(key) + const freshDueAt = shouldSkipFresh(candidate, reason) ? freshRetryAt(candidate) : null + const dueAt = freshDueAt ?? Date.now() + (reason === 'post-push' ? POST_PUSH_DELAY_MS : 0) + if (existing) { + existing.aliases.set(alias.cacheKey, alias) + const shouldPromoteExisting = + priority > existing.priority || + isManual(reason) || + (priority >= existing.priority && dueAt < existing.dueAt && bypassesFreshnessDelay(reason)) + if (shouldPromoteExisting) { + existing.priority = priority + existing.reason = reason + existing.dueAt = Math.min(existing.dueAt, dueAt) + existing.candidate = candidate + existing.windowId = windowId ?? existing.windowId + } + } else { + queue.set(key, { + key, + candidate, + aliases: new Map([[alias.cacheKey, alias]]), + reason, + priority, + dueAt, + windowId + }) + } + // Why: visible/SWR refreshes are background maintenance and may sit behind + // the budget queue. Only user/action-driven queueing should surface in UI. + if (shouldBroadcastQueued(reason, dueAt)) { + broadcast({ aliases: [alias], reason, status: 'queued' }) + } + scheduleDrain() +} + +export function reportVisiblePRRefreshCandidates( + candidates: GitHubPRRefreshCandidate[], + generation: number, + windowId: number +): void { + const existingVisible = visibleByWindow.get(windowId) + if (existingVisible && generation < existingVisible.generation) { + return + } + visibleByWindow.set(windowId, { generation, keys: new Set(candidates.map(refreshKey)) }) + for (const [key, entry] of queue) { + if (entry.reason === 'visible' && !isVisibleKey(key)) { + queue.delete(key) + errorBackoff.delete(key) + broadcast({ + aliases: Array.from(entry.aliases.values()), + reason: 'visible', + status: 'skipped', + skippedReason: 'fresh' + }) + } + } + for (const candidate of candidates) { + enqueuePRRefresh(candidate, 'visible', 40, windowId) + } +} + +export async function refreshPRNow(candidate: GitHubPRRefreshCandidate): Promise { + const alias: GitHubPRRefreshAlias = { + cacheKey: candidate.cacheKey, + repoId: candidate.repoId, + repoPath: candidate.repoPath, + branch: candidate.branch, + worktreeId: candidate.worktreeId + } + const key = refreshKey(candidate) + const existing = queue.get(key) + const aliases = existing ? Array.from(existing.aliases.values()) : [alias] + if (!aliases.some((entry) => entry.cacheKey === alias.cacheKey)) { + aliases.push(alias) + } + const skippedReason = validateCandidate(candidate) + if (skippedReason) { + removeQueuedAliasForInvalidCandidate(key, alias) + const outcome: PRRefreshOutcome = { + kind: 'upstream-error', + errorType: 'unknown', + message: `Cannot refresh PR for this worktree: ${skippedReason}`, + fetchedAt: Date.now() + } + broadcast({ aliases: [alias], reason: 'manual', status: 'skipped', skippedReason }) + return outcome + } + + queue.delete(key) + const requestSequence = nextSequence() + broadcast({ aliases, reason: 'manual', status: 'in-flight' }, requestSequence) + const outcome = await getPRForBranchOutcome( + candidate.repoPath, + candidate.branch, + candidate.linkedPRNumber ?? null, + candidate.connectionId ?? null + ) + outcomeObserver?.(candidate, outcome) + broadcast({ aliases, reason: 'manual', outcome }, requestSequence) + scheduleVisibleFollowUp(key, candidate, outcome, 40, aliases) + return outcome +} diff --git a/src/main/ipc/github.ts b/src/main/ipc/github.ts index d2e527301..a2397255d 100644 --- a/src/main/ipc/github.ts +++ b/src/main/ipc/github.ts @@ -8,7 +8,10 @@ import type { Repo, GitHubIssueUpdate, GitHubOwnerRepo, - GitHubPullRequestStateUpdate + GitHubPullRequestStateUpdate, + GitHubPRRefreshCandidate, + GitHubPRRefreshReason, + PRRefreshOutcome } from '../../shared/types' import type { Store } from '../persistence' import type { StatsCollector } from '../stats/collector' @@ -43,6 +46,12 @@ import { checkOrcaStarred, starOrca } from '../github/client' +import { + enqueuePRRefresh, + refreshPRNow, + reportVisiblePRRefreshCandidates, + setPRRefreshOutcomeObserver +} from '../github/pr-refresh-coordinator' import { getWorkItemDetails, getPRFileContents } from '../github/work-item-details' import { getRateLimit } from '../github/rate-limit' import { diagnoseGhAuth } from '../github/auth-diagnose' @@ -135,6 +144,26 @@ function repoConnectionId(repo: Repo): string | null { } export function registerGitHubHandlers(store: Store, stats: StatsCollector): void { + function recordPRIfNeeded(repo: Repo, outcome: PRRefreshOutcome): void { + if (outcome.kind === 'found' && !stats.hasCountedPR(outcome.pr.url)) { + stats.record({ + type: 'pr_created', + at: Date.now(), + repoId: repo.id, + meta: { prNumber: outcome.pr.number, prUrl: outcome.pr.url } + }) + } + } + + setPRRefreshOutcomeObserver((candidate, outcome) => { + const repo = + store.getRepos().find((r) => r.id === candidate.repoId) ?? + store.getRepos().find((r) => resolve(r.path) === resolve(candidate.repoPath)) + if (repo) { + recordPRIfNeeded(repo, outcome) + } + }) + ipcMain.handle( 'gh:prForBranch', async (_event, args: { repoPath: string; branch: string; linkedPRNumber?: number | null }) => { @@ -160,6 +189,66 @@ export function registerGitHubHandlers(store: Store, stats: StatsCollector): voi } ) + ipcMain.handle( + 'gh:refreshPRNow', + async (_event, args: { candidate: GitHubPRRefreshCandidate }) => { + const repo = assertRegisteredRepo(args.candidate.repoPath, store) + const outcome = await refreshPRNow({ + ...args.candidate, + repoPath: repo.path, + repoId: repo.id, + connectionId: repo.connectionId ?? args.candidate.connectionId, + connectionState: repo.connectionId ? 'connected' : args.candidate.connectionState + }) + recordPRIfNeeded(repo, outcome) + return outcome + } + ) + + ipcMain.handle( + 'gh:enqueuePRRefresh', + ( + _event, + args: { + candidate: GitHubPRRefreshCandidate + reason: GitHubPRRefreshReason + priority?: number + } + ) => { + const repo = assertRegisteredRepo(args.candidate.repoPath, store) + enqueuePRRefresh( + { + ...args.candidate, + repoPath: repo.path, + repoId: repo.id, + connectionId: repo.connectionId ?? args.candidate.connectionId, + connectionState: repo.connectionId ? 'connected' : args.candidate.connectionState + }, + args.reason, + args.priority ?? 0 + ) + return true + } + ) + + ipcMain.handle( + 'gh:reportVisiblePRRefreshCandidates', + (event, args: { candidates: GitHubPRRefreshCandidate[]; generation: number }) => { + const candidates = args.candidates.map((candidate) => { + const repo = assertRegisteredRepo(candidate.repoPath, store) + return { + ...candidate, + repoPath: repo.path, + repoId: repo.id, + connectionId: repo.connectionId ?? candidate.connectionId, + connectionState: repo.connectionId ? 'connected' : candidate.connectionState + } + }) + reportVisiblePRRefreshCandidates(candidates, args.generation, event.sender.id) + return true + } + ) + ipcMain.handle('gh:issue', (_event, args: { repoPath: string; number: number }) => { const repo = assertRegisteredRepo(args, store) return getIssue(repo.path, args.number, repoConnectionId(repo)) diff --git a/src/preload/api-types.ts b/src/preload/api-types.ts index c6cbe3632..9ae47c883 100644 --- a/src/preload/api-types.ts +++ b/src/preload/api-types.ts @@ -70,6 +70,9 @@ import type { MarkdownDocument, FloatingTerminalCwdRequest, GitHubIssueUpdate, + GitHubPRRefreshCandidate, + GitHubPRRefreshEvent, + GitHubPRRefreshReason, GetRateLimitResult, NotificationDispatchRequest, NotificationDispatchResult, @@ -83,6 +86,7 @@ import type { PRCheckRunDetails, PRComment, PRInfo, + PRRefreshOutcome, Repo, ShellHydrationFailureReason, SparsePreset, @@ -769,6 +773,17 @@ export type PreloadApi = { branch: string linkedPRNumber?: number | null }) => Promise + refreshPRNow: (args: { candidate: GitHubPRRefreshCandidate }) => Promise + enqueuePRRefresh: (args: { + candidate: GitHubPRRefreshCandidate + reason: GitHubPRRefreshReason + priority?: number + }) => Promise + reportVisiblePRRefreshCandidates: (args: { + candidates: GitHubPRRefreshCandidate[] + generation: number + }) => Promise + onPRRefreshEvent: (callback: (event: GitHubPRRefreshEvent) => void) => () => void issue: (args: { repoPath: string repoId?: string diff --git a/src/preload/index.ts b/src/preload/index.ts index 97ce2406f..6415ee39d 100644 --- a/src/preload/index.ts +++ b/src/preload/index.ts @@ -16,6 +16,9 @@ import type { CustomPet, FsChangedPayload, GetRateLimitResult, + GitHubPRRefreshCandidate, + GitHubPRRefreshEvent, + GitHubPRRefreshReason, GitHubAssignableUser, GitHubCommentResult, GitHubWorkItem, @@ -771,6 +774,27 @@ const api = { linkedPRNumber?: number | null }): Promise => ipcRenderer.invoke('gh:prForBranch', args), + refreshPRNow: (args: { candidate: GitHubPRRefreshCandidate }): Promise => + ipcRenderer.invoke('gh:refreshPRNow', args), + + enqueuePRRefresh: (args: { + candidate: GitHubPRRefreshCandidate + reason: GitHubPRRefreshReason + priority?: number + }): Promise => ipcRenderer.invoke('gh:enqueuePRRefresh', args), + + reportVisiblePRRefreshCandidates: (args: { + candidates: GitHubPRRefreshCandidate[] + generation: number + }): Promise => ipcRenderer.invoke('gh:reportVisiblePRRefreshCandidates', args), + + onPRRefreshEvent: (callback: (event: GitHubPRRefreshEvent) => void): (() => void) => { + const listener = (_event: Electron.IpcRendererEvent, event: GitHubPRRefreshEvent): void => + callback(event) + ipcRenderer.on('gh:prRefreshEvent', listener) + return () => ipcRenderer.removeListener('gh:prRefreshEvent', listener) + }, + issue: (args: { repoPath: string; repoId?: string; number: number }): Promise => ipcRenderer.invoke('gh:issue', args), diff --git a/src/renderer/src/App.tsx b/src/renderer/src/App.tsx index d6a2698a3..ecb337163 100644 --- a/src/renderer/src/App.tsx +++ b/src/renderer/src/App.tsx @@ -241,6 +241,8 @@ function App(): React.JSX.Element { fetchSettings: s.fetchSettings, initGitHubCache: s.initGitHubCache, refreshAllGitHub: s.refreshAllGitHub, + reportVisibleGitHubPRRefreshCandidates: s.reportVisibleGitHubPRRefreshCandidates, + bumpGitHubPRVisibleRefreshGeneration: s.bumpGitHubPRVisibleRefreshGeneration, hydrateWorkspaceSession: s.hydrateWorkspaceSession, hydrateTabsSession: s.hydrateTabsSession, hydrateEditorSession: s.hydrateEditorSession, @@ -927,6 +929,9 @@ function App(): React.JSX.Element { const handler = (): void => { if (document.visibilityState === 'visible') { actions.refreshAllGitHub() + actions.bumpGitHubPRVisibleRefreshGeneration() + } else { + actions.reportVisibleGitHubPRRefreshCandidates([], Date.now()) } } document.addEventListener('visibilitychange', handler) diff --git a/src/renderer/src/components/right-sidebar/ChecksPanel.tsx b/src/renderer/src/components/right-sidebar/ChecksPanel.tsx index e9d672744..ac9a27828 100644 --- a/src/renderer/src/components/right-sidebar/ChecksPanel.tsx +++ b/src/renderer/src/components/right-sidebar/ChecksPanel.tsx @@ -44,6 +44,7 @@ export default function ChecksPanel(): React.JSX.Element { const getHostedReviewCreationEligibility = useAppStore( (s) => s.getHostedReviewCreationEligibility ) + const enqueueGitHubPRRefresh = useAppStore((s) => s.enqueueGitHubPRRefresh) const gitConflictOperationByWorktree = useAppStore((s) => s.gitConflictOperationByWorktree) const gitStatusByWorktree = useAppStore((s) => s.gitStatusByWorktree) const remoteStatusesByWorktree = useAppStore((s) => s.remoteStatusesByWorktree) @@ -83,6 +84,8 @@ export default function ChecksPanel(): React.JSX.Element { const prevChecksRef = useRef('') const conflictSummaryRefreshKeyRef = useRef(null) const asyncResultKeyRef = useRef('') + const refreshRequestKeyRef = useRef(null) + const refreshContextKeyRef = useRef(null) // Why: the sidebar no longer uses key={activeWorktreeId} to force a full // remount on worktree switch (that caused an IPC storm on Windows). @@ -103,13 +106,22 @@ export default function ChecksPanel(): React.JSX.Element { setCreatePrDialogOpen(false) setCreatePrPushFirst(false) conflictSummaryRefreshKeyRef.current = null + refreshRequestKeyRef.current = null } // Find active worktree and repo const branch = activeWorktree ? activeWorktree.branch.replace(/^refs\/heads\//, '') : '' const isFolder = repo ? isFolderRepo(repo) : false const prCacheKey = repo && branch ? `${repo.id}::${branch}` : '' + const refreshContextKey = `${activeWorktreeId ?? ''}::${repo?.id ?? ''}::${branch}` + if (refreshContextKey !== refreshContextKeyRef.current) { + refreshContextKeyRef.current = refreshContextKey + refreshRequestKeyRef.current = null + } const pr: PRInfo | null = prCacheKey ? (prCache[prCacheKey]?.data ?? null) : null + const prRefreshState = useAppStore((s) => + prCacheKey ? s.prRefreshStates[prCacheKey] : undefined + ) const prNumber = pr?.number ?? null const remoteStatus = activeWorktreeId ? remoteStatusesByWorktree[activeWorktreeId] : undefined const hasUncommittedChanges = activeWorktreeId @@ -152,10 +164,12 @@ export default function ChecksPanel(): React.JSX.Element { [] ) useEffect(() => { - if (repo && !isFolder && branch) { - void fetchPRForBranch(repo.path, branch, { repoId: repo.id, linkedPRNumber: linkedPR }) + if (isPanelVisible && repo && !isFolder && branch) { + if (activeWorktreeId) { + enqueueGitHubPRRefresh(activeWorktreeId, 'swr', 30) + } } - }, [repo, isFolder, branch, linkedPR, fetchPRForBranch]) + }, [repo, isFolder, branch, activeWorktreeId, enqueueGitHubPRRefresh, isPanelVisible]) useEffect(() => { if (!repo || isFolder || !branch || !isPanelVisible) { @@ -202,7 +216,14 @@ export default function ChecksPanel(): React.JSX.Element { ]) useEffect(() => { - if (!repo || isFolder || !branch || !pr || pr.mergeable !== 'CONFLICTING') { + if ( + !repo || + isFolder || + !branch || + !pr || + pr.mergeable !== 'CONFLICTING' || + !activeWorktreeId + ) { conflictSummaryRefreshKeyRef.current = null setConflictDetailsRefreshing(false) return @@ -231,7 +252,7 @@ export default function ChecksPanel(): React.JSX.Element { setConflictDetailsRefreshing(false) } }) - }, [repo, isFolder, branch, pr, linkedPR, fetchPRForBranch]) + }, [repo, isFolder, branch, pr, activeWorktreeId, linkedPR, fetchPRForBranch]) // Fetch checks via cached store method const fetchChecks = useCallback( @@ -411,7 +432,9 @@ export default function ChecksPanel(): React.JSX.Element { return } const initialRequestKey = checksPanelAsyncResultKey(repo.id, branch, prNumber, pr?.prRepo) - let activeRefreshKey = initialRequestKey + const refreshRequestKey = `${activeWorktreeId ?? ''}::${repo.id}::${branch}::${Date.now()}::${Math.random()}` + refreshRequestKeyRef.current = refreshRequestKey + const isCurrentRequest = (): boolean => refreshRequestKeyRef.current === refreshRequestKey setIsRefreshing(true) try { const refreshedPR = await fetchPRForBranch(repo.path, branch, { @@ -419,6 +442,9 @@ export default function ChecksPanel(): React.JSX.Element { repoId: repo.id, linkedPRNumber: linkedPR }) + if (!isCurrentRequest()) { + return + } await refreshHostedReviewCard(fetchHostedReviewForBranch, { repoPath: repo.path, repoId: repo.id, @@ -426,20 +452,22 @@ export default function ChecksPanel(): React.JSX.Element { linkedGitHubPR: refreshedPR?.number ?? linkedPR, linkedGitLabMR }) + if (!isCurrentRequest()) { + return + } if (refreshedPR) { - const requestKey = checksPanelAsyncResultKey( + const prRequestKey = checksPanelAsyncResultKey( repo.id, branch, refreshedPR.number, refreshedPR.prRepo ) - if (!isCurrentAsyncResult(initialRequestKey) && !isCurrentAsyncResult(requestKey)) { + if (!isCurrentAsyncResult(initialRequestKey) && !isCurrentRequest()) { return } // Why: a forced PR refresh can discover the PR number before React has // repainted from prCache; make this refresh's follow-up checks current. - asyncResultKeyRef.current = requestKey - activeRefreshKey = requestKey + asyncResultKeyRef.current = prRequestKey // Why: call fetchPRChecks directly with the refreshed PR's headSha so // we don't pass the stale headSha captured by `fetchChecks`'s closure // before the PR refresh completed (covers external force-pushes and @@ -453,7 +481,7 @@ export default function ChecksPanel(): React.JSX.Element { { force: true, repoId: repo.id } ).then( (result) => { - if (!isCurrentAsyncResult(requestKey)) { + if (!isCurrentRequest() || !isCurrentAsyncResult(prRequestKey)) { return } setChecks(result) @@ -467,7 +495,7 @@ export default function ChecksPanel(): React.JSX.Element { prevChecksRef.current = signature }, (err) => { - if (!isCurrentAsyncResult(requestKey)) { + if (!isCurrentRequest() || !isCurrentAsyncResult(prRequestKey)) { return } console.warn('Failed to fetch PR checks:', err) @@ -475,42 +503,81 @@ export default function ChecksPanel(): React.JSX.Element { } ) setChecksLoading(true) - const refreshedComments = fetchComments({ + setCommentsLoading(true) + const refreshedComments = fetchPRComments(repo.path, refreshedPR.number, { force: true, - prNumberOverride: refreshedPR.number, - prRepoOverride: refreshedPR.prRepo - }) + repoId: repo.id, + prRepo: refreshedPR.prRepo + }).then( + (result) => { + if (isCurrentRequest() && isCurrentAsyncResult(prRequestKey)) { + setComments(result) + } + }, + (err) => { + if (!isCurrentRequest() || !isCurrentAsyncResult(prRequestKey)) { + return + } + console.warn('Failed to fetch PR comments:', err) + setComments([]) + } + ) await Promise.all([ refreshedChecks.finally(() => { - if (isCurrentAsyncResult(requestKey)) { + if (isCurrentRequest() && isCurrentAsyncResult(prRequestKey)) { setChecksLoading(false) } }), - refreshedComments + refreshedComments.finally(() => { + if (isCurrentRequest() && isCurrentAsyncResult(prRequestKey)) { + setCommentsLoading(false) + } + }) ]) - } else if (isCurrentAsyncResult(initialRequestKey)) { + } else if (isCurrentRequest()) { setChecks([]) setComments([]) } } finally { - if (isCurrentAsyncResult(activeRefreshKey)) { + if (isCurrentRequest()) { setIsRefreshing(false) } } }, [ repo, branch, + activeWorktreeId, prNumber, pr?.prRepo, linkedPR, linkedGitLabMR, fetchPRForBranch, fetchPRChecks, - fetchComments, + fetchPRComments, fetchHostedReviewForBranch, isCurrentAsyncResult ]) + const handleEntryRefresh = useCallback( + (options: { refreshChecks: boolean; refreshComments: boolean }) => { + if (!repo || !branch || !activeWorktreeId) { + return + } + // Why: entering the Checks tab is automatic UI behavior, not an explicit + // user refresh. Route PR refresh through the coordinator so rate-limit + // guards still apply; only force detail panes that the entry freshness rule + // already proved stale, so tab entry stays fresh without broad fan-out. + enqueueGitHubPRRefresh(activeWorktreeId, 'active', 80) + if (options.refreshChecks) { + void fetchChecks({ force: true }) + } + if (options.refreshComments) { + void fetchComments({ force: true }) + } + }, + [repo, branch, activeWorktreeId, enqueueGitHubPRRefresh, fetchChecks, fetchComments] + ) + // Why: force a freshness check on each "entry" into the Checks tab so PRs // opened outside Orca, externally force-pushed heads, and stale checks/comments // appear without waiting for the cache TTL. The grace window suppresses @@ -518,7 +585,7 @@ export default function ChecksPanel(): React.JSX.Element { // docs/refresh-on-checks-tab.md. const entryKey = isPanelVisible && repo && !isFolder && branch - ? `${activeWorktreeId ?? ''}::${repo.path}::${branch}` + ? `${activeWorktreeId ?? ''}::${repo.id}::${branch}` : '' const lastEntryKeyRef = useRef('') useEffect(() => { @@ -534,24 +601,30 @@ export default function ChecksPanel(): React.JSX.Element { } lastEntryKeyRef.current = entryKey + const now = Date.now() const stale = shouldEntryRefresh({ prFetchedAt, checksFetchedAt, commentsFetchedAt, prNumber, - now: Date.now(), + now, graceMs: ENTRY_REFRESH_GRACE_MS }) if (!stale) { return } + const cutoff = now - ENTRY_REFRESH_GRACE_MS + const refreshChecks = + prNumber !== null && (checksFetchedAt === undefined || checksFetchedAt < cutoff) + const refreshComments = + prNumber !== null && (commentsFetchedAt === undefined || commentsFetchedAt < cutoff) // Reset polling attention state so the forced fetch's signature establishes // a fresh baseline rather than colliding with the previous PR's backoff. pollIntervalRef.current = 30_000 prevChecksRef.current = '' - void handleRefresh() - }, [entryKey, prFetchedAt, checksFetchedAt, commentsFetchedAt, prNumber, handleRefresh]) + handleEntryRefresh({ refreshChecks, refreshComments }) + }, [entryKey, prFetchedAt, checksFetchedAt, commentsFetchedAt, prNumber, handleEntryRefresh]) const handleStartEdit = useCallback(() => { if (!pr) { @@ -846,6 +919,10 @@ export default function ChecksPanel(): React.JSX.Element { : conflictOperation === 'cherry-pick' ? 'Cherry-pick' : null + const isQueuedPRRefresh = prRefreshState?.status === 'queued' + const isInFlightPRRefresh = prRefreshState?.status === 'in-flight' + const isPausedPRRefresh = prRefreshState?.status === 'paused' + const isErroredPRRefresh = prRefreshState?.status === 'error' const canCreate = hostedReviewCreation?.canCreate const canPushCreate = hostedReviewCreation?.blockedReason === 'needs_push' @@ -868,14 +945,28 @@ export default function ChecksPanel(): React.JSX.Element { )}
- {operationInProgress ? `${operationLabel} in progress` : 'No pull request found'} + {operationInProgress + ? `${operationLabel} in progress` + : isErroredPRRefresh + ? 'Could not refresh pull request' + : isQueuedPRRefresh || isInFlightPRRefresh + ? 'Checking for pull request' + : 'No pull request found'}
{operationInProgress ? 'PR checks will be available after the operation completes' - : canPushCreate - ? 'Push your branch before creating a pull request.' - : 'Create a pull request to start checks and review.'} + : isErroredPRRefresh + ? 'GitHub status could not be refreshed. Existing cached data was preserved.' + : isQueuedPRRefresh + ? 'Waiting to refresh GitHub status for this branch' + : isInFlightPRRefresh + ? 'Refreshing GitHub status for this branch' + : isPausedPRRefresh + ? 'GitHub refresh is paused by the current rate-limit budget' + : canPushCreate + ? 'Push your branch before creating a pull request.' + : 'Create a pull request to start checks and review.'}
{!operationInProgress && (
@@ -992,7 +1083,7 @@ export default function ChecksPanel(): React.JSX.Element { {/* Updated at */} {pr.updatedAt && (
- Updated {new Date(pr.updatedAt).toLocaleString()} + PR updated {new Date(pr.updatedAt).toLocaleString()}
)} diff --git a/src/renderer/src/components/right-sidebar/SourceControl.tsx b/src/renderer/src/components/right-sidebar/SourceControl.tsx index 732ad57bb..6e8f29433 100644 --- a/src/renderer/src/components/right-sidebar/SourceControl.tsx +++ b/src/renderer/src/components/right-sidebar/SourceControl.tsx @@ -436,6 +436,8 @@ function SourceControlInner(): React.JSX.Element { (s) => s.getHostedReviewCreationEligibility ) const fetchPRForBranch = useAppStore((s) => s.fetchPRForBranch) + const prCache = useAppStore((s) => s.prCache) + const enqueueGitHubPRRefresh = useAppStore((s) => s.enqueueGitHubPRRefresh) const updateRepo = useAppStore((s) => s.updateRepo) const setGitStatus = useAppStore((s) => s.setGitStatus) const updateWorktreeGitIdentity = useAppStore((s) => s.updateWorktreeGitIdentity) @@ -773,8 +775,12 @@ function SourceControlInner(): React.JSX.Element { const hostedReviewEntry = hostedReviewCacheKey ? hostedReviewCache[hostedReviewCacheKey] : undefined + const activePrCacheKey = activeRepo && branchName ? `${activeRepo.id}::${branchName}` : null + const activePrFromQueue = activePrCacheKey ? (prCache[activePrCacheKey]?.data ?? null) : null const hostedReview: HostedReviewInfo | null = hostedReviewCacheKey - ? (hostedReviewEntry?.data ?? null) + ? activePrFromQueue + ? { provider: 'github', ...activePrFromQueue, status: activePrFromQueue.checksStatus } + : (hostedReviewEntry?.data ?? null) : null const linkedGitHubPR = activeWorktree?.linkedPR ?? null @@ -790,7 +796,14 @@ function SourceControlInner(): React.JSX.Element { (linkedGitHubPR !== null || linkedGitLabMR !== null) && hostedReviewEntry === undefined useEffect(() => { - if (!isBranchVisible || !activeRepo || isFolder || !branchName || branchName === 'HEAD') { + if ( + !isBranchVisible || + !activeRepo || + isFolder || + !branchName || + branchName === 'HEAD' || + !activeWorktreeId + ) { return } // Why: the Source Control panel renders branch review status directly. @@ -804,9 +817,14 @@ function SourceControlInner(): React.JSX.Element { linkedGitLabMR, staleWhileRevalidate: true }) + // Why: the GitHub-specific cache powers grouping/check panels; keep that + // refresh behind the coordinator so Source Control does not bypass pacing. + enqueueGitHubPRRefresh(activeWorktreeId, 'swr', 30) }, [ activeRepo, + activeWorktreeId, branchName, + enqueueGitHubPRRefresh, fetchHostedReviewForBranch, isBranchVisible, isFolder, diff --git a/src/renderer/src/components/sidebar/WorktreeList.tsx b/src/renderer/src/components/sidebar/WorktreeList.tsx index 024449153..84b74a098 100644 --- a/src/renderer/src/components/sidebar/WorktreeList.tsx +++ b/src/renderer/src/components/sidebar/WorktreeList.tsx @@ -473,6 +473,13 @@ const VirtualizedWorktreeViewport = React.memo(function VirtualizedWorktreeViewp const [pinDragOver, setPinDragOver] = useState(false) const [lineageReconnectWorktreeId, setLineageReconnectWorktreeId] = useState(null) const canReorderRepoHeaders = groupBy === 'repo' && repoGroupOrdering === 'manual' + const lastVisibleRefreshKeyRef = useRef('') + const reportVisibleGitHubPRRefreshCandidates = useAppStore( + (s) => s.reportVisibleGitHubPRRefreshCandidates + ) + const cardProps = useAppStore((s) => s.worktreeCardProperties) + const sshConnectedGeneration = useAppStore((s) => s.sshConnectedGeneration) + const prVisibleRefreshGeneration = useAppStore((s) => s.prVisibleRefreshGeneration) // Drag is only meaningful when repo headers are using manual order. The // controller is still constructed for hook order stability when inert. @@ -1034,6 +1041,49 @@ const VirtualizedWorktreeViewport = React.memo(function VirtualizedWorktreeViewp requestScrollViewportUpdate() }, [markScrollMovement, requestScrollViewportUpdate]) + useEffect(() => { + if (document.visibilityState !== 'visible') { + lastVisibleRefreshKeyRef.current = '__document_hidden__' + return + } + if (groupBy !== 'pr-status' && !cardProps.includes('pr') && !cardProps.includes('ci')) { + if (lastVisibleRefreshKeyRef.current !== '__hidden__') { + lastVisibleRefreshKeyRef.current = '__hidden__' + reportVisibleGitHubPRRefreshCandidates([], Date.now()) + } + return + } + const scrollEl = scrollRef.current + if (!scrollEl) { + return + } + const viewportTop = scrollEl.scrollTop + const viewportBottom = viewportTop + scrollEl.clientHeight + const visibleRows = virtualItems + .filter((item) => item.start < viewportBottom && item.end > viewportTop) + .map((item) => renderRows[item.index]) + .filter((row): row is Extract => row?.type === 'item') + .filter((row) => row.repo?.kind === 'git' && !row.worktree.isBare && row.worktree.branch) + const visibleWorktreeIds = visibleRows.map((row) => row.worktree.id) + const visibleIdentity = visibleRows + .map((row) => `${row.worktree.id}:${row.worktree.branch}:${row.worktree.linkedPR ?? ''}`) + .join('|') + const key = `${visibleIdentity}:${sshConnectedGeneration}:${prVisibleRefreshGeneration}:${cardProps.join(',')}` + if (!key || key === lastVisibleRefreshKeyRef.current) { + return + } + lastVisibleRefreshKeyRef.current = key + reportVisibleGitHubPRRefreshCandidates(visibleWorktreeIds, Date.now()) + }, [ + cardProps, + groupBy, + renderRows, + reportVisibleGitHubPRRefreshCandidates, + prVisibleRefreshGeneration, + sshConnectedGeneration, + virtualItems + ]) + const activeDescendantId = activeWorktreeId != null && activeWorktreeRowIndex !== -1 && diff --git a/src/renderer/src/hooks/useIpcEvents.ts b/src/renderer/src/hooks/useIpcEvents.ts index 5487a4d4e..a580de095 100644 --- a/src/renderer/src/hooks/useIpcEvents.ts +++ b/src/renderer/src/hooks/useIpcEvents.ts @@ -556,6 +556,14 @@ export function useIpcEvents(): void { }) ) + if (window.api.gh?.onPRRefreshEvent) { + unsubs.push( + window.api.gh.onPRRefreshEvent((event) => { + useAppStore.getState().applyGitHubPRRefreshEvent(event) + }) + ) + } + unsubs.push( window.api.ui.onOpenSettings(() => { useAppStore.getState().openSettingsPage() @@ -1655,11 +1663,6 @@ export function useIpcEvents(): void { } if (state.status === 'connected') { - // Why: the file explorer may have tried (and failed) to load the tree - // before the SSH connection was established. Bumping the generation - // lets it detect that providers are now available and retry. - store.bumpSshConnectedGeneration() - void Promise.all(remoteRepos.map((r) => store.fetchWorktrees(r.id))).then(async () => { await useAppStore.getState().fetchWorktreeLineage() // Why: terminal panes that failed to spawn (no PTY provider on cold diff --git a/src/renderer/src/store/slices/editor.ts b/src/renderer/src/store/slices/editor.ts index b93272e41..283760d5a 100644 --- a/src/renderer/src/store/slices/editor.ts +++ b/src/renderer/src/store/slices/editor.ts @@ -2388,6 +2388,10 @@ export const createEditorSlice: StateCreator = (s get().endRemoteOperation() } void get().fetchUpstreamStatus(worktreeId, worktreePath, connectionId) + const refreshGitHubForWorktree = get().refreshGitHubForWorktree + if (typeof refreshGitHubForWorktree === 'function') { + refreshGitHubForWorktree(worktreeId) + } }, pullBranch: async (worktreeId, worktreePath, connectionId) => { get().beginRemoteOperation('pull') @@ -2400,6 +2404,10 @@ export const createEditorSlice: StateCreator = (s get().endRemoteOperation() } void get().fetchUpstreamStatus(worktreeId, worktreePath, connectionId) + const refreshGitHubForWorktree = get().refreshGitHubForWorktree + if (typeof refreshGitHubForWorktree === 'function') { + refreshGitHubForWorktree(worktreeId) + } }, syncBranch: async (worktreeId, worktreePath, connectionId, pushTarget) => { // Why: same shape as pushBranch / pullBranch — fire-and-forget the @@ -2411,6 +2419,7 @@ export const createEditorSlice: StateCreator = (s // user invoked Sync; the underlying push is implementation detail. The // outer catch must then skip toasting to avoid a double-toast. let pushStageToastShown = false + let pushed = false try { const context = { settings: get().settings, worktreeId, worktreePath, connectionId } await fetchRuntimeGit(context) @@ -2423,6 +2432,7 @@ export const createEditorSlice: StateCreator = (s if (upstreamStatus.ahead > 0) { try { await pushRuntimeGit(context, { pushTarget }) + pushed = true } catch (error) { // Why: format under the user-facing operation (sync) rather than // the inner step (push) — the user clicked Sync and shouldn't see @@ -2445,6 +2455,12 @@ export const createEditorSlice: StateCreator = (s get().endRemoteOperation() } void get().fetchUpstreamStatus(worktreeId, worktreePath, connectionId) + if (pushed) { + const refreshGitHubForWorktree = get().refreshGitHubForWorktree + if (typeof refreshGitHubForWorktree === 'function') { + refreshGitHubForWorktree(worktreeId) + } + } }, fetchBranch: async (worktreeId, worktreePath, connectionId) => { // Why: same shape as pushBranch / pullBranch — fire-and-forget the diff --git a/src/renderer/src/store/slices/github-checks.ts b/src/renderer/src/store/slices/github-checks.ts index 9dc1927cd..c0fd035a2 100644 --- a/src/renderer/src/store/slices/github-checks.ts +++ b/src/renderer/src/store/slices/github-checks.ts @@ -39,6 +39,7 @@ export function syncPRChecksStatus( repoId: string | undefined, branch: string | undefined, checks: PRCheckDetail[], + headSha?: string, prRepo?: GitHubOwnerRepo | null ): Partial | null { const normalized = branch ? normalizeBranchName(branch) : '' @@ -53,7 +54,10 @@ export function syncPRChecksStatus( } // 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)) { + if (prRepo !== undefined && !samePRRepo(prEntry.data.prRepo, prRepo)) { + return null + } + if (headSha && prEntry.data.headSha && prEntry.data.headSha !== headSha) { return null } diff --git a/src/renderer/src/store/slices/github.test.ts b/src/renderer/src/store/slices/github.test.ts index 442859032..83c6a4779 100644 --- a/src/renderer/src/store/slices/github.test.ts +++ b/src/renderer/src/store/slices/github.test.ts @@ -18,6 +18,8 @@ const runtimeEnvironmentTransportCall = vi.fn() const mockApi = { gh: { prForBranch: vi.fn().mockResolvedValue(null), + refreshPRNow: vi.fn(), + enqueuePRRefresh: vi.fn().mockResolvedValue(undefined), issue: vi.fn().mockResolvedValue(null), prChecks: vi.fn().mockResolvedValue([]), prComments: vi.fn().mockResolvedValue([]), @@ -567,6 +569,62 @@ describe('createGitHubSlice.fetchPRComments', () => { noCache: true }) }) + + it('preserves cached checks when the checks IPC fails', async () => { + const store = createTestStore() + const repoPath = '/repo' + const branch = 'feature/test' + const checksCacheKey = `${repoPath}::pr-checks::12` + const cachedChecks = [ + { name: 'build', status: 'completed', conclusion: 'failure', url: null } as const + ] + + store.setState({ + checksCache: { + [checksCacheKey]: { + data: cachedChecks, + fetchedAt: 1, + headSha: 'abc123head' + } + } + } as unknown as Partial) + mockApi.gh.prChecks.mockRejectedValueOnce(new Error('rate limited')) + + await expect( + store.getState().fetchPRChecks(repoPath, 12, branch, 'abc123head', null, { force: true }) + ).resolves.toEqual(cachedChecks) + + expect(store.getState().checksCache[checksCacheKey]?.data).toEqual(cachedChecks) + expect(store.getState().checksCache[checksCacheKey]?.fetchedAt).toBe(1) + }) + + it('does not return cached checks for a different requested head SHA after IPC failure', async () => { + const store = createTestStore() + const repoPath = '/repo' + const branch = 'feature/test' + const checksCacheKey = `${repoPath}::pr-checks::12` + const oldHeadChecks = [ + { name: 'build', status: 'completed', conclusion: 'success', url: null } as const + ] + + store.setState({ + checksCache: { + [checksCacheKey]: { + data: oldHeadChecks, + fetchedAt: 1, + headSha: 'old-head' + } + } + } as unknown as Partial) + mockApi.gh.prChecks.mockRejectedValueOnce(new Error('rate limited')) + + await expect( + store.getState().fetchPRChecks(repoPath, 12, branch, 'new-head', null, { force: true }) + ).resolves.toEqual([]) + + expect(store.getState().checksCache[checksCacheKey]?.data).toEqual(oldHeadChecks) + expect(store.getState().checksCache[checksCacheKey]?.headSha).toBe('old-head') + }) }) describe('createGitHubSlice.fetchPRForBranch', () => { @@ -574,6 +632,8 @@ describe('createGitHubSlice.fetchPRForBranch', () => { vi.clearAllMocks() resetRemoteRuntimeMocks() mockApi.gh.prForBranch.mockResolvedValue(null) + mockApi.gh.refreshPRNow.mockReset() + mockApi.gh.refreshPRNow.mockResolvedValue({ kind: 'no-pr', fetchedAt: Date.now() }) }) it('lets a forced refresh bypass a non-forced inflight request and keeps the newer result', async () => { @@ -581,6 +641,8 @@ describe('createGitHubSlice.fetchPRForBranch', () => { const repoPath = '/repo' const branch = 'feature/test' const prCacheKey = `${repoPath}::${branch}` + const refreshPRNow = mockApi.gh.refreshPRNow + ;(mockApi.gh as unknown as { refreshPRNow?: typeof refreshPRNow }).refreshPRNow = undefined let resolveInitial: ((value: null) => void) | undefined const initialRequest = new Promise((resolve) => { @@ -591,17 +653,341 @@ describe('createGitHubSlice.fetchPRForBranch', () => { .mockReturnValueOnce(initialRequest) .mockResolvedValueOnce(makePR({ number: 99, title: 'Forced refresh PR' })) - const initialFetch = store.getState().fetchPRForBranch(repoPath, branch) - const forcedFetch = store.getState().fetchPRForBranch(repoPath, branch, { force: true }) + try { + const initialFetch = store.getState().fetchPRForBranch(repoPath, branch) + const forcedFetch = store.getState().fetchPRForBranch(repoPath, branch, { force: true }) - await expect(forcedFetch).resolves.toMatchObject({ number: 99, title: 'Forced refresh PR' }) - expect(mockApi.gh.prForBranch).toHaveBeenCalledTimes(2) - expect(store.getState().prCache[prCacheKey]?.data).toMatchObject({ number: 99 }) + await expect(forcedFetch).resolves.toMatchObject({ number: 99, title: 'Forced refresh PR' }) + expect(mockApi.gh.prForBranch).toHaveBeenCalledTimes(2) + expect(store.getState().prCache[prCacheKey]?.data).toMatchObject({ number: 99 }) - resolveInitial?.(null) - await expect(initialFetch).resolves.toBeNull() + resolveInitial?.(null) + await expect(initialFetch).resolves.toBeNull() - expect(store.getState().prCache[prCacheKey]?.data).toMatchObject({ number: 99 }) + expect(store.getState().prCache[prCacheKey]?.data).toMatchObject({ number: 99 }) + } finally { + mockApi.gh.refreshPRNow = refreshPRNow + } + }) + + it('passes SSH connection identity to GitHub refresh IPC for SSH-backed repos', async () => { + const store = createTestStore() + const repoPath = '/repo' + const branch = 'feature/test' + const pr = makePR({ number: 44 }) + + store.setState({ + repos: [ + { + id: 'repo-1', + path: repoPath, + name: 'repo', + kind: 'git', + connectionId: 'ssh-1' + } + ], + prCache: { + [`repo-1::${branch}`]: { + data: pr, + fetchedAt: Date.now() + } + } + } as unknown as Partial) + mockApi.gh.refreshPRNow.mockResolvedValueOnce({ + kind: 'found', + pr, + fetchedAt: Date.now() + }) + + await expect( + store.getState().fetchPRForBranch(repoPath, branch, { force: true }) + ).resolves.toMatchObject({ number: 44 }) + expect(mockApi.gh.prForBranch).not.toHaveBeenCalled() + expect(mockApi.gh.refreshPRNow).toHaveBeenCalledWith({ + candidate: expect.objectContaining({ + repoId: 'repo-1', + repoPath, + branch, + cacheKey: `repo-1::${branch}`, + connectionId: 'ssh-1' + }) + }) + }) + + it('preserves cached PR data when a forced coordinator refresh errors', async () => { + const store = createTestStore() + const repoPath = '/repo' + const branch = 'feature/test' + const cachedPR = makePR({ number: 12 }) + + store.setState({ + repos: [{ id: 'repo-1', path: repoPath, name: 'repo', kind: 'git' }], + prCache: { + [`repo-1::${branch}`]: { + data: cachedPR, + fetchedAt: 1 + } + } + } as unknown as Partial) + mockApi.gh.refreshPRNow.mockResolvedValueOnce({ + kind: 'upstream-error', + errorType: 'network', + message: 'network unavailable', + fetchedAt: Date.now() + }) + + await expect( + store.getState().fetchPRForBranch(repoPath, branch, { force: true }) + ).resolves.toEqual(cachedPR) + expect(store.getState().prCache[`repo-1::${branch}`]?.data).toEqual(cachedPR) + }) + + it('records PR refresh errors without clearing cached PR data', () => { + const store = createTestStore() + const repoPath = '/repo' + const branch = 'feature/test' + const cacheKey = `${repoPath}::${branch}` + const cachedPR = makePR({ number: 12 }) + + store.setState({ + prCache: { + [cacheKey]: { + data: cachedPR, + fetchedAt: 1 + } + } + } as unknown as Partial) + + store.getState().applyGitHubPRRefreshEvent({ + sequence: 1, + aliases: [{ cacheKey, repoPath, branch }], + reason: 'manual', + outcome: { + kind: 'upstream-error', + errorType: 'network', + message: 'network unavailable', + fetchedAt: Date.now() + } + }) + + expect(store.getState().prCache[cacheKey]?.data).toEqual(cachedPR) + expect(store.getState().prRefreshStates[cacheKey]).toMatchObject({ + status: 'error', + reason: 'manual', + message: 'network unavailable' + }) + }) +}) + +describe('createGitHubSlice.refreshGitHubForWorktreeIfStale', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it('enqueues active PR refresh even when the cached PR is fresh', () => { + const store = createTestStore() + const repoPath = '/repo' + const branch = 'feature/test' + const worktreeId = 'wt-1' + + store.setState({ + repos: [{ id: 'repo-1', path: repoPath, name: 'repo', kind: 'git' }], + worktreesByRepo: { + 'repo-1': [ + { + id: worktreeId, + repoId: 'repo-1', + path: '/repo/worktrees/test', + branch, + displayName: 'test', + isMainWorktree: false, + isBare: false, + isArchived: false + } + ] + }, + worktreeCardProperties: ['pr'], + prCache: { + [`repo-1::${branch}`]: { + data: makePR({ state: 'open' }), + fetchedAt: Date.now() + } + } + } as unknown as Partial) + + store.getState().refreshGitHubForWorktreeIfStale(worktreeId) + + expect(mockApi.gh.enqueuePRRefresh).toHaveBeenCalledWith({ + candidate: expect.objectContaining({ + repoPath, + branch, + cacheKey: `repo-1::${branch}`, + cachedPRState: 'open' + }), + reason: 'active', + priority: 80 + }) + }) + + it('does not enqueue active PR refresh when no PR-related surface is visible', () => { + const store = createTestStore() + const repoPath = '/repo' + const branch = 'feature/test' + const worktreeId = 'wt-1' + + store.setState({ + repos: [{ id: 'repo-1', path: repoPath, name: 'repo', kind: 'git' }], + groupBy: 'repo', + worktreeCardProperties: ['comment'], + rightSidebarOpen: false, + rightSidebarTab: 'source-control', + worktreesByRepo: { + 'repo-1': [ + { + id: worktreeId, + repoId: 'repo-1', + path: '/repo/worktrees/test', + branch, + displayName: 'test', + isMainWorktree: false, + isBare: false, + isArchived: false + } + ] + } + } as unknown as Partial) + + store.getState().refreshGitHubForWorktreeIfStale(worktreeId) + + expect(mockApi.gh.enqueuePRRefresh).not.toHaveBeenCalled() + }) + + it('enqueues active PR refresh IPC for connected SSH-backed repos', () => { + const store = createTestStore() + const repoPath = '/repo' + const branch = 'feature/test' + const worktreeId = 'wt-1' + + store.setState({ + repos: [ + { + id: 'repo-1', + path: repoPath, + name: 'repo', + kind: 'git', + connectionId: 'ssh-1' + } + ], + groupBy: 'pr-status', + sshConnectionStates: new Map([['ssh-1', { status: 'connected' }]]), + worktreesByRepo: { + 'repo-1': [ + { + id: worktreeId, + repoId: 'repo-1', + path: '/repo/worktrees/test', + branch, + displayName: 'test', + isMainWorktree: false, + isBare: false, + isArchived: false + } + ] + } + } as unknown as Partial) + + store.getState().refreshGitHubForWorktreeIfStale(worktreeId) + + expect(mockApi.gh.enqueuePRRefresh).toHaveBeenCalledWith({ + candidate: expect.objectContaining({ + repoPath, + branch, + connectionId: 'ssh-1', + connectionState: 'connected' + }), + reason: 'active', + priority: 80 + }) + }) + + it('enqueues active PR refresh when source control is the visible PR surface', () => { + const store = createTestStore() + const repoPath = '/repo' + const branch = 'feature/test' + const worktreeId = 'wt-1' + + store.setState({ + repos: [{ id: 'repo-1', path: repoPath, name: 'repo', kind: 'git' }], + groupBy: 'repo', + worktreeCardProperties: ['comment'], + rightSidebarOpen: true, + rightSidebarTab: 'source-control', + worktreesByRepo: { + 'repo-1': [ + { + id: worktreeId, + repoId: 'repo-1', + path: '/repo/worktrees/test', + branch, + displayName: 'test', + isMainWorktree: false, + isBare: false, + isArchived: false + } + ] + } + } as unknown as Partial) + + store.getState().refreshGitHubForWorktreeIfStale(worktreeId) + + expect(mockApi.gh.enqueuePRRefresh).toHaveBeenCalledWith({ + candidate: expect.objectContaining({ repoPath, branch }), + reason: 'active', + priority: 80 + }) + }) +}) + +describe('createGitHubSlice.refreshAllGitHub', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + it('refreshes stale PR data when source control is the visible PR surface', () => { + const store = createTestStore() + const repoPath = '/repo' + const branch = 'feature/test' + + store.setState({ + repos: [{ id: 'repo-1', path: repoPath, name: 'repo', kind: 'git' }], + groupBy: 'repo', + worktreeCardProperties: ['comment'], + rightSidebarOpen: true, + rightSidebarTab: 'source-control', + worktreesByRepo: { + 'repo-1': [ + { + id: 'wt-1', + repoId: 'repo-1', + path: '/repo/worktrees/test', + branch, + displayName: 'test', + isMainWorktree: false, + isBare: false, + isArchived: false, + lastActivityAt: 1 + } + ] + } + } as unknown as Partial) + + store.getState().refreshAllGitHub() + + expect(mockApi.gh.enqueuePRRefresh).toHaveBeenCalledWith({ + candidate: expect.objectContaining({ repoPath, branch }), + reason: 'swr', + priority: 10 + }) }) }) diff --git a/src/renderer/src/store/slices/github.ts b/src/renderer/src/store/slices/github.ts index e4227b89e..5762232ab 100644 --- a/src/renderer/src/store/slices/github.ts +++ b/src/renderer/src/store/slices/github.ts @@ -8,9 +8,13 @@ import type { GitHubOwnerRepo, IssueSourcePreference, PRInfo, + GitHubPRRefreshCandidate, + GitHubPRRefreshEvent, + GitHubPRRefreshReason, IssueInfo, PRCheckDetail, PRComment, + Repo, Worktree, GitHubWorkItem } from '../../../../shared/types' @@ -24,7 +28,7 @@ import type { GitHubProjectViewError } from '../../../../shared/github-project-types' import { sortWorkItemsByUpdatedAt, PER_REPO_FETCH_LIMIT } from '../../../../shared/work-items' -import { syncPRChecksStatus } from './github-checks' +import { deriveCheckStatusFromChecks, syncPRChecksStatus } from './github-checks' import { callRuntimeRpc, getActiveRuntimeTarget } from '../../runtime/runtime-rpc-client' // ─── ProjectV2 cache types ──────────────────────────────────────────── @@ -78,6 +82,18 @@ function queryOverrideKeyPart(queryOverride: string | undefined): string { return `:q=${queryOverride}` } +function getRuntimeRepoTarget( + state: AppState, + repoPath: string +): { target: { kind: 'environment'; environmentId: string }; repo: Repo } | null { + const target = getActiveRuntimeTarget(state.settings) + if (target.kind !== 'environment') { + return null + } + const repo = state.repos.find((candidate) => candidate.path === repoPath) + return repo ? { target, repo } : null +} + export function projectViewCacheKey( ownerType: GetProjectViewTableArgs['ownerType'], owner: string, @@ -261,6 +277,7 @@ export type WorkItemsCacheError = ClassifiedError & { source: GitHubOwnerRepo } export type CacheEntry = { data: T | null fetchedAt: number + headSha?: string /** * Resolved issue/PR owner/repo slugs for this entry. Set only on entries * populated by `fetchWorkItems` — PR and issue single-item caches don't @@ -293,6 +310,18 @@ type RepoScopedFetchOptions = FetchOptions & { repoId?: string } +type PRRefreshState = { + status: 'queued' | 'in-flight' | 'paused' | 'skipped' | 'error' + reason: GitHubPRRefreshReason + updatedAt: number + pausedUntil?: number + message?: string +} + +function bypassesGitHubPRRefreshFreshness(reason: GitHubPRRefreshReason): boolean { + return reason === 'manual' || reason === 'active' || reason === 'post-push' +} + const CACHE_TTL = 300_000 // 5 minutes (stale data shown instantly, then refreshed) const CHECKS_CACHE_TTL = 60_000 // 1 minute — checks change more frequently // Why: the NewWorkspace page's work-item list is a browse surface, not a @@ -420,6 +449,53 @@ function isFresh(entry: CacheEntry | undefined, ttl = CACHE_TTL): entry is return entry !== undefined && Date.now() - entry.fetchedAt < ttl } +function findWorktreeById(state: AppState, worktreeId: string): Worktree | null { + for (const worktrees of Object.values(state.worktreesByRepo)) { + const worktree = worktrees.find((w) => w.id === worktreeId) + if (worktree) { + return worktree + } + } + return null +} + +function buildPRRefreshCandidate( + state: AppState, + worktree: Worktree, + repoPath?: string +): GitHubPRRefreshCandidate | null { + const repo = state.repos.find((r) => r.id === worktree.repoId) + if (!repo) { + return null + } + const branch = worktree.branch.replace(/^refs\/heads\//, '') + const cacheKey = repoScopedCacheKey(repoPath ?? repo.path, repo.id, branch) + const sshStatus = repo.connectionId + ? state.sshConnectionStates.get(repo.connectionId)?.status + : null + return { + repoId: repo.id, + repoPath: repoPath ?? repo.path, + repoKind: repo.kind ?? 'git', + branch, + cacheKey, + worktreeId: worktree.id, + linkedPRNumber: worktree.linkedPR ?? null, + isBare: worktree.isBare, + isArchived: worktree.isArchived, + connectionId: repo.connectionId ?? null, + connectionState: repo.connectionId + ? sshStatus === 'connected' + ? 'connected' + : 'disconnected' + : 'unknown', + cachedFetchedAt: state.prCache[cacheKey]?.fetchedAt ?? null, + cachedHasPR: state.prCache[cacheKey]?.data ? true : state.prCache[cacheKey] ? false : null, + cachedPRState: state.prCache[cacheKey]?.data?.state ?? null, + cachedChecksStatus: state.prCache[cacheKey]?.data?.checksStatus ?? null + } +} + /** * Evict the oldest entries from a cache record when it exceeds the max size. * Returns a pruned copy, or the original reference if no eviction was needed. @@ -465,6 +541,9 @@ export type GitHubSlice = { issueCache: Record> checksCache: Record> commentsCache: Record> + prRefreshSequences: Record + prRefreshStates: Record + prVisibleRefreshGeneration: number // Why: keyed by repoId + limit + query so remote repos with the same path on // different SSH targets do not share issue/PR results. // from cache instantly on mount (and on hover-prefetch from sidebar buttons) @@ -504,6 +583,14 @@ export type GitHubSlice = { refreshAllGitHub: () => void refreshGitHubForWorktree: (worktreeId: string) => void refreshGitHubForWorktreeIfStale: (worktreeId: string) => void + enqueueGitHubPRRefresh: ( + worktreeId: string, + reason: GitHubPRRefreshReason, + priority?: number + ) => void + reportVisibleGitHubPRRefreshCandidates: (worktreeIds: string[], generation: number) => void + bumpGitHubPRVisibleRefreshGeneration: () => void + applyGitHubPRRefreshEvent: (event: GitHubPRRefreshEvent) => void /** * Why: returns cached work items immediately (null if none) and fires a * background refresh when stale. Callers can render the cached list while @@ -652,6 +739,9 @@ export const createGitHubSlice: StateCreator = (s issueCache: {}, checksCache: {}, commentsCache: {}, + prRefreshSequences: {}, + prRefreshStates: {}, + prVisibleRefreshGeneration: 0, workItemsCache: {}, workItemsInvalidationNonce: 0, projectViewCache: {}, @@ -1323,7 +1413,10 @@ export const createGitHubSlice: StateCreator = (s }, fetchPRForBranch: async (repoPath, branch, options): Promise => { - const repoId = options?.repoId ?? get().repos?.find((repo) => repo.path === repoPath)?.id + const repo = get().repos?.find((candidate) => + options?.repoId ? candidate.id === options.repoId : candidate.path === repoPath + ) + const repoId = options?.repoId ?? repo?.id const cacheKey = repoScopedCacheKey(repoPath, repoId, branch) const cached = get().prCache[cacheKey] // Why: if a prior caller without a linkedPR cached `null` for this branch, @@ -1346,22 +1439,53 @@ export const createGitHubSlice: StateCreator = (s const linkedPRNumber = options?.linkedPRNumber ?? null const request = (async () => { try { - const pr = await window.api.gh.prForBranch({ repoPath, repoId, branch, linkedPRNumber }) + const runtimeRepo = getRuntimeRepoTarget(get(), repoPath) + const outcome = runtimeRepo + ? await callRuntimeRpc( + runtimeRepo.target, + 'github.prForBranch', + { repo: runtimeRepo.repo.id, branch, linkedPRNumber }, + { timeoutMs: 30_000 } + ).then((pr) => + pr + ? ({ kind: 'found', pr, fetchedAt: Date.now() } as const) + : ({ kind: 'no-pr', fetchedAt: Date.now() } as const) + ) + : await (async () => { + const candidate: GitHubPRRefreshCandidate = { + repoId: repoId ?? '', + repoPath, + repoKind: repo?.kind ?? 'git', + branch, + cacheKey, + linkedPRNumber, + connectionId: repo?.connectionId ?? null, + cachedFetchedAt: cached?.fetchedAt ?? null + } + return window.api.gh.refreshPRNow + ? await window.api.gh.refreshPRNow({ candidate }) + : await window.api.gh + .prForBranch({ repoPath, repoId, branch, linkedPRNumber }) + .then((pr) => + pr + ? ({ kind: 'found', pr, fetchedAt: Date.now() } as const) + : ({ kind: 'no-pr', fetchedAt: Date.now() } as const) + ) + })() + const pr: PRInfo | null = + outcome.kind === 'found' ? outcome.pr : outcome.kind === 'no-pr' ? null : null + if (outcome.kind === 'upstream-error') { + return cached?.data ?? null + } if (prRequestGenerations.get(cacheKey) === generation) { set((s) => ({ - prCache: { ...s.prCache, [cacheKey]: { data: pr, fetchedAt: Date.now() } } + prCache: { ...s.prCache, [cacheKey]: { data: pr, fetchedAt: outcome.fetchedAt } } })) debouncedSaveCache(get()) } - return pr + return pr ?? null } catch (err) { console.error('Failed to fetch PR:', err) - if (prRequestGenerations.get(cacheKey) === generation) { - set((s) => ({ - prCache: { ...s.prCache, [cacheKey]: { data: null, fetchedAt: Date.now() } } - })) - debouncedSaveCache(get()) - } return null } finally { const activeRequest = inflightPRRequests.get(cacheKey) @@ -1426,8 +1550,13 @@ export const createGitHubSlice: StateCreator = (s ): Promise => { const repoId = options?.repoId ?? get().repos?.find((repo) => repo.path === repoPath)?.id const cacheKey = repoScopedCacheKey(repoPath, repoId, prChecksCacheSuffix(prNumber, prRepo)) + const inflightKey = `${cacheKey}::${headSha ?? 'unknown'}` const cached = get().checksCache[cacheKey] - if (!options?.force && isFresh(cached, CHECKS_CACHE_TTL)) { + if ( + !options?.force && + isFresh(cached, CHECKS_CACHE_TTL) && + (!headSha || cached.headSha === headSha) + ) { const cachedChecks = cached.data ?? [] const prStatusUpdate = syncPRChecksStatus( get(), @@ -1435,6 +1564,7 @@ export const createGitHubSlice: StateCreator = (s repoId, branch, cachedChecks, + cached.headSha, prRepo ) if (prStatusUpdate) { @@ -1444,27 +1574,52 @@ export const createGitHubSlice: StateCreator = (s return cachedChecks } - const inflightRequest = inflightChecksRequests.get(cacheKey) + const inflightRequest = inflightChecksRequests.get(inflightKey) if (inflightRequest) { return inflightRequest } const request = (async () => { try { - const checks = (await window.api.gh.prChecks({ - repoPath, - repoId, - prNumber, - headSha, - prRepo: prRepo ?? null, - noCache: options?.force - })) as PRCheckDetail[] + const runtimeRepo = getRuntimeRepoTarget(get(), repoPath) + const checks = runtimeRepo + ? await callRuntimeRpc( + runtimeRepo.target, + 'github.prChecks', + { + repo: runtimeRepo.repo.id, + prNumber, + headSha, + prRepo: prRepo ?? null, + noCache: options?.force + }, + { timeoutMs: 30_000 } + ) + : ((await window.api.gh.prChecks({ + repoPath, + repoId, + prNumber, + headSha, + prRepo: prRepo ?? null, + noCache: options?.force + })) as PRCheckDetail[]) set((s) => { const nextState: Partial = { - checksCache: { ...s.checksCache, [cacheKey]: { data: checks, fetchedAt: Date.now() } } + checksCache: { + ...s.checksCache, + [cacheKey]: { data: checks, fetchedAt: Date.now(), headSha } + } } - const prStatusUpdate = syncPRChecksStatus(s, repoPath, repoId, branch, checks, prRepo) + const prStatusUpdate = syncPRChecksStatus( + s, + repoPath, + repoId, + branch, + checks, + headSha, + prRepo + ) if (prStatusUpdate?.prCache) { nextState.prCache = prStatusUpdate.prCache } @@ -1475,13 +1630,17 @@ export const createGitHubSlice: StateCreator = (s return checks } catch (err) { console.error('Failed to fetch PR checks:', err) - return get().checksCache[cacheKey]?.data ?? [] + const latestCached = get().checksCache[cacheKey] + if (latestCached?.data && (!headSha || latestCached.headSha === headSha)) { + return latestCached.data + } + return [] } finally { - inflightChecksRequests.delete(cacheKey) + inflightChecksRequests.delete(inflightKey) } })() - inflightChecksRequests.set(cacheKey, request) + inflightChecksRequests.set(inflightKey, request) return request }, @@ -1566,12 +1725,151 @@ export const createGitHubSlice: StateCreator = (s return ok }, + enqueueGitHubPRRefresh: (worktreeId, reason, priority = 0) => { + const state = get() + const worktree = findWorktreeById(state, worktreeId) + const candidate = worktree ? buildPRRefreshCandidate(state, worktree) : null + if (!candidate) { + return + } + const enqueue = window.api.gh.enqueuePRRefresh + if (enqueue) { + void enqueue({ candidate, reason, priority }) + .then((queued) => { + if (queued === false) { + return get().fetchPRForBranch(candidate.repoPath, candidate.branch, { + force: bypassesGitHubPRRefreshFreshness(reason), + repoId: candidate.repoId, + linkedPRNumber: candidate.linkedPRNumber ?? null + }) + } + return null + }) + .catch((err) => { + console.warn('Failed to enqueue PR refresh:', err) + }) + } + }, + + reportVisibleGitHubPRRefreshCandidates: (worktreeIds, generation) => { + const state = get() + const candidates = worktreeIds + .map((id) => { + const worktree = findWorktreeById(state, id) + return worktree ? buildPRRefreshCandidate(state, worktree) : null + }) + .filter((candidate): candidate is GitHubPRRefreshCandidate => candidate !== null) + const reportVisible = window.api.gh.reportVisiblePRRefreshCandidates + if (reportVisible) { + void reportVisible({ candidates, generation }).catch((err) => { + console.warn('Failed to report visible PR refresh candidates:', err) + }) + } + }, + + bumpGitHubPRVisibleRefreshGeneration: () => { + set((s) => ({ prVisibleRefreshGeneration: s.prVisibleRefreshGeneration + 1 })) + }, + + applyGitHubPRRefreshEvent: (event) => { + set((s) => { + const nextSequences = { ...s.prRefreshSequences } + const nextStates = { ...s.prRefreshStates } + let nextPRCache = s.prCache + let changed = false + + for (const alias of event.aliases) { + const previousSequence = nextSequences[alias.cacheKey] ?? 0 + if ( + event.outcome ? event.sequence < previousSequence : event.sequence <= previousSequence + ) { + continue + } + nextSequences[alias.cacheKey] = event.sequence + changed = true + + if (event.outcome) { + delete nextStates[alias.cacheKey] + if (event.outcome.kind === 'upstream-error') { + nextStates[alias.cacheKey] = { + status: 'error', + reason: event.reason, + updatedAt: Date.now(), + message: event.outcome.message + } + continue + } + const data = + event.outcome.kind === 'found' + ? (() => { + const pr = event.outcome.pr + const checksCacheKeys = [ + ...(alias.repoId + ? [ + repoScopedCacheKey( + alias.repoPath, + alias.repoId, + prChecksCacheSuffix(pr.number, pr.prRepo) + ) + ] + : []), + repoScopedCacheKey( + alias.repoPath, + undefined, + prChecksCacheSuffix(pr.number, pr.prRepo) + ), + `${alias.repoPath}::pr-checks::${pr.number}` + ] + const checksEntry = checksCacheKeys + .map((key) => s.checksCache[key]) + .find((entry) => entry?.data) + if ( + checksEntry?.data && + checksEntry.headSha && + pr.headSha && + checksEntry.headSha === pr.headSha && + event.outcome.fetchedAt - checksEntry.fetchedAt < CHECKS_CACHE_TTL + ) { + return { ...pr, checksStatus: deriveCheckStatusFromChecks(checksEntry.data) } + } + return pr + })() + : null + nextPRCache = { + ...nextPRCache, + [alias.cacheKey]: { data, fetchedAt: event.outcome.fetchedAt } + } + continue + } + + if (event.status) { + nextStates[alias.cacheKey] = { + status: event.status, + reason: event.reason, + updatedAt: Date.now(), + pausedUntil: event.pausedUntil + } + } + } + + return changed + ? { + prRefreshSequences: nextSequences, + prRefreshStates: nextStates, + prCache: nextPRCache + } + : {} + }) + if (event.outcome && event.outcome.kind !== 'upstream-error') { + debouncedSaveCache(get()) + } + }, + refreshAllGitHub: () => { - // Invalidate checks and comments caches so they refresh on next access. + // Invalidate comments cache so it refreshes on next access. // Also evict old entries from prCache and issueCache to prevent unbounded // growth across many repos and branches over a long-running session. set((s) => ({ - checksCache: {}, commentsCache: {}, prCache: evictStaleEntries(s.prCache), issueCache: evictStaleEntries(s.issueCache) @@ -1588,6 +1886,17 @@ export const createGitHubSlice: StateCreator = (s // Only re-fetch PR/issue entries that are already stale — skip fresh ones const state = get() const now = Date.now() + const stalePRCandidates: { candidate: GitHubPRRefreshCandidate; score: number }[] = [] + const cardProps = state.worktreeCardProperties ?? [] + const isPRStatusGrouping = state.groupBy === 'pr-status' + const rightSidebarShowsPR = + state.rightSidebarOpen && + (state.rightSidebarTab === 'checks' || state.rightSidebarTab === 'source-control') + const shouldRefreshPRs = + isPRStatusGrouping || + rightSidebarShowsPR || + cardProps.includes('pr') || + cardProps.includes('ci') for (const worktrees of Object.values(state.worktreesByRepo)) { for (const wt of worktrees) { @@ -1597,11 +1906,19 @@ export const createGitHubSlice: StateCreator = (s } const branch = wt.branch.replace(/^refs\/heads\//, '') - if (!wt.isBare && branch) { + if (shouldRefreshPRs && !wt.isBare && branch) { const prKey = repoScopedCacheKey(repo.path, repo.id, branch) const prEntry = state.prCache[prKey] if (!prEntry || now - prEntry.fetchedAt >= CACHE_TTL) { - void get().fetchPRForBranch(repo.path, branch, { repoId: repo.id }) + const candidate = buildPRRefreshCandidate(state, wt) + if (candidate) { + stalePRCandidates.push({ + candidate, + score: + (state.activeWorktreeId === wt.id ? Number.MAX_SAFE_INTEGER : 0) + + wt.lastActivityAt + }) + } } } if (wt.linkedIssue) { @@ -1613,6 +1930,12 @@ export const createGitHubSlice: StateCreator = (s } } } + const candidatesToRefresh = stalePRCandidates + .sort((a, b) => b.score - a.score) + .slice(0, isPRStatusGrouping ? stalePRCandidates.length : 5) + for (const { candidate } of candidatesToRefresh) { + void window.api.gh.enqueuePRRefresh?.({ candidate, reason: 'swr', priority: 10 }) + } }, refreshGitHubForWorktree: (worktreeId) => { @@ -1656,7 +1979,10 @@ export const createGitHubSlice: StateCreator = (s // Re-fetch (skip when branch is empty — detached HEAD during rebase) if (!worktree.isBare && branch) { - void get().fetchPRForBranch(repo.path, branch, { force: true, repoId: repo.id }) + const candidate = buildPRRefreshCandidate(get(), worktree) + if (candidate) { + void window.api.gh.enqueuePRRefresh?.({ candidate, reason: 'post-push', priority: 100 }) + } } if (worktree.linkedIssue) { void get().fetchIssue(repo.path, worktree.linkedIssue, { repoId: repo.id }) @@ -1793,9 +2119,9 @@ export const createGitHubSlice: StateCreator = (s }) }, - // Why: worktree switches previously force-refreshed GitHub data on every - // click, bypassing the 5-min TTL. This variant only fetches when stale, - // avoiding unnecessary API calls and latency during rapid switching. + // Why: activation is the user's strongest freshness signal. A PR can merge + // seconds after the last sidebar poll; enqueue through the coordinator so + // clicks revalidate PR state without bypassing coalescing/rate-limit guards. refreshGitHubForWorktreeIfStale: (worktreeId) => { const state = get() let worktree: Worktree | undefined @@ -1816,12 +2142,19 @@ export const createGitHubSlice: StateCreator = (s const now = Date.now() const branch = worktree.branch.replace(/^refs\/heads\//, '') - const prKey = repoScopedCacheKey(repo.path, repo.id, branch) - const prEntry = state.prCache[prKey] - const prStale = !prEntry || now - prEntry.fetchedAt >= CACHE_TTL + const cardProps = state.worktreeCardProperties ?? [] + const shouldRefreshPR = + state.groupBy === 'pr-status' || + cardProps.includes('pr') || + cardProps.includes('ci') || + (state.rightSidebarOpen && + (state.rightSidebarTab === 'checks' || state.rightSidebarTab === 'source-control')) - if (!worktree.isBare && branch && prStale) { - void get().fetchPRForBranch(repo.path, branch, { force: true, repoId: repo.id }) + if (shouldRefreshPR && !worktree.isBare && branch) { + const candidate = buildPRRefreshCandidate(state, worktree) + if (candidate) { + void window.api.gh.enqueuePRRefresh?.({ candidate, reason: 'active', priority: 80 }) + } } if (worktree.linkedIssue) { diff --git a/src/renderer/src/store/slices/ssh.ts b/src/renderer/src/store/slices/ssh.ts index 4fc773840..e2ae98fe5 100644 --- a/src/renderer/src/store/slices/ssh.ts +++ b/src/renderer/src/store/slices/ssh.ts @@ -54,7 +54,6 @@ export type SshSlice = { setRemoteWorkspaceSyncStatus: (targetId: string, status: RemoteWorkspaceSyncStatus) => void enqueueSshCredentialRequest: (req: SshCredentialRequest) => void removeSshCredentialRequest: (requestId: string) => void - bumpSshConnectedGeneration: () => void setPortForwards: (targetId: string, forwards: PortForwardEntry[]) => void clearPortForwards: (targetId: string) => void setDetectedPorts: (targetId: string, ports: DetectedPort[]) => void @@ -74,8 +73,15 @@ export const createSshSlice: StateCreator = (set) => setSshConnectionState: (targetId, state) => set((s) => { const next = new Map(s.sshConnectionStates) + const previous = next.get(targetId) next.set(targetId, state) - return { sshConnectionStates: next } + return { + sshConnectionStates: next, + sshConnectedGeneration: + previous?.status !== 'connected' && state.status === 'connected' + ? s.sshConnectedGeneration + 1 + : s.sshConnectedGeneration + } }), setSshTargetLabels: (labels) => set({ sshTargetLabels: labels }), @@ -111,8 +117,6 @@ export const createSshSlice: StateCreator = (set) => set((s) => ({ sshCredentialQueue: s.sshCredentialQueue.filter((req) => req.requestId !== requestId) })), - bumpSshConnectedGeneration: () => - set((s) => ({ sshConnectedGeneration: s.sshConnectedGeneration + 1 })), setPortForwards: (targetId, forwards) => set((s) => { diff --git a/src/renderer/src/store/slices/worktrees.ts b/src/renderer/src/store/slices/worktrees.ts index c93056e8a..0d2c70689 100644 --- a/src/renderer/src/store/slices/worktrees.ts +++ b/src/renderer/src/store/slices/worktrees.ts @@ -1475,9 +1475,8 @@ export const createWorktreeSlice: StateCreator } }) - // Why: force-refreshing GitHub data on every switch burned API rate limit - // quota and added 200-800ms latency. Only refresh when cache is actually - // stale (>5 min old). Users can still force-refresh via the sidebar button. + // Why: activation is explicit enough to revalidate PR state immediately; + // the GitHub coordinator still coalesces requests and applies rate guards. if (worktreeId) { get().refreshGitHubForWorktreeIfStale(worktreeId) } diff --git a/src/renderer/src/web/web-preload-api.ts b/src/renderer/src/web/web-preload-api.ts index b76ff725e..456d69ac9 100644 --- a/src/renderer/src/web/web-preload-api.ts +++ b/src/renderer/src/web/web-preload-api.ts @@ -688,6 +688,20 @@ function createGitHubApi(): NonNullable['gh']> { viewer: () => Promise.resolve(null), repoSlug: direct('github.repoSlug'), prForBranch: direct('github.prForBranch'), + refreshPRNow: async ({ candidate }) => { + const pr = await callRuntimeResult('github.prForBranch', { + repo: candidate.repoId || candidate.repoPath, + repoPath: candidate.repoPath, + branch: candidate.branch, + linkedPRNumber: candidate.linkedPRNumber ?? null + }) + return pr + ? { kind: 'found', pr, fetchedAt: Date.now() } + : { kind: 'no-pr', fetchedAt: Date.now() } + }, + enqueuePRRefresh: () => Promise.resolve(false), + reportVisiblePRRefreshCandidates: () => Promise.resolve(false), + onPRRefreshEvent: () => noopUnsubscribe, issue: direct('github.issue'), workItem: direct('github.workItem'), workItemByOwnerRepo: direct('github.workItemByOwnerRepo'), diff --git a/src/shared/types.ts b/src/shared/types.ts index bdc8c2090..41e8bbc43 100644 --- a/src/shared/types.ts +++ b/src/shared/types.ts @@ -608,6 +608,88 @@ export type PRInfo = { conflictSummary?: PRConflictSummary } +export type PRRefreshOutcome = + | { kind: 'found'; pr: PRInfo; fetchedAt: number } + | { kind: 'no-pr'; fetchedAt: number } + | { + kind: 'upstream-error' + errorType: + | 'rate_limited' + | 'auth' + | 'network' + | 'permission' + | 'repo_unavailable' + | 'gh_unavailable' + | 'unknown' + message: string + fetchedAt: number + } + +export type GitHubPRRefreshReason = 'visible' | 'active' | 'post-push' | 'manual' | 'swr' + +export type GitHubPRRefreshAlias = { + cacheKey: string + repoId?: string + repoPath: string + branch: string + worktreeId?: string +} + +export type GitHubPRRefreshCandidate = GitHubPRRefreshAlias & { + linkedPRNumber?: number | null + repoKind: RepoKind + repoId: string + isBare?: boolean + isArchived?: boolean + connectionId?: string | null + connectionState?: 'connected' | 'disconnected' | 'unknown' + cachedFetchedAt?: number | null + cachedHasPR?: boolean | null + cachedPRState?: PRState | null + cachedChecksStatus?: CheckStatus | null +} + +export type GitHubPRRefreshSkippedReason = + | 'fresh' + | 'not-git' + | 'bare' + | 'archived' + | 'disconnected' + | 'remote' + | 'rate-limit' + +type GitHubPRRefreshEventBase = { + sequence: number + reason: GitHubPRRefreshReason + aliases: GitHubPRRefreshAlias[] +} + +export type GitHubPRRefreshEvent = + | (GitHubPRRefreshEventBase & { + outcome: PRRefreshOutcome + status?: never + pausedUntil?: never + skippedReason?: never + }) + | (GitHubPRRefreshEventBase & { + status: 'queued' | 'in-flight' + outcome?: never + pausedUntil?: never + skippedReason?: never + }) + | (GitHubPRRefreshEventBase & { + status: 'paused' + pausedUntil: number + skippedReason: 'rate-limit' + outcome?: never + }) + | (GitHubPRRefreshEventBase & { + status: 'skipped' + skippedReason: GitHubPRRefreshSkippedReason + outcome?: never + pausedUntil?: never + }) + export type PRCheckDetail = { name: string status: 'queued' | 'in_progress' | 'completed'