From 56ab5fd1dc6f94aad8ccaaa4abe4f716c9a7e9cc Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Sun, 2 Aug 2026 12:05:50 -0700 Subject: [PATCH] =?UTF-8?q?fix(tasks):=20make=20GitHub=20pagination=20hone?= =?UTF-8?q?st=20=E2=80=94=20cap=20unreachable=20pages,=20survive=20backgro?= =?UTF-8?q?und=20refreshes,=20explain=20empty=20pages=20(#11584)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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> --- .../client-work-items-query-paging.test.ts | 19 +- src/main/github/client-work-items.test.ts | 3 +- src/main/github/client.ts | 17 +- src/main/github/gh-error-classification.ts | 8 + src/main/github/gh-utils.test.ts | 14 ++ src/main/github/gh-utils.ts | 6 +- src/renderer/src/components/TaskPage.tsx | 123 +++++++++-- .../task-page-work-item-pagination.test.ts | 193 ++++++++++++++++++ .../task-page-work-item-pagination.ts | 121 ++++++++++- src/renderer/src/i18n/locales/en.json | 5 +- src/renderer/src/i18n/locales/es.json | 5 +- src/renderer/src/i18n/locales/ja.json | 5 +- src/renderer/src/i18n/locales/ko.json | 5 +- src/renderer/src/i18n/locales/zh.json | 5 +- .../slices/github-work-items-query-bounds.ts | 1 + src/renderer/src/store/slices/github.test.ts | 139 ++++++++++++- src/renderer/src/store/slices/github.ts | 43 +++- src/shared/github-work-items-query-bounds.ts | 11 + src/shared/types.ts | 1 + 19 files changed, 688 insertions(+), 36 deletions(-) diff --git a/src/main/github/client-work-items-query-paging.test.ts b/src/main/github/client-work-items-query-paging.test.ts index f9cf934e2..7af6b124c 100644 --- a/src/main/github/client-work-items-query-paging.test.ts +++ b/src/main/github/client-work-items-query-paging.test.ts @@ -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' }) diff --git a/src/main/github/client-work-items.test.ts b/src/main/github/client-work-items.test.ts index 0226b2af4..84119b3b2 100644 --- a/src/main/github/client-work-items.test.ts +++ b/src/main/github/client-work-items.test.ts @@ -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', () => ({ diff --git a/src/main/github/client.ts b/src/main/github/client.ts index 3684d7729..24772ebee 100644 --- a/src/main/github/client.ts +++ b/src/main/github/client.ts @@ -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 => { @@ -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: { diff --git a/src/main/github/gh-error-classification.ts b/src/main/github/gh-error-classification.ts index 2de37919d..4d2453899 100644 --- a/src/main/github/gh-error-classification.ts +++ b/src/main/github/gh-error-classification.ts @@ -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()}` } +} diff --git a/src/main/github/gh-utils.test.ts b/src/main/github/gh-utils.test.ts index 0d7329e21..ca9098e53 100644 --- a/src/main/github/gh-utils.test.ts +++ b/src/main/github/gh-utils.test.ts @@ -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) + }) }) diff --git a/src/main/github/gh-utils.ts b/src/main/github/gh-utils.ts index d2148edc3..d67701899 100644 --- a/src/main/github/gh-utils.ts +++ b/src/main/github/gh-utils.ts @@ -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, diff --git a/src/renderer/src/components/TaskPage.tsx b/src/renderer/src/components/TaskPage.tsx index 3ff8a909b..99d0785c7 100644 --- a/src/renderer/src/components/TaskPage.tsx +++ b/src/renderer/src/components/TaskPage.tsx @@ -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(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) + // 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(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, 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 22ad6bf6c..670c862be 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 @@ -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') 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 e8f0a8749..8209b61bd 100644 --- a/src/renderer/src/components/task-page-work-item-pagination.ts +++ b/src/renderer/src/components/task-page-work-item-pagination.ts @@ -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( diff --git a/src/renderer/src/i18n/locales/en.json b/src/renderer/src/i18n/locales/en.json index 641b9b018..7e276012c 100644 --- a/src/renderer/src/i18n/locales/en.json +++ b/src/renderer/src/i18n/locales/en.json @@ -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", diff --git a/src/renderer/src/i18n/locales/es.json b/src/renderer/src/i18n/locales/es.json index f251c3e0f..02bc8bf3b 100644 --- a/src/renderer/src/i18n/locales/es.json +++ b/src/renderer/src/i18n/locales/es.json @@ -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", diff --git a/src/renderer/src/i18n/locales/ja.json b/src/renderer/src/i18n/locales/ja.json index 0e53b3b1e..b7ce3fcef 100644 --- a/src/renderer/src/i18n/locales/ja.json +++ b/src/renderer/src/i18n/locales/ja.json @@ -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": "閉じる", diff --git a/src/renderer/src/i18n/locales/ko.json b/src/renderer/src/i18n/locales/ko.json index d44e2d750..aa9b6bf60 100644 --- a/src/renderer/src/i18n/locales/ko.json +++ b/src/renderer/src/i18n/locales/ko.json @@ -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": "닫기", diff --git a/src/renderer/src/i18n/locales/zh.json b/src/renderer/src/i18n/locales/zh.json index 626cbcc86..cfc09f5ae 100644 --- a/src/renderer/src/i18n/locales/zh.json +++ b/src/renderer/src/i18n/locales/zh.json @@ -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": "关闭", diff --git a/src/renderer/src/store/slices/github-work-items-query-bounds.ts b/src/renderer/src/store/slices/github-work-items-query-bounds.ts index c60529c3e..0ac2107a8 100644 --- a/src/renderer/src/store/slices/github-work-items-query-bounds.ts +++ b/src/renderer/src/store/slices/github-work-items-query-bounds.ts @@ -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' diff --git a/src/renderer/src/store/slices/github.test.ts b/src/renderer/src/store/slices/github.test.ts index 91269b0b3..13f720094 100644 --- a/src/renderer/src/store/slices/github.test.ts +++ b/src/renderer/src/store/slices/github.test.ts @@ -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) + + 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) + + 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) + + 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() diff --git a/src/renderer/src/store/slices/github.ts b/src/renderer/src/store/slices/github.ts index 4f1975658..5ebe76f2c 100644 --- a/src/renderer/src/store/slices/github.ts +++ b/src/renderer/src/store/slices/github.ts @@ -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 = (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 = (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 = (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 = (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 = (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 ) } diff --git a/src/shared/github-work-items-query-bounds.ts b/src/shared/github-work-items-query-bounds.ts index 4d3ae6e6d..48f90bceb 100644 --- a/src/shared/github-work-items-query-bounds.ts +++ b/src/shared/github-work-items-query-bounds.ts @@ -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 diff --git a/src/shared/types.ts b/src/shared/types.ts index faf50e6eb..b52aa1365 100644 --- a/src/shared/types.ts +++ b/src/shared/types.ts @@ -2065,6 +2065,7 @@ export type ListWorkItemsResult = { } 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