mirror of
https://github.com/stablyai/orca.git
synced 2026-10-19 16:03:26 +00:00
fix(tasks): clamp against the committed count, not the click-time closure
Round-3 review: the count promise routinely resolves between click and response, so deciding the end-of-data clamp from the closure value let a stale null overwrite a real count. applyEmptyPageClamp now runs inside the functional updater against the committed value, never raises an earlier clamp, and a window 422 coinciding with a thrown sibling repo resolves as load-failed so the toast and the clamp always agree.
This commit is contained in:
@@ -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,
|
||||
|
||||
@@ -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 })
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user