Skip to content

chore(nav): type wallet, asset, and settings shell navigators (Phase 3)#33306

Open
weitingsun wants to merge 3 commits into
mainfrom
chore/navigation-typing-phase-3-shells-wallet-settings
Open

chore(nav): type wallet, asset, and settings shell navigators (Phase 3)#33306
weitingsun wants to merge 3 commits into
mainfrom
chore/navigation-typing-phase-3-shells-wallet-settings

Conversation

@weitingsun

@weitingsun weitingsun commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Description

Phase 3 of the incremental React Navigation typing migration — app shell navigators.

Shared navigation hosts (AssetStackFlow, WalletTabHome, SettingsView, TrendingView, BrowserTabHome, Webview, SetPasswordFlow, ImportPrivateKeyView, RootModalFlow, Home, Main) were still loosely typed as NestedNavigationParams (or equivalent { screen: string; params?: object }), so nested navigate(container, { screen, params }) calls into those shells could not be checked against real screen param shapes.

This PR adds shell-level param lists and wires them into RootStackParamList via NavigatorScreenParams, matching the Phase 3 pattern already used for feature stacks (Perps / Rewards / Predict / Card / Earn / etc.).

This is primarily types-only — no intentional navigation architecture change. One small Onboarding call-site cleanup aligns SuccessErrorSheet params with the typed sheet contract (primaryButtonLabel).

What changed

Shell param lists (app/components/Nav/Main/types/navigation.ts)

  • Shells 1: AssetStackParamList, WalletTabStackParamList / WalletTabHomeParamList, SettingsStackParamList
  • Shells 2: TrendingViewStackParamList, BrowserTabHomeParamList, WebviewStackParamList, SetPasswordFlowParamList, ImportPrivateKeyStackParamList
  • Shells 3: MoneyTabStackParamList, TransactionsHomeParamList, RewardsHomeParamList, HomeTabsParamList, MainStackParamList, MainFlowParamList

Root modal flow (app/core/NavigationService/types.ts)

  • RootModalFlowParamList — screens registered inside App’s RootModalFlow
  • RootModalFlow / Home / Main (and the Shells 1–2 hosts above) → NavigatorScreenParams<…>

Param accuracy / call-site cleanup

  • Tightened stub shapes for TransactionDetailsSheetParams and MultichainTransactionDetailsParams to match what those sheets actually receive
  • Onboarding SuccessErrorSheet navigations now pass primaryButtonLabel (was incorrect buttonLabel)

Changelog

CHANGELOG entry:null

Related issues

Fixes:https://consensyssoftware.atlassian.net/browse/MCWP-674

Manual testing steps

N/A

Screenshots/Recordings

N/A

Pre-merge author checklist

Performance checks (if applicable)

  • I've tested on Android
    • Ideally on a mid-range device; emulator is acceptable
  • I've tested with a power user scenario
    • Use these power-user SRPs to import wallets with many accounts and tokens
  • I've instrumented key operations with Sentry traces for production performance metrics

For performance guidelines and tooling, see the Performance Guide.

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

Note

Low Risk
Compile-time navigation typing and param-shape fixes; the only runtime tweak is correcting SuccessErrorSheet button props on onboarding error flows.

Overview
Phase 3 of the React Navigation typing migration: app-shell navigators that were still NestedNavigationParams (or loose { screen, params }) now have dedicated param lists and are wired into RootStackParamList via NavigatorScreenParams.

A new navigation.ts defines shell lists for asset/wallet/settings stacks, home tabs, main flow, browser/webview/set-password/import flows, and related hosts. NavigationService/types.ts adds RootModalFlowParamList and replaces loose typing on Main, Home, RootModalFlow, SettingsView, WalletTabHome, AssetStackFlow, and similar routes with nested NavigatorScreenParams.

Modal route stubs are tightened: TransactionDetailsSheetParams and MultichainTransactionDetailsParams match what those sheets actually receive (full tx/display payloads instead of optional transactionId only).

Onboarding SuccessErrorSheet navigations now pass primaryButtonLabel instead of buttonLabel, with matching test updates—aligning call sites with the typed sheet contract.

