feat: Allow MM Connect deeplinks to be retried sequentially (MetaMask#30346)

jiexi · web-flow · commit 0cd15ef428e7 · 2026-05-26T22:06:36.000Z
https://consensyssoftware.atlassian.net/browse/WAPI-1386  ## **Description** Currently when a MM Connect deeplink is used once, it cannot be used again even if the connection failed to establish for some reason unless the app is restarted. This PR changes it so that a deeplink can be reused but only if a connection has not already successfully been established with it. ## **Changelog**  CHANGELOG entry: Improves MM Connect deeplink handling ## **Related issues** Fixes: ## **Manual testing steps** ```gherkin Feature: my feature name Scenario: user [verb for user action] Given [describe expected initial app state] When user [verb for user action] Then [describe expected outcome] ``` ## **Screenshots/Recordings**  ### **Before**  ### **After**  ## **Pre-merge author checklist**  - [ ] 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). - [ ] I've completed the PR template to the best of my ability - [ ] I've included tests if applicable - [ ] 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. #### 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](https://consensyssoftware.atlassian.net/wiki/spaces/TL1/pages/edit-v2/401401446401?draftShareId=9d77e1e1-4bdc-4be1-9ebb-ccd916988d93) to import wallets with many accounts and tokens - [ ] I've instrumented key operations with Sentry traces for production performance metrics - See [`trace()`](/app/util/trace.ts) for usage and [`addToken`](/app/components/Views/AddAsset/components/AddCustomToken/AddCustomToken.tsx#L274) for an example For performance guidelines and tooling, see the [Performance Guide](https://consensyssoftware.atlassian.net/wiki/spaces/TL1/pages/400085549067/Performance+Guide+for+Engineers). ## **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] > **Medium Risk** > Touches connection-establishment control flow and capacity eviction ordering; mistakes could lead to skipped connections or unexpected evictions, though changes are localized and covered by new tests. > > **Overview** > **Enables sequential reuse of MM Connect (MWP) deeplinks after failures** by clearing the in-flight deeplink guard (`deeplinks`) in a `finally` block, so the same URL can be retried without restarting the app. > > Adds a duplicate-connection safeguard in `handleConnectDeeplink` to **skip creating a new connection when a connection with the same session `id` already exists**, and updates tests to cover sequential duplicate handling and retry-after-failure behavior. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 3e1f731. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup>
diff --git a/app/core/SDKConnectV2/services/connection-registry.test.ts b/app/core/SDKConnectV2/services/connection-registry.test.ts
@@ -739,6 +739,63 @@ describe('ConnectionRegistry', () => {
       expect(mockStore.save).toHaveBeenCalledTimes(1);
     });
 
+    it('skips creating a connection if one with the same id already exists', async () => {
+      // Given: a registry ready to handle connections
+      registry = new ConnectionRegistry(
+        RELAY_URL,
+        mockKeyManager,
+        mockHostApp,
+        mockStore,
+      );
+
+      // When: handling the same deeplink twice sequentially. After the first
+      // call resolves the in-flight `deeplinks` guard is cleared (see finally
+      // block), so the second call would otherwise proceed — the connection
+      // id duplicate check is what must short-circuit it.
+      await registry.handleConnectDeeplink(validDeeplink);
+      jest.clearAllMocks();
+
+      await registry.handleConnectDeeplink(validDeeplink);
+
+      // Then: the second call must not create a duplicate connection nor
+      // surface a loading state or error to the user.
+      expect(Connection.create).not.toHaveBeenCalled();
+      expect(mockConnection.connect).not.toHaveBeenCalled();
+      expect(mockStore.save).not.toHaveBeenCalled();
+      expect(mockHostApp.showConnectionLoading).not.toHaveBeenCalled();
+      expect(mockHostApp.showConnectionError).not.toHaveBeenCalled();
+      expect(mockHostApp.syncConnectionList).not.toHaveBeenCalled();
+    });
+
+    it('allows retrying the same deeplink URL after a previous attempt fails', async () => {
+      // Given: a registry where the first connect() attempt fails
+      registry = new ConnectionRegistry(
+        RELAY_URL,
+        mockKeyManager,
+        mockHostApp,
+        mockStore,
+      );
+
+      mockConnection.connect.mockRejectedValueOnce(new Error('Connect failed'));
+
+      // First call fails — no connection is registered.
+      await registry.handleConnectDeeplink(validDeeplink);
+      expect(mockHostApp.showConnectionError).toHaveBeenCalledTimes(1);
+
+      jest.clearAllMocks();
+
+      // When: the same URL is submitted again after the failure. The
+      // `deeplinks` in-flight guard must have been cleared in the finally
+      // block, allowing the retry to proceed.
+      await registry.handleConnectDeeplink(validDeeplink);
+
+      // Then: the retry runs the full happy path.
+      expect(Connection.create).toHaveBeenCalledTimes(1);
+      expect(mockConnection.connect).toHaveBeenCalledTimes(1);
+      expect(mockStore.save).toHaveBeenCalledTimes(1);
+      expect(mockHostApp.showConnectionError).not.toHaveBeenCalled();
+    });
+
     it('should handle deeplinks with no payload parameter', async () => {
       // Given: a registry ready to handle connections
       registry = new ConnectionRegistry(
diff --git a/app/core/SDKConnectV2/services/connection-registry.ts b/app/core/SDKConnectV2/services/connection-registry.ts
@@ -301,9 +301,17 @@ export class ConnectionRegistry {
           message: 'External transactions cannot use internal origins',
         });
       }
-      await this.evictIfAtCapacity();
 
       connInfo = this.toConnectionInfo(connReq);
+      if (this.connections.has(connInfo.id)) {
+        logger.debug(
+          'Already have a connection with this id, skipping',
+          redactUrl(url),
+        );
+        return;
+      }
+
+      await this.evictIfAtCapacity();
 
       this.hostapp.showConnectionLoading(connInfo);
       conn = await Connection.create(
@@ -364,6 +372,7 @@ export class ConnectionRegistry {
 
       if (conn) await this.disconnect(conn.id);
     } finally {
+      this.deeplinks.delete(url);
       // Loading-toast dismissal rules:
       // - On failure, always dismiss the loading toast. Otherwise the user
       //   would briefly see both a "loading" toast and the error toast at
