From f7a343e77b3d791a364f3bf3fcae47b1c3adfa4b Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Thu, 30 Jul 2026 08:24:18 -0700 Subject: [PATCH] fix(tasks): split the proven window limit from the count slot Round-5 review: min-ing the count against an applied clamp pinned a SPECULATIVE end-of-data withdrawal that raced ahead of the count, permanently collapsing the bar for the generation. Proven window-422 limits now live in provenPageLimit (set once, only lowered, reset per generation); the count overwrites its own slot unconditionally; and deriveAdvertisedTotalPages (unit-tested for both arrival orders) caps the count-or-fallback estimate with the proven limit, floored at the loaded pages. --- src/renderer/src/components/TaskPage.tsx | 34 +++++--- .../task-page-work-item-pagination.test.ts | 84 ++++++++++++++++--- .../task-page-work-item-pagination.ts | 44 ++++++++-- 3 files changed, 129 insertions(+), 33 deletions(-) diff --git a/src/renderer/src/components/TaskPage.tsx b/src/renderer/src/components/TaskPage.tsx index bcd96ae0810..35b7ad47aca 100644 --- a/src/renderer/src/components/TaskPage.tsx +++ b/src/renderer/src/components/TaskPage.tsx @@ -206,7 +206,9 @@ import { import { shouldHideTaskPageListChrome } from '@/components/task-page-list-chrome-visibility' import { applyEmptyPageClamp, + applyWindowPageLimit, buildSelectedReposKey, + deriveAdvertisedTotalPages, getTaskPagePerRepoLimit, resolveEmptyPageOutcome, taskPageToGitHubApiPage @@ -3817,6 +3819,10 @@ export default function TaskPage(): React.JSX.Element { const [paginationLoading, setPaginationLoading] = useState(false) const [loadingTargetPage, setLoadingTargetPage] = useState(null) const [countedTotalPages, setCountedTotalPages] = useState(null) + // Proven window-422 page limit — separate from the count so a late count + // can't resurrect proven-unreachable pages, nor be pinned by a speculative + // withdrawal (see deriveAdvertisedTotalPages). + const [provenPageLimit, setProvenPageLimit] = useState(null) const fetchWorkItemsNextPage = useAppStore((s) => s.fetchWorkItemsNextPage) const countWorkItemsAcrossRepos = useAppStore((s) => s.countWorkItemsAcrossRepos) @@ -6155,10 +6161,12 @@ export default function TaskPage(): React.JSX.Element { const fallbackTotalPages = lastLoadedPageFull ? Math.max(pages.length, lastLoadedPageIndex + 2) : Math.max(1, pages.length) - const totalPages = - countedTotalPages && countedTotalPages > 0 - ? Math.max(pages.length, countedTotalPages) - : fallbackTotalPages + const totalPages = deriveAdvertisedTotalPages({ + loadedPages: pages.length, + countedTotalPages, + fallbackTotalPages, + provenPageLimit + }) // Why: load only the clicked page so a high-page jump doesn't exhaust GitHub's Search API rate bucket. const handleLoadNextPage = useCallback( @@ -6209,6 +6217,7 @@ export default function TaskPage(): React.JSX.Element { ), { id: 'work-items-page-unreachable' } ) + setProvenPageLimit((previous) => applyWindowPageLimit(previous, target)) } else if (reason === 'load-failed') { toast.error( translate( @@ -6218,10 +6227,11 @@ export default function TaskPage(): React.JSX.Element { ), { id: 'work-items-page-load-failed' } ) + } else { + setCountedTotalPages((previous) => + applyEmptyPageClamp(previous, { target, failedCount, issueErrorTypes }) + ) } - setCountedTotalPages((previous) => - applyEmptyPageClamp(previous, { target, failedCount, issueErrorTypes }) - ) return } setPages((previous) => { @@ -6329,6 +6339,7 @@ export default function TaskPage(): React.JSX.Element { setPages([page0]) setCurrentPage(0) setCountedTotalPages(null) + setProvenPageLimit(null) setTasksError(null) setFailedCount(0) // reset so a prior failure banner doesn't linger setGithubUnavailable(false) @@ -6433,11 +6444,10 @@ export default function TaskPage(): React.JSX.Element { githubPerRepoPageLimit ).then(({ totalPages: countedPages }) => { if (!cancelled) { - // Why: min against an already-applied clamp — the count must not - // re-advertise pages a probe has proven unreachable this generation. - setCountedTotalPages((previous) => - previous !== null && previous > 0 ? Math.min(previous, countedPages) : countedPages - ) + // Why: the count overwrites unconditionally — proven window limits live + // in provenPageLimit, so a late count can't be pinned by a speculative + // end-of-data withdrawal, and can't resurrect proven-dead pages either. + setCountedTotalPages(countedPages) } }) 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 c459c9b028f..393309ab73b 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 @@ -3,7 +3,9 @@ import type { GitHubWorkItem } from '../../../shared/types' import { accumulateWorkItemPages, applyEmptyPageClamp, + applyWindowPageLimit, buildSelectedReposKey, + deriveAdvertisedTotalPages, getTaskPagePerRepoLimit, resolveEmptyPageOutcome, taskPageToGitHubApiPage, @@ -147,28 +149,84 @@ describe('applyEmptyPageClamp', () => { expect(applyEmptyPageClamp(33, { ...clean, target: 2 })).toBe(33) }) - it('clamps while the count is unknown or failed', () => { + it('withdraws the speculative page 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', () => { + it('never touches the count for window or failure outcomes', () => { + // Window 422 limits live in provenPageLimit (applyWindowPageLimit), not + // the count slot. expect( applyEmptyPageClamp(39, { target: 33, failedCount: 0, issueErrorTypes: ['validation_error'] }) + ).toBe(39) + expect(applyEmptyPageClamp(33, { target: 5, failedCount: 2, issueErrorTypes: [] })).toBe(33) + expect(applyEmptyPageClamp(null, { target: 5, failedCount: 2, issueErrorTypes: [] })).toBe(null) + }) +}) + +describe('applyWindowPageLimit', () => { + it('sets, only lowers, and never goes below one page', () => { + expect(applyWindowPageLimit(null, 33)).toBe(33) + expect(applyWindowPageLimit(33, 20)).toBe(20) + expect(applyWindowPageLimit(20, 33)).toBe(20) + expect(applyWindowPageLimit(null, 0)).toBe(1) + }) +}) + +describe('deriveAdvertisedTotalPages', () => { + it('is order-independent: a count landing after a speculative withdrawal wins', () => { + // Probe first: end-of-data wrote countedTotalPages=1 (speculative). Count + // then overwrites with 39 — the bar must show 39, not stay collapsed. + expect( + deriveAdvertisedTotalPages({ + loadedPages: 1, + countedTotalPages: 39, + fallbackTotalPages: 2, + provenPageLimit: null + }) + ).toBe(39) + }) + + it('caps a real count with the proven window limit', () => { + expect( + deriveAdvertisedTotalPages({ + loadedPages: 3, + countedTotalPages: 39, + fallbackTotalPages: 4, + provenPageLimit: 33 + }) ).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) + it('uses the fallback while the count is unknown/failed, still window-capped', () => { + expect( + deriveAdvertisedTotalPages({ + loadedPages: 1, + countedTotalPages: null, + fallbackTotalPages: 2, + provenPageLimit: null + }) + ).toBe(2) + expect( + deriveAdvertisedTotalPages({ + loadedPages: 1, + countedTotalPages: 0, + fallbackTotalPages: 5, + provenPageLimit: 3 + }) + ).toBe(3) + }) + + it('never advertises fewer pages than are loaded', () => { + expect( + deriveAdvertisedTotalPages({ + loadedPages: 5, + countedTotalPages: 2, + fallbackTotalPages: 2, + provenPageLimit: 2 + }) + ).toBe(5) }) }) 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 d032f7e028d..362736f941f 100644 --- a/src/renderer/src/components/task-page-work-item-pagination.ts +++ b/src/renderer/src/components/task-page-work-item-pagination.ts @@ -82,11 +82,13 @@ export function resolveEmptyPageOutcome(args: { } /** - * 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. + * Functional-updater body for the SPECULATIVE end-of-data withdrawal only. + * Must be evaluated against the COMMITTED count (React updater `previous`), + * not a click-time closure — the count promise routinely resolves between + * click and response. Window 422 clamps are PROVEN and live in their own + * state (applyWindowPageLimit); keeping them out of the count slot lets a + * later real count overwrite the speculative value instead of being pinned + * under it. */ export function applyEmptyPageClamp( previous: number | null, @@ -96,11 +98,37 @@ export function applyEmptyPageClamp( issueErrorTypes: readonly ClassifiedError['type'][] } ): number | null { - const next = resolveEmptyPageOutcome({ ...args, countedTotalPages: previous }).clampTotalPagesTo - if (next === null) { + const outcome = resolveEmptyPageOutcome({ ...args, countedTotalPages: previous }) + if (outcome.reason !== 'end-of-data' || outcome.clampTotalPagesTo === null) { return previous } - return previous !== null && previous > 0 ? Math.min(previous, next) : next + return outcome.clampTotalPagesTo +} + +/** Proven window 422 limit: set once, only ever lowered, reset per generation. */ +export function applyWindowPageLimit(previous: number | null, target: number): number { + const clamp = Math.max(1, Math.floor(target)) + return previous === null ? clamp : Math.min(previous, clamp) +} + +/** + * Advertised page count = the count-or-fallback estimate, capped by the proven + * window limit, floored at the loaded pages. Splitting the proven cap from the + * count slot makes the result order-independent: a count arriving after a + * speculative withdrawal overwrites it, while a proven limit survives. + */ +export function deriveAdvertisedTotalPages(args: { + loadedPages: number + countedTotalPages: number | null + fallbackTotalPages: number + provenPageLimit: number | null +}): number { + const uncapped = + args.countedTotalPages && args.countedTotalPages > 0 + ? Math.max(args.loadedPages, args.countedTotalPages) + : args.fallbackTotalPages + const capped = args.provenPageLimit === null ? uncapped : Math.min(uncapped, args.provenPageLimit) + return Math.max(args.loadedPages, capped) } /**