mirror of
https://github.com/stablyai/orca.git
synced 2026-09-22 16:02:32 +00:00
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
This commit is contained in:
@@ -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)
|
||||
})
|
||||
|
||||
+66
-14
@@ -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<typeof getHostedReviewLocalGitOptions>
|
||||
|
||||
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<T>(operation: Promise<T>, signal: AbortSignal): Promise<T> {
|
||||
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<PRCheckRunDetails | null> {
|
||||
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<string, unknown> | 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()
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -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()
|
||||
})
|
||||
})
|
||||
+38
-11
@@ -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<void> {
|
||||
function githubOperationAbortError(): Error {
|
||||
const error = new Error('GitHub operation aborted')
|
||||
error.name = 'AbortError'
|
||||
return error
|
||||
}
|
||||
|
||||
export function acquire(signal?: AbortSignal): Promise<void> {
|
||||
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()
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user