Add performance skills: measurement + React/Redux anti-pattern reviews

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

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' <source-dir>`
+
+## 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' <source-dir> | 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' <source-dir> | 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.

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.

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 | `<Context.Provider value={{ a, b }}>` 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 |

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
+```

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