fix(github): pin work-item list ordering to updated-desc so cursor pagination reaches every page (#8658)
* fix(github): pin work-item list ordering to updated-desc so cursor pagination reaches every page The Tasks page paginates work items with an updatedAt cursor (updated:<oldest-item), but the underlying gh calls never pinned a sort: 'gh issue list' defaults to created-desc and '--search' defaults to best-match. Items created long ago but updated recently therefore never appeared on any page — page 0 (created order) skipped them and every later page excluded them via the cursor — so the pager advertised pages the fetch chain could never reach, clicks on them clamped to the last real page, and cross-page ordering was scrambled. Append sort:updated-desc to every list/search invocation so the fetch order matches the cursor field on the first and all subsequent pages. Verified against a live 588-issue repo: the cursor chain previously died around page 5; it now traverses 585/588 unique issues (the remainder is the pre-existing strict '<' boundary edge for items sharing the cursor's exact timestamp). Fixes #8649 * fix(github): make work-item cursor pagination lossless at updatedAt boundaries Builds on the sort-pin fix: switch the pagination cursor from strict 'updated:<' to inclusive 'updated:<=' so items sharing the boundary row's exact updatedAt are no longer skipped between pages (the residual 3/588 edge in #8649). The inclusive bound re-fetches the boundary rows, so dedupe them by repoId+id (a bare item.id like 'issue:9' collides across repos). Extract the page accumulation out of the 12k-line TaskPage component into a pure, unit-tested helper (accumulateWorkItemPages) that dedupes and backfills: it accumulates fresh rows across fetches and emits uniform pageSize pages, so deduped pages never shrink below the size totalPages (count / effectivePageSize) assumes — which would otherwise strand the tail items and break the no-count degraded pager. Also hoist the updated-desc ordering into a named WORK_ITEM_LIST_SORT_QUALIFIER constant so the cursor's ordering contract has one home. Tradeoff: when per-repo fetch size equals pageSize, the boundary dedupe costs one extra fetch per page; acceptable for interactive pagination and bounded by the gh rate-limit guard. Persisting the cursor/buffer across calls is a possible follow-up. --------- Co-authored-by: OrcaWin <alpha-eng@stably.ai>
This commit is contained in:
moseoh
GitHub
parent
8b86e7ead7
commit
7d8c4fdca1
|
|
@ -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' }
|
||||
)
|
||||
|
|
|
|||
|
|
@ -455,6 +455,12 @@ type MainWorkItem = Omit<GitHubWorkItem, 'repoId'>
|
|||
|
||||
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:<DATE to
|
||||
// fetch items older than the cursor. We use the oldest item's updatedAt
|
||||
// from the previous page as the cursor.
|
||||
// Why: cursor-based pagination. GitHub search supports updated:<=DATE to
|
||||
// fetch items at or older than the cursor. We use the oldest item's updatedAt
|
||||
// from the previous page as the cursor. The bound is inclusive (`<=`) so items
|
||||
// sharing the boundary row's exact updatedAt aren't skipped between pages; the
|
||||
// renderer dedupes the re-fetched boundary rows by id (#8649).
|
||||
if (before) {
|
||||
searchParts.push(`updated:<${before}`)
|
||||
searchParts.push(`updated:<=${before}`)
|
||||
}
|
||||
if (kind === 'pr' && query.reviewRequested) {
|
||||
searchParts.push(`review-requested:${query.reviewRequested}`)
|
||||
|
|
@ -1011,9 +1019,14 @@ function buildWorkItemListArgs(args: {
|
|||
if (query.freeText) {
|
||||
searchParts.push(query.freeText)
|
||||
}
|
||||
if (searchParts.length > 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
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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(() => {
|
||||
|
|
|
|||
|
|
@ -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 })
|
||||
})
|
||||
})
|
||||
|
|
@ -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:<=<cursor>`. 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<GitHubWorkItem, 'id' | 'repoId'>): 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<WorkItemPageFetchResult>
|
||||
/** 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<AccumulateWorkItemPagesResult> {
|
||||
const { existingPages, initialCursor, targetPage, pageSize, fetchPage, isCancelled } = args
|
||||
|
||||
const seen = new Set<string>()
|
||||
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 }
|
||||
}
|
||||
Loading…
Reference in New Issue