From 25fd4cad8bc20a364b141421d8ea279de915d247 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 16 Feb 2026 03:27:10 +0000 Subject: [PATCH 1/2] fix: resolve PR loading hang with pagination and concurrency control MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GetDifferences and GetCommentsForPullRequest APIs were not handling pagination (NextToken), silently truncating results for large PRs. Blob fetches fired all at once (N files × 2) causing API throttling. - Add pagination loop for GetDifferences in both PR detail and commit diff - Add pagination loop for GetCommentsForPullRequest - Limit concurrent blob fetches to 5 via worker pool pattern - Extract shared fetchBlobTexts helper used by both PR and commit views https://claude.ai/code/session_01MyhBKncxxkLaLfCRxge4yC --- src/app.tsx | 73 +++++++++++----------- src/services/codecommit.test.ts | 92 ++++++++++++++++++++++++++++ src/services/codecommit.ts | 103 +++++++++++++++++++------------- 3 files changed, 188 insertions(+), 80 deletions(-) diff --git a/src/app.tsx b/src/app.tsx index 5c6dd20..e45a5da 100644 --- a/src/app.tsx +++ b/src/app.tsx @@ -41,6 +41,38 @@ import { updateComment, } from "./services/codecommit.js"; +const BLOB_FETCH_CONCURRENCY = 5; + +async function fetchBlobTexts( + client: CodeCommitClient, + repositoryName: string, + differences: Difference[], +): Promise> { + const texts = new Map(); + let index = 0; + + async function worker() { + while (index < differences.length) { + const i = index++; + const diff = differences[i]!; + const beforeBlobId = diff.beforeBlob?.blobId; + const afterBlobId = diff.afterBlob?.blobId; + const key = `${beforeBlobId ?? ""}:${afterBlobId ?? ""}`; + + const [before, after] = await Promise.all([ + beforeBlobId ? getBlobContent(client, repositoryName, beforeBlobId) : Promise.resolve(""), + afterBlobId ? getBlobContent(client, repositoryName, afterBlobId) : Promise.resolve(""), + ]); + + texts.set(key, { before, after }); + } + } + + const workerCount = Math.min(BLOB_FETCH_CONCURRENCY, differences.length); + await Promise.all(Array.from({ length: workerCount }, () => worker())); + return texts; +} + type Screen = "repos" | "prs" | "detail"; interface PaginationState { @@ -231,27 +263,7 @@ export function App({ client, initialRepo }: AppProps) { const reactionsPromise = reloadReactions(detail.commentThreads); - const blobPromise = (async () => { - const blobFetches = detail.differences.map(async (diff) => { - const beforeBlobId = diff.beforeBlob?.blobId; - const afterBlobId = diff.afterBlob?.blobId; - const key = `${beforeBlobId ?? ""}:${afterBlobId ?? ""}`; - - const [before, after] = await Promise.all([ - beforeBlobId ? getBlobContent(client, selectedRepo, beforeBlobId) : Promise.resolve(""), - afterBlobId ? getBlobContent(client, selectedRepo, afterBlobId) : Promise.resolve(""), - ]); - - return { key, before, after }; - }); - - const blobResults = await Promise.all(blobFetches); - const texts = new Map(); - for (const result of blobResults) { - texts.set(result.key, { before: result.before, after: result.after }); - } - return texts; - })(); + const blobPromise = fetchBlobTexts(client, selectedRepo, detail.differences); const commitsPromise = sourceCommit && mergeBase @@ -511,24 +523,7 @@ export function App({ client, initialRepo }: AppProps) { const diffs = await getCommitDifferences(client, selectedRepo, parentId, commit.commitId); setCommitDifferences(diffs); - const blobFetches = diffs.map(async (diff) => { - const beforeBlobId = diff.beforeBlob?.blobId; - const afterBlobId = diff.afterBlob?.blobId; - const key = `${beforeBlobId ?? ""}:${afterBlobId ?? ""}`; - - const [before, after] = await Promise.all([ - beforeBlobId ? getBlobContent(client, selectedRepo, beforeBlobId) : Promise.resolve(""), - afterBlobId ? getBlobContent(client, selectedRepo, afterBlobId) : Promise.resolve(""), - ]); - - return { key, before, after }; - }); - - const blobResults = await Promise.all(blobFetches); - const texts = new Map(); - for (const result of blobResults) { - texts.set(result.key, { before: result.before, after: result.after }); - } + const texts = await fetchBlobTexts(client, selectedRepo, diffs); setCommitDiffTexts(texts); } finally { setIsLoadingCommitDiff(false); diff --git a/src/services/codecommit.test.ts b/src/services/codecommit.test.ts index 5f77c0d..fc19531 100644 --- a/src/services/codecommit.test.ts +++ b/src/services/codecommit.test.ts @@ -250,6 +250,50 @@ describe("getPullRequestDetail", () => { }), ); }); + + it("paginates GetDifferences when NextToken is returned", async () => { + mockSend.mockResolvedValueOnce({ + pullRequest: { + pullRequestId: "42", + title: "big PR", + pullRequestTargets: [ + { + sourceCommit: "abc123", + destinationCommit: "def456", + }, + ], + }, + }); + // Promise.all runs getAllDifferences and getComments concurrently. + // Mock order matches actual call order: + // 1. GetDifferencesCommand (page 1) - from getAllDifferences + // 2. GetCommentsForPullRequestCommand - from getComments (concurrent) + // 3. GetDifferencesCommand (page 2) - from getAllDifferences loop + mockSend.mockResolvedValueOnce({ + differences: [ + { + beforeBlob: { blobId: "b1", path: "file1.ts" }, + afterBlob: { blobId: "b2", path: "file1.ts" }, + }, + ], + NextToken: "page2-token", + }); + mockSend.mockResolvedValueOnce({ commentsForPullRequestData: [] }); + mockSend.mockResolvedValueOnce({ + differences: [ + { + beforeBlob: { blobId: "b3", path: "file2.ts" }, + afterBlob: { blobId: "b4", path: "file2.ts" }, + }, + ], + NextToken: undefined, + }); + + const detail = await getPullRequestDetail(mockClient, "42", "my-service"); + expect(detail.differences).toHaveLength(2); + expect(detail.differences[0].beforeBlob?.path).toBe("file1.ts"); + expect(detail.differences[1].beforeBlob?.path).toBe("file2.ts"); + }); }); describe("listPullRequests edge cases", () => { @@ -802,6 +846,33 @@ describe("getComments", () => { expect(threads[0].comments[2].commentId).toBe("reply-3"); expect(threads[0].comments[3].commentId).toBe("reply-2"); }); + + it("paginates when nextToken is returned", async () => { + mockSend.mockResolvedValueOnce({ + commentsForPullRequestData: [ + { + comments: [{ commentId: "c1", content: "Page 1 comment" }], + }, + ], + nextToken: "page2-token", + }); + mockSend.mockResolvedValueOnce({ + commentsForPullRequestData: [ + { + comments: [{ commentId: "c2", content: "Page 2 comment" }], + }, + ], + nextToken: undefined, + }); + + const threads = await getComments(mockClient, "42"); + expect(threads).toHaveLength(2); + expect(threads[0].comments[0].commentId).toBe("c1"); + expect(threads[1].comments[0].commentId).toBe("c2"); + expect(mockSend).toHaveBeenCalledTimes(2); + // Second call should include nextToken + expect(mockSend.mock.calls[1][0].input.nextToken).toBe("page2-token"); + }); }); describe("postCommentReply", () => { @@ -1492,6 +1563,27 @@ describe("getCommitDifferences", () => { afterCommitSpecifier: "commitDEF", }); }); + + it("paginates when NextToken is returned", async () => { + mockSend.mockResolvedValueOnce({ + differences: [ + { beforeBlob: { blobId: "b1", path: "a.ts" }, afterBlob: { blobId: "b2", path: "a.ts" } }, + ], + NextToken: "token-2", + }); + mockSend.mockResolvedValueOnce({ + differences: [ + { beforeBlob: { blobId: "b3", path: "b.ts" }, afterBlob: { blobId: "b4", path: "b.ts" } }, + ], + NextToken: undefined, + }); + + const result = await getCommitDifferences(mockClient, "my-service", "parent1", "commit1"); + expect(result).toHaveLength(2); + expect(result[0].beforeBlob?.path).toBe("a.ts"); + expect(result[1].beforeBlob?.path).toBe("b.ts"); + expect(mockSend).toHaveBeenCalledTimes(2); + }); }); describe("updateComment", () => { diff --git a/src/services/codecommit.ts b/src/services/codecommit.ts index 0eeb360..b1f2ecf 100644 --- a/src/services/codecommit.ts +++ b/src/services/codecommit.ts @@ -136,15 +136,7 @@ export async function getPullRequestDetail( const [diffResult, commentThreads] = await Promise.all([ hasCommits - ? client - .send( - new GetDifferencesCommand({ - repositoryName, - beforeCommitSpecifier: target!.destinationCommit!, - afterCommitSpecifier: target!.sourceCommit!, - }), - ) - .then((r) => r.differences ?? []) + ? getAllDifferences(client, repositoryName, target!.destinationCommit!, target!.sourceCommit!) : Promise.resolve([]), getComments(client, pullRequestId, { repositoryName, @@ -184,31 +176,41 @@ export async function getComments( }, ): Promise { const commentThreads: CommentThread[] = []; - const commentsCommand = new GetCommentsForPullRequestCommand({ - pullRequestId, - ...(params?.repositoryName ? { repositoryName: params.repositoryName } : {}), - ...(params?.afterCommitId && params?.beforeCommitId - ? { - afterCommitId: params.afterCommitId, - beforeCommitId: params.beforeCommitId, - } - : {}), - }); - const commentsResponse = await client.send(commentsCommand); - for (const thread of commentsResponse.commentsForPullRequestData ?? []) { - const location = thread.location?.filePath - ? { - filePath: thread.location.filePath, - filePosition: thread.location.filePosition ?? 0, - relativeFileVersion: - (thread.location.relativeFileVersion as "BEFORE" | "AFTER") ?? "AFTER", - } - : null; - commentThreads.push({ - location, - comments: sortCommentsRootFirst(thread.comments ?? []), - }); - } + let nextToken: string | undefined; + + do { + const commentsResponse = await client.send( + new GetCommentsForPullRequestCommand({ + pullRequestId, + ...(params?.repositoryName ? { repositoryName: params.repositoryName } : {}), + ...(params?.afterCommitId && params?.beforeCommitId + ? { + afterCommitId: params.afterCommitId, + beforeCommitId: params.beforeCommitId, + } + : {}), + ...(nextToken ? { nextToken } : {}), + }), + ); + + for (const thread of commentsResponse.commentsForPullRequestData ?? []) { + const location = thread.location?.filePath + ? { + filePath: thread.location.filePath, + filePosition: thread.location.filePosition ?? 0, + relativeFileVersion: + (thread.location.relativeFileVersion as "BEFORE" | "AFTER") ?? "AFTER", + } + : null; + commentThreads.push({ + location, + comments: sortCommentsRootFirst(thread.comments ?? []), + }); + } + + nextToken = commentsResponse.nextToken; + } while (nextToken); + return commentThreads; } @@ -468,19 +470,38 @@ export async function getCommitsForPR( return commits.reverse(); } +async function getAllDifferences( + client: CodeCommitClient, + repositoryName: string, + beforeCommitSpecifier: string, + afterCommitSpecifier: string, +): Promise { + const differences: Difference[] = []; + let nextToken: string | undefined; + + do { + const response = await client.send( + new GetDifferencesCommand({ + repositoryName, + beforeCommitSpecifier, + afterCommitSpecifier, + ...(nextToken ? { NextToken: nextToken } : {}), + }), + ); + differences.push(...(response.differences ?? [])); + nextToken = response.NextToken; + } while (nextToken); + + return differences; +} + export async function getCommitDifferences( client: CodeCommitClient, repositoryName: string, beforeCommitId: string, afterCommitId: string, ): Promise { - const command = new GetDifferencesCommand({ - repositoryName, - beforeCommitSpecifier: beforeCommitId, - afterCommitSpecifier: afterCommitId, - }); - const response = await client.send(command); - return response.differences ?? []; + return getAllDifferences(client, repositoryName, beforeCommitId, afterCommitId); } export async function updateComment( From 4d150e4bf84bfe90cca691fca715b2d75e2ef0a8 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 16 Feb 2026 03:40:35 +0000 Subject: [PATCH 2/2] fix: rewrite getCommitsForPR with BFS and throttle reaction fetches getCommitsForPR previously walked only parentIds[0] sequentially. For PRs with merge commits, this followed the wrong branch and made up to 100 sequential API calls before hitting MAX_COMMITS. Rewrite to BFS: each level fetches commits in parallel, all parents are visited, and mergeBase is pre-marked as visited to stop correctly. Also add concurrency control (5 workers) to getReactionsForComments, which was firing all reaction fetches in parallel with no limit. https://claude.ai/code/session_01MyhBKncxxkLaLfCRxge4yC --- src/services/codecommit.test.ts | 41 +++++++++++++++++++++ src/services/codecommit.ts | 63 ++++++++++++++++++++++----------- 2 files changed, 84 insertions(+), 20 deletions(-) diff --git a/src/services/codecommit.test.ts b/src/services/codecommit.test.ts index fc19531..533eb10 100644 --- a/src/services/codecommit.test.ts +++ b/src/services/codecommit.test.ts @@ -1526,6 +1526,47 @@ describe("getCommitsForPR", () => { const result = await getCommitsForPR(mockClient, "my-service", "commit-0", "unreachable"); expect(result).toHaveLength(100); }); + + it("handles merge commits by visiting all parents via BFS", async () => { + // Graph: base -- A -- M (sourceCommit, merge commit) + // / + // B -- + // M.parents = [A, B], A.parents = [base], B.parents = [base] + mockSend.mockResolvedValueOnce({ + commit: { + commitId: "M", + parents: ["A", "B"], + message: "Merge commit", + author: { name: "watany", date: "1707868803" }, + }, + }); + // BFS level 2: A and B fetched in parallel + mockSend.mockResolvedValueOnce({ + commit: { + commitId: "A", + parents: ["base"], + message: "Commit A", + author: { name: "watany", date: "1707868801" }, + }, + }); + mockSend.mockResolvedValueOnce({ + commit: { + commitId: "B", + parents: ["base"], + message: "Commit B", + author: { name: "watany", date: "1707868802" }, + }, + }); + + const result = await getCommitsForPR(mockClient, "my-service", "M", "base"); + expect(result).toHaveLength(3); + const ids = result.map((c) => c.commitId); + expect(ids).toContain("M"); + expect(ids).toContain("A"); + expect(ids).toContain("B"); + // Only 3 API calls (M, A, B) - base is never fetched + expect(mockSend).toHaveBeenCalledTimes(3); + }); }); describe("getCommitDifferences", () => { diff --git a/src/services/codecommit.ts b/src/services/codecommit.ts index b1f2ecf..e8b5872 100644 --- a/src/services/codecommit.ts +++ b/src/services/codecommit.ts @@ -457,14 +457,33 @@ export async function getCommitsForPR( sourceCommit: string, mergeBase: string, ): Promise { + if (sourceCommit === mergeBase) return []; + const commits: CommitInfo[] = []; - let currentId = sourceCommit; + const visited = new Set([mergeBase]); + let currentBatch = [sourceCommit]; + + while (currentBatch.length > 0 && commits.length < MAX_COMMITS) { + const toFetch = currentBatch.filter((id) => !visited.has(id)); + if (toFetch.length === 0) break; - while (currentId !== mergeBase && commits.length < MAX_COMMITS) { - const commit = await getCommit(client, repositoryName, currentId); - commits.push(commit); - if (commit.parentIds.length === 0) break; - currentId = commit.parentIds[0]!; + const limited = toFetch.slice(0, MAX_COMMITS - commits.length); + for (const id of limited) visited.add(id); + + const batchResults = await Promise.all( + limited.map((id) => getCommit(client, repositoryName, id)), + ); + commits.push(...batchResults); + + const nextBatch: string[] = []; + for (const commit of batchResults) { + for (const parentId of commit.parentIds) { + if (!visited.has(parentId)) { + nextBatch.push(parentId); + } + } + } + currentBatch = nextBatch; } return commits.reverse(); @@ -574,27 +593,31 @@ export async function getReactionsForComment( })); } +const REACTION_FETCH_CONCURRENCY = 5; + export async function getReactionsForComments( client: CodeCommitClient, commentIds: string[], ): Promise { const results: ReactionsByComment = new Map(); - - const fetches = commentIds.map(async (commentId) => { - try { - const reactions = await getReactionsForComment(client, commentId); - return { commentId, reactions }; - } catch { - return { commentId, reactions: [] }; - } - }); - - const settled = await Promise.all(fetches); - for (const { commentId, reactions } of settled) { - if (reactions.length > 0) { - results.set(commentId, reactions); + let index = 0; + + async function worker() { + while (index < commentIds.length) { + const i = index++; + const commentId = commentIds[i]!; + try { + const reactions = await getReactionsForComment(client, commentId); + if (reactions.length > 0) { + results.set(commentId, reactions); + } + } catch { + // skip failed reactions + } } } + const workerCount = Math.min(REACTION_FETCH_CONCURRENCY, commentIds.length); + await Promise.all(Array.from({ length: workerCount }, () => worker())); return results; }