From 8182141fcdc8eb0c65481cfd88338c44be251915 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Mon, 20 Jul 2026 19:14:27 -0700 Subject: [PATCH] perf(sidebar): stabilize worktree card props so cards memo-bail on order-preserving epoch bumps (#9392) * perf(sidebar): stable per-group onLineageToggle identity so lineage-parent WorktreeCards memo-bail Co-authored-by: Orca * perf(sidebar): reuse array identity for visible/rendered/selected worktrees so cards memo-bail on epoch bumps Co-authored-by: Orca --------- Co-authored-by: Orca --- .../WorktreeList.card-memo-stability.test.tsx | 323 ++++++++++++++++++ .../src/components/sidebar/WorktreeList.tsx | 66 ++-- .../sidebar/worktree-agent-row-selectors.ts | 5 +- ...ktree-lineage-toggle-handler-cache.test.ts | 48 +++ .../worktree-lineage-toggle-handler-cache.ts | 26 ++ 5 files changed, 447 insertions(+), 21 deletions(-) create mode 100644 src/renderer/src/components/sidebar/WorktreeList.card-memo-stability.test.tsx create mode 100644 src/renderer/src/components/sidebar/worktree-lineage-toggle-handler-cache.test.ts create mode 100644 src/renderer/src/components/sidebar/worktree-lineage-toggle-handler-cache.ts diff --git a/src/renderer/src/components/sidebar/WorktreeList.card-memo-stability.test.tsx b/src/renderer/src/components/sidebar/WorktreeList.card-memo-stability.test.tsx new file mode 100644 index 000000000..9d0a2e369 --- /dev/null +++ b/src/renderer/src/components/sidebar/WorktreeList.card-memo-stability.test.tsx @@ -0,0 +1,323 @@ +// @vitest-environment happy-dom + +import { act, type ReactNode } from 'react' +import { createRoot, type Root } from 'react-dom/client' +import { afterEach, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest' +import type { Repo, Worktree, WorktreeCardProperty } from '../../../../shared/types' + +globalThis.IS_REACT_ACT_ENVIRONMENT = true + +const mockStore = vi.hoisted(() => ({ + state: {} as Record, + activateWorktreeFromSidebar: vi.fn(), + openModal: vi.fn() +})) + +// Counts invocations of the memo'd card's inner render function. A bail-out +// (React.memo shallow-equal props) does NOT invoke it — which is the claim +// under test: order-preserving epoch bumps must not re-render cards. +const cardRenderSpy = vi.hoisted(() => vi.fn()) + +type WorktreeListComponent = React.ComponentType<{ + scrollOffsetRef: React.RefObject + scrollAnchorRef: React.RefObject +}> + +let WorktreeList: WorktreeListComponent + +vi.mock('@/store', () => { + const useAppStore = ((selector: (state: Record) => unknown) => + selector(mockStore.state)) as (( + selector: (state: Record) => unknown + ) => unknown) & { + getState: () => Record + } + useAppStore.getState = () => mockStore.state + return { useAppStore } +}) + +vi.mock('@tanstack/react-virtual', () => ({ + defaultRangeExtractor: ({ startIndex, endIndex }: { startIndex: number; endIndex: number }) => + Array.from({ length: endIndex - startIndex + 1 }, (_, index) => startIndex + index), + measureElement: () => 32, + useVirtualizer: ({ count }: { count: number }) => ({ + elementsCache: new Map(), + getTotalSize: () => count * 96, + getVirtualItems: () => + Array.from({ length: count }, (_, index) => ({ + index, + key: `row-${index}`, + start: index * 96 + })), + measureElement: vi.fn(), + scrollToIndex: vi.fn() + }) +})) + +vi.mock('@/hooks/useVirtualizedScrollAnchor', () => ({ + VIRTUALIZED_SCROLL_ANCHOR_RECORD_EVENT: 'orca:test-record-scroll-anchor', + useVirtualizedScrollAnchor: vi.fn() +})) + +vi.mock('./project-header-drag', () => ({ + useRepoHeaderDrag: () => ({ + state: { draggingRepoId: null, dropIndicatorY: null }, + onHandlePointerDown: vi.fn() + }), + isRepoHeaderActionTarget: () => false +})) + +vi.mock('@/components/ui/hover-card', () => ({ + HoverCard: ({ children }: { children: ReactNode }) => <>{children}, + HoverCardContent: ({ children }: { children: ReactNode }) =>
{children}
, + HoverCardTrigger: ({ children }: { children: ReactNode }) => <>{children} +})) + +vi.mock('@/components/ui/tooltip', () => ({ + Tooltip: ({ children }: { children: ReactNode }) => <>{children}, + TooltipContent: ({ children }: { children: ReactNode }) => <>{children}, + TooltipTrigger: ({ children }: { children: ReactNode }) => <>{children} +})) + +vi.mock('@/components/ui/dropdown-menu', () => ({ + DropdownMenu: ({ children }: { children: ReactNode }) => <>{children}, + DropdownMenuContent: ({ children }: { children: ReactNode }) =>
{children}
, + DropdownMenuItem: ({ children, onSelect }: { children: ReactNode; onSelect?: () => void }) => ( + + ), + DropdownMenuSeparator: () =>
, + DropdownMenuSub: ({ children }: { children: ReactNode }) => <>{children}, + DropdownMenuSubContent: ({ children }: { children: ReactNode }) =>
{children}
, + DropdownMenuSubTrigger: ({ children }: { children: ReactNode }) =>
{children}
, + DropdownMenuTrigger: ({ children }: { children: ReactNode }) => <>{children} +})) + +vi.mock('@/lib/sidebar-worktree-activation', () => ({ + activateWorktreeFromSidebar: mockStore.activateWorktreeFromSidebar +})) + +vi.mock('@/lib/worktree-activation', () => ({ + activateAndRevealWorktree: vi.fn() +})) + +vi.mock('@/runtime/runtime-rpc-client', () => ({ + getActiveRuntimeTarget: () => ({ kind: 'local' }), + callRuntimeRpc: vi.fn() +})) + +vi.mock('./WorktreeCardAgents', () => ({ + default: () =>
Agent row
, + SUPPRESS_WORKTREE_LIST_SCROLL_ADJUSTMENT_EVENT: 'orca:test-suppress-scroll-adjustment' +})) + +vi.mock('./WorktreeCard', async () => { + const ReactModule = await import('react') + const MockWorktreeCard = ReactModule.memo(function WorktreeCard({ + worktree + }: { + worktree: Worktree + }) { + cardRenderSpy(worktree.id) + return
+ }) + return { default: MockWorktreeCard } +}) + +function makeRepo(): Repo { + return { + id: 'repo-1', + path: '/tmp/card-memo-stability', + displayName: 'card-memo-stability', + badgeColor: '#999999', + addedAt: 1 + } +} + +function makeWorktree(args: { id: string; displayName: string; sortOrder: number }): Worktree { + return { + id: args.id, + instanceId: `${args.id}-instance`, + repoId: 'repo-1', + path: `/tmp/card-memo-stability/${args.id}`, + displayName: args.displayName, + branch: `${args.id}-branch`, + head: 'abc123', + isBare: false, + isMainWorktree: false, + comment: '', + linkedIssue: null, + linkedPR: null, + linkedLinearIssue: null, + isArchived: false, + isUnread: false, + isPinned: false, + sortOrder: args.sortOrder, + lastActivityAt: args.sortOrder + } +} + +function makeFolderWorkspacePathStatusState(): Record { + return { + fetchFolderWorkspacePathStatus: vi.fn(), + folderWorkspacePathStatuses: {}, + folderWorkspaces: [], + getFolderWorkspacePathStatusCacheKey: (request: unknown) => JSON.stringify(request), + getFreshFolderWorkspacePathStatus: vi.fn(() => null) + } +} + +function setFlatWorktreeState(): void { + const repo = makeRepo() + const worktrees = [ + makeWorktree({ id: 'wt-a', displayName: 'alpha', sortOrder: 20 }), + makeWorktree({ id: 'wt-b', displayName: 'beta', sortOrder: 10 }) + ] + mockStore.state = { + ...makeFolderWorkspacePathStatusState(), + activeModal: '', + activeView: 'terminal', + activeWorktreeId: null, + agentStatusByPaneKey: {}, + agentStatusEpoch: 0, + browserTabsByWorktree: {}, + clearPendingRevealWorktreeId: vi.fn(), + collapsedGroups: new Set(), + deleteStateByWorktreeId: {}, + detectedWorktreesByRepo: {}, + fetchHostedReviewForBranch: vi.fn(), + fetchIssue: vi.fn(), + fetchLinearIssue: vi.fn(), + filterRepoIds: [], + gitConflictOperationByWorktree: {}, + groupBy: 'none', + hideDefaultBranchWorkspace: false, + hostedReviewCache: {}, + issueCache: {}, + linearIssueCache: {}, + linearStatus: null, + migrationUnsupportedByPtyId: {}, + openModal: mockStore.openModal, + openSettingsPage: vi.fn(), + openSettingsTarget: null, + openTaskPage: vi.fn(), + pendingRevealWorktree: null, + prCache: {}, + projectGroups: [], + ptyIdsByTabId: {}, + recordFeatureInteraction: vi.fn(), + remoteBranchConflictByWorktreeId: {}, + reorderRepos: vi.fn(), + reportVisibleGitHubPRRefreshCandidates: vi.fn(), + repos: [repo], + retainedAgentsByPaneKey: {}, + revealWorktreeInSidebar: vi.fn(), + runtimePaneTitlesByTabId: {}, + setFilterRepoIds: vi.fn(), + setHideDefaultBranchWorkspace: vi.fn(), + setRenamingWorktreeId: vi.fn(), + setShowSleepingWorkspaces: vi.fn(), + setSortBy: vi.fn(), + setWorktreesPinnedAndReveal: vi.fn(), + settings: null, + showSleepingWorkspaces: true, + sortBy: 'manual', + sortEpoch: 0, + sshConnectedGeneration: 0, + sshConnectionStates: new Map(), + sshTargetLabels: new Map(), + tabsByWorktree: {}, + terminalLayoutsByTabId: {}, + toggleCollapsedGroup: vi.fn(), + updateRepo: vi.fn(), + updateWorktreeMeta: vi.fn(), + updateWorktreesMeta: vi.fn(), + workspaceHostScope: 'all', + workspacePortScan: null, + workspaceStatuses: [], + worktreeCardProperties: ['status', 'pr', 'comment'] satisfies WorktreeCardProperty[], + worktreeLineageById: {}, + worktreesByRepo: { + [repo.id]: worktrees + } + } +} + +const mountedRoots: Root[] = [] + +async function renderList(root: Root): Promise { + await act(async () => { + // Why fresh ref objects each render: they defeat WorktreeList's own memo + // like a store-subscription re-render would, so the test exercises a full + // parent re-render and isolates whether the CARDS bail. + root.render( + + ) + }) +} + +describe('WorktreeCard memo bail-out across epoch bumps', () => { + beforeAll(async () => { + WorktreeList = (await import('./WorktreeList')).default as WorktreeListComponent + }, 60_000) + + beforeEach(() => { + vi.clearAllMocks() + setFlatWorktreeState() + }) + + afterEach(async () => { + await act(async () => { + for (const root of mountedRoots.splice(0)) { + root.unmount() + } + }) + document.body.innerHTML = '' + }) + + it('does not re-render cards on an order-preserving sortEpoch bump', async () => { + const container = document.createElement('div') + document.body.appendChild(container) + const root = createRoot(container) + mountedRoots.push(root) + + await renderList(root) + expect(container.querySelectorAll('[data-mock-worktree-card]')).toHaveLength(2) + + // Baseline: a parent re-render with unchanged store state must not + // re-invoke card render functions. + const countAfterMount = cardRenderSpy.mock.calls.length + await renderList(root) + expect(cardRenderSpy.mock.calls.length).toBe(countAfterMount) + + // Order-preserving epoch bump: same worktrees, same order, new epoch. + // Manual sort applies the bump without the smart-sort settle debounce. + mockStore.state = { ...mockStore.state, sortEpoch: 1 } + await renderList(root) + + expect(cardRenderSpy.mock.calls.length).toBe(countAfterMount) + }) + + it('re-renders a card when its own worktree data changes', async () => { + const container = document.createElement('div') + document.body.appendChild(container) + const root = createRoot(container) + mountedRoots.push(root) + + await renderList(root) + const countAfterMount = cardRenderSpy.mock.calls.length + + const worktrees = (mockStore.state.worktreesByRepo as Record)['repo-1']! + mockStore.state = { + ...mockStore.state, + sortEpoch: 2, + worktreesByRepo: { + 'repo-1': [{ ...worktrees[0]!, displayName: 'alpha renamed' }, worktrees[1]!] + } + } + await renderList(root) + + // The changed card re-renders; identity reuse must not freeze real updates. + expect(cardRenderSpy.mock.calls.length).toBeGreaterThan(countAfterMount) + expect(cardRenderSpy).toHaveBeenCalledWith('wt-a') + }) +}) diff --git a/src/renderer/src/components/sidebar/WorktreeList.tsx b/src/renderer/src/components/sidebar/WorktreeList.tsx index a71753eaa..fcc71abb9 100644 --- a/src/renderer/src/components/sidebar/WorktreeList.tsx +++ b/src/renderer/src/components/sidebar/WorktreeList.tsx @@ -24,6 +24,8 @@ import { Trash2 } from 'lucide-react' import { useAppStore } from '@/store' +import { createLineageToggleHandlerCache } from './worktree-lineage-toggle-handler-cache' +import { reuseArrayIfEqual } from './worktree-agent-row-selectors' import { useShallow } from 'zustand/react/shallow' import type { AppState } from '@/store/types' import { @@ -312,6 +314,14 @@ type ProjectGroupDeleteDialogState = { removeContainedProjects: boolean } +// Why: epoch-driven recomputes often produce arrays whose contents and order are unchanged; reusing the previous identity when element-wise equal keeps downstream memos and React.memo'd cards bailing out. Safe only because elements (Worktree objects / id strings) are immutably REPLACED on change — never wrap arrays of mutated-in-place objects. +function useReusedArrayIdentity(next: T[]): T[] { + const previousRef = useRef(next) + const result = reuseArrayIfEqual(previousRef.current, next) + previousRef.current = result + return result +} + // Debounce re-sort after a sortEpoch bump so background score changes don't jar row positions. const SORT_SETTLE_MS = 3_000 const USER_SCROLL_MEASUREMENT_ADJUSTMENT_SUPPRESS_MS = 500 @@ -2449,6 +2459,12 @@ const VirtualizedWorktreeViewport = React.memo(function VirtualizedWorktreeViewp }, [recordCurrentScrollAnchor, toggleGroup] ) + // Why: memo'd WorktreeCard needs a per-group-key stable onLineageToggle + // identity to bail out of re-renders; see worktree-lineage-toggle-handler-cache. + const getLineageToggleHandler = useMemo( + () => createLineageToggleHandlerCache(toggleGroupWithScrollAnchor), + [toggleGroupWithScrollAnchor] + ) const navigateWorktree = useCallback( (direction: 'up' | 'down') => { @@ -4856,11 +4872,7 @@ const VirtualizedWorktreeViewport = React.memo(function VirtualizedWorktreeViewp lineageChildrenStyle={lineageChildrenStyle} onLineageToggle={ lineageToggleGroupKey - ? (event) => { - event.preventDefault() - event.stopPropagation() - toggleGroupWithScrollAnchor(lineageToggleGroupKey) - } + ? getLineageToggleHandler(lineageToggleGroupKey) : undefined } /> @@ -5462,7 +5474,7 @@ const WorktreeList = React.memo(function WorktreeList({ }, [sortedIds, sortBy]) // Flatten/filter/sort via the shared utility so card order matches Cmd+1–9 numbering. - const visibleWorktrees = useMemo(() => { + const recomputedVisibleWorktrees = useMemo(() => { void agentStatusEpoch const ids = computeVisibleWorktreeIds(worktreesByRepo, sortedIds, { filterRepoIds, @@ -5514,6 +5526,10 @@ const WorktreeList = React.memo(function WorktreeList({ worktreeLineageById, worktreesByRepo ]) + // Why: agentStatusEpoch bumps recompute this memo even when membership and + // order are unchanged; keeping the previous identity stops the whole + // rows/sectionRows/renderedWorktrees chain from churning per epoch. + const visibleWorktrees = useReusedArrayIdentity(recomputedVisibleWorktrees) const worktrees = visibleWorktrees const collapsedGroups = useAppStore((s) => s.collapsedGroups) @@ -5842,9 +5858,14 @@ const WorktreeList = React.memo(function WorktreeList({ () => getRenderedWorktreesInSidebarOrder(sectionRows, pinnedDisplayPolicy), [pinnedDisplayPolicy, sectionRows] ) - const renderedWorktreeIds = useMemo( - () => uniqueWorktreeIds(renderedWorktrees.map((worktree) => worktree.id)), - [renderedWorktrees] + // Why: order-preserving sectionRows rebuilds must not give this array a new + // identity — updateSelectionForGesture depends on it, and a fresh identity + // there defeats React.memo bail-out for every WorktreeCard on epoch bumps. + const renderedWorktreeIds = useReusedArrayIdentity( + useMemo( + () => uniqueWorktreeIds(renderedWorktrees.map((worktree) => worktree.id)), + [renderedWorktrees] + ) ) const [selectedWorktreeIds, setSelectedWorktreeIds] = useState>(new Set()) const [selectionAnchorId, setSelectionAnchorId] = useState(null) @@ -5862,18 +5883,23 @@ const WorktreeList = React.memo(function WorktreeList({ setSelectionAnchorId(prunedSelection.anchorId) } - const selectedWorktrees = useMemo(() => { - if (selectedWorktreeIds.size === 0) { - return [] - } - const selected = new Map() - for (const worktree of renderedWorktrees) { - if (selectedWorktreeIds.has(worktree.id) && !selected.has(worktree.id)) { - selected.set(worktree.id, worktree) + // Why identity reuse: the empty/unchanged-selection case must keep one array + // identity — selectForContextMenu and both drag-start handlers depend on + // this array, and card memo bail-out depends on those staying stable. + const selectedWorktrees = useReusedArrayIdentity( + useMemo(() => { + if (selectedWorktreeIds.size === 0) { + return [] } - } - return Array.from(selected.values()) - }, [renderedWorktrees, selectedWorktreeIds]) + const selected = new Map() + for (const worktree of renderedWorktrees) { + if (selectedWorktreeIds.has(worktree.id) && !selected.has(worktree.id)) { + selected.set(worktree.id, worktree) + } + } + return Array.from(selected.values()) + }, [renderedWorktrees, selectedWorktreeIds]) + ) useEffect(() => { if (selectedWorktreeIds.size === 0) { diff --git a/src/renderer/src/components/sidebar/worktree-agent-row-selectors.ts b/src/renderer/src/components/sidebar/worktree-agent-row-selectors.ts index 35ff6853b..f964d0241 100644 --- a/src/renderer/src/components/sidebar/worktree-agent-row-selectors.ts +++ b/src/renderer/src/components/sidebar/worktree-agent-row-selectors.ts @@ -51,7 +51,10 @@ let liveEntriesByWorktreeCache: LiveEntriesByWorktreeCache | null = null let migrationUnsupportedByWorktreeCache: MigrationUnsupportedByWorktreeCache | null = null let retainedEntriesByWorktreeCache: RetainedEntriesByWorktreeCache | null = null -function reuseArrayIfEqual(previous: T[] | undefined, next: T[]): T[] { +// Why exported: WorktreeList reuses this exact-equality identity check to keep +// derived arrays referentially stable across order-preserving epoch bumps so +// memo'd cards can bail out of re-render. +export function reuseArrayIfEqual(previous: T[] | undefined, next: T[]): T[] { if (!previous || previous.length !== next.length) { return next } diff --git a/src/renderer/src/components/sidebar/worktree-lineage-toggle-handler-cache.test.ts b/src/renderer/src/components/sidebar/worktree-lineage-toggle-handler-cache.test.ts new file mode 100644 index 000000000..670560092 --- /dev/null +++ b/src/renderer/src/components/sidebar/worktree-lineage-toggle-handler-cache.test.ts @@ -0,0 +1,48 @@ +import { describe, expect, it, vi } from 'vitest' +import type React from 'react' +import { createLineageToggleHandlerCache } from './worktree-lineage-toggle-handler-cache' + +const makeEvent = (): React.MouseEvent => + ({ + preventDefault: vi.fn(), + stopPropagation: vi.fn() + }) as unknown as React.MouseEvent + +describe('createLineageToggleHandlerCache', () => { + it('returns a referentially stable handler for the same group key across calls', () => { + const getHandler = createLineageToggleHandlerCache(vi.fn()) + + const first = getHandler('lineage:alpha') + const second = getHandler('lineage:alpha') + + // Why: stable identity is what lets React.memo'd WorktreeCard bail out. + expect(second).toBe(first) + }) + + it('returns distinct handlers for distinct group keys', () => { + const getHandler = createLineageToggleHandlerCache(vi.fn()) + + expect(getHandler('lineage:alpha')).not.toBe(getHandler('lineage:beta')) + }) + + it('prevents default, stops propagation, and toggles the bound group key', () => { + const toggleGroup = vi.fn() + const getHandler = createLineageToggleHandlerCache(toggleGroup) + const event = makeEvent() + + getHandler('lineage:alpha')(event) + + expect(event.preventDefault).toHaveBeenCalledTimes(1) + expect(event.stopPropagation).toHaveBeenCalledTimes(1) + expect(toggleGroup).toHaveBeenCalledExactlyOnceWith('lineage:alpha') + }) + + it('keeps each cached handler bound to its own group key', () => { + const toggleGroup = vi.fn() + const getHandler = createLineageToggleHandlerCache(toggleGroup) + + getHandler('lineage:beta')(makeEvent()) + + expect(toggleGroup).toHaveBeenCalledExactlyOnceWith('lineage:beta') + }) +}) diff --git a/src/renderer/src/components/sidebar/worktree-lineage-toggle-handler-cache.ts b/src/renderer/src/components/sidebar/worktree-lineage-toggle-handler-cache.ts new file mode 100644 index 000000000..de9012f41 --- /dev/null +++ b/src/renderer/src/components/sidebar/worktree-lineage-toggle-handler-cache.ts @@ -0,0 +1,26 @@ +import type React from 'react' + +export type LineageToggleHandler = (event: React.MouseEvent) => void + +// Why: WorktreeCard is React.memo'd; an inline arrow per row would give +// onLineageToggle a fresh identity every render and defeat the memo bail-out +// for every lineage-parent card on each sort/status epoch bump. Cache one +// handler per group key (bounded by lineage-group count) so identity is stable. +export const createLineageToggleHandlerCache = ( + toggleGroup: (groupKey: string) => void +): ((groupKey: string) => LineageToggleHandler) => { + const handlersByGroupKey = new Map() + return (groupKey: string) => { + const cached = handlersByGroupKey.get(groupKey) + if (cached) { + return cached + } + const handler: LineageToggleHandler = (event) => { + event.preventDefault() + event.stopPropagation() + toggleGroup(groupKey) + } + handlersByGroupKey.set(groupKey, handler) + return handler + } +}