Conversation
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.
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.
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`.
`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 #99 adds — this branch predates it and only fails once combined.
`react-render-delta`, `extension-profiling`, `data-analysis` and `benchmark-design` measure; the two scans and the mobile reference updates review. Verified disjoint — no citation crosses the split.
Compiler adoption is a different subject from the render antipattern scans, and neither scan cites it. The three inbound links go with it, so nothing here points at a file this branch no longer carries. Also removes five `MetaMask-planning` epic and audit-ticket ids from four files. They are a private repo's numbers and this repo is public; none of them is on `main`, so this branch introduced all five. Provenance now reads "the extension performance audit". The public `metamask-extension` PR references are kept.
…he single-entry cache BaseController v2 state is Immer-produced, so an unchanged path keeps its reference, and the PR's own cascade reference already said so. reselect 5's `createSelector` defaults to `weakMapMemoize`, which caches per argument and only thrashes on an unstable one. React Strict Mode does not amplify through a cascade by a clean factor of 2. The skills also no longer contradict themselves: mobile runs the React Compiler, the taxonomy has eight patterns, a cold-path `JSON.stringify` is allowed, a cancelled flag is as good as `AbortController`, and the mobile citation points at line 183.
…ch reaches A span that fires once per transaction still fans out when the transaction fires at high volume, and its failures stay fully sampled. A kill-switch reaches only the builds that contain it.
MajorLift
marked this pull request as ready for review
September 18, 2026 14:25
rvelaz
approved these changes
Sep 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
This PR adds review-time checks for React effect antipatterns and for the Redux selector antipatterns that cause render cascades. A selector that returns a fresh reference on every call raises no build error, fails no test and draws no user report, so it ships unless a review looks for it.
Overview
Extends the
performancedomain with two scan skills and the three knowledge files they cite. Both skills carrymetamask-extensionandmetamask-mobileoverlays.effect-antipattern-scanreviews PR diffs that add or modifyuseEffectfor the effect antipatterns.domains/performance/knowledge/effect-antipatterns.md: the platform-agnostic taxonomy ofuseEffectpatterns that cause unnecessary renders, memory leaks or race conditions.selector-antipattern-scanreviews and diagnoses Redux selector antipatterns that cause render cascades, before and after merge.domains/performance/knowledge/selector-antipatterns.md: the platform-agnostic taxonomy of selector patterns that break memoization.domains/performance/knowledge/render-cascade.md: how one state change triggers several re-render cycles.performanceskill gainsmm-selector-cascade,mm-state-normalizationandmm-useeffect-antipatterns. These andmm-selector-memoizationmap the knowledge-file patterns onto the mobile codebase instead of redefining them.Not covered: neither scan checks
renderItemidentity instability, and neither scan's overlays mentionFlatListorFlashList.Sources: Frontend Performance Optimization Guidelines (MetaMask/contributor-docs#159).