fix: Fix gas fee polling not being stopped for popup UI instances in chrome #23594
Conversation
|
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. |
ui/pages/confirmations/send/send-content/__snapshots__/send-content.component.test.js.snap
Outdated
Show resolved
Hide resolved
ui/hooks/usePolling.ts
Outdated
| @@ -20,6 +24,7 @@ const usePolling = (usePollingOptions: UsePollingOptions) => { | |||
|
|
|||
| const cleanup = () => { | |||
| if (pollTokenRef.current) { | |||
| removePollingTokenFromAppState(pollTokenRef.current); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ahh i see. Yeah they should be coupled in another function
There was a problem hiding this comment.
apologies. done here instead 029f53e
made more sense to couple in the actions
| @@ -5687,6 +5688,7 @@ export default class MetamaskController extends EventEmitter { | |||
| this.appStateController.store.getState()[appStatePollingTokenType]; | |||
| pollingTokensToDisconnect.forEach((pollingToken) => { | |||
| this.gasFeeController.disconnectPoller(pollingToken); | |||
| this.gasFeeController.stopPollingByPollingToken(pollingToken); | |||
There was a problem hiding this comment.
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 was a problem hiding this comment.
there are a few references to the old API still
There was a problem hiding this comment.
@jiexi can you remind me where these references are? These use the same polling tokens?
There was a problem hiding this comment.
There was a problem hiding this comment.
the poll token from the legacy polling and the networkClientId based polling are not the same
There was a problem hiding this comment.
Ok and so its just a noop when we pass one for the new polling to the old and vice versa?
There was a problem hiding this comment.
correct. calling disconnectPoller and stopPollingByPollingToken with polling tokens it cannot find is a noop without errors thrown
jiexi
force-pushed
the
jl/fix-gas-fee-polling-hook-background-connection
branch
from
602758f to
3b4fb8f
Compare
Codecov ReportAttention: Patch coverage is
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. |
Builds ready [0570881]
Page Load Metrics (1420 ± 629 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
metamaskbot
added
the
release-11.15.0
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
usePollinghook 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
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist