From 5eed81d187a369a81660e740068e40361be1942b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= <4062236+iscekic@users.noreply.github.com> Date: Tue, 20 Oct 2026 01:26:25 +0000 Subject: [PATCH 1/5] fix(github): cap listing pagination per call Branch and repository inventories share one page collector with a fixed page ceiling, and it logs a warning when it stops at that ceiling. Review-thread comment follow-ups are capped per thread and per request, with a small concurrency limit, so one review-threads call performs a fixed amount of provider work and keeps a bounded response. --- apps/web/src/lib/github-pr-review/dtos.ts | 13 ++ .../review-thread-comments.test.ts | 142 ++++++++++++++++++ .../github/adapter.pagination.test.ts | 134 +++++++++++++++++ .../integrations/platforms/github/adapter.ts | 136 ++++++++--------- .../routers/github-pr-review-router.test.ts | 110 ++++++++++++++ .../src/routers/github-pr-review-router.ts | 141 ++++++++++++----- 6 files changed, 574 insertions(+), 102 deletions(-) create mode 100644 apps/web/src/lib/integrations/platforms/github/adapter.pagination.test.ts diff --git a/apps/web/src/lib/github-pr-review/dtos.ts b/apps/web/src/lib/github-pr-review/dtos.ts index 1b0318f4bcf..c3104501c75 100644 --- a/apps/web/src/lib/github-pr-review/dtos.ts +++ b/apps/web/src/lib/github-pr-review/dtos.ts @@ -230,3 +230,16 @@ export const REVIEW_THREAD_COMMENTS_PAGE_SIZE = 50; // dropped and whatever was collected is returned (silent truncation). export const CONVERSATION_COMMENTS_PAGE_SIZE = 100; export const CONVERSATION_COMMENTS_MAX_PAGES = 5; +// Per-thread review comments beyond the first page: at most 5 follow-up pages +// of 50, so one thread shows its initial 50 plus at most 250 follow-up comments. +// Past the ceiling the thread keeps what was collected (silent truncation), +// matching the conversation-comment scan above. +export const REVIEW_THREAD_COMMENTS_MAX_PAGES = 5; +// One review-threads request shares this follow-up comment budget across every +// thread on the page, so a pull request with many long threads cannot multiply +// the retained response. Threads reached after the budget is spent keep their +// initial page only. +export const REVIEW_THREAD_COMMENTS_MAX_PER_REQUEST = 500; +// Threads on one page are enriched with at most this many concurrent GitHub +// requests. +export const REVIEW_THREADS_ENRICHMENT_CONCURRENCY = 5; diff --git a/apps/web/src/lib/github-pr-review/review-thread-comments.test.ts b/apps/web/src/lib/github-pr-review/review-thread-comments.test.ts index a85bb251e14..7c171e4cbda 100644 --- a/apps/web/src/lib/github-pr-review/review-thread-comments.test.ts +++ b/apps/web/src/lib/github-pr-review/review-thread-comments.test.ts @@ -3,9 +3,14 @@ */ import { CONVERSATION_COMMENTS_QUERY_FOR_TEST, + enrichReviewThreads, fetchAllThreadComments, REVIEW_THREAD_COMMENTS_FOLLOWUP_QUERY_FOR_TEST, } from '@/routers/github-pr-review-router'; +import { + REVIEW_THREAD_COMMENTS_MAX_PER_REQUEST, + REVIEW_THREAD_COMMENTS_PAGE_SIZE, +} from '@/lib/github-pr-review/dtos'; function commentNode( id: number, @@ -103,6 +108,7 @@ describe('fetchAllThreadComments', () => { pageInfo: { hasNextPage: true, endCursor: 'c1' }, nodes: [commentNode(1)], }, + budget: { remainingFollowUpComments: REVIEW_THREAD_COMMENTS_MAX_PER_REQUEST }, }); // All three pages aggregated to completion — no silent truncation. @@ -194,6 +200,7 @@ describe('fetchAllThreadComments', () => { ]), ], }, + budget: { remainingFollowUpComments: REVIEW_THREAD_COMMENTS_MAX_PER_REQUEST }, }); expect(comments[0]?.reactions).toEqual([ @@ -202,3 +209,138 @@ describe('fetchAllThreadComments', () => { ]); }); }); + +function commentPage(startId: number, count: number, hasNextPage: boolean) { + return { + pageInfo: { hasNextPage, endCursor: hasNextPage ? `cursor_${startId + count}` : null }, + nodes: Array.from({ length: count }, (_, index) => commentNode(startId + index)), + }; +} + +function threadNode(index: number, comments: ReturnType) { + return { + id: `thread_${index}`, + isResolved: false, + isOutdated: false, + subjectType: 'LINE' as const, + path: `src/file-${index}.ts`, + line: index + 1, + startLine: null, + originalLine: null, + originalStartLine: null, + diffSide: 'RIGHT' as const, + comments, + }; +} + +describe('thread comment pagination bounds', () => { + it('stops at the per-thread page ceiling while GitHub keeps reporting another page', async () => { + let nextId = 1000; + const request = jest.fn(async () => { + const start = nextId; + nextId += REVIEW_THREAD_COMMENTS_PAGE_SIZE; + return { + data: { + data: { + node: { comments: commentPage(start, REVIEW_THREAD_COMMENTS_PAGE_SIZE, true) }, + }, + }, + }; + }); + + const comments = await fetchAllThreadComments({ + octokit: { request } as never, + threadId: 'thread_1', + initialConnection: commentPage(1, REVIEW_THREAD_COMMENTS_PAGE_SIZE, true), + // Far above the per-thread ceiling: the page ceiling is the binding cap. + budget: { remainingFollowUpComments: 1_000_000 }, + }); + + expect(request).toHaveBeenCalledTimes(5); + // Initial page plus the 5-follow-up-page ceiling, 50 comments per page. + expect(comments).toHaveLength(300); + }); + + it('spends one follow-up budget across threads and stops asking once it is empty', async () => { + // One page of 50 plus 10 from the next page, then the budget is spent. + const budget = { remainingFollowUpComments: 60 }; + let nextId = 5000; + const request = jest.fn(async () => { + const start = nextId; + nextId += REVIEW_THREAD_COMMENTS_PAGE_SIZE; + return { + data: { + data: { + node: { comments: commentPage(start, REVIEW_THREAD_COMMENTS_PAGE_SIZE, true) }, + }, + }, + }; + }); + + const first = await fetchAllThreadComments({ + octokit: { request } as never, + threadId: 'thread_1', + initialConnection: commentPage(1, 1, true), + budget, + }); + const requestsAfterFirst = request.mock.calls.length; + const second = await fetchAllThreadComments({ + octokit: { request } as never, + threadId: 'thread_2', + initialConnection: commentPage(2, 1, true), + budget, + }); + + expect(request).toHaveBeenCalledTimes(requestsAfterFirst); + expect(first).toHaveLength(61); + expect(second).toHaveLength(1); + expect(budget.remainingFollowUpComments).toBe(0); + }); + + it('never runs more than the configured number of thread comment requests at once', async () => { + let inFlight = 0; + let maxInFlight = 0; + const request = jest.fn(async () => { + inFlight += 1; + maxInFlight = Math.max(maxInFlight, inFlight); + // Yield one microtask so every concurrent slot is observed as in flight + // before any request settles; no wall-clock delay is involved. + await Promise.resolve(); + inFlight -= 1; + return { data: { data: { node: { comments: commentPage(9000, 1, false) } } } }; + }); + const nodes = Array.from({ length: 15 }, (_, index) => + threadNode(index, commentPage(index * 10 + 1, 1, true)) + ); + + const threads = await enrichReviewThreads({ octokit: { request } as never, nodes }); + + expect(threads).toHaveLength(nodes.length); + expect(request).toHaveBeenCalledTimes(nodes.length); + expect(maxInFlight).toBe(5); + }); + + it('caps follow-up comments per request while every thread keeps its initial page', async () => { + const request = jest.fn(async () => ({ + data: { + data: { + node: { + comments: commentPage(2000, REVIEW_THREAD_COMMENTS_PAGE_SIZE, true), + }, + }, + }, + })); + const threadCount = 20; + const nodes = Array.from({ length: threadCount }, (_, index) => + threadNode(index, commentPage(index * 10 + 1, 1, true)) + ); + + const threads = await enrichReviewThreads({ octokit: { request } as never, nodes }); + + const collected = threads.reduce((total, thread) => total + thread.comments.length, 0); + expect(collected - threadCount).toBe(500); + // Threads reached after the budget is spent still carry their initial page. + expect(threads.some(thread => thread.comments.length === 1)).toBe(true); + expect(threads.every(thread => thread.comments.length >= 1)).toBe(true); + }); +}); diff --git a/apps/web/src/lib/integrations/platforms/github/adapter.pagination.test.ts b/apps/web/src/lib/integrations/platforms/github/adapter.pagination.test.ts new file mode 100644 index 00000000000..93e7fe55ee7 --- /dev/null +++ b/apps/web/src/lib/integrations/platforms/github/adapter.pagination.test.ts @@ -0,0 +1,134 @@ +/** + * @jest-environment node + */ + +const mockListBranches = jest.fn(); +const mockListReposAccessibleToInstallation = jest.fn(); +const mockGetRepo = jest.fn(); + +jest.mock('@octokit/rest', () => ({ + Octokit: jest.fn().mockImplementation(() => ({ + apps: { listReposAccessibleToInstallation: mockListReposAccessibleToInstallation }, + repos: { get: mockGetRepo, listBranches: mockListBranches }, + })), +})); +jest.mock('@octokit/auth-app', () => ({ + createAppAuth: () => async () => ({ + token: 'installation-token', + expiresAt: '2099-01-01T00:00:00Z', + }), +})); +jest.mock('./app-selector', () => ({ + getGitHubAppCredentials: () => ({ appId: '1', privateKey: 'test' }), +})); +jest.mock('../../github/runtime-authorization', () => ({ + assertGitHubInstallationRuntimeAuthorized: jest.fn(async () => undefined), +})); +jest.mock('@kilocode/web-shared/lib/utils.server', () => ({ + logExceptInTest: jest.fn(), + warnExceptInTest: jest.fn(), +})); + +import { warnExceptInTest } from '@kilocode/web-shared/lib/utils.server'; +import { + fetchGitHubBranches, + fetchGitHubRepositories, + fetchGitHubRepositoriesForMaintenance, +} from './adapter'; + +function branchPage(page: number, count: number) { + return { + data: Array.from({ length: count }, (_, index) => ({ name: `branch-${page}-${index}` })), + }; +} + +function repoPage(page: number, count: number, archivedCount = 0) { + return { + data: { + repositories: Array.from({ length: count }, (_, index) => ({ + id: page * 1000 + index, + name: `repo-${page}-${index}`, + full_name: `acme/repo-${page}-${index}`, + private: false, + archived: index < archivedCount, + created_at: '2024-01-01T00:00:00Z', + })), + }, + }; +} + +beforeEach(() => { + jest.clearAllMocks(); +}); + +describe('fetchGitHubBranches pagination bounds', () => { + it('stops at a fixed page count when the provider keeps returning full pages', async () => { + mockGetRepo.mockResolvedValue({ data: { default_branch: 'main' } }); + mockListBranches.mockImplementation(async ({ page }: { page: number }) => + branchPage(page, 100) + ); + + const branches = await fetchGitHubBranches('123456', 'acme/repo'); + + expect(mockListBranches).toHaveBeenCalledTimes(10); + expect(branches).toHaveLength(1000); + expect(branches[0]).toEqual({ name: 'branch-1-0', isDefault: false }); + expect(warnExceptInTest).toHaveBeenCalledTimes(1); + }); + + it('stops on a short page without reaching the ceiling and marks the default branch', async () => { + mockGetRepo.mockResolvedValue({ data: { default_branch: 'branch-2-0' } }); + mockListBranches + .mockImplementationOnce(async () => branchPage(1, 100)) + .mockImplementationOnce(async () => branchPage(2, 3)); + + const branches = await fetchGitHubBranches('123456', 'acme/repo'); + + expect(mockListBranches).toHaveBeenCalledTimes(2); + expect(branches).toHaveLength(103); + expect(branches.find(branch => branch.isDefault)).toEqual({ + name: 'branch-2-0', + isDefault: true, + }); + expect(warnExceptInTest).not.toHaveBeenCalled(); + }); +}); + +describe('fetchGitHubRepositories pagination bounds', () => { + it('stops at a fixed page count when the provider keeps returning full pages', async () => { + mockListReposAccessibleToInstallation.mockImplementation(async ({ page }: { page: number }) => + repoPage(page, 100) + ); + + const repositories = await fetchGitHubRepositories('123456'); + + expect(mockListReposAccessibleToInstallation).toHaveBeenCalledTimes(50); + expect(repositories).toHaveLength(5000); + expect(warnExceptInTest).toHaveBeenCalledTimes(1); + }); + + it('stops on a short page and still drops archived repositories', async () => { + mockListReposAccessibleToInstallation + .mockImplementationOnce(async () => repoPage(1, 100)) + .mockImplementationOnce(async () => repoPage(2, 3, 1)); + + const repositories = await fetchGitHubRepositories('123456'); + + expect(mockListReposAccessibleToInstallation).toHaveBeenCalledTimes(2); + expect(repositories).toHaveLength(102); + expect(repositories.map(repository => repository.full_name)).not.toContain('acme/repo-2-0'); + expect(warnExceptInTest).not.toHaveBeenCalled(); + }); + + it('bounds the maintenance listing with the same ceiling', async () => { + mockListReposAccessibleToInstallation.mockImplementation(async ({ page }: { page: number }) => + repoPage(page, 100) + ); + + const repositories = await fetchGitHubRepositoriesForMaintenance('123456', 'standard'); + + expect(mockListReposAccessibleToInstallation).toHaveBeenCalledTimes(50); + expect(repositories).toHaveLength(5000); + expect(warnExceptInTest).toHaveBeenCalledTimes(1); + }); +}); diff --git a/apps/web/src/lib/integrations/platforms/github/adapter.ts b/apps/web/src/lib/integrations/platforms/github/adapter.ts index c6f5ed1d387..e924260d853 100644 --- a/apps/web/src/lib/integrations/platforms/github/adapter.ts +++ b/apps/web/src/lib/integrations/platforms/github/adapter.ts @@ -180,6 +180,31 @@ type GitHubBranch = { isDefault: boolean; }; +const LISTING_PAGE_SIZE = 100; +// One listing call performs at most this many provider page requests and keeps +// at most `pages × LISTING_PAGE_SIZE` rows. Past the ceiling the remainder is +// dropped and the call logs a warning, the same bounded-scan convention the +// capped review scans use. +const MAX_REPOSITORY_PAGES = 50; // 5,000 repositories +const MAX_BRANCH_PAGES = 10; // 1,000 branches + +async function collectListingPages(params: { + maxPages: number; + scope: string; + fetchPage: (page: number) => Promise; +}): Promise { + const collected: T[] = []; + for (let page = 1; page <= params.maxPages; page += 1) { + const items = await params.fetchPage(page); + collected.push(...items); + if (items.length < LISTING_PAGE_SIZE) return collected; + } + warnExceptInTest( + `GitHub ${params.scope} reached the ${params.maxPages}-page ceiling; returning the first ${collected.length} entries` + ); + return collected; +} + export async function fetchGitHubRepositories( installationId: string, appType: GitHubAppType = 'standard', @@ -194,33 +219,23 @@ export async function fetchGitHubRepositories( ); const octokit = new Octokit({ auth: tokenData.token }); - const repositories: GitHubRepository[] = []; - let page = 1; - const perPage = 100; - - while (true) { - const { data } = await octokit.apps.listReposAccessibleToInstallation({ - per_page: 100, - page, - }); - - repositories.push( - ...data.repositories - .filter(repo => !repo.archived) - .map(repo => ({ - id: repo.id, - name: repo.name, - full_name: repo.full_name, - private: repo.private, - created_at: repo.created_at ?? new Date().toISOString(), - })) - ); - - if (data.repositories.length < perPage) break; - page++; - } + const repositories = await collectListingPages({ + maxPages: MAX_REPOSITORY_PAGES, + scope: 'repository listing', + fetchPage: async page => + (await octokit.apps.listReposAccessibleToInstallation({ per_page: LISTING_PAGE_SIZE, page })) + .data.repositories, + }); - return repositories; + return repositories + .filter(repository => !repository.archived) + .map(repository => ({ + id: repository.id, + name: repository.name, + full_name: repository.full_name, + private: repository.private, + created_at: repository.created_at ?? new Date().toISOString(), + })); } export async function fetchGitHubRepositoriesForMaintenance( @@ -229,25 +244,23 @@ export async function fetchGitHubRepositoriesForMaintenance( ): Promise { const tokenData = await generateGitHubInstallationTokenForMaintenance(installationId, appType); const octokit = new Octokit({ auth: tokenData.token }); - const repositories: GitHubRepository[] = []; - let page = 1; - while (true) { - const response = await octokit.apps.listReposAccessibleToInstallation({ per_page: 100, page }); - repositories.push( - ...response.data.repositories - .filter(repository => !repository.archived) - .map(repository => ({ - id: repository.id, - name: repository.name, - full_name: repository.full_name, - private: repository.private, - created_at: repository.created_at ?? new Date().toISOString(), - })) - ); - if (response.data.repositories.length < 100) break; - page += 1; - } - return repositories; + const repositories = await collectListingPages({ + maxPages: MAX_REPOSITORY_PAGES, + scope: 'maintenance repository listing', + fetchPage: async page => + (await octokit.apps.listReposAccessibleToInstallation({ per_page: LISTING_PAGE_SIZE, page })) + .data.repositories, + }); + + return repositories + .filter(repository => !repository.archived) + .map(repository => ({ + id: repository.id, + name: repository.name, + full_name: repository.full_name, + private: repository.private, + created_at: repository.created_at ?? new Date().toISOString(), + })); } export async function fetchGitHubBranches( @@ -273,30 +286,17 @@ export async function fetchGitHubBranches( }); const defaultBranch = repoData.default_branch; - const branches: GitHubBranch[] = []; - let page = 1; - const perPage = 100; - - while (true) { - const { data } = await octokit.repos.listBranches({ - owner, - repo, - per_page: perPage, - page, - }); - - branches.push( - ...data.map(branch => ({ - name: branch.name, - isDefault: branch.name === defaultBranch, - })) - ); - - if (data.length < perPage) break; - page++; - } + const branches = await collectListingPages({ + maxPages: MAX_BRANCH_PAGES, + scope: `branch listing for ${repositoryFullName}`, + fetchPage: async page => + (await octokit.repos.listBranches({ owner, repo, per_page: LISTING_PAGE_SIZE, page })).data, + }); - return branches; + return branches.map(branch => ({ + name: branch.name, + isDefault: branch.name === defaultBranch, + })); } export async function fetchGitHubInstallationDetails( diff --git a/apps/web/src/routers/github-pr-review-router.test.ts b/apps/web/src/routers/github-pr-review-router.test.ts index ab04fb3b714..98a57710a3e 100644 --- a/apps/web/src/routers/github-pr-review-router.test.ts +++ b/apps/web/src/routers/github-pr-review-router.test.ts @@ -807,6 +807,116 @@ describe('githubPrReviewRouter infinite-query inputs accept the tRPC direction f }); }); +describe('githubPrReviewRouter.listReviewThreads comment pagination bounds', () => { + const PAGE_SIZE = 50; + + function reviewComment(databaseId: number) { + return { + databaseId, + id: `PRRC_${databaseId}`, + body: `comment ${databaseId}`, + createdAt: '2024-01-01T00:00:00Z', + author: { login: 'octocat', avatarUrl: 'https://x/y.png' }, + reactionGroups: [], + }; + } + + it('caps follow-up pages and total follow-up comments for one request', async () => { + getGitHubUserAccessToken.mockResolvedValueOnce(connected('t1', 'auth_1', 1)); + const caller = createCaller({ user: { id: 'user-1' } as User }); + let followUpRequests = 0; + + buildOctokit('t1').request.mockImplementation( + async (_path: string, body: { query: string; variables?: Record }) => { + if (body.query.includes('PrReviewConversationComments')) { + return { + data: { + data: { + repository: { + pullRequest: { + comments: { nodes: [], pageInfo: { hasNextPage: false, endCursor: null } }, + }, + }, + }, + }, + }; + } + if (body.query.includes('PrReviewThreadComments')) { + followUpRequests += 1; + const threadId = String(body.variables?.threadId ?? 'thread'); + return { + data: { + data: { + node: { + comments: { + // The provider always reports another full page. + pageInfo: { hasNextPage: true, endCursor: `${threadId}-${followUpRequests}` }, + nodes: Array.from({ length: PAGE_SIZE }, (_, index) => + reviewComment(100000 + followUpRequests * PAGE_SIZE + index) + ), + }, + }, + }, + }, + }; + } + return { + data: { + data: { + repository: { + pullRequest: { + reviewThreads: { + pageInfo: { hasNextPage: false, endCursor: null }, + nodes: Array.from({ length: PAGE_SIZE }, (_, threadIndex) => ({ + id: `PRRT_${threadIndex}`, + isResolved: false, + isOutdated: false, + subjectType: 'LINE', + path: `src/file-${threadIndex}.ts`, + line: threadIndex + 1, + startLine: null, + originalLine: null, + originalStartLine: null, + diffSide: 'RIGHT', + comments: { + pageInfo: { hasNextPage: true, endCursor: `initial-${threadIndex}` }, + nodes: Array.from({ length: PAGE_SIZE }, (_, index) => + reviewComment(threadIndex * 1000 + index + 1) + ), + }, + })), + }, + }, + }, + }, + }, + }; + } + ); + + const result = await caller.listReviewThreads({ + owner: 'octocat', + repo: 'hello', + number: 1, + direction: 'forward', + }); + + const threads = result.threads as Array<{ comments: unknown[] }>; + expect(threads).toHaveLength(PAGE_SIZE); + // 500 follow-up comments per request at 50 per page. + expect(followUpRequests).toBe(10); + const followUpComments = threads.reduce( + (total: number, thread) => total + thread.comments.length - PAGE_SIZE, + 0 + ); + expect(followUpComments).toBe(500); + // Every thread keeps its initial page; threads reached after the shared + // budget is spent carry no follow-up comments. + expect(threads.every(thread => thread.comments.length >= PAGE_SIZE)).toBe(true); + expect(threads.some(thread => thread.comments.length === PAGE_SIZE)).toBe(true); + }); +}); + describe('githubPrReviewRouter.listReviewThreads conversation comments', () => { const emptyThreads = { nodes: [] as unknown[], diff --git a/apps/web/src/routers/github-pr-review-router.ts b/apps/web/src/routers/github-pr-review-router.ts index e7d6e9f7958..7aa461f5de8 100644 --- a/apps/web/src/routers/github-pr-review-router.ts +++ b/apps/web/src/routers/github-pr-review-router.ts @@ -1,8 +1,10 @@ import 'server-only'; import * as z from 'zod'; +import pLimit from 'p-limit'; import { createHash } from 'node:crypto'; import { TRPCError } from '@trpc/server'; +import type { Octokit } from '@octokit/rest'; import { and, eq } from 'drizzle-orm'; import { baseProcedure, createTRPCRouter } from '@kilocode/web-shared/lib/trpc/init'; @@ -37,6 +39,10 @@ import { FILES_PAGE_SIZE, INBOX_PAGE_SIZE, REVIEW_THREADS_PAGE_SIZE, + REVIEW_THREAD_COMMENTS_MAX_PAGES, + REVIEW_THREAD_COMMENTS_MAX_PER_REQUEST, + REVIEW_THREAD_COMMENTS_PAGE_SIZE, + REVIEW_THREADS_ENRICHMENT_CONCURRENCY, } from '@/lib/github-pr-review/dtos'; import { throwTrpcFromGraphQlErrors, withGitHubUserTokenRetry } from '@/lib/github-pr-review/retry'; import { classifyGitHubHttpError } from '@/lib/github-pr-review/errors'; @@ -576,7 +582,16 @@ function normalizeReactions(groups: GraphQlReactionGroup[]) { .filter(r => r.count > 0); } -function normalizeComment(node: GraphQlCommentNode) { +type ReviewCommentDto = { + databaseId: number; + id: string; + body: string; + createdAt: string; + author: { login: string; avatarUrl: string } | null; + reactions: Array<{ content: string; count: number; viewerHasReacted: boolean }>; +}; + +function normalizeComment(node: GraphQlCommentNode): ReviewCommentDto { return { databaseId: node.databaseId, id: node.id, @@ -617,22 +632,36 @@ export const PR_REVIEW_GRAPHQL_DOCUMENTS = { export const normalizeReactions_FOR_TEST = normalizeReactions; export const normalizeComment_FOR_TEST = normalizeComment; +// Follow-up comment budget for one review-threads request. `listReviewThreads` +// creates one and hands it to every thread on the page, so the total number of +// extra comments retained by a single call is fixed regardless of how many +// threads the pull request carries. +export type ThreadCommentBudget = { remainingFollowUpComments: number }; + export async function fetchAllThreadComments(args: { octokit: ReturnType; threadId: string; initialConnection: GraphQlCommentConnection; -}): Promise[]> { - const { octokit, threadId, initialConnection } = args; - const collected: ReturnType[] = - initialConnection.nodes.map(normalizeComment); + budget: ThreadCommentBudget; +}): Promise { + const { octokit, threadId, initialConnection, budget } = args; + const collected: ReviewCommentDto[] = initialConnection.nodes.map(normalizeComment); let cursor: string | null = initialConnection.pageInfo.endCursor; let hasNext = initialConnection.pageInfo.hasNextPage; - // Follow the comment cursor until GitHub reports no next page, so DTO - // threads always carry the complete comment list (no silent truncation). - while (hasNext && cursor) { + // Follow the comment cursor page by page, bounded per thread (page count) and + // per request (the shared budget), so a long thread or a pull request full of + // long threads keeps the work and the response bounded. Past either ceiling + // the thread keeps what was collected. + for (let page = 1; page <= REVIEW_THREAD_COMMENTS_MAX_PAGES; page += 1) { + if (!hasNext || !cursor) break; + // Reserve the page's share before requesting it, so concurrent threads + // never fetch a page whose comments the budget can no longer hold. + const reserved = Math.min(REVIEW_THREAD_COMMENTS_PAGE_SIZE, budget.remainingFollowUpComments); + if (reserved <= 0) break; + budget.remainingFollowUpComments -= reserved; const response = (await octokit.request('POST /graphql', { query: REVIEW_THREAD_COMMENTS_FOLLOWUP_QUERY, - variables: { threadId, first: 50, after: cursor }, + variables: { threadId, first: REVIEW_THREAD_COMMENTS_PAGE_SIZE, after: cursor }, })) as { data: { data: { node: { comments: GraphQlCommentConnection } | null } | null; @@ -641,14 +670,80 @@ export async function fetchAllThreadComments(args: { }; throwTrpcFromGraphQlErrors(response.data.errors as never); const node = response.data.data?.node; - if (!node) break; - collected.push(...node.comments.nodes.map(normalizeComment)); + if (!node) { + budget.remainingFollowUpComments += reserved; + break; + } + const nodes = node.comments.nodes.slice(0, reserved); + // A short page frees the unused part of the reservation. + budget.remainingFollowUpComments += reserved - nodes.length; + collected.push(...nodes.map(normalizeComment)); hasNext = node.comments.pageInfo.hasNextPage; cursor = node.comments.pageInfo.endCursor; } return collected; } +export type ReviewThreadEnrichment = { + id: string; + isResolved: boolean; + isOutdated: boolean; + subjectType: GraphQlReviewThreadNode['subjectType']; + path: string | null; + line: number | null; + startLine: number | null; + originalLine: number | null; + originalStartLine: number | null; + diffSide: GraphQlReviewThreadNode['diffSide']; + diffHunk: string | null; + comments: ReviewCommentDto[]; +}; + +/** + * Attach every thread's comments to its node for one review-threads page. + * The threads share one follow-up comment budget, and at most + * `REVIEW_THREADS_ENRICHMENT_CONCURRENCY` GitHub requests are in flight, so a + * pull request with many long threads keeps both provider work and the + * response bounded. A thread reached after the budget is spent keeps its + * initial-page comments. + */ +export async function enrichReviewThreads(args: { + octokit: Octokit; + nodes: GraphQlReviewThreadNode[]; +}): Promise { + const { octokit, nodes } = args; + const budget: ThreadCommentBudget = { + remainingFollowUpComments: REVIEW_THREAD_COMMENTS_MAX_PER_REQUEST, + }; + const limit = pLimit(REVIEW_THREADS_ENRICHMENT_CONCURRENCY); + return Promise.all( + nodes.map(node => + limit(async () => { + const comments = await fetchAllThreadComments({ + octokit, + threadId: node.id, + initialConnection: node.comments, + budget, + }); + return { + id: node.id, + isResolved: node.isResolved, + isOutdated: node.isOutdated, + subjectType: node.subjectType, + path: node.path, + line: node.line, + startLine: node.startLine, + originalLine: node.originalLine, + originalStartLine: node.originalStartLine, + diffSide: node.diffSide, + diffHunk: node.comments.nodes[0]?.diffHunk ?? null, + comments, + }; + }) + ) + ); +} + async function fetchConversationCommentsPage(args: { octokit: ReturnType; owner: string; @@ -1771,29 +1866,7 @@ export const githubPrReviewRouter = createTRPCRouter({ endCursor: null, }); } - const threads = await Promise.all( - connection.nodes.map(async node => { - const comments = await fetchAllThreadComments({ - octokit, - threadId: node.id, - initialConnection: node.comments, - }); - return { - id: node.id, - isResolved: node.isResolved, - isOutdated: node.isOutdated, - subjectType: node.subjectType, - path: node.path, - line: node.line, - startLine: node.startLine, - originalLine: node.originalLine, - originalStartLine: node.originalStartLine, - diffSide: node.diffSide, - diffHunk: node.comments.nodes[0]?.diffHunk ?? null, - comments, - }; - }) - ); + const threads = await enrichReviewThreads({ octokit, nodes: connection.nodes }); return buildReviewThreadsResult({ threads: threads as never, conversation, From c178cceff437d480eff19519cdf8eebd531b5cb1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= <4062236+iscekic@users.noreply.github.com> Date: Tue, 20 Oct 2026 01:49:45 +0000 Subject: [PATCH 2/5] fix(github): split thread comment budget per thread Review-thread follow-up comments are now divided evenly across the page's threads before any request runs, so identical responses give identical results, and each request asks only for the comments it keeps. The listing collector reads GitHub's Link header and warns at its page ceiling only when the provider still reports another page. --- .../review-thread-comments.test.ts | 39 ++++++------ .../github/adapter.pagination.test.ts | 33 +++++++++- .../integrations/platforms/github/adapter.ts | 63 ++++++++++++++----- .../github/review-shared-repositories.test.ts | 1 + .../routers/github-pr-review-router.test.ts | 10 ++- .../src/routers/github-pr-review-router.ts | 37 ++++++----- 6 files changed, 120 insertions(+), 63 deletions(-) diff --git a/apps/web/src/lib/github-pr-review/review-thread-comments.test.ts b/apps/web/src/lib/github-pr-review/review-thread-comments.test.ts index 7c171e4cbda..48b387aff7c 100644 --- a/apps/web/src/lib/github-pr-review/review-thread-comments.test.ts +++ b/apps/web/src/lib/github-pr-review/review-thread-comments.test.ts @@ -261,11 +261,13 @@ describe('thread comment pagination bounds', () => { expect(comments).toHaveLength(300); }); - it('spends one follow-up budget across threads and stops asking once it is empty', async () => { - // One page of 50 plus 10 from the next page, then the budget is spent. + it('asks for only what the allowance keeps and stops when it is spent', async () => { + // One page of 50 plus 10 from the next page, then the allowance is spent. const budget = { remainingFollowUpComments: 60 }; + const requestedFirsts: number[] = []; let nextId = 5000; - const request = jest.fn(async () => { + const request = jest.fn(async (_path: string, options: { variables: { first: number } }) => { + requestedFirsts.push(options.variables.first); const start = nextId; nextId += REVIEW_THREAD_COMMENTS_PAGE_SIZE; return { @@ -277,23 +279,16 @@ describe('thread comment pagination bounds', () => { }; }); - const first = await fetchAllThreadComments({ + const comments = await fetchAllThreadComments({ octokit: { request } as never, threadId: 'thread_1', initialConnection: commentPage(1, 1, true), budget, }); - const requestsAfterFirst = request.mock.calls.length; - const second = await fetchAllThreadComments({ - octokit: { request } as never, - threadId: 'thread_2', - initialConnection: commentPage(2, 1, true), - budget, - }); - expect(request).toHaveBeenCalledTimes(requestsAfterFirst); - expect(first).toHaveLength(61); - expect(second).toHaveLength(1); + // The second request asks only for the 10 comments the allowance still holds. + expect(requestedFirsts).toEqual([50, 10]); + expect(comments).toHaveLength(61); expect(budget.remainingFollowUpComments).toBe(0); }); @@ -320,7 +315,7 @@ describe('thread comment pagination bounds', () => { expect(maxInFlight).toBe(5); }); - it('caps follow-up comments per request while every thread keeps its initial page', async () => { + it('splits the request budget evenly before any request and repeats it identically', async () => { const request = jest.fn(async () => ({ data: { data: { @@ -335,12 +330,16 @@ describe('thread comment pagination bounds', () => { threadNode(index, commentPage(index * 10 + 1, 1, true)) ); - const threads = await enrichReviewThreads({ octokit: { request } as never, nodes }); + const first = await enrichReviewThreads({ octokit: { request } as never, nodes }); + const second = await enrichReviewThreads({ octokit: { request } as never, nodes }); - const collected = threads.reduce((total, thread) => total + thread.comments.length, 0); + const collected = first.reduce((total, thread) => total + thread.comments.length, 0); + // 500 follow-up comments split across 20 threads: 25 each. expect(collected - threadCount).toBe(500); - // Threads reached after the budget is spent still carry their initial page. - expect(threads.some(thread => thread.comments.length === 1)).toBe(true); - expect(threads.every(thread => thread.comments.length >= 1)).toBe(true); + expect(first.every(thread => thread.comments.length === 26)).toBe(true); + // Identical responses produce identical per-thread results every run. + expect(second.map(thread => thread.comments.length)).toEqual( + first.map(thread => thread.comments.length) + ); }); }); diff --git a/apps/web/src/lib/integrations/platforms/github/adapter.pagination.test.ts b/apps/web/src/lib/integrations/platforms/github/adapter.pagination.test.ts index 93e7fe55ee7..557f7f5c818 100644 --- a/apps/web/src/lib/integrations/platforms/github/adapter.pagination.test.ts +++ b/apps/web/src/lib/integrations/platforms/github/adapter.pagination.test.ts @@ -36,13 +36,14 @@ import { fetchGitHubRepositoriesForMaintenance, } from './adapter'; -function branchPage(page: number, count: number) { +function branchPage(page: number, count: number, link?: string) { return { data: Array.from({ length: count }, (_, index) => ({ name: `branch-${page}-${index}` })), + headers: link ? { link } : {}, }; } -function repoPage(page: number, count: number, archivedCount = 0) { +function repoPage(page: number, count: number, archivedCount = 0, link?: string) { return { data: { repositories: Array.from({ length: count }, (_, index) => ({ @@ -54,6 +55,7 @@ function repoPage(page: number, count: number, archivedCount = 0) { created_at: '2024-01-01T00:00:00Z', })), }, + headers: link ? { link } : {}, }; } @@ -92,6 +94,33 @@ describe('fetchGitHubBranches pagination bounds', () => { }); expect(warnExceptInTest).not.toHaveBeenCalled(); }); + + it('warns only when a full page at the ceiling still has a next page', async () => { + mockGetRepo.mockResolvedValue({ data: { default_branch: 'main' } }); + mockListBranches.mockImplementation(async ({ page }: { page: number }) => + branchPage( + page, + 100, + page < 10 + ? `; rel="next"` + : '; rel="first"' + ) + ); + + await fetchGitHubBranches('123456', 'acme/repo'); + expect(mockListBranches).toHaveBeenCalledTimes(10); + // The final page reports no next page, so nothing was dropped. + expect(warnExceptInTest).not.toHaveBeenCalled(); + + jest.clearAllMocks(); + mockGetRepo.mockResolvedValue({ data: { default_branch: 'main' } }); + mockListBranches.mockImplementation(async ({ page }: { page: number }) => + branchPage(page, 100, `; rel="next"`) + ); + + await fetchGitHubBranches('123456', 'acme/repo'); + expect(warnExceptInTest).toHaveBeenCalledTimes(1); + }); }); describe('fetchGitHubRepositories pagination bounds', () => { diff --git a/apps/web/src/lib/integrations/platforms/github/adapter.ts b/apps/web/src/lib/integrations/platforms/github/adapter.ts index e924260d853..3ea602d96a9 100644 --- a/apps/web/src/lib/integrations/platforms/github/adapter.ts +++ b/apps/web/src/lib/integrations/platforms/github/adapter.ts @@ -182,26 +182,36 @@ type GitHubBranch = { const LISTING_PAGE_SIZE = 100; // One listing call performs at most this many provider page requests and keeps -// at most `pages × LISTING_PAGE_SIZE` rows. Past the ceiling the remainder is -// dropped and the call logs a warning, the same bounded-scan convention the -// capped review scans use. +// at most `pages × LISTING_PAGE_SIZE` rows. Reaching the ceiling means more rows +// may exist, in which case the remainder is dropped and the call logs a warning, +// the same bounded-scan convention the capped review scans use. const MAX_REPOSITORY_PAGES = 50; // 5,000 repositories const MAX_BRANCH_PAGES = 10; // 1,000 branches +type ListingPage = { items: T[]; hasNextPage?: boolean }; + +/** GitHub's `Link` header is authoritative about another page when present. */ +function linkHasNextPage(link: string | undefined): boolean | undefined { + return typeof link === 'string' ? link.includes('rel="next"') : undefined; +} + async function collectListingPages(params: { maxPages: number; scope: string; - fetchPage: (page: number) => Promise; + fetchPage: (page: number) => Promise>; }): Promise { const collected: T[] = []; for (let page = 1; page <= params.maxPages; page += 1) { - const items = await params.fetchPage(page); + const { items, hasNextPage } = await params.fetchPage(page); collected.push(...items); if (items.length < LISTING_PAGE_SIZE) return collected; + // A provider that says the full page is the last one ends the walk here. + if (hasNextPage === false) return collected; + if (page < params.maxPages) continue; + warnExceptInTest( + `GitHub ${params.scope} reached the ${params.maxPages}-page ceiling; returning the first ${collected.length} entries` + ); } - warnExceptInTest( - `GitHub ${params.scope} reached the ${params.maxPages}-page ceiling; returning the first ${collected.length} entries` - ); return collected; } @@ -222,9 +232,16 @@ export async function fetchGitHubRepositories( const repositories = await collectListingPages({ maxPages: MAX_REPOSITORY_PAGES, scope: 'repository listing', - fetchPage: async page => - (await octokit.apps.listReposAccessibleToInstallation({ per_page: LISTING_PAGE_SIZE, page })) - .data.repositories, + fetchPage: async page => { + const response = await octokit.apps.listReposAccessibleToInstallation({ + per_page: LISTING_PAGE_SIZE, + page, + }); + return { + items: response.data.repositories, + hasNextPage: linkHasNextPage(response.headers.link), + }; + }, }); return repositories @@ -247,9 +264,16 @@ export async function fetchGitHubRepositoriesForMaintenance( const repositories = await collectListingPages({ maxPages: MAX_REPOSITORY_PAGES, scope: 'maintenance repository listing', - fetchPage: async page => - (await octokit.apps.listReposAccessibleToInstallation({ per_page: LISTING_PAGE_SIZE, page })) - .data.repositories, + fetchPage: async page => { + const response = await octokit.apps.listReposAccessibleToInstallation({ + per_page: LISTING_PAGE_SIZE, + page, + }); + return { + items: response.data.repositories, + hasNextPage: linkHasNextPage(response.headers.link), + }; + }, }); return repositories @@ -289,8 +313,15 @@ export async function fetchGitHubBranches( const branches = await collectListingPages({ maxPages: MAX_BRANCH_PAGES, scope: `branch listing for ${repositoryFullName}`, - fetchPage: async page => - (await octokit.repos.listBranches({ owner, repo, per_page: LISTING_PAGE_SIZE, page })).data, + fetchPage: async page => { + const response = await octokit.repos.listBranches({ + owner, + repo, + per_page: LISTING_PAGE_SIZE, + page, + }); + return { items: response.data, hasNextPage: linkHasNextPage(response.headers.link) }; + }, }); return branches.map(branch => ({ diff --git a/apps/web/src/lib/integrations/platforms/github/review-shared-repositories.test.ts b/apps/web/src/lib/integrations/platforms/github/review-shared-repositories.test.ts index 8ac7f096c40..abe8cb932de 100644 --- a/apps/web/src/lib/integrations/platforms/github/review-shared-repositories.test.ts +++ b/apps/web/src/lib/integrations/platforms/github/review-shared-repositories.test.ts @@ -49,6 +49,7 @@ jest.mock('@octokit/rest', () => ({ { id: 1, name: 'repo', full_name: 'acme/repo', private: true, archived: false }, ], }, + headers: {}, }), }, })), diff --git a/apps/web/src/routers/github-pr-review-router.test.ts b/apps/web/src/routers/github-pr-review-router.test.ts index 98a57710a3e..f61756df349 100644 --- a/apps/web/src/routers/github-pr-review-router.test.ts +++ b/apps/web/src/routers/github-pr-review-router.test.ts @@ -903,17 +903,15 @@ describe('githubPrReviewRouter.listReviewThreads comment pagination bounds', () const threads = result.threads as Array<{ comments: unknown[] }>; expect(threads).toHaveLength(PAGE_SIZE); - // 500 follow-up comments per request at 50 per page. - expect(followUpRequests).toBe(10); + // 500 follow-up comments split across 50 threads: 10 each, one request each. + expect(followUpRequests).toBe(50); const followUpComments = threads.reduce( (total: number, thread) => total + thread.comments.length - PAGE_SIZE, 0 ); expect(followUpComments).toBe(500); - // Every thread keeps its initial page; threads reached after the shared - // budget is spent carry no follow-up comments. - expect(threads.every(thread => thread.comments.length >= PAGE_SIZE)).toBe(true); - expect(threads.some(thread => thread.comments.length === PAGE_SIZE)).toBe(true); + // Every thread keeps its initial page plus its even share. + expect(threads.every(thread => thread.comments.length === PAGE_SIZE + 10)).toBe(true); }); }); diff --git a/apps/web/src/routers/github-pr-review-router.ts b/apps/web/src/routers/github-pr-review-router.ts index 7aa461f5de8..2ecdcdb8973 100644 --- a/apps/web/src/routers/github-pr-review-router.ts +++ b/apps/web/src/routers/github-pr-review-router.ts @@ -632,10 +632,10 @@ export const PR_REVIEW_GRAPHQL_DOCUMENTS = { export const normalizeReactions_FOR_TEST = normalizeReactions; export const normalizeComment_FOR_TEST = normalizeComment; -// Follow-up comment budget for one review-threads request. `listReviewThreads` -// creates one and hands it to every thread on the page, so the total number of -// extra comments retained by a single call is fixed regardless of how many -// threads the pull request carries. +// Follow-up comment allowance for one thread. `enrichReviewThreads` splits the +// request's budget across the page's threads before any request runs, so one +// call's retained follow-up comments stay fixed and each thread's share never +// depends on completion order. export type ThreadCommentBudget = { remainingFollowUpComments: number }; export async function fetchAllThreadComments(args: { @@ -649,19 +649,18 @@ export async function fetchAllThreadComments(args: { let cursor: string | null = initialConnection.pageInfo.endCursor; let hasNext = initialConnection.pageInfo.hasNextPage; // Follow the comment cursor page by page, bounded per thread (page count) and - // per request (the shared budget), so a long thread or a pull request full of - // long threads keeps the work and the response bounded. Past either ceiling - // the thread keeps what was collected. + // by the thread's allowance, so a long thread keeps the work and the response + // bounded. Past either ceiling the thread keeps what was collected. for (let page = 1; page <= REVIEW_THREAD_COMMENTS_MAX_PAGES; page += 1) { if (!hasNext || !cursor) break; - // Reserve the page's share before requesting it, so concurrent threads - // never fetch a page whose comments the budget can no longer hold. + // Reserve the page's share before requesting it and ask only for that many + // comments, so the provider never returns more than the budget keeps. const reserved = Math.min(REVIEW_THREAD_COMMENTS_PAGE_SIZE, budget.remainingFollowUpComments); if (reserved <= 0) break; budget.remainingFollowUpComments -= reserved; const response = (await octokit.request('POST /graphql', { query: REVIEW_THREAD_COMMENTS_FOLLOWUP_QUERY, - variables: { threadId, first: REVIEW_THREAD_COMMENTS_PAGE_SIZE, after: cursor }, + variables: { threadId, first: reserved, after: cursor }, })) as { data: { data: { node: { comments: GraphQlCommentConnection } | null } | null; @@ -701,20 +700,20 @@ export type ReviewThreadEnrichment = { /** * Attach every thread's comments to its node for one review-threads page. - * The threads share one follow-up comment budget, and at most - * `REVIEW_THREADS_ENRICHMENT_CONCURRENCY` GitHub requests are in flight, so a - * pull request with many long threads keeps both provider work and the - * response bounded. A thread reached after the budget is spent keeps its - * initial-page comments. + * The request's follow-up comment budget is split evenly across the threads + * before any request runs, so identical responses produce identical results + * regardless of completion order, and at most + * `REVIEW_THREADS_ENRICHMENT_CONCURRENCY` GitHub requests are in flight. A + * thread keeps its initial-page comments beyond its share. */ export async function enrichReviewThreads(args: { octokit: Octokit; nodes: GraphQlReviewThreadNode[]; }): Promise { const { octokit, nodes } = args; - const budget: ThreadCommentBudget = { - remainingFollowUpComments: REVIEW_THREAD_COMMENTS_MAX_PER_REQUEST, - }; + const perThreadFollowUpComments = nodes.length + ? Math.floor(REVIEW_THREAD_COMMENTS_MAX_PER_REQUEST / nodes.length) + : 0; const limit = pLimit(REVIEW_THREADS_ENRICHMENT_CONCURRENCY); return Promise.all( nodes.map(node => @@ -723,7 +722,7 @@ export async function enrichReviewThreads(args: { octokit, threadId: node.id, initialConnection: node.comments, - budget, + budget: { remainingFollowUpComments: perThreadFollowUpComments }, }); return { id: node.id, From 874b067a04d2e4b656d3e4b7f81a2fc4cabb2ee0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= <4062236+iscekic@users.noreply.github.com> Date: Tue, 20 Oct 2026 01:56:10 +0000 Subject: [PATCH 3/5] chore(github): describe the per-thread review comment split The budget comment now states how the request limits are divided. --- apps/web/src/lib/github-pr-review/dtos.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/apps/web/src/lib/github-pr-review/dtos.ts b/apps/web/src/lib/github-pr-review/dtos.ts index c3104501c75..6406187ac69 100644 --- a/apps/web/src/lib/github-pr-review/dtos.ts +++ b/apps/web/src/lib/github-pr-review/dtos.ts @@ -235,10 +235,10 @@ export const CONVERSATION_COMMENTS_MAX_PAGES = 5; // Past the ceiling the thread keeps what was collected (silent truncation), // matching the conversation-comment scan above. export const REVIEW_THREAD_COMMENTS_MAX_PAGES = 5; -// One review-threads request shares this follow-up comment budget across every -// thread on the page, so a pull request with many long threads cannot multiply -// the retained response. Threads reached after the budget is spent keep their -// initial page only. +// One review-threads request spends at most this many follow-up comments. +// `enrichReviewThreads` divides them evenly across the page's threads before +// any request runs, so the split depends only on the response content and never +// on which thread's requests finish first. export const REVIEW_THREAD_COMMENTS_MAX_PER_REQUEST = 500; // Threads on one page are enriched with at most this many concurrent GitHub // requests. From be63fbfe5712328b5ecba0c50b5f9a4a4a7f594e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= <4062236+iscekic@users.noreply.github.com> Date: Tue, 20 Oct 2026 02:06:17 +0000 Subject: [PATCH 4/5] fix(github): mark review threads whose comment list is partial Each review thread now reports whether the response carries all of its comments, so a client can present a long thread as partial instead of as the whole list. --- apps/web/src/lib/github-pr-review/dtos.ts | 3 +++ apps/web/src/lib/github-pr-review/mappers.ts | 2 ++ .../github-pr-review/review-thread-comments.test.ts | 13 ++++++++----- .../web/src/routers/github-pr-review-router.test.ts | 6 +++++- apps/web/src/routers/github-pr-review-router.ts | 10 +++++++--- 5 files changed, 25 insertions(+), 9 deletions(-) diff --git a/apps/web/src/lib/github-pr-review/dtos.ts b/apps/web/src/lib/github-pr-review/dtos.ts index 6406187ac69..621af64c4f5 100644 --- a/apps/web/src/lib/github-pr-review/dtos.ts +++ b/apps/web/src/lib/github-pr-review/dtos.ts @@ -181,6 +181,9 @@ export const GitHubPrReviewReviewThreadSchema = z diffSide: z.enum(['LEFT', 'RIGHT']).nullable(), diffHunk: z.string().nullable(), comments: z.array(GitHubPrReviewReviewCommentSchema), + // True when the thread has more comments than this response includes, so a + // client can present the list as partial rather than as the whole thread. + commentsTruncated: z.boolean().optional(), }) .strict(); export type GitHubPrReviewReviewThread = z.infer; diff --git a/apps/web/src/lib/github-pr-review/mappers.ts b/apps/web/src/lib/github-pr-review/mappers.ts index 2376f2800fb..ec9d2b07531 100644 --- a/apps/web/src/lib/github-pr-review/mappers.ts +++ b/apps/web/src/lib/github-pr-review/mappers.ts @@ -398,6 +398,7 @@ export type GraphQlReviewThreadInput = { originalStartLine?: number | null; diffSide?: 'LEFT' | 'RIGHT' | null; diffHunk?: string | null; + commentsTruncated?: boolean; comments: GraphQlReviewCommentInput[]; }; @@ -451,6 +452,7 @@ export function buildReviewThreadsResult(args: { originalStartLine: t.originalStartLine ?? null, diffSide, diffHunk: t.diffHunk || null, + commentsTruncated: t.commentsTruncated ?? false, comments: t.comments.map(mapReviewComment), }; }); diff --git a/apps/web/src/lib/github-pr-review/review-thread-comments.test.ts b/apps/web/src/lib/github-pr-review/review-thread-comments.test.ts index 48b387aff7c..3709c356305 100644 --- a/apps/web/src/lib/github-pr-review/review-thread-comments.test.ts +++ b/apps/web/src/lib/github-pr-review/review-thread-comments.test.ts @@ -101,7 +101,7 @@ describe('fetchAllThreadComments', () => { const octokit = { request } as never; - const comments = await fetchAllThreadComments({ + const { comments, truncated } = await fetchAllThreadComments({ octokit, threadId: 'thread_1', initialConnection: { @@ -111,8 +111,9 @@ describe('fetchAllThreadComments', () => { budget: { remainingFollowUpComments: REVIEW_THREAD_COMMENTS_MAX_PER_REQUEST }, }); - // All three pages aggregated to completion — no silent truncation. + // All three pages aggregated to completion. expect(comments.map(c => c.databaseId)).toEqual([1, 2, 3]); + expect(truncated).toBe(false); // Zero-count reactionGroups are dropped; DTO reactions stay empty. expect(comments.map(c => c.reactions)).toEqual([[], [], []]); expect(request).toHaveBeenCalledTimes(2); @@ -150,7 +151,7 @@ describe('fetchAllThreadComments', () => { }); it('keeps only reactionGroups with totalCount > 0 in the DTO shape', async () => { - const comments = await fetchAllThreadComments({ + const { comments } = await fetchAllThreadComments({ octokit: { request: jest.fn() } as never, threadId: 'thread_1', initialConnection: { @@ -248,7 +249,7 @@ describe('thread comment pagination bounds', () => { }; }); - const comments = await fetchAllThreadComments({ + const { comments, truncated } = await fetchAllThreadComments({ octokit: { request } as never, threadId: 'thread_1', initialConnection: commentPage(1, REVIEW_THREAD_COMMENTS_PAGE_SIZE, true), @@ -259,6 +260,7 @@ describe('thread comment pagination bounds', () => { expect(request).toHaveBeenCalledTimes(5); // Initial page plus the 5-follow-up-page ceiling, 50 comments per page. expect(comments).toHaveLength(300); + expect(truncated).toBe(true); }); it('asks for only what the allowance keeps and stops when it is spent', async () => { @@ -279,7 +281,7 @@ describe('thread comment pagination bounds', () => { }; }); - const comments = await fetchAllThreadComments({ + const { comments, truncated } = await fetchAllThreadComments({ octokit: { request } as never, threadId: 'thread_1', initialConnection: commentPage(1, 1, true), @@ -289,6 +291,7 @@ describe('thread comment pagination bounds', () => { // The second request asks only for the 10 comments the allowance still holds. expect(requestedFirsts).toEqual([50, 10]); expect(comments).toHaveLength(61); + expect(truncated).toBe(true); expect(budget.remainingFollowUpComments).toBe(0); }); diff --git a/apps/web/src/routers/github-pr-review-router.test.ts b/apps/web/src/routers/github-pr-review-router.test.ts index f61756df349..43cd0466d7c 100644 --- a/apps/web/src/routers/github-pr-review-router.test.ts +++ b/apps/web/src/routers/github-pr-review-router.test.ts @@ -901,7 +901,10 @@ describe('githubPrReviewRouter.listReviewThreads comment pagination bounds', () direction: 'forward', }); - const threads = result.threads as Array<{ comments: unknown[] }>; + const threads = result.threads as Array<{ + comments: unknown[]; + commentsTruncated?: boolean; + }>; expect(threads).toHaveLength(PAGE_SIZE); // 500 follow-up comments split across 50 threads: 10 each, one request each. expect(followUpRequests).toBe(50); @@ -912,6 +915,7 @@ describe('githubPrReviewRouter.listReviewThreads comment pagination bounds', () expect(followUpComments).toBe(500); // Every thread keeps its initial page plus its even share. expect(threads.every(thread => thread.comments.length === PAGE_SIZE + 10)).toBe(true); + expect(threads.every(thread => thread.commentsTruncated === true)).toBe(true); }); }); diff --git a/apps/web/src/routers/github-pr-review-router.ts b/apps/web/src/routers/github-pr-review-router.ts index 2ecdcdb8973..06702cd61d2 100644 --- a/apps/web/src/routers/github-pr-review-router.ts +++ b/apps/web/src/routers/github-pr-review-router.ts @@ -643,7 +643,7 @@ export async function fetchAllThreadComments(args: { threadId: string; initialConnection: GraphQlCommentConnection; budget: ThreadCommentBudget; -}): Promise { +}): Promise<{ comments: ReviewCommentDto[]; truncated: boolean }> { const { octokit, threadId, initialConnection, budget } = args; const collected: ReviewCommentDto[] = initialConnection.nodes.map(normalizeComment); let cursor: string | null = initialConnection.pageInfo.endCursor; @@ -680,7 +680,9 @@ export async function fetchAllThreadComments(args: { hasNext = node.comments.pageInfo.hasNextPage; cursor = node.comments.pageInfo.endCursor; } - return collected; + // Leaving the loop with another page still available means the thread has + // comments this response does not include. + return { comments: collected, truncated: hasNext && cursor !== null }; } export type ReviewThreadEnrichment = { @@ -695,6 +697,7 @@ export type ReviewThreadEnrichment = { originalStartLine: number | null; diffSide: GraphQlReviewThreadNode['diffSide']; diffHunk: string | null; + commentsTruncated: boolean; comments: ReviewCommentDto[]; }; @@ -718,7 +721,7 @@ export async function enrichReviewThreads(args: { return Promise.all( nodes.map(node => limit(async () => { - const comments = await fetchAllThreadComments({ + const { comments, truncated } = await fetchAllThreadComments({ octokit, threadId: node.id, initialConnection: node.comments, @@ -736,6 +739,7 @@ export async function enrichReviewThreads(args: { originalStartLine: node.originalStartLine, diffSide: node.diffSide, diffHunk: node.comments.nodes[0]?.diffHunk ?? null, + commentsTruncated: truncated, comments, }; }) From 817c3976070180c18f763237b84950d7ce596020 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= <4062236+iscekic@users.noreply.github.com> Date: Wed, 21 Oct 2026 19:37:46 +0000 Subject: [PATCH 5/5] fix(github): bound the inline review comment scan Route the inline review comment listing through the shared bounded page collector, with the same page ceiling the bot review-comment scan already uses, and cover the ceiling with mocked full pages. --- .../github/adapter.pagination.test.ts | 83 +++++++++++++++++++ .../integrations/platforms/github/adapter.ts | 59 ++++++------- 2 files changed, 108 insertions(+), 34 deletions(-) diff --git a/apps/web/src/lib/integrations/platforms/github/adapter.pagination.test.ts b/apps/web/src/lib/integrations/platforms/github/adapter.pagination.test.ts index 557f7f5c818..efdda044d9f 100644 --- a/apps/web/src/lib/integrations/platforms/github/adapter.pagination.test.ts +++ b/apps/web/src/lib/integrations/platforms/github/adapter.pagination.test.ts @@ -5,11 +5,13 @@ const mockListBranches = jest.fn(); const mockListReposAccessibleToInstallation = jest.fn(); const mockGetRepo = jest.fn(); +const mockListReviewComments = jest.fn(); jest.mock('@octokit/rest', () => ({ Octokit: jest.fn().mockImplementation(() => ({ apps: { listReposAccessibleToInstallation: mockListReposAccessibleToInstallation }, repos: { get: mockGetRepo, listBranches: mockListBranches }, + pulls: { listReviewComments: mockListReviewComments }, })), })); jest.mock('@octokit/auth-app', () => ({ @@ -34,6 +36,7 @@ import { fetchGitHubBranches, fetchGitHubRepositories, fetchGitHubRepositoriesForMaintenance, + fetchPRInlineComments, } from './adapter'; function branchPage(page: number, count: number, link?: string) { @@ -161,3 +164,83 @@ describe('fetchGitHubRepositories pagination bounds', () => { expect(warnExceptInTest).toHaveBeenCalledTimes(1); }); }); + +function commentPage(page: number, count: number, link?: string) { + return { + data: Array.from({ length: count }, (_, index) => ({ + id: page * 1000 + index, + path: `src/file-${index}.ts`, + line: index % 2 === 0 ? index + 1 : null, + body: `comment-${page}-${index}`, + position: index % 2 === 0 ? 1 : null, + user: { login: 'reviewer' }, + })), + headers: link ? { link } : {}, + }; +} + +describe('fetchPRInlineComments pagination bounds', () => { + it('stops at a fixed page count when the provider keeps returning full pages', async () => { + mockListReviewComments.mockImplementation(async ({ page }: { page: number }) => + commentPage(page, 100) + ); + + const comments = await fetchPRInlineComments('123456', 'acme', 'repo', 7); + + expect(mockListReviewComments).toHaveBeenCalledTimes(5); + expect(comments).toHaveLength(500); + expect(comments[0]).toEqual({ + id: 1000, + path: 'src/file-0.ts', + line: 1, + body: 'comment-1-0', + isOutdated: false, + user: { login: 'reviewer' }, + }); + expect(comments[1]).toEqual({ + id: 1001, + path: 'src/file-1.ts', + line: null, + body: 'comment-1-1', + isOutdated: true, + user: { login: 'reviewer' }, + }); + expect(warnExceptInTest).toHaveBeenCalledTimes(1); + }); + + it('stops on a short page without reaching the ceiling', async () => { + mockListReviewComments + .mockImplementationOnce(async () => commentPage(1, 100)) + .mockImplementationOnce(async () => commentPage(2, 4)); + + const comments = await fetchPRInlineComments('123456', 'acme', 'repo', 7); + + expect(mockListReviewComments).toHaveBeenCalledTimes(2); + expect(comments).toHaveLength(104); + expect(warnExceptInTest).not.toHaveBeenCalled(); + }); + + it('warns only when a full page at the ceiling still has a next page', async () => { + mockListReviewComments.mockImplementation(async ({ page }: { page: number }) => + commentPage( + page, + 100, + page < 5 + ? `; rel="next"` + : '; rel="first"' + ) + ); + + await fetchPRInlineComments('123456', 'acme', 'repo', 7); + expect(mockListReviewComments).toHaveBeenCalledTimes(5); + expect(warnExceptInTest).not.toHaveBeenCalled(); + + jest.clearAllMocks(); + mockListReviewComments.mockImplementation(async ({ page }: { page: number }) => + commentPage(page, 100, `; rel="next"`) + ); + + await fetchPRInlineComments('123456', 'acme', 'repo', 7); + expect(warnExceptInTest).toHaveBeenCalledTimes(1); + }); +}); diff --git a/apps/web/src/lib/integrations/platforms/github/adapter.ts b/apps/web/src/lib/integrations/platforms/github/adapter.ts index 3ea602d96a9..91b80ea446f 100644 --- a/apps/web/src/lib/integrations/platforms/github/adapter.ts +++ b/apps/web/src/lib/integrations/platforms/github/adapter.ts @@ -187,6 +187,9 @@ const LISTING_PAGE_SIZE = 100; // the same bounded-scan convention the capped review scans use. const MAX_REPOSITORY_PAGES = 50; // 5,000 repositories const MAX_BRANCH_PAGES = 10; // 1,000 branches +// Same ceiling as the bot's review-comment scan, so the two reads over the same +// provider endpoint walk the same number of pages at most. +const MAX_INLINE_COMMENT_PAGES = 5; // 500 inline review comments type ListingPage = { items: T[]; hasNextPage?: boolean }; @@ -723,41 +726,29 @@ export async function fetchPRInlineComments( const tokenData = await generateGitHubInstallationToken(installationId, appType); const octokit = new Octokit({ auth: tokenData.token }); - const comments: Array<{ - id: number; - path: string; - line: number | null; - body: string; - isOutdated: boolean; - user: { login: string }; - }> = []; - - let page = 1; - const perPage = 100; - - while (true) { - const { data } = await octokit.pulls.listReviewComments({ - owner, - repo, - pull_number: prNumber, - per_page: perPage, - page, - }); - - comments.push( - ...data.map(c => ({ - id: c.id, - path: c.path, - line: c.line ?? null, - body: c.body, - isOutdated: c.position === null, // null position = outdated - user: { login: c.user?.login ?? 'unknown' }, - })) - ); + const pages = await collectListingPages({ + maxPages: MAX_INLINE_COMMENT_PAGES, + scope: `inline review comment listing for ${owner}/${repo}#${prNumber}`, + fetchPage: async page => { + const response = await octokit.pulls.listReviewComments({ + owner, + repo, + pull_number: prNumber, + per_page: LISTING_PAGE_SIZE, + page, + }); + return { items: response.data, hasNextPage: linkHasNextPage(response.headers.link) }; + }, + }); - if (data.length < perPage) break; - page++; - } + const comments = pages.map(c => ({ + id: c.id, + path: c.path, + line: c.line ?? null, + body: c.body, + isOutdated: c.position === null, // null position = outdated + user: { login: c.user?.login ?? 'unknown' }, + })); logExceptInTest('[fetchPRInlineComments] Fetched comments', { owner,