[Bug]: Bring popup to the foreground functionality is not working as expected.

[sleepytanya]

Describe the bug

When a user has an unapproved transaction pending, initiating a connect request does not trigger the expected popup to appear.
After the first request is rejected and the subsequent request can be accessed MM can break with an error.
Related issues:

#25393

#25361

Slack discussion links:

https://consensys.slack.com/archives/C03ETQA9EPK/p1695933147044299

https://consensys.slack.com/archives/C03ETQA9EPK/p1695932531166009

Expected behavior

All necessary popups are displayed appropriately.

Screenshots/Recordings

Screen.Recording.2024-06-17.at.10.36.41.AM.mov

Steps to reproduce

1. Connect to test-dapp
2. Click button to start legacy send transaction; popup comes up, I see a "1" in the extension icon, I ignore the popup window
3. Open new tab, go to Uniswap
4. Click "Connect" button, click MetaMask; I see a "2" in the extension icon, no popup occurs
5. Click "Connect" button again, click MetaMask; I see a "3" in the extension icon, no popup occurs

Error messages or log output

No response

Version

12.0.0

Build type

None

Browser

Chrome

Operating system

MacOS

Hardware wallet

No response

Additional context

No response

Severity

No response

[jiexi]

To be clear here, subsequent requests in steps 4 and 5 should cause a popup that shows the pending confirmation from step 2 to show up

[hesterbruikman]

@sleepytanya @jiexi To make sure I understand the issue, is this right?

GIVEN A pending transaction request
AND MetaMask is closed
AND Initiate a dapp connection
THEN Count goes up, but popup does not open automatically

It looks like the confirmation from step 2 is shown when opening MM manually, but expected is for the popup to open automatically. Is that right?

[sleepytanya]

@hesterbruikman Right!
If you open popup manually by clicking MM icon you'll see pending transaction. There is no navigation for the user to be able to see another pending request.

You can also reject unapproved transaction from the homepage. But if you reject from there the next request will be displayed in fullscreen:

Screen.Recording.2024-06-18.at.17.35.28.mov

@hesterbruikman Also while at the full screen homepage MM sometimes breaks with an error:

[seaona]

ℹ️ I've added repro steps for the Cannot destructure property 'balance' of 'K[Z]' as it is undefined here

[sleepytanya]

Latest build [https://github.com/MetaMask/metamask-extension/commit/eebc4d5b2284024eaeb190cddfa89990f593ca34]

Popup behavior didn't change:

• only the first popup displayed
• Cannot destructure property 'balance' of 'K[Z]' as it is undefined is present
• Cannot read properties of undefined (reading 'permission') is present
• Cannot destructure property 'balance' of 'account[fromAddress]' as it is undefined is present

connectTxTx.mov

Screen.Recording.2024-06-20.at.17.43.05.mov

[darkwing]

Findings thus far:

1. The function that ultimately shows the popup is triggerUi in app/scripts/background.js
2. triggerUi is called when NOTIFICATION_MANAGER_EVENTS.POPUP_CLOSED is signaled and when MetamaskController's showUserConfirmation is called.
3. Clicking "Send Transaction" in test dapp on the current network brings forth the popup every time
4. Switch to a tab connected to a different network and triggering a transaction (ex: PancakeSwap swap) does not trigger triggerUi via MetamaskController's showUserConfirmation
5. It's clear that MetamaskController's showUserConfirmation only calls triggerUi when the confirmation is on the currently selected network
6. showUserConfirmation is called by ApprovalController's showApprovalRequest

[sleepytanya]

New error.

1. Start send+swap txs with low gas
2. Send another (approve) tx from a dapp
3. Confirmation is stuck on the spinner
4. Activity tab has two txs - Pending and Unapproved
5. Unapprovedtxs can't be rejected
6. If user rejects first tx MM displays an error Cannot destructure property 'balance' of 'Q[z]' as it is undefined.

[darkwing]

There's a separate issue for balance destructuring issues.

[jiexi]

The issue is the QueuedRequestController will only allow requests to continue on to the ApprovalController if all currently unapproved transactions belong to the same networkClientId. Once a request (that requires confirmation) is received for a networkClientId that differs from the current set of unapproved transactions, that transaction and all transactions afterward (including those matching the original networkClientId with unapproved transactions still visible and scrubbable by the user) are held in the request queue until the current set of unapproved transactions for the first networkClientId are approved/rejected.

This means that the notification window will not be focused starting with the first transaction that differs from the existing unapproved transactions batch.

I believe the fix here is to expose the showUserConfirmation/triggerUi hook to either the QueuedRequestMiddleware or Controller and call that each time a request is queued.

[hesterbruikman]

@jiexi is my understanding correct that your PR fixes the issue: Popup does not appear when a new confirmation request is received, for a networkClientId that differs from the current set of unapproved transactions?

If so it seems like we can close this issue when your PR gets merged.

There might be residual work related to queue batches not represented clearly in the UI, but that seems like a larger project and out of scope for v12.

cc @darkwing

[jiexi]

Should be fixed by #25514

[benjisclowder]

Collaborative Effort Required for Root Cause Analysis (RCA) on Critical Issues

We are quickly approaching the end of the quarter and we encourage you once again to take some moments and perform RCA on this critical issue. You may do so by answering the questions below:

1. What PR fixed the issue?
2. Can you pinpoint the commit from which the issue originated?
3. Write a short explanation of the technical cause of the bug
4. How could we have avoided merging this bug? What would have had to be different about our code, tests or processes?
4.1. Were there any missing unit, e2e or manual tests that could have preempted this issue?
4.2. Were there any other elements lacking, such as typed code, comprehensive documentation, well-architected APIs, etc., that might have prevented this issue?
4.3. If your answer to a and b is no, then is there anything at all that you can think of that, if it had been different before this bug was introduced, would have prevented it from being merged?

Please provide your answers as a reply to this comment and tag me as well.

You can read more about the initiative here. Thank you!

Tagging eng. who added the fix: @jiexi