From f0094eb70eeb191ed35d7e6561540cf96a83bc30 Mon Sep 17 00:00:00 2001 From: Jinwoo-H Date: Tue, 12 May 2026 16:10:25 -0400 Subject: [PATCH] Fix stuck PR refresh queued indicator Co-authored-by: Orca --- .../github/pr-refresh-coordinator.test.ts | 129 ++++++++++++++++++ src/main/github/pr-refresh-coordinator.ts | 29 +++- 2 files changed, 153 insertions(+), 5 deletions(-) create mode 100644 src/main/github/pr-refresh-coordinator.test.ts 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 00000000000..72e86a6d66c --- /dev/null +++ b/src/main/github/pr-refresh-coordinator.test.ts @@ -0,0 +1,129 @@ +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 + } +} + +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 a delayed visible follow-up as queued after a successful refresh', 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(1) + expect(queuedEvents[0].reason).toBe('visible') + 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) + }) +}) diff --git a/src/main/github/pr-refresh-coordinator.ts b/src/main/github/pr-refresh-coordinator.ts index 94b98099a5e..e5bc9a3b059 100644 --- a/src/main/github/pr-refresh-coordinator.ts +++ b/src/main/github/pr-refresh-coordinator.ts @@ -84,10 +84,18 @@ 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 ): GitHubPRRefreshEvent['skippedReason'] | null { @@ -113,12 +121,16 @@ function shouldSkipFresh( candidate: GitHubPRRefreshCandidate, reason: GitHubPRRefreshReason ): boolean { - if (isManual(reason) || candidate.cachedFetchedAt == null) { + if (bypassesFreshnessDelay(reason) || candidate.cachedFetchedAt == null) { return false } return Date.now() - candidate.cachedFetchedAt < refreshIntervalForCandidate(candidate) } +function shouldBroadcastQueued(dueAt: number): boolean { + return dueAt - Date.now() <= 5_000 +} + function freshRetryAt(candidate: GitHubPRRefreshCandidate): number | null { return candidate.cachedFetchedAt == null ? null @@ -166,7 +178,8 @@ function scheduleVisibleFollowUp( dueAt: retryAt, windowId }) - broadcast({ aliases, reason: 'visible', status: 'queued' }) + // 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 } @@ -257,7 +270,7 @@ async function drainQueue(): Promise { return } - const budgetDelay = isBackground(next.reason) ? nextBudgetDelay() : 0 + const budgetDelay = isBudgetedBackground(next.reason) ? nextBudgetDelay() : 0 if (budgetDelay > 0) { scheduleDrain(budgetDelay) return @@ -312,7 +325,9 @@ async function drainQueue(): Promise { scheduleDrain(Math.max(1_000, retryAt - Date.now())) continue } - noteBackgroundStart() + if (isBudgetedBackground(next.reason)) { + noteBackgroundStart() + } noteRateLimitSpend('graphql') noteRateLimitSpend('core') } @@ -384,7 +399,11 @@ export function enqueuePRRefresh( windowId }) } - broadcast({ aliases: [alias], reason, status: 'queued' }) + // Why: freshness follow-ups can be scheduled minutes out. Only immediate + // queueing should surface in the card UI as pending work. + if (shouldBroadcastQueued(dueAt)) { + broadcast({ aliases: [alias], reason, status: 'queued' }) + } scheduleDrain() }