diff --git a/apps/code/src/main/services/git/schemas.ts b/apps/code/src/main/services/git/schemas.ts index 0398400f4d..f25a73f69c 100644 --- a/apps/code/src/main/services/git/schemas.ts +++ b/apps/code/src/main/services/git/schemas.ts @@ -400,10 +400,33 @@ 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(), + resolved: z.boolean(), +}); +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 76bdf49597..afe6a4ff4f 100644 --- a/apps/code/src/main/services/git/service.test.ts +++ b/apps/code/src/main/services/git/service.test.ts @@ -319,3 +319,221 @@ 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"); + }); + + it("throws with the GraphQL error message when GitHub returns 200 with errors", async () => { + mockExecGh.mockResolvedValueOnce({ + exitCode: 0, + stdout: JSON.stringify({ + data: null, + errors: [{ message: "Resource not accessible by integration" }], + }), + }); + + await expect( + service.getPrReviewComments("https://github.com/owner/repo/pull/1"), + ).rejects.toThrow("Resource not accessible by integration"); + }); +}); + +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 a45d2eb9d7..99ee93a957 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,222 @@ 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[]; + }; + }; + }; + }; + errors?: Array<{ message: string }>; + }; + + 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; + if (data.errors?.length) { + throw new Error( + `GraphQL error: ${data.errors.map((e) => e.message).join("; ")}`, + ); + } + 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 } }; + }; + errors?: Array<{ message: string }>; + }; + if (data.errors?.length) { + log.warn("Failed to resolve/unresolve review thread", { + threadNodeId, + resolved, + error: data.errors.map((e) => e.message).join("; "), + }); + return { success: false, isResolved: !resolved }; + } + 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 b364d7aad7..21b7e65099 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,13 @@ export const gitRouter = router({ getService().replyToPrComment(input.prUrl, input.commentId, input.body), ), + resolveReviewThread: publicProcedure + .input(resolveReviewThreadInput) + .output(resolveReviewThreadOutput) + .mutation(({ input }) => + getService().resolveReviewThread(input.threadNodeId, input.resolved), + ), + 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 21fa950678..d6ff8259bf 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 && ( + + )} + ); @@ -243,6 +265,8 @@ export function PrCommentThread({ }: PrCommentThreadProps) { const { threadId, + nodeId, + isResolved: initialIsResolved, comments, isOutdated, isFileLevel, @@ -250,11 +274,16 @@ export function PrCommentThread({ side: annotationSide, } = metadata; const side = annotationSide === "deletions" ? "old" : "new"; - const { reply } = usePrCommentActions(prUrl); + const { reply, resolve } = usePrCommentActions(prUrl); const [showReplyBox, setShowReplyBox] = useState(false); const [pendingReply, setPendingReply] = useState(null); + const [isResolved, setIsResolved] = useState(initialIsResolved); const textareaRef = useRef(null); + useEffect(() => { + setIsResolved(initialIsResolved); + }, [initialIsResolved]); + // Clear pending reply once the real comments list includes it const lastCommentId = comments[comments.length - 1]?.id; const prevLastCommentIdRef = useRef(lastCommentId); @@ -301,14 +330,27 @@ export function PrCommentThread({ [], ); + const handleResolveToggle = useCallback(async () => { + const next = !isResolved; + setIsResolved(next); + const success = await resolve(nodeId, next); + if (!success) setIsResolved(!next); + }, [isResolved, nodeId, resolve]); + return (
- {(isOutdated || isFileLevel) && ( + {(isResolved || isOutdated || isFileLevel) && ( + {isResolved && ( + + + Resolved + + )} {isFileLevel && ( @@ -362,6 +404,8 @@ export function PrCommentThread({ endLine={endLine} side={side} comments={comments} + isResolved={isResolved} + onResolveToggle={handleResolveToggle} showReplyBox={showReplyBox} pendingReply={pendingReply} onShowReplyBox={() => setShowReplyBox(true)} diff --git a/apps/code/src/renderer/features/code-review/hooks/usePrCommentActions.ts b/apps/code/src/renderer/features/code-review/hooks/usePrCommentActions.ts index 7dbafe2fa6..365f1a03ca 100644 --- a/apps/code/src/renderer/features/code-review/hooks/usePrCommentActions.ts +++ b/apps/code/src/renderer/features/code-review/hooks/usePrCommentActions.ts @@ -33,5 +33,33 @@ export function usePrCommentActions(prUrl: string | null) { [prUrl, queryClient, trpc], ); - return { reply }; + const resolve = useCallback( + async (threadNodeId: string, resolved: boolean): Promise => { + if (!prUrl) return false; + const errorMessage = resolved + ? "Failed to resolve thread" + : "Failed to unresolve thread"; + try { + const result = await trpcClient.git.resolveReviewThread.mutate({ + prUrl, + threadNodeId, + resolved, + }); + if (!result.success) { + toast.error(errorMessage); + return false; + } + await queryClient.invalidateQueries( + trpc.git.getPrReviewComments.queryFilter({ prUrl }), + ); + return true; + } catch { + toast.error(errorMessage); + return false; + } + }, + [prUrl, queryClient, trpc], + ); + + return { reply, resolve }; } diff --git a/apps/code/src/renderer/features/code-review/types.ts b/apps/code/src/renderer/features/code-review/types.ts index a7e145e8fa..ca77ca73d2 100644 --- a/apps/code/src/renderer/features/code-review/types.ts +++ b/apps/code/src/renderer/features/code-review/types.ts @@ -26,6 +26,8 @@ export interface DraftCommentMetadata { export interface PrCommentMetadata { kind: "pr-comment"; threadId: number; + nodeId: string; + isResolved: boolean; comments: PrReviewComment[]; isOutdated: boolean; isFileLevel: boolean; diff --git a/apps/code/src/renderer/features/code-review/utils/prCommentAnnotations.ts b/apps/code/src/renderer/features/code-review/utils/prCommentAnnotations.ts index f45fefb703..e704f9e567 100644 --- a/apps/code/src/renderer/features/code-review/utils/prCommentAnnotations.ts +++ b/apps/code/src/renderer/features/code-review/utils/prCommentAnnotations.ts @@ -4,6 +4,8 @@ import type { AnnotationMetadata } from "../types"; export interface PrCommentThread { rootId: number; + nodeId: string; + isResolved: boolean; comments: PrReviewComment[]; filePath: string; } @@ -31,6 +33,8 @@ function buildAnnotation( metadata: { kind: "pr-comment", threadId: thread.rootId, + nodeId: thread.nodeId, + isResolved: thread.isResolved, comments: thread.comments, isOutdated, isFileLevel, diff --git a/apps/code/src/renderer/features/git-interaction/hooks/usePrDetails.ts b/apps/code/src/renderer/features/git-interaction/hooks/usePrDetails.ts index f32d8401ff..8ea85d8db6 100644 --- a/apps/code/src/renderer/features/git-interaction/hooks/usePrDetails.ts +++ b/apps/code/src/renderer/features/git-interaction/hooks/usePrDetails.ts @@ -1,4 +1,4 @@ -import type { PrReviewComment } from "@main/services/git/schemas"; +import type { PrReviewThread } from "@main/services/git/schemas"; import type { PrCommentThread } from "@renderer/features/code-review/utils/prCommentAnnotations"; import { useTRPC } from "@renderer/trpc"; import { useQuery } from "@tanstack/react-query"; @@ -8,30 +8,18 @@ interface UsePrDetailsOptions { includeComments?: boolean; } -function groupCommentsIntoThreads( - comments: PrReviewComment[], -): Map { - const threads = new Map(); - - for (const comment of comments) { - const rootId = comment.in_reply_to_id ?? comment.id; - const existing = threads.get(rootId); - if (existing) { - existing.comments.push(comment); - } else { - threads.set(rootId, { - rootId, - comments: [comment], - filePath: comment.path, - }); - } - } - - for (const thread of threads.values()) { - thread.comments.sort((a, b) => a.created_at.localeCompare(b.created_at)); +function threadsToMap(threads: PrReviewThread[]): Map { + const map = new Map(); + for (const thread of threads) { + map.set(thread.rootId, { + rootId: thread.rootId, + nodeId: thread.nodeId, + isResolved: thread.isResolved, + comments: thread.comments, + filePath: thread.filePath, + }); } - - return threads; + return map; } export function usePrDetails( @@ -66,7 +54,7 @@ export function usePrDetails( ); const commentThreads = useMemo( - () => groupCommentsIntoThreads(commentsQuery.data ?? []), + () => threadsToMap(commentsQuery.data ?? []), [commentsQuery.data], );