Skip to content

feat: Extends performance domain with anti-pattern scanning skills - #43

Closed
MajorLift wants to merge 16 commits into
MetaMask:mainfrom
MajorLift:add/performance
Closed

MajorLift wants to merge 16 commits into
MetaMask:mainfrom
MajorLift:add/performance

Conversation

@MajorLift

@MajorLift MajorLift commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

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 performance domain with two scan skills and the three knowledge files they cite. Both skills carry metamask-extension and metamask-mobile overlays.

  • effect-antipattern-scan reviews PR diffs that add or modify useEffect for the effect antipatterns.
    • domains/performance/knowledge/effect-antipatterns.md: the platform-agnostic taxonomy of useEffect patterns that cause unnecessary renders, memory leaks or race conditions.
  • selector-antipattern-scan reviews 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.
  • The mobile performance skill gains mm-selector-cascade, mm-state-normalization and mm-useeffect-antipatterns. These and mm-selector-memoization map the knowledge-file patterns onto the mobile codebase instead of redefining them.

Not covered: neither scan checks renderItem identity instability, and neither scan's overlays mention FlatList or FlashList.

Sources: Frontend Performance Optimization Guidelines (MetaMask/contributor-docs#159).

MajorLift added a commit that referenced this pull request Jun 10, 2026
- 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
MajorLift added a commit that referenced this pull request Jul 22, 2026
- 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
MajorLift added a commit that referenced this pull request Jul 30, 2026
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.
@MajorLift MajorLift changed the title Add performance skills: measurement + React/Redux anti-pattern reviews feat(performance): shift-left performance — review-time detection, render proof, and a single-source knowledge base Jul 30, 2026
@MajorLift MajorLift changed the title feat(performance): shift-left performance — review-time detection, render proof, and a single-source knowledge base feat(performance): React/Redux performance (cross-client) - anti-pattern detection, delta measurement Jul 30, 2026
@MajorLift MajorLift changed the title feat(performance): React/Redux performance (cross-client) - anti-pattern detection, delta measurement feat(performance): Frontend performance - anti-pattern detection, delta attribution Jul 30, 2026
@MajorLift MajorLift changed the title feat(performance): Frontend performance - anti-pattern detection, delta attribution feat(performance): add shift-left detection, proof, and measurement skills Jul 30, 2026
@MajorLift MajorLift changed the title feat(performance): add shift-left detection, proof, and measurement skills feat(performance): frontend performance (cross-client) - anti-pattern detection, fix proof, delta measurement Jul 30, 2026
@MajorLift MajorLift changed the title feat(performance): frontend performance (cross-client) - anti-pattern detection, fix proof, delta measurement feat(performance): Frontend performance (cross-client) - Anti-pattern detection, delta attribution Jul 30, 2026
@MajorLift MajorLift changed the title feat(performance): Frontend performance (cross-client) - Anti-pattern detection, delta attribution feat: add cross-client frontend performance anti-pattern detection and delta attribution skills Jul 30, 2026
@MajorLift MajorLift changed the title feat: add cross-client frontend performance anti-pattern detection and delta attribution skills feat(performance): add cross-client frontend anti-pattern detection and performance delta attribution skills Jul 30, 2026
@MajorLift
MajorLift marked this pull request as draft July 30, 2026 14:03
@MajorLift MajorLift changed the title feat(performance): add cross-client frontend anti-pattern detection and performance delta attribution skills feat(performance): add frontend anti-pattern detection and delta attribution skills Jul 30, 2026
@MajorLift
MajorLift marked this pull request as ready for review July 30, 2026 18:15
@MajorLift

MajorLift commented Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

Context budget

What this PR costs an agent, measured from an install rather than read from the diff. Three tiers, and only the first is unavoidable.

Skill Frontmatter Selected + refs & knowledge
data-analysis 221 chars ~1,428 tok ~1,428 tok
effect-antipattern-scan 89 chars ~1,193 tok ~1,193 tok
extension-profiling 134 chars ~680 tok ~680 tok
react-render-delta 780 chars ~1,938 tok ~1,938 tok
selector-antipattern-scan 100 chars ~2,510 tok ~2,510 tok
benchmark-design 109 chars ~882 tok ~882 tok

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

tools/install --repo metamask-extension --maturity experimental against this branch at c18c751a7, measured per installed skill directory. Repo overlays are merged into the emitted SKILL.md, so they land in the selected tier rather than being missed by a source-byte count. Token figures are bytes/4 — a proxy for scale, not accounting.

These figures are pinned to the commit above and drift on every push; #96 tracks automating them.

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.
@MajorLift MajorLift changed the title feat(performance): add frontend anti-pattern detection and delta attribution skills feat: Extends performance domain - anti-pattern scans, render delta, profiling, data analysis skills Aug 31, 2026
@MajorLift MajorLift changed the title feat: Extends performance domain - anti-pattern scans, render delta, profiling, data analysis skills feat: Extends performance domain - /effect-antipattern-scan, /selector-antipattern-scan, /react-render-delta, /extension-profiling, /benchmark-design, /data-analysis skills Aug 31, 2026
@MajorLift MajorLift changed the title feat: Extends performance domain - /effect-antipattern-scan, /selector-antipattern-scan, /react-render-delta, /extension-profiling, /benchmark-design, /data-analysis skills feat: Extends performance domain - cross-client learnings from performance audit and "shift-left" initiative Aug 31, 2026
@MajorLift MajorLift changed the title feat: Extends performance domain - cross-client learnings from performance audit and "shift-left" initiative feat: Extends performance domain - Extension performance audit learnings applied to cross-client skills, knowledges Aug 31, 2026
@MajorLift MajorLift changed the title feat: Extends performance domain - Extension performance audit learnings applied to cross-client skills, knowledges feat: Extends performance domain with performance audit, benchmark gate learnings Sep 1, 2026
MajorLift added a commit that referenced this pull request Sep 1, 2026
The symptom table and the description both named `react-render-proof`. No skill
of that name exists on `main` or in any open pull request; the render engine is
`react-render-delta`, added by #43 and carried by #84 and #108, and installed
locally as `mms-react-render-delta`.

The other six engines this skill routes to all resolve to skills in open pull
requests, so this was the only wrong name rather than one of three. Checked
against the repository rather than against an install: none of the seven is on
`main` yet, which is a merge-ordering fact and not a defect here.

Nothing validates this today. #103, which would have checked cross-skill
references, was closed as superseded by #87, and #87 is not merged — so this
name would have shipped unflagged.
`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.
@MajorLift MajorLift changed the title feat: Extends performance domain with performance audit, benchmark gate learnings feat(performance): scan PR diffs for the React and Redux antipatterns that cascade renders Sep 1, 2026
@MajorLift MajorLift changed the title feat(performance): scan PR diffs for the React and Redux antipatterns that cascade renders feat: Extends performance domain with antipattern scan skills Sep 1, 2026
@MajorLift MajorLift changed the title feat: Extends performance domain with antipattern scan skills feat: Extends performance domain - /effect-antipattern-scan, /selector-antipattern-scan skills Sep 1, 2026
@MajorLift MajorLift changed the title feat: Extends performance domain - /effect-antipattern-scan, /selector-antipattern-scan skills feat: Extends performance domain with anti-pattern scanning skills Sep 1, 2026
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
MajorLift requested a review from rvelaz September 15, 2026 20:02
@MajorLift MajorLift closed this by deleting the head repository Sep 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant