-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Fix switch network popup #25299
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving but we should look into why putting this into shouldShowNetworkInfo
is problematic. Thank you Nidhi!
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #25299 +/- ##
===========================================
- Coverage 65.63% 65.63% -0.00%
===========================================
Files 1375 1375
Lines 54553 54555 +2
Branches 14294 14296 +2
===========================================
+ Hits 35803 35804 +1
- Misses 18750 18751 +1 ☔ View full report in Codecov by Sentry. |
Builds ready [ab1a7b4]
Page Load Metrics (63 ± 8 ms)
Bundle size diffs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. I verified the behavior before and after. And I do get the popup when eventually opening MM
Build from current branch, orangefox test account. What I think might be happening with the network notifications. On the home screen we show notification 'You are now using ***' and user doesn't close it because in order to see this notification user must click on the MM icon or switch to MM tab. When user switches network while having this notification on the home screen then the bug might happen.
notif.mov
btChain.movSometimes the logic for showing notification seems absolutely random. I also observed that sometimes we show notification for such a short time that it is hard to notice but it is still there. fast.movNotification is shown briefly on the home screen: homeNotif.movNotification shown when I connect to sushiswaps and the second notification when switching from Moonbeam to Blast: sushiBlast.mov |
Missing release label release-12.0.0 on PR. Adding release label release-12.0.0 on PR and removing other release labels(release-12.1.0), as PR was cherry-picked in branch 12.0.0. |
This PR is to ensure the network switch modal doesn't show up in a flash when network is switched via the dapp
Related issues
Fixes: #25196
Manual testing steps
Screenshots/Recordings
Before
338417177-79e9c5a7-ba32-4a82-b919-8c65fc27e16f.mov
After
Screen.Recording.2024-06-13.at.2.14.52.PM.mov
Pre-merge author checklist
Pre-merge reviewer checklist