From e386eb0ecd24dab928f67fc2a545a10898d08c18 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Wed, 2 Sep 2026 03:31:32 -0700 Subject: [PATCH] perf(editor,source-control): batch closed-tab model sweeps, drop 40 store subscriptions Closing N diff tabs scanned the global Monaco model registry 2N times and rendered both URI forms for every retained model on each scan. The Source Control panel opened 42 store subscriptions from one hook, 40 of which watched action identities that are fixed at store construction and can never change. --- .../closed-editor-tab-cache-sweep.test.ts | 10 +- .../editor/closed-editor-tab-cache-sweep.ts | 45 +++- .../editor/closed-editor-tab-disposal.test.ts | 235 ++++++++++++++++++ .../editor/closed-editor-tab-disposal.ts | 86 +++++++ .../editor/diff-monaco-model-disposal.ts | 69 ++++- .../editor/useClosedEditorTabCleanup.ts | 70 +----- ...store-actions.store-subscriptions.test.tsx | 166 +++++++++++++ .../listing/use-store-actions.ts | 153 +++++------- 8 files changed, 654 insertions(+), 180 deletions(-) create mode 100644 src/renderer/src/components/editor/closed-editor-tab-disposal.test.ts create mode 100644 src/renderer/src/components/editor/closed-editor-tab-disposal.ts create mode 100644 src/renderer/src/components/right-sidebar/source-control/listing/use-store-actions.store-subscriptions.test.tsx diff --git a/src/renderer/src/components/editor/closed-editor-tab-cache-sweep.test.ts b/src/renderer/src/components/editor/closed-editor-tab-cache-sweep.test.ts index 1b975d81339..726795a31dd 100644 --- a/src/renderer/src/components/editor/closed-editor-tab-cache-sweep.test.ts +++ b/src/renderer/src/components/editor/closed-editor-tab-cache-sweep.test.ts @@ -7,7 +7,7 @@ const position = (pageNumber: number): PdfViewPosition => ({ pageNumber, top: 0, describe('sweepClosedPdfViewPositions', () => { it('deletes the unscoped :pdf entry', () => { const cache = new Map([['/a.pdf:pdf', position(4)]]) - sweepClosedPdfViewPositions(cache, '/a.pdf') + sweepClosedPdfViewPositions(cache, ['/a.pdf']) expect(cache.size).toBe(0) }) @@ -17,7 +17,7 @@ describe('sweepClosedPdfViewPositions', () => { ['/a.pdf::tab-2:pdf', position(9)], ['/a.pdf::tab-3:pdf', position(11)] ]) - sweepClosedPdfViewPositions(cache, '/a.pdf') + sweepClosedPdfViewPositions(cache, ['/a.pdf']) expect(cache.size).toBe(0) }) @@ -27,7 +27,7 @@ describe('sweepClosedPdfViewPositions', () => { ['/b.pdf:pdf', position(7)], ['/b.pdf::tab-2:pdf', position(8)] ]) - sweepClosedPdfViewPositions(cache, '/a.pdf') + sweepClosedPdfViewPositions(cache, ['/a.pdf']) expect([...cache.keys()]).toEqual(['/b.pdf:pdf', '/b.pdf::tab-2:pdf']) }) @@ -36,13 +36,13 @@ describe('sweepClosedPdfViewPositions', () => { ['/report.pdf:pdf', position(2)], ['/report.pdf.bak:pdf', position(3)] ]) - sweepClosedPdfViewPositions(cache, '/report.pdf') + sweepClosedPdfViewPositions(cache, ['/report.pdf']) expect([...cache.keys()]).toEqual(['/report.pdf.bak:pdf']) }) it('is a no-op when the file has no cached position', () => { const cache = new Map([['/b.pdf:pdf', position(7)]]) - sweepClosedPdfViewPositions(cache, '/a.pdf') + sweepClosedPdfViewPositions(cache, ['/a.pdf']) expect(cache.size).toBe(1) }) }) diff --git a/src/renderer/src/components/editor/closed-editor-tab-cache-sweep.ts b/src/renderer/src/components/editor/closed-editor-tab-cache-sweep.ts index d53f0212723..b9769fb9dc8 100644 --- a/src/renderer/src/components/editor/closed-editor-tab-cache-sweep.ts +++ b/src/renderer/src/components/editor/closed-editor-tab-cache-sweep.ts @@ -1,23 +1,50 @@ import type { PdfViewPosition } from '@/lib/scroll-cache' -function deleteCacheEntriesByPrefix(cache: Map, prefix: string): void { +/** + * Drops every pane-scoped (`::…`) entry belonging to any of `owners` in one pass over + * the cache, rather than one pass per owner. Split out from the cleanup hook so it is testable + * without pulling in the hook's `monaco-editor` import. + */ +export function deletePaneScopedCacheEntries( + cache: Map, + owners: readonly string[] +): void { + if (owners.length === 0) { + return + } + + const ownerSet = new Set(owners) for (const key of cache.keys()) { - if (key.startsWith(prefix)) { + if (hasPaneScopeOwner(key, ownerSet)) { cache.delete(key) } } } -/** - * Release the PDF positions a closed edit tab owns. Split out from the cleanup - * hook so it is testable without pulling in the hook's `monaco-editor` import. - */ +/** Equivalent to `key.startsWith(`${owner}::`)` for any owner in the set, probing `::` boundaries. */ +function hasPaneScopeOwner(key: string, owners: ReadonlySet): boolean { + for ( + let boundary = key.indexOf('::'); + boundary !== -1; + boundary = key.indexOf('::', boundary + 1) + ) { + if (owners.has(key.slice(0, boundary))) { + return true + } + } + + return false +} + +/** Release the PDF positions closed edit tabs own. */ export function sweepClosedPdfViewPositions( cache: Map, - filePath: string + filePaths: readonly string[] ): void { // Why: the `::`-scoped sweep does not cover the single-colon suffix, so the // unscoped key needs its own delete (same shape as :rich / :preview). - cache.delete(`${filePath}:pdf`) - deleteCacheEntriesByPrefix(cache, `${filePath}::`) + for (const filePath of filePaths) { + cache.delete(`${filePath}:pdf`) + } + deletePaneScopedCacheEntries(cache, filePaths) } diff --git a/src/renderer/src/components/editor/closed-editor-tab-disposal.test.ts b/src/renderer/src/components/editor/closed-editor-tab-disposal.test.ts new file mode 100644 index 00000000000..0223a4394b0 --- /dev/null +++ b/src/renderer/src/components/editor/closed-editor-tab-disposal.test.ts @@ -0,0 +1,235 @@ +import { beforeEach, describe, expect, it } from 'vitest' +import { + diffViewStateCache, + editorSelectionCache, + pdfViewPositionCache, + scrollTopCache +} from '@/lib/scroll-cache' +import type { OpenFile } from '@/store/slices/editor' +import { disposeClosedEditorTabs } from './closed-editor-tab-disposal' +import { + getDiffViewerMonacoModelPaths, + getDiffViewerMonacoModelPathPrefixes, + type MonacoModelRegistry +} from './diff-monaco-model-disposal' + +const CLOSED_DIFF_TAB_COUNT = 100 +const RETAINED_MODEL_COUNT = 320 + +type FakeModel = { + path: string + attached: boolean + disposed: boolean + dispose: () => void + isAttachedToEditor: () => boolean + uri: { toString: (skipEncoding?: boolean) => string } +} + +type FakeRegistry = MonacoModelRegistry & { + models: FakeModel[] + counters: { getModelsCalls: number; uriToStringCalls: number } +} + +function createRegistry(models: FakeModel[]): FakeRegistry { + const counters = { getModelsCalls: 0, uriToStringCalls: 0 } + const byPath = new Map(models.map((model) => [model.path, model])) + for (const model of models) { + model.uri.toString = () => { + counters.uriToStringCalls += 1 + return model.path + } + } + return { + models, + counters, + Uri: { parse: (value: string) => value }, + editor: { + getModel: (uri: unknown) => byPath.get(String(uri)) ?? null, + getModels: () => { + counters.getModelsCalls += 1 + return models + } + } + } +} + +function createModel(path: string, attached = false): FakeModel { + const model: FakeModel = { + path, + attached, + disposed: false, + dispose: () => { + model.disposed = true + }, + isAttachedToEditor: () => model.attached, + uri: { toString: () => path } + } + return model +} + +function diffTab(id: string): OpenFile { + return { id, mode: 'diff', filePath: `/repo/${id}.ts` } as OpenFile +} + +/** The pre-fix shape: one full registry scan, with both URI renderings, per owned prefix. */ +function disposeByPrefixPerTab(registry: FakeRegistry, prefixes: readonly string[]): void { + for (const prefix of prefixes) { + for (const model of registry.editor.getModels()) { + const uriString = model.uri.toString(true) + const encodedUriString = model.uri.toString() + if ( + uriString === prefix || + uriString.startsWith(`${prefix}:`) || + encodedUriString === prefix || + encodedUriString.startsWith(`${prefix}:`) + ) { + if (!model.isAttachedToEditor()) { + model.dispose() + } + } + } + } +} + +/** + * 100 closed diff tabs, of which 60 still hold retained models (some with a large-diff generation + * suffix, some attached), plus 200 unrelated retained models from other tabs. + */ +function buildScenario(): { + closedTabs: OpenFile[] + models: FakeModel[] + prefixes: string[] +} { + const closedTabs = Array.from({ length: CLOSED_DIFF_TAB_COUNT }, (_, i) => diffTab(`tab-${i}`)) + const models: FakeModel[] = [] + + for (let i = 0; i < 60; i += 1) { + const base = getDiffViewerMonacoModelPaths({ + modelKey: `tab-${i}`, + generationSuffix: '' + }) + models.push(createModel(base.originalModelPath, i % 10 === 0)) + models.push(createModel(base.modifiedModelPath)) + if (i % 3 === 0) { + const regenerated = getDiffViewerMonacoModelPaths({ + modelKey: `tab-${i}`, + generationSuffix: ':large-diff-generation:2' + }) + models.push(createModel(regenerated.originalModelPath)) + } + } + + // Still-open tabs and plain edit models the sweep must not touch. + for (let i = 0; models.length < RETAINED_MODEL_COUNT; i += 1) { + const stillOpen = getDiffViewerMonacoModelPaths({ + modelKey: `open-tab-${i}`, + generationSuffix: '' + }) + models.push(createModel(stillOpen.originalModelPath)) + models.push(createModel(`/repo/src/file-${i}.ts`)) + } + + const prefixes = closedTabs.flatMap((tab) => { + const { originalModelPathPrefix, modifiedModelPathPrefix } = + getDiffViewerMonacoModelPathPrefixes(tab.id) + return [originalModelPathPrefix, modifiedModelPathPrefix] + }) + + return { closedTabs, models, prefixes } +} + +beforeEach(() => { + scrollTopCache.clear() + editorSelectionCache.clear() + diffViewStateCache.clear() + pdfViewPositionCache.clear() +}) + +describe('disposeClosedEditorTabs', () => { + it('scans the model registry once per batch instead of twice per closed diff tab', () => { + const batched = buildScenario() + const batchedRegistry = createRegistry(batched.models) + disposeClosedEditorTabs(batchedRegistry, batched.closedTabs) + + const perTab = buildScenario() + const perTabRegistry = createRegistry(perTab.models) + disposeByPrefixPerTab(perTabRegistry, perTab.prefixes) + + // Pre-fix: 2 scans per closed tab, each rendering both URI forms for every retained model. + expect(perTabRegistry.counters.getModelsCalls).toBe(CLOSED_DIFF_TAB_COUNT * 2) + expect(perTabRegistry.counters.uriToStringCalls).toBe( + CLOSED_DIFF_TAB_COUNT * 2 * perTab.models.length * 2 + ) + + expect(batchedRegistry.counters.getModelsCalls).toBe(1) + expect(batchedRegistry.counters.uriToStringCalls).toBeLessThanOrEqual(batched.models.length * 2) + }) + + it('disposes exactly the models the per-tab sweep disposed', () => { + const batched = buildScenario() + disposeClosedEditorTabs(createRegistry(batched.models), batched.closedTabs) + + const perTab = buildScenario() + disposeByPrefixPerTab(createRegistry(perTab.models), perTab.prefixes) + + const disposedPaths = (models: FakeModel[]): string[] => + models + .filter((m) => m.disposed) + .map((m) => m.path) + .sort() + + expect(disposedPaths(batched.models)).toEqual(disposedPaths(perTab.models)) + expect(disposedPaths(batched.models).length).toBeGreaterThan(0) + // Attached models survive, as does everything owned by a still-open tab. + expect(batched.models.filter((m) => m.attached).every((m) => !m.disposed)).toBe(true) + expect( + batched.models.filter((m) => m.path.includes('open-tab-')).every((m) => !m.disposed) + ).toBe(true) + }) + + it('sweeps pane-scoped cache entries for closed edit tabs in one pass per cache', () => { + scrollTopCache.set('/repo/a.ts', 10) + scrollTopCache.set('/repo/a.ts::pane-1', 20) + scrollTopCache.set('/repo/a.ts:rich', 30) + scrollTopCache.set('/repo/b.ts::pane-1', 40) + editorSelectionCache.set('/repo/a.ts::pane-2', [] as never) + pdfViewPositionCache.set('/repo/a.ts:pdf', { + pageNumber: 1, + top: 0, + left: 0 + }) + pdfViewPositionCache.set('/repo/a.ts::pane-1:pdf', { + pageNumber: 2, + top: 0, + left: 0 + }) + + disposeClosedEditorTabs(createRegistry([]), [ + { id: '/repo/a.ts', mode: 'edit', filePath: '/repo/a.ts' } as OpenFile + ]) + + expect([...scrollTopCache.keys()]).toEqual(['/repo/b.ts::pane-1']) + expect(editorSelectionCache.size).toBe(0) + expect(pdfViewPositionCache.size).toBe(0) + }) + + it('drops diff view state and preview scroll entries for closed diff tabs', () => { + diffViewStateCache.set('tab-1', {} as never) + diffViewStateCache.set('tab-1::pane-1', {} as never) + diffViewStateCache.set('tab-10', {} as never) + scrollTopCache.set('tab-1:preview', 5) + scrollTopCache.set('tab-1::pane-1', 6) + + disposeClosedEditorTabs(createRegistry([]), [diffTab('tab-1')]) + + expect([...diffViewStateCache.keys()]).toEqual(['tab-10']) + expect(scrollTopCache.size).toBe(0) + }) + + it('is a no-op when nothing closed', () => { + const registry = createRegistry([createModel('diff:original:tab-1:tab-1')]) + disposeClosedEditorTabs(registry, []) + expect(registry.counters.getModelsCalls).toBe(0) + expect(registry.models[0].disposed).toBe(false) + }) +}) diff --git a/src/renderer/src/components/editor/closed-editor-tab-disposal.ts b/src/renderer/src/components/editor/closed-editor-tab-disposal.ts new file mode 100644 index 00000000000..ddde3a74502 --- /dev/null +++ b/src/renderer/src/components/editor/closed-editor-tab-disposal.ts @@ -0,0 +1,86 @@ +import type { OpenFile } from '@/store/slices/editor' +import { + editorSelectionCache, + diffViewStateCache, + pdfViewPositionCache, + scrollTopCache +} from '@/lib/scroll-cache' +import { + disposeUnattachedMonacoModelsByPathPrefixes, + getDiffViewerMonacoModelPathPrefixes, + type MonacoModelRegistry +} from './diff-monaco-model-disposal' +import { + deletePaneScopedCacheEntries, + sweepClosedPdfViewPositions +} from './closed-editor-tab-cache-sweep' + +/** + * Releases the Monaco models and view-state cache entries owned by a batch of closed tabs. + * + * Why the batch shape: every prefix sweep here is a full scan of a shared registry or cache, so + * doing one per closed tab makes "close all"/worktree-switch quadratic in retained models. Takes + * the monaco namespace as an argument so it stays testable without importing `monaco-editor`. + */ +export function disposeClosedEditorTabs( + monacoRegistry: MonacoModelRegistry, + closedFiles: readonly OpenFile[] +): void { + if (closedFiles.length === 0) { + return + } + + const diffModelPathPrefixes: string[] = [] + const scrollTopOwners: string[] = [] + const editorSelectionOwners: string[] = [] + const diffViewStateOwners: string[] = [] + const closedPdfFilePaths: string[] = [] + + for (const closedFile of closedFiles) { + switch (closedFile.mode) { + case 'edit': + // Why: the edit model URI is constructed via monaco.Uri.parse(filePath) + // to match @monaco-editor/react's `path` prop convention. + monacoRegistry.editor.getModel(monacoRegistry.Uri.parse(closedFile.filePath))?.dispose() + scrollTopCache.delete(closedFile.filePath) + // Why: markdown and mermaid surfaces keep mode-scoped scroll positions. + scrollTopCache.delete(`${closedFile.filePath}:rich`) + scrollTopCache.delete(`${closedFile.filePath}:preview`) + scrollTopCache.delete(`${closedFile.filePath}:mermaid-diagram`) + editorSelectionCache.delete(closedFile.filePath) + scrollTopOwners.push(closedFile.filePath) + editorSelectionOwners.push(closedFile.filePath) + // Why: only 'edit' tabs ever get a PDF scroll key (see EditorContent). + closedPdfFilePaths.push(closedFile.filePath) + break + case 'markdown-preview': + // Why: preview tabs own pane-scoped preview scroll cache entries even + // though they do not retain Monaco models. + scrollTopCache.delete(`${closedFile.id}:preview`) + scrollTopOwners.push(closedFile.id) + break + case 'diff': { + // Why: kept diff models are keyed by tab id, and fallback recovery can + // append generation suffixes; closing the tab owns that whole namespace. + const { originalModelPathPrefix, modifiedModelPathPrefix } = + getDiffViewerMonacoModelPathPrefixes(closedFile.id) + diffModelPathPrefixes.push(originalModelPathPrefix, modifiedModelPathPrefix) + diffViewStateCache.delete(closedFile.id) + diffViewStateOwners.push(closedFile.id) + scrollTopCache.delete(`${closedFile.id}:preview`) + scrollTopOwners.push(closedFile.id) + break + } + case 'conflict-review': + break + case 'check-details': + break + } + } + + disposeUnattachedMonacoModelsByPathPrefixes(monacoRegistry, diffModelPathPrefixes) + deletePaneScopedCacheEntries(scrollTopCache, scrollTopOwners) + deletePaneScopedCacheEntries(editorSelectionCache, editorSelectionOwners) + deletePaneScopedCacheEntries(diffViewStateCache, diffViewStateOwners) + sweepClosedPdfViewPositions(pdfViewPositionCache, closedPdfFilePaths) +} diff --git a/src/renderer/src/components/editor/diff-monaco-model-disposal.ts b/src/renderer/src/components/editor/diff-monaco-model-disposal.ts index 8252abc8381..f833e53ec5b 100644 --- a/src/renderer/src/components/editor/diff-monaco-model-disposal.ts +++ b/src/renderer/src/components/editor/diff-monaco-model-disposal.ts @@ -16,7 +16,7 @@ type DisposableMonacoModel = Pick, + bounds: { shortestPrefixLength: number; longestPrefixLength: number } +): boolean { + if (ownedPrefixes.has(uriString)) { + return true + } + + for ( + let boundary = uriString.indexOf(':'); + boundary !== -1 && boundary <= bounds.longestPrefixLength; + boundary = uriString.indexOf(':', boundary + 1) + ) { + if ( + boundary >= bounds.shortestPrefixLength && + ownedPrefixes.has(uriString.slice(0, boundary)) + ) { + return true + } + } + + return false +} + function disposeUnattachedMonacoModel(model: DisposableMonacoModel | null): void { if (!model || model.isAttachedToEditor()) { return diff --git a/src/renderer/src/components/editor/useClosedEditorTabCleanup.ts b/src/renderer/src/components/editor/useClosedEditorTabCleanup.ts index 56e25045353..e86fbbc43f6 100644 --- a/src/renderer/src/components/editor/useClosedEditorTabCleanup.ts +++ b/src/renderer/src/components/editor/useClosedEditorTabCleanup.ts @@ -1,80 +1,22 @@ import { useEffect, useRef } from 'react' import * as monaco from 'monaco-editor' import type { OpenFile } from '@/store/slices/editor' -import { - editorSelectionCache, - diffViewStateCache, - pdfViewPositionCache, - scrollTopCache -} from '@/lib/scroll-cache' -import { - disposeUnattachedMonacoModelsByPathPrefix, - getDiffViewerMonacoModelPathPrefixes -} from './diff-monaco-model-disposal' -import { sweepClosedPdfViewPositions } from './closed-editor-tab-cache-sweep' - -function deleteCacheEntriesByPrefix(cache: Map, prefix: string): void { - for (const key of cache.keys()) { - if (key.startsWith(prefix)) { - cache.delete(key) - } - } -} +import { disposeClosedEditorTabs } from './closed-editor-tab-disposal' export function useClosedEditorTabCleanup(openFiles: OpenFile[]): void { const prevOpenFilesRef = useRef>(new Map()) useEffect(() => { const currentFilesById = new Map(openFiles.map((f) => [f.id, f])) + const closedFiles: OpenFile[] = [] for (const [prevId, prevFile] of prevOpenFilesRef.current) { if (!currentFilesById.has(prevId)) { - disposeClosedEditorTab(prevId, prevFile) + closedFiles.push(prevFile) } } + // Why one call for the whole removal batch: each sweep scans a shared registry/cache, so + // per-tab sweeps make a "close all" quadratic in retained models. + disposeClosedEditorTabs(monaco, closedFiles) prevOpenFilesRef.current = currentFilesById }, [openFiles]) } - -function disposeClosedEditorTab(prevId: string, prevFile: OpenFile): void { - switch (prevFile.mode) { - case 'edit': - // Why: the edit model URI is constructed via monaco.Uri.parse(filePath) - // to match @monaco-editor/react's `path` prop convention. - monaco.editor.getModel(monaco.Uri.parse(prevFile.filePath))?.dispose() - scrollTopCache.delete(prevFile.filePath) - deleteCacheEntriesByPrefix(scrollTopCache, `${prevFile.filePath}::`) - // Why: markdown and mermaid surfaces keep mode-scoped scroll positions. - scrollTopCache.delete(`${prevFile.filePath}:rich`) - scrollTopCache.delete(`${prevFile.filePath}:preview`) - scrollTopCache.delete(`${prevFile.filePath}:mermaid-diagram`) - editorSelectionCache.delete(prevFile.filePath) - deleteCacheEntriesByPrefix(editorSelectionCache, `${prevFile.filePath}::`) - // Why: only 'edit' tabs ever get a PDF scroll key (see EditorContent). - sweepClosedPdfViewPositions(pdfViewPositionCache, prevFile.filePath) - break - case 'markdown-preview': - // Why: preview tabs own pane-scoped preview scroll cache entries even - // though they do not retain Monaco models. - scrollTopCache.delete(`${prevFile.id}:preview`) - deleteCacheEntriesByPrefix(scrollTopCache, `${prevFile.id}::`) - break - case 'diff': - // Why: kept diff models are keyed by tab id, and fallback recovery can - // append generation suffixes; closing the tab owns that whole namespace. - { - const { originalModelPathPrefix, modifiedModelPathPrefix } = - getDiffViewerMonacoModelPathPrefixes(prevId) - disposeUnattachedMonacoModelsByPathPrefix(monaco, originalModelPathPrefix) - disposeUnattachedMonacoModelsByPathPrefix(monaco, modifiedModelPathPrefix) - } - diffViewStateCache.delete(prevId) - deleteCacheEntriesByPrefix(diffViewStateCache, `${prevId}::`) - scrollTopCache.delete(`${prevId}:preview`) - deleteCacheEntriesByPrefix(scrollTopCache, `${prevId}::`) - break - case 'conflict-review': - break - case 'check-details': - break - } -} diff --git a/src/renderer/src/components/right-sidebar/source-control/listing/use-store-actions.store-subscriptions.test.tsx b/src/renderer/src/components/right-sidebar/source-control/listing/use-store-actions.store-subscriptions.test.tsx new file mode 100644 index 00000000000..dca92f903f1 --- /dev/null +++ b/src/renderer/src/components/right-sidebar/source-control/listing/use-store-actions.store-subscriptions.test.tsx @@ -0,0 +1,166 @@ +// @vitest-environment happy-dom + +import { act, useState, type ReactNode } from 'react' +import { createRoot, type Root } from 'react-dom/client' +import { afterEach, describe, expect, it } from 'vitest' +import { useAppStore } from '@/store' +import { readStoreListenerCount } from '@/store/store-listener-census' +import { useSourceControlStoreActions, type SourceControlStoreActions } from './use-store-actions' + +const originalState = useAppStore.getState() + +let root: Root | null = null +let container: HTMLDivElement | null = null + +function mount(node: ReactNode): void { + container = document.createElement('div') + document.body.appendChild(container) + root = createRoot(container) + act(() => root?.render(node)) +} + +function unmount(): void { + if (root) { + act(() => root?.unmount()) + } + root = null + container?.remove() + container = null +} + +function listenerCount(): number { + const count = readStoreListenerCount() + if (count === null) { + throw new Error('store listener census unavailable') + } + return count +} + +afterEach(() => { + unmount() + useAppStore.setState(originalState, true) +}) + +/** Everything the hook returns that is a store action rather than subscribed state. */ +const ACTION_KEYS = Object.keys(originalState).filter( + (key) => typeof (originalState as Record)[key] === 'function' +) + +describe('useSourceControlStoreActions store subscriptions', () => { + it('keeps only the two generation-record maps subscribed', () => { + const baseline = listenerCount() + + function Probe(): null { + useSourceControlStoreActions() + return null + } + mount() + + // Why 2: `pullRequestGenerationRecords` and `commitMessageGenerationRecords` are the only + // entries that are state; the other 40 are actions read through getState(). + expect(listenerCount() - baseline).toBe(2) + + unmount() + expect(listenerCount()).toBe(baseline) + }) + + it('returns the same object across an unrelated store write and re-render', () => { + let latest: SourceControlStoreActions | null = null + let rerender: (() => void) | null = null + + function Probe(): null { + const [, setTick] = useState(0) + rerender = () => setTick((t) => t + 1) + latest = useSourceControlStoreActions() + return null + } + mount() + + const first = latest + expect(first).not.toBeNull() + + act(() => { + useAppStore.setState({ + rightSidebarOpen: !originalState.rightSidebarOpen + }) + }) + act(() => rerender?.()) + + expect(latest).toBe(first) + }) + + it('still tracks the generation-record maps it subscribes to', () => { + let latest: SourceControlStoreActions | null = null + function Probe(): null { + latest = useSourceControlStoreActions() + return null + } + function read(): SourceControlStoreActions { + if (!latest) { + throw new Error('probe did not render') + } + return latest + } + mount() + + const before = read() + const record = { status: 'pending' } as never + act(() => { + useAppStore.setState({ + pullRequestGenerationRecords: { 'wt-1': record } + }) + }) + + expect(read()).not.toBe(before) + expect(read().prGenerationRecords).toEqual({ 'wt-1': record }) + + const afterPr = read() + act(() => { + useAppStore.setState({ + commitMessageGenerationRecords: { 'wt-1': record } + }) + }) + expect(read()).not.toBe(afterPr) + expect(read().commitMessageGenerationRecords).toEqual({ 'wt-1': record }) + }) + + it('hands back the live store action references', () => { + let latest: SourceControlStoreActions | null = null + function Probe(): null { + latest = useSourceControlStoreActions() + return null + } + mount() + + const state = useAppStore.getState() as unknown as Record + const returned = latest as unknown as Record + const returnedActionKeys = Object.keys(returned).filter( + (key) => typeof returned[key] === 'function' + ) + + expect(returnedActionKeys.length).toBe(40) + for (const key of returnedActionKeys) { + expect(returned[key]).toBe(state[key]) + } + }) + + it('never reassigns a store action, which is what makes getState() safe here', () => { + const before = useAppStore.getState() as unknown as Record + const snapshot = new Map(ACTION_KEYS.map((key) => [key, before[key]])) + + // Drive real writes through several slices, then confirm no action identity moved. + act(() => { + useAppStore.getState().setRightSidebarOpen(true) + useAppStore.getState().setRightSidebarTab('source-control') + useAppStore.getState().allocatePullRequestGenerationRequestId() + useAppStore.getState().setPullRequestGenerationRecord('wt-1', { status: 'pending' } as never) + useAppStore.getState().setCommitMessageGenerationRecord('wt-1', { + status: 'pending' + } as never) + }) + + const after = useAppStore.getState() as unknown as Record + const moved = ACTION_KEYS.filter((key) => after[key] !== snapshot.get(key)) + expect(moved).toEqual([]) + }) +}) diff --git a/src/renderer/src/components/right-sidebar/source-control/listing/use-store-actions.ts b/src/renderer/src/components/right-sidebar/source-control/listing/use-store-actions.ts index 230cc4aebeb..d85c7cd04d8 100644 --- a/src/renderer/src/components/right-sidebar/source-control/listing/use-store-actions.ts +++ b/src/renderer/src/components/right-sidebar/source-control/listing/use-store-actions.ts @@ -1,105 +1,70 @@ +import { useMemo } from 'react' import { useAppStore } from '@/store' /** - * Binds every store action the Source Control panel dispatches. Each entry keeps its own selector so - * the returned references stay stable and can be used directly in downstream dependency arrays. + * Binds every store action the Source Control panel dispatches. + * + * Why `getState()` and not one selector each: zustand action identities are fixed when the store is + * built and no slice ever puts one in a `set()` payload, so subscribing to them can never fire. The + * 40 action subscriptions only added 40 live listeners and 40 selector runs to every store write + * while the panel was mounted. The two generation-record maps are real state, so they stay + * subscribed. + * + * Reference stability is preserved and slightly stronger than before: each action keeps the single + * identity it was created with, and the returned object itself is now stable until one of the two + * subscribed maps changes, so downstream dependency arrays keep working. */ export function useSourceControlStoreActions() { - const updateSettings = useAppStore((s) => s.updateSettings) - const openSettingsTarget = useAppStore((s) => s.openSettingsTarget) - const openSettingsPage = useAppStore((s) => s.openSettingsPage) - const fetchHostedReviewForBranch = useAppStore((s) => s.fetchHostedReviewForBranch) - const getHostedReviewCreationEligibility = useAppStore( - (s) => s.getHostedReviewCreationEligibility - ) - const createHostedReview = useAppStore((s) => s.createHostedReview) - const createStackedHostedReview = useAppStore((s) => s.createStackedHostedReview) - const updateWorktreeMeta = useAppStore((s) => s.updateWorktreeMeta) - const openModal = useAppStore((s) => s.openModal) - const fetchPRForBranch = useAppStore((s) => s.fetchPRForBranch) - const enqueueGitHubPRRefresh = useAppStore((s) => s.enqueueGitHubPRRefresh) - const updateRepo = useAppStore((s) => s.updateRepo) - const setGitStatus = useAppStore((s) => s.setGitStatus) - const updateWorktreeGitIdentity = useAppStore((s) => s.updateWorktreeGitIdentity) - const beginGitBranchCompareRequest = useAppStore((s) => s.beginGitBranchCompareRequest) - const setGitBranchCompareResult = useAppStore((s) => s.setGitBranchCompareResult) - const fetchUpstreamStatus = useAppStore((s) => s.fetchUpstreamStatus) - const ensureHostedReviewPushTarget = useAppStore((s) => s.ensureHostedReviewPushTarget) - const setUpstreamStatus = useAppStore((s) => s.setUpstreamStatus) - const pushBranch = useAppStore((s) => s.pushBranch) - const pullBranch = useAppStore((s) => s.pullBranch) - const fastForwardBranch = useAppStore((s) => s.fastForwardBranch) - const syncBranch = useAppStore((s) => s.syncBranch) - const rebaseFromBase = useAppStore((s) => s.rebaseFromBase) - const fetchBranch = useAppStore((s) => s.fetchBranch) - const revealInExplorer = useAppStore((s) => s.revealInExplorer) - const openConflictReview = useAppStore((s) => s.openConflictReview) - const openAllDiffs = useAppStore((s) => s.openAllDiffs) - const openBranchAllDiffs = useAppStore((s) => s.openBranchAllDiffs) - const deleteDiffComment = useAppStore((s) => s.deleteDiffComment) - const clearDiffComments = useAppStore((s) => s.clearDiffComments) - const clearDiffCommentsForFile = useAppStore((s) => s.clearDiffCommentsForFile) - const setRightSidebarOpen = useAppStore((s) => s.setRightSidebarOpen) - const setRightSidebarTab = useAppStore((s) => s.setRightSidebarTab) const prGenerationRecords = useAppStore((s) => s.pullRequestGenerationRecords) - const allocatePullRequestGenerationRequestId = useAppStore( - (s) => s.allocatePullRequestGenerationRequestId - ) - const setPullRequestGenerationRecord = useAppStore((s) => s.setPullRequestGenerationRecord) - const updatePullRequestGenerationRecord = useAppStore((s) => s.updatePullRequestGenerationRecord) const commitMessageGenerationRecords = useAppStore((s) => s.commitMessageGenerationRecords) - const allocateCommitMessageGenerationRequestId = useAppStore( - (s) => s.allocateCommitMessageGenerationRequestId - ) - const setCommitMessageGenerationRecord = useAppStore((s) => s.setCommitMessageGenerationRecord) - const updateCommitMessageGenerationRecord = useAppStore( - (s) => s.updateCommitMessageGenerationRecord - ) - return { - allocateCommitMessageGenerationRequestId, - allocatePullRequestGenerationRequestId, - beginGitBranchCompareRequest, - clearDiffComments, - clearDiffCommentsForFile, - commitMessageGenerationRecords, - createHostedReview, - createStackedHostedReview, - deleteDiffComment, - enqueueGitHubPRRefresh, - ensureHostedReviewPushTarget, - fastForwardBranch, - fetchBranch, - fetchHostedReviewForBranch, - fetchPRForBranch, - fetchUpstreamStatus, - getHostedReviewCreationEligibility, - openAllDiffs, - openBranchAllDiffs, - openConflictReview, - openModal, - openSettingsPage, - openSettingsTarget, - prGenerationRecords, - pullBranch, - pushBranch, - rebaseFromBase, - revealInExplorer, - setCommitMessageGenerationRecord, - setGitBranchCompareResult, - setGitStatus, - setPullRequestGenerationRecord, - setRightSidebarOpen, - setRightSidebarTab, - setUpstreamStatus, - syncBranch, - updateCommitMessageGenerationRecord, - updatePullRequestGenerationRecord, - updateRepo, - updateSettings, - updateWorktreeGitIdentity, - updateWorktreeMeta - } + return useMemo(() => { + const state = useAppStore.getState() + return { + allocateCommitMessageGenerationRequestId: state.allocateCommitMessageGenerationRequestId, + allocatePullRequestGenerationRequestId: state.allocatePullRequestGenerationRequestId, + beginGitBranchCompareRequest: state.beginGitBranchCompareRequest, + clearDiffComments: state.clearDiffComments, + clearDiffCommentsForFile: state.clearDiffCommentsForFile, + commitMessageGenerationRecords, + createHostedReview: state.createHostedReview, + createStackedHostedReview: state.createStackedHostedReview, + deleteDiffComment: state.deleteDiffComment, + enqueueGitHubPRRefresh: state.enqueueGitHubPRRefresh, + ensureHostedReviewPushTarget: state.ensureHostedReviewPushTarget, + fastForwardBranch: state.fastForwardBranch, + fetchBranch: state.fetchBranch, + fetchHostedReviewForBranch: state.fetchHostedReviewForBranch, + fetchPRForBranch: state.fetchPRForBranch, + fetchUpstreamStatus: state.fetchUpstreamStatus, + getHostedReviewCreationEligibility: state.getHostedReviewCreationEligibility, + openAllDiffs: state.openAllDiffs, + openBranchAllDiffs: state.openBranchAllDiffs, + openConflictReview: state.openConflictReview, + openModal: state.openModal, + openSettingsPage: state.openSettingsPage, + openSettingsTarget: state.openSettingsTarget, + prGenerationRecords, + pullBranch: state.pullBranch, + pushBranch: state.pushBranch, + rebaseFromBase: state.rebaseFromBase, + revealInExplorer: state.revealInExplorer, + setCommitMessageGenerationRecord: state.setCommitMessageGenerationRecord, + setGitBranchCompareResult: state.setGitBranchCompareResult, + setGitStatus: state.setGitStatus, + setPullRequestGenerationRecord: state.setPullRequestGenerationRecord, + setRightSidebarOpen: state.setRightSidebarOpen, + setRightSidebarTab: state.setRightSidebarTab, + setUpstreamStatus: state.setUpstreamStatus, + syncBranch: state.syncBranch, + updateCommitMessageGenerationRecord: state.updateCommitMessageGenerationRecord, + updatePullRequestGenerationRecord: state.updatePullRequestGenerationRecord, + updateRepo: state.updateRepo, + updateSettings: state.updateSettings, + updateWorktreeGitIdentity: state.updateWorktreeGitIdentity, + updateWorktreeMeta: state.updateWorktreeMeta + } + }, [commitMessageGenerationRecords, prGenerationRecords]) } export type SourceControlStoreActions = ReturnType