feat: add navigation skill with MetaMask Mobile overlay - #148
feat: add navigation skill with MetaMask Mobile overlay#148NicolasMassart wants to merge 1 commit into
Conversation
88a8b65 to
452c175
Compare
| | Non-UI navigator | `app/core/NavigationService/` — `NavigationService.navigation` (`navigate` and `reset` are deferred with `requestAnimationFrame`) | | ||
| | Route names | `app/constants/navigation/Routes.ts` | | ||
| | Typed params | `app/core/NavigationService/types.ts` and navigator `ParamList` types under `app/components/Nav/` | | ||
| | Screens | `useNavigation` from `@react-navigation/native` (React Navigation v5) | |
There was a problem hiding this comment.
v7, not v5 — package.json pins @react-navigation/native 7.3.14, native-stack 7.18.6, bottom-tabs 7.18.14, and @react-navigation/stack isn't installed. An agent trusting "v5" will reach for APIs and a package that don't exist.
|
|
||
| - Route names from `Routes`, not string literals. | ||
| - Component screens use `useNavigation`. Non-UI callers (deeplinks, Engine, sagas, `Authentication`) use `NavigationService.navigation`. | ||
| - Typed params. Tests mock `NavigationService` / `@react-navigation/native` as existing tests in that folder already do. |
There was a problem hiding this comment.
Typed params only holds for useNavigation(), which is checked against RootStackParamList via the ReactNavigation.RootParamList merge in app/core/NavigationService/types.ts. NavigationService.navigation is NavigationContainerRef<ParamListBase> — nothing through it is type-checked. Please carve that out, otherwise this reads as type safety on exactly the callers (deeplinks, Authentication) where a wrong param shape fails silently.
| ## Requirements | ||
|
|
||
| - Route names from `Routes`, not string literals. | ||
| - Component screens use `useNavigation`. Non-UI callers (deeplinks, Engine, sagas, `Authentication`) use `NavigationService.navigation`. |
There was a problem hiding this comment.
Non-blocking: components legitimately use NavigationService when there's no navigator context (PerpsTutorialCarousel), and older class screens use props.navigation. Stated this absolutely it invites refactors of code that's fine — a preference reads better than a rule.
2 participants
Description
Adds
navigationin theplatformdomain: sharedskill.md, MetaMask Mobile overlay, and CHANGELOG. Markedbase: trueso it installs with the default base set.Why a shared skill plus a repo overlay.
skill.mdis the workflow every consumer shares.repos/metamask-mobile.mdis the Mobile wiring. A Mobile-only skill would be fewer files today; the next consumer would then copy or fork it. Overlay merge is the existing pattern in this repo.Why
base: true. Engineers who never select theplatformdomain still get this skill onpostinstall, same as the other base skills. Relying on--includeorSKILLS_DOMAINS=platformwould miss most clones. Cost: always-on description tokens.--exclude/SKILLS_EXCLUDEstill skips it.Fixes MCWP-829.
Type of Change
Skill Details (if adding a new skill)
Provider Name: MetaMask
Skill Name: navigation
Brief Description: App navigation with named routes, with a MetaMask Mobile overlay for Routes and NavigationService.
Checklist
Testing
yarn audit:skillsaccepts the new skill frontmatterbase: truestill installs the skill when its domain is filtered out--exclude/SKILLS_EXCLUDEstill skips it when explicitly excludedProof of testing the skill
See the generated skill test report: skill-test-navigation.local.md
Additional Context
MCWP-829
Made with Cursor