-
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: notification slowness and crashes #25946
fix: notification slowness and crashes #25946
Conversation
making multiple calls to snaps can cause internal 429 responses and requests become queued. This adds simple in-memory caching to prevent calling the snap with same values
interesting bug that happens with communication with our controllers. Removing the calls to disable profile sync, and instead we will add this edge case on the UI or better orchestrate comms.
as notifications are coming from multiple data sources, if a data source is retrying, then notifications will take longer to load.
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. |
Builds ready [f8f0dae]
Page Load Metrics (237 ± 243 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [5f76720]
Page Load Metrics (151 ± 191 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #25946 +/- ##
===========================================
- Coverage 69.70% 69.67% -0.03%
===========================================
Files 1400 1400
Lines 49464 49510 +46
Branches 13687 13689 +2
===========================================
+ Hits 34478 34494 +16
- Misses 14986 15016 +30 ☔ View full report in Codecov by Sentry. |
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.
I found two edge cases regarding state management, but they involve behaviors that a user normally wouldn’t apply and, in any case, they don’t cause issues in managing the extension’s state. We can approve this PR and look into further optimizing this page in the future :)
Quality Gate passedIssues Measures |
app/scripts/controllers/metamask-notifications/services/feature-announcements.ts
Show resolved
Hide resolved
app/scripts/controllers/metamask-notifications/services/feature-announcements.ts
Show resolved
Hide resolved
app/scripts/controllers/metamask-notifications/metamask-notifications.ts
Show resolved
Hide resolved
Builds ready [3902451]
Page Load Metrics (70 ± 9 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
More than one release label on PR. Keeping the lowest one (release-12.0.0) on PR and removing other release labels (release-12.3.0). |
Description
This is a series of fixes added to core libraries. Adding to extension, and soon we will migrate extension to use shared libraries.
Related issues
MetaMask/core#4530
MetaMask/core#4531
MetaMask/core#4532
MetaMask/core#4533
MetaMask/core#4536
#25749
Manual testing steps
Before: some settings would error or not load
After: you should be able to toggle accounts and not see errors.
Screenshots/Recordings
Before
After
https://www.loom.com/share/49b582e8c33b4199bdafc994a3f6087f?sid=9f94a885-351b-4fee-84d8-f72c97506e7d
Pre-merge author checklist
Pre-merge reviewer checklist