feat(performance): add frontend anti-pattern detection and delta attribution skills - #43
feat(performance): add frontend anti-pattern detection and delta attribution skills#43MajorLift wants to merge 13 commits into
Conversation
- WDYR (`wdyr.js`, `useSelector` diff tracking) as the live cascade tracer - input-unstable vs output-unstable tool selection (`resultEqualityCheck`) - a live cascade nullifies downstream memo/virtualization/compiler wins - plain-function selector sweep; span quota guardrail; redux-persist caveat
- WDYR (`wdyr.js`, `useSelector` diff tracking) as the live cascade tracer - input-unstable vs output-unstable tool selection (`resultEqualityCheck`) - a live cascade nullifies downstream memo/virtualization/compiler wins - plain-function selector sweep; span quota guardrail; redux-persist caveat
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.
The three files this PR adds each restated a pattern that also exists generically. Rather than duplicate, each now opens with a scope note naming the knowledge file that owns the definition — `effect-anti-patterns` for the lifecycle patterns, `render-cascade` and `selector-anti-patterns` for the cascade and shape ones — and keeps what only it can say: the verified Mobile instances, this store's real dependency graph, and the fix recipes. Citations are by name rather than relative link: `install` copies domain knowledge/ and a skill's references/ as siblings under the installed skill directory, so a repo-correct relative path breaks once installed. The knowledge files themselves land in the performance-skills PR (#43); until that merges these notes name a file that is not yet present, which is why they are names and not links.
Folds in the react-render-proof skill (was MetaMask#82) and the mobile reference-library additions (was MetaMask#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.
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.
`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.
`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`.
Context budgetWhat this PR costs an agent, measured from an install rather than read from the diff. Three tiers, and only the first is unavoidable.
Frontmatter is the only tier paid unconditionally — every agent loads it on every run once the skill is installed, used or not, because it is what the agent reads to decide relevance. The 28 skills across the eleven open skill PRs sit at a median of ~1,716 tokens selected and ~1,860 with references followed. All are within the 1,536-character description budget. Selected is paid only when the agent picks the skill. + refs & knowledge is the ceiling if every bundled reference is then read; it is a worst case, not an expectation. Method
These figures are pinned to the commit above and drift on every push; #96 tracks automating them. |
`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.
`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`.
…g it `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-<name>/`. Also updates two sibling names that no longer resolve: `pr-validate` is now `evidence`, and `memory-leak-hunt` is now `memory-leak`.
`-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.
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.
The installer emits `mms-react-render-delta`; the description advertised `/react-render-delta`, which resolves to nothing. Caught by the check MetaMask#99 adds — this branch predates it and only fails once combined.
Builds out the
performancedomain around one idea: shift performance work left in the loop — catch the regression at review time, prove the fix moved something, and do both against a shared, non-duplicated body of knowledge.Supersedes #82 and #49, both closed; all three shared the same knowledge substrate, so they review as one change.
The loop
effect-antipattern-scan,selector-antipattern-scan— grep-driven checklists over a PR diff, with extension and mobile overlaysreact-render-delta— re-render counts, selector recomputes, A/B arms at a fixed commitdata-analysis— turn raw numbers into a defensible before/after;benchmark-design— design a benchmark that measures what you think it does;extension-profiling— compare branches with WDYR and the React DevTools Profilermm-*reference library and its audit playbook, for MetaMask MobileKnowledge is the single source
knowledge/selector-antipatterns.mdandknowledge/effect-antipatterns.mdare the canonical, platform-agnostic taxonomy. Everything else names them rather than restating them.That mattered because the definitions had started to exist in two places — these files, and the
performanceskill'smm-*references already onmain. Same patterns, same worked examples, two homes that would drift. Now:mm-selector-memoization; the effect file absorbed the dependency-side patterns frommm-hook-dependency-arraysand the lifecycle-side ones (derived state, effect chains, uncancelled async).mm-selector-memoization.mdkeeps everything only it can say — this codebase's selector creators, the verified instance table withfile:line, the fix recipes, the scoped greps, the don't-over-correct caveats — and now maps each generic pattern onto the codebase instead of redefining it. No mobile-specific content was removed.Citations are by name, not relative link — deliberately.
tools/installcopies domainknowledge/and a skill'sreferences/as siblings under the installed skill directory, while in the repo they sit three levels apart. So../../../knowledge/x.mdresolves here and breaks once installed;../knowledge/x.mddoes the reverse. No relative path is correct in both layouts. Section anchors are gone for a related reason — they broke the moment the taxonomy was renumbered.Worth stating plainly since "single source" invites the question: on disk, a consumer still receives one copy of each knowledge file per skill —
installcallscopy_domain_knowledgeonce per skill per operator. The single source is in this repo (one file to edit, no authored drift), not in the installed output.react-render-deltaProve a rendering or memoization change actually reduced work. The falsifier is the interesting part: an arm whose treatment never reached the built bundle. A null from undelivered treatment is indistinguishable from a null from a genuinely small effect, and reports as the second — so the skill gates on delivery before reading any delta. Covers
why-did-you-rendercounts, reselect's real.recomputations(), and A/B arms toggled at a fixed commit rather than across a merge boundary.Mobile reference library
Field-tested before review — it produced 13 tickets in previously unidentified areas, and its own guardrails ruled out roughly 90% of candidate findings, which are encoded back into the references.
mm-react-compiler-error-triage) — the'Todo'split plus thepanicThresholdratchet; turns ~7,000 apparent errors into a 31-file backlog.mm-selector-cascade) — traverse the selector graph to closure, fix the root, then delete the downstreamisEqualband-aids instead of accumulating more.mm-state-normalization) — parameterized selectors with single-entry caches recompute per row, forever.mm-useeffect-antipatterns) — the missing other half ofmm-hook-dependency-arrays.Showcase —
react-render-deltaon a real cascadeThe regressions that hide worst are in repeated work — renders, selector recomputations, allocations that run more often than they need to. A single-action stopwatch reads them as noise and production telemetry never emits them at all. The count is the signal, and it only exists once you add the instrument.
A render-cascade epic in
metamask-extension, measured across both arms with why-did-you-render:1d6d1811b80d37542a08Same flow, same build configuration, arms differing only by commit — a 66% reduction that no timing measurement would have surfaced, because each individual render is far below the noise floor of a stopwatch.
Two of the constituent fixes show what the counts localise to:
getPendingApprovalsgetInternalAccountsBoth are selectors returning a fresh reference on every call, so every consumer re-rendered on every state change. Invisible in a profile as anything but diffuse cost; unambiguous as a count.
The falsifier the skill enforces: an arm whose treatment never reached the built bundle. A null result from undelivered treatment is indistinguishable from a null result from a small effect, and reports as the second. So delivery is checked in each arm before any delta is interpreted — the counting instrument has to be present and the change actually built, or the comparison means nothing.
Notes
@metamask/skillsCLI package; skill-only PRs don't add one.babel-plugin-react-compiler@1.0.0dist this repo ships).frontend-performance.md· metamask-extension#38007, #37147 · Is there a benefit to removing existing memoization code? reactwg/react-compiler#16. Validation tickets: metamask-mobile#31490–#31501, #31507, #31509.Validation runs
Trial runs of this PR's skills against merged
metamask-extensionPRs nobody flagged. Every claim was re-verified against the real diff before posting. Clean results are included on purpose — a skill that only ever reports problems cannot be calibrated.react-render-deltareact-render-deltaselector-antipattern-scan?? {}fresh object, unmemoized, intouseSelectorconstfeeding a FlashListEach comment carries a trial-run disclaimer and links back here for feedback.
Coverage gap this surfaced. No skill here detects
renderItemidentity instability: neither antipattern-scan mobile overlay mentionsrenderItem,FlatList, orFlashList, and theperformanceoverlay carries the guidance without a detection for it. The run also hit the overlay's own rule — don't suggestuseMemo/useCallbackwithout profiler evidence — so the comment reports the identity delta and names what to measure rather than prescribing the patch.