perf(diff): window the combined-diff file tree rows on large reviews

`combined-diff-file-tree.tsx` had three unvirtualized `rows.map(...)` sites, so a 900-file review mounted all 931 tree rows at once. Route the three through a `CombinedDiffFileTreeRows` wrapper over the existing `SourceControlVirtualFileList`, reusing its `SOURCE_CONTROL_VIRTUALIZE_MIN_ROWS = 50` threshold and scroll-margin machinery, with the tree's own 24px row estimate. `SourceControlVirtualFileList` gains one optional `estimateRowHeightPx` prop that defaults to its current constant, so source control is unchanged. Below the threshold the rows stay in natural flow and the markup is unchanged. Above it, find-in-page, select-all-copy and Tab order see only the mounted window — the same trade already accepted for the source-control panel.
2026-09-30 16:02:56 +00:00 · 2026-09-02 12:57:54 -07:00
parent 3d67f2be2c
commit cee0033471
5 changed files with 248 additions and 46 deletions
@@ -24,6 +24,9 @@ export type CombinedDiffTreeNode = SourceControlTreeNode<
  GitStagingArea | CombinedDiffBranchTreeArea
 >

+// Why: every row is a single `py-1 text-xs` line (16px line box + 8px padding); measureElement
+// still corrects, but a wrong estimate makes the virtualized tree's scrollbar jump on first paint.
+export const COMBINED_DIFF_TREE_ROW_HEIGHT_PX = 24
 const COMBINED_DIFF_TREE_INDENT_PX = 12
 const COMBINED_DIFF_TREE_DIRECTORY_PADDING_PX = 8
 const COMBINED_DIFF_TREE_FILE_PADDING_PX = 20