Reviewed by Cursor Bugbot for commit cb64ba0. Bugbot is set up for automated code reviews on this repo. Configure here.

@github-actions

Copy link
Copy Markdown
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@metamask-ci metamask-ci Bot added the team-mobile-platform Mobile Platform team label Jul 14, 2026
@metamask-ci

metamask-ci Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

PR template — items to address before "Ready for review"

Warnings — informational, address before merging:

  • Pre-merge author checklist has unchecked items (e.g. "I've tested on Android"). Every box must be consciously checked — see docs/readme/ready-for-review.md.

See docs/readme/ready-for-review.md for the full Definition of Ready for Review.

@github-actions

Copy link
Copy Markdown
Contributor

🔍 Smart E2E Test Selection

  • Selected E2E tags: SmokeSeedlessOnboarding, SmokeWalletPlatform, SmokeConfirmations, SmokeNetworkExpansion, SmokeAccounts
  • Selected Performance tags: None (no tests recommended)
  • Risk Level: medium
  • AI Confidence: 82%
click to see 🤖 AI reasoning details

E2E Test Selection:

Changes analyzed:

  1. app/components/Views/Onboarding/index.tsx + test — Renames buttonLabelprimaryButtonLabel in error sheet navigation params for OAuth errors and no-internet-connection errors during seedless onboarding. This is a functional change to the error UI in the onboarding flow. → SmokeSeedlessOnboarding

  2. app/components/Views/Modals/Modals.types.tsTransactionDetailsSheetParams changed from a simple transactionId?: string to a rich typed object with tx: TransactionMeta, transactionElement, transactionDetails, showSpeedUpModal, showCancelModal. This is a significant functional change to how the TransactionDetailsSheet modal is invoked — any call site passing the old shape would break. → SmokeWalletPlatform (transaction history), SmokeConfirmations (transaction details/speed-up/cancel flows)

  3. app/components/Views/MultichainAccounts/MultichainAccounts.types.tsMultichainTransactionDetailsParams changed from transactionId?: string to displayData: MultichainTransactionDisplayData + transaction: Transaction. Affects multichain account transaction detail navigation. → SmokeNetworkExpansion (Solana/non-EVM accounts), SmokeAccounts (account management)

  4. app/core/NavigationService/types.ts (CRITICAL) + app/components/Nav/Main/types/navigation.ts (NEW) — Large TypeScript refactoring: replaces NestedNavigationParams with proper NavigatorScreenParams<T> generics across all major navigation stacks. Also moves RootModalFlowParams inline as RootModalFlowParamList. These are TypeScript-only type changes with no runtime behavior change — they improve type safety but don't alter navigation logic. No additional tags needed beyond those already selected for functional changes.

Tag dependency checks:

  • SmokeConfirmations: standalone, no required co-tags from description for this scenario
  • SmokeWalletPlatform: standalone for transaction history
  • SmokeNetworkExpansion: standalone for multichain accounts
  • SmokeAccounts: standalone for account management
  • SmokeSeedlessOnboarding: standalone for onboarding flows

Performance tests: No performance-sensitive code paths changed. All changes are TypeScript type definitions or a minor prop rename in error handling. No performance tags warranted.

Performance Test Selection:
All changes are TypeScript type definitions (navigation param lists, modal params) or a minor prop rename (buttonLabel → primaryButtonLabel) in error handling UI. None of these changes affect rendering performance, data fetching, or app launch/load times. No performance tests are warranted.

View GitHub Actions results

@github-actions

Copy link
Copy Markdown
Contributor

🧪 Flaky unit test detection

Run history flaky detection

View recent run history

Historical failure rate is a hint, not proof — review each suggestion in context. See the flaky-test-detection skill for the full pattern reference and manual audit workflow.

Failures / runs sampled per window:

File 7d 15d 30d
app/components/Views/Onboarding/index.test.tsx 0/113 0/174 0/373

AI-detected flaky patterns

