Refresh visible reviews automatically and stop settled merged polling (#25788)

Refresh only actual on-screen review cards and the selected visible review panel, with a single metadata owner per execution host. Open reviews refresh every 60 seconds when selected and 120 seconds otherwise. Settled merged reviews stop automatic refresh; pending merged checks continue, hidden rows stop, and work changes or stale re-exposure discover fresh state.

Remove the polling setting. Preserve provider/SSH/runtime ownership, mixed-version cache behavior, failure backoff, bounded foreground admission, request coalescing, and mutation invalidation. Cover older persisted caches and unknown HEADs.

Validated with 1,628 focused tests, typechecking, lint/code-quality gates, hidden Electron viewport checks, and parallel Codex/Claude Opus 5.5 adversarial reviews. Measurements and policy details are in PR #25788.

Fixes #25746

Co-authored-by: ggbdpq <ggbdpq@gmail.com>
This commit is contained in:
Neil
2026-10-06 00:53:49 -07:00
committed by GitHub
co-authored by ggbdpq
parent 9905765e3b
commit a2f197fde7
100 changed files with 5190 additions and 1795 deletions
+23
View File
@@ -208,6 +208,29 @@ function expectGraphQLRollupCall(callIndex = 1, noCache = false): void {
}
describe('getPRChecks', () => {
it('shares concurrent checks reads and honors explicit uncached refreshes', async () => {
vi.clearAllMocks()
getOwnerRepoMock.mockResolvedValue({ owner: 'acme', repo: 'widgets' })
acquireMock.mockResolvedValue(undefined)
const response = graphQLChecksResponse()
let finish: ((value: typeof response) => void) | undefined
ghExecFileAsyncMock.mockImplementation(
() =>
new Promise<typeof response>((resolve) => {
finish = resolve
})
)
const first = getPRChecks('/repo', 12)
const second = getPRChecks('/repo', 12)
await vi.waitFor(() => expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(1))
finish?.(response)
await expect(Promise.all([first, second])).resolves.toEqual([[], []])
ghExecFileAsyncMock.mockResolvedValue(response)
await getPRChecks('/repo', 12, undefined, undefined, { noCache: true })
expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(2)
expect(ghExecFileAsyncMock.mock.calls[1][0]).not.toContain('--cache')
})
beforeEach(() => {
execFileAsyncMock.mockReset()
ghExecFileAsyncMock.mockReset()
@@ -0,0 +1,196 @@
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
import type { PRRefreshOutcome } from '../../shared/github/pull-request-refresh-types'
import type { GitHubRepoContext, LocalGitExecOptions } from './github-repository-identity'
import { getRepoExecutionHostId } from '../../shared/execution-host'
import { hostedReviewInfoFromGitHubPRInfo } from '../../shared/hosted-review-github'
import { makePR } from './pr-refresh-coordinator-test-harness'
import {
__resetHostedReviewBranchCacheForTests,
invalidateHostedReviewBranchCache,
withHostedReviewBranchCache
} from '../source-control/hosted-review-branch-cache'
const mocks = vi.hoisted(() => ({ resolve: vi.fn(), acquire: vi.fn(), release: vi.fn() }))
vi.mock('./gh-utils', () => ({
acquire: mocks.acquire,
release: mocks.release,
githubRepoContext: (
repoPath: string,
connectionId: string | null,
options: LocalGitExecOptions
) => ({
repoPath,
connectionId,
...options
}),
ghRepoExecOptions: (context: GitHubRepoContext) => context
}))
vi.mock('./client/lookup/branch-lookup-resolution', () => ({
resolvePRForBranchOutcome: mocks.resolve
}))
vi.mock('../providers/ssh-git-dispatch', () => ({ getSshGitProviderGeneration: () => 1 }))
import { getPRForBranchOutcome } from './client/lookup/pr-for-branch-outcome'
const outcome: PRRefreshOutcome = { kind: 'no-pr', fetchedAt: 1 }
beforeEach(() => {
vi.clearAllMocks()
__resetHostedReviewBranchCacheForTests()
mocks.acquire.mockResolvedValue(undefined)
})
afterEach(() => {
vi.unstubAllEnvs()
})
describe('PR lookup coalescing', () => {
it('shares pending reads across callers and releases them after settlement', async () => {
let finish: ((value: PRRefreshOutcome) => void) | undefined
mocks.resolve.mockImplementation(
() =>
new Promise<PRRefreshOutcome>((resolve) => {
finish = resolve
})
)
const first = getPRForBranchOutcome('/repo', 'refs/heads/topic')
const second = getPRForBranchOutcome('/repo', 'topic')
await vi.waitFor(() => expect(mocks.resolve).toHaveBeenCalledTimes(1))
finish?.(outcome)
expect(await Promise.all([first, second])).toEqual([outcome, outcome])
mocks.resolve.mockResolvedValue(outcome)
await getPRForBranchOutcome('/repo', 'topic')
expect(mocks.resolve).toHaveBeenCalledTimes(2)
})
it('shares a background read with a foreground caller of the same lookup', async () => {
mocks.resolve.mockResolvedValue(outcome)
await Promise.all([
getPRForBranchOutcome('/repo', 'topic', null, null, null, {
localGitExecOptions: { admissionTier: 'background' }
}),
getPRForBranchOutcome('/repo', 'topic', null, null, null, {
localGitExecOptions: { admissionTier: 'interactive' }
})
])
expect(mocks.resolve).toHaveBeenCalledTimes(1)
})
it('isolates heads, fallback hints, accounts, execution hosts, and credentials', async () => {
mocks.resolve.mockResolvedValue(outcome)
const requests = [
getPRForBranchOutcome('/repo', 'topic'),
getPRForBranchOutcome('/repo', 'topic', 12),
getPRForBranchOutcome('/repo', 'topic', null, 'ssh-1'),
getPRForBranchOutcome('/repo', 'topic', null, null, 12),
getPRForBranchOutcome('/repo', 'topic', null, null, null, { currentHeadOid: 'other-head' }),
getPRForBranchOutcome('/repo', 'topic', null, null, null, {
localGitExecOptions: { wslDistro: 'Ubuntu' }
}),
getPRForBranchOutcome('/repo', 'topic', null, null, null, {
localGitExecOptions: { ghAccount: { host: 'github.com', user: 'other' } }
})
]
vi.stubEnv('GH_TOKEN', 'test-only-other-token')
requests.push(getPRForBranchOutcome('/repo', 'topic'))
await Promise.all(requests)
expect(mocks.resolve).toHaveBeenCalledTimes(8)
})
it('cleans up errors so later reads can recover', async () => {
mocks.resolve.mockRejectedValueOnce(new Error('network down')).mockResolvedValue(outcome)
const results = await Promise.all([
getPRForBranchOutcome('/repo', 'topic'),
getPRForBranchOutcome('/repo', 'topic')
])
expect(results.every((result) => result.kind === 'upstream-error')).toBe(true)
expect(mocks.resolve).toHaveBeenCalledTimes(1)
await expect(getPRForBranchOutcome('/repo', 'topic')).resolves.toEqual(outcome)
expect(mocks.resolve).toHaveBeenCalledTimes(2)
})
it.each([
{ label: 'native', connectionId: null, options: {} },
{ label: 'WSL', connectionId: null, options: { localGitExecOptions: { wslDistro: 'Ubuntu' } } },
{ label: 'SSH', connectionId: 'host / encoded', options: {} }
])(
'does not adopt a pre-creation no-PR read after $label invalidation',
async ({ connectionId, options }) => {
const created: PRRefreshOutcome = { kind: 'found', pr: makePR({ number: 13 }), fetchedAt: 2 }
const createdReview = hostedReviewInfoFromGitHubPRInfo(created.pr)
let finish: (value: PRRefreshOutcome) => void = () => {}
mocks.resolve
.mockImplementationOnce(
() =>
new Promise<PRRefreshOutcome>((resolve) => {
finish = resolve
})
)
.mockResolvedValue(created)
const executionHostId = getRepoExecutionHostId({ connectionId })
const identity = { repoPath: '/repo', executionHostId, branch: 'topic', ...options }
const lookup = async () => {
const result = await getPRForBranchOutcome(
'/repo',
'topic',
null,
connectionId,
null,
options
)
if (result.kind === 'upstream-error') {
throw new Error(result.message)
}
return result.kind === 'found' ? hostedReviewInfoFromGitHubPRInfo(result.pr) : null
}
const beforeCreation = withHostedReviewBranchCache(identity, { headOid: null }, lookup)
await vi.waitFor(() => expect(mocks.resolve).toHaveBeenCalledTimes(1))
invalidateHostedReviewBranchCache('/repo', executionHostId)
const afterCreation = withHostedReviewBranchCache(identity, { headOid: null }, lookup)
try {
await vi.waitFor(() => expect(mocks.resolve).toHaveBeenCalledTimes(2))
await expect(afterCreation).resolves.toEqual(createdReview)
} finally {
finish(outcome)
await Promise.all([beforeCreation, afterCreation])
}
await expect(
withHostedReviewBranchCache(identity, { headOid: null }, lookup)
).resolves.toEqual(createdReview)
expect(mocks.resolve).toHaveBeenCalledTimes(2)
}
)
it('keeps native, WSL, and SSH pending reads scoped during invalidation', async () => {
const completions: ((value: PRRefreshOutcome) => void)[] = []
mocks.resolve.mockImplementation(
() =>
new Promise<PRRefreshOutcome>((resolve) => {
completions.push(resolve)
})
)
const native = () => getPRForBranchOutcome('/repo', 'topic')
const wsl = () =>
getPRForBranchOutcome('/repo', 'topic', null, null, null, {
localGitExecOptions: { wslDistro: 'Ubuntu' }
})
const ssh = () => getPRForBranchOutcome('/repo', 'topic', null, 'host / encoded')
const reads = [native(), wsl(), ssh()]
try {
await vi.waitFor(() => expect(mocks.resolve).toHaveBeenCalledTimes(3))
invalidateHostedReviewBranchCache('/other', 'local')
reads.push(native(), wsl(), ssh())
invalidateHostedReviewBranchCache(
'/repo',
getRepoExecutionHostId({ connectionId: 'host / encoded' })
)
reads.push(native(), wsl(), ssh())
await vi.waitFor(() => expect(mocks.resolve).toHaveBeenCalledTimes(4))
invalidateHostedReviewBranchCache('/repo', 'local')
reads.push(native(), wsl(), ssh())
await vi.waitFor(() => expect(mocks.resolve).toHaveBeenCalledTimes(6))
} finally {
for (const finish of completions) {
finish(outcome)
}
await Promise.all(reads)
}
})
})
+30 -1
View File
@@ -1,3 +1,6 @@
import { runCoalescedProbe, type CoalescedProbes } from '../../../git/coalesced-probe'
import { getSshGitProviderGeneration } from '../../../providers/ssh-git-dispatch'
import { githubReadExecutionScope } from '../../github-read-execution-scope'
import type { PRCheckDetail } from '../../../../shared/github/check-types'
import { GITHUB_WORK_ITEMS_SSH_REMOTE_REQUIRED_MESSAGE } from '../../../../shared/work-items'
import { ghExecFileAsync, acquire, release, type LocalGitExecOptions } from '../../gh-utils'
@@ -120,7 +123,7 @@ export async function getPRChecksViaRestFallback(
* Uses GitHub's combined GraphQL rollup so check runs and legacy commit statuses
* arrive in one cached request; suite-only approval blockers are included too.
*/
export async function getPRChecks(
async function readPRChecks(
repoPath: string,
prNumber: number,
headSha?: string,
@@ -231,3 +234,29 @@ export async function getPRChecks(
throw err
}
}
const checksReads: CoalescedProbes<PRCheckDetail[]> = new Map()
export function getPRChecks(
repoPath: string,
prNumber: number,
headSha?: string,
prRepo?: GitHubApiRepository | null,
options?: { noCache?: boolean },
connectionId?: string | null,
localGitOptions: LocalGitExecOptions = {}
): Promise<PRCheckDetail[]> {
const key = JSON.stringify([
repoPath,
prNumber,
headSha ?? null,
prRepo ?? null,
Boolean(options?.noCache),
connectionId ?? null,
connectionId ? getSshGitProviderGeneration(connectionId) : null,
githubReadExecutionScope(localGitOptions)
])
return runCoalescedProbe(checksReads, key, () =>
readPRChecks(repoPath, prNumber, headSha, prRepo, options, connectionId, localGitOptions)
)
}
@@ -1,5 +1,6 @@
import { z } from 'zod'
import { createHash } from 'node:crypto'
import { githubReadExecutionScope as workItemSearchScope } from '../../github-read-execution-scope'
export { githubReadExecutionScope as workItemSearchScope } from '../../github-read-execution-scope'
import { BoundedMap } from '../../../../shared/bounded-map'
import { runCoalescedProbe, type CoalescedProbes } from '../../../git/coalesced-probe'
import { createGhRateLimitBlockedError } from '../../../git/gh-rate-limit-breaker'
@@ -48,22 +49,6 @@ const responses = new BoundedMap<string, { at: number; value: unknown }>({
sizeOf: (value, key) => Buffer.byteLength(key) + Buffer.byteLength(JSON.stringify(value))
})
export function workItemSearchScope(
options: GitHubRepoExecOptions,
environment: NodeJS.ProcessEnv = options.env ?? process.env
): string {
// gh wrappers and credential selection can depend on cwd and the inherited environment.
return createHash('sha256')
.update(
JSON.stringify([
options,
process.cwd(),
Object.entries(environment).sort(([a], [b]) => a.localeCompare(b))
])
)
.digest('hex')
}
export function requestWorkItemSearch<T>(request: SearchRequest): Promise<SearchResponse<T>> {
const environment = { ...(request.environment ?? request.options.env ?? process.env) }
const scope = workItemSearchScope(request.options, environment)
@@ -1,9 +1,19 @@
import { getRepoExecutionHostId } from '../../../../shared/execution-host'
import {
hostedReviewRepoScope,
scopeGeneration
} from '../../../source-control/hosted-review-scope-generations'
import { runCoalescedProbe, type CoalescedProbes } from '../../../git/coalesced-probe'
import { getSshGitProviderGeneration } from '../../../providers/ssh-git-dispatch'
import { githubReadExecutionScope } from '../../github-read-execution-scope'
import type { PRRefreshOutcome } from '../../../../shared/github/pull-request-refresh-types'
import { acquire, release, ghRepoExecOptions, githubRepoContext } from '../../gh-utils'
import { hostedReviewLocalGitOptionArgs, githubPRStackExecutionScope } from './../github-exec-scope'
import type { GitHubPRBranchLookupOptions } from './pull-request-lookup-data'
import { prRefreshUpstreamError } from './../gh-error-predicates'
import { resolvePRForBranchOutcome } from './branch-lookup-resolution'
const reads: CoalescedProbes<PRRefreshOutcome> = new Map()
export async function getPRForBranchOutcome(
repoPath: string,
branch: string,
@@ -23,22 +33,41 @@ export async function getPRForBranchOutcome(
const ghOptions = ghRepoExecOptions(context)
const executionScope = githubPRStackExecutionScope(connectionId, localGitOptions)
await acquire()
try {
return await resolvePRForBranchOutcome({
repoPath,
branchName,
linkedPRNumber,
connectionId,
fallbackPRNumber,
options,
localGitOptions,
ghOptions,
executionScope
})
} catch (err) {
return prRefreshUpstreamError(err)
} finally {
release()
}
const key = JSON.stringify([
executionScope,
connectionId ? getSshGitProviderGeneration(connectionId) : null,
repoPath,
scopeGeneration(hostedReviewRepoScope(repoPath, getRepoExecutionHostId({ connectionId }))),
branchName,
linkedPRNumber ?? null,
fallbackPRNumber ?? null,
options.acceptMergedFallbackPR ?? false,
options.currentHeadOid ?? null,
githubReadExecutionScope(ghOptions)
])
return runCoalescedProbe(
reads,
key,
async () => {
await acquire()
try {
return await resolvePRForBranchOutcome({
repoPath,
branchName,
linkedPRNumber,
connectionId,
fallbackPRNumber,
options,
localGitOptions,
ghOptions,
executionScope
})
} catch (err) {
return prRefreshUpstreamError(err)
} finally {
release()
}
},
2 * 60_000
)
}
+7 -1
View File
@@ -1,3 +1,4 @@
import { invalidateReviewLookupsAfterPRMutation } from '../../pr-mutation-review-invalidation'
import type { PRConflictSummary } from '../../../../shared/github/pull-request-types'
import { getPRConflictSummary } from '../../conflict-summary'
import { ghExecFileAsync, acquire, release, type LocalGitExecOptions } from '../../gh-utils'
@@ -59,7 +60,7 @@ export async function mergePR(
)
release()
concurrencySlotHeld = false
return await mergeGitHubPRStack({
const result = await mergeGitHubPRStack({
repository: ownerRepo,
prNumber,
method,
@@ -67,6 +68,10 @@ export async function mergePR(
headSha: restData.headRefOid,
ghOptions
})
if (result.ok) {
invalidateReviewLookupsAfterPRMutation(repoPath, connectionId)
}
return result
}
const mergeBlocker = await getPRMergeBlocker(
repoPath,
@@ -89,6 +94,7 @@ export async function mergePR(
...ghOptions,
env: { ...process.env, GH_PROMPT_DISABLED: '1' }
})
invalidateReviewLookupsAfterPRMutation(repoPath, connectionId)
return { ok: true }
} catch (err) {
const message =
@@ -1,3 +1,4 @@
import { invalidateReviewLookupsAfterPRMutation } from '../../pr-mutation-review-invalidation'
import type { GitHubPRMergeMethod } from '../../../../shared/github/pull-request-types'
import {
ghExecFileAsync,
@@ -165,13 +166,17 @@ export async function setPRAutoMerge(
await acquire()
try {
if (enabled) {
return await enablePRAutoMerge(
const result = await enablePRAutoMerge(
prNumber,
method,
ownerRepo,
ghOptions,
githubPRStackExecutionScope(connectionId, localGitOptions)
)
if (result.ok) {
invalidateReviewLookupsAfterPRMutation(repoPath, connectionId)
}
return result
}
const args = ['pr', 'merge', String(prNumber), '--disable-auto']
if (ownerRepo) {
@@ -181,6 +186,7 @@ export async function setPRAutoMerge(
...ghOptions,
env: { ...process.env, GH_PROMPT_DISABLED: '1' }
})
invalidateReviewLookupsAfterPRMutation(repoPath, connectionId)
return { ok: true }
} catch (err) {
const message =
@@ -1,3 +1,4 @@
import { invalidateReviewLookupsAfterPRMutation } from '../../pr-mutation-review-invalidation'
import {
ghExecFileAsync,
acquire,
@@ -35,6 +36,7 @@ export async function updatePRTitle(
await ghExecFileAsync(args, {
...ghOptions
})
invalidateReviewLookupsAfterPRMutation(repoPath, connectionId)
return true
} catch (err) {
console.warn('updatePRTitle failed:', err)
@@ -89,6 +91,7 @@ export async function updatePRDetails(
],
ghOptions
)
invalidateReviewLookupsAfterPRMutation(repoPath, connectionId)
return { ok: true }
} catch (err) {
const message =
@@ -1,3 +1,4 @@
import { invalidateReviewLookupsAfterPRMutation } from '../../pr-mutation-review-invalidation'
import {
acquire,
classifyPullRequestUpdateError,
@@ -30,6 +31,7 @@ export async function markPRReadyForReview(
['pr', 'ready', String(prNumber), '--repo', `${ownerRepo.owner}/${ownerRepo.repo}`],
ghOptions
)
invalidateReviewLookupsAfterPRMutation(repoPath, connectionId)
return { ok: true }
} catch (err) {
const message =
@@ -1,3 +1,4 @@
import { invalidateReviewLookupsAfterPRMutation } from '../../pr-mutation-review-invalidation'
import { ghExecFileAsync, acquire, release, type LocalGitExecOptions } from '../../gh-utils'
import { resolveGitHubRepoExecution, type GitHubApiRepository } from '../../github-api-repository'
export async function requestPRReviewers(
@@ -31,6 +32,7 @@ export async function requestPRReviewers(
...ghOptions,
env: { ...process.env, GH_PROMPT_DISABLED: '1' }
})
invalidateReviewLookupsAfterPRMutation(repoPath, connectionId)
return { ok: true }
} catch (err) {
const message =
@@ -72,6 +74,7 @@ export async function removePRReviewers(
...ghOptions,
env: { ...process.env, GH_PROMPT_DISABLED: '1' }
})
invalidateReviewLookupsAfterPRMutation(repoPath, connectionId)
return { ok: true }
} catch (err) {
const message =
@@ -1,3 +1,4 @@
import { invalidateReviewLookupsAfterPRMutation } from '../../pr-mutation-review-invalidation'
import type { GitHubPullRequestStateUpdate } from '../../../../shared/issue-mutation-types'
import {
ghExecFileAsync,
@@ -35,6 +36,7 @@ export async function updatePRState(
...ghOptions
}
)
invalidateReviewLookupsAfterPRMutation(repoPath, connectionId)
return { ok: true }
} catch (err) {
const message =
@@ -330,6 +330,29 @@ describe('isGitHubHostAuthenticated', () => {
expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(1)
})
it('retains a known authenticated host for 15 minutes', async () => {
mockHostAuthenticated()
await isGitHubHostAuthenticated('github.acme-corp.com', '/repo')
await vi.advanceTimersByTimeAsync(14 * 60_000)
await isGitHubHostAuthenticated('github.acme-corp.com', '/repo')
expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(1)
await vi.advanceTimersByTimeAsync(60_000)
await isGitHubHostAuthenticated('github.acme-corp.com', '/repo')
expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(2)
})
it('rechecks host authentication after the credential environment changes', async () => {
mockHostAuthenticated()
await isGitHubHostAuthenticated('github.acme-corp.com', '/repo')
vi.stubEnv('GH_ENTERPRISE_TOKEN', 'test-only-changed-token')
try {
await isGitHubHostAuthenticated('github.acme-corp.com', '/repo')
expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(2)
} finally {
vi.unstubAllEnvs()
}
})
it('coalesces concurrent probes for the same runtime and host', async () => {
let finishProbe: (() => void) | undefined
ghExecFileAsyncMock.mockImplementation(
@@ -1,3 +1,4 @@
import { githubReadExecutionScope } from './github-read-execution-scope'
import { ghExecFileAsync } from '../git/runner'
import type { GitHubOwnerRepo } from '../../shared/github/pull-request-types'
import {
@@ -31,7 +32,8 @@ export type GitHubEnterpriseRepoSlug = GitHubOwnerRepo & { host: string }
// host `gh auth status` reports as logged-in is definitively a GitHub host. This
// mirrors the `glab auth status` signal GitLab self-hosted detection uses, so a
// GHES remote is not left to fall through to Gitea (#8312).
const HOST_AUTH_TTL_MS = 60_000
const HOST_AUTH_TTL_MS = 15 * 60_000
const HOST_AUTH_MISS_TTL_MS = 60_000
const HOST_AUTH_CACHE_MAX_ENTRIES = 512
type HostAuthCacheEntry = {
@@ -145,7 +147,7 @@ async function resolveAuthenticatedGitHubHost(
localGitOptions: LocalGitExecOptions = {}
): Promise<string | null | undefined> {
const normalizedHost = normalizeGitHubHost(host)?.authority ?? host.trim().toLowerCase()
const cacheKey = `${runtimeCacheKey(repoPath, connectionId, localGitOptions.wslDistro)}\0${normalizedHost}`
const cacheKey = `${runtimeCacheKey(repoPath, connectionId, localGitOptions.wslDistro)}\0${normalizedHost}\0${githubReadExecutionScope({ ghAccount: localGitOptions.ghAccount })}`
const now = Date.now()
pruneHostAuthCache(now)
const cached = hostAuthCache.get(cacheKey)
@@ -179,7 +181,7 @@ async function resolveAuthenticatedGitHubHost(
}
hostAuthCache.set(cacheKey, {
authenticatedHost,
expiresAt: Date.now() + HOST_AUTH_TTL_MS
expiresAt: Date.now() + (authenticatedHost ? HOST_AUTH_TTL_MS : HOST_AUTH_MISS_TTL_MS)
})
pruneHostAuthCache(Date.now())
return authenticatedHost
@@ -0,0 +1,19 @@
import { createHash } from 'node:crypto'
import type { GitHubRepoExecOptions } from './github-api-repository'
export function githubReadExecutionScope(
options: GitHubRepoExecOptions,
environment: NodeJS.ProcessEnv = options.env ?? process.env
): string {
const { admissionTier: _admissionTier, ...executionOptions } = options
// gh wrappers and credential selection can depend on cwd and the inherited environment.
return createHash('sha256')
.update(
JSON.stringify([
executionOptions,
process.cwd(),
Object.entries(environment).sort(([a], [b]) => a.localeCompare(b))
])
)
.digest('hex')
}
@@ -0,0 +1,166 @@
import { beforeEach, describe, expect, it, vi } from 'vitest'
import type { PRRefreshOutcome } from '../../shared/github/pull-request-refresh-types'
import type { Repo } from '../../shared/repo-types'
import type { GitHubRepoContext, LocalGitExecOptions } from './github-repository-identity'
import type { Store } from '../persistence'
import { __resetHostedReviewBranchCacheForTests } from '../source-control/hosted-review-branch-cache'
const mocks = vi.hoisted(() => ({
resolve: vi.fn(),
exec: vi.fn(),
handlers: new Map<string, (event: unknown, args: unknown) => Promise<unknown>>()
}))
vi.mock('electron', () => ({
ipcMain: {
handle: (channel: string, handler: (event: unknown, args: unknown) => Promise<unknown>) =>
mocks.handlers.set(channel, handler)
}
}))
vi.mock('./gh-utils', () => ({
acquire: vi.fn().mockResolvedValue(undefined),
release: vi.fn(),
ghExecFileAsync: mocks.exec,
classifyPullRequestUpdateError: (message: string) => ({ message }),
classifyGhError: (message: string) => ({ message }),
githubRepoContext: (
repoPath: string,
connectionId: string | null,
options: LocalGitExecOptions
) => ({ repoPath, connectionId, ...options }),
ghRepoExecOptions: (context: GitHubRepoContext) => context
}))
vi.mock('./github-api-repository', () => ({
resolveGitHubRepoExecution: vi.fn().mockResolvedValue({
ownerRepo: { owner: 'acme', repo: 'widgets' },
ghOptions: {}
})
}))
vi.mock('./client/lookup/pr-number-lookup', () => ({
getRestPRByNumber: vi.fn().mockResolvedValue({ stack: null }),
getPRByNumber: vi.fn().mockResolvedValue(null)
}))
vi.mock('./client/lookup/branch-lookup-resolution', () => ({
resolvePRForBranchOutcome: mocks.resolve
}))
vi.mock('../providers/ssh-git-dispatch', () => ({ getSshGitProviderGeneration: () => 1 }))
vi.mock('../ipc/github-work-item-mutation-events', () => ({
broadcastGitHubWorkItemMutation: vi.fn()
}))
vi.mock('../project-runtime-git-options', () => ({ getLocalProjectGhExecOptions: () => ({}) }))
import { getPRForBranchOutcome } from './client/lookup/pr-for-branch-outcome'
import { registerGitHubPRMutationHandlers } from '../ipc/github-pr-mutation-handlers'
import { RuntimeGitHubReviewMutationCommands } from '../runtime/runtime-github-review-mutation-commands'
const local: Repo = { id: 'local', path: '/repo', displayName: 'repo', badgeColor: '', addedAt: 0 }
const ssh: Repo = { ...local, id: 'ssh', connectionId: 'ssh-1' }
const otherSsh: Repo = { ...local, id: 'other-ssh', connectionId: 'ssh-2' }
// oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: handlers only call getRepos.
const store = { getRepos: () => [local, ssh, otherSsh] } as unknown as Store
registerGitHubPRMutationHandlers(store)
const runtime = new RuntimeGitHubReviewMutationCommands({
resolveRepo: async (selector) => [local, ssh, otherSsh].find((repo) => repo.id === selector)!,
getLocalGitArgs: () => []
})
function ipc(channel: string, repo: Repo, args: Record<string, unknown>): Promise<unknown> {
return mocks.handlers.get(channel)!(
{ sender: { id: 1 } },
{ repoPath: repo.path, repoId: repo.id, ...args }
)
}
const mutations: { label: string; run: (repo: Repo) => Promise<unknown> }[] = [
{ label: 'IPC merge', run: (repo) => ipc('gh:mergePR', repo, { prNumber: 7 }) },
{
label: 'IPC close',
run: (repo) => ipc('gh:updatePRState', repo, { prNumber: 7, updates: { state: 'closed' } })
},
{ label: 'IPC ready', run: (repo) => ipc('gh:markPRReadyForReview', repo, { prNumber: 7 }) },
{
label: 'IPC auto-merge',
run: (repo) => ipc('gh:setPRAutoMerge', repo, { prNumber: 7, enabled: false })
},
{ label: 'IPC title', run: (repo) => ipc('gh:updatePRTitle', repo, { prNumber: 7, title: 'T' }) },
{
label: 'IPC reviewers',
run: (repo) => ipc('gh:requestPRReviewers', repo, { prNumber: 7, reviewers: ['octo'] })
},
{ label: 'RPC merge', run: (repo) => runtime.mergeRepoPR(repo.id, 7) },
{
label: 'RPC reopen',
run: (repo) => runtime.updateRepoPRState(repo.id, 7, { state: 'open' })
},
{ label: 'RPC ready', run: (repo) => runtime.markRepoPRReadyForReview(repo.id, 7) },
{ label: 'RPC details', run: (repo) => runtime.updateRepoPRDetails(repo.id, 7, { body: 'B' }) },
{ label: 'RPC reviewers', run: (repo) => runtime.removeRepoPRReviewers(repo.id, 7, ['octo']) }
]
let finishHeld: (value: PRRefreshOutcome) => void = () => {}
const stale: PRRefreshOutcome = { kind: 'no-pr', fetchedAt: 1 }
const fresh: PRRefreshOutcome = { kind: 'no-pr', fetchedAt: 2 }
function holdLookup(repo: Repo): Promise<PRRefreshOutcome> {
mocks.resolve.mockImplementationOnce(
() =>
new Promise<PRRefreshOutcome>((resolve) => {
finishHeld = resolve
})
)
return getPRForBranchOutcome(repo.path, 'topic', null, repo.connectionId ?? null)
}
beforeEach(() => {
vi.clearAllMocks()
__resetHostedReviewBranchCacheForTests()
mocks.exec.mockResolvedValue({ stdout: '', stderr: '' })
mocks.resolve.mockResolvedValue(fresh)
})
describe('PR mutation review-lookup fencing', () => {
it.each(mutations)('$label starts a fresh lookup after success', async ({ run }) => {
for (const repo of [local, ssh]) {
mocks.resolve.mockClear()
const held = holdLookup(repo)
await vi.waitFor(() => expect(mocks.resolve).toHaveBeenCalledTimes(1))
expect([true, { ok: true }]).toContainEqual(await run(repo))
const after = getPRForBranchOutcome(repo.path, 'topic', null, repo.connectionId ?? null)
finishHeld(stale)
expect(await Promise.all([held, after])).toEqual([stale, fresh])
expect(mocks.resolve).toHaveBeenCalledTimes(2)
}
})
it.each(mutations)('$label keeps sharing the in-flight lookup after failure', async ({ run }) => {
mocks.exec.mockRejectedValue(new Error('denied'))
const held = holdLookup(local)
await vi.waitFor(() => expect(mocks.resolve).toHaveBeenCalledTimes(1))
await run(local)
const after = getPRForBranchOutcome(local.path, 'topic', null, null)
finishHeld(stale)
expect(await Promise.all([held, after])).toEqual([stale, stale])
expect(mocks.resolve).toHaveBeenCalledTimes(1)
})
it('fences only the host that ran the mutation', async () => {
const heldLocal = holdLookup(local)
await vi.waitFor(() => expect(mocks.resolve).toHaveBeenCalledTimes(1))
const finishLocal = finishHeld
const heldOther = holdLookup(otherSsh)
await vi.waitFor(() => expect(mocks.resolve).toHaveBeenCalledTimes(2))
const finishOther = finishHeld
await runtime.mergeRepoPR(ssh.id, 7)
await ipc('gh:mergePR', ssh, { prNumber: 7 })
const afterLocal = getPRForBranchOutcome(local.path, 'topic', null, null)
const afterOther = getPRForBranchOutcome(otherSsh.path, 'topic', null, 'ssh-2')
finishLocal(stale)
finishOther(stale)
expect(await Promise.all([heldLocal, afterLocal, heldOther, afterOther])).toEqual([
stale,
stale,
stale,
stale
])
expect(mocks.resolve).toHaveBeenCalledTimes(2)
})
})
@@ -0,0 +1,10 @@
import { getRepoExecutionHostId } from '../../shared/execution-host'
import { invalidateHostedReviewBranchCache } from '../source-control/hosted-review-branch-cache'
// A post-mutation refresh must not join an older lookup or reuse its cached answer.
export function invalidateReviewLookupsAfterPRMutation(
repoPath: string,
connectionId: string | null | undefined
): void {
invalidateHostedReviewBranchCache(repoPath, getRepoExecutionHostId({ connectionId }))
}
+44 -28
View File
@@ -1,3 +1,4 @@
import { reviewRefreshIntervalMs } from '../../shared/review-refresh-policy'
import type {
GitHubPRRefreshAlias,
GitHubPRRefreshCandidate,
@@ -6,7 +7,6 @@ import type {
PRRefreshOutcome
} from '../../shared/github/pull-request-refresh-types'
import type { GitHubPRBranchLookupOptions } from './client'
import { NO_REVIEW_REFRESH_INTERVAL_MS } from '../source-control/hosted-review-refresh-pacing'
export const MANUAL_MERGEABILITY_PENDING_REFRESH_MS = 2_500
export const POST_PUSH_DELAY_MS = 2_500
@@ -106,14 +106,18 @@ export function shouldSkipFresh(
candidate: GitHubPRRefreshCandidate,
reason: GitHubPRRefreshReason
): boolean {
if (bypassesFreshnessDelay(reason) || candidate.cachedFetchedAt == null) {
if (
bypassesFreshnessDelay(reason) ||
candidate.cachedFetchedAt == null ||
hasStaleHead(candidate)
) {
return false
}
return Date.now() - candidate.cachedFetchedAt < refreshIntervalForCandidate(candidate)
}
export function freshRetryAt(candidate: GitHubPRRefreshCandidate): number | null {
return candidate.cachedFetchedAt == null
return candidate.cachedFetchedAt == null || hasStaleHead(candidate)
? null
: candidate.cachedFetchedAt + refreshIntervalForCandidate(candidate)
}
@@ -144,6 +148,7 @@ export function visibleCandidateAfterOutcome(
return {
...candidate,
cachedFetchedAt: outcome.fetchedAt,
cachedHeadOid: candidate.currentHeadOid ?? null,
cachedHasPR: outcome.kind === 'found',
cachedPRState: outcome.kind === 'found' ? outcome.pr.state : null,
cachedChecksStatus: outcome.kind === 'found' ? outcome.pr.checksStatus : null,
@@ -152,31 +157,23 @@ export function visibleCandidateAfterOutcome(
}
}
function refreshIntervalForCandidate(candidate: GitHubPRRefreshCandidate): number {
if (candidate.cachedPRState === 'closed' || candidate.cachedPRState === 'merged') {
return 30 * 60_000
}
if (candidate.cachedHasPR === false) {
return NO_REVIEW_REFRESH_INTERVAL_MS
}
if (
candidate.cachedHasPR === true &&
candidate.cachedPRState === 'open' &&
candidate.cachedMergeable === 'UNKNOWN' &&
!hasResolvedMergeStateStatus(candidate.cachedMergeStateStatus)
) {
return 10_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 60_000
export function hasStaleHead(candidate: GitHubPRRefreshCandidate): boolean {
return (
candidate.currentHeadOid != null &&
candidate.cachedHeadOid != null &&
candidate.currentHeadOid !== candidate.cachedHeadOid
)
}
export function refreshIntervalForCandidate(candidate: GitHubPRRefreshCandidate): number {
return (
reviewRefreshIntervalMs({
state: candidate.cachedPRState,
checksStatus: candidate.cachedChecksStatus,
hasReview: candidate.cachedHasPR,
selected: candidate.isSelected
}) ?? Number.POSITIVE_INFINITY
)
}
function hasResolvedMergeStateStatus(status: string | null | undefined): boolean {
@@ -191,3 +188,22 @@ export function isMergeabilityPendingOutcome(outcome: PRRefreshOutcome): boolean
!hasResolvedMergeStateStatus(outcome.pr.mergeStateStatus)
)
}
export function sameAliasRequestIdentity(
left: GitHubPRRefreshAlias,
right: GitHubPRRefreshAlias
): boolean {
return (
left.cacheKey === right.cacheKey &&
left.repoId === right.repoId &&
left.repoPath === right.repoPath &&
left.branch === right.branch &&
left.worktreeId === right.worktreeId &&
left.connectionId === right.connectionId &&
left.executionHostId === right.executionHostId &&
left.linkedPRNumber === right.linkedPRNumber &&
left.fallbackPRNumber === right.fallbackPRNumber &&
left.fallbackPRSource === right.fallbackPRSource &&
left.currentHeadOid === right.currentHeadOid
)
}
@@ -326,7 +326,7 @@ describe('pr-refresh-coordinator', () => {
1
)
await vi.runOnlyPendingTimersAsync()
await vi.advanceTimersByTimeAsync(90_000)
await vi.advanceTimersByTimeAsync(120_000)
const outcomeEvents = sendMock.mock.calls
.map(([, event]) => event)
@@ -0,0 +1,198 @@
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
import type { PRRefreshOutcome } from '../../shared/github/pull-request-refresh-types'
import { runCoalescedProbe, type CoalescedProbes } from '../git/coalesced-probe'
import { makeCandidate, makePR } from './pr-refresh-coordinator-test-harness'
const { coordinatorMocks, moduleMocks } = await vi.hoisted(async () => {
const moduleMocks = await import('./pr-refresh-coordinator-test-mocks')
return { coordinatorMocks: moduleMocks.createPRRefreshCoordinatorMocks(), moduleMocks }
})
vi.mock('electron', () => moduleMocks.electronModuleMock(coordinatorMocks))
vi.mock('./client', () => moduleMocks.clientModuleMock(coordinatorMocks))
vi.mock('./github-api-repository', () =>
moduleMocks.githubApiRepositoryModuleMock(coordinatorMocks)
)
vi.mock('./rate-limit', () => moduleMocks.rateLimitModuleMock(coordinatorMocks))
vi.mock('../ipc/ui', () => moduleMocks.ipcUiModuleMock(coordinatorMocks))
const { getPRForBranchOutcomeMock, getOriginGitHubApiRepositoryMock, sendMock } = coordinatorMocks
function found(state: 'open' | 'closed' | 'merged'): PRRefreshOutcome {
return { kind: 'found', pr: makePR({ state, checksStatus: 'success' }), fetchedAt: Date.now() }
}
describe('main refresh completion ordering', () => {
beforeEach(() => moduleMocks.resetPRRefreshCoordinatorMocks(coordinatorMocks))
afterEach(() => vi.useRealTimers())
it.each([
{ older: 'open', newer: 'merged' },
{ older: 'closed', newer: 'open' }
] as const)(
'ignores expired coalesced $older reads after a newer $newer result',
async ({ older, newer }) => {
const reads: CoalescedProbes<PRRefreshOutcome> = new Map()
let finishOlder: (outcome: PRRefreshOutcome) => void = () => {}
const provider = vi.fn<() => Promise<PRRefreshOutcome>>()
provider
.mockImplementationOnce(
() => new Promise<PRRefreshOutcome>((resolve) => (finishOlder = resolve))
)
.mockImplementation(async () => found(newer))
getPRForBranchOutcomeMock.mockImplementation(() =>
runCoalescedProbe(reads, 'same-provider-lookup', provider, 120_000)
)
const {
reportVisiblePRRefreshCandidates,
refreshPRNow,
setPRRefreshOutcomeObserver,
_getPRRefreshQueueSizeForTests
} = await import('./pr-refresh-coordinator')
const observer = vi.fn()
setPRRefreshOutcomeObserver(observer)
const candidate = makeCandidate({ isSelected: true })
reportVisiblePRRefreshCandidates([candidate], 1, 1)
await vi.advanceTimersByTimeAsync(0)
await vi.advanceTimersByTimeAsync(120_000)
await refreshPRNow(candidate)
expect(provider).toHaveBeenCalledTimes(2)
await vi.advanceTimersByTimeAsync(1_000)
finishOlder(found(older))
await vi.advanceTimersByTimeAsync(0)
expect(observer).toHaveBeenCalledTimes(1)
expect(observer.mock.calls[0]?.[1]).toMatchObject({ pr: { state: newer } })
expect(_getPRRefreshQueueSizeForTests()).toBe(newer === 'merged' ? 0 : 1)
await vi.advanceTimersByTimeAsync(59_000)
expect(provider).toHaveBeenCalledTimes(newer === 'merged' ? 2 : 3)
if (newer === 'merged') {
await vi.advanceTimersByTimeAsync(900_000)
expect(provider).toHaveBeenCalledTimes(2)
}
}
)
it.each(['network', 'rate_limited'] as const)(
'ignores a late %s error after a newer settled result',
async (errorType) => {
let finishOlder: (outcome: PRRefreshOutcome) => void = () => {}
getPRForBranchOutcomeMock
.mockImplementationOnce(
() => new Promise<PRRefreshOutcome>((resolve) => (finishOlder = resolve))
)
.mockImplementation(async () => found('merged'))
const {
reportVisiblePRRefreshCandidates,
refreshPRNow,
_getPRRefreshErrorBackoffCountForTests,
_getPRRefreshQueueSizeForTests
} = await import('./pr-refresh-coordinator')
const candidate = makeCandidate({ isSelected: true })
reportVisiblePRRefreshCandidates([candidate], 1, 1)
await vi.advanceTimersByTimeAsync(0)
await refreshPRNow(candidate)
finishOlder({
kind: 'upstream-error',
errorType,
message: 'old failure',
fetchedAt: Date.now(),
retryDisabledUntil: Date.now() + 300_000
})
await vi.advanceTimersByTimeAsync(0)
expect(_getPRRefreshErrorBackoffCountForTests()).toBe(0)
expect(_getPRRefreshQueueSizeForTests()).toBe(0)
await refreshPRNow(candidate)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(3)
}
)
it('fences older direct refresh completions without suppressing their caller result', async () => {
let finishOlder: (outcome: PRRefreshOutcome) => void = () => {}
getPRForBranchOutcomeMock
.mockImplementationOnce(
() => new Promise<PRRefreshOutcome>((resolve) => (finishOlder = resolve))
)
.mockImplementation(async () => found('merged'))
const { refreshPRNow, setPRRefreshOutcomeObserver } = await import('./pr-refresh-coordinator')
const observer = vi.fn()
setPRRefreshOutcomeObserver(observer)
const candidate = makeCandidate()
const older = refreshPRNow(candidate)
await vi.advanceTimersByTimeAsync(0)
await refreshPRNow(candidate)
finishOlder(found('closed'))
await expect(older).resolves.toMatchObject({ pr: { state: 'closed' } })
expect(observer).toHaveBeenCalledTimes(1)
expect(sendMock.mock.calls.filter(([, event]) => event.outcome)).toHaveLength(2)
})
it('preserves a newer failure gate when an older successful lookup completes', async () => {
let finishOlder: (outcome: PRRefreshOutcome) => void = () => {}
getPRForBranchOutcomeMock
.mockImplementationOnce(
() => new Promise<PRRefreshOutcome>((resolve) => (finishOlder = resolve))
)
.mockImplementation(async () => ({
kind: 'upstream-error',
errorType: 'rate_limited',
message: 'wait',
fetchedAt: Date.now(),
retryDisabledUntil: Date.now() + 300_000
}))
const { reportVisiblePRRefreshCandidates, refreshPRNow } =
await import('./pr-refresh-coordinator')
const candidate = makeCandidate({ isSelected: true })
reportVisiblePRRefreshCandidates([candidate], 1, 1)
await vi.advanceTimersByTimeAsync(0)
await refreshPRNow(candidate)
finishOlder(found('open'))
await vi.advanceTimersByTimeAsync(60_000)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(2)
await expect(refreshPRNow(candidate)).resolves.toMatchObject({ errorType: 'rate_limited' })
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(2)
})
it('keeps a shared provider read coalesced while only the newest caller adopts it', async () => {
const reads: CoalescedProbes<PRRefreshOutcome> = new Map()
let finish: (outcome: PRRefreshOutcome) => void = () => {}
const provider = vi.fn(() => new Promise<PRRefreshOutcome>((resolve) => (finish = resolve)))
getPRForBranchOutcomeMock.mockImplementation(() =>
runCoalescedProbe(reads, 'same-provider-lookup', provider, 120_000)
)
const { reportVisiblePRRefreshCandidates, refreshPRNow, setPRRefreshOutcomeObserver } =
await import('./pr-refresh-coordinator')
const observer = vi.fn()
setPRRefreshOutcomeObserver(observer)
const candidate = makeCandidate({ isSelected: true })
reportVisiblePRRefreshCandidates([candidate], 1, 1)
await vi.advanceTimersByTimeAsync(0)
const manual = refreshPRNow(candidate)
await vi.advanceTimersByTimeAsync(0)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(2)
expect(provider).toHaveBeenCalledTimes(1)
finish(found('merged'))
await manual
await vi.advanceTimersByTimeAsync(900_000)
expect(observer).toHaveBeenCalledTimes(1)
expect(provider).toHaveBeenCalledTimes(1)
})
it('does not start an older queued read when admission finishes after a newer direct read', async () => {
let finishAdmission: () => void = () => {}
getOriginGitHubApiRepositoryMock.mockImplementationOnce(
() =>
new Promise<null>((resolve) => {
finishAdmission = () => resolve(null)
})
)
getPRForBranchOutcomeMock.mockImplementation(async () => found('merged'))
const { reportVisiblePRRefreshCandidates, refreshPRNow } =
await import('./pr-refresh-coordinator')
const candidate = makeCandidate({ isSelected: true })
reportVisiblePRRefreshCandidates([candidate], 1, 1)
await vi.advanceTimersByTimeAsync(0)
await refreshPRNow(candidate)
finishAdmission()
await vi.advanceTimersByTimeAsync(900_000)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(1)
})
})
@@ -0,0 +1,87 @@
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
const { coordinatorMocks, moduleMocks } = await vi.hoisted(async () => {
const moduleMocks = await import('./pr-refresh-coordinator-test-mocks')
return { coordinatorMocks: moduleMocks.createPRRefreshCoordinatorMocks(), moduleMocks }
})
vi.mock('electron', () => moduleMocks.electronModuleMock(coordinatorMocks))
vi.mock('./client', () => moduleMocks.clientModuleMock(coordinatorMocks))
vi.mock('./github-api-repository', () =>
moduleMocks.githubApiRepositoryModuleMock(coordinatorMocks)
)
vi.mock('./rate-limit', () => moduleMocks.rateLimitModuleMock(coordinatorMocks))
vi.mock('../ipc/ui', () => moduleMocks.ipcUiModuleMock(coordinatorMocks))
import { makeCandidate } from './pr-refresh-coordinator-test-harness'
import { PRRefreshQueue } from './pr-refresh-queue'
const { getPRForBranchOutcomeMock } = coordinatorMocks
describe('failed main refresh pacing', () => {
beforeEach(() => {
moduleMocks.resetPRRefreshCoordinatorMocks(coordinatorMocks)
getPRForBranchOutcomeMock.mockImplementation(async () => ({
kind: 'upstream-error',
errorType: 'network',
message: 'offline',
fetchedAt: Date.now()
}))
})
afterEach(() => vi.useRealTimers())
it.each([
{ cachedPRState: 'open', cachedHasPR: true, isSelected: true, interval: 60_000 },
{ cachedPRState: 'open', cachedHasPR: true, isSelected: false, interval: 120_000 },
{ cachedPRState: 'closed', cachedHasPR: true, isSelected: true, interval: 900_000 },
{ cachedPRState: null, cachedHasPR: false, isSelected: false, interval: 900_000 },
{ cachedPRState: null, cachedHasPR: false, isSelected: true, interval: 60_000 },
{ cachedPRState: 'merged', cachedHasPR: true, isSelected: false, interval: 60_000 }
] as const)(
'keeps $cachedPRState selected=$isSelected retries behind $interval ms',
async ({ interval, ...state }) => {
const { reportVisiblePRRefreshCandidates } = await import('./pr-refresh-coordinator')
reportVisiblePRRefreshCandidates(
[makeCandidate({ ...state, cachedChecksStatus: 'success' })],
1,
1
)
await vi.advanceTimersByTimeAsync(0)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(1)
await vi.advanceTimersByTimeAsync(interval - 1)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(1)
await vi.advanceTimersByTimeAsync(1)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(2)
await vi.advanceTimersByTimeAsync(Math.max(interval, 120_000) - 1)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(2)
await vi.advanceTimersByTimeAsync(1)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(3)
}
)
it('panel foreground exposure cannot bypass a failed lookup deadline', async () => {
const { reportVisiblePRRefreshCandidates, enqueuePRRefresh } =
await import('./pr-refresh-coordinator')
const candidate = makeCandidate({ isSelected: true })
reportVisiblePRRefreshCandidates([candidate], 1, 1)
await vi.advanceTimersByTimeAsync(0)
enqueuePRRefresh(candidate, 'visible', 80, 1)
await vi.advanceTimersByTimeAsync(59_999)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(1)
await vi.advanceTimersByTimeAsync(1)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(2)
})
it('keeps request ownership bounded and rejects evicted completions', () => {
const queue = new PRRefreshQueue(() => {})
for (let sequence = 1; sequence <= 1_001; sequence++) {
queue.noteRequestStarted(`branch-${sequence}`, sequence)
}
expect(queue.ownsRequest('branch-1', 1)).toBe(false)
expect(queue.ownsRequest('branch-1001', 1_001)).toBe(true)
queue.protectBackgroundUntil('branch-1001', Date.now() + 300_000)
expect(queue.ownsRequest('branch-1001', 1_001)).toBe(true)
queue.noteRequestStarted('branch-1001', 1_002)
expect(queue.ownsRequest('branch-1001', 1_001)).toBe(false)
expect(queue.ownsRequest('branch-1001', 1_002)).toBe(true)
})
})
@@ -0,0 +1,127 @@
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
const { coordinatorMocks, moduleMocks } = await vi.hoisted(async () => {
const moduleMocks = await import('./pr-refresh-coordinator-test-mocks')
return { coordinatorMocks: moduleMocks.createPRRefreshCoordinatorMocks(), moduleMocks }
})
vi.mock('electron', () => moduleMocks.electronModuleMock(coordinatorMocks))
vi.mock('./client', () => moduleMocks.clientModuleMock(coordinatorMocks))
vi.mock('./github-api-repository', () =>
moduleMocks.githubApiRepositoryModuleMock(coordinatorMocks)
)
vi.mock('./rate-limit', () => moduleMocks.rateLimitModuleMock(coordinatorMocks))
vi.mock('../ipc/ui', () => moduleMocks.ipcUiModuleMock(coordinatorMocks))
import { makeCandidate, makePR } from './pr-refresh-coordinator-test-harness'
const { getPRForBranchOutcomeMock } = coordinatorMocks
describe('foreground refresh pacing', () => {
beforeEach(() => {
moduleMocks.resetPRRefreshCoordinatorMocks(coordinatorMocks)
getPRForBranchOutcomeMock.mockImplementation(async () => ({
kind: 'found',
pr: makePR({ checksStatus: 'success' }),
fetchedAt: Date.now()
}))
})
afterEach(() => {
vi.useRealTimers()
})
it('limits five selected admissions to three starts until 30 seconds, including deselected queued rows', async () => {
const { enqueuePRRefresh, reportVisiblePRRefreshCandidates } =
await import('./pr-refresh-coordinator')
const cards = Array.from({ length: 5 }, (_, index) =>
makeCandidate({
cacheKey: `/repo::feature/${index}`,
branch: `feature/${index}`,
worktreeId: `wt-${index}`,
cachedPRState: 'open',
cachedChecksStatus: 'success',
cachedHasPR: true,
cachedFetchedAt: Date.now()
})
)
for (let index = 0; index < cards.length; index += 1) {
const candidates = cards.map((card, cardIndex) => ({
...card,
cachedFetchedAt: cardIndex === index ? null : card.cachedFetchedAt,
isSelected: cardIndex === index
}))
reportVisiblePRRefreshCandidates(candidates, index + 1, 1)
enqueuePRRefresh(candidates[index], 'visible', 80, 1)
await vi.advanceTimersByTimeAsync(0)
}
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(3)
await vi.advanceTimersByTimeAsync(29_999)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(3)
await vi.advanceTimersByTimeAsync(1)
expect(getPRForBranchOutcomeMock.mock.calls.map((call) => call[1])).toEqual([
'feature/0',
'feature/1',
'feature/2',
'feature/4',
'feature/3'
])
})
it('uses the list budget for ordinary priority-80 periodic follow-ups', async () => {
const { enqueuePRRefresh, reportVisiblePRRefreshCandidates } =
await import('./pr-refresh-coordinator')
coordinatorMocks.getAllWebContentsMock.mockReturnValue(
Array.from({ length: 5 }, (_, index) => ({ id: index + 1, isDestroyed: () => false }))
)
const candidates = Array.from({ length: 5 }, (_, index) =>
makeCandidate({
cacheKey: `/repo::feature/${index}`,
branch: `feature/${index}`,
worktreeId: `wt-${index}`,
isSelected: true
})
)
// Independent windows can expose different selected rows in the same runtime.
for (const [index, candidate] of candidates.entries()) {
reportVisiblePRRefreshCandidates([candidate], 1, index + 1)
enqueuePRRefresh(candidate, 'visible', 80, index + 1)
}
await vi.advanceTimersByTimeAsync(0)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(5)
await vi.advanceTimersByTimeAsync(60_000)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(6)
await vi.advanceTimersByTimeAsync(9_999)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(6)
await vi.advanceTimersByTimeAsync(1)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(7)
})
it('keeps the explicit manual mergeability follow-up outside the foreground burst budget', async () => {
const { enqueuePRRefresh, refreshPRNow, reportVisiblePRRefreshCandidates } =
await import('./pr-refresh-coordinator')
for (let index = 0; index < 3; index += 1) {
enqueuePRRefresh(
makeCandidate({ branch: `active/${index}`, cacheKey: `/repo::active/${index}` }),
'active',
80,
1
)
}
await vi.advanceTimersByTimeAsync(0)
const candidate = makeCandidate({ isSelected: true })
reportVisiblePRRefreshCandidates([candidate], 1, 1)
getPRForBranchOutcomeMock.mockResolvedValue({
kind: 'found',
pr: makePR({ mergeable: 'UNKNOWN' }),
fetchedAt: Date.now()
})
await refreshPRNow(candidate)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(4)
await vi.advanceTimersByTimeAsync(2_500)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(5)
})
})
@@ -0,0 +1,346 @@
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
import type { PRRefreshOutcome } from '../../shared/github/pull-request-refresh-types'
const { coordinatorMocks, moduleMocks } = await vi.hoisted(async () => {
const moduleMocks = await import('./pr-refresh-coordinator-test-mocks')
return { coordinatorMocks: moduleMocks.createPRRefreshCoordinatorMocks(), moduleMocks }
})
vi.mock('electron', () => moduleMocks.electronModuleMock(coordinatorMocks))
vi.mock('./client', () => moduleMocks.clientModuleMock(coordinatorMocks))
vi.mock('./github-api-repository', () =>
moduleMocks.githubApiRepositoryModuleMock(coordinatorMocks)
)
vi.mock('./rate-limit', () => moduleMocks.rateLimitModuleMock(coordinatorMocks))
vi.mock('../ipc/ui', () => moduleMocks.ipcUiModuleMock(coordinatorMocks))
import { makeCandidate, makePR } from './pr-refresh-coordinator-test-harness'
const { getAllWebContentsMock, getPRForBranchOutcomeMock, sendMock } = coordinatorMocks
function twoWindows(): void {
getAllWebContentsMock.mockReturnValue([1, 2].map((id) => ({ id, isDestroyed: () => false })))
}
function found(state: 'open' | 'merged' = 'open'): PRRefreshOutcome {
return { kind: 'found', pr: makePR({ state, checksStatus: 'success' }), fetchedAt: Date.now() }
}
describe('visibility-aware coordinator scheduling', () => {
beforeEach(() => {
moduleMocks.resetPRRefreshCoordinatorMocks(coordinatorMocks)
getPRForBranchOutcomeMock.mockImplementation(async () => found())
})
afterEach(() => {
vi.useRealTimers()
})
it.each([
[true, 60_000],
[false, 120_000]
] as const)('refreshes selected=%s at %s ms', async (isSelected, interval) => {
const { reportVisiblePRRefreshCandidates } = await import('./pr-refresh-coordinator')
reportVisiblePRRefreshCandidates([makeCandidate({ isSelected })], 1, 1)
await vi.advanceTimersByTimeAsync(interval - 1)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(1)
await vi.advanceTimersByTimeAsync(1)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(2)
})
it('uses the strongest tier across windows and slows down when its owner hides', async () => {
twoWindows()
const { reportVisiblePRRefreshCandidates } = await import('./pr-refresh-coordinator')
reportVisiblePRRefreshCandidates([makeCandidate()], 1, 1)
await vi.advanceTimersByTimeAsync(0)
await vi.advanceTimersByTimeAsync(20_000)
reportVisiblePRRefreshCandidates([makeCandidate({ isSelected: true })], 1, 2)
await vi.advanceTimersByTimeAsync(40_000)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(2)
reportVisiblePRRefreshCandidates([], 2, 2)
await vi.advanceTimersByTimeAsync(119_999)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(2)
await vi.advanceTimersByTimeAsync(1)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(3)
reportVisiblePRRefreshCandidates([], 2, 1)
await vi.advanceTimersByTimeAsync(600_000)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(3)
})
it('re-times a fresh cached follow-up when selected changes', async () => {
const { reportVisiblePRRefreshCandidates } = await import('./pr-refresh-coordinator')
const candidate = makeCandidate({
cachedFetchedAt: Date.now(),
cachedHasPR: true,
cachedPRState: 'open'
})
reportVisiblePRRefreshCandidates([candidate], 1, 1)
await vi.advanceTimersByTimeAsync(20_000)
reportVisiblePRRefreshCandidates([{ ...candidate, isSelected: true }], 2, 1)
await vi.advanceTimersByTimeAsync(39_999)
expect(getPRForBranchOutcomeMock).not.toHaveBeenCalled()
await vi.advanceTimersByTimeAsync(1)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(1)
})
it('preserves a selection change during an in-flight lookup', async () => {
twoWindows()
let resolve: (outcome: PRRefreshOutcome) => void = () => {}
getPRForBranchOutcomeMock.mockImplementationOnce(
() =>
new Promise<PRRefreshOutcome>((done) => {
resolve = done
})
)
const { reportVisiblePRRefreshCandidates } = await import('./pr-refresh-coordinator')
reportVisiblePRRefreshCandidates([makeCandidate()], 1, 1)
await vi.advanceTimersByTimeAsync(0)
reportVisiblePRRefreshCandidates([makeCandidate({ isSelected: true })], 1, 2)
resolve(found())
await vi.advanceTimersByTimeAsync(59_999)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(1)
await vi.advanceTimersByTimeAsync(1)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(2)
})
it('does not resurrect periodic work after every window hides during a lookup', async () => {
let resolve: (outcome: PRRefreshOutcome) => void = () => {}
getPRForBranchOutcomeMock.mockImplementationOnce(
() =>
new Promise<PRRefreshOutcome>((done) => {
resolve = done
})
)
const { reportVisiblePRRefreshCandidates, _getPRRefreshQueueSizeForTests } =
await import('./pr-refresh-coordinator')
reportVisiblePRRefreshCandidates([makeCandidate()], 1, 1)
await vi.advanceTimersByTimeAsync(0)
reportVisiblePRRefreshCandidates([], 2, 1)
resolve(found())
await vi.advanceTimersByTimeAsync(600_000)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(1)
expect(_getPRRefreshQueueSizeForTests()).toBe(0)
})
it('allows one settled-merged lookup on re-exposure after cooldown', async () => {
getPRForBranchOutcomeMock.mockImplementation(async () => found('merged'))
const { reportVisiblePRRefreshCandidates, _getPRRefreshQueueSizeForTests } =
await import('./pr-refresh-coordinator')
const candidate = makeCandidate()
reportVisiblePRRefreshCandidates([candidate], 1, 1)
await vi.advanceTimersByTimeAsync(0)
reportVisiblePRRefreshCandidates([], 2, 1)
const merged = makeCandidate({
cachedFetchedAt: Date.now(),
cachedHasPR: true,
cachedPRState: 'merged',
cachedChecksStatus: 'neutral'
})
await vi.advanceTimersByTimeAsync(9_999)
reportVisiblePRRefreshCandidates([merged], 3, 1)
await vi.advanceTimersByTimeAsync(0)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(1)
reportVisiblePRRefreshCandidates([], 4, 1)
await vi.advanceTimersByTimeAsync(1)
reportVisiblePRRefreshCandidates([merged], 5, 1)
await vi.advanceTimersByTimeAsync(0)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(2)
reportVisiblePRRefreshCandidates([merged], 6, 1)
await vi.advanceTimersByTimeAsync(24 * 60 * 60_000)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(2)
expect(_getPRRefreshQueueSizeForTests()).toBe(0)
})
it.each(['merged', 'closed'] as const)(
'discovers new work when a cached %s head changes',
async (cachedPRState) => {
const { reportVisiblePRRefreshCandidates } = await import('./pr-refresh-coordinator')
reportVisiblePRRefreshCandidates(
[
makeCandidate({
cachedFetchedAt: Date.now(),
cachedHasPR: true,
cachedPRState,
cachedChecksStatus: 'success',
cachedHeadOid: 'old',
currentHeadOid: 'new'
})
],
1,
1
)
await vi.advanceTimersByTimeAsync(0)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(1)
expect(getPRForBranchOutcomeMock.mock.calls[0][5]?.currentHeadOid).toBe('new')
await vi.advanceTimersByTimeAsync(120_000)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(2)
}
)
it('retains stale PR state and error backoff across reports and hiding', async () => {
getPRForBranchOutcomeMock.mockImplementation(async () => ({
kind: 'upstream-error',
errorType: 'network',
message: 'offline',
fetchedAt: Date.now()
}))
const { reportVisiblePRRefreshCandidates } = await import('./pr-refresh-coordinator')
const candidate = makeCandidate({
cachedHasPR: true,
cachedPRState: 'open',
cachedHeadOid: 'old',
currentHeadOid: 'new'
})
reportVisiblePRRefreshCandidates([candidate], 1, 1)
await vi.advanceTimersByTimeAsync(0)
reportVisiblePRRefreshCandidates([], 2, 1)
reportVisiblePRRefreshCandidates([candidate], 3, 1)
reportVisiblePRRefreshCandidates([{ ...candidate, isSelected: true }], 4, 1)
await vi.advanceTimersByTimeAsync(119_999)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(1)
await vi.advanceTimersByTimeAsync(1)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(2)
reportVisiblePRRefreshCandidates([candidate], 5, 1)
await vi.advanceTimersByTimeAsync(119_999)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(2)
await vi.advanceTimersByTimeAsync(1)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(3)
expect(sendMock.mock.calls.map(([, event]) => event.outcome?.kind).filter(Boolean)).toEqual([
'upstream-error',
'upstream-error',
'upstream-error'
])
})
it('pulls a closed follow-up forward when HEAD changes while it is queued', async () => {
const { reportVisiblePRRefreshCandidates } = await import('./pr-refresh-coordinator')
const candidate = makeCandidate({
cachedFetchedAt: Date.now(),
cachedHasPR: true,
cachedPRState: 'closed',
cachedHeadOid: 'old',
currentHeadOid: 'old'
})
reportVisiblePRRefreshCandidates([candidate], 1, 1)
await vi.advanceTimersByTimeAsync(20_000)
reportVisiblePRRefreshCandidates([{ ...candidate, currentHeadOid: 'new' }], 2, 1)
await vi.advanceTimersByTimeAsync(0)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(1)
expect(getPRForBranchOutcomeMock.mock.calls[0][5]?.currentHeadOid).toBe('new')
})
it('keeps an in-flight head change behind the lookup failure backoff', async () => {
let resolve: (outcome: PRRefreshOutcome) => void = () => {}
getPRForBranchOutcomeMock.mockImplementationOnce(
() =>
new Promise<PRRefreshOutcome>((done) => {
resolve = done
})
)
const { reportVisiblePRRefreshCandidates } = await import('./pr-refresh-coordinator')
const candidate = makeCandidate({ currentHeadOid: 'old' })
reportVisiblePRRefreshCandidates([candidate], 1, 1)
await vi.advanceTimersByTimeAsync(0)
reportVisiblePRRefreshCandidates([{ ...candidate, currentHeadOid: 'new' }], 2, 1)
resolve({
kind: 'upstream-error',
errorType: 'network',
message: 'offline',
fetchedAt: Date.now()
})
await vi.advanceTimersByTimeAsync(119_999)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(1)
await vi.advanceTimersByTimeAsync(1)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(2)
expect(getPRForBranchOutcomeMock.mock.calls[1][5]?.currentHeadOid).toBe('new')
})
it.each([true, false])(
'preserves a rate-limit gate across selected reports (hidden=%s)',
async (hidden) => {
getPRForBranchOutcomeMock.mockImplementation(async () => ({
kind: 'upstream-error',
errorType: 'rate_limited',
message: 'wait',
fetchedAt: Date.now(),
retryDisabledUntil: 301_000
}))
const { reportVisiblePRRefreshCandidates } = await import('./pr-refresh-coordinator')
const candidate = makeCandidate()
reportVisiblePRRefreshCandidates([candidate], 1, 1)
await vi.advanceTimersByTimeAsync(0)
if (hidden) {
reportVisiblePRRefreshCandidates([], 2, 1)
}
await vi.advanceTimersByTimeAsync(10_000)
reportVisiblePRRefreshCandidates([{ ...candidate, isSelected: true }], 3, 1)
await vi.advanceTimersByTimeAsync(289_999)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(1)
await vi.advanceTimersByTimeAsync(1)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(2)
}
)
it('discovers a new PR on settled-merged re-exposure and resumes selected polling', async () => {
getPRForBranchOutcomeMock
.mockImplementationOnce(async () => found('merged'))
.mockImplementation(async () => ({
kind: 'found',
pr: makePR({ number: 13 }),
fetchedAt: Date.now()
}))
const { reportVisiblePRRefreshCandidates } = await import('./pr-refresh-coordinator')
reportVisiblePRRefreshCandidates([makeCandidate({ isSelected: true })], 1, 1)
await vi.advanceTimersByTimeAsync(0)
const merged = makeCandidate({
isSelected: true,
cachedFetchedAt: Date.now(),
cachedHasPR: true,
cachedPRState: 'merged',
cachedChecksStatus: 'success'
})
reportVisiblePRRefreshCandidates([], 2, 1)
await vi.advanceTimersByTimeAsync(120_000)
reportVisiblePRRefreshCandidates([merged], 3, 1)
await vi.advanceTimersByTimeAsync(0)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(2)
expect(
sendMock.mock.calls.map(([, event]) => event.outcome?.pr?.number).filter(Boolean)
).toEqual([12, 13])
await vi.advanceTimersByTimeAsync(59_999)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(2)
await vi.advanceTimersByTimeAsync(1)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(3)
})
it.each([
[true, 60_000],
[false, 900_000]
] as const)('discovers missing PRs at selected=%s cadence', async (isSelected, interval) => {
getPRForBranchOutcomeMock.mockImplementation(async () => ({
kind: 'no-pr',
fetchedAt: Date.now()
}))
const { reportVisiblePRRefreshCandidates } = await import('./pr-refresh-coordinator')
reportVisiblePRRefreshCandidates([makeCandidate({ isSelected })], 1, 1)
await vi.advanceTimersByTimeAsync(interval - 1)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(1)
await vi.advanceTimersByTimeAsync(1)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(2)
})
})
it('foreground exposure bypasses list spacing once while preserving cooldown and periodic budget', async () => {
moduleMocks.resetPRRefreshCoordinatorMocks(coordinatorMocks)
getPRForBranchOutcomeMock.mockImplementation(async () => found())
const { reportVisiblePRRefreshCandidates, enqueuePRRefresh } =
await import('./pr-refresh-coordinator')
const other = makeCandidate({ branch: 'other', cacheKey: 'other', worktreeId: 'other' })
const selected = makeCandidate({ branch: 'selected', cacheKey: 'selected', isSelected: true })
reportVisiblePRRefreshCandidates([other, selected], 1, 1)
await vi.advanceTimersByTimeAsync(0)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(1)
enqueuePRRefresh(selected, 'visible', 80, 1)
await vi.advanceTimersByTimeAsync(0)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(2)
enqueuePRRefresh(selected, 'visible', 80, 1)
await vi.advanceTimersByTimeAsync(9_999)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(2)
vi.useRealTimers()
})
@@ -81,7 +81,7 @@ describe('pr-refresh-coordinator', () => {
expect(_getPRRefreshErrorBackoffCountForTests()).toBe(0)
})
it('retries visible PRs with unknown mergeability before the success-check interval', async () => {
it('uses the regular cadence for automatic unknown mergeability refreshes', async () => {
const { reportVisiblePRRefreshCandidates } = await import('./pr-refresh-coordinator')
getPRForBranchOutcomeMock
.mockResolvedValueOnce({
@@ -97,7 +97,7 @@ describe('pr-refresh-coordinator', () => {
reportVisiblePRRefreshCandidates([makeCandidate()], 1, 1)
await vi.advanceTimersByTimeAsync(0)
await vi.advanceTimersByTimeAsync(9_999)
await vi.advanceTimersByTimeAsync(119_999)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(1)
@@ -149,4 +149,114 @@ describe('pr-refresh-coordinator', () => {
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(3)
})
it('stops polling settled merged PRs, including repeated visibility reports', async () => {
const { reportVisiblePRRefreshCandidates } = await import('./pr-refresh-coordinator')
getPRForBranchOutcomeMock.mockImplementation(async () => ({
kind: 'found',
pr: makePR({ checksStatus: 'success', state: 'merged' }),
fetchedAt: Date.now()
}))
reportVisiblePRRefreshCandidates([makeCandidate()], 1, 1)
await vi.advanceTimersByTimeAsync(0)
const merged = makeCandidate({
cachedFetchedAt: Date.now(),
cachedHasPR: true,
cachedPRState: 'merged',
cachedChecksStatus: 'success'
})
await vi.advanceTimersByTimeAsync(31 * 60_000)
reportVisiblePRRefreshCandidates([merged], 2, 1)
await vi.advanceTimersByTimeAsync(0)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(1)
await vi.advanceTimersByTimeAsync(24 * 60 * 60_000 - 31 * 60_000)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(1)
})
it.each([
['closed', 'success', 15 * 60_000],
['merged', 'pending', 60_000]
] as const)('keeps watching %s PRs with %s checks', async (state, checksStatus, interval) => {
const { reportVisiblePRRefreshCandidates } = await import('./pr-refresh-coordinator')
getPRForBranchOutcomeMock.mockImplementation(async () => ({
kind: 'found',
pr: makePR({ checksStatus, state }),
fetchedAt: Date.now()
}))
reportVisiblePRRefreshCandidates([makeCandidate()], 1, 1)
await vi.advanceTimersByTimeAsync(interval)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(2)
})
it('checks merged reviews with missing cached checks again after 60 seconds', async () => {
const { reportVisiblePRRefreshCandidates } = await import('./pr-refresh-coordinator')
getPRForBranchOutcomeMock.mockResolvedValue({
kind: 'found',
pr: makePR({ state: 'merged', checksStatus: 'success' }),
fetchedAt: Date.now()
})
reportVisiblePRRefreshCandidates(
[
makeCandidate({
cachedFetchedAt: Date.now(),
cachedHasPR: true,
cachedPRState: 'merged',
cachedChecksStatus: null
})
],
1,
1
)
await vi.advanceTimersByTimeAsync(59_999)
expect(getPRForBranchOutcomeMock).not.toHaveBeenCalled()
await vi.advanceTimersByTimeAsync(1)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(1)
})
it('resumes visible polling when a manual refresh discovers a live PR', async () => {
const { refreshPRNow, reportVisiblePRRefreshCandidates } =
await import('./pr-refresh-coordinator')
getPRForBranchOutcomeMock
.mockImplementationOnce(async () => ({
kind: 'found',
pr: makePR({ state: 'merged', checksStatus: 'success' }),
fetchedAt: Date.now()
}))
.mockImplementation(async () => ({
kind: 'found',
pr: makePR({ number: 13, checksStatus: 'pending' }),
fetchedAt: Date.now()
}))
const candidate = makeCandidate()
reportVisiblePRRefreshCandidates([candidate], 1, 1)
await vi.advanceTimersByTimeAsync(0)
await refreshPRNow(candidate)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(2)
await vi.advanceTimersByTimeAsync(120_000)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(3)
})
it.each(['active', 'manual', 'post-push'] as const)(
'refreshes merged PRs on %s',
async (reason) => {
const { enqueuePRRefresh } = await import('./pr-refresh-coordinator')
getPRForBranchOutcomeMock.mockResolvedValue({
kind: 'found',
pr: makePR(),
fetchedAt: Date.now()
})
enqueuePRRefresh(
makeCandidate({
cachedFetchedAt: Date.now(),
cachedPRState: 'merged',
cachedChecksStatus: 'success'
}),
reason,
80,
1
)
await vi.advanceTimersByTimeAsync(2_500)
expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(1)
}
)
})
+29 -2
View File
@@ -51,6 +51,8 @@ export function clearVisiblePRRefreshWindow(windowId: number): void {
pacing.clearActiveBurstWindow(windowId)
if (hadVisibleRefreshes) {
removeInvisibleVisibleRefreshes()
queue.retimeVisible((key, candidate) => visibility.candidate(key, candidate))
drainer.schedule()
}
}
@@ -75,6 +77,11 @@ export function enqueuePRRefresh(
}
const enqueued = queue.enqueue(candidate, reason, priority, windowId)
const pending = queue.get(key)
if (pending && reason === 'visible' && candidate.isSelected && priority >= 80) {
// Foreground exposure keeps retry gates while bypassing the list budget once.
pending.bypassBackgroundBudget = true
}
events.record(enqueued.coalesced ? 'coalesced' : 'enqueued', reason)
if (shouldBroadcastQueued(reason, enqueued.dueAt)) {
events.broadcast({ aliases: [enqueued.alias], reason, status: 'queued' })
@@ -87,13 +94,27 @@ export function reportVisiblePRRefreshCandidates(
generation: number,
windowId: number
): void {
const newlyExposed = new Set(candidates.map(refreshKey).filter((key) => !visibility.has(key)))
if (!visibility.report(candidates, generation, windowId)) {
return
}
removeInvisibleVisibleRefreshes()
queue.retimeVisible((key, candidate) => visibility.candidate(key, candidate))
for (const candidate of candidates) {
enqueuePRRefresh(candidate, 'visible', 40, windowId)
const key = refreshKey(candidate)
if (validateCandidate(candidate)) {
enqueuePRRefresh(candidate, 'visible', 40, windowId)
continue
}
queue.enqueue(
visibility.candidate(key, candidate),
'visible',
40,
windowId,
newlyExposed.has(key)
)
}
drainer.schedule()
}
export function _getVisiblePRRefreshWindowCountForTests(): number {
@@ -138,6 +159,7 @@ export async function refreshPRNow(
const primaryGateUntil = await prRefreshRateLimitPausedUntil(candidate, false)
const gateUntil = Math.max(primaryGateUntil ?? 0, retry.manualGateUntil(key))
if (gateUntil > Date.now()) {
queue.protectBackgroundUntil(key, gateUntil)
queue.set(key, {
key,
candidate,
@@ -168,6 +190,7 @@ export async function refreshPRNow(
queue.delete(key)
const requestSequence = events.nextSequence()
const requestStartedAt = Date.now()
queue.noteRequestStarted(key, requestSequence)
events.broadcast({ aliases, reason, status: 'in-flight', requestStartedAt }, requestSequence)
const outcome = await getPRForBranchOutcome(
candidate.repoPath,
@@ -177,10 +200,14 @@ export async function refreshPRNow(
candidate.linkedPRNumber == null ? (candidate.fallbackPRNumber ?? null) : null,
...hostedReviewOptionArgs(candidate, reason)
)
if (!queue.ownsRequest(key, requestSequence)) {
events.broadcast({ aliases, reason, outcome, requestStartedAt }, requestSequence)
return outcome
}
let plannedRetryAt: number | undefined
let broadcastOutcome = outcome
if (outcome.kind === 'upstream-error' && visibility.has(key)) {
plannedRetryAt = retry.nextVisibleErrorRetryAt(key)
plannedRetryAt = retry.nextVisibleErrorRetryAt(key, visibility.candidate(key, candidate))
broadcastOutcome = retry.withErrorSchedule(outcome, plannedRetryAt)
}
events.observe(candidate, outcome)
+11 -3
View File
@@ -6,6 +6,14 @@ const BACKGROUND_BUDGET_MAX = 20
const ACTIVE_BURST_WINDOW_MS = 30_000
const ACTIVE_BURST_MAX = 3
export function usesActiveRefreshPacing(entry: PRRefreshQueueEntry): boolean {
// Selected admission survives deselection while queued; periodic follow-ups clear the flag.
return (
entry.reason === 'active' ||
(entry.reason === 'visible' && entry.priority >= 80 && entry.bypassBackgroundBudget === true)
)
}
export class PRRefreshPacing {
private readonly backgroundStarts: number[] = []
private readonly activeStartsByScope = new Map<string, number[]>()
@@ -42,7 +50,7 @@ export class PRRefreshPacing {
}
activeOrder(a: PRRefreshQueueEntry, b: PRRefreshQueueEntry): number {
if (a.reason !== 'active' || b.reason !== 'active') {
if (!usesActiveRefreshPacing(a) || !usesActiveRefreshPacing(b)) {
return 0
}
if (this.activeBurstScope(a) !== this.activeBurstScope(b)) {
@@ -52,7 +60,7 @@ export class PRRefreshPacing {
}
entryDelay(entry: PRRefreshQueueEntry): number {
const activeDelay = entry.reason === 'active' ? this.nextActiveBurstDelay(entry) : 0
const activeDelay = usesActiveRefreshPacing(entry) ? this.nextActiveBurstDelay(entry) : 0
if (activeDelay > 0) {
return activeDelay
}
@@ -63,7 +71,7 @@ export class PRRefreshPacing {
}
isActiveBurstDelayed(entry: PRRefreshQueueEntry): boolean {
return entry.reason === 'active' && this.nextActiveBurstDelay(entry) > 0
return usesActiveRefreshPacing(entry) && this.nextActiveBurstDelay(entry) > 0
}
noteActiveStart(entry: PRRefreshQueueEntry): void {
+49 -10
View File
@@ -5,15 +5,17 @@ import type {
} from '../../shared/github/pull-request-refresh-types'
import { getPRForBranchOutcome } from './client'
import {
aliasFromCandidate,
freshRetryAt,
hostedReviewOptionArgs,
isBackground,
isMergeabilityPendingOutcome,
sameAliasRequestIdentity,
validateCandidate,
visibleCandidateAfterOutcome
} from './pr-refresh-candidate-policy'
import type { PRRefreshEventPublisher } from './pr-refresh-event-publisher'
import type { PRRefreshPacing } from './pr-refresh-pacing'
import { type PRRefreshPacing, usesActiveRefreshPacing } from './pr-refresh-pacing'
import type { PRRefreshQueue, PRRefreshQueueEntry } from './pr-refresh-queue'
import { prRefreshRateLimitPausedUntil } from './pr-refresh-rate-limit-gate'
import type { PRRefreshRetryState } from './pr-refresh-retry-state'
@@ -50,12 +52,16 @@ export class PRRefreshQueueDrainer {
windowId?: number,
options?: { pendingMergeabilityDelayMs?: number; plannedRetryAt?: number }
): void {
if (!this.visibility.has(key)) {
this.retry.reset(key)
return
}
if (outcome.kind === 'upstream-error') {
const retryAt = options?.plannedRetryAt ?? this.retry.nextVisibleErrorRetryAt(key)
const retryAt = Math.max(
options?.plannedRetryAt ??
this.retry.nextVisibleErrorRetryAt(key, this.visibility.candidate(key, candidate)),
this.retry.manualGateUntil(key)
)
this.queue.protectBackgroundUntil(key, retryAt)
if (!this.visibility.has(key)) {
return
}
this.queue.setVisibleFollowUp({
key,
candidate,
@@ -70,13 +76,31 @@ export class PRRefreshQueueDrainer {
return
}
this.retry.reset(key)
const followUpCandidate = visibleCandidateAfterOutcome(candidate, outcome)
const refreshed = visibleCandidateAfterOutcome(candidate, outcome)
this.visibility.update(refreshed)
if (!this.visibility.has(key)) {
return
}
const followUpCandidate = this.visibility.candidate(key, refreshed)
const regularDueAt = freshRetryAt(followUpCandidate) ?? Date.now()
const pendingDueAt =
options?.pendingMergeabilityDelayMs !== undefined && isMergeabilityPendingOutcome(outcome)
? outcome.fetchedAt + options.pendingMergeabilityDelayMs
: null
const dueAt = pendingDueAt === null ? regularDueAt : Math.min(regularDueAt, pendingDueAt)
if (!Number.isFinite(dueAt)) {
const pending = this.queue.get(key)
if (
pending?.reason === 'visible' &&
sameAliasRequestIdentity(
aliasFromCandidate(pending.candidate),
aliasFromCandidate(followUpCandidate)
)
) {
this.queue.delete(key)
}
return
}
this.queue.setVisibleFollowUp({
key,
candidate: followUpCandidate,
@@ -86,6 +110,7 @@ export class PRRefreshQueueDrainer {
dueAt,
queuedAt: this.queue.nextOrder(),
bypassBackgroundBudget: pendingDueAt !== null,
followUp: true,
windowId
})
this.schedule(Math.max(0, dueAt - Date.now()))
@@ -146,7 +171,6 @@ export class PRRefreshQueueDrainer {
continue
}
if (next.reason === 'visible' && !this.visibility.has(next.key)) {
this.retry.reset(next.key)
this.events.broadcast({
aliases,
reason: next.reason,
@@ -157,6 +181,7 @@ export class PRRefreshQueueDrainer {
}
const requestSequence = this.events.nextSequence()
const requestStartedAt = Date.now()
this.queue.noteRequestStarted(next.key, requestSequence)
this.events.broadcast(
{ aliases, reason: next.reason, status: 'in-flight', requestStartedAt },
requestSequence
@@ -164,7 +189,11 @@ export class PRRefreshQueueDrainer {
if (isBackground(next.reason)) {
const pausedUntil = await prRefreshRateLimitPausedUntil(next.candidate, true)
if (!this.queue.ownsRequest(next.key, requestSequence)) {
continue
}
if (pausedUntil !== null) {
this.queue.protectBackgroundUntil(next.key, pausedUntil)
this.queue.set(next.key, { ...next, dueAt: pausedUntil })
this.events.broadcast({
aliases,
@@ -182,7 +211,7 @@ export class PRRefreshQueueDrainer {
) {
this.pacing.noteBackgroundStart()
}
if (next.reason === 'active') {
if (usesActiveRefreshPacing(next)) {
this.pacing.noteActiveStart(next)
}
}
@@ -195,10 +224,20 @@ export class PRRefreshQueueDrainer {
next.candidate.linkedPRNumber == null ? (next.candidate.fallbackPRNumber ?? null) : null,
...hostedReviewOptionArgs(next.candidate, next.reason)
)
if (!this.queue.ownsRequest(next.key, requestSequence)) {
this.events.broadcast(
{ aliases, reason: next.reason, outcome, requestStartedAt },
requestSequence
)
continue
}
let plannedRetryAt: number | undefined
let broadcastOutcome = outcome
if (outcome.kind === 'upstream-error' && this.visibility.has(next.key)) {
plannedRetryAt = this.retry.nextVisibleErrorRetryAt(next.key)
plannedRetryAt = this.retry.nextVisibleErrorRetryAt(
next.key,
this.visibility.candidate(next.key, next.candidate)
)
broadcastOutcome = this.retry.withErrorSchedule(outcome, plannedRetryAt)
}
this.events.observe(next.candidate, outcome)
@@ -192,10 +192,10 @@ describe('pr-refresh queue growth bounds', () => {
})
reportVisiblePRRefreshCandidates([first], 1, 1)
await vi.advanceTimersByTimeAsync(100_000)
await vi.advanceTimersByTimeAsync(119_999)
expect(getPRForBranchOutcomeMock.mock.calls.map((call) => call[1])).toEqual(['churn/1'])
await vi.advanceTimersByTimeAsync(500_001)
await vi.advanceTimersByTimeAsync(1)
expect(getPRForBranchOutcomeMock.mock.calls.map((call) => call[1])).toEqual([
'churn/1',
'churn/2'
+81 -52
View File
@@ -1,3 +1,4 @@
import { REVIEW_REFRESH_COOLDOWN_MS } from '../../shared/review-refresh-policy'
import type {
GitHubPRRefreshAlias,
GitHubPRRefreshCandidate,
@@ -9,6 +10,8 @@ import {
freshRetryAt,
POST_PUSH_DELAY_MS,
refreshKey,
sameAliasRequestIdentity,
refreshIntervalForCandidate,
shouldSkipFresh
} from './pr-refresh-candidate-policy'
@@ -22,6 +25,7 @@ export type PRRefreshQueueEntry = {
queuedAt: number
bypassBackgroundBudget?: boolean
activeDelayNotified?: boolean
followUp?: boolean
windowId?: number
}
@@ -32,6 +36,8 @@ export type PRRefreshEnqueue = {
coalesced: boolean
}
type PRRefreshRequestState = { until: number; requestSequence?: number }
/** A worktree has one live branch at a time, so a second cacheKey for it is a
* branch it moved off. Drop those: a linked-PR key survives every branch
* switch, so while its entry is parked (rate-limit pause, error backoff, or
@@ -70,25 +76,6 @@ function mergeFollowUpAlias(
return undefined
}
function sameAliasRequestIdentity(
left: GitHubPRRefreshAlias,
right: GitHubPRRefreshAlias
): boolean {
return (
left.cacheKey === right.cacheKey &&
left.repoId === right.repoId &&
left.repoPath === right.repoPath &&
left.branch === right.branch &&
left.worktreeId === right.worktreeId &&
left.connectionId === right.connectionId &&
left.executionHostId === right.executionHostId &&
left.linkedPRNumber === right.linkedPRNumber &&
left.fallbackPRNumber === right.fallbackPRNumber &&
left.fallbackPRSource === right.fallbackPRSource &&
left.currentHeadOid === right.currentHeadOid
)
}
/** A manual refresh merges its alias into its own copy of the map and writes it
* back, so re-entry through `set` has to re-apply the same bound; later
* insertions are the newer branch and win. */
@@ -109,6 +96,8 @@ function dropSupersededWorktreeAliases(aliases: Map<string, GitHubPRRefreshAlias
export class PRRefreshQueue {
private readonly entries = new Map<string, PRRefreshQueueEntry>()
private order = 0
// Completion ownership shares the cooldown map's bound.
private readonly backgroundNotBefore = new Map<string, PRRefreshRequestState>()
constructor(private readonly resetRetryState: (key: string) => void) {}
@@ -142,17 +131,73 @@ export class PRRefreshQueue {
return this.entries.get(key)?.aliases.size ?? 0
}
protectBackgroundUntil(key: string, until: number, requestSequence?: number): void {
const owner = requestSequence ?? this.backgroundNotBefore.get(key)?.requestSequence
this.backgroundNotBefore.delete(key)
this.backgroundNotBefore.set(key, { until, requestSequence: owner })
const pending = this.entries.get(key)
if (pending && !bypassesFreshnessDelay(pending.reason)) {
pending.dueAt = Math.max(pending.dueAt, until)
}
const oldest = this.backgroundNotBefore.keys().next().value
if (this.backgroundNotBefore.size > 1_000 && oldest !== undefined) {
this.backgroundNotBefore.delete(oldest)
this.resetRetryState(oldest)
}
}
noteRequestStarted(key: string, requestSequence: number): void {
this.protectBackgroundUntil(key, Date.now() + REVIEW_REFRESH_COOLDOWN_MS, requestSequence)
}
ownsRequest(key: string, requestSequence: number): boolean {
return this.backgroundNotBefore.get(key)?.requestSequence === requestSequence
}
retimeVisible(
candidateFor: (key: string, candidate: GitHubPRRefreshCandidate) => GitHubPRRefreshCandidate
): void {
for (const entry of this.entries.values()) {
if (entry.reason !== 'visible' || !entry.followUp || entry.bypassBackgroundBudget) {
continue
}
entry.candidate = candidateFor(entry.key, entry.candidate)
entry.dueAt = Math.max(
freshRetryAt(entry.candidate) ?? Date.now(),
this.backgroundNotBefore.get(entry.key)?.until ?? 0
)
if (!Number.isFinite(entry.dueAt)) {
this.entries.delete(entry.key)
}
}
}
enqueue(
candidate: GitHubPRRefreshCandidate,
reason: GitHubPRRefreshReason,
priority: number,
windowId?: number
windowId?: number,
reexposed = false
): PRRefreshEnqueue {
const alias = aliasFromCandidate(candidate)
const key = refreshKey(candidate)
const existing = this.entries.get(key)
const freshDueAt = shouldSkipFresh(candidate, reason) ? freshRetryAt(candidate) : null
const dueAt = freshDueAt ?? Date.now() + (reason === 'post-push' ? POST_PUSH_DELAY_MS : 0)
const stopped = !Number.isFinite(refreshIntervalForCandidate(candidate))
const exposureDueAt =
reexposed &&
stopped &&
candidate.cachedFetchedAt != null &&
Date.now() - candidate.cachedFetchedAt >= REVIEW_REFRESH_COOLDOWN_MS
? Date.now()
: null
const dueAt = Math.max(
exposureDueAt ?? freshDueAt ?? Date.now() + (reason === 'post-push' ? POST_PUSH_DELAY_MS : 0),
bypassesFreshnessDelay(reason) ? 0 : (this.backgroundNotBefore.get(key)?.until ?? 0)
)
if (!Number.isFinite(dueAt) && !existing) {
return { alias, key, dueAt, coalesced: false }
}
if (!existing) {
this.entries.set(key, {
key,
@@ -162,12 +207,21 @@ export class PRRefreshQueue {
priority,
dueAt,
queuedAt: this.nextOrder(),
followUp: reason === 'visible' && freshDueAt !== null && exposureDueAt === null,
windowId
})
return { alias, key, dueAt, coalesced: false }
}
setLiveAlias(existing.aliases, alias)
if (
reason === 'visible' &&
!shouldSkipFresh(candidate, reason) &&
existing.reason === 'visible'
) {
existing.dueAt = Math.min(existing.dueAt, dueAt)
existing.followUp = false
}
const shouldPromote =
priority > existing.priority ||
reason === 'manual' ||
@@ -186,7 +240,8 @@ export class PRRefreshQueue {
...existing.candidate,
cacheKey: candidate.cacheKey,
branch: candidate.branch,
currentHeadOid: candidate.currentHeadOid ?? null
currentHeadOid: candidate.currentHeadOid ?? null,
isSelected: candidate.isSelected
}
}
return { alias, key, dueAt, coalesced: true }
@@ -207,9 +262,7 @@ export class PRRefreshQueue {
if (existing.candidate.cacheKey === alias.cacheKey) {
existing.candidate = {
...existing.candidate,
cacheKey: replacement.cacheKey,
branch: replacement.branch,
worktreeId: replacement.worktreeId,
...replacement,
currentHeadOid: replacement.currentHeadOid ?? null,
isArchived: false,
isBare: false
@@ -219,31 +272,9 @@ export class PRRefreshQueue {
pruneWorktreeAliases(worktreeId: string): void {
for (const [key, entry] of this.entries) {
let removed = false
for (const [cacheKey, alias] of entry.aliases) {
for (const alias of entry.aliases.values()) {
if (alias.worktreeId === worktreeId) {
entry.aliases.delete(cacheKey)
removed = true
}
}
if (!removed) {
continue
}
if (entry.aliases.size === 0) {
this.entries.delete(key)
this.resetRetryState(key)
continue
}
if (entry.candidate.worktreeId === worktreeId) {
const replacement = entry.aliases.values().next().value
if (replacement) {
entry.candidate = {
...entry.candidate,
cacheKey: replacement.cacheKey,
branch: replacement.branch,
worktreeId: replacement.worktreeId,
currentHeadOid: replacement.currentHeadOid ?? null
}
this.removeInvalidAlias(key, alias)
}
}
}
@@ -256,7 +287,6 @@ export class PRRefreshQueue {
continue
}
this.entries.delete(key)
this.resetRetryState(key)
removed.push(entry)
}
return removed
@@ -282,8 +312,7 @@ export class PRRefreshQueue {
if (
candidateSuperseded ||
bypassesFreshnessDelay(existing.reason) ||
existing.priority > entry.priority ||
existing.dueAt <= entry.dueAt
existing.priority > entry.priority
) {
return
}
+10 -3
View File
@@ -1,5 +1,9 @@
import type { PRRefreshOutcome } from '../../shared/github/pull-request-refresh-types'
import type {
GitHubPRRefreshCandidate,
PRRefreshOutcome
} from '../../shared/github/pull-request-refresh-types'
import { lookupBackoffDelayMs } from '../source-control/hosted-review-refresh-pacing'
import { refreshIntervalForCandidate } from './pr-refresh-candidate-policy'
export class PRRefreshRetryState {
private readonly errorBackoff = new Map<string, { failures: number; retryAt: number }>()
@@ -26,9 +30,12 @@ export class PRRefreshRetryState {
return this.manualRetryGates.get(key) ?? 0
}
nextVisibleErrorRetryAt(key: string): number {
nextVisibleErrorRetryAt(key: string, candidate: GitHubPRRefreshCandidate): number {
const failures = (this.errorBackoff.get(key)?.failures ?? 0) + 1
const retryAt = Date.now() + lookupBackoffDelayMs(failures)
const interval = refreshIntervalForCandidate(candidate)
const retryAt =
Date.now() +
Math.max(lookupBackoffDelayMs(failures), Number.isFinite(interval) ? interval : 0)
this.errorBackoff.set(key, { failures, retryAt })
return retryAt
}
+64 -9
View File
@@ -3,7 +3,10 @@ import type { GitHubPRRefreshCandidate } from '../../shared/github/pull-request-
import { refreshKey } from './pr-refresh-candidate-policy'
export class PRRefreshVisibility {
private readonly visibleByWindow = new Map<number, { generation: number; keys: Set<string> }>()
private readonly visibleByWindow = new Map<
number,
{ generation: number; candidates: Map<string, GitHubPRRefreshCandidate> }
>()
get windowCount(): number {
return this.visibleByWindow.size
@@ -14,31 +17,83 @@ export class PRRefreshVisibility {
}
report(candidates: GitHubPRRefreshCandidate[], generation: number, windowId: number): boolean {
this.pruneDestroyedWindows()
const existing = this.visibleByWindow.get(windowId)
if (existing && generation < existing.generation) {
return false
}
this.visibleByWindow.set(windowId, { generation, keys: new Set(candidates.map(refreshKey)) })
const reported = new Map<string, GitHubPRRefreshCandidate>()
for (const candidate of candidates) {
const key = refreshKey(candidate)
const previous = this.candidate(key, candidate)
reported.set(key, {
...previous,
...candidate,
...(previous.cachedFetchedAt != null &&
previous.cachedFetchedAt > (candidate.cachedFetchedAt ?? 0)
? this.cachedFields(previous)
: {}),
isSelected: candidate.isSelected === true || reported.get(key)?.isSelected === true
})
}
this.visibleByWindow.set(windowId, { generation, candidates: reported })
return true
}
has(key: string): boolean {
this.pruneDestroyedWindows()
return Array.from(this.visibleByWindow.values()).some((window) => window.candidates.has(key))
}
candidate(key: string, fallback: GitHubPRRefreshCandidate): GitHubPRRefreshCandidate {
let latest = fallback
let selected = false
for (const window of this.visibleByWindow.values()) {
const candidate = window.candidates.get(key)
if (!candidate) {
continue
}
selected ||= candidate.isSelected === true
if ((candidate.cachedFetchedAt ?? 0) > (latest.cachedFetchedAt ?? 0)) {
latest = { ...fallback, ...this.cachedFields(candidate) }
}
}
return { ...latest, isSelected: selected }
}
update(candidate: GitHubPRRefreshCandidate): void {
const key = refreshKey(candidate)
for (const window of this.visibleByWindow.values()) {
const current = window.candidates.get(key)
if (current && (current.cachedFetchedAt ?? 0) <= (candidate.cachedFetchedAt ?? 0)) {
window.candidates.set(key, { ...current, ...this.cachedFields(candidate) })
}
}
}
private cachedFields(candidate: GitHubPRRefreshCandidate): Partial<GitHubPRRefreshCandidate> {
return {
cachedFetchedAt: candidate.cachedFetchedAt,
cachedHeadOid: candidate.cachedHeadOid,
cachedHasPR: candidate.cachedHasPR,
cachedPRState: candidate.cachedPRState,
cachedChecksStatus: candidate.cachedChecksStatus,
cachedMergeable: candidate.cachedMergeable,
cachedMergeStateStatus: candidate.cachedMergeStateStatus
}
}
private pruneDestroyedWindows(): void {
const liveWindowIds = new Set(
webContents
.getAllWebContents()
.filter((contents) => !contents.isDestroyed())
.map((contents) => contents.id)
)
for (const windowId of Array.from(this.visibleByWindow.keys())) {
for (const windowId of this.visibleByWindow.keys()) {
if (!liveWindowIds.has(windowId)) {
this.visibleByWindow.delete(windowId)
}
}
for (const visible of this.visibleByWindow.values()) {
if (visible.keys.has(key)) {
return true
}
}
return false
}
}