From c1e75477f37d7ff887fd86097bc4f04e3572e437 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Mon, 10 Aug 2026 23:11:45 -0700 Subject: [PATCH] Fix static analysis page stuck in loading state (#13674) * Fix static analysis page stuck in loading state - Bound check-details requests with 30s timeout, matching remote RPC budget - Track request IDs to discard stale responses when context changes - Propagate githubRepository through store and components for proper routing - Add retry button for failed check-details loads - Improve accessibility with ARIA labels for loading and error states * Fix static analysis page stuck in loading state When an open check-details tab's repository is removed, the loading state would continue indefinitely because the fetch was still being triggered. Prevent the fetch call in this scenario to unblock the UI. Also migrates translation keys to obfuscated identifiers. * Fix static analysis page stuck in loading state Add deadline-based timeouts and request ID tracking to prevent stale responses from freezing the checks panel. Include abort signal propagation throughout the request chain and provide retry UI for failed check details loads. * fix(checks): prevent loading state from getting stuck on retry - Consolidate mount checks into a helper function - Details now clear when a new request begins - Add i18n strings for retry status --- src/main/git/runner-command-exec.test.ts | 16 + src/main/git/runner.ts | 25 +- .../github/client-pr-check-details.test.ts | 78 +++- src/main/github/client.ts | 80 +++- src/main/github/gh-utils-concurrency.test.ts | 21 ++ src/main/github/gh-utils.ts | 49 ++- src/main/runtime/orca-runtime.test.ts | 40 +- src/main/runtime/orca-runtime.ts | 7 +- src/main/runtime/rpc/methods/github.test.ts | 26 ++ src/main/runtime/rpc/methods/github.ts | 20 +- .../src/components/GitHubItemDialog.tsx | 352 +++++++++++------- .../src/components/PullRequestPage.tsx | 352 +++++++++++------- .../editor/CheckRunDetailsPanel.tsx | 12 +- .../editor/check-run-details-tab.ts | 24 +- .../github-checks-tab-state.test.ts | 188 +++++++++- .../src/components/github-checks-tab-state.ts | 70 +++- ...github-item-dialog-source-boundary.test.ts | 36 ++ .../pull-request-page-host-boundary.test.ts | 35 ++ .../components/right-sidebar/ChecksPanel.tsx | 1 + .../FolderWorkspacePrChecksPanel.tsx | 2 +- .../FolderWorkspacePrChecksRow.tsx | 1 + .../checks-list-expanded-details.test.tsx | 151 +++++++- .../right-sidebar/checks-panel-content.tsx | 172 +++++++-- .../parent-pr-checks-refresh.test.ts | 5 +- .../right-sidebar/parent-pr-checks-refresh.ts | 2 +- .../parent-pr-checks-row-types.ts | 10 +- .../parent-pr-checks-rows.test.ts | 6 +- .../right-sidebar/parent-pr-checks-rows.ts | 3 +- src/renderer/src/i18n/locales/en.json | 22 +- src/renderer/src/i18n/locales/es.json | 5 +- src/renderer/src/i18n/locales/ja.json | 5 +- src/renderer/src/i18n/locales/ko.json | 5 +- src/renderer/src/i18n/locales/zh.json | 5 +- .../github-check-details-timeout.test.ts | 85 +++++ .../runtime/github-check-details-timeout.ts | 40 ++ src/renderer/src/store/slices/editor.test.ts | 153 +++++++- src/renderer/src/store/slices/editor.ts | 45 ++- src/renderer/src/store/slices/github.test.ts | 128 ++++++- src/renderer/src/store/slices/github.ts | 44 ++- src/shared/github-check-details-deadline.ts | 6 + src/shared/hosted-review-github.test.ts | 6 +- src/shared/hosted-review-github.ts | 1 + src/shared/hosted-review.ts | 10 +- 43 files changed, 1939 insertions(+), 405 deletions(-) create mode 100644 src/main/github/gh-utils-concurrency.test.ts create mode 100644 src/renderer/src/runtime/github-check-details-timeout.test.ts create mode 100644 src/renderer/src/runtime/github-check-details-timeout.ts create mode 100644 src/shared/github-check-details-deadline.ts diff --git a/src/main/git/runner-command-exec.test.ts b/src/main/git/runner-command-exec.test.ts index 41975f9d665..dd24b8c8fb0 100644 --- a/src/main/git/runner-command-exec.test.ts +++ b/src/main/git/runner-command-exec.test.ts @@ -232,6 +232,22 @@ describe('runner execFile timeout handling', () => { expect(child.kill).toHaveBeenCalled() }) + it('kills an active gh execution when its caller aborts', async () => { + const child = createMockChildProcess(1234) + execFileMock.mockReturnValue(child) + const controller = new AbortController() + const promise = ghExecFileAsync(['api', 'repos/stablyai/orca/issues/5388'], { + cwd: '/repo', + signal: controller.signal + }) + const rejection = expect(promise).rejects.toMatchObject({ name: 'AbortError' }) + + controller.abort() + + await rejection + expect(child.kill).toHaveBeenCalled() + }) + it('honors explicit gh timeouts', async () => { const child = createMockChildProcess(1234) execFileMock.mockReturnValue(child) diff --git a/src/main/git/runner.ts b/src/main/git/runner.ts index 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