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