Load all storage data into cache during Onyx.init#752
Load all storage data into cache during Onyx.init#752fabioh8010 wants to merge 10 commits intoExpensify:mainfrom
Conversation
|
Reassure output The 🔴 are expected because of this PR very purpose – load all storage data into cache during startup. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c251c8440
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| for (const [key, value] of pairs) { | ||
| // RAM-only keys should never be loaded from storage as they may have stale persisted data | ||
| // from before the key was migrated to RAM-only. | ||
| if (isRamOnlyKey(key)) { | ||
| continue; | ||
| } | ||
| allDataFromStorage[key] = value; |
There was a problem hiding this comment.
Exclude skippable members when loading all storage keys
During init we now preload every stored pair into allDataFromStorage, but this loop only filters RAM-only keys. If a collection member ID is listed in skippableCollectionMemberIDs and already exists on disk from a previous session, it will now be inserted into cache and delivered to subscribers after restart, which breaks the skip contract and can reintroduce the large-data/perf problems this option is meant to avoid. The previous lazy get/multiGet paths explicitly filtered these IDs, so the eager-load path needs the same check.
Useful? React with 👍 / 👎.
|
Web tests well. Mobile app testing currently blocked by this bug on main |
Details
This PR replaces Onyx's lazy per-key cache population strategy with a single bulk read of the entire storage database during init(), and removes the cache eviction system that is no longer needed.
E/App PR: Expensify/App#85210
Related Issues
Expensify/App#85252
Automated Tests
Unit tests added/changed to cover the changes.
Manual Tests
Author Checklist
### Related Issuessection aboveTestssectiontoggleReportand notonIconClick)myBool && <MyComponent />.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)Avataris modified, I verified thatAvataris working as expected in all cases)mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
android.mov
Android: mWeb Chrome
We couldn't record for Android web as emulator crashes randomly.
iOS: Native
ios.mov
iOS: mWeb Safari
ios-web.mov
MacOS: Chrome / Safari
web.mov
MacOS: Desktop