From 9f3f08d59b24bb2b450cef184bf43cd7146f038a Mon Sep 17 00:00:00 2001 From: OrcaWin Date: Wed, 28 Oct 2026 21:20:23 -0700 Subject: [PATCH] Reuse search counts and cursors across GitHub work-item reads (#35552) * Reuse work-item page totals for matching count queries * Verify page totals avoid later count requests * Read numbered PR pages with reusable cursors * Combine short cursor prefixes with the requested page * Use advanced search for grouped PR cursor queries * Validate combined quota changes against the final PR base * Verify forced and credential-scoped PR pages bypass saved cursors * Batch uncached PR reads and reuse equivalent PR totals * Match Boolean PR totals to the advanced list predicate * Keep malformed PR count groups on the scoped legacy search --------- Co-authored-by: OrcaWin <293788423+OrcaWin@users.noreply.github.com> --- .../__fixtures__/work-item-search-api.ts | 17 +- .../github/client/list/count-work-items.ts | 42 ++- .../client/list/work-item-list-request.ts | 10 +- .../github/client/list/work-item-pages.ts | 51 ++-- .../github/client/list/work-item-pr-page.ts | 121 ++++++++ .../client/list/work-item-search-batch.ts | 100 ++++++- .../client/list/work-item-search-page.ts | 33 +- .../github/work-item-pr-cursor-page.test.ts | 282 ++++++++++++++++++ .../work-item-search-count-reuse.test.ts | 130 ++++++++ .../github/work-item-search-freshness.test.ts | 39 ++- .../work-item-search-pagination.test.ts | 20 +- .../github/work-item-search-semantics.test.ts | 25 +- ...github-work-item-search-burst.unit.test.ts | 1 + 13 files changed, 795 insertions(+), 76 deletions(-) create mode 100644 src/main/github/client/list/work-item-pr-page.ts create mode 100644 src/main/github/work-item-pr-cursor-page.test.ts create mode 100644 src/main/github/work-item-search-count-reuse.test.ts diff --git a/src/main/github/__fixtures__/work-item-search-api.ts b/src/main/github/__fixtures__/work-item-search-api.ts index e48a5818cb9..aeb3c4920c3 100644 --- a/src/main/github/__fixtures__/work-item-search-api.ts +++ b/src/main/github/__fixtures__/work-item-search-api.ts @@ -24,6 +24,9 @@ export class WorkItemSearchApi { throw new Error(`Unexpected fixture search ${query}`) } const repo = /repo:([^\s]+)/.exec(query)?.[1] ?? 'unknown/repo' + if (!this.specialNodes && /(?:^|\s)type:pr(?:\s|$)/.test(query)) { + return [] + } return ( this.specialNodes ?? Array.from({ length: this.rowsPerRepo }, (_, index) => ({ @@ -135,7 +138,7 @@ export class WorkItemSearchApi { const query = args.find((arg) => arg.startsWith('query='))?.slice(6) ?? '' const fields = [ ...query.matchAll( - /(r\d+): search\(type: ISSUE, query: ("(?:[^"\\]|\\.)*"), first: (\d+)(?:, after: ("(?:[^"\\]|\\.)*"))?\)/g + /(r\d+): search\(type: (ISSUE|ISSUE_ADVANCED), query: ("(?:[^"\\]|\\.)*"), first: (\d+)(?:, after: ("(?:[^"\\]|\\.)*"))?\)/g ) ] if (!fields.length) { @@ -146,24 +149,24 @@ export class WorkItemSearchApi { for (let index = 0; index < fields.length; index++) { const field = fields[index] this.graphqlFields++ - const search = z.string().parse(JSON.parse(field[2])) + const search = z.string().parse(JSON.parse(field[3])) if (this.aliasErrorRepo && search.includes(`repo:${this.aliasErrorRepo} `)) { data[field[1]] = null errors.push({ message: 'fixture repository search unavailable', path: [field[1]] }) continue } - const first = Number(field[3]) - const cursor = field[4] ? z.string().parse(JSON.parse(field[4])) : undefined + const first = Number(field[4]) + const cursor = field[5] ? z.string().parse(JSON.parse(field[5])) : undefined const saved = cursor ? this.cursors.get(cursor) : undefined - if (cursor && (!saved || saved.query !== search)) { + if (cursor && (!saved || saved.query !== `${field[2]}:${search}`)) { throw new Error('Unknown or cross-query opaque cursor') } const offset = saved?.offset ?? 0 - const rows = this.rows(search) + const rows = field[2] === 'ISSUE' && search.startsWith('( ') ? [] : this.rows(search) const page = rows.slice(offset, offset + first) const next = offset + page.length const endCursor = `opaque:${++this.nextCursor}:cursor` - this.cursors.set(endCursor, { query: search, offset: next }) + this.cursors.set(endCursor, { query: `${field[2]}:${search}`, offset: next }) const selection = query.slice(field.index, fields[index + 1]?.index ?? query.length) data[field[1]] = { issueCount: this.reportedCount ?? rows.length, diff --git a/src/main/github/client/list/count-work-items.ts b/src/main/github/client/list/count-work-items.ts index 4d8c2183d46..98f57cf2dbe 100644 --- a/src/main/github/client/list/count-work-items.ts +++ b/src/main/github/client/list/count-work-items.ts @@ -22,9 +22,39 @@ import { spendsSharedGitHubComQuota } from '../../rate-limit' import { sameOwnerRepo } from './../github-exec-scope' -import { resolvePrWorkItemSource } from './work-item-list-request' +import { buildWorkItemListRequest, resolvePrWorkItemSource } from './work-item-list-request' import { buildSearchQueryString, defaultOpenWorkItemQuery } from './work-item-search-query' import { searchWorkItemCount, usesGraphqlWorkItemSearch } from './work-item-search-page' + +function canGroupSearchText(text: string): boolean { + let depth = 0 + let quoted = false + let escaped = false + for (const char of text) { + if (quoted) { + if (escaped) { + escaped = false + } else if (char === '\\') { + escaped = true + } else if (char === '"') { + quoted = false + } + } else if (char === '"') { + quoted = true + } else if (char === '\\') { + return false + } else if (char === '(') { + depth += 1 + } else if (char === ')') { + depth -= 1 + if (depth < 0) { + return false + } + } + } + return depth === 0 && !quoted +} + export async function countWorkItemsForQuery( repoPath: string, ownerRepo: OwnerRepo, @@ -40,6 +70,16 @@ export async function countWorkItemsForQuery( if (usesGraphqlWorkItemSearch(ownerRepo, ghOptions)) { ghOptions.env = { ...process.env } try { + if (query.scope === 'pr' && canGroupSearchText(query.freeText)) { + const request = buildWorkItemListRequest({ + kind: 'pr', + ownerRepo, + query, + limit: 1, + page: 1 + }) + return await searchWorkItemCount(request.search, ghOptions, 'ISSUE_ADVANCED') + } return await searchWorkItemCount(searchQ, ghOptions) } catch (error) { if (error instanceof Error && error.name === 'AbortError') { diff --git a/src/main/github/client/list/work-item-list-request.ts b/src/main/github/client/list/work-item-list-request.ts index d02b5ec1c28..1a7207e654f 100644 --- a/src/main/github/client/list/work-item-list-request.ts +++ b/src/main/github/client/list/work-item-list-request.ts @@ -13,6 +13,7 @@ import { WORK_ITEM_NUMBER_SORT_QUALIFIER, quoteGitHubSearchValue } from './work- export type WorkItemListRequest = { args: string[] offset: number + search: string } export function normalizeWorkItemPage(page: number | undefined): number { @@ -80,7 +81,8 @@ export function buildWorkItemListRequest(args: { '--jq', '.items' ], - offset: 0 + offset: 0, + search: searchParts.join(' ') } } @@ -98,7 +100,11 @@ export function buildWorkItemListRequest(args: { ] out.push('--repo', `${ownerRepo.owner}/${ownerRepo.repo}`) out.push('--search', searchParts.join(' ')) - return { args: out, offset: (page - 1) * limit } + return { + args: out, + offset: (page - 1) * limit, + search: `( ${searchParts.join(' ')} ) repo:${ownerRepo.owner}/${ownerRepo.repo} type:pr` + } } // Carry per-source failures through IPC so pagination cannot mistake them for end-of-data. diff --git a/src/main/github/client/list/work-item-pages.ts b/src/main/github/client/list/work-item-pages.ts index 90163bfa7a5..76948bd09e7 100644 --- a/src/main/github/client/list/work-item-pages.ts +++ b/src/main/github/client/list/work-item-pages.ts @@ -3,7 +3,6 @@ import { classifyGitHubUnavailable } from '../../../../shared/github/api-availab import { parseTaskQuery, type ParsedTaskQuery } from '../../../../shared/task-query' import { sortWorkItemsByNumber } from '../../../../shared/work-items' import { - ghExecFileAsync, classifyListIssuesError, classifyListPrsError, ghRepoExecOptions, @@ -15,13 +14,12 @@ import { githubHostExecOptions } from '../../github-api-repository' import { githubPRStackExecutionScope } from './../github-exec-scope' import { hydrateWorkItemRepositoryMergeMetadata } from './../detect/hydrate-work-item-merge-metadata' import type { MainWorkItem } from './../map/work-item-field-coercion' -import { mapPullRequestWorkItem } from './../map/work-item' import { - buildWorkItemListRequest, assertSshRepoHasResolvedGitHubSource, type PartialWorkItemsResult } from './work-item-list-request' import { listIssueWorkItemPage } from './work-item-issue-page' +import { listPullRequestWorkItemPage } from './work-item-pr-page' export async function listRecentWorkItems( repoPath: string, issueOwnerRepo: OwnerRepo | null, @@ -35,15 +33,6 @@ export async function listRecentWorkItems( const ghOptions = ghRepoExecOptions(githubRepoContext(repoPath, connectionId, localGitOptions)) assertSshRepoHasResolvedGitHubSource({ connectionId, issueOwnerRepo, prOwnerRepo }) const recentQuery = parseTaskQuery('is:open') - const prRequest = prOwnerRepo - ? buildWorkItemListRequest({ - kind: 'pr', - ownerRepo: prOwnerRepo, - limit, - query: recentQuery, - page - }) - : null // Isolate missing sources to avoid global searches, and retain successful halves on partial failure. const [issuesSettled, prsSettled] = await Promise.allSettled([ issueOwnerRepo @@ -59,12 +48,16 @@ export async function listRecentWorkItems( noCache }) : Promise.resolve([]), - prRequest && prOwnerRepo - ? ghExecFileAsync(prRequest.args, { - ...ghOptions, - ...githubHostExecOptions(prOwnerRepo) + prOwnerRepo + ? listPullRequestWorkItemPage({ + ownerRepo: prOwnerRepo, + query: recentQuery, + limit, + page, + options: { ...ghOptions, ...githubHostExecOptions(prOwnerRepo) }, + noCache }) - : Promise.resolve({ stdout: '[]' }) + : Promise.resolve([]) ]) let issues: MainWorkItem[] = [] @@ -81,9 +74,7 @@ export async function listRecentWorkItems( let prs: MainWorkItem[] = [] if (prsSettled.status === 'fulfilled') { - prs = (JSON.parse(prsSettled.value.stdout) as Record[]) - .slice(prRequest?.offset ?? 0, (prRequest?.offset ?? 0) + limit) - .map((item) => mapPullRequestWorkItem(item, prOwnerRepo)) + prs = prsSettled.value prs = await hydrateWorkItemRepositoryMergeMetadata( prs, prOwnerRepo, @@ -173,21 +164,15 @@ export async function listQueriedWorkItems( if (!prOwnerRepo) { return [] } - const request = buildWorkItemListRequest({ - kind: 'pr', - ownerRepo: prOwnerRepo, - limit, - query, - page: page ?? 1 - }) try { - const { stdout } = await ghExecFileAsync(request.args, { - ...ghOptions, - ...githubHostExecOptions(prOwnerRepo) + const mapped = await listPullRequestWorkItemPage({ + ownerRepo: prOwnerRepo, + limit, + query, + page: page ?? 1, + options: { ...ghOptions, ...githubHostExecOptions(prOwnerRepo) }, + noCache }) - const mapped = (JSON.parse(stdout) as Record[]) - .slice(request.offset, request.offset + limit) - .map((item) => mapPullRequestWorkItem(item, prOwnerRepo)) const hydrated = await hydrateWorkItemRepositoryMergeMetadata( mapped, prOwnerRepo, diff --git a/src/main/github/client/list/work-item-pr-page.ts b/src/main/github/client/list/work-item-pr-page.ts new file mode 100644 index 00000000000..0dea5c98ecb --- /dev/null +++ b/src/main/github/client/list/work-item-pr-page.ts @@ -0,0 +1,121 @@ +import { z } from 'zod' +import type { ParsedTaskQuery } from '../../../../shared/task-query' +import { ghExecFileAsync, type OwnerRepo } from '../../gh-utils' +import type { GitHubRepoExecOptions } from '../../github-api-repository' +import { mapPullRequestWorkItem } from '../map/work-item' +import type { MainWorkItem } from '../map/work-item-field-coercion' +import { buildWorkItemListRequest } from './work-item-list-request' +import { searchWorkItemPage, usesGraphqlWorkItemSearch } from './work-item-search-page' + +const PR_NODE_SELECTION = `__typename ... on PullRequest { + number title state url updatedAt isDraft headRefName baseRefName headRefOid + headRepositoryOwner { login } + author { __typename login avatarUrl ... on User { name } } + labels(first: 100) { nodes { name } pageInfo { hasNextPage } } + reviewRequests(first: 100) { + nodes { requestedReviewer { + __typename + ... on User { login name avatarUrl } + ... on Bot { login avatarUrl } + ... on Team { slug name } + ... on Mannequin { login avatarUrl } + } } + pageInfo { hasNextPage } + } +}` +const actorSchema = z.object({ + __typename: z.string().optional(), + login: z.string(), + avatarUrl: z.string(), + name: z.string().nullish() +}) +const prNodeSchema = z.object({ + __typename: z.literal('PullRequest'), + number: z.number().int().positive(), + title: z.string(), + state: z.enum(['OPEN', 'CLOSED', 'MERGED']), + url: z.string(), + updatedAt: z.string(), + isDraft: z.boolean(), + headRefName: z.string(), + baseRefName: z.string(), + headRefOid: z.string(), + headRepositoryOwner: z.object({ login: z.string() }).nullable(), + author: actorSchema.nullable(), + labels: z.object({ + nodes: z.array(z.object({ name: z.string() })), + pageInfo: z.object({ hasNextPage: z.literal(false) }) + }), + reviewRequests: z.object({ + nodes: z.array( + z + .object({ + requestedReviewer: z + .object({ + login: z.string().optional(), + avatarUrl: z.string().optional(), + name: z.string().nullish() + }) + .nullable() + }) + .nullable() + ), + pageInfo: z.object({ hasNextPage: z.literal(false) }) + }) +}) + +export async function listPullRequestWorkItemPage(args: { + ownerRepo: OwnerRepo + query: ParsedTaskQuery + limit: number + page: number + options: GitHubRepoExecOptions + noCache?: boolean +}): Promise { + const request = buildWorkItemListRequest({ kind: 'pr', ...args }) + const preferGraphql = args.limit <= 100 && usesGraphqlWorkItemSearch(args.ownerRepo, args.options) + const options = preferGraphql + ? { ...args.options, env: { ...(args.options.env ?? process.env) } } + : args.options + if (preferGraphql) { + try { + const nodes = await searchWorkItemPage({ + search: request.search, + searchType: 'ISSUE_ADVANCED', + nodeSelection: PR_NODE_SELECTION, + limit: args.limit, + page: args.page, + options, + noCache: args.noCache, + cacheResults: false + }) + return z + .array(prNodeSchema) + .parse(nodes) + .map((node) => + mapPullRequestWorkItem( + { + ...node, + author: + node.author?.__typename === 'Bot' + ? { ...node.author, login: `app/${node.author.login}` } + : node.author, + labels: node.labels.nodes, + reviewRequests: node.reviewRequests.nodes + }, + args.ownerRepo + ) + ) + } catch (error) { + if (error instanceof Error && error.name === 'AbortError') { + throw error + } + } + } + const { stdout } = await ghExecFileAsync(request.args, options) + return z + .array(z.record(z.string(), z.unknown())) + .parse(JSON.parse(stdout)) + .slice(request.offset, request.offset + args.limit) + .map((item) => mapPullRequestWorkItem(item, args.ownerRepo)) +} diff --git a/src/main/github/client/list/work-item-search-batch.ts b/src/main/github/client/list/work-item-search-batch.ts index c7779b95269..d4550ad2f57 100644 --- a/src/main/github/client/list/work-item-search-batch.ts +++ b/src/main/github/client/list/work-item-search-batch.ts @@ -20,20 +20,25 @@ const envelopeSchema = z.object({ .optional() }) type Envelope = z.infer +export type WorkItemSearchType = 'ISSUE' | 'ISSUE_ADVANCED' type SearchRequest = { search: string + searchType?: WorkItemSearchType first: number after?: string selection: string + countSearch?: string options: GitHubRepoExecOptions environment?: NodeJS.ProcessEnv noCache?: boolean + cacheResults?: boolean } type PendingSearch = { request: SearchRequest environment: NodeJS.ProcessEnv resolve: (value: unknown) => void reject: (error: unknown) => void + countReaders?: PendingSearch[] } export const WORK_ITEM_SEARCH_CACHE_MS = 120_000 @@ -49,18 +54,58 @@ const responses = new BoundedMap({ sizeOf: (value, key) => Buffer.byteLength(key) + Buffer.byteLength(JSON.stringify(value)) }) -export function requestWorkItemSearch(request: SearchRequest): Promise> { - const environment = { ...(request.environment ?? request.options.env ?? process.env) } - const scope = workItemSearchScope(request.options, environment) - const key = JSON.stringify([ +function responseKey( + scope: string, + request: Pick +): string { + return JSON.stringify([ scope, + request.searchType ?? 'ISSUE', request.search, request.first, request.after, request.selection ]) - const cached = request.noCache ? undefined : responses.get(key) +} + +function countFromResponse(value: unknown): { issueCount: number } | undefined { + if (value && typeof value === 'object' && 'issueCount' in value) { + const count = value.issueCount + if (typeof count === 'number' && Number.isSafeInteger(count) && count >= 0) { + return { issueCount: count } + } + } + return undefined +} + +function rememberCount( + scope: string, + request: SearchRequest, + response: SearchResponse +): void { + const count = countFromResponse(response.value) + if (!request.countSearch || !count) { + return + } + const key = responseKey(scope, { + search: request.countSearch, + searchType: request.searchType, + first: 1, + selection: 'issueCount' + }) + const previous = responses.get(key) + if (!previous || previous.at <= response.at) { + responses.set(key, { at: response.at, value: count }) + } +} + +export function requestWorkItemSearch(request: SearchRequest): Promise> { + const environment = { ...(request.environment ?? request.options.env ?? process.env) } + const scope = workItemSearchScope(request.options, environment) + const key = responseKey(scope, request) + const cached = request.noCache || request.cacheResults === false ? undefined : responses.get(key) if (cached && Date.now() - cached.at < WORK_ITEM_SEARCH_CACHE_MS) { + rememberCount(scope, request, cached) // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: The cache key includes the complete selection; callers validate its response shape. return Promise.resolve(cached as SearchResponse) } @@ -78,7 +123,10 @@ export function requestWorkItemSearch(request: SearchRequest): Promise> @@ -90,9 +138,31 @@ function flushSearches(key: string): void { if (!queue) { return } + const countSources = new Map() + for (const entry of queue) { + if (entry.request.countSearch) { + countSources.set( + JSON.stringify([entry.request.searchType ?? 'ISSUE', entry.request.countSearch]), + entry + ) + } + } + const independent = queue.filter((entry) => { + const source = + entry.request.selection === 'issueCount' && entry.request.first === 1 && !entry.request.after + ? countSources.get( + JSON.stringify([entry.request.searchType ?? 'ISSUE', entry.request.search]) + ) + : undefined + if (source) { + ;(source.countReaders ??= []).push(entry) + return false + } + return true + }) let batch: PendingSearch[] = [] let characters = 0 - for (const entry of queue) { + for (const entry of independent) { const size = searchSelection(entry.request, batch.length).length if (batch.length && (batch.length === MAX_BATCH || characters + size > MAX_BATCH_QUERY_CHARS)) { void executeSearches(batch) @@ -109,7 +179,7 @@ function flushSearches(key: string): void { function searchSelection(request: SearchRequest, index: number): string { const after = request.after ? `, after: ${JSON.stringify(request.after)}` : '' - return `r${index}: search(type: ISSUE, query: ${JSON.stringify(request.search)}, first: ${request.first}${after}) { ${request.selection} }` + return `r${index}: search(type: ${request.searchType ?? 'ISSUE'}, query: ${JSON.stringify(request.search)}, first: ${request.first}${after}) { ${request.selection} }` } async function executeSearches(batch: PendingSearch[]): Promise { @@ -160,6 +230,17 @@ async function executeSearches(batch: PendingSearch[]): Promise { (error) => !error.path?.length || error.path[0] === alias ) const value = envelope.data?.[alias] + const count = countFromResponse(value) + for (const reader of entry.countReaders ?? []) { + if ( + count && + !errors?.some(({ path }) => !path || path.length < 2 || path[1] === 'issueCount') + ) { + reader.resolve(count) + } else { + reader.reject(new Error('GitHub search response missing count')) + } + } if (errors?.length || value === undefined || value === null) { entry.reject( new Error( @@ -174,6 +255,9 @@ async function executeSearches(batch: PendingSearch[]): Promise { } catch (error) { for (const entry of batch) { entry.reject(error) + for (const reader of entry.countReaders ?? []) { + reader.reject(error) + } } } } diff --git a/src/main/github/client/list/work-item-search-page.ts b/src/main/github/client/list/work-item-search-page.ts index e03c16e777b..b22fb90c3d5 100644 --- a/src/main/github/client/list/work-item-search-page.ts +++ b/src/main/github/client/list/work-item-search-page.ts @@ -5,7 +5,8 @@ import type { GitHubRepoExecOptions } from '../../github-api-repository' import { requestWorkItemSearch, workItemSearchScope, - WORK_ITEM_SEARCH_CACHE_MS + WORK_ITEM_SEARCH_CACHE_MS, + type WorkItemSearchType } from './work-item-search-batch' type PageInfo = { endCursor: string | null; hasNextPage: boolean } @@ -27,10 +28,12 @@ export function usesGraphqlWorkItemSearch( export async function searchWorkItemCount( search: string, - options: GitHubRepoExecOptions + options: GitHubRepoExecOptions, + searchType: WorkItemSearchType = 'ISSUE' ): Promise { const { value: result } = await requestWorkItemSearch<{ issueCount: number }>({ search, + searchType, first: 1, selection: 'issueCount', options @@ -43,11 +46,13 @@ export async function searchWorkItemCount( export async function searchWorkItemPage(args: { search: string + searchType?: WorkItemSearchType nodeSelection: string limit: number page: number options: GitHubRepoExecOptions noCache?: boolean + cacheResults?: boolean }): Promise { const { options, noCache } = args const environment = { ...(options.env ?? process.env) } @@ -60,7 +65,11 @@ export async function searchWorkItemPage(args: { throw new Error('Only the first 1000 search results are available (HTTP 422)') } const search = `${args.search} sort:created-desc` - const scope = JSON.stringify([workItemSearchScope(options, environment), search]) + const scope = JSON.stringify([ + workItemSearchScope(options, environment), + args.searchType ?? 'ISSUE', + search + ]) let position = 0 let after: string | undefined if (!noCache) { @@ -87,16 +96,19 @@ export async function searchWorkItemPage(args: { cursors.set(`${scope}:${position}`, { at, cursor: info.endCursor }) } } - while (position < offset) { + while (offset - position + limit > 100) { const first = Math.min(100, offset - position) const { value: skipped, at } = await requestWorkItemSearch>({ search, + searchType: args.searchType, + countSearch: args.search, first, after, selection: 'issueCount pageInfo { endCursor hasNextPage }', options, environment, - noCache + noCache, + cacheResults: args.cacheResults }) if (!skipped.pageInfo || !Number.isSafeInteger(skipped.issueCount)) { throw new Error('GitHub search response missing pagination') @@ -113,16 +125,19 @@ export async function searchWorkItemPage(args: { } const { value: result, at } = await requestWorkItemSearch>({ search, - first: limit, + searchType: args.searchType, + first: offset - position + limit, + countSearch: args.search, after, selection: `issueCount pageInfo { endCursor hasNextPage } nodes { ${args.nodeSelection} }`, options, environment, - noCache + noCache, + cacheResults: args.cacheResults }) if (!Array.isArray(result.nodes) || !result.pageInfo) { throw new Error('GitHub search response missing page') } - remember(offset + result.nodes.length, result.pageInfo, at) - return result.nodes + remember(position + result.nodes.length, result.pageInfo, at) + return result.nodes.slice(offset - position, offset - position + limit) } diff --git a/src/main/github/work-item-pr-cursor-page.test.ts b/src/main/github/work-item-pr-cursor-page.test.ts new file mode 100644 index 00000000000..4f8070926b1 --- /dev/null +++ b/src/main/github/work-item-pr-cursor-page.test.ts @@ -0,0 +1,282 @@ +import { expect, it, vi } from 'vitest' +import { api, capture, sourceContext } from './work-item-search-test-harness' +import { parseTaskQuery } from '../../shared/task-query' +import { listPullRequestWorkItemPage } from './client/list/work-item-pr-page' +import { listWorkItems } from './client/list/list-work-items' +import { searchWorkItemPage } from './client/list/work-item-search-page' +import { requestWorkItemSearch } from './client/list/work-item-search-batch' + +const ownerRepo = { owner: 'fixture', repo: 'repo', host: 'github.com' } +function rows() { + return Array.from({ length: 300 }, (_, index) => ({ + __typename: 'PullRequest', + number: 10000 - index, + title: `PR ${index}`, + state: 'OPEN', + url: `https://github.com/fixture/repo/pull/${10000 - index}`, + updatedAt: '2026-10-01T00:00:00Z', + author: { login: 'author', name: 'Author', avatarUrl: 'https://avatars.example/author' }, + isDraft: false, + headRefName: `branch-${index}`, + baseRefName: 'main', + headRefOid: `head-${index}`, + headRepositoryOwner: { login: 'fork-owner' }, + labels: { nodes: [{ name: 'bug' }], pageInfo: { hasNextPage: false } }, + reviewRequests: { + nodes: [ + { + requestedReviewer: { + login: 'reviewer', + name: 'Reviewer', + avatarUrl: 'https://avatars.example/reviewer' + } + }, + { requestedReviewer: { __typename: 'Team', slug: 'team', name: 'Team' } }, + null + ], + pageInfo: { hasNextPage: false } + } + })) +} +function page(number: number, noCache = false) { + return listPullRequestWorkItemPage({ + ownerRepo, + query: parseTaskQuery('is:pr is:open'), + limit: 24, + page: number, + options: {}, + noCache + }) +} + +it('reads only the next PR page while browsing ten numbered pages', async () => { + api.specialNodes = rows() + const numbers: number[] = [] + for (let number = 1; number <= 10; number++) { + numbers.push(...(await page(number)).map((item) => item.number)) + } + expect(numbers).toEqual(Array.from({ length: 240 }, (_, index) => 10000 - index)) + expect(api.graphqlCalls).toBe(10) + expect(api.graphqlFields).toBe(10) + expect(api.calls.some(({ args }) => args[0] === 'pr')).toBe(false) +}) + +it('keeps content uncached and preserves rich rows through the production listing', async () => { + api.specialNodes = rows() + const first = await listWorkItems('fixture/repo', 24, 'is:pr is:open') + expect(first.items[0]).toMatchObject({ + title: 'PR 0', + labels: ['bug'], + branchName: 'branch-0', + baseRefName: 'main', + headSha: 'head-0', + isCrossRepository: true, + author: 'author', + reviewRequests: [{ login: 'reviewer', name: 'Reviewer' }] + }) + api.specialNodes[0].title = 'New title' + expect((await page(1))[0].title).toBe('New title') + expect( + api.calls.filter(({ args }) => args.some((arg) => arg.includes('r0: search('))) + ).toHaveLength(2) +}) + +it('starts without saved cursors on a manual refresh of a later page', async () => { + api.specialNodes = rows() + await page(1) + await page(2) + const before = api.graphqlCalls + expect((await page(2, true))[0].number).toBe(9976) + expect(api.graphqlCalls - before).toBe(1) + expect(api.calls.at(-1)?.args.join(' ')).toContain('first: 48') + expect(api.calls.at(-1)?.args.join(' ')).not.toContain('after:') +}) + +it.each(['labels', 'reviewRequests', 'unavailable'])( + 'retains the rich CLI fallback for %s', + async (reason) => { + api.specialNodes = rows() + if (reason !== 'unavailable') { + api.specialNodes[0][reason] = { nodes: [], pageInfo: { hasNextPage: true } } + } + capture.mockImplementation(async (binary, args, options) => { + if (reason === 'unavailable' && args.includes('graphql')) { + throw Object.assign(new Error('GraphQL unavailable'), { stderr: 'HTTP 500: unavailable' }) + } + return args[0] === 'pr' + ? { + stdout: JSON.stringify([ + { + number: 7, + title: 'Fallback', + state: 'OPEN', + labels: [{ name: 'all labels' }], + reviewRequests: [{ login: 'fallback-reviewer' }] + } + ]), + stderr: '' + } + : api.capture(binary, args, options) + }) + expect((await page(1))[0]).toMatchObject({ + title: 'Fallback', + labels: ['all labels'], + reviewRequests: [{ login: 'fallback-reviewer' }] + }) + expect( + capture.mock.calls.some( + ([, args]) => args.includes('--search') && args.includes('is:open sort:created-desc') + ) + ).toBe(true) + } +) + +it('retains the Enterprise CLI route', async () => { + sourceContext.host = 'github.example.com' + await listWorkItems('fixture/repo', 24, 'is:pr') + expect(api.graphqlCalls).toBe(0) + expect(api.calls[0].args).toContain('--json') +}) + +it('does not share PR cursors across credentials', async () => { + api.specialNodes = rows() + await page(1) + vi.stubEnv('GH_TOKEN', 'fixture-new') + const before = api.graphqlCalls + expect((await page(2))[0].number).toBe(9976) + expect(api.graphqlCalls - before).toBe(1) + expect(api.calls.at(-1)?.args.join(' ')).toContain('first: 48') + expect(api.calls.at(-1)?.args.join(' ')).not.toContain('after:') +}) + +it('routes sequential production PR pages through cursor reads', async () => { + api.specialNodes = rows() + const numbers: number[] = [] + capture.mockImplementation(async (binary, args, options) => { + if (args[0] === 'pr' && args[1] === 'list') { + const limit = Number(args[args.indexOf('--limit') + 1]) + return { + stdout: JSON.stringify( + rows() + .slice(0, limit) + .map((row) => ({ + ...row, + labels: row.labels.nodes, + reviewRequests: row.reviewRequests.nodes + })) + ), + stderr: '' + } + } + return api.capture(binary, args, options) + }) + for (let number = 1; number <= 10; number++) { + numbers.push( + ...(await listWorkItems('fixture/repo', 24, 'is:pr is:open', number)).items.map( + (item) => item.number + ) + ) + } + expect(numbers).toEqual(Array.from({ length: 240 }, (_, index) => 10000 - index)) + expect( + capture.mock.calls.filter(([, args]) => args[0] === 'pr' && args[1] === 'list') + ).toHaveLength(0) + expect( + capture.mock.calls.filter(([, args]) => args.some((arg) => arg.includes('r0: search('))) + ).toHaveLength(10) +}) + +it('keeps Boolean search operators inside the repository and PR constraints', async () => { + api.specialNodes = rows() + api.expectedSearch = + '( is:open title:foo OR title:bar sort:created-desc ) repo:fixture/repo type:pr' + expect( + await listPullRequestWorkItemPage({ + ownerRepo, + query: parseTaskQuery('is:pr is:open title:foo OR title:bar'), + limit: 24, + page: 1, + options: {} + }) + ).toHaveLength(24) + expect(api.calls.some(({ args }) => args[0] === 'pr')).toBe(false) +}) + +it('combines a cold short prefix with the requested PR page', async () => { + api.specialNodes = rows() + expect((await page(2))[0].number).toBe(9976) + expect(api.graphqlCalls).toBe(1) + expect(api.calls[0].args.join(' ')).toContain('first: 48') +}) + +it('uses advanced search for the grouped PR query and preserves CLI bot author display', async () => { + api.specialNodes = rows() + api.specialNodes[0].author = { + __typename: 'Bot', + login: 'dependabot', + avatarUrl: 'https://example.test/bot' + } + expect((await page(1))[0].author).toBe('app/dependabot') + expect(api.calls[0].args.join(' ')).toContain('search(type: ISSUE_ADVANCED,') +}) + +it('isolates search response coalescing, content and cursor identity by search type', async () => { + api.specialNodes = rows() + const args = { + search: 'repo:fixture/search-type-isolation is:open', + nodeSelection: '__typename ... on PullRequest { number }', + limit: 24, + page: 1, + options: {} + } + await Promise.all([ + searchWorkItemPage({ ...args, searchType: 'ISSUE' }), + searchWorkItemPage({ ...args, searchType: 'ISSUE_ADVANCED' }) + ]) + expect(api.graphqlFields).toBe(2) + await searchWorkItemPage({ ...args, searchType: 'ISSUE' }) + await searchWorkItemPage({ ...args, searchType: 'ISSUE_ADVANCED' }) + expect(api.graphqlFields).toBe(2) + + const cold = { ...args, search: 'repo:fixture/cursor-type-isolation is:open' } + await searchWorkItemPage({ ...cold, searchType: 'ISSUE' }) + expect(await searchWorkItemPage({ ...cold, page: 2, searchType: 'ISSUE_ADVANCED' })).toHaveLength( + 24 + ) + expect(api.calls.at(-1)?.args.join(' ')).toContain('first: 48') +}) + +it('does not attach legacy count readers to advanced pages or seed their cache', async () => { + api.specialNodes = rows() + const search = 'repo:fixture/count-type-isolation is:open' + const advancedPage = () => + searchWorkItemPage({ + search, + searchType: 'ISSUE_ADVANCED', + nodeSelection: '__typename ... on PullRequest { number }', + limit: 24, + page: 1, + options: {} + }) + const legacyCount = () => + requestWorkItemSearch({ search, first: 1, selection: 'issueCount', options: {} }) + await Promise.all([advancedPage(), legacyCount()]) + expect(api.graphqlFields).toBe(2) + + await searchWorkItemPage({ + search: `${search} label:only-advanced`, + searchType: 'ISSUE_ADVANCED', + nodeSelection: '__typename ... on PullRequest { number }', + limit: 24, + page: 1, + options: {} + }) + const before = api.graphqlFields + await requestWorkItemSearch({ + search: `${search} label:only-advanced`, + first: 1, + selection: 'issueCount', + options: {} + }) + expect(api.graphqlFields - before).toBe(1) +}) diff --git a/src/main/github/work-item-search-count-reuse.test.ts b/src/main/github/work-item-search-count-reuse.test.ts new file mode 100644 index 00000000000..c18f416c866 --- /dev/null +++ b/src/main/github/work-item-search-count-reuse.test.ts @@ -0,0 +1,130 @@ +import { expect, it, vi } from 'vitest' +import { api, capture } from './work-item-search-test-harness' +import { listWorkItems } from './client/list/list-work-items' +import { countWorkItems } from './client/list/count-work-items' +import { searchWorkItemCount, searchWorkItemPage } from './client/list/work-item-search-page' + +it('uses the issue page total for a simultaneous or later production count', async () => { + api.reportedCount = 2748 + const [page, count] = await Promise.all([ + listWorkItems('fixture/repo', 24, 'is:issue is:open'), + countWorkItems('fixture/repo', 'is:issue is:open') + ]) + expect(page.items).toHaveLength(24) + expect(count).toBe(2748) + expect(api.graphqlCalls).toBe(1) + expect(api.graphqlFields).toBe(1) + expect(await countWorkItems('fixture/repo', 'is:issue is:open')).toBe(2748) + expect(api.graphqlCalls).toBe(1) + expect(api.restSearches).toBe(0) +}) + +it('serves a later matching count without another request', async () => { + api.reportedCount = 2748 + await listWorkItems('fixture/repo', 24, 'is:issue') + expect(await countWorkItems('fixture/repo', 'is:issue')).toBe(2748) + expect(api.graphqlCalls).toBe(1) +}) + +it.each([ + 'is:pr is:open', + 'is:pr is:closed', + 'is:pr is:draft label:bug', + 'is:pr fix OR feat', + 'is:pr (github OR terminal) AND fix', + 'is:pr "fix ) OR ("', + String.raw`is:pr "fix \" ) OR ( \" feat"` +])('reuses a rich PR page total for matching counts: %s', async (query) => { + api.specialNodes = [] + api.reportedCount = 2748 + const [page, count] = await Promise.all([ + listWorkItems('fixture/repo', 24, query), + countWorkItems('fixture/repo', query) + ]) + expect(page.items).toEqual([]) + expect(count).toBe(2748) + expect(api.graphqlCalls).toBe(1) + expect(api.graphqlFields).toBe(1) + expect(await countWorkItems('fixture/repo', query)).toBe(2748) + expect(api.graphqlCalls).toBe(1) + expect(api.restSearches).toBe(0) +}) + +it.each(['fix ) OR (', '( fix', '"fix ) OR (', String.raw`fix \") OR (\" open`])( + 'keeps malformed PR count text on the original scoped query: %s', + async (text) => { + api.expectedSearch = `repo:fixture/repo is:pull-request ${text}` + api.reportedCount = 17 + expect(await countWorkItems('fixture/repo', `is:pr ${text}`)).toBe(17) + expect(api.graphqlCalls).toBe(1) + expect(api.calls.at(-1)?.args.join(' ')).toContain('search(type: ISSUE,') + expect(api.restSearches).toBe(0) + } +) + +it('removes count-only aliases before applying the existing batch limit', async () => { + const pairs = Array.from({ length: 25 }, (_, index) => { + const search = `repo:fixture/repo-${index} is:issue` + return [ + searchWorkItemCount(search, {}), + searchWorkItemPage({ search, nodeSelection: '__typename', limit: 24, page: 1, options: {} }) + ] + }) + await Promise.all(pairs.flat()) + expect(api.graphqlFields).toBe(25) + expect(api.graphqlCalls).toBe(3) +}) + +it('retains a valid total when a page field fails and the list needs REST recovery', async () => { + capture.mockImplementation(async (binary, args, options) => { + const result = await api.capture(binary, args, options) + if (!args.includes('graphql')) { + return result + } + const envelope = JSON.parse(result.stdout) + const alias = Object.keys(envelope.data).find((key) => /^r\d+$/.test(key)) + envelope.errors = [{ path: [alias, 'nodes', 0, 'title'], message: 'Unavailable title' }] + throw Object.assign(new Error('Partial GraphQL failure'), { + stdout: JSON.stringify(envelope), + stderr: 'Partial GraphQL failure' + }) + }) + const [page, count] = await Promise.all([ + listWorkItems('fixture/repo', 24, 'is:issue'), + countWorkItems('fixture/repo', 'is:issue') + ]) + expect(page.items).toHaveLength(24) + expect(count).toBe(120) + expect(api.graphqlFields).toBe(1) + expect(api.restSearches).toBe(1) +}) + +it('does not renew the page total when a cached page is read again', async () => { + await listWorkItems('fixture/repo', 24, 'is:issue') + vi.setSystemTime(119000) + await listWorkItems('fixture/repo', 24, 'is:issue') + api.reportedCount = 121 + vi.setSystemTime(120001) + expect(await countWorkItems('fixture/repo', 'is:issue')).toBe(121) + expect(api.graphqlCalls).toBe(2) +}) + +it('keeps forced page reads separate from cached count reads', async () => { + await listWorkItems('fixture/repo', 24, 'is:issue', 1, undefined, undefined, true) + expect(await countWorkItems('fixture/repo', 'is:issue')).toBe(120) + expect(api.graphqlCalls).toBe(2) + expect(api.graphqlFields).toBe(2) +}) + +it('does not share totals across predicates, credentials or execution scopes', async () => { + await listWorkItems('fixture/repo', 24, 'is:issue') + api.reportedCount = 17 + expect(await countWorkItems('fixture/repo', 'is:issue label:bug')).toBe(17) + expect(await countWorkItems('fixture/other', 'is:issue')).toBe(17) + vi.stubEnv('GH_TOKEN', 'fixture-new-credential') + expect(await countWorkItems('fixture/repo', 'is:issue')).toBe(17) + expect( + await countWorkItems('fixture/repo', 'is:issue', undefined, undefined, { wslDistro: 'Ubuntu' }) + ).toBe(17) + expect(api.graphqlFields).toBe(5) +}) diff --git a/src/main/github/work-item-search-freshness.test.ts b/src/main/github/work-item-search-freshness.test.ts index bbc59bc471e..d67d196b80f 100644 --- a/src/main/github/work-item-search-freshness.test.ts +++ b/src/main/github/work-item-search-freshness.test.ts @@ -1,6 +1,6 @@ import { expect, it, vi } from 'vitest' import { api, capture } from './work-item-search-test-harness' -import { searchWorkItemCount } from './client/list/work-item-search-page' +import { searchWorkItemCount, searchWorkItemPage } from './client/list/work-item-search-page' it('does not renew a gh-cached response after bounded response-cache eviction', async () => { const ghCache = new Map< @@ -30,3 +30,40 @@ it('does not renew a gh-cached response after bounded response-cache eviction', vi.setSystemTime(120001) expect(await searchWorkItemCount(search, {})).toBe(121) }) + +it('starts a forced read independently of an uncached content read in flight', async () => { + let release: (() => void) | undefined + let started: (() => void) | undefined + const firstStarted = new Promise((resolve) => { + started = resolve + }) + capture.mockImplementation(async (binary, args, options) => { + const result = await api.capture(binary, args, options) + if (api.graphqlCalls === 1) { + const held = new Promise((resolve) => { + release = resolve + }) + started?.() + await held + } + return result + }) + const args = { + search: 'repo:fixture/repo is:issue', + nodeSelection: '__typename', + limit: 24, + page: 1, + options: {}, + cacheResults: false + } + const ordinary = searchWorkItemPage(args) + await firstStarted + const forced = searchWorkItemPage({ ...args, noCache: true }) + try { + await vi.waitFor(() => expect(api.graphqlCalls).toBe(2)) + } finally { + release?.() + } + expect(await ordinary).toHaveLength(24) + expect(await forced).toHaveLength(24) +}) diff --git a/src/main/github/work-item-search-pagination.test.ts b/src/main/github/work-item-search-pagination.test.ts index 5e07878e5e6..02b54a8a978 100644 --- a/src/main/github/work-item-search-pagination.test.ts +++ b/src/main/github/work-item-search-pagination.test.ts @@ -20,11 +20,11 @@ it('restores a cold numbered page using only API-issued opaque cursors', async ( expect(third.items.map((item) => item.number)).toEqual( Array.from({ length: 24 }, (_, i) => 9952 - i) ) - expect(api.graphqlCalls).toBe(2) - expect(api.calls[0].args.join(' ')).not.toContain(' nodes {') - expect(api.calls[1].args.join(' ')).toContain('after: "opaque:1:cursor"') + expect(api.graphqlCalls).toBe(1) + expect(api.calls[0].args.join(' ')).toContain('first: 72') + expect(api.calls[0].args.join(' ')).not.toContain('after:') expect((await issuePage(4)).items[0].number).toBe(9928) - expect(api.graphqlCalls).toBe(3) + expect(api.graphqlCalls).toBe(2) expect((await issuePage(6)).items).toEqual([]) expect(api.restSearches).toBe(0) }) @@ -48,15 +48,15 @@ it('walks long jumps without node hydration and retains the authoritative 1000-r it('bypasses both page and cursor caches on refresh and expires retained entries', async () => { await issuePage(3) - expect(api.graphqlCalls).toBe(2) + expect(api.graphqlCalls).toBe(1) await issuePage(3) - expect(api.graphqlCalls).toBe(2) + expect(api.graphqlCalls).toBe(1) await issuePage(3, true) - expect(api.graphqlCalls).toBe(4) - expect(api.calls.slice(-2).every((call) => !call.args.includes('--cache'))).toBe(true) + expect(api.graphqlCalls).toBe(2) + expect(api.calls.at(-1)?.args).not.toContain('--cache') vi.setSystemTime(120001) await issuePage(3) - expect(api.graphqlCalls).toBe(6) + expect(api.graphqlCalls).toBe(3) expect(api.restSearches).toBe(0) }) @@ -66,6 +66,6 @@ it('does not renew cursor freshness when re-reading a cached page', async () => await issuePage(1) vi.setSystemTime(120001) await issuePage(2) - expect(api.graphqlCalls).toBe(3) + expect(api.graphqlCalls).toBe(2) expect(api.calls[1].args.join(' ')).not.toContain('after:') }) diff --git a/src/main/github/work-item-search-semantics.test.ts b/src/main/github/work-item-search-semantics.test.ts index 1a13406d3d4..f419bc0014f 100644 --- a/src/main/github/work-item-search-semantics.test.ts +++ b/src/main/github/work-item-search-semantics.test.ts @@ -1,4 +1,4 @@ -import { expect, it } from 'vitest' +import { expect, it, vi } from 'vitest' import { api } from './work-item-search-test-harness' import { listWorkItems } from './client/list/list-work-items' import { countWorkItems } from './client/list/count-work-items' @@ -70,16 +70,31 @@ it.each([ 'review-requested:"some user" reviewed-by:someone', 'review-requested:"some user" reviewed-by:someone sort:created-desc', 'repo:fixture/repo is:pull-request review-requested:"some user" reviewed-by:someone' + ], + [ + 'is:pr title:foo OR title:bar', + 'title:foo OR title:bar sort:created-desc', + 'repo:fixture/repo is:pull-request title:foo OR title:bar' ] ])('keeps rich PR lists and full count predicates for %s', async (query, prSearch, countSearch) => { + api.expectedSearch = `( ${prSearch} ) repo:fixture/repo type:pr` + api.specialNodes = [] + api.reportedCount = 120 expect((await listWorkItems('fixture/repo', 24, query)).items).toEqual([]) - expect(api.graphqlCalls).toBe(0) + expect(api.graphqlCalls).toBe(1) expect(api.restSearches).toBe(0) - expect(api.calls[0].args).toContain(prSearch) - expect(api.calls[0].args).toContain('--json') - api.expectedSearch = countSearch + const request = api.calls.find(({ args }) => args.includes('graphql')) + expect(request?.args.join(' ')).toContain( + JSON.stringify(`( ${prSearch} ) repo:fixture/repo type:pr sort:created-desc`) + ) + expect(request?.args.join(' ')).toContain('reviewRequests(first: 100)') expect(await countWorkItems('fixture/repo', query)).toBe(120) expect(api.graphqlCalls).toBe(1) + vi.stubEnv('GH_TOKEN', 'fixture-rest-fallback') + api.graphqlAvailable = false + api.expectedSearch = countSearch + expect(await countWorkItems('fixture/repo', query)).toBe(120) + expect(api.restSearches).toBe(1) }) it('falls back with the original numbered query when GraphQL is unavailable', async () => { diff --git a/tests/e2e/github-work-item-search-burst.unit.test.ts b/tests/e2e/github-work-item-search-burst.unit.test.ts index bcebb88ec58..2f39739b12a 100644 --- a/tests/e2e/github-work-item-search-burst.unit.test.ts +++ b/tests/e2e/github-work-item-search-burst.unit.test.ts @@ -59,6 +59,7 @@ it.each(['counts-first', 'lists-first'] as const)( expect(total).toEqual({ totalCount: 36 * 120, totalPages: 5 }) expect(api.restSearches).toBe(0) expect(api.rejected).toBe(0) + expect(api.calls.some(({ args }) => args[0] === 'pr')).toBe(false) expect(api.graphqlCalls).toBeLessThanOrEqual(72) } )