From 016df33f00ea3588c1b945fcb2ec985ba8d8ac79 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sun, 9 Aug 2026 23:14:17 -0700 Subject: [PATCH] fix(github): complete PR reactions for CodeRabbit reviews (#13456) * feat(github): add PR comment reaction controls * feat(github): add full PR comment reaction picker * fix(github): cover all reactable PR comment paths * fix(github): reconcile comment reactions with main * fix(github): preserve focus on failed reaction removal --- src/main/github/client.test.ts | 54 +++++++++++ src/main/github/client.ts | 38 +++++++- .../components/github/CommentReactions.tsx | 62 +++++++++--- .../components/right-sidebar/ChecksPanel.tsx | 11 ++- .../right-sidebar/checks-panel-content.tsx | 8 +- .../helpers/pr-comments-sidebar-fixture.ts | 3 +- tests/e2e/pr-comments-sidebar-cards.spec.ts | 95 ++++++++++++++++--- 7 files changed, 237 insertions(+), 34 deletions(-) diff --git a/src/main/github/client.test.ts b/src/main/github/client.test.ts index eef3aace0e7..a33b9b38d20 100644 --- a/src/main/github/client.test.ts +++ b/src/main/github/client.test.ts @@ -4044,6 +4044,60 @@ describe('GitHub GraphQL rate-limit guard', () => { expect(noteRateLimitSpendMock).not.toHaveBeenCalledWith('graphql') }) + it('maps review summary reaction subjects from GraphQL', async () => { + getOwnerRepoMock.mockResolvedValueOnce({ owner: 'acme', repo: 'widgets' }) + ghExecFileAsyncMock + .mockResolvedValueOnce({ + stdout: JSON.stringify({ + data: { + repository: { + pullRequest: { + reviewThreads: { nodes: [] }, + comments: { nodes: [] }, + reviews: { + nodes: [ + { + id: 'PRR_44', + databaseId: 44, + author: { + __typename: 'Bot', + login: 'coderabbitai', + avatarUrl: 'https://avatar' + }, + body: 'Automated review summary', + createdAt: '2026-04-01T00:00:00Z', + url: 'https://github.com/acme/widgets/pull/7#pullrequestreview-44', + reactionGroups: [ + { + content: 'ROCKET', + viewerHasReacted: true, + reactors: { totalCount: 2 } + } + ] + } + ] + } + } + } + } + }) + }) + .mockResolvedValueOnce({ stdout: '[]' }) + .mockResolvedValueOnce({ stdout: '[]' }) + + await expect(getPRComments('/repo-root', 7)).resolves.toEqual([ + expect.objectContaining({ + id: 44, + reactionSubjectId: 'PRR_44', + isBot: true, + reactions: [{ content: 'rocket', count: 2, viewerHasReacted: true }] + }) + ]) + expect(ghExecFileAsyncMock.mock.calls[0]?.[0]).toEqual( + expect.arrayContaining([expect.stringContaining('reviews(first: 100)')]) + ) + }) + it('uses explicit PR repo for comments when a fork PR is discovered', async () => { rateLimitGuardMock.mockImplementation(((bucket: string) => bucket === 'graphql' diff --git a/src/main/github/client.ts b/src/main/github/client.ts index 46ea1180e24..7518c9d3581 100644 --- a/src/main/github/client.ts +++ b/src/main/github/client.ts @@ -4157,6 +4157,23 @@ query($owner: String!, $repo: String!, $pr: Int!) { } } } + reviews(first: 100) { + nodes { + id + databaseId + author { __typename login avatarUrl(size: 48) } + body + createdAt + url + reactionGroups { + content + viewerHasReacted + reactors { + totalCount + } + } + } + } } } }` @@ -4284,6 +4301,7 @@ export async function getPRComments( url: string reactionGroups?: GitHubGraphQLReactionGroup[] | null } + let graphQLReviewSummaries: PRComment[] | undefined const reviewComments: PRComment[] = [] if (threadsResult.status === 'fulfilled' && threadsResult.value) { const threadsData = JSON.parse(threadsResult.value.stdout) as { @@ -4292,6 +4310,7 @@ export async function getPRComments( pullRequest: { reviewThreads: { nodes: GQLThread[] } comments?: { nodes: GQLIssueComment[] } + reviews?: { nodes: GQLIssueComment[] } } } } @@ -4313,6 +4332,21 @@ export async function getPRComments( if (graphQLIssueComments.length > 0) { issueComments = graphQLIssueComments } + graphQLReviewSummaries = (pullRequest.reviews?.nodes ?? []) + .filter((review) => review.body?.trim()) + .map( + (review): PRComment => ({ + id: review.databaseId, + author: review.author?.login ?? 'ghost', + authorAvatarUrl: review.author?.avatarUrl ?? '', + body: review.body, + createdAt: review.createdAt, + url: review.url, + isBot: review.author?.__typename === 'Bot', + reactionSubjectId: review.id, + reactions: mapGraphQLReactionGroups(review.reactionGroups) + }) + ) const threads = pullRequest.reviewThreads.nodes for (const thread of threads) { @@ -4353,7 +4387,9 @@ export async function getPRComments( html_url: string } let reviewSummaries: PRComment[] = [] - if (reviewsResult.status === 'fulfilled') { + if (graphQLReviewSummaries) { + reviewSummaries = graphQLReviewSummaries + } else if (reviewsResult.status === 'fulfilled') { reviewSummaries = (JSON.parse(reviewsResult.value.stdout) as RESTReview[]) .filter((r) => r.body?.trim()) .map( diff --git a/src/renderer/src/components/github/CommentReactions.tsx b/src/renderer/src/components/github/CommentReactions.tsx index bbdca0fbe3d..21aa4ac2316 100644 --- a/src/renderer/src/components/github/CommentReactions.tsx +++ b/src/renderer/src/components/github/CommentReactions.tsx @@ -37,9 +37,15 @@ export function CommentReactions({ }: { reactions?: GitHubReaction[] className?: string - onReactionChange?: (content: GitHubReactionContent, reacted: boolean) => Promise | void + onReactionChange?: ( + content: GitHubReactionContent, + reacted: boolean + ) => Promise | boolean }): React.JSX.Element | null { const visibleReactions = (reactions ?? []).filter((reaction) => reaction.count > 0) + const addReactionButtonRef = React.useRef(null) + const pickerGroupRef = React.useRef(null) + const mutationPendingRef = React.useRef(false) const [open, setOpen] = React.useState(false) const [pendingContent, setPendingContent] = React.useState(null) if (visibleReactions.length === 0 && !onReactionChange) { @@ -48,16 +54,25 @@ export function CommentReactions({ const changeReaction = async ( content: GitHubReactionContent, - reacted: boolean + reacted: boolean, + closePicker: boolean, + focusTriggerAfterChange = false ): Promise => { - if (!onReactionChange || pendingContent) { + if (!onReactionChange || mutationPendingRef.current) { return } + mutationPendingRef.current = true setPendingContent(content) - setOpen(false) + if (focusTriggerAfterChange) { + addReactionButtonRef.current?.focus() + } try { - await onReactionChange(content, reacted) + const changed = await onReactionChange(content, reacted) + if (changed && closePicker) { + setOpen(false) + } } finally { + mutationPendingRef.current = false setPendingContent(null) } } @@ -76,7 +91,7 @@ export function CommentReactions({ type="button" variant="outline" size="xs" - disabled={pendingContent !== null} + aria-disabled={pendingContent !== null} className={cn( 'h-6 gap-1 rounded-full px-2 text-[12px] font-normal', reaction.viewerHasReacted && 'border-ring bg-accent text-accent-foreground' @@ -91,7 +106,14 @@ export function CommentReactions({ value2: reaction.count === 1 ? '' : 's' } )} - onClick={() => void changeReaction(reaction.content, !reaction.viewerHasReacted)} + onClick={() => + void changeReaction( + reaction.content, + !reaction.viewerHasReacted, + false, + Boolean(reaction.viewerHasReacted && reaction.count === 1) + ) + } > {reaction.count} @@ -121,10 +143,11 @@ export function CommentReactions({ diff --git a/src/renderer/src/components/right-sidebar/ChecksPanel.tsx b/src/renderer/src/components/right-sidebar/ChecksPanel.tsx index 805a125d565..9fc66a78819 100644 --- a/src/renderer/src/components/right-sidebar/ChecksPanel.tsx +++ b/src/renderer/src/components/right-sidebar/ChecksPanel.tsx @@ -2945,10 +2945,14 @@ export default function ChecksPanel(): React.JSX.Element { ) const handleSetReaction = useCallback( - async (comment: PRComment, content: GitHubReactionContent, reacted: boolean): Promise => { + async ( + comment: PRComment, + content: GitHubReactionContent, + reacted: boolean + ): Promise => { const reactionSubjectId = comment.reactionSubjectId if (!repo || !prNumber || !pr?.prRepo || !reactionSubjectId) { - return + return false } const requestKey = checksPanelAsyncResultKey( prCacheKey, @@ -2968,7 +2972,7 @@ export default function ChecksPanel(): React.JSX.Element { { repoId: repo.id, prRepo: pr.prRepo } ) if (!isCurrentAsyncResult(requestKey) || ok) { - return + return ok } setComments((current) => restoreReactionOnSubject(current, reactionSubjectId, content, previousReaction) @@ -2979,6 +2983,7 @@ export default function ChecksPanel(): React.JSX.Element { 'Failed to update reaction.' ) ) + return false }, [branch, isCurrentAsyncResult, pr, prCacheKey, prNumber, repo, setPRCommentReaction] ) diff --git a/src/renderer/src/components/right-sidebar/checks-panel-content.tsx b/src/renderer/src/components/right-sidebar/checks-panel-content.tsx index dda1c3ee4e1..2b2b695b189 100644 --- a/src/renderer/src/components/right-sidebar/checks-panel-content.tsx +++ b/src/renderer/src/components/right-sidebar/checks-panel-content.tsx @@ -1756,7 +1756,7 @@ function CommentRow({ comment: PRComment, content: GitHubReactionContent, reacted: boolean - ) => Promise + ) => Promise onQueueForAgent?: () => void }): React.JSX.Element { const automated = isBotPRComment(comment, botAuthorOverrides) @@ -2070,7 +2070,7 @@ function PRCommentGroupView({ comment: PRComment, content: GitHubReactionContent, reacted: boolean - ) => Promise + ) => Promise onQueueForAgent?: () => void }): React.JSX.Element { // Reply targets a specific comment id so any comment in a thread — root or @@ -2219,7 +2219,7 @@ function ResolvedCommentGroupsSection({ comment: PRComment, content: GitHubReactionContent, reacted: boolean - ) => Promise + ) => Promise }): React.JSX.Element | null { if (groups.length === 0) { return null @@ -2344,7 +2344,7 @@ export function PRCommentsList({ comment: PRComment, content: GitHubReactionContent, reacted: boolean - ) => Promise + ) => Promise }): React.JSX.Element { const presentation = React.useMemo(() => getPRCommentPresentationClasses(), []) const [commentFilter, setCommentFilter] = useState('all') diff --git a/tests/e2e/helpers/pr-comments-sidebar-fixture.ts b/tests/e2e/helpers/pr-comments-sidebar-fixture.ts index b44aaa84c3c..3fc047ec1bf 100644 --- a/tests/e2e/helpers/pr-comments-sidebar-fixture.ts +++ b/tests/e2e/helpers/pr-comments-sidebar-fixture.ts @@ -10,7 +10,7 @@ export type PRCommentsSidebarSeed = { export const FIXTURE_COMMENTS: PRComment[] = [ { id: 101, - author: 'alice', + author: 'coderabbitai', authorAvatarUrl: '', body: 'Please update this handler before merge.', createdAt: '2026-05-14T10:00:00.000Z', @@ -18,6 +18,7 @@ export const FIXTURE_COMMENTS: PRComment[] = [ reactionSubjectId: 'PRRC_101', threadId: 'thread-open', path: 'src/handler.ts', + isBot: true, isResolved: false }, { diff --git a/tests/e2e/pr-comments-sidebar-cards.spec.ts b/tests/e2e/pr-comments-sidebar-cards.spec.ts index ee95b3f74cb..d0cfa4498c2 100644 --- a/tests/e2e/pr-comments-sidebar-cards.spec.ts +++ b/tests/e2e/pr-comments-sidebar-cards.spec.ts @@ -57,7 +57,7 @@ test.describe('PR comments sidebar cards view', () => { await expect(orcaPage.getByText('Needs review · 1')).toBeVisible() await expect(orcaPage.getByText('Please update this handler before merge.')).toBeVisible() - await expect(orcaPage.getByText('alice')).toBeVisible() + await expect(orcaPage.getByText('coderabbitai')).toBeVisible() await expect(orcaPage.getByText('LGTM on the overall approach.')).toBeVisible() const openThreadCard = orcaPage.getByTestId('pr-comment-group').filter({ @@ -75,7 +75,7 @@ test.describe('PR comments sidebar cards view', () => { 'Please update this handler before merge.', 'LGTM on the overall approach.' ) - await expectOpenTextNotShiftedLeft(openThreadCard, conversationCard, 'alice', 'bob') + await expectOpenTextNotShiftedLeft(openThreadCard, conversationCard, 'coderabbitai', 'bob') const resolvedTrigger = orcaPage.getByRole('button', { name: 'Resolved · 1' }) await expect(resolvedTrigger).toBeVisible() @@ -124,37 +124,106 @@ test.describe('PR comments sidebar cards view', () => { expect(positions[1]).toBeLessThan(positions[2]) }) - test('adds reactions to conversation and review-thread comments', async ({ orcaPage }) => { + test('adds reactions to conversation and review-thread comments', async ({ + orcaPage + }, testInfo) => { const { worktreeId } = await seedPRCommentsSidebarFixture(orcaPage) await openChecks(orcaPage, worktreeId) await expect(orcaPage.getByText('Needs review · 1')).toBeVisible({ timeout: 10_000 }) + const reviewThreadCard = orcaPage.getByTestId('pr-comment-group').filter({ + hasText: 'Please update this handler before merge.' + }) + const threadReactionButton = reviewThreadCard.getByRole('button', { name: 'Add reaction' }) + await orcaPage.screenshot({ path: testInfo.outputPath('reaction-before.png') }) + await threadReactionButton.click() + await expect(orcaPage.getByRole('group', { name: 'Add reaction' })).toBeFocused() + await orcaPage.waitForTimeout(300) + await orcaPage.screenshot({ path: testInfo.outputPath('reaction-picker.png') }) + await orcaPage.getByRole('button', { name: 'Add rocket reaction' }).click() + await expect(orcaPage.getByRole('group', { name: 'Add reaction' })).toBeHidden() + const selectedRocket = reviewThreadCard.getByRole('button', { name: '1 rocket reaction' }) + await expect(selectedRocket).toHaveAttribute('aria-pressed', 'true') + await selectedRocket.focus() + await orcaPage.waitForTimeout(300) + await orcaPage.screenshot({ path: testInfo.outputPath('reaction-after.png') }) + await selectedRocket.press('Enter') + await expect(selectedRocket).toHaveCount(0) + await expect(threadReactionButton).toBeFocused() + const conversationCard = orcaPage.getByTestId('pr-comment-group').filter({ hasText: 'LGTM on the overall approach.' }) const conversationReactionButton = conversationCard.getByRole('button', { name: 'Add reaction' }) - await conversationReactionButton.evaluate((element) => (element as HTMLElement).click()) - const heartReactionButton = orcaPage.getByRole('button', { name: 'Add heart reaction' }) + await conversationReactionButton.click() + const conversationPicker = orcaPage.getByRole('group', { name: 'Add reaction' }).last() + const heartReactionButton = conversationPicker.getByRole('button', { + name: 'Add heart reaction' + }) await expect(heartReactionButton).toBeVisible() await heartReactionButton.evaluate((element) => (element as HTMLElement).click()) await expect( conversationCard.getByRole('button', { name: '1 heart reaction' }) ).toHaveAttribute('aria-pressed', 'true') await expect(orcaPage.getByRole('button', { name: 'Add rocket reaction' })).toHaveCount(0) + }) + + test('keeps reaction focus while a remote mutation fails', async ({ orcaPage }) => { + const { worktreeId } = await seedPRCommentsSidebarFixture(orcaPage) + await openChecks(orcaPage, worktreeId) + await expect(orcaPage.getByText('Needs review · 1')).toBeVisible({ timeout: 10_000 }) + await orcaPage.evaluate(() => { + window.__store?.setState({ + setPRCommentReaction: async () => { + await new Promise((resolve) => window.setTimeout(resolve, 300)) + return false + } + }) + }) const reviewThreadCard = orcaPage.getByTestId('pr-comment-group').filter({ hasText: 'Please update this handler before merge.' }) - const threadReactionButton = reviewThreadCard.getByRole('button', { name: 'Add reaction' }) - await threadReactionButton.evaluate((element) => (element as HTMLElement).click()) - await orcaPage - .getByRole('button', { name: 'Add rocket reaction' }) - .evaluate((element) => (element as HTMLElement).click()) - await expect( - reviewThreadCard.getByRole('button', { name: '1 rocket reaction' }) - ).toHaveAttribute('aria-pressed', 'true') + const addReaction = reviewThreadCard.getByRole('button', { name: 'Add reaction' }) + await addReaction.focus() + await addReaction.press('Enter') + const picker = orcaPage.getByRole('group', { name: 'Add reaction' }) + await expect(picker).toBeFocused() + const rocket = picker.getByRole('button', { name: /rocket reaction/ }) + await rocket.focus() + await rocket.press('Enter') + await expect(rocket).toBeFocused() + await expect(rocket).toHaveAttribute('aria-disabled', 'true') + await rocket.press('Enter') + await expect(picker).toBeVisible() + await expect(rocket).toHaveAttribute('aria-disabled', 'false') + await expect(rocket).toBeFocused() + await expect(rocket).toHaveAccessibleName('Add rocket reaction') + + await orcaPage.evaluate(() => { + window.__store?.setState({ setPRCommentReaction: async () => true }) + }) + await rocket.press('Enter') + const selectedRocket = reviewThreadCard.getByRole('button', { name: '1 rocket reaction' }) + await expect(selectedRocket).toHaveAttribute('aria-pressed', 'true') + await orcaPage.evaluate(() => { + window.__store?.setState({ + setPRCommentReaction: async () => { + await new Promise((resolve) => window.setTimeout(resolve, 300)) + return false + } + }) + }) + await selectedRocket.focus() + await selectedRocket.press('Enter') + await expect(addReaction).toBeFocused() + await expect(addReaction).toHaveAttribute('aria-disabled', 'true') + await expect(selectedRocket).toHaveCount(0) + await expect(selectedRocket).toHaveCount(1) + await expect(addReaction).toHaveAttribute('aria-disabled', 'false') + await expect(addReaction).toBeFocused() }) test('queues an open thread for the agent from the visible row action and menu fallback', async ({