fix(code-review): support resolving GitHub PR review threads#2337
Open
ShekharDhangar wants to merge 1 commit into
Open
fix(code-review): support resolving GitHub PR review threads#2337ShekharDhangar wants to merge 1 commit into
ShekharDhangar wants to merge 1 commit into
Conversation
Contributor
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
apps/code/src/main/services/git/service.test.ts:442-495
**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.
### Issue 2 of 3
apps/code/src/main/services/git/service.ts:1366
**`rootId` silently falls back to `0` for empty threads**
`comments[0]?.id ?? 0` means any thread that somehow has an empty `comments.nodes` array — possible if GitHub returns a malformed page — gets `rootId: 0`. Multiple such threads would overwrite each other in the `threadsToMap` `Map`, silently dropping all but the last one. A `null` fallback (matching the rule of using `null` to signal an intended absence) or an early guard that skips the thread would be more defensive.
```suggestion
rootId: comments[0]?.id ?? null,
```
### Issue 3 of 3
apps/code/src/main/services/git/service.ts:1326-1340
**GraphQL-level errors are not checked**
GitHub's GraphQL endpoint returns HTTP 200 even for error responses (`{"errors":[...], "data": null}`). The code only checks `exitCode !== 0`, so a permission error, rate-limit, or invalid variable would leave `data.data` as `null` and throw a `TypeError` on the next line (`Cannot read properties of null (reading 'repository')`). The outer `catch` re-throws this as an opaque error rather than the descriptive message from `data.errors`. Adding an explicit check for `data.errors` before accessing `data.data` would produce a clearer failure here and in `resolveReviewThread`.
Reviews (1): Last reviewed commit: "fix(code-review): support resolving GitH..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+442
to
+495
| }); | ||
|
|
||
| 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"); | ||
| }); |
Contributor
There was a problem hiding this comment.
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!
| return { | ||
| nodeId: thread.id, | ||
| isResolved: thread.isResolved, | ||
| rootId: comments[0]?.id ?? 0, |
Contributor
There was a problem hiding this comment.
rootId silently falls back to 0 for empty threads
comments[0]?.id ?? 0 means any thread that somehow has an empty comments.nodes array — possible if GitHub returns a malformed page — gets rootId: 0. Multiple such threads would overwrite each other in the threadsToMap Map, silently dropping all but the last one. A null fallback (matching the rule of using null to signal an intended absence) or an early guard that skips the thread would be more defensive.
Suggested change
| rootId: comments[0]?.id ?? 0, | |
| rootId: comments[0]?.id ?? null, |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/main/services/git/service.ts
Line: 1366
Comment:
**`rootId` silently falls back to `0` for empty threads**
`comments[0]?.id ?? 0` means any thread that somehow has an empty `comments.nodes` array — possible if GitHub returns a malformed page — gets `rootId: 0`. Multiple such threads would overwrite each other in the `threadsToMap` `Map`, silently dropping all but the last one. A `null` fallback (matching the rule of using `null` to signal an intended absence) or an early guard that skips the thread would be more defensive.
```suggestion
rootId: comments[0]?.id ?? null,
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+1326
to
1340
| 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, | ||
| }, | ||
| ); |
Contributor
There was a problem hiding this comment.
GitHub's GraphQL endpoint returns HTTP 200 even for error responses ({"errors":[...], "data": null}). The code only checks exitCode !== 0, so a permission error, rate-limit, or invalid variable would leave data.data as null and throw a TypeError on the next line (Cannot read properties of null (reading 'repository')). The outer catch re-throws this as an opaque error rather than the descriptive message from data.errors. Adding an explicit check for data.errors before accessing data.data would produce a clearer failure here and in resolveReviewThread.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/main/services/git/service.ts
Line: 1326-1340
Comment:
**GraphQL-level errors are not checked**
GitHub's GraphQL endpoint returns HTTP 200 even for error responses (`{"errors":[...], "data": null}`). The code only checks `exitCode !== 0`, so a permission error, rate-limit, or invalid variable would leave `data.data` as `null` and throw a `TypeError` on the next line (`Cannot read properties of null (reading 'repository')`). The outer `catch` re-throws this as an opaque error rather than the descriptive message from `data.errors`. Adding an explicit check for `data.errors` before accessing `data.data` would produce a clearer failure here and in `resolveReviewThread`.
How can I resolve this? If you propose a fix, please make it concise.
ShekharDhangar
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
GitHub PR comments in Code only supported replying. There was no way to resolve or unresolve review threads from the app.
Changes
PrCommentThreadNotes
How did you test this?
pnpm --filter code typecheckpnpm exec biome check apps/code/src/main/services/git/schemas.ts apps/code/src/main/services/git/service.test.ts apps/code/src/main/services/git/service.ts apps/code/src/main/trpc/routers/git.ts apps/code/src/renderer/features/code-review/components/PrCommentThread.tsx apps/code/src/renderer/features/code-review/hooks/usePrCommentActions.ts apps/code/src/renderer/features/code-review/types.ts apps/code/src/renderer/features/code-review/utils/prCommentAnnotations.ts apps/code/src/renderer/features/git-interaction/hooks/usePrDetails.tscd apps/code && pnpm exec vitest run src/main/services/git/service.test.tsPublish to changelog?
no
Closes #2248