fix(tasks): only an all-window-422 empty page may clamp; harden count merges

Round-4 review: a sibling repo's envelope 403/404 arrives with
failedCount still 0, so the window branch now requires every error to be
the window 422 (non-window validation errors are demoted at the store);
the count resolution mins against an applied clamp instead of
re-advertising withdrawn pages; the generation effect mirrors
taskResumeApplied so its doc claim holds.
This commit is contained in:
Brennan Benson
2026-07-30 08:07:50 -07:00
parent e5c6367ea0
commit bf53b53d4c
5 changed files with 64 additions and 8 deletions
+7 -2
View File
@@ -3835,7 +3835,8 @@ export default function TaskPage(): React.JSX.Element {
workItemsInvalidationNonce,
taskRefreshNonce,
taskSource,
githubMode
githubMode,
taskResumeApplied
])
// Why: the dialog's "Use" button routes through the same direct-launch flow as the row-level "Use" CTA so behavior is consistent regardless of entry point.
@@ -6432,7 +6433,11 @@ export default function TaskPage(): React.JSX.Element {
githubPerRepoPageLimit
).then(({ totalPages: countedPages }) => {
if (!cancelled) {
setCountedTotalPages(countedPages)
// 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
)
}
})
@@ -80,10 +80,10 @@ describe('resolveEmptyPageOutcome', () => {
).toEqual({ reason: 'window-unreachable', clampTotalPagesTo: 33 })
})
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 — and the transient
// failure is the actionable signal, so it wins the toast too.
it('resolves as load-failed when a window 422 coincides with a sibling failure', () => {
// Repos advance independently; another repo's failure — thrown or on the
// envelope channel — says nothing about the healthy repos' remaining
// pages, and the transient failure is the actionable signal.
expect(
resolveEmptyPageOutcome({
...base,
@@ -92,6 +92,13 @@ describe('resolveEmptyPageOutcome', () => {
issueErrorTypes: ['validation_error']
})
).toEqual({ reason: 'load-failed', clampTotalPagesTo: null })
expect(
resolveEmptyPageOutcome({
...base,
target: 5,
issueErrorTypes: ['validation_error', 'permission_denied']
})
).toEqual({ reason: 'load-failed', clampTotalPagesTo: null })
})
it('reports a failure without clamping for transient or unclassified errors', () => {
@@ -65,7 +65,13 @@ export function resolveEmptyPageOutcome(args: {
countedTotalPages: number | null
}): EmptyPageOutcome {
const clamp = Math.max(1, Math.floor(args.target))
if (args.issueErrorTypes.includes('validation_error') && args.failedCount === 0) {
// Why: every error must be the window 422 — a sibling repo's envelope
// 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')
if (onlyWindowErrors && args.failedCount === 0) {
return { reason: 'window-unreachable', clampTotalPagesTo: clamp }
}
if (args.failedCount > 0 || args.issueErrorTypes.length > 0) {
@@ -6694,6 +6694,37 @@ describe('createGitHubSlice.fetchWorkItems source/error envelope', () => {
expect(result).toEqual({ items: [], failedCount: 0, issueErrorTypes: ['validation_error'] })
})
it('demotes non-window validation errors so they cannot drive the unreachable clamp', async () => {
runtimeEnvironmentCall.mockResolvedValueOnce({
id: 'rpc-work-items-page-422-other',
ok: true,
result: {
items: [],
sources: {
issues: { owner: 'up', repo: 'r' },
prs: null,
originCandidate: { owner: 'up', repo: 'r' },
upstreamCandidate: null
},
errors: {
issues: { type: 'validation_error', message: 'Validation Failed: query is malformed' }
}
},
_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<AppState>)
const result = await store
.getState()
.fetchWorkItemsNextPage([{ repoId: 'caller-repo-id', path: '/server/repo' }], 24, 100, '', 2)
expect(result).toEqual({ items: [], failedCount: 0, issueErrorTypes: ['unknown'] })
})
it('routes work-item counts through the active runtime environment', async () => {
runtimeEnvironmentCall.mockResolvedValueOnce({
id: 'rpc-work-items-count',
+8 -1
View File
@@ -2821,7 +2821,14 @@ export const createGitHubSlice: StateCreator<AppState, [], [], GitHubSlice> = (s
})
// Why: page-N failures aren't in the per-repo banner (keyed on the initial fetch); log them so pagination failures are observable instead of silently truncating (richer surface deferred, design doc §6).
if (envelope.errors?.issues) {
issueErrorTypes.push(envelope.errors.issues.type)
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(
type === 'validation_error' && !/first 1000 search results/i.test(message)
? 'unknown'
: type
)
console.warn(
`[workItems] next page ${r.repoId} issues-side partial failure:`,
envelope.errors.issues