diff --git a/src/main/github/client.ts b/src/main/github/client.ts index f1df222b93d..d248a34d169 100644 --- a/src/main/github/client.ts +++ b/src/main/github/client.ts @@ -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 => { @@ -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: { diff --git a/src/main/github/gh-error-classification.ts b/src/main/github/gh-error-classification.ts index 2de37919d25..4d2453899ac 100644 --- a/src/main/github/gh-error-classification.ts +++ b/src/main/github/gh-error-classification.ts @@ -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()}` } +} diff --git a/src/main/github/gh-utils.ts b/src/main/github/gh-utils.ts index d2148edc3c1..d6770189924 100644 --- a/src/main/github/gh-utils.ts +++ b/src/main/github/gh-utils.ts @@ -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, diff --git a/src/renderer/src/components/TaskPage.tsx b/src/renderer/src/components/TaskPage.tsx index 35b7ad47aca..e022e792f1a 100644 --- a/src/renderer/src/components/TaskPage.tsx +++ b/src/renderer/src/components/TaskPage.tsx @@ -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, diff --git a/src/renderer/src/components/task-page-work-item-pagination.test.ts b/src/renderer/src/components/task-page-work-item-pagination.test.ts index 393309ab73b..670c862be98 100644 --- a/src/renderer/src/components/task-page-work-item-pagination.test.ts +++ b/src/renderer/src/components/task-page-work-item-pagination.test.ts @@ -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) }) }) diff --git a/src/renderer/src/components/task-page-work-item-pagination.ts b/src/renderer/src/components/task-page-work-item-pagination.ts index 362736f941f..8209b61bdf0 100644 --- a/src/renderer/src/components/task-page-work-item-pagination.ts +++ b/src/renderer/src/components/task-page-work-item-pagination.ts @@ -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 }) diff --git a/src/renderer/src/store/slices/github.test.ts b/src/renderer/src/store/slices/github.test.ts index f2a56e12c0e..8ad3b952723 100644 --- a/src/renderer/src/store/slices/github.test.ts +++ b/src/renderer/src/store/slices/github.test.ts @@ -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) + + 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() diff --git a/src/renderer/src/store/slices/github.ts b/src/renderer/src/store/slices/github.ts index 6f5d2efb7c7..109c72a1ffd 100644 --- a/src/renderer/src/store/slices/github.ts +++ b/src/renderer/src/store/slices/github.ts @@ -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 = (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 = (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 = (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 = (s }) ) const merged = sortWorkItemsByNumber(perProjectResults.flat()).slice(0, displayLimit) - return { items: merged, failedCount, issueErrorTypes } + return { items: merged, failedCount, errorTypes } }, countWorkItemsAcrossRepos: async (repos, query, perRepoLimit) => { diff --git a/src/shared/types.ts b/src/shared/types.ts index cbbfa0d8776..8022d05abf9 100644 --- a/src/shared/types.ts +++ b/src/shared/types.ts @@ -2055,6 +2055,7 @@ export type ListWorkItemsResult = { } 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