From ea076fb73cf8e23f7d58a9294dc4d0d348057ad8 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Wed, 10 Jun 2026 08:01:20 -0400 Subject: [PATCH 1/9] Add React Compiler triage, selector-cascade, `useEffect`, and state-normalization references to the `performance` skill MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cross-platform React/Redux guidance adapted from contributor-docs `frontend-performance.md` and the extension performance audit (MetaMask-planning#6571; extension PRs #38007, #37147), wired into the mobile overlay's routing tables. Additive only — no changes to existing mobile-specific facts or guidance. --- .../references/mm-hook-dependency-arrays.md | 1 + .../mm-react-compiler-error-triage.md | 90 +++++++++++ .../references/mm-react-compiler.md | 1 + .../references/mm-redux-antipatterns.md | 2 + .../references/mm-selector-cascade.md | 98 ++++++++++++ .../references/mm-selector-memoization.md | 2 + .../references/mm-state-normalization.md | 126 ++++++++++++++++ .../references/mm-useeffect-antipatterns.md | 142 ++++++++++++++++++ .../performance/repos/metamask-mobile.md | 8 +- 9 files changed, 469 insertions(+), 1 deletion(-) create mode 100644 domains/performance/skills/performance/references/mm-react-compiler-error-triage.md create mode 100644 domains/performance/skills/performance/references/mm-selector-cascade.md create mode 100644 domains/performance/skills/performance/references/mm-state-normalization.md create mode 100644 domains/performance/skills/performance/references/mm-useeffect-antipatterns.md diff --git a/domains/performance/skills/performance/references/mm-hook-dependency-arrays.md b/domains/performance/skills/performance/references/mm-hook-dependency-arrays.md index 5733ff81..a11ecdf0 100644 --- a/domains/performance/skills/performance/references/mm-hook-dependency-arrays.md +++ b/domains/performance/skills/performance/references/mm-hook-dependency-arrays.md @@ -83,5 +83,6 @@ For each hit, ask: *does this dependency change identity every render?* If yes, ## Related +- [mm-useeffect-antipatterns.md](mm-useeffect-antipatterns.md) — the effect-body side: derived state, effect chains, unmount-safe async, cleanup - [js-react-compiler.md](js-react-compiler.md) / [mm-react-compiler.md](mm-react-compiler.md) — automatic memoization on opted-in paths - [js-concurrent-react.md](js-concurrent-react.md) — defer expensive derived work diff --git a/domains/performance/skills/performance/references/mm-react-compiler-error-triage.md b/domains/performance/skills/performance/references/mm-react-compiler-error-triage.md new file mode 100644 index 00000000..9d8e8ae1 --- /dev/null +++ b/domains/performance/skills/performance/references/mm-react-compiler-error-triage.md @@ -0,0 +1,90 @@ +--- +title: React Compiler Error Triage & Coverage Accounting (MetaMask) +impact: HIGH +tags: react-compiler, panicThreshold, error-triage, coverage, babel, build +--- + +# Skill: React Compiler Error Triage & Coverage Accounting + +The React Compiler **fails open**: when it can't compile a component, it silently skips it and ships the unoptimized original. The build stays green, DevTools shows no warning — you just don't get the memoization. Once the compiler is enabled broadly (metamask-mobile#31171 enabled v1.0.0 app-wide), the question stops being "is it on?" and becomes **"what is it actually compiling, and which of its errors are worth fixing?"** This file is the triage playbook. Extension PR metamask-extension#38007 is the reference implementation. + +## The `panicThreshold` ladder + +`panicThreshold` controls when a compiler diagnostic fails the build instead of silently skipping the file: + +| Setting | Build fails on | Use for | +|---|---|---| +| `'none'` (default) | never — every failed file is **silently skipped** | production builds, always | +| `'critical_errors'` | only critical errors (compiler-internal invariant violations) | CI / debug builds, first ratchet target | +| `'all_errors'` | every diagnostic, including unsupported syntax | CI / debug builds, end-state ratchet | + +**The ratchet strategy** (extension roadmap, MetaMask-planning#6552 → #6553): keep production at `'none'` permanently; aim for a *non-production* build that passes at `'critical_errors'`, then at `'all_errors'`. Each ratchet step turns a class of silent skips into a visible, fixable error list. Never enable a non-`'none'` threshold in a release build — one un-compilable file would block the release for an optimization that is optional by design. + +## Triage: unsupported syntax vs. legitimate errors + +Compiler diagnostics are **not one bucket**. The logger event's `category` field separates them, and the distinction decides whether you act: + +- **`category === 'Todo'` → "unsupported."** Syntax or a pattern the compiler *itself* has not implemented yet. There is **no actionable fix on our side** — rewriting working code to appease an unimplemented compiler path is wasted effort and churn. Count these separately, leave the code alone, and re-check after compiler upgrades. +- **Any other category (e.g. `InvalidReact`, `InvalidJS`) → legitimate, actionable.** A real Rules-of-React violation in our code (mutation during render, conditional hooks, side effects in render). Fixing it both unlocks compilation *and* removes a latent correctness bug. + +A healthcheck that doesn't make this split is noise: the `Todo` count swamps the actionable list and the team learns to ignore the output. The extension's webpack wrapper makes the split in ~10 lines: + +```ts +// adapted from metamask-extension development/webpack/utils/loaders/reactCompilerLoaderWrapper.ts +// (mobile equivalent: pass a `logger` in babel-plugin-react-compiler options) +logger: { + logEvent(filename, event) { + switch (event.kind) { + case 'CompileSuccess': record(filename, 'compiled'); break; + case 'CompileSkip': record(filename, 'skipped'); break; + case 'CompileError': { + const category = event.detail?.options?.category ?? event.detail?.category; + // 'Todo' = not yet supported by the compiler — no actionable fix on our side + record(filename, category === 'Todo' ? 'unsupported' : 'error'); + break; + } + } + }, +} +``` + +The extension exposes this as `yarn webpack --reactCompilerVerbose` (per-file ✅/⏭️/🔍/❌ output + summary stats) and `--reactCompilerDebug={all|critical|none}` (maps to `panicThreshold: '_errors'`). On mobile the same taxonomy is available through the Babel plugin's `logger` option or `eslint-plugin-react-compiler` (the lint rule runs the same analysis the compiler does). + +## Coverage accounting + +Track four buckets — **compiled / skipped / errors / unsupported** — at file and component granularity, with **worst-status-wins per file** (`error > unsupported > skipped > compiled`): a file with five compiled components and one error is an *error file*, otherwise mixed files inflate the compiled count and the number lies to you. + +What the buckets tell you: + +- **compiled** — your real optimization coverage. "The compiler is enabled" claims nothing; this number does. +- **errors** — the actionable backlog. Each is a Rules-of-React fix. +- **unsupported** — the compiler's backlog, not yours. Trend it across compiler upgrades. +- **skipped** — intentional exclusions: test/story files, `'use no memo'` directives, and **class components** (never compiled — metamask-mobile#30919 counted 53 at full enablement; migration to function components is the only way to move them into the compiled bucket). + +## Staged adoption roadmap + +The extension's sequence (epic MetaMask-planning#6549) generalizes to any repo: + +1. **Lint clean:** update `eslint-plugin-react-hooks` / `eslint-plugin-react-compiler` to latest; fix violations — these are exactly what the compiler will refuse to compile. +2. **Audit opt-outs:** every `'use no memo'` carries a reason + TODO; the count only goes down. `grep -rn "use no memo" app --include="*.ts*"`. +3. **Ratchet `critical_errors`:** non-prod build passes; fix what surfaces. +4. **Ratchet `all_errors`:** remaining actionable errors fixed; what's left is the `Todo` (unsupported) set, which you wait out. + +## Verify + +- Per component: `Memo ✨` badge in React DevTools (see [js-profile-react.md](js-profile-react.md)). +- Per repo: the compiled-files count from the logger stats rises (or at least doesn't silently fall) release over release — silent coverage regressions are the failure mode this file exists to catch. +- After a compiler version bump: re-run the verbose build and diff the `unsupported` list — `Todo`s that became `compiled` are free wins; new `error`s are regressions to triage. + +## Don't over-correct + +- **Never "fix" a `Todo`.** Rewriting working code around an unimplemented compiler feature is churn with no perf evidence; the next compiler release may compile it as-is. +- Don't gate releases on compiler errors (`panicThreshold` stays `'none'` in production builds). +- Don't treat `skipped` as a problem — tests, stories, and deliberate opt-outs belong there. The smell is *unexplained* `'use no memo'` directives, not the bucket itself. +- A component without `Memo ✨` is not automatically a bug to chase — check the buckets first; it may be `unsupported`. + +## Related + +- [mm-react-compiler.md](mm-react-compiler.md) — enabling the compiler in this repo (Babel config, Metro cache, ESLint healthcheck) +- [js-react-compiler.md](js-react-compiler.md) — how the compiler transforms code; Rules-of-React background +- [mm-selector-cascade.md](mm-selector-cascade.md) — what the compiler **cannot** fix: unstable values crossing file boundaries (selectors, imported hooks) diff --git a/domains/performance/skills/performance/references/mm-react-compiler.md b/domains/performance/skills/performance/references/mm-react-compiler.md index e95d70d6..b23e4bc5 100644 --- a/domains/performance/skills/performance/references/mm-react-compiler.md +++ b/domains/performance/skills/performance/references/mm-react-compiler.md @@ -68,4 +68,5 @@ Fix the ESLint `react-compiler` warnings on a path before/after opting it in. ## Related - [js-react-compiler.md](js-react-compiler.md) — upstream reference on how the compiler transforms code +- [mm-react-compiler-error-triage.md](mm-react-compiler-error-triage.md) — triaging compiler errors (`Todo`/unsupported vs actionable), `panicThreshold` ratcheting, and measuring real coverage - [mm-selector-memoization.md](mm-selector-memoization.md) — fix data-layer re-renders the compiler can't diff --git a/domains/performance/skills/performance/references/mm-redux-antipatterns.md b/domains/performance/skills/performance/references/mm-redux-antipatterns.md index a830bc35..dd1d853a 100644 --- a/domains/performance/skills/performance/references/mm-redux-antipatterns.md +++ b/domains/performance/skills/performance/references/mm-redux-antipatterns.md @@ -86,5 +86,7 @@ grep -rn "dispatch(" app --include="*.ts" --include="*.tsx" | grep -v ".test." \ ## Related - [mm-selector-memoization.md](mm-selector-memoization.md) — the upstream fix for Pattern 1 +- [mm-selector-cascade.md](mm-selector-cascade.md) — repairing the whole dependency graph and removing accumulated `isEqual` band-aids after the root fix +- [mm-state-normalization.md](mm-state-normalization.md) — consolidating many `useSelector` calls into one view selector - [mm-context-performance.md](mm-context-performance.md) — the Context equivalent of over-broad subscriptions - [js-profile-react.md](js-profile-react.md) — confirm the re-render reduction diff --git a/domains/performance/skills/performance/references/mm-selector-cascade.md b/domains/performance/skills/performance/references/mm-selector-cascade.md new file mode 100644 index 00000000..a3a92989 --- /dev/null +++ b/domains/performance/skills/performance/references/mm-selector-cascade.md @@ -0,0 +1,98 @@ +--- +title: Selector Dependency Cascades — Blast Radius & Repair (MetaMask) +impact: CRITICAL +tags: reselect, cascade, dependency-graph, isEqual, structural-sharing, react-compiler +--- + +# Skill: Selector Dependency Cascades + +[mm-selector-memoization.md](mm-selector-memoization.md) catalogues the broken-selector *patterns*. This file is about what happens **downstream of one broken root selector** — and how to repair the whole graph instead of patching its leaves. Reference case: extension PR metamask-extension#37147, where a single identity output selector (`getInternalAccounts`) was recomputing through **15 direct + 35+ transitive consumer selectors into 50+ components on every dispatch** — every 5-second balance poll, every keystroke in the send flow. + +## Anatomy of a cascade + +```ts +// ❌ the root: identity output selector — memoizes nothing, new "result" every dispatch +export const getInternalAccounts = createSelector( + (state) => state.engine.internalAccounts.accounts, + (accounts) => accounts, // output === input: the cache can never hit meaningfully +); +``` + +Every consumer selector that takes the root as an input now sees a "changed" input on every dispatch, recomputes, and — because most result functions allocate (`.filter()`, `.map()`, `Object.values()`) — emits its *own* fresh reference, propagating the invalidation one layer further. Three layers down, nobody remembers the root; they see "my selector keeps firing" and reach for local fixes: + +```ts +// ❌ the band-aids that accumulate downstream of a broken root +const accounts = useSelector(getAccountsByScope, isEqual); // deep compare per dispatch +export const getX = createDeepEqualSelector(getInternalAccounts, …); // deep compare per dispatch +export const getMemoizedAccounts = createSelector(getInternalAccounts, (a) => a); // does nothing +``` + +Each band-aid suppresses the re-render for one consumer while *adding* an O(n) deep comparison on every dispatch — and the cascade cost scales superlinearly with power-user data (see [mm-power-user-scenario.md](mm-power-user-scenario.md)). + +## Step 1 — Measure the blast radius before fixing + +The repair PR's evidence (and its review) should enumerate the graph, the way #37147 did: + +1. **Direct consumers:** every selector that lists the suspect as an input. `grep -rn "getInternalAccounts" app/selectors --include="*.ts"` +2. **Transitive consumers:** repeat one level down for each direct consumer (in practice 2 levels reaches most of the graph). +3. **Component consumers:** `useSelector` call sites of anything in the graph. +4. **Recomputation count:** instrument with `selector.recomputations()` (reselect) or a `console.count` in the result function across a few dispatches (a balance poll is a convenient metronome). + +A before/after table — *recomputations per dispatch, re-renders per poll cycle, on the same interaction* — is what distinguishes a verified cascade fix from a speculative refactor. + +## Step 2 — Fix the root, not the 50 consumers + +Memoizing consumers one by one is whack-a-mole: each fix adds comparison cost and the graph keeps re-deriving from a poisoned root. Trace **upward** (who are my inputs? are *they* stable?) until you hit the selector whose output identity changes without its data changing — that's the root. Fix its memoization there (patterns + recipes in [mm-selector-memoization.md](mm-selector-memoization.md)). + +**Know your reference-stability contract first.** What the correct fix looks like depends on whether your store gives you stable references for unchanged data: + +- With **Immer-based reducers** (Redux Toolkit), structural sharing guarantees `state.a.b` keeps its reference **iff** nothing under that path changed. Under that contract, a plain `createSelector` over a *narrow* input is already correct, and deep-equal selectors are pure overhead. +- Where state is replaced wholesale on sync (documented for this repo's controller-state slices in [mm-selector-memoization.md](mm-selector-memoization.md)), input references break even when data didn't change, and `createDeepEqualSelector` at the *root* is the pragmatic tool. + +Establish which contract a slice actually follows (log `prev === next` for the input across two unrelated dispatches) before choosing — the answer differs per slice, and assuming the wrong contract either reintroduces the cascade or buys deep-compares you don't need. + +## Step 3 — Sweep the graph and *remove* the band-aids + +This is the step most fixes skip. After the root is stable, every downstream `isEqual`, `createDeepEqualSelector`-wrapping-a-now-stable-input, and `getMemoized*` duplicate is dead weight: it still runs its deep comparison on every dispatch, and it **masks regressions** — if the root breaks again, the band-aids hide it until the app is slow everywhere again. + +```bash +# downstream band-aid sweep, scoped to the fixed graph +grep -rn "useSelector(.*isEqual)" app --include="*.tsx" | grep -v ".test." +grep -rn "createDeepEqualSelector" app/selectors --include="*.ts" +grep -rn "getMemoized\|selectMemoized" app/selectors --include="*.ts" +``` + +For each hit that consumes the fixed root (directly or transitively): remove the equality argument / downgrade to plain `createSelector`, and re-verify the consumer doesn't re-render on unrelated dispatches. #37147 deleted the band-aids in the same PR as the root fix — that's the model. + +## What the React Compiler can and cannot do here + +The compiler memoizes **within a file**. A `useSelector` result, an imported hook's return value, or an external context value is opaque to it — if the selector hands back a fresh reference, the compiled component still re-renders, and any derivation from it still recomputes (extension audit ticket MetaMask-planning#6661): + +```tsx +const tokens = useSelector(selectTokens); // compiler cannot see/stabilize this +const rows = tokens.map(toRow); // ❌ recomputes every render even when compiled +const rows = useMemo(() => tokens.map(toRow), [tokens]); // ✅ still needed +``` + +Rule of thumb: values that **cross a file boundary** (Redux selectors, imported hooks/functions, external context) keep their manual `useMemo`/`useCallback`; same-file props/state derivations can lean on the compiler. Fix the selector graph first — automatic memoization downstream of an unstable root optimizes nothing. + +## Don't over-correct + +- Not every busy selector is a cascade root — a selector returning a **primitive** breaks the chain at that point regardless of recomputation (allocation waste ≠ re-render bug). +- A *global* top-level cascade (an unstable value in a root provider/HOC re-rendering the whole tree on every state change) is a pattern the extension audit found at app root — worth **ruling out** with one profiler pass ("why did this render?" on a top-level component during an unrelated dispatch), but don't assume it exists here; verify before restructuring providers. See [mm-context-performance.md](mm-context-performance.md) for the provider-value mechanics. +- Don't add `useMemo` around every `useSelector` read preemptively — only where a non-primitive result feeds a derivation or a memoized child (see guardrails in [mm-hook-dependency-arrays.md](mm-hook-dependency-arrays.md)). + +## Verify + +1. Root selector returns the **same reference** across two unrelated dispatches (the contract test from [mm-selector-memoization.md](mm-selector-memoization.md)). +2. Recomputation counts on direct + transitive consumers drop to ~0 on unrelated dispatches. +3. Profiler on a top consumer (account list, send flow): the re-render cascade is gone during a balance poll. +4. The band-aid greps above return no hits inside the repaired graph. + +## Related + +- [mm-selector-memoization.md](mm-selector-memoization.md) — the root-selector patterns and fix recipes +- [mm-redux-antipatterns.md](mm-redux-antipatterns.md) — `useSelector(x, isEqual)` as symptom; per-consumer view +- [mm-unstable-hook-return.md](mm-unstable-hook-return.md) — the same cascade shape, with a hook as the root +- [mm-state-normalization.md](mm-state-normalization.md) — state shape that prevents cascade-prone selectors +- [mm-react-compiler-error-triage.md](mm-react-compiler-error-triage.md) — confirming what the compiler actually covers diff --git a/domains/performance/skills/performance/references/mm-selector-memoization.md b/domains/performance/skills/performance/references/mm-selector-memoization.md index b9e117db..66cbd288 100644 --- a/domains/performance/skills/performance/references/mm-selector-memoization.md +++ b/domains/performance/skills/performance/references/mm-selector-memoization.md @@ -113,4 +113,6 @@ Escalate severity by one level if the selector is imported in **10+ files**. ## Related - [mm-redux-antipatterns.md](mm-redux-antipatterns.md) — `useSelector(x, isEqual)` is the *symptom* of a broken selector; fix the selector, then remove the `isEqual`. +- [mm-selector-cascade.md](mm-selector-cascade.md) — graph-level view: blast radius of one broken root, and sweeping out downstream band-aids after the fix. +- [mm-state-normalization.md](mm-state-normalization.md) — state/selector *shape*: O(1) lookups, parameterized-selector cache thrashing, view-selector consolidation. - [js-profile-react.md](js-profile-react.md) — prove the re-render reduction. diff --git a/domains/performance/skills/performance/references/mm-state-normalization.md b/domains/performance/skills/performance/references/mm-state-normalization.md new file mode 100644 index 00000000..ea27cfbe --- /dev/null +++ b/domains/performance/skills/performance/references/mm-state-normalization.md @@ -0,0 +1,126 @@ +--- +title: State Normalization & Selector Shape (MetaMask) +impact: HIGH +tags: redux, normalization, selectors, O(1)-lookups, cache-thrashing, useSelector +--- + +# Skill: State Normalization & Selector Shape + +Selector *memoization* fixes when things recompute; state and selector **shape** fixes how much each recomputation costs and how many subscriptions fire. The patterns here come from the extension performance audit (MetaMask-planning#6580, #6484), where linear scans and reshaping selectors multiplied across power-user data: with 1,000 tokens, 27 `.find()`-based lookups per render is 27,000 comparisons — per render. + +## Pattern — O(n) scans where the state shape should provide O(1) lookups + +```ts +// ❌ linear scan through all accounts on every call +export const getAccountByAddress = createSelector( + selectAccounts, + (_, address) => address, + (accounts, address) => + Object.values(accounts).find((a) => a.address.toLowerCase() === address.toLowerCase()), +); +``` + +When lookups by some key are frequent, **index the state once** instead of scanning per consumer: + +```ts +// ✅ build the index once per data change; lookups are O(1) key access +export const selectAccountsByAddress = createSelector(selectAccounts, (accounts) => + Object.fromEntries(Object.values(accounts).map((a) => [a.address.toLowerCase(), a])), +); +// consumers key into the memoized index — no scan, no per-arg selector cache to bust +const account = useSelector(selectAccountsByAddress)[address.toLowerCase()]; +``` + +Normalized shape (`byId` / `byAddress` maps + an `ids` array for order) is the same idea applied at the reducer level — the index is maintained on write instead of derived on read. + +## Pattern — parameterized selector cache thrashing + +`createSelector` has a **single-entry cache**. A parameterized selector called with different arguments from different components busts that one cache slot on every call: + +```ts +// ❌ each component's call evicts the previous component's result +const a1 = useSelector((s) => getAccountByAddress(s, addr1)); // miss +const a2 = useSelector((s) => getAccountByAddress(s, addr2)); // miss, evicts addr1 +const a3 = useSelector((s) => getAccountByAddress(s, addr3)); // miss, evicts addr2 — and so on every render cycle +``` + +In a list rendering N rows, the "memoized" selector recomputes N times per render, forever. Fixes, in order of preference: + +1. **Lookup-map selector** (above): select the whole memoized index once; key into it. Sidesteps per-arg caching entirely. +2. **Per-instance selector**: a factory (`makeSelectAccountByAddress()`) instantiated in the component with `useMemo`, so each call site owns its own cache slot. +3. **Bigger cache**: reselect's `lruMemoize` with `maxSize: N` — last resort; sizing is a guess that goes stale. + +```bash +# parameterized selectors: second input selector reads the argument, not state +grep -rn "(_, \|(_state" app/selectors --include="*.ts" +``` + +## Pattern — selectors that reorganize nested state + +```ts +// ❌ inverts { account → chain → tokens } into { chain → account → tokens } on every recompute +export const getTokensByChain = createSelector(selectAllTokens, (byAccount) => { + const byChain = {}; + for (const [account, chains] of Object.entries(byAccount)) + for (const [chainId, tokens] of Object.entries(chains)) + (byChain[chainId] ??= {})[account] = tokens; + return byChain; +}); +``` + +A full restructure allocates a new tree every recomputation — expensive to build, and every consumer sees a fresh reference. If two access patterns are both hot, **store both shapes** (maintain the second index in the reducer on write) or normalize so both reads are key lookups. A reshaping selector is acceptable only for cold paths. + +## Pattern — deep property access instead of composed input selectors + +```ts +// ❌ re-derives the full path; recomputes when ANY ancestor changes; nothing is reusable +export const getGroupName = (state, walletId, groupId) => + state.engine.accountTree.wallets[walletId]?.groups[groupId]?.metadata?.name; +``` + +Compose granular selectors at each level (`selectWallets` → `selectWalletById` → `selectGroupById` → …). Each layer memoizes independently, intermediate results are reusable by other selectors, and a change to one wallet no longer recomputes selectors reading a different one. This is also what keeps inputs *narrow* — the prerequisite for the memoization patterns in [mm-selector-memoization.md](mm-selector-memoization.md). + +## Pattern — many useSelector calls where one view selector should exist + +```tsx +// ❌ 11 store subscriptions; each runs on every dispatch; component re-checks 11 results +const quotes = useSelector(getQuotes); +const currency = useSelector(getCurrentCurrency); +const gasFee = useSelector(getGasFee); +// … ×8 more +``` + +Each `useSelector` is an independent store subscription with its own equality check per dispatch. When a component reads 5+ related values, combine them into **one memoized view selector** that returns the assembled view-model. One subscription, one equality check, one place where the shape is defined — and a natural home for the derivation logic that would otherwise sit unmemoized in the component (where, note, the React Compiler can't stabilize it — see [mm-selector-cascade.md](mm-selector-cascade.md)). + +The same consolidation applies to **duplicate derived-data implementations**: the extension audit found 4+ independent fiat-conversion code paths recomputing the same numbers in different components. One canonical selector ends both the wasted compute and the drift between implementations. + +## How to find + +```bash +# linear scans inside selectors/hooks +grep -rn "Object.values(.*)\.\(find\|filter\)\|\.find((" app/selectors app/components --include="*.ts*" | grep -v ".test." + +# reshaping selectors: nested loops/reduce building objects in a result function +grep -rn -B2 "??= {}\|reduce((acc" app/selectors --include="*.ts" + +# components with many subscriptions (5+ useSelector in one file = consolidation candidate) +grep -rc "useSelector(" app/components --include="*.tsx" | awk -F: '$2>=5' | sort -t: -k2 -rn | head -20 +``` + +## Verify + +- Lookup fix: recomputation count on the index selector is ~1 per data change (not per render); list scroll/render time drops in the Profiler. +- Consolidation: the component's "why did this render" shows one subscription firing instead of N; render count per dispatch drops. +- Normalization: reducer tests confirm both shapes stay in sync on write. + +## Don't over-correct + +- Don't normalize a slice that's only ever iterated in full — indexes pay for themselves on *keyed lookups*, not on `.map()` over everything. +- Don't merge *unrelated* selectors into one mega view selector — that re-couples components to data they don't read and re-renders them for it. Consolidate related values consumed together. +- `maxSize`/factory-selector machinery is for genuinely parameterized hot paths; for one or two call sites the lookup-map pattern is simpler and stays correct. + +## Related + +- [mm-selector-memoization.md](mm-selector-memoization.md) — memoization correctness for the selectors shaped here +- [mm-selector-cascade.md](mm-selector-cascade.md) — graph-level repair when a root selector poisons consumers +- [mm-redux-antipatterns.md](mm-redux-antipatterns.md) — inline selectors and `isEqual` band-aids diff --git a/domains/performance/skills/performance/references/mm-useeffect-antipatterns.md b/domains/performance/skills/performance/references/mm-useeffect-antipatterns.md new file mode 100644 index 00000000..2ae982a8 --- /dev/null +++ b/domains/performance/skills/performance/references/mm-useeffect-antipatterns.md @@ -0,0 +1,142 @@ +--- +title: useEffect Lifecycle Anti-Patterns (MetaMask) +impact: HIGH +tags: useEffect, setState, cleanup, AbortController, unmount, memory-leaks +--- + +# Skill: useEffect Lifecycle Anti-Patterns + +[mm-hook-dependency-arrays.md](mm-hook-dependency-arrays.md) covers *when* effects re-run (the deps side). This file covers what goes wrong **inside and after** the effect: state derived in effects instead of render, effects chained off each other's setState, async work that outlives the component, and missing cleanup. These patterns cause extra render passes, memory leaks, and the classic "setState on unmounted component" warnings — and they're invisible to selector/re-render sweeps. + +## Pattern — derived state via useEffect + setState ("you might not need an effect") + +```tsx +// ❌ two render passes per change: render → effect → setState → render again +const [visibleTokens, setVisibleTokens] = useState([]); +useEffect(() => { + setVisibleTokens(tokens.filter((t) => !t.hidden)); +}, [tokens]); + +// ✅ derive during render — one pass, no state to drift out of sync +const visibleTokens = useMemo(() => tokens.filter((t) => !t.hidden), [tokens]); +``` + +If a value is computable from props/state/store, compute it in render (memoize only if it's expensive or feeds a memoized child). State + effect is for *synchronizing with something external*, not for derivation. + +## Pattern — cascading effect chains + +```tsx +// ❌ effect A sets state → triggers effect B → sets state → triggers effect C… +useEffect(() => { setAccount(deriveAccount(accounts, selected)); }, [accounts, selected]); +useEffect(() => { setBalances(deriveBalances(account)); }, [account]); +useEffect(() => { setFiat(deriveFiat(balances, rate)); }, [balances, rate]); +// 4 render passes for one upstream change, and the intermediate renders show stale combinations +``` + +**Fix:** collapse the chain into render-time derivation (one `useMemo` per step, or one for the lot). Each link in a setState-chain is a full extra render pass *and* a window where the UI shows an inconsistent intermediate state. + +## Pattern — async work that outlives the component + +```tsx +// ❌ fetch resolves after unmount (or after the input changed) → setState on dead component / stale data wins +useEffect(() => { + fetchTokenMetadata(address).then((meta) => setMetadata(meta)); +}, [address]); +``` + +Two equivalent fixes — pick one and use it consistently: + +```tsx +// ✅ cancelled flag — cheapest, works for any promise +useEffect(() => { + let cancelled = false; + fetchTokenMetadata(address).then((meta) => { + if (!cancelled) setMetadata(meta); + }); + return () => { cancelled = true; }; +}, [address]); + +// ✅ AbortController — also cancels the network request itself (RN fetch supports `signal`) +useEffect(() => { + const controller = new AbortController(); + fetch(url, { signal: controller.signal }) + .then((r) => r.json()) + .then(setData) + .catch((e) => { if (e.name !== 'AbortError') setError(e); }); + return () => controller.abort(); +}, [url]); +``` + +The cancelled flag prevents the *setState*; AbortController additionally stops the request from consuming bandwidth/battery. The race-condition variant (stale response overwriting fresh data when `address` changes quickly) is fixed by the same cleanup — the old effect's closure is cancelled before the new one runs. + +**Codify, don't copy-paste** (extension epic MetaMask-planning#6525): once a repo has three hand-rolled cancelled flags, extract shared hooks — `useIsMounted()`, `useAbortableEffect(fn, deps)` (effect receives a signal), `useEventListener(target, event, handler)` (auto-removes on unmount) — so cleanup is the default, not per-site diligence. + +## Pattern — missing cleanup for timers / subscriptions / listeners + +```tsx +// ❌ each mount adds another interval/listener; none are removed +useEffect(() => { + const id = setInterval(refreshGasEstimate, 15000); + emitter.on('update', onUpdate); +}, []); + +// ✅ every subscription returns its teardown +useEffect(() => { + const id = setInterval(refreshGasEstimate, 15000); + emitter.on('update', onUpdate); + return () => { clearInterval(id); emitter.off('update', onUpdate); }; +}, []); +``` + +Leaked intervals keep firing (and keep dispatching) forever; leaked listeners hold the closure — and everything it captured — out of garbage collection. See [js-memory-leaks.md](js-memory-leaks.md) for hunting these in a running app, and [mm-streaming-realtime.md](mm-streaming-realtime.md) for subscription lifecycles tied to visibility. + +## Pattern — regular variable where a ref is needed + +```tsx +// ❌ reset to false on every render — the guard never works +let hasLoggedImpression = false; +useEffect(() => { + if (!hasLoggedImpression) { logImpression(); hasLoggedImpression = true; } +}); + +// ✅ useRef persists across renders without triggering them +const hasLoggedImpression = useRef(false); +``` + +Any mutable flag/cache/previous-value that must survive re-renders but shouldn't cause them belongs in a ref, not a closure variable (and not state). + +## Pattern — large objects captured in effect closures + +An effect (or its cleanup) that closes over a large object — full token lists, raw API payloads — pins that object in memory for as long as the subscription lives. Extract the fields you need into locals *before* the closure, or read through a ref, so the big object can be collected. + +## How to find + +```bash +# setState-from-effect derivation candidates (review hits — some are legitimate syncs) +grep -rn -A3 "useEffect(" app --include="*.tsx" | grep -B1 "set[A-Z]" | grep -v ".test." + +# fetch/promises in effects with no signal/cancelled handling nearby +grep -rn -A6 "useEffect(" app --include="*.ts*" | grep -E "fetch\(|\.then\(" | grep -v "signal\|cancelled\|abort" | grep -v ".test." + +# intervals/timeouts/listeners inside effects — then eyeball for a `return () =>` teardown +grep -rn "setInterval\|setTimeout\|addEventListener\|\.on(" app --include="*.ts*" | grep -v ".test." | grep -v "clear\|remove\|off(" +``` + +## Verify + +- React DevTools highlight-updates: the derive-in-render fix removes the double render pass on the affected component. +- No "setState on unmounted component" / no stale-data flash when rapidly switching the input (account/network) that drives the effect. +- For cleanup fixes: navigate to the screen and back N times → timer/listener count stays flat (see [js-memory-leaks.md](js-memory-leaks.md)). + +## Don't over-correct + +- Effects that *synchronize with external systems* (subscriptions, navigation, imperative APIs) are the legitimate use — don't mechanically rewrite every effect as `useMemo`. +- An async effect whose component provably never unmounts mid-flight (e.g. root-level, app lifetime) doesn't need a cancelled flag — but say so in review rather than assuming. +- Don't wrap trivial derivations in `useMemo` while de-effecting — plain expressions are fine until profiling or a memoized child says otherwise. + +## Related + +- [mm-hook-dependency-arrays.md](mm-hook-dependency-arrays.md) — the deps side: JSON.stringify, inline literals, stale closures +- [js-memory-leaks.md](js-memory-leaks.md) — measuring leaks the missing cleanups cause +- [mm-streaming-realtime.md](mm-streaming-realtime.md) — subscription setup/teardown for real-time screens +- [mm-unstable-hook-return.md](mm-unstable-hook-return.md) — unstable hook returns that make effects re-run diff --git a/domains/performance/skills/performance/repos/metamask-mobile.md b/domains/performance/skills/performance/repos/metamask-mobile.md index d077f7df..fa4fede6 100644 --- a/domains/performance/skills/performance/repos/metamask-mobile.md +++ b/domains/performance/skills/performance/repos/metamask-mobile.md @@ -54,6 +54,9 @@ Always pair measurement with the **power-user scenario on Android** — see [ref | `useSelector` returns new refs; `useSelector(x, isEqual)` band-aids | [mm-redux-antipatterns.md](references/mm-redux-antipatterns.md) | | Whole subtree re-renders under a Context provider | [mm-context-performance.md](references/mm-context-performance.md) | | `useEffect`/`useMemo` re-runs constantly; `JSON.stringify` in deps | [mm-hook-dependency-arrays.md](references/mm-hook-dependency-arrays.md) | +| Effect chains (`setState` in effect triggers next effect); setState after unmount; missing timer/listener cleanup | [mm-useeffect-antipatterns.md](references/mm-useeffect-antipatterns.md) | +| **One selector change re-renders half the app**; `isEqual`/`createDeepEqualSelector` band-aids accumulating downstream | [mm-selector-cascade.md](references/mm-selector-cascade.md) | +| O(n) `.find()` scans per render; parameterized selector recomputes for every list row; component with 5+ `useSelector` calls | [mm-state-normalization.md](references/mm-state-normalization.md) | | Animation janky; `useNativeDriver: false` on width/height | [mm-layout-animations.md](references/mm-layout-animations.md) → [js-animations-reanimated.md](references/js-animations-reanimated.md) | | List scroll jank / unbounded list | [js-lists-flatlist-flashlist.md](references/js-lists-flatlist-flashlist.md) | | Search/filter input blocks typing | [js-concurrent-react.md](references/js-concurrent-react.md) | @@ -67,6 +70,7 @@ Always pair measurement with the **power-user scenario on Android** — see [ref | Native module / sync method blocking JS | [native-sdks-over-polyfills.md](references/native-sdks-over-polyfills.md) | | Native lib crashes on 16KB-page Android | [native-android-16kb-alignment.md](references/native-android-16kb-alignment.md) | | Enable automatic memoization | [mm-react-compiler.md](references/mm-react-compiler.md) → [js-react-compiler.md](references/js-react-compiler.md) | +| Compiler is enabled but a component shows no `Memo ✨`; compiler errors in build output — which are real? | [mm-react-compiler-error-triage.md](references/mm-react-compiler-error-triage.md) | ## Verified anti-pattern catalogue (this codebase) @@ -86,6 +90,8 @@ Ordered by impact. Each links to the guide with the fix. **The `Where` column li | High | lodash main-package imports (98 files, no tree-shaking) | 98 files | [bundle-library-size.md](references/bundle-library-size.md) | | High | FlatList missing perf props on growing lists | 65 FlatList JSX | [js-lists-flatlist-flashlist.md](references/js-lists-flatlist-flashlist.md) | | High | AppState listener without cleanup | `app/core/SDKConnectV2/services/connection-registry.ts:487` | [js-memory-leaks.md](references/js-memory-leaks.md) | +| High | Parameterized selector (single-entry cache, busted per arg) doing an O(n) `Object.values().flat().find()` scan per call | `selectSingleTokenByAddressAndChainId` `app/selectors/tokensController.ts:174`; also `app/selectors/assets/assets-list.ts`, `app/selectors/moneyAccountController/index.ts` | [mm-state-normalization.md](references/mm-state-normalization.md) | +| Medium | Async effect without cancellation; setState-chain effects; derived state via useEffect+setState | feature-specific — run the guide's greps | [mm-useeffect-antipatterns.md](references/mm-useeffect-antipatterns.md) | | Medium | Inline `useSelector(state => state.x)` bypassing named selectors | 3 files | [mm-redux-antipatterns.md](references/mm-redux-antipatterns.md) | | Medium | Lottie where Rive fits (Rive already installed) | 5 files | [js-animations-reanimated.md](references/js-animations-reanimated.md) | | Low | dayjs + luxon both present (dedup) | 4 + 6 files | [bundle-library-size.md](references/bundle-library-size.md) | @@ -101,4 +107,4 @@ Ordered by impact. Each links to the guide with the fix. **The `Where` column li ## Attribution -Generic React Native references (`js-*`, `native-*`, `bundle-*`) adapted from "The Ultimate Guide to React Native Optimization" by Callstack. MetaMask-specific guidance (`mm-*`) from the internal Performance Guide for Engineers and verified codebase audits. +Generic React Native references (`js-*`, `native-*`, `bundle-*`) adapted from "The Ultimate Guide to React Native Optimization" by Callstack. MetaMask-specific guidance (`mm-*`) from the internal Performance Guide for Engineers and verified codebase audits. Cross-platform React/Redux guidance (`mm-selector-cascade`, `mm-useeffect-antipatterns`, `mm-state-normalization`, `mm-react-compiler-error-triage`) adapted from MetaMask contributor-docs [`frontend-performance.md`](https://github.com/MetaMask/contributor-docs/blob/main/docs/frontend-performance.md) and the extension performance audit (MetaMask-planning#6571; extension PRs metamask-extension#38007, metamask-extension#37147). From c654ea175b25788de9f3abb1d058e5d07a50ec25 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Wed, 10 Jun 2026 09:33:10 -0400 Subject: [PATCH 2/9] Note exhaustive dependency-tree traversal and topological fix ordering in `mm-selector-cascade` --- .../references/mm-selector-cascade.md | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/domains/performance/skills/performance/references/mm-selector-cascade.md b/domains/performance/skills/performance/references/mm-selector-cascade.md index a3a92989..48fe9ef6 100644 --- a/domains/performance/skills/performance/references/mm-selector-cascade.md +++ b/domains/performance/skills/performance/references/mm-selector-cascade.md @@ -29,18 +29,26 @@ export const getMemoizedAccounts = createSelector(getInternalAccounts, (a) => a) Each band-aid suppresses the re-render for one consumer while *adding* an O(n) deep comparison on every dispatch — and the cascade cost scales superlinearly with power-user data (see [mm-power-user-scenario.md](mm-power-user-scenario.md)). -## Step 1 — Measure the blast radius before fixing +## Step 1 — Traverse the dependency tree exhaustively before fixing -The repair PR's evidence (and its review) should enumerate the graph, the way #37147 did: +The repair PR's evidence (and its review) should enumerate the graph **to closure** — every selector reachable from the suspect, not just its immediate neighborhood — the way #37147 did: 1. **Direct consumers:** every selector that lists the suspect as an input. `grep -rn "getInternalAccounts" app/selectors --include="*.ts"` -2. **Transitive consumers:** repeat one level down for each direct consumer (in practice 2 levels reaches most of the graph). +2. **Transitive consumers:** repeat for each direct consumer until the frontier adds no new selectors. Don't stop at a fixed depth — cascades often have **more than one broken root**, and a partial map produces a wrong fix order. 3. **Component consumers:** `useSelector` call sites of anything in the graph. 4. **Recomputation count:** instrument with `selector.recomputations()` (reselect) or a `console.count` in the result function across a few dispatches (a balance poll is a convenient metronome). A before/after table — *recomputations per dispatch, re-renders per poll cycle, on the same interaction* — is what distinguishes a verified cascade fix from a speculative refactor. -## Step 2 — Fix the root, not the 50 consumers +## Step 2 — Plan the memoization fix order from the map: roots first + +Write down the fix order before writing any fix. The order is **topological** — roots, then their descendants, layer by layer: + +- A descendant fix can't be *verified* while any of its inputs is still unstable: its output identity keeps changing for upstream reasons, so the before/after numbers measure the wrong thing. +- Most descendant "problems" stop being fixes once the roots are stable — they reclassify from "add memoization here" to "remove the band-aid here" (Step 4). The plan is what tells you which is which *in advance*, instead of memoizing selectors that were only recomputing because of their inputs. +- If the map surfaced multiple roots sharing consumers, fix them together — otherwise the shared consumers keep re-rendering and the first root's win never shows up in the numbers. + +## Step 3 — Fix the root, not the 50 consumers Memoizing consumers one by one is whack-a-mole: each fix adds comparison cost and the graph keeps re-deriving from a poisoned root. Trace **upward** (who are my inputs? are *they* stable?) until you hit the selector whose output identity changes without its data changing — that's the root. Fix its memoization there (patterns + recipes in [mm-selector-memoization.md](mm-selector-memoization.md)). @@ -51,7 +59,7 @@ Memoizing consumers one by one is whack-a-mole: each fix adds comparison cost an Establish which contract a slice actually follows (log `prev === next` for the input across two unrelated dispatches) before choosing — the answer differs per slice, and assuming the wrong contract either reintroduces the cascade or buys deep-compares you don't need. -## Step 3 — Sweep the graph and *remove* the band-aids +## Step 4 — Sweep the graph and *remove* the band-aids This is the step most fixes skip. After the root is stable, every downstream `isEqual`, `createDeepEqualSelector`-wrapping-a-now-stable-input, and `getMemoized*` duplicate is dead weight: it still runs its deep comparison on every dispatch, and it **masks regressions** — if the root breaks again, the band-aids hide it until the app is slow everywhere again. From 48a2a9e1f5cca8b78ff61ed6b91cfe66b07a1b00 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Wed, 10 Jun 2026 12:49:53 -0400 Subject: [PATCH 3/9] Wire new references into audit/planning playbooks; note effect-deps memoization exception The effect-dependency exception follows official compiler guidance: removing manual memoization whose output feeds a `useEffect` dependency can over/under- fire effects (reactwg/react-compiler#16). --- .../performance/references/mm-audit-playbook.md | 16 +++++++++++++--- .../skills/performance/references/mm-planning.md | 4 ++-- .../performance/references/mm-react-compiler.md | 2 ++ .../references/mm-selector-cascade.md | 1 + 4 files changed, 18 insertions(+), 5 deletions(-) diff --git a/domains/performance/skills/performance/references/mm-audit-playbook.md b/domains/performance/skills/performance/references/mm-audit-playbook.md index b3f302c7..e9fb8b86 100644 --- a/domains/performance/skills/performance/references/mm-audit-playbook.md +++ b/domains/performance/skills/performance/references/mm-audit-playbook.md @@ -42,8 +42,9 @@ Read the call sites: is a data hook running for tabs/pages/items that aren't vis grep -rn "createSelector(" app/selectors --include="*.ts" | grep -v createDeepEqualSelector grep -rn "=> .*\.\(map\|filter\|sort\|reverse\)\|new Set\|new Map\|Object\.\(values\|keys\|entries\)\|?? {}\|?? \[\]" app/selectors --include="*.ts" grep -rn "\.sort(\|\.reverse(\|\.push(\|\.splice(" app/selectors --include="*.ts" # mutation +grep -rn "(_state\|(_," app/selectors --include="*.ts" # parameterized selectors — single-entry cache → mm-state-normalization.md ``` -Check each result function for: identity/passthrough, new collection without deep-equal, mutation, `state=>state` input. +Check each result function for: identity/passthrough, new collection without deep-equal, mutation, `state=>state` input. If one broken selector has **many consumers**, switch to the cascade playbook — map the dependency tree to closure and plan the fix order *before* fixing anything: [mm-selector-cascade.md](mm-selector-cascade.md). ### Redux / useSelector → [mm-redux-antipatterns.md](mm-redux-antipatterns.md) ```bash @@ -57,11 +58,18 @@ grep -rn "dispatch(" app --include="*.ts" --include="*.tsx" | grep -v ".test." | grep -rn "Provider value={{" app --include="*.tsx" | grep -v ".test." ``` -### Hooks → [mm-hook-dependency-arrays.md](mm-hook-dependency-arrays.md) +### Hooks → [mm-hook-dependency-arrays.md](mm-hook-dependency-arrays.md) / [mm-useeffect-antipatterns.md](mm-useeffect-antipatterns.md) ```bash grep -rn "\[JSON.stringify\|, JSON.stringify" app --include="*.ts" --include="*.tsx" | grep -v ".test." +grep -rn -A6 "useEffect(" app --include="*.ts" --include="*.tsx" | grep -E "fetch\(|\.then\(" | grep -v "signal\|cancelled\|abort" | grep -v ".test." # async effects without cancellation ``` -(`exhaustive-deps` is NOT linted in this repo — check effect deps by hand.) +(`exhaustive-deps` is NOT linted in this repo — check effect deps by hand.) For effect-body problems — derived state via useEffect+setState, effect chains, post-unmount setState — use the read pass in [mm-useeffect-antipatterns.md](mm-useeffect-antipatterns.md). + +### React Compiler coverage → [mm-react-compiler-error-triage.md](mm-react-compiler-error-triage.md) +```bash +grep -rn "use no memo" app --include="*.ts" --include="*.tsx" # opt-outs: each needs a reason + TODO +``` +For a re-render-heavy screen, confirm the components are actually **compiled** (`Memo ✨` in DevTools) before suggesting manual memoization — they may be sitting in the error/unsupported bucket. ### Animations → [mm-layout-animations.md](mm-layout-animations.md) ```bash @@ -107,6 +115,8 @@ Confirm any hit with the Profiler ("why did this render?") before asserting — - [ ] No real-time / high-frequency data dispatched to Redux - [ ] `Context.Provider value` is memoized (not an inline object) - [ ] No `JSON.stringify` in a hot dependency array +- [ ] Async effects guard against post-unmount / stale setState (cancelled flag or `AbortController`) +- [ ] No new parameterized selector (single-entry cache) on a list/hot path — use a lookup-map selector instead - [ ] Layout animations use Reanimated v3, not `Animated` + `useNativeDriver:false` - [ ] Growable lists use FlashList with stable keys (+ `getItemType` if mixed) - [ ] New event listeners / timers / subscriptions have cleanup diff --git a/domains/performance/skills/performance/references/mm-planning.md b/domains/performance/skills/performance/references/mm-planning.md index ca23c644..3416ac43 100644 --- a/domains/performance/skills/performance/references/mm-planning.md +++ b/domains/performance/skills/performance/references/mm-planning.md @@ -22,7 +22,7 @@ The cheapest performance fix is the one you make before writing code. Catch arch | Real-time / WebSocket data | Updates faster than once per user action? | Never put it in Redux. Local state / shared value / direct UI update. Manage subscribe/unsubscribe by visibility + app foreground/background; avoid double-subscribe. See [mm-redux-antipatterns.md](mm-redux-antipatterns.md). | | Unbounded data | Can the list/dataset grow without ceiling? | Paginate + virtualize from day one; plan server-side filtering. | | Large lists | >~50 items now, infinite later? | FlashList v2 with stable keys + `getItemType`; no heavy work per item. [js-lists-flatlist-flashlist.md](js-lists-flatlist-flashlist.md) | -| New selector / derived state | Adding `createSelector`? | Decide memoization + equality up front; never identity/mutation. [mm-selector-memoization.md](mm-selector-memoization.md) | +| New selector / derived state | Adding `createSelector`? | Decide memoization + equality up front; never identity/mutation. [mm-selector-memoization.md](mm-selector-memoization.md). Frequent keyed lookups? Decide the lookup shape now (keyed index vs O(n) scan) — [mm-state-normalization.md](mm-state-normalization.md) | | Heavy computation | Big transforms, sorts, regex on large input? | Server offload, or memoize, or defer with `useDeferredValue`. | | Crypto | Hashing/signing/derivation in hot path? | `react-native-quick-crypto` (already installed); keep off the JS thread. | | New npm dependency | Adds to `package.json`? | Check size (Expo Atlas / bundlephobia); avoid main-package/barrel imports; reuse existing libs (we already have dayjs, luxon, lodash). [bundle-library-size.md](bundle-library-size.md) | @@ -34,7 +34,7 @@ The cheapest performance fix is the one you make before writing code. Catch arch ## System-design checklist -- **State shape:** new Redux slice for real-time data? → flag. New selector? → memoization + equality decided now. +- **State shape:** new Redux slice for real-time data? → flag. New selector? → memoization + equality decided now. Frequent lookups by key? → plan a `byId`/`byAddress` index ([mm-state-normalization.md](mm-state-normalization.md)). - **Subscription lifecycle:** diagram subscribe/unsubscribe tied to mount/unmount + foreground/background; no double-subscribe; cleanup guaranteed. - **List strategy:** ScrollView only for <20 fixed items; FlashList for anything that can grow; no `.map()` in JSX for growable lists. - **Data flow:** minimize how many components subscribe to a frequently-updating selector. diff --git a/domains/performance/skills/performance/references/mm-react-compiler.md b/domains/performance/skills/performance/references/mm-react-compiler.md index b23e4bc5..4f987cfb 100644 --- a/domains/performance/skills/performance/references/mm-react-compiler.md +++ b/domains/performance/skills/performance/references/mm-react-compiler.md @@ -51,6 +51,8 @@ React Compiler auto-memoizes components, callbacks, and computed values at build On opted-in paths you can gradually drop hand-written `useMemo`/`useCallback`/`React.memo` once the compiler is verified working — but do it deliberately and re-measure. Off opted-in paths, manual memoization still matters. +**Exception — effect dependencies.** Keep any `useMemo`/`useCallback` whose output is used as a `useEffect` dependency, here or in a consumer: the compiler's memoization is not guaranteed to match the manual strategy, and a mismatch causes over/under-firing of effects or infinite loops — a correctness change, not a perf tweak. Official guidance is to leave existing manual memoization in place and only omit it in *new* code ([reactwg/react-compiler#16](https://github.com/reactwg/react-compiler/discussions/16)). + ## What breaks compilation (it will skip the component) - Mutating props or state during render. diff --git a/domains/performance/skills/performance/references/mm-selector-cascade.md b/domains/performance/skills/performance/references/mm-selector-cascade.md index 48fe9ef6..55aa423a 100644 --- a/domains/performance/skills/performance/references/mm-selector-cascade.md +++ b/domains/performance/skills/performance/references/mm-selector-cascade.md @@ -96,6 +96,7 @@ Rule of thumb: values that **cross a file boundary** (Redux selectors, imported 2. Recomputation counts on direct + transitive consumers drop to ~0 on unrelated dispatches. 3. Profiler on a top consumer (account list, send flow): the re-render cascade is gone during a balance poll. 4. The band-aid greps above return no hits inside the repaired graph. +5. Lock the win in CI: add a Reassure `*.perf-test.tsx` on a top consumer so the cascade can't silently return. ## Related From 2349b3c8b4aef99672be6d033e9c97ec97c2f613 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Wed, 10 Jun 2026 13:25:16 -0400 Subject: [PATCH 4/9] Pull verified insights from skills PRs #43/#41 and the Performance Guide - WDYR (`wdyr.js`, `useSelector` diff tracking) as the live cascade tracer - input-unstable vs output-unstable tool selection (`resultEqualityCheck`) - a live cascade nullifies downstream memo/virtualization/compiler wins - plain-function selector sweep; span quota guardrail; redux-persist caveat --- .../skills/performance/references/mm-audit-playbook.md | 1 + .../performance/skills/performance/references/mm-planning.md | 2 +- .../skills/performance/references/mm-selector-cascade.md | 5 +++++ .../performance/skills/performance/references/mm-tools.md | 4 ++++ 4 files changed, 11 insertions(+), 1 deletion(-) diff --git a/domains/performance/skills/performance/references/mm-audit-playbook.md b/domains/performance/skills/performance/references/mm-audit-playbook.md index e9fb8b86..bb6a25c2 100644 --- a/domains/performance/skills/performance/references/mm-audit-playbook.md +++ b/domains/performance/skills/performance/references/mm-audit-playbook.md @@ -43,6 +43,7 @@ grep -rn "createSelector(" app/selectors --include="*.ts" | grep -v createDeepEq grep -rn "=> .*\.\(map\|filter\|sort\|reverse\)\|new Set\|new Map\|Object\.\(values\|keys\|entries\)\|?? {}\|?? \[\]" app/selectors --include="*.ts" grep -rn "\.sort(\|\.reverse(\|\.push(\|\.splice(" app/selectors --include="*.ts" # mutation grep -rn "(_state\|(_," app/selectors --include="*.ts" # parameterized selectors — single-entry cache → mm-state-normalization.md +grep -rnE "export (function|const) (get|select)[A-Z][A-Za-z]* = \(state|export function (get|select)" app/selectors --include="*.ts" # plain unmemoized function selectors (no createSelector at all) ``` Check each result function for: identity/passthrough, new collection without deep-equal, mutation, `state=>state` input. If one broken selector has **many consumers**, switch to the cascade playbook — map the dependency tree to closure and plan the fix order *before* fixing anything: [mm-selector-cascade.md](mm-selector-cascade.md). diff --git a/domains/performance/skills/performance/references/mm-planning.md b/domains/performance/skills/performance/references/mm-planning.md index 3416ac43..3a610685 100644 --- a/domains/performance/skills/performance/references/mm-planning.md +++ b/domains/performance/skills/performance/references/mm-planning.md @@ -20,7 +20,7 @@ The cheapest performance fix is the one you make before writing code. Catch arch | Risk | Trigger question | Default mitigation | |---|---|---| | Real-time / WebSocket data | Updates faster than once per user action? | Never put it in Redux. Local state / shared value / direct UI update. Manage subscribe/unsubscribe by visibility + app foreground/background; avoid double-subscribe. See [mm-redux-antipatterns.md](mm-redux-antipatterns.md). | -| Unbounded data | Can the list/dataset grow without ceiling? | Paginate + virtualize from day one; plan server-side filtering. | +| Unbounded data | Can the list/dataset grow without ceiling? | Paginate + virtualize from day one; plan server-side filtering. Never persist unbounded data via redux-persist — use a dedicated storage layer. | | Large lists | >~50 items now, infinite later? | FlashList v2 with stable keys + `getItemType`; no heavy work per item. [js-lists-flatlist-flashlist.md](js-lists-flatlist-flashlist.md) | | New selector / derived state | Adding `createSelector`? | Decide memoization + equality up front; never identity/mutation. [mm-selector-memoization.md](mm-selector-memoization.md). Frequent keyed lookups? Decide the lookup shape now (keyed index vs O(n) scan) — [mm-state-normalization.md](mm-state-normalization.md) | | Heavy computation | Big transforms, sorts, regex on large input? | Server offload, or memoize, or defer with `useDeferredValue`. | diff --git a/domains/performance/skills/performance/references/mm-selector-cascade.md b/domains/performance/skills/performance/references/mm-selector-cascade.md index 55aa423a..7d44e75d 100644 --- a/domains/performance/skills/performance/references/mm-selector-cascade.md +++ b/domains/performance/skills/performance/references/mm-selector-cascade.md @@ -29,6 +29,8 @@ export const getMemoizedAccounts = createSelector(getInternalAccounts, (a) => a) Each band-aid suppresses the re-render for one consumer while *adding* an O(n) deep comparison on every dispatch — and the cascade cost scales superlinearly with power-user data (see [mm-power-user-scenario.md](mm-power-user-scenario.md)). +A live cascade also **nullifies every optimization downstream of it**: `React.memo` children re-render anyway (their props are fresh refs), virtualized rows churn, compiler-memoized components re-render (the unstable value crosses the file boundary), and `useMemo`s recompute. Fix the cascade before evaluating any other optimization on the screen — and re-measure them after. + ## Step 1 — Traverse the dependency tree exhaustively before fixing The repair PR's evidence (and its review) should enumerate the graph **to closure** — every selector reachable from the suspect, not just its immediate neighborhood — the way #37147 did: @@ -37,6 +39,7 @@ The repair PR's evidence (and its review) should enumerate the graph **to closur 2. **Transitive consumers:** repeat for each direct consumer until the frontier adds no new selectors. Don't stop at a fixed depth — cascades often have **more than one broken root**, and a partial map produces a wrong fix order. 3. **Component consumers:** `useSelector` call sites of anything in the graph. 4. **Recomputation count:** instrument with `selector.recomputations()` (reselect) or a `console.count` in the result function across a few dispatches (a balance poll is a convenient metronome). +5. **WDYR pass — already wired in this repo:** `wdyr.js` at the repo root tracks `useSelector` hook diffs. Run `ENABLE_WHY_DID_YOU_RENDER=true yarn start`, reproduce one dispatch, and every consumer logging *same values, different reference* is a node in the cascade — the live counterpart of the static map above. A before/after table — *recomputations per dispatch, re-renders per poll cycle, on the same interaction* — is what distinguishes a verified cascade fix from a speculative refactor. @@ -59,6 +62,8 @@ Memoizing consumers one by one is whack-a-mole: each fix adds comparison cost an Establish which contract a slice actually follows (log `prev === next` for the input across two unrelated dispatches) before choosing — the answer differs per slice, and assuming the wrong contract either reintroduces the cascade or buys deep-compares you don't need. +Then match the tool to **which side is unstable**: an unstable *input* (slice replaced wholesale on sync) calls for a deep-equal **input** compare (`createDeepEqualSelector`); a stable input with an unstable *output* (the result function allocates a fresh collection) calls for a `resultEqualityCheck`, so an unchanged result returns the cached ref. Deep-equalizing inputs to paper over an allocating result function runs the wrong comparison on every dispatch. + ## Step 4 — Sweep the graph and *remove* the band-aids This is the step most fixes skip. After the root is stable, every downstream `isEqual`, `createDeepEqualSelector`-wrapping-a-now-stable-input, and `getMemoized*` duplicate is dead weight: it still runs its deep comparison on every dispatch, and it **masks regressions** — if the root breaks again, the band-aids hide it until the app is slow everywhere again. diff --git a/domains/performance/skills/performance/references/mm-tools.md b/domains/performance/skills/performance/references/mm-tools.md index e7e7d2df..7f4b4eab 100644 --- a/domains/performance/skills/performance/references/mm-tools.md +++ b/domains/performance/skills/performance/references/mm-tools.md @@ -84,6 +84,9 @@ Then read what's emitted — a load log (e.g. `source: 'cache' | 'fresh_fetch'`, "Components re-render too much" → React Native DevTools → "why did this render?" → mm-selector-memoization.md / mm-redux-antipatterns.md + → or WDYR (wired at wdyr.js, tracks useSelector diffs): ENABLE_WHY_DID_YOU_RENDER=true yarn start + — logs consumers re-rendering on same-values/new-reference; ideal for tracing a selector cascade + → mm-selector-cascade.md "Search/filter input lags while typing" → js-concurrent-react.md (useDeferredValue) — and memo() the expensive child @@ -162,6 +165,7 @@ endTrace({ name: TraceName.AssetDetails }); // end const x = trace({ name: TraceName.Tokens, op: TraceOperation.UIStartup }, () => build()); ``` - New flow → add a `TraceName` (+ `TraceOperation`) to `app/util/trace.ts`, then wrap it. +- **Quota guardrail:** never start a span per list item, per row, or per poll tick — span volume multiplies by data size × user count. A high-frequency span needs a deterministic sub-sample gate (and a kill-switch) before it ships. - **Component-level: use a per-feature measurement hook, not raw `trace()`.** The repo convention is a declarative `useXMeasurement` hook (e.g. `app/components/UI/Predict/hooks/usePredictMeasurement.ts`, `usePerpsMeasurement`, `useSectionPerformance`) that starts on mount and ends when conditions are true — which structurally enforces the "end on data-loaded, not mount" rule below: ```ts usePredictMeasurement({ traceName: TraceName.PredictMarketDetailsView, conditions: [dataLoaded, !isLoading] }); From a29d5508d6e097a1623966af9458e80b780eca67 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Wed, 10 Jun 2026 13:52:27 -0400 Subject: [PATCH 5/9] Add audit-wave mode to `mm-audit-playbook`: shared-infra sweeps, acceptance criteria, cross-platform check --- .../skills/performance/references/mm-audit-playbook.md | 1 + 1 file changed, 1 insertion(+) diff --git a/domains/performance/skills/performance/references/mm-audit-playbook.md b/domains/performance/skills/performance/references/mm-audit-playbook.md index bb6a25c2..a649a037 100644 --- a/domains/performance/skills/performance/references/mm-audit-playbook.md +++ b/domains/performance/skills/performance/references/mm-audit-playbook.md @@ -12,6 +12,7 @@ For reviewing a PR/diff or auditing a file, component, or feature. Output: findi - **Targeted** (single file / component / small diff): read the files and report concrete findings with `file:line`. - **Broad** (whole feature / repo): run the grep sweeps below and triage hits; don't read everything. +- **Audit wave / program** (scheduled audit of a surface or division): per-surface audits miss mechanism-level patterns that live in *shared* infrastructure (`app/selectors`, shared hooks, the store) — run the cross-cutting sweeps below over the shared dirs **once per wave**, not once per team, and route findings to surface owners. Attach quantified acceptance criteria up front (template in [mm-planning.md](mm-planning.md)). If the surface ships on both platforms, cross-check the sibling platform's audit findings for the same surface before fresh discovery — the React/Redux mechanism patterns recur across extension and mobile. Always: **measure before asserting impact** where feasible, and respect the guardrails at the bottom (don't over-flag). From 20fa10964702ff29d017099c8ca91a218fe36179 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Wed, 10 Jun 2026 14:36:10 -0400 Subject: [PATCH 6/9] Correct view-selector consolidation guidance for batched controller sync `useSelector` count alone is weak evidence on this codebase: controller state changes batch into a 250ms flush (`app/core/Batcher`) dispatched inside `unstable_batchedUpdates` (`EngineService`), so checks run a few times per second and React renders once per flush. Flag expensive or unstable selectors among the N instead; consolidation is the exception (per-row components, unmemoized derivations), not the default. --- .../performance/references/mm-state-normalization.md | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/domains/performance/skills/performance/references/mm-state-normalization.md b/domains/performance/skills/performance/references/mm-state-normalization.md index ea27cfbe..96c5bfc4 100644 --- a/domains/performance/skills/performance/references/mm-state-normalization.md +++ b/domains/performance/skills/performance/references/mm-state-normalization.md @@ -90,7 +90,9 @@ const gasFee = useSelector(getGasFee); // … ×8 more ``` -Each `useSelector` is an independent store subscription with its own equality check per dispatch. When a component reads 5+ related values, combine them into **one memoized view selector** that returns the assembled view-model. One subscription, one equality check, one place where the shape is defined — and a natural home for the derivation logic that would otherwise sit unmemoized in the component (where, note, the React Compiler can't stabilize it — see [mm-selector-cascade.md](mm-selector-cascade.md)). +Each `useSelector` is an independent store subscription with its own equality check per store notification. **Check the dispatch cadence before flagging count alone:** in this codebase, controller state changes batch into a 250ms flush (`app/core/Batcher`, `EngineService`'s `updateBatcher`) and dispatch inside `unstable_batchedUpdates`, so checks run at most a few times per second and React renders once per flush — N cheap accessor reads are *not* a problem. The actionable findings inside a high-count component are the **expensive** selectors (cost paid on every check) and the **unstable-ref** selectors (a re-render per flush) — triage and fix those individually first. + +Consolidating related reads into **one memoized view selector** still earns its keep in two cases: a component repeated per row (per-row × per-flush multiplication of any expensive check), and derivation logic that would otherwise sit unmemoized in the component (where the React Compiler can't stabilize it — see [mm-selector-cascade.md](mm-selector-cascade.md)). One subscription, one equality check, one place where the shape is defined. The same consolidation applies to **duplicate derived-data implementations**: the extension audit found 4+ independent fiat-conversion code paths recomputing the same numbers in different components. One canonical selector ends both the wasted compute and the drift between implementations. @@ -103,7 +105,7 @@ grep -rn "Object.values(.*)\.\(find\|filter\)\|\.find((" app/selectors app/compo # reshaping selectors: nested loops/reduce building objects in a result function grep -rn -B2 "??= {}\|reduce((acc" app/selectors --include="*.ts" -# components with many subscriptions (5+ useSelector in one file = consolidation candidate) +# components with many subscriptions — triage the N selectors for cost/stability, don't flag the count itself grep -rc "useSelector(" app/components --include="*.tsx" | awk -F: '$2>=5' | sort -t: -k2 -rn | head -20 ``` @@ -117,6 +119,7 @@ grep -rc "useSelector(" app/components --include="*.tsx" | awk -F: '$2>=5' | sor - Don't normalize a slice that's only ever iterated in full — indexes pay for themselves on *keyed lookups*, not on `.map()` over everything. - Don't merge *unrelated* selectors into one mega view selector — that re-couples components to data they don't read and re-renders them for it. Consolidate related values consumed together. +- Don't flag a component for its `useSelector` **count** — with batched controller sync (250ms flush + `unstable_batchedUpdates`), N cheap subscriptions are noise. Flag the expensive or unstable selectors *among* them. - `maxSize`/factory-selector machinery is for genuinely parameterized hot paths; for one or two call sites the lookup-map pattern is simpler and stays correct. ## Related From 39d5f8db8b16a0a62c785716c1c63decad32cc70 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Wed, 10 Jun 2026 14:50:44 -0400 Subject: [PATCH 7/9] Encode audit-verified triage calibration: memoizer checks, inline-accessor note, engine slice contract From a per-selector triage of the 10 highest-subscription components: ~90% of reads ruled out (flag booleans, primitive accessors, useMemo'd factories). Adds: check for `weakMapMemoize` (and fresh-object args that defeat it) before flagging parameterized selectors; inline accessors returning primitives are cleanup not perf findings; verified engine slice mechanics (per-controller key replace + Immer structural sharing); deep-equal selectors are output-stable but pay O(input) per check. --- .../skills/performance/references/mm-redux-antipatterns.md | 2 ++ .../skills/performance/references/mm-selector-cascade.md | 2 ++ .../skills/performance/references/mm-state-normalization.md | 4 +++- 3 files changed, 7 insertions(+), 1 deletion(-) diff --git a/domains/performance/skills/performance/references/mm-redux-antipatterns.md b/domains/performance/skills/performance/references/mm-redux-antipatterns.md index dd1d853a..965811a7 100644 --- a/domains/performance/skills/performance/references/mm-redux-antipatterns.md +++ b/domains/performance/skills/performance/references/mm-redux-antipatterns.md @@ -38,6 +38,8 @@ const browserTabs = useSelector((state: any) => state.browser.tabs); // also dro **Why it's wrong:** an inline accessor returning an array/object hands a fresh reference to the consumer whenever that slice changes (and defeats reuse/memoization across the app). For derived data it's worse — `useSelector(s => s.items.filter(...))` allocates every render. +**Perf-triage note:** an inline accessor returning a **primitive or stable field** (`s => s.settings.basicFunctionalityEnabled`) is reuse/type debt, not a re-render bug — the new arrow function per render is irrelevant; only the result's identity matters. Flag it for cleanup, not as a perf finding. + **Fix:** create a named selector in `app/selectors/`: ```ts // selectors/browser.ts diff --git a/domains/performance/skills/performance/references/mm-selector-cascade.md b/domains/performance/skills/performance/references/mm-selector-cascade.md index 7d44e75d..ee75395c 100644 --- a/domains/performance/skills/performance/references/mm-selector-cascade.md +++ b/domains/performance/skills/performance/references/mm-selector-cascade.md @@ -64,6 +64,8 @@ Establish which contract a slice actually follows (log `prev === next` for the i Then match the tool to **which side is unstable**: an unstable *input* (slice replaced wholesale on sync) calls for a deep-equal **input** compare (`createDeepEqualSelector`); a stable input with an unstable *output* (the result function allocates a fresh collection) calls for a `resultEqualityCheck`, so an unchanged result returns the cached ref. Deep-equalizing inputs to paper over an allocating result function runs the wrong comparison on every dispatch. +Verified mechanism for this repo (`app/core/redux/slices/engine`): `UPDATE_BG_STATE` replaces only the **changed controller's key** with `Engine.state[key]`, and BaseController v2 state is Immer-produced — so an unchanged controller keeps its reference across flushes, and unchanged paths *within* a changed controller are structurally shared. Plain accessors into controller state are stable by construction; deep-equal is only warranted where a selector's *inputs* genuinely churn. And remember a deep-equal selector is output-**stable** but pays its compare per check, scaled by input size — over a power-user transaction history that is an O(n) deep compare per consumer per flush. + ## Step 4 — Sweep the graph and *remove* the band-aids This is the step most fixes skip. After the root is stable, every downstream `isEqual`, `createDeepEqualSelector`-wrapping-a-now-stable-input, and `getMemoized*` duplicate is dead weight: it still runs its deep comparison on every dispatch, and it **masks regressions** — if the root breaks again, the band-aids hide it until the app is slow everywhere again. diff --git a/domains/performance/skills/performance/references/mm-state-normalization.md b/domains/performance/skills/performance/references/mm-state-normalization.md index 96c5bfc4..33a24994 100644 --- a/domains/performance/skills/performance/references/mm-state-normalization.md +++ b/domains/performance/skills/performance/references/mm-state-normalization.md @@ -44,7 +44,7 @@ const a2 = useSelector((s) => getAccountByAddress(s, addr2)); // miss, evicts ad const a3 = useSelector((s) => getAccountByAddress(s, addr3)); // miss, evicts addr2 — and so on every render cycle ``` -In a list rendering N rows, the "memoized" selector recomputes N times per render, forever. Fixes, in order of preference: +In a list rendering N rows, the "memoized" selector recomputes N times per render, forever. **Check the memoizer before flagging:** this codebase already uses `weakMapMemoize` for some parameterized selectors (e.g. `selectNetworkConfigurationByChainId`), which caches per-argument and doesn't thrash — but only for *stable* arguments. A fresh **object literal** argument per call (`selectAsset(state, { address, chainId, isStaked })`) defeats `weakMapMemoize` too: every call is a new WeakMap key. Fixes, in order of preference: 1. **Lookup-map selector** (above): select the whole memoized index once; key into it. Sidesteps per-arg caching entirely. 2. **Per-instance selector**: a factory (`makeSelectAccountByAddress()`) instantiated in the component with `useMemo`, so each call site owns its own cache slot. @@ -92,6 +92,8 @@ const gasFee = useSelector(getGasFee); Each `useSelector` is an independent store subscription with its own equality check per store notification. **Check the dispatch cadence before flagging count alone:** in this codebase, controller state changes batch into a 250ms flush (`app/core/Batcher`, `EngineService`'s `updateBatcher`) and dispatch inside `unstable_batchedUpdates`, so checks run at most a few times per second and React renders once per flush — N cheap accessor reads are *not* a problem. The actionable findings inside a high-count component are the **expensive** selectors (cost paid on every check) and the **unstable-ref** selectors (a re-render per flush) — triage and fix those individually first. +Audit calibration (this codebase, 2026-06): a per-selector triage of the 10 highest-count components (9-19 reads each) ruled out ~90% of reads — feature-flag booleans, primitive accessors, and correctly `useMemo`'d factory selectors. The real findings were per-row parameterized selectors and deep-equal selectors over power-user-scaled data. The count was noise; the triage found what mattered. + Consolidating related reads into **one memoized view selector** still earns its keep in two cases: a component repeated per row (per-row × per-flush multiplication of any expensive check), and derivation logic that would otherwise sit unmemoized in the component (where the React Compiler can't stabilize it — see [mm-selector-cascade.md](mm-selector-cascade.md)). One subscription, one equality check, one place where the shape is defined. The same consolidation applies to **duplicate derived-data implementations**: the extension audit found 4+ independent fiat-conversion code paths recomputing the same numbers in different components. One canonical selector ends both the wasted compute and the drift between implementations. From d26935c0efaf23d06b6df78714a3f271838c8d2e Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Wed, 10 Jun 2026 15:27:34 -0400 Subject: [PATCH 8/9] Add the extension enablement-run split counts as the canonical triage example in `mm-react-compiler-error-triage` --- .../performance/references/mm-react-compiler-error-triage.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/domains/performance/skills/performance/references/mm-react-compiler-error-triage.md b/domains/performance/skills/performance/references/mm-react-compiler-error-triage.md index 9d8e8ae1..68db17f8 100644 --- a/domains/performance/skills/performance/references/mm-react-compiler-error-triage.md +++ b/domains/performance/skills/performance/references/mm-react-compiler-error-triage.md @@ -27,7 +27,7 @@ Compiler diagnostics are **not one bucket**. The logger event's `category` field - **`category === 'Todo'` → "unsupported."** Syntax or a pattern the compiler *itself* has not implemented yet. There is **no actionable fix on our side** — rewriting working code to appease an unimplemented compiler path is wasted effort and churn. Count these separately, leave the code alone, and re-check after compiler upgrades. - **Any other category (e.g. `InvalidReact`, `InvalidJS`) → legitimate, actionable.** A real Rules-of-React violation in our code (mutation during render, conditional hooks, side effects in render). Fixing it both unlocks compilation *and* removes a latent correctness bug. -A healthcheck that doesn't make this split is noise: the `Todo` count swamps the actionable list and the team learns to ignore the output. The extension's webpack wrapper makes the split in ~10 lines: +A healthcheck that doesn't make this split is noise: the `Todo` count swamps the actionable list and the team learns to ignore the output. The extension's verbose run at enablement (metamask-extension#38007) is the canonical illustration — of 7,308 files processed: 253 compiled, **31 actionable errors**, **7,024 unsupported** (`Todo`). Without the split that reads as ~7,000 hopeless errors; with it, the team's backlog is 31 files and the rest is the compiler's to burn down across upgrades. The extension's webpack wrapper makes the split in ~10 lines: ```ts // adapted from metamask-extension development/webpack/utils/loaders/reactCompilerLoaderWrapper.ts From 661814d0e511b57d6ae99791229070ff3b687f53 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Thu, 11 Jun 2026 09:07:06 -0400 Subject: [PATCH 9/9] Add proactive tree-ranking and deep-equal input-stability checks to the cascade playbooks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Result-function greps miss an input function that builds a fresh composite per call, which silently downgrades `createDeepEqualSelector` into a whole-composite deep compare on every check. Found post-hoc via manual tree mapping (`getStateForAssetSelector` → asset-surface root, metamask-mobile#31561); this encodes the search so the skill finds the next one itself. --- .../skills/performance/references/mm-audit-playbook.md | 1 + .../skills/performance/references/mm-selector-cascade.md | 2 ++ 2 files changed, 3 insertions(+) diff --git a/domains/performance/skills/performance/references/mm-audit-playbook.md b/domains/performance/skills/performance/references/mm-audit-playbook.md index a649a037..ff756989 100644 --- a/domains/performance/skills/performance/references/mm-audit-playbook.md +++ b/domains/performance/skills/performance/references/mm-audit-playbook.md @@ -107,6 +107,7 @@ Grep finds *syntactic* patterns. The highest-impact re-render bugs are *data-flo - **Render-phase side effects / setState** — any `setState(...)`, `dispatch(...)`, or `trackEvent(...)` in a render body (not inside `useEffect`/`useCallback`)? Triggers extra render passes. - **O(n²) reduce-with-spread** — `reduce((acc, x) => ({ ...acc, ... }), {})` rebuilt every render. - **Per-item subscription hooks** — trace each into its manager; shared subscription = fine, per-subscriber whole-dataset snapshot = bug. → [mm-streaming-realtime.md](mm-streaming-realtime.md) +- **Deep-equal selector inputs** — for every `createDeepEqualSelector`, read its *input selectors*: an input function that allocates a fresh composite per call (object spreads of controller state, other selectors' results collected into a new object) forces the deep compare to run over the whole composite on every check — and no result-function grep catches it. `grep -rn -B3 "createDeepEqualSelector(" app/selectors` lists the sites; read each first argument. → [mm-selector-cascade.md](mm-selector-cascade.md) (proactive mode) Confirm any hit with the Profiler ("why did this render?") before asserting — see [mm-tools.md](mm-tools.md). diff --git a/domains/performance/skills/performance/references/mm-selector-cascade.md b/domains/performance/skills/performance/references/mm-selector-cascade.md index ee75395c..71ea0dfa 100644 --- a/domains/performance/skills/performance/references/mm-selector-cascade.md +++ b/domains/performance/skills/performance/references/mm-selector-cascade.md @@ -43,6 +43,8 @@ The repair PR's evidence (and its review) should enumerate the graph **to closur A before/after table — *recomputations per dispatch, re-renders per poll cycle, on the same interaction* — is what distinguishes a verified cascade fix from a speculative refactor. +**Proactive mode — find the big trees without waiting for a symptom.** Rank roots by blast radius first (grep each selector's name across `app/` for consumer-file counts; appearances inside other selectors' input arrays give direct dependents), then for each large root verify its **input reference-stability**, not just its result function. The pattern that defeats every result-function grep: an input *function* that builds a fresh composite per call — spreading controller states and collecting other selectors' results into a new object. It looks disciplined, passes all pattern sweeps, and silently downgrades a `createDeepEqualSelector` into a whole-composite deep compare on **every check**. Verified instance: `getStateForAssetSelector` feeding `selectAssetsBySelectedAccountGroup` (`app/selectors/assets/assets-list.ts:107`) — the root of the asset-surface tree (15+ dependent selectors, including the per-row `selectAsset`), deep-comparing effectively the entire asset state per consumer per flush. + ## Step 2 — Plan the memoization fix order from the map: roots first Write down the fix order before writing any fix. The order is **topological** — roots, then their descendants, layer by layer: