fix: Fix gas fee polling not being stopped for popup UI instances in chrome by jiexi · Pull Request #23594 · MetaMask/metamask-extension

jiexi · 2024-03-20T00:12:30Z

Description

Currently extension popups in chrome do not properly clean up react effects. Previously, gas fee polling started from UI components were also tracked by MetaMaskController and cleaned up when the controller lost connection with the popup (or fullscreen, or notification) UI instances. The new gas fee polling hook failed to account for this and did not properly track its polling tokens with background which introduced the possibility of gas fee polling not stopping correctly in certain instances. This PR fixes this by updating the usePolling hook to add and remove poll tokens from app state and also updates MetaMaskController to stop any active gas fee polling based on the polling tokens in app state.

Related issues

Fixes: https://github.com/MetaMask/MetaMask-planning/issues/2014

Manual testing steps

In chrome

Open a popup instance of extension (click the extension icon from the toolbar)
Open background console network tab
Get to the send transaction confirmation screen
Verify gas fee requests are happening
Close the popup instance by unfocusing it
Verify gas fee requests have stopped

Screenshots/Recordings

Before

After

Pre-merge author checklist

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.

github-actions · 2024-03-20T00:12:42Z

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.

…nnection

ui/pages/confirmations/send/send-content/__snapshots__/send-content.component.test.js.snap

Gudahtt · 2024-03-22T13:38:52Z

ui/hooks/usePolling.ts


    const cleanup = () => {
      if (pollTokenRef.current) {
+        removePollingTokenFromAppState(pollTokenRef.current);


Do we ever want to stop tracking a token without stopping polling? Or the other way around - stop polling without removing a token from the app state? This seems like it should be one single function, rather than two.

Ahh i see. Yeah they should be coupled in another function

done here: 3b4fb8f

apologies. done here instead 029f53e

made more sense to couple in the actions

Gudahtt · 2024-03-22T13:45:24Z

app/scripts/metamask-controller.js

      this.appStateController.store.getState()[appStatePollingTokenType];
    pollingTokensToDisconnect.forEach((pollingToken) => {
      this.gasFeeController.disconnectPoller(pollingToken);
+      this.gasFeeController.stopPollingByPollingToken(pollingToken);


It looks like this is the new polling API, whereas disconnectPoller is the old API. Is that correct? Are we using both of these simultaneously?

there are a few references to the old API still

@jiexi can you remind me where these references are? These use the same polling tokens?

metamask-extension/ui/pages/confirmations/confirm-transaction-base/confirm-transaction-base.component.js

Line 43 in 0a3ef25

getGasFeeEstimatesAndStartPolling,

metamask-extension/ui/ducks/send/send.js

Line 53 in 0a3ef25

getGasFeeEstimatesAndStartPolling,

the poll token from the legacy polling and the networkClientId based polling are not the same

Ok and so its just a noop when we pass one for the new polling to the old and vice versa?

correct. calling disconnectPoller and stopPollingByPollingToken with polling tokens it cannot find is a noop without errors thrown

…nnection

codecov · 2024-03-25T16:41:16Z

Codecov Report

Attention: Patch coverage is 22.22222% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 68.79%. Comparing base (b3d93fa) to head (0570881).

Files	Patch %	Lines
ui/store/actions.ts	28.57%	5 Missing ⚠️
app/scripts/metamask-controller.js	0.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #23594      +/-   ##
===========================================
- Coverage    68.82%   68.79%   -0.03%     
===========================================
  Files         1170     1170              
  Lines        44391    44406      +15     
  Branches     11879    11881       +2     
===========================================
- Hits         30549    30547       -2     
- Misses       13842    13859      +17

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…nnection

metamaskbot · 2024-03-25T18:30:59Z

Builds ready [0570881]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 2, 3, 4, 5, 6
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1420 ± 629 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	74	301	146	58	28
		domContentLoaded	14	151	36	29	14
		load	58	3129	1420	1309	629
		domInteractive	14	151	36	29	14

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 90 Bytes (0.00%)
ui: 61 Bytes (0.00%)
common: 0 Bytes (0.00%)

adonesky1

LGTM!

jiexi added 2 commits March 19, 2024 17:03

tracking polling token in usePolling

ac8bf75

update spec mocks

988285f

jiexi added the team-wallet-api-platform-deprecated DEPRECATED: please use "team-wallet-integrations" instead label Mar 20, 2024

jiexi marked this pull request as ready for review March 20, 2024 00:16

jiexi requested a review from a team as a code owner March 20, 2024 00:16

jiexi requested a review from shanejonas March 20, 2024 00:17

Merge branch 'develop' into jl/fix-gas-fee-polling-hook-background-co…

d7f223e

…nnection

Gudahtt reviewed Mar 22, 2024

View reviewed changes

ui/pages/confirmations/send/send-content/__snapshots__/send-content.component.test.js.snap Outdated Show resolved Hide resolved

Gudahtt reviewed Mar 22, 2024

View reviewed changes

Couple polling token tracking to MMC api

3b4fb8f

jiexi force-pushed the jl/fix-gas-fee-polling-hook-background-connection branch from 602758f to 3b4fb8f Compare March 22, 2024 21:49

jiexi requested review from a team and kumavis as code owners March 22, 2024 21:49

jiexi added 2 commits March 22, 2024 14:50

Merge branch 'develop' into jl/fix-gas-fee-polling-hook-background-co…

7e3812f

…nnection

lint

a8dbb6e

jiexi removed request for a team and kumavis March 22, 2024 22:11

jiexi added 3 commits March 22, 2024 15:42

couple in actions instead

029f53e

Merge branch 'develop' into jl/fix-gas-fee-polling-hook-background-co…

72b8900

…nnection

Merge branch 'develop' into jl/fix-gas-fee-polling-hook-background-co…

bd3473b

…nnection

shanejonas approved these changes Mar 25, 2024

View reviewed changes

Merge branch 'develop' into jl/fix-gas-fee-polling-hook-background-co…

0570881

…nnection

adonesky1 approved these changes Mar 26, 2024

View reviewed changes

jiexi merged commit 3523565 into develop Mar 26, 2024

jiexi deleted the jl/fix-gas-fee-polling-hook-background-connection branch March 26, 2024 17:48

github-actions bot locked and limited conversation to collaborators Mar 26, 2024

metamaskbot added the release-11.15.0 Issue or pull request that will be included in release 11.15.0 label Mar 26, 2024

Conversation

jiexi commented Mar 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

Uh oh!

github-actions bot commented Mar 20, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Mar 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

metamaskbot commented Mar 25, 2024

Uh oh!

adonesky1 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jiexi commented Mar 20, 2024 •

edited

Loading

codecov bot commented Mar 25, 2024 •

edited

Loading