@@ -0,0 +1,63 @@
+import React from 'react'
+import { SourceControlVirtualFileList } from '@/components/right-sidebar/source-control/listing/virtual-file-list'
+import type {
+  CombinedDiffFileTreeEntry,
+  CombinedDiffFileTreeMode
+} from '../resolve-changes/combined-diff-section-identity'
+import {
+  CombinedDiffFileTreeRow,
+  COMBINED_DIFF_TREE_ROW_HEIGHT_PX
+} from './combined-diff-file-tree-row'
+import type { CombinedDiffTreeNode } from './combined-diff-file-tree-model'
+
+/**
+ * One flattened tree section, windowed inside the file tree's scroller. A 900-file review flattens
+ * to over a thousand rows; below the virtualize threshold the rows stay in natural flow so small
+ * diffs keep byte-identical markup.
+ */
+export function CombinedDiffFileTreeRows({
+  rows,
+  mode,
+  worktreePath,
+  activeSectionKey,
+  sectionIndexByKey,
+  collapsedDirectoryKeys,
+  visibleFileCounts,
+  scrollElement,
+  onToggleDirectory,
+  onNavigate
+}: {
+  rows: readonly CombinedDiffTreeNode[]
+  mode: CombinedDiffFileTreeMode
+  worktreePath: string
+  activeSectionKey: string | null
+  sectionIndexByKey: ReadonlyMap<string, number>
+  collapsedDirectoryKeys: ReadonlySet<string>
+  visibleFileCounts: ReadonlyMap<string, number> | undefined
+  scrollElement: HTMLDivElement | null
+  onToggleDirectory: (key: string) => void
+  onNavigate: (entry: CombinedDiffFileTreeEntry) => void
+}): React.JSX.Element {
+  return (
+    <SourceControlVirtualFileList
+      rows={rows}
+      scrollElement={scrollElement}
+      estimateRowHeightPx={COMBINED_DIFF_TREE_ROW_HEIGHT_PX}
+      getRowKey={(node) => node.key}
+      renderRow={(node) => (
+        <CombinedDiffFileTreeRow
+          key={node.key}
+          node={node}
+          mode={mode}
+          worktreePath={worktreePath}
+          activeSectionKey={activeSectionKey}
+          sectionIndexByKey={sectionIndexByKey}
+          isCollapsed={collapsedDirectoryKeys.has(node.key)}
+          visibleFileCount={visibleFileCounts?.get(node.key)}
+          onToggleDirectory={onToggleDirectory}
+          onNavigate={onNavigate}
+        />
+      )}
+    />
+  )
+}
@@ -0,0 +1,146 @@
+// @vitest-environment happy-dom
+
+import React, { act } from 'react'
+import { createRoot, type Root } from 'react-dom/client'
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
+import { SOURCE_CONTROL_VIRTUALIZE_MIN_ROWS } from '@/components/right-sidebar/source-control/listing/virtual-file-list'
+import type { GitBranchChangeEntry } from '../../../../../../shared/git-diff-compare-types'
+import type { CombinedDiffFileTreeRow as CombinedDiffFileTreeRowComponent } from './combined-diff-file-tree-row'
+
+const mountedRows = vi.hoisted(() => ({ count: 0 }))
+
+vi.mock('@/store', () => ({
+  useAppStore: (selector: (state: Record<string, unknown>) => unknown) =>
+    selector({ combinedDiffFileTreeWidth: 420, setCombinedDiffFileTreeWidth: () => {} })
+}))
+
+vi.mock('./combined-diff-file-tree-row', async (importOriginal) => {
+  const actual = (await importOriginal()) as {
+    CombinedDiffFileTreeRow: typeof CombinedDiffFileTreeRowComponent
+  }
+  const react = await import('react')
+  const Row = actual.CombinedDiffFileTreeRow
+  const CountingRow = react.memo((props: React.ComponentProps<typeof Row>) => {
+    react.useEffect(() => {
+      mountedRows.count += 1
+      return () => {
+        mountedRows.count -= 1
+      }
+    }, [])
+    return react.createElement(Row, props)
+  })
+  return { ...actual, CombinedDiffFileTreeRow: CountingRow }
+})
+
+const { CombinedDiffFileTree } = await import('./combined-diff-file-tree')
+const { createCombinedDiffSectionIndexMap } =
+  await import('../resolve-changes/combined-diff-section-identity')
+const { getCombinedDiffBranchEntriesInTreeOrder } = await import('./combined-diff-file-tree-filter')
+
+const VIEWPORT_HEIGHT_PX = 600
+const TREE_ROW_HEIGHT_PX = 24
+const EMPTY_VIEWED_KEYS: ReadonlySet<string> = new Set()
+
+class NoopResizeObserver implements ResizeObserver {
+  observe(): void {}
+  unobserve(): void {}
+  disconnect(): void {}
+}
+
+let host: HTMLDivElement
+let root: Root
+
+beforeEach(() => {
+  globalThis.IS_REACT_ACT_ENVIRONMENT = true
+  mountedRows.count = 0
+  host = document.createElement('div')
+  document.body.appendChild(host)
+  root = createRoot(host)
+  vi.stubGlobal('ResizeObserver', NoopResizeObserver)
+  vi.spyOn(HTMLElement.prototype, 'offsetHeight', 'get').mockImplementation(
+    function (this: HTMLElement) {
+      return this.classList.contains('overflow-auto') ? VIEWPORT_HEIGHT_PX : TREE_ROW_HEIGHT_PX
+    }
+  )
+  vi.spyOn(Element.prototype, 'getBoundingClientRect').mockImplementation(function (this: Element) {
+    const height = this.classList.contains('overflow-auto')
+      ? VIEWPORT_HEIGHT_PX
+      : TREE_ROW_HEIGHT_PX
+    return {
+      top: 0,
+      bottom: height,
+      height,
+      left: 0,
+      right: 240,
+      width: 240,
+      x: 0,
+      y: 0,
+      toJSON: () => ({})
+    } as DOMRect
+  })
+})
+
+afterEach(() => {
+  act(() => root.unmount())
+  host.remove()
+  vi.unstubAllGlobals()
+  vi.restoreAllMocks()
+})
+
+/** `fileCount` files spread over `directoryCount` directories, in the viewer's own tree order. */
+function buildEntries(fileCount: number, directoryCount: number): GitBranchChangeEntry[] {
+  const raw: GitBranchChangeEntry[] = Array.from({ length: fileCount }, (_, index) => ({
+    path: `src/dir${String(index % directoryCount).padStart(2, '0')}/file-${String(index).padStart(4, '0')}.ts`,
+    status: 'modified'
+  }))
+  return getCombinedDiffBranchEntriesInTreeOrder('commit', raw)
+}
+
+function renderTree(entries: readonly GitBranchChangeEntry[]): void {
+  const sectionIndexByKey = createCombinedDiffSectionIndexMap(
+    entries.map((entry) => ({ key: `combined-commit:${entry.path}` }))
+  )
+  act(() => {
+    root.render(
+      <CombinedDiffFileTree
+        mode="commit"
+        worktreePath="/repo"
+        entries={entries}
+        sectionIndexByKey={sectionIndexByKey}
+        activeSectionKey={null}
+        viewedSectionKeys={EMPTY_VIEWED_KEYS}
+        collapsed={false}
+        onCollapsedChange={() => {}}
+        onNavigate={() => {}}
+      />
+    )
+  })
+}
+
+describe('combined diff file tree row windowing', () => {
+  it('mounts every row below the virtualize threshold', () => {
+    const fileCount = SOURCE_CONTROL_VIRTUALIZE_MIN_ROWS - 10
+    const directoryCount = 4
+    renderTree(buildEntries(fileCount, directoryCount))
+
+    // `src` plus one directory row per leaf directory, plus one row per file.
+    const totalRows = 1 + directoryCount + fileCount
+    expect(totalRows).toBeLessThan(SOURCE_CONTROL_VIRTUALIZE_MIN_ROWS)
+    expect(mountedRows.count).toBe(totalRows)
+    expect(host.querySelector('[data-testid="source-control-virtual-list"]')).toBeNull()
+    // Natural flow: no absolutely positioned wrappers, exactly the pre-virtualization markup.
+    expect(host.querySelectorAll('[data-index]').length).toBe(0)
+  })
+
+  it('mounts only a window of rows for a large review', () => {
+    const fileCount = 900
+    const directoryCount = 30
+    renderTree(buildEntries(fileCount, directoryCount))
+
+    const totalRows = 1 + directoryCount + fileCount
+    expect(host.querySelector('[data-testid="source-control-virtual-list"]')).not.toBeNull()
+    expect(mountedRows.count).toBeGreaterThan(0)
+    // A 600px viewport plus overscan: bounded by the window, not by the review size.
+    expect(mountedRows.count).toBeLessThan(totalRows / 10)
+  })
+})
@@ -12,7 +12,7 @@ import type {
  CombinedDiffFileTreeEntry,
  CombinedDiffFileTreeMode
 } from '../resolve-changes/combined-diff-section-identity'
