diff --git a/domains/performance/knowledge/effect-antipatterns.md b/domains/performance/knowledge/effect-antipatterns.md new file mode 100644 index 00000000..2b24006d --- /dev/null +++ b/domains/performance/knowledge/effect-antipatterns.md @@ -0,0 +1,149 @@ +--- +name: effect-antipatterns +domain: performance +description: The React `useEffect` patterns that cause unnecessary renders, memory leaks, or race conditions — the canonical, platform-agnostic taxonomy that per-repo effect references instantiate +--- + +# Effect Anti-Patterns + +**This file is the single source for the pattern taxonomy.** Per-repo references — such as +the `mm-hook-dependency-arrays` and `mm-useeffect-antipatterns` references shipped with the +`performance` skill — name these patterns rather than redefining them, and add what only +they can: verified instances with `file:line`, repo-specific lint gaps, and fix recipes. + +Two halves, and they fail differently. Patterns 1–2 are about **when an effect re-runs** +(the dependency side). Patterns 3–5 are about **what happens inside and after it** (the +lifecycle side). + +## 1. Unstable dependency identity + +A dependency array is supposed to be a cheap identity check. Anything that produces a new +value every render defeats it — and usually signals an unstable reference upstream. + +```typescript +// ❌ serializes on EVERY render just to build the dep key +useEffect(() => { doSomething(config) }, [JSON.stringify(config)]) + +// ❌ new object every render → effect runs every render (or loops forever) +useEffect(() => { ... }, [{ id: user.id }]) + +// ✅ stabilize the reference upstream, then depend on it directly +const stableConfig = useMemo(() => derive(a, b), [a, b]) +useEffect(() => { doSomething(stableConfig) }, [stableConfig]) + +// ✅ or depend on the primitives +useEffect(() => { ... }, [user.id]) +``` + +Stabilizing the source beats hashing it. If you genuinely cannot, a primitive key computed +**once** (`useMemo(() => xs.join(','), [xs])`) still beats a per-render `JSON.stringify`. + +Detection: grep for `JSON.stringify` inside a dependency array, and for inline `{`/`[` +literals in the dep position. + +## 2. Wrong dependencies + +```typescript +// ❌ empty deps but reads state → stale closure, value frozen at first render +const onPress = useCallback(() => doThing(count), []) + +// ❌ empty deps and reads nothing → this was never a hook, hoist it out +const config = useMemo(() => ({ a: 1, b: 2 }), []) +``` + +**Fix:** include what you read; or if there is genuinely nothing to read, move the constant +outside the component. Where `react-hooks/exhaustive-deps` is not enabled, this is not +caught automatically and must be reviewed by hand. + +## 3. Derived state via effect + setState + +If a value is computable from props/state/store, compute it during render. State plus an +effect is for *synchronizing with something external*, not for derivation. + +```typescript +// ❌ two render passes per change: render → effect → setState → render again +const [visible, setVisible] = useState([]) +useEffect(() => { setVisible(items.filter((t) => !t.hidden)) }, [items]) + +// ✅ derive during render — one pass, no state to drift out of sync +const visible = useMemo(() => items.filter((t) => !t.hidden), [items]) +``` + +The React docs call this out directly: +[You Might Not Need an Effect](https://react.dev/learn/you-might-not-need-an-effect). + +### 3a. Cascading effect chains + +The same mistake compounded: effect A sets state, which triggers effect B, which sets +state, which triggers effect C. Each link is a full extra render pass *and* a window where +the UI shows an inconsistent intermediate combination. + +**Fix:** collapse the chain into render-time derivation — one `useMemo` per step, or one +for the lot. + +## 4. Missing timer cleanup + +Every `setInterval` and recurring `setTimeout` started in an effect must be cleared in its +cleanup. Otherwise the timer outlives unmount, fires against dead state, and leaks in +proportion to how often the component mounts. + +```typescript +// ❌ BROKEN: timer leaks after unmount +useEffect(() => { setInterval(poll, 1000) }, []) + +// ✅ FIXED +useEffect(() => { + const id = setInterval(poll, 1000) + return () => clearInterval(id) +}, []) +``` + +## 5. Uncancelled async work + +Async work started in an effect can resolve *after* unmount — or after the input changed, +letting a stale response overwrite a newer one. + +```typescript +// ❌ fetch races unmount; stale data can win +useEffect(() => { fetchMeta(address).then(setMeta) }, [address]) + +// ✅ cancelled flag — cheapest, works for any promise +useEffect(() => { + let cancelled = false + fetchMeta(address).then((m) => { if (!cancelled) setMeta(m) }) + return () => { cancelled = true } +}, [address]) + +// ✅ AbortController — also cancels the request itself +useEffect(() => { + const ctrl = new AbortController() + fetch(url, { signal: ctrl.signal }) + .then((r) => setData(r)) + .catch((e) => { if (e.name !== 'AbortError') throw e }) + return () => ctrl.abort() +}, [url]) +``` + +Pick one and apply it consistently. + +## Why these matter + +- **Renders.** Derived-state effects double every render in the affected subtree, and + chains multiply it. +- **Memory.** Uncleared timers and subscriptions leak proportional to mount count. +- **Correctness.** Uncancelled async work produces "state update on an unmounted + component" warnings and, worse, races where an older response overwrites a newer one. + +## Don't over-correct + +- Don't add `useMemo`/`useCallback` everywhere — only where profiling shows wasted work, or + where a memoized child depends on the reference. Compilers handle many cases on opted-in + paths. +- A `JSON.stringify` on a cold path with a small object is acceptable. Prioritize hot render + paths. + +## Related + +- `render-cascade` — how effect-driven re-renders propagate through the component graph. +- `selector-antipatterns` — the store-side counterpart; an unstable selector result is a + common source of the unstable dependency in pattern 1. diff --git a/domains/performance/knowledge/render-cascade.md b/domains/performance/knowledge/render-cascade.md new file mode 100644 index 00000000..e872081f --- /dev/null +++ b/domains/performance/knowledge/render-cascade.md @@ -0,0 +1,68 @@ +--- +name: render-cascade +domain: performance +description: React+Redux render cascade failure mode — single state change triggers multiple re-render cycles +--- + +# Render Cascade + +Single state change → broken selector returns new reference → `useSelector` detects "change" → parent re-renders all children → children trigger more selectors → cycle repeats 5+ times before stabilizing. + +## Cost Scaling + +| Factor | Impact | +|--------|--------| +| Component tree depth | Each level multiplies re-renders | +| User data size | O(n) selectors × n items = O(n²) operations | +| State update frequency | Background polling compounds the problem | + +Power users (large datasets, many accounts/tokens/transactions) are disproportionately affected. + +## Root Causes + +| Cause | Pattern | Fix | +|-------|---------|-----| +| Plain function selector | `export function get...` | Wrap in `createSelector` | +| Identity function selector | Transform in input, identity in result | Move transform to result function | +| Unnecessary deep equality | `createDeepEqualSelector` on stable Immer inputs | Use `createSelector` | +| O(n) lookup | `.find()` in selector | Normalize state to map; use direct access | +| Chained transforms (unmemoized) | Multiple `.map`/`.filter` in plain function | Single `createSelector` with all transforms | +| Context provider instability | `` inline | `useMemo` the value | +| Props recreation | `useParams()` passed directly as prop | `useMemo` the props object | + +## Selector Creator Decision Tree + +``` +Is INPUT unstable (not from Immer/Redux)? +├── YES → createDeepEqualSelector +└── NO → Is OUTPUT unstable (new array/object from transform)? + ├── YES → createResultEqualSelector (or createShallowResultSelector) + └── NO → createSelector +``` + +## Fix Order — Root Selectors First + +Selectors form a dependency graph. When a root selector returns an unstable reference, the cost cascades: + +- recomputations: **O(m)** — all m dependent selectors recompute +- cascade depth: **O(log m)** — propagates through the tree +- re-renders: **O(m × k)** — each selector triggers k subscribers + +**Fixing downstream selectors is ineffective until the upstream root is stable** — a fixed `getActiveAccount` still receives a new input every render if `getAccounts` is broken. + +``` +getAccountsObject (stable) + └─ getAccounts (broken: returns new array) + ├─ getActiveAccount ├─ getAccountCount └─ getAccountNames … +``` + +Triage the dependency graph top-down; fix roots first. + +## Why Cascade Breaks All Other Optimizations + +| Optimization | Without Cascade Fix | With Cascade Fix | +|---|---|---| +| Virtualization | Parent still re-renders all | Works as intended | +| `React.memo` | Parent defeats it | Works as intended | +| React Compiler | Can't cross file boundaries | Complements selectors | +| `useMemo`/`useCallback` | Recreated on parent render | Stable references | diff --git a/domains/performance/knowledge/selector-antipatterns.md b/domains/performance/knowledge/selector-antipatterns.md new file mode 100644 index 00000000..2d754e89 --- /dev/null +++ b/domains/performance/knowledge/selector-antipatterns.md @@ -0,0 +1,163 @@ +--- +name: selector-antipatterns +domain: performance +description: The Redux selector patterns that break memoization and cause render cascades — the canonical, platform-agnostic taxonomy that per-repo selector references instantiate +--- + +# Selector Anti-Patterns + +**This file is the single source for the pattern taxonomy.** Per-repo references — such as +the `mm-selector-memoization` reference shipped with the `performance` skill — name these +patterns rather than redefining them, and add what only they can: the codebase's own +selector-creator utilities, verified instances with `file:line`, and fix recipes. + +Every pattern below has the same failure shape: `useSelector` returns a **new reference** +when the underlying data did not change, so every consumer re-renders. One broken selector +near the root of the graph cascades through everything downstream, and the cost scales +superlinearly with user data. + +## 1. Unmemoized selector + +A plain function that allocates. No memoization at all — a new reference on every call. + +```typescript +// ❌ BROKEN +export function getPendingApprovals(state) { + return Object.values(state.pendingApprovals ?? {}); +} + +// ✅ FIXED +const getPendingApprovalsObject = (state) => state.pendingApprovals ?? {}; +export const getPendingApprovals = createSelector( + getPendingApprovalsObject, + (approvals) => Object.values(approvals), +); +``` + +Detection: grep exported `function get…` in the selectors directory. + +## 2. Identity / passthrough result + +The transform happens in the **input** and the result function returns its input unchanged, +so the cache can never hit. A plain `createSelector` only helps when its *inputs* are +reference-stable, and an input selector that allocates on every call never is. + +```typescript +// ❌ BROKEN: Object.values() in the INPUT creates a new array each call +export const getAccounts = createSelector( + (state) => Object.values(state.accounts), + (accounts) => accounts, // identity — cache never hits +); + +// ✅ FIXED: stable input, transform in the OUTPUT +export const getAccounts = createSelector( + (state) => state.accounts, // stable structural reference + (accounts) => Object.values(accounts), +); +``` + +Detection: the reselect/Jest warning `"result function returned its own inputs"`. + +## 3. New collection allocated in the result function + +Even a correctly-shaped `createSelector` returns a new reference whenever it recomputes — +and if its inputs are unstable, that is every dispatch. + +```typescript +// ❌ new array/Set/Map/object every call → always "changed" +(accounts) => Object.values(accounts).sort(...) +(transactions) => new Set(transactions.flatMap(...)) +(items) => items.filter(...) +(state) => state.swapsTransactions ?? {} // a fresh {} on every nullish hit +``` + +**Fix:** a deep-equal selector creator (returns the *cached* reference when data is +unchanged), a stable module-level constant for the empty case, or a result-equality check. + +## 4. Mutation in the result function + +```typescript +// ❌ mutates the input array AND returns a corrupting reference +createSelector([getItems], (items) => { items.sort(cmp); return items; }) +``` + +**Fix:** copy first — `[...items].sort(cmp)`. + +## 5. Over-broad input + +`state => state`, or a large slice, as an input selector forces recomputation on **any** +state change anywhere. Narrow the input to the smallest slice that actually feeds the +result. + +## 6. Unnecessary deep equality + +Deep-equal creators cost O(n) per comparison. Reaching for one when the input is already +reference-stable pays that cost for nothing — and deep-comparing a large slice on every +dispatch can be worse than the re-render it prevents. + +```typescript +// ❌ UNNECESSARY: this slice is already reference-stable +const getAccounts = createDeepEqualSelector( + (state) => state.accounts, + (accounts) => transformAccounts(accounts), +); +``` + +Prefer **narrowing the input** over deep-equalizing a giant object. + +## 7. O(n) lookups over unnormalized state + +`.find()` over `Object.values()` is O(n). With n items × m selectors per state change that +is O(n×m) on every dispatch. + +```typescript +// ❌ BROKEN +export const getAccountByAddress = (state, address) => + Object.values(state.accounts).find((a) => a.address === address); + +// ✅ FIXED: normalized state, O(1) access +export const getAccountByAddress = (state, address) => state.accounts[address]; +``` + +## 8. Chained unmemoized transforms + +Each transform allocates. Several in sequence means several new references per call. + +```typescript +// ❌ BROKEN: 3 new arrays per call +export function getSortedItems(state) { + const items = Object.values(state.items); // array 1 + const filtered = items.filter(isVisible); // array 2 + return filtered.sort(byDate); // array 3 +} + +// ✅ FIXED: single memoized output +export const getSortedItems = createSelector( + (state) => state.items, + getFilterCriteria, + (items, criteria) => + Object.values(items).filter((i) => matchesCriteria(i, criteria)).sort(byDate), +); +``` + +## Selector creator decision tree + +``` +Is the INPUT unstable (a fresh object/array every dispatch)? +├── YES → deep-equal selector creator (but prefer narrowing the input first) +└── NO → Is the OUTPUT unstable (a new array/object from the transform)? + ├── YES → result-equality selector creator + └── NO → plain createSelector +``` + +## Don't over-correct + +- A selector returning a **primitive** is fine even if it filters internally — the consumer + memoizes on the primitive value. Wasteful allocation, not a re-render bug. +- Memoization is not free. Prefer narrowing inputs over adding comparison work. + +## Related + +- `render-cascade` — what one broken root selector does to the component graph downstream. +- Per-repo instances: the `mm-selector-memoization` reference documents a codebase's own + selector creators, its verified broken selectors, and the fix recipe for each. diff --git a/domains/performance/skills/effect-antipattern-scan/repos/metamask-extension.md b/domains/performance/skills/effect-antipattern-scan/repos/metamask-extension.md new file mode 100644 index 00000000..ee5fb3b2 --- /dev/null +++ b/domains/performance/skills/effect-antipattern-scan/repos/metamask-extension.md @@ -0,0 +1,27 @@ +--- +repo: metamask-extension +parent: effect-antipattern-scan +--- + +## Paths + +- Component sources: [`ui/`](https://github.com/MetaMask/metamask-extension/tree/main/ui) +- Shared hooks: [`ui/hooks/`](https://github.com/MetaMask/metamask-extension/tree/main/ui/hooks) + +## Commands + +```bash +# Pattern 1: JSON.stringify in deps +grep -rnE 'useEffect\([^)]*\[.*JSON\.stringify' ui/ --include="*.ts" --include="*.tsx" + +# Pattern 3: setInterval / setTimeout +grep -rnE 'setInterval|setTimeout' ui/ --include="*.ts" --include="*.tsx" + +# Pattern 4: fetch inside useEffect (manual review required for context) +grep -rn 'fetch(' ui/ --include="*.ts" --include="*.tsx" +``` + +## Reference Docs + +- [Frontend Performance Optimization Guidelines](https://github.com/MetaMask/contributor-docs/pull/159) (contributor-docs PR #159) +- [You Might Not Need an Effect](https://react.dev/learn/you-might-not-need-an-effect) diff --git a/domains/performance/skills/effect-antipattern-scan/repos/metamask-mobile.md b/domains/performance/skills/effect-antipattern-scan/repos/metamask-mobile.md new file mode 100644 index 00000000..b005c307 --- /dev/null +++ b/domains/performance/skills/effect-antipattern-scan/repos/metamask-mobile.md @@ -0,0 +1,31 @@ +--- +repo: metamask-mobile +parent: effect-antipattern-scan +--- + +## Paths + +- Component sources: [`app/`](https://github.com/MetaMask/metamask-mobile/tree/main/app) +- Shared hooks: [`app/component-library/hooks/`](https://github.com/MetaMask/metamask-mobile/tree/main/app/component-library/hooks) + +## Commands + +```bash +# Pattern 1: JSON.stringify in deps +grep -rnE 'useEffect\([^)]*\[.*JSON\.stringify' app/ --include="*.ts" --include="*.tsx" + +# Pattern 3: setInterval / setTimeout +grep -rnE 'setInterval|setTimeout' app/ --include="*.ts" --include="*.tsx" + +# Pattern 4: fetch inside useEffect (manual review required for context) +grep -rn 'fetch(' app/ --include="*.ts" --include="*.tsx" +``` + +## Differences from Extension + +- Prefer `AbortController` for all new async effects. No shared `useIsMounted` hook exists. +- React Native's `fetch` behaves identically to browser `fetch` for cancellation purposes. + +## Reference Docs + +- [You Might Not Need an Effect](https://react.dev/learn/you-might-not-need-an-effect) diff --git a/domains/performance/skills/effect-antipattern-scan/skill.md b/domains/performance/skills/effect-antipattern-scan/skill.md new file mode 100644 index 00000000..3424d4e9 --- /dev/null +++ b/domains/performance/skills/effect-antipattern-scan/skill.md @@ -0,0 +1,55 @@ +--- +maturity: experimental +name: effect-antipattern-scan +description: Review PR diffs that add or modify `useEffect` for the systemic React effect antipatterns +--- + +# Effect Anti-Pattern Review + +**Scope:** Pre-merge review of PRs that add or modify `useEffect` calls. The workflow is a grep-driven checklist against the patterns catalogued in the **`effect-antipatterns`** knowledge file, which is the single source for their definitions and fixes (installed alongside this skill under `knowledge/`). + +Applies to both `metamask-extension` and `metamask-mobile`. See overlays for repo-specific paths. + +## When To Use + +- Reviewing a PR that adds or modifies a `useEffect` call +- Reviewing a PR that adds `setInterval`, `setTimeout`, `fetch`, or `addEventListener` inside a component +- Investigating a "Can't perform a React state update on an unmounted component" warning + +## Do Not Use When + +- Reviewing selector or render-cascade issues (use [`selector-antipattern-scan`](../selector-antipattern-scan/skill.md)) +- Reviewing non-React code (background scripts, workers, test utilities) +- Reviewing an effect that is intentionally one-shot with no async work or timers (check patterns below anyway, but most do not apply) + +## Workflow + +1. **List changed files with `useEffect`.** `git diff --name-only origin/main...HEAD | xargs grep -l 'useEffect'` +2. **Run the [grep checklist](#grep-checklist)** against the changed files. +3. **For each hit, map to a pattern** in `effect-antipatterns` and apply the fix from the knowledge file. +4. **Block on unstable dependency identity**, unless it is a `JSON.stringify` on a cold path with a small object. Do not merge otherwise. +5. **Block on a timer without cleanup.** Any `setInterval` / `setTimeout` without a matching `clearInterval` / `clearTimeout` in the cleanup function is blocking. +6. **Require cancellation for async effects.** Any `fetch` / network call inside `useEffect` must guard against a stale response, with a cancelled flag or `AbortController`. + +## Grep Checklist + +| Pattern (`effect-antipatterns` §) | Detection | +|---|---| +| §1 Unstable dependency identity | `grep -rnE 'useEffect.*\[.*JSON\.stringify' `, plus inline `{`/`[` literals in the dep position | +| §2 Wrong dependencies | Hand review — empty deps that read state (stale closure), or deps that read nothing | +| §3 Derived state via effect + setState | Hand review — `useEffect` that calls `setX` from other state/props; §3a for chains of them | +| §4 Missing timer cleanup | `grep -rnE 'setInterval\|setTimeout' ` then check each effect returns a cleanup | +| §5 Uncancelled async work | `grep -rnB2 -A10 'fetch\(' ` within `useEffect` blocks | + +See the repo overlay for the concrete `` path. + +## Common Pitfalls + +| Mistake | Correct approach | +|---|---| +| Accept `JSON.stringify` in deps because "the effect needs to rerun when X changes" | Destructure to primitives or `useMemo` the object on a hot path. A cold path with a small object can stringify | +| Accept a state-mirror effect because "the computation is expensive" | Use `useMemo` for expensive derivations. Effects are for side effects, not state derivation | +| Let `setInterval` ship without cleanup because "the component rarely unmounts" | Cleanup is non-negotiable — unmount frequency doesn't matter, correctness does | +| Treat "can't perform state update on unmounted component" as a cosmetic warning | It is a data race. An old response can overwrite a new one | +| Add a lint rule disable on `react-hooks/exhaustive-deps` | Almost always wrong. Destructure or memoize instead | +| Refactor toward `useEffect` + `setState` because it "feels like state" | You probably do not need an effect. See [You Might Not Need an Effect](https://react.dev/learn/you-might-not-need-an-effect) | diff --git a/domains/performance/skills/performance/references/mm-audit-playbook.md b/domains/performance/skills/performance/references/mm-audit-playbook.md index b3f302c7..f259d7c2 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). @@ -42,8 +43,10 @@ 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, cache thrashing on unstable args → 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. +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 +60,12 @@ 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). ### Animations → [mm-layout-animations.md](mm-layout-animations.md) ```bash @@ -97,6 +101,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). @@ -107,6 +112,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 (cache thrashing on unstable args) 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-hook-dependency-arrays.md b/domains/performance/skills/performance/references/mm-hook-dependency-arrays.md index 5733ff81..117cc1dd 100644 --- a/domains/performance/skills/performance/references/mm-hook-dependency-arrays.md +++ b/domains/performance/skills/performance/references/mm-hook-dependency-arrays.md @@ -8,6 +8,8 @@ tags: useEffect, useMemo, useCallback, dependencies, JSON.stringify Dependency arrays decide when `useEffect`/`useMemo`/`useCallback` re-run. The most common MetaMask problem is **`JSON.stringify` inside a dependency array** — it runs a synchronous serialization on every render just to compute the dependency key, which is both expensive and a sign the upstream reference is unstable. +> **Scope.** This file is the *dependency* half of effect performance, instantiated for this codebase. The platform-agnostic taxonomy — unstable dependency identity, wrong dependencies, derived state via effect, cascading effect chains, missing cleanup, uncancelled async — lives in the **`effect-antipatterns`** knowledge file, installed alongside this skill under `knowledge/`. Read that for the general shape; read this for the verified instances and the repo's lint gaps. + ## Pattern — `JSON.stringify` in deps ```ts @@ -83,5 +85,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-planning.md b/domains/performance/skills/performance/references/mm-planning.md index ca23c644..3a610685 100644 --- a/domains/performance/skills/performance/references/mm-planning.md +++ b/domains/performance/skills/performance/references/mm-planning.md @@ -20,9 +20,9 @@ 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) | +| 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-redux-antipatterns.md b/domains/performance/skills/performance/references/mm-redux-antipatterns.md index a830bc35..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 @@ -86,5 +88,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..df798790 --- /dev/null +++ b/domains/performance/skills/performance/references/mm-selector-cascade.md @@ -0,0 +1,121 @@ +--- +title: Selector Dependency Cascades — Blast Radius & Repair (MetaMask) +impact: CRITICAL +tags: reselect, cascade, dependency-graph, isEqual, structural-sharing, react-compiler +--- + +# Skill: Selector Dependency Cascades + +> **Scope.** What a broken root selector does to the component graph is defined generically +> in the **`render-cascade`** knowledge file, and the selector patterns that cause it in +> **`selector-antipatterns`** — both installed alongside this skill under `knowledge/`. +> This file is the MetaMask Mobile instance: the real dependency graph, its blast radius, +> and the repair order. + +[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)). + +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: + +1. **Direct consumers:** every selector that lists the suspect as an input. `grep -rn "getInternalAccounts" app/selectors --include="*.ts"` +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. + +**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: + +- 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)). + +**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, 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. + +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. + +```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 (from the extension performance audit): + +```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. +5. Lock the win in CI: add a Reassure `*.perf-test.tsx` on a top consumer so the cascade can't silently return. + +## 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 diff --git a/domains/performance/skills/performance/references/mm-selector-memoization.md b/domains/performance/skills/performance/references/mm-selector-memoization.md index b9e117db..672a05df 100644 --- a/domains/performance/skills/performance/references/mm-selector-memoization.md +++ b/domains/performance/skills/performance/references/mm-selector-memoization.md @@ -13,38 +13,22 @@ Broken or absent memoization in widely-used selectors is the single highest-impa - `createSelector` from `reselect` — reference-equality on inputs. - **`createDeepEqualSelector`** from `app/selectors/util.ts` — `createSelectorCreator(lruMemoize, deepEqual)`. Recomputes only when inputs are **deeply** equal-or-not. Use this when an input selector returns a fresh object/array on every dispatch (very common with controller state slices). -## The four deadly patterns +## The patterns, and what they look like here -### 1. Identity / passthrough in a plain `createSelector` -```ts -// ❌ Does nothing — output is the input, but the input ref changes every dispatch -export const selectX = createSelector(selectControllerState, (s) => s.things); -``` -A plain `createSelector` only helps if its **inputs** are reference-stable. Controller-state slices usually are not. Result: recomputes + new ref every dispatch. - -**Fix:** use `createDeepEqualSelector`, or narrow the input to the smallest stable slice. +The pattern taxonomy itself lives in the **`selector-antipatterns`** knowledge file, +installed alongside this skill under `knowledge/`. It is the single source — read it for the +full definition, the worked before/after of each, and the selector-creator decision tree. +This section maps each pattern onto *this* codebase. -### 2. New collection in the result function -```ts -// ❌ new array/Set/Map/object every call → always "changed" -(accounts) => Object.values(accounts).sort(...) -(transactions) => new Set(transactions.flatMap(...)) -(items) => items.filter(...) -(state) => state.swapsTransactions ?? {} // new {} when nullish -``` -Even a correct `createSelector` produces a new reference whenever it recomputes; if the inputs aren't stable, that's every dispatch. - -**Fix:** `createDeepEqualSelector` (deep-compares so it returns the *cached* ref when data is unchanged), or a stable module-level constant for the empty case, or a `resultEqualityCheck`. - -### 3. Mutation in the result function -```ts -// ❌ mutates the input array AND returns a new-but-corrupting ref -createSelector([getItems], (items) => { items.sort(cmp); return items; }) -``` -**Fix:** copy first — `[...items].sort(cmp)`. +| Pattern | How it shows up in Mobile | Fix here | +|---|---|---| +| **Identity / passthrough result** | `createSelector(selectControllerState, (s) => s.things)`, where `selectControllerState` returns the whole controller: that reference changes whenever any field in the controller updates, even when `.things` did not, so the selector recomputes and returns a new ref on every update to that controller | `createDeepEqualSelector`, or narrow the input to the smallest stable slice | +| **New collection in the result function** | `Object.values(...).sort(...)`, `new Set(...flatMap(...))`, `items.filter(...)`, `state.swapsTransactions ?? {}` | `createDeepEqualSelector`, a stable module-level constant for the empty case, or a `resultEqualityCheck` | +| **Mutation in the result function** | `createSelector([getItems], (items) => { items.sort(cmp); return items; })` | copy first — `[...items].sort(cmp)` | +| **Over-broad input** | `state => state`, or a whole controller slice, as an input selector | narrow the input | +| **Unnecessary deep equality** | reaching for `createDeepEqualSelector` on an already-stable slice | plain `createSelector`; see *Don't over-correct* below | -### 4. `state => state` (or a huge slice) as an input selector -Forces recomputation on **any** state change anywhere. Narrow the input. +The two that dominate the verified instances below are the first two. ## Verified MetaMask instances @@ -113,4 +97,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..4221ef4f --- /dev/null +++ b/domains/performance/skills/performance/references/mm-state-normalization.md @@ -0,0 +1,136 @@ +--- +title: State Normalization & Selector Shape (MetaMask) +impact: HIGH +tags: redux, normalization, selectors, O(1)-lookups, cache-thrashing, useSelector +--- + +# Skill: State Normalization & Selector Shape + +> **Scope.** The generic form of the O(n)-lookup problem is `selector-antipatterns` §7, +> in the knowledge file installed alongside this skill under `knowledge/`. This file is the +> MetaMask Mobile instance, plus the parameterized-selector cache-thrashing and +> view-selector consolidation work that is specific to this store's 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, 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`'s default memoizer, `weakMapMemoize`, caches per argument identity instead of in one slot, but only for *stable* arguments. A fresh **object literal** argument per call busts it every time: + +```ts +// ❌ a new object literal every call, no cache hit ever +const a1 = useSelector((s) => selectAsset(s, { address: addr1 })); // miss +const a2 = useSelector((s) => selectAsset(s, { address: addr2 })); // miss, unrelated to addr1's entry +const a3 = useSelector((s) => selectAsset(s, { address: addr3 })); // miss, and so on every render cycle +``` + +In a list rendering N rows, the "memoized" selector recomputes N times per render, forever. **Check the memoizer before flagging:** this codebase's parameterized selectors use `weakMapMemoize` by default (e.g. `selectNetworkConfigurationByChainId`), which caches per-argument and doesn't thrash for *stable* arguments. The object-literal case above is the exception. 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 (`makeSelectAsset()`) 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 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. + +## 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 — 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 +``` + +## 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. +- 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 + +- [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-tools.md b/domains/performance/skills/performance/references/mm-tools.md index 9939808b..c22fb4e3 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 @@ -166,6 +169,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 one-span-per-transaction ratio is still fan-out if the transaction fires at high volume, and its failure spans should stay at 100% rather than being flat-sampled to zero. A high-frequency span needs a deterministic sub-sample gate (and a kill-switch, which only reaches the population already running the build that contains it and is not an immediate brake on a partial rollout) 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] }); 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..ebcb4345 --- /dev/null +++ b/domains/performance/skills/performance/references/mm-useeffect-antipatterns.md @@ -0,0 +1,149 @@ +--- +title: useEffect Lifecycle Anti-Patterns (MetaMask) +impact: HIGH +tags: useEffect, setState, cleanup, AbortController, unmount, memory-leaks +--- + +# Skill: useEffect Lifecycle Anti-Patterns + +> **Scope.** The platform-agnostic taxonomy — unstable dependency identity, wrong +> dependencies, derived state via effect, cascading effect chains, missing timer cleanup, +> uncancelled async — is the single source in the **`effect-antipatterns`** knowledge file, +> installed alongside this skill under `knowledge/`. This file is the MetaMask Mobile +> instance of its lifecycle half: the verified instances, the repo's own idioms, and the +> fix recipes. + +[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** (from the extension performance audit): 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 f3d2d98a..1de3fc1e 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) | @@ -88,6 +91,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 doing an O(n) `Object.values().flat().find()` scan per call | `selectSingleTokenByAddressAndChainId` `app/selectors/tokensController.ts:183`; 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) | @@ -103,4 +108,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`) 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 (extension PRs metamask-extension#38007, metamask-extension#37147). diff --git a/domains/performance/skills/selector-antipattern-scan/repos/metamask-extension.md b/domains/performance/skills/selector-antipattern-scan/repos/metamask-extension.md new file mode 100644 index 00000000..82801a08 --- /dev/null +++ b/domains/performance/skills/selector-antipattern-scan/repos/metamask-extension.md @@ -0,0 +1,43 @@ +--- +repo: metamask-extension +parent: selector-antipattern-scan +--- + +## Paths + +- Selector definitions: [`ui/selectors/`](https://github.com/MetaMask/metamask-extension/tree/main/ui/selectors) +- Selector creators: [`shared/lib/selectors/selector-creators.ts`](https://github.com/MetaMask/metamask-extension/blob/main/shared/lib/selectors/selector-creators.ts) — source of truth for `createSelector`, `createDeepEqualSelector`, `createResultEqualSelector`, `createShallowResultSelector` +- Controller state shape: [`app/scripts/metamask-controller.js`](https://github.com/MetaMask/metamask-extension/blob/main/app/scripts/metamask-controller.js) +- Component consumption sites: anywhere under [`ui/`](https://github.com/MetaMask/metamask-extension/tree/main/ui) that calls `useSelector` + +## Commands + +```bash +# Enable WDYR for post-merge diagnosis +ENABLE_WHY_DID_YOU_RENDER=true yarn start + +# Pre-merge grep checklist +grep -rE 'export function get' ui/selectors/ --include="*.ts" +grep -rn createDeepEqualSelector ui/ --include="*.ts" +grep -rnE 'useSelector\([^,]+,\s*(isEqual|shallowEqual)' ui/ --include="*.ts" --include="*.tsx" +grep -rnE '\.find\(' ui/selectors/ +``` + +## Selector Creators + +`shared/lib/selectors/selector-creators.ts` + +| Creator | Use Case | +|---------|----------| +| `createSelector` | Standard memoization (default) | +| `createDeepEqualSelector` | Genuinely unstable inputs (rare — see [narrow exception](../skill.md#overuse-of-createdeepequalselector)) | +| `createResultEqualSelector` | Unstable outputs requiring deep comparison | +| `createShallowResultSelector` | Unstable outputs, shallow comparison sufficient | + +## Example Fix Methodology + +[PR #37147](https://github.com/MetaMask/metamask-extension/pull/37147) fixed `getInternalAccounts` as the canonical example. Before: `createSelector(selectInternalAccounts, (accounts) => accounts)` (identity function, defeats memoization). After: `createSelector(getInternalAccountsObject, (accounts) => Object.values(accounts))`. Impact: 50+ component re-renders eliminated per state update. + +## Reference + +- [Frontend Performance Optimization Guidelines](https://github.com/MetaMask/contributor-docs/pull/159) (contributor-docs PR #159) diff --git a/domains/performance/skills/selector-antipattern-scan/repos/metamask-mobile.md b/domains/performance/skills/selector-antipattern-scan/repos/metamask-mobile.md new file mode 100644 index 00000000..94e05a67 --- /dev/null +++ b/domains/performance/skills/selector-antipattern-scan/repos/metamask-mobile.md @@ -0,0 +1,35 @@ +--- +repo: metamask-mobile +parent: selector-antipattern-scan +--- + +## Paths + +- Selector definitions: [`app/selectors/`](https://github.com/MetaMask/metamask-mobile/tree/main/app/selectors) +- Redux store: [`app/store/`](https://github.com/MetaMask/metamask-mobile/tree/main/app/store) +- Engine / state shape: [`app/core/Engine/Engine.ts`](https://github.com/MetaMask/metamask-mobile/blob/main/app/core/Engine/Engine.ts) +- WDYR setup: [`wdyr.js`](https://github.com/MetaMask/metamask-mobile/blob/main/wdyr.js) at repo root +- Component consumption sites: anywhere under [`app/`](https://github.com/MetaMask/metamask-mobile/tree/main/app) that calls `useSelector` + +## Commands + +```bash +# Enable WDYR (env var gate, same as extension) +ENABLE_WHY_DID_YOU_RENDER=true yarn start + +# Pre-merge grep checklist +grep -rE 'export function get' app/selectors/ --include="*.ts" +grep -rn createDeepEqualSelector app/ --include="*.ts" +grep -rnE 'useSelector\([^,]+,\s*(isEqual|shallowEqual)' app/ --include="*.ts" --include="*.tsx" +grep -rnE '\.find\(' app/selectors/ +``` + +## WDYR + +Mobile has `wdyr.js` at the repo root. It is gated on `__DEV__ && process.env.ENABLE_WHY_DID_YOU_RENDER === 'true'` and imported from the entry file. No manual setup required — flip the env var and restart Metro. + +Current configuration (at time of authoring): `trackAllPureComponents: true`, `onlyLogs: true` (Metro/Hermes console doesn't group well). + +## Differences from Extension + +- React Compiler adoption and `"use no memo"` opt-outs are not extension-only: mobile has the compiler wired in `babel.config.js` too, via `scripts/react-compiler.js`. diff --git a/domains/performance/skills/selector-antipattern-scan/skill.md b/domains/performance/skills/selector-antipattern-scan/skill.md new file mode 100644 index 00000000..646664d3 --- /dev/null +++ b/domains/performance/skills/selector-antipattern-scan/skill.md @@ -0,0 +1,125 @@ +--- +maturity: experimental +name: selector-antipattern-scan +description: Review and diagnose Redux selector antipatterns that cause render cascades, pre-merge and post-merge +--- + +# Selector Anti-Pattern Review + +**Scope:** Redux selector antipatterns are the dominant cause of React render cascades in the MetaMask UI. This skill covers both review phases: pre-merge PR review (grep-driven checklist) and post-merge diagnosis (WDYR-driven workflow). Both modes resolve to the same root cause and the same fix set, catalogued in the **`selector-antipatterns`** and **`render-cascade`** knowledge files — the single source for their definitions (installed alongside this skill under `knowledge/`). + +Both `metamask-extension` and `metamask-mobile` share the same React + Redux architecture; this skill applies to both (see overlays for repo-specific paths). + +## When To Use + +- **Pre-merge.** Reviewing a PR that touches a `selectors/` directory, adds a `useSelector` call, or modifies a `createSelector` / `createDeepEqualSelector` definition +- **Post-merge.** Re-renders are disproportionate to state change size, performance degrades non-linearly with user data size, or components re-render during idle +- **Triage.** A WDYR counter jumps 5+ times per action, or a React render counter shows unexpected re-renders + +## Do Not Use When + +- Non-selector performance concerns (effects → use `effect-antipattern-scan`, context providers, virtualization) +- Network-bound slowness (use the Network panel, not WDYR) +- Startup or initial-mount perf (use startup profiling) +- Non-React trees (worker messaging, background script perf) + +## Mode A: Pre-Merge Review (grep-driven) + +1. **List changed selector/consumer files.** `git diff --name-only origin/main...HEAD | grep -E '(selectors|useSelector)'` +2. **Run the [grep checklist](#grep-checklist)** against the changed files. +3. **Match each hit to a pattern** in `selector-antipatterns` or to one of the [team-specific workarounds](#team-specific-workarounds) below. +4. **Block on Jest warning.** If the PR's test run surfaces `"result function returned its own inputs"`, the PR introduces an identity/passthrough result (`selector-antipatterns` §2). Do not merge. +5. **Require a fix, not a justification.** None of the eight patterns have a valid use case. See [Pitfalls](#common-pitfalls) for the narrow `createDeepEqualSelector` exception. + +## Mode B: Post-Merge Diagnosis (WDYR-driven) + +1. **Confirm cascade.** Add a render counter to a high-level component. If count jumps 5+ per action, cascade is confirmed. + ```tsx + const [count, increment] = useReducer((n) => n + 1, 0) + useEffect(() => { increment() }) + console.log('Render:', count) + ``` +2. **Enable WDYR.** `ENABLE_WHY_DID_YOU_RENDER=true yarn start` (same env var on extension and mobile). +3. **Identify root component.** The first WDYR log is the cascade origin. Do not fix downstream symptoms first. +4. **Classify via the [WDYR message table](#wdyr-message-interpretation).** If the root cause is a selector, return to [Mode A](#mode-a-pre-merge-review-grep-driven) and apply the fix set. If it is a context value or prop identity issue, see the `render-cascade` knowledge file. +5. **Verify.** Repeat the action. Confirm the counter stabilizes (e.g. 0→2, not 0→25). React Strict Mode's amplification compounds non-linearly through a cascade, so compare before/after under the same Strict Mode setting rather than dividing by a fixed factor. + +## Grep Checklist + +| Pattern (`selector-antipatterns` §) | Detection | +|---|---| +| §1 Unmemoized selector | `grep -rE 'export function get' /` | +| §2 Identity / passthrough result | Jest warning `result function returned its own inputs` | +| §3 New collection in the result function | `grep -rnE 'new Set\|new Map\|Object\.(values\|keys\|entries)\|\?\? \{\}\|\?\? \[\]\|=> \(\{\|=> \[' /` | +| §4 Mutation in the result function | `grep -rnE '\.sort\(\|\.reverse\(\|\.push\(\|\.splice\(' /` | +| §5 Over-broad input | `grep -rn 'state) => state\b' /` | +| §6 Unnecessary deep equality | `grep -rn 'createDeepEqualSelector' /` then verify each input is genuinely unstable | +| §7 O(n) lookup | `grep -rnE '\.find\(.*=>.*address' /` | +| §8 Chained unmemoized transforms | `grep -rnE 'export function get.*\{' / -A5`, then look for several `.filter/.map/.sort` without memoization | + +The `=> ({` and `=> [` alternates catch a result function that *returns* a fresh literal rather than constructing a named collection. A trial run missed a real instance without them: `(metamask) => ({ userRegion: ..., ... })` builds a new object every recompute and matches none of the collection constructors. + +See the repo overlay for the concrete `` path. + +## Team-Specific Workarounds + +Two patterns show up beyond those in the knowledge file. Both are workarounds for broken selectors downstream. The fix is always to fix the selector, never to propagate the workaround. + +### `useSelector(selector, isEqual)` from `react-redux` + +```typescript +// Workaround that hides the real problem +const accounts = useSelector(getAccounts, isEqual) +``` + +- **Detection:** `grep -rnE 'useSelector\([^,]+,\s*(isEqual|shallowEqual)'` +- **Review action:** Find `getAccounts` (or whichever selector). Fix it to return a stable reference. Remove the `isEqual` argument in the same PR. +- **Why it's wrong:** Deep equality at the consumption site adds O(n) per render and leaves every other consumer of the same selector broken. + +### Overuse of `createDeepEqualSelector` + +```typescript +// Unnecessary when input is from Immer-managed Redux state +const getTokens = createDeepEqualSelector( + (state) => state.metamask.tokens, + (tokens) => transformTokens(tokens), +) +``` + +- **Detection:** `grep -rn createDeepEqualSelector /` +- **Review action:** For each instance, check if the inputs come from Redux state. If yes, swap to `createSelector`. Immer already gives stable references. +- **The narrow exception:** Inputs that are genuinely not from Immer/Redux state (e.g. derived from a non-Redux source, or passed in as props). These stay. + +## WDYR Message Interpretation + +For post-merge diagnosis, map the WDYR log message to the root cause: + +| Message | Root Cause | Fix | +|---------|------------|-----| +| `different objects that are equal by value` | Object recreated | `useMemo` (or fix selector that produced it) | +| `different functions with the same name` | Callback recreated | `useCallback` with stable deps | +| `different React elements` | JSX passed as prop | Extract to constant | +| `props object itself changed but values equal` | Parent cascade | Fix parent, not child | +| `[hook useContext result]` | Context value unstable | `useMemo` provider value | + +## Diagnostic Signals + +| Red | Green | +|-----|-------| +| Same component 5+ times in WDYR | Re-render count ≤ expected per action | +| Counter jumps 5+ per action | No WDYR logs during idle | +| Render count scales with data size | Render count stable regardless of data | +| Re-renders during idle | — | + +## Common Pitfalls + +| Mistake | Correct approach | +|---|---| +| Accept `useSelector(sel, isEqual)` because "it works" | The underlying selector is broken; fix it and remove the workaround | +| Approve `createDeepEqualSelector` without checking input source | Trace every input to verify it's not already Immer-stable | +| Treat the eight patterns as preferences | They are measurably broken — each generates CI warnings | +| Ask the author to justify rather than fix | None of the patterns have a valid use case except the narrow exception above | +| Review only the selector definition, not consumption sites | Pattern 1 (plain function) hides at the call site | +| Fix downstream components first during post-merge diagnosis | Fix the root-cause selector; downstream fixes become wasted work | +| Add `React.memo` to symptom component | Requires stable parent. Fix the parent (usually a selector) first | +| Divide WDYR counts by 1 | React Strict Mode double-renders, but not by a clean factor through a cascade. Compare before/after under the same setting |