feat: add analytics skill with MetaMask Mobile overlay

[NicolasMassart]

Description

Adds analytics in a new platform domain: shared skill.md, MetaMask Mobile overlay, CODEOWNERS for /domains/platform/, and README/CHANGELOG for the domain. Marked base: true so it installs with the default base set.

Why a new platform domain?
Analytics is a product-platform concern. Its own domain keeps ownership and domain filters aligned with that. Cost: a domain that starts with one skill. Benefit: a place for later platform skills.

Fixes MCWP-826.

Type of Change

• New skill
• Skill improvement/update
• Bug fix
• Documentation update
• Other (please describe):

Skill Details (if adding a new skill)

Provider Name: MetaMask
Skill Name: analytics
Brief Description: Product analytics and event tracking guidance, with a MetaMask Mobile overlay for the canonical tracking API.

Checklist

• I have read the CONTRIBUTING.md guidelines
• My skill follows the SKILL_TEMPLATE.md format
• I have tested this skill with an AI agent
• My skill does not contain any secrets, private keys, or sensitive data
• I have added appropriate documentation
• My changes don't break existing skills

Testing

• Confirm yarn audit:skills accepts the new skill frontmatter
• Install the skill and confirm it is selected when adding or reviewing tracked events in MetaMask Mobile
• Confirm base: true still installs the skill when its domain is filtered out
• Confirm --exclude / SKILLS_EXCLUDE still skips it when explicitly excluded

Proof of testing the skill

See the generated skill test report: skill-test-analytics.local.md

Additional Context

MCWP-826

[Copilot]

Pull request overview

Adds a new repo-agnostic analytics skill and a MetaMask Mobile-specific overlay documenting the canonical analytics tracking API and testing patterns, and updates top-level docs/changelog to reflect mandatory: true installation behavior.

Changes:

• Add new analytics skill (marked mandatory: true) with a basic workflow for event selection, building, emission, and test mocking.
• Add metamask-mobile overlay documenting canonical tracking entry points and example usage/mocking patterns.
• Document mandatory: true behavior in README.md and record the addition in CHANGELOG.md.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
README.md	Documents how mandatory: true interacts with domain filtering and excludes.
domains/coding/skills/analytics/skill.md	Introduces the base analytics skill with frontmatter and a simple workflow.
domains/coding/skills/analytics/repos/metamask-mobile.md	Adds MetaMask Mobile overlay describing canonical tracking APIs, examples, and test mocking guidance.
CHANGELOG.md	Notes the new analytics skill and its mandatory: true behavior.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[andrepimenta]

Nice idea — a single authoritative account of mobile tracking is worth having, and most of the overlay checks out (all cited paths, exports and event names exist; useMetrics is genuinely gone; addSensitiveProperties is genuinely deprecated; the VIEW_ALL_ASSETS_CLICKED example is accurate).

I verified every factual claim against a current metamask-mobile checkout and ran the repo's own checks: yarn audit:skills is clean (0 errors, 0 warnings) and node --test passes (69). So CI won't catch any of the below — these are all content issues, and they matter more than usual because agents copy these snippets verbatim.

Eight things I'd want fixed before merge, inline. The pattern in most of them: the guidance describes a plausible API rather than the one the codebase actually uses.

