mirror of
https://github.com/stablyai/orca.git
synced 2026-10-20 00:03:45 +00:00
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.
This commit is contained in:
@@ -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<number | null>(null)
|
||||
const [countedTotalPages, setCountedTotalPages] = useState<number | null>(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<number | null>(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)
|
||||
}
|
||||
})
|
||||
|
||||
|
||||
@@ -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)
|
||||
})
|
||||
})
|
||||
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
Reference in New Issue
Block a user