fix: Use custom state manager for cronjob controller#3539
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3539 +/- ##
=======================================
Coverage 98.26% 98.26%
=======================================
Files 411 411
Lines 11619 11624 +5
Branches 1810 1810
=======================================
+ Hits 11417 11422 +5
Misses 202 202 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
packages/snaps-controllers/src/cronjob/CronjobController.test.ts
Outdated
Show resolved
Hide resolved
c846098 to
0672fed
Compare
| ...state, | ||
| ...stateManager.getInitialState(), |
There was a problem hiding this comment.
I guess this means we will always overwrite an obsolete state after the first init but that looks fine for now.
There was a problem hiding this comment.
getInitialState may be undefined
There was a problem hiding this comment.
Yeah and we still need to recover the state the first time
There was a problem hiding this comment.
Bug: State Initialization Fails When Undefined is Spread
The CronjobController constructor's state initialization attempts to spread stateManager.getInitialState(), which can return undefined. Spreading undefined in an object literal causes a TypeError at runtime. Additionally, the state merge order (...state, ...stateManager.getInitialState()) means that properties from the state constructor parameter are silently overridden by those from stateManager.getInitialState() if keys conflict.
packages/snaps-controllers/src/cronjob/CronjobController.ts#L183-L188
snaps/packages/snaps-controllers/src/cronjob/CronjobController.ts
Lines 183 to 188 in 0672fed
Was this report helpful? Give feedback by reacting with 👍 or 👎
This updates the cronjob controller to add a custom
stateManagerproperty for persisting state. This is a temporary workaround for the controller persisting state too often, causing the entire client state to be written.