diff --git a/config/oxlint-plugins/app-store-performance.mjs b/config/oxlint-plugins/app-store-performance.mjs index 2fc37e07ddb..d8bfe4131d9 100644 --- a/config/oxlint-plugins/app-store-performance.mjs +++ b/config/oxlint-plugins/app-store-performance.mjs @@ -8,6 +8,19 @@ const ALLOCATING_METHODS = new Set([ 'toSpliced', 'with' ]) +const ALLOCATING_OBJECT_STATICS = new Set([ + 'assign', + 'create', + 'entries', + 'fromEntries', + 'keys', + 'values' +]) +const FUNCTION_NODES = new Set([ + 'ArrowFunctionExpression', + 'FunctionDeclaration', + 'FunctionExpression' +]) function identifierName(node) { return node?.type === 'Identifier' ? node.name : null @@ -25,8 +38,12 @@ function propertyName(node) { : null } +function functionNode(node) { + return FUNCTION_NODES.has(node?.type) ? node : null +} + function returnedExpressions(selector) { - if (selector?.type !== 'ArrowFunctionExpression' && selector?.type !== 'FunctionExpression') { + if (!functionNode(selector)) { return [] } if (selector.body.type !== 'BlockStatement') { @@ -37,10 +54,7 @@ function returnedExpressions(selector) { if (!node || typeof node !== 'object') { return } - if ( - node !== selector.body && - ['ArrowFunctionExpression', 'FunctionDeclaration', 'FunctionExpression'].includes(node.type) - ) { + if (node !== selector.body && FUNCTION_NODES.has(node.type)) { return } if (node.type === 'ReturnStatement') { @@ -76,10 +90,7 @@ function unwrapShallowSelector(selector, shallowHooks) { } function isIdentitySelector(selector) { - if (selector?.type !== 'ArrowFunctionExpression' && selector?.type !== 'FunctionExpression') { - return false - } - const parameter = selector.params[0] + const parameter = functionNode(selector)?.params[0] if (parameter?.type !== 'Identifier') { return false } @@ -88,14 +99,23 @@ function isIdentitySelector(selector) { ) } -function isAllocatingExpression(expression) { - if (expression?.type === 'ConditionalExpression') { - return ( - isAllocatingExpression(expression.consequent) || isAllocatingExpression(expression.alternate) - ) - } - if (expression?.type === 'LogicalExpression') { - return isAllocatingExpression(expression.left) || isAllocatingExpression(expression.right) +/** + * `everyBranch` decides how a conditional counts. An inline selector is flagged + * when ANY branch allocates; a helper the selector delegates to must allocate on + * EVERY branch, so the `cache.get(k) ?? build(state)` identity-caching shape is + * not a false positive. + */ +function allocates(expression, everyBranch) { + const branches = + expression?.type === 'ConditionalExpression' + ? [expression.consequent, expression.alternate] + : expression?.type === 'LogicalExpression' + ? [expression.left, expression.right] + : null + if (branches) { + return everyBranch + ? branches.every((branch) => allocates(branch, true)) + : branches.some((branch) => allocates(branch, false)) } if ( expression?.type === 'ArrayExpression' || @@ -107,23 +127,16 @@ function isAllocatingExpression(expression) { if (expression?.type !== 'CallExpression') { return false } - const method = propertyName(expression.callee) - if (method && ALLOCATING_METHODS.has(method)) { - return true - } const callee = expression.callee + const method = propertyName(callee) return ( - callee.type === 'MemberExpression' && - identifierName(callee.object) === 'Object' && - ['assign', 'create', 'entries', 'fromEntries', 'keys', 'values'].includes(propertyName(callee)) + ALLOCATING_METHODS.has(method) || + (identifierName(callee.object) === 'Object' && ALLOCATING_OBJECT_STATICS.has(method)) ) } -function importedLocalName(specifier, importedName) { - if (specifier.type !== 'ImportSpecifier' || identifierName(specifier.imported) !== importedName) { - return null - } - return identifierName(specifier.local) +function isAllocatingExpression(expression) { + return allocates(expression, false) } // Project-local zustand hooks follow the useStore convention; React's @@ -135,29 +148,27 @@ function isLocalModuleSource(source) { return typeof source === 'string' && (source.startsWith('.') || source.startsWith('@/')) } -function isStoreHookName(name) { - return typeof name === 'string' && STORE_HOOK_NAME.test(name) && !NON_STORE_HOOKS.has(name) -} - -function selectorFunction(node) { - return node?.type === 'ArrowFunctionExpression' || node?.type === 'FunctionExpression' - ? node - : null +/** Module scope only: a component-local helper must not shadow a same-named import. */ +function isModuleScope(node) { + const parent = node.parent + return ( + parent?.type === 'Program' || + (parent?.type === 'ExportNamedDeclaration' && parent.parent?.type === 'Program') + ) } /** Records module-scope `const selectX = (state) => ...` so identifier selectors resolve. */ function recordNamedSelector(node, state) { - if (node.type === 'FunctionDeclaration') { - const name = identifierName(node.id) - if (name) { - state.namedSelectors.set(name, node) - } + if (!isModuleScope(node)) { return } - for (const declarator of node.declarations ?? []) { - const name = identifierName(declarator.id) - const initializer = selectorFunction(declarator.init) - if (name && initializer) { + const declared = + node.type === 'FunctionDeclaration' + ? [[node.id, node]] + : node.declarations.map((declarator) => [declarator.id, declarator.init]) + for (const [id, initializer] of declared) { + const name = identifierName(id) + if (name && functionNode(initializer)) { state.namedSelectors.set(name, initializer) } } @@ -165,51 +176,23 @@ function recordNamedSelector(node, state) { /** Inline function, or a module-scope selector referenced by name. */ function resolveSelector(argument, state) { - const inline = selectorFunction(argument) - if (inline) { - return inline - } - const name = identifierName(argument) - return name ? (state.namedSelectors.get(name) ?? null) : null -} - -/** - * Allocation that happens on EVERY call, used when following a selector into a - * helper. Deliberately stricter than isAllocatingExpression: a helper that returns - * a cached reference on one branch and builds a fresh one on another is the normal - * identity-caching shape, and flagging it would be a false positive. - */ -function alwaysAllocates(expression) { - if (expression?.type === 'ConditionalExpression') { - return alwaysAllocates(expression.consequent) && alwaysAllocates(expression.alternate) - } - if (expression?.type === 'LogicalExpression') { - return alwaysAllocates(expression.left) && alwaysAllocates(expression.right) - } - return ( - expression?.type === 'ArrayExpression' || - expression?.type === 'ObjectExpression' || - expression?.type === 'NewExpression' || - (expression?.type === 'CallExpression' && - ALLOCATING_METHODS.has(propertyName(expression.callee) ?? '')) - ) + return functionNode(argument) ?? state.namedSelectors.get(identifierName(argument)) ?? null } /** * One hop: a selector that delegates to a module-scope helper is the idiomatic * shape here, and neither the inline-body check nor a reviewer reading the call - * site can see what that helper returns. + * site can see what that helper returns. An unresolvable helper is left alone. */ function expandThroughNamedHelper(expression, state) { - if (expression?.type !== 'CallExpression') { - return [expression] - } - const helper = state.namedSelectors.get(identifierName(expression.callee) ?? '') - if (!helper) { - return [expression] - } - const returned = returnedExpressions(helper) - return returned.length > 0 && returned.every(alwaysAllocates) ? returned : [expression] + const helper = + expression?.type === 'CallExpression' + ? state.namedSelectors.get(identifierName(expression.callee)) + : undefined + const returned = helper ? returnedExpressions(helper) : [] + return returned.length > 0 && returned.every((entry) => allocates(entry, true)) + ? returned + : [expression] } function createRuleState() { @@ -222,32 +205,27 @@ function createRuleState() { } function recordImports(node, state) { - if (node.source?.value === 'zustand/react/shallow') { - for (const specifier of node.specifiers) { - const localName = importedLocalName(specifier, 'useShallow') - if (localName) { - state.shallowHooks.add(localName) - } - } - } - for (const specifier of node.specifiers) { - const localName = importedLocalName(specifier, 'useAppStore') - if (localName) { - state.appStoreHooks.add(localName) - } - } - if (!isLocalModuleSource(node.source?.value)) { - return - } + const source = node.source?.value for (const specifier of node.specifiers) { if (specifier.type !== 'ImportSpecifier') { continue } - if (isStoreHookName(identifierName(specifier.imported))) { - const localName = identifierName(specifier.local) - if (localName) { - state.appStoreHooks.add(localName) - } + const imported = identifierName(specifier.imported) + const localName = identifierName(specifier.local) + if (!imported || !localName) { + continue + } + if (source === 'zustand/react/shallow' && imported === 'useShallow') { + state.shallowHooks.add(localName) + } + // useAppStore is the app store wherever it is re-exported from; sibling + // stores are trusted by naming convention only when they come from this codebase. + if ( + STORE_HOOK_NAME.test(imported) && + !NON_STORE_HOOKS.has(imported) && + (imported === 'useAppStore' || isLocalModuleSource(source)) + ) { + state.appStoreHooks.add(localName) } } } @@ -307,9 +285,7 @@ function deferredSelectorRule(inspect) { ) const report = inspect({ selector: resolveSelector(argument, state), - argument, shallow, - node, state }) if (report) { diff --git a/config/scripts/app-store-performance-plugin.test.mjs b/config/scripts/app-store-performance-plugin.test.mjs index 649b246e4e7..d8e2568165f 100644 --- a/config/scripts/app-store-performance-plugin.test.mjs +++ b/config/scripts/app-store-performance-plugin.test.mjs @@ -68,6 +68,20 @@ describe('app store performance Oxlint plugin', () => { ]) }) + it('does not let a component-local helper resolve a same-named imported selector', () => { + const diagnostics = lintSource(` + import { useAppStore } from '@/store' + import { selectRows } from './selectors' + const Other = () => { + const selectRows = (state) => state.rows.map((row) => row.id) + return selectRows + } + const Imported = () => useAppStore(selectRows) + `) + + expect(diagnostics).toEqual([]) + }) + it('covers sibling store hooks but not useSyncExternalStore', () => { const diagnostics = lintSource(` import { usePluginPanelsStore } from '@/store/plugin-panels' diff --git a/src/renderer/src/store/index.ts b/src/renderer/src/store/index.ts index dea29f384c3..5f783852f82 100644 --- a/src/renderer/src/store/index.ts +++ b/src/renderer/src/store/index.ts @@ -1,4 +1,4 @@ -import { create } from 'zustand' +import { create, type StateCreator } from 'zustand' import type { AppState } from './types' import { createRepoSlice } from './slices/repos' import { createSparsePresetsSlice } from './slices/sparse-presets' @@ -60,8 +60,16 @@ import { } from '@/lib/renderer-memory-profile' import { estimateStateCollectionKB } from '@/lib/state-collection-byte-estimate' +// Why dev-only: nothing in the app arms the churn probe, so a shipped build would +// pay its wrapper frame on every write for a diagnostic it can never read. The +// cascade probe stays unconditional because crash telemetry arms it in the field. +const withDevelopmentStoreProbes = (createState: StateCreator) => + import.meta.env.DEV || e2eConfig.exposeStore + ? withStoreIdentityChurnProbe(createState) + : createState + export const useAppStore = create()( - withStoreIdentityChurnProbe( + withDevelopmentStoreProbes( withReactCommitCascadeWriteProbe((...a) => { // Why: the inner api is only reachable here, before create() copies subscribe onto the hook. installStoreListenerCensus(a[2]) diff --git a/src/renderer/src/store/store-identity-churn-probe.test.ts b/src/renderer/src/store/store-identity-churn-probe.test.ts index f51d8c87a43..04ae08e12d3 100644 --- a/src/renderer/src/store/store-identity-churn-probe.test.ts +++ b/src/renderer/src/store/store-identity-churn-probe.test.ts @@ -1,5 +1,6 @@ -import { beforeEach, describe, expect, it } from 'vitest' -import { create } from 'zustand' +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { create, type StoreApi } from 'zustand' +import { withReactCommitCascadeWriteProbe } from './react-commit-cascade-write-probe' import { armStoreIdentityChurnProbe, disarmStoreIdentityChurnProbe, @@ -97,20 +98,86 @@ describe('store identity churn probe', () => { expect(row.sites[0].site).toContain('store-identity-churn-probe.test') }) - it('runs a functional update exactly once', () => { - // The probe resolves the updater itself before handing the object to zustand; - // calling it twice would double any work a slice does inside its updater. + it('leaves a functional updater to zustand: called once, with the live state', () => { const store = createProbeStore() - let calls = 0 + const seen: unknown[] = [] armStoreIdentityChurnProbe() store.setState((state) => { - calls += 1 + seen.push(state) return { counter: state.counter + 1 } }) + store.setState((state) => { + seen.push(state) + return { rows: [{ ...state.rows[0] }] } + }) - expect(calls).toBe(1) + expect(seen).toHaveLength(2) + expect(seen[1]).toMatchObject({ counter: 1 }) expect(store.getState().counter).toBe(1) + expect(churnFor('rows')).toBe(1) + }) + + it('ignores a write zustand itself drops as identical', () => { + const store = createProbeStore() + armStoreIdentityChurnProbe() + + store.setState((state) => state) + + expect(readStoreIdentityChurnReport()).toEqual([]) + }) + + it('passes disarmed writes straight through without reading state', () => { + const innerSet = vi.fn() + const innerGet = vi.fn(() => ({ counter: 0 })) + const api = { setState: innerSet, getState: innerGet } as unknown as StoreApi<{ + counter: number + }> + const creator = withStoreIdentityChurnProbe<{ counter: number }>(() => ({ counter: 0 })) + creator(innerSet, innerGet, api) + const updater = (state: { counter: number }) => ({ counter: state.counter + 1 }) + + api.setState(updater, true) + + // The disarmed path forwards the exact arguments and never calls get(). + expect(innerSet).toHaveBeenCalledTimes(1) + expect(innerSet.mock.calls[0]).toEqual([updater, true]) + expect(innerGet).not.toHaveBeenCalled() + }) + + it('composes with the cascade probe without dropping or doubling a write', () => { + // Mirrors store/index.ts: churn probe outermost, cascade probe inside it. + const store = create()( + withStoreIdentityChurnProbe( + withReactCommitCascadeWriteProbe((set) => ({ + rows: [{ id: 'a', label: 'A' }], + entries: { a: { status: 'idle' } }, + counter: 0, + refresh: (rows) => set({ rows }), + touch: (id, status) => + set((state) => ({ entries: { ...state.entries, [id]: { status } } })), + bump: () => set((state) => ({ counter: state.counter + 1 })) + })) + ) + ) + let updaterCalls = 0 + armStoreIdentityChurnProbe({ captureSites: true }) + + store.getState().bump() + store.setState((state) => { + updaterCalls += 1 + return { counter: state.counter + 10 } + }) + store.getState().refresh([{ id: 'a', label: 'A' }]) + store.setState({ ...store.getState(), counter: 100 }, true) + + expect(updaterCalls).toBe(1) + expect(store.getState().counter).toBe(100) + expect(store.getState().rows).toEqual([{ id: 'a', label: 'A' }]) + // The named site is this test, not the sibling probe's wrapper frame. + const [row] = readStoreIdentityChurnReport() + expect(row).toMatchObject({ field: 'rows', churnedWrites: 1 }) + expect(row.sites[0].site).toContain('store-identity-churn-probe.test') }) it('still sees churn on a replace write', () => { diff --git a/src/renderer/src/store/store-identity-churn-probe.ts b/src/renderer/src/store/store-identity-churn-probe.ts index 42e4908efbf..32b289c13b9 100644 --- a/src/renderer/src/store/store-identity-churn-probe.ts +++ b/src/renderer/src/store/store-identity-churn-probe.ts @@ -9,8 +9,13 @@ * no data change to show for it. * * Cost when disarmed: one boolean field load per write, matching - * react-commit-cascade-write-probe. Comparison work only happens while armed, - * so this never sits on the keystroke path in a shipped build. + * react-commit-cascade-write-probe. Comparison work only happens while armed. + * Nothing in the app arms it, so store/index.ts installs it only in dev and + * store-exposing builds; a shipped build never runs the wrapper at all. + * + * The wrapper never resolves a functional updater itself: zustand keeps sole + * ownership of when and with what argument an updater runs, so the probe cannot + * double-invoke it or hand it a stale state. */ import type { StateCreator } from 'zustand' @@ -87,18 +92,16 @@ export function armStoreIdentityChurnProbe(options?: { captureSites?: boolean }) } // Why the first non-probe, non-zustand frame: the caller that built the partial is -// the code to fix; the frames above it are the shared write plumbing. +// the code to fix; the frames above it are the shared write plumbing. Every store +// write middleware is named *-probe.ts, so a sibling wrapper's frame is skipped too. const SOURCE_FRAME = /:\d+:\d+\)?$/ +const PROBE_FRAME = /-probe\.[cm]?[jt]s\b/ function callingSite(): string { const stack = new Error('store identity churn site').stack?.split('\n') ?? [] for (const line of stack.slice(2)) { const frame = line.trim() - if ( - SOURCE_FRAME.test(frame) && - !frame.includes('store-identity-churn-probe.ts') && - !frame.includes('node_modules') - ) { + if (SOURCE_FRAME.test(frame) && !PROBE_FRAME.test(frame) && !frame.includes('node_modules')) { return frame } } @@ -134,10 +137,10 @@ function recordSite(field: string): void { } /** - * `fields` is the write's own keys, not the whole state: `set(partial)` merges, so - * no field outside the partial can have changed. Scanning the full post-write state - * would make the armed cost scale with the store's field count instead of the - * write's size. + * `fields` is the write's own keys when they are knowable: `set(partial)` merges, + * so no field outside the partial can have changed. A functional updater or a + * replace write falls back to every field; the extra cost there is one Object.is + * per untouched field, since the deep compare only runs on replaced references. */ function recordWrite( previous: Record, @@ -180,21 +183,18 @@ export function withStoreIdentityChurnProbe( return } const previous = get() as Record - // Why resolve the updater here: zustand would otherwise compute the partial - // internally and the probe could only recover the changed fields by scanning - // the whole state. Same function, same argument, called once. - const resolved = - typeof partial === 'function' - ? (partial as (state: Record) => unknown)(previous) - : partial - ;(set as (nextPartial: unknown, nextReplace?: unknown) => void)(resolved, replace) + // Why the write is passed through untouched: zustand owns when and how an + // updater runs. The probe only compares the states on either side of it. + ;(set as (nextPartial: unknown, nextReplace?: unknown) => void)(partial, replace) try { const next = get() as Record - // A replace write drops absent fields, so every field is in play. + if (next === previous) { + return + } const fields = - replace === true || resolved === null || typeof resolved !== 'object' - ? Object.keys(next) - : Object.keys(resolved as Record) + replace !== true && partial !== null && typeof partial === 'object' + ? Object.keys(partial) + : Object.keys(next) recordWrite(previous, next, fields) } catch { // A diagnostic on the app's universal write path must never break writes.