From 78d8342b4d0a3cec0c4a15223c36634150793536 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Fri, 5 Jun 2026 14:53:40 -0400 Subject: [PATCH 01/12] Add performance skills: measurement + React/Redux anti-pattern reviews --- CHANGELOG.md | 4 + .../knowledge/effect-anti-patterns.md | 101 +++++++++++ .../knowledge/metrics-pipeline-design.md | 67 +++++++ .../performance/knowledge/render-cascade.md | 68 ++++++++ .../knowledge/selector-anti-patterns.md | 115 ++++++++++++ .../web-vitals-attribution-import.md | 19 ++ .../web-vitals-production-vs-benchmarks.md | 21 +++ .../knowledge/web-vitals-runtime-metrics.md | 22 +++ .../performance/skills/data-analysis/skill.md | 164 ++++++++++++++++++ .../repos/metamask-extension.md | 27 +++ .../repos/metamask-mobile.md | 31 ++++ .../effect-anti-pattern-review/skill.md | 54 ++++++ .../repos/metamask-extension.md | 43 +++++ .../repos/metamask-mobile.md | 35 ++++ .../selector-anti-pattern-review/skill.md | 120 +++++++++++++ 15 files changed, 891 insertions(+) create mode 100644 domains/performance/knowledge/effect-anti-patterns.md create mode 100644 domains/performance/knowledge/metrics-pipeline-design.md create mode 100644 domains/performance/knowledge/render-cascade.md create mode 100644 domains/performance/knowledge/selector-anti-patterns.md create mode 100644 domains/performance/knowledge/web-vitals-attribution-import.md create mode 100644 domains/performance/knowledge/web-vitals-production-vs-benchmarks.md create mode 100644 domains/performance/knowledge/web-vitals-runtime-metrics.md create mode 100644 domains/performance/skills/data-analysis/skill.md create mode 100644 domains/performance/skills/effect-anti-pattern-review/repos/metamask-extension.md create mode 100644 domains/performance/skills/effect-anti-pattern-review/repos/metamask-mobile.md create mode 100644 domains/performance/skills/effect-anti-pattern-review/skill.md create mode 100644 domains/performance/skills/selector-anti-pattern-review/repos/metamask-extension.md create mode 100644 domains/performance/skills/selector-anti-pattern-review/repos/metamask-mobile.md create mode 100644 domains/performance/skills/selector-anti-pattern-review/skill.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 383d49c7..614ec51c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- Add `performance` skills: `data-analysis`, web-vitals + metrics-pipeline knowledge, and `effect-anti-pattern-review` / `selector-anti-pattern-review` (review-time complements to the `perf-*` optimization skills) with `render-cascade` / anti-pattern knowledge + ## [0.1.0] ### Added diff --git a/domains/performance/knowledge/effect-anti-patterns.md b/domains/performance/knowledge/effect-anti-patterns.md new file mode 100644 index 00000000..00a5d9b1 --- /dev/null +++ b/domains/performance/knowledge/effect-anti-patterns.md @@ -0,0 +1,101 @@ +--- +name: effect-anti-patterns +domain: performance +description: Four React `useEffect` patterns that cause unnecessary renders, memory leaks, or race conditions +--- + +# Effect Anti-Patterns + +Four `useEffect` patterns that are systemically broken in React codebases. Each pattern has a broken example, a fixed example, and a detection recipe. + +## 1. `JSON.stringify` in Dependency Array + +`JSON.stringify` produces a new string on every render when the input is an object. React compares dependency arrays by reference for primitives and by identity for objects. A stringified object is a new primitive every render, so the effect fires every render. + +```typescript +// ❌ BROKEN: effect runs on every render +useEffect(() => { + doSomething(config) +}, [JSON.stringify(config)]) + +// ✅ FIXED: destructure and depend on primitives +const { a, b } = config +useEffect(() => { + doSomething({ a, b }) +}, [a, b]) + +// ✅ ALSO FIXED: stabilize via useMemo +const stableConfig = useMemo(() => config, [config.a, config.b]) +useEffect(() => { + doSomething(stableConfig) +}, [stableConfig]) +``` + +Detection: `grep -rnE 'useEffect.*\[.*JSON\.stringify' ` + +## 2. `useEffect` + `setState` (State Mirror Pattern) + +Using an effect to mirror one piece of state into another is almost always wrong. The computed value should be derived inline or via `useMemo`. Mirror-effects trigger an extra render and create synchronization bugs. + +```typescript +// ❌ BROKEN: two renders, possible stale state +const [fullName, setFullName] = useState('') +useEffect(() => { + setFullName(`${first} ${last}`) +}, [first, last]) + +// ✅ FIXED: derived inline, one render +const fullName = `${first} ${last}` + +// ✅ ALSO FIXED: memoized if expensive +const fullName = useMemo(() => expensiveJoin(first, last), [first, last]) +``` + +The React docs explicitly call this out: [You Might Not Need an Effect](https://react.dev/learn/you-might-not-need-an-effect). + +Detection: `grep -rnB1 -A3 'useEffect' | grep -B2 -A1 'set[A-Z]'` (review hits manually) + +## 3. Missing Interval/Timer Cleanup + +Every `setInterval` and `setTimeout` inside an effect must be cleared in the cleanup function. Otherwise the timer survives component unmount and fires on dead state, leaking memory and causing "setState on unmounted component" warnings. + +```typescript +// ❌ BROKEN: timer leaks after unmount +useEffect(() => { + setInterval(poll, 1000) +}, []) + +// ✅ FIXED +useEffect(() => { + const id = setInterval(poll, 1000) + return () => clearInterval(id) +}, []) +``` + +Detection: `grep -rnB2 -A10 'setInterval\|setTimeout' | grep -B5 'useEffect' | grep -v 'clearInterval\|clearTimeout'` + +## 4. Missing `AbortController` in Async Effects + +Async work inside an effect should be cancellable. Without `AbortController`, a request initiated before unmount can resolve after unmount, triggering `setState` on a dead component and masking memory issues. + +```typescript +// ❌ BROKEN: fetch races unmount +useEffect(() => { + fetch(url).then((r) => setData(r)) +}, [url]) + +// ✅ FIXED +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]) +``` + +## Why These Matter + +- **Renders.** State-mirror effects double every render in the affected component tree. +- **Memory.** Uncleared intervals and timers leak proportional to how often the component mounts. +- **Correctness.** Async effects without cancellation cause `"Can't perform a React state update on an unmounted component"` warnings and, worse, data races where an older response overwrites a newer one. diff --git a/domains/performance/knowledge/metrics-pipeline-design.md b/domains/performance/knowledge/metrics-pipeline-design.md new file mode 100644 index 00000000..03821ef5 --- /dev/null +++ b/domains/performance/knowledge/metrics-pipeline-design.md @@ -0,0 +1,67 @@ +--- +name: metrics-pipeline-design +domain: performance +description: Four-layer metric pipeline architecture for E2E benchmarks, with domain-specific statistical bounds and split reporting paths. +--- + +# Metrics Pipeline Design + +Architecture for adding metric types to an E2E benchmark suite. Separates collection, running, statistics, and reporting into independent layers. + +## Architecture + +``` +Collector → Runner → Statistics → Reporter +``` + +| Layer | Responsibility | +|-------|----------------| +| **Collector** | Extract raw metric from browser/extension per iteration | +| **Runner** | Per-iteration capture + aggregation orchestration | +| **Statistics** | Domain-specific filtering, outlier detection, percentiles | +| **Reporter** | Per-run spans (for quality gate comparison) + aggregated structured logs (for dashboards) | + +Flow files call the collector and return snapshots alongside timers. No flow file does statistics or reporting. + +## Adding a New Metric Type + +1. **Create collector** — function returning typed snapshot with nullable fields for unobserved metrics +2. **Define types** — per-run snapshot, aggregated (reuse `TimerStatistics` for numeric fields), summary +3. **Add domain-specific bounds** — each numeric field gets `{ min, max, allowZero }` +4. **Wire into runner** — collect alongside timers, call aggregation +5. **Add reporter** — per-run spans with `setMeasurement`, aggregated summary as structured log + +## Domain-Specific Statistical Bounds + +Generic timer bounds (1ms–120s, zero=invalid) silently discard valid data from other domains. + +```typescript +// WRONG: CLS values (0–1) all rejected by min=1ms floor +const result = filterBySanityChecks(clsValues); // → empty array + +// RIGHT: per-metric bounds +const BOUNDS = { + inp: { min: 1, max: 30_000, allowZero: false }, // ms + lcp: { min: 1, max: 60_000, allowZero: false }, // ms + cls: { min: 0, max: 10, allowZero: true }, // unitless ratio +}; +``` + +**Rule:** When adding a new metric type, verify whether existing `filterBySanityChecks` assumptions (ms units, zero=invalid) hold. If not, define metric-specific bounds. + +`allowZero` is the critical distinction: CLS=0 means perfect stability (valid); timer=0ms means measurement error (invalid). + +## Split Reporting Path + +| Data | Mechanism | Rationale | +|------|-----------|-----------| +| Aggregated statistics (mean, p75, p95) | Structured log | Low cardinality, dashboard-friendly | +| Per-run snapshots | Sentry spans + `setMeasurement` | Preserves granularity, enables quality gate comparison via Mann-Whitney U | + +`tracesSampleRate: 1.0` required in CI so all per-run spans are captured. + +## SDK Isolation Pattern + +When CI benchmark scripts run in Node but the extension uses a browser SDK (e.g. `@sentry/node` vs `@sentry/browser`): these never share a process. The package manager resolves separate versions per dependency tree. No compatibility issue — they are fully isolated under different lockfile entries. + +Risk: a shared module accidentally importing from the wrong SDK at bundle time. Mitigation: keep the CI SDK as a devDependency excluded from extension builds. 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-anti-patterns.md b/domains/performance/knowledge/selector-anti-patterns.md new file mode 100644 index 00000000..60bf1473 --- /dev/null +++ b/domains/performance/knowledge/selector-anti-patterns.md @@ -0,0 +1,115 @@ +--- +name: selector-anti-patterns +domain: performance +description: Five Redux selector patterns that that break selector memoization and cause render cascades +--- + +# Selector Anti-Patterns + +Each pattern causes `useSelector` to return a new reference on every call, triggering unnecessary re-renders. + +## The Five Patterns + +### 1. Plain Function Selector + +No memoization. Returns new reference every call. + +```typescript +// ❌ BROKEN +export function getPendingApprovals(state) { + return Object.values(state.metamask.pendingApprovals ?? {}); +} + +// ✅ FIXED +const getPendingApprovalsObject = (state) => state.metamask.pendingApprovals ?? {}; +export const getPendingApprovals = createSelector( + getPendingApprovalsObject, + (approvals) => Object.values(approvals), +); +``` + +Detection: `grep -r "export function get" ui/selectors/` + +### 2. Identity Function Selector + +Transform in input, identity in result → memoization is broken. + +```typescript +// ❌ BROKEN: Object.values() in INPUT creates new array +export const getAccounts = createSelector( + (state) => Object.values(state.accounts), + (accounts) => accounts, // identity — cache never hits +); + +// ✅ FIXED: Stable input, transform in OUTPUT +export const getAccounts = createSelector( + (state) => state.accounts, // stable Immer reference + (accounts) => Object.values(accounts), +); +``` + +Detection: Jest warning `"result function returned its own inputs"` + +### 3. Unnecessary Deep Equality + +`createDeepEqualSelector` adds O(n) overhead when Immer already provides stable references. + +```typescript +// ❌ UNNECESSARY: state.accounts is already stable +const getAccounts = createDeepEqualSelector( + (state) => state.metamask.accounts, + (accounts) => transformAccounts(accounts), +); + +// ✅ CORRECT +const getAccounts = createSelector( + (state) => state.metamask.accounts, + (accounts) => transformAccounts(accounts), +); +``` + +Use `createDeepEqualSelector` only when inputs are genuinely not from Immer/Redux state. + +### 4. O(n) Lookups + +`.find()` on Object.values is O(n). With n items × m selectors per state change = O(n×m). + +```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]; +``` + +### 5. Chained Transforms (Unmemoized) + +Each transform creates a new array. Multiple transforms = multiple 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 INPUT unstable (not from Immer/Redux)? +├── YES → createDeepEqualSelector +└── NO → Is OUTPUT unstable (new array/object from transform)? + ├── YES → createResultEqualSelector (or createShallowResultSelector) + └── NO → createSelector +``` diff --git a/domains/performance/knowledge/web-vitals-attribution-import.md b/domains/performance/knowledge/web-vitals-attribution-import.md new file mode 100644 index 00000000..98c0067b --- /dev/null +++ b/domains/performance/knowledge/web-vitals-attribution-import.md @@ -0,0 +1,19 @@ +--- +name: web-vitals-attribution-import +domain: performance +description: web-vitals/attribution is a module import path, not a separate package — no meaningful bundle cost, gives the symptom→cause link +--- + +# Web Vitals Attribution Import + +`web-vitals/attribution` is a **module import path**, not a separate package. The attribution build: +- Provides which script/element caused each metric +- Does **not** meaningfully increase production bundle size (tree-shaking applies) + +Don't skip it for "bundle size" reasons — that's a misread. + +## Why it matters +Attribution is the symptom→cause link: +- INP spike of 500ms +- Attribution: `eventTarget: '#confirm-swap-button'`, `eventType: 'click'` +- Combined with tracing → identifies the controller that blocked diff --git a/domains/performance/knowledge/web-vitals-production-vs-benchmarks.md b/domains/performance/knowledge/web-vitals-production-vs-benchmarks.md new file mode 100644 index 00000000..b6bfdf63 --- /dev/null +++ b/domains/performance/knowledge/web-vitals-production-vs-benchmarks.md @@ -0,0 +1,21 @@ +--- +name: web-vitals-production-vs-benchmarks +domain: performance +description: Web Vitals need different collection in production (web-vitals lib) vs benchmarks (PerformanceObserver); no TBT in prod +--- + +# Web Vitals — Production vs Benchmarks + +Collection differs by environment due to timing constraints. + +## Production — `web-vitals` library +- Reports on `visibilitychange` / `pagehide` +- Handles browser quirks, bfcache, session windowing +- Attribution build shows which element/script caused the metric +- Metrics: **INP, LCP, CLS** (not TBT) + +**Why no TBT in production:** TBT is cumulative and unbounded — it grows indefinitely over an open-ended session. INP is per-interaction → meaningful for real users. TBT fits bounded flows (benchmarks), not sessions. + +## Benchmarks — direct `PerformanceObserver` +- Query on demand (not dependent on page hide) +- Fits an existing `collectMetrics()` pattern diff --git a/domains/performance/knowledge/web-vitals-runtime-metrics.md b/domains/performance/knowledge/web-vitals-runtime-metrics.md new file mode 100644 index 00000000..e05d7c55 --- /dev/null +++ b/domains/performance/knowledge/web-vitals-runtime-metrics.md @@ -0,0 +1,22 @@ +--- +name: web-vitals-runtime-metrics +domain: performance +description: Core Web Vitals (INP, TBT) are runtime responsiveness metrics, not just page-load — high-value for extension UX gates +--- + +# Web Vitals as Runtime Metrics + +Core Web Vitals (INP, TBT) measure **runtime responsiveness**, not just page load. For a browser extension this distinction is critical. + +- **Page load is less relevant** — the popup opens fast; there's no traditional navigation. +- **Runtime interactions matter** — every button click, form submit, confirmation. INP and TBT measure responsiveness during interactions → high-value for extension UX quality gates. + +## Orthogonal to distributed tracing + +| | Web Vitals | Distributed Tracing | +|---|---|---| +| Question | "How did the user perceive it?" | "Which controller caused it?" | +| Scope | user perception | operation attribution | +| Granularity | per-interaction aggregate | per-operation breakdown | + +Use both — perception (web vitals) + attribution (tracing) — not one instead of the other. diff --git a/domains/performance/skills/data-analysis/skill.md b/domains/performance/skills/data-analysis/skill.md new file mode 100644 index 00000000..4566045d --- /dev/null +++ b/domains/performance/skills/data-analysis/skill.md @@ -0,0 +1,164 @@ +--- +maturity: experimental +name: data-analysis +description: Structured approach for analyzing metrics, attributing changes, and communicating findings — five phases (collection → filtering → curation → questioning → synthesis), confidence assignment, audience-appropriate artifacts +--- + +# Data Analysis Skill + +Structured approach for analyzing metrics, attributing changes, and communicating findings. + +--- + +## When to Use + +- Performance analysis from production metrics +- Attribution of improvements/regressions to code changes +- Creating executive summaries or stakeholder communications +- Any analysis requiring correlation of changes to measured outcomes + +--- + +## Quick Reference + +### Five Phases + +``` +Collection → Filtering → Curation → Questioning → Synthesis +``` + +| Phase | Key Question | Output | +| ----------- | --------------------------- | ----------------------------------- | +| Collection | What are we measuring? | Baseline, scope, change list | +| Filtering | What's signal vs. noise? | Categorized changes with confidence | +| Curation | What correlates with what? | Attribution table | +| Questioning | Do we KNOW or BELIEVE this? | Validated claims with caveats | +| Synthesis | Who needs to know what? | Audience-appropriate artifacts | + +### Confidence Assignment + +| Level | Use When | +| ---------- | ---------------------------------------------------------------- | +| **High** | Clear mechanism + timing alignment + targets measured population | +| **Medium** | Plausible mechanism but confounded by other changes | +| **Low** | Speculative or enabling-only | + +### Attribution Table Template + +| Change | Evidence | Release | Metric | Confidence | Notes | +| ------------- | ----------- | --------- | ----------------- | ------------ | --------------------- | +| [Description] | [PR/commit] | [version] | [affected metric] | High/Med/Low | [mechanism or caveat] | + +--- + +## Process + +### 1. Collection + +```markdown +**Metrics:** [What are you measuring?] +**Population:** [Who? All users, p75, specific cohort?] +**Period:** [Measurement window - release tags or dates] +**Source:** [APM, logs, synthetic benchmarks?] +**Baseline:** [Starting values with methodology] +``` + +Enumerate ALL changes in scope: + +- Code changes (PRs, commits) +- Config changes +- External factors (traffic, user growth, infrastructure) + +### 2. Filtering + +Categorize each change: + +- **Direct:** Clear causal path to measured metric +- **Indirect:** Enabling infrastructure (value materializes later) +- **Unknown:** In scope but mechanism unclear +- **Noise:** Unlikely to affect measured metrics + +### 3. Curation + +Build attribution table: + +1. Map changes to metric movements by release +2. Note co-landed changes (shared attribution) +3. Flag anomalies (improvement without cause, unexplained regression) +4. Separate measured vs. post-cutoff work + +### 4. Questioning + +Challenge every attribution: + +- [ ] "Do we KNOW this, or do we BELIEVE this?" +- [ ] "What would need to be true for this to be wrong?" +- [ ] "Are there alternative explanations?" + +Document what's missing: + +- [ ] Unexplained improvements +- [ ] Unexplained regressions +- [ ] Work that SHOULD have helped but didn't +- [ ] Metrics you wish you had + +### 5. Synthesis + +Create audience-appropriate artifacts: + +| Artifact | Audience | Focus | +| --------------------- | --------------- | ------------------------------------- | +| Executive Summary | Leadership | Hard data, key wins, team recognition | +| Attribution Catalogue | Engineering | Detailed per-change analysis | +| Methodology Doc | Future analysts | Process, assumptions, data sources | +| Communication Post | Stakeholders | Exciting but honest, caveats visible | + +--- + +## Communication Template + +```markdown +**[Metric]: [Before] → [After] ([Change %])** + +Population: [Who this measures] +Caveat: [Key limitation] +What's NOT included: [Equally interesting gaps] + +Notable contributors: + +- [Change 1] — [mechanism] +- [Change 2] — [mechanism] + +Bottom line: [One sentence impact statement] +``` + +--- + +## Anti-Patterns + +| Don't | Do Instead | +| ---------------------------------------- | ------------------------------------------------ | +| Claim causation from correlation | "Correlates with" or "plausible contributor" | +| Attribute release total to single change | Note multiple changes, unknown isolated impact | +| Bury caveats in footnotes | Caveats are part of the story | +| Use superlatives without data | Let numbers speak | +| Hide uncertainty | Use qualifiers: "likely," "plausible," "unknown" | + +--- + +## Checklist + +Before finalizing: + +- [ ] Measurement methodology documented +- [ ] Baseline values recorded with source +- [ ] All changes in scope enumerated +- [ ] Confidence levels assigned with justification +- [ ] Unexplained anomalies noted +- [ ] Limitations explicitly stated +- [ ] What's NOT included documented +- [ ] Uncertainty reflected in language +- [ ] Links/references for all claims +- [ ] Multiple artifacts for different audiences + +--- diff --git a/domains/performance/skills/effect-anti-pattern-review/repos/metamask-extension.md b/domains/performance/skills/effect-anti-pattern-review/repos/metamask-extension.md new file mode 100644 index 00000000..67d41395 --- /dev/null +++ b/domains/performance/skills/effect-anti-pattern-review/repos/metamask-extension.md @@ -0,0 +1,27 @@ +--- +repo: metamask-extension +parent: effect-anti-pattern-review +--- + +## Paths + +- Component sources: [`ui/`](https://github.com/MetaMask/metamask-extension/tree/develop/ui) +- Shared hooks: [`ui/hooks/`](https://github.com/MetaMask/metamask-extension/tree/develop/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-anti-pattern-review/repos/metamask-mobile.md b/domains/performance/skills/effect-anti-pattern-review/repos/metamask-mobile.md new file mode 100644 index 00000000..dde98072 --- /dev/null +++ b/domains/performance/skills/effect-anti-pattern-review/repos/metamask-mobile.md @@ -0,0 +1,31 @@ +--- +repo: metamask-mobile +parent: effect-anti-pattern-review +--- + +## 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-anti-pattern-review/skill.md b/domains/performance/skills/effect-anti-pattern-review/skill.md new file mode 100644 index 00000000..97283ccb --- /dev/null +++ b/domains/performance/skills/effect-anti-pattern-review/skill.md @@ -0,0 +1,54 @@ +--- +maturity: experimental +name: effect-anti-pattern-review +description: Review PR diffs that add or modify `useEffect` for the four systemic React effect anti-patterns +--- + +# 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 four patterns catalogued in [`effect-anti-patterns`](../../knowledge/effect-anti-patterns.md). + +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-anti-pattern-review`](../selector-anti-pattern-review/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-anti-patterns`](../../knowledge/effect-anti-patterns.md) and apply the fix from the knowledge file. +4. **Block on pattern 1.** `JSON.stringify` in a dependency array is always broken. Do not merge. +5. **Block on pattern 3 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 use `AbortController`. + +## Grep Checklist + +| Pattern | Detection | Knowledge ref | +|---|---|---| +| 1. `JSON.stringify` in deps | `grep -rnE 'useEffect.*\[.*JSON\.stringify' ` | [§1](../../knowledge/effect-anti-patterns.md#1-jsonstringify-in-dependency-array) | +| 2. State-mirror effect | Hand review — look for `useEffect` that calls `setX` based on other state/props | [§2](../../knowledge/effect-anti-patterns.md#2-useeffect--setstate-state-mirror-pattern) | +| 3. Missing interval/timer cleanup | `grep -rnE 'setInterval\|setTimeout' ` then check each effect returns a cleanup | [§3](../../knowledge/effect-anti-patterns.md#3-missing-intervaltimer-cleanup) | +| 4. Missing `AbortController` | `grep -rnB2 -A10 'fetch\(' ` within `useEffect` blocks | [§4](../../knowledge/effect-anti-patterns.md#4-missing-abortcontroller-in-async-effects) | + +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 — never 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/selector-anti-pattern-review/repos/metamask-extension.md b/domains/performance/skills/selector-anti-pattern-review/repos/metamask-extension.md new file mode 100644 index 00000000..aafb8e9d --- /dev/null +++ b/domains/performance/skills/selector-anti-pattern-review/repos/metamask-extension.md @@ -0,0 +1,43 @@ +--- +repo: metamask-extension +parent: selector-anti-pattern-review +--- + +## Paths + +- Selector definitions: [`ui/selectors/`](https://github.com/MetaMask/metamask-extension/tree/develop/ui/selectors) +- Selector creators: [`shared/lib/selectors/selector-creators.ts`](https://github.com/MetaMask/metamask-extension/blob/develop/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/develop/app/scripts/metamask-controller.js) +- Component consumption sites: anywhere under [`ui/`](https://github.com/MetaMask/metamask-extension/tree/develop/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-anti-pattern-review/repos/metamask-mobile.md b/domains/performance/skills/selector-anti-pattern-review/repos/metamask-mobile.md new file mode 100644 index 00000000..cb28ddab --- /dev/null +++ b/domains/performance/skills/selector-anti-pattern-review/repos/metamask-mobile.md @@ -0,0 +1,35 @@ +--- +repo: metamask-mobile +parent: selector-anti-pattern-review +--- + +## 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 extension-only at this time. diff --git a/domains/performance/skills/selector-anti-pattern-review/skill.md b/domains/performance/skills/selector-anti-pattern-review/skill.md new file mode 100644 index 00000000..31b30be8 --- /dev/null +++ b/domains/performance/skills/selector-anti-pattern-review/skill.md @@ -0,0 +1,120 @@ +--- +maturity: experimental +name: selector-anti-pattern-review +description: Review and diagnose Redux selector anti-patterns that cause render cascades, pre-merge and post-merge +--- + +# Selector Anti-Pattern Review + +**Scope:** Redux selector anti-patterns 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 [`selector-anti-patterns`](../../knowledge/selector-anti-patterns.md) and [`render-cascade`](../../knowledge/render-cascade.md). + +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-anti-pattern-review`, 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-anti-patterns`](../../knowledge/selector-anti-patterns.md) (numbered 1–5) 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 [Pattern 2](../../knowledge/selector-anti-patterns.md#2-identity-function-selector). Do not merge. +5. **Require a fix, not a justification.** None of the five 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 [`render-cascade`](../../knowledge/render-cascade.md). +5. **Verify.** Repeat the action. Confirm the counter stabilizes (e.g. 0→2, not 0→25). Divide raw counts by 2 under React Strict Mode. + +## Grep Checklist + +| Pattern | Detection | Knowledge ref | +|---|---|---| +| 1. Plain function selector | `grep -rE 'export function get' /` | [§1](../../knowledge/selector-anti-patterns.md#1-plain-function-selector) | +| 2. Identity function selector | Jest warning `result function returned its own inputs` | [§2](../../knowledge/selector-anti-patterns.md#2-identity-function-selector) | +| 3. Unnecessary `createDeepEqualSelector` | `grep -rn 'createDeepEqualSelector' /` then verify each input is not from Immer state | [§3](../../knowledge/selector-anti-patterns.md#3-unnecessary-deep-equality) | +| 4. O(n) lookup | `grep -rnE '\.find\(.*=>.*address' /` | [§4](../../knowledge/selector-anti-patterns.md#4-on-lookups) | +| 5. Chained unmemoized transforms | `grep -rnE 'export function get.*\{' / -A5` and check for multiple `.filter/.map/.sort` without memoization | [§5](../../knowledge/selector-anti-patterns.md#5-chained-transforms-unmemoized) | + +See the repo overlay for the concrete `` path. + +## Team-Specific Workarounds + +Two patterns show up beyond the five 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 five 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. Divide raw counts by 2 | From 6e6bbb7c83755d20233abac6ef6d5076af265376 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Thu, 30 Jul 2026 08:22:24 -0400 Subject: [PATCH 02/12] Make knowledge/ the single source for the React perf pattern taxonomy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The selector and effect anti-pattern definitions existed in two places: these knowledge files, and the `performance` skill's own mm-* references already on main. Same patterns, same worked examples, two homes that would drift. knowledge/selector-anti-patterns.md and knowledge/effect-anti-patterns.md are now the canonical, platform-agnostic taxonomy — the union of both sides. The selector file absorbs mutation-in-result and over-broad-input from mm-selector-memoization; the effect file absorbs the dependency-side patterns from mm-hook-dependency-arrays and the lifecycle-side patterns (derived state, effect chains, uncancelled async). mm-selector-memoization.md keeps everything only it can say — the codebase's own selector creators, the verified instance table with file:line, the fix recipes, the scoped greps, the don't-over-correct caveats — and maps each generic pattern onto this codebase instead of redefining it. mm-hook-dependency-arrays.md keeps its richer JSON.stringify treatment and gains a scope note. Citations are by NAME, not by relative link. `install` copies domain knowledge/ and a skill's references/ as siblings under the installed skill directory, so `../../../knowledge/x.md` resolves in the repo and breaks once installed, and `../knowledge/x.md` does the reverse. Section anchors are dropped for the same reason — they broke the moment the taxonomy was renumbered. Also drops the CHANGELOG entry: that file tracks the @metamask/skills CLI package, no merged skill-only PR adds one, and it was this branch's sole conflict with main. --- CHANGELOG.md | 4 - .../knowledge/effect-anti-patterns.md | 146 ++++++++++++------ .../knowledge/selector-anti-patterns.md | 120 +++++++++----- .../effect-anti-pattern-review/skill.md | 23 +-- .../references/mm-hook-dependency-arrays.md | 2 + .../references/mm-selector-memoization.md | 48 ++---- .../selector-anti-pattern-review/skill.md | 27 ++-- 7 files changed, 226 insertions(+), 144 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 614ec51c..383d49c7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,10 +7,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] -### Added - -- Add `performance` skills: `data-analysis`, web-vitals + metrics-pipeline knowledge, and `effect-anti-pattern-review` / `selector-anti-pattern-review` (review-time complements to the `perf-*` optimization skills) with `render-cascade` / anti-pattern knowledge - ## [0.1.0] ### Added diff --git a/domains/performance/knowledge/effect-anti-patterns.md b/domains/performance/knowledge/effect-anti-patterns.md index 00a5d9b1..3b74afe0 100644 --- a/domains/performance/knowledge/effect-anti-patterns.md +++ b/domains/performance/knowledge/effect-anti-patterns.md @@ -1,69 +1,95 @@ --- name: effect-anti-patterns domain: performance -description: Four React `useEffect` patterns that cause unnecessary renders, memory leaks, or race conditions +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 -Four `useEffect` patterns that are systemically broken in React codebases. Each pattern has a broken example, a fixed example, and a detection recipe. +**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. -## 1. `JSON.stringify` in Dependency Array +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). -`JSON.stringify` produces a new string on every render when the input is an object. React compares dependency arrays by reference for primitives and by identity for objects. A stringified object is a new primitive every render, so the effect fires every render. +## 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 -// ❌ BROKEN: effect runs on every render -useEffect(() => { - doSomething(config) -}, [JSON.stringify(config)]) +// ❌ serializes on EVERY render just to build the dep key +useEffect(() => { doSomething(config) }, [JSON.stringify(config)]) -// ✅ FIXED: destructure and depend on primitives -const { a, b } = config -useEffect(() => { - doSomething({ a, b }) -}, [a, b]) +// ❌ new object every render → effect runs every render (or loops forever) +useEffect(() => { ... }, [{ id: user.id }]) -// ✅ ALSO FIXED: stabilize via useMemo -const stableConfig = useMemo(() => config, [config.a, config.b]) -useEffect(() => { - doSomething(stableConfig) -}, [stableConfig]) +// ✅ 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]) ``` -Detection: `grep -rnE 'useEffect.*\[.*JSON\.stringify' ` +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`. -## 2. `useEffect` + `setState` (State Mirror Pattern) +Detection: grep for `JSON.stringify` inside a dependency array, and for inline `{`/`[` +literals in the dep position. -Using an effect to mirror one piece of state into another is almost always wrong. The computed value should be derived inline or via `useMemo`. Mirror-effects trigger an extra render and create synchronization bugs. +## 2. Wrong dependencies ```typescript -// ❌ BROKEN: two renders, possible stale state -const [fullName, setFullName] = useState('') -useEffect(() => { - setFullName(`${first} ${last}`) -}, [first, last]) +// ❌ empty deps but reads state → stale closure, value frozen at first render +const onPress = useCallback(() => doThing(count), []) -// ✅ FIXED: derived inline, one render -const fullName = `${first} ${last}` +// ❌ empty deps and reads nothing → this was never a hook, hoist it out +const config = useMemo(() => ({ a: 1, b: 2 }), []) +``` -// ✅ ALSO FIXED: memoized if expensive -const fullName = useMemo(() => expensiveJoin(first, last), [first, last]) +**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 explicitly call this out: [You Might Not Need an Effect](https://react.dev/learn/you-might-not-need-an-effect). +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 -Detection: `grep -rnB1 -A3 'useEffect' | grep -B2 -A1 'set[A-Z]'` (review hits manually) +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. -## 3. Missing Interval/Timer Cleanup +**Fix:** collapse the chain into render-time derivation — one `useMemo` per step, or one +for the lot. -Every `setInterval` and `setTimeout` inside an effect must be cleared in the cleanup function. Otherwise the timer survives component unmount and fires on dead state, leaking memory and causing "setState on unmounted component" warnings. +## 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) -}, []) +useEffect(() => { setInterval(poll, 1000) }, []) // ✅ FIXED useEffect(() => { @@ -72,19 +98,23 @@ useEffect(() => { }, []) ``` -Detection: `grep -rnB2 -A10 'setInterval\|setTimeout' | grep -B5 'useEffect' | grep -v 'clearInterval\|clearTimeout'` +## 5. Uncancelled async work -## 4. Missing `AbortController` in Async Effects - -Async work inside an effect should be cancellable. Without `AbortController`, a request initiated before unmount can resolve after unmount, triggering `setState` on a dead component and masking memory issues. +Async work started in an effect can resolve *after* unmount — or after the input changed, +letting a stale response overwrite a newer one. ```typescript -// ❌ BROKEN: fetch races unmount +// ❌ fetch races unmount; stale data can win +useEffect(() => { fetchMeta(address).then(setMeta) }, [address]) + +// ✅ cancelled flag — cheapest, works for any promise useEffect(() => { - fetch(url).then((r) => setData(r)) -}, [url]) + let cancelled = false + fetchMeta(address).then((m) => { if (!cancelled) setMeta(m) }) + return () => { cancelled = true } +}, [address]) -// ✅ FIXED +// ✅ AbortController — also cancels the request itself useEffect(() => { const ctrl = new AbortController() fetch(url, { signal: ctrl.signal }) @@ -94,8 +124,26 @@ useEffect(() => { }, [url]) ``` -## Why These Matter +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 -- **Renders.** State-mirror effects double every render in the affected component tree. -- **Memory.** Uncleared intervals and timers leak proportional to how often the component mounts. -- **Correctness.** Async effects without cancellation cause `"Can't perform a React state update on an unmounted component"` warnings and, worse, data races where an older response overwrites a newer one. +- `render-cascade` — how effect-driven re-renders propagate through the component graph. +- `selector-anti-patterns` — 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/selector-anti-patterns.md b/domains/performance/knowledge/selector-anti-patterns.md index 60bf1473..8bf32e9f 100644 --- a/domains/performance/knowledge/selector-anti-patterns.md +++ b/domains/performance/knowledge/selector-anti-patterns.md @@ -1,78 +1,114 @@ --- name: selector-anti-patterns domain: performance -description: Five Redux selector patterns that that break selector memoization and cause render cascades +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 -Each pattern causes `useSelector` to return a new reference on every call, triggering unnecessary re-renders. +**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. -## The Five Patterns +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. Plain Function Selector +## 1. Unmemoized selector -No memoization. Returns new reference every call. +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.metamask.pendingApprovals ?? {}); + return Object.values(state.pendingApprovals ?? {}); } // ✅ FIXED -const getPendingApprovalsObject = (state) => state.metamask.pendingApprovals ?? {}; +const getPendingApprovalsObject = (state) => state.pendingApprovals ?? {}; export const getPendingApprovals = createSelector( getPendingApprovalsObject, (approvals) => Object.values(approvals), ); ``` -Detection: `grep -r "export function get" ui/selectors/` +Detection: grep exported `function get…` in the selectors directory. -### 2. Identity Function Selector +## 2. Identity / passthrough result -Transform in input, identity in result → memoization is broken. +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; controller-state slices usually are not. ```typescript -// ❌ BROKEN: Object.values() in INPUT creates new array +// ❌ 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 OUTPUT +// ✅ FIXED: stable input, transform in the OUTPUT export const getAccounts = createSelector( - (state) => state.accounts, // stable Immer reference + (state) => state.accounts, // stable structural reference (accounts) => Object.values(accounts), ); ``` -Detection: Jest warning `"result function returned its own inputs"` +Detection: the reselect/Jest warning `"result function returned its own inputs"`. -### 3. Unnecessary Deep Equality +## 3. New collection allocated in the result function -`createDeepEqualSelector` adds O(n) overhead when Immer already provides stable references. +Even a correctly-shaped `createSelector` returns a new reference whenever it recomputes — +and if its inputs are unstable, that is every dispatch. ```typescript -// ❌ UNNECESSARY: state.accounts is already stable -const getAccounts = createDeepEqualSelector( - (state) => state.metamask.accounts, - (accounts) => transformAccounts(accounts), -); +// ❌ 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)`. -// ✅ CORRECT -const getAccounts = createSelector( - (state) => state.metamask.accounts, +## 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), ); ``` -Use `createDeepEqualSelector` only when inputs are genuinely not from Immer/Redux state. +Prefer **narrowing the input** over deep-equalizing a giant object. -### 4. O(n) Lookups +## 7. O(n) lookups over unnormalized state -`.find()` on Object.values is O(n). With n items × m selectors per state change = O(n×m). +`.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 @@ -83,16 +119,16 @@ export const getAccountByAddress = (state, address) => export const getAccountByAddress = (state, address) => state.accounts[address]; ``` -### 5. Chained Transforms (Unmemoized) +## 8. Chained unmemoized transforms -Each transform creates a new array. Multiple transforms = multiple new references per call. +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 + return filtered.sort(byDate); // array 3 } // ✅ FIXED: single memoized output @@ -104,12 +140,24 @@ export const getSortedItems = createSelector( ); ``` -## Selector Creator Decision Tree +## 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 +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-anti-pattern-review/skill.md b/domains/performance/skills/effect-anti-pattern-review/skill.md index 97283ccb..1ecc8d86 100644 --- a/domains/performance/skills/effect-anti-pattern-review/skill.md +++ b/domains/performance/skills/effect-anti-pattern-review/skill.md @@ -1,12 +1,12 @@ --- maturity: experimental name: effect-anti-pattern-review -description: Review PR diffs that add or modify `useEffect` for the four systemic React effect anti-patterns +description: Review PR diffs that add or modify `useEffect` for the systemic React effect anti-patterns --- # 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 four patterns catalogued in [`effect-anti-patterns`](../../knowledge/effect-anti-patterns.md). +**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-anti-patterns`** 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. @@ -26,19 +26,20 @@ Applies to both `metamask-extension` and `metamask-mobile`. See overlays for rep 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-anti-patterns`](../../knowledge/effect-anti-patterns.md) and apply the fix from the knowledge file. -4. **Block on pattern 1.** `JSON.stringify` in a dependency array is always broken. Do not merge. -5. **Block on pattern 3 without cleanup.** Any `setInterval` / `setTimeout` without a matching `clearInterval` / `clearTimeout` in the cleanup function is blocking. +3. **For each hit, map to a pattern** in `effect-anti-patterns` and apply the fix from the knowledge file. +4. **Block on unstable dependency identity.** `JSON.stringify` in a dependency array is always broken. Do not merge. +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 use `AbortController`. ## Grep Checklist -| Pattern | Detection | Knowledge ref | -|---|---|---| -| 1. `JSON.stringify` in deps | `grep -rnE 'useEffect.*\[.*JSON\.stringify' ` | [§1](../../knowledge/effect-anti-patterns.md#1-jsonstringify-in-dependency-array) | -| 2. State-mirror effect | Hand review — look for `useEffect` that calls `setX` based on other state/props | [§2](../../knowledge/effect-anti-patterns.md#2-useeffect--setstate-state-mirror-pattern) | -| 3. Missing interval/timer cleanup | `grep -rnE 'setInterval\|setTimeout' ` then check each effect returns a cleanup | [§3](../../knowledge/effect-anti-patterns.md#3-missing-intervaltimer-cleanup) | -| 4. Missing `AbortController` | `grep -rnB2 -A10 'fetch\(' ` within `useEffect` blocks | [§4](../../knowledge/effect-anti-patterns.md#4-missing-abortcontroller-in-async-effects) | +| Pattern (`effect-anti-patterns` §) | 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. 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..babcb790 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-anti-patterns`** 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 diff --git a/domains/performance/skills/performance/references/mm-selector-memoization.md b/domains/performance/skills/performance/references/mm-selector-memoization.md index b9e117db..6bbb7636 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 - -### 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. - -### 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)`. - -### 4. `state => state` (or a huge slice) as an input selector -Forces recomputation on **any** state change anywhere. Narrow the input. +## The patterns, and what they look like here + +The pattern taxonomy itself lives in the **`selector-anti-patterns`** 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. + +| Pattern | How it shows up in Mobile | Fix here | +|---|---|---| +| **Identity / passthrough result** | `createSelector(selectControllerState, (s) => s.things)` — controller-state slices are not reference-stable, so it recomputes and returns a new ref every dispatch | `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 | + +The two that dominate the verified instances below are the first two. ## Verified MetaMask instances diff --git a/domains/performance/skills/selector-anti-pattern-review/skill.md b/domains/performance/skills/selector-anti-pattern-review/skill.md index 31b30be8..0b541d08 100644 --- a/domains/performance/skills/selector-anti-pattern-review/skill.md +++ b/domains/performance/skills/selector-anti-pattern-review/skill.md @@ -6,7 +6,7 @@ description: Review and diagnose Redux selector anti-patterns that cause render # Selector Anti-Pattern Review -**Scope:** Redux selector anti-patterns 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 [`selector-anti-patterns`](../../knowledge/selector-anti-patterns.md) and [`render-cascade`](../../knowledge/render-cascade.md). +**Scope:** Redux selector anti-patterns 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-anti-patterns`** 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). @@ -27,8 +27,8 @@ Both `metamask-extension` and `metamask-mobile` share the same React + Redux arc 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-anti-patterns`](../../knowledge/selector-anti-patterns.md) (numbered 1–5) 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 [Pattern 2](../../knowledge/selector-anti-patterns.md#2-identity-function-selector). Do not merge. +3. **Match each hit to a pattern** in `selector-anti-patterns` 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-anti-patterns` §2). Do not merge. 5. **Require a fix, not a justification.** None of the five patterns have a valid use case. See [Pitfalls](#common-pitfalls) for the narrow `createDeepEqualSelector` exception. ## Mode B: Post-Merge Diagnosis (WDYR-driven) @@ -41,24 +41,27 @@ Both `metamask-extension` and `metamask-mobile` share the same React + Redux arc ``` 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 [`render-cascade`](../../knowledge/render-cascade.md). +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). Divide raw counts by 2 under React Strict Mode. ## Grep Checklist -| Pattern | Detection | Knowledge ref | -|---|---|---| -| 1. Plain function selector | `grep -rE 'export function get' /` | [§1](../../knowledge/selector-anti-patterns.md#1-plain-function-selector) | -| 2. Identity function selector | Jest warning `result function returned its own inputs` | [§2](../../knowledge/selector-anti-patterns.md#2-identity-function-selector) | -| 3. Unnecessary `createDeepEqualSelector` | `grep -rn 'createDeepEqualSelector' /` then verify each input is not from Immer state | [§3](../../knowledge/selector-anti-patterns.md#3-unnecessary-deep-equality) | -| 4. O(n) lookup | `grep -rnE '\.find\(.*=>.*address' /` | [§4](../../knowledge/selector-anti-patterns.md#4-on-lookups) | -| 5. Chained unmemoized transforms | `grep -rnE 'export function get.*\{' / -A5` and check for multiple `.filter/.map/.sort` without memoization | [§5](../../knowledge/selector-anti-patterns.md#5-chained-transforms-unmemoized) | +| Pattern (`selector-anti-patterns` §) | 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 | See the repo overlay for the concrete `` path. ## Team-Specific Workarounds -Two patterns show up beyond the five in the knowledge file. Both are workarounds for broken selectors downstream. The fix is always to fix the selector, never to propagate the workaround. +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` From f5aa6f9be786e27c07db6be9bf4762eb29bccb9e Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Thu, 30 Jul 2026 08:38:40 -0400 Subject: [PATCH 03/12] Consolidate the shift-left performance work into one domain PR MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Folds in the react-render-proof skill (was #82) and the mobile reference-library additions (was #49). All three were the same effort seen from different ends — moving performance work earlier in the loop — and they share a substrate, so reviewing them apart meant reviewing the substrate three times. The loop this domain now covers: - catch it at review — effect/selector anti-pattern review skills, driven by the knowledge taxonomy - prove it moved — react-render-proof, with a delivery gate so an arm whose treatment never reached the bundle cannot report as a null - measure it honestly — data-analysis, benchmark hygiene, web-vitals framing - know the codebase — the mm-* reference library and its audit playbook Also neutralizes five references to private planning tickets, which do not belong on a public repository — they named internal epic and audit-ticket numbers. The surrounding guidance is unchanged; only the identifiers are gone. --- .../references/mm-audit-playbook.md | 19 ++- .../references/mm-hook-dependency-arrays.md | 1 + .../performance/references/mm-planning.md | 6 +- .../mm-react-compiler-error-triage.md | 90 +++++++++++ .../references/mm-react-compiler.md | 3 + .../references/mm-redux-antipatterns.md | 4 + .../references/mm-selector-cascade.md | 122 ++++++++++++++ .../references/mm-selector-memoization.md | 2 + .../references/mm-state-normalization.md | 136 ++++++++++++++++ .../skills/performance/references/mm-tools.md | 20 ++- .../references/mm-useeffect-antipatterns.md | 149 ++++++++++++++++++ .../performance/repos/metamask-mobile.md | 10 +- .../skills/react-render-proof/skill.md | 112 +++++++++++++ 13 files changed, 661 insertions(+), 13 deletions(-) create mode 100644 domains/performance/skills/performance/references/mm-react-compiler-error-triage.md create mode 100644 domains/performance/skills/performance/references/mm-selector-cascade.md create mode 100644 domains/performance/skills/performance/references/mm-state-normalization.md create mode 100644 domains/performance/skills/performance/references/mm-useeffect-antipatterns.md create mode 100644 domains/performance/skills/react-render-proof/skill.md diff --git a/domains/performance/skills/performance/references/mm-audit-playbook.md b/domains/performance/skills/performance/references/mm-audit-playbook.md index b3f302c7..ff756989 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 — single-entry cache → mm-state-normalization.md +grep -rnE "export (function|const) (get|select)[A-Z][A-Za-z]* = \(state|export function (get|select)" app/selectors --include="*.ts" # plain unmemoized function selectors (no createSelector at all) ``` -Check each result function for: identity/passthrough, new collection without deep-equal, mutation, `state=>state` input. +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,18 @@ grep -rn "dispatch(" app --include="*.ts" --include="*.tsx" | grep -v ".test." | grep -rn "Provider value={{" app --include="*.tsx" | grep -v ".test." ``` -### Hooks → [mm-hook-dependency-arrays.md](mm-hook-dependency-arrays.md) +### Hooks → [mm-hook-dependency-arrays.md](mm-hook-dependency-arrays.md) / [mm-useeffect-antipatterns.md](mm-useeffect-antipatterns.md) ```bash grep -rn "\[JSON.stringify\|, JSON.stringify" app --include="*.ts" --include="*.tsx" | grep -v ".test." +grep -rn -A6 "useEffect(" app --include="*.ts" --include="*.tsx" | grep -E "fetch\(|\.then\(" | grep -v "signal\|cancelled\|abort" | grep -v ".test." # async effects without cancellation ``` -(`exhaustive-deps` is NOT linted in this repo — check effect deps by hand.) +(`exhaustive-deps` is NOT linted in this repo — check effect deps by hand.) For effect-body problems — derived state via useEffect+setState, effect chains, post-unmount setState — use the read pass in [mm-useeffect-antipatterns.md](mm-useeffect-antipatterns.md). + +### React Compiler coverage → [mm-react-compiler-error-triage.md](mm-react-compiler-error-triage.md) +```bash +grep -rn "use no memo" app --include="*.ts" --include="*.tsx" # opt-outs: each needs a reason + TODO +``` +For a re-render-heavy screen, confirm the components are actually **compiled** (`Memo ✨` in DevTools) before suggesting manual memoization — they may be sitting in the error/unsupported bucket. ### Animations → [mm-layout-animations.md](mm-layout-animations.md) ```bash @@ -97,6 +107,7 @@ Grep finds *syntactic* patterns. The highest-impact re-render bugs are *data-flo - **Render-phase side effects / setState** — any `setState(...)`, `dispatch(...)`, or `trackEvent(...)` in a render body (not inside `useEffect`/`useCallback`)? Triggers extra render passes. - **O(n²) reduce-with-spread** — `reduce((acc, x) => ({ ...acc, ... }), {})` rebuilt every render. - **Per-item subscription hooks** — trace each into its manager; shared subscription = fine, per-subscriber whole-dataset snapshot = bug. → [mm-streaming-realtime.md](mm-streaming-realtime.md) +- **Deep-equal selector inputs** — for every `createDeepEqualSelector`, read its *input selectors*: an input function that allocates a fresh composite per call (object spreads of controller state, other selectors' results collected into a new object) forces the deep compare to run over the whole composite on every check — and no result-function grep catches it. `grep -rn -B3 "createDeepEqualSelector(" app/selectors` lists the sites; read each first argument. → [mm-selector-cascade.md](mm-selector-cascade.md) (proactive mode) Confirm any hit with the Profiler ("why did this render?") before asserting — see [mm-tools.md](mm-tools.md). @@ -107,6 +118,8 @@ Confirm any hit with the Profiler ("why did this render?") before asserting — - [ ] No real-time / high-frequency data dispatched to Redux - [ ] `Context.Provider value` is memoized (not an inline object) - [ ] No `JSON.stringify` in a hot dependency array +- [ ] Async effects guard against post-unmount / stale setState (cancelled flag or `AbortController`) +- [ ] No new parameterized selector (single-entry cache) on a list/hot path — use a lookup-map selector instead - [ ] Layout animations use Reanimated v3, not `Animated` + `useNativeDriver:false` - [ ] Growable lists use FlashList with stable keys (+ `getItemType` if mixed) - [ ] New event listeners / timers / subscriptions have cleanup diff --git a/domains/performance/skills/performance/references/mm-hook-dependency-arrays.md b/domains/performance/skills/performance/references/mm-hook-dependency-arrays.md index babcb790..5ff27dc0 100644 --- a/domains/performance/skills/performance/references/mm-hook-dependency-arrays.md +++ b/domains/performance/skills/performance/references/mm-hook-dependency-arrays.md @@ -85,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-react-compiler-error-triage.md b/domains/performance/skills/performance/references/mm-react-compiler-error-triage.md new file mode 100644 index 00000000..e726279d --- /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, tracked internally): 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 (internal epic) 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 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..403f9e5c --- /dev/null +++ b/domains/performance/skills/performance/references/mm-selector-cascade.md @@ -0,0 +1,122 @@ +--- +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-anti-patterns`** — 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 (documented for this repo's controller-state slices in [mm-selector-memoization.md](mm-selector-memoization.md)), input references break even when data didn't change, and `createDeepEqualSelector` at the *root* is the pragmatic tool. + +Establish which contract a slice actually follows (log `prev === next` for the input across two unrelated dispatches) before choosing — the answer differs per slice, and assuming the wrong contract either reintroduces the cascade or buys deep-compares you don't need. + +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 (internal audit ticket): + +```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 +- [mm-react-compiler-error-triage.md](mm-react-compiler-error-triage.md) — confirming what the compiler actually covers diff --git a/domains/performance/skills/performance/references/mm-selector-memoization.md b/domains/performance/skills/performance/references/mm-selector-memoization.md index 6bbb7636..c6731a62 100644 --- a/domains/performance/skills/performance/references/mm-selector-memoization.md +++ b/domains/performance/skills/performance/references/mm-selector-memoization.md @@ -97,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..dc59b349 --- /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-anti-patterns` §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 (internal audit tickets), where linear scans and reshaping selectors multiplied across power-user data: with 1,000 tokens, 27 `.find()`-based lookups per render is 27,000 comparisons — per render. + +## Pattern — O(n) scans where the state shape should provide O(1) lookups + +```ts +// ❌ linear scan through all accounts on every call +export const getAccountByAddress = createSelector( + selectAccounts, + (_, address) => address, + (accounts, address) => + Object.values(accounts).find((a) => a.address.toLowerCase() === address.toLowerCase()), +); +``` + +When lookups by some key are frequent, **index the state once** instead of scanning per consumer: + +```ts +// ✅ build the index once per data change; lookups are O(1) key access +export const selectAccountsByAddress = createSelector(selectAccounts, (accounts) => + Object.fromEntries(Object.values(accounts).map((a) => [a.address.toLowerCase(), a])), +); +// consumers key into the memoized index — no scan, no per-arg selector cache to bust +const account = useSelector(selectAccountsByAddress)[address.toLowerCase()]; +``` + +Normalized shape (`byId` / `byAddress` maps + an `ids` array for order) is the same idea applied at the reducer level — the index is maintained on write instead of derived on read. + +## Pattern — parameterized selector cache thrashing + +`createSelector` has a **single-entry cache**. A parameterized selector called with different arguments from different components busts that one cache slot on every call: + +```ts +// ❌ each component's call evicts the previous component's result +const a1 = useSelector((s) => getAccountByAddress(s, addr1)); // miss +const a2 = useSelector((s) => getAccountByAddress(s, addr2)); // miss, evicts addr1 +const a3 = useSelector((s) => getAccountByAddress(s, addr3)); // miss, evicts addr2 — and so on every render cycle +``` + +In a list rendering N rows, the "memoized" selector recomputes N times per render, forever. **Check the memoizer before flagging:** this codebase already uses `weakMapMemoize` for some parameterized selectors (e.g. `selectNetworkConfigurationByChainId`), which caches per-argument and doesn't thrash — but only for *stable* arguments. A fresh **object literal** argument per call (`selectAsset(state, { address, chainId, isStaked })`) defeats `weakMapMemoize` too: every call is a new WeakMap key. Fixes, in order of preference: + +1. **Lookup-map selector** (above): select the whole memoized index once; key into it. Sidesteps per-arg caching entirely. +2. **Per-instance selector**: a factory (`makeSelectAccountByAddress()`) instantiated in the component with `useMemo`, so each call site owns its own cache slot. +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 e7e7d2df..5de42764 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 @@ -95,8 +98,10 @@ Then read what's emitted — a load log (e.g. `source: 'cache' | 'fresh_fetch'`, "A hook/component re-renders the whole list even though children are memoized" → mm-unstable-hook-return.md (a hook returns a new array/object ref every render → defeats downstream memo) -"I can't see network calls on Android" - → Reactotron (DevTools network tab doesn't work on Android) + +"Is this slow flow data-bound or render-bound?" + → Network panel for request timings → js-network-panel.md + → Performance panel for the full React+JS+network timeline → js-performance-panel.md "Memory grows over a session / crashes after long use" → js-memory-leaks.md (JS) or native-memory-leaks.md (native) @@ -129,15 +134,17 @@ Then read what's emitted — a load log (e.g. `source: 'cache' | 'fresh_fetch'`, - **Interpret:** JS drops → expensive renders/selectors/computation. UI drops → native rendering/animation. Both → start JS-side. - **Next:** React Native DevTools. -### React Native DevTools (re-renders, timing, memory) — iOS + Android +### React Native DevTools (re-renders, timing, memory, performance) — iOS + Android - **Open:** press `j` in Metro, or shake → "Open DevTools". Hermes is on for both platforms, so this works everywhere. - **Profiler:** ⚙️ → enable "Record why each component rendered" → Start → reproduce the **exact** interaction → Stop. - **Read:** flamegraph (yellow = slow), Ranked view (slowest first), right panel "why did this render?" (props/hook/parent). - **Next:** props churn → `useCallback`/memo; selector new-ref → [mm-selector-memoization.md](mm-selector-memoization.md); parent re-render → move state down / [mm-context-performance.md](mm-context-performance.md). - **JS CPU:** the JavaScript Profiler tab → Heavy (Bottom-Up) for non-React hot functions. +- **Performance panel:** Performance tab → Record → run the flow → Stop. Shows React scheduler phases, the Components flamegraph, the JS Thread, and Network events on one timeline. Use it when the Profiler alone doesn't explain a slow flow. See [js-performance-panel.md](js-performance-panel.md). +- **Network panel:** Network tab → records `fetch()`/`XHR`/`` automatically. Timings, headers, response previews, and an Initiator call stack. Tells you if a slow screen is data-bound vs render-bound. See [js-network-panel.md](js-network-panel.md). -### Reactotron (network on Android) -- Network inspection when the DevTools network tab is unavailable on Android. +### Reactotron (network on Android — pre-0.83 fallback) +- Network inspection for **pre-0.83** setups where the DevTools Network tab is unavailable on Android. On RN 0.83+ prefer the DevTools [Network panel](js-network-panel.md) (works on Android). WebSocket events still aren't captured by the Network panel — Reactotron remains useful there. ### Flashlight (optional, external — NOT installed in this repo) - **Not in `package.json`** — it's an external Callstack tool. Don't assume it's available; for day-to-day FPS use Perf Monitor + RN DevTools. @@ -162,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 high-frequency span needs a deterministic sub-sample gate (and a kill-switch) before it ships. - **Component-level: use a per-feature measurement hook, not raw `trace()`.** The repo convention is a declarative `useXMeasurement` hook (e.g. `app/components/UI/Predict/hooks/usePredictMeasurement.ts`, `usePerpsMeasurement`, `useSectionPerformance`) that starts on mount and ends when conditions are true — which structurally enforces the "end on data-loaded, not mount" rule below: ```ts usePredictMeasurement({ traceName: TraceName.PredictMarketDetailsView, conditions: [dataLoaded, !isLoading] }); @@ -217,4 +225,4 @@ A code session often has no running device, so "Measure first" isn't literally p ## Related -- [mm-power-user-scenario.md](mm-power-user-scenario.md) · [js-measure-fps.md](js-measure-fps.md) · [js-profile-react.md](js-profile-react.md) · [native-measure-tti.md](native-measure-tti.md) +- [mm-power-user-scenario.md](mm-power-user-scenario.md) · [js-measure-fps.md](js-measure-fps.md) · [js-profile-react.md](js-profile-react.md) · [js-performance-panel.md](js-performance-panel.md) · [js-network-panel.md](js-network-panel.md) · [native-measure-tti.md](native-measure-tti.md) 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..f9eb1fd8 --- /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-anti-patterns`** 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** (internal extension epic): once a repo has three hand-rolled cancelled flags, extract shared hooks — `useIsMounted()`, `useAbortableEffect(fn, deps)` (effect receives a signal), `useEventListener(target, event, handler)` (auto-removes on unmount) — so cleanup is the default, not per-site diligence. + +## Pattern — missing cleanup for timers / subscriptions / listeners + +```tsx +// ❌ each mount adds another interval/listener; none are removed +useEffect(() => { + const id = setInterval(refreshGasEstimate, 15000); + emitter.on('update', onUpdate); +}, []); + +// ✅ every subscription returns its teardown +useEffect(() => { + const id = setInterval(refreshGasEstimate, 15000); + emitter.on('update', onUpdate); + return () => { clearInterval(id); emitter.off('update', onUpdate); }; +}, []); +``` + +Leaked intervals keep firing (and keep dispatching) forever; leaked listeners hold the closure — and everything it captured — out of garbage collection. See [js-memory-leaks.md](js-memory-leaks.md) for hunting these in a running app, and [mm-streaming-realtime.md](mm-streaming-realtime.md) for subscription lifecycles tied to visibility. + +## Pattern — regular variable where a ref is needed + +```tsx +// ❌ reset to false on every render — the guard never works +let hasLoggedImpression = false; +useEffect(() => { + if (!hasLoggedImpression) { logImpression(); hasLoggedImpression = true; } +}); + +// ✅ useRef persists across renders without triggering them +const hasLoggedImpression = useRef(false); +``` + +Any mutable flag/cache/previous-value that must survive re-renders but shouldn't cause them belongs in a ref, not a closure variable (and not state). + +## Pattern — large objects captured in effect closures + +An effect (or its cleanup) that closes over a large object — full token lists, raw API payloads — pins that object in memory for as long as the subscription lives. Extract the fields you need into locals *before* the closure, or read through a ref, so the big object can be collected. + +## How to find + +```bash +# setState-from-effect derivation candidates (review hits — some are legitimate syncs) +grep -rn -A3 "useEffect(" app --include="*.tsx" | grep -B1 "set[A-Z]" | grep -v ".test." + +# fetch/promises in effects with no signal/cancelled handling nearby +grep -rn -A6 "useEffect(" app --include="*.ts*" | grep -E "fetch\(|\.then\(" | grep -v "signal\|cancelled\|abort" | grep -v ".test." + +# intervals/timeouts/listeners inside effects — then eyeball for a `return () =>` teardown +grep -rn "setInterval\|setTimeout\|addEventListener\|\.on(" app --include="*.ts*" | grep -v ".test." | grep -v "clear\|remove\|off(" +``` + +## Verify + +- React DevTools highlight-updates: the derive-in-render fix removes the double render pass on the affected component. +- No "setState on unmounted component" / no stale-data flash when rapidly switching the input (account/network) that drives the effect. +- For cleanup fixes: navigate to the screen and back N times → timer/listener count stays flat (see [js-memory-leaks.md](js-memory-leaks.md)). + +## Don't over-correct + +- Effects that *synchronize with external systems* (subscriptions, navigation, imperative APIs) are the legitimate use — don't mechanically rewrite every effect as `useMemo`. +- An async effect whose component provably never unmounts mid-flight (e.g. root-level, app lifetime) doesn't need a cancelled flag — but say so in review rather than assuming. +- Don't wrap trivial derivations in `useMemo` while de-effecting — plain expressions are fine until profiling or a memoized child says otherwise. + +## Related + +- [mm-hook-dependency-arrays.md](mm-hook-dependency-arrays.md) — the deps side: JSON.stringify, inline literals, stale closures +- [js-memory-leaks.md](js-memory-leaks.md) — measuring leaks the missing cleanups cause +- [mm-streaming-realtime.md](mm-streaming-realtime.md) — subscription setup/teardown for real-time screens +- [mm-unstable-hook-return.md](mm-unstable-hook-return.md) — unstable hook returns that make effects re-run diff --git a/domains/performance/skills/performance/repos/metamask-mobile.md b/domains/performance/skills/performance/repos/metamask-mobile.md index d077f7df..e136589d 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) | @@ -61,12 +64,15 @@ Always pair measurement with the **power-user scenario on Android** — see [ref | **Real-time / websocket screen slow or janky** (prices, order book, live balances); slow only on first-open / after backgrounding | [mm-streaming-realtime.md](references/mm-streaming-realtime.md) | | **List re-renders fully even though children are memoized** (a hook returns a new array/object every render) | [mm-unstable-hook-return.md](references/mm-unstable-hook-return.md) | | FPS drops; want to localize JS vs UI thread | [js-measure-fps.md](references/js-measure-fps.md) → [js-profile-react.md](references/js-profile-react.md) | +| Need a full timeline (React scheduler + JS + network) for a slow flow, not just re-renders | [js-performance-panel.md](references/js-performance-panel.md) | +| Inspect network requests / API timings; is a slow screen data-bound or render-bound? | [js-network-panel.md](references/js-network-panel.md) | | Memory grows over a session | [js-memory-leaks.md](references/js-memory-leaks.md) / [native-memory-leaks.md](references/native-memory-leaks.md) | | Slow startup (TTI) | [native-measure-tti.md](references/native-measure-tti.md) → [bundle-analyze-js.md](references/bundle-analyze-js.md) | | Bundle too big / barrel imports / heavy lib | [bundle-barrel-exports.md](references/bundle-barrel-exports.md) → [bundle-analyze-js.md](references/bundle-analyze-js.md) → [bundle-library-size.md](references/bundle-library-size.md) | | Native module / sync method blocking JS | [native-sdks-over-polyfills.md](references/native-sdks-over-polyfills.md) | | Native lib crashes on 16KB-page Android | [native-android-16kb-alignment.md](references/native-android-16kb-alignment.md) | | Enable automatic memoization | [mm-react-compiler.md](references/mm-react-compiler.md) → [js-react-compiler.md](references/js-react-compiler.md) | +| Compiler is enabled but a component shows no `Memo ✨`; compiler errors in build output — which are real? | [mm-react-compiler-error-triage.md](references/mm-react-compiler-error-triage.md) | ## Verified anti-pattern catalogue (this codebase) @@ -86,6 +92,8 @@ Ordered by impact. Each links to the guide with the fix. **The `Where` column li | High | lodash main-package imports (98 files, no tree-shaking) | 98 files | [bundle-library-size.md](references/bundle-library-size.md) | | High | FlatList missing perf props on growing lists | 65 FlatList JSX | [js-lists-flatlist-flashlist.md](references/js-lists-flatlist-flashlist.md) | | High | AppState listener without cleanup | `app/core/SDKConnectV2/services/connection-registry.ts:487` | [js-memory-leaks.md](references/js-memory-leaks.md) | +| High | Parameterized selector (single-entry cache, busted per arg) doing an O(n) `Object.values().flat().find()` scan per call | `selectSingleTokenByAddressAndChainId` `app/selectors/tokensController.ts:174`; also `app/selectors/assets/assets-list.ts`, `app/selectors/moneyAccountController/index.ts` | [mm-state-normalization.md](references/mm-state-normalization.md) | +| Medium | Async effect without cancellation; setState-chain effects; derived state via useEffect+setState | feature-specific — run the guide's greps | [mm-useeffect-antipatterns.md](references/mm-useeffect-antipatterns.md) | | Medium | Inline `useSelector(state => state.x)` bypassing named selectors | 3 files | [mm-redux-antipatterns.md](references/mm-redux-antipatterns.md) | | Medium | Lottie where Rive fits (Rive already installed) | 5 files | [js-animations-reanimated.md](references/js-animations-reanimated.md) | | Low | dayjs + luxon both present (dedup) | 4 + 6 files | [bundle-library-size.md](references/bundle-library-size.md) | @@ -101,4 +109,4 @@ Ordered by impact. Each links to the guide with the fix. **The `Where` column li ## Attribution -Generic React Native references (`js-*`, `native-*`, `bundle-*`) adapted from "The Ultimate Guide to React Native Optimization" by Callstack. MetaMask-specific guidance (`mm-*`) from the internal Performance Guide for Engineers and verified codebase audits. +Generic React Native references (`js-*`, `native-*`, `bundle-*`) adapted from "The Ultimate Guide to React Native Optimization" by Callstack. MetaMask-specific guidance (`mm-*`) from the internal Performance Guide for Engineers and verified codebase audits. Cross-platform React/Redux guidance (`mm-selector-cascade`, `mm-useeffect-antipatterns`, `mm-state-normalization`, `mm-react-compiler-error-triage`) adapted from MetaMask contributor-docs [`frontend-performance.md`](https://github.com/MetaMask/contributor-docs/blob/main/docs/frontend-performance.md) and the extension performance audit (extension PRs metamask-extension#38007, metamask-extension#37147). diff --git a/domains/performance/skills/react-render-proof/skill.md b/domains/performance/skills/react-render-proof/skill.md new file mode 100644 index 00000000..4cb4fa2c --- /dev/null +++ b/domains/performance/skills/react-render-proof/skill.md @@ -0,0 +1,112 @@ +--- +name: react-render-proof +description: Prove a React rendering or memoization change actually reduced work, with a delivery gate and a reported band. Covers re-render counts (why-did-you-render), selector recomputes (reselect's real `.recomputations()` API), and A/B arms toggled at a FIXED commit rather than across a merge boundary. The falsifier is an arm whose treatment never reached the built bundle — a null from undelivered treatment is indistinguishable from a null from a small effect and reports as the second. Triggers on /react-render-proof, or when asked to prove a component stopped over-rendering, measure selector recomputation, validate a memoization/React Compiler change, run a render-count A/B, or interpret a re-render benchmark. Callable by pr-validate as the engine behind its React render & selector proof evidence category. +maturity: experimental +--- + +# /react-render-proof + +A render-count number is worthless until two things are true: the **treatment reached the +artifact the browser executes**, and the number is reported as a **band** rather than a point. +Most of this skill is those two checks. The measurement itself is easy; the failure mode is +reporting a difference between arms that never differed. + +> **Falsifier.** An arm whose manipulation cannot be observed in the built bundle. If you +> cannot point at output that differs *in kind* between arms — a symbol present in one and +> absent in the other, a flag line in a log — the A/B is not designed yet, and any delta it +> produces is noise with a story attached. + +## Method + +1. **Name the delivery check before building arms, and verify it emits.** State how the run + itself will show the arms differ, then confirm that output exists on one real build before + scaling to N repeats. This ordering is the whole skill. A measurement launched before the + instrument is proven emits a null that reads exactly like "no effect". + +2. **Derive the needle from output at the stage you will grep, not one stage upstream.** + Compiler and bundler output are not the same text. Worked failures, both real: + - React Compiler at `target: '17'` emits `react-compiler-runtime`; at `target: '19'` it + emits `react/compiler-runtime`. Grepping for the wrong one returns 0 in *both* arms and + fails the arm that actually got the treatment. + - `_c(` is the form **babel** emits. Metro transforms it further — in a real 119 MB React + Native bundle it scored 13 hits, every one a minified vendor identifier + (`function _c(e,t){return e|t}`), while the true compiler output went uncounted. The form + that survived metro was `memo_cache_sentinel` (4681 in the treated arm vs 166 in the + control). Same needle, two bundlers, two different answers. + + Compile one real file through the project's own config and read the output. Ten minutes + here saves a whole run. + +3. **A name is not a witness — count what only exists when the module is included.** A bare + module specifier appears in bundled `package.json` dependency lists whether or not the + module was ever pulled in; a clean control arm scored exactly 1 that way and was wrongly + failed. Gate on artifacts that cannot appear otherwise (a runtime sentinel, a compiled call + site). Keep the specifier count as a diagnostic — 3081-vs-1 is informative, it just isn't a + boolean. + +4. **Use the library's real counter before injecting your own.** `reselect` exposes + **`.recomputations()`** on memoized selectors — a genuine API, not a patch. Read it (sample + on an interval if the count should visibly climb). An injected `console.log` you added to a + selector body is an authored claim, not an observation; reach for it only when no real API + exists, and say so when you do. *(Note: pr-validate's C4 entry long claimed there was "no + built-in selector-call counter". There is.)* + +5. **Toggle at a fixed commit, not across a merge boundary.** Same tree in both arms, one + thing different. A commit boundary drags in unrelated change you will then be unable to + exclude. When the real commit bundles two changes (a scope change *and* a version bump), + reproduce only the one under test — moving both reintroduces the confound the fixed-commit + design exists to remove, and can silently flip your delivery needle mid-experiment. + +6. **Repeat the capture, not the build; report the band.** Counts vary run to run — one + baseline measured 153/164/224 across three runs. The build dominates cost (~6 min vs ~90 s + per capture), so repeats are nearly free. Publish one artifact; report every repeat's count. + +7. **When the delta is under the spread, say "not resolvable at this n" and give the MDE.** + Not "no effect". State the smallest detectable effect and what n would resolve the observed + difference. A real worked result: 112–128 vs 115–133, delta 4.6%, t=1.33 — with delivery + proven (1244 compiled sites vs 0), so the null was about effect size, not plumbing. + +8. **A check that finds nothing needs a positive control.** Before believing a zero, confirm + the same check finds something it should. A search that returned "0 references" looked like + confirmation until searching for a string known to be present *also* returned 0 — the index + didn't reach that content and the zero meant nothing. + +## Gates, in order + +| gate | asserts | on failure | +|---|---|---| +| source manipulation | the intended edit applied, and *only* it | abort the arm | +| **delivery** | the change reached the built bundle | abort **before any capture** | +| metric | the instrument emitted a non-zero count on capture 1 | abort before spending repeats | + +Each catches what the previous cannot. Source changing is not delivery; delivery is not the +instrument working. Wire them as script-level aborts so a broken arm cannot report a number — +"refusing to emit a render count from an arm whose treatment is unproven" is the correct +output, and it is not a failure of the run. + +**Never relax a gate to make an arm pass.** When a gate fires, go read the artifact and find +the mechanism first. Loosening is the work-reducing direction, which is exactly where scrutiny +collapses. Demoting a needle from gate to diagnostic *after* proving it fires for an unrelated +reason is legitimate; doing it because the arm failed is not. + +## What the count does and does not mean + +WDYR counts **every** re-render in the measured window, including boot settling — not only the +cascade a given fix targeted. So an RCA predicting "→ 0 re-renders" for a specific cascade is +not refuted by a non-zero WDYR total. Say which quantity you measured, and don't let a global +counter stand in for a scoped claim. + +Global application does not imply a large effect: 1244 auto-memoized call sites moved one +interaction's re-render count under 5%. Reach for a flow the change plausibly dominates, and +treat a single flow as a lower bound on reach, not a summary of it. + +## Caveats to publish with the number + +Fixture parity (structurally matched vs byte-identical), arm ordering (randomized or not), +what window the counter covers, and how many flows were measured. State them; they are cheap +and their absence is what makes a number unfalsifiable. + +## Related + +- `pr-validate` — packages this skill's output as its C4 evidence category. +- `memory-leak-hunt`, `supply-chain-audit` — sibling engines behind other categories. From ed3d6ebea516e3d65cd95dd1f0e3e4fec01bdd82 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Thu, 30 Jul 2026 08:41:34 -0400 Subject: [PATCH 04/12] Restore the internal planning-ticket references MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit These name MetaMask-org planning epics and audit tickets. The audience for this repo is the MetaMask org, for whom those identifiers are load-bearing context — they are where the guidance came from and where the follow-up lives. The scrub line is personal references, not org-internal ones. --- .../performance/references/mm-react-compiler-error-triage.md | 4 ++-- .../skills/performance/references/mm-selector-cascade.md | 2 +- .../skills/performance/references/mm-state-normalization.md | 2 +- .../performance/references/mm-useeffect-antipatterns.md | 2 +- .../performance/skills/performance/repos/metamask-mobile.md | 2 +- 5 files changed, 6 insertions(+), 6 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 e726279d..68db17f8 100644 --- a/domains/performance/skills/performance/references/mm-react-compiler-error-triage.md +++ b/domains/performance/skills/performance/references/mm-react-compiler-error-triage.md @@ -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, tracked internally): 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, 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 @@ -63,7 +63,7 @@ What the buckets tell you: ## Staged adoption roadmap -The extension's sequence (internal epic) generalizes to any repo: +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*"`. diff --git a/domains/performance/skills/performance/references/mm-selector-cascade.md b/domains/performance/skills/performance/references/mm-selector-cascade.md index 403f9e5c..387e9cbe 100644 --- a/domains/performance/skills/performance/references/mm-selector-cascade.md +++ b/domains/performance/skills/performance/references/mm-selector-cascade.md @@ -89,7 +89,7 @@ For each hit that consumes the fixed root (directly or transitively): remove the ## 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 (internal audit ticket): +The compiler memoizes **within a file**. A `useSelector` result, an imported hook's return value, or an external context value is opaque to it — if the selector hands back a fresh reference, the compiled component still re-renders, and any derivation from it still recomputes (extension audit ticket MetaMask-planning#6661): ```tsx const tokens = useSelector(selectTokens); // compiler cannot see/stabilize this diff --git a/domains/performance/skills/performance/references/mm-state-normalization.md b/domains/performance/skills/performance/references/mm-state-normalization.md index dc59b349..6a8b0bc1 100644 --- a/domains/performance/skills/performance/references/mm-state-normalization.md +++ b/domains/performance/skills/performance/references/mm-state-normalization.md @@ -11,7 +11,7 @@ tags: redux, normalization, selectors, O(1)-lookups, cache-thrashing, useSelecto > 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 (internal audit tickets), 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. +Selector *memoization* fixes when things recompute; state and selector **shape** fixes how much each recomputation costs and how many subscriptions fire. The patterns here come from the extension performance audit (MetaMask-planning#6580, #6484), where linear scans and reshaping selectors multiplied across power-user data: with 1,000 tokens, 27 `.find()`-based lookups per render is 27,000 comparisons — per render. ## Pattern — O(n) scans where the state shape should provide O(1) lookups diff --git a/domains/performance/skills/performance/references/mm-useeffect-antipatterns.md b/domains/performance/skills/performance/references/mm-useeffect-antipatterns.md index f9eb1fd8..7b24d982 100644 --- a/domains/performance/skills/performance/references/mm-useeffect-antipatterns.md +++ b/domains/performance/skills/performance/references/mm-useeffect-antipatterns.md @@ -76,7 +76,7 @@ useEffect(() => { 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** (internal extension epic): 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. +**Codify, don't copy-paste** (extension epic MetaMask-planning#6525): once a repo has three hand-rolled cancelled flags, extract shared hooks — `useIsMounted()`, `useAbortableEffect(fn, deps)` (effect receives a signal), `useEventListener(target, event, handler)` (auto-removes on unmount) — so cleanup is the default, not per-site diligence. ## Pattern — missing cleanup for timers / subscriptions / listeners diff --git a/domains/performance/skills/performance/repos/metamask-mobile.md b/domains/performance/skills/performance/repos/metamask-mobile.md index e136589d..c22f6162 100644 --- a/domains/performance/skills/performance/repos/metamask-mobile.md +++ b/domains/performance/skills/performance/repos/metamask-mobile.md @@ -109,4 +109,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. Cross-platform React/Redux guidance (`mm-selector-cascade`, `mm-useeffect-antipatterns`, `mm-state-normalization`, `mm-react-compiler-error-triage`) adapted from MetaMask contributor-docs [`frontend-performance.md`](https://github.com/MetaMask/contributor-docs/blob/main/docs/frontend-performance.md) and the extension performance audit (extension PRs metamask-extension#38007, metamask-extension#37147). +Generic React Native references (`js-*`, `native-*`, `bundle-*`) adapted from "The Ultimate Guide to React Native Optimization" by Callstack. MetaMask-specific guidance (`mm-*`) from the internal Performance Guide for Engineers and verified codebase audits. Cross-platform React/Redux guidance (`mm-selector-cascade`, `mm-useeffect-antipatterns`, `mm-state-normalization`, `mm-react-compiler-error-triage`) adapted from MetaMask contributor-docs [`frontend-performance.md`](https://github.com/MetaMask/contributor-docs/blob/main/docs/frontend-performance.md) and the extension performance audit (MetaMask-planning#6571; extension PRs metamask-extension#38007, metamask-extension#37147). From 8f6762ff33a0308c700c52de58e954048222e1b7 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Thu, 30 Jul 2026 13:22:51 -0400 Subject: [PATCH 05/12] Take in the benchmarking skills MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `benchmark-design` and `browser-extension-profiling` are the capture half of the measurement work already here: `data-analysis` turns raw numbers into a defensible before/after, and `react-render-proof` proves a specific change moved work. Both arrived from the platform PR, which shipped them alongside unrelated extension-runtime skills. `benchmark-design` stays in `testing` — that domain already owns benchmark methodology (`performance-testing`) — and brings its `benchmark-statistical-hygiene` knowledge with it. The PR spans two domains because the subject does, not because it is a grab bag. --- .../browser-extension-profiling/skill.md | 65 +++++++++++++++++ .../benchmark-statistical-hygiene.md | 45 ++++++++++++ .../testing/skills/benchmark-design/skill.md | 71 +++++++++++++++++++ 3 files changed, 181 insertions(+) create mode 100644 domains/performance/skills/browser-extension-profiling/skill.md create mode 100644 domains/testing/knowledge/benchmark-statistical-hygiene.md create mode 100644 domains/testing/skills/benchmark-design/skill.md diff --git a/domains/performance/skills/browser-extension-profiling/skill.md b/domains/performance/skills/browser-extension-profiling/skill.md new file mode 100644 index 00000000..d956dea8 --- /dev/null +++ b/domains/performance/skills/browser-extension-profiling/skill.md @@ -0,0 +1,65 @@ +--- +maturity: experimental +name: browser-extension-profiling +description: Compare browser extension performance between branches using WDYR, React DevTools Profiler, and E2E benchmarks with statistical rigor. +--- + +# Browser Extension Profiling + +Methodology for profiling and comparing extension performance across branches or commits. + +## When To Use + +- Validating that a refactor reduces unnecessary re-renders (needs before/after comparison) +- Establishing baseline metrics for a performance initiative +- Investigating a reported UI slowdown in the extension + +## Do Not Use When + +- Single-run comparisons — statistical significance requires ≥10 runs per scenario +- The change touches only non-render paths (background scripts, network with no UI impact) +- Target behavior is server-side latency, not UI rendering + +## Workflow + +1. **Build both branches** with `yarn build:test` on the same machine and Chrome version + +2. **WDYR profiling** (unnecessary re-render counts) + ```bash + ENABLE_WHY_DID_YOU_RENDER=true yarn start + ``` + Flags to watch: + - `different objects that are equal by value` → object recreation + - `different functions with the same name` → callback recreation + - `props object itself changed but values equal` → parent cascade + +3. **React DevTools Profiler** for flame graphs and commit timings + ```bash + yarn devtools:react + ``` + +4. **E2E benchmarks** for scenario durations + ```bash + yarn test:e2e:benchmark + ``` + +5. **Collect ≥10 runs** per scenario. Discard top/bottom 10%. Report mean, median, stddev, p75, p95. + +6. **Statistical threshold:** Cohen's d > 0.5 for a meaningful difference. + +## Common Pitfalls + +| Mistake | Correct Approach | +|---------|-----------------| +| Running branches on different machines or Chrome versions | Same machine, same Chrome, no other apps running | +| Pooling all runs including noisy late-session ones | Compute per-round stats first; report cleanest signal with explicit round attribution | +| Reporting absolute re-render counts without scenario context | Normalize per-action; cascade fixes show multiplied impact at root | +| Skipping cache and state reset between runs | Clear browser cache, reset extension state for each run | + +## Pre-Profiling Checklist + +- [ ] Both branches built with `yarn build:test` +- [ ] Same machine, same Chrome version +- [ ] No other tabs or applications running +- [ ] WDYR enabled: `ENABLE_WHY_DID_YOU_RENDER=true` +- [ ] Cache and extension state cleared between runs diff --git a/domains/testing/knowledge/benchmark-statistical-hygiene.md b/domains/testing/knowledge/benchmark-statistical-hygiene.md new file mode 100644 index 00000000..7015bbc5 --- /dev/null +++ b/domains/testing/knowledge/benchmark-statistical-hygiene.md @@ -0,0 +1,45 @@ +--- +name: benchmark-statistical-hygiene +domain: testing +description: Three patterns for defensible A/B benchmark results: per-round best-subset reporting, fix-vector isolation, and artifact sort-order trap. +--- + +# Benchmark Statistical Hygiene + +Three patterns that prevent the most common classes of invalid benchmark conclusions. + +## Pattern: Per-Round Best-Subset Reporting + +Later benchmark rounds accumulate system noise (background load, memory pressure, I/O contention). Pooling all rounds blindly treats noisy late-session data equally with clean early-session data. + +**Instead:** Compute per-round statistics first, then report the cleanest signal per metric with explicit round attribution. + +``` +Round 1 (clean): metric X → treatment wins, p=0.04, d=-1.7 +Round 2 (moderate): metric X → treatment wins, p=0.08, d=-0.9 +Round 3 (noisy): metric X → no effect, p=0.90, d=+0.04 + +Pooled (all): metric X → no effect, p=0.50, d=-0.2 ← signal destroyed + +Correct report: "X improved 49% (Round 1, n=5, p=0.04, d=-1.7). + Pooled n=20 loses significance due to Round 3 outliers." +``` + +A small N with large effect size (|d| > 1.5, p < 0.05) is more defensible than a large N where noise has diluted significance to nothing. + +## Pattern: Isolate the Fix Vector + +Design each benchmark flow to exercise the optimization's specific input vector as its primary signal source. Incidental coverage produces fragile results where signal-to-noise depends on how much of the measured duration is optimization-affected. + +| | Weak | Strong | +|-|------|--------| +| Design | End-to-end flow that incidentally triggers target once among many other operations | Rapid sequence of actions each triggering the target with minimal other overhead | +| Optimization signal | ~5% of measured duration | ~80% of measured duration | + +## Pattern: Artifact Sort-Order Trap + +Unpadded iteration numbers in filenames break lexicographic sorting: `iteration-1, iteration-10, iteration-2, ...` interleaves data from different rounds when processed in glob order. + +**Rule:** When processing sequentially-numbered artifacts, extract the embedded timestamp or numeric value for sorting. Never rely on string sort order when numbers cross digit boundaries. + +**Diagnosis:** If pipeline results look implausible (p-values that are too perfect, round-level stats that don't match spot checks), print the actual file ordering the pipeline used. Check for lexicographic interleaving at digit boundaries. Re-sort by extracted timestamp or zero-padded key. diff --git a/domains/testing/skills/benchmark-design/skill.md b/domains/testing/skills/benchmark-design/skill.md new file mode 100644 index 00000000..578691df --- /dev/null +++ b/domains/testing/skills/benchmark-design/skill.md @@ -0,0 +1,71 @@ +--- +maturity: experimental +name: benchmark-design +description: Design, run, and analyze E2E performance benchmarks — session hygiene, per-round reporting, artifact grouping +--- + +# Benchmark Design + +## When To Use + +- Writing a new E2E benchmark flow +- Interpreting or presenting benchmark results +- Adding new metrics to existing benchmarks +- Diagnosing unexpected benchmark results + +## Do Not Use When + +- Adding unit, integration, or correctness E2E tests +- Profiling a single user-reported slowdown (use `selector-anti-pattern-review`) +- Writing micro-benchmarks outside the E2E harness + +## Workflow + +1. **Design the flow** — target ONE optimization vector per benchmark. Maximize ratio of optimization-affected time to total measured time. +2. **Run reference benchmarks first** in any session — session state degrades over time. +3. **Compute per-round statistics** before pooling. Check each round for stability (CV < 0.3 is a reasonable threshold). +4. **Group artifacts by timestamp**, not filename sort order. +5. **Report per-metric best subset** with explicit round attribution. Show pooled data as supplementary. + +## Flow Design by Optimization Type + +| Optimization | Primary cascade vector | Recommended flow | +|---|---|---| +| Selector memoization | State mutations | Multi-confirmation queue | +| Context memoization | Any state update | Account switching cycle | +| HOC stabilization | Route changes | Rapid route cycling (8+ transitions) | +| Dead code removal | Navigation | Return-to-home timer | + +## Session Hygiene + +System state degrades over long sessions — background load and memory pressure inflate variance and can **invert** treatment effects. + +- Run reference/critical benchmarks first +- If a late round contradicts clean earlier rounds, suspect session degradation before re-running the full suite + +## Artifact Grouping + +Filenames use `{test}-iteration-{N}-{ISO-timestamp}.json`. Unpadded N produces incorrect lexicographic sort. + +```javascript +// Extract seconds-since-midnight for round assignment +const match = filename.match(/T(\d{2})-(\d{2})-(\d{2})/); +const secondsOfDay = +match[1] * 3600 + +match[2] * 60 + +match[3]; +// Group by time range — never by filename position or array index +``` + +## Adding Metrics + +Extend `collectMetrics()` in `test/e2e/webdriver/driver.js` and register the metric key in `test/e2e/benchmarks/utils/constants.ts` → `ALL_METRICS`. + +- **Performance API metrics** (paint, navigation timing): collect directly inside `collectMetrics()` via `window.performance.getEntriesByType(...)`. +- **Long Task / TBT metrics**: already wired — `collectMetrics()` reads `window.stateHooks.getLongTaskMetricsWithTBT()`. Adding new long-task-derived metrics requires extending the `stateHooks` observer, not the driver. + +## Common Pitfalls + +| Mistake | Correct Approach | +|---------|-----------------| +| Pool all rounds before checking per-round stats | Per-round first — late-session noise can invert the treatment effect | +| Sort artifacts by filename | Extract ISO timestamp; sort by numeric time value | +| Benchmark flow that exercises multiple vectors | One vector per flow — mixed flows produce ambiguous signal | +| Report pooled p-value as primary result | Report cleanest per-metric signal with round attribution; pooled is supplementary | From b576c7ec1b3d6e79de2ab5a984b389f9a170ad08 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Thu, 30 Jul 2026 13:41:40 -0400 Subject: [PATCH 06/12] Point the extension overlays at `main`, not `develop` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `metamask-extension` moved its default branch to `main`; `develop` still exists but its last commit is 2026-01-15, so six links in the extension overlays resolved to code roughly six months stale. They loaded, which is why nothing caught it — a frozen branch is worse than a dead one here, since the reader gets plausible but outdated source. All five cited paths verified present on `main` (HTTP 200): `ui/`, `ui/hooks/`, `ui/selectors/`, `shared/lib/selectors/selector-creators.ts`, and `app/scripts/metamask-controller.js`. --- .../repos/metamask-extension.md | 4 ++-- .../repos/metamask-extension.md | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/domains/performance/skills/effect-anti-pattern-review/repos/metamask-extension.md b/domains/performance/skills/effect-anti-pattern-review/repos/metamask-extension.md index 67d41395..532b74c6 100644 --- a/domains/performance/skills/effect-anti-pattern-review/repos/metamask-extension.md +++ b/domains/performance/skills/effect-anti-pattern-review/repos/metamask-extension.md @@ -5,8 +5,8 @@ parent: effect-anti-pattern-review ## Paths -- Component sources: [`ui/`](https://github.com/MetaMask/metamask-extension/tree/develop/ui) -- Shared hooks: [`ui/hooks/`](https://github.com/MetaMask/metamask-extension/tree/develop/ui/hooks) +- 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 diff --git a/domains/performance/skills/selector-anti-pattern-review/repos/metamask-extension.md b/domains/performance/skills/selector-anti-pattern-review/repos/metamask-extension.md index aafb8e9d..b3ca4992 100644 --- a/domains/performance/skills/selector-anti-pattern-review/repos/metamask-extension.md +++ b/domains/performance/skills/selector-anti-pattern-review/repos/metamask-extension.md @@ -5,10 +5,10 @@ parent: selector-anti-pattern-review ## Paths -- Selector definitions: [`ui/selectors/`](https://github.com/MetaMask/metamask-extension/tree/develop/ui/selectors) -- Selector creators: [`shared/lib/selectors/selector-creators.ts`](https://github.com/MetaMask/metamask-extension/blob/develop/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/develop/app/scripts/metamask-controller.js) -- Component consumption sites: anywhere under [`ui/`](https://github.com/MetaMask/metamask-extension/tree/develop/ui) that calls `useSelector` +- 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 From 6e15178ae445de7616ab29beda77bfa6d8b66ea4 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Fri, 31 Jul 2026 08:30:33 -0400 Subject: [PATCH 07/12] Shorten and normalise skill names `browser-extension-profiling` drops `browser-`, which distinguishes nothing: an extension is a browser extension, and the `extension-` half is what separates it from the mobile work this domain also covers. `anti-pattern` loses its hyphen in identifiers, matching what `main` already ships in `review-antipatterns.md` and `mm-redux-antipatterns.md`. Both skills and both knowledge files move together, since a skill and its knowledge sharing a stem is what makes the by-name citation convention resolvable. Prose inside the two renamed skills is normalised with them so each file agrees with its own name; hyphenated prose elsewhere is left alone as pre-existing and outside this change. --- ...ect-anti-patterns.md => effect-antipatterns.md} | 4 ++-- ...r-anti-patterns.md => selector-antipatterns.md} | 2 +- .../repos/metamask-extension.md | 2 +- .../repos/metamask-mobile.md | 2 +- .../skill.md | 12 ++++++------ .../skill.md | 2 +- .../references/mm-hook-dependency-arrays.md | 2 +- .../performance/references/mm-selector-cascade.md | 2 +- .../references/mm-selector-memoization.md | 2 +- .../references/mm-state-normalization.md | 2 +- .../references/mm-useeffect-antipatterns.md | 2 +- .../repos/metamask-extension.md | 2 +- .../repos/metamask-mobile.md | 2 +- .../skill.md | 14 +++++++------- domains/testing/skills/benchmark-design/skill.md | 2 +- 15 files changed, 27 insertions(+), 27 deletions(-) rename domains/performance/knowledge/{effect-anti-patterns.md => effect-antipatterns.md} (97%) rename domains/performance/knowledge/{selector-anti-patterns.md => selector-antipatterns.md} (99%) rename domains/performance/skills/{effect-anti-pattern-review => effect-antipattern-review}/repos/metamask-extension.md (96%) rename domains/performance/skills/{effect-anti-pattern-review => effect-antipattern-review}/repos/metamask-mobile.md (96%) rename domains/performance/skills/{effect-anti-pattern-review => effect-antipattern-review}/skill.md (87%) rename domains/performance/skills/{browser-extension-profiling => extension-profiling}/skill.md (98%) rename domains/performance/skills/{selector-anti-pattern-review => selector-antipattern-review}/repos/metamask-extension.md (98%) rename domains/performance/skills/{selector-anti-pattern-review => selector-antipattern-review}/repos/metamask-mobile.md (97%) rename domains/performance/skills/{selector-anti-pattern-review => selector-antipattern-review}/skill.md (87%) diff --git a/domains/performance/knowledge/effect-anti-patterns.md b/domains/performance/knowledge/effect-antipatterns.md similarity index 97% rename from domains/performance/knowledge/effect-anti-patterns.md rename to domains/performance/knowledge/effect-antipatterns.md index 3b74afe0..2b24006d 100644 --- a/domains/performance/knowledge/effect-anti-patterns.md +++ b/domains/performance/knowledge/effect-antipatterns.md @@ -1,5 +1,5 @@ --- -name: effect-anti-patterns +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 --- @@ -145,5 +145,5 @@ Pick one and apply it consistently. ## Related - `render-cascade` — how effect-driven re-renders propagate through the component graph. -- `selector-anti-patterns` — the store-side counterpart; an unstable selector result is a +- `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/selector-anti-patterns.md b/domains/performance/knowledge/selector-antipatterns.md similarity index 99% rename from domains/performance/knowledge/selector-anti-patterns.md rename to domains/performance/knowledge/selector-antipatterns.md index 8bf32e9f..df65609f 100644 --- a/domains/performance/knowledge/selector-anti-patterns.md +++ b/domains/performance/knowledge/selector-antipatterns.md @@ -1,5 +1,5 @@ --- -name: selector-anti-patterns +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 --- diff --git a/domains/performance/skills/effect-anti-pattern-review/repos/metamask-extension.md b/domains/performance/skills/effect-antipattern-review/repos/metamask-extension.md similarity index 96% rename from domains/performance/skills/effect-anti-pattern-review/repos/metamask-extension.md rename to domains/performance/skills/effect-antipattern-review/repos/metamask-extension.md index 532b74c6..af9b4298 100644 --- a/domains/performance/skills/effect-anti-pattern-review/repos/metamask-extension.md +++ b/domains/performance/skills/effect-antipattern-review/repos/metamask-extension.md @@ -1,6 +1,6 @@ --- repo: metamask-extension -parent: effect-anti-pattern-review +parent: effect-antipattern-review --- ## Paths diff --git a/domains/performance/skills/effect-anti-pattern-review/repos/metamask-mobile.md b/domains/performance/skills/effect-antipattern-review/repos/metamask-mobile.md similarity index 96% rename from domains/performance/skills/effect-anti-pattern-review/repos/metamask-mobile.md rename to domains/performance/skills/effect-antipattern-review/repos/metamask-mobile.md index dde98072..6f19aaed 100644 --- a/domains/performance/skills/effect-anti-pattern-review/repos/metamask-mobile.md +++ b/domains/performance/skills/effect-antipattern-review/repos/metamask-mobile.md @@ -1,6 +1,6 @@ --- repo: metamask-mobile -parent: effect-anti-pattern-review +parent: effect-antipattern-review --- ## Paths diff --git a/domains/performance/skills/effect-anti-pattern-review/skill.md b/domains/performance/skills/effect-antipattern-review/skill.md similarity index 87% rename from domains/performance/skills/effect-anti-pattern-review/skill.md rename to domains/performance/skills/effect-antipattern-review/skill.md index 1ecc8d86..5f2ab763 100644 --- a/domains/performance/skills/effect-anti-pattern-review/skill.md +++ b/domains/performance/skills/effect-antipattern-review/skill.md @@ -1,12 +1,12 @@ --- maturity: experimental -name: effect-anti-pattern-review -description: Review PR diffs that add or modify `useEffect` for the systemic React effect anti-patterns +name: effect-antipattern-review +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-anti-patterns`** knowledge file, which is the single source for their definitions and fixes (installed alongside this skill under `knowledge/`). +**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. @@ -18,7 +18,7 @@ Applies to both `metamask-extension` and `metamask-mobile`. See overlays for rep ## Do Not Use When -- Reviewing selector or render-cascade issues (use [`selector-anti-pattern-review`](../selector-anti-pattern-review/skill.md)) +- Reviewing selector or render-cascade issues (use [`selector-antipattern-review`](../selector-antipattern-review/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) @@ -26,14 +26,14 @@ Applies to both `metamask-extension` and `metamask-mobile`. See overlays for rep 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-anti-patterns` and apply the fix from the knowledge file. +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.** `JSON.stringify` in a dependency array is always broken. Do not merge. 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 use `AbortController`. ## Grep Checklist -| Pattern (`effect-anti-patterns` §) | Detection | +| 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 | diff --git a/domains/performance/skills/browser-extension-profiling/skill.md b/domains/performance/skills/extension-profiling/skill.md similarity index 98% rename from domains/performance/skills/browser-extension-profiling/skill.md rename to domains/performance/skills/extension-profiling/skill.md index d956dea8..c2a80272 100644 --- a/domains/performance/skills/browser-extension-profiling/skill.md +++ b/domains/performance/skills/extension-profiling/skill.md @@ -1,6 +1,6 @@ --- maturity: experimental -name: browser-extension-profiling +name: extension-profiling description: Compare browser extension performance between branches using WDYR, React DevTools Profiler, and E2E benchmarks with statistical rigor. --- 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 5ff27dc0..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,7 +8,7 @@ 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-anti-patterns`** 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. +> **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 diff --git a/domains/performance/skills/performance/references/mm-selector-cascade.md b/domains/performance/skills/performance/references/mm-selector-cascade.md index 387e9cbe..5892e599 100644 --- a/domains/performance/skills/performance/references/mm-selector-cascade.md +++ b/domains/performance/skills/performance/references/mm-selector-cascade.md @@ -8,7 +8,7 @@ tags: reselect, cascade, dependency-graph, isEqual, structural-sharing, react-co > **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-anti-patterns`** — both installed alongside this skill under `knowledge/`. +> **`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. diff --git a/domains/performance/skills/performance/references/mm-selector-memoization.md b/domains/performance/skills/performance/references/mm-selector-memoization.md index c6731a62..414c48b0 100644 --- a/domains/performance/skills/performance/references/mm-selector-memoization.md +++ b/domains/performance/skills/performance/references/mm-selector-memoization.md @@ -15,7 +15,7 @@ Broken or absent memoization in widely-used selectors is the single highest-impa ## The patterns, and what they look like here -The pattern taxonomy itself lives in the **`selector-anti-patterns`** knowledge file, +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. diff --git a/domains/performance/skills/performance/references/mm-state-normalization.md b/domains/performance/skills/performance/references/mm-state-normalization.md index 6a8b0bc1..0bcaa57c 100644 --- a/domains/performance/skills/performance/references/mm-state-normalization.md +++ b/domains/performance/skills/performance/references/mm-state-normalization.md @@ -6,7 +6,7 @@ tags: redux, normalization, selectors, O(1)-lookups, cache-thrashing, useSelecto # Skill: State Normalization & Selector Shape -> **Scope.** The generic form of the O(n)-lookup problem is `selector-anti-patterns` §7, +> **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. diff --git a/domains/performance/skills/performance/references/mm-useeffect-antipatterns.md b/domains/performance/skills/performance/references/mm-useeffect-antipatterns.md index 7b24d982..ad54d055 100644 --- a/domains/performance/skills/performance/references/mm-useeffect-antipatterns.md +++ b/domains/performance/skills/performance/references/mm-useeffect-antipatterns.md @@ -8,7 +8,7 @@ tags: useEffect, setState, cleanup, AbortController, unmount, memory-leaks > **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-anti-patterns`** knowledge file, +> 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. diff --git a/domains/performance/skills/selector-anti-pattern-review/repos/metamask-extension.md b/domains/performance/skills/selector-antipattern-review/repos/metamask-extension.md similarity index 98% rename from domains/performance/skills/selector-anti-pattern-review/repos/metamask-extension.md rename to domains/performance/skills/selector-antipattern-review/repos/metamask-extension.md index b3ca4992..81dceb6b 100644 --- a/domains/performance/skills/selector-anti-pattern-review/repos/metamask-extension.md +++ b/domains/performance/skills/selector-antipattern-review/repos/metamask-extension.md @@ -1,6 +1,6 @@ --- repo: metamask-extension -parent: selector-anti-pattern-review +parent: selector-antipattern-review --- ## Paths diff --git a/domains/performance/skills/selector-anti-pattern-review/repos/metamask-mobile.md b/domains/performance/skills/selector-antipattern-review/repos/metamask-mobile.md similarity index 97% rename from domains/performance/skills/selector-anti-pattern-review/repos/metamask-mobile.md rename to domains/performance/skills/selector-antipattern-review/repos/metamask-mobile.md index cb28ddab..b77ab8c6 100644 --- a/domains/performance/skills/selector-anti-pattern-review/repos/metamask-mobile.md +++ b/domains/performance/skills/selector-antipattern-review/repos/metamask-mobile.md @@ -1,6 +1,6 @@ --- repo: metamask-mobile -parent: selector-anti-pattern-review +parent: selector-antipattern-review --- ## Paths diff --git a/domains/performance/skills/selector-anti-pattern-review/skill.md b/domains/performance/skills/selector-antipattern-review/skill.md similarity index 87% rename from domains/performance/skills/selector-anti-pattern-review/skill.md rename to domains/performance/skills/selector-antipattern-review/skill.md index 0b541d08..a98d65f5 100644 --- a/domains/performance/skills/selector-anti-pattern-review/skill.md +++ b/domains/performance/skills/selector-antipattern-review/skill.md @@ -1,12 +1,12 @@ --- maturity: experimental -name: selector-anti-pattern-review -description: Review and diagnose Redux selector anti-patterns that cause render cascades, pre-merge and post-merge +name: selector-antipattern-review +description: Review and diagnose Redux selector antipatterns that cause render cascades, pre-merge and post-merge --- # Selector Anti-Pattern Review -**Scope:** Redux selector anti-patterns 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-anti-patterns`** and **`render-cascade`** knowledge files — the single source for their definitions (installed alongside this skill under `knowledge/`). +**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). @@ -18,7 +18,7 @@ Both `metamask-extension` and `metamask-mobile` share the same React + Redux arc ## Do Not Use When -- Non-selector performance concerns (effects → use `effect-anti-pattern-review`, context providers, virtualization) +- Non-selector performance concerns (effects → use `effect-antipattern-review`, 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) @@ -27,8 +27,8 @@ Both `metamask-extension` and `metamask-mobile` share the same React + Redux arc 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-anti-patterns` 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-anti-patterns` §2). Do not merge. +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 five patterns have a valid use case. See [Pitfalls](#common-pitfalls) for the narrow `createDeepEqualSelector` exception. ## Mode B: Post-Merge Diagnosis (WDYR-driven) @@ -46,7 +46,7 @@ Both `metamask-extension` and `metamask-mobile` share the same React + Redux arc ## Grep Checklist -| Pattern (`selector-anti-patterns` §) | Detection | +| Pattern (`selector-antipatterns` §) | Detection | |---|---| | §1 Unmemoized selector | `grep -rE 'export function get' /` | | §2 Identity / passthrough result | Jest warning `result function returned its own inputs` | diff --git a/domains/testing/skills/benchmark-design/skill.md b/domains/testing/skills/benchmark-design/skill.md index 578691df..641ef6c5 100644 --- a/domains/testing/skills/benchmark-design/skill.md +++ b/domains/testing/skills/benchmark-design/skill.md @@ -16,7 +16,7 @@ description: Design, run, and analyze E2E performance benchmarks — session hyg ## Do Not Use When - Adding unit, integration, or correctness E2E tests -- Profiling a single user-reported slowdown (use `selector-anti-pattern-review`) +- Profiling a single user-reported slowdown (use `selector-antipattern-review`) - Writing micro-benchmarks outside the E2E harness ## Workflow From 14196b2d9fe6500a7934de93db0aaec17d88f0b6 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Fri, 31 Jul 2026 09:40:05 -0400 Subject: [PATCH 08/12] Rename the antipattern skills from `-review` to `-scan` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `scan` says what they do. Both walk a diff looking for a known set of shapes and report what they find; `review` implied a judgement they do not make and overlapped with the correctness review these deliberately are not. The suffix still carries its original job of keeping each skill distinct from the knowledge file it cites — `selector-antipatterns.md` and `effect-antipatterns.md` — which the by-name citation resolver needs, since it matches on filename. Installed as `mms-selector-antipattern-scan` and `mms-effect-antipattern-scan`. --- .../repos/metamask-extension.md | 2 +- .../repos/metamask-mobile.md | 2 +- .../skill.md | 4 ++-- .../repos/metamask-extension.md | 2 +- .../repos/metamask-mobile.md | 2 +- .../skill.md | 4 ++-- domains/testing/skills/benchmark-design/skill.md | 2 +- 7 files changed, 9 insertions(+), 9 deletions(-) rename domains/performance/skills/{effect-antipattern-review => effect-antipattern-scan}/repos/metamask-extension.md (96%) rename domains/performance/skills/{effect-antipattern-review => effect-antipattern-scan}/repos/metamask-mobile.md (96%) rename domains/performance/skills/{effect-antipattern-review => effect-antipattern-scan}/skill.md (97%) rename domains/performance/skills/{selector-antipattern-review => selector-antipattern-scan}/repos/metamask-extension.md (98%) rename domains/performance/skills/{selector-antipattern-review => selector-antipattern-scan}/repos/metamask-mobile.md (97%) rename domains/performance/skills/{selector-antipattern-review => selector-antipattern-scan}/skill.md (98%) diff --git a/domains/performance/skills/effect-antipattern-review/repos/metamask-extension.md b/domains/performance/skills/effect-antipattern-scan/repos/metamask-extension.md similarity index 96% rename from domains/performance/skills/effect-antipattern-review/repos/metamask-extension.md rename to domains/performance/skills/effect-antipattern-scan/repos/metamask-extension.md index af9b4298..ee5fb3b2 100644 --- a/domains/performance/skills/effect-antipattern-review/repos/metamask-extension.md +++ b/domains/performance/skills/effect-antipattern-scan/repos/metamask-extension.md @@ -1,6 +1,6 @@ --- repo: metamask-extension -parent: effect-antipattern-review +parent: effect-antipattern-scan --- ## Paths diff --git a/domains/performance/skills/effect-antipattern-review/repos/metamask-mobile.md b/domains/performance/skills/effect-antipattern-scan/repos/metamask-mobile.md similarity index 96% rename from domains/performance/skills/effect-antipattern-review/repos/metamask-mobile.md rename to domains/performance/skills/effect-antipattern-scan/repos/metamask-mobile.md index 6f19aaed..b005c307 100644 --- a/domains/performance/skills/effect-antipattern-review/repos/metamask-mobile.md +++ b/domains/performance/skills/effect-antipattern-scan/repos/metamask-mobile.md @@ -1,6 +1,6 @@ --- repo: metamask-mobile -parent: effect-antipattern-review +parent: effect-antipattern-scan --- ## Paths diff --git a/domains/performance/skills/effect-antipattern-review/skill.md b/domains/performance/skills/effect-antipattern-scan/skill.md similarity index 97% rename from domains/performance/skills/effect-antipattern-review/skill.md rename to domains/performance/skills/effect-antipattern-scan/skill.md index 5f2ab763..f890fcc8 100644 --- a/domains/performance/skills/effect-antipattern-review/skill.md +++ b/domains/performance/skills/effect-antipattern-scan/skill.md @@ -1,6 +1,6 @@ --- maturity: experimental -name: effect-antipattern-review +name: effect-antipattern-scan description: Review PR diffs that add or modify `useEffect` for the systemic React effect antipatterns --- @@ -18,7 +18,7 @@ Applies to both `metamask-extension` and `metamask-mobile`. See overlays for rep ## Do Not Use When -- Reviewing selector or render-cascade issues (use [`selector-antipattern-review`](../selector-antipattern-review/skill.md)) +- 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) diff --git a/domains/performance/skills/selector-antipattern-review/repos/metamask-extension.md b/domains/performance/skills/selector-antipattern-scan/repos/metamask-extension.md similarity index 98% rename from domains/performance/skills/selector-antipattern-review/repos/metamask-extension.md rename to domains/performance/skills/selector-antipattern-scan/repos/metamask-extension.md index 81dceb6b..82801a08 100644 --- a/domains/performance/skills/selector-antipattern-review/repos/metamask-extension.md +++ b/domains/performance/skills/selector-antipattern-scan/repos/metamask-extension.md @@ -1,6 +1,6 @@ --- repo: metamask-extension -parent: selector-antipattern-review +parent: selector-antipattern-scan --- ## Paths diff --git a/domains/performance/skills/selector-antipattern-review/repos/metamask-mobile.md b/domains/performance/skills/selector-antipattern-scan/repos/metamask-mobile.md similarity index 97% rename from domains/performance/skills/selector-antipattern-review/repos/metamask-mobile.md rename to domains/performance/skills/selector-antipattern-scan/repos/metamask-mobile.md index b77ab8c6..1b324e3a 100644 --- a/domains/performance/skills/selector-antipattern-review/repos/metamask-mobile.md +++ b/domains/performance/skills/selector-antipattern-scan/repos/metamask-mobile.md @@ -1,6 +1,6 @@ --- repo: metamask-mobile -parent: selector-antipattern-review +parent: selector-antipattern-scan --- ## Paths diff --git a/domains/performance/skills/selector-antipattern-review/skill.md b/domains/performance/skills/selector-antipattern-scan/skill.md similarity index 98% rename from domains/performance/skills/selector-antipattern-review/skill.md rename to domains/performance/skills/selector-antipattern-scan/skill.md index a98d65f5..be3b81fe 100644 --- a/domains/performance/skills/selector-antipattern-review/skill.md +++ b/domains/performance/skills/selector-antipattern-scan/skill.md @@ -1,6 +1,6 @@ --- maturity: experimental -name: selector-antipattern-review +name: selector-antipattern-scan description: Review and diagnose Redux selector antipatterns that cause render cascades, pre-merge and post-merge --- @@ -18,7 +18,7 @@ Both `metamask-extension` and `metamask-mobile` share the same React + Redux arc ## Do Not Use When -- Non-selector performance concerns (effects → use `effect-antipattern-review`, context providers, virtualization) +- 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) diff --git a/domains/testing/skills/benchmark-design/skill.md b/domains/testing/skills/benchmark-design/skill.md index 641ef6c5..5f343006 100644 --- a/domains/testing/skills/benchmark-design/skill.md +++ b/domains/testing/skills/benchmark-design/skill.md @@ -16,7 +16,7 @@ description: Design, run, and analyze E2E performance benchmarks — session hyg ## Do Not Use When - Adding unit, integration, or correctness E2E tests -- Profiling a single user-reported slowdown (use `selector-antipattern-review`) +- Profiling a single user-reported slowdown (use `selector-antipattern-scan`) - Writing micro-benchmarks outside the E2E harness ## Workflow From 843a49ed99226bc2bb19194241bebf27b5081808 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Fri, 31 Jul 2026 10:26:02 -0400 Subject: [PATCH 09/12] Name the evidence category in `react-render-proof` instead of indexing it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `C4` is an address into `evidence-catalog.md`. A reader who has not opened the catalog cannot resolve it, and the frontmatter `description` cannot link out to one. Both sites now name the category and link the catalog by URL — a relative path would not survive installation, which flattens skills to `mms-/`. Also updates two sibling names that no longer resolve: `pr-validate` is now `evidence`, and `memory-leak-hunt` is now `memory-leak`. --- domains/performance/skills/react-render-proof/skill.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/domains/performance/skills/react-render-proof/skill.md b/domains/performance/skills/react-render-proof/skill.md index 4cb4fa2c..3f613696 100644 --- a/domains/performance/skills/react-render-proof/skill.md +++ b/domains/performance/skills/react-render-proof/skill.md @@ -1,6 +1,6 @@ --- name: react-render-proof -description: Prove a React rendering or memoization change actually reduced work, with a delivery gate and a reported band. Covers re-render counts (why-did-you-render), selector recomputes (reselect's real `.recomputations()` API), and A/B arms toggled at a FIXED commit rather than across a merge boundary. The falsifier is an arm whose treatment never reached the built bundle — a null from undelivered treatment is indistinguishable from a null from a small effect and reports as the second. Triggers on /react-render-proof, or when asked to prove a component stopped over-rendering, measure selector recomputation, validate a memoization/React Compiler change, run a render-count A/B, or interpret a re-render benchmark. Callable by pr-validate as the engine behind its React render & selector proof evidence category. +description: Prove a React rendering or memoization change actually reduced work, with a delivery gate and a reported band. Covers re-render counts (why-did-you-render), selector recomputes (reselect's real `.recomputations()` API), and A/B arms toggled at a FIXED commit rather than across a merge boundary. The falsifier is an arm whose treatment never reached the built bundle — a null from undelivered treatment is indistinguishable from a null from a small effect and reports as the second. Triggers on /react-render-proof, or when asked to prove a component stopped over-rendering, measure selector recomputation, validate a memoization/React Compiler change, run a render-count A/B, or interpret a re-render benchmark. Callable by `evidence` as its React render & selector proof engine. maturity: experimental --- @@ -48,8 +48,8 @@ reporting a difference between arms that never differed. **`.recomputations()`** on memoized selectors — a genuine API, not a patch. Read it (sample on an interval if the count should visibly climb). An injected `console.log` you added to a selector body is an authored claim, not an observation; reach for it only when no real API - exists, and say so when you do. *(Note: pr-validate's C4 entry long claimed there was "no - built-in selector-call counter". There is.)* + exists, and say so when you do. *(Note: the evidence catalog's render-and-selector entry long claimed there + was "no built-in selector-call counter". There is.)* 5. **Toggle at a fixed commit, not across a merge boundary.** Same tree in both arms, one thing different. A commit boundary drags in unrelated change you will then be unable to @@ -108,5 +108,5 @@ and their absence is what makes a number unfalsifiable. ## Related -- `pr-validate` — packages this skill's output as its C4 evidence category. -- `memory-leak-hunt`, `supply-chain-audit` — sibling engines behind other categories. +- `evidence` — packages this skill's output as its [React render & selector proof category](https://github.com/MetaMask/skills/blob/main/domains/pr-workflow/skills/evidence/references/evidence-catalog.md). +- `memory-leak`, `supply-chain-audit` — sibling engines behind other categories. From c18c751a79157d2cbfe3a3de69d07a94a8396548 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Fri, 31 Jul 2026 11:53:26 -0400 Subject: [PATCH 10/12] Rename `react-render-proof` to `react-render-delta` `-proof` as a noun suffix reads as "immune to", so the old name parsed as "immune to React renders". `-delta` names what the skill actually produces, and matches the skill's own insistence that its output is a measured quantity rather than a boolean. --- .../{react-render-proof => react-render-delta}/skill.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) rename domains/performance/skills/{react-render-proof => react-render-delta}/skill.md (98%) diff --git a/domains/performance/skills/react-render-proof/skill.md b/domains/performance/skills/react-render-delta/skill.md similarity index 98% rename from domains/performance/skills/react-render-proof/skill.md rename to domains/performance/skills/react-render-delta/skill.md index 3f613696..919c17c3 100644 --- a/domains/performance/skills/react-render-proof/skill.md +++ b/domains/performance/skills/react-render-delta/skill.md @@ -1,10 +1,10 @@ --- -name: react-render-proof -description: Prove a React rendering or memoization change actually reduced work, with a delivery gate and a reported band. Covers re-render counts (why-did-you-render), selector recomputes (reselect's real `.recomputations()` API), and A/B arms toggled at a FIXED commit rather than across a merge boundary. The falsifier is an arm whose treatment never reached the built bundle — a null from undelivered treatment is indistinguishable from a null from a small effect and reports as the second. Triggers on /react-render-proof, or when asked to prove a component stopped over-rendering, measure selector recomputation, validate a memoization/React Compiler change, run a render-count A/B, or interpret a re-render benchmark. Callable by `evidence` as its React render & selector proof engine. +name: react-render-delta +description: Prove a React rendering or memoization change actually reduced work, with a delivery gate and a reported band. Covers re-render counts (why-did-you-render), selector recomputes (reselect's real `.recomputations()` API), and A/B arms toggled at a FIXED commit rather than across a merge boundary. The falsifier is an arm whose treatment never reached the built bundle — a null from undelivered treatment is indistinguishable from a null from a small effect and reports as the second. Triggers on /react-render-delta, or when asked to prove a component stopped over-rendering, measure selector recomputation, validate a memoization/React Compiler change, run a render-count A/B, or interpret a re-render benchmark. Callable by `evidence` as its React render & selector proof engine. maturity: experimental --- -# /react-render-proof +# /react-render-delta A render-count number is worthless until two things are true: the **treatment reached the artifact the browser executes**, and the number is reported as a **band** rather than a point. From 7b3eeaa04a60a739306621ed417cba5407c1f95d Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Fri, 31 Jul 2026 20:06:07 -0400 Subject: [PATCH 11/12] =?UTF-8?q?Widen=20the=20=C2=A73=20grep=20to=20catch?= =?UTF-8?q?=20result=20functions=20returning=20fresh=20literals?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by running the skill against real merged PRs: the §3 detection matched only named collection constructors, so a result function returning an object literal directly went undetected. `(metamask) => ({ userRegion: ..., ... })` builds a new object on every recompute and matches none of `new Set`, `new Map`, `Object.values`, `?? {}`, or `?? []`. Adds `=> ({` and `=> [` as alternates, with the reason recorded beside the table so the next person does not narrow it again. --- domains/performance/skills/selector-antipattern-scan/skill.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/domains/performance/skills/selector-antipattern-scan/skill.md b/domains/performance/skills/selector-antipattern-scan/skill.md index be3b81fe..ee32d7d7 100644 --- a/domains/performance/skills/selector-antipattern-scan/skill.md +++ b/domains/performance/skills/selector-antipattern-scan/skill.md @@ -50,13 +50,15 @@ Both `metamask-extension` and `metamask-mobile` share the same React + Redux arc |---|---| | §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)\|\?\? \{\}\|\?\? \[\]' /` | +| §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 From 34b935c953dda2cb416629ee3a2f81b4146e8207 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Tue, 4 Aug 2026 08:00:47 -0400 Subject: [PATCH 12/12] Name the installed command in `react-render-delta`'s description MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The installer emits `mms-react-render-delta`; the description advertised `/react-render-delta`, which resolves to nothing. Caught by the check #99 adds — this branch predates it and only fails once combined. --- domains/performance/skills/react-render-delta/skill.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/domains/performance/skills/react-render-delta/skill.md b/domains/performance/skills/react-render-delta/skill.md index 919c17c3..7b50c00b 100644 --- a/domains/performance/skills/react-render-delta/skill.md +++ b/domains/performance/skills/react-render-delta/skill.md @@ -1,6 +1,6 @@ --- name: react-render-delta -description: Prove a React rendering or memoization change actually reduced work, with a delivery gate and a reported band. Covers re-render counts (why-did-you-render), selector recomputes (reselect's real `.recomputations()` API), and A/B arms toggled at a FIXED commit rather than across a merge boundary. The falsifier is an arm whose treatment never reached the built bundle — a null from undelivered treatment is indistinguishable from a null from a small effect and reports as the second. Triggers on /react-render-delta, or when asked to prove a component stopped over-rendering, measure selector recomputation, validate a memoization/React Compiler change, run a render-count A/B, or interpret a re-render benchmark. Callable by `evidence` as its React render & selector proof engine. +description: Prove a React rendering or memoization change actually reduced work, with a delivery gate and a reported band. Covers re-render counts (why-did-you-render), selector recomputes (reselect's real `.recomputations()` API), and A/B arms toggled at a FIXED commit rather than across a merge boundary. The falsifier is an arm whose treatment never reached the built bundle — a null from undelivered treatment is indistinguishable from a null from a small effect and reports as the second. Triggers on /mms-react-render-delta, or when asked to prove a component stopped over-rendering, measure selector recomputation, validate a memoization/React Compiler change, run a render-count A/B, or interpret a re-render benchmark. Callable by `evidence` as its React render & selector proof engine. maturity: experimental ---