-import { CombinedDiffFileTreeRow } from './combined-diff-file-tree-row'
+import { CombinedDiffFileTreeRows } from './combined-diff-file-tree-rows'
 import { useCombinedDiffFileTreeResize } from './use-combined-diff-file-tree-resize'
 import { translate } from '@/i18n/i18n'
 import {
@@ -50,6 +50,8 @@ export function CombinedDiffFileTree({
  const [query, setQuery] = React.useState('')
  const [excludedExtensions, setExcludedExtensions] = React.useState<Set<string>>(() => new Set())
  const [includeViewed, setIncludeViewed] = React.useState(true)
+  // Why: state, not a ref — the virtualized row lists need the scroller on their own mount pass.
+  const [listScrollElement, setListScrollElement] = React.useState<HTMLDivElement | null>(null)
  const { handleResizeKeyDown, handleResizeStart, maxWidth, minWidth, treeRef, width } =
    useCombinedDiffFileTreeResize(collapsed)
  const toggleDirectory = React.useCallback((key: string) => {
@@ -171,6 +173,17 @@ export function CombinedDiffFileTree({
    return null
  }

+  const sharedRowProps = {
+    mode,
+    worktreePath,
+    activeSectionKey,
+    sectionIndexByKey,
+    collapsedDirectoryKeys,
+    scrollElement: listScrollElement,
+    onToggleDirectory: toggleDirectory,
+    onNavigate
+  }
+
  return (
    // Why: this column must be height-bounded so the file list, not the page,
    // owns overflow when review diffs have more files than fit on screen.
@@ -283,7 +296,7 @@ export function CombinedDiffFileTree({
          </Popover>
        </div>
      </div>
-      <div className="min-h-0 flex-1 overflow-auto py-1 scrollbar-sleek">
+      <div ref={setListScrollElement} className="min-h-0 flex-1 overflow-auto py-1 scrollbar-sleek">
        {visibleEntryCount === 0 ? (
          <div className="px-3 py-6 text-center text-xs text-muted-foreground">
            {translate(
@@ -309,20 +322,11 @@ export function CombinedDiffFileTree({
                  <div className="px-3 pb-1 text-[11px] font-semibold uppercase tracking-[0.05em] text-muted-foreground">
                    {group.label}
                  </div>
-                  {rows.map((node) => (
-                    <CombinedDiffFileTreeRow
-                      key={node.key}
-                      node={node}
-                      mode={mode}
-                      worktreePath={worktreePath}
-                      activeSectionKey={activeSectionKey}
-                      sectionIndexByKey={sectionIndexByKey}
-                      isCollapsed={collapsedDirectoryKeys.has(node.key)}
-                      visibleFileCount={visibleFileCounts?.get(node.key)}
-                      onToggleDirectory={toggleDirectory}
-                      onNavigate={onNavigate}
-                    />
-                  ))}
+                  <CombinedDiffFileTreeRows
+                    rows={rows}
+                    visibleFileCounts={visibleFileCounts}
+                    {...sharedRowProps}
+                  />
                </div>
              )
            })}
@@ -334,38 +338,20 @@ export function CombinedDiffFileTree({
                    'Committed on Branch'
                  )}
                </div>
-                {(branchVisibleRows?.rows ?? branchRows).map((node) => (
-                  <CombinedDiffFileTreeRow
-                    key={node.key}
-                    node={node}
-                    mode={mode}
-                    worktreePath={worktreePath}
-                    activeSectionKey={activeSectionKey}
-                    sectionIndexByKey={sectionIndexByKey}
-                    isCollapsed={collapsedDirectoryKeys.has(node.key)}
-                    visibleFileCount={branchVisibleRows?.visibleFileCounts.get(node.key)}
-                    onToggleDirectory={toggleDirectory}
-                    onNavigate={onNavigate}
-                  />
-                ))}
+                <CombinedDiffFileTreeRows
+                  rows={branchVisibleRows?.rows ?? branchRows}
+                  visibleFileCounts={branchVisibleRows?.visibleFileCounts}
+                  {...sharedRowProps}
+                />
              </div>
            ) : null}
          </>
        ) : (
-          (branchVisibleRows?.rows ?? branchRows).map((node) => (
-            <CombinedDiffFileTreeRow
-              key={node.key}
-              node={node}
-              mode={mode}
-              worktreePath={worktreePath}
-              activeSectionKey={activeSectionKey}
-              sectionIndexByKey={sectionIndexByKey}
-              isCollapsed={collapsedDirectoryKeys.has(node.key)}
-              visibleFileCount={branchVisibleRows?.visibleFileCounts.get(node.key)}
-              onToggleDirectory={toggleDirectory}
-              onNavigate={onNavigate}
-            />
-          ))
+          <CombinedDiffFileTreeRows
+            rows={branchVisibleRows?.rows ?? branchRows}
+            visibleFileCounts={branchVisibleRows?.visibleFileCounts}
+            {...sharedRowProps}
+          />
        )}
      </div>
      <div
@@ -83,7 +83,8 @@ export function SourceControlVirtualFileList<TRow>({
  rows,
  getRowKey,
  renderRow,
-  scrollElement
+  scrollElement,
+  estimateRowHeightPx = SOURCE_CONTROL_FILE_ROW_HEIGHT_PX
 }: {
  rows: readonly TRow[]
  getRowKey: (row: TRow) => string
@@ -92,6 +93,9 @@ export function SourceControlVirtualFileList<TRow>({
  // yet when this component's mount effects run, so a ref would leave the
  // virtualizer unobserved until some unrelated re-render.
  scrollElement: HTMLDivElement | null
+  // Why: callers outside source control have their own row paddings; measureElement
+  // still corrects, but a wrong estimate makes the initial scrollbar jump.
+  estimateRowHeightPx?: number
 }): React.JSX.Element {
  const containerRef = useRef<HTMLDivElement>(null)
  const [scrollMargin, setScrollMargin] = useState(0)
@@ -124,7 +128,7 @@ export function SourceControlVirtualFileList<TRow>({
    count: rows.length,
    enabled: virtualize && scrollElement !== null,
    getScrollElement: () => scrollElement,
-    estimateSize: () => SOURCE_CONTROL_FILE_ROW_HEIGHT_PX,
+    estimateSize: () => estimateRowHeightPx,
    overscan: SOURCE_CONTROL_FILE_ROW_OVERSCAN,
    scrollMargin,
    // Why: stable row keys let the virtualizer carry item identity across