fix(tasks): make GitHub pagination honest — cap unreachable pages, survive background refreshes, explain empty pages (#11584)
* fix(tasks): cap advertised GitHub pages at the search result window GitHub's Search API rejects requests past its first-1000-results window with HTTP 422, but totalPages was derived from the raw total_count, so the pagination bar advertised pages that could never load and clicks on them silently did nothing (#11485). Cap per-repo advertised pages at floor(1000 / perRepoLimit), and when a page load comes back empty, say so with a toast instead of ignoring the click — clamping the advertised count only when no fetch threw, so transient failures don't shrink the bar. * fix(tasks): key pagination resets on repo selection, not array identity The repos store installs a fresh array on every repos:changed event, so the pagination-reset effect fired on background refreshes and bumped the request generation, silently discarding any in-flight page navigation — clicking an unloaded page did nothing whenever a repo refresh landed during the fetch. Key the effect on the stable selection string instead. * fix(tasks): distinguish end-of-data, window 422s, and failures on empty pages Adversarial-review round 1 rework: - fetchWorkItemsNextPage now returns issue-side envelope error types — the channel the search-window 422 actually travels on (failedCount only counts thrown repo calls). - resolveEmptyPageOutcome (unit-tested) maps an empty page to window-unreachable (clamp + toast), load-failed (toast only; may be transient), or end-of-data (silently withdraw the speculative page the count-fallback advertises). - The work-items fetch effect is keyed on selectedReposKey too — its unconditional page reset re-fired on every repos:changed array identity, bouncing the user to page 1 mid-click. The key now includes the resolved GitHub source context so identity changes still re-dispatch. - Toasts carry stable ids so repeats replace instead of stack. - Cap comment documents the conservative PR-scope tail loss; cap tests pinned at shipped (36 → 27) and dividing (25 → 40) limits. * fix(tasks): withdraw the speculative page when the failed count is zero countedTotalPages of 0 comes from a swallowed count failure and routes totalPages through the fallback, so the clamp must replace it like null. * fix(tasks): tighten empty-page outcomes after round-2 review - en.json's loadPageUnreachable carried the pre-reword text, and the catalog beats the inline default — the two toasts were identical. - end-of-data clamps only while the count is unknown/failed: the PR list path swallows its own failures into clean-empty results, and clamping a real count silently hid healthy pages (worse than the pre-fix no-op). - A window 422 no longer clamps when a sibling repo's fetch threw. - The generation effect mirrors every fetch-effect dep that resets page state, so manual refresh/source switches invalidate in-flight clicks. - selectedReposKey extracted as buildSelectedReposKey with stability tests; envelope error types wire-tested through the store. * 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. * 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. * 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. * fix(tasks): surface PR-side list failures so they can't read as end-of-data Round-6 review: PartialWorkItemsResult had no PR error slot, so a swallowed gh pr list failure reached the renderer as a clean empty page — and with the count blocked (0) the speculative withdrawal deleted the pagination bar with no toast and no recovery (a regression vs main's silent no-op). PR-side errors now ride the envelope (errors.prs), demoted so they can never join the issue-only window-422 signal; errorTypes replaces issueErrorTypes; an empty page that a real count said should exist now toasts instead of looking dead. * test(tasks): cover the PR-error envelope end-to-end; neutral no-more-results toast Round-7 review: the two literal gh-utils mocks lacked classifyListPrsError (a PR-side rejection in those suites would TypeError instead of assert), and the producer half of the errors.prs contract had no main-side test — added both, plus a classifier contract test pinning the search-window phrase the renderer keys on. The refused-clamp toast now reads the committed count via a synchronous ref mirror instead of the click-time closure, and says 'No more results' — nothing failed on that branch. Both toast keys plus the new one are translated in es/ja/ko/zh. * fix(tasks): preserve final reachable GitHub search page * Extract GitHub search result window error pattern to shared constant Extract the 1000-result window detection pattern to a single source of truth so the classifier and consumer stay synchronized. The pattern is the only signal separating a permanently unreachable page from a transient validation failure, so drift or trimming silently demotes window 422s to generic failures and stops capping the advertised page count (#11485). --------- Co-authored-by: Jinjing <6427696+AmethystLiang@users.noreply.github.com>
This commit is contained in:
Brennan Benson
GitHub
parent
98ae8e4c8c
commit
56ab5fd1dc
|
|
@ -63,7 +63,8 @@ vi.mock('./gh-utils', () => ({
|
|||
release: releaseMock,
|
||||
_resetOwnerRepoCache: vi.fn(),
|
||||
classifyGhError: (stderr: string) => ({ type: 'unknown', message: stderr }),
|
||||
classifyListIssuesError: (stderr: string) => ({ type: 'unknown', message: stderr })
|
||||
classifyListIssuesError: (stderr: string) => ({ type: 'unknown', message: stderr }),
|
||||
classifyListPrsError: (stderr: string) => ({ type: 'unknown', message: stderr })
|
||||
}))
|
||||
|
||||
vi.mock('../git/runner', () => ({
|
||||
|
|
@ -266,6 +267,22 @@ describe('listWorkItems query paging', () => {
|
|||
expect(items.map((item) => item.number)).toEqual([4, 3])
|
||||
})
|
||||
|
||||
it('lifts a swallowed PR-side failure onto errors.prs instead of reading as end-of-data', async () => {
|
||||
getIssueOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' })
|
||||
getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' })
|
||||
// Non-availability failure (plain 403): swallowed into [], must still surface.
|
||||
ghExecFileAsyncMock.mockRejectedValueOnce(new Error('HTTP 403: Forbidden'))
|
||||
|
||||
const envelope = await listWorkItems('/repo-root', 10, 'is:pr is:open', 2)
|
||||
|
||||
expect(envelope.items).toEqual([])
|
||||
expect(envelope.errors?.issues).toBeUndefined()
|
||||
expect(envelope.errors?.prs).toEqual({
|
||||
type: 'unknown',
|
||||
message: expect.stringContaining('HTTP 403')
|
||||
})
|
||||
})
|
||||
|
||||
it('filters pull request rows out of issue Search API results', async () => {
|
||||
getIssueOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' })
|
||||
getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' })
|
||||
|
|
|
|||
|
|
@ -55,7 +55,8 @@ vi.mock('./gh-utils', () => ({
|
|||
release: releaseMock,
|
||||
_resetOwnerRepoCache: vi.fn(),
|
||||
classifyGhError: (stderr: string) => ({ type: 'unknown', message: stderr }),
|
||||
classifyListIssuesError: (stderr: string) => ({ type: 'unknown', message: stderr })
|
||||
classifyListIssuesError: (stderr: string) => ({ type: 'unknown', message: stderr }),
|
||||
classifyListPrsError: (stderr: string) => ({ type: 'unknown', message: stderr })
|
||||
}))
|
||||
|
||||
vi.mock('../git/runner', () => ({
|
||||
|
|
|
|||
|
|
@ -55,6 +55,7 @@ import {
|
|||
release,
|
||||
classifyGhError,
|
||||
classifyListIssuesError,
|
||||
classifyListPrsError,
|
||||
ghRepoExecOptions,
|
||||
githubRepoContext,
|
||||
getRemoteUrlForRepo,
|
||||
|
|
@ -1122,10 +1123,11 @@ function buildWorkItemListRequest(args: {
|
|||
return { args: out, offset: (page - 1) * limit }
|
||||
}
|
||||
|
||||
// Why: shared shape so listWorkItems can lift the issue-side error (#1076 silent wrongness) into the IPC envelope; PR errors out of scope (§6).
|
||||
// Why: shared shape so listWorkItems can lift per-side errors (#1076 silent wrongness) into the IPC envelope — a swallowed side reads as end-of-data to pagination (#11485).
|
||||
type PartialWorkItemsResult = {
|
||||
items: MainWorkItem[]
|
||||
issuesError?: ClassifiedError
|
||||
prsError?: ClassifiedError
|
||||
}
|
||||
|
||||
function assertSshRepoHasResolvedGitHubSource(args: {
|
||||
|
|
@ -1279,6 +1281,7 @@ async function listQueriedWorkItems(
|
|||
let successfulRequestCount = 0
|
||||
let nonAvailabilityFailureCount = 0
|
||||
let availabilityError: unknown
|
||||
let prsError: ClassifiedError | undefined
|
||||
|
||||
// Why: surface the issue-side error separately for the IPC envelope; PR-side keeps prior swallow-and-log (parent doc §6).
|
||||
const issueFetch = (async (): Promise<PartialWorkItemsResult> => {
|
||||
|
|
@ -1350,6 +1353,7 @@ async function listQueriedWorkItems(
|
|||
} catch (err) {
|
||||
console.warn('listQueriedWorkItems PRs partial failure:', err)
|
||||
const stderr = err instanceof Error ? err.message : String(err)
|
||||
prsError = classifyListPrsError(stderr)
|
||||
if (classifyGitHubUnavailable(stderr)) {
|
||||
availabilityError ??= err
|
||||
} else {
|
||||
|
|
@ -1366,7 +1370,8 @@ async function listQueriedWorkItems(
|
|||
}
|
||||
return {
|
||||
items: sortWorkItemsByNumber([...issueResult.items, ...prItems]).slice(0, limit),
|
||||
issuesError: issueResult.issuesError
|
||||
issuesError: issueResult.issuesError,
|
||||
prsError
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -1424,7 +1429,13 @@ export async function listWorkItems(
|
|||
localGitOptions
|
||||
)
|
||||
|
||||
const errors = partial.issuesError ? { issues: partial.issuesError } : undefined
|
||||
const errors =
|
||||
partial.issuesError || partial.prsError
|
||||
? {
|
||||
...(partial.issuesError ? { issues: partial.issuesError } : {}),
|
||||
...(partial.prsError ? { prs: partial.prsError } : {})
|
||||
}
|
||||
: undefined
|
||||
return {
|
||||
items: partial.items,
|
||||
sources: {
|
||||
|
|
|
|||
|
|
@ -55,3 +55,11 @@ export function classifyListIssuesError(stderr: string): ClassifiedError {
|
|||
}
|
||||
return { type: c.type, message: readMessages[c.type] }
|
||||
}
|
||||
|
||||
// Why: PR-side list failures need the same read-op classification — pagination
|
||||
// decisions key on the type, and swallowing them made failures look like
|
||||
// end-of-data (#11485).
|
||||
export function classifyListPrsError(stderr: string): ClassifiedError {
|
||||
const c = classifyGhError(stderr)
|
||||
return { type: c.type, message: `Failed to load pull requests: ${stderr.trim()}` }
|
||||
}
|
||||
|
|
|
|||
|
|
@ -38,6 +38,7 @@ import {
|
|||
__resetLocalGitConfigSignatureCacheForTests,
|
||||
readLocalGitConfigSignature
|
||||
} from './local-git-config-signature'
|
||||
import { GITHUB_SEARCH_RESULT_WINDOW_ERROR_PATTERN } from '../../shared/github-work-items-query-bounds'
|
||||
|
||||
describe('github owner/repo resolution', () => {
|
||||
beforeEach(() => {
|
||||
|
|
@ -807,4 +808,17 @@ describe('gh error classification', () => {
|
|||
message: 'Issues are disabled on this repository.'
|
||||
})
|
||||
})
|
||||
|
||||
// Why: the renderer detects the Search API 1000-result window by matching
|
||||
// GITHUB_SEARCH_RESULT_WINDOW_ERROR_PATTERN against this message (#11485) —
|
||||
// trimming the raw stderr out of the validation_error copy, or drifting the
|
||||
// pattern off GitHub's real wording, would silently downgrade every window
|
||||
// 422 to a generic failure. The stderr stays verbatim so this pins both ends.
|
||||
it('keeps the search-window phrase in validation_error list messages', () => {
|
||||
const stderr =
|
||||
'Command failed: gh api --hostname github.com search/issues\nValidation Failed: Only the first 1000 search results are available (HTTP 422)'
|
||||
const classified = classifyListIssuesError(stderr)
|
||||
expect(classified.type).toBe('validation_error')
|
||||
expect(classified.message).toMatch(GITHUB_SEARCH_RESULT_WINDOW_ERROR_PATTERN)
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -9,7 +9,11 @@ import { extractExecError, parseRetryAfterMs } from '../git/exec-error'
|
|||
// WSL-aware routing. Repo-scoped callers should use the runner exports below.
|
||||
export const execFileAsync = promisify(execFile)
|
||||
export { ghExecFileAsync, gitExecFileAsync, extractExecError, parseRetryAfterMs }
|
||||
export { classifyGhError, classifyListIssuesError } from './gh-error-classification'
|
||||
export {
|
||||
classifyGhError,
|
||||
classifyListIssuesError,
|
||||
classifyListPrsError
|
||||
} from './gh-error-classification'
|
||||
export {
|
||||
_getOwnerRepoCacheSize,
|
||||
_resetOwnerRepoCache,
|
||||
|
|
|
|||
|
|
@ -205,7 +205,12 @@ import {
|
|||
} from '@/components/task-page-cache-selectors'
|
||||
import { shouldHideTaskPageListChrome } from '@/components/task-page-list-chrome-visibility'
|
||||
import {
|
||||
applyEmptyPageClamp,
|
||||
applyWindowPageLimit,
|
||||
buildSelectedReposKey,
|
||||
deriveAdvertisedTotalPages,
|
||||
getTaskPagePerRepoLimit,
|
||||
resolveEmptyPageOutcome,
|
||||
taskPageToGitHubApiPage
|
||||
} from '@/components/task-page-work-item-pagination'
|
||||
import { sortWorkItemsByNumber } from '../../../shared/work-items'
|
||||
|
|
@ -3175,6 +3180,18 @@ export default function TaskPage(): React.JSX.Element {
|
|||
[eligibleRepos, repoSelection]
|
||||
)
|
||||
|
||||
// Why: see buildSelectedReposKey — array-identity deps re-fire on every
|
||||
// repos:changed even when the selection is unchanged. The context part is
|
||||
// resolved as GitHub, but every provider-independent field (projectId,
|
||||
// hostId, projectHostSetupId, repoId) is identical across providers, so the
|
||||
// GitLab effect can key off this too — it passes no gitlabProjectRef, so its
|
||||
// context carries no providerIdentity of its own. Thread a projectRef into
|
||||
// that call and this key needs a GitLab-scoped part.
|
||||
const selectedReposKey = useMemo(
|
||||
() => buildSelectedReposKey(selectedRepos, (r) => getTaskPageRepoSourceContext(r, 'github')),
|
||||
[selectedRepos]
|
||||
)
|
||||
|
||||
// Why: many affordances need *a* repo; use the first selected as default, while cross-repo dialogs still let the user override per-action.
|
||||
const primaryRepo = selectedRepos[0] ?? null
|
||||
const linearWorkspaces = linearStatus.workspaces ?? []
|
||||
|
|
@ -3777,14 +3794,34 @@ 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)
|
||||
// Why: synchronous mirror of countedTotalPages — the empty-page branch needs
|
||||
// the committed value, not a click-time closure, and refs update immediately.
|
||||
const countedTotalPagesRef = useRef<number | null>(null)
|
||||
const fetchWorkItemsNextPage = useAppStore((s) => s.fetchWorkItemsNextPage)
|
||||
const countWorkItemsAcrossRepos = useAppStore((s) => s.countWorkItemsAcrossRepos)
|
||||
|
||||
// Why: keyed on selectedReposKey, not the selectedRepos array — a background
|
||||
// repos:changed refresh mid-flight would otherwise bump the generation and
|
||||
// silently discard the user's page navigation (#11485). Mirrors every dep of
|
||||
// the fetch effect that resets page state, so a reset always invalidates
|
||||
// in-flight page requests.
|
||||
useEffect(() => {
|
||||
paginationGenerationRef.current += 1
|
||||
setPaginationLoading(false)
|
||||
setLoadingTargetPage(null)
|
||||
}, [selectedRepos, appliedTaskSearch, workItemsInvalidationNonce])
|
||||
}, [
|
||||
selectedReposKey,
|
||||
appliedTaskSearch,
|
||||
workItemsInvalidationNonce,
|
||||
taskRefreshNonce,
|
||||
taskSource,
|
||||
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.
|
||||
const githubTaskDrawerWorkItem = useAppStore((s) => s.githubTaskDrawerWorkItem)
|
||||
|
|
@ -4799,15 +4836,6 @@ export default function TaskPage(): React.JSX.Element {
|
|||
jiraTaskSourceContext
|
||||
])
|
||||
|
||||
// Why: stable string key for selectedRepos so the GitLab effect doesn't re-run on every parent render from a new array ref.
|
||||
const selectedReposKey = useMemo(
|
||||
() =>
|
||||
selectedRepos
|
||||
.map((r) => `${r.id}|${r.path}|${r.connectionId ?? ''}|${r.executionHostId ?? ''}`)
|
||||
.join(','),
|
||||
[selectedRepos]
|
||||
)
|
||||
|
||||
// Why: fetch GitLab Issues and MRs separately so errors stay isolated per tab (mirrors GitHub's split endpoints).
|
||||
useEffect(() => {
|
||||
if (taskSource !== 'gitlab') {
|
||||
|
|
@ -4914,7 +4942,7 @@ export default function TaskPage(): React.JSX.Element {
|
|||
return () => {
|
||||
stale = true
|
||||
}
|
||||
// eslint-disable-next-line react-hooks/exhaustive-deps -- selectedReposKey encodes the only selectedRepos fields read above; keying off the array ref would re-run on every parent render.
|
||||
// eslint-disable-next-line react-hooks/exhaustive-deps -- selectedReposKey covers every selectedRepos field read above (see its GitHub-scoped-context note); keying off the array ref would re-run on every parent render.
|
||||
}, [taskSource, gitlabView, activeGitlabFilter, gitlabRefreshNonce, selectedReposKey])
|
||||
|
||||
// Why: Todos fetch has its own effect — different trigger (no chip filter) and data path (gl.todos is user-scoped, not repo-scoped).
|
||||
|
|
@ -6111,10 +6139,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(
|
||||
|
|
@ -6135,7 +6165,7 @@ export default function TaskPage(): React.JSX.Element {
|
|||
setPaginationLoading(true)
|
||||
setLoadingTargetPage(target)
|
||||
try {
|
||||
const { items } = await fetchWorkItemsNextPage(
|
||||
const { items, failedCount, errorTypes } = await fetchWorkItemsNextPage(
|
||||
repoArgs,
|
||||
githubPerRepoPageLimit,
|
||||
githubPageSize,
|
||||
|
|
@ -6146,6 +6176,55 @@ export default function TaskPage(): React.JSX.Element {
|
|||
return
|
||||
}
|
||||
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.
|
||||
// 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,
|
||||
errorTypes,
|
||||
countedTotalPages: null
|
||||
})
|
||||
if (reason === 'window-unreachable') {
|
||||
toast.error(
|
||||
translate(
|
||||
'auto.components.TaskPage.loadPageUnreachable',
|
||||
'Page {{value0}} is beyond what GitHub search can return.',
|
||||
{ value0: String(target + 1) }
|
||||
),
|
||||
{ id: 'work-items-page-unreachable' }
|
||||
)
|
||||
setProvenPageLimit((previous) => applyWindowPageLimit(previous, target))
|
||||
} else if (reason === 'load-failed') {
|
||||
toast.error(
|
||||
translate(
|
||||
'auto.components.TaskPage.loadPageFailed',
|
||||
'Page {{value0}} could not be loaded from GitHub.',
|
||||
{ value0: String(target + 1) }
|
||||
),
|
||||
{ id: 'work-items-page-load-failed' }
|
||||
)
|
||||
} else {
|
||||
// Why: with a real count the clamp is refused, so without feedback
|
||||
// the click would look dead — the count over-advertised; nothing
|
||||
// failed, so the copy stays neutral. The ref carries the committed
|
||||
// count, immune to the click-time closure race.
|
||||
const committedCount = countedTotalPagesRef.current
|
||||
if (committedCount !== null && committedCount > 0) {
|
||||
toast(
|
||||
translate(
|
||||
'auto.components.TaskPage.loadPageNoMoreResults',
|
||||
'No more results on page {{value0}}.',
|
||||
{ value0: String(target + 1) }
|
||||
),
|
||||
{ id: 'work-items-page-no-more-results' }
|
||||
)
|
||||
}
|
||||
const next = applyEmptyPageClamp(committedCount, { target, failedCount, errorTypes })
|
||||
countedTotalPagesRef.current = next
|
||||
setCountedTotalPages(next)
|
||||
}
|
||||
return
|
||||
}
|
||||
setPages((previous) => {
|
||||
|
|
@ -6253,6 +6332,8 @@ export default function TaskPage(): React.JSX.Element {
|
|||
setPages([page0])
|
||||
setCurrentPage(0)
|
||||
setCountedTotalPages(null)
|
||||
countedTotalPagesRef.current = null
|
||||
setProvenPageLimit(null)
|
||||
setTasksError(null)
|
||||
setFailedCount(0) // reset so a prior failure banner doesn't linger
|
||||
setGithubUnavailable(false)
|
||||
|
|
@ -6357,6 +6438,10 @@ export default function TaskPage(): React.JSX.Element {
|
|||
githubPerRepoPageLimit
|
||||
).then(({ totalPages: countedPages }) => {
|
||||
if (!cancelled) {
|
||||
// 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.
|
||||
countedTotalPagesRef.current = countedPages
|
||||
setCountedTotalPages(countedPages)
|
||||
}
|
||||
})
|
||||
|
|
@ -6365,9 +6450,13 @@ export default function TaskPage(): React.JSX.Element {
|
|||
cancelled = true
|
||||
}
|
||||
// Why: store selectors are stable (omit from deps); workItemsInvalidationNonce included so a preference flip re-dispatches.
|
||||
// selectedReposKey stands in for selectedRepos — the array gets a fresh
|
||||
// identity on every repos:changed event, and re-running this effect then
|
||||
// resets pagination mid-click (#11485). The key covers every repo field the
|
||||
// requests read.
|
||||
// eslint-disable-next-line react-hooks/exhaustive-deps
|
||||
}, [
|
||||
selectedRepos,
|
||||
selectedReposKey,
|
||||
appliedTaskSearch,
|
||||
taskRefreshNonce,
|
||||
taskSource,
|
||||
|
|
|
|||
|
|
@ -2,7 +2,12 @@ import { describe, expect, it, vi } from 'vitest'
|
|||
import type { GitHubWorkItem } from '../../../shared/types'
|
||||
import {
|
||||
accumulateWorkItemPages,
|
||||
applyEmptyPageClamp,
|
||||
applyWindowPageLimit,
|
||||
buildSelectedReposKey,
|
||||
deriveAdvertisedTotalPages,
|
||||
getTaskPagePerRepoLimit,
|
||||
resolveEmptyPageOutcome,
|
||||
taskPageToGitHubApiPage,
|
||||
workItemIdentity
|
||||
} from './task-page-work-item-pagination'
|
||||
|
|
@ -59,6 +64,194 @@ describe('numbered GitHub pagination', () => {
|
|||
})
|
||||
})
|
||||
|
||||
describe('resolveEmptyPageOutcome', () => {
|
||||
const base = { failedCount: 0, errorTypes: [] as const, countedTotalPages: null }
|
||||
|
||||
it('clamps and reports unreachable when the search window 422 is present', () => {
|
||||
expect(
|
||||
resolveEmptyPageOutcome({ ...base, target: 33, errorTypes: ['validation_error'] })
|
||||
).toEqual({ reason: 'window-unreachable', clampTotalPagesTo: 33 })
|
||||
// A real count is still clamped — the count over-advertising is the bug.
|
||||
expect(
|
||||
resolveEmptyPageOutcome({
|
||||
...base,
|
||||
target: 33,
|
||||
errorTypes: ['validation_error'],
|
||||
countedTotalPages: 39
|
||||
})
|
||||
).toEqual({ reason: 'window-unreachable', clampTotalPagesTo: 33 })
|
||||
})
|
||||
|
||||
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,
|
||||
target: 5,
|
||||
failedCount: 1,
|
||||
errorTypes: ['validation_error']
|
||||
})
|
||||
).toEqual({ reason: 'load-failed', clampTotalPagesTo: null })
|
||||
expect(
|
||||
resolveEmptyPageOutcome({
|
||||
...base,
|
||||
target: 5,
|
||||
errorTypes: ['validation_error', 'permission_denied']
|
||||
})
|
||||
).toEqual({ reason: 'load-failed', clampTotalPagesTo: null })
|
||||
})
|
||||
|
||||
it('reports a failure without clamping for transient or unclassified errors', () => {
|
||||
expect(resolveEmptyPageOutcome({ ...base, target: 5, failedCount: 2 })).toEqual({
|
||||
reason: 'load-failed',
|
||||
clampTotalPagesTo: null
|
||||
})
|
||||
for (const type of ['permission_denied', 'not_found', 'rate_limited', 'unknown'] as const) {
|
||||
expect(resolveEmptyPageOutcome({ ...base, target: 5, errorTypes: [type] })).toEqual({
|
||||
reason: 'load-failed',
|
||||
clampTotalPagesTo: null
|
||||
})
|
||||
}
|
||||
})
|
||||
|
||||
it('withdraws the speculative page on clean empty only while the count is unknown', () => {
|
||||
expect(resolveEmptyPageOutcome({ ...base, target: 2 })).toEqual({
|
||||
reason: 'end-of-data',
|
||||
clampTotalPagesTo: 2
|
||||
})
|
||||
// 0 means the count itself failed — treat like unknown.
|
||||
expect(resolveEmptyPageOutcome({ ...base, target: 2, countedTotalPages: 0 })).toEqual({
|
||||
reason: 'end-of-data',
|
||||
clampTotalPagesTo: 2
|
||||
})
|
||||
// A real count must not shrink: the PR list path swallows its own failures
|
||||
// into clean-empty results, and clamping would hide healthy pages silently.
|
||||
expect(resolveEmptyPageOutcome({ ...base, target: 2, countedTotalPages: 34 })).toEqual({
|
||||
reason: 'end-of-data',
|
||||
clampTotalPagesTo: null
|
||||
})
|
||||
// Never clamps below one advertised page.
|
||||
expect(resolveEmptyPageOutcome({ ...base, target: 0 })).toEqual({
|
||||
reason: 'end-of-data',
|
||||
clampTotalPagesTo: 1
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
describe('applyEmptyPageClamp', () => {
|
||||
const clean = { failedCount: 0, errorTypes: [] 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('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 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, errorTypes: ['validation_error'] })
|
||||
).toBe(39)
|
||||
expect(applyEmptyPageClamp(33, { target: 5, failedCount: 2, errorTypes: [] })).toBe(33)
|
||||
expect(applyEmptyPageClamp(null, { target: 5, failedCount: 2, errorTypes: [] })).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('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)
|
||||
})
|
||||
})
|
||||
|
||||
describe('buildSelectedReposKey', () => {
|
||||
const contextFor = (r: { id: string }) => ({ provider: 'github', repoId: r.id })
|
||||
|
||||
it('is stable across fresh arrays with identical contents', () => {
|
||||
const a = [{ id: 'r1', path: '/a', connectionId: null, executionHostId: null }]
|
||||
const b = [{ ...a[0] }]
|
||||
expect(buildSelectedReposKey(a, contextFor)).toBe(buildSelectedReposKey(b, contextFor))
|
||||
})
|
||||
|
||||
it('changes when any request-relevant field changes', () => {
|
||||
const repo = { id: 'r1', path: '/a', connectionId: null, executionHostId: null }
|
||||
const key = buildSelectedReposKey([repo], contextFor)
|
||||
expect(buildSelectedReposKey([{ ...repo, executionHostId: 'ssh:host' }], contextFor)).not.toBe(
|
||||
key
|
||||
)
|
||||
expect(buildSelectedReposKey([{ ...repo, path: '/b' }], contextFor)).not.toBe(key)
|
||||
expect(
|
||||
buildSelectedReposKey([repo], (r) => ({ provider: 'github', repoId: r.id, owner: 'new' }))
|
||||
).not.toBe(key)
|
||||
})
|
||||
})
|
||||
|
||||
describe('accumulateWorkItemPages', () => {
|
||||
it('drops the re-fetched boundary row that shares the previous page cursor', async () => {
|
||||
const boundary = item('r', 'issue:2', '2026-07-02')
|
||||
|
|
|
|||
|
|
@ -1,4 +1,4 @@
|
|||
import type { GitHubWorkItem } from '../../../shared/types'
|
||||
import type { ClassifiedError, GitHubWorkItem } from '../../../shared/types'
|
||||
|
||||
/**
|
||||
* Cross-repo Tasks pagination is cursor-based on `updatedAt`: each page's oldest
|
||||
|
|
@ -35,6 +35,125 @@ export function taskPageToGitHubApiPage(taskPage: number): number {
|
|||
return Math.max(0, Math.floor(taskPage)) + 1
|
||||
}
|
||||
|
||||
export type EmptyPageOutcome = {
|
||||
reason: 'window-unreachable' | 'load-failed' | 'end-of-data'
|
||||
/** New advertised page count, or null to leave the current count alone.
|
||||
* Window clamps are applied via applyWindowPageLimit — this field carries
|
||||
* the same value there for symmetry, but the count slot must not use it. */
|
||||
clampTotalPagesTo: number | null
|
||||
}
|
||||
|
||||
/**
|
||||
* 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. When a sibling
|
||||
* repo's fetch also threw, the transient failure wins (load-failed, no
|
||||
* clamp) so the toast and the clamp never disagree. The window signal is
|
||||
* issue-side only: PR-side errors arrive demoted (never validation_error).
|
||||
* - any other per-repo error (thrown, or on either envelope channel) may be
|
||||
* transient (rate limit, permissions), so surface it but keep the count;
|
||||
* - no error at all is end-of-data. That only warrants a clamp while the count
|
||||
* is unknown (null) or failed (0), to withdraw the speculative page
|
||||
* `fallbackTotalPages` advertises — a real count must not shrink.
|
||||
*/
|
||||
export function resolveEmptyPageOutcome(args: {
|
||||
target: number
|
||||
failedCount: number
|
||||
errorTypes: readonly ClassifiedError['type'][]
|
||||
countedTotalPages: number | null
|
||||
}): EmptyPageOutcome {
|
||||
const clamp = Math.max(1, Math.floor(args.target))
|
||||
// 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.errorTypes.length > 0 && args.errorTypes.every((type) => type === 'validation_error')
|
||||
if (onlyWindowErrors && args.failedCount === 0) {
|
||||
return { reason: 'window-unreachable', clampTotalPagesTo: clamp }
|
||||
}
|
||||
if (args.failedCount > 0 || args.errorTypes.length > 0) {
|
||||
return { reason: 'load-failed', clampTotalPagesTo: null }
|
||||
}
|
||||
const countUnknown = args.countedTotalPages === null || args.countedTotalPages === 0
|
||||
return { reason: 'end-of-data', clampTotalPagesTo: countUnknown ? clamp : null }
|
||||
}
|
||||
|
||||
/**
|
||||
* 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,
|
||||
args: {
|
||||
target: number
|
||||
failedCount: number
|
||||
errorTypes: readonly ClassifiedError['type'][]
|
||||
}
|
||||
): number | null {
|
||||
const outcome = resolveEmptyPageOutcome({ ...args, countedTotalPages: previous })
|
||||
if (outcome.reason !== 'end-of-data' || outcome.clampTotalPagesTo === null) {
|
||||
return previous
|
||||
}
|
||||
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)
|
||||
}
|
||||
|
||||
/**
|
||||
* 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
|
||||
* with the selection unchanged — resetting pagination mid-click (#11485). The
|
||||
* key must cover every repo field the work-item requests read; the caller
|
||||
* supplies the resolved source context (which must stay free of timestamps).
|
||||
*/
|
||||
export function buildSelectedReposKey<
|
||||
T extends {
|
||||
id: string
|
||||
path: string
|
||||
connectionId?: string | null
|
||||
executionHostId?: string | null
|
||||
}
|
||||
>(repos: readonly T[], sourceContextFor: (repo: T) => unknown): string {
|
||||
return repos
|
||||
.map(
|
||||
(r) =>
|
||||
`${r.id}|${r.path}|${r.connectionId ?? ''}|${r.executionHostId ?? ''}|${JSON.stringify(
|
||||
sourceContextFor(r)
|
||||
)}`
|
||||
)
|
||||
.join(',')
|
||||
}
|
||||
|
||||
// Why: provider pages cannot spill truncated rows into the next page. Divide
|
||||
// the display budget up front so every fetched row remains reachable.
|
||||
export function getTaskPagePerRepoLimit(
|
||||
|
|
|
|||
|
|
@ -1897,7 +1897,10 @@
|
|||
"75a38d7df8": "GitHub data is temporarily unavailable. Its API may be down, rate-limited, or unreachable. Please try again shortly.",
|
||||
"noGithubSourceDetected": "No GitHub source detected for",
|
||||
"noGithubSourceDetectedHint": "it may have no GitHub remote, or the source could not be resolved.",
|
||||
"jiraLinkSourceUnavailable": "Couldn’t link this Jira issue. Reconnect Jira or pick the matching site, then try again."
|
||||
"jiraLinkSourceUnavailable": "Couldn’t link this Jira issue. Reconnect Jira or pick the matching site, then try again.",
|
||||
"loadPageUnreachable": "Page {{value0}} is beyond what GitHub search can return.",
|
||||
"loadPageFailed": "Page {{value0}} could not be loaded from GitHub.",
|
||||
"loadPageNoMoreResults": "No more results on page {{value0}}."
|
||||
},
|
||||
"Terminal": {
|
||||
"73768427cf": "Close",
|
||||
|
|
|
|||
|
|
@ -1856,7 +1856,10 @@
|
|||
"75a38d7df8": "GitHub data is temporarily unavailable. Its API may be down, rate-limited, or unreachable. Please try again shortly.",
|
||||
"noGithubSourceDetected": "No GitHub source detected for",
|
||||
"noGithubSourceDetectedHint": "it may have no GitHub remote, or the source could not be resolved.",
|
||||
"jiraLinkSourceUnavailable": "Couldn’t link this Jira issue. Reconnect Jira or pick the matching site, then try again."
|
||||
"jiraLinkSourceUnavailable": "Couldn’t link this Jira issue. Reconnect Jira or pick the matching site, then try again.",
|
||||
"loadPageUnreachable": "La página {{value0}} está más allá de lo que la búsqueda de GitHub puede devolver.",
|
||||
"loadPageFailed": "No se pudo cargar la página {{value0}} desde GitHub.",
|
||||
"loadPageNoMoreResults": "No hay más resultados en la página {{value0}}."
|
||||
},
|
||||
"Terminal": {
|
||||
"73768427cf": "Cerrar",
|
||||
|
|
|
|||
|
|
@ -1856,7 +1856,10 @@
|
|||
"75a38d7df8": "GitHub data is temporarily unavailable. Its API may be down, rate-limited, or unreachable. Please try again shortly.",
|
||||
"noGithubSourceDetected": "No GitHub source detected for",
|
||||
"noGithubSourceDetectedHint": "it may have no GitHub remote, or the source could not be resolved.",
|
||||
"jiraLinkSourceUnavailable": "Couldn’t link this Jira issue. Reconnect Jira or pick the matching site, then try again."
|
||||
"jiraLinkSourceUnavailable": "Couldn’t link this Jira issue. Reconnect Jira or pick the matching site, then try again.",
|
||||
"loadPageUnreachable": "ページ {{value0}} は GitHub 検索が返せる範囲を超えています。",
|
||||
"loadPageFailed": "GitHub からページ {{value0}} を読み込めませんでした。",
|
||||
"loadPageNoMoreResults": "ページ {{value0}} にはこれ以上結果がありません。"
|
||||
},
|
||||
"Terminal": {
|
||||
"73768427cf": "閉じる",
|
||||
|
|
|
|||
|
|
@ -1856,7 +1856,10 @@
|
|||
"75a38d7df8": "GitHub data is temporarily unavailable. Its API may be down, rate-limited, or unreachable. Please try again shortly.",
|
||||
"noGithubSourceDetected": "No GitHub source detected for",
|
||||
"noGithubSourceDetectedHint": "it may have no GitHub remote, or the source could not be resolved.",
|
||||
"jiraLinkSourceUnavailable": "Couldn’t link this Jira issue. Reconnect Jira or pick the matching site, then try again."
|
||||
"jiraLinkSourceUnavailable": "Couldn’t link this Jira issue. Reconnect Jira or pick the matching site, then try again.",
|
||||
"loadPageUnreachable": "{{value0}} 페이지는 GitHub 검색이 반환할 수 있는 범위를 벗어났습니다.",
|
||||
"loadPageFailed": "GitHub에서 {{value0}} 페이지를 불러오지 못했습니다.",
|
||||
"loadPageNoMoreResults": "{{value0}} 페이지에 더 이상 결과가 없습니다."
|
||||
},
|
||||
"Terminal": {
|
||||
"73768427cf": "닫기",
|
||||
|
|
|
|||
|
|
@ -1856,7 +1856,10 @@
|
|||
"75a38d7df8": "GitHub data is temporarily unavailable. Its API may be down, rate-limited, or unreachable. Please try again shortly.",
|
||||
"noGithubSourceDetected": "No GitHub source detected for",
|
||||
"noGithubSourceDetectedHint": "it may have no GitHub remote, or the source could not be resolved.",
|
||||
"jiraLinkSourceUnavailable": "Couldn’t link this Jira issue. Reconnect Jira or pick the matching site, then try again."
|
||||
"jiraLinkSourceUnavailable": "Couldn’t link this Jira issue. Reconnect Jira or pick the matching site, then try again.",
|
||||
"loadPageUnreachable": "第 {{value0}} 页超出了 GitHub 搜索可返回的范围。",
|
||||
"loadPageFailed": "无法从 GitHub 加载第 {{value0}} 页。",
|
||||
"loadPageNoMoreResults": "第 {{value0}} 页没有更多结果。"
|
||||
},
|
||||
"Terminal": {
|
||||
"73768427cf": "关闭",
|
||||
|
|
|
|||
|
|
@ -1,4 +1,5 @@
|
|||
export {
|
||||
GITHUB_SEARCH_RESULT_WINDOW_ERROR_PATTERN,
|
||||
GITHUB_WORK_ITEMS_QUERY_MAX_BYTES,
|
||||
isGitHubWorkItemsQueryTooLarge
|
||||
} from '../../../../shared/github-work-items-query-bounds'
|
||||
|
|
|
|||
|
|
@ -6656,10 +6656,108 @@ describe('createGitHubSlice.fetchWorkItems source/error envelope', () => {
|
|||
})
|
||||
expect(result).toEqual({
|
||||
items: [{ ...item, repoId: 'caller-repo-id' }],
|
||||
failedCount: 0
|
||||
failedCount: 0,
|
||||
errorTypes: []
|
||||
})
|
||||
})
|
||||
|
||||
it('surfaces issue-side envelope errors as errorTypes on next-page fetches', async () => {
|
||||
runtimeEnvironmentCall.mockResolvedValueOnce({
|
||||
id: 'rpc-work-items-page-422',
|
||||
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: 'only the first 1000 search results' }
|
||||
}
|
||||
},
|
||||
_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, '', 34)
|
||||
|
||||
// The window 422 travels on the envelope error channel, not failedCount —
|
||||
// resolveEmptyPageOutcome keys on this exact string (#11485).
|
||||
expect(result).toEqual({ items: [], failedCount: 0, errorTypes: ['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, errorTypes: ['unknown'] })
|
||||
})
|
||||
|
||||
it('surfaces PR-side envelope errors demoted so they read as failures, never window 422s', async () => {
|
||||
runtimeEnvironmentCall.mockResolvedValueOnce({
|
||||
id: 'rpc-work-items-page-prs-error',
|
||||
ok: true,
|
||||
result: {
|
||||
items: [],
|
||||
sources: {
|
||||
issues: null,
|
||||
prs: { owner: 'up', repo: 'r' },
|
||||
originCandidate: { owner: 'up', repo: 'r' },
|
||||
upstreamCandidate: null
|
||||
},
|
||||
errors: {
|
||||
prs: { type: 'validation_error', message: 'Failed to load pull requests: bad flag' }
|
||||
}
|
||||
},
|
||||
_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)
|
||||
|
||||
// A swallowed PR-side failure must not read as end-of-data (#11485), and a
|
||||
// PR-side validation error must never join the issue-only window signal.
|
||||
expect(result).toEqual({ items: [], failedCount: 0, errorTypes: ['unknown'] })
|
||||
})
|
||||
|
||||
it('routes work-item counts through the active runtime environment', async () => {
|
||||
runtimeEnvironmentCall.mockResolvedValueOnce({
|
||||
id: 'rpc-work-items-count',
|
||||
|
|
@ -6727,6 +6825,43 @@ describe('createGitHubSlice.fetchWorkItems source/error envelope', () => {
|
|||
expect(result).toEqual({ totalCount: 101, totalPages: 3 })
|
||||
})
|
||||
|
||||
it('caps advertised pages at the GitHub search result window', async () => {
|
||||
const store = createTestStore()
|
||||
mockApi.gh.countWorkItems.mockResolvedValueOnce(1170).mockResolvedValueOnce(1)
|
||||
|
||||
const result = await store.getState().countWorkItemsAcrossRepos(
|
||||
[
|
||||
{ repoId: 'large-repo', path: '/local/large' },
|
||||
{ repoId: 'small-repo', path: '/local/small' }
|
||||
],
|
||||
'is:issue',
|
||||
30
|
||||
)
|
||||
|
||||
// 1170 results → 39 naive pages, but the Search API 422s once a page
|
||||
// starts past its 1000-result window; ceil(1000 / 30) = 34 stay reachable.
|
||||
expect(result).toEqual({ totalCount: 1171, totalPages: 34 })
|
||||
})
|
||||
|
||||
it('pins the search-window cap at dividing and non-dividing per-repo limits', async () => {
|
||||
const store = createTestStore()
|
||||
// 36 is the shipped single-repo limit: page 28 starts at result 973 and is
|
||||
// served; page 29 starts past 1000 and 422s.
|
||||
mockApi.gh.countWorkItems.mockResolvedValueOnce(2000)
|
||||
await expect(
|
||||
store
|
||||
.getState()
|
||||
.countWorkItemsAcrossRepos([{ repoId: 'repo-id', path: '/local/repo' }], 'is:issue', 36)
|
||||
).resolves.toEqual({ totalCount: 2000, totalPages: 28 })
|
||||
// 25 divides 1000 evenly: the full window stays reachable (40 pages).
|
||||
mockApi.gh.countWorkItems.mockResolvedValueOnce(2000)
|
||||
await expect(
|
||||
store
|
||||
.getState()
|
||||
.countWorkItemsAcrossRepos([{ repoId: 'repo-id', path: '/local/repo' }], 'is:issue', 25)
|
||||
).resolves.toEqual({ totalCount: 2000, totalPages: 40 })
|
||||
})
|
||||
|
||||
it('rejects oversized work-item queries before cache keys or provider calls', async () => {
|
||||
const store = createTestStore()
|
||||
const secret = 'github-work-items-secret'
|
||||
|
|
@ -6755,7 +6890,7 @@ describe('createGitHubSlice.fetchWorkItems source/error envelope', () => {
|
|||
oversizedQuery,
|
||||
1
|
||||
)
|
||||
).resolves.toEqual({ items: [], failedCount: 0 })
|
||||
).resolves.toEqual({ items: [], failedCount: 0, errorTypes: [] })
|
||||
await expect(
|
||||
store
|
||||
.getState()
|
||||
|
|
|
|||
|
|
@ -53,7 +53,10 @@ import { rightSidebarShowsPullRequestData } from '@/lib/right-sidebar-visibility
|
|||
import { hostedReviewInfoFromGitHubPRInfo } from '../../../../shared/hosted-review-github'
|
||||
import { getHostedReviewCacheKey, linkedReviewHintKey } from './hosted-review-cache-identity'
|
||||
import { getGitHubPRCacheKey, getGitHubRepoCacheKey } from './github-cache-key'
|
||||
import { isGitHubWorkItemsQueryTooLarge } from './github-work-items-query-bounds'
|
||||
import {
|
||||
GITHUB_SEARCH_RESULT_WINDOW_ERROR_PATTERN,
|
||||
isGitHubWorkItemsQueryTooLarge
|
||||
} from './github-work-items-query-bounds'
|
||||
import { classifyGitHubUnavailable } from '../../../../shared/github-api-availability'
|
||||
import { isMacAppDataPath } from '@/lib/passive-macos-app-data-access'
|
||||
import { translate } from '@/i18n/i18n'
|
||||
|
|
@ -655,6 +658,9 @@ const CHECKS_CACHE_TTL = 60_000 // 1 minute — checks change more frequently
|
|||
const EMPTY_CHECKS_CACHE_TTL = 10_000
|
||||
// Why: the work-item list is a browse surface, not a source of truth, so 60s staleness is fine (SWR keeps it current).
|
||||
const WORK_ITEMS_CACHE_TTL = 60_000
|
||||
// GitHub's Search API serves the page that starts within its first 1000 results;
|
||||
// the next page 422s even when the final reachable page crosses the boundary.
|
||||
const GITHUB_SEARCH_RESULT_WINDOW = 1000
|
||||
// Why: long-lived (matches repos.ts) so the user has time to read + act on persist failures before the toast vanishes.
|
||||
const ERROR_TOAST_DURATION = 60_000
|
||||
|
||||
|
|
@ -1964,7 +1970,11 @@ export type GitHubSlice = {
|
|||
displayLimit: number,
|
||||
query: string,
|
||||
page: number
|
||||
) => Promise<{ items: GitHubWorkItem[]; failedCount: number }>
|
||||
) => Promise<{
|
||||
items: GitHubWorkItem[]
|
||||
failedCount: number
|
||||
errorTypes: ClassifiedError['type'][]
|
||||
}>
|
||||
/** Count items and derive pages from the largest per-repo result set. */
|
||||
countWorkItemsAcrossRepos: (
|
||||
repos: {
|
||||
|
|
@ -2784,9 +2794,10 @@ export const createGitHubSlice: StateCreator<AppState, [], [], GitHubSlice> = (s
|
|||
|
||||
fetchWorkItemsNextPage: async (repos, perRepoLimit, displayLimit, query, page) => {
|
||||
if (isGitHubWorkItemsQueryTooLarge(query)) {
|
||||
return { items: [], failedCount: 0 }
|
||||
return { items: [], failedCount: 0, errorTypes: [] }
|
||||
}
|
||||
let failedCount = 0
|
||||
const errorTypes: ClassifiedError['type'][] = []
|
||||
const perProjectResults = await Promise.all(
|
||||
repos.map(async (r) => {
|
||||
const requestState = get()
|
||||
|
|
@ -2812,11 +2823,30 @@ 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) {
|
||||
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.
|
||||
errorTypes.push(
|
||||
type === 'validation_error' &&
|
||||
!GITHUB_SEARCH_RESULT_WINDOW_ERROR_PATTERN.test(message)
|
||||
? 'unknown'
|
||||
: type
|
||||
)
|
||||
console.warn(
|
||||
`[workItems] next page ${r.repoId} issues-side partial failure:`,
|
||||
envelope.errors.issues
|
||||
)
|
||||
}
|
||||
if (envelope.errors?.prs) {
|
||||
// Why: the window 422 is issue-side only — a PR-side validation
|
||||
// error must never join the unreachable signal.
|
||||
const { type } = envelope.errors.prs
|
||||
errorTypes.push(type === 'validation_error' ? 'unknown' : type)
|
||||
console.warn(
|
||||
`[workItems] next page ${r.repoId} prs-side partial failure:`,
|
||||
envelope.errors.prs
|
||||
)
|
||||
}
|
||||
return envelope.items.map((item): GitHubWorkItem => ({ ...item, repoId: r.repoId }))
|
||||
} catch (err) {
|
||||
if (isGitHubWorkItemsSshRemoteRequiredError(err)) {
|
||||
|
|
@ -2831,7 +2861,7 @@ export const createGitHubSlice: StateCreator<AppState, [], [], GitHubSlice> = (s
|
|||
})
|
||||
)
|
||||
const merged = sortWorkItemsByNumber(perProjectResults.flat()).slice(0, displayLimit)
|
||||
return { items: merged, failedCount }
|
||||
return { items: merged, failedCount, errorTypes }
|
||||
},
|
||||
|
||||
countWorkItemsAcrossRepos: async (repos, query, perRepoLimit) => {
|
||||
|
|
@ -2839,6 +2869,8 @@ export const createGitHubSlice: StateCreator<AppState, [], [], GitHubSlice> = (s
|
|||
return { totalCount: 0, totalPages: 0 }
|
||||
}
|
||||
const normalizedLimit = Math.max(1, Math.floor(perRepoLimit))
|
||||
// Why: GitHub 422s pages that start past its 1000-result search window.
|
||||
const maxReachablePages = Math.max(1, Math.ceil(GITHUB_SEARCH_RESULT_WINDOW / normalizedLimit))
|
||||
const counts = await Promise.all(
|
||||
repos.map(async (r) => {
|
||||
// Why: same stampede cap as item-fetch — without a slot a 90-repo selection fires 90 concurrent count IPCs before the main-side rate-limit guard sees the first 403.
|
||||
|
|
@ -2870,7 +2902,8 @@ export const createGitHubSlice: StateCreator<AppState, [], [], GitHubSlice> = (s
|
|||
totalCount: counts.reduce((sum, count) => sum + count, 0),
|
||||
// Why: repos advance independently by page, so take the max across repos — a sum/page-width undercounts when one repo owns most results.
|
||||
totalPages: counts.reduce(
|
||||
(maxPages, count) => Math.max(maxPages, Math.ceil(count / normalizedLimit)),
|
||||
(maxPages, count) =>
|
||||
Math.max(maxPages, Math.min(Math.ceil(count / normalizedLimit), maxReachablePages)),
|
||||
0
|
||||
)
|
||||
}
|
||||
|
|
|
|||
|
|
@ -2,6 +2,17 @@ import { isClipboardTextByteLengthOverLimit } from './clipboard-text'
|
|||
|
||||
export const GITHUB_WORK_ITEMS_QUERY_MAX_BYTES = 8 * 1024
|
||||
|
||||
/**
|
||||
* GitHub's Search API only pages through the first 1000 matches; beyond that it
|
||||
* 422s with "Only the first 1000 search results are available". Matching that
|
||||
* free-text wording is the ONLY signal separating a permanently unreachable page
|
||||
* from a transient failure (#11485), so the phrase is pinned here rather than
|
||||
* inlined: if GitHub rewords it, window 422s silently demote to generic
|
||||
* failures and the advertised page count stops being capped.
|
||||
* `gh-utils.test.ts` asserts the classified message still carries this phrase.
|
||||
*/
|
||||
export const GITHUB_SEARCH_RESULT_WINDOW_ERROR_PATTERN = /first 1000 search results/i
|
||||
|
||||
export function isGitHubWorkItemsQueryTooLarge(
|
||||
query: string,
|
||||
maxBytes = GITHUB_WORK_ITEMS_QUERY_MAX_BYTES
|
||||
|
|
|
|||
|
|
@ -2065,6 +2065,7 @@ export type ListWorkItemsResult<T> = {
|
|||
}
|
||||
errors?: {
|
||||
issues?: ClassifiedError
|
||||
prs?: ClassifiedError
|
||||
}
|
||||
/** True when the user's per-repo preference was `'upstream'` but no upstream
|
||||
* remote is configured, so the resolver fell back to origin. Renderer uses
|
||||
|
|
|
|||
Loading…
Reference in New Issue