From 6f5d5442c3f4d790437e8d526bcb8ceb5cd0e56e Mon Sep 17 00:00:00 2001 From: Jinwoo-H Date: Mon, 10 Aug 2026 00:48:54 -0400 Subject: [PATCH] fix(github): harden comment reaction updates --- .../github/client-pr-local-runtime.test.ts | 18 ++++++-- src/main/github/client.ts | 2 + src/main/github/issues.test.ts | 6 ++- src/main/github/issues.ts | 2 + .../components/right-sidebar/ChecksPanel.tsx | 10 ++--- .../src/lib/pr-comment-reactions.test.ts | 24 +++++++++- src/renderer/src/lib/pr-comment-reactions.ts | 44 ++++++++++++++++--- src/renderer/src/store/slices/github.test.ts | 28 ++++++++++-- src/renderer/src/store/slices/github.ts | 14 +++--- .../helpers/source-control-ai-generation.ts | 20 ++++++--- 10 files changed, 134 insertions(+), 34 deletions(-) diff --git a/src/main/github/client-pr-local-runtime.test.ts b/src/main/github/client-pr-local-runtime.test.ts index a2aa0595c74..0e7d4013d2e 100644 --- a/src/main/github/client-pr-local-runtime.test.ts +++ b/src/main/github/client-pr-local-runtime.test.ts @@ -225,10 +225,14 @@ describe('GitHub PR local runtime routing', () => { return { stdout: '[]' } } if (endpoint.endsWith('/pulls/7/comments/11/replies')) { - return { stdout: JSON.stringify({ id: 12, user: null, body: 'Reply' }) } + return { + stdout: JSON.stringify({ id: 12, node_id: 'PRRC_reply_12', user: null, body: 'Reply' }) + } } if (endpoint.endsWith('/pulls/7/comments')) { - return { stdout: JSON.stringify({ id: 13, user: null, body: 'Inline' }) } + return { + stdout: JSON.stringify({ id: 13, node_id: 'PRRC_inline_13', user: null, body: 'Inline' }) + } } return { stdout: '', stderr: '' } }) @@ -250,7 +254,10 @@ describe('GitHub PR local runtime routing', () => { prRepo, localGitOptions ) - ).resolves.toMatchObject({ ok: true }) + ).resolves.toMatchObject({ + ok: true, + comment: { reactionSubjectId: 'PRRC_reply_12' } + }) await expect( addPRReviewComment({ repoPath: '/repo-root', @@ -263,7 +270,10 @@ describe('GitHub PR local runtime routing', () => { path: 'src/app.ts', line: 10 }) - ).resolves.toMatchObject({ ok: true }) + ).resolves.toMatchObject({ + ok: true, + comment: { reactionSubjectId: 'PRRC_inline_13' } + }) await expect( updatePRTitle('/repo-root', 7, 'New title', null, prRepo, localGitOptions) ).resolves.toBe(true) diff --git a/src/main/github/client.ts b/src/main/github/client.ts index c1a9d0e3891..46ea1180e24 100644 --- a/src/main/github/client.ts +++ b/src/main/github/client.ts @@ -4561,6 +4561,7 @@ export async function resolveReviewThread( function mapReviewCommentResponse( data: { id?: number + node_id?: string | null user: { login: string; avatar_url: string; type?: string } | null body?: string created_at?: string @@ -4576,6 +4577,7 @@ function mapReviewCommentResponse( ): PRComment { return { id: data.id ?? Date.now(), + reactionSubjectId: data.node_id?.trim() || undefined, author: data.user?.login ?? 'You', authorAvatarUrl: data.user?.avatar_url ?? '', body: data.body ?? body, diff --git a/src/main/github/issues.test.ts b/src/main/github/issues.test.ts index e9b8a10282c..23a7242810c 100644 --- a/src/main/github/issues.test.ts +++ b/src/main/github/issues.test.ts @@ -195,6 +195,7 @@ describe('issue source operations', () => { ghExecFileAsyncMock.mockResolvedValueOnce({ stdout: JSON.stringify({ id: 9, + node_id: 'IC_enterprise_9', user: { login: 'octo', avatar_url: '', type: 'User' }, body: 'Enterprise comment', created_at: '2026-07-16T00:00:00.000Z', @@ -208,7 +209,10 @@ describe('issue source operations', () => { repo: 'orca', host: 'github.acme-corp.com' }) - ).resolves.toMatchObject({ ok: true }) + ).resolves.toMatchObject({ + ok: true, + comment: { reactionSubjectId: 'IC_enterprise_9' } + }) expect(ghExecFileAsyncMock).toHaveBeenCalledWith( [ diff --git a/src/main/github/issues.ts b/src/main/github/issues.ts index 3ffa6dc662a..e2a4dd52677 100644 --- a/src/main/github/issues.ts +++ b/src/main/github/issues.ts @@ -473,6 +473,7 @@ export async function addIssueComment( ) const data = JSON.parse(stdout) as { id?: number + node_id?: string | null user: { login: string; avatar_url: string; type?: string } | null body?: string created_at?: string @@ -483,6 +484,7 @@ export async function addIssueComment( } const comment: PRComment = { id: data.id, + reactionSubjectId: data.node_id?.trim() || undefined, author: data.user?.login ?? 'You', authorAvatarUrl: data.user?.avatar_url ?? '', body: data.body ?? body, diff --git a/src/renderer/src/components/right-sidebar/ChecksPanel.tsx b/src/renderer/src/components/right-sidebar/ChecksPanel.tsx index 44abe773e62..805a125d565 100644 --- a/src/renderer/src/components/right-sidebar/ChecksPanel.tsx +++ b/src/renderer/src/components/right-sidebar/ChecksPanel.tsx @@ -25,7 +25,7 @@ import { useActiveWorktree, useRepoById } from '@/store/selectors' import { useChecksPanelTerminalWorktree } from './use-checks-panel-terminal-worktree' import { cn } from '@/lib/utils' import { openHttpLink } from '@/lib/http-link-routing' -import { setReactionOnSubject } from '@/lib/pr-comment-reactions' +import { restoreReactionOnSubject, setReactionOnSubject } from '@/lib/pr-comment-reactions' import { Button } from '@/components/ui/button' import { DetachedHeadBadge } from '@/components/DetachedHeadBadge' import { @@ -2957,7 +2957,7 @@ export default function ChecksPanel(): React.JSX.Element { pr.prRepo, pr.headSha ) - const previousReactions = comment.reactions + const previousReaction = comment.reactions?.find((reaction) => reaction.content === content) setComments((current) => setReactionOnSubject(current, reactionSubjectId, content, reacted)) const ok = await setPRCommentReaction( repo.path, @@ -2971,11 +2971,7 @@ export default function ChecksPanel(): React.JSX.Element { return } setComments((current) => - current.map((entry) => - entry.reactionSubjectId === reactionSubjectId - ? { ...entry, reactions: previousReactions } - : entry - ) + restoreReactionOnSubject(current, reactionSubjectId, content, previousReaction) ) toast.error( translate( diff --git a/src/renderer/src/lib/pr-comment-reactions.test.ts b/src/renderer/src/lib/pr-comment-reactions.test.ts index 5e5b69fab20..d742fde85d8 100644 --- a/src/renderer/src/lib/pr-comment-reactions.test.ts +++ b/src/renderer/src/lib/pr-comment-reactions.test.ts @@ -1,6 +1,10 @@ import { describe, expect, it } from 'vitest' import type { PRComment } from '../../../shared/types' -import { setCommentReaction, setReactionOnSubject } from './pr-comment-reactions' +import { + restoreCommentReaction, + setCommentReaction, + setReactionOnSubject +} from './pr-comment-reactions' function comment(overrides: Partial = {}): PRComment { return { @@ -46,4 +50,22 @@ describe('setCommentReaction', () => { expect(result[0].reactions).toEqual([{ content: 'eyes', count: 1, viewerHasReacted: true }]) expect(result[1]).toBe(other) }) + + it('restores only the failed reaction while preserving concurrent updates', () => { + const result = restoreCommentReaction( + comment({ + reactions: [ + { content: 'heart', count: 3, viewerHasReacted: true }, + { content: 'eyes', count: 1, viewerHasReacted: false } + ] + }), + 'heart', + { content: 'heart', count: 2, viewerHasReacted: false } + ) + + expect(result.reactions).toEqual([ + { content: 'heart', count: 2, viewerHasReacted: false }, + { content: 'eyes', count: 1, viewerHasReacted: false } + ]) + }) }) diff --git a/src/renderer/src/lib/pr-comment-reactions.ts b/src/renderer/src/lib/pr-comment-reactions.ts index 80f67303456..f124f082d17 100644 --- a/src/renderer/src/lib/pr-comment-reactions.ts +++ b/src/renderer/src/lib/pr-comment-reactions.ts @@ -11,6 +11,13 @@ export const GITHUB_REACTION_ORDER: readonly GitHubReactionContent[] = [ 'eyes' ] +function sortReactions(reactions: GitHubReaction[]): GitHubReaction[] { + return reactions.sort( + (left, right) => + GITHUB_REACTION_ORDER.indexOf(left.content) - GITHUB_REACTION_ORDER.indexOf(right.content) + ) +} + export function setCommentReaction( comment: PRComment, content: GitHubReactionContent, @@ -28,17 +35,27 @@ export function setCommentReaction( count: nextCount, viewerHasReacted: reacted } - const nextReactions = reactions - .filter((reaction) => reaction.content !== content) - .concat(nextCount > 0 ? nextReaction : []) - .sort( - (left, right) => - GITHUB_REACTION_ORDER.indexOf(left.content) - GITHUB_REACTION_ORDER.indexOf(right.content) - ) + const nextReactions = sortReactions( + reactions + .filter((reaction) => reaction.content !== content) + .concat(nextCount > 0 ? nextReaction : []) + ) return { ...comment, reactions: nextReactions.length > 0 ? nextReactions : undefined } } +export function restoreCommentReaction( + comment: PRComment, + content: GitHubReactionContent, + previousReaction?: GitHubReaction +): PRComment { + const reactions = sortReactions([ + ...(comment.reactions ?? []).filter((reaction) => reaction.content !== content), + ...(previousReaction ? [previousReaction] : []) + ]) + return { ...comment, reactions: reactions.length > 0 ? reactions : undefined } +} + export function setReactionOnSubject( comments: readonly PRComment[], reactionSubjectId: string, @@ -51,3 +68,16 @@ export function setReactionOnSubject( : comment ) } + +export function restoreReactionOnSubject( + comments: readonly PRComment[], + reactionSubjectId: string, + content: GitHubReactionContent, + previousReaction?: GitHubReaction +): PRComment[] { + return comments.map((comment) => + comment.reactionSubjectId === reactionSubjectId + ? restoreCommentReaction(comment, content, previousReaction) + : comment + ) +} diff --git a/src/renderer/src/store/slices/github.test.ts b/src/renderer/src/store/slices/github.test.ts index 05866e6ec71..cadbc5acb8f 100644 --- a/src/renderer/src/store/slices/github.test.ts +++ b/src/renderer/src/store/slices/github.test.ts @@ -1713,7 +1713,10 @@ describe('createGitHubSlice PR comment mutations', () => { }) it('rolls back an optimistic reaction when GitHub rejects it', async () => { - mockApi.gh.setPRCommentReaction.mockResolvedValueOnce(false) + let resolveReaction!: (ok: boolean) => void + mockApi.gh.setPRCommentReaction.mockImplementationOnce( + () => new Promise((resolve) => (resolveReaction = resolve)) + ) const store = createTestStore() const repoPath = '/repo' const repoId = 'repo-id' @@ -1739,14 +1742,33 @@ describe('createGitHubSlice PR comment mutations', () => { } } as unknown as Partial) - const ok = await store.getState().setPRCommentReaction(repoPath, 12, 'IC_10', 'heart', true, { + const pending = store.getState().setPRCommentReaction(repoPath, 12, 'IC_10', 'heart', true, { repoId, prRepo: { owner: 'Acme', repo: 'Widgets' } }) + const optimisticEntry = store.getState().commentsCache[cacheKey] + store.setState({ + commentsCache: { + ...store.getState().commentsCache, + [cacheKey]: { + ...optimisticEntry, + data: (optimisticEntry?.data ?? []).map((comment) => ({ + ...comment, + reactions: [ + ...(comment.reactions ?? []), + { content: 'eyes' as const, count: 1, viewerHasReacted: false } + ] + })) + } + } + }) + resolveReaction(false) + const ok = await pending expect(ok).toBe(false) expect(store.getState().commentsCache[cacheKey]?.data?.[0].reactions).toEqual([ - { content: 'heart', count: 2, viewerHasReacted: false } + { content: 'heart', count: 2, viewerHasReacted: false }, + { content: 'eyes', count: 1, viewerHasReacted: false } ]) }) diff --git a/src/renderer/src/store/slices/github.ts b/src/renderer/src/store/slices/github.ts index d1d0ebe886c..21b66f2e9bd 100644 --- a/src/renderer/src/store/slices/github.ts +++ b/src/renderer/src/store/slices/github.ts @@ -75,7 +75,7 @@ import { type TaskSourceContext } from '../../../../shared/task-source-context' import { normalizeGitHubPRForBranchOutcome } from '../../../../shared/github-pr-for-branch-outcome' -import { setReactionOnSubject } from '@/lib/pr-comment-reactions' +import { restoreReactionOnSubject, setReactionOnSubject } from '@/lib/pr-comment-reactions' // ─── ProjectV2 cache types ──────────────────────────────────────────── // Why: separate from CacheEntry — project-view has a single GraphQL source (no issue/PR fallback) and a distinct error union. @@ -3879,6 +3879,9 @@ export const createGitHubSlice: StateCreator = (s const previousComment = get().commentsCache[cacheKey]?.data?.find( (comment) => comment.reactionSubjectId === reactionSubjectId ) + const previousReaction = previousComment?.reactions?.find( + (reaction) => reaction.content === content + ) set((state) => { const entry = state.commentsCache[cacheKey] if (!entry?.data) { @@ -3941,10 +3944,11 @@ export const createGitHubSlice: StateCreator = (s ...state.commentsCache, [cacheKey]: { ...entry, - data: entry.data.map((comment) => - comment.reactionSubjectId === reactionSubjectId - ? { ...comment, reactions: previousComment.reactions } - : comment + data: restoreReactionOnSubject( + entry.data, + reactionSubjectId, + content, + previousReaction ) } } diff --git a/tests/e2e/helpers/source-control-ai-generation.ts b/tests/e2e/helpers/source-control-ai-generation.ts index c408c6c4a6a..149f6a019d3 100644 --- a/tests/e2e/helpers/source-control-ai-generation.ts +++ b/tests/e2e/helpers/source-control-ai-generation.ts @@ -31,22 +31,30 @@ export async function openChecks(page: Page, worktreeId: string): Promise const state = window.__store?.getState() state?.setActiveWorktree(targetWorktreeId) state?.setRightSidebarOpen(true) - state?.setRightSidebarTab('checks') }, worktreeId) await expect .poll( () => page.evaluate((targetWorktreeId) => { const state = window.__store?.getState() - return ( - state?.activeWorktreeId === targetWorktreeId && - state.rightSidebarOpen && - state.rightSidebarTab === 'checks' - ) + return state?.activeWorktreeId === targetWorktreeId && state.rightSidebarOpen }, worktreeId), { timeout: 5_000 } ) .toBe(true) + const checksButton = page.getByRole('button', { name: 'Checks', exact: true }) + await expect + .poll( + async () => { + if ((await page.evaluate(() => window.__store?.getState().rightSidebarTab)) !== 'checks') { + await checksButton.click() + } + await page.waitForTimeout(250) + return page.evaluate(() => window.__store?.getState().rightSidebarTab) + }, + { timeout: 10_000 } + ) + .toBe('checks') } export async function seedCreatePrComposer(page: Page): Promise<{