diff --git a/apps/web/src/lib/github-pr-review/dtos.ts b/apps/web/src/lib/github-pr-review/dtos.ts index 1b0318f4bcf..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; @@ -230,3 +233,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 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. +export const REVIEW_THREADS_ENRICHMENT_CONCURRENCY = 5; 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 a85bb251e14..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 @@ -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, @@ -96,17 +101,19 @@ describe('fetchAllThreadComments', () => { const octokit = { request } as never; - const comments = await fetchAllThreadComments({ + const { comments, truncated } = await fetchAllThreadComments({ octokit, threadId: 'thread_1', initialConnection: { 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. + // 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); @@ -144,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: { @@ -194,6 +201,7 @@ describe('fetchAllThreadComments', () => { ]), ], }, + budget: { remainingFollowUpComments: REVIEW_THREAD_COMMENTS_MAX_PER_REQUEST }, }); expect(comments[0]?.reactions).toEqual([ @@ -202,3 +210,139 @@ 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, truncated } = 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); + expect(truncated).toBe(true); + }); + + 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 (_path: string, options: { variables: { first: number } }) => { + requestedFirsts.push(options.variables.first); + const start = nextId; + nextId += REVIEW_THREAD_COMMENTS_PAGE_SIZE; + return { + data: { + data: { + node: { comments: commentPage(start, REVIEW_THREAD_COMMENTS_PAGE_SIZE, true) }, + }, + }, + }; + }); + + const { comments, truncated } = await fetchAllThreadComments({ + octokit: { request } as never, + threadId: 'thread_1', + initialConnection: commentPage(1, 1, true), + budget, + }); + + // 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); + }); + + 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('splits the request budget evenly before any request and repeats it identically', 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 first = await enrichReviewThreads({ octokit: { request } as never, nodes }); + const second = await enrichReviewThreads({ octokit: { request } as never, nodes }); + + 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); + 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 new file mode 100644 index 00000000000..efdda044d9f --- /dev/null +++ b/apps/web/src/lib/integrations/platforms/github/adapter.pagination.test.ts @@ -0,0 +1,246 @@ +/** + * @jest-environment node + */ + +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', () => ({ + 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, + fetchPRInlineComments, +} from './adapter'; + +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, link?: string) { + 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', + })), + }, + headers: link ? { link } : {}, + }; +} + +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(); + }); + + 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', () => { + 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); + }); +}); + +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 c6f5ed1d387..91b80ea446f 100644 --- a/apps/web/src/lib/integrations/platforms/github/adapter.ts +++ b/apps/web/src/lib/integrations/platforms/github/adapter.ts @@ -180,6 +180,44 @@ 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. 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 +// 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 }; + +/** 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>; +}): Promise { + const collected: T[] = []; + for (let page = 1; page <= params.maxPages; page += 1) { + 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` + ); + } + return collected; +} + export async function fetchGitHubRepositories( installationId: string, appType: GitHubAppType = 'standard', @@ -194,33 +232,30 @@ 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 => { + const response = await octokit.apps.listReposAccessibleToInstallation({ + per_page: LISTING_PAGE_SIZE, + page, + }); + return { + items: response.data.repositories, + hasNextPage: linkHasNextPage(response.headers.link), + }; + }, + }); - 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 +264,30 @@ 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 => { + const response = await octokit.apps.listReposAccessibleToInstallation({ + per_page: LISTING_PAGE_SIZE, + page, + }); + return { + items: response.data.repositories, + hasNextPage: linkHasNextPage(response.headers.link), + }; + }, + }); + + 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 +313,24 @@ 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 => { + 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; + return branches.map(branch => ({ + name: branch.name, + isDefault: branch.name === defaultBranch, + })); } export async function fetchGitHubInstallationDetails( @@ -692,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, 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 ab04fb3b714..43cd0466d7c 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,118 @@ 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[]; + commentsTruncated?: boolean; + }>; + expect(threads).toHaveLength(PAGE_SIZE); + // 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 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); + }); +}); + 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..06702cd61d2 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,35 @@ export const PR_REVIEW_GRAPHQL_DOCUMENTS = { export const normalizeReactions_FOR_TEST = normalizeReactions; export const normalizeComment_FOR_TEST = normalizeComment; +// 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: { octokit: ReturnType; threadId: string; initialConnection: GraphQlCommentConnection; -}): Promise[]> { - const { octokit, threadId, initialConnection } = args; - const collected: ReturnType[] = - initialConnection.nodes.map(normalizeComment); + budget: ThreadCommentBudget; +}): 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; 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 + // 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 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: 50, after: cursor }, + variables: { threadId, first: reserved, after: cursor }, })) as { data: { data: { node: { comments: GraphQlCommentConnection } | null } | null; @@ -641,12 +669,82 @@ 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; + // 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 = { + 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; + commentsTruncated: boolean; + comments: ReviewCommentDto[]; +}; + +/** + * Attach every thread's comments to its node for one review-threads page. + * 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 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 => + limit(async () => { + const { comments, truncated } = await fetchAllThreadComments({ + octokit, + threadId: node.id, + initialConnection: node.comments, + budget: { remainingFollowUpComments: perThreadFollowUpComments }, + }); + 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, + commentsTruncated: truncated, + comments, + }; + }) + ) + ); } async function fetchConversationCommentsPage(args: { @@ -1771,29 +1869,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,