fix(mm_connect): report deeplink failures to Sentry#30343
Conversation
The SDK-V2 connection registry's catch blocks for handleMwpDeeplink and handleConnectDeeplink were only routing errors through a local logger that calls console.error with a prefix. The app's Sentry init does not include captureConsoleIntegration, so these errors (including the "Failed to handle connect deeplink" string) were silently dropped and invisible in production observability — leaving the failure_reason property on the REMOTE_CONNECTION_REQUEST_FAILED MetaMetrics event as the only signal, with no stack trace. Wire both catch sites to Logger.error from app/util/Logger so failures hit captureException via a Sentry scope, with searchable tags (feature, operation) and contextual data (redacted URL, dapp metadata, SDK version/platform). The existing logger.error console output is kept so local debugging is unchanged.
|
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. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c451bf2. Configure here.
| ); | ||
| }); | ||
|
|
||
| it('should report dispatch failures to Sentry via Logger.error', async () => { |
There was a problem hiding this comment.
New test names use banned "should" prefix
Low Severity
All three new test names start with "should", which is banned by the unit testing guidelines ("NEVER use 'should' in test names — this is a hard rule with zero exceptions"). The tests 'should report dispatch failures to Sentry via Logger.error', 'should report connect failures to Sentry with dapp/sdk context', and 'should report parse failures to Sentry even when no connection request is available' need action-oriented names instead, e.g. 'reports dispatch failures to Sentry via Logger.error'.
Additional Locations (2)
Triggered by project rule: Unit Testing Guidelines
Reviewed by Cursor Bugbot for commit c451bf2. Configure here.
The feature tag now uses the public-facing product name "MetaMask Connect" (mm-connect) so Sentry dashboards/alerts align with the external naming. Added an inline comment at both catch sites acknowledging that the surrounding code (directory `SDKConnectV2/`, class `ConnectionRegistry`, log prefix `[SDKConnectV2]`) still uses the older nomenclature and needs to migrate.
🔍 Smart E2E Test Selection
click to see 🤖 AI reasoning detailsE2E Test Selection:
No E2E tags are needed — this is a pure observability/error-reporting improvement with no risk to existing functionality. Performance Test Selection: |
|
Description
The SDK-V2
ConnectionRegistrycatches errors inhandleMwpDeeplinkandhandleConnectDeeplinkand routes them through a locallogger(app/core/SDKConnectV2/services/logger.ts) that just prefixes and callsconsole.error. The app's Sentry init (app/util/sentry/utils.ts) only wires updedupeIntegrationandextraErrorDataIntegration— there's nocaptureConsoleIntegration— so those errors (including the well-known\"Failed to handle connect deeplink\"string) are silently dropped and never reach Sentry in production.The only existing signal for these failures is the
failure_reasonproperty on theREMOTE_CONNECTION_REQUEST_FAILEDMetaMetrics event, which is useful for aggregate counts but provides no stack trace, no breadcrumbs, and no context.This PR wires both deeplink-entry catch sites to
Logger.errorfromapp/util/Logger, which already callscaptureExceptioninside a Sentry scope, respects the metrics opt-in, and supports tags/context. The existinglogger.errorconsole output is kept so local debugging is unchanged (andLogger.erroris a no-op in__DEV__anyway).What's reported
Scope is intentionally limited to the two deeplink-entry catches — the most user-impactful failures, triggered directly by an incoming deeplink. The other `console.error`-only sites in the registry (`initialize`, `evictIfAtCapacity`, `reconnectAll`, `handleSimpleDeeplink` 'not found' log) can be addressed in a follow-up if useful.
Changelog
CHANGELOG entry: null
Related issues
Fixes:
Companion PR (provides the QA test surface): feat(playground): add MWP deeplink failure repro panel.
Manual testing steps
Setup
Feature: SDK-V2 deeplink failure observability
Scenario: parse failure produces a Sentry event with gracefully missing dapp metadata
Given the device is set up per the Setup steps above
When the user taps the Tap on mobile button on the Payload is not valid JSON row
Then the MetaMask Mobile app opens and shows a Connection failed toast
And a `REMOTE_CONNECTION_REQUEST_FAILED` MetaMetrics event fires with `failure_reason` containing `SyntaxError` or similar JSON parse text
And a Sentry event is captured with:
- tag `feature: mm-connect`
- tag `operation: handle_connect_deeplink`
- context `mwp_deeplink.url` ending in `[REDACTED]`
- context `mwp_deeplink.dapp_url`, `dapp_name`, `sdk_version`, `sdk_platform` all `undefined` (because parsing failed before `connReq` was assigned)
Scenario: handshake failure produces a Sentry event with full dapp/sdk context
Given the device is set up per the Setup steps above
When the user taps the Tap on mobile button on the Control: well-formed connect deeplink row
Then the deeplink parses successfully (the dapp metadata is well-formed) but the relay handshake will time out / fail because no wallet is on the other end of the fake session id
And a Sentry event is captured with:
- tag `feature: mm-connect`
- tag `operation: handle_connect_deeplink`
- context `mwp_deeplink.dapp_url: "https://playground.metamask.io\"\`
- context `mwp_deeplink.dapp_name: "MMC Playground (QA Repro)"`
- context `mwp_deeplink.sdk_version: "0.0.0-qa-repro"`
- context `mwp_deeplink.sdk_platform: "JavaScript"`
Scenario: each remaining failure branch is observable
Given the device is set up per the Setup steps above
When the user taps each of the remaining rows (No payload param, Payload parses but is not a ConnectionRequest, sessionRequest.id is not a UUID, dapp.url claims to be an internal origin, c=1 flag but plain payload, Payload over 1MB)
Then each one produces both a `REMOTE_CONNECTION_REQUEST_FAILED` MetaMetrics event and a Sentry event tagged `feature: mm-connect`, `operation: handle_connect_deeplink`
Scenario (regression): existing deeplink flow is unchanged
Given the device has an existing persisted MWP connection from a real dapp
When the user re-opens the dapp and triggers the resume connection flow (simple deeplink with `?id=…`)
Then nothing in the connection flow has changed; no new Sentry events are emitted unless the inner flow actually throws
Verifying in Sentry
In the project Sentry dashboard:
Screenshots/Recordings
N/A — observability-only change, no user-visible UI difference.
Before
Failures in MWP deeplink dispatch logged only to the device console; no Sentry event.
After
Failures captured in Sentry with searchable tags and dapp/sdk context. Existing console.error preserved for local debugging.
Pre-merge author checklist
Pre-merge reviewer checklist