-
Notifications
You must be signed in to change notification settings - Fork 581
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
Debounce state updates #3258
Debounce state updates #3258
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3258 +/- ##
=======================================
Coverage 94.98% 94.98%
=======================================
Files 511 511
Lines 11299 11313 +14
Branches 1738 1741 +3
=======================================
+ Hits 10732 10746 +14
Misses 567 567 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Marking as draft because this needs some further discussion about the debounce timeout. |
Does this means we merge updates or that we skip one if they are sent too close to each others ? |
@GuillaumeRx We always persist the entire state object, not partial updates. If there are multiple state updates in a row, we only persist the last (most up to date) one now. |
I guess this is okay as this does not really change how the API previously worked but I'm worried that Snaps making multiple non-related updates to the state in a row might start having some state updates missing ? 🤔 |
It should only be the persistence that is affected, the actual state update is put into memory right away |
c24a7f4
to
5a19f0c
Compare
newSnapState, | ||
encrypted, | ||
); | ||
readonly #persistSnapState = debounce( |
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 just realized, we should debounce per Snap ID and encrypted true/false otherwise we will have data loss with this
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.
Good catch, didn't think about encrypted true/false.
This changes the persisting logic in the Snap Controller to debounce state updates for each Snap individually by 500 milliseconds. This means that if the Snap updates state multiple times within 500 milliseconds, the state is only persisted once, 500 milliseconds after the last update.