diff --git a/src/main/git/runner-command-exec.test.ts b/src/main/git/runner-command-exec.test.ts index 41975f9d665..dd24b8c8fb0 100644 --- a/src/main/git/runner-command-exec.test.ts +++ b/src/main/git/runner-command-exec.test.ts @@ -232,6 +232,22 @@ describe('runner execFile timeout handling', () => { expect(child.kill).toHaveBeenCalled() }) + it('kills an active gh execution when its caller aborts', async () => { + const child = createMockChildProcess(1234) + execFileMock.mockReturnValue(child) + const controller = new AbortController() + const promise = ghExecFileAsync(['api', 'repos/stablyai/orca/issues/5388'], { + cwd: '/repo', + signal: controller.signal + }) + const rejection = expect(promise).rejects.toMatchObject({ name: 'AbortError' }) + + controller.abort() + + await rejection + expect(child.kill).toHaveBeenCalled() + }) + it('honors explicit gh timeouts', async () => { const child = createMockChildProcess(1234) execFileMock.mockReturnValue(child) diff --git a/src/main/git/runner.ts b/src/main/git/runner.ts index ccde4a492f2..5371ab520d3 100644 --- a/src/main/git/runner.ts +++ b/src/main/git/runner.ts @@ -1438,8 +1438,24 @@ const GH_RETRY_DELAYS_MS = [250, 1000] as const const GH_RETRY_AFTER_MAX_MS = 30_000 const DEFAULT_GH_EXEC_TIMEOUT_MS = 30_000 -async function sleep(ms: number): Promise { - return new Promise((resolve) => setTimeout(resolve, ms)) +async function sleep(ms: number, signal?: AbortSignal): Promise { + if (signal?.aborted) { + throw createAbortError() + } + await new Promise((resolve, reject) => { + const timer = setTimeout(finish, ms) + const onAbort = (): void => finish(createAbortError()) + function finish(error?: Error): void { + clearTimeout(timer) + signal?.removeEventListener('abort', onAbort) + if (error) { + reject(error) + } else { + resolve() + } + } + signal?.addEventListener('abort', onAbort, { once: true }) + }) } function defaultGhExecTimeoutMs(env: NodeJS.ProcessEnv = process.env): number { @@ -1611,7 +1627,8 @@ export async function ghExecFileAsync( maxBuffer: options.maxBuffer, // Why: bound gh so one stuck child fails visibly instead of wedging the IPC lane. timeout: options.timeout ?? defaultGhExecTimeoutMs(options.env), - env: nonInteractiveGhEnv(options.env) + env: nonInteractiveGhEnv(options.env), + signal: options.signal }) return { stdout: stdout as string, stderr: stderr as string } } catch (err) { @@ -1653,7 +1670,7 @@ export async function ghExecFileAsync( retryAfterMs !== null ? Math.min(retryAfterMs, GH_RETRY_AFTER_MAX_MS) : GH_RETRY_DELAYS_MS[attempt] - await sleep(delayMs) + await sleep(delayMs, options.signal) continue } throw err diff --git a/src/main/github/client-pr-check-details.test.ts b/src/main/github/client-pr-check-details.test.ts index b092b01eb94..4bb2fcbae76 100644 --- a/src/main/github/client-pr-check-details.test.ts +++ b/src/main/github/client-pr-check-details.test.ts @@ -148,20 +148,86 @@ describe('getPRCheckDetails', () => { expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( 1, ['api', 'repos/acme/widgets/check-runs/88'], - { cwd: '/repo-root', host: 'github.com' } + expect.objectContaining({ + cwd: '/repo-root', + host: 'github.com', + signal: expect.any(AbortSignal) + }) ) expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( 2, ['api', 'repos/acme/widgets/check-runs/88/annotations?per_page=20'], - { cwd: '/repo-root', host: 'github.com' } + expect.objectContaining({ + cwd: '/repo-root', + host: 'github.com', + signal: expect.any(AbortSignal) + }) ) expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( 3, ['api', 'repos/acme/widgets/actions/runs/77/jobs?per_page=100'], - { cwd: '/repo-root', host: 'github.com' } + expect.objectContaining({ + cwd: '/repo-root', + host: 'github.com', + signal: expect.any(AbortSignal) + }) ) }) + it('rejects provider failures so callers can distinguish them from unavailable details', async () => { + getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) + ghExecFileAsyncMock.mockRejectedValueOnce(new Error('authentication failed')) + + await expect(getPRCheckDetails('/repo-root', { checkRunId: 88 })).rejects.toThrow( + 'authentication failed' + ) + }) + + it('aborts host work at the cumulative check-details deadline', async () => { + vi.useFakeTimers() + try { + getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) + ghExecFileAsyncMock.mockImplementation( + (_args: string[], options: { signal?: AbortSignal }) => + new Promise((_resolve, reject) => { + options.signal?.addEventListener( + 'abort', + () => { + const error = new Error('aborted') + error.name = 'AbortError' + reject(error) + }, + { once: true } + ) + }) + ) + const request = getPRCheckDetails('/repo-root', { checkRunId: 88 }) + const rejection = expect(request).rejects.toThrow('Timed out loading check details.') + + await vi.advanceTimersByTimeAsync(25_000) + + await rejection + } finally { + vi.useRealTimers() + } + }) + + it('stops waiting for shared repository resolution at the host deadline', async () => { + vi.useFakeTimers() + try { + getOwnerRepoMock.mockImplementationOnce(() => new Promise(() => {})) + const request = getPRCheckDetails('/repo-root', { checkRunId: 88 }) + const rejection = expect(request).rejects.toThrow('Timed out loading check details.') + + await vi.advanceTimersByTimeAsync(25_000) + + await rejection + expect(ghExecFileAsyncMock).not.toHaveBeenCalled() + } finally { + vi.useRealTimers() + } + }) + it('fetches sliced log tails for failed workflow jobs only', async () => { getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) const actionUrl = 'https://github.com/acme/widgets/actions/runs/77/job/88' @@ -225,7 +291,11 @@ describe('getPRCheckDetails', () => { expect(ghExecFileAsyncMock).toHaveBeenNthCalledWith( 4, ['api', 'repos/acme/widgets/actions/jobs/8801/logs'], - { cwd: '/repo-root', host: 'github.com' } + expect.objectContaining({ + cwd: '/repo-root', + host: 'github.com', + signal: expect.any(AbortSignal) + }) ) expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(4) }) diff --git a/src/main/github/client.ts b/src/main/github/client.ts index 4f95432fcfd..34f6db086a4 100644 --- a/src/main/github/client.ts +++ b/src/main/github/client.ts @@ -123,8 +123,12 @@ import { spendsSharedGitHubComQuota, type RateLimitBucketKind } from './rate-limit' +import { + GITHUB_CHECK_DETAILS_HOST_TIMEOUT_MS, + GITHUB_CHECK_DETAILS_TIMEOUT_MESSAGE +} from '../../shared/github-check-details-deadline' -type GhExecOptions = GitHubRepoExecOptions +type GhExecOptions = GitHubRepoExecOptions & { signal?: AbortSignal } type HostedReviewLocalGitOptions = ReturnType const ORCA_REPO = 'stablyai/orca' @@ -149,6 +153,30 @@ function setPrCheckLogTailCache(cacheKey: string, logTail: string | null): void prCheckLogTailCache.delete(oldestKey) } } + +function rethrowCheckDetailsAbort(signal: AbortSignal | undefined, error: unknown): void { + if (signal?.aborted) { + throw error + } +} + +function waitForCheckDetailsResolution(operation: Promise, signal: AbortSignal): Promise { + if (signal.aborted) { + return Promise.reject(signal.reason) + } + return new Promise((resolve, reject) => { + const finish = (settle: () => void): void => { + signal.removeEventListener('abort', onAbort) + settle() + } + const onAbort = (): void => finish(() => reject(signal.reason)) + signal.addEventListener('abort', onAbort, { once: true }) + void operation.then( + (value) => finish(() => resolve(value)), + (error) => finish(() => reject(error)) + ) + }) +} const MERGE_QUEUE_CACHE_TTL_MS = 10 * 60 * 1000 const MERGE_QUEUE_UNKNOWN_CACHE_TTL_MS = 60 * 1000 const MERGE_QUEUE_CACHE_MAX_ENTRIES = 256 @@ -3893,6 +3921,7 @@ async function attachFailedJobLogTails( ) job.logTail = sliceCheckLogTail(stdout) } catch (err) { + rethrowCheckDetailsAbort(ghOptions.signal, err) console.warn('getPRCheckDetails workflow job log fetch failed:', err) job.logTail = null } @@ -3925,20 +3954,34 @@ export async function getPRCheckDetails( prRepo?: GitHubApiRepository | null }, connectionId?: string | null, - localGitOptions: LocalGitExecOptions = {} + localGitOptions: LocalGitExecOptions = {}, + callerSignal?: AbortSignal ): Promise { - const { ownerRepo, ghOptions } = await resolveGitHubRepoExecution( - repoPath, - args.prRepo, - connectionId, - localGitOptions - ) - if (!ownerRepo) { - return null + const controller = new AbortController() + let hostDeadlineExpired = false + const forwardCallerAbort = (): void => controller.abort(callerSignal?.reason) + if (callerSignal?.aborted) { + forwardCallerAbort() + } else { + callerSignal?.addEventListener('abort', forwardCallerAbort, { once: true }) } - - await acquire() + const hostDeadline = setTimeout(() => { + hostDeadlineExpired = true + controller.abort(new Error(GITHUB_CHECK_DETAILS_TIMEOUT_MESSAGE)) + }, GITHUB_CHECK_DETAILS_HOST_TIMEOUT_MS) + let acquired = false try { + const resolved = await waitForCheckDetailsResolution( + resolveGitHubRepoExecution(repoPath, args.prRepo, connectionId, localGitOptions), + controller.signal + ) + if (!resolved.ownerRepo) { + return null + } + const ownerRepo = resolved.ownerRepo + const ghOptions: GhExecOptions = { ...resolved.ghOptions, signal: controller.signal } + await acquire(controller.signal) + acquired = true let checkRun: Record | null = null let annotations: PRCheckRunDetails['annotations'] = [] if (args.checkRunId) { @@ -3957,6 +4000,7 @@ export async function getPRCheckDetails( ) annotations = mapCheckAnnotations(JSON.parse(annotationsResult.stdout)) } catch (err) { + rethrowCheckDetailsAbort(controller.signal, err) console.warn('getPRCheckDetails annotations fetch failed:', err) } } @@ -3975,6 +4019,7 @@ export async function getPRCheckDetails( jobs = mapWorkflowJobs(JSON.parse(stdout), args.checkName) await attachFailedJobLogTails(jobs, ownerRepo, ghOptions) } catch (err) { + rethrowCheckDetailsAbort(controller.signal, err) console.warn('getPRCheckDetails workflow jobs fetch failed:', err) } } @@ -3999,9 +4044,16 @@ export async function getPRCheckDetails( } } catch (err) { console.warn('getPRCheckDetails failed:', err) - return null + if (hostDeadlineExpired && !callerSignal?.aborted) { + throw new Error(GITHUB_CHECK_DETAILS_TIMEOUT_MESSAGE) + } + throw err } finally { - release() + clearTimeout(hostDeadline) + callerSignal?.removeEventListener('abort', forwardCallerAbort) + if (acquired) { + release() + } } } diff --git a/src/main/github/gh-utils-concurrency.test.ts b/src/main/github/gh-utils-concurrency.test.ts new file mode 100644 index 00000000000..de6389d260a --- /dev/null +++ b/src/main/github/gh-utils-concurrency.test.ts @@ -0,0 +1,21 @@ +import { describe, expect, it } from 'vitest' +import { acquire, release } from './gh-utils' + +describe('GitHub concurrency', () => { + it('removes aborted waiters without consuming a permit', async () => { + await Promise.all([acquire(), acquire(), acquire(), acquire()]) + const controller = new AbortController() + const queued = acquire(controller.signal) + + controller.abort() + + await expect(queued).rejects.toMatchObject({ name: 'AbortError' }) + release() + await expect(acquire()).resolves.toBeUndefined() + + release() + release() + release() + release() + }) +}) diff --git a/src/main/github/gh-utils.ts b/src/main/github/gh-utils.ts index d6770189924..ffeb2ca22c7 100644 --- a/src/main/github/gh-utils.ts +++ b/src/main/github/gh-utils.ts @@ -40,25 +40,52 @@ export type { PRRepositoryCandidates, ResolvedIssueSource } from './github-owner const MAX_CONCURRENT = 4 let running = 0 -const queue: (() => void)[] = [] +type QueueEntry = { + signal?: AbortSignal + start: () => void + reject: (error: Error) => void +} +const queue: QueueEntry[] = [] -export function acquire(): Promise { +function githubOperationAbortError(): Error { + const error = new Error('GitHub operation aborted') + error.name = 'AbortError' + return error +} + +export function acquire(signal?: AbortSignal): Promise { + if (signal?.aborted) { + return Promise.reject(githubOperationAbortError()) + } if (running < MAX_CONCURRENT) { running += 1 return Promise.resolve() } - return new Promise((resolve) => - queue.push(() => { - running += 1 - resolve() - }) - ) + return new Promise((resolve, reject) => { + const entry: QueueEntry = { + signal, + reject, + start: () => { + signal?.removeEventListener('abort', onAbort) + running += 1 + resolve() + } + } + const onAbort = (): void => { + const index = queue.indexOf(entry) + if (index === -1) { + return + } + queue.splice(index, 1) + reject(githubOperationAbortError()) + } + signal?.addEventListener('abort', onAbort, { once: true }) + queue.push(entry) + }) } export function release(): void { running -= 1 const next = queue.shift() - if (next) { - next() - } + next?.start() } diff --git a/src/main/runtime/orca-runtime.test.ts b/src/main/runtime/orca-runtime.test.ts index 90a40f9da06..bef36832e11 100644 --- a/src/main/runtime/orca-runtime.test.ts +++ b/src/main/runtime/orca-runtime.test.ts @@ -6636,6 +6636,24 @@ describe('OrcaRuntimeService', () => { ) }) + it('forwards check-details cancellation without local Git overrides', async () => { + const runtime = new OrcaRuntimeService(store) + const signal = new AbortController().signal + + await runtime.getRepoPRCheckDetails('id:repo-1', { checkRunId: 9 }, signal) + + expect(getGitHubPRCheckDetailsMock).toHaveBeenCalledWith( + TEST_REPO_PATH, + { + checkRunId: 9, + prRepo: null + }, + null, + {}, + signal + ) + }) + it('routes runtime GitHub PR details and actions through the selected WSL project runtime', async () => { setPlatform('win32') const runtimeStore = { @@ -6659,6 +6677,7 @@ describe('OrcaRuntimeService', () => { const runtime = new OrcaRuntimeService(runtimeStore as never) const localGitOptions = { wslDistro: 'Ubuntu' } const prRepo = { owner: 'acme', repo: 'orca', host: 'github.acme.test' } + const checkDetailsSignal = new AbortController().signal await runtime.getRepoPRForBranch('id:repo-1', 'feature/wsl', 42, 43) await runtime.getRepoWorkItem('id:repo-1', 42, 'pr') @@ -6670,13 +6689,17 @@ describe('OrcaRuntimeService', () => { failedOnly: true, prRepo }) - await runtime.getRepoPRCheckDetails('id:repo-1', { - checkRunId: 9, - workflowRunId: 8, - checkName: 'lint', - url: 'https://example.com/check', - prRepo - }) + await runtime.getRepoPRCheckDetails( + 'id:repo-1', + { + checkRunId: 9, + workflowRunId: 8, + checkName: 'lint', + url: 'https://example.com/check', + prRepo + }, + checkDetailsSignal + ) await runtime.getRepoPRComments('id:repo-1', 42, prRepo, { noCache: true }) await runtime.getRepoPRFileContents('id:repo-1', { prNumber: 42, @@ -6778,7 +6801,8 @@ describe('OrcaRuntimeService', () => { prRepo }, null, - localGitOptions + localGitOptions, + checkDetailsSignal ) expect(getGitHubPRCommentsMock).toHaveBeenCalledWith( TEST_REPO_PATH, diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 10e143df938..44644d96ed9 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -19998,14 +19998,17 @@ export class OrcaRuntimeService { checkName?: string url?: string | null prRepo?: GitHubOwnerRepo | null - } + }, + signal?: AbortSignal ): Promise>> { const repo = await this.resolveRepoSelector(repoSelector) + const localGitOptions = this.getLocalGitExecutionOptionArgs(repo)[0] ?? {} return getPRCheckDetails( repo.path, { ...args, prRepo: args.prRepo ?? null }, repo.connectionId ?? null, - ...this.getLocalGitExecutionOptionArgs(repo) + localGitOptions, + signal ) } diff --git a/src/main/runtime/rpc/methods/github.test.ts b/src/main/runtime/rpc/methods/github.test.ts index 0a283b52072..72a4e150077 100644 --- a/src/main/runtime/rpc/methods/github.test.ts +++ b/src/main/runtime/rpc/methods/github.test.ts @@ -192,6 +192,32 @@ describe('github RPC methods', () => { expect(response).toMatchObject({ ok: true, result: [] }) }) + it('forwards request cancellation to PR check-details work', async () => { + const runtime = { + getRuntimeId: () => 'test-runtime', + getRepoPRCheckDetails: vi.fn().mockResolvedValue(null) + } as unknown as OrcaRuntimeService + const dispatcher = new RpcDispatcher({ runtime, methods: GITHUB_METHODS }) + const controller = new AbortController() + + await dispatcher.dispatch( + makeRequest('github.prCheckDetails', { repo: 'repo-1', checkRunId: 9 }), + { signal: controller.signal } + ) + + expect(runtime.getRepoPRCheckDetails).toHaveBeenCalledWith( + 'repo-1', + { + checkRunId: 9, + workflowRunId: undefined, + checkName: undefined, + url: undefined, + prRepo: null + }, + controller.signal + ) + }) + it('fetches PR comments on the runtime server with explicit PR repo', async () => { const runtime = { getRuntimeId: () => 'test-runtime', diff --git a/src/main/runtime/rpc/methods/github.ts b/src/main/runtime/rpc/methods/github.ts index ed715e4e0e8..c9f8935b128 100644 --- a/src/main/runtime/rpc/methods/github.ts +++ b/src/main/runtime/rpc/methods/github.ts @@ -439,14 +439,18 @@ export const GITHUB_METHODS: RpcMethod[] = [ defineMethod({ name: 'github.prCheckDetails', params: PullRequestCheckDetails, - handler: async (params, { runtime }) => - runtime.getRepoPRCheckDetails(params.repo, { - checkRunId: params.checkRunId, - workflowRunId: params.workflowRunId, - checkName: params.checkName, - url: params.url, - prRepo: params.prRepo ?? null - }) + handler: async (params, { runtime, signal }) => + runtime.getRepoPRCheckDetails( + params.repo, + { + checkRunId: params.checkRunId, + workflowRunId: params.workflowRunId, + checkName: params.checkName, + url: params.url, + prRepo: params.prRepo ?? null + }, + signal + ) }), defineMethod({ name: 'github.rerunPRChecks', diff --git a/src/renderer/src/components/GitHubItemDialog.tsx b/src/renderer/src/components/GitHubItemDialog.tsx index fc1f9505114..092ba2a7d0f 100644 --- a/src/renderer/src/components/GitHubItemDialog.tsx +++ b/src/renderer/src/components/GitHubItemDialog.tsx @@ -93,10 +93,12 @@ import { } from '@/components/editor/large-diff-section-content' import { CHECK_COLOR, CHECK_ICON } from '@/components/right-sidebar/checks-panel-content' import { + beginGitHubChecksTabDetails, createGitHubChecksTabState, + resetGitHubChecksTabForSource, resolveGitHubChecksTabState, + settleGitHubChecksTabDetails, toggleGitHubChecksTabExpandedKey, - updateGitHubChecksTabDetails, updateGitHubChecksTabLocalChecks, type CheckDetailsLoadState } from '@/components/github-checks-tab-state' @@ -3216,19 +3218,46 @@ function ChecksTab({ variant?: 'compact' | 'page' onChecksUpdated: (checks: PRCheckDetail[]) => void }): React.JSX.Element { - const [refreshing, setRefreshing] = useState(false) - const [rerunning, setRerunning] = useState(false) const [fixingChecks, setFixingChecks] = useState(false) - const [checksState, setChecksState] = useState(() => createGitHubChecksTabState(checks)) const mountedRef = useMountedRef() - const resolvedChecksState = resolveGitHubChecksTabState(checksState, checks) + const prRepo = useMemo(() => resolvePullRequestRepo(item), [item]) + const nextCheckDetailsRequestIdRef = useRef(0) + const checkDetailsContextKey = [ + sourceContext ? getTaskSourceCacheScope(sourceContext) : 'local', + repoId ?? item.repoId ?? '', + repoPath ?? '', + prRepo ? githubRepoIdentityKey(prRepo) : '', + item.id, + item.number, + headSha ?? '' + ].join('\0') + const [checksState, setChecksState] = useState(() => + createGitHubChecksTabState(checks, checkDetailsContextKey) + ) + const resolvedChecksState = resolveGitHubChecksTabState( + checksState, + checks, + checkDetailsContextKey + ) + const committedChecksContextOwnerRef = useRef(resolvedChecksState.contextOwner) + const nextChecksRefreshRequestIdRef = useRef(0) + const activeChecksRefreshRequestIdRef = useRef(null) + const [refreshingOwner, setRefreshingOwner] = useState<{ + contextOwner: object + requestId: number + } | null>(null) + const refreshing = refreshingOwner?.contextOwner === resolvedChecksState.contextOwner + const [rerunningOwner, setRerunningOwner] = useState(null) + const rerunning = rerunningOwner === resolvedChecksState.contextOwner + useLayoutEffect(() => { + committedChecksContextOwnerRef.current = resolvedChecksState.contextOwner + }, [resolvedChecksState.contextOwner]) if (resolvedChecksState !== checksState) { // Why: a parent check refresh replaces the source list; reset local state before stale rows/details can paint. setChecksState(resolvedChecksState) } const { localChecks, expandedCheckKey, detailsByCheckKey } = resolvedChecksState const list = useMemo(() => localChecks ?? checks ?? [], [checks, localChecks]) - const prRepo = useMemo(() => resolvePullRequestRepo(item), [item]) const runtimeHost = getGitHubSourceRuntimeHost(sourceContext) const canUseChecksRepoContext = canUseGitHubRepoContext(repoPath, sourceContext) const sorted = sortChecksBySeverity(list) @@ -3259,72 +3288,107 @@ function ChecksTab({ : 'text-muted-foreground' const canFixBrokenChecks = Boolean((repoId ?? item.repoId) && failedChecks.length > 0) - const handleRefresh = useCallback(async (): Promise => { - if (!canUseChecksRepoContext) { - toast.error( - translate( - 'auto.components.GitHubItemDialog.e7007aa1d8', - 'Unable to refresh checks without a repository path.' + const handleRefresh = useCallback( + async (expectedContextOwner?: object): Promise => { + if (!canUseChecksRepoContext) { + toast.error( + translate( + 'auto.components.GitHubItemDialog.e7007aa1d8', + 'Unable to refresh checks without a repository path.' + ) ) - ) - return null - } - setRefreshing(true) - try { - const nextChecks = (await (runtimeHost - ? callRuntimeRpc( - { kind: 'environment', environmentId: runtimeHost.environmentId }, - 'github.prChecks', - { - repo: getGitHubRuntimeRepoId(sourceContext, repoId ?? item.repoId), + return null + } + const refreshContextOwner = expectedContextOwner ?? committedChecksContextOwnerRef.current + if (committedChecksContextOwnerRef.current !== refreshContextOwner) { + return null + } + const refreshRequestId = ++nextChecksRefreshRequestIdRef.current + activeChecksRefreshRequestIdRef.current = refreshRequestId + setRefreshingOwner({ contextOwner: refreshContextOwner, requestId: refreshRequestId }) + try { + const nextChecks = (await (runtimeHost + ? callRuntimeRpc( + { kind: 'environment', environmentId: runtimeHost.environmentId }, + 'github.prChecks', + { + repo: getGitHubRuntimeRepoId(sourceContext, repoId ?? item.repoId), + prNumber: item.number, + headSha, + prRepo, + noCache: true + }, + { timeoutMs: 30_000 } + ) + : window.api.gh.prChecks({ + repoPath: repoPath ?? '', + repoId: repoId ?? undefined, + sourceContext, prNumber: item.number, headSha, prRepo, noCache: true - }, - { timeoutMs: 30_000 } + }))) as PRCheckDetail[] + if ( + !mountedRef.current || + committedChecksContextOwnerRef.current !== refreshContextOwner || + activeChecksRefreshRequestIdRef.current !== refreshRequestId + ) { + return null + } + setChecksState((current) => + current.contextOwner === refreshContextOwner + ? updateGitHubChecksTabLocalChecks(resetGitHubChecksTabForSource(current), nextChecks) + : current + ) + onChecksUpdated(nextChecks) + return nextChecks + } catch (err) { + if ( + mountedRef.current && + committedChecksContextOwnerRef.current === refreshContextOwner && + activeChecksRefreshRequestIdRef.current === refreshRequestId + ) { + toast.error( + err instanceof Error + ? err.message + : translate('auto.components.GitHubItemDialog.0bbdc673c1', 'Failed to refresh checks') ) - : window.api.gh.prChecks({ - repoPath: repoPath ?? '', - repoId: repoId ?? undefined, - sourceContext, - prNumber: item.number, - headSha, - prRepo, - noCache: true - }))) as PRCheckDetail[] - setChecksState((current) => updateGitHubChecksTabLocalChecks(current, nextChecks)) - onChecksUpdated(nextChecks) - return nextChecks - } catch (err) { - toast.error( - err instanceof Error - ? err.message - : translate('auto.components.GitHubItemDialog.0bbdc673c1', 'Failed to refresh checks') - ) - return null - } finally { - setRefreshing(false) - } - }, [ - canUseChecksRepoContext, - headSha, - item.number, - item.repoId, - onChecksUpdated, - runtimeHost, - prRepo, - repoId, - repoPath, - sourceContext - ]) + } + return null + } finally { + if (activeChecksRefreshRequestIdRef.current === refreshRequestId) { + activeChecksRefreshRequestIdRef.current = null + } + if (mountedRef.current) { + setRefreshingOwner((current) => + current?.requestId === refreshRequestId ? null : current + ) + } + } + }, + [ + canUseChecksRepoContext, + headSha, + item.number, + item.repoId, + mountedRef, + onChecksUpdated, + runtimeHost, + prRepo, + repoId, + repoPath, + sourceContext + ] + ) const handleRerun = useCallback( async (failedOnly: boolean): Promise => { if (!canUseChecksRepoContext || rerunning) { return } - setRerunning(true) + const rerunContextOwner = committedChecksContextOwnerRef.current + setRerunningOwner(rerunContextOwner) try { const result = runtimeHost ? await callRuntimeRpc>>( @@ -3348,6 +3412,9 @@ function ChecksTab({ failedOnly, prRepo }) + if (!mountedRef.current || committedChecksContextOwnerRef.current !== rerunContextOwner) { + return + } if (!result.ok) { toast.error(result.error) return @@ -3357,15 +3424,19 @@ function ChecksTab({ ? translate('auto.components.GitHubItemDialog.ddafe851e1', 'Check rerun requested') : translate('auto.components.GitHubItemDialog.e463ec935f', 'Check reruns requested') ) - await handleRefresh() + await handleRefresh(rerunContextOwner) } catch (err) { - toast.error( - err instanceof Error - ? err.message - : translate('auto.components.GitHubItemDialog.9e7c221b8d', 'Failed to rerun checks') - ) + if (mountedRef.current && committedChecksContextOwnerRef.current === rerunContextOwner) { + toast.error( + err instanceof Error + ? err.message + : translate('auto.components.GitHubItemDialog.9e7c221b8d', 'Failed to rerun checks') + ) + } } finally { - setRerunning(false) + if (mountedRef.current) { + setRerunningOwner((current) => (current === rerunContextOwner ? null : current)) + } } }, [ @@ -3374,6 +3445,7 @@ function ChecksTab({ headSha, item.number, item.repoId, + mountedRef, prRepo, runtimeHost, rerunning, @@ -3442,40 +3514,35 @@ function ChecksTab({ } }, [failedChecks.length, fixingChecks, item, list, repoId]) - const handleToggleCheckDetails = useCallback( - (check: PRCheckDetail): void => { - const key = getCheckDetailsKey(check) - setChecksState((current) => toggleGitHubChecksTabExpandedKey(current, key)) - if ( - !canUseChecksRepoContext || - detailsByCheckKey[key] || - (!check.checkRunId && !check.workflowRunId && !check.url) - ) { + const requestCheckDetails = useCallback( + (check: PRCheckDetail, key: string): void => { + if (!canUseChecksRepoContext || (!check.checkRunId && !check.workflowRunId && !check.url)) { return } - setChecksState((current) => - updateGitHubChecksTabDetails(current, key, { - loading: true, - details: null, - error: null - }) - ) - const detailsRequest = runtimeHost - ? callRuntimeRpc>>( - { kind: 'environment', environmentId: runtimeHost.environmentId }, - 'github.prCheckDetails', - { - repo: getGitHubRuntimeRepoId(sourceContext, repoId ?? item.repoId), - checkRunId: check.checkRunId, - workflowRunId: check.workflowRunId, - checkName: check.name, - url: check.url, - prRepo - }, - { timeoutMs: 30_000 } - ) - : withGitHubCheckDetailsTimeout( - window.api.gh.prCheckDetails({ + const requestId = ++nextCheckDetailsRequestIdRef.current + const commit = (next: Omit): void => { + if (!mountedRef.current) { + return + } + setChecksState((current) => settleGitHubChecksTabDetails(current, key, requestId, next)) + } + setChecksState((current) => beginGitHubChecksTabDetails(current, key, requestId)) + const detailsRequest = withGitHubCheckDetailsTimeout((signal) => + runtimeHost + ? callRuntimeRpc>>( + { kind: 'environment', environmentId: runtimeHost.environmentId }, + 'github.prCheckDetails', + { + repo: getGitHubRuntimeRepoId(sourceContext, repoId ?? item.repoId), + checkRunId: check.checkRunId, + workflowRunId: check.workflowRunId, + checkName: check.name, + url: check.url, + prRepo + }, + { timeoutMs: 30_000, signal } + ) + : window.api.gh.prCheckDetails({ repoPath: repoPath ?? '', repoId: repoId ?? undefined, sourceContext, @@ -3485,36 +3552,36 @@ function ChecksTab({ url: check.url, prRepo }) - ) + ) void detailsRequest .then((details) => { - if (!mountedRef.current) { - return - } - setChecksState((current) => - updateGitHubChecksTabDetails(current, key, { - loading: false, - details, - error: details ? null : 'No inline details are available for this check.' - }) - ) + commit({ + loading: false, + details, + error: details + ? null + : translate( + 'auto.components.GitHubItemDialog.e15a8b77ef', + 'No inline details are available for this check.' + ) + }) }) .catch((err) => { - if (!mountedRef.current) { - return - } - setChecksState((current) => - updateGitHubChecksTabDetails(current, key, { - loading: false, - details: null, - error: err instanceof Error ? err.message : 'Failed to load check details.' - }) - ) + commit({ + loading: false, + details: null, + error: + err instanceof Error + ? err.message + : translate( + 'auto.components.GitHubItemDialog.e45324fbed', + 'Failed to load check details.' + ) + }) }) }, [ canUseChecksRepoContext, - detailsByCheckKey, item.repoId, mountedRef, runtimeHost, @@ -3525,6 +3592,18 @@ function ChecksTab({ ] ) + const handleToggleCheckDetails = useCallback( + (check: PRCheckDetail): void => { + const key = getCheckDetailsKey(check) + setChecksState((current) => toggleGitHubChecksTabExpandedKey(current, key)) + if (detailsByCheckKey[key]) { + return + } + requestCheckDetails(check, key) + }, + [detailsByCheckKey, requestCheckDetails] + ) + const refreshAction = ( @@ -3718,7 +3797,7 @@ function ChecksTab({ return (
- {state?.loading ? ( + {state?.loading && !state.error ? (
{translate('auto.components.GitHubItemDialog.934d87ab96', 'Loading check details…')} @@ -3749,7 +3828,27 @@ function ChecksTab({ )}
- {state?.error &&
{state.error}
} + {state?.error && ( +
+ + {state.error} + + +
+ )} {hasOutput && (
diff --git a/src/renderer/src/components/PullRequestPage.tsx b/src/renderer/src/components/PullRequestPage.tsx index 4ef21776947..33cfeb2f8c6 100644 --- a/src/renderer/src/components/PullRequestPage.tsx +++ b/src/renderer/src/components/PullRequestPage.tsx @@ -89,10 +89,12 @@ import { import { CHECK_COLOR, CHECK_ICON } from '@/components/right-sidebar/checks-panel-content' import { SourceControlAgentActionDialog } from '@/components/right-sidebar/SourceControlAgentActionDialog' import { + beginGitHubChecksTabDetails, createGitHubChecksTabState, + resetGitHubChecksTabForSource, resolveGitHubChecksTabState, + settleGitHubChecksTabDetails, toggleGitHubChecksTabExpandedKey, - updateGitHubChecksTabDetails, updateGitHubChecksTabLocalChecks, type CheckDetailsLoadState } from '@/components/github-checks-tab-state' @@ -3218,13 +3220,41 @@ function ChecksTab({ const repo = useAppStore((s) => targetRepoId ? (s.repos.find((candidate) => candidate.id === targetRepoId) ?? null) : null ) - const [refreshing, setRefreshing] = useState(false) - const [rerunning, setRerunning] = useState(false) const [fixingChecks, setFixingChecks] = useState(false) const [fixChecksComposerPrompt, setFixChecksComposerPrompt] = useState(null) - const [checksState, setChecksState] = useState(() => createGitHubChecksTabState(checks)) const mountedRef = useMountedRef() - const resolvedChecksState = resolveGitHubChecksTabState(checksState, checks) + const prRepo = useMemo(() => resolvePullRequestRepo(item), [item]) + const nextCheckDetailsRequestIdRef = useRef(0) + const checkDetailsContextKey = [ + sourceContext ? getTaskSourceCacheScope(sourceContext) : 'local', + repoId ?? item.repoId ?? '', + repoPath ?? '', + prRepo ? githubRepoIdentityKey(prRepo) : '', + item.id, + item.number, + headSha ?? '' + ].join('\0') + const [checksState, setChecksState] = useState(() => + createGitHubChecksTabState(checks, checkDetailsContextKey) + ) + const resolvedChecksState = resolveGitHubChecksTabState( + checksState, + checks, + checkDetailsContextKey + ) + const committedChecksContextOwnerRef = useRef(resolvedChecksState.contextOwner) + const nextChecksRefreshRequestIdRef = useRef(0) + const activeChecksRefreshRequestIdRef = useRef(null) + const [refreshingOwner, setRefreshingOwner] = useState<{ + contextOwner: object + requestId: number + } | null>(null) + const refreshing = refreshingOwner?.contextOwner === resolvedChecksState.contextOwner + const [rerunningOwner, setRerunningOwner] = useState(null) + const rerunning = rerunningOwner === resolvedChecksState.contextOwner + useLayoutEffect(() => { + committedChecksContextOwnerRef.current = resolvedChecksState.contextOwner + }, [resolvedChecksState.contextOwner]) if (resolvedChecksState !== checksState) { // Why: reconcile before paint when a parent check refresh replaces the source list, so stale rows/details never show. setChecksState(resolvedChecksState) @@ -3318,7 +3348,6 @@ function ChecksTab({ }, [item, targetRepoId] ) - const prRepo = useMemo(() => resolvePullRequestRepo(item), [item]) const runtimeHost = getGitHubSourceRuntimeHost(sourceContext) const canUseChecksRepoContext = canUseGitHubRepoContext(repoPath, sourceContext) const sorted = sortChecksBySeverity(list) @@ -3349,72 +3378,107 @@ function ChecksTab({ : 'text-muted-foreground' const canFixBrokenChecks = Boolean((repoId ?? item.repoId) && failedChecks.length > 0) - const handleRefresh = useCallback(async (): Promise => { - if (!canUseChecksRepoContext) { - toast.error( - translate( - 'auto.components.PullRequestPage.c057f2fcb0', - 'Unable to refresh checks without a repository path.' + const handleRefresh = useCallback( + async (expectedContextOwner?: object): Promise => { + if (!canUseChecksRepoContext) { + toast.error( + translate( + 'auto.components.PullRequestPage.c057f2fcb0', + 'Unable to refresh checks without a repository path.' + ) ) - ) - return null - } - setRefreshing(true) - try { - const nextChecks = (await (runtimeHost - ? callRuntimeRpc( - { kind: 'environment', environmentId: runtimeHost.environmentId }, - 'github.prChecks', - { - repo: getGitHubRuntimeRepoId(sourceContext, repoId ?? item.repoId), + return null + } + const refreshContextOwner = expectedContextOwner ?? committedChecksContextOwnerRef.current + if (committedChecksContextOwnerRef.current !== refreshContextOwner) { + return null + } + const refreshRequestId = ++nextChecksRefreshRequestIdRef.current + activeChecksRefreshRequestIdRef.current = refreshRequestId + setRefreshingOwner({ contextOwner: refreshContextOwner, requestId: refreshRequestId }) + try { + const nextChecks = (await (runtimeHost + ? callRuntimeRpc( + { kind: 'environment', environmentId: runtimeHost.environmentId }, + 'github.prChecks', + { + repo: getGitHubRuntimeRepoId(sourceContext, repoId ?? item.repoId), + prNumber: item.number, + headSha, + prRepo, + noCache: true + }, + { timeoutMs: 30_000 } + ) + : window.api.gh.prChecks({ + repoPath: repoPath ?? '', + repoId: repoId ?? undefined, + sourceContext, prNumber: item.number, headSha, prRepo, noCache: true - }, - { timeoutMs: 30_000 } + }))) as PRCheckDetail[] + if ( + !mountedRef.current || + committedChecksContextOwnerRef.current !== refreshContextOwner || + activeChecksRefreshRequestIdRef.current !== refreshRequestId + ) { + return null + } + setChecksState((current) => + current.contextOwner === refreshContextOwner + ? updateGitHubChecksTabLocalChecks(resetGitHubChecksTabForSource(current), nextChecks) + : current + ) + onChecksUpdated(nextChecks) + return nextChecks + } catch (err) { + if ( + mountedRef.current && + committedChecksContextOwnerRef.current === refreshContextOwner && + activeChecksRefreshRequestIdRef.current === refreshRequestId + ) { + toast.error( + err instanceof Error + ? err.message + : translate('auto.components.PullRequestPage.246b2c6456', 'Failed to refresh checks') ) - : window.api.gh.prChecks({ - repoPath: repoPath ?? '', - repoId: repoId ?? undefined, - sourceContext, - prNumber: item.number, - headSha, - prRepo, - noCache: true - }))) as PRCheckDetail[] - setChecksState((current) => updateGitHubChecksTabLocalChecks(current, nextChecks)) - onChecksUpdated(nextChecks) - return nextChecks - } catch (err) { - toast.error( - err instanceof Error - ? err.message - : translate('auto.components.PullRequestPage.246b2c6456', 'Failed to refresh checks') - ) - return null - } finally { - setRefreshing(false) - } - }, [ - canUseChecksRepoContext, - headSha, - item.number, - item.repoId, - onChecksUpdated, - runtimeHost, - prRepo, - repoId, - repoPath, - sourceContext - ]) + } + return null + } finally { + if (activeChecksRefreshRequestIdRef.current === refreshRequestId) { + activeChecksRefreshRequestIdRef.current = null + } + if (mountedRef.current) { + setRefreshingOwner((current) => + current?.requestId === refreshRequestId ? null : current + ) + } + } + }, + [ + canUseChecksRepoContext, + headSha, + item.number, + item.repoId, + mountedRef, + onChecksUpdated, + runtimeHost, + prRepo, + repoId, + repoPath, + sourceContext + ] + ) const handleRerun = useCallback( async (failedOnly: boolean): Promise => { if (!canUseChecksRepoContext || rerunning) { return } - setRerunning(true) + const rerunContextOwner = committedChecksContextOwnerRef.current + setRerunningOwner(rerunContextOwner) try { const result = runtimeHost ? await callRuntimeRpc>>( @@ -3438,6 +3502,9 @@ function ChecksTab({ failedOnly, prRepo }) + if (!mountedRef.current || committedChecksContextOwnerRef.current !== rerunContextOwner) { + return + } if (!result.ok) { toast.error(result.error) return @@ -3447,15 +3514,19 @@ function ChecksTab({ ? translate('auto.components.PullRequestPage.5963a6a852', 'Check rerun requested') : translate('auto.components.PullRequestPage.18f2af42ac', 'Check reruns requested') ) - await handleRefresh() + await handleRefresh(rerunContextOwner) } catch (err) { - toast.error( - err instanceof Error - ? err.message - : translate('auto.components.PullRequestPage.788a782bb0', 'Failed to rerun checks') - ) + if (mountedRef.current && committedChecksContextOwnerRef.current === rerunContextOwner) { + toast.error( + err instanceof Error + ? err.message + : translate('auto.components.PullRequestPage.788a782bb0', 'Failed to rerun checks') + ) + } } finally { - setRerunning(false) + if (mountedRef.current) { + setRerunningOwner((current) => (current === rerunContextOwner ? null : current)) + } } }, [ @@ -3464,6 +3535,7 @@ function ChecksTab({ headSha, item.number, item.repoId, + mountedRef, prRepo, runtimeHost, rerunning, @@ -3526,40 +3598,29 @@ function ChecksTab({ } }, [failedChecks.length, fixingChecks, item, list, targetRepoId]) - const handleToggleCheckDetails = useCallback( - (check: PRCheckDetail): void => { - const key = getCheckDetailsKey(check) - setChecksState((current) => toggleGitHubChecksTabExpandedKey(current, key)) - if ( - !canUseChecksRepoContext || - detailsByCheckKey[key] || - (!check.checkRunId && !check.workflowRunId && !check.url) - ) { + const requestCheckDetails = useCallback( + (check: PRCheckDetail, key: string): void => { + if (!canUseChecksRepoContext || (!check.checkRunId && !check.workflowRunId && !check.url)) { return } - setChecksState((current) => - updateGitHubChecksTabDetails(current, key, { - loading: true, - details: null, - error: null - }) - ) - const detailsRequest = runtimeHost - ? callRuntimeRpc>>( - { kind: 'environment', environmentId: runtimeHost.environmentId }, - 'github.prCheckDetails', - { - repo: getGitHubRuntimeRepoId(sourceContext, repoId ?? item.repoId), - checkRunId: check.checkRunId, - workflowRunId: check.workflowRunId, - checkName: check.name, - url: check.url, - prRepo - }, - { timeoutMs: 30_000 } - ) - : withGitHubCheckDetailsTimeout( - window.api.gh.prCheckDetails({ + const requestId = ++nextCheckDetailsRequestIdRef.current + setChecksState((current) => beginGitHubChecksTabDetails(current, key, requestId)) + const detailsRequest = withGitHubCheckDetailsTimeout((signal) => + runtimeHost + ? callRuntimeRpc>>( + { kind: 'environment', environmentId: runtimeHost.environmentId }, + 'github.prCheckDetails', + { + repo: getGitHubRuntimeRepoId(sourceContext, repoId ?? item.repoId), + checkRunId: check.checkRunId, + workflowRunId: check.workflowRunId, + checkName: check.name, + url: check.url, + prRepo + }, + { timeoutMs: 30_000, signal } + ) + : window.api.gh.prCheckDetails({ repoPath: repoPath ?? '', repoId: repoId ?? undefined, sourceContext, @@ -3569,17 +3630,22 @@ function ChecksTab({ url: check.url, prRepo }) - ) + ) void detailsRequest .then((details) => { if (!mountedRef.current) { return } setChecksState((current) => - updateGitHubChecksTabDetails(current, key, { + settleGitHubChecksTabDetails(current, key, requestId, { loading: false, details, - error: details ? null : 'No inline details are available for this check.' + error: details + ? null + : translate( + 'auto.components.PullRequestPage.6b1d5ee3e4', + 'No inline details are available for this check.' + ) }) ) }) @@ -3588,17 +3654,22 @@ function ChecksTab({ return } setChecksState((current) => - updateGitHubChecksTabDetails(current, key, { + settleGitHubChecksTabDetails(current, key, requestId, { loading: false, details: null, - error: err instanceof Error ? err.message : 'Failed to load check details.' + error: + err instanceof Error + ? err.message + : translate( + 'auto.components.PullRequestPage.e04c027d98', + 'Failed to load check details.' + ) }) ) }) }, [ canUseChecksRepoContext, - detailsByCheckKey, item.repoId, mountedRef, runtimeHost, @@ -3609,6 +3680,18 @@ function ChecksTab({ ] ) + const handleToggleCheckDetails = useCallback( + (check: PRCheckDetail): void => { + const key = getCheckDetailsKey(check) + setChecksState((current) => toggleGitHubChecksTabExpandedKey(current, key)) + if (detailsByCheckKey[key]) { + return + } + requestCheckDetails(check, key) + }, + [detailsByCheckKey, requestCheckDetails] + ) + const refreshAction = ( @@ -3802,7 +3885,7 @@ function ChecksTab({ return (
- {state?.loading ? ( + {state?.loading && !state.error ? (
{translate('auto.components.PullRequestPage.d8e82b7f15', 'Loading check details…')} @@ -3833,7 +3916,27 @@ function ChecksTab({ )}
- {state?.error &&
{state.error}
} + {state?.error && ( +
+
+ {state.error} +
+ +
+ )} {hasOutput && (
diff --git a/src/renderer/src/components/github-checks-tab-state.test.ts b/src/renderer/src/components/github-checks-tab-state.test.ts index e4e9a63bae9..a502d0b9a2b 100644 --- a/src/renderer/src/components/github-checks-tab-state.test.ts +++ b/src/renderer/src/components/github-checks-tab-state.test.ts @@ -1,8 +1,10 @@ import { describe, expect, it } from 'vitest' import type { PRCheckDetail } from '../../../shared/types' import { + beginGitHubChecksTabDetails, createGitHubChecksTabState, resolveGitHubChecksTabState, + settleGitHubChecksTabDetails, toggleGitHubChecksTabExpandedKey, updateGitHubChecksTabDetails, updateGitHubChecksTabLocalChecks @@ -18,11 +20,12 @@ const check = (name: string): PRCheckDetail => ({ describe('github checks tab state', () => { it('preserves local check state while the source checks reference is unchanged', () => { const sourceChecks = [check('unit')] - const state = updateGitHubChecksTabLocalChecks(createGitHubChecksTabState(sourceChecks), [ - check('refreshed') - ]) + const state = updateGitHubChecksTabLocalChecks( + createGitHubChecksTabState(sourceChecks, 'repo-a'), + [check('refreshed')] + ) - expect(resolveGitHubChecksTabState(state, sourceChecks)).toBe(state) + expect(resolveGitHubChecksTabState(state, sourceChecks, 'repo-a')).toBe(state) }) it('resets local checks and expanded details when source checks change', () => { @@ -30,14 +33,18 @@ describe('github checks tab state', () => { const nextSource = [check('next')] const stateWithDetails = updateGitHubChecksTabDetails( toggleGitHubChecksTabExpandedKey( - updateGitHubChecksTabLocalChecks(createGitHubChecksTabState(oldSource), [check('local')]), + updateGitHubChecksTabLocalChecks(createGitHubChecksTabState(oldSource, 'repo-a'), [ + check('local') + ]), 'unit' ), 'unit', { loading: true, details: null, error: null } ) - expect(resolveGitHubChecksTabState(stateWithDetails, nextSource)).toEqual({ + expect(resolveGitHubChecksTabState(stateWithDetails, nextSource, 'repo-a')).toEqual({ + contextKey: 'repo-a', + contextOwner: stateWithDetails.contextOwner, sourceChecks: nextSource, localChecks: null, expandedCheckKey: null, @@ -47,11 +54,15 @@ describe('github checks tab state', () => { it('toggles expanded check keys without discarding loaded details', () => { const sourceChecks = [check('unit')] - const state = updateGitHubChecksTabDetails(createGitHubChecksTabState(sourceChecks), 'unit', { - loading: false, - details: null, - error: 'No details' - }) + const state = updateGitHubChecksTabDetails( + createGitHubChecksTabState(sourceChecks, 'repo-a'), + 'unit', + { + loading: false, + details: null, + error: 'No details' + } + ) const expanded = toggleGitHubChecksTabExpandedKey(state, 'unit') const collapsed = toggleGitHubChecksTabExpandedKey(expanded, 'unit') @@ -60,4 +71,124 @@ describe('github checks tab state', () => { expect(collapsed.expandedCheckKey).toBeNull() expect(collapsed.detailsByCheckKey).toBe(state.detailsByCheckKey) }) + + it('settles details only for the request that still owns the check key', () => { + const sourceChecks = [check('unit')] + const loading = beginGitHubChecksTabDetails( + createGitHubChecksTabState(sourceChecks, 'repo-a'), + 'unit', + 2 + ) + + expect( + settleGitHubChecksTabDetails(loading, 'unit', 1, { + loading: false, + details: null, + error: 'stale' + }) + ).toBe(loading) + expect( + settleGitHubChecksTabDetails(loading, 'unit', 2, { + loading: false, + details: null, + error: 'current' + }).detailsByCheckKey.unit + ).toEqual({ requestId: 2, loading: false, details: null, error: 'current' }) + }) + + it('gives a retry ownership over an older in-flight request', () => { + const sourceChecks = [check('unit')] + const first = beginGitHubChecksTabDetails( + createGitHubChecksTabState(sourceChecks, 'repo-a'), + 'unit', + 1 + ) + const retry = beginGitHubChecksTabDetails(first, 'unit', 2) + + expect(retry.detailsByCheckKey.unit).toEqual({ + requestId: 2, + loading: true, + details: null, + error: null + }) + + expect( + settleGitHubChecksTabDetails(retry, 'unit', 1, { + loading: false, + details: null, + error: 'old failure' + }) + ).toBe(retry) + expect( + settleGitHubChecksTabDetails(retry, 'unit', 2, { + loading: false, + details: null, + error: 'retry failure' + }).detailsByCheckKey.unit + ).toEqual({ requestId: 2, loading: false, details: null, error: 'retry failure' }) + }) + + it('keeps a retry error visible while the replacement request is loading', () => { + const sourceChecks = [check('unit')] + const failed = settleGitHubChecksTabDetails( + beginGitHubChecksTabDetails(createGitHubChecksTabState(sourceChecks, 'repo-a'), 'unit', 1), + 'unit', + 1, + { loading: false, details: null, error: 'first failure' } + ) + + expect(beginGitHubChecksTabDetails(failed, 'unit', 2).detailsByCheckKey.unit).toEqual({ + requestId: 2, + loading: true, + details: null, + error: 'first failure' + }) + }) + + it('drops settlement after a source refresh clears request ownership', () => { + const oldSource = [check('old')] + const loading = updateGitHubChecksTabDetails( + createGitHubChecksTabState(oldSource, 'repo-a'), + 'unit', + { + requestId: 1, + loading: true, + details: null, + error: null + } + ) + const refreshed = resolveGitHubChecksTabState(loading, [check('new')], 'repo-a') + + expect(refreshed.contextOwner).toBe(loading.contextOwner) + + expect( + settleGitHubChecksTabDetails(refreshed, 'unit', 1, { + loading: false, + details: null, + error: 'stale' + }) + ).toBe(refreshed) + }) + + it('resets request ownership when the context changes with the same checks array', () => { + const sourceChecks = [check('unit')] + const loading = updateGitHubChecksTabDetails( + createGitHubChecksTabState(sourceChecks, 'repo-a'), + 'unit', + { requestId: 1, loading: true, details: null, error: null } + ) + + const nextContext = resolveGitHubChecksTabState(loading, sourceChecks, 'repo-b') + expect(nextContext).toEqual({ + contextKey: 'repo-b', + contextOwner: expect.any(Object), + sourceChecks, + localChecks: null, + expandedCheckKey: null, + detailsByCheckKey: {} + }) + const revisited = resolveGitHubChecksTabState(nextContext, sourceChecks, 'repo-a') + expect(revisited.contextOwner).not.toBe(loading.contextOwner) + expect(revisited.contextOwner).not.toBe(nextContext.contextOwner) + }) }) diff --git a/src/renderer/src/components/github-checks-tab-state.ts b/src/renderer/src/components/github-checks-tab-state.ts index 2995af7d0b9..10dec028259 100644 --- a/src/renderer/src/components/github-checks-tab-state.ts +++ b/src/renderer/src/components/github-checks-tab-state.ts @@ -1,12 +1,15 @@ import type { PRCheckDetail, PRCheckRunDetails } from '../../../shared/types' export type CheckDetailsLoadState = { + requestId?: number loading: boolean details: PRCheckRunDetails | null error: string | null } export type GitHubChecksTabState = { + contextKey: string + contextOwner: object sourceChecks: GitHubChecksSource localChecks: PRCheckDetail[] | null expandedCheckKey: string | null @@ -15,8 +18,13 @@ export type GitHubChecksTabState = { type GitHubChecksSource = readonly PRCheckDetail[] | null | undefined -export function createGitHubChecksTabState(sourceChecks: GitHubChecksSource): GitHubChecksTabState { +export function createGitHubChecksTabState( + sourceChecks: GitHubChecksSource, + contextKey: string +): GitHubChecksTabState { return { + contextKey, + contextOwner: {}, sourceChecks, localChecks: null, expandedCheckKey: null, @@ -26,9 +34,29 @@ export function createGitHubChecksTabState(sourceChecks: GitHubChecksSource): Gi export function resolveGitHubChecksTabState( state: GitHubChecksTabState, - sourceChecks: GitHubChecksSource + sourceChecks: GitHubChecksSource, + contextKey: string ): GitHubChecksTabState { - return state.sourceChecks === sourceChecks ? state : createGitHubChecksTabState(sourceChecks) + if (state.contextKey !== contextKey) { + return createGitHubChecksTabState(sourceChecks, contextKey) + } + return state.sourceChecks === sourceChecks + ? state + : resetGitHubChecksTabForSource(state, sourceChecks) +} + +export function resetGitHubChecksTabForSource( + state: GitHubChecksTabState, + sourceChecks: GitHubChecksSource = state.sourceChecks +): GitHubChecksTabState { + return { + contextKey: state.contextKey, + contextOwner: state.contextOwner, + sourceChecks, + localChecks: null, + expandedCheckKey: null, + detailsByCheckKey: {} + } } export function updateGitHubChecksTabLocalChecks( @@ -64,3 +92,29 @@ export function updateGitHubChecksTabDetails( } } } + +export function beginGitHubChecksTabDetails( + state: GitHubChecksTabState, + key: string, + requestId: number +): GitHubChecksTabState { + const current = state.detailsByCheckKey[key] + return updateGitHubChecksTabDetails(state, key, { + requestId, + loading: true, + details: null, + error: current?.error ?? null + }) +} + +export function settleGitHubChecksTabDetails( + state: GitHubChecksTabState, + key: string, + requestId: number, + details: Omit +): GitHubChecksTabState { + if (state.detailsByCheckKey[key]?.requestId !== requestId) { + return state + } + return updateGitHubChecksTabDetails(state, key, { ...details, requestId }) +} diff --git a/src/renderer/src/components/github-item-dialog-source-boundary.test.ts b/src/renderer/src/components/github-item-dialog-source-boundary.test.ts index 265aeaf9417..6c487a5d140 100644 --- a/src/renderer/src/components/github-item-dialog-source-boundary.test.ts +++ b/src/renderer/src/components/github-item-dialog-source-boundary.test.ts @@ -231,8 +231,41 @@ describe('GitHubItemDialog source host boundaries', () => { expect(checksSection).toContain('window.api.gh.rerunPRChecks({') expect(checksSection).toContain('prCheckDetails({') expect(checksSection).toMatch( - /withGitHubCheckDetailsTimeout\(\s*window\.api\.gh\.prCheckDetails\(\{/ + /withGitHubCheckDetailsTimeout\(\(signal\) =>\s*runtimeHost\s*\?\s*callRuntimeRpc[\s\S]*:\s*window\.api\.gh\.prCheckDetails\(\{/ ) + expect(checksSection).toContain('{ timeoutMs: 30_000, signal }') + }) + + it('makes failed check detail loads retryable and fences stale responses', () => { + const source = componentSource('GitHubItemDialog.tsx') + const checksSection = sourceBetween( + source, + 'function ChecksTab', + 'function GitHubLabelsSettingsLink' + ) + + expect(checksSection).toContain('createGitHubChecksTabState(checks, checkDetailsContextKey)') + expect(checksSection).toContain('checksState,\n checks,\n checkDetailsContextKey') + expect(checksSection).toContain('resetGitHubChecksTabForSource(current)') + expect(checksSection).toContain( + 'committedChecksContextOwnerRef.current !== refreshContextOwner' + ) + expect(checksSection).toContain('activeChecksRefreshRequestIdRef.current !== refreshRequestId') + expect(checksSection).toContain('current.contextOwner === refreshContextOwner') + expect(checksSection).toContain( + 'const rerunContextOwner = committedChecksContextOwnerRef.current' + ) + expect(checksSection).toContain('committedChecksContextOwnerRef.current !== rerunContextOwner') + expect(checksSection).toContain('await handleRefresh(rerunContextOwner)') + expect(checksSection).toContain('!mountedRef.current ||') + expect(checksSection).toContain('settleGitHubChecksTabDetails(current, key, requestId, next)') + expect(checksSection).toContain( + 'onClick={() => requestCheckDetails(check, getCheckDetailsKey(check))}' + ) + expect(checksSection).toContain('disabled={state.loading}') + expect(checksSection).toContain('aria-busy={state.loading}') + expect(checksSection).toContain("translate('githubChecks.retrying', 'Retrying…')") + expect(checksSection).toContain("'Retry'") }) it('uses hydrated work item details for the page checks tab', () => { diff --git a/src/renderer/src/components/pull-request-page-host-boundary.test.ts b/src/renderer/src/components/pull-request-page-host-boundary.test.ts index 84c391b92f0..223df6300d8 100644 --- a/src/renderer/src/components/pull-request-page-host-boundary.test.ts +++ b/src/renderer/src/components/pull-request-page-host-boundary.test.ts @@ -250,7 +250,39 @@ describe('PullRequestPage host boundaries', () => { expect(checksSection).toContain('window.api.gh.rerunPRChecks({') expect(checksSection).toContain('prCheckDetails({') expect(checksSection).toMatch( - /withGitHubCheckDetailsTimeout\(\s*window\.api\.gh\.prCheckDetails\(\{/ + /withGitHubCheckDetailsTimeout\(\(signal\) =>\s*runtimeHost\s*\?\s*callRuntimeRpc[\s\S]*:\s*window\.api\.gh\.prCheckDetails\(\{/ + ) + expect(checksSection).toContain('{ timeoutMs: 30_000, signal }') + }) + + it('makes failed check detail loads retryable and fences stale settlements', () => { + const source = componentSource('PullRequestPage.tsx') + const checksSection = sourceBetween(source, 'function ChecksTab', 'function MentionTextarea') + + expect(checksSection).toContain('const requestId = ++nextCheckDetailsRequestIdRef.current') + expect(checksSection).toContain('settleGitHubChecksTabDetails(current, key, requestId') + expect(checksSection).toContain('createGitHubChecksTabState(checks, checkDetailsContextKey)') + expect(checksSection).toContain('checksState,\n checks,\n checkDetailsContextKey') + expect(checksSection).toContain('resetGitHubChecksTabForSource(current)') + expect(checksSection).toContain( + 'committedChecksContextOwnerRef.current !== refreshContextOwner' + ) + expect(checksSection).toContain('activeChecksRefreshRequestIdRef.current !== refreshRequestId') + expect(checksSection).toContain('current.contextOwner === refreshContextOwner') + expect(checksSection).toContain( + 'const rerunContextOwner = committedChecksContextOwnerRef.current' + ) + expect(checksSection).toContain('committedChecksContextOwnerRef.current !== rerunContextOwner') + expect(checksSection).toContain('await handleRefresh(rerunContextOwner)') + expect(checksSection).toContain('!mountedRef.current ||') + expect(checksSection).toContain( + 'onClick={() => requestCheckDetails(check, getCheckDetailsKey(check))}' + ) + expect(checksSection).toContain('disabled={state.loading}') + expect(checksSection).toContain('aria-busy={state.loading}') + expect(checksSection).toContain("translate('githubChecks.retrying', 'Retrying…')") + expect(checksSection).toContain( + "translate('auto.components.PullRequestPage.5df7c41d2a', 'Retry')" ) }) diff --git a/src/renderer/src/components/right-sidebar/checks-list-expanded-details.test.tsx b/src/renderer/src/components/right-sidebar/checks-list-expanded-details.test.tsx index f3b61948121..8555d53b345 100644 --- a/src/renderer/src/components/right-sidebar/checks-list-expanded-details.test.tsx +++ b/src/renderer/src/components/right-sidebar/checks-list-expanded-details.test.tsx @@ -280,10 +280,14 @@ describe('ChecksList expanded check details', () => { }) it('retries an inline details error', async () => { + let resolveRetry: (details: PRCheckRunDetails) => void = () => {} + const retryRequest = new Promise((resolve) => { + resolveRetry = resolve + }) const onLoadCheckDetails = vi .fn<() => Promise>() .mockRejectedValueOnce(new Error('GitHub request failed')) - .mockResolvedValueOnce(checkDetails) + .mockReturnValueOnce(retryRequest) renderChecksList({ onLoadCheckDetails }) await act(async () => { @@ -293,14 +297,25 @@ describe('ChecksList expanded check details', () => { const retry = [...container.querySelectorAll('button')].find( (candidate) => candidate.textContent?.trim() === 'Retry' ) + retry!.focus() await act(async () => { retry!.dispatchEvent(new MouseEvent('click', { bubbles: true })) await Promise.resolve() - await Promise.resolve() }) expect(onLoadCheckDetails).toHaveBeenCalledTimes(2) + expect(document.activeElement).toBe(retry) + expect(retry?.disabled).toBe(true) + expect(retry?.textContent).toContain('Retrying…') + expect(retry?.getAttribute('aria-busy')).toBe('true') + expect(container.textContent).toContain('GitHub request failed') + + await act(async () => { + resolveRetry(checkDetails) + await retryRequest + }) + expect(container.textContent).toContain('Verify failed') }) diff --git a/src/renderer/src/components/right-sidebar/checks-panel-content.tsx b/src/renderer/src/components/right-sidebar/checks-panel-content.tsx index 72403dbd2e8..0d00acd3f09 100644 --- a/src/renderer/src/components/right-sidebar/checks-panel-content.tsx +++ b/src/renderer/src/components/right-sidebar/checks-panel-content.tsx @@ -756,7 +756,7 @@ function CheckRunDetails({
)} - {state?.loading ? ( + {state?.loading && !state.error ? (
@@ -824,13 +824,17 @@ function CheckRunDetails({ variant="outline" size="xs" className="shrink-0" + disabled={state.loading} + aria-busy={state.loading} onClick={onRetry} > - - {translate( - 'auto.components.right.sidebar.checks.panel.content.dcb3c546fe', - 'Retry' - )} + + {state.loading + ? translate('githubChecks.retrying', 'Retrying…') + : translate( + 'auto.components.right.sidebar.checks.panel.content.dcb3c546fe', + 'Retry' + )}
)} @@ -1143,16 +1147,17 @@ export function ChecksList({ } const requestContextKey = checkDetailsContextKey const requestId = createCheckRunDetailsRequestId() + const retryError = detailsByCheckKey[row.key]?.error ?? null setDetailsByCheckKey((current) => ({ ...current, - [row.key]: { requestId, loading: true, details: null, error: null } + [row.key]: { requestId, loading: true, details: null, error: retryError } })) if (resolvedWorktreeId) { patchOpenCheckRunDetails(resolvedWorktreeId, requestContextKey, row.check, { requestId, details: null, loading: true, - error: null, + error: retryError, githubRepository: githubRepository ?? null, gitlabProjectRef: getGitLabProjectRef?.() ?? null }) diff --git a/src/renderer/src/i18n/locales/en.json b/src/renderer/src/i18n/locales/en.json index 3c693dfc7eb..1f5f127f338 100644 --- a/src/renderer/src/i18n/locales/en.json +++ b/src/renderer/src/i18n/locales/en.json @@ -34,6 +34,9 @@ "failed": "The browser page stopped unexpectedly. Retry to restore it." } }, + "githubChecks": { + "retrying": "Retrying…" + }, "settings": { "appearance": { "language": { @@ -1126,7 +1129,10 @@ "activity": "Activity", "noActivity": "No activity yet." }, - "checkActionRequiredHint": "This check needs a manual action on GitHub (for example, approving the workflow run) before merging is unblocked." + "checkActionRequiredHint": "This check needs a manual action on GitHub (for example, approving the workflow run) before merging is unblocked.", + "e15a8b77ef": "No inline details are available for this check.", + "e45324fbed": "Failed to load check details.", + "dcb3c546fe": "Retry" }, "GitLabItemDialog": { "65e784c1f1": "Reopen", @@ -1615,7 +1621,10 @@ "8ff5ae8866": "Assignees", "82c87eceb9": "Edit assignees", "1ff5d979df": "No one assigned", - "checkActionRequiredHint": "This check needs a manual action on GitHub (for example, approving the workflow run) before merging is unblocked." + "checkActionRequiredHint": "This check needs a manual action on GitHub (for example, approving the workflow run) before merging is unblocked.", + "6b1d5ee3e4": "No inline details are available for this check.", + "e04c027d98": "Failed to load check details.", + "5df7c41d2a": "Retry" }, "QuickOpen": { "1dbd3f59ff": "Move", diff --git a/src/renderer/src/runtime/github-check-details-timeout.test.ts b/src/renderer/src/runtime/github-check-details-timeout.test.ts index 1031331ca55..a06c27d7012 100644 --- a/src/renderer/src/runtime/github-check-details-timeout.test.ts +++ b/src/renderer/src/runtime/github-check-details-timeout.test.ts @@ -5,7 +5,8 @@ import { } from './github-check-details-timeout' vi.mock('@/i18n/i18n', () => ({ - translate: (_key: string, fallback: string) => fallback + translate: (key: string, fallback: string) => + key === 'auto.runtime.githubCheckDetailsTimeout.timedOut' ? 'Localized timeout.' : fallback })) describe('withGitHubCheckDetailsTimeout', () => { @@ -18,16 +19,67 @@ describe('withGitHubCheckDetailsTimeout', () => { }) it('passes through a result and clears its deadline', async () => { - await expect(withGitHubCheckDetailsTimeout(Promise.resolve('details'))).resolves.toBe('details') + await expect(withGitHubCheckDetailsTimeout(() => Promise.resolve('details'))).resolves.toBe( + 'details' + ) expect(vi.getTimerCount()).toBe(0) }) - it('rejects a stalled local request after the remote RPC budget', async () => { - const stalled = withGitHubCheckDetailsTimeout(new Promise(() => {})) - const assertion = expect(stalled).rejects.toThrow('Timed out loading check details.') + it('rejects a stalled renderer operation after the check-details budget', async () => { + let operationSignal: AbortSignal | undefined + const stalled = withGitHubCheckDetailsTimeout((signal) => { + operationSignal = signal + return new Promise(() => {}) + }) + const assertion = expect(stalled).rejects.toThrow('Localized timeout.') + + await vi.advanceTimersByTimeAsync(GITHUB_CHECK_DETAILS_TIMEOUT_MS) + + await assertion + expect(operationSignal?.aborted).toBe(true) + }) + + it('arms the renderer deadline before starting the operation', async () => { + let timerCountWhenStarted = 0 + + const result = withGitHubCheckDetailsTimeout(async () => { + timerCountWhenStarted = vi.getTimerCount() + return 'details' + }) + + await expect(result).resolves.toBe('details') + expect(timerCountWhenStarted).toBe(1) + expect(vi.getTimerCount()).toBe(0) + }) + + it('keeps the timeout error when abort-aware work rejects synchronously', async () => { + const stalled = withGitHubCheckDetailsTimeout( + (signal) => + new Promise((_resolve, reject) => { + signal.addEventListener('abort', () => reject(new Error('operation aborted')), { + once: true + }) + }) + ) + const assertion = expect(stalled).rejects.toThrow('Localized timeout.') await vi.advanceTimersByTimeAsync(GITHUB_CHECK_DETAILS_TIMEOUT_MS) await assertion }) + + it.each([ + 'Timed out loading check details.', + "Error invoking remote method 'gh:prCheckDetails': Error: Timed out loading check details." + ])('normalizes host timeout errors: %s', async (message) => { + await expect( + withGitHubCheckDetailsTimeout(() => Promise.reject(new Error(message))) + ).rejects.toMatchObject({ message: 'Localized timeout.' }) + }) + + it('preserves unrelated operation failures', async () => { + await expect( + withGitHubCheckDetailsTimeout(() => Promise.reject(new Error('authentication failed'))) + ).rejects.toThrow('authentication failed') + }) }) diff --git a/src/renderer/src/runtime/github-check-details-timeout.ts b/src/renderer/src/runtime/github-check-details-timeout.ts index 9e077fb995c..1ccaca60d6f 100644 --- a/src/renderer/src/runtime/github-check-details-timeout.ts +++ b/src/renderer/src/runtime/github-check-details-timeout.ts @@ -1,24 +1,39 @@ import { translate } from '@/i18n/i18n' +import { + GITHUB_CHECK_DETAILS_TIMEOUT_MESSAGE, + isGitHubCheckDetailsTimeout +} from '../../../shared/github-check-details-deadline' export const GITHUB_CHECK_DETAILS_TIMEOUT_MS = 30_000 -/** Bound local IPC the same way runtime RPC bounds remote check-detail requests. */ -export async function withGitHubCheckDetailsTimeout(pending: Promise): Promise { +function translatedTimeoutError(): Error { + return new Error( + translate( + 'auto.runtime.githubCheckDetailsTimeout.timedOut', + GITHUB_CHECK_DETAILS_TIMEOUT_MESSAGE + ) + ) +} + +/** Bound the renderer operation and cancel transports that support AbortSignal. */ +export async function withGitHubCheckDetailsTimeout( + operation: (signal: AbortSignal) => Promise +): Promise { + const controller = new AbortController() let timer: ReturnType | undefined const timeout = new Promise((_, reject) => { timer = setTimeout(() => { - reject( - new Error( - translate( - 'auto.runtime.githubCheckDetailsTimeout.timedOut', - 'Timed out loading check details.' - ) - ) - ) + reject(translatedTimeoutError()) + controller.abort() }, GITHUB_CHECK_DETAILS_TIMEOUT_MS) }) try { - return await Promise.race([pending, timeout]) + return await Promise.race([operation(controller.signal), timeout]) + } catch (error) { + if (isGitHubCheckDetailsTimeout(error)) { + throw translatedTimeoutError() + } + throw error } finally { clearTimeout(timer) } diff --git a/src/renderer/src/store/slices/github.test.ts b/src/renderer/src/store/slices/github.test.ts index cadbc5acb8f..1040bdee41f 100644 --- a/src/renderer/src/store/slices/github.test.ts +++ b/src/renderer/src/store/slices/github.test.ts @@ -33,6 +33,7 @@ import { GITHUB_WORK_ITEMS_QUERY_MAX_BYTES } from './github-work-items-query-bou const runtimeEnvironmentCall = vi.fn() const runtimeEnvironmentTransportCall = vi.fn() +const runtimeEnvironmentSubscribe = vi.fn() const mockApi = { gh: { @@ -62,7 +63,8 @@ const mockApi = { create: vi.fn() }, runtimeEnvironments: { - call: runtimeEnvironmentTransportCall + call: runtimeEnvironmentTransportCall, + subscribe: runtimeEnvironmentSubscribe }, cache: { getGitHub: vi.fn().mockResolvedValue(null), @@ -77,9 +79,30 @@ function resetRemoteRuntimeMocks() { clearRuntimeCompatibilityCacheForTests() runtimeEnvironmentCall.mockReset() runtimeEnvironmentTransportCall.mockReset() + runtimeEnvironmentSubscribe.mockReset() runtimeEnvironmentTransportCall.mockImplementation((args: RuntimeEnvironmentCallRequest) => { return createCompatibleRuntimeStatusResponseIfNeeded(args) ?? runtimeEnvironmentCall(args) }) + runtimeEnvironmentSubscribe.mockImplementation( + async ( + args: RuntimeEnvironmentCallRequest, + handlers: { + onResponse: (response: unknown) => void + onError: (error: { message: string }) => void + } + ) => { + let active = true + void Promise.resolve(runtimeEnvironmentCall(args)).then( + (response) => active && handlers.onResponse(response), + (error) => active && handlers.onError({ message: String(error) }) + ) + return { + unsubscribe: () => { + active = false + } + } + } + ) } function createTestStore() { @@ -1444,6 +1467,109 @@ describe('createGitHubSlice.fetchPRCheckDetails', () => { expect(mockApi.gh.prCheckDetails).not.toHaveBeenCalled() }) + it('bounds the whole runtime check-detail load when compatibility probing stalls', async () => { + vi.useFakeTimers() + try { + runtimeEnvironmentTransportCall.mockImplementation(() => new Promise(() => {})) + const store = createTestStore() + const repoPath = '/repo' + const repoId = 'repo-id' + + store.setState({ + settings: { activeRuntimeEnvironmentId: 'env-1' } as AppState['settings'], + repos: [ + { + id: repoId, + path: repoPath, + name: 'repo', + kind: 'git', + executionHostId: 'runtime:env-1' + } + ] + } as unknown as Partial) + + const request = store + .getState() + .fetchPRCheckDetails(repoPath, { checkRunId: 123, checkName: 'build' }, { repoId }) + const rejection = expect(request).rejects.toThrow('Timed out loading check details.') + + await vi.advanceTimersByTimeAsync(30_000) + + await rejection + expect(runtimeEnvironmentCall).not.toHaveBeenCalled() + expect(mockApi.gh.prCheckDetails).not.toHaveBeenCalled() + } finally { + vi.useRealTimers() + } + }) + + it('shares one timeout budget between runtime compatibility and check details', async () => { + vi.useFakeTimers() + try { + runtimeEnvironmentTransportCall.mockImplementation((args: RuntimeEnvironmentCallRequest) => { + const compatibility = createCompatibleRuntimeStatusResponseIfNeeded(args) + if (compatibility) { + return new Promise((resolve) => setTimeout(() => resolve(compatibility), 20_000)) + } + return runtimeEnvironmentCall(args) + }) + runtimeEnvironmentCall.mockImplementation(() => new Promise(() => {})) + const store = createTestStore() + const repoPath = '/repo' + const repoId = 'repo-id' + store.setState({ + settings: { activeRuntimeEnvironmentId: 'env-1' } as AppState['settings'], + repos: [ + { + id: repoId, + path: repoPath, + name: 'repo', + kind: 'git', + executionHostId: 'runtime:env-1' + } + ] + } as unknown as Partial) + + const request = store + .getState() + .fetchPRCheckDetails(repoPath, { checkRunId: 123, checkName: 'build' }, { repoId }) + let settled = false + void request.then( + () => { + settled = true + }, + () => { + settled = true + } + ) + const rejection = expect(request).rejects.toThrow('Timed out loading check details.') + + await vi.advanceTimersByTimeAsync(20_000) + expect(runtimeEnvironmentCall).toHaveBeenCalledWith({ + selector: 'env-1', + method: 'github.prCheckDetails', + params: { + repo: repoId, + checkRunId: 123, + workflowRunId: undefined, + checkName: 'build', + url: undefined, + prRepo: null + }, + timeoutMs: 30_000 + }) + + await vi.advanceTimersByTimeAsync(9_999) + expect(settled).toBe(false) + await vi.advanceTimersByTimeAsync(1) + + await rejection + expect(settled).toBe(true) + } finally { + vi.useRealTimers() + } + }) + it('loads known local repo check details through local IPC when a runtime is focused', async () => { const store = createTestStore() const repoPath = '/repo' diff --git a/src/renderer/src/store/slices/github.ts b/src/renderer/src/store/slices/github.ts index 012df2ea2b7..6a214736378 100644 --- a/src/renderer/src/store/slices/github.ts +++ b/src/renderer/src/store/slices/github.ts @@ -3575,21 +3575,24 @@ export const createGitHubSlice: StateCreator = (s repoPath, options?.sourceContext ) - return requestContext.target.kind === 'environment' - ? await callRuntimeRpc( - { kind: 'environment', environmentId: requestContext.target.environmentId }, - 'github.prCheckDetails', - { - repo: requestContext.target.runtimeRepoId, - checkRunId: args.checkRunId, - workflowRunId: args.workflowRunId, - checkName: args.checkName, - url: args.url, - prRepo: args.prRepo ?? null - }, - { timeoutMs: 30_000 } + const requestTarget = requestContext.target + return requestTarget.kind === 'environment' + ? await withGitHubCheckDetailsTimeout((signal) => + callRuntimeRpc( + { kind: 'environment', environmentId: requestTarget.environmentId }, + 'github.prCheckDetails', + { + repo: requestTarget.runtimeRepoId, + checkRunId: args.checkRunId, + workflowRunId: args.workflowRunId, + checkName: args.checkName, + url: args.url, + prRepo: args.prRepo ?? null + }, + { timeoutMs: 30_000, signal } + ) ) - : await withGitHubCheckDetailsTimeout( + : await withGitHubCheckDetailsTimeout(() => window.api.gh.prCheckDetails({ repoPath, repoId, diff --git a/src/shared/github-check-details-deadline.ts b/src/shared/github-check-details-deadline.ts new file mode 100644 index 00000000000..2886265fc92 --- /dev/null +++ b/src/shared/github-check-details-deadline.ts @@ -0,0 +1,6 @@ +export const GITHUB_CHECK_DETAILS_HOST_TIMEOUT_MS = 25_000 +export const GITHUB_CHECK_DETAILS_TIMEOUT_MESSAGE = 'Timed out loading check details.' + +export function isGitHubCheckDetailsTimeout(error: unknown): boolean { + return error instanceof Error && error.message.endsWith(GITHUB_CHECK_DETAILS_TIMEOUT_MESSAGE) +}