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

sleepytanya · 2024-06-18T20:26:17Z

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

340407819-904bd247-432b-4b71-9465-d3547e498e91

Steps to reproduce

Connect to test-dapp
Click button to start legacy send transaction; popup comes up, I see a "1" in the extension icon, I ignore the popup window
Open new tab, go to Uniswap
Click "Connect" button, click MetaMask; I see a "2" in the extension icon, no popup occurs
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 · 2024-06-18T20:31:30Z

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 · 2024-06-18T20:57:35Z

@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 · 2024-06-18T21:40:41Z

@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 · 2024-06-19T08:37:36Z

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

sleepytanya · 2024-06-20T21:41:09Z

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 · 2024-06-21T20:32:52Z

Findings thus far:

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

sleepytanya · 2024-06-21T20:58:37Z

New error.

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

darkwing · 2024-06-24T14:38:29Z

There's a separate issue for balance destructuring issues.

jiexi · 2024-06-24T19:19:44Z

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 · 2024-06-25T15:10:53Z

@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

…onfirmation popup focus in request queue (#25497)  ## **Description**  [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/25497?quickstart=1) ## **Related issues** See: MetaMask/core#4457 See: #25397 ## **Manual testing steps** 1. Ensure request queue is toggled on 2. In Tab 1, Visit the test dapp 3. run `await window.ethereum.request({method: 'wallet_switchEthereumChain', params: [{"chainId": "0xaa36a7"}]})` in console to change the network to sepolia for this dapp 4. In Tab 2, Visit a different dapp 5. run `await window.ethereum.request({method: 'wallet_switchEthereumChain', params: [{"chainId": "1"}]})` in console to change the network to mainnet for this dapp 6. In Tab 1, click the Send Transaction button, but ignore the confirmation notification (leave it open) 7. In Tab 2, run `await window.ethereum.request({method: 'eth_requestAccounts'})` 8. The confirmation notification window should be focused and visible now, but still be displaying the pending transaction from before ## **Screenshots/Recordings**  ### **Before**  ### **After** https://github.com/MetaMask/metamask-extension/assets/918701/48693083-fe60-402f-8798-c32b5db541b6 ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.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.

jiexi · 2024-06-25T18:37:37Z

Should be fixed by #25514

benjisclowder · 2024-08-12T15:16:49Z

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

sleepytanya added type-bug team-wallet-ux regression-RC-12.0.0 labels Jun 18, 2024

github-project-automation bot added this to Bugs by severity and Bugs by team Jun 18, 2024

github-project-automation bot moved this to To be fixed in Bugs by severity Jun 18, 2024

github-project-automation bot moved this to To be fixed in Bugs by team Jun 18, 2024

This was referenced Jun 18, 2024

[Bug]: Unexpected popup behavior when interacting with dapps #25393

Closed

[Bug]: The popup for the transactions initiated from a dapp are not shown if user has an unapproved transaction #25361

Closed

adonesky1 mentioned this issue Jun 20, 2024

[Bug]: Multichain - The MM popup keeps opening and closing in an infinite loop when I come from an unresponsive network #25408

Closed

vpintorico added the team-wallet-api-platform label Jun 20, 2024

vandan removed the team-wallet-api-platform label Jun 20, 2024

sleepytanya mentioned this issue Jun 20, 2024

[Bug]: Multichain - Cannot destructure property 'balance' of 'K[Z]' as it is undefined. and Cannot destructure property 'balance' of 'accounts[fromAddress]' as it is undefined. #25406

Closed

gauthierpetetin added the Sev1-high High severity; partial loss of service with severe impact upon users, with no workaround. label Jun 24, 2024

danjm added the release-blocker This bug is blocking the next release label Jun 24, 2024

jiexi mentioned this issue Jun 24, 2024

feat: Focus approval request UI when a request is queued MetaMask/core#4456

Merged

3 tasks

jiexi closed this as completed in MetaMask/core#4456 Jun 24, 2024

jiexi closed this as completed in MetaMask/core@9b2b7bc Jun 24, 2024

github-project-automation bot moved this from To be fixed to Fixed in Bugs by team Jun 24, 2024

github-project-automation bot moved this from To be fixed to Fixed in Bugs by severity Jun 24, 2024

jiexi reopened this Jun 24, 2024

github-project-automation bot moved this from Fixed to To be fixed in Bugs by team Jun 24, 2024

github-project-automation bot moved this from Fixed to To be fixed in Bugs by severity Jun 24, 2024

jiexi mentioned this issue Jun 24, 2024

feat: bump @metamask/queued-request controller to ^2.0.0 to fix confirmation popup focus in request queue #25497

Merged

7 tasks

danjm closed this as completed Jun 26, 2024

github-project-automation bot moved this from To be fixed to Fixed in Bugs by severity Jun 26, 2024

github-project-automation bot moved this from To be fixed to Fixed in Bugs by team Jun 26, 2024

gauthierpetetin removed this from Bugs by team Jul 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

sleepytanya commented Jun 18, 2024 •

edited

Loading

jiexi commented Jun 18, 2024

hesterbruikman commented Jun 18, 2024

sleepytanya commented Jun 18, 2024 •

edited

Loading

seaona commented Jun 19, 2024

sleepytanya commented Jun 20, 2024 •

edited

Loading

darkwing commented Jun 21, 2024

sleepytanya commented Jun 21, 2024 •

edited

Loading

darkwing commented Jun 24, 2024

jiexi commented Jun 24, 2024

hesterbruikman commented Jun 25, 2024

jiexi commented Jun 25, 2024

benjisclowder commented Aug 12, 2024

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

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

Comments

sleepytanya commented Jun 18, 2024 • edited Loading

Describe the bug

Expected behavior

Screenshots/Recordings

Steps to reproduce

Error messages or log output

Version

Build type

Browser

Operating system

Hardware wallet

Additional context

Severity

jiexi commented Jun 18, 2024

hesterbruikman commented Jun 18, 2024

sleepytanya commented Jun 18, 2024 • edited Loading

seaona commented Jun 19, 2024

sleepytanya commented Jun 20, 2024 • edited Loading

darkwing commented Jun 21, 2024

sleepytanya commented Jun 21, 2024 • edited Loading

darkwing commented Jun 24, 2024

jiexi commented Jun 24, 2024

hesterbruikman commented Jun 25, 2024

jiexi commented Jun 25, 2024

benjisclowder commented Aug 12, 2024

sleepytanya commented Jun 18, 2024 •

edited

Loading

sleepytanya commented Jun 18, 2024 •

edited

Loading

sleepytanya commented Jun 20, 2024 •

edited

Loading

sleepytanya commented Jun 21, 2024 •

edited

Loading