From 5b54bd94a0d522d5c19b9f0b03833f3fa4b532df Mon Sep 17 00:00:00 2001 From: brennanb2025 Date: Mon, 6 Jul 2026 18:02:34 -0700 Subject: [PATCH] perf(source-control): virtualize the changed-files list The Source Control panel mounted every non-collapsed file row, so large changesets (big refactors, lockfiles, branch compare with hundreds of entries) created hundreds to thousands of DOM rows and re-rendered them all on every 3s git-status refresh (`STA-351`, `STA-1280`). Window each per-section list (uncommitted tree/list, committed-on-branch tree/list) with @tanstack/react-virtual against the panel's shared scroller, extracted into source-control-virtual-file-list.tsx. Sections under 50 rows keep the exact pre-virtualization DOM so small changesets see zero change; larger sections mount only viewport + overscan rows. Fixed-height estimates with element measurement keep identical refreshes from moving the scroll position, and stable row keys carry item identity across status polls. Co-authored-by: Orca --- .../right-sidebar/SourceControl.tsx | 54 ++- .../SourceControl.virtual-file-list.test.tsx | 430 ++++++++++++++++++ .../source-control-virtual-file-list.tsx | 106 +++++ 3 files changed, 580 insertions(+), 10 deletions(-) create mode 100644 src/renderer/src/components/right-sidebar/SourceControl.virtual-file-list.test.tsx create mode 100644 src/renderer/src/components/right-sidebar/source-control-virtual-file-list.tsx diff --git a/src/renderer/src/components/right-sidebar/SourceControl.tsx b/src/renderer/src/components/right-sidebar/SourceControl.tsx index b097fab1358..fae797dfacb 100644 --- a/src/renderer/src/components/right-sidebar/SourceControl.tsx +++ b/src/renderer/src/components/right-sidebar/SourceControl.tsx @@ -108,6 +108,7 @@ import { type SourceControlEntryGroups, type SourceControlSectionArea } from './source-control-section-order' +import { SourceControlVirtualFileList } from './source-control-virtual-file-list' import { buildActiveOpenFileSignature, buildActiveOpenRowKeys @@ -795,6 +796,10 @@ export function clearRemoteActionErrorsForCompletedConflictOperations({ function SourceControlInner(): React.JSX.Element { const sourceControlRef = useRef(null) + // Why: the changed-file sections virtualize against the panel's shared + // scroller rather than owning a nested scroll container. State (not a ref) + // so the lists re-render and start observing once the element attaches. + const [fileListScrollElement, setFileListScrollElement] = useState(null) const isMac = useMemo(() => navigator.userAgent.includes('Mac'), []) const pendingCommentEditorRevealFrameIdsRef = useRef([]) // Why: React setState is async, so a rapid double-click on the Commit @@ -5627,6 +5632,7 @@ function SourceControlInner(): React.JSX.Element { )}
0 ? 50 : undefined }} > @@ -5957,8 +5963,12 @@ function SourceControlInner(): React.JSX.Element { } /> {!isCollapsed && - (sourceControlViewMode === 'tree' - ? (visibleTreeRowsBySection[id] ?? []).map((node) => { + (sourceControlViewMode === 'tree' ? ( + node.key} + renderRow={(node) => { if (node.type === 'submodule-placeholder') { return ( ) - }) - : (visibleListRowsBySection[id] ?? []).map((row) => { + }} + /> + ) : ( + + row.type === 'submodule-placeholder' + ? row.key + : `${row.entry.area}::${row.entry.path}` + } + renderRow={(row) => { if (row.type === 'submodule-placeholder') { return ( ) - }))} + }} + /> + ))}
) })} @@ -6117,8 +6139,12 @@ function SourceControlInner(): React.JSX.Element { } /> {!collapsedSections.has('branch') && - (sourceControlViewMode === 'tree' - ? visibleBranchTreeRows.map((node) => { + (sourceControlViewMode === 'tree' ? ( + node.key} + renderRow={(node) => { if (node.type === 'directory') { return ( ) - }) - : filteredBranchEntries.map((entry) => ( + }} + /> + ) : ( + `branch:${entry.path}`} + renderRow={(entry) => ( openCommittedDiff(entry, event)} commentCount={diffCommentCountByPath.get(entry.path) ?? 0} /> - )))} + )} + /> + ))} )} diff --git a/src/renderer/src/components/right-sidebar/SourceControl.virtual-file-list.test.tsx b/src/renderer/src/components/right-sidebar/SourceControl.virtual-file-list.test.tsx new file mode 100644 index 00000000000..2dc5f0bc474 --- /dev/null +++ b/src/renderer/src/components/right-sidebar/SourceControl.virtual-file-list.test.tsx @@ -0,0 +1,430 @@ +// @vitest-environment happy-dom + +import { act } 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' +import type { GitStatusEntry } from '../../../../shared/types' +import SourceControl from './SourceControl' +import { + SOURCE_CONTROL_FILE_ROW_HEIGHT_PX, + SOURCE_CONTROL_FILE_ROW_OVERSCAN, + SOURCE_CONTROL_VIRTUALIZE_MIN_ROWS +} from './source-control-virtual-file-list' + +const mocks = vi.hoisted(() => { + const activeRepo = { + id: 'repo-1', + path: '/repo', + displayName: 'Repo', + badgeColor: '#000', + addedAt: 0 + } + const activeWorktree = { + id: 'wt-1', + repoId: 'repo-1', + path: '/repo/wt', + head: 'abcdef123', + branch: 'refs/heads/feature/virtual-list', + isBare: false, + isMainWorktree: false, + displayName: 'feature/virtual-list', + comment: '', + linkedIssue: null, + linkedPR: null, + linkedLinearIssue: null, + linkedGitLabMR: null, + linkedGitLabIssue: null, + isArchived: false, + isUnread: false, + isPinned: false, + sortOrder: 0, + lastActivityAt: 0 + } + return { + activeRepo, + activeWorktree, + state: {} as Record + } +}) + +vi.mock('@/store', () => { + const useAppStore = Object.assign( + (selector?: (state: Record) => unknown) => + selector ? selector(mocks.state) : mocks.state, + { + getState: () => mocks.state + } + ) + return { useAppStore } +}) + +vi.mock('@/store/selectors', () => ({ + useActiveWorktree: () => mocks.activeWorktree, + useRepoById: (repoId: string | null) => + repoId === mocks.activeRepo.id ? mocks.activeRepo : null, + useWorktreeMap: () => new Map([[mocks.activeWorktree.id, mocks.activeWorktree]]) +})) + +vi.mock('@/components/confirmation-dialog', () => ({ + useConfirmationDialog: () => vi.fn().mockResolvedValue(true) +})) + +vi.mock('./git-status-refresh', () => ({ + refreshGitStatusForWorktree: vi.fn().mockResolvedValue(undefined) +})) + +const VIEWPORT_HEIGHT_PX = 600 +// Rows the viewport can show plus overscan on both edges plus the partial +// rows clipped at each edge of the window. +const MAX_MOUNTED_ROWS = + Math.ceil(VIEWPORT_HEIGHT_PX / SOURCE_CONTROL_FILE_ROW_HEIGHT_PX) + + 2 * SOURCE_CONTROL_FILE_ROW_OVERSCAN + + 2 + +function gitEntry(overrides: Partial): GitStatusEntry { + return { + path: 'src/file.ts', + area: 'unstaged', + status: 'modified', + added: 1, + removed: 0, + ...overrides + } +} + +function manyEntries(count: number): GitStatusEntry[] { + return Array.from({ length: count }, (_, index) => + gitEntry({ path: `src/file-${String(index).padStart(3, '0')}.ts` }) + ) +} + +function noopAsync(value: unknown = undefined): () => Promise { + return vi.fn().mockResolvedValue(value) +} + +function resetState(overrides: Partial> = {}): void { + vi.clearAllMocks() + mocks.state = { + activeWorktreeId: mocks.activeWorktree.id, + activeGroupIdByWorktree: { [mocks.activeWorktree.id]: 'group-1' }, + groupsByWorktree: { [mocks.activeWorktree.id]: [{ id: 'group-1', activeTabId: null }] }, + repos: [mocks.activeRepo], + worktreesByRepo: { [mocks.activeRepo.id]: [mocks.activeWorktree] }, + rightSidebarOpen: false, + rightSidebarTab: 'source-control', + gitStatusByWorktree: { [mocks.activeWorktree.id]: [] }, + gitBranchChangesByWorktree: { [mocks.activeWorktree.id]: [] }, + gitBranchCompareSummaryByWorktree: { [mocks.activeWorktree.id]: null }, + gitConflictOperationByWorktree: {}, + remoteStatusesByWorktree: {}, + isRemoteOperationActive: false, + inFlightRemoteOpKind: null, + settings: null, + hostedReviewCache: {}, + prCache: {}, + commitMessageGenerationRecords: {}, + pullRequestGenerationRecords: {}, + openFiles: [], + activeFileIdByWorktree: {}, + activeTabTypeByWorktree: {}, + getDiffComments: vi.fn(() => []), + updateSettings: noopAsync(), + openSettingsTarget: vi.fn(), + openSettingsPage: vi.fn(), + fetchHostedReviewForBranch: noopAsync(), + getHostedReviewCreationEligibility: noopAsync(null), + createHostedReview: noopAsync({ ok: false, error: 'not available' }), + updateWorktreeMeta: noopAsync(), + fetchPRForBranch: noopAsync(), + enqueueGitHubPRRefresh: vi.fn(), + updateRepo: noopAsync(), + setGitStatus: vi.fn(), + updateWorktreeGitIdentity: vi.fn(), + beginGitBranchCompareRequest: vi.fn(() => 'request-key'), + setGitBranchCompareResult: vi.fn(), + clearGitBranchCompare: vi.fn(), + fetchUpstreamStatus: noopAsync(), + setUpstreamStatus: vi.fn(), + pushBranch: noopAsync(), + pullBranch: noopAsync(), + fastForwardBranch: noopAsync(), + syncBranch: noopAsync(), + rebaseFromBase: noopAsync(), + fetchBranch: noopAsync(), + revealInExplorer: vi.fn(), + trackConflictPath: vi.fn(), + openDiff: vi.fn(), + openFile: vi.fn(), + setEditorViewMode: vi.fn(), + setMarkdownViewMode: vi.fn(), + setPendingEditorReveal: vi.fn(), + openConflictFile: vi.fn(), + openConflictReview: vi.fn(), + openBranchDiff: vi.fn(), + createEmptySplitGroup: vi.fn(() => 'group-2'), + openAllDiffs: vi.fn(), + openBranchAllDiffs: vi.fn(), + openCommitAllDiffs: vi.fn(), + deleteDiffComment: noopAsync(true), + clearDiffComments: noopAsync(true), + clearDiffCommentsForFile: noopAsync(true), + setScrollToDiffCommentId: vi.fn(), + setRightSidebarOpen: vi.fn(), + setRightSidebarTab: vi.fn(), + allocateCommitMessageGenerationRequestId: vi.fn(() => 'commit-generation-1'), + setCommitMessageGenerationRecord: vi.fn(), + updateCommitMessageGenerationRecord: vi.fn(), + pruneCommitMessageGenerationRecords: vi.fn(), + allocatePullRequestGenerationRequestId: vi.fn(() => 'pr-generation-1'), + setPullRequestGenerationRecord: vi.fn(), + updatePullRequestGenerationRecord: vi.fn(), + prunePullRequestGenerationRecords: vi.fn(), + ...overrides + } +} + +let container: HTMLDivElement +let root: Root + +class NoopResizeObserver { + observe(): void {} + unobserve(): void {} + disconnect(): void {} +} + +beforeEach(() => { + globalThis.IS_REACT_ACT_ENVIRONMENT = true + resetState() + container = document.createElement('div') + document.body.appendChild(container) + root = createRoot(container) + + // Why: happy-dom has no layout. Give the virtualizer a 600px scroll viewport + // and 24px rows (both rect + row measurement read offsetHeight), and keep + // the observer path inert so measurements stay deterministic. + vi.stubGlobal('ResizeObserver', NoopResizeObserver) + vi.spyOn(HTMLElement.prototype, 'offsetHeight', 'get').mockImplementation( + function (this: HTMLElement) { + return this.classList.contains('overflow-auto') + ? VIEWPORT_HEIGHT_PX + : SOURCE_CONTROL_FILE_ROW_HEIGHT_PX + } + ) + vi.spyOn(Element.prototype, 'getBoundingClientRect').mockImplementation(function (this: Element) { + // The panel scroller acts as the fixed viewport; everything inside it + // shifts up by scrollTop, which is what the list's scroll-margin math + // reads. Leaf rows only ever contribute their 24px height. + const isScroller = this.classList.contains('overflow-auto') + const scroller = isScroller ? null : this.closest('.overflow-auto') + const top = isScroller ? 0 : -(scroller?.scrollTop ?? 0) + return { + top, + bottom: top + SOURCE_CONTROL_FILE_ROW_HEIGHT_PX, + height: SOURCE_CONTROL_FILE_ROW_HEIGHT_PX, + left: 0, + right: 240, + width: 240, + x: 0, + y: top, + toJSON: () => ({}) + } as DOMRect + }) +}) + +afterEach(() => { + act(() => root.unmount()) + container.remove() + vi.unstubAllGlobals() + vi.restoreAllMocks() +}) + +function renderSourceControl(): void { + act(() => { + root.render( + + + + ) + }) +} + +function scroller(): HTMLDivElement { + const element = container.querySelector('.overflow-auto') + if (!element) { + throw new Error('source control scroller not found') + } + return element +} + +function scrollTo(offset: number): void { + const element = scroller() + // Why: happy-dom clamps scrollTop against its zero-height layout; pin the + // property so the virtualizer's scroll handler reads the intended offset. + Object.defineProperty(element, 'scrollTop', { + configurable: true, + writable: true, + value: offset + }) + act(() => { + element.dispatchEvent(new Event('scroll')) + }) +} + +function mountedRows(): HTMLDivElement[] { + return Array.from( + container.querySelectorAll('[data-testid="source-control-entry"]') + ) +} + +function row(path: string): HTMLDivElement | null { + return container.querySelector( + `[data-source-control-path="${path}"][data-source-control-area="unstaged"]` + ) +} + +function virtualList(): HTMLDivElement | null { + return container.querySelector('[data-testid="source-control-virtual-list"]') +} + +describe('SourceControl virtualized changed-files list', () => { + it('bounds mounted rows by viewport + overscan with 500 entries', () => { + resetState({ + gitStatusByWorktree: { [mocks.activeWorktree.id]: manyEntries(500) } + }) + renderSourceControl() + + expect(virtualList()).toBeTruthy() + const mounted = mountedRows().length + expect(mounted).toBeGreaterThan(0) + expect(mounted).toBeLessThanOrEqual(MAX_MOUNTED_ROWS) + }) + + it('windows rows while scrolling instead of mounting the tail', () => { + resetState({ + gitStatusByWorktree: { [mocks.activeWorktree.id]: manyEntries(500) } + }) + renderSourceControl() + + expect(row('src/file-000.ts')).toBeTruthy() + expect(row('src/file-250.ts')).toBeNull() + + scrollTo(250 * SOURCE_CONTROL_FILE_ROW_HEIGHT_PX) + + expect(row('src/file-250.ts')).toBeTruthy() + expect(row('src/file-000.ts')).toBeNull() + expect(mountedRows().length).toBeLessThanOrEqual(MAX_MOUNTED_ROWS) + }) + + it('keeps row selection across the virtualized boundary', () => { + resetState({ + gitStatusByWorktree: { [mocks.activeWorktree.id]: manyEntries(500) } + }) + renderSourceControl() + + const target = row('src/file-000.ts') + expect(target).toBeTruthy() + act(() => { + target?.dispatchEvent(new MouseEvent('contextmenu', { bubbles: true })) + }) + expect(row('src/file-000.ts')?.className).toContain('bg-accent/60') + expect(container.textContent).toContain('1 selected') + + // The selected row scrolls out of the mounted window but stays selected. + scrollTo(240 * SOURCE_CONTROL_FILE_ROW_HEIGHT_PX) + expect(row('src/file-000.ts')).toBeNull() + expect(container.textContent).toContain('1 selected') + + scrollTo(0) + expect(row('src/file-000.ts')?.className).toContain('bg-accent/60') + }) + + it('collapses and re-expands a virtualized section', () => { + resetState({ + gitStatusByWorktree: { [mocks.activeWorktree.id]: manyEntries(500) } + }) + renderSourceControl() + expect(mountedRows().length).toBeGreaterThan(0) + + const changesHeader = Array.from(container.querySelectorAll('button')).find((button) => + button.textContent?.includes('Changes') + ) + expect(changesHeader).toBeTruthy() + + act(() => { + changesHeader?.dispatchEvent(new MouseEvent('click', { bubbles: true })) + }) + expect(mountedRows().length).toBe(0) + expect(virtualList()).toBeNull() + + act(() => { + changesHeader?.dispatchEvent(new MouseEvent('click', { bubbles: true })) + }) + expect(mountedRows().length).toBeGreaterThan(0) + expect(mountedRows().length).toBeLessThanOrEqual(MAX_MOUNTED_ROWS) + }) + + it('keeps the scroll offset and window when a refresh delivers identical entries', () => { + resetState({ + gitStatusByWorktree: { [mocks.activeWorktree.id]: manyEntries(500) } + }) + renderSourceControl() + + scrollTo(100 * SOURCE_CONTROL_FILE_ROW_HEIGHT_PX) + expect(row('src/file-100.ts')).toBeTruthy() + const heightBefore = virtualList()?.style.height + + // A status refresh rebuilds every entry object with identical content. + mocks.state = { + ...mocks.state, + gitStatusByWorktree: { + [mocks.activeWorktree.id]: manyEntries(500).map((entry) => ({ ...entry })) + } + } + renderSourceControl() + + expect(scroller().scrollTop).toBe(100 * SOURCE_CONTROL_FILE_ROW_HEIGHT_PX) + expect(row('src/file-100.ts')).toBeTruthy() + expect(virtualList()?.style.height).toBe(heightBefore) + }) + + it('renders small changesets in full without the virtualization container', () => { + const paths = ['src/a.ts', 'src/b.ts', 'src/c.ts'] + resetState({ + gitStatusByWorktree: { + [mocks.activeWorktree.id]: paths.map((path) => gitEntry({ path })) + } + }) + renderSourceControl() + + expect(paths.length).toBeLessThan(SOURCE_CONTROL_VIRTUALIZE_MIN_ROWS) + expect(virtualList()).toBeNull() + expect(mountedRows().length).toBe(paths.length) + for (const path of paths) { + expect(row(path)).toBeTruthy() + } + }) + + it('bounds mounted tree-view rows for large changesets', () => { + resetState({ + settings: { sourceControlViewMode: 'tree' }, + gitStatusByWorktree: { + [mocks.activeWorktree.id]: Array.from({ length: 500 }, (_, index) => + gitEntry({ + path: `src/dir-${String(Math.floor(index / 20)).padStart(2, '0')}/file-${String( + index + ).padStart(3, '0')}.ts` + }) + ) + } + }) + renderSourceControl() + + expect(virtualList()).toBeTruthy() + const mounted = container.querySelectorAll( + '[data-testid="source-control-virtual-list"] [data-index]' + ).length + expect(mounted).toBeGreaterThan(0) + expect(mounted).toBeLessThanOrEqual(MAX_MOUNTED_ROWS) + }) +}) diff --git a/src/renderer/src/components/right-sidebar/source-control-virtual-file-list.tsx b/src/renderer/src/components/right-sidebar/source-control-virtual-file-list.tsx new file mode 100644 index 00000000000..8c71cbe7279 --- /dev/null +++ b/src/renderer/src/components/right-sidebar/source-control-virtual-file-list.tsx @@ -0,0 +1,106 @@ +import { useLayoutEffect, useRef, useState } from 'react' +import { useVirtualizer } from '@tanstack/react-virtual' + +// Why: below this count plain rows keep the DOM identical to the +// pre-virtualization markup (natural flow, no absolute positioning), so small +// changesets keep exact scrollbar and flicker-free behavior. `STA-351` / +// `STA-1280` jank only appears with hundreds of rows. +export const SOURCE_CONTROL_VIRTUALIZE_MIN_ROWS = 50 +// Why: rows are one py-1 text-xs line, except conflict/submodule rows which +// add a second label line — so estimate the common height and let +// measureElement correct the tall variants. Identical entries measure +// identically, so a git-status refresh cannot move the scroll position. +export const SOURCE_CONTROL_FILE_ROW_HEIGHT_PX = 24 +export const SOURCE_CONTROL_FILE_ROW_OVERSCAN = 10 + +/** + * Windows one source-control section's rows inside the panel's shared + * scroller. Sections below SOURCE_CONTROL_VIRTUALIZE_MIN_ROWS render plainly; + * larger ones mount only viewport + overscan rows. + */ +export function SourceControlVirtualFileList({ + rows, + getRowKey, + renderRow, + scrollElement +}: { + rows: readonly TRow[] + getRowKey: (row: TRow) => string + renderRow: (row: TRow) => React.ReactNode + // Why: a state-held element, not a ref — ancestor host refs are not attached + // yet when this component's mount effects run, so a ref would leave the + // virtualizer unobserved until some unrelated re-render. + scrollElement: HTMLDivElement | null +}): React.JSX.Element { + const containerRef = useRef(null) + const [scrollMargin, setScrollMargin] = useState(0) + const virtualize = rows.length >= SOURCE_CONTROL_VIRTUALIZE_MIN_ROWS + + // Why: the section shares the panel scroller with the commit area and + // sibling sections, so the virtualizer needs this list's offset inside that + // scroller. Everything above changes height only through React renders + // (commit drafts, banners, section collapse), so a per-render layout read + // keeps the margin current without observers. The guarded setState makes the + // measure/render loop converge in one extra pass. + // eslint-disable-next-line react-hooks/exhaustive-deps -- no dependency list on purpose: any sibling render can move this list inside the shared scroller. + useLayoutEffect(() => { + const container = containerRef.current + if (!container || !scrollElement) { + return + } + const nextMargin = Math.round( + container.getBoundingClientRect().top - + scrollElement.getBoundingClientRect().top + + scrollElement.scrollTop + ) + setScrollMargin((current) => (current === nextMargin ? current : nextMargin)) + }) + + const virtualizer = useVirtualizer({ + count: rows.length, + enabled: virtualize && scrollElement !== null, + getScrollElement: () => scrollElement, + estimateSize: () => SOURCE_CONTROL_FILE_ROW_HEIGHT_PX, + overscan: SOURCE_CONTROL_FILE_ROW_OVERSCAN, + scrollMargin, + // Why: stable row keys let the virtualizer carry item identity across + // status refreshes instead of remounting the window each poll. + getItemKey: (index) => { + const row = rows[index] + return row === undefined ? index : getRowKey(row) + } + }) + + if (!virtualize) { + return <>{rows.map((row) => renderRow(row))} + } + + return ( +
+ {virtualizer.getVirtualItems().map((item) => { + const row = rows[item.index] + if (row === undefined) { + return null + } + return ( +
+ {renderRow(row)} +
+ ) + })} +
+ ) +}