FYI the three existing Copilot comments are anchored to the pre-move domains/coding/ path, so they read as stale — worth re-checking that their points (the "two emission paths" wording, the ## Require heading, messenger.call vs initMessenger.call) still got addressed in the move.

[andrepimenta]

Thanks for turning this around quickly — and for going to the live call sites rather than just rewording. The controllers example now matches app/core/Engine/controllers/identity/user-storage-controller-init.ts:69-76 verbatim, which is exactly the right instinct for a skill whose whole job is to be copied.

Re-verified all eight against 698d1196:

#	Blocker	Status
1	cold_start in source	✅ Fixed — { type: 'cold_start', source: 'direct' }, both values check out against AppOpenedType/AppOpenedSource
2	Messenger snippet won't compile	✅ Fixed — cast + the exact comment from Engine/utils/analytics.ts:48-58
3	Raw messenger drops A/B enrichment	⚠️ Fixed, but the enrichment claim is now too strong — see :26
4	Sensitive-property relocation	✅ Fixed — drop-only + sign-off, and the isAnonymous explanation is accurate
5	EVENT_NAME inversion	⚠️ Half fixed — see :40
6	Rule bans sanctioned helpers	⚠️ Fixed for *Tracking.ts, still over-broad — see :168
7	createMockEventBuilder misuse	❌ Over-corrected into a new defect — see :42
8	README base: true	❌ Not fixed — README.md:390-391 is byte-identical; the fix commit never touched that file

So: five genuinely closed, one untouched, two traded for new problems. The two new ones at :108 and :42 are, I think, more severe than what they replaced — one fabricates A/B assignments for every user, the other breaks 36 existing test files. Details inline.

One item that is not yours to fix, flagged for completeness: app/util/analytics/AnalyticsEventBuilder.ts:39's @deprecated JSDoc in metamask-mobile still says "drop them or move them to addProperties when that is safe", which now contradicts your (correct) drop-only rule. The JSDoc is closer to the code than the skill is, so the privacy path is still reachable via IntelliSense. I'll open a separate one-liner on mobile for that — no action needed here, commented at :164 only so the contradiction is on the record.

Two carried-over items I didn't re-comment on, since they're unchanged and minor next to the above: the jest.clearAllMocks() claim at :136-138 is still false (clearAllMocks calls mockClear(), which preserves implementations — only resetAllMocks removes them; the repo's own analyticsMock.ts:66 has the same bug, so it's inherited), and the CHANGELOG Unreleased block now documents this PR's own review cycle ("Move analytics from coding…", "Align the Mobile analytics overlay with…") for a skill being added in the same block — worth collapsing into the single Added entry, with a ([#140](https://github.com/MetaMask/skills/pull/140)) suffix to match the rest of the file.

[andrepimenta]

Still unresolved (blocker 8). These two lines are byte-identical to the reviewed revision — 698d1196 doesn't touch README.md. Re-confirmed all three counterexamples on this head:

1. Maturity filter precedes the bypass. tools/install line 481 runs before the base bypass at 486-494 and never consults $is_base, so --maturity stable drops a base: true experimental skill.
2. Overlay gate follows it. Lines 533-534. bash tools/install --repo metamask-extension --target . --dry-run still prints skill: analytics / skipped (no overlay for metamask-extension).
3. The JS CLI has no bypass at all. filterSkills in bin/metamask-skills.mjs:682-695 applies domainMatches unconditionally, so metamask-skills list --domain coding hides base skills.

The new CHANGELOG.md:12 line repeats the same overstatement.

This matters beyond the sentence being imprecise: it's the reason platform/analytics reaches mobile engineers only, which sits awkwardly with CODEOWNERS:20 assigning the domain to extension-platform and core-platform, and README.md:146 advertising it to "MM product eng" generally. Either document the two extra gates, or reconsider whether platform earns its own domain while it holds one mobile-only skill.

[andrepimenta]

New defect, and I'd rate it above the one it replaced. This fabricates an A/B assignment: the property is unconditional and both the flag key and the variant are hardcoded.

Mobile's SSOT gates it (docs/ab-testing.md, "Handle custom tracker paths manually"):

...(isActive && {
  active_ab_tests: [createActiveABTestAssignment(FEATURE_AB_TEST_KEY, variantName)],
})

where isActive and variantName come from useABTest.

As written, an agent copying this emits active_ab_tests: [{key:'flagKey', value:'treatment', key_value_pair:'flagKey=treatment'}] for every user, regardless of whether they were assigned to that experiment or that variant. Blocker 1 corrupted one property on one event; this reports the entire user base as enrolled in treatment, which poisons the experiment breakdown and is much harder to notice after the fact.

Suggest mirroring the SSOT's gated form, with variantName shown as coming from useABTest rather than a literal.

[andrepimenta]

The blocker-7 fix over-corrected — this breaks 36 existing test files.

My original point was narrow: createMockEventBuilder needs a jest.fn(() => ...) wrapper when you use it. This turned it into the required default, which is a different and larger claim.

createMockEventBuilder's stub discards everything (analyticsMock.ts:146-160):

addProperties: jest.fn().mockReturnThis(),
build: jest.fn().mockReturnValue({ name: 'mock-event', properties: {}, sensitiveProperties: {} }),

So any test asserting on real payloads breaks. Counts in the repo: 36 files use createEventBuilder: AnalyticsEventBuilder.createEventBuilder, 22 use the mock. Concretely, applying this rule to app/components/UI/SimulationDetails/useSimulationMetrics.test.ts flips the green assertion at line 392 to red, because actual becomes {name:'mock-event', properties:{}}. The repo's own configureUseAnalyticsExternalLinkMock (analyticsMock.ts:141) uses the real builder too.

This is the same failure mode as blocker 5 — a blanket preference that silently reds existing tests — reintroduced one layer down. It also deleted the line that said Prefer AnalyticsEventBuilder.createEventBuilder.

And the override is redundant anyway: createMockUseAnalyticsHook's default createEventBuilder (analyticsMock.ts:84-95) is already exactly jest.fn(() => <stub builder>), so specifying it adds nothing unless you're overriding away from the stub.

Suggested change

	- UI tests: `createMockUseAnalyticsHook` wrapping `useAnalytics`, including when the file already mocks the hook; `createEventBuilder: jest.fn(() => createMockEventBuilder())`
	- UI tests: `createMockUseAnalyticsHook` wrapping `useAnalytics`, including when the file already mocks the hook. Default to `createEventBuilder: AnalyticsEventBuilder.createEventBuilder` so assertions see real payloads; use `createEventBuilder: jest.fn(() => createMockEventBuilder())` only when the test asserts on a simplified payload

The snippet at :154 needs the same change.

[andrepimenta]

"(A/B enrichment + try/catch)" is too strong — enrichment is opt-in per event name.

Routing through the Engine util is necessary but not sufficient. enrichWithABTests matches via hasEventName(mapping, eventName) against AB_TEST_ANALYTICS_MAPPINGS (~20 entries) in app/util/analytics/abTestAnalyticsRegistry.ts. docs/ab-testing.md ("Use active_ab_tests for business events") states both conditions: the event must go through a shared wrapper and "the event name is registered in app/util/analytics/abTestAnalyticsRegistry.ts".

The overlay never mentions the registry, or the feature-local abTestConfig.ts that feeds it. So the blocker-3 failure is still fully reachable: agent adds a new event for a running experiment, routes it through buildAndTrackEvent as instructed, reads "(A/B enrichment + try/catch)", never registers the name — active_ab_tests is absent and the event drops out of the experiment. The difference from before is that the skill now actively assures them it's handled, which is worse than saying nothing.

Suggest either qualifying this to "try/catch, plus A/B enrichment for event names registered in abTestAnalyticsRegistry.ts", or adding the registration step to the workflow.

[andrepimenta]

Three problems in the replacement UI example.

1. The imports are at mutually impossible depths. '../../hooks/useAnalytics/useAnalytics' is 2 levels up; '../../../../util/analytics/actionButtonTracking' is 4. app/util sits exactly one level above app/components, so if hooks resolves at ../../ then util must be ../../../util/.... No file location satisfies both. The real call site confirms the one-level offset — app/components/Views/Homepage/components/HomepageActionButtonsGrid/buttons/SwapButton.tsx:7,16 uses '../../../../../hooks/...' and '../../../../../../util/analytics/actionButtonTracking'. An agent that trusts the hooks path gets Cannot find module (it resolves above the repo root).

2. The hook is called at module top level. const { trackEvent, createEventBuilder } = useAnalytics(); followed by a bare trackActionButtonClick(...) statement, with no component or useCallback wrapper. Copied literally that's Invalid hook call and fires the event at import time rather than on press. SwapButton.tsx:49,59-64 calls the hook in the component body and the tracker inside useCallback.

3. This replaced the only generic UI example. The previous block showed trackEvent(createEventBuilder(<event>).addProperties({...}).build()) — the shape of all 527 createEventBuilder(MetaMetricsEvents. call sites. trackActionButtonClick covers exactly one catalog event (ACTION_BUTTON_CLICKED), so an agent adding a new UI event with no typed helper now has nothing here to copy. The nearest builder examples are analytics.trackEvent (non-React) and buildAndTrackEvent (controllers), so the likely outcome is analytics.trackEvent inside a component — which violates this file's own first Requirement at :36.

Suggest keeping both: the generic useAnalytics + builder example as the default, and the typed-helper example as the "if one exists for your event" case. Wrapping both in a real component would fix 1 and 2 at the same time.

[andrepimenta]

The carve-out is a filename glob, so it still bans things the SSOT requires.

Thanks for adding it — but *Tracking.ts under app/util/analytics/ doesn't cover the set:

• abTestConfig.ts is mandatory, and still rejected. docs/ab-testing.md step 3 requires a feature-local abTestConfig.ts exporting an ABTestAnalyticsMapping; app/util/analytics/abTestAnalyticsRegistry.ts imports ~20 of them from app/components/.../abTestConfig. It's a typed event map, it's feature-local, and it doesn't match *Tracking.ts — so an agent wiring a new experiment is told by the SSOT to create one and by this bullet to reject it.
• Two platform emitters don't match the pattern. app/util/analytics/appInstallEvent.ts:171-181 and walletSetupCompletedAttributionReplay.ts both build events and call analytics.trackEvent. Both remain banned.

The glob patches the six filenames I happened to list rather than the mechanism. Ownership/location is the durable version: platform-owned helpers under app/util/analytics/ are the typed-helper layer — use them; don't add a feature-local tracking layer of your own, with an explicit exception for abTestConfig.ts.

[andrepimenta]

Half of blocker 5 is fixed; the new-site half creates a different split.

"Existing call sites keep MetaMetricsEvents.*" resolves the test-breakage risk — thank you, that was the urgent part.

The remainder: skill.md step 1 tells the agent to register the event as "EVENT_NAME + generateOpt in catalog modules", which produces both EVENT_NAME.FOO and MetaMetricsEvents.FOO = generateOpt(EVENT_NAME.FOO) — the pattern at MetaMetrics.events.ts:912,1546,1626. This line then routes the new call site to EVENT_NAME.FOO, so the wrapper the agent just created has zero consumers, and new sites diverge in form from the 527 existing ones.

There's a latent failure in that split: if anyone later adds action/description to that generateOpt call, the EVENT_NAME call site silently ships without properties.action/properties.name — precisely the loss the Reject bullet at :174 forbids.

Picking one form for all sites removes the split. Given 527 vs 16, MetaMetricsEvents.* is the cheaper convergence point.

[andrepimenta]

The dimension bug is fixed, but the example choice is still the hazard — and now a quieter one.

App Opened is owned exclusively by app/core/AppStateEventListener.ts, and this file's own last Reject bullet (:179) is "Firing an existing catalog event at a new lifecycle". So the canonical non-React example demonstrates the thing the file forbids five lines from the end.

Because the property values are now correct, a copied event is indistinguishable from a real one: an agent pasting this into any non-React module emits a second well-formed App Opened with {type:'cold_start', source:'direct'}, silently inflating the cold-start/direct bucket. Before the fix, at least the bad data was obvious.

A neutral event with no live emitter — or an obvious placeholder like MetaMetricsEvents.YOUR_EVENT — carries the same teaching value at zero risk. Same applies to the escape-hatch snippet at :103.

[andrepimenta]

No action needed here — this rule is correct. Recording a contradiction it now has with mobile, so it isn't lost.

app/util/analytics/AnalyticsEventBuilder.ts:39 in metamask-mobile still reads:

@deprecated Use addProperties instead. When editing a leftover call site, review those fields and drop them or move them to addProperties when that is safe.

An agent with the call site open gets "move them … when that is safe" from IntelliSense and "Do not relocate without human sign-off" from here. The JSDoc is closer to the code and will usually win, so the isAnonymous flip (AnalyticsEventBuilder.ts:75-80) stays reachable.

I'll open a separate PR on metamask-mobile to align the JSDoc with this rule. Flagging only so the two don't drift further.

[andrepimenta]

Consider cutting this block — it's unreachable by the file's own rules.

The Reject bullet at :176-177 bans raw initMessenger.call('AnalyticsController:trackEvent', …) "when Engine trackEvent / buildAndTrackEvent is available" — and since availability is just an import, it's always available. So nothing should ever reach this path, yet it's the longest code block in the file (30 lines) and the only copy target for a 12-line structural cast that exists in Engine/utils/analytics.ts precisely so no call site has to write it. Combined with the fabricated active_ab_tests at :108, the most likely use of this block is someone copying it incorrectly.

One sentence would carry the same information: if you must bypass the util, copy the cast from app/core/Engine/utils/analytics.ts:47-59 and attach active_ab_tests yourself.

Small related thing in the prose at :29: "keep the Engine-util cast" doesn't quite parse — the util's cast is internal to it, so a bypassing call site has nothing to keep; it has to re-derive the cast. Worth rewording to "re-derive" or "copy".