From 1adc446bcdabf345600d11bbd62b4b15d3631833 Mon Sep 17 00:00:00 2001 From: brennanb2025 Date: Fri, 26 Jun 2026 20:37:49 -0700 Subject: [PATCH] Tighten PR refresh perf accounting Co-authored-by: Orca --- .../github/pr-refresh-coordinator.test.ts | 56 ++++++++++++++++--- src/main/github/pr-refresh-coordinator.ts | 14 +++-- src/renderer/src/store/slices/github.test.ts | 17 +++++- src/renderer/src/store/slices/github.ts | 4 ++ 4 files changed, 76 insertions(+), 15 deletions(-) diff --git a/src/main/github/pr-refresh-coordinator.test.ts b/src/main/github/pr-refresh-coordinator.test.ts index d1798a494fa..ce55cd317d1 100644 --- a/src/main/github/pr-refresh-coordinator.test.ts +++ b/src/main/github/pr-refresh-coordinator.test.ts @@ -3,14 +3,19 @@ request timestamps, and follow-up scheduling against shared module state. */ 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() - }) -) +const { + sendMock, + getAllWebContentsMock, + getPRForBranchOutcomeMock, + getRateLimitMock, + noteRateLimitSpendMock +} = vi.hoisted(() => ({ + sendMock: vi.fn(), + getAllWebContentsMock: vi.fn(), + getPRForBranchOutcomeMock: vi.fn(), + getRateLimitMock: vi.fn(), + noteRateLimitSpendMock: vi.fn() +})) vi.mock('electron', () => ({ webContents: { @@ -24,7 +29,7 @@ vi.mock('./client', () => ({ vi.mock('./rate-limit', () => ({ getRateLimit: getRateLimitMock, - noteRateLimitSpend: vi.fn(), + noteRateLimitSpend: noteRateLimitSpendMock, rateLimitGuard: vi.fn(() => ({ blocked: false })) })) @@ -80,6 +85,7 @@ describe('pr-refresh-coordinator', () => { getAllWebContentsMock.mockReset() getPRForBranchOutcomeMock.mockReset() getRateLimitMock.mockReset() + noteRateLimitSpendMock.mockReset() getAllWebContentsMock.mockReturnValue([ { id: 1, @@ -770,6 +776,38 @@ describe('pr-refresh-coordinator', () => { ) }) + it('refreshes empty-branch candidates when a worktree HEAD hint exists', async () => { + const { refreshPRNow } = await import('./pr-refresh-coordinator') + getPRForBranchOutcomeMock.mockResolvedValueOnce({ + kind: 'no-pr', + fetchedAt: Date.now() + }) + + await refreshPRNow(makeCandidate({ branch: '', worktreeHead: 'abcdef1' })) + + expect(getPRForBranchOutcomeMock).toHaveBeenCalledWith('/repo', '', null, null, null, { + currentHeadOid: 'abcdef1' + }) + }) + + it('accounts for the extra core probe on background PR refreshes', async () => { + const { reportVisiblePRRefreshCandidates } = await import('./pr-refresh-coordinator') + getPRForBranchOutcomeMock.mockResolvedValueOnce({ + kind: 'no-pr', + fetchedAt: Date.now() + }) + + reportVisiblePRRefreshCandidates([makeCandidate({ worktreeHead: 'abcdef1' })], 1, 1) + await vi.runOnlyPendingTimersAsync() + await vi.advanceTimersByTimeAsync(10_000) + + expect(noteRateLimitSpendMock.mock.calls.map(([bucket]) => bucket)).toEqual([ + 'core', + 'core', + 'graphql' + ]) + }) + it('preserves coalesced aliases across visible follow-up refreshes', async () => { const { reportVisiblePRRefreshCandidates } = await import('./pr-refresh-coordinator') getPRForBranchOutcomeMock diff --git a/src/main/github/pr-refresh-coordinator.ts b/src/main/github/pr-refresh-coordinator.ts index 34a9d18ce4a..b9f60046db7 100644 --- a/src/main/github/pr-refresh-coordinator.ts +++ b/src/main/github/pr-refresh-coordinator.ts @@ -236,7 +236,12 @@ function validateCandidate( if (candidate.connectionId && candidate.connectionState === 'disconnected') { return 'disconnected' } - if (!candidate.branch && typeof candidate.linkedPRNumber !== 'number') { + if ( + !candidate.branch && + typeof candidate.linkedPRNumber !== 'number' && + typeof candidate.fallbackPRNumber !== 'number' && + !candidate.worktreeHead + ) { return 'fresh' } return null @@ -463,10 +468,9 @@ function isMergeabilityPendingOutcome(outcome: PRRefreshOutcome): boolean { } 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'] + // Why: branch misses can now spend core on both branch and merge-commit REST + // probes; keep the coarse queue budget conservative until lookup cost is exact. + return ['core', 'core', 'graphql'] } function noteBackgroundStart(): void { diff --git a/src/renderer/src/store/slices/github.test.ts b/src/renderer/src/store/slices/github.test.ts index c6d5025e5dc..ff7ae5cfd16 100644 --- a/src/renderer/src/store/slices/github.test.ts +++ b/src/renderer/src/store/slices/github.test.ts @@ -1712,6 +1712,17 @@ describe('createGitHubSlice.fetchPRForBranch', () => { connectionId: 'ssh-1' } ], + worktreesByRepo: { + 'repo-1': [ + makePRRefreshWorktree({ + id: 'wt-direct-refresh-head', + repoId: 'repo-1', + path: `${repoPath}/worktrees/direct-refresh`, + branch, + head: 'direct-head-oid' + }) + ] + }, prCache: { [`repo-1::${branch}`]: { data: pr, @@ -1726,7 +1737,10 @@ describe('createGitHubSlice.fetchPRForBranch', () => { }) await expect( - store.getState().fetchPRForBranch(repoPath, branch, { force: true }) + store.getState().fetchPRForBranch(repoPath, branch, { + force: true, + worktreeId: 'wt-direct-refresh-head' + }) ).resolves.toMatchObject({ number: 44 }) expect(mockApi.gh.prForBranch).not.toHaveBeenCalled() expect(mockApi.gh.refreshPRNow).toHaveBeenCalledWith({ @@ -1735,6 +1749,7 @@ describe('createGitHubSlice.fetchPRForBranch', () => { repoPath, branch, cacheKey: `ssh:ssh-1::repo-1::${branch}`, + worktreeHead: 'direct-head-oid', connectionId: 'ssh-1' }) }) diff --git a/src/renderer/src/store/slices/github.ts b/src/renderer/src/store/slices/github.ts index a239db8a26f..fbd270f20aa 100644 --- a/src/renderer/src/store/slices/github.ts +++ b/src/renderer/src/store/slices/github.ts @@ -2957,6 +2957,9 @@ export const createGitHubSlice: StateCreator = (s : ({ kind: 'no-pr', fetchedAt: Date.now() } as const) ) : await (async () => { + const requestWorktree = options?.worktreeId + ? findWorktreeById(get(), options.worktreeId) + : null const candidate: GitHubPRRefreshCandidate = { repoId: repoId ?? '', repoPath, @@ -2964,6 +2967,7 @@ export const createGitHubSlice: StateCreator = (s branch, cacheKey, worktreeId: options?.worktreeId, + worktreeHead: requestWorktree?.head ?? null, linkedPRNumber, fallbackPRNumber, fallbackPRSource,