From 169ab144fb4e5ad91149031fcd28bccae510f894 Mon Sep 17 00:00:00 2001 From: Jinwoo-H Date: Tue, 12 May 2026 17:19:06 -0400 Subject: [PATCH] Fix GitHub refresh review follow-ups Co-authored-by: Orca --- src/main/github/client.test.ts | 37 ++++++ src/main/github/client.ts | 121 +++++++++++------- src/main/github/pr-refresh-coordinator.ts | 20 ++- .../components/right-sidebar/ChecksPanel.tsx | 98 ++++++++------ .../src/components/sidebar/WorktreeCard.tsx | 32 +++-- src/renderer/src/store/slices/github.ts | 5 +- src/shared/types.ts | 9 +- 7 files changed, 212 insertions(+), 110 deletions(-) diff --git a/src/main/github/client.test.ts b/src/main/github/client.test.ts index 4af9e9aaba6..365c895b627 100644 --- a/src/main/github/client.test.ts +++ b/src/main/github/client.test.ts @@ -102,6 +102,43 @@ describe('getPRForBranch', () => { expect(pr?.mergeable).toBe('MERGEABLE') }) + 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({ diff --git a/src/main/github/client.ts b/src/main/github/client.ts index 930919cc295..b714c675744 100644 --- a/src/main/github/client.ts +++ b/src/main/github/client.ts @@ -51,9 +51,25 @@ import { deriveCheckStatus } from './mappers' import { mapGraphQLReactionGroups, type GitHubGraphQLReactionGroup } from './comment-reactions' +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()}` + ) + } +} + type PRBranchData = { number: number title: string @@ -984,11 +1000,10 @@ export async function getWorkItemByOwnerRepo( * Get PR info for a given branch using gh CLI. * Returns null if gh is not installed, or no PR exists for the branch. * - * When `linkedPRNumber` is provided and the branch lookup yields nothing, - * falls back to looking up the PR by number. This handles "create from PR" - * worktrees, whose branch is a fresh local branch (not the PR's head ref) — - * the branch-keyed lookup misses, but the user still expects the linked PR - * to surface on the worktree card. + * When `linkedPRNumber` is provided, 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, @@ -1012,10 +1027,45 @@ export async function getPRForBranchOutcome( const ownerRepo = await getOwnerRepo(repoPath) let data: PRBranchData | null = null - // During a rebase the worktree is in detached HEAD and branch is empty. - // An empty --head filter causes gh to return an arbitrary PR — skip the - // branch lookup and rely on the linkedPR fallback below if available. - if (branchName) { + if (typeof linkedPRNumber === 'number') { + const args = ownerRepo + ? [ + 'pr', + 'view', + String(linkedPRNumber), + '--repo', + `${ownerRepo.owner}/${ownerRepo.repo}`, + '--json', + 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,baseRefName,headRefName,baseRefOid,headRefOid' + ] + : [ + 'pr', + 'view', + String(linkedPRNumber), + '--json', + 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,baseRefName,headRefName,baseRefOid,headRefOid' + ] + try { + const { stdout } = await ghExecFileAsync(args, { cwd: repoPath }) + data = JSON.parse(stdout) + } catch (err) { + if (!isNoPullRequestError(err)) { + return { + kind: 'upstream-error', + errorType: classifyPRRefreshError(err), + message: err instanceof Error ? err.message : String(err), + fetchedAt: Date.now() + } + } + // 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 (ownerRepo) { const { stdout } = await ghExecFileAsync( [ @@ -1059,44 +1109,6 @@ export async function getPRForBranchOutcome( } } - if (!data && typeof linkedPRNumber === 'number') { - const args = ownerRepo - ? [ - 'pr', - 'view', - String(linkedPRNumber), - '--repo', - `${ownerRepo.owner}/${ownerRepo.repo}`, - '--json', - 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,baseRefName,headRefName,baseRefOid,headRefOid' - ] - : [ - 'pr', - 'view', - String(linkedPRNumber), - '--json', - 'number,title,state,url,statusCheckRollup,updatedAt,isDraft,mergeable,baseRefName,headRefName,baseRefOid,headRefOid' - ] - try { - const { stdout } = await ghExecFileAsync(args, { cwd: repoPath }) - data = JSON.parse(stdout) - } catch (err) { - if (!isNoPullRequestError(err)) { - return { - kind: 'upstream-error', - errorType: classifyPRRefreshError(err), - message: err instanceof Error ? err.message : String(err), - fetchedAt: Date.now() - } - } - // 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) { return { kind: 'no-pr', fetchedAt: Date.now() } } @@ -1145,6 +1157,12 @@ export async function getPRChecks( options?: { noCache?: boolean } ): Promise { const ownerRepo = headSha ? await getOwnerRepo(repoPath) : null + if (ownerRepo && headSha) { + await assertRateLimitBudget('core') + } + // Why: even the REST check-runs path can fall back to `gh pr checks`, so + // guard GraphQL before acquiring the global gh lock. + await assertRateLimitBudget('graphql') await acquire() try { if (ownerRepo && headSha) { @@ -1160,6 +1178,7 @@ export async function getPRChecks( ], { cwd: repoPath } ) + noteRateLimitSpend('core') const data = JSON.parse(stdout) as { check_runs: { name: string @@ -1187,6 +1206,7 @@ export async function getPRChecks( ['pr', 'checks', String(prNumber), '--json', 'name,state,link'], { cwd: repoPath } ) + noteRateLimitSpend('graphql') const data = JSON.parse(stdout) as { name: string; state: string; link: string }[] return data.map((d) => ({ name: d.name, @@ -1265,6 +1285,10 @@ export async function getPRComments( options?: { noCache?: boolean } ): Promise { const ownerRepo = await getOwnerRepo(repoPath) + if (ownerRepo) { + await assertRateLimitBudget('core') + } + await assertRateLimitBudget('graphql') await acquire() try { if (ownerRepo) { @@ -1309,6 +1333,8 @@ export async function getPRComments( { cwd: repoPath, encoding: 'utf-8' } ) ]) + noteRateLimitSpend('core', 2) + noteRateLimitSpend('graphql') // Parse issue comments (REST) type RESTComment = { @@ -1459,6 +1485,7 @@ export async function getPRComments( ['pr', 'view', String(prNumber), '--json', 'comments'], { cwd: repoPath, encoding: 'utf-8' } ) + noteRateLimitSpend('graphql') const data = JSON.parse(stdout) as { comments: { author: { login: string } diff --git a/src/main/github/pr-refresh-coordinator.ts b/src/main/github/pr-refresh-coordinator.ts index 6a67db63721..cb5a7d1dc50 100644 --- a/src/main/github/pr-refresh-coordinator.ts +++ b/src/main/github/pr-refresh-coordinator.ts @@ -118,6 +118,9 @@ function validateCandidate( if (candidate.isArchived) { return 'archived' } + if (candidate.connectionId && candidate.connectionState === 'connected') { + return 'remote' + } if (candidate.connectionId && candidate.connectionState === 'disconnected') { return 'disconnected' } @@ -468,6 +471,12 @@ export async function refreshPRNow(candidate: GitHubPRRefreshCandidate): Promise 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) { const outcome: PRRefreshOutcome = { @@ -476,19 +485,20 @@ export async function refreshPRNow(candidate: GitHubPRRefreshCandidate): Promise message: `Cannot refresh PR for this worktree: ${skippedReason}`, fetchedAt: Date.now() } - broadcast({ aliases: [alias], reason: 'manual', status: 'skipped', skippedReason }) + broadcast({ aliases, reason: 'manual', status: 'skipped', skippedReason }) return outcome } - queue.delete(refreshKey(candidate)) + queue.delete(key) const requestSequence = nextSequence() - broadcast({ aliases: [alias], reason: 'manual', status: 'in-flight' }, requestSequence) + broadcast({ aliases, reason: 'manual', status: 'in-flight' }, requestSequence) const outcome = await getPRForBranchOutcome( candidate.repoPath, candidate.branch, candidate.linkedPRNumber ?? null ) - broadcast({ aliases: [alias], reason: 'manual', outcome }, requestSequence) - scheduleVisibleFollowUp(refreshKey(candidate), candidate, outcome, 40, [alias]) + outcomeObserver?.(candidate, outcome) + broadcast({ aliases, reason: 'manual', outcome }, requestSequence) + scheduleVisibleFollowUp(key, candidate, outcome, 40, aliases) return outcome } diff --git a/src/renderer/src/components/right-sidebar/ChecksPanel.tsx b/src/renderer/src/components/right-sidebar/ChecksPanel.tsx index 628a7ad9efb..07f160ef4ec 100644 --- a/src/renderer/src/components/right-sidebar/ChecksPanel.tsx +++ b/src/renderer/src/components/right-sidebar/ChecksPanel.tsx @@ -76,6 +76,9 @@ export default function ChecksPanel(): React.JSX.Element { const isFolder = repo ? isFolderRepo(repo) : false const prCacheKey = repo && branch ? `${repo.path}::${branch}` : '' 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 conflictOperation = activeWorktreeId ? (gitConflictOperationByWorktree[activeWorktreeId] ?? 'unknown') @@ -311,42 +314,25 @@ export default function ChecksPanel(): React.JSX.Element { } }, [repo, branch, linkedPR, fetchPRForBranch, fetchPRChecks, fetchComments]) - const handleEntryRefresh = useCallback(async () => { - 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, while checks/comments can refresh from cached PR data. - enqueueGitHubPRRefresh(activeWorktreeId, 'active', 80) - - if (!pr) { - setChecks([]) - setComments([]) - return - } - - const refreshedChecks = fetchPRChecks(repo.path, pr.number, branch, pr.headSha, { - force: true - }).then( - (result) => { - setChecks(result) - const signature = JSON.stringify(result.map((c) => `${c.name}:${c.status}:${c.conclusion}`)) - pollIntervalRef.current = - signature === prevChecksRef.current - ? Math.min(pollIntervalRef.current * 2, 120_000) - : 30_000 - prevChecksRef.current = signature - }, - (err) => { - console.warn('Failed to fetch PR checks:', err) - setChecks([]) + const handleEntryRefresh = useCallback( + (options: { refreshChecks: boolean; refreshComments: boolean }) => { + if (!repo || !branch || !activeWorktreeId) { + return } - ) - setChecksLoading(true) - const refreshedComments = fetchComments({ force: true, prNumberOverride: pr.number }) - await Promise.all([refreshedChecks.finally(() => setChecksLoading(false)), refreshedComments]) - }, [repo, branch, activeWorktreeId, enqueueGitHubPRRefresh, pr, fetchPRChecks, fetchComments]) + // 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 @@ -371,23 +357,29 @@ 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 handleEntryRefresh() + handleEntryRefresh({ refreshChecks, refreshComments }) }, [entryKey, prFetchedAt, checksFetchedAt, commentsFetchedAt, prNumber, handleEntryRefresh]) const handleStartEdit = useCallback(() => { @@ -505,21 +497,37 @@ 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 isCheckingPR = isQueuedPRRefresh || isInFlightPRRefresh + const isPausedPRRefresh = prRefreshState?.status === 'paused' return (
- {operationInProgress ? `${operationLabel} in progress` : 'No pull request found'} + {operationInProgress + ? `${operationLabel} in progress` + : isQueuedPRRefresh + ? 'Refresh queued' + : isInFlightPRRefresh + ? 'Checking for pull request' + : 'No pull request found'}
{operationInProgress ? 'PR checks will be available after the operation completes' - : 'Push your branch and open a PR to see checks here'} + : isQueuedPRRefresh + ? 'GitHub status will refresh shortly' + : isInFlightPRRefresh + ? 'Refreshing GitHub status for this branch' + : isPausedPRRefresh + ? 'GitHub refresh is paused by the current rate-limit budget' + : 'Push your branch and open a PR to see checks here'}
{!operationInProgress && ( )}
@@ -616,7 +630,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/sidebar/WorktreeCard.tsx b/src/renderer/src/components/sidebar/WorktreeCard.tsx index b16e7f2cd93..02494227812 100644 --- a/src/renderer/src/components/sidebar/WorktreeCard.tsx +++ b/src/renderer/src/components/sidebar/WorktreeCard.tsx @@ -7,9 +7,9 @@ import { Tooltip, TooltipTrigger, TooltipContent } from '@/components/ui/tooltip import { AlertTriangle, Bell, + Clock3, GitMerge, LoaderCircle, - PauseCircle, CircleCheck, CircleX, Server, @@ -286,6 +286,7 @@ const WorktreeCard = React.memo(function WorktreeCard({ ) const showIssue = cardProps.includes('issue') + const showPRMeta = cardProps.includes('pr') && Boolean(pr) // Same rationale for issues: once that section is hidden, polling only burns // GitHub calls and keeps stale-but-invisible data warm for no user benefit. @@ -522,21 +523,24 @@ const WorktreeCard = React.memo(function WorktreeCard({ - {prRefreshState?.status === 'in-flight' && ( + {!showPRMeta && prRefreshState?.status === 'queued' && ( + + )} + {!showPRMeta && prRefreshState?.status === 'in-flight' && ( )} - {prRefreshState?.status === 'paused' && ( - - )} - {!['in-flight', 'paused'].includes(prRefreshState?.status ?? '') && + {(showPRMeta || + !['queued', 'in-flight'].includes(prRefreshState?.status ?? '')) && pr.checksStatus === 'success' && ( )} - {!['in-flight', 'paused'].includes(prRefreshState?.status ?? '') && + {(showPRMeta || + !['queued', 'in-flight'].includes(prRefreshState?.status ?? '')) && pr.checksStatus === 'failure' && ( )} - {!['in-flight', 'paused'].includes(prRefreshState?.status ?? '') && + {(showPRMeta || + !['queued', 'in-flight'].includes(prRefreshState?.status ?? '')) && pr.checksStatus === 'pending' && ( )} @@ -544,11 +548,13 @@ const WorktreeCard = React.memo(function WorktreeCard({ - {prRefreshState?.status === 'in-flight' - ? 'Refreshing CI checks' - : prRefreshState?.status === 'paused' - ? 'CI refresh paused' - : `CI checks ${checksLabel(pr.checksStatus).toLowerCase()}`} + {!showPRMeta && prRefreshState?.status === 'queued' + ? 'CI refresh queued' + : !showPRMeta && prRefreshState?.status === 'in-flight' + ? 'Refreshing CI checks' + : prRefreshState?.status === 'paused' + ? `CI checks ${checksLabel(pr.checksStatus).toLowerCase()} (refresh paused)` + : `CI checks ${checksLabel(pr.checksStatus).toLowerCase()}`} diff --git a/src/renderer/src/store/slices/github.ts b/src/renderer/src/store/slices/github.ts index 8301f50867a..dd78cf18cd2 100644 --- a/src/renderer/src/store/slices/github.ts +++ b/src/renderer/src/store/slices/github.ts @@ -1283,7 +1283,8 @@ export const createGitHubSlice: StateCreator = (s linkedPRNumber, cachedFetchedAt: cached?.fetchedAt ?? null } - const outcome = window.api.gh.refreshPRNow + const usesRefreshCoordinator = Boolean(window.api.gh.refreshPRNow) + const outcome = usesRefreshCoordinator ? await window.api.gh.refreshPRNow({ candidate }) : await window.api.gh .prForBranch({ repoPath, branch, linkedPRNumber }) @@ -1294,7 +1295,7 @@ export const createGitHubSlice: StateCreator = (s ) const pr: PRInfo | null = outcome.kind === 'found' ? outcome.pr : outcome.kind === 'no-pr' ? null : null - if (prRequestGenerations.get(cacheKey) === generation) { + if (!usesRefreshCoordinator && prRequestGenerations.get(cacheKey) === generation) { if (outcome.kind !== 'upstream-error') { set((s) => ({ prCache: { ...s.prCache, [cacheKey]: { data: pr, fetchedAt: outcome.fetchedAt } } diff --git a/src/shared/types.ts b/src/shared/types.ts index 27c694ae1fd..df41a516214 100644 --- a/src/shared/types.ts +++ b/src/shared/types.ts @@ -537,7 +537,14 @@ export type GitHubPRRefreshEvent = { outcome?: PRRefreshOutcome status?: 'queued' | 'in-flight' | 'paused' | 'skipped' pausedUntil?: number - skippedReason?: 'fresh' | 'not-git' | 'bare' | 'archived' | 'disconnected' | 'rate-limit' + skippedReason?: + | 'fresh' + | 'not-git' + | 'bare' + | 'archived' + | 'disconnected' + | 'remote' + | 'rate-limit' } export type PRCheckDetail = {