From cbacb36adde1d3900563aad4aee97742c63efda7 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 3 Sep 2026 03:48:55 -0700 Subject: [PATCH] perf(source-control): sort branch entries before filtering, gate projections by view mode Two dead-work fixes in the Source Control file projection. 1. filterAndSortSourceControlPathEntries copied and re-sorted the uncapped branch entry list with Intl.Collator on every keystroke. Sort once on branchEntries, filter after: Array#filter preserves order and compareFileNames is a total order, so filter(sort(x)) === sort(filter(x)). 2. The tree projection was built in list mode and the list projection in tree mode, then discarded. Gate each memo on sourceControlViewMode and return a shared empty projection, matching the combined-diff file tree precedent. --- .../source-control-file-filter.test.ts | 21 -- .../source-control/listing/file-filter.ts | 10 - .../listing/use-file-projection-work.test.tsx | 275 ++++++++++++++++++ .../listing/use-file-projection.ts | 54 +++- 4 files changed, 320 insertions(+), 40 deletions(-) create mode 100644 src/renderer/src/components/right-sidebar/source-control/listing/use-file-projection-work.test.tsx diff --git a/src/renderer/src/components/right-sidebar/source-control-file-filter.test.ts b/src/renderer/src/components/right-sidebar/source-control-file-filter.test.ts index 6b2419c9195..18d8ad5f3b9 100644 --- a/src/renderer/src/components/right-sidebar/source-control-file-filter.test.ts +++ b/src/renderer/src/components/right-sidebar/source-control-file-filter.test.ts @@ -1,7 +1,6 @@ import { describe, expect, it } from 'vitest' import { SOURCE_CONTROL_FILE_FILTER_QUERY_MAX_BYTES, - filterAndSortSourceControlPathEntries, filterSourceControlGroupedPathEntries, filterSourceControlPathEntries, getSourceControlFileFilterState, @@ -10,26 +9,6 @@ import { } from './source-control/listing/file-filter' describe('source-control-file-filter', () => { - it('naturally orders committed branch rows without mutating store input', () => { - const entries = [ - { path: 'migrations/100.sql' }, - { path: 'migrations/9.sql' }, - { path: 'migrations/99.sql' } - ] - - expect( - filterAndSortSourceControlPathEntries(entries, { - normalizedFilter: '', - tooLarge: false - }).map((entry) => entry.path) - ).toEqual(['migrations/9.sql', 'migrations/99.sql', 'migrations/100.sql']) - expect(entries.map((entry) => entry.path)).toEqual([ - 'migrations/100.sql', - 'migrations/9.sql', - 'migrations/99.sql' - ]) - }) - it('normalizes bounded queries and filters entries by path', () => { const filter = getSourceControlFileFilterState(' SRC/button ') diff --git a/src/renderer/src/components/right-sidebar/source-control/listing/file-filter.ts b/src/renderer/src/components/right-sidebar/source-control/listing/file-filter.ts index 4bdd676bcf5..ae379b51d4c 100644 --- a/src/renderer/src/components/right-sidebar/source-control/listing/file-filter.ts +++ b/src/renderer/src/components/right-sidebar/source-control/listing/file-filter.ts @@ -1,5 +1,4 @@ import { isClipboardTextByteLengthOverLimit } from '../../../../../../shared/clipboard-text' -import { compareFileNames } from '../../../../../../shared/file-name-sort' export const SOURCE_CONTROL_FILE_FILTER_QUERY_MAX_BYTES = 2 * 1024 @@ -49,15 +48,6 @@ export function filterSourceControlPathEntries return entries.filter((entry) => entry.path.toLowerCase().includes(filter.normalizedFilter)) } -export function filterAndSortSourceControlPathEntries( - entries: T[], - filter: SourceControlFileFilterState -): T[] { - return [...filterSourceControlPathEntries(entries, filter)].sort((a, b) => - compareFileNames(a.path, b.path) - ) -} - export function filterSourceControlGroupedPathEntries( grouped: SourceControlGroupedPathEntries, filter: SourceControlFileFilterState diff --git a/src/renderer/src/components/right-sidebar/source-control/listing/use-file-projection-work.test.tsx b/src/renderer/src/components/right-sidebar/source-control/listing/use-file-projection-work.test.tsx new file mode 100644 index 00000000000..b7ffc17300b --- /dev/null +++ b/src/renderer/src/components/right-sidebar/source-control/listing/use-file-projection-work.test.tsx @@ -0,0 +1,275 @@ +// @vitest-environment happy-dom + +import { renderHook } from '@testing-library/react' +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type { GitBranchChangeEntry } from '../../../../../../shared/git-diff-compare-types' +import type { GitStatusEntry } from '../../../../../../shared/git-status-types' +import type { SourceControlViewMode } from '../../../../../../shared/ui-chrome-types' +import type * as FileNameSortModule from '../../../../../../shared/file-name-sort' +import type * as SourceControlTreeModule from '../../source-control-tree' +import type * as SubmoduleExpansionModule from './submodule-expansion' + +const counters = vi.hoisted(() => ({ + compareFileNames: 0, + buildGitStatusSourceControlTree: 0, + buildSourceControlTree: 0, + flattenSourceControlTree: 0, + injectExpandedSubmoduleRows: 0, + injectExpandedSubmoduleEntries: 0 +})) + +vi.mock('../../../../../../shared/file-name-sort', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + compareFileNames: (a: string, b: string) => { + counters.compareFileNames += 1 + return actual.compareFileNames(a, b) + } + } +}) + +vi.mock('../../source-control-tree', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + buildGitStatusSourceControlTree: ( + ...args: Parameters + ) => { + counters.buildGitStatusSourceControlTree += 1 + return actual.buildGitStatusSourceControlTree(...args) + }, + buildSourceControlTree: ((...args: unknown[]) => { + counters.buildSourceControlTree += 1 + return (actual.buildSourceControlTree as (...a: unknown[]) => unknown)(...args) + }) as typeof actual.buildSourceControlTree, + flattenSourceControlTree: ((...args: unknown[]) => { + counters.flattenSourceControlTree += 1 + return (actual.flattenSourceControlTree as (...a: unknown[]) => unknown)(...args) + }) as typeof actual.flattenSourceControlTree + } +}) + +vi.mock('./submodule-expansion', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + injectExpandedSubmoduleRows: ((...args: unknown[]) => { + counters.injectExpandedSubmoduleRows += 1 + return (actual.injectExpandedSubmoduleRows as (...a: unknown[]) => unknown)(...args) + }) as typeof actual.injectExpandedSubmoduleRows, + injectExpandedSubmoduleEntries: ((...args: unknown[]) => { + counters.injectExpandedSubmoduleEntries += 1 + return (actual.injectExpandedSubmoduleEntries as (...a: unknown[]) => unknown)(...args) + }) as typeof actual.injectExpandedSubmoduleEntries + } +}) + +const { compareFileNames } = await import('../../../../../../shared/file-name-sort') +const { getSourceControlFileFilterState, filterSourceControlPathEntries } = + await import('./file-filter') +const { useSourceControlFileProjection } = await import('./use-file-projection') + +const NO_ENTRIES: GitStatusEntry[] = [] +const NO_COLLAPSED_TREE_DIRS = new Set() +const NO_EXPANDED_SUBMODULES = new Set() +const NO_COLLAPSED_SECTIONS = new Set() +const NO_SUBMODULE_STATUS = {} +const GROUP_ORDER = ['unstaged', 'staged', 'untracked'] as const + +type ProjectionProps = { + entries: GitStatusEntry[] + branchEntries: GitBranchChangeEntry[] + filterQuery: string + sourceControlViewMode: SourceControlViewMode +} + +function renderProjection(initialProps: ProjectionProps) { + return renderHook( + (props: ProjectionProps) => + useSourceControlFileProjection({ + entries: props.entries, + branchEntries: props.branchEntries, + filterQuery: props.filterQuery, + sourceControlGroupOrder: GROUP_ORDER, + activeWorktreeId: 'wt-1', + worktreePath: '/repo', + isFolder: false, + collapsedTreeDirs: NO_COLLAPSED_TREE_DIRS, + expandedSubmoduleKeys: NO_EXPANDED_SUBMODULES, + submoduleStatusByKey: NO_SUBMODULE_STATUS, + sourceControlViewMode: props.sourceControlViewMode, + collapsedSections: NO_COLLAPSED_SECTIONS + }), + { initialProps } + ) +} + +function makeBranchEntries(count: number): GitBranchChangeEntry[] { + return Array.from({ length: count }, (_, index) => ({ + path: `src/area-${index % 7}/nested/deep-${index % 13}/file-${index}.ts`, + status: 'modified' as const + })) +} + +/** Numeric collation, case variants, unicode, collator ties, and an exact duplicate path. */ +const ORDERING_FIXTURE: GitBranchChangeEntry[] = [ + { path: 'migrations/100.sql', status: 'modified' }, + { path: 'migrations/9.sql', status: 'modified' }, + { path: 'migrations/99.sql', status: 'added' }, + { path: 'migrations/02.sql', status: 'modified' }, + { path: 'migrations/2.sql', status: 'deleted' }, + { path: 'src/Button.tsx', status: 'modified' }, + { path: 'src/button.tsx', status: 'added' }, + { path: 'src/éclair.ts', status: 'modified' }, + { path: 'src/eclair.ts', status: 'deleted' }, + { path: 'src/Éclair.ts', status: 'modified' }, + { path: 'src/日本語.ts', status: 'added' }, + { path: 'src/dup.ts', status: 'modified' }, + { path: 'src/dup.ts', status: 'added' } +] + +/** The pre-change implementation: filter, then copy-and-sort. */ +function legacyFilterThenSort( + entries: GitBranchChangeEntry[], + filterQuery: string +): GitBranchChangeEntry[] { + const state = getSourceControlFileFilterState(filterQuery) + return [...filterSourceControlPathEntries(entries, state)].sort((a, b) => + compareFileNames(a.path, b.path) + ) +} + +function makeStatusEntries(count: number): GitStatusEntry[] { + return Array.from({ length: count }, (_, index) => ({ + path: `src/area-${index % 7}/nested/deep-${index % 13}/file-${index}.ts`, + status: 'modified' as const, + area: (['unstaged', 'staged', 'untracked'] as const)[index % 3] + })) +} + +beforeEach(() => { + for (const key of Object.keys(counters) as (keyof typeof counters)[]) { + counters[key] = 0 + } +}) + +describe('useSourceControlFileProjection branch entry ordering', () => { + it('sorts committed branch entries once across many filter changes', () => { + const branchEntries = makeBranchEntries(400) + const { rerender } = renderProjection({ + entries: NO_ENTRIES, + branchEntries, + filterQuery: '', + sourceControlViewMode: 'list' + }) + + const comparesForInitialSort = counters.compareFileNames + expect(comparesForInitialSort).toBeGreaterThan(0) + + for (const filterQuery of ['f', 'fi', 'fil', 'file', 'file-', 'file-1']) { + rerender({ entries: NO_ENTRIES, branchEntries, filterQuery, sourceControlViewMode: 'list' }) + } + + expect(counters.compareFileNames).toBe(comparesForInitialSort) + }) + + it('produces the same order as the previous filter-then-sort for every filter', () => { + const { result, rerender } = renderProjection({ + entries: NO_ENTRIES, + branchEntries: ORDERING_FIXTURE, + filterQuery: '', + sourceControlViewMode: 'list' + }) + + for (const filterQuery of ['', 'src', 'MIGRATIONS', 'é', '9', 'dup', 'no-match']) { + rerender({ + entries: NO_ENTRIES, + branchEntries: ORDERING_FIXTURE, + filterQuery, + sourceControlViewMode: 'list' + }) + expect(result.current.filteredBranchEntries).toEqual( + legacyFilterThenSort(ORDERING_FIXTURE, filterQuery) + ) + } + }) + + it('does not mutate the store-owned branch entry array', () => { + const branchEntries = [...ORDERING_FIXTURE] + renderProjection({ + entries: NO_ENTRIES, + branchEntries, + filterQuery: '', + sourceControlViewMode: 'list' + }) + + expect(branchEntries).toEqual(ORDERING_FIXTURE) + }) +}) + +describe('useSourceControlFileProjection view-mode gating', () => { + const entries = makeStatusEntries(120) + const branchEntries = makeBranchEntries(120) + + it('builds no tree projection in list mode', () => { + const { result } = renderProjection({ + entries, + branchEntries, + filterQuery: '', + sourceControlViewMode: 'list' + }) + + expect(counters.buildGitStatusSourceControlTree).toBe(0) + expect(counters.buildSourceControlTree).toBe(0) + expect(counters.flattenSourceControlTree).toBe(0) + expect(counters.injectExpandedSubmoduleRows).toBe(0) + expect(counters.injectExpandedSubmoduleEntries).toBeGreaterThan(0) + expect(result.current.visibleTreeRowsBySection).toEqual({}) + expect(result.current.visibleBranchTreeRows).toEqual([]) + }) + + it('builds no list projection in tree mode', () => { + const { result } = renderProjection({ + entries, + branchEntries, + filterQuery: '', + sourceControlViewMode: 'tree' + }) + + expect(counters.injectExpandedSubmoduleEntries).toBe(0) + expect(counters.buildGitStatusSourceControlTree).toBeGreaterThan(0) + expect(counters.buildSourceControlTree).toBeGreaterThan(0) + expect(result.current.visibleListRowsBySection).toEqual({}) + }) + + it('has the other mode fully projected on the first render after a switch', () => { + const { result, rerender } = renderProjection({ + entries, + branchEntries, + filterQuery: '', + sourceControlViewMode: 'list' + }) + const listRows = result.current.visibleListRowsBySection + const listSelectionCount = result.current.visibleSelectionEntries.length + expect(listSelectionCount).toBe(entries.length) + + rerender({ entries, branchEntries, filterQuery: '', sourceControlViewMode: 'tree' }) + + expect(result.current.visibleListRowsBySection).toEqual({}) + expect(result.current.visibleBranchTreeRows.length).toBeGreaterThan(0) + expect( + Object.values(result.current.visibleTreeRowsBySection).reduce( + (total, rows) => total + rows.length, + 0 + ) + ).toBeGreaterThan(0) + expect(result.current.visibleSelectionEntries.length).toBe(listSelectionCount) + + rerender({ entries, branchEntries, filterQuery: '', sourceControlViewMode: 'list' }) + + expect(result.current.visibleTreeRowsBySection).toEqual({}) + expect(result.current.visibleBranchTreeRows).toEqual([]) + expect(result.current.visibleListRowsBySection).toEqual(listRows) + }) +}) diff --git a/src/renderer/src/components/right-sidebar/source-control/listing/use-file-projection.ts b/src/renderer/src/components/right-sidebar/source-control/listing/use-file-projection.ts index c885eaa33dc..056a758bb8f 100644 --- a/src/renderer/src/components/right-sidebar/source-control/listing/use-file-projection.ts +++ b/src/renderer/src/components/right-sidebar/source-control/listing/use-file-projection.ts @@ -2,10 +2,11 @@ import { useMemo } from 'react' import type { GitBranchChangeEntry } from '../../../../../../shared/git-diff-compare-types' import type { GitStatusEntry } from '../../../../../../shared/git-status-types' import type { SourceControlViewMode } from '../../../../../../shared/ui-chrome-types' +import { compareFileNames } from '../../../../../../shared/file-name-sort' import { compareGitStatusEntries } from '../../source-control-status-sort' import { - filterAndSortSourceControlPathEntries, filterSourceControlGroupedPathEntries, + filterSourceControlPathEntries, getSourceControlFileFilterState, type SourceControlFileFilterState } from './file-filter' @@ -60,6 +61,19 @@ export type SourceControlFileProjection = { visibleSelectionEntries: FlatEntry[] } +// Why: only one view mode is ever rendered, so building the other mode's projection is pure dead +// work (precedent: the combined-diff file tree short-circuits the same way while collapsed). +const EMPTY_TREE_ROOTS_BY_SECTION: Readonly< + Partial> +> = Object.freeze({}) +const EMPTY_TREE_ROWS_BY_SECTION: Readonly< + Partial> +> = Object.freeze({}) +const EMPTY_LIST_ROWS_BY_SECTION: Readonly< + Partial> +> = Object.freeze({}) +const EMPTY_BRANCH_TREE_NODES: SourceControlTreeNode[] = [] + export function useSourceControlFileProjection({ entries, branchEntries, @@ -127,12 +141,21 @@ export function useSourceControlFileProjection({ [unfilteredDisplaySections] ) + // Why: sorting before filtering keeps the collator off the keystroke path; Array#filter preserves + // order and compareFileNames is a total order, so filter(sort(x)) === sort(filter(x)). + const sortedBranchEntries = useMemo( + () => [...branchEntries].sort((a, b) => compareFileNames(a.path, b.path)), + [branchEntries] + ) const filteredBranchEntries = useMemo( - () => filterAndSortSourceControlPathEntries(branchEntries, fileFilterState), - [branchEntries, fileFilterState] + () => filterSourceControlPathEntries(sortedBranchEntries, fileFilterState), + [fileFilterState, sortedBranchEntries] ) const treeRootsBySection = useMemo(() => { + if (sourceControlViewMode !== 'tree') { + return EMPTY_TREE_ROOTS_BY_SECTION + } const roots: Partial> = {} for (const section of displaySections) { @@ -148,9 +171,12 @@ export function useSourceControlFileProjection({ : sectionRoots } return roots - }, [displaySections]) + }, [displaySections, sourceControlViewMode]) const visibleTreeRowsBySection = useMemo(() => { + if (sourceControlViewMode !== 'tree') { + return EMPTY_TREE_ROWS_BY_SECTION + } const rows: Partial> = {} for (const section of displaySections) { rows[section.id] = injectExpandedSubmoduleRows( @@ -167,11 +193,15 @@ export function useSourceControlFileProjection({ displaySections, treeRootsBySection, expandedSubmoduleKeys, + sourceControlViewMode, submoduleStatusByKey ]) // List view needs the same lazy submodule expansion as tree view, spliced into the flat entry list. const visibleListRowsBySection = useMemo(() => { + if (sourceControlViewMode !== 'list') { + return EMPTY_LIST_ROWS_BY_SECTION + } const rows: Partial> = {} for (const section of displaySections) { rows[section.id] = injectExpandedSubmoduleEntries( @@ -183,15 +213,21 @@ export function useSourceControlFileProjection({ ) } return rows - }, [displaySections, expandedSubmoduleKeys, submoduleStatusByKey]) + }, [displaySections, expandedSubmoduleKeys, sourceControlViewMode, submoduleStatusByKey]) const branchTreeRoots = useMemo( - () => compactSourceControlTree(buildSourceControlTree('branch', filteredBranchEntries)), - [filteredBranchEntries] + () => + sourceControlViewMode === 'tree' + ? compactSourceControlTree(buildSourceControlTree('branch', filteredBranchEntries)) + : EMPTY_BRANCH_TREE_NODES, + [filteredBranchEntries, sourceControlViewMode] ) const visibleBranchTreeRows = useMemo( - () => flattenSourceControlTree(branchTreeRoots, collapsedTreeDirs), - [branchTreeRoots, collapsedTreeDirs] + () => + sourceControlViewMode === 'tree' + ? flattenSourceControlTree(branchTreeRoots, collapsedTreeDirs) + : EMPTY_BRANCH_TREE_NODES, + [branchTreeRoots, collapsedTreeDirs, sourceControlViewMode] ) const visibleSelectionEntries = useMemo(() => {