app/components/Views/Onboarding/index.test.tsx

  • J6 — Arbitrary setTimeout/sleep used as a synchronization barrier (high)
    • flushPromises relies on setImmediate (a real timer) as a zero-delay synchronization barrier after async OAuth flows and navigation. This is non-deterministic and a known source of flakiness (see J6 in skill; also appears 321 times across the codebase). The small PR diff only updates error sheet prop names and does not touch this logic. Historical data shows 0 failures but the pattern is present and risky.
    • Suggested fix in app/components/Views/Onboarding/index.test.tsx:37:
      -const flushPromises = () => new Promise((resolve) => setImmediate(resolve));
      -
      -... (used in multiple tests e.g.)
      -      await act(async () => {
      -        await googleOAuthFunction(true);
      -        await flushPromises();
      -        await flushPromises();
      -      });
      -
      +const flushPromises = () => new Promise((resolve) => setImmediate(resolve)); // remove entirely
      +
      +// In test bodies replace with:
      +      await act(async () => {
      +        await googleOAuthFunction(true);
      +      });
      +      await waitFor(() => {
      +        expect(mockAnalytics.trackEvent).toHaveBeenCalledWith(
      +          MetaMetricsEvents.SOCIAL_LOGIN_COMPLETED,
      +          expect.any(Object)
      +        );
      +      });
      +
  • J9 — Module-level mutable let bindings not reset in beforeEach (high)
    • mockHasTestOverrides is a module-level mutable let (shared across all tests). It is set to true in the E2E/migration tests but the top-level beforeEach() only does jest.clearAllMocks() and a few specific mockReturnValue calls — no reset to false. This creates order-dependent behavior (J9). The PR changes do not address shared state isolation.
    • Suggested fix in app/components/Views/Onboarding/index.test.tsx:58:
      -let mockHasTestOverrides = false;
      -jest.mock('../../../util/test/utils', () => ({
      -  ...jest.requireActual('../../../util/test/utils'),
      -  hasTestOverrides: () => mockHasTestOverrides,
      -}));
      -
      -// later in test:
      -mockHasTestOverrides = true;
      +let mockHasTestOverrides = false;
      +
      +beforeEach(() => {
      +  jest.clearAllMocks();
      +  mockHasTestOverrides = false; // explicit reset
      +  mockCreateEventBuilder.mockClear();
      +  // ...
      +});
      +
  • J10 — jest.spyOn() without restoreAllMocks() afterward (medium)
    • Multiple tests (showNotification, disableBackPress, componentDidMount) use jest.spyOn(Animated, 'timing'), jest.spyOn(BackHandler, 'addEventListener'), and similar without jest.restoreAllMocks() (or mockRestore) in afterEach. clearAllMocks() does not restore original implementations. Spies therefore leak across tests (J10; 219 files in codebase exhibit this). No historical failures but matches the pattern exactly.
    • Suggested fix in app/components/Views/Onboarding/index.test.tsx:3120:
      -beforeEach(() => {
      -  jest.useFakeTimers();
      -  jest.clearAllMocks();
      -  jest.spyOn(BackHandler, 'addEventListener').mockImplementation(() => ({
      -    remove: jest.fn(),
      -  }));
      -});
      -
      -afterEach(() => {
      -  jest.useRealTimers();
      -});
      +beforeEach(() => {
      +  jest.useFakeTimers();
      +  jest.clearAllMocks();
      +  jest.spyOn(BackHandler, 'addEventListener').mockImplementation(() => ({
      +    remove: jest.fn(),
      +  }));
      +});
      +
      +afterEach(() => {
      +  jest.useRealTimers();
      +  jest.restoreAllMocks();
      +});
      +

This check is informational only and does not block merging.

@sonarqubecloud

Copy link
Copy Markdown

@weitingsun weitingsun marked this pull request as ready for review July 15, 2026 18:26
@weitingsun weitingsun requested review from a team as code owners July 15, 2026 18:26
@github-actions github-actions Bot added the risk:high AI analysis: high risk label Jul 15, 2026

@Cal-L Cal-L left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@weitingsun weitingsun self-assigned this Jul 15, 2026
@weitingsun weitingsun enabled auto-merge July 16, 2026 01:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

risk:high AI analysis: high risk size-M team-mobile-platform Mobile Platform team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants