xiaoxue_admin
/
orca
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Fix PR comment selection persistence leak (#7688) 

Bound the PR/MR queued-comment selection cache to 1024 LRU contexts and make cache recency commit-safe so long renderer sessions cannot retain unbounded unsent selections.
This commit is contained in:
Neil 2026-07-11 14:03:14 -07:00 committed by GitHub
parent 19b63c4cb4
commit d8a9da7351
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 218 additions and 48 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

143
src/renderer/src/components/right-sidebar/pr-comments-list-selection.test.tsx
View File

@ -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>): PRComment {
@ -83,20 +89,47 @@ function comment(overrides: Partial<PRComment>): PRComment {
function renderList(props: {
comments: PRComment[]
contextKey?: string
strictMode?: boolean
onResolveSelectedCommentsWithAI?: (groups: PRCommentGroup[]) => void
clearRequest?: PRCommentsListSelectionClearRequest | null
}): void {
const list = (
<TooltipProvider>
<PRCommentsList
comments={props.comments}
commentsLoading={false}
selectionContextKey={props.contextKey ?? 'review:42'}
selectionClearRequest={props.clearRequest}
onResolveSelectedCommentsWithAI={props.onResolveSelectedCommentsWithAI ?? vi.fn()}
/>
</TooltipProvider>
)
act(() => {
root.render(props.strictMode ? <StrictMode>{list}</StrictMode> : list)
})
}
const neverSettles = new Promise<void>(() => {})
function SuspendForever(): ReactNode {
throw neverSettles
}
function renderAbandonedList(comments: PRComment[], contextKey: string): void {
act(() => {
root.render(
<TooltipProvider>
<PRCommentsList
comments={props.comments}
commentsLoading={false}
selectionContextKey={props.contextKey ?? 'review:42'}
selectionClearRequest={props.clearRequest}
onResolveSelectedCommentsWithAI={props.onResolveSelectedCommentsWithAI ?? vi.fn()}
/>
</TooltipProvider>
<Suspense fallback={<div>Loading review</div>}>
<TooltipProvider>
<PRCommentsList
key={`abandoned:${contextKey}`}
comments={comments}
commentsLoading={false}
selectionContextKey={contextKey}
onResolveSelectedCommentsWithAI={vi.fn()}
/>
<SuspendForever />
</TooltipProvider>
</Suspense>
)
})
}
@ -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<HTMLButtonElement>(
'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)
})
})

123
src/renderer/src/components/right-sidebar/pr-comments-list-selection.ts
View File

@ -26,26 +26,53 @@ type PRCommentsListSelectionState = {
}
const EMPTY_SELECTED_GROUP_IDS = new Set<string>()
// 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<string> }
>()
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<string>,
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<number | null>(null)
const [selectionState, setSelectionState] = useState<PRCommentsListSelectionState>(() =>
createSelectionState(selectionContextKey)
)
const [renderedSelectionState, setRenderedSelectionState] =
useState<PRCommentsListSelectionState>(() => 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<string>()
}
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<string>()
}
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 {