From 78d8342b4d0a3cec0c4a15223c36634150793536 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Fri, 5 Jun 2026 14:53:40 -0400 Subject: [PATCH] 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 383d49c..614ec51 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 0000000..00a5d9b --- /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 0000000..03821ef --- /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 0000000..e872081 --- /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 0000000..60bf147 --- /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 0000000..98c0067 --- /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 0000000..b6bfdf6 --- /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 0000000..e05d7c5 --- /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 0000000..4566045 --- /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 0000000..67d4139 --- /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 0000000..dde9807 --- /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 0000000..97283cc --- /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 0000000..aafb8e9 --- /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 0000000..cb28dda --- /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 0000000..31b30be --- /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 |