Skip to content

Fix open in browser#300

Merged
brunobar79 merged 1 commit into
masterfrom
fix-open-in-browser
Jan 9, 2019
Merged

Fix open in browser#300
brunobar79 merged 1 commit into
masterfrom
fix-open-in-browser

Conversation

@estebanmino

Copy link
Copy Markdown
Contributor

Description

This PR fixes #290

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

Issue

Resolves #???

@brunobar79 brunobar79 self-requested a review January 9, 2019 20:23

@brunobar79 brunobar79 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.

👍

@brunobar79 brunobar79 merged commit c29d1a1 into master Jan 9, 2019
@brunobar79 brunobar79 deleted the fix-open-in-browser branch February 11, 2019 21:42
rickycodes pushed a commit that referenced this pull request Jan 31, 2022
pull Bot pushed a commit to Dustin4444/metamask-mobile that referenced this pull request May 21, 2026
## **Description**

The SDK-V2 `ConnectionRegistry` catches errors in `handleMwpDeeplink`
and `handleConnectDeeplink` and routes them through a local `logger`
(`app/core/SDKConnectV2/services/logger.ts`) that just prefixes and
calls `console.error`. The app's Sentry init
(`app/util/sentry/utils.ts`) only wires up `dedupeIntegration` and
`extraErrorDataIntegration` — there's no `captureConsoleIntegration` —
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_reason`
property on the `REMOTE_CONNECTION_REQUEST_FAILED` MetaMetrics 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.error` from
`app/util/Logger`, which already calls `captureException` inside a
Sentry scope, respects the metrics opt-in, and supports tags/context.
The existing `logger.error` console output is kept so local debugging is
unchanged (and `Logger.error` is a no-op in `__DEV__` anyway).

### What's reported

- **Tags** (searchable in Sentry):
- \`feature: 'mm-connect'\` — intentionally uses the public-facing
product name even though the directory/class still use the older
\`SDKConnectV2\` nomenclature. There's an inline comment in the code
making this explicit; the internal naming needs to migrate but tagging
Sentry with the public name now avoids renaming dashboards/alerts later.
  - \`operation: 'handle_mwp_deeplink'\` | \`'handle_connect_deeplink'\`
- **Context** (\`mwp_deeplink\`):
  - \`url\` (redacted via \`redactUrl\` — query/fragment stripped)
- For \`handle_connect_deeplink\`: \`dapp_url\`, \`dapp_name\`,
\`sdk_version\`, \`sdk_platform\` (when the connection request parsed
successfully; gracefully \`undefined\` when parse failed)

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](MetaMask/connect-monorepo#300).

## **Manual testing steps**

> Easiest way to exercise every branch reproducibly: use the companion
playground PR linked above. It adds a collapsible *QA: MWP deeplink
failure repros* card with one tappable deeplink per failure branch.
Without that PR you can still test by manually constructing deeplinks
(URLs documented in the playground PR description and table).

### Setup

1. Install a debug build of this PR's branch on a real device (Sentry is
disabled in E2E / when \`MM_SENTRY_DSN\` is missing — see
\`app/util/sentry/utils.ts\` \`setupSentry\`).
2. Accept the metrics opt-in during onboarding (Sentry
\`captureException\` only fires when \`METRICS_OPT_IN === AGREED\`).
3. From a desktop browser, deploy or run the [playground branch from
connect-monorepo PR
MetaMask#300](MetaMask/connect-monorepo#300) and open it
on the device's mobile browser. Expand the *QA: MWP deeplink failure
repros* section.

### 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:

1. Filter by \`feature:mm-connect\` → only the new events from this PR
should appear.
2. Filter by \`operation:handle_connect_deeplink\` vs
\`handle_mwp_deeplink\` to separate the connect-flow failures from the
wrapper-dispatch failures.
3. On any event, the *Additional Data* panel should show the
\`mwp_deeplink\` context block with the redacted URL and dapp/sdk
fields.

## **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**

- [x] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile
Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] I've included tests if applicable
- [x] I've documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I've applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants