diff --git a/src/renderer/src/components/right-sidebar/pr-comments-list-selection.test.tsx b/src/renderer/src/components/right-sidebar/pr-comments-list-selection.test.tsx index ec89aa872..a7b9af215 100644 --- a/src/renderer/src/components/right-sidebar/pr-comments-list-selection.test.tsx +++ b/src/renderer/src/components/right-sidebar/pr-comments-list-selection.test.tsx @@ -1,6 +1,8 @@ // @vitest-environment happy-dom -import { act, type ReactNode } from 'react' +globalThis.IS_REACT_ACT_ENVIRONMENT = true + +import { act, StrictMode, Suspense, type ReactNode } from 'react' import { createRoot, type Root } from 'react-dom/client' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { TooltipProvider } from '@/components/ui/tooltip' @@ -46,7 +48,10 @@ vi.mock('@/components/ui/dropdown-menu', () => ({ import type { PRComment } from '../../../../shared/types' import type { PRCommentGroup } from '@/lib/pr-comment-groups' import { - clearPRCommentsListSelection, + MAX_PERSISTED_PR_COMMENTS_LIST_SELECTIONS, + clearPRCommentsListSelectionsForTests, + getPRCommentsListSelectionCountForTests, + seedPRCommentsListSelectionForTests, type PRCommentsListSelectionClearRequest } from './pr-comments-list-selection' import { PRCommentsList } from './checks-panel-content' @@ -55,7 +60,7 @@ let container: HTMLDivElement let root: Root beforeEach(() => { - clearPRCommentsListSelection('review:42') + clearPRCommentsListSelectionsForTests() container = document.createElement('div') document.body.appendChild(container) root = createRoot(container) @@ -66,6 +71,7 @@ afterEach(() => { root.unmount() }) container.remove() + clearPRCommentsListSelectionsForTests() }) function comment(overrides: Partial): PRComment { @@ -83,20 +89,47 @@ function comment(overrides: Partial): PRComment { function renderList(props: { comments: PRComment[] contextKey?: string + strictMode?: boolean onResolveSelectedCommentsWithAI?: (groups: PRCommentGroup[]) => void clearRequest?: PRCommentsListSelectionClearRequest | null }): void { + const list = ( + + + + ) + act(() => { + root.render(props.strictMode ? {list} : list) + }) +} + +const neverSettles = new Promise(() => {}) + +function SuspendForever(): ReactNode { + throw neverSettles +} + +function renderAbandonedList(comments: PRComment[], contextKey: string): void { act(() => { root.render( - - - + Loading review}> + + + + + ) }) } @@ -303,6 +336,23 @@ describe('PRCommentsList comment resolution selection', () => { expect(container.querySelector('button[role="checkbox"]')).toBeNull() }) + it('drops an empty selection from the persisted context cache', () => { + renderList({ + comments: [comment({ id: 1, threadId: 'thread-1', path: 'src/a.ts', isResolved: false })] + }) + clickButton('Queue for agent') + + const selectedCheckbox = container.querySelector( + 'button[role="checkbox"][aria-checked="true"]' + ) + expect(selectedCheckbox).not.toBeNull() + act(() => { + selectedCheckbox?.dispatchEvent(new MouseEvent('click', { bubbles: true })) + }) + + expect(getPRCommentsListSelectionCountForTests()).toBe(0) + }) + it('clears sent standalone bot comments from the queue when the parent confirms launch', () => { const comments = [ comment({ @@ -378,5 +428,74 @@ describe('PRCommentsList comment resolution selection', () => { }) expect(hasButton('Send 1 queued comments to AI')).toBe(false) + expect(getPRCommentsListSelectionCountForTests()).toBe(0) + }) + + it('keeps GitHub and GitLab review selections isolated under Strict Mode replay', () => { + const comments = [comment({ id: 1, threadId: 'thread-1', path: 'src/a.ts' })] + const githubContext = 'local::repo::branch::github::42::github-head' + const gitlabContext = 'local::repo::branch::gitlab::42::gitlab-head' + + renderList({ comments, contextKey: githubContext, strictMode: true }) + clickButton('Queue for agent') + renderList({ comments, contextKey: gitlabContext, strictMode: true }) + clickButton('Queue for agent') + + expect(getPRCommentsListSelectionCountForTests()).toBe(2) + renderList({ comments, contextKey: githubContext, strictMode: true }) + expect(hasButton('Send 1 queued comments to AI')).toBe(true) + renderList({ comments, contextKey: gitlabContext, strictMode: true }) + expect(hasButton('Send 1 queued comments to AI')).toBe(true) + }) + + it('does not refresh LRU recency for an abandoned Suspense render', () => { + const comments = [comment({ id: 1, threadId: 'thread-1', path: 'src/a.ts' })] + const queuedGroupIds = ['thread:thread-1'] as const + const seedContext = (contextKey: string): void => { + seedPRCommentsListSelectionForTests(contextKey, queuedGroupIds) + } + + seedContext('review:oldest') + for (let i = 0; i < MAX_PERSISTED_PR_COMMENTS_LIST_SELECTIONS - 1; i += 1) { + seedContext(`review:recent-${i}`) + } + + renderAbandonedList(comments, 'review:oldest') + expect(container.textContent).toContain('Loading review') + seedContext('review:new') + + renderList({ comments, contextKey: 'review:oldest' }) + expect(hasButton('Send 1 queued comments to AI')).toBe(false) + renderList({ comments, contextKey: 'review:recent-0' }) + expect(hasButton('Send 1 queued comments to AI')).toBe(true) + }) + + it('bounds persisted review contexts while retaining recently restored selections', () => { + const comments = [comment({ id: 1, threadId: 'thread-1', path: 'src/a.ts', isResolved: false })] + const queuedGroupIds = ['thread:thread-1'] as const + const seedContext = (contextKey: string): void => { + seedPRCommentsListSelectionForTests(contextKey, queuedGroupIds) + } + + seedContext('review:keep') + for (let i = 0; i < MAX_PERSISTED_PR_COMMENTS_LIST_SELECTIONS - 1; i += 1) { + seedContext(`review:stale-${i}`) + } + + // Why: committed remount must refresh LRU recency for the restored context. + renderList({ comments, contextKey: 'review:keep' }) + expect(hasButton('Send 1 queued comments to AI')).toBe(true) + + seedContext('review:new') + + expect(getPRCommentsListSelectionCountForTests()).toBe( + MAX_PERSISTED_PR_COMMENTS_LIST_SELECTIONS + ) + + renderList({ comments, contextKey: 'review:stale-0' }) + expect(hasButton('Send 1 queued comments to AI')).toBe(false) + + renderList({ comments, contextKey: 'review:keep' }) + expect(hasButton('Send 1 queued comments to AI')).toBe(true) }) }) diff --git a/src/renderer/src/components/right-sidebar/pr-comments-list-selection.ts b/src/renderer/src/components/right-sidebar/pr-comments-list-selection.ts index 71b51ad5b..6767e405c 100644 --- a/src/renderer/src/components/right-sidebar/pr-comments-list-selection.ts +++ b/src/renderer/src/components/right-sidebar/pr-comments-list-selection.ts @@ -26,26 +26,53 @@ type PRCommentsListSelectionState = { } const EMPTY_SELECTED_GROUP_IDS = new Set() +// Why: queued selections need to survive sidebar remounts, but old PR/MR +// contexts can disappear without another clear signal in a long renderer run. +export const MAX_PERSISTED_PR_COMMENTS_LIST_SELECTIONS = 1024 const persistedSelectionByContextKey = new Map< string, { isSelectingForAI: boolean; selectedGroupIds: Set } >() +function trimPersistedSelectionContexts(): void { + while (persistedSelectionByContextKey.size > MAX_PERSISTED_PR_COMMENTS_LIST_SELECTIONS) { + const oldestContextKey = persistedSelectionByContextKey.keys().next().value + if (oldestContextKey === undefined) { + break + } + persistedSelectionByContextKey.delete(oldestContextKey) + } +} + function persistSelectionState(state: PRCommentsListSelectionState): void { if (!state.contextKey) { return } - if (!state.isSelectingForAI && state.selectedGroupIds.size === 0) { + if (state.selectedGroupIds.size === 0) { persistedSelectionByContextKey.delete(state.contextKey) return } + persistedSelectionByContextKey.delete(state.contextKey) persistedSelectionByContextKey.set(state.contextKey, { isSelectingForAI: state.isSelectingForAI, selectedGroupIds: new Set(state.selectedGroupIds) }) + trimPersistedSelectionContexts() } -function createSelectionState(contextKey: string | undefined): PRCommentsListSelectionState { +function refreshPersistedSelectionContext(contextKey: string | undefined): void { + if (!contextKey) { + return + } + const persisted = persistedSelectionByContextKey.get(contextKey) + if (!persisted) { + return + } + persistedSelectionByContextKey.delete(contextKey) + persistedSelectionByContextKey.set(contextKey, persisted) +} + +function readSelectionState(contextKey: string | undefined): PRCommentsListSelectionState { const persisted = contextKey ? persistedSelectionByContextKey.get(contextKey) : undefined return { contextKey, @@ -60,20 +87,48 @@ export function clearPRCommentsListSelection(contextKey: string | undefined): vo } } +export function clearPRCommentsListSelectionsForTests(): void { + persistedSelectionByContextKey.clear() +} + +export function getPRCommentsListSelectionCountForTests(): number { + return persistedSelectionByContextKey.size +} + +// Why: bound/LRU tests need to fill the cache without 1024 React mounts per case. +export function seedPRCommentsListSelectionForTests( + contextKey: string, + selectedGroupIds: Iterable, + isSelectingForAI = true +): void { + persistSelectionState({ + contextKey, + isSelectingForAI, + selectedGroupIds: new Set(selectedGroupIds) + }) +} + export function usePRCommentsListSelection( comments: PRComment[], selectionContextKey: string | undefined, clearRequest?: PRCommentsListSelectionClearRequest | null ): PRCommentsListSelection { const lastClearRequestTokenRef = useRef(null) - const [selectionState, setSelectionState] = useState(() => - createSelectionState(selectionContextKey) - ) + const [renderedSelectionState, setRenderedSelectionState] = + useState(() => readSelectionState(selectionContextKey)) + const selectionState = + renderedSelectionState.contextKey === selectionContextKey + ? renderedSelectionState + : readSelectionState(selectionContextKey) + const commitSelectionState = useCallback((next: PRCommentsListSelectionState): void => { + persistSelectionState(next) + setRenderedSelectionState(next) + }, []) useEffect(() => { - setSelectionState((prev) => - prev.contextKey === selectionContextKey ? prev : createSelectionState(selectionContextKey) - ) + // Why: only a committed context may affect LRU order; render can be + // abandoned or replayed by Strict Mode/Suspense. + refreshPersistedSelectionContext(selectionContextKey) }, [selectionContextKey]) useEffect(() => { @@ -90,9 +145,8 @@ export function usePRCommentsListSelection( isSelectingForAI: false, selectedGroupIds: new Set() } - persistSelectionState(next) - setSelectionState(next) - }, [clearRequest, selectionContextKey]) + commitSelectionState(next) + }, [clearRequest, commitSelectionState, selectionContextKey]) // Why: selectable groups come from the unfiltered list so switching the // audience filter doesn't silently drop already-selected comments. @@ -138,10 +192,10 @@ export function usePRCommentsListSelection( isSelectingForAI: selectionState.isSelectingForAI, selectedGroupIds: new Set(selectedGroupIds) } - persistSelectionState(next) - setSelectionState(next) + commitSelectionState(next) }, [ candidateSelectedGroupIds, + commitSelectionState, comments.length, isCurrentSelectionContext, selectedGroupIds, @@ -169,10 +223,9 @@ export function usePRCommentsListSelection( isSelectingForAI: true, selectedGroupIds: new Set([groupId]) } - persistSelectionState(next) - setSelectionState(next) + commitSelectionState(next) }, - [selectableGroupsById, selectionContextKey] + [commitSelectionState, selectableGroupsById, selectionContextKey] ) const clearSelection = useCallback((): void => { @@ -181,34 +234,32 @@ export function usePRCommentsListSelection( isSelectingForAI: false, selectedGroupIds: new Set() } - persistSelectionState(next) - setSelectionState(next) - }, [selectionContextKey]) + commitSelectionState(next) + }, [commitSelectionState, selectionContextKey]) const toggleGroupSelection = useCallback( (groupId: string, checked: boolean): void => { if (!selectableGroupsById.has(groupId)) { return } - setSelectionState((prev) => { - const base = - prev.contextKey === selectionContextKey ? prev.selectedGroupIds : EMPTY_SELECTED_GROUP_IDS - const next = new Set([...base].filter((id) => selectableGroupsById.has(id))) - if (checked) { - next.add(groupId) - } else { - next.delete(groupId) - } - const nextState = { - contextKey: selectionContextKey, - isSelectingForAI: true, - selectedGroupIds: next - } - persistSelectionState(nextState) - return nextState + const current = readSelectionState(selectionContextKey) + const base = + current.contextKey === selectionContextKey + ? current.selectedGroupIds + : EMPTY_SELECTED_GROUP_IDS + const next = new Set([...base].filter((id) => selectableGroupsById.has(id))) + if (checked) { + next.add(groupId) + } else { + next.delete(groupId) + } + commitSelectionState({ + contextKey: selectionContextKey, + isSelectingForAI: true, + selectedGroupIds: next }) }, - [selectableGroupsById, selectionContextKey] + [commitSelectionState, selectableGroupsById, selectionContextKey] ) return {