Skip to content

fix(code-review): support resolving GitHub PR review threads #2337

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ShekharDhangar wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(code-review): support resolving GitHub PR review threads #2337

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 23 additions & 1 deletion apps/code/src/main/services/git/schemas.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -400,10 +400,32 @@ export const prReviewCommentSchema = z.object({

export type PrReviewComment = z.infer<typeof prReviewCommentSchema>;

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<typeof prReviewThreadSchema>;

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({
Expand Down
204 changes: 204 additions & 0 deletions apps/code/src/main/services/git/service.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
});
Comment on lines +442 to +495
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot May 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Non-parameterised resolve/unresolve tests

The two test cases for resolve and unresolve in GitService.resolveReviewThread are almost identical — same fixture shape, same assertions, same non-zero exit-code test duplicated. The team guideline says parameterised tests are always preferred, and this is a textbook it.each opportunity: [["resolves", true, "resolveReviewThread"], ["unresolves", false, "unresolveReviewThread"]] covers both branches without repeating the mock setup and assertion blocks.

Context Used: Do not attempt to comment on incorrect alphabetica... (source)

Prompt To Fix With AI 
This is a comment left during a code review.
Path: apps/code/src/main/services/git/service.test.ts
Line: 442-495

Comment:
**Non-parameterised resolve/unresolve tests**

The two test cases for resolve and unresolve in `GitService.resolveReviewThread` are almost identical — same fixture shape, same assertions, same non-zero exit-code test duplicated. The team guideline says parameterised tests are always preferred, and this is a textbook `it.each` opportunity: `[["resolves", true, "resolveReviewThread"], ["unresolves", false, "unresolveReviewThread"]]` covers both branches without repeating the mock setup and assertion blocks.

**Context Used:** Do not attempt to comment on incorrect alphabetica... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))

How can I resolve this? If you propose a fix, please make it concise.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!


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 });
});
});
Loading