From adf0da86a18e3b25cdb632b5c60e98c6fd8efc3d Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Tue, 1 Sep 2026 10:27:52 -0400 Subject: [PATCH 1/8] Add React Compiler error triage to the mobile `performance` skill MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `mm-react-compiler-error-triage` — sorting compiler errors into `Todo` and unsupported versus actionable, ratcheting `panicThreshold`, and the `useMemo`/`useCallback` exception for effect dependencies, whose output the compiler's memoization does not preserve. Split out of #43: compiler adoption is a different subject from the render antipattern scans, and nothing in either scan cites it. --- .../mm-react-compiler-error-triage.md | 90 +++++++++++++++++++ .../references/mm-react-compiler.md | 3 + 2 files changed, 93 insertions(+) create mode 100644 domains/performance/skills/performance/references/mm-react-compiler-error-triage.md 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..68db17f8 --- /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 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 +// (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..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. @@ -68,4 +70,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 From 913131612de87b3f466a78596b7f29dfb49bbea6 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Mon, 14 Sep 2026 08:00:02 -0400 Subject: [PATCH 2/8] Count a module-scope `'use no memo'` file as `compiled`, not `skipped` `babel-plugin-react-compiler` 1.0.0 logs `CompileSuccess` for every function in a file whose top-level directive then discards the transform. Only a directive inside one function's body logs `CompileSkip`. The compiled count therefore rises with opt-outs and is read against the opt-out count. --- .../references/mm-react-compiler-error-triage.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) 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 68db17f8..8dee7c5b 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 @@ -56,10 +56,10 @@ Track four buckets — **compiled / skipped / errors / unsupported** — at file What the buckets tell you: -- **compiled** — your real optimization coverage. "The compiler is enabled" claims nothing; this number does. +- **compiled** — your real optimization coverage, minus one gap: a module-scope `'use no memo'` directive still logs `CompileSuccess` for every function in the file before the directive discards the transform, so those files land here too, not in `skipped`. "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). +- **skipped** — intentional exclusions: test/story files 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). A `'use no memo'` directive lands here only when it sits inside one function's body, logging `CompileSkip`. A module-scope one, at the top of the file, logs as `compiled` instead. ## Staged adoption roadmap @@ -73,14 +73,14 @@ The extension's sequence (epic MetaMask-planning#6549) generalizes to any repo: ## 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. +- Per repo: the compiled-files count from the logger stats rises (or at least doesn't silently fall) release over release, read against the `'use no memo'` opt-out count from the roadmap's audit step. A module-scope directive still logs as `compiled`, so the raw count alone can rise while real coverage doesn't. 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. +- Don't treat `skipped` as a problem — tests, stories, and function-body opt-outs belong there (a module-scope `'use no memo'` counts as `compiled`, not `skipped`). 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 From 40cde826fa88993541426d2b5b86c37d308bb59f Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Mon, 14 Sep 2026 08:07:03 -0400 Subject: [PATCH 3/8] Describe mobile's React Compiler as app-wide, as `babel.config.js` has it metamask-mobile#31171 (enable the React Compiler across the Metro bundle) replaced the two-path allowlist. The plugin now applies to every file except under Jest, with no `target` override, so its default of React 19 applies. The triage reference also named two internal planning tickets. --- .../mm-react-compiler-error-triage.md | 4 +- .../references/mm-react-compiler.md | 40 +++++++++---------- 2 files changed, 21 insertions(+), 23 deletions(-) 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 8dee7c5b..9a6587e8 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 @@ -18,7 +18,7 @@ The React Compiler **fails open**: when it can't compile a component, it silentl | `'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. +**The ratchet strategy** (extension roadmap): 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 @@ -63,7 +63,7 @@ What the buckets tell you: ## Staged adoption roadmap -The extension's sequence (epic MetaMask-planning#6549) generalizes to any repo: +The extension's sequence 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*"`. diff --git a/domains/performance/skills/performance/references/mm-react-compiler.md b/domains/performance/skills/performance/references/mm-react-compiler.md index 4f987cfb..53f94134 100644 --- a/domains/performance/skills/performance/references/mm-react-compiler.md +++ b/domains/performance/skills/performance/references/mm-react-compiler.md @@ -1,29 +1,29 @@ --- title: React Compiler (MetaMask) impact: HIGH -tags: react-compiler, memoization, babel, incremental-adoption +tags: react-compiler, memoization, babel, app-wide --- # Skill: React Compiler in MetaMask -React Compiler auto-memoizes components, callbacks, and computed values at build time — removing most of the need for manual `React.memo`/`useMemo`/`useCallback`. MetaMask adopts it **incrementally**, path by path, via `babel.config.js`. +React Compiler auto-memoizes components, callbacks, and computed values at build time — removing most of the need for manual `React.memo`/`useMemo`/`useCallback`. It runs **app-wide** in this repo (metamask-mobile#31171 enabled v1.0.0 app-wide), wired through `babel.config.js`. ## Current state (verified) - `babel-plugin-react-compiler`, `react-compiler-runtime`, and `eslint-plugin-react-compiler` are installed. - ESLint: `react-compiler/react-compiler: 'warn'` is enabled. -- `babel.config.js`: `target: '18'`, plugin runs **first**, and only these paths are opted in: +- `babel.config.js`: the `react-compiler` plugin runs **first** and applies to every file, with no path allowlist and no `target` override, so the plugin's own default, `target: '19'`, applies. It's disabled under Jest, to avoid a `jest.mock` hoisting conflict with the compiler's injected `_c` helper: ```js - // babel.config.js → plugins → ['react-compiler', { target: '18', sources }] - const pathsToInclude = [ - 'app/components/Nav', - 'app/components/UI/DeepLinkModal', - ]; - return pathsToInclude.some((path) => filename.includes(path)); + // scripts/react-compiler.js + const isTestEnv = process.env.NODE_ENV === 'test'; + const reactCompilerBabelConfig = isTestEnv ? [] : [reactCompilerPlugin]; ``` -- `react-native-reanimated/plugin` must remain **last** in the plugin list (required for `'worklet'`). + Set `REACT_COMPILER_LOG_FAILURES=true` to attach a logger that appends every `CompileError`/`CompileSkip` event to a git-ignored `react-compiler.log`. Without it, the compiler keeps its quiet default (no logger, no bailout output). +- `react-native-worklets/plugin` must remain **last** in the plugin list. It compiles `'worklet'` directives. `react-native-reanimated/plugin` is reanimated v4's deprecated alias for it. -## Opt a new feature in +## Fix a component the compiler skips + +The compiler already runs app-wide (see above). There's no allowlist to edit. When a component isn't picking up the compiler's memoization: 1. **Check for Rules-of-React violations first.** The compiler silently skips components that break the rules (safe, but you lose the optimization). @@ -32,14 +32,12 @@ React Compiler auto-memoizes components, callbacks, and computed values at build yarn eslint # react-compiler/react-compiler warnings = what the compiler would skip ``` (The standalone `react-compiler-healthcheck` CLI gives a repo-wide count, but - it isn't installed; the ESLint plugin runs the same Rules-of-React checks on - the paths you're opting in.) -2. **Add the path** to `pathsToInclude` in `babel.config.js` (a directory prefix or a specific file path; `filename.includes` matches substrings). -3. **Clear Metro's cache** — it caches compiled output aggressively: + it isn't installed; the ESLint plugin runs the same Rules-of-React checks.) +2. **Clear Metro's cache** — it caches compiled output aggressively: ```bash yarn watch:clean ``` -4. **Verify:** in React DevTools, optimized components show a **`Memo ✨`** badge. You can also confirm `'use no memo'` isn't silently opting a component out. +3. **Verify:** in React DevTools, optimized components show a **`Memo ✨`** badge. You can also confirm `'use no memo'` isn't silently opting a component out. ## What it does / doesn't do @@ -49,7 +47,7 @@ React Compiler auto-memoizes components, callbacks, and computed values at build ## Interaction with manual memoization -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. +You can gradually drop hand-written `useMemo`/`useCallback`/`React.memo` once the compiler is verified working on a path, but do it deliberately and re-measure. Under Jest, and on any file carrying `'use no memo'`, the compiler doesn't run, so manual memoization still matters there. **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)). @@ -59,13 +57,13 @@ On opted-in paths you can gradually drop hand-written `useMemo`/`useCallback`/`R - Side effects during render (e.g. incrementing a module variable). - Other Rules-of-React violations flagged by the ESLint plugin / healthcheck. -Fix the ESLint `react-compiler` warnings on a path before/after opting it in. +Fix the ESLint `react-compiler` warnings on a path to get it compiling. ## Don't -- Don't hardcode the current path list as if it's permanent — it grows over time; read `babel.config.js`. -- Don't use `target: '19'` blindly — the repo targets `'18'`; match it. -- Don't reorder the reanimated plugin away from last. +- Don't assume there's a path list to edit. The compiler applies app-wide, and the only files it skips are under Jest or carrying `'use no memo'`. +- Don't assume the compiler targets React 18. `scripts/react-compiler.js` sets no `target` option, so the plugin's default, `target: '19'`, applies. +- Don't reorder `react-native-worklets/plugin` away from last in the plugin list. ## Related From c92856cf28842d1a45a2af8ebacac0a40f5ac6f9 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Mon, 14 Sep 2026 08:48:08 -0400 Subject: [PATCH 4/8] Test whether a `'use no memo'` directive is load-bearing before keeping it Removing the directive and re-running the compiler settles it: zero new errors means the directive was masking nothing. --- .../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 9a6587e8..2b0d2c24 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 @@ -66,7 +66,7 @@ What the buckets tell you: The extension's sequence 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*"`. +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*"`. A directive can be masking nothing: remove it and re-run the compiler, and zero new errors means it was not load-bearing. 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. From 782936eb4e724a906994f50f96dc403b5a334db1 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Tue, 15 Sep 2026 07:50:24 -0400 Subject: [PATCH 5/8] Add restart-key and effect-protocol rules to `perf-hooks-effects`, and drop a stale-closure example --- .../repos/metamask-extension.md | 122 ++++++++++++++++-- 1 file changed, 110 insertions(+), 12 deletions(-) diff --git a/domains/performance/skills/perf-hooks-effects/repos/metamask-extension.md b/domains/performance/skills/perf-hooks-effects/repos/metamask-extension.md index 349d9527..e97fc908 100644 --- a/domains/performance/skills/perf-hooks-effects/repos/metamask-extension.md +++ b/domains/performance/skills/perf-hooks-effects/repos/metamask-extension.md @@ -344,7 +344,7 @@ const useHistoricalPrices = () => { **DO:** - Split effects when conditional logic excludes some dependencies -- Ensure all dependencies in array are actually used +- Ensure every dependency is either read by the effect or is a named restart key (see Rule: Name Restart Keys) **DON'T:** @@ -374,19 +374,47 @@ const useHistoricalPrices = ({ isEvm, chainId, address }: Props) => { fetchPrices(chainId, address); } }, [isEvm, chainId, address]); // All deps are used +}; +``` - // OR Option 2: Separate effects - useEffect(() => { - if (isEvm) return; - fetchPrices(chainId, address); - }, [isEvm]); // Only depends on condition +### Rule: Name Restart Keys - useEffect(() => { - if (!isEvm) { - fetchPrices(chainId, address); - } - }, [chainId, address]); // Only when not EVM -}; +**DO:** + +- When an effect synchronizes with an external system (a connection, a subscription, a background registration) and a dependency's only job is to tear that down and start it again when it changes, keep it in the array and say so in a comment beside it +- Prefer passing the value into the call the effect makes, so the effect reads it and the dependency explains itself + +**DON'T:** + +- Leave a dependency the effect body never reads without a comment. A reviewer cannot tell a restart key from a dead dependency, and the next edit deletes it +- Delete a restart key to satisfy "include only what the effect reads". The effect then keeps a session bound to the old value + +**Example - WRONG:** + +```typescript +useEffect(() => { + const session = startSession(address); + return () => session.stop(); +}, [address, networkId]); // networkId is never read: restart key, or leftover? +``` + +**Example - CORRECT:** + +```typescript +useEffect(() => { + const session = startSession(address); + return () => session.stop(); +}, [ + address, + // Not read above: a network change must end this session and start a new one. + networkId, +]); + +// Or make the dependency real by passing it in: +useEffect(() => { + const session = startSession({ address, networkId }); + return () => session.stop(); +}, [address, networkId]); ``` ### Rule: Use useRef for Persistent Values @@ -856,6 +884,76 @@ const PriceTicker = ({ tokenAddress }: PriceTickerProps) => { }; ``` +### Rule: Keep Multi-Step Async Protocols Out of the Effect + +**DO:** + +- When an effect acquires an external resource through more than one async step, with a timeout or more than one way to end, move that protocol into a plain object owned outside React: `start()` returns a handle, and `handle.stop()` releases the resource +- Keep the effect to acquiring and releasing: start in the body, stop in the cleanup +- Unit-test the object's end paths directly (ready, timeout, failure, stop while a step is in flight), without rendering + +**DON'T:** + +- Grow a state machine of closure flags (`cancelled`, `ended`, `ready`) inside the effect. Every end path needs its own cleanup, every fix has to reason about every flag at once, and the only way to test it is through `renderHook` + +**Why:** every line of such an effect can pass the rules above (stable dependencies, timers cleared, a cancelled flag on the async chain) while the bugs sit between its end paths: a timeout that fires mid-acquire, a release sent before registration, a release that also runs on the success path. Those bugs belong to the protocol, not to any one line, so a line-level review does not catch them. + +**Example - WRONG:** + +```typescript +useEffect(() => { + let cancelled = false; + let ended = false; + let ready = false; + const finish = (reason: string) => { + if (!ended) { + ended = true; + report(reason); + } + }; + const timeout = setTimeout(() => { + cancelled = true; + finish('timeout'); + release(id); + }, 30_000); + register(id) + .then(() => !cancelled && init(address)) + .then(() => !cancelled && start(id)) + .then(() => { + if (!cancelled) { + ready = true; + finish('ready'); + } + }) + .catch(() => { + finish('failed'); + if (!cancelled) release(id); + }) + .finally(() => clearTimeout(timeout)); + return () => { + cancelled = true; + clearTimeout(timeout); + finish('released'); + release(id, ready); + }; +}, [address]); +``` + +**Example - CORRECT:** + +```typescript +// preload-session.ts: a plain object, unit-tested without React +export function startPreloadSession(address: string): { stop: () => void } { + // register, init, start, the timeout, and every end path live here +} + +// The hook only acquires and releases +useEffect(() => { + const session = startPreloadSession(address); + return () => session.stop(); +}, [address]); +``` + ### Rule: Avoid Large Object Retention in Closures **DO:** From a5585ab26248062f60ac42df7ea374275363a383 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Wed, 16 Sep 2026 10:38:05 -0400 Subject: [PATCH 6/8] Require `useEffect` to read every dependency instead of commenting restart keys --- .../repos/metamask-extension.md | 60 ++++++++++++------- 1 file changed, 37 insertions(+), 23 deletions(-) diff --git a/domains/performance/skills/perf-hooks-effects/repos/metamask-extension.md b/domains/performance/skills/perf-hooks-effects/repos/metamask-extension.md index e97fc908..88ce95b6 100644 --- a/domains/performance/skills/perf-hooks-effects/repos/metamask-extension.md +++ b/domains/performance/skills/perf-hooks-effects/repos/metamask-extension.md @@ -344,7 +344,7 @@ const useHistoricalPrices = () => { **DO:** - Split effects when conditional logic excludes some dependencies -- Ensure every dependency is either read by the effect or is a named restart key (see Rule: Name Restart Keys) +- Ensure the effect reads every dependency (see Rule: Don't List Dependencies the Effect Doesn't Read) **DON'T:** @@ -377,44 +377,58 @@ const useHistoricalPrices = ({ isEvm, chainId, address }: Props) => { }; ``` -### Rule: Name Restart Keys +### Rule: Don't List Dependencies the Effect Doesn't Read + +The dependency array describes the effect's code. A value that must restart the effect when it changes has to be a value the effect uses. **DO:** -- When an effect synchronizes with an external system (a connection, a subscription, a background registration) and a dependency's only job is to tear that down and start it again when it changes, keep it in the array and say so in a comment beside it -- Prefer passing the value into the call the effect makes, so the effect reads it and the dependency explains itself +- Treat a dependency the effect never reads as a finding, even when the effect is meant to restart when it changes +- If the effect's work depends on the value, pass the value into that work: the call that starts the connection, subscription or background registration +- If the value marks a scope whose state must all reset when it changes, render that scope as a component and give it the value as its `key` +- If the value changes because of a specific action, do the work where that action is handled instead of in an effect that watches for the change **DON'T:** -- Leave a dependency the effect body never reads without a comment. A reviewer cannot tell a restart key from a dead dependency, and the next edit deletes it -- Delete a restart key to satisfy "include only what the effect reads". The effect then keeps a session bound to the old value +- Delete the dependency without one of the changes above. The effect then keeps a session bound to the old value +- Keep the unread dependency and explain it in a comment. React Compiler's effect-dependency validation (`validateExhaustiveEffectDependencies`, lint rule `react-hooks/exhaustive-effect-dependencies`, off by default) reports it as unnecessary, and with that validation on, the compiler leaves the whole component or hook uncompiled, or fails the build under a strict `panicThreshold` +- Add `'use no memo'` to protect the dependency array. It opts the whole function out of React Compiler, and React documents it as a temporary debugging tool +- Add a parameter the called code ignores just so the effect reads the value. That moves the unread value into the call + +**Reference:** [Removing Effect Dependencies](https://react.dev/learn/removing-effect-dependencies), [Resetting all state when a prop changes](https://react.dev/learn/you-might-not-need-an-effect#resetting-all-state-when-a-prop-changes), [`"use no memo"`](https://react.dev/reference/react-compiler/directives/use-no-memo) **Example - WRONG:** ```typescript -useEffect(() => { - const session = startSession(address); - return () => session.stop(); -}, [address, networkId]); // networkId is never read: restart key, or leftover? +const useSession = ({ address, networkId }: Props) => { + useEffect(() => { + const session = startSession(address); + return () => session.stop(); + }, [address, networkId]); // networkId is never read +}; ``` **Example - CORRECT:** ```typescript -useEffect(() => { - const session = startSession(address); - return () => session.stop(); -}, [ - address, - // Not read above: a network change must end this session and start a new one. - networkId, -]); +// The session is for one network, so the effect passes the network in +const useSession = ({ address, networkId }: Props) => { + useEffect(() => { + const session = startSession({ address, networkId }); + return () => session.stop(); + }, [address, networkId]); +}; -// Or make the dependency real by passing it in: -useEffect(() => { - const session = startSession({ address, networkId }); - return () => session.stop(); -}, [address, networkId]); +// Caches must be cleared when the account or network changes: +// a keyed child unmounts on the change, and its cleanup clears them +const CacheScope = () => { + useEffect(() => () => clearCaches(), []); + return null; +}; + +const Root = ({ address, networkId }: Props) => ( + +); ``` ### Rule: Use useRef for Persistent Values From 3bdde1bf67759749bf834f298ffb9bbaffa08897 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Wed, 16 Sep 2026 11:23:49 -0400 Subject: [PATCH 7/8] Minimize `useEffect` dependencies by reading primitives, not by dropping a value the effect uses --- .../repos/metamask-extension.md | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/domains/performance/skills/perf-hooks-effects/repos/metamask-extension.md b/domains/performance/skills/perf-hooks-effects/repos/metamask-extension.md index 88ce95b6..828937f1 100644 --- a/domains/performance/skills/perf-hooks-effects/repos/metamask-extension.md +++ b/domains/performance/skills/perf-hooks-effects/repos/metamask-extension.md @@ -48,37 +48,39 @@ const TokenDisplay = ({ token }: TokenDisplayProps) => { **DO:** -- Reduce dependencies by moving values to default parameters when possible -- Only include dependencies that actually trigger the effect +- Depend on the primitive the effect reads (an ID, an address, a number) rather than an object rebuilt on each render +- Keep values the effect does not use out of the effect **DON'T:** - Include unnecessary dependencies that cause effects to run too often +- Drop a value the effect reads to make it run less often. A default parameter does not stop it being a dependency (see Rule: Include All Dependencies in useEffect) **Example - WRONG:** ```typescript -const TokenBalance = ({ address, network, refreshInterval }: Props) => { +const TokenBalance = ({ account, network, refreshInterval }: Props) => { const [balance, setBalance] = useState('0'); useEffect(() => { const fetch = async () => { - const result = await fetchBalance(address, network); + const result = await fetchBalance(account.address, network); setBalance(result); }; fetch(); const interval = setInterval(fetch, refreshInterval); return () => clearInterval(interval); - }, [address, network, refreshInterval]); // Effect runs too often + }, [account, network, refreshInterval]); // A parent passing account={{ address }} rebuilds it on every render, so the effect refetches and resets the interval each time }; ``` **Example - CORRECT:** ```typescript -const TokenBalance = ({ address, network, refreshInterval = 10000 }: Props) => { +const TokenBalance = ({ account, network, refreshInterval }: Props) => { const [balance, setBalance] = useState('0'); + const { address } = account; useEffect(() => { const fetch = async () => { @@ -89,7 +91,7 @@ const TokenBalance = ({ address, network, refreshInterval = 10000 }: Props) => { fetch(); const interval = setInterval(fetch, refreshInterval); return () => clearInterval(interval); - }, [address, network]); // refreshInterval moved to default param + }, [address, network, refreshInterval]); // The address string is equal across renders }; ``` From f954019e21fa4271156ca99f61a9ec1aad71ddc1 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Thu, 17 Sep 2026 19:16:28 -0400 Subject: [PATCH 8/8] Make the unread-dependency rule reachable from the React Compiler side It was stated only as an effects rule, and the cost it names is a compiler cost: an entry the effect never reads makes the compiler skip the whole function, so the hook ships unmemoized. Someone asking what loses them compiler coverage opens the compiler overlay, where the rule was absent. Zero hits for it across all four react-compiler surfaces before this. --- .../repos/metamask-extension.md | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/domains/performance/skills/perf-react-compiler/repos/metamask-extension.md b/domains/performance/skills/perf-react-compiler/repos/metamask-extension.md index 4ef9d41b..363f5549 100644 --- a/domains/performance/skills/perf-react-compiler/repos/metamask-extension.md +++ b/domains/performance/skills/perf-react-compiler/repos/metamask-extension.md @@ -694,6 +694,46 @@ const TokenList = ({ tokens, filter }: TokenListProps) => { }; ``` +### ❌ Anti-Pattern: Dependencies the Effect Never Reads + +**Problem:** An entry the effect does not read is not a harmless extra. The compiler's +effect-dependency validation treats it as an error and **skips the whole function**, so the +hook ships with no memoization at all. A comment next to the array does not help: the check +reads the array, not the prose. + +**Solution:** Make the value one the effect actually reads, by passing it into the work the +effect starts. Where it marks a scope that must reset as a whole, put a `key` on a component +instead. Never satisfy it with a comment, with `'use no memo'`, or with a parameter the callee +ignores. + +**Example - WRONG:** + +```typescript +useEffect(() => { + const session = manager.startPreload({ address, accountChanged }); + return () => session.stop(); +}, [ + address, + // Provider or network changes release the old preload and register a new one. + provider, + isTestnet, +]); +``` + +**Example - CORRECT:** + +```typescript +useEffect(() => { + // `provider` and `isTestnet` are now read, so the array describes the code. + const session = manager.startPreload({ address, accountChanged, provider, isTestnet }); + return () => session.stop(); +}, [address, provider, isTestnet]); +``` + +**Why this lives here as well as in `perf-hooks-effects`:** the rule is stated there as an +effects rule, and the cost is a React Compiler cost. Someone asking what loses them compiler +coverage opens this file, so the rule has to be reachable from this side too. + ### ❌ Anti-Pattern: Using Index as Key for Dynamic Lists **Problem:** Breaks React's reconciliation when lists can be reordered, filtered, or have items added/removed.