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 cfa5aa5ca1f..2bca4a4e034 100644 --- a/src/main/git/runner.ts +++ b/src/main/git/runner.ts @@ -1454,8 +1454,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 { @@ -1627,7 +1643,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) { @@ -1669,7 +1686,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 828f3290647..f95bfe7fdd8 100644 --- a/src/main/github/client.ts +++ b/src/main/github/client.ts @@ -124,9 +124,13 @@ 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' import { hydrateGitHubPRStack, mergeGitHubPRStack } from './github-pr-stack' -type GhExecOptions = GitHubRepoExecOptions +type GhExecOptions = GitHubRepoExecOptions & { signal?: AbortSignal } type HostedReviewLocalGitOptions = ReturnType const ORCA_REPO = 'stablyai/orca' @@ -151,6 +155,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 @@ -4094,6 +4122,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 } @@ -4126,20 +4155,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) { @@ -4158,6 +4201,7 @@ export async function getPRCheckDetails( ) annotations = mapCheckAnnotations(JSON.parse(annotationsResult.stdout)) } catch (err) { + rethrowCheckDetailsAbort(controller.signal, err) console.warn('getPRCheckDetails annotations fetch failed:', err) } } @@ -4176,6 +4220,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) } } @@ -4200,9 +4245,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 031c9be80c5..6545486a3a2 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 cb7e3d38287..424fab261b4 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -20109,14 +20109,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 0d415d8d2d8..eee4a5a106c 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' @@ -145,6 +147,7 @@ import { buildPRCommentConversationReplyBody } from '@/components/right-sidebar/ import { useAppStore } from '@/store' import { useAllWorktrees } from '@/store/selectors' import { callRuntimeRpc, getActiveRuntimeTarget } from '@/runtime/runtime-rpc-client' +import { withGitHubCheckDetailsTimeout } from '@/runtime/github-check-details-timeout' import { useRepoLabels, useRepoAssignees, useImmediateMutation } from '@/hooks/useIssueMetadata' import { useRepoLabelsBySlug, useRepoAssigneesBySlug } from '@/hooks/useGitHubSlugMetadata' import { GitHubMarkdownComposer } from '@/components/github/GitHubMarkdownComposer' @@ -3219,19 +3222,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) @@ -3262,72 +3292,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>>( @@ -3351,6 +3416,9 @@ function ChecksTab({ failedOnly, prRepo }) + if (!mountedRef.current || committedChecksContextOwnerRef.current !== rerunContextOwner) { + return + } if (!result.ok) { toast.error(result.error) return @@ -3360,15 +3428,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)) + } } }, [ @@ -3377,6 +3449,7 @@ function ChecksTab({ headSha, item.number, item.repoId, + mountedRef, prRepo, runtimeHost, rerunning, @@ -3445,77 +3518,74 @@ 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), + 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, checkRunId: check.checkRunId, workflowRunId: check.workflowRunId, checkName: check.name, url: check.url, prRepo - }, - { timeoutMs: 30_000 } - ) - : window.api.gh.prCheckDetails({ - repoPath: repoPath ?? '', - repoId: repoId ?? undefined, - sourceContext, - checkRunId: check.checkRunId, - workflowRunId: check.workflowRunId, - checkName: check.name, - 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, @@ -3526,6 +3596,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 = ( @@ -3719,7 +3801,7 @@ function ChecksTab({ return (
- {state?.loading ? ( + {state?.loading && !state.error ? (
{translate('auto.components.GitHubItemDialog.934d87ab96', 'Loading check details…')} @@ -3750,7 +3832,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 3dc3dcc7531..69969b5f3bd 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' @@ -138,6 +140,7 @@ import { buildPRCommentConversationReplyBody } from '@/components/right-sidebar/ import { useAppStore } from '@/store' import { useAllWorktrees } from '@/store/selectors' import { callRuntimeRpc, getActiveRuntimeTarget } from '@/runtime/runtime-rpc-client' +import { withGitHubCheckDetailsTimeout } from '@/runtime/github-check-details-timeout' import { useRepoLabels, useRepoAssignees, useImmediateMutation } from '@/hooks/useIssueMetadata' import { useRepoLabelsBySlug, useRepoAssigneesBySlug } from '@/hooks/useGitHubSlugMetadata' import { @@ -3221,13 +3224,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) @@ -3321,7 +3352,6 @@ function ChecksTab({ }, [item, targetRepoId] ) - const prRepo = useMemo(() => resolvePullRequestRepo(item), [item]) const runtimeHost = getGitHubSourceRuntimeHost(sourceContext) const canUseChecksRepoContext = canUseGitHubRepoContext(repoPath, sourceContext) const sorted = sortChecksBySeverity(list) @@ -3352,72 +3382,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>>( @@ -3441,6 +3506,9 @@ function ChecksTab({ failedOnly, prRepo }) + if (!mountedRef.current || committedChecksContextOwnerRef.current !== rerunContextOwner) { + return + } if (!result.ok) { toast.error(result.error) return @@ -3450,15 +3518,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)) + } } }, [ @@ -3467,6 +3539,7 @@ function ChecksTab({ headSha, item.number, item.repoId, + mountedRef, prRepo, runtimeHost, rerunning, @@ -3529,77 +3602,74 @@ 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), + 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, checkRunId: check.checkRunId, workflowRunId: check.workflowRunId, checkName: check.name, url: check.url, prRepo - }, - { timeoutMs: 30_000 } - ) - : window.api.gh.prCheckDetails({ - repoPath: repoPath ?? '', - repoId: repoId ?? undefined, - sourceContext, - checkRunId: check.checkRunId, - workflowRunId: check.workflowRunId, - checkName: check.name, - 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.PullRequestPage.6b1d5ee3e4', + '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.PullRequestPage.e04c027d98', + 'Failed to load check details.' + ) + }) }) }, [ canUseChecksRepoContext, - detailsByCheckKey, item.repoId, mountedRef, runtimeHost, @@ -3610,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 = ( @@ -3803,7 +3885,7 @@ function ChecksTab({ return (
- {state?.loading ? ( + {state?.loading && !state.error ? (
{translate('auto.components.PullRequestPage.d8e82b7f15', 'Loading check details…')} @@ -3834,7 +3916,27 @@ function ChecksTab({ )}
- {state?.error &&
{state.error}
} + {state?.error && ( +
+
+ {state.error} +
+ +
+ )} {hasOutput && (
diff --git a/src/renderer/src/components/editor/CheckRunDetailsPanel.tsx b/src/renderer/src/components/editor/CheckRunDetailsPanel.tsx index 2a9a8c4ecd5..50350fb1b47 100644 --- a/src/renderer/src/components/editor/CheckRunDetailsPanel.tsx +++ b/src/renderer/src/components/editor/CheckRunDetailsPanel.tsx @@ -247,7 +247,11 @@ export function CheckRunDetailsPanel({
{loading ? ( -
+
{translate( 'auto.components.editor.CheckRunDetailsPanel.1f2b980522', @@ -256,7 +260,11 @@ export function CheckRunDetailsPanel({
) : (
- {error &&
{error}
} + {error && ( +
+ {error} +
+ )} {hasOutput && (
diff --git a/src/renderer/src/components/editor/check-run-details-tab.ts b/src/renderer/src/components/editor/check-run-details-tab.ts index bbc8fc1180b..02e833b61b6 100644 --- a/src/renderer/src/components/editor/check-run-details-tab.ts +++ b/src/renderer/src/components/editor/check-run-details-tab.ts @@ -1,21 +1,34 @@ import type { GitLabProjectRef } from '../../../../shared/gitlab-types' -import type { PRCheckDetail, PRCheckRunDetails } from '../../../../shared/types' +import type { + GitHubRepositoryIdentity, + PRCheckDetail, + PRCheckRunDetails +} from '../../../../shared/types' export type OpenCheckRunDetailsState = { contextKey: string check: PRCheckDetail + requestId?: number details: PRCheckRunDetails | null loading: boolean error: string | null + githubRepository?: GitHubRepositoryIdentity | null /** Why: fork/cross-project MR jobs live outside the repo's own project, so reloads need the pipeline's project. */ gitlabProjectRef?: GitLabProjectRef | null } export type CheckRunDetailsTabPatch = Pick< OpenCheckRunDetailsState, - 'details' | 'loading' | 'error' | 'gitlabProjectRef' + 'requestId' | 'details' | 'loading' | 'error' | 'githubRepository' | 'gitlabProjectRef' > +let nextCheckRunDetailsRequestId = 0 + +export function createCheckRunDetailsRequestId(): number { + nextCheckRunDetailsRequestId += 1 + return nextCheckRunDetailsRequestId +} + export function isSameGitLabProjectRef( a: GitLabProjectRef | null, b: GitLabProjectRef | null @@ -23,6 +36,13 @@ export function isSameGitLabProjectRef( return a === b || (a?.host === b?.host && a?.path === b?.path) } +export function isSameGitHubRepository( + a: GitHubRepositoryIdentity | null, + b: GitHubRepositoryIdentity | null +): boolean { + return a === b || (a?.owner === b?.owner && a?.repo === b?.repo && a?.host === b?.host) +} + export function getCheckRunTabIdentity(check: PRCheckDetail): string { if (check.checkRunId) { return `check-run:${check.checkRunId}` 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..66a0c06b722 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 type { PRCheckDetail, PRCheckRunDetails } from '../../../shared/types' import { + beginGitHubChecksTabDetails, createGitHubChecksTabState, resolveGitHubChecksTabState, + settleGitHubChecksTabDetails, toggleGitHubChecksTabExpandedKey, updateGitHubChecksTabDetails, updateGitHubChecksTabLocalChecks @@ -15,14 +17,30 @@ const check = (name: string): PRCheckDetail => ({ url: null }) +const checkRunDetails: PRCheckRunDetails = { + name: 'unit', + status: 'completed', + conclusion: 'failure', + url: null, + detailsUrl: null, + startedAt: null, + completedAt: null, + title: 'Unit tests', + summary: null, + text: null, + annotations: [], + jobs: [] +} + 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 +48,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 +69,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 +86,142 @@ 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('clears loaded details when a retry starts after a successful load', () => { + const sourceChecks = [check('unit')] + const loaded = settleGitHubChecksTabDetails( + beginGitHubChecksTabDetails(createGitHubChecksTabState(sourceChecks, 'repo-a'), 'unit', 1), + 'unit', + 1, + { loading: false, details: checkRunDetails, error: null } + ) + + expect(loaded.detailsByCheckKey.unit.details).toBe(checkRunDetails) + expect(beginGitHubChecksTabDetails(loaded, 'unit', 2).detailsByCheckKey.unit).toEqual({ + requestId: 2, + loading: true, + details: null, + error: null + }) + }) + + 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..e408eb6ef9d 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( @@ -51,6 +79,11 @@ export function toggleGitHubChecksTabExpandedKey( } } +/** + * Unfenced write: an entry stored without a `requestId` silently drops the + * settlement of any request already in flight for that key. Use + * `beginGitHubChecksTabDetails` whenever a settlement is expected. + */ export function updateGitHubChecksTabDetails( state: GitHubChecksTabState, key: string, @@ -64,3 +97,34 @@ 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 + }) +} + +/** + * A context change is fenced indirectly: `resolveGitHubChecksTabState` swaps in + * a state with an empty `detailsByCheckKey`, so requests from the old context + * find no owning entry here and settle into nothing. + */ +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 a773910d332..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 @@ -230,6 +230,42 @@ describe('GitHubItemDialog source host boundaries', () => { expect(checksSection).toContain('window.api.gh.prChecks({') expect(checksSection).toContain('window.api.gh.rerunPRChecks({') expect(checksSection).toContain('prCheckDetails({') + expect(checksSection).toMatch( + /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 10407760e6d..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 @@ -249,6 +249,41 @@ describe('PullRequestPage host boundaries', () => { expect(checksSection).toContain('window.api.gh.prChecks({') expect(checksSection).toContain('window.api.gh.rerunPRChecks({') expect(checksSection).toContain('prCheckDetails({') + expect(checksSection).toMatch( + /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')" + ) }) it('routes edit metadata and mutations through the PR source context', () => { diff --git a/src/renderer/src/components/right-sidebar/ChecksPanel.tsx b/src/renderer/src/components/right-sidebar/ChecksPanel.tsx index 3a6bceee5f1..3ce46b39dbb 100644 --- a/src/renderer/src/components/right-sidebar/ChecksPanel.tsx +++ b/src/renderer/src/components/right-sidebar/ChecksPanel.tsx @@ -4553,6 +4553,7 @@ export default function ChecksPanel(): React.JSX.Element { checksLoading={checksLoading} checkDetailsContextKey={stateRequestKey} onLoadCheckDetails={handleLoadCheckDetails} + githubRepository={pr?.prRepo ?? null} getGitLabProjectRef={getGitLabProjectRef} /> )} diff --git a/src/renderer/src/components/right-sidebar/FolderWorkspacePrChecksPanel.tsx b/src/renderer/src/components/right-sidebar/FolderWorkspacePrChecksPanel.tsx index 0c64561edec..f2e3bdda640 100644 --- a/src/renderer/src/components/right-sidebar/FolderWorkspacePrChecksPanel.tsx +++ b/src/renderer/src/components/right-sidebar/FolderWorkspacePrChecksPanel.tsx @@ -181,7 +181,7 @@ export default function FolderWorkspacePrChecksPanel({ workflowRunId: check.workflowRunId, checkName: check.name, url: check.url, - prRepo: null + prRepo: row.githubRepository ?? null }, { repoId: row.repo.id } ) diff --git a/src/renderer/src/components/right-sidebar/FolderWorkspacePrChecksRow.tsx b/src/renderer/src/components/right-sidebar/FolderWorkspacePrChecksRow.tsx index 29d82748467..cd07701bde7 100644 --- a/src/renderer/src/components/right-sidebar/FolderWorkspacePrChecksRow.tsx +++ b/src/renderer/src/components/right-sidebar/FolderWorkspacePrChecksRow.tsx @@ -128,6 +128,7 @@ export function FolderWorkspacePrChecksRow({ checksLoading={row.isRefreshing} checkDetailsContextKey={row.refreshIdentity} onLoadCheckDetails={onLoadCheckDetails} + githubRepository={row.githubRepository ?? null} worktreeId={row.worktree.id} detailsStickySurface="card" /> 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 1a4bfdd8b58..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 @@ -7,6 +7,8 @@ import { TooltipProvider } from '@/components/ui/tooltip' import type { PRCheckDetail, PRCheckRunDetails } from '../../../../shared/types' import { ChecksList } from './checks-panel-content' +globalThis.IS_REACT_ACT_ENVIRONMENT = true + const openCheckRunDetails = vi.fn() const patchOpenCheckRunDetails = vi.fn() const activeWorktreeState = vi.hoisted(() => ({ @@ -84,6 +86,7 @@ function renderChecksList( props: Partial<{ worktreeId: string detailsStickySurface: 'sidebar' | 'card' + checkDetailsContextKey: string checks: PRCheckDetail[] onLoadCheckDetails: (check: PRCheckDetail) => Promise }> = {} @@ -94,7 +97,7 @@ function renderChecksList( { expect(stickyBar?.textContent).not.toContain('View full logs') }) + it('finishes the full-details tab when its sidebar unmounts during loading', async () => { + let resolveDetails: (details: PRCheckRunDetails) => void = () => {} + const request = new Promise((resolve) => { + resolveDetails = resolve + }) + renderChecksList({ worktreeId: 'wt-child-1', onLoadCheckDetails: () => request }) + + await act(async () => { + await Promise.resolve() + }) + patchOpenCheckRunDetails.mockClear() + const button = [...container.querySelectorAll('button')].find((candidate) => + candidate.textContent?.includes('View full details') + ) + + act(() => { + button!.dispatchEvent(new MouseEvent('click', { bubbles: true })) + root.render(
) + }) + await act(async () => { + resolveDetails(checkDetails) + await request + }) + + expect(patchOpenCheckRunDetails).toHaveBeenCalledWith( + 'wt-child-1', + 'repo:42', + failingCheck, + expect.objectContaining({ details: checkDetails, loading: false, error: null }) + ) + }) + + it('shows a load error in the full-details tab after its sidebar unmounts', async () => { + let rejectDetails: (error: Error) => void = () => {} + const request = new Promise((_resolve, reject) => { + rejectDetails = reject + }) + renderChecksList({ worktreeId: 'wt-child-1', onLoadCheckDetails: () => request }) + + await act(async () => { + await Promise.resolve() + }) + patchOpenCheckRunDetails.mockClear() + const button = [...container.querySelectorAll('button')].find((candidate) => + candidate.textContent?.includes('View full details') + ) + + act(() => { + button!.dispatchEvent(new MouseEvent('click', { bubbles: true })) + root.render(
) + }) + await act(async () => { + rejectDetails(new Error('GitHub request failed')) + await request.catch(() => undefined) + }) + + expect(patchOpenCheckRunDetails).toHaveBeenCalledWith( + 'wt-child-1', + 'repo:42', + failingCheck, + expect.objectContaining({ details: null, loading: false, error: 'GitHub request failed' }) + ) + }) + + 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')) + .mockReturnValueOnce(retryRequest) + renderChecksList({ onLoadCheckDetails }) + + await act(async () => { + await Promise.resolve() + await Promise.resolve() + }) + 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() + }) + + 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') + }) + + it('ignores a stale inline result after returning to the same context', async () => { + const requests: { + resolve: (details: PRCheckRunDetails) => void + reject: (error: Error) => void + }[] = [] + const onLoadCheckDetails = vi.fn( + () => + new Promise((resolve, reject) => { + requests.push({ resolve, reject }) + }) + ) + + renderChecksList({ checkDetailsContextKey: 'repo:A', onLoadCheckDetails }) + await act(async () => { + await Promise.resolve() + await Promise.resolve() + }) + renderChecksList({ checkDetailsContextKey: 'repo:B', onLoadCheckDetails }) + await act(async () => { + await Promise.resolve() + await Promise.resolve() + }) + renderChecksList({ checkDetailsContextKey: 'repo:A', onLoadCheckDetails }) + await act(async () => { + await Promise.resolve() + await Promise.resolve() + }) + + expect(requests).toHaveLength(3) + await act(async () => { + requests[2]!.resolve(checkDetails) + await Promise.resolve() + }) + await act(async () => { + requests[0]!.reject(new Error('stale request failed')) + await Promise.resolve() + }) + + expect(container.textContent).toContain('Verify failed') + expect(container.textContent).not.toContain('stale request failed') + }) + it('uses resolved details when showing the action-required fallback hint', async () => { renderChecksList({ onLoadCheckDetails: async () => ({ 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 2b052da0a0f..7091d995996 100644 --- a/src/renderer/src/components/right-sidebar/checks-panel-content.tsx +++ b/src/renderer/src/components/right-sidebar/checks-panel-content.tsx @@ -94,6 +94,7 @@ import type { PRCheckRunDetails, PRComment, GitHubReactionContent, + GitHubRepositoryIdentity, PRConflictSummary, PRMergeableState } from '../../../../shared/types' @@ -111,6 +112,7 @@ import { useActiveWorktree } from '@/store/selectors' import { useAppStore } from '@/store' import { sortChecksBySeverity } from '../../../../shared/pr-check-severity-order' import { summarizeProviderChecks } from '../../../../shared/provider-check-summary' +import { createCheckRunDetailsRequestId } from '@/components/editor/check-run-details-tab' export const PullRequestIcon = GitPullRequest @@ -548,6 +550,7 @@ export function ConflictTriageStrip({ } type CheckDetailsLoadState = { + requestId?: number loading: boolean details: PRCheckRunDetails | null error: string | null @@ -679,7 +682,9 @@ function CheckRunDetails({ checkDetailsContextKey, worktreeId, detailsStickySurface = 'sidebar', - getGitLabProjectRef + getGitLabProjectRef, + githubRepository, + onRetry }: { check: PRCheckDetail state: CheckDetailsLoadState | undefined @@ -688,6 +693,8 @@ function CheckRunDetails({ detailsStickySurface?: CheckDetailsStickySurface /** Why: a getter, not a value — the source ref is filled by an async fetch and would read stale during render. */ getGitLabProjectRef?: () => GitLabProjectRef | null + githubRepository?: GitHubRepositoryIdentity | null + onRetry: () => void }): React.JSX.Element { const openCheckRunDetails = useAppStore((s) => s.openCheckRunDetails) const details = state?.details @@ -724,9 +731,11 @@ function CheckRunDetails({ return } openCheckRunDetails(worktreeId, checkDetailsContextKey, check, { + requestId: state?.requestId, details: state?.details ?? null, loading: state?.loading ?? false, error: state?.error ?? null, + githubRepository: githubRepository ?? null, gitlabProjectRef: getGitLabProjectRef?.() ?? null }) } @@ -753,8 +762,8 @@ function CheckRunDetails({
)} - {state?.loading ? ( -
+ {state?.loading && !state.error ? ( +
{translate( @@ -811,7 +820,30 @@ function CheckRunDetails({ )}
- {state?.error &&
{state.error}
} + {state?.error && ( +
+ + {state.error} + + +
+ )} {hasOutput && (
@@ -979,7 +1011,8 @@ export function ChecksList({ onLoadCheckDetails, worktreeId: worktreeIdOverride, detailsStickySurface = 'sidebar', - getGitLabProjectRef + getGitLabProjectRef, + githubRepository }: { checks: PRCheckDetail[] checksLoading: boolean @@ -990,6 +1023,7 @@ export function ChecksList({ detailsStickySurface?: CheckDetailsStickySurface /** Why: a getter, not a value — the source ref is filled by an async fetch and would read stale during render. */ getGitLabProjectRef?: () => GitLabProjectRef | null + githubRepository?: GitHubRepositoryIdentity | null }): React.JSX.Element { const activeWorktree = useActiveWorktree() const resolvedWorktreeId = worktreeIdOverride ?? activeWorktree?.id ?? null @@ -1118,55 +1152,114 @@ export function ChecksList({ return } const requestContextKey = checkDetailsContextKey + const requestId = createCheckRunDetailsRequestId() + const retryError = detailsByCheckKey[row.key]?.error ?? null setDetailsByCheckKey((current) => ({ ...current, - [row.key]: { loading: true, details: null, error: null } + [row.key]: { requestId, loading: true, details: null, error: retryError } })) - void onLoadCheckDetails(row.check) + if (resolvedWorktreeId) { + patchOpenCheckRunDetails(resolvedWorktreeId, requestContextKey, row.check, { + requestId, + details: null, + loading: true, + error: retryError, + githubRepository: githubRepository ?? null, + gitlabProjectRef: getGitLabProjectRef?.() ?? null + }) + } + const request = Promise.resolve().then(() => onLoadCheckDetails(row.check)) + void request .then((details) => { - if (detailsContextRef.current !== requestContextKey) { - return - } - setDetailsByCheckKey((current) => ({ - ...current, - [row.key]: { - loading: false, + if (resolvedWorktreeId) { + patchOpenCheckRunDetails(resolvedWorktreeId, requestContextKey, row.check, { + requestId, details, + loading: false, error: details ? null : translate( 'auto.components.right.sidebar.checks.panel.content.e15a8b77ef', 'No inline details are available for this check.' ), - // Why: a detail-less result is only final for this status — re-arm the retry once the job moves on. - errorAt: details - ? undefined - : { status: row.check.status, conclusion: row.check.conclusion } - } - })) - }) - .catch((err) => { + githubRepository: githubRepository ?? null, + gitlabProjectRef: getGitLabProjectRef?.() ?? null + }) + } if (detailsContextRef.current !== requestContextKey) { return } - setDetailsByCheckKey((current) => ({ - ...current, - [row.key]: { - loading: false, - details: null, - error: - err instanceof Error - ? err.message - : translate( - 'auto.components.right.sidebar.checks.panel.content.checkDetailsLoadFailed', - 'Failed to load check details.' - ), - errorAt: { status: row.check.status, conclusion: row.check.conclusion } + setDetailsByCheckKey((current) => { + if (current[row.key]?.requestId !== requestId) { + return current } - })) + return { + ...current, + [row.key]: { + requestId, + loading: false, + details, + error: details + ? null + : translate( + 'auto.components.right.sidebar.checks.panel.content.e15a8b77ef', + 'No inline details are available for this check.' + ), + // Why: a detail-less result is only final for this status — re-arm the retry once the job moves on. + errorAt: details + ? undefined + : { status: row.check.status, conclusion: row.check.conclusion } + } + } + }) + }) + .catch((err) => { + const error = + err instanceof Error + ? err.message + : translate( + 'auto.components.right.sidebar.checks.panel.content.e45324fbed', + 'Failed to load check details.' + ) + if (resolvedWorktreeId) { + patchOpenCheckRunDetails(resolvedWorktreeId, requestContextKey, row.check, { + requestId, + details: null, + loading: false, + error, + githubRepository: githubRepository ?? null, + gitlabProjectRef: getGitLabProjectRef?.() ?? null + }) + } + if (detailsContextRef.current !== requestContextKey) { + return + } + setDetailsByCheckKey((current) => { + if (current[row.key]?.requestId !== requestId) { + return current + } + return { + ...current, + [row.key]: { + requestId, + loading: false, + details: null, + error, + errorAt: { status: row.check.status, conclusion: row.check.conclusion } + } + } + }) }) }, - [checkDetailsContextKey, detailsByCheckKey, onLoadCheckDetails] + [ + checkDetailsContextKey, + detailsByCheckKey, + getGitLabProjectRef, + githubRepository, + onLoadCheckDetails, + patchOpenCheckRunDetails, + resolvedWorktreeId + ] ) useEffect(() => { @@ -1190,9 +1283,11 @@ export function ChecksList({ continue } patchOpenCheckRunDetails(resolvedWorktreeId, checkDetailsContextKey, row.check, { + requestId: detailsState.requestId, details: detailsState.details ?? null, loading: detailsState.loading ?? false, error: detailsState.error ?? null, + githubRepository: githubRepository ?? null, gitlabProjectRef: getGitLabProjectRef?.() ?? null }) } @@ -1200,6 +1295,7 @@ export function ChecksList({ checkDetailsContextKey, detailsByCheckKey, getGitLabProjectRef, + githubRepository, patchOpenCheckRunDetails, resolvedWorktreeId, rows @@ -1376,6 +1472,8 @@ export function ChecksList({ worktreeId={resolvedWorktreeId} detailsStickySurface={detailsStickySurface} getGitLabProjectRef={getGitLabProjectRef} + githubRepository={githubRepository} + onRetry={() => requestCheckDetails(row)} /> )}
diff --git a/src/renderer/src/components/right-sidebar/parent-pr-checks-refresh.test.ts b/src/renderer/src/components/right-sidebar/parent-pr-checks-refresh.test.ts index 2b9e5dcbe33..9cf77aa2565 100644 --- a/src/renderer/src/components/right-sidebar/parent-pr-checks-refresh.test.ts +++ b/src/renderer/src/components/right-sidebar/parent-pr-checks-refresh.test.ts @@ -126,7 +126,8 @@ describe('parent PR checks refresh', () => { worktrees: [unlinked, linked], repos: [repo] }) - const fetchHostedReviewForBranch = vi.fn(async () => makeReview()) + const githubRepository = { owner: 'upstream', repo: 'project' } + const fetchHostedReviewForBranch = vi.fn(async () => makeReview({ githubRepository })) const fetchPRChecks = vi.fn(async () => []) await runLimitedParentPrChecksRefreshes({ @@ -151,7 +152,7 @@ describe('parent PR checks refresh', () => { staleWhileRevalidate: true } ]) - expect(fetchPRChecks).toHaveBeenCalledWith('/repo', 7, 'feature', 'abc123', null, { + expect(fetchPRChecks).toHaveBeenCalledWith('/repo', 7, 'feature', 'abc123', githubRepository, { repoId: 'repo-1', force: false }) diff --git a/src/renderer/src/components/right-sidebar/parent-pr-checks-refresh.ts b/src/renderer/src/components/right-sidebar/parent-pr-checks-refresh.ts index 3d5ea1bbc8b..2404950b422 100644 --- a/src/renderer/src/components/right-sidebar/parent-pr-checks-refresh.ts +++ b/src/renderer/src/components/right-sidebar/parent-pr-checks-refresh.ts @@ -151,7 +151,7 @@ async function refreshParentPrChecksCandidate( review.number, candidate.branch, review.headSha, - null, + review.githubRepository ?? null, { repoId: candidate.repo.id, force } ) } diff --git a/src/renderer/src/components/right-sidebar/parent-pr-checks-row-types.ts b/src/renderer/src/components/right-sidebar/parent-pr-checks-row-types.ts index 9e6f3c2f784..18fc4459da6 100644 --- a/src/renderer/src/components/right-sidebar/parent-pr-checks-row-types.ts +++ b/src/renderer/src/components/right-sidebar/parent-pr-checks-row-types.ts @@ -1,4 +1,11 @@ -import type { CheckStatus, PRCheckDetail, PRInfo, Repo, Worktree } from '../../../../shared/types' +import type { + CheckStatus, + GitHubRepositoryIdentity, + PRCheckDetail, + PRInfo, + Repo, + Worktree +} from '../../../../shared/types' import type { HostedReviewInfo } from '../../../../shared/hosted-review' import type { AppState } from '@/store' import { translate } from '@/i18n/i18n' @@ -59,6 +66,7 @@ export type ParentPrChecksRow = { reviewState: HostedReviewInfo['state'] | null reviewStatus: HostedReviewInfo['status'] | null provider: HostedReviewInfo['provider'] | null + githubRepository?: GitHubRepositoryIdentity | null summary: string detailNames: string[] checks: PRCheckDetail[] diff --git a/src/renderer/src/components/right-sidebar/parent-pr-checks-rows.test.ts b/src/renderer/src/components/right-sidebar/parent-pr-checks-rows.test.ts index d261df5851e..c9e2ebc18cc 100644 --- a/src/renderer/src/components/right-sidebar/parent-pr-checks-rows.test.ts +++ b/src/renderer/src/components/right-sidebar/parent-pr-checks-rows.test.ts @@ -590,7 +590,8 @@ describe('buildParentPrChecksProjection', () => { it('reads scoped GitHub checks detail names without using details as aggregate truth', () => { const repo = makeRepo({ connectionId: 'ssh-1' }) const worktree = makeWorktree({ id: 'repo-1::/feature' }) - const review = makeReview({ status: 'failure', headSha: 'abc123' }) + const githubRepository = { owner: 'upstream', repo: 'project' } + const review = makeReview({ status: 'failure', headSha: 'abc123', githubRepository }) const hostedKey = getHostedReviewCacheKey( repo.path, 'feature', @@ -601,7 +602,7 @@ describe('buildParentPrChecksProjection', () => { const checksKey = getGitHubRepoCacheKey( repo.path, repo.id, - prChecksCacheSuffix(12, null, 'abc123'), + prChecksCacheSuffix(12, githubRepository, 'abc123'), settings, repo.connectionId ) @@ -628,6 +629,7 @@ describe('buildParentPrChecksProjection', () => { expect(projection.rows[0]?.detailNames).toEqual(['build']) expect(projection.rows[0]?.status).toBe('failing') + expect(projection.rows[0]?.githubRepository).toEqual(githubRepository) }) it('prioritizes action-required check detail names before truncating the preview', () => { diff --git a/src/renderer/src/components/right-sidebar/parent-pr-checks-rows.ts b/src/renderer/src/components/right-sidebar/parent-pr-checks-rows.ts index 6330c3bbee9..72f4ab921bc 100644 --- a/src/renderer/src/components/right-sidebar/parent-pr-checks-rows.ts +++ b/src/renderer/src/components/right-sidebar/parent-pr-checks-rows.ts @@ -147,6 +147,7 @@ function buildParentPrChecksRow( reviewState: review?.state ?? fallbackDisplay?.state ?? null, reviewStatus: review?.status ?? fallbackDisplay?.status ?? null, provider: review?.provider ?? fallbackDisplay?.provider ?? null, + githubRepository: review?.provider === 'github' ? (review.githubRepository ?? null) : null, summary: getRowSummary(status, review, detailNames), detailNames, checks: checkDetails, @@ -251,7 +252,7 @@ function getGitHubChecksEntry( args: ParentPrChecksRowSourceArgs & { repo: Repo }, review: HostedReviewInfo ): ParentPrChecksCacheEntry | undefined { - const prRepo = null + const prRepo = review.githubRepository ?? null const withHead = getGitHubRepoCacheKey( args.repo.path, args.repo.id, diff --git a/src/renderer/src/i18n/locales/en.json b/src/renderer/src/i18n/locales/en.json index 0ac928f9927..1888c99acc4 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": { @@ -251,7 +254,8 @@ "51f15c37d3": "Cannot open directory: {{value0}}", "f2e00db373": "File not found: {{value0}}", "checkRunDetailsUnavailable": "No details are available for this check.", - "checkRunDetailsLoadFailed": "Failed to load check details." + "checkRunDetailsLoadFailed": "Failed to load check details.", + "checkRunDetailsRepoUnavailable": "Repository details are unavailable for this check." }, "github": { "f129c42773": "GitHub did not return the new comment.", @@ -1125,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", @@ -1614,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", @@ -11166,6 +11176,7 @@ "checksUnresolvedChip": "unresolved", "checksUnresolvedStripHint": "These checks finished without a pass or fail verdict.", "e15a8b77ef": "No inline details are available for this check.", + "dcb3c546fe": "Retry", "679bf2093c": "Copy log excerpt", "d713f500b2": "Log excerpt", "a916648574": "Open details", @@ -11232,7 +11243,7 @@ "actionRequiredHint": "This check needs a manual action on GitHub (for example, approving the workflow run) before merging is unblocked.", "b3195cba33": "Unmark author as bot", "f588b46a6c": "Mark author as bot", - "checkDetailsLoadFailed": "Failed to load check details." + "e45324fbed": "Failed to load check details." }, "empty": { "state": { @@ -14981,6 +14992,9 @@ "loadFailed": "Failed to load the GitLab job log.", "emptyTrace": "No log is available for this GitLab job.", "timedOut": "Timed out loading the GitLab job log." + }, + "githubCheckDetailsTimeout": { + "timedOut": "Timed out loading check details." } }, "ssh": { diff --git a/src/renderer/src/i18n/locales/es.json b/src/renderer/src/i18n/locales/es.json index 3870c3906ef..2cae124b261 100644 --- a/src/renderer/src/i18n/locales/es.json +++ b/src/renderer/src/i18n/locales/es.json @@ -7,6 +7,9 @@ "webDescription": "Vuelva a cargar el cliente web o vuelva a conectarse al servidor Orca vinculado." } }, + "githubChecks": { + "retrying": "Reintentando…" + }, "settings": { "appearance": { "language": { @@ -10751,7 +10754,7 @@ "actionRequiredHint": "Este check requiere una acción manual en GitHub (por ejemplo, aprobar la ejecución del workflow) antes de que el merge quede desbloqueado.", "b3195cba33": "Desmarcar autor como bot", "f588b46a6c": "Marcar autor como bot", - "checkDetailsLoadFailed": "No se pudieron cargar los detalles del check." + "e45324fbed": "No se pudieron cargar los detalles del check." }, "empty": { "state": { diff --git a/src/renderer/src/i18n/locales/ja.json b/src/renderer/src/i18n/locales/ja.json index 6865fd30aa6..ea96427ee3e 100644 --- a/src/renderer/src/i18n/locales/ja.json +++ b/src/renderer/src/i18n/locales/ja.json @@ -7,6 +7,9 @@ "webDescription": "Web クライアントを再試行するか、ペアリングされたランタイムに再接続します。" } }, + "githubChecks": { + "retrying": "再試行中…" + }, "settings": { "appearance": { "language": { @@ -10751,7 +10754,7 @@ "actionRequiredHint": "マージのブロックが解除されるまでに、このチェックには GitHub 上での手動操作(例: ワークフロー実行の承認)が必要です。", "b3195cba33": "作成者のボット指定を解除", "f588b46a6c": "作成者をボットとして指定", - "checkDetailsLoadFailed": "チェック詳細の読み込みに失敗しました。" + "e45324fbed": "チェック詳細の読み込みに失敗しました。" }, "empty": { "state": { diff --git a/src/renderer/src/i18n/locales/ko.json b/src/renderer/src/i18n/locales/ko.json index 8b1d695f107..059a8e298ac 100644 --- a/src/renderer/src/i18n/locales/ko.json +++ b/src/renderer/src/i18n/locales/ko.json @@ -7,6 +7,9 @@ "webDescription": "웹 클라이언트를 다시 시도하거나 페어링된 런타임에 다시 연결하세요." } }, + "githubChecks": { + "retrying": "재시도 중…" + }, "settings": { "appearance": { "language": { @@ -10751,7 +10754,7 @@ "actionRequiredHint": "이 검사는 병합 차단이 해제되기 전에 GitHub에서 수동 작업(예: 워크플로 실행 승인)이 필요합니다.", "b3195cba33": "작성자의 봇 지정 해제", "f588b46a6c": "작성자를 봇으로 지정", - "checkDetailsLoadFailed": "체크 세부 정보를 불러오지 못했습니다." + "e45324fbed": "체크 세부 정보를 불러오지 못했습니다." }, "empty": { "state": { diff --git a/src/renderer/src/i18n/locales/zh.json b/src/renderer/src/i18n/locales/zh.json index f91467d7bff..4c06ad72500 100644 --- a/src/renderer/src/i18n/locales/zh.json +++ b/src/renderer/src/i18n/locales/zh.json @@ -7,6 +7,9 @@ "webDescription": "重试 Web 客户端或重新连接到配对的运行时。" } }, + "githubChecks": { + "retrying": "正在重试..." + }, "settings": { "appearance": { "language": { @@ -10771,7 +10774,7 @@ "actionRequiredHint": "此检查需要在 GitHub 上执行手动操作(例如批准工作流运行)后才能解除合并阻止。", "b3195cba33": "取消将作者标记为机器人", "f588b46a6c": "将作者标记为机器人", - "checkDetailsLoadFailed": "加载检查详细信息失败。" + "e45324fbed": "加载检查详细信息失败。" }, "empty": { "state": { diff --git a/src/renderer/src/runtime/github-check-details-timeout.test.ts b/src/renderer/src/runtime/github-check-details-timeout.test.ts new file mode 100644 index 00000000000..a06c27d7012 --- /dev/null +++ b/src/renderer/src/runtime/github-check-details-timeout.test.ts @@ -0,0 +1,85 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { + GITHUB_CHECK_DETAILS_TIMEOUT_MS, + withGitHubCheckDetailsTimeout +} from './github-check-details-timeout' + +vi.mock('@/i18n/i18n', () => ({ + translate: (key: string, fallback: string) => + key === 'auto.runtime.githubCheckDetailsTimeout.timedOut' ? 'Localized timeout.' : fallback +})) + +describe('withGitHubCheckDetailsTimeout', () => { + beforeEach(() => { + vi.useFakeTimers() + }) + + afterEach(() => { + vi.useRealTimers() + }) + + it('passes through a result and clears its deadline', async () => { + await expect(withGitHubCheckDetailsTimeout(() => Promise.resolve('details'))).resolves.toBe( + 'details' + ) + expect(vi.getTimerCount()).toBe(0) + }) + + 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 new file mode 100644 index 00000000000..1ccaca60d6f --- /dev/null +++ b/src/renderer/src/runtime/github-check-details-timeout.ts @@ -0,0 +1,40 @@ +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 + +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(translatedTimeoutError()) + controller.abort() + }, GITHUB_CHECK_DETAILS_TIMEOUT_MS) + }) + try { + 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/editor.test.ts b/src/renderer/src/store/slices/editor.test.ts index 8b87ee976c7..b63a9d16e1a 100644 --- a/src/renderer/src/store/slices/editor.test.ts +++ b/src/renderer/src/store/slices/editor.test.ts @@ -3080,18 +3080,24 @@ describe('createEditorSlice conflict status reconciliation', () => { url: null, checkRunId: 42 } + const githubRepository = { owner: 'upstream', repo: 'project' } store.getState().openCheckRunDetails('wt-1', 'repo:99', check, { details: null, loading: false, - error: null + error: null, + githubRepository }) await store.getState().reloadOpenCheckRunDetailsTab('wt-1::check-details::check-run:42') expect(fetchPRCheckDetails).toHaveBeenCalledWith( '/repo', - expect.objectContaining({ checkRunId: 42, checkName: 'verify' }), + expect.objectContaining({ + checkRunId: 42, + checkName: 'verify', + prRepo: githubRepository + }), { repoId: 'repo-1' } ) expect(store.getState().openFiles).toContainEqual( @@ -3105,6 +3111,43 @@ describe('createEditorSlice conflict status reconciliation', () => { ) }) + it('stops loading when an open check-details tab loses its repository', async () => { + const fetchPRCheckDetails = vi.fn() + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const store = createStore()((...args: any[]) => ({ + activeWorktreeId: 'wt-1', + repos: [], + worktreesByRepo: {}, + fetchPRCheckDetails, + ...createEditorSlice(...(args as Parameters)) + })) as unknown as StoreApi + const check = { + name: 'verify', + status: 'completed' as const, + conclusion: 'failure' as const, + url: null, + checkRunId: 42 + } + + store.getState().openCheckRunDetails('wt-1', 'repo:99', check, { + details: null, + loading: true, + error: null + }) + await store.getState().reloadOpenCheckRunDetailsTab('wt-1::check-details::check-run:42') + + expect(fetchPRCheckDetails).not.toHaveBeenCalled() + expect(store.getState().openFiles).toContainEqual( + expect.objectContaining({ + id: 'wt-1::check-details::check-run:42', + checkRunDetails: expect.objectContaining({ + loading: false, + error: 'Repository details are unavailable for this check.' + }) + }) + ) + }) + // Regression for #7732: refreshing a GitLab job tab through the GitHub check-runs // API returns null and blanks the tab the user just asked to reload. it('reloads an open GitLab job tab through the job trace client', async () => { @@ -3304,6 +3347,112 @@ describe('createEditorSlice conflict status reconciliation', () => { ) }) + it('ignores a stale check-details request after the tab context changes', () => { + const store = createEditorTabsStore() + const check = { + name: 'verify', + status: 'completed' as const, + conclusion: 'failure' as const, + url: null, + checkRunId: 42 + } + + store.getState().openCheckRunDetails('wt-1', 'repo:old', check, { + details: null, + loading: true, + error: null + }) + store.getState().openCheckRunDetails('wt-1', 'repo:new', check, { + details: null, + loading: true, + error: null + }) + store.getState().patchOpenCheckRunDetails('wt-1', 'repo:old', check, { + details: null, + loading: false, + error: 'stale request failed' + }) + + expect( + store.getState().openFiles.find((file) => file.id === 'wt-1::check-details::check-run:42') + ?.checkRunDetails + ).toEqual( + expect.objectContaining({ contextKey: 'repo:new', details: null, loading: true, error: null }) + ) + }) + + it('ignores an older check-details request in the same context', () => { + const store = createEditorTabsStore() + const check = { + name: 'verify', + status: 'completed' as const, + conclusion: 'failure' as const, + url: null, + checkRunId: 42 + } + + store.getState().openCheckRunDetails('wt-1', 'repo:99', check, { + requestId: 1, + details: null, + loading: true, + error: null + }) + store.getState().patchOpenCheckRunDetails('wt-1', 'repo:99', check, { + requestId: 2, + details: null, + loading: true, + error: null + }) + store.getState().patchOpenCheckRunDetails('wt-1', 'repo:99', check, { + requestId: 1, + details: null, + loading: false, + error: 'stale request failed' + }) + + expect( + store.getState().openFiles.find((file) => file.id === 'wt-1::check-details::check-run:42') + ?.checkRunDetails + ).toEqual(expect.objectContaining({ requestId: 2, details: null, loading: true, error: null })) + }) + + it('does not reopen a check-details tab with an older sidebar snapshot', () => { + const store = createEditorTabsStore() + const check = { + name: 'verify', + status: 'completed' as const, + conclusion: 'failure' as const, + url: null, + checkRunId: 42 + } + + store.getState().openCheckRunDetails('wt-1', 'repo:99', check, { + requestId: 2, + details: null, + loading: false, + error: 'newer result' + }) + store.getState().openFile({ + filePath: '/repo/other.ts', + relativePath: 'other.ts', + worktreeId: 'wt-1', + language: 'typescript', + mode: 'edit' + }) + store.getState().openCheckRunDetails('wt-1', 'repo:99', check, { + requestId: 1, + details: null, + loading: false, + error: 'stale result' + }) + + expect(store.getState().activeFileId).toBe('wt-1::check-details::check-run:42') + expect( + store.getState().openFiles.find((file) => file.id === 'wt-1::check-details::check-run:42') + ?.checkRunDetails + ).toEqual(expect.objectContaining({ requestId: 2, error: 'newer result' })) + }) + it('opens check full details as a center-pane editor tab', () => { const store = createEditorTabsStore() const check = { diff --git a/src/renderer/src/store/slices/editor.ts b/src/renderer/src/store/slices/editor.ts index 971ab20cba6..4eda195dfa2 100644 --- a/src/renderer/src/store/slices/editor.ts +++ b/src/renderer/src/store/slices/editor.ts @@ -14,7 +14,9 @@ import { isPathInsideOrEqual } from '../../../../shared/cross-platform-path' import { resolveMarkdownLinkTarget } from '@/components/editor/markdown-internal-links' import { buildCheckRunDetailsTabId, + createCheckRunDetailsRequestId, getCheckRunDetailsTabLabel, + isSameGitHubRepository, isSameGitLabProjectRef, type CheckRunDetailsTabPatch, type OpenCheckRunDetailsState @@ -3802,14 +3804,21 @@ export const createEditorSlice: StateCreator = (s const checkRunDetails: OpenCheckRunDetailsState = { contextKey, check, + requestId: state.requestId, details: state.details, loading: state.loading, error: state.error, + githubRepository: state.githubRepository ?? null, gitlabProjectRef: state.gitlabProjectRef ?? null } set((s) => { const existing = s.openFiles.find((f) => f.id === id) if (existing) { + const existingDetails = existing.checkRunDetails + const incomingIsStale = + existingDetails?.contextKey === contextKey && + existingDetails.requestId !== undefined && + (state.requestId === undefined || state.requestId < existingDetails.requestId) return { openFiles: s.openFiles.map((f) => f.id === id @@ -3818,7 +3827,7 @@ export const createEditorSlice: StateCreator = (s mode: 'check-details' as const, relativePath: label, language: 'plaintext', - checkRunDetails + checkRunDetails: incomingIsStale ? existingDetails : checkRunDetails } : f ), @@ -3860,24 +3869,39 @@ export const createEditorSlice: StateCreator = (s return s } const current = existing.checkRunDetails + if (current.contextKey !== contextKey) { + return s + } + if ( + state.requestId !== undefined && + current.requestId !== undefined && + state.requestId < current.requestId + ) { + return s + } // Why: the sidebar resolves the MR's project asynchronously, so an early patch // must not blank a ref we already know. + const githubRepository = state.githubRepository ?? current.githubRepository ?? null const gitlabProjectRef = state.gitlabProjectRef ?? current.gitlabProjectRef ?? null const nextCheckRunDetails: OpenCheckRunDetailsState = { contextKey, check, + requestId: state.requestId ?? current.requestId, details: state.details, loading: state.loading, error: state.error, + githubRepository, gitlabProjectRef } if ( current.contextKey === nextCheckRunDetails.contextKey && + current.requestId === nextCheckRunDetails.requestId && current.check.status === nextCheckRunDetails.check.status && current.check.conclusion === nextCheckRunDetails.check.conclusion && current.loading === nextCheckRunDetails.loading && current.error === nextCheckRunDetails.error && current.details === nextCheckRunDetails.details && + isSameGitHubRepository(current.githubRepository ?? null, githubRepository) && isSameGitLabProjectRef(current.gitlabProjectRef ?? null, gitlabProjectRef) ) { return s @@ -3897,16 +3921,25 @@ export const createEditorSlice: StateCreator = (s if (!file || file.mode !== 'check-details' || !checkRunDetails) { return } + const { contextKey, check } = checkRunDetails + const requestId = createCheckRunDetailsRequestId() + const patch = (next: CheckRunDetailsTabPatch): void => { + get().patchOpenCheckRunDetails(file.worktreeId, contextKey, check, { ...next, requestId }) + } const worktree = findWorktreeById(state.worktreesByRepo, file.worktreeId) const repoId = worktree?.repoId ?? getRepoIdFromWorktreeId(file.worktreeId) const repo = state.repos.find((candidate) => candidate.id === repoId) if (!repo?.path) { + patch({ + details: checkRunDetails.details, + loading: false, + error: translate( + 'auto.store.slices.editor.checkRunDetailsRepoUnavailable', + 'Repository details are unavailable for this check.' + ) + }) return } - const { contextKey, check } = checkRunDetails - const patch = (next: CheckRunDetailsTabPatch): void => { - get().patchOpenCheckRunDetails(file.worktreeId, contextKey, check, next) - } patch({ details: checkRunDetails.details, loading: true, error: null }) try { // Why: refreshing a GitLab job tab through the GitHub check-runs API returns @@ -3927,7 +3960,7 @@ export const createEditorSlice: StateCreator = (s workflowRunId: check.workflowRunId, checkName: check.name, url: check.url, - prRepo: null + prRepo: checkRunDetails.githubRepository ?? null }, { repoId: repo.id } ) diff --git a/src/renderer/src/store/slices/github.test.ts b/src/renderer/src/store/slices/github.test.ts index 4b7d19ad284..f77b8577a94 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 5b16b72796c..7ab87e09731 100644 --- a/src/renderer/src/store/slices/github.ts +++ b/src/renderer/src/store/slices/github.ts @@ -76,6 +76,7 @@ import { } from '../../../../shared/task-source-context' import { normalizeGitHubPRForBranchOutcome } from '../../../../shared/github-pr-for-branch-outcome' import { restoreReactionOnSubject, setReactionOnSubject } from '@/lib/pr-comment-reactions' +import { withGitHubCheckDetailsTimeout } from '@/runtime/github-check-details-timeout' import { getGitHubRepoLookupIndex } from './github-repo-lookup-index' // ─── ProjectV2 cache types ──────────────────────────────────────────── @@ -3592,30 +3593,35 @@ 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, + 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(() => + window.api.gh.prCheckDetails({ + repoPath, + repoId, checkRunId: args.checkRunId, workflowRunId: args.workflowRunId, checkName: args.checkName, url: args.url, - prRepo: args.prRepo ?? null - }, - { timeoutMs: 30_000 } + prRepo: args.prRepo ?? null, + sourceContext: options?.sourceContext + }) ) - : ((await window.api.gh.prCheckDetails({ - repoPath, - repoId, - checkRunId: args.checkRunId, - workflowRunId: args.workflowRunId, - checkName: args.checkName, - url: args.url, - prRepo: args.prRepo ?? null, - sourceContext: options?.sourceContext - })) as PRCheckRunDetails | null) }, fetchPRComments: async (repoPath, prNumber, options): Promise => { 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) +} diff --git a/src/shared/hosted-review-github.test.ts b/src/shared/hosted-review-github.test.ts index 88cca4d51e5..bbc17fa785b 100644 --- a/src/shared/hosted-review-github.test.ts +++ b/src/shared/hosted-review-github.test.ts @@ -15,7 +15,8 @@ const pr: PRInfo = { describe('hostedReviewInfoFromGitHubPRInfo', () => { it('maps PRInfo into sidebar hosted review metadata', () => { - const review = hostedReviewInfoFromGitHubPRInfo(pr) + const githubRepository = { owner: 'upstream', repo: 'orca' } + const review = hostedReviewInfoFromGitHubPRInfo({ ...pr, prRepo: githubRepository }) expect(review).toMatchObject({ provider: 'github', @@ -24,7 +25,8 @@ describe('hostedReviewInfoFromGitHubPRInfo', () => { state: 'open', status: 'pending', mergeable: 'MERGEABLE', - headSha: 'abc123' + headSha: 'abc123', + githubRepository }) }) }) diff --git a/src/shared/hosted-review-github.ts b/src/shared/hosted-review-github.ts index 51633f849d5..3cf76175bda 100644 --- a/src/shared/hosted-review-github.ts +++ b/src/shared/hosted-review-github.ts @@ -17,6 +17,7 @@ export function hostedReviewInfoFromGitHubPRInfo(pr: PRInfo): HostedReviewInfo { ...(pr.mergeQueueRequired !== undefined ? { mergeQueueRequired: pr.mergeQueueRequired } : {}), ...(pr.mergeStateStatus !== undefined ? { mergeStateStatus: pr.mergeStateStatus } : {}), ...(pr.headSha ? { headSha: pr.headSha } : {}), + ...(pr.prRepo ? { githubRepository: pr.prRepo } : {}), ...(pr.confirmedContainedHeadOid ? { confirmedContainedHeadOid: pr.confirmedContainedHeadOid } : {}), diff --git a/src/shared/hosted-review.ts b/src/shared/hosted-review.ts index 7646cb97ceb..0d25a637e41 100644 --- a/src/shared/hosted-review.ts +++ b/src/shared/hosted-review.ts @@ -1,4 +1,10 @@ -import type { CheckStatus, PRConflictSummary, PRMergeableState, PRReviewDecision } from './types' +import type { + CheckStatus, + GitHubRepositoryIdentity, + PRConflictSummary, + PRMergeableState, + PRReviewDecision +} from './types' export type HostedReviewProvider = | 'github' @@ -30,6 +36,8 @@ export type HostedReviewInfo = { mergeQueueRequired?: boolean | null mergeStateStatus?: string | null headSha?: string + /** GitHub repository that owns the PR; absent on older runtimes and other providers. */ + githubRepository?: GitHubRepositoryIdentity // Why: mirrors PRInfo.confirmedContainedHeadOid so merged-review staleness // checks accept a worktree head confirmed to be part of the merged PR. confirmedContainedHeadOid?: string