diff --git a/src/renderer/src/components/TaskPage.tsx b/src/renderer/src/components/TaskPage.tsx index acbe1a6b0e1..835a9ba26a6 100644 --- a/src/renderer/src/components/TaskPage.tsx +++ b/src/renderer/src/components/TaskPage.tsx @@ -205,6 +205,7 @@ import { } from '@/components/task-page-cache-selectors' import { shouldHideTaskPageListChrome } from '@/components/task-page-list-chrome-visibility' import { + applyEmptyPageClamp, buildSelectedReposKey, getTaskPagePerRepoLimit, resolveEmptyPageOutcome, @@ -6190,13 +6191,15 @@ export default function TaskPage(): React.JSX.Element { if (items.length === 0) { // Why: see resolveEmptyPageOutcome — a dead click needs feedback only // when something actually failed; a clean empty probe is end-of-data. - const outcome = resolveEmptyPageOutcome({ + // The reason never depends on the count, so it's safe to derive here; + // the clamp is not (see applyEmptyPageClamp) and runs in the updater. + const { reason } = resolveEmptyPageOutcome({ target, failedCount, issueErrorTypes, - countedTotalPages + countedTotalPages: null }) - if (outcome.reason === 'window-unreachable') { + if (reason === 'window-unreachable') { toast.error( translate( 'auto.components.TaskPage.loadPageUnreachable', @@ -6205,7 +6208,7 @@ export default function TaskPage(): React.JSX.Element { ), { id: 'work-items-page-unreachable' } ) - } else if (outcome.reason === 'load-failed') { + } else if (reason === 'load-failed') { toast.error( translate( 'auto.components.TaskPage.loadPageFailed', @@ -6215,14 +6218,9 @@ export default function TaskPage(): React.JSX.Element { { id: 'work-items-page-load-failed' } ) } - const clamp = outcome.clampTotalPagesTo - if (clamp !== null) { - // Why: 0 means the count itself failed (fallback path) — replace it - // like null, or the speculative fallback page is never withdrawn. - setCountedTotalPages((previous) => - previous !== null && previous > 0 && previous < clamp ? previous : clamp - ) - } + setCountedTotalPages((previous) => + applyEmptyPageClamp(previous, { target, failedCount, issueErrorTypes }) + ) return } setPages((previous) => { @@ -6247,7 +6245,6 @@ 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 a54161fd9a1..52bc565f581 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 @@ -2,6 +2,7 @@ import { describe, expect, it, vi } from 'vitest' import type { GitHubWorkItem } from '../../../shared/types' import { accumulateWorkItemPages, + applyEmptyPageClamp, buildSelectedReposKey, getTaskPagePerRepoLimit, resolveEmptyPageOutcome, @@ -79,9 +80,10 @@ describe('resolveEmptyPageOutcome', () => { ).toEqual({ reason: 'window-unreachable', clampTotalPagesTo: 33 }) }) - it('does not clamp on a window 422 when a sibling repo fetch threw', () => { + it('resolves as load-failed when a window 422 coincides with a thrown repo fetch', () => { // Repos advance independently; another repo's transient failure says - // nothing about the healthy repos' remaining pages. + // nothing about the healthy repos' remaining pages — and the transient + // failure is the actionable signal, so it wins the toast too. expect( resolveEmptyPageOutcome({ ...base, @@ -89,7 +91,7 @@ describe('resolveEmptyPageOutcome', () => { failedCount: 1, issueErrorTypes: ['validation_error'] }) - ).toEqual({ reason: 'window-unreachable', clampTotalPagesTo: null }) + ).toEqual({ reason: 'load-failed', clampTotalPagesTo: null }) }) it('reports a failure without clamping for transient or unclassified errors', () => { @@ -129,6 +131,40 @@ describe('resolveEmptyPageOutcome', () => { }) }) +describe('applyEmptyPageClamp', () => { + const clean = { failedCount: 0, issueErrorTypes: [] as const } + + it('keeps a real count that resolved between click and response', () => { + // The count promise routinely lands mid-flight; the committed value, not + // the click-time closure, must decide whether end-of-data may clamp. + expect(applyEmptyPageClamp(33, { ...clean, target: 2 })).toBe(33) + }) + + it('clamps while the count is unknown or failed', () => { + expect(applyEmptyPageClamp(null, { ...clean, target: 2 })).toBe(2) + expect(applyEmptyPageClamp(0, { ...clean, target: 2 })).toBe(2) + }) + + it('never raises an earlier clamp', () => { + // A prior probe clamped to 20; a later window 422 at page 33 must not + // re-advertise pages 21-33. + expect( + applyEmptyPageClamp(20, { target: 33, failedCount: 0, issueErrorTypes: ['validation_error'] }) + ).toBe(20) + }) + + it('clamps a real count on a window 422 — the count over-advertising is the bug', () => { + expect( + applyEmptyPageClamp(39, { target: 33, failedCount: 0, issueErrorTypes: ['validation_error'] }) + ).toBe(33) + }) + + it('leaves the count alone on failures', () => { + expect(applyEmptyPageClamp(33, { target: 5, failedCount: 2, issueErrorTypes: [] })).toBe(33) + expect(applyEmptyPageClamp(null, { target: 5, failedCount: 2, issueErrorTypes: [] })).toBe(null) + }) +}) + describe('buildSelectedReposKey', () => { const contextFor = (r: { id: string }) => ({ provider: 'github', repoId: r.id }) 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 1a7cf450a78..89a90f36b47 100644 --- a/src/renderer/src/components/task-page-work-item-pagination.ts +++ b/src/renderer/src/components/task-page-work-item-pagination.ts @@ -45,8 +45,11 @@ export type EmptyPageOutcome = { * An empty page load has three distinct meanings, and only the caller-side * error channels can tell them apart (#11485): * - a `validation_error` is GitHub's 422 for pages past its 1000-result search - * window — the page can never load, so stop advertising it (unless a sibling - * repo's fetch threw, which says nothing about the other repos' windows); + * 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; * - no error at all is end-of-data. That only warrants a clamp while the count @@ -62,11 +65,8 @@ export function resolveEmptyPageOutcome(args: { countedTotalPages: number | null }): EmptyPageOutcome { const clamp = Math.max(1, Math.floor(args.target)) - if (args.issueErrorTypes.includes('validation_error')) { - return { - reason: 'window-unreachable', - clampTotalPagesTo: args.failedCount === 0 ? clamp : null - } + if (args.issueErrorTypes.includes('validation_error') && args.failedCount === 0) { + return { reason: 'window-unreachable', clampTotalPagesTo: clamp } } if (args.failedCount > 0 || args.issueErrorTypes.length > 0) { return { reason: 'load-failed', clampTotalPagesTo: null } @@ -75,6 +75,28 @@ export function resolveEmptyPageOutcome(args: { return { reason: 'end-of-data', clampTotalPagesTo: countUnknown ? clamp : null } } +/** + * Functional-updater body for the advertised-count clamp. Must be evaluated + * against the COMMITTED count (React updater `previous`), not a click-time + * closure — the count promise routinely resolves between click and response, + * and a stale-null closure would let an end-of-data clamp overwrite a real + * count. Math.min preserves the never-raise property of earlier clamps. + */ +export function applyEmptyPageClamp( + previous: number | null, + args: { + target: number + failedCount: number + issueErrorTypes: readonly ClassifiedError['type'][] + } +): number | null { + const next = resolveEmptyPageOutcome({ ...args, countedTotalPages: previous }).clampTotalPagesTo + if (next === null) { + return previous + } + return previous !== null && previous > 0 ? Math.min(previous, next) : next +} + /** * Stable identity string for a repo selection. The repos store installs a fresh * array on every repos:changed event, so effects keyed on array identity re-fire