From e9492f2a1001c9fc73b45604fe5c76e7d8bad164 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Mon, 10 Aug 2026 21:44:35 -0700 Subject: [PATCH] Fix static analysis page stuck in loading state Add deadline-based timeouts and request ID tracking to prevent stale responses from freezing the checks panel. Include abort signal propagation throughout the request chain and provide retry UI for failed check details loads. --- src/main/git/runner-command-exec.test.ts | 16 + src/main/git/runner.ts | 25 +- .../github/client-pr-check-details.test.ts | 78 +++- src/main/github/client.ts | 80 +++- src/main/github/gh-utils-concurrency.test.ts | 21 ++ src/main/github/gh-utils.ts | 49 ++- src/main/runtime/orca-runtime.test.ts | 40 +- src/main/runtime/orca-runtime.ts | 7 +- src/main/runtime/rpc/methods/github.test.ts | 26 ++ src/main/runtime/rpc/methods/github.ts | 20 +- .../src/components/GitHubItemDialog.tsx | 345 +++++++++++------- .../src/components/PullRequestPage.tsx | 317 ++++++++++------ .../github-checks-tab-state.test.ts | 153 +++++++- .../src/components/github-checks-tab-state.ts | 60 ++- ...github-item-dialog-source-boundary.test.ts | 35 +- .../pull-request-page-host-boundary.test.ts | 34 +- .../checks-list-expanded-details.test.tsx | 19 +- .../right-sidebar/checks-panel-content.tsx | 21 +- src/renderer/src/i18n/locales/en.json | 13 +- .../github-check-details-timeout.test.ts | 62 +++- .../runtime/github-check-details-timeout.ts | 37 +- src/renderer/src/store/slices/github.test.ts | 128 ++++++- src/renderer/src/store/slices/github.ts | 31 +- src/shared/github-check-details-deadline.ts | 6 + 24 files changed, 1283 insertions(+), 340 deletions(-) create mode 100644 src/main/github/gh-utils-concurrency.test.ts create mode 100644 src/shared/github-check-details-deadline.ts 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) +}