Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Original file line number Diff line number Diff line change
@@ -1,8 +1,19 @@
import { CheckCircle2, ExternalLink, Loader2, MinusCircle, XCircle } from 'lucide-react';
import { observer } from 'mobx-react-lite';
import { useMemo } from 'react';
import { useCallback, useMemo } from 'react';
import { getProjectSshConnectionId } from '@renderer/features/projects/stores/project-selectors';
import { nextDefaultConversationTitle } from '@renderer/features/tasks/conversations/conversation-title-utils';
import { useEffectiveProvider } from '@renderer/features/tasks/conversations/use-effective-provider';
import { useSyncCheckRuns } from '@renderer/features/tasks/diff-view/state/use-check-runs';
import { useAgentAutoApproveDefaults } from '@renderer/features/tasks/hooks/useAgentAutoApproveDefaults';
import {
useConversations,
useTaskViewContext,
useWorkspaceViewModel,
} from '@renderer/features/tasks/task-view-context';
import { toast } from '@renderer/lib/hooks/use-toast';
import { rpc } from '@renderer/lib/ipc';
import { pastePromptInjection } from '@renderer/lib/pty/prompt-injection';
import { EmptyState } from '@renderer/lib/ui/empty-state';
import {
computeCheckBucket,
Expand All @@ -12,7 +23,11 @@ import {
} from '@renderer/utils/github';
import type { PullRequest, PullRequestComment } from '@shared/pull-requests';
import { CommentsList } from './comments-list';
import { buildPullRequestConversationItems } from './pull-request-conversation';
import {
buildPullRequestConversationItems,
formatPullRequestCommentForAgent,
type AddressablePullRequestComment,
} from './pull-request-conversation';
import { usePullRequestComments } from './use-pull-request-comments';

const EMPTY_COMMENTS: PullRequestComment[] = [];
Expand Down Expand Up @@ -107,13 +122,109 @@ export function ChecksList({ checks }: { checks: CheckRun[] }) {
}

export const PrChecksList = observer(function PrChecksList({ pr }: { pr: PullRequest }) {
const { projectId, taskId } = useTaskViewContext();
const taskView = useWorkspaceViewModel();
const conversations = useConversations();
const connectionId = getProjectSshConnectionId(projectId);
const { providerId, createDisabled } = useEffectiveProvider(connectionId);
const autoApproveDefaults = useAgentAutoApproveDefaults();
const { checks } = useSyncCheckRuns(pr);
const commentsQuery = usePullRequestComments(pr);
const comments = commentsQuery.data ?? EMPTY_COMMENTS;
const conversationItems = useMemo(
() => buildPullRequestConversationItems(pr, comments),
[pr, comments]
);
const activeConversationId = taskView.tabManager.activeConversationId;
const activeSession = activeConversationId
? conversations.sessions.get(activeConversationId)
: undefined;
const activeConversationStore = activeConversationId
? conversations.conversations.get(activeConversationId)
: undefined;
const activeSessionId = activeSession?.sessionId;

const handleAddressInActiveChat = useCallback(
async (comment: AddressablePullRequestComment) => {
if (!activeSessionId) {
toast({
title: 'No active chat',
description: 'Open a conversation tab before addressing a PR comment.',
variant: 'destructive',
});
return;
}

try {
const text = formatPullRequestCommentForAgent(pr, comment);
await pastePromptInjection({
providerId: activeConversationStore?.data.providerId,
text,
forceBracketedPaste: true,
sendInput: (data) => rpc.pty.sendInput(activeSessionId, data),
});
await rpc.pty.sendInput(activeSessionId, '\r');
activeSession?.pty?.terminal.focus();
taskView.setFocusedRegion('main');
} catch {
toast({
title: 'Failed to send comment',
description: 'Try again after the chat is ready.',
variant: 'destructive',
});
}
},
[activeConversationStore?.data.providerId, activeSession, activeSessionId, pr, taskView]
);

const handleAddressInNewChat = useCallback(
async (comment: AddressablePullRequestComment) => {
if (createDisabled || !providerId) {
toast({
title: 'No agent available',
description: 'Install or select an available agent before creating a chat.',
variant: 'destructive',
});
return;
}

const id = crypto.randomUUID();
const title = nextDefaultConversationTitle(
providerId,
Array.from(conversations.conversations.values(), (conversation) => conversation.data)
);

try {
await conversations.createConversation({
id,
projectId,
taskId,
provider: providerId,
title,
autoApprove: autoApproveDefaults.getDefault(providerId),
initialPrompt: formatPullRequestCommentForAgent(pr, comment),
});
taskView.tabGroupManager.openConversation(id);
taskView.setFocusedRegion('main');
} catch {
toast({
title: 'Failed to create chat',
description: 'Try again after the current task finishes loading.',
variant: 'destructive',
});
}
},
[
autoApproveDefaults,
conversations,
createDisabled,
projectId,
providerId,
pr,
taskId,
taskView,
]
);

if (checks.length === 0 && conversationItems.length === 0 && !commentsQuery.isLoading) {
return <EmptyState label="No checks or comments" description="Nothing available yet" />;
Expand All @@ -135,6 +246,8 @@ export const PrChecksList = observer(function PrChecksList({ pr }: { pr: PullReq
comments={conversationItems}
isLoading={commentsQuery.isLoading}
error={commentsQuery.error}
onAddressInActiveChat={activeSessionId ? handleAddressInActiveChat : undefined}
onAddressInNewChat={!createDisabled && providerId ? handleAddressInNewChat : undefined}
/>
</section>
</div>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,20 @@
import { ExternalLink, MessageSquare } from 'lucide-react';
import { ExternalLink, MessageSquare, Send } from 'lucide-react';
import { useMemo } from 'react';
import { rpc } from '@renderer/lib/ipc';
import {
ContextMenu,
ContextMenuContent,
ContextMenuItem,
ContextMenuTrigger,
} from '@renderer/lib/ui/context-menu';
import { MarkdownRenderer } from '@renderer/lib/ui/markdown-renderer';
import { RelativeTime } from '@renderer/lib/ui/relative-time';
import { Tooltip, TooltipContent, TooltipTrigger } from '@renderer/lib/ui/tooltip';
import { cn } from '@renderer/utils/utils';
import {
isAddressablePullRequestComment,
sortPullRequestConversationItems,
type AddressablePullRequestComment,
type PullRequestConversationItem,
} from './pull-request-conversation';

Expand All @@ -22,12 +31,22 @@ function isBotAuthor(comment: PullRequestConversationItem): boolean {
return comment.author?.userName.endsWith('[bot]') ?? false;
}

function CommentItem({ comment }: { comment: PullRequestConversationItem }) {
function CommentItem({
comment,
onAddressInActiveChat,
onAddressInNewChat,
}: {
comment: PullRequestConversationItem;
onAddressInActiveChat?: (comment: AddressablePullRequestComment) => void;
onAddressInNewChat?: (comment: AddressablePullRequestComment) => void;
}) {
const location = commentLocationLabel(comment);
const author = commentAuthorLabel(comment);
const avatarRadiusClass = isBotAuthor(comment) ? 'rounded' : 'rounded-full';

return (
const addressable = isAddressablePullRequestComment(comment);
const canAddressInActiveChat = addressable && !!onAddressInActiveChat;
const canAddressInNewChat = addressable && !!onAddressInNewChat;
const content = (
<div className="group relative flex w-full min-w-0 gap-2 rounded-md px-3 py-2 text-left hover:bg-background-1">
{comment.author?.avatarUrl ? (
<img
Expand Down Expand Up @@ -71,24 +90,74 @@ function CommentItem({ comment }: { comment: PullRequestConversationItem }) {
<MarkdownRenderer content={comment.body} variant="compact" allowHtml />
</div>
</div>
<button
className="absolute top-2 right-3 hidden items-center justify-center rounded bg-background-1 px-1 py-0.5 text-foreground-muted group-hover:flex hover:text-foreground"
onClick={() => void rpc.app.openExternal(comment.url)}
>
<ExternalLink className="size-3.5" />
</button>
<div className="absolute top-2 right-3 hidden items-center gap-1 group-hover:flex">
{canAddressInActiveChat && (
<Tooltip>
<TooltipTrigger
render={
<button
type="button"
className="flex items-center gap-1 rounded bg-background-1 px-1.5 py-0.5 text-xs text-foreground-muted hover:text-foreground"
onClick={() => onAddressInActiveChat(comment)}
>
<Send className="size-3" />
Address comment
</button>
}
/>
<TooltipContent>Send this comment to active chat</TooltipContent>
</Tooltip>
)}
<button
type="button"
className="flex items-center justify-center rounded bg-background-1 px-1 py-0.5 text-foreground-muted hover:text-foreground"
onClick={() => void rpc.app.openExternal(comment.url)}
>
<ExternalLink className="size-3.5" />
</button>
</div>
</div>
);

if (!addressable) return content;

return (
<ContextMenu>
Comment on lines +122 to +125

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 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.

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!

<ContextMenuTrigger>{content}</ContextMenuTrigger>
<ContextMenuContent>
<ContextMenuItem
disabled={!canAddressInActiveChat}
onClick={() => {
if (canAddressInActiveChat) onAddressInActiveChat(comment);
}}
>
Address comment in active chat
</ContextMenuItem>
<ContextMenuItem
disabled={!canAddressInNewChat}
onClick={() => {
if (canAddressInNewChat) onAddressInNewChat(comment);
}}
>
Address comment in new chat
</ContextMenuItem>
</ContextMenuContent>
</ContextMenu>
);
}

export function CommentsList({
comments,
isLoading,
error,
onAddressInActiveChat,
onAddressInNewChat,
}: {
comments: PullRequestConversationItem[];
isLoading?: boolean;
error?: Error | null;
onAddressInActiveChat?: (comment: AddressablePullRequestComment) => void;
onAddressInNewChat?: (comment: AddressablePullRequestComment) => void;
}) {
const sorted = useMemo(() => [...comments].sort(sortPullRequestConversationItems), [comments]);

Expand All @@ -107,7 +176,12 @@ export function CommentsList({
return (
<div className="flex flex-col gap-[1px]">
{sorted.map((comment) => (
<CommentItem key={comment.id} comment={comment} />
<CommentItem
key={comment.id}
comment={comment}
onAddressInActiveChat={onAddressInActiveChat}
onAddressInNewChat={onAddressInNewChat}
/>
))}
{isLoading && (
<div className="px-3 py-2 text-xs text-foreground-passive">Loading comments...</div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ export type PullRequestDescriptionItem = {

export type PullRequestConversationItem = PullRequestDescriptionItem | PullRequestComment;

export type AddressablePullRequestComment = PullRequestComment;

function descriptionItemForPr(pr: PullRequest): PullRequestDescriptionItem | null {
const body = pr.description?.trim();
if (!body) return null;
Expand Down Expand Up @@ -57,3 +59,49 @@ export function buildPullRequestConversationItems(
const description = descriptionItemForPr(pr);
return [...(description ? [description] : []), ...comments];
}

export function isAddressablePullRequestComment(
item: PullRequestConversationItem
): item is AddressablePullRequestComment {
return item.kind !== 'description';
}

function commentAuthorLabel(comment: AddressablePullRequestComment): string {
return comment.author?.displayName ?? comment.author?.userName ?? 'Unknown author';
}

function commentLocationLabel(comment: AddressablePullRequestComment): string | null {
if (!comment.path) return null;
return comment.line ? `${comment.path}:${comment.line}` : comment.path;
}

function escapeXmlText(value: string): string {
return value.replaceAll('&', '&amp;').replaceAll('<', '&lt;').replaceAll('>', '&gt;');
}

function metadataLine(label: string, value: string): string {
return `${label}: ${escapeXmlText(value)}`;
}

export function formatPullRequestCommentForAgent(
pr: PullRequest,
comment: AddressablePullRequestComment
): string {
const prLabel = pr.identifier ? `${pr.identifier} ${pr.title}` : pr.title;
const location = commentLocationLabel(comment);
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);
Comment on lines +92 to +101

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 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 &amp;, 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 `&amp;`, 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.


return `${metadata.join('\n')}

Body:
${escapeXmlText(comment.body.trim())}`;
}
Loading
Loading