From b25d4e4810ecd2730d24d198f3616ea440292655 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Thu, 5 Nov 2026 21:09:17 +0000 Subject: [PATCH 1/2] docs(evals): ui-development reshape eval for #212 and #213 Co-authored-by: George Marshall --- .../ui-development-reshape-2026-11-05.md | 478 ++++++++++++++++++ 1 file changed, 478 insertions(+) create mode 100644 docs/evals/ui-development-reshape-2026-11-05.md diff --git a/docs/evals/ui-development-reshape-2026-11-05.md b/docs/evals/ui-development-reshape-2026-11-05.md new file mode 100644 index 00000000..a5c0b6cb --- /dev/null +++ b/docs/evals/ui-development-reshape-2026-11-05.md @@ -0,0 +1,478 @@ +# UI development skills reshape eval — 2026-11-05 + +PRs: [#212](https://github.com/MetaMask/skills/pull/212) (Mobile), [#213](https://github.com/MetaMask/skills/pull/213) (Extension) +Agent: Cursor cloud agent, eval branch `cursor/ui-development-reshape-eval-682b` + +Skill heads under test: + +- #212 `cursor/mmds-mobile-skills-refresh-7309` @ `2d1ba46` +- #213 `cursor/mmds-extension-skills-no-storybook-mcp-7309` @ `9c57432` +- A/B baseline: `main` @ `c40ef31` + +Consumers (shallow clones of `main`, 2026-11-05): + +| Consumer | MMDS package (lockfile) | Shared | Preset | +| --- | --- | --- | --- | +| metamask-mobile | `@metamask/design-system-react-native` 0.55.0 (npm latest 0.56.0) | `design-system-shared` 0.44.0 | `design-system-twrnc-preset` 0.14.0 | +| metamask-extension | `@metamask/design-system-react` 0.47.0 (npm latest 0.51.0) | `design-system-shared` 0.41.0 | `design-system-tailwind-preset` 0.15.0 | + +Storybook MCP and Context Forge were treated as unavailable and were never configured. + +## Summary + +| Scenario | Mobile (#212) | Extension (#213) | +| --- | --- | --- | +| 1. Types win over manifest | Pass | Pass | +| 2. Restyle with foundations | Pass | Partial (ISS-006, ISS-008) | +| 3. Legacy* migration | Pass | Partial (ISS-009) | +| 4. Review checklist | Pass | Pass | +| Install smoke | Pass (ISS-002 pre-existing) | Pass (ISS-002 pre-existing) | + +Rubric: in all eight runs the agent did not block on Storybook MCP, read installed types before choosing anything, used the matching hosted manifest for discovery only, and followed no hard-coded MMDS version. The two Partials are overlay gaps, not wrong guidance about sources. + +Issues: 13 total. 3 major (ISS-003, ISS-009, ISS-011), 7 minor, 3 notes, 0 blockers. + +Merge verdict from an agent-eval perspective: **safe to merge together, not separately.** + +- The shared base does what the reshape promises. "Installed types win" correctly refused `ButtonPrimary` (Mobile) and `inputAriaLabel` (Extension), which exist on MMDS `main` and in the manifests but not at the pinned 0.55.0 / 0.47.0. +- Merge #212 and #213 back to back (ISS-003). Either one alone ships a contradictory skill to the other client. +- Before or right after merge, fix ISS-009 in the #213 Extension overlay (enum alias path, about two lines). +- Reconcile ISS-011 in Notion: the Active Design Brain rule ranks MMDS `main` above the installed package. That is a Notion change, not a skill change. +- Everything else can follow up. + +## Issues + +### ISS-001 — No full `yarn install` of either consumer; types come from the published tarballs +- Platform: Install +- Scenario: Setup +- Severity: note +- Expected: `node_modules` from a real `yarn install` of each app. +- Actual: I did not run the full app installs (Mobile has native and postinstall steps, Extension has LavaMoat allow-scripts). Instead I extracted the npm tarballs at the exact lockfile versions into `/node_modules/@metamask/` (`design-system-react-native@0.55.0`, `design-system-shared@0.44.0`, `design-system-twrnc-preset@0.14.0`, `design-tokens@13.0.0` for Mobile; `design-system-react@0.47.0`, `design-system-shared@0.41.0`, `design-system-tailwind-preset@0.15.0`, `design-tokens@12.0.0` for Extension). +- Evidence: `npm pack @metamask/@` then `tar xzf`. Every `node_modules` path the skills name resolved, for example `dist/components/index.d.cts` for both packages. +- Suggested fix: None for the skills. Type-checking the proposed snippets (`tsc`) was not possible without the full install; the snippets were checked by reading the `.d.cts` files. + +### ISS-002 — Installed "Strategy" links are broken in every installed copy +- Platform: Shared (pre-existing on `main`) +- Scenario: Install smoke +- Severity: minor +- Expected: `[Agentic Design System Strategy](...)` in the installed `SKILL.md` / `RULE.md` resolves to the copied `knowledge/agentic-design-system-strategy.md`. +- Actual: The base links `../../knowledge/agentic-design-system-strategy.md` and the Extension overlay links `../../../knowledge/agentic-design-system-strategy.md`. Both are source-tree paths. After install the file lives at `./knowledge/agentic-design-system-strategy.md` next to `SKILL.md`, so both links point outside the skill folder. `references/reading-mmds-manifests.md` resolves correctly. +- Evidence: In `metamask-mobile/.claude/skills/mms-ui-development/` and `metamask-extension/.claude/skills/mms-ui-development/`, `test -e ../../knowledge/...` and `test -e ../../../knowledge/...` both fail, and `test -e knowledge/agentic-design-system-strategy.md` succeeds. The `main` install has the same two links. +- Suggested fix: Rewrite `../../knowledge/` and `../../../knowledge/` to `knowledge/` in `tools/install` when it merges base and overlay (the installer already copies `knowledge/`), or drop the duplicate Strategy link from the Extension overlay. Not caused by #212/#213. + +### ISS-003 — #212 and #213 are coupled: merging either one alone ships a contradictory skill to the other platform +- Platform: Shared +- Scenario: Install smoke (cross-PR) +- Severity: major +- Expected: Each PR can merge on its own without degrading the other client. +- Actual: Both PRs add the same new base `skill.md`, `references/reading-mmds-manifests.md`, and knowledge edits (byte-identical, separate commits `b931b41` and `260dac1`), but each one reshapes only its own overlay. + - With #212 alone, Extension gets the new base ("never block on it, do not install, configure, or authenticate it") followed by the old `main` overlay's "## Required Storybook MCP Workflow" ("Start by querying `storybook-broker-mcp` with `list-all-documentation`"). The installed file has 363 lines and two "When to use" sections and two checklists. + - With #213 alone, Mobile gets the new base plus the old 430-line Mobile overlay, which still says "**LAST RESORT**: Custom components with StyleSheet". That contradicts the new base and the new overlay's "No `StyleSheet.create()`". The installed file has 639 lines. +- Evidence: `cd <#212 worktree> && ./tools/install --repo metamask-extension --target /tmp/x-ext --domain ui`, then `grep -n "Required Storybook MCP" /tmp/x-ext/.claude/skills/mms-ui-development/SKILL.md` matches line 278. With the #213 worktree and `--repo metamask-mobile`, line 25 of the overlay section contains "LAST RESORT: Custom components with StyleSheet". +- Suggested fix: Merge #212 and #213 together, back to back with no release in between, or move the base, reference, and knowledge changes into one PR that also carries both overlays. Positive: merging #212 and then #213 onto `main` is conflict-free (`git merge-tree` is clean), and the shared CHANGELOG bullet appears once. + +### ISS-004 — Manifest `name` is not always a component name; "pattern" entries look like components +- Platform: Shared +- Scenario: 1 (Types win over manifest) +- Severity: minor +- Expected: `reading-mmds-manifests.md` lists every kind of non-component entry so an agent that "matches on `name`" does not mistake one for an exportable component. +- Actual: The reference warns that "Some entries are design-token or foundation pages rather than components". Both manifests also contain **pattern** stories whose `name` is either a component-looking identifier (`patterns-marketing-moments` → `"MarketingMoment"`, with an `import` of `Box, Button, Icon, Text`) or a code fragment (`patterns-backgrounds` / `patterns-wallet-home` → `"() => null"`, and `components-headerstandardanimated` → `"HeaderStandardAnimated as unknown as ComponentType"`). A name search for `MarketingMoment` finds an entry, but neither installed package exports it. Mobile has `app/components/UI/MarketingMomentTitle` and Extension has `ui/components/ui/marketing-moment` instead, which the Extension overlay does point to. +- Evidence: `jq -r '.components[] | select(.name|test("^[A-Z][A-Za-z]+$")|not) | "\(.id)\t\(.name)"' components.json` on both manifests; `jq '.components[] | select(.name=="MarketingMoment") | {id,path,import}'`. +- Suggested fix: In the reference's Shape section, add: "Entries whose `id` starts with `patterns-` are composition examples, not components. Check `path` (a `src/components//` story is a component) and confirm the name in the installed export index." Optionally name the Mobile `MarketingMomentTitle` path in the Mobile overlay, the way the Extension overlay names `marketing-moment`. + +### ISS-005 — Deprecation rule does not cover a named replacement that the installed package does not export +- Platform: Extension (rule is in the shared base) +- Scenario: 1 (Types win over manifest) +- Severity: minor +- Expected: The skill tells the agent what to do when `@deprecated` names a replacement that is missing from the installed version or exists only on the other platform. +- Actual: The base says "Use the replacement the JSDoc names; if none is named, keep existing usage and record the gap." In `@metamask/design-system-react@0.47.0` (Extension), `ButtonFilter` is tagged `@deprecated Use \`FilterButton\` instead`, and the React manifest says the same. `FilterButton` is not exported by React 0.47.0 or 0.51.0 (latest) and has no React manifest entry. It exists only in React Native. Read literally, the rule sends the agent to a component that does not exist on this platform, which the same base forbids ("never port an API from the other platform"). Extension uses `ButtonFilter` in `ui/components/app/perps/dropdown/dropdown.tsx` and `perps-market-category-pill.tsx`. +- Evidence: `node_modules/@metamask/design-system-react/dist/components/ButtonFilter/ButtonFilter.d.cts:4`; `grep -c FilterButton dist/components/index.d.cts` returns `0` at 0.47.0 and at 0.51.0; `jq '[.components[]|select(.name=="FilterButton")]|length' react-manifest.json` returns `0`. +- Suggested fix: Change the base sentence to "Use the replacement the JSDoc names **if the installed platform package exports it**; otherwise keep existing usage and record the gap." Report the dangling React deprecation to MMDS. + +### ISS-006 — "Do not create or modify SASS or SCSS files" blocks deleting SCSS that a Tailwind restyle makes obsolete +- Platform: Extension +- Scenario: 2 (Restyle with foundations) +- Severity: minor +- Expected: The overlay lets the agent delete the SCSS rule, file, and `@import` that a restyle replaces. Leaving dead SCSS behind is worse. +- Actual: The styling surface says "Do not create or modify SASS or SCSS files", and the checklist says "No SASS or SCSS files created or modified." Restyling `ui/components/multichain/funding-method-modal/funding-method-item.tsx` to Tailwind replaces `.funding-method-item { cursor: pointer; &:hover { background-color: var(--color-background-hover) } }` in `funding-method-modal/index.scss`. Cleaning up means deleting that file and the `@import 'funding-method-modal';` line in `ui/components/multichain/multichain-components.scss:27`. Read literally, that edit fails the checklist, so an agent either leaves dead CSS or breaks the rule. +- Evidence: Extension overlay, "Styling surface" and "Extension checklist"; `grep -rn funding-method-modal ui --include=*.scss`. +- Suggested fix: Change the rule to "Do not add SASS or SCSS. Deleting rules, files, and `@import`s that your change makes unused is expected." Make the same change in the checklist. + +### ISS-007 — Mobile overlay says "no new `.styles.ts`" but not how to retire an existing `useStyles` + `.styles.ts` pair +- Platform: Mobile +- Scenario: 2 (Restyle with foundations) +- Severity: note +- Expected: A short conversion rule for the very common existing pattern (`useStyles(styleSheet, {})` from `app/components/hooks/useStyles` plus a `*.styles.ts` that calls `StyleSheet.create`). +- Actual: The overlay forbids `StyleSheet.create()` and new `.styles.ts` files, but says nothing about converting an existing pair. The `main` overlay had a "Component Conversion Guide" table. I could still do the conversion from the base and overlay rules (map to typed `Box` props or `twClassName`, then delete the styles file and the hook call). Also, "Interactive list rows use MMDS `ListItem` with its interactive mode and the pressed background token" reads as if the agent should add `bg-pressed`. In 0.55.0, `ListItem` with `isInteractive` already applies `pressed && 'bg-pressed'` (`dist/components/ListItem/ListItem.mjs:34`), so adding it again is redundant. +- Evidence: `app/components/UI/Ramp/components/MenuItem/MenuItem.{tsx,styles.ts}`; Mobile overlay "Styling surface" and "Interactive list rows…" bullets. +- Suggested fix: Add one bullet: "When you touch a file that uses `useStyles` + `*.styles.ts`, move the styles to typed `Box` props or `twClassName` and delete the styles file and hook call." Reword the `ListItem` bullet to "…with `isInteractive`; it applies the pressed background itself." + +### ISS-008 — Extension overlay has no guidance for clickable rows or for typed `Box` props versus `className` +- Platform: Extension +- Scenario: 2 (Restyle with foundations) +- Severity: minor +- Expected: Guidance on the common "clickable list row" case and on whether MMDS React `Box` typed props (`flexDirection`, `gap`, `padding`, `alignItems`) are preferred over raw `className` utilities, matching the guidance the Mobile overlay gives. +- Actual: The React package (0.47.0) has no `ListItem` (the React manifest has none either; it is React Native only). The overlay only says "Use Tailwind class names (`className`) for styling and layout." The real target is a legacy `Box` with `onClick` and no role or keyboard support. An agent has to decide on its own to use `Box asChild` around a native ` + +); +``` + +- `hover:bg-hover` and `active:bg-pressed` are preset-generated (`design-system-tailwind-preset/dist/colors.mjs:16-17`). There are no arbitrary values, and Tailwind removes the default palette and font sizes (`tailwind.config.js:23-30`). +- The cleanup deletes `funding-method-modal/index.scss` and its `@import` in `multichain-components.scss:27`. This literally "modifies SCSS" (ISS-006). +- Semantics improved: a native `button` replaces the clickable `div`. The overlay has no clickable-row pattern (ISS-008). + +### Scenario 3 — Legacy* migration + +**Mobile (#212) — Pass** (with note ISS-012; Notion status ISS-010) + +| MCP | Types | Manifest | Overlay | No pin | +| --- | --- | --- | --- | --- | +| ✅ | ✅ | ✅ | ✅ | ✅ | + +Target: `app/components/UI/Notification/SwitchLoadingModal/Loader.tsx`, which imports legacy default `Text` + `TextVariant` from `component-library/components/Texts/Text`, legacy default `Icon` + `IconSize`/`IconName`/`IconColor`, and MMDS `Button`. The incremental step migrates `Text` only: + +```tsx +import { + Button, + ButtonSize, + ButtonVariant, + Text, + TextVariant, +} from '@metamask/design-system-react-native'; +import LegacyIcon, { + IconColor as LegacyIconColor, + IconName as LegacyIconName, + IconSize as LegacyIconSize, +} from '../../../../component-library/components/Icons/Icon'; +// … + + {errorContent} + +``` + +- `TextVariant.BodyMD` becomes `TextVariant.BodyMd` (`design-system-shared/dist/types/Text/Text.types.d.cts:47`). The hand-set `lineHeight: 20` is dropped because the variant sets it (base: "Never hand-set … line heights"). +- The overlay's default-export example (`import LegacyText, { TextVariant as LegacyTextVariant } …`) maps directly onto `Icon`. Aliasing `Icon` here follows the base wording literally. Repository practice would leave it alone until MMDS `Icon` is imported (ISS-012). +- Finishing the step later: swap `LegacyIcon` for MMDS `Icon` (`IconColor.Error` → `IconColor.ErrorDefault`) and remove the `Legacy*` import. +- Notion: the `Legacy*` rule is **Proposed** (2026-11-04) while the skill enforces it as Active (ISS-010). + +**Extension (#213) — Partial** (ISS-009) + +| MCP | Types | Manifest | Overlay | No pin | +| --- | --- | --- | --- | --- | +| ✅ | ✅ | ✅ | ⚠️ overlay misses the enum path | ✅ | + +Target: `funding-method-item.tsx` (above), incremental step migrating `Text` only: + +```tsx +import { Icon, IconName, IconSize, Text, TextColor, TextVariant } from '@metamask/design-system-react'; +import { Box, Text as LegacyText } from '../../component-library'; +import { + AlignItems, + Display, + FlexDirection, + TextColor as LegacyTextColor, + TextVariant as LegacyTextVariant, +} from '../../../helpers/constants/design-system'; +``` + +- Legacy `Box` keeps its plain name because MMDS `Box` is not imported in this step (ISS-012 reading). `LegacyText` is needed only if some `Text` usages stay legacy. +- The enum aliases (`TextVariant`, `TextColor`) are the real clash, and they come from `helpers/constants/design-system`, a path the overlay never mentions (ISS-009). `Display`/`FlexDirection`/`AlignItems` are legacy-only and keep their names. +- Notion: same Proposed status as Mobile (ISS-010). + +### Scenario 4 — Review checklist + +**Mobile (#212) — Pass** + +| MCP | Types | Manifest | Overlay | No pin | +| --- | --- | --- | --- | --- | +| ✅ | ✅ | ✅ | ✅ | ✅ | + +Invented patch, a new `app/components/UI/Ramp/components/FeeSheet/FeeSheet.tsx`: + +```tsx +1 import React from 'react'; +2 import { ScrollView, StyleSheet, View } from 'react-native'; +3 import { +4 BottomSheet, +5 ButtonBase, +6 ButtonIcon, +7 ButtonPrimary, +8 IconName, +9 Text, +10 TextVariant, +11 } from '@metamask/design-system-react-native'; +12 +13 const styles = StyleSheet.create({ row: { padding: 15, borderRadius: 16 } }); +14 +15 export const FeeSheet = ({ onClose, onConfirm, fee }: Props) => ( +16 +17 undefined} /> +18 +19 +20 {fee} +21 +22 +23 ({ opacity: pressed ? 0.5 : 1 })} onPress={onConfirm}>Back +24 Confirm +25 +26 ); +``` + +Review findings, following the base Review workflow and the Mobile checklist: + +| # | File:line | Finding | Evidence | Fix | +| --- | --- | --- | --- | --- | +| 1 | `FeeSheet.tsx:7,24` | `ButtonPrimary` is not exported by the installed package (invented or future API). | `dist/components/index.d.cts` at 0.55.0 has no `ButtonPrimary`; it ships in 0.56.0. | `