mirror of
https://github.com/stablyai/orca.git
synced 2026-10-20 00:03:45 +00:00
fix(tasks): surface PR-side list failures so they can't read as end-of-data
Round-6 review: PartialWorkItemsResult had no PR error slot, so a swallowed gh pr list failure reached the renderer as a clean empty page — and with the count blocked (0) the speculative withdrawal deleted the pagination bar with no toast and no recovery (a regression vs main's silent no-op). PR-side errors now ride the envelope (errors.prs), demoted so they can never join the issue-only window-422 signal; errorTypes replaces issueErrorTypes; an empty page that a real count said should exist now toasts instead of looking dead.
This commit is contained in:
@@ -54,6 +54,7 @@ import {
|
||||
release,
|
||||
classifyGhError,
|
||||
classifyListIssuesError,
|
||||
classifyListPrsError,
|
||||
ghRepoExecOptions,
|
||||
githubRepoContext,
|
||||
getRemoteUrlForRepo,
|
||||
@@ -1098,10 +1099,11 @@ function buildWorkItemListRequest(args: {
|
||||
return { args: out, offset: (page - 1) * limit }
|
||||
}
|
||||
|
||||
// Why: shared shape so listWorkItems can lift the issue-side error (#1076 silent wrongness) into the IPC envelope; PR errors out of scope (§6).
|
||||
// Why: shared shape so listWorkItems can lift per-side errors (#1076 silent wrongness) into the IPC envelope — a swallowed side reads as end-of-data to pagination (#11485).
|
||||
type PartialWorkItemsResult = {
|
||||
items: MainWorkItem[]
|
||||
issuesError?: ClassifiedError
|
||||
prsError?: ClassifiedError
|
||||
}
|
||||
|
||||
function assertSshRepoHasResolvedGitHubSource(args: {
|
||||
@@ -1255,6 +1257,7 @@ async function listQueriedWorkItems(
|
||||
let successfulRequestCount = 0
|
||||
let nonAvailabilityFailureCount = 0
|
||||
let availabilityError: unknown
|
||||
let prsError: ClassifiedError | undefined
|
||||
|
||||
// Why: surface the issue-side error separately for the IPC envelope; PR-side keeps prior swallow-and-log (parent doc §6).
|
||||
const issueFetch = (async (): Promise<PartialWorkItemsResult> => {
|
||||
@@ -1326,6 +1329,7 @@ async function listQueriedWorkItems(
|
||||
} catch (err) {
|
||||
console.warn('listQueriedWorkItems PRs partial failure:', err)
|
||||
const stderr = err instanceof Error ? err.message : String(err)
|
||||
prsError = classifyListPrsError(stderr)
|
||||
if (classifyGitHubUnavailable(stderr)) {
|
||||
availabilityError ??= err
|
||||
} else {
|
||||
@@ -1342,7 +1346,8 @@ async function listQueriedWorkItems(
|
||||
}
|
||||
return {
|
||||
items: sortWorkItemsByNumber([...issueResult.items, ...prItems]).slice(0, limit),
|
||||
issuesError: issueResult.issuesError
|
||||
issuesError: issueResult.issuesError,
|
||||
prsError
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1400,7 +1405,13 @@ export async function listWorkItems(
|
||||
localGitOptions
|
||||
)
|
||||
|
||||
const errors = partial.issuesError ? { issues: partial.issuesError } : undefined
|
||||
const errors =
|
||||
partial.issuesError || partial.prsError
|
||||
? {
|
||||
...(partial.issuesError ? { issues: partial.issuesError } : {}),
|
||||
...(partial.prsError ? { prs: partial.prsError } : {})
|
||||
}
|
||||
: undefined
|
||||
return {
|
||||
items: partial.items,
|
||||
sources: {
|
||||
|
||||
@@ -55,3 +55,11 @@ export function classifyListIssuesError(stderr: string): ClassifiedError {
|
||||
}
|
||||
return { type: c.type, message: readMessages[c.type] }
|
||||
}
|
||||
|
||||
// Why: PR-side list failures need the same read-op classification — pagination
|
||||
// decisions key on the type, and swallowing them made failures look like
|
||||
// end-of-data (#11485).
|
||||
export function classifyListPrsError(stderr: string): ClassifiedError {
|
||||
const c = classifyGhError(stderr)
|
||||
return { type: c.type, message: `Failed to load pull requests: ${stderr.trim()}` }
|
||||
}
|
||||
|
||||
@@ -9,7 +9,11 @@ import { extractExecError, parseRetryAfterMs } from '../git/exec-error'
|
||||
// WSL-aware routing. Repo-scoped callers should use the runner exports below.
|
||||
export const execFileAsync = promisify(execFile)
|
||||
export { ghExecFileAsync, gitExecFileAsync, extractExecError, parseRetryAfterMs }
|
||||
export { classifyGhError, classifyListIssuesError } from './gh-error-classification'
|
||||
export {
|
||||
classifyGhError,
|
||||
classifyListIssuesError,
|
||||
classifyListPrsError
|
||||
} from './gh-error-classification'
|
||||
export {
|
||||
_getOwnerRepoCacheSize,
|
||||
_resetOwnerRepoCache,
|
||||
|
||||
@@ -6187,7 +6187,7 @@ export default function TaskPage(): React.JSX.Element {
|
||||
setPaginationLoading(true)
|
||||
setLoadingTargetPage(target)
|
||||
try {
|
||||
const { items, failedCount, issueErrorTypes } = await fetchWorkItemsNextPage(
|
||||
const { items, failedCount, errorTypes } = await fetchWorkItemsNextPage(
|
||||
repoArgs,
|
||||
githubPerRepoPageLimit,
|
||||
githubPageSize,
|
||||
@@ -6205,7 +6205,7 @@ export default function TaskPage(): React.JSX.Element {
|
||||
const { reason } = resolveEmptyPageOutcome({
|
||||
target,
|
||||
failedCount,
|
||||
issueErrorTypes,
|
||||
errorTypes,
|
||||
countedTotalPages: null
|
||||
})
|
||||
if (reason === 'window-unreachable') {
|
||||
@@ -6228,8 +6228,22 @@ export default function TaskPage(): React.JSX.Element {
|
||||
{ id: 'work-items-page-load-failed' }
|
||||
)
|
||||
} else {
|
||||
// Why: with a real count the clamp is refused, so without a toast
|
||||
// the click would look dead — the count mis-advertised the page.
|
||||
// countedTotalPages is a click-time closure read; worst case the
|
||||
// toast decision is off in a race, which is benign.
|
||||
if (countedTotalPages !== null && countedTotalPages > 0) {
|
||||
toast.error(
|
||||
translate(
|
||||
'auto.components.TaskPage.loadPageFailed',
|
||||
'Page {{value0}} could not be loaded from GitHub.',
|
||||
{ value0: String(target + 1) }
|
||||
),
|
||||
{ id: 'work-items-page-load-failed' }
|
||||
)
|
||||
}
|
||||
setCountedTotalPages((previous) =>
|
||||
applyEmptyPageClamp(previous, { target, failedCount, issueErrorTypes })
|
||||
applyEmptyPageClamp(previous, { target, failedCount, errorTypes })
|
||||
)
|
||||
}
|
||||
return
|
||||
@@ -6256,6 +6270,7 @@ export default function TaskPage(): React.JSX.Element {
|
||||
paginationLoading,
|
||||
selectedRepos,
|
||||
currentPage,
|
||||
countedTotalPages,
|
||||
appliedTaskSearch,
|
||||
fetchWorkItemsNextPage,
|
||||
githubPageSize,
|
||||
|
||||
@@ -65,18 +65,18 @@ describe('numbered GitHub pagination', () => {
|
||||
})
|
||||
|
||||
describe('resolveEmptyPageOutcome', () => {
|
||||
const base = { failedCount: 0, issueErrorTypes: [] as const, countedTotalPages: null }
|
||||
const base = { failedCount: 0, errorTypes: [] as const, countedTotalPages: null }
|
||||
|
||||
it('clamps and reports unreachable when the search window 422 is present', () => {
|
||||
expect(
|
||||
resolveEmptyPageOutcome({ ...base, target: 33, issueErrorTypes: ['validation_error'] })
|
||||
resolveEmptyPageOutcome({ ...base, target: 33, errorTypes: ['validation_error'] })
|
||||
).toEqual({ reason: 'window-unreachable', clampTotalPagesTo: 33 })
|
||||
// A real count is still clamped — the count over-advertising is the bug.
|
||||
expect(
|
||||
resolveEmptyPageOutcome({
|
||||
...base,
|
||||
target: 33,
|
||||
issueErrorTypes: ['validation_error'],
|
||||
errorTypes: ['validation_error'],
|
||||
countedTotalPages: 39
|
||||
})
|
||||
).toEqual({ reason: 'window-unreachable', clampTotalPagesTo: 33 })
|
||||
@@ -91,14 +91,14 @@ describe('resolveEmptyPageOutcome', () => {
|
||||
...base,
|
||||
target: 5,
|
||||
failedCount: 1,
|
||||
issueErrorTypes: ['validation_error']
|
||||
errorTypes: ['validation_error']
|
||||
})
|
||||
).toEqual({ reason: 'load-failed', clampTotalPagesTo: null })
|
||||
expect(
|
||||
resolveEmptyPageOutcome({
|
||||
...base,
|
||||
target: 5,
|
||||
issueErrorTypes: ['validation_error', 'permission_denied']
|
||||
errorTypes: ['validation_error', 'permission_denied']
|
||||
})
|
||||
).toEqual({ reason: 'load-failed', clampTotalPagesTo: null })
|
||||
})
|
||||
@@ -109,7 +109,7 @@ describe('resolveEmptyPageOutcome', () => {
|
||||
clampTotalPagesTo: null
|
||||
})
|
||||
for (const type of ['permission_denied', 'not_found', 'rate_limited', 'unknown'] as const) {
|
||||
expect(resolveEmptyPageOutcome({ ...base, target: 5, issueErrorTypes: [type] })).toEqual({
|
||||
expect(resolveEmptyPageOutcome({ ...base, target: 5, errorTypes: [type] })).toEqual({
|
||||
reason: 'load-failed',
|
||||
clampTotalPagesTo: null
|
||||
})
|
||||
@@ -141,7 +141,7 @@ describe('resolveEmptyPageOutcome', () => {
|
||||
})
|
||||
|
||||
describe('applyEmptyPageClamp', () => {
|
||||
const clean = { failedCount: 0, issueErrorTypes: [] as const }
|
||||
const clean = { failedCount: 0, errorTypes: [] as const }
|
||||
|
||||
it('keeps a real count that resolved between click and response', () => {
|
||||
// The count promise routinely lands mid-flight; the committed value, not
|
||||
@@ -158,10 +158,10 @@ describe('applyEmptyPageClamp', () => {
|
||||
// Window 422 limits live in provenPageLimit (applyWindowPageLimit), not
|
||||
// the count slot.
|
||||
expect(
|
||||
applyEmptyPageClamp(39, { target: 33, failedCount: 0, issueErrorTypes: ['validation_error'] })
|
||||
applyEmptyPageClamp(39, { target: 33, failedCount: 0, errorTypes: ['validation_error'] })
|
||||
).toBe(39)
|
||||
expect(applyEmptyPageClamp(33, { target: 5, failedCount: 2, issueErrorTypes: [] })).toBe(33)
|
||||
expect(applyEmptyPageClamp(null, { target: 5, failedCount: 2, issueErrorTypes: [] })).toBe(null)
|
||||
expect(applyEmptyPageClamp(33, { target: 5, failedCount: 2, errorTypes: [] })).toBe(33)
|
||||
expect(applyEmptyPageClamp(null, { target: 5, failedCount: 2, errorTypes: [] })).toBe(null)
|
||||
})
|
||||
})
|
||||
|
||||
|
||||
@@ -37,7 +37,9 @@ export function taskPageToGitHubApiPage(taskPage: number): number {
|
||||
|
||||
export type EmptyPageOutcome = {
|
||||
reason: 'window-unreachable' | 'load-failed' | 'end-of-data'
|
||||
/** New advertised page count, or null to leave the current count alone. */
|
||||
/** New advertised page count, or null to leave the current count alone.
|
||||
* Window clamps are applied via applyWindowPageLimit — this field carries
|
||||
* the same value there for symmetry, but the count slot must not use it. */
|
||||
clampTotalPagesTo: number | null
|
||||
}
|
||||
|
||||
@@ -47,21 +49,18 @@ export type EmptyPageOutcome = {
|
||||
* - a `validation_error` is GitHub's 422 for pages past its 1000-result search
|
||||
* window — the page can never load, so stop advertising it. When a sibling
|
||||
* repo's fetch also threw, the transient failure wins (load-failed, no
|
||||
* clamp) so the toast and the clamp never disagree. Note this branch is
|
||||
* issue-scope-only: the PR list path self-caps at the window and never 422s,
|
||||
* so PR-tab dead clicks resolve as end-of-data.
|
||||
* - any other per-repo error (thrown or issue-side envelope) may be transient
|
||||
* (rate limit, permissions), so surface it but keep the advertised count;
|
||||
* clamp) so the toast and the clamp never disagree. The window signal is
|
||||
* issue-side only: PR-side errors arrive demoted (never validation_error).
|
||||
* - any other per-repo error (thrown, or on either envelope channel) may be
|
||||
* transient (rate limit, permissions), so surface it but keep the count;
|
||||
* - no error at all is end-of-data. That only warrants a clamp while the count
|
||||
* is unknown (null) or failed (0), to withdraw the speculative page
|
||||
* `fallbackTotalPages` advertises — a real count must not shrink, because a
|
||||
* provider that swallows its own failures (the PR list path) also produces
|
||||
* clean-empty results.
|
||||
* `fallbackTotalPages` advertises — a real count must not shrink.
|
||||
*/
|
||||
export function resolveEmptyPageOutcome(args: {
|
||||
target: number
|
||||
failedCount: number
|
||||
issueErrorTypes: readonly ClassifiedError['type'][]
|
||||
errorTypes: readonly ClassifiedError['type'][]
|
||||
countedTotalPages: number | null
|
||||
}): EmptyPageOutcome {
|
||||
const clamp = Math.max(1, Math.floor(args.target))
|
||||
@@ -69,12 +68,11 @@ export function resolveEmptyPageOutcome(args: {
|
||||
// 403/404 alongside it means a repo that may still have pages, so the
|
||||
// transient-failure branch must win the toast and block the clamp.
|
||||
const onlyWindowErrors =
|
||||
args.issueErrorTypes.length > 0 &&
|
||||
args.issueErrorTypes.every((type) => type === 'validation_error')
|
||||
args.errorTypes.length > 0 && args.errorTypes.every((type) => type === 'validation_error')
|
||||
if (onlyWindowErrors && args.failedCount === 0) {
|
||||
return { reason: 'window-unreachable', clampTotalPagesTo: clamp }
|
||||
}
|
||||
if (args.failedCount > 0 || args.issueErrorTypes.length > 0) {
|
||||
if (args.failedCount > 0 || args.errorTypes.length > 0) {
|
||||
return { reason: 'load-failed', clampTotalPagesTo: null }
|
||||
}
|
||||
const countUnknown = args.countedTotalPages === null || args.countedTotalPages === 0
|
||||
@@ -95,7 +93,7 @@ export function applyEmptyPageClamp(
|
||||
args: {
|
||||
target: number
|
||||
failedCount: number
|
||||
issueErrorTypes: readonly ClassifiedError['type'][]
|
||||
errorTypes: readonly ClassifiedError['type'][]
|
||||
}
|
||||
): number | null {
|
||||
const outcome = resolveEmptyPageOutcome({ ...args, countedTotalPages: previous })
|
||||
|
||||
@@ -6657,11 +6657,11 @@ describe('createGitHubSlice.fetchWorkItems source/error envelope', () => {
|
||||
expect(result).toEqual({
|
||||
items: [{ ...item, repoId: 'caller-repo-id' }],
|
||||
failedCount: 0,
|
||||
issueErrorTypes: []
|
||||
errorTypes: []
|
||||
})
|
||||
})
|
||||
|
||||
it('surfaces issue-side envelope errors as issueErrorTypes on next-page fetches', async () => {
|
||||
it('surfaces issue-side envelope errors as errorTypes on next-page fetches', async () => {
|
||||
runtimeEnvironmentCall.mockResolvedValueOnce({
|
||||
id: 'rpc-work-items-page-422',
|
||||
ok: true,
|
||||
@@ -6691,7 +6691,7 @@ describe('createGitHubSlice.fetchWorkItems source/error envelope', () => {
|
||||
|
||||
// The window 422 travels on the envelope error channel, not failedCount —
|
||||
// resolveEmptyPageOutcome keys on this exact string (#11485).
|
||||
expect(result).toEqual({ items: [], failedCount: 0, issueErrorTypes: ['validation_error'] })
|
||||
expect(result).toEqual({ items: [], failedCount: 0, errorTypes: ['validation_error'] })
|
||||
})
|
||||
|
||||
it('demotes non-window validation errors so they cannot drive the unreachable clamp', async () => {
|
||||
@@ -6722,7 +6722,40 @@ describe('createGitHubSlice.fetchWorkItems source/error envelope', () => {
|
||||
.getState()
|
||||
.fetchWorkItemsNextPage([{ repoId: 'caller-repo-id', path: '/server/repo' }], 24, 100, '', 2)
|
||||
|
||||
expect(result).toEqual({ items: [], failedCount: 0, issueErrorTypes: ['unknown'] })
|
||||
expect(result).toEqual({ items: [], failedCount: 0, errorTypes: ['unknown'] })
|
||||
})
|
||||
|
||||
it('surfaces PR-side envelope errors demoted so they read as failures, never window 422s', async () => {
|
||||
runtimeEnvironmentCall.mockResolvedValueOnce({
|
||||
id: 'rpc-work-items-page-prs-error',
|
||||
ok: true,
|
||||
result: {
|
||||
items: [],
|
||||
sources: {
|
||||
issues: null,
|
||||
prs: { owner: 'up', repo: 'r' },
|
||||
originCandidate: { owner: 'up', repo: 'r' },
|
||||
upstreamCandidate: null
|
||||
},
|
||||
errors: {
|
||||
prs: { type: 'validation_error', message: 'Failed to load pull requests: bad flag' }
|
||||
}
|
||||
},
|
||||
_meta: { runtimeId: 'remote-runtime' }
|
||||
})
|
||||
const store = createTestStore()
|
||||
store.setState({
|
||||
settings: { activeRuntimeEnvironmentId: 'env-1' },
|
||||
repos: [{ id: 'runtime-repo-id', path: '/server/repo', name: 'repo', kind: 'git' }]
|
||||
} as unknown as Partial<AppState>)
|
||||
|
||||
const result = await store
|
||||
.getState()
|
||||
.fetchWorkItemsNextPage([{ repoId: 'caller-repo-id', path: '/server/repo' }], 24, 100, '', 2)
|
||||
|
||||
// A swallowed PR-side failure must not read as end-of-data (#11485), and a
|
||||
// PR-side validation error must never join the issue-only window signal.
|
||||
expect(result).toEqual({ items: [], failedCount: 0, errorTypes: ['unknown'] })
|
||||
})
|
||||
|
||||
it('routes work-item counts through the active runtime environment', async () => {
|
||||
@@ -6857,7 +6890,7 @@ describe('createGitHubSlice.fetchWorkItems source/error envelope', () => {
|
||||
oversizedQuery,
|
||||
1
|
||||
)
|
||||
).resolves.toEqual({ items: [], failedCount: 0, issueErrorTypes: [] })
|
||||
).resolves.toEqual({ items: [], failedCount: 0, errorTypes: [] })
|
||||
await expect(
|
||||
store
|
||||
.getState()
|
||||
|
||||
@@ -1971,7 +1971,7 @@ export type GitHubSlice = {
|
||||
) => Promise<{
|
||||
items: GitHubWorkItem[]
|
||||
failedCount: number
|
||||
issueErrorTypes: ClassifiedError['type'][]
|
||||
errorTypes: ClassifiedError['type'][]
|
||||
}>
|
||||
/** Count items and derive pages from the largest per-repo result set. */
|
||||
countWorkItemsAcrossRepos: (
|
||||
@@ -2792,10 +2792,10 @@ export const createGitHubSlice: StateCreator<AppState, [], [], GitHubSlice> = (s
|
||||
|
||||
fetchWorkItemsNextPage: async (repos, perRepoLimit, displayLimit, query, page) => {
|
||||
if (isGitHubWorkItemsQueryTooLarge(query)) {
|
||||
return { items: [], failedCount: 0, issueErrorTypes: [] }
|
||||
return { items: [], failedCount: 0, errorTypes: [] }
|
||||
}
|
||||
let failedCount = 0
|
||||
const issueErrorTypes: ClassifiedError['type'][] = []
|
||||
const errorTypes: ClassifiedError['type'][] = []
|
||||
const perProjectResults = await Promise.all(
|
||||
repos.map(async (r) => {
|
||||
const requestState = get()
|
||||
@@ -2824,7 +2824,7 @@ export const createGitHubSlice: StateCreator<AppState, [], [], GitHubSlice> = (s
|
||||
const { type, message } = envelope.errors.issues
|
||||
// Why: only the 1000-result-window 422 may drive the unreachable
|
||||
// clamp; demote other validation errors so they read as failures.
|
||||
issueErrorTypes.push(
|
||||
errorTypes.push(
|
||||
type === 'validation_error' && !/first 1000 search results/i.test(message)
|
||||
? 'unknown'
|
||||
: type
|
||||
@@ -2834,6 +2834,16 @@ export const createGitHubSlice: StateCreator<AppState, [], [], GitHubSlice> = (s
|
||||
envelope.errors.issues
|
||||
)
|
||||
}
|
||||
if (envelope.errors?.prs) {
|
||||
// Why: the window 422 is issue-side only — a PR-side validation
|
||||
// error must never join the unreachable signal.
|
||||
const { type } = envelope.errors.prs
|
||||
errorTypes.push(type === 'validation_error' ? 'unknown' : type)
|
||||
console.warn(
|
||||
`[workItems] next page ${r.repoId} prs-side partial failure:`,
|
||||
envelope.errors.prs
|
||||
)
|
||||
}
|
||||
return envelope.items.map((item): GitHubWorkItem => ({ ...item, repoId: r.repoId }))
|
||||
} catch (err) {
|
||||
if (isGitHubWorkItemsSshRemoteRequiredError(err)) {
|
||||
@@ -2848,7 +2858,7 @@ export const createGitHubSlice: StateCreator<AppState, [], [], GitHubSlice> = (s
|
||||
})
|
||||
)
|
||||
const merged = sortWorkItemsByNumber(perProjectResults.flat()).slice(0, displayLimit)
|
||||
return { items: merged, failedCount, issueErrorTypes }
|
||||
return { items: merged, failedCount, errorTypes }
|
||||
},
|
||||
|
||||
countWorkItemsAcrossRepos: async (repos, query, perRepoLimit) => {
|
||||
|
||||
@@ -2055,6 +2055,7 @@ export type ListWorkItemsResult<T> = {
|
||||
}
|
||||
errors?: {
|
||||
issues?: ClassifiedError
|
||||
prs?: ClassifiedError
|
||||
}
|
||||
/** True when the user's per-repo preference was `'upstream'` but no upstream
|
||||
* remote is configured, so the resolver fell back to origin. Renderer uses
|
||||
|
||||
Reference in New Issue
Block a user