diff --git a/src/main/github/client-work-items.test.ts b/src/main/github/client-work-items.test.ts index 66e728908..d0e93eb30 100644 --- a/src/main/github/client-work-items.test.ts +++ b/src/main/github/client-work-items.test.ts @@ -189,7 +189,9 @@ describe('listWorkItems', () => { '--repo', 'acme/widgets', '--assignee', - '@me' + '@me', + '--search', + 'sort:updated-desc' ], { cwd: '/repo-root' } ) @@ -205,7 +207,9 @@ describe('listWorkItems', () => { '--repo', 'acme/widgets', '--assignee', - '@me' + '@me', + '--search', + 'sort:updated-desc' ], { cwd: '/repo-root' } ) @@ -406,7 +410,9 @@ describe('listWorkItems', () => { 'acme/widgets', '--state', 'open', - '--draft' + '--draft', + '--search', + 'sort:updated-desc' ], { cwd: '/repo-root' } ) @@ -464,7 +470,9 @@ describe('listWorkItems', () => { '--repo', 'acme/widgets', '--state', - 'merged' + 'merged', + '--search', + 'sort:updated-desc' ], { cwd: '/repo-root' } ) @@ -528,7 +536,7 @@ describe('listWorkItems', () => { const { items } = await listWorkItems('/repo-root', 10, 'is:pr is:closed') expect(ghExecFileAsyncMock).toHaveBeenCalledWith( - expect.arrayContaining(['--state', 'closed', '--search', '-is:merged']), + expect.arrayContaining(['--state', 'closed', '--search', '-is:merged sort:updated-desc']), { cwd: '/repo-root' } ) expect(items).toMatchObject([{ id: 'pr:9', type: 'pr', state: 'closed' }]) @@ -597,7 +605,7 @@ describe('listWorkItems', () => { expect(ghExecFileAsyncMock).toHaveBeenCalledTimes(1) expect(ghExecFileAsyncMock).toHaveBeenCalledWith( - expect.arrayContaining(['--search', 'review-requested:@me']), + expect.arrayContaining(['--search', 'review-requested:@me sort:updated-desc']), { cwd: '/repo-root' } ) expect(ghExecFileAsyncMock).not.toHaveBeenCalledWith( @@ -606,6 +614,34 @@ describe('listWorkItems', () => { ) }) + it('pins list ordering to updated-desc so the updatedAt cursor pages consistently', async () => { + getIssueOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) + getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) + ghExecFileAsyncMock.mockResolvedValueOnce({ stdout: '[]' }) + + await listWorkItems('/repo-root', 10, 'is:issue is:open') + + expect(ghExecFileAsyncMock).toHaveBeenCalledWith( + expect.arrayContaining(['--search', 'sort:updated-desc']), + { cwd: '/repo-root' } + ) + }) + + it('combines the inclusive updatedAt cursor with updated-desc ordering on later pages', async () => { + getIssueOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) + getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) + ghExecFileAsyncMock.mockResolvedValueOnce({ stdout: '[]' }) + + await listWorkItems('/repo-root', 10, 'is:issue is:open', '2026-07-01T00:00:00Z') + + // Why: the bound is inclusive (`<=`) so boundary items sharing the cursor's + // exact updatedAt aren't skipped; the renderer dedupes the re-fetched rows. + expect(ghExecFileAsyncMock).toHaveBeenCalledWith( + expect.arrayContaining(['--search', 'updated:<=2026-07-01T00:00:00Z sort:updated-desc']), + { cwd: '/repo-root' } + ) + }) + it('returns open issues and PRs for the all-open preset query', async () => { getIssueOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) @@ -652,7 +688,9 @@ describe('listWorkItems', () => { '--repo', 'acme/widgets', '--state', - 'open' + 'open', + '--search', + 'sort:updated-desc' ], { cwd: '/repo-root' } ) @@ -667,7 +705,9 @@ describe('listWorkItems', () => { '--repo', 'acme/widgets', '--state', - 'open' + 'open', + '--search', + 'sort:updated-desc' ], { cwd: '/repo-root' } ) diff --git a/src/main/github/client.ts b/src/main/github/client.ts index 5848454e6..5330de5f7 100644 --- a/src/main/github/client.ts +++ b/src/main/github/client.ts @@ -455,6 +455,12 @@ type MainWorkItem = Omit const WORK_ITEM_ISSUE_LIST_JSON_FIELDS = 'number,title,state,url,labels,updatedAt,author,assignees' +// Why: the Tasks pager slices pages on updatedAt, so every list/search fetch +// must return rows newest-updated-first for the cursor to advance correctly. +// This is the search-qualifier spelling of the same contract the REST list +// paths express as `sort=updated&direction=desc`; keep the two in sync (#8649). +const WORK_ITEM_LIST_SORT_QUALIFIER = 'sort:updated-desc' + const WORK_ITEM_PR_LIST_JSON_FIELDS = 'number,title,state,url,labels,updatedAt,author,isDraft,headRefName,baseRefName,headRefOid,headRepositoryOwner,reviewRequests' @@ -996,11 +1002,13 @@ function buildWorkItemListArgs(args: { if (excludeMergedFromClosed) { searchParts.push('-is:merged') } - // Why: cursor-based pagination. GitHub search supports updated: 0) { - out.push('--search', searchParts.join(' ')) - } + // Why: pagination cursors slice on updatedAt, but `gh issue list` defaults + // to created-desc and `--search` defaults to best-match. `gh issue/pr list` + // has no --sort flag, so the only lever is the search query — which forces us + // to always emit --search. Without pinning the sort, recently-updated old + // items never appear on any page, so the pager advertises pages the fetch + // chain can never reach (#8649). + searchParts.push(WORK_ITEM_LIST_SORT_QUALIFIER) + out.push('--search', searchParts.join(' ')) return out } diff --git a/src/renderer/src/components/TaskPage.tsx b/src/renderer/src/components/TaskPage.tsx index 3b538d443..99fc42d71 100644 --- a/src/renderer/src/components/TaskPage.tsx +++ b/src/renderer/src/components/TaskPage.tsx @@ -205,6 +205,7 @@ import { type TaskPageRepoSourceState } from '@/components/task-page-cache-selectors' import { shouldHideTaskPageListChrome } from '@/components/task-page-list-chrome-visibility' +import { accumulateWorkItemPages } from '@/components/task-page-work-item-pagination' import LinearIssueAttributeFilterDropdowns from '@/components/linear-issue-attribute-filter-dropdowns' import { resolveLinearIssueAttributeFilterPrimaryTeam } from '@/components/linear-issue-attribute-filter-primary-team' import { @@ -6280,32 +6281,32 @@ export default function TaskPage(): React.JSX.Element { setPaginationLoading(true) setLoadingTargetPage(target) try { - let cursor = oldestItem.updatedAt - let loadedPages = pages.length - const newPages: GitHubWorkItem[][] = [] - - while (loadedPages <= target) { - const { items } = await fetchWorkItemsNextPage( - repoArgs, - PER_REPO_FETCH_LIMIT, - CROSS_REPO_DISPLAY_LIMIT, - q, - cursor - ) - if (paginationGenerationRef.current !== requestGeneration) { - return + const result = await accumulateWorkItemPages({ + existingPages: pages, + initialCursor: oldestItem.updatedAt, + targetPage: target, + // Why: uniform page size keeps deduped pages from shrinking below the + // size `totalPages` (count ÷ effectivePageSize) assumes — otherwise + // the per-page overlap loss would strand the tail items. + pageSize: effectivePageSize, + isCancelled: () => paginationGenerationRef.current !== requestGeneration, + fetchPage: async (cursor) => { + const { items } = await fetchWorkItemsNextPage( + repoArgs, + PER_REPO_FETCH_LIMIT, + CROSS_REPO_DISPLAY_LIMIT, + q, + cursor + ) + return { items } } - if (items.length === 0) { - break - } - newPages.push(items) - cursor = items.at(-1)!.updatedAt - loadedPages += 1 + }) + if (result.cancelled) { + return } - - if (newPages.length > 0) { - setPages((prev) => [...prev, ...newPages]) - setCurrentPage(target < loadedPages ? target : loadedPages - 1) + if (result.newPages.length > 0) { + setPages((prev) => [...prev, ...result.newPages]) + setCurrentPage(target < result.loadedPages ? target : result.loadedPages - 1) } } catch (err) { console.error('Failed to load next page:', err) @@ -6316,7 +6317,14 @@ export default function TaskPage(): React.JSX.Element { } } }, - [paginationLoading, selectedRepos, pages, appliedTaskSearch, fetchWorkItemsNextPage] + [ + paginationLoading, + selectedRepos, + pages, + appliedTaskSearch, + fetchWorkItemsNextPage, + effectivePageSize + ] ) useEffect(() => { 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 new file mode 100644 index 000000000..b3bc3301d --- /dev/null +++ b/src/renderer/src/components/task-page-work-item-pagination.test.ts @@ -0,0 +1,280 @@ +import { describe, expect, it, vi } from 'vitest' +import type { GitHubWorkItem } from '../../../shared/types' +import { accumulateWorkItemPages, workItemIdentity } from './task-page-work-item-pagination' + +function item(repoId: string, id: string, updatedAt: string): GitHubWorkItem { + return { + id, + type: 'issue', + number: Number.parseInt(id.split(':')[1] ?? '0', 10), + title: id, + state: 'open', + url: `https://example.test/${id}`, + labels: [], + updatedAt, + author: null, + repoId + } +} + +// Build a run of issues numbered `from`..`to` (inclusive) with strictly +// decreasing updatedAt, on repo `r`, so `at(-1)` is the oldest. +function run(r: string, from: number, to: number): GitHubWorkItem[] { + const out: GitHubWorkItem[] = [] + for (let n = from; n <= to; n += 1) { + // Larger number => newer; encode as a descending timestamp so sort order is + // unambiguous and distinct. + const ts = `2026-01-01T00:00:${String(1000 - n).padStart(4, '0')}Z` + out.push(item(r, `issue:${n}`, ts)) + } + return out +} + +describe('workItemIdentity', () => { + it('qualifies the bare id with the repo so cross-repo collisions are distinct', () => { + expect(workItemIdentity(item('repo-a', 'issue:9', 't'))).toBe('repo-a:issue:9') + expect(workItemIdentity(item('repo-b', 'issue:9', 't'))).not.toBe( + workItemIdentity(item('repo-a', 'issue:9', 't')) + ) + }) +}) + +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') + const existing = [[item('r', 'issue:1', '2026-07-03'), boundary]] + // Inclusive cursor re-returns issue:2 (boundary) then genuinely older rows, + // enough to fill a full page of size 2 after dedup. + const fetchPage = vi.fn().mockResolvedValue({ + items: [boundary, item('r', 'issue:3', '2026-07-01'), item('r', 'issue:4', '2026-07-00')] + }) + + const result = await accumulateWorkItemPages({ + existingPages: existing, + initialCursor: '2026-07-02', + targetPage: 1, + pageSize: 2, + fetchPage, + isCancelled: () => false + }) + + expect(result.cancelled).toBe(false) + if (result.cancelled) { + return + } + expect(result.newPages).toEqual([ + [item('r', 'issue:3', '2026-07-01'), item('r', 'issue:4', '2026-07-00')] + ]) + expect(result.loadedPages).toBe(2) + expect(fetchPage).toHaveBeenCalledWith('2026-07-02') + }) + + it('backfills across fetches so a deduped page stays full and the tail stays reachable', async () => { + // Regression: pageSize 3. Page 0 = issues 1..3. Inclusive cursor re-fetches + // the boundary (issue:3), so a naive "one fetch = one page" scheme would emit + // a 2-item page and the count-derived totalPages would strand the tail. + const existing = [run('r', 1, 3)] + const cursor0 = run('r', 1, 3).at(-1)!.updatedAt + const fetchPage = vi + .fn() + // <=cursor0 re-returns issue:3 (boundary) + 4,5 => 2 fresh, not yet a page. + .mockResolvedValueOnce({ items: run('r', 3, 5) }) + // Backfill fetches again from issue:5's ts and gets 5 (boundary) + 6 => 1 fresh. + .mockResolvedValueOnce({ items: run('r', 5, 6) }) + + const result = await accumulateWorkItemPages({ + existingPages: existing, + initialCursor: cursor0, + targetPage: 1, + pageSize: 3, + fetchPage, + isCancelled: () => false + }) + + expect(result.cancelled).toBe(false) + if (result.cancelled) { + return + } + // One uniform full page of the 3 genuinely-new items 4,5,6 — nothing stranded. + expect(result.newPages).toEqual([run('r', 4, 6)]) + expect(result.loadedPages).toBe(2) + expect(fetchPage).toHaveBeenCalledTimes(2) + expect(fetchPage).toHaveBeenNthCalledWith(2, run('r', 3, 5).at(-1)!.updatedAt) + }) + + it('does not confuse same-numbered items from different repos', async () => { + const existing = [[item('repo-a', 'issue:9', '2026-07-02')]] + // repo-b's issue:9 is a different item and must survive dedup. + const fetchPage = vi + .fn() + .mockResolvedValue({ items: [item('repo-b', 'issue:9', '2026-07-01')] }) + + const result = await accumulateWorkItemPages({ + existingPages: existing, + initialCursor: '2026-07-02', + targetPage: 1, + pageSize: 1, + fetchPage, + isCancelled: () => false + }) + + expect(result.cancelled).toBe(false) + if (result.cancelled) { + return + } + expect(result.newPages).toEqual([[item('repo-b', 'issue:9', '2026-07-01')]]) + expect(result.loadedPages).toBe(2) + }) + + it('flushes a short final page when the source is exhausted mid-page', async () => { + const existing = [run('r', 1, 3)] + const cursor0 = run('r', 1, 3).at(-1)!.updatedAt + const fetchPage = vi + .fn() + // 2 fresh rows, short of pageSize 3... + .mockResolvedValueOnce({ items: run('r', 3, 5) }) + // ...then the source is exhausted. + .mockResolvedValueOnce({ items: [] }) + + const result = await accumulateWorkItemPages({ + existingPages: existing, + initialCursor: cursor0, + targetPage: 1, + pageSize: 3, + fetchPage, + isCancelled: () => false + }) + + expect(result.cancelled).toBe(false) + if (result.cancelled) { + return + } + // Short final page rather than dropping issues 4,5. + expect(result.newPages).toEqual([run('r', 4, 5)]) + expect(result.loadedPages).toBe(2) + }) + + it('stops (flushing progress) when a full page yields nothing new', async () => { + const existing = [run('r', 1, 3)] + const cursor0 = run('r', 1, 3).at(-1)!.updatedAt + // First fetch adds issues 4,5 (2 fresh); second fetch re-returns only + // already-seen rows (a >pageSize same-timestamp run) — no forward progress. + const fetchPage = vi + .fn() + .mockResolvedValueOnce({ items: run('r', 3, 5) }) + .mockResolvedValueOnce({ items: run('r', 4, 5) }) + + const result = await accumulateWorkItemPages({ + existingPages: existing, + initialCursor: cursor0, + targetPage: 5, + pageSize: 3, + fetchPage, + isCancelled: () => false + }) + + expect(result.cancelled).toBe(false) + if (result.cancelled) { + return + } + // The 2 buffered fresh rows are flushed rather than lost. + expect(result.newPages).toEqual([run('r', 4, 5)]) + expect(fetchPage).toHaveBeenCalledTimes(2) + }) + + it('chains full pages until the target page is reached', async () => { + const existing = [run('r', 1, 2)] + const cursor0 = run('r', 1, 2).at(-1)!.updatedAt + const fetchPage = vi + .fn() + .mockResolvedValueOnce({ items: run('r', 2, 4) }) // boundary 2 + 3,4 + .mockResolvedValueOnce({ items: run('r', 4, 6) }) // boundary 4 + 5,6 + + const result = await accumulateWorkItemPages({ + existingPages: existing, + initialCursor: cursor0, + targetPage: 2, + pageSize: 2, + fetchPage, + isCancelled: () => false + }) + + expect(result.cancelled).toBe(false) + if (result.cancelled) { + return + } + expect(result.newPages).toEqual([run('r', 3, 4), run('r', 5, 6)]) + expect(result.loadedPages).toBe(3) + }) + + it('stops at an empty page (source exhausted) with no buffered rows', async () => { + const existing = [[item('r', 'issue:1', '2026-07-02')]] + const fetchPage = vi.fn().mockResolvedValue({ items: [] }) + + const result = await accumulateWorkItemPages({ + existingPages: existing, + initialCursor: '2026-07-02', + targetPage: 3, + pageSize: 3, + fetchPage, + isCancelled: () => false + }) + + expect(result).toEqual({ cancelled: false, newPages: [], loadedPages: 1 }) + }) + + it('recovers leftover rows discarded at the target boundary on the next call', async () => { + const pageSize = 3 + // Call 1: page0 = 1..3, target page 1. The fetch returns more than one + // page of fresh rows; issue 7 is past the emitted page and gets discarded + // (it belongs to page 2, which the user hasn't requested yet). + const existing1 = [run('r', 1, 3)] + const fetch1 = vi.fn().mockResolvedValueOnce({ items: run('r', 3, 7) }) // boundary 3 + 4,5,6,7 + const call1 = await accumulateWorkItemPages({ + existingPages: existing1, + initialCursor: run('r', 1, 3).at(-1)!.updatedAt, + targetPage: 1, + pageSize, + fetchPage: fetch1, + isCancelled: () => false + }) + expect(call1.cancelled).toBe(false) + if (call1.cancelled) { + return + } + expect(call1.newPages).toEqual([run('r', 4, 6)]) // 7 discarded + + // Call 2: page1 is now on screen. The inclusive cursor re-fetches the + // boundary (6, deduped) plus the discarded 7 and beyond — 7 must reappear + // exactly once, in order, with no duplicate of 4..6. + const call2 = await accumulateWorkItemPages({ + existingPages: [...existing1, ...call1.newPages], + initialCursor: call1.newPages.at(-1)!.at(-1)!.updatedAt, // ts(6) + targetPage: 2, + pageSize, + fetchPage: vi.fn().mockResolvedValueOnce({ items: run('r', 6, 9) }), // boundary 6 + 7,8,9 + isCancelled: () => false + }) + expect(call2.cancelled).toBe(false) + if (call2.cancelled) { + return + } + expect(call2.newPages).toEqual([run('r', 7, 9)]) // 7 recovered, deduped, in order + }) + + it('reports cancellation and discards fetched pages when superseded', async () => { + const existing = [[item('r', 'issue:1', '2026-07-02')]] + const fetchPage = vi.fn().mockResolvedValue({ items: [item('r', 'issue:2', '2026-07-01')] }) + + const result = await accumulateWorkItemPages({ + existingPages: existing, + initialCursor: '2026-07-02', + targetPage: 1, + pageSize: 1, + fetchPage, + isCancelled: () => true + }) + + expect(result).toEqual({ cancelled: true }) + }) +}) diff --git a/src/renderer/src/components/task-page-work-item-pagination.ts b/src/renderer/src/components/task-page-work-item-pagination.ts new file mode 100644 index 000000000..a332fe675 --- /dev/null +++ b/src/renderer/src/components/task-page-work-item-pagination.ts @@ -0,0 +1,125 @@ +import type { GitHubWorkItem } from '../../../shared/types' + +/** + * Cross-repo Tasks pagination is cursor-based on `updatedAt`: each page's oldest + * row seeds the next fetch as `updated:<=`. The bound is inclusive so + * items sharing the boundary row's exact timestamp aren't skipped between pages + * (#8649) — which means the boundary rows come back on the next fetch and must + * be deduped by identity here. + * + * Because dedup removes the re-fetched overlap, a single fetch no longer yields + * a full page. If we emitted each deduped fetch as its own page, pages 1+ would + * be one row short of `pageSize` while `totalPages` (count ÷ pageSize) still + * assumed full pages — stranding the tail items. So we backfill: accumulate + * fresh rows across as many fetches as needed and emit uniform `pageSize` pages, + * flushing a short final page only when the source is exhausted. + * + * Kept as a pure function (out of the TaskPage component) so the dedup + + * cursor-advance + fixed-page contract is unit-testable without a DOM. + * + * Tradeoff: when the per-repo fetch size equals `pageSize` the boundary dedup + * costs one extra fetch per page (and re-fetches the sub-page leftover on the + * next advance). Acceptable for interactive pagination and bounded by the gh + * rate-limit guard; persisting the cursor/buffer across calls to avoid the + * re-fetch is a possible follow-up. + */ + +// Why: `item.id` (e.g. "issue:9") is only unique within a repo — two selected +// repos can carry the same bare id. Key dedup on repo + id, matching the row +// key the table renders with. +export function workItemIdentity(item: Pick): string { + return `${item.repoId}:${item.id}` +} + +export type WorkItemPageFetchResult = { items: GitHubWorkItem[] } + +export type AccumulateWorkItemPagesArgs = { + /** Pages already on screen; their items seed the dedup set. */ + existingPages: readonly GitHubWorkItem[][] + /** updatedAt of the oldest currently-loaded row — the first fetch cursor. */ + initialCursor: string + /** 0-indexed page the user is trying to reach. */ + targetPage: number + /** Uniform display page size (the component's effectivePageSize). */ + pageSize: number + /** Fetch one page for the given inclusive cursor. */ + fetchPage: (cursor: string) => Promise + /** Returns true if the request was superseded and results should be dropped. */ + isCancelled: () => boolean +} + +export type AccumulateWorkItemPagesResult = + | { cancelled: true } + | { + cancelled: false + /** Freshly fetched, deduped, uniform-sized pages to append. */ + newPages: GitHubWorkItem[][] + /** Total loaded page count after appending (existing + new). */ + loadedPages: number + } + +/** + * Fetch pages until `targetPage` is reached (or the source is exhausted), + * deduping re-fetched boundary rows and emitting uniform `pageSize` pages so + * every item lands on exactly one page and page sizes stay consistent with the + * count-derived `totalPages`. + */ +export async function accumulateWorkItemPages( + args: AccumulateWorkItemPagesArgs +): Promise { + const { existingPages, initialCursor, targetPage, pageSize, fetchPage, isCancelled } = args + + const seen = new Set() + for (const page of existingPages) { + for (const item of page) { + seen.add(workItemIdentity(item)) + } + } + + let cursor = initialCursor + let loadedPages = existingPages.length + const newPages: GitHubWorkItem[][] = [] + // Fresh rows not yet emitted as a page, held until they fill `pageSize`. + let buffer: GitHubWorkItem[] = [] + + const emitFullPages = (): void => { + while (buffer.length >= pageSize && loadedPages <= targetPage) { + newPages.push(buffer.slice(0, pageSize)) + buffer = buffer.slice(pageSize) + loadedPages += 1 + } + } + + while (loadedPages <= targetPage) { + const { items } = await fetchPage(cursor) + if (isCancelled()) { + return { cancelled: true } + } + if (items.length === 0) { + break + } + const fresh = items.filter((item) => !seen.has(workItemIdentity(item))) + // Advance from the raw fetch, not the deduped rows, so the cursor tracks + // real data. If a full page yields nothing new the cursor can't move past + // this timestamp (a rare >pageSize same-timestamp run), so stop rather than + // re-fetch the same window forever. + cursor = items.at(-1)!.updatedAt + if (fresh.length === 0) { + break + } + for (const item of fresh) { + seen.add(workItemIdentity(item)) + } + buffer.push(...fresh) + emitFullPages() + } + + // Source exhausted (or stalled) with a partial page still buffered: emit it as + // a short final page so those items remain reachable. + if (buffer.length > 0 && loadedPages <= targetPage) { + newPages.push(buffer) + loadedPages += 1 + } + + return { cancelled: false, newPages, loadedPages } +}