Skip to content

Conversation

@smilingkylan
Copy link
Contributor

@smilingkylan smilingkylan commented Nov 19, 2025

Description

Fixed critical bugs and test failures in the Universal Router feature flag integration introduced in the previous PR. The changes ensure proper fallback behavior to the legacy deep link handling system and correct TypeScript type compliance in tests.

Key Changes

  1. Fixed delegateToLegacy behavior: Changed return value from { handled: true } to { handled: false } to properly signal when the new router cannot handle a link, allowing the legacy system to process it.

  2. Corrected feature flag architecture: Moved feature flag checks from inside the UniversalRouter.route() method to the UniversalRouterIntegration.processWithNewRouter() layer, ensuring proper system-level and action-level gating before router initialization.

  3. Fixed test expectations: Updated all test files to match the corrected behavior:

    • UniversalRouter.test.ts: Updated delegateToLegacy expectations
    • UniversalRouterIntegration.test.ts: Updated feature flag mock implementations
    • platformNewLinkHandler.test.ts: Added missing cacheTimestamp property to mock state
  4. Resolved linting issues: Fixed trailing spaces, missing semicolons, and namespace import warnings.

Why These Changes Were Needed

The original implementation had an architectural flaw where delegateToLegacy() returned handled: true even though it didn't actually handle the link. This caused links to fail silently instead of falling back to the legacy system. Additionally, feature flag checks were performed at the wrong architectural layer, leading to incorrect fallback behavior when individual actions were disabled.

Trade-offs

  • Removed the isNewHandlerEnabled() private method in favor of centralized feature flag checking in the integration layer, which is more architecturally sound but requires all feature flag logic to live in one place.
  • Changed from toEqual() to toMatchObject() in some tests to allow for additional metadata fields, making tests slightly less strict but more maintainable.

CHANGELOG entry

CHANGELOG entry: null

Related issues

https://consensyssoftware.atlassian.net/jira/polaris/projects/MWMR/ideas/view/8845676?selectedIssue=MWMR-10

Manual testing steps

Test Case 1: System flag disabled

Given the platformNewLinkHandlerSystem flag is false
When a deep link is opened (e.g., metamask://home)
Then the legacy system processes the link successfully

Test Case 2: Action flag disabled

Given the platformNewLinkHandlerSystem flag is true
And the platformNewLinkHandlerActions.home flag is false
When a home deep link is opened (e.g., metamask://home)
Then the legacy system processes the link successfully

Test Case 3: Both flags enabled

Given the platformNewLinkHandlerSystem flag is true
And the platformNewLinkHandlerActions.home flag is true
When a home deep link is opened (e.g., metamask://home)
Then the new Universal Router handles the link successfully

Screenshots/Recordings

N/A - Backend logic changes only, no UI changes.

Pre-merge author checklist

  • I've followed MetaMask Contributor Docs and MetaMask Mobile Coding Standards
  • I've completed the PR template to the best of my ability
  • I've included tests: unit / integration tests / e2e tests
  • I've tested my contribution both manually and with tests on iOS and Android
  • I've ensured my commits are well organized and follow Conventional Commits format
  • I've documented my code using TSDoc format where appropriate

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 issue it closes and includes the necessary testing evidence such as recordings and or screenshots.

Suggested PR Title: fix: correct Universal Router feature flag fallback behavior

Branch Name: fix/universal-router-feature-flag-fixes (or with issue number if applicable)

Labels to Add:

  • team-[your-team] (required)
  • bug
  • feature-flag

Note

Routes deep links through the new Universal Router when system/action flags are enabled and correctly falls back to legacy when unhandled; adds selectors and updates tests.

  • Deep link handling:
    • Integrate UniversalRouterIntegration into handleUniversalLink to attempt new routing first; call handled() and return when successful.
    • Update UniversalRouter.delegateToLegacy to return { handled: false, metadata: { reason: 'no_handler' } } and remove legacy adapter usage.
  • Feature flags:
    • Add selectors selectPlatformNewLinkHandlerSystemEnabled and selectPlatformNewLinkHandlerActions under selectors/featureFlagController/platformNewLinkHandler.
    • Use these selectors in UniversalRouterIntegration to gate by system and per-action flags; normalize URL to determine action before routing; initialize and route via UniversalRouter.
  • Tests:
    • Adjust UniversalRouter.test.ts expectations to handled: false with reason: 'no_handler' when unhandled/fallback.
    • Update UniversalRouterIntegration.test.ts to mock new selectors and validate gated routing.
    • Add tests for new selectors in platformNewLinkHandler.test.ts.

Written by Cursor Bugbot for commit aac826e. This will update automatically on new commits. Configure here.

@github-actions
Copy link
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.

@metamaskbot metamaskbot added the team-mobile-platform Mobile Platform team label Nov 19, 2025
@smilingkylan smilingkylan marked this pull request as ready for review November 19, 2025 21:46
@smilingkylan smilingkylan requested a review from a team as a code owner November 19, 2025 21:46
@smilingkylan smilingkylan added no-changelog no-changelog Indicates no external facing user changes, therefore no changelog documentation needed no changelog required No changelog entry is required for this change labels Nov 19, 2025
@metamaskbot metamaskbot added the INVALID-PR-TEMPLATE PR's body doesn't match template label Nov 19, 2025
@MetaMask MetaMask deleted a comment from cursor bot Nov 19, 2025
@sonarqubecloud
Copy link

@github-actions
Copy link
Contributor

🔍 Smart E2E Test Selection

  • Selected E2E tags: SmokeAccounts, SmokeCore, SmokeConfirmationsRedesigned, SmokeIdentity, SmokeNetworkAbstractions, SmokeNetworkExpansion, SmokeTrade, SmokeWalletPlatform, SmokeWalletUX, SmokeAssets, SmokeSwaps, SmokeStake, SmokeCard, SmokeNotifications, SmokeRewards, SmokePerps, SmokeRamps, SmokeMultiChainPermissions, SmokeAnalytics, SmokeMultiChainAPI, SmokePredictions
  • Risk Level: high
  • AI Confidence: %
click to see 🤖 AI reasoning details

Fallback: AI analysis did not complete successfully. Running all tests.

View GitHub Actions results

if (wasHandledByNewRouter) {
handled();
return;
}
Copy link

Choose a reason for hiding this comment

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

Bug: Missing handled callback in legacy deeplink flow

The handled() callback is only invoked when the new router processes the link, but not when falling back to the legacy system. Previously, handled() was called unconditionally after the modal check. Now when wasHandledByNewRouter returns false, the legacy switch-case executes without ever calling handled(), breaking the deeplink handling flow and preventing proper cleanup or state management that depends on this callback.

Fix in Cursor Fix in Web

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

INVALID-PR-TEMPLATE PR's body doesn't match template no changelog required No changelog entry is required for this change no-changelog no-changelog Indicates no external facing user changes, therefore no changelog documentation needed size-M team-mobile-platform Mobile Platform team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants