From 478409b201b271798aea6c063f47a73996af03f5 Mon Sep 17 00:00:00 2001 From: shekhardhangar Date: Mon, 25 May 2026 01:27:29 +0530 Subject: [PATCH] fix(code-review): support resolving GitHub PR review threads --- apps/code/src/main/services/git/schemas.ts | 24 ++- .../src/main/services/git/service.test.ts | 204 ++++++++++++++++++ apps/code/src/main/services/git/service.ts | 201 +++++++++++++++-- apps/code/src/main/trpc/routers/git.ts | 16 ++ .../components/PrCommentThread.tsx | 50 ++++- .../code-review/hooks/usePrCommentActions.ts | 32 ++- .../renderer/features/code-review/types.ts | 2 + .../code-review/utils/prCommentAnnotations.ts | 4 + .../git-interaction/hooks/usePrDetails.ts | 38 ++-- 9 files changed, 528 insertions(+), 43 deletions(-) diff --git a/apps/code/src/main/services/git/schemas.ts b/apps/code/src/main/services/git/schemas.ts index 0398400f4..936a3abb5 100644 --- a/apps/code/src/main/services/git/schemas.ts +++ b/apps/code/src/main/services/git/schemas.ts @@ -400,10 +400,32 @@ export const prReviewCommentSchema = z.object({ export type PrReviewComment = z.infer; +export const prReviewThreadSchema = z.object({ + nodeId: z.string(), + isResolved: z.boolean(), + rootId: z.number(), + filePath: z.string(), + comments: z.array(prReviewCommentSchema), +}); +export type PrReviewThread = z.infer; + export const getPrReviewCommentsInput = z.object({ prUrl: z.string(), }); -export const getPrReviewCommentsOutput = z.array(prReviewCommentSchema); +export const getPrReviewCommentsOutput = z.array(prReviewThreadSchema); + +// resolveReviewThread schemas +export const resolveReviewThreadInput = z.object({ + prUrl: z.string(), + threadNodeId: z.string(), +}); +export const resolveReviewThreadOutput = z.object({ + success: z.boolean(), + isResolved: z.boolean(), +}); +export type ResolveReviewThreadOutput = z.infer< + typeof resolveReviewThreadOutput +>; // replyToPrComment schemas export const replyToPrCommentInput = z.object({ diff --git a/apps/code/src/main/services/git/service.test.ts b/apps/code/src/main/services/git/service.test.ts index 76bdf4959..2436bb45d 100644 --- a/apps/code/src/main/services/git/service.test.ts +++ b/apps/code/src/main/services/git/service.test.ts @@ -319,3 +319,207 @@ describe("mapPrState", () => { expect(mapPrState("something", false, false)).toBeNull(); }); }); + +describe("GitService.getPrReviewComments", () => { + let service: GitService; + + beforeEach(() => { + vi.clearAllMocks(); + service = new GitService( + {} as LlmGatewayService, + {} as WorkspaceService, + { getSessionEnvForTask: async () => ({}) } as unknown as AgentService, + ); + }); + + const makeThread = (id: string, commentId: number) => ({ + id, + isResolved: false, + isOutdated: false, + path: "src/foo.ts", + diffSide: "RIGHT", + line: 10, + originalLine: 10, + startLine: null, + startDiffSide: null, + subjectType: "LINE", + comments: { + nodes: [ + { + databaseId: commentId, + body: "looks good", + path: "src/foo.ts", + diffHunk: "@@ -1,3 +1,4 @@", + replyTo: null, + author: { + login: "alice", + avatarUrl: "https://example.com/alice.png", + }, + createdAt: "2024-01-01T00:00:00Z", + updatedAt: "2024-01-01T00:00:00Z", + }, + ], + }, + }); + + const makePage = ( + threads: object[], + hasNextPage: boolean, + endCursor: string | null, + ) => ({ + exitCode: 0, + stdout: JSON.stringify({ + data: { + repository: { + pullRequest: { + reviewThreads: { + pageInfo: { hasNextPage, endCursor }, + nodes: threads, + }, + }, + }, + }, + }), + }); + + it("returns empty array for non-PR URL", async () => { + const result = await service.getPrReviewComments( + "https://github.com/owner/repo", + ); + expect(result).toEqual([]); + expect(mockExecGh).not.toHaveBeenCalled(); + }); + + it("maps a single-page response to PrReviewThread[]", async () => { + mockExecGh.mockResolvedValueOnce( + makePage([makeThread("T_1", 101)], false, null), + ); + + const result = await service.getPrReviewComments( + "https://github.com/owner/repo/pull/1", + ); + + expect(result).toHaveLength(1); + expect(result[0]).toMatchObject({ + nodeId: "T_1", + isResolved: false, + rootId: 101, + filePath: "src/foo.ts", + }); + expect(result[0].comments[0]).toMatchObject({ + id: 101, + body: "looks good", + side: "RIGHT", + line: 10, + subject_type: "line", + }); + }); + + it("fetches all pages when hasNextPage is true", async () => { + mockExecGh + .mockResolvedValueOnce( + makePage([makeThread("T_1", 101)], true, "cursor-abc"), + ) + .mockResolvedValueOnce(makePage([makeThread("T_2", 102)], false, null)); + + const result = await service.getPrReviewComments( + "https://github.com/owner/repo/pull/1", + ); + + expect(mockExecGh).toHaveBeenCalledTimes(2); + expect(result).toHaveLength(2); + expect(result.map((t) => t.nodeId)).toEqual(["T_1", "T_2"]); + + const secondCall = JSON.parse(mockExecGh.mock.calls[1][1].input); + expect(secondCall.variables.cursor).toBe("cursor-abc"); + }); + + it("returns partial results when MAX_THREAD_PAGES is exceeded", async () => { + let n = 0; + mockExecGh.mockImplementation(async () => { + n += 1; + return makePage([makeThread(`T_${n}`, 100 + n)], true, `cursor-${n}`); + }); + + const result = await service.getPrReviewComments( + "https://github.com/owner/repo/pull/1", + ); + + expect(mockExecGh).toHaveBeenCalledTimes(50); + expect(result).toHaveLength(50); + expect(result[0]?.nodeId).toBe("T_1"); + expect(result[49]?.nodeId).toBe("T_50"); + }); + + it("throws when gh exits with non-zero", async () => { + mockExecGh.mockResolvedValueOnce({ + exitCode: 1, + stderr: "auth error", + stdout: "", + }); + + await expect( + service.getPrReviewComments("https://github.com/owner/repo/pull/1"), + ).rejects.toThrow("Failed to fetch PR review threads"); + }); +}); + +describe("GitService.resolveReviewThread", () => { + let service: GitService; + + beforeEach(() => { + vi.clearAllMocks(); + service = new GitService( + {} as LlmGatewayService, + {} as WorkspaceService, + { getSessionEnvForTask: async () => ({}) } as unknown as AgentService, + ); + }); + + it("resolves a thread and returns isResolved: true", async () => { + mockExecGh.mockResolvedValueOnce({ + exitCode: 0, + stdout: JSON.stringify({ + data: { + resolveReviewThread: { thread: { id: "T_1", isResolved: true } }, + }, + }), + }); + + const result = await service.resolveReviewThread("T_1", true); + + expect(result).toEqual({ success: true, isResolved: true }); + const body = JSON.parse(mockExecGh.mock.calls[0][1].input); + expect(body.query).toContain("resolveReviewThread"); + expect(body.variables.threadId).toBe("T_1"); + }); + + it("unresolves a thread and returns isResolved: false", async () => { + mockExecGh.mockResolvedValueOnce({ + exitCode: 0, + stdout: JSON.stringify({ + data: { + unresolveReviewThread: { thread: { id: "T_1", isResolved: false } }, + }, + }), + }); + + const result = await service.resolveReviewThread("T_1", false); + + expect(result).toEqual({ success: true, isResolved: false }); + const body = JSON.parse(mockExecGh.mock.calls[0][1].input); + expect(body.query).toContain("unresolveReviewThread"); + }); + + it("returns success: false when gh exits with non-zero", async () => { + mockExecGh.mockResolvedValueOnce({ + exitCode: 1, + stderr: "network error", + stdout: "", + }); + + const result = await service.resolveReviewThread("T_1", true); + + expect(result).toEqual({ success: false, isResolved: false }); + }); +}); diff --git a/apps/code/src/main/services/git/service.ts b/apps/code/src/main/services/git/service.ts index a45d2eb9d..b609ca674 100644 --- a/apps/code/src/main/services/git/service.ts +++ b/apps/code/src/main/services/git/service.ts @@ -72,11 +72,13 @@ import type { PrActionType, PrDetailsByUrlOutput, PrReviewComment, + PrReviewThread, PrStatusOutput, PublishOutput, PullOutput, PushOutput, ReplyToPrCommentOutput, + ResolveReviewThreadOutput, SyncOutput, UpdatePrByUrlOutput, } from "./schemas"; @@ -1216,34 +1218,207 @@ export class GitService extends TypedEventEmitter { } } - public async getPrReviewComments(prUrl: string): Promise { + public async getPrReviewComments(prUrl: string): Promise { const pr = parseGithubUrl(prUrl); if (pr?.kind !== "pr") return []; const { owner, repo, number } = pr; + // Position fields (line, side, etc.) live on the thread, not on individual comments. + const query = ` + query($owner: String!, $repo: String!, $number: Int!, $cursor: String) { + repository(owner: $owner, name: $repo) { + pullRequest(number: $number) { + reviewThreads(first: 100, after: $cursor) { + pageInfo { hasNextPage endCursor } + nodes { + id + isResolved + isOutdated + path + diffSide + line + originalLine + startLine + startDiffSide + subjectType + comments(first: 100) { + nodes { + databaseId + body + path + diffHunk + replyTo { databaseId } + author { login avatarUrl } + createdAt + updatedAt + } + } + } + } + } + } + } + `; + + type ThreadNode = { + id: string; + isResolved: boolean; + isOutdated: boolean; + path: string; + diffSide: "LEFT" | "RIGHT"; + line: number | null; + originalLine: number | null; + startLine: number | null; + startDiffSide: "LEFT" | "RIGHT" | null; + subjectType: "LINE" | "FILE" | null; + comments: { + nodes: Array<{ + databaseId: number; + body: string; + path: string; + diffHunk: string; + replyTo: { databaseId: number } | null; + author: { login: string; avatarUrl: string }; + createdAt: string; + updatedAt: string; + }>; + }; + }; + + type PageResponse = { + data: { + repository: { + pullRequest: { + reviewThreads: { + pageInfo: { hasNextPage: boolean; endCursor: string | null }; + nodes: ThreadNode[]; + }; + }; + }; + }; + }; + + const MAX_THREAD_PAGES = 50; // 50 × 100 = 5 000 threads max + try { - const result = await execGh([ - "api", - `repos/${owner}/${repo}/pulls/${number}/comments`, - "--paginate", - "--slurp", - ]); + const allNodes: ThreadNode[] = []; + let cursor: string | null = null; + let completed = false; + + for (let page = 0; page < MAX_THREAD_PAGES; page++) { + const result = await execGh(["api", "graphql", "--input", "-"], { + input: JSON.stringify({ + query, + variables: { owner, repo, number, cursor }, + }), + }); - if (result.exitCode !== 0) { - throw new Error( - `Failed to fetch PR review comments: ${result.stderr || result.error || "Unknown error"}`, + if (result.exitCode !== 0) { + throw new Error( + `Failed to fetch PR review threads: ${result.stderr || result.error || "Unknown error"}`, + ); + } + + const data = JSON.parse(result.stdout) as PageResponse; + const reviewThreads = data.data.repository.pullRequest.reviewThreads; + allNodes.push(...reviewThreads.nodes); + if (!reviewThreads.pageInfo.hasNextPage) { + completed = true; + break; + } + cursor = reviewThreads.pageInfo.endCursor; + } + + if (!completed) { + log.warn( + "getPrReviewComments hit MAX_THREAD_PAGES; returning partial results", + { + prUrl, + returned: allNodes.length, + }, ); } - const pages = JSON.parse(result.stdout) as PrReviewComment[][]; - return pages.flat(); + return allNodes.map((thread) => { + const comments: PrReviewComment[] = thread.comments.nodes.map((c) => ({ + id: c.databaseId, + body: c.body, + path: c.path, + diff_hunk: c.diffHunk, + line: thread.line, + original_line: thread.originalLine, + side: thread.diffSide, + start_line: thread.startLine, + start_side: thread.startDiffSide, + in_reply_to_id: c.replyTo?.databaseId ?? null, + user: { login: c.author.login, avatar_url: c.author.avatarUrl }, + created_at: c.createdAt, + updated_at: c.updatedAt, + subject_type: thread.subjectType + ? (thread.subjectType.toLowerCase() as "line" | "file") + : null, + })); + + return { + nodeId: thread.id, + isResolved: thread.isResolved, + rootId: comments[0]?.id ?? 0, + filePath: thread.path, + comments, + }; + }); } catch (error) { - log.warn("Failed to fetch PR review comments", { prUrl, error }); + log.warn("Failed to fetch PR review threads", { prUrl, error }); throw error; } } + public async resolveReviewThread( + threadNodeId: string, + resolved: boolean, + ): Promise { + const mutation = resolved + ? `mutation($threadId: ID!) { resolveReviewThread(input: { threadId: $threadId }) { thread { id isResolved } } }` + : `mutation($threadId: ID!) { unresolveReviewThread(input: { threadId: $threadId }) { thread { id isResolved } } }`; + + try { + const result = await execGh(["api", "graphql", "--input", "-"], { + input: JSON.stringify({ + query: mutation, + variables: { threadId: threadNodeId }, + }), + }); + + if (result.exitCode !== 0) { + log.warn("Failed to resolve/unresolve review thread", { + threadNodeId, + resolved, + error: result.stderr || result.error, + }); + return { success: false, isResolved: !resolved }; + } + + const data = JSON.parse(result.stdout) as { + data: { + resolveReviewThread?: { thread: { isResolved: boolean } }; + unresolveReviewThread?: { thread: { isResolved: boolean } }; + }; + }; + const thread = + data.data.resolveReviewThread?.thread ?? + data.data.unresolveReviewThread?.thread; + + return { success: true, isResolved: thread?.isResolved ?? resolved }; + } catch (error) { + log.warn("Failed to resolve/unresolve review thread", { + threadNodeId, + error, + }); + return { success: false, isResolved: !resolved }; + } + } + public async replyToPrComment( prUrl: string, commentId: number, diff --git a/apps/code/src/main/trpc/routers/git.ts b/apps/code/src/main/trpc/routers/git.ts index b364d7aad..c06a99a61 100644 --- a/apps/code/src/main/trpc/routers/git.ts +++ b/apps/code/src/main/trpc/routers/git.ts @@ -74,6 +74,8 @@ import { pushOutput, replyToPrCommentInput, replyToPrCommentOutput, + resolveReviewThreadInput, + resolveReviewThreadOutput, searchGithubRefsInput, searchGithubRefsOutput, stageFilesInput, @@ -391,6 +393,20 @@ export const gitRouter = router({ getService().replyToPrComment(input.prUrl, input.commentId, input.body), ), + resolveReviewThread: publicProcedure + .input(resolveReviewThreadInput) + .output(resolveReviewThreadOutput) + .mutation(({ input }) => + getService().resolveReviewThread(input.threadNodeId, true), + ), + + unresolveReviewThread: publicProcedure + .input(resolveReviewThreadInput) + .output(resolveReviewThreadOutput) + .mutation(({ input }) => + getService().resolveReviewThread(input.threadNodeId, false), + ), + getBranchChangedFiles: publicProcedure .input(getBranchChangedFilesInput) .output(getBranchChangedFilesOutput) diff --git a/apps/code/src/renderer/features/code-review/components/PrCommentThread.tsx b/apps/code/src/renderer/features/code-review/components/PrCommentThread.tsx index 21fa95067..969445c80 100644 --- a/apps/code/src/renderer/features/code-review/components/PrCommentThread.tsx +++ b/apps/code/src/renderer/features/code-review/components/PrCommentThread.tsx @@ -2,9 +2,11 @@ import { MarkdownRenderer } from "@features/editor/components/MarkdownRenderer"; import { sendPromptToAgent } from "@features/sessions/utils/sendPromptToAgent"; import type { PrReviewComment } from "@main/services/git/schemas"; import { + ArrowCounterClockwise, CaretDown, CaretUp, ChatCircle, + CheckCircle, File, Robot, WarningCircle, @@ -39,6 +41,8 @@ interface ThreadActionBarProps { endLine: number; side: "old" | "new"; comments: PrReviewComment[]; + isResolved: boolean; + onResolveToggle: () => void; showReplyBox: boolean; pendingReply: string | null; onShowReplyBox: () => void; @@ -55,6 +59,8 @@ function ThreadActionBar({ endLine, side, comments, + isResolved, + onResolveToggle, showReplyBox, pendingReply, onShowReplyBox, @@ -103,6 +109,22 @@ function ThreadActionBar({ )} + {prUrl && ( + + )} +