diff --git a/src/renderer/src/components/editor/combined-diff/browse-files/combined-diff-file-tree-row.tsx b/src/renderer/src/components/editor/combined-diff/browse-files/combined-diff-file-tree-row.tsx index b71aab643b5..dfb417e2518 100644 --- a/src/renderer/src/components/editor/combined-diff/browse-files/combined-diff-file-tree-row.tsx +++ b/src/renderer/src/components/editor/combined-diff/browse-files/combined-diff-file-tree-row.tsx @@ -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 diff --git a/src/renderer/src/components/editor/combined-diff/browse-files/combined-diff-file-tree-rows.tsx b/src/renderer/src/components/editor/combined-diff/browse-files/combined-diff-file-tree-rows.tsx new file mode 100644 index 00000000000..229f9561ac0 --- /dev/null +++ b/src/renderer/src/components/editor/combined-diff/browse-files/combined-diff-file-tree-rows.tsx @@ -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 + collapsedDirectoryKeys: ReadonlySet + visibleFileCounts: ReadonlyMap | undefined + scrollElement: HTMLDivElement | null + onToggleDirectory: (key: string) => void + onNavigate: (entry: CombinedDiffFileTreeEntry) => void +}): React.JSX.Element { + return ( + node.key} + renderRow={(node) => ( + + )} + /> + ) +} diff --git a/src/renderer/src/components/editor/combined-diff/browse-files/combined-diff-file-tree-windowing.test.tsx b/src/renderer/src/components/editor/combined-diff/browse-files/combined-diff-file-tree-windowing.test.tsx new file mode 100644 index 00000000000..2989f0bdeb7 --- /dev/null +++ b/src/renderer/src/components/editor/combined-diff/browse-files/combined-diff-file-tree-windowing.test.tsx @@ -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) => 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) => { + 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 = 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( + {}} + 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) + }) +}) diff --git a/src/renderer/src/components/editor/combined-diff/browse-files/combined-diff-file-tree.tsx b/src/renderer/src/components/editor/combined-diff/browse-files/combined-diff-file-tree.tsx index dc08942f8d5..c0eb5204c73 100644 --- a/src/renderer/src/components/editor/combined-diff/browse-files/combined-diff-file-tree.tsx +++ b/src/renderer/src/components/editor/combined-diff/browse-files/combined-diff-file-tree.tsx @@ -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>(() => 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(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({ -
+
{visibleEntryCount === 0 ? (
{translate( @@ -309,20 +322,11 @@ export function CombinedDiffFileTree({
{group.label}
- {rows.map((node) => ( - - ))} +
) })} @@ -334,38 +338,20 @@ export function CombinedDiffFileTree({ 'Committed on Branch' )}
- {(branchVisibleRows?.rows ?? branchRows).map((node) => ( - - ))} +
) : null} ) : ( - (branchVisibleRows?.rows ?? branchRows).map((node) => ( - - )) + )}
({ 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({ // 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(null) const [scrollMargin, setScrollMargin] = useState(0) @@ -124,7 +128,7 @@ export function SourceControlVirtualFileList({ 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