diff --git a/.agents/skills/ab-testing-implementation/SKILL.md b/.agents/skills/ab-testing-implementation/SKILL.md new file mode 100644 index 000000000000..a7a0173b8dff --- /dev/null +++ b/.agents/skills/ab-testing-implementation/SKILL.md @@ -0,0 +1,111 @@ +--- +name: ab-testing-implementation +description: Implement and review MetaMask Extension A/B tests using the canonical repository standard. Use for any task that adds or modifies A/B test flags, variant configs, useABTest usage, analytics payloads, or A/B-test-related tests and docs. +--- + +# A/B Testing Implementation + +Canonical workflow for implementing and reviewing MetaMask Extension A/B +tests. + +Do not use this skill for general analytics work that does not involve A/B +test flags, `useABTest`, `active_ab_tests`, or related tests/docs. + +## Required Response Sections + +1. `Implementation Checklist` +2. `Files To Modify` +3. `Analytics Payload Changes` +4. `Tests To Run` +5. `Compliance Check Result` + +## Agent Execution Standard + +For implementation or review tasks, follow this workflow: + +1. Run discovery before edits. + +```bash +rg -n "useABTest\\(|active_ab_tests|ab_tests|Abtest|feature-flag-registry|RemoteFeatureFlagController" app shared ui test docs +rg -n "Experiment Viewed|ExperimentViewed" app shared ui +``` + +2. Confirm the intended experiment shape. + - Use a threshold-array remote flag value for production defaults. + - Keep reused variants or metadata centralized in a config module when + multiple files need the same definitions. + - Use the same experiment key format as mobile: + `{teamName}{ticketId}Abtest{TestName}`. +3. Implement the assignment logic correctly. + - Prefer `useABTest(flagKey, variants)` and keep a `control` variant in + the variants object. + - Use `variantName` and `isActive` from the hook for business-event + instrumentation. + - If assignment is missing, invalid, or unresolved, the hook falls back + to `control` and `isActive: false`. +4. Implement analytics correctly. + - Rely on `useABTest` for the automatic `Experiment Viewed` exposure + event. + - Add `active_ab_tests` only to business events, and only when the + assignment is active. + - Never add new `ab_tests:` payloads. If a legacy touchpoint cannot be + migrated in the same change, keep the line annotated with + `LEGACY_AB_TEST_ALLOWED` and explain why. +5. Use the canonical event payload shapes. + +```typescript +const experiment = useABTest('swapsSWAPS4135AbtestNumpadQuickAmounts', { + control: { buttons: [25, 50, 75, 'MAX'] }, + treatment: { buttons: [50, 75, 90, 'MAX'] }, +}); + +const activeABTests = experiment.isActive + ? [ + { + key: 'swapsSWAPS4135AbtestNumpadQuickAmounts', + value: experiment.variantName, + }, + ] + : undefined; +``` + +6. Update tests and fixtures when behavior or flag plumbing changes. + - Register every new remote A/B test flag in + `test/e2e/feature-flags/feature-flag-registry.ts` with the production + default threshold-array JSON value. + - Use test overrides such as `manifestFlags.remoteFeatureFlags` or + `FixtureBuilder.withRemoteFeatureFlags(...)` when a test needs + deterministic assignment. + - If the change is copy-only or config-only, you may skip new tests with a brief rationale. +7. Run the A/B compliance checker using the repository's current supported + invocation and report the result. + - Checker path: + `.agents/skills/ab-testing-implementation/scripts/check-ab-testing-compliance.ts` + +```bash +# Current pre-commit / local implementation example +node --import tsx .agents/skills/ab-testing-implementation/scripts/check-ab-testing-compliance.ts --staged + +# Current review-mode / explicit file set example +node --import tsx .agents/skills/ab-testing-implementation/scripts/check-ab-testing-compliance.ts --files app/path/to/file.ts,test/path/to/file.spec.ts --base origin/main +``` + +## Review Checklist + +- Confirm `useABTest` always has a `control` variant. +- Confirm `Experiment Viewed` is not emitted manually when `useABTest` is in + use. +- Confirm business events use `active_ab_tests` rather than `ab_tests`. +- Confirm E2E flag registration and local test overrides remain production-accurate. +- Confirm the compliance checker result is included in the final response. + +## Related Files + +- `ui/hooks/useABTest.ts` +- `ui/hooks/useABTest.test.ts` +- `ui/selectors/remote-feature-flags.ts` +- `shared/constants/metametrics.ts` +- `test/e2e/feature-flags/feature-flag-registry.ts` + +Use `docs/ab-testing.md` only when you need deeper background, additional +examples, FAQ answers, or local override guidance beyond this workflow. diff --git a/.agents/skills/ab-testing-implementation/agents/openai.yaml b/.agents/skills/ab-testing-implementation/agents/openai.yaml new file mode 100644 index 000000000000..525077c200ea --- /dev/null +++ b/.agents/skills/ab-testing-implementation/agents/openai.yaml @@ -0,0 +1,4 @@ +interface: + display_name: "A/B Test Implementation" + short_description: "Implement and validate extension A/B tests." + default_prompt: "Use $ab-testing-implementation to implement or review an A/B test with the extension's canonical useABTest and active_ab_tests standards." diff --git a/.agents/skills/ab-testing-implementation/scripts/check-ab-testing-compliance.ts b/.agents/skills/ab-testing-implementation/scripts/check-ab-testing-compliance.ts new file mode 100644 index 000000000000..41076e4cda48 --- /dev/null +++ b/.agents/skills/ab-testing-implementation/scripts/check-ab-testing-compliance.ts @@ -0,0 +1,408 @@ +import { readFileSync } from 'fs'; +import process from 'process'; +import { spawnSync } from 'child_process'; + +type Mode = 'staged' | 'files'; + +type CommandResult = { + status: number; + stdout: string; + stderr: string; +}; + +const CODE_FILE_REGEX = /\.(ts|tsx|js|jsx)$/u; +const TEST_FILE_REGEX = /\.(test|spec)\.(ts|tsx|js|jsx)$/u; +const VALID_FLAG_KEY_REGEX = + /^[a-z][A-Za-z0-9]*[A-Z]{2,}[0-9]+Abtest[A-Z][A-Za-z0-9]*$/u; +const ABTEST_STRING_LITERAL_REGEX = /(['"])([A-Za-z0-9]*Abtest[A-Za-z0-9]*)\1/gu; +const RISKY_CHANGE_REGEX = + /useABTest\(|active_ab_tests\s*:|ab_tests\s*:|trackEvent\(|createEventBuilder\(|MetaMetricsEvents\.|Experiment Viewed|ExperimentViewed/u; + +function usage(): string { + return `Usage: + check-ab-testing-compliance.ts -h | --help + check-ab-testing-compliance.ts --staged + check-ab-testing-compliance.ts --files [--base ] + +Checks changed files for A/B testing implementation compliance. + +Rules: + - Fail: New ab_tests payload additions in checked code diffs + - Fail: Malformed literal active_ab_tests objects missing key/value + - Fail: Inline useABTest variants object missing control + - Warn: Flag key naming mismatch for Abtest keys + - Warn: Risky A/B integration changes without test-file updates`; +} + +function failWithUsage(message: string): never { + console.error(message); + console.error(usage()); + process.exit(2); +} + +function runCommand(command: string, args: string[]): CommandResult { + const result = spawnSync(command, args, { + cwd: process.cwd(), + encoding: 'utf8', + }); + + return { + status: result.status ?? 1, + stdout: result.stdout ?? '', + stderr: result.stderr ?? '', + }; +} + +function runGit(args: string[]): CommandResult { + return runCommand('git', args); +} + +function splitUniqueLines(...texts: string[]): string[] { + const seen = new Set(); + + return texts + .flatMap((text) => text.split('\n')) + .map((line) => line.trim()) + .filter((line) => { + if (!line || seen.has(line)) { + return false; + } + + seen.add(line); + return true; + }); +} + +function trim(value: string): string { + return value.trim(); +} + +function isCodeFile(file: string): boolean { + return CODE_FILE_REGEX.test(file); +} + +function isTestFile(file: string): boolean { + return TEST_FILE_REGEX.test(file) || file.includes('/__tests__/'); +} + +function isValidFlagKey(key: string): boolean { + return VALID_FLAG_KEY_REGEX.test(key); +} + +function refExists(ref: string): boolean { + return runGit(['rev-parse', '--verify', ref]).status === 0; +} + +function isTracked(file: string): boolean { + return runGit(['ls-files', '--error-unmatch', file]).status === 0; +} + +function existsInHead(file: string): boolean { + return runGit(['cat-file', '-e', `HEAD:${file}`]).status === 0; +} + +function collectStagedFiles(): string[] { + return splitUniqueLines( + runGit(['diff', '--cached', '--name-only', '--diff-filter=ACMR']).stdout, + ); +} + +function collectWorktreeFiles(): string[] { + return splitUniqueLines( + runGit(['diff', '--name-only', '--diff-filter=ACMR']).stdout, + runGit(['ls-files', '--others', '--exclude-standard']).stdout, + ); +} + +function collectExplicitFiles(filesArg: string): string[] { + return splitUniqueLines(...filesArg.split(',').map(trim)); +} + +function extractAddedLinesFromDiff(diff: string): string[] { + return diff + .split('\n') + .filter((line) => line.startsWith('+') && !line.startsWith('+++ ')) + .map((line) => line.slice(1)); +} + +function getAddedLines( + file: string, + mode: Mode, + fallbackToWorktree: boolean, + baseRef: string, +): string[] { + if (mode === 'staged') { + if (fallbackToWorktree) { + if (!isTracked(file)) { + return readFileSync(file, 'utf8').split('\n'); + } + + return extractAddedLinesFromDiff( + runGit(['diff', '--unified=0', '--', file]).stdout, + ); + } + + return extractAddedLinesFromDiff( + runGit(['diff', '--cached', '--unified=0', '--', file]).stdout, + ); + } + + if (!existsInHead(file)) { + return readFileSync(file, 'utf8').split('\n'); + } + + if (baseRef && refExists(baseRef)) { + return extractAddedLinesFromDiff( + runGit(['diff', '--unified=0', `${baseRef}...HEAD`, '--', file]).stdout, + ); + } + + if (isTracked(file)) { + return extractAddedLinesFromDiff( + runGit(['diff', '--unified=0', 'HEAD', '--', file]).stdout, + ); + } + + return []; +} + +function countOccurrences(segment: string, char: '(' | ')'): number { + return Array.from(segment).filter((current) => current === char).length; +} + +function dedupe(items: string[]): string[] { + return Array.from(new Set(items)); +} + +function printList(title: string, items: string[]): void { + if (items.length === 0) { + return; + } + + console.log(''); + console.log(title); + for (const item of dedupe(items)) { + console.log(`- ${item}`); + } +} + +function main(): void { + let mode: Mode | null = null; + let filesArg = ''; + let baseRef = ''; + + const args = process.argv.slice(2); + + for (let index = 0; index < args.length; index += 1) { + const arg = args[index]; + + if (arg === '--staged') { + if (mode) { + failWithUsage('ERROR: Choose exactly one mode: --staged or --files.'); + } + mode = 'staged'; + continue; + } + + if (arg === '--files') { + if (mode) { + failWithUsage('ERROR: Choose exactly one mode: --staged or --files.'); + } + mode = 'files'; + filesArg = args[index + 1] ?? ''; + if (!filesArg) { + console.error('ERROR: --files requires a comma-separated value.'); + process.exit(2); + } + index += 1; + continue; + } + + if (arg === '--base') { + baseRef = args[index + 1] ?? ''; + if (!baseRef) { + console.error( + 'ERROR: --base requires a git ref (for example origin/main).', + ); + process.exit(2); + } + index += 1; + continue; + } + + if (arg === '--help' || arg === '-h') { + console.log(usage()); + process.exit(0); + } + + failWithUsage(`ERROR: Unknown argument: ${arg}`); + } + + if (!mode) { + failWithUsage('ERROR: Choose exactly one mode: --staged or --files.'); + } + + if (mode === 'files' && !baseRef) { + for (const candidate of ['origin/main', 'main', 'HEAD~1']) { + if (refExists(candidate)) { + baseRef = candidate; + break; + } + } + } + + let fallbackToWorktree = false; + let fallbackNote = ''; + + let changedFiles = + mode === 'staged' ? collectStagedFiles() : collectExplicitFiles(filesArg); + + if (mode === 'staged' && changedFiles.length === 0) { + fallbackToWorktree = true; + fallbackNote = + 'Info: no staged files found; falling back to working-tree changed files.'; + changedFiles = collectWorktreeFiles(); + } + + if (changedFiles.length === 0) { + if (mode === 'staged') { + console.log( + 'A/B compliance check: no staged files and no working-tree changed files to inspect.', + ); + } else { + console.log('A/B compliance check: no files to inspect from --files input.'); + } + process.exit(0); + } + + const failures: string[] = []; + const warnings: string[] = []; + const abRiskyChangeFiles = new Set(); + let testChanged = false; + + for (const file of changedFiles) { + if (isTestFile(file)) { + testChanged = true; + continue; + } + + if (!isCodeFile(file)) { + continue; + } + + const addedLines = getAddedLines(file, mode, fallbackToWorktree, baseRef); + if (addedLines.length === 0) { + continue; + } + + const addedText = addedLines.join('\n'); + + if (RISKY_CHANGE_REGEX.test(addedText)) { + abRiskyChangeFiles.add(file); + } + + for (let index = 0; index < addedLines.length; index += 1) { + const line = addedLines[index]; + + if ( + /(^|[^A-Za-z0-9_])ab_tests\s*:/.test(line) && + !line.includes('LEGACY_AB_TEST_ALLOWED') + ) { + failures.push( + `${file}: added 'ab_tests' payload. New ab_tests payloads are forbidden.`, + ); + } + + if (/active_ab_tests\s*:/.test(line)) { + if (/active_ab_tests\s*:\s*(\[|\{)/.test(line)) { + const window = addedLines.slice(index, index + 9).join('\n'); + if (!/key\s*:/.test(window) || !/value\s*:/.test(window)) { + failures.push( + `${file}: malformed literal active_ab_tests object (expected key and value).`, + ); + } + } + } + + if (/useABTest\s*\(/.test(line)) { + const callSegments: string[] = []; + let parenDepth = 0; + + for (let segmentIndex = index; segmentIndex < addedLines.length; segmentIndex += 1) { + let segment = addedLines[segmentIndex]; + if (segmentIndex === index) { + segment = `useABTest${segment.split('useABTest').slice(1).join('useABTest')}`; + } + + callSegments.push(segment); + parenDepth += countOccurrences(segment, '('); + parenDepth -= countOccurrences(segment, ')'); + + if (parenDepth <= 0) { + break; + } + } + + const callWindow = callSegments.join('\n'); + const normalizedCall = callWindow.replace(/\n/g, ' '); + if ( + /useABTest\s*\([^,]+,\s*\{/.test(normalizedCall) && + !/control\s*:/.test(callWindow) + ) { + failures.push( + `${file}: inline useABTest variants object is missing control.`, + ); + } + } + + const useAbTestLiteralKey = line.match( + /useABTest\s*\(\s*['"]([^'"]+)['"]/u, + )?.[1]; + + if ( + useAbTestLiteralKey && + !isValidFlagKey(useAbTestLiteralKey) + ) { + warnings.push( + `${file}: flag key '${useAbTestLiteralKey}' does not match {team}{TICKET}Abtest{Name}.`, + ); + } + + for (const match of line.matchAll(ABTEST_STRING_LITERAL_REGEX)) { + const key = match[2]; + if (key === useAbTestLiteralKey) { + continue; + } + + if (!isValidFlagKey(key)) { + warnings.push( + `${file}: Abtest key '${key}' does not match {team}{TICKET}Abtest{Name}.`, + ); + } + } + } + } + + if (abRiskyChangeFiles.size > 0 && !testChanged) { + warnings.push( + 'Risky A/B integration changes were detected without any test-file updates. For copy/config-only changes, document rationale in your response.', + ); + } + + console.log('A/B compliance check summary'); + console.log(`Mode: ${mode}`); + if (fallbackNote) { + console.log(fallbackNote); + } + if (mode === 'files' && baseRef) { + console.log(`Base ref: ${baseRef}`); + } + console.log(`Files inspected: ${changedFiles.length}`); + + printList('Failures:', failures); + printList('Warnings:', warnings); + + process.exit(failures.length > 0 ? 1 : 0); +} + +main(); diff --git a/.browserslistrc b/.browserslistrc index 37ec9d624480..ace1ad8ca58b 100644 --- a/.browserslistrc +++ b/.browserslistrc @@ -1 +1 @@ -chrome >= 89, firefox >= 89 +chrome >= 113, firefox >= 115 diff --git a/.claude/skills/ab-testing-implementation/SKILL.md b/.claude/skills/ab-testing-implementation/SKILL.md new file mode 100644 index 000000000000..a1b08802d938 --- /dev/null +++ b/.claude/skills/ab-testing-implementation/SKILL.md @@ -0,0 +1,6 @@ +--- +name: ab-testing-implementation +summary: Implement or review extension A/B tests using the repository standard. +--- + +Follow `.agents/skills/ab-testing-implementation/SKILL.md`. diff --git a/.cursor/commands/CODEBOT.md b/.cursor/commands/CODEBOT.md index deecaac6faba..b12c73afbab9 100644 --- a/.cursor/commands/CODEBOT.md +++ b/.cursor/commands/CODEBOT.md @@ -473,8 +473,6 @@ Output the analysis in this **concise format**: 1. What is the reason for the change? 2. What is the improvement/solution?] -[![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/PR?quickstart=1) - ## **Changelog** [If user-facing change:] diff --git a/.cursor/rules/e2e-testing-guidelines/RULE.md b/.cursor/rules/e2e-testing-guidelines/RULE.md index 0c1ccbc92df4..f177dfc11dc5 100644 --- a/.cursor/rules/e2e-testing-guidelines/RULE.md +++ b/.cursor/rules/e2e-testing-guidelines/RULE.md @@ -90,6 +90,8 @@ For new test code, use `FixtureBuilderV2` by default. - `withCurrencyController` - `withKeyringController` - `withMetaMetricsController` +- `withMultichainAssetsRatesController` +- `withMultichainRatesController` - `withNameController` - `withNetworkController` - `withNetworkEnablementController` @@ -106,6 +108,8 @@ For new test code, use `FixtureBuilderV2` by default. **Custom convenience methods**: - `withConversionRateDisabled` +- `withConversionRates` +- `withCurrencyRates` - `withKeyringControllerAdditionalAccountVault` - `withKeyringControllerMultiSRP` - `withKeyringControllerOldVault` diff --git a/.depcheckrc.yml b/.depcheckrc.yml index 724003cc8981..0dba5f3d489a 100644 --- a/.depcheckrc.yml +++ b/.depcheckrc.yml @@ -85,6 +85,9 @@ ignores: - 'react-compiler-webpack' # build tool - 'thread-loader' # build tool # babel + - '@babel/plugin-transform-class-properties' + - '@babel/plugin-transform-private-methods' + - '@babel/plugin-transform-private-property-in-object' - '@babel/plugin-transform-logical-assignment-operators' - 'babel-plugin-react-compiler' # used in image optimization script diff --git a/.eslintrc.js b/.eslintrc.js index 19f6698e303c..608d87dfd037 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -311,6 +311,12 @@ module.exports = { }, }, }, + { + files: ['.agents/**/*.ts'], + rules: { + 'import-x/no-nodejs-modules': 'off', + }, + }, /** * == Everything else == * @@ -510,7 +516,7 @@ module.exports = { 'test/unit-global/*.test.js', 'ui/**/*.test.js', 'ui/__mocks__/*.js', - 'shared/lib/error-utils.test.js', + 'shared/lib/error-utils.test.ts', ], extends: ['@metamask/eslint-config-jest'], parserOptions: { diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 18dd55dffb75..01c31940e549 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -48,7 +48,7 @@ development/circular-deps.ts @MetaMask/extension-security-team # For now, restricting approvals inside the .devcontainer folder to devs # who were involved with the Codespaces project. -.devcontainer/ @MetaMask/extension-security-team @HowardBraham +.devcontainer/ @MetaMask/extension-platform @HowardBraham # Design System to own code for the component-library folder # Slack handle: @metamask-design-system-team | Slack channel: #metamask-design-system @@ -211,8 +211,8 @@ app/scripts/lib/createMainFrameOriginMiddleware.ts @MetaMask/ app/scripts/lib/createTracingMiddleware.ts @MetaMask/wallet-integrations # mUSD -**/musd/** @MetaMask/metamask-earn -shared/lib/deep-links/routes/musd.ts @MetaMask/metamask-earn +**/musd/** @MetaMask/earn +shared/lib/deep-links/routes/musd.ts @MetaMask/earn # Extension Platform owns build, development tooling, # CI, background process, and platform infrastructure. Changes to diff --git a/.github/pull-request-template.md b/.github/pull-request-template.md index d502f8b67a86..ab25a4612c85 100644 --- a/.github/pull-request-template.md +++ b/.github/pull-request-template.md @@ -11,8 +11,6 @@ Write a short description of the changes included in this pull request, also inc 2. What is the improvement/solution? --> -[![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/PR?quickstart=1) - ## **Changelog**