feat(pr-comments): address PR comments from sidebar#2384
Open
arnestrickmann wants to merge 2 commits into
Open
feat(pr-comments): address PR comments from sidebar#2384arnestrickmann wants to merge 2 commits into
arnestrickmann wants to merge 2 commits into
Conversation
Contributor
Greptile SummaryAdds per-comment "Address" actions to PR comments in the Checks sidebar, letting users send a formatted PR comment (with metadata: PR title, URLs, author, location, status) either to the currently active chat or into a brand-new default-agent conversation. The PR description item is explicitly excluded from these actions.
Confidence Score: 4/5The new address-comment feature is well-contained and the main paths are tested; no data loss or breakage risk on the happy path. The URL escaping in metadataLine will silently corrupt &-containing URLs for non-GitHub providers, and the context menu can render in a fully-disabled state. Both are isolated to the new feature and don't affect existing functionality. pull-request-conversation.ts — the URL escaping behaviour in metadataLine warrants a second look; comments-list.tsx — the ContextMenu guard condition.
|
| Filename | Overview |
|---|---|
| src/renderer/features/tasks/diff-view/changes-panel/components/pr-entry/pull-request-conversation.ts | Adds AddressablePullRequestComment type alias, isAddressablePullRequestComment guard, and formatPullRequestCommentForAgent — XML-escaping is applied uniformly to all metadata values including URLs, which can corrupt &-containing URLs from non-GitHub providers. |
| src/renderer/features/tasks/diff-view/changes-panel/components/pr-entry/checks-list.tsx | Adds handleAddressInActiveChat / handleAddressInNewChat callbacks wired to CommentsList; logic and dependency arrays look correct, and guards are passed through the prop conditional correctly. |
| src/renderer/features/tasks/diff-view/changes-panel/components/pr-entry/comments-list.tsx | Adds inline 'Address comment' button and right-click ContextMenu to CommentItem; ContextMenu is rendered for all addressable comments even when both action handlers are undefined, resulting in a fully-disabled context menu in that state. |
| src/renderer/tests/pr-comments-list.test.ts | Adds targeted tests for address-action rendering and formatPullRequestCommentForAgent; coverage for the description-item exclusion and XML escaping is solid. |
Sequence Diagram
sequenceDiagram
participant User
participant CommentItem
participant PrChecksList
participant pastePromptInjection
participant rpc
participant conversations
User->>CommentItem: hover → click "Address comment"
CommentItem->>PrChecksList: onAddressInActiveChat(comment)
PrChecksList->>PrChecksList: formatPullRequestCommentForAgent(pr, comment)
PrChecksList->>pastePromptInjection: paste text to activeSession
pastePromptInjection->>rpc: pty.sendInput(sessionId, data)
PrChecksList->>rpc: pty.sendInput(sessionId, '\r') ← auto-submit
PrChecksList->>PrChecksList: setFocusedRegion('main')
User->>CommentItem: right-click → "Address in new chat"
CommentItem->>PrChecksList: onAddressInNewChat(comment)
PrChecksList->>PrChecksList: formatPullRequestCommentForAgent(pr, comment)
PrChecksList->>conversations: "createConversation({ initialPrompt })"
PrChecksList->>PrChecksList: tabGroupManager.openConversation(id)
PrChecksList->>PrChecksList: setFocusedRegion('main')
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
src/renderer/features/tasks/diff-view/changes-panel/components/pr-entry/pull-request-conversation.ts:92-101
`metadataLine` applies `escapeXmlText` to every value, including URL fields. Any URL containing `&` in query parameters (common in GitLab/Bitbucket merge-request URLs, redirect links, etc.) will arrive in the agent's prompt as `&`, making the URL unusable as-is. URL metadata lines should skip HTML entity escaping.
```suggestion
const metadata = [
metadataLine('Pull request', prLabel),
`Pull request URL: ${pr.url}`,
`Comment URL: ${comment.url}`,
metadataLine('Author', commentAuthorLabel(comment)),
metadataLine('Created', comment.createdAt),
location ? metadataLine('Location', location) : null,
comment.isResolved ? 'Status: resolved' : null,
comment.isOutdated ? 'Status: outdated' : null,
].filter((line): line is string => line !== null);
```
### Issue 2 of 2
src/renderer/features/tasks/diff-view/changes-panel/components/pr-entry/comments-list.tsx:122-125
The `ContextMenu` is rendered for every addressable comment regardless of whether either handler is provided. When both `onAddressInActiveChat` and `onAddressInNewChat` are `undefined` (no active session and no available provider), right-clicking any comment shows a context menu whose two items are both disabled — a dead-end UX with no actionable option. Skipping the context menu wrapper when no action is available avoids presenting useless UI.
```suggestion
if (!addressable || (!canAddressInActiveChat && !canAddressInNewChat)) return content;
return (
<ContextMenu>
```
Reviews (1): Last reviewed commit: "feat(pr-comments): address PR comments f..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+92
to
+101
| const metadata = [ | ||
| metadataLine('Pull request', prLabel), | ||
| metadataLine('Pull request URL', pr.url), | ||
| metadataLine('Comment URL', comment.url), | ||
| metadataLine('Author', commentAuthorLabel(comment)), | ||
| metadataLine('Created', comment.createdAt), | ||
| location ? metadataLine('Location', location) : null, | ||
| comment.isResolved ? 'Status: resolved' : null, | ||
| comment.isOutdated ? 'Status: outdated' : null, | ||
| ].filter((line): line is string => line !== null); |
Contributor
There was a problem hiding this comment.
metadataLine applies escapeXmlText to every value, including URL fields. Any URL containing & in query parameters (common in GitLab/Bitbucket merge-request URLs, redirect links, etc.) will arrive in the agent's prompt as &, making the URL unusable as-is. URL metadata lines should skip HTML entity escaping.
Suggested change
| const metadata = [ | |
| metadataLine('Pull request', prLabel), | |
| metadataLine('Pull request URL', pr.url), | |
| metadataLine('Comment URL', comment.url), | |
| metadataLine('Author', commentAuthorLabel(comment)), | |
| metadataLine('Created', comment.createdAt), | |
| location ? metadataLine('Location', location) : null, | |
| comment.isResolved ? 'Status: resolved' : null, | |
| comment.isOutdated ? 'Status: outdated' : null, | |
| ].filter((line): line is string => line !== null); | |
| const metadata = [ | |
| metadataLine('Pull request', prLabel), | |
| `Pull request URL: ${pr.url}`, | |
| `Comment URL: ${comment.url}`, | |
| metadataLine('Author', commentAuthorLabel(comment)), | |
| metadataLine('Created', comment.createdAt), | |
| location ? metadataLine('Location', location) : null, | |
| comment.isResolved ? 'Status: resolved' : null, | |
| comment.isOutdated ? 'Status: outdated' : null, | |
| ].filter((line): line is string => line !== null); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/renderer/features/tasks/diff-view/changes-panel/components/pr-entry/pull-request-conversation.ts
Line: 92-101
Comment:
`metadataLine` applies `escapeXmlText` to every value, including URL fields. Any URL containing `&` in query parameters (common in GitLab/Bitbucket merge-request URLs, redirect links, etc.) will arrive in the agent's prompt as `&`, making the URL unusable as-is. URL metadata lines should skip HTML entity escaping.
```suggestion
const metadata = [
metadataLine('Pull request', prLabel),
`Pull request URL: ${pr.url}`,
`Comment URL: ${comment.url}`,
metadataLine('Author', commentAuthorLabel(comment)),
metadataLine('Created', comment.createdAt),
location ? metadataLine('Location', location) : null,
comment.isResolved ? 'Status: resolved' : null,
comment.isOutdated ? 'Status: outdated' : null,
].filter((line): line is string => line !== null);
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+122
to
+125
| if (!addressable) return content; | ||
|
|
||
| return ( | ||
| <ContextMenu> |
Contributor
There was a problem hiding this comment.
ContextMenu is rendered for every addressable comment regardless of whether either handler is provided. When both onAddressInActiveChat and onAddressInNewChat are undefined (no active session and no available provider), right-clicking any comment shows a context menu whose two items are both disabled — a dead-end UX with no actionable option. Skipping the context menu wrapper when no action is available avoids presenting useless UI.
Suggested change
| if (!addressable) return content; | |
| return ( | |
| <ContextMenu> | |
| if (!addressable || (!canAddressInActiveChat && !canAddressInNewChat)) return content; | |
| return ( | |
| <ContextMenu> |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/renderer/features/tasks/diff-view/changes-panel/components/pr-entry/comments-list.tsx
Line: 122-125
Comment:
The `ContextMenu` is rendered for every addressable comment regardless of whether either handler is provided. When both `onAddressInActiveChat` and `onAddressInNewChat` are `undefined` (no active session and no available provider), right-clicking any comment shows a context menu whose two items are both disabled — a dead-end UX with no actionable option. Skipping the context menu wrapper when no action is available avoids presenting useless UI.
```suggestion
if (!addressable || (!canAddressInActiveChat && !canAddressInNewChat)) return content;
return (
<ContextMenu>
```
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!
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.
Summary
Validation
Note: the full
pnpm run testcommand was attempted and hit unrelated timeout failures in existing git/ssh tests.