[Structural Sharing] PR 3: Frozen collection snapshots with lazy rebuild and per-key merge#768
Conversation
…uctural-sharing-cache-pr-3
Replace mutable collectionData with frozen collectionSnapshots and dirty tracking. Collection snapshots are lazily rebuilt on read, returning stable frozen references for structural sharing. The merge() method now operates per-key instead of spreading the entire storageMap, and hasValueChanged() uses reference equality as a fast path. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
# Conflicts: # lib/OnyxCache.ts
fabioh8010
changed the title
|
@Julesssss @tgolen ready to review! |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f580cee2d
ℹ️ 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".
AndrewGable
requested review from
Julesssss and
tgolen
and removed request for
AndrewGable
| if (this.storageKeys.size > 0) { | ||
| return FROZEN_EMPTY_COLLECTION; | ||
| } | ||
| return undefined; |
There was a problem hiding this comment.
This is feedback from Claude so I could be mistaken, but should we be checking for storage keys from a specific key? Rather than any key.
The decision between "this collection is empty" vs "this collection hasn't loaded yet"
hinges entirely on whether storageKeys has any key in it — from any collection or
non-collection key.
- getCollectionData("report_") is called before the report collection has loaded from
storage.
So in theory, subscribers might temporarily receive an empty object instead of FROZEN_EMPTY_COLLECTION?
| import OnyxKeys from './OnyxKeys'; | ||
|
|
||
| /** Frozen object containing all collection members — safe to return by reference */ | ||
| type CollectionSnapshot = Readonly<NonUndefined<OnyxCollection<KeyValueMapping[OnyxKey]>>>; |
There was a problem hiding this comment.
I understand there's a high cost to deep-freezing the object at runtime, but should we consider using ReadonlyDeep from type-fest to prevent accidental mutations lower in the tree at compile time?
| } | ||
|
|
||
| const snapshot = this.collectionSnapshots.get(collectionKey); | ||
| if (!snapshot || Object.keys(snapshot).length === 0) { |
There was a problem hiding this comment.
micro-optimization: Skip an array allocation of all the keys - it's not necessary for an empty check. Using for-in instead will stop as soon as the first value is found:
diff --git a/lib/OnyxCache.ts b/lib/OnyxCache.ts
index 063ea5eb..6980e69c 100644
--- a/lib/OnyxCache.ts
+++ b/lib/OnyxCache.ts
@@ -538,7 +538,7 @@ class OnyxCache {
}
const snapshot = this.collectionSnapshots.get(collectionKey);
- if (!snapshot || Object.keys(snapshot).length === 0) {
+ if (utils.isEmptyObject(snapshot)) {
// If we know we have storage keys loaded, return a stable empty reference
// to avoid new {} allocations that break useSyncExternalStore === equality.
if (this.storageKeys.size > 0) {
diff --git a/lib/utils.ts b/lib/utils.ts
index 1fca1c8a..efdc5173 100644
--- a/lib/utils.ts
+++ b/lib/utils.ts
@@ -172,7 +172,17 @@ function mergeObject<TObject extends Record<string, unknown>>(
/** Checks whether the given object is an object and not null/undefined. */
function isEmptyObject<T>(obj: T | EmptyValue): obj is EmptyValue {
- return typeof obj === 'object' && Object.keys(obj || {}).length === 0;
+ if (typeof obj !== 'object') {
+ return false;
+ }
+ // Use is for-in loop to avoid using Object.keys to do an unnecessary array allocation
+ // eslint-disable-next-line no-restricted-syntax
+ for (const key in obj) {
+ if (Object.hasOwn(obj, key)) {
+ return false;
+ }
+ }
+ return true;
}
/**This should be faster (O(1) instead of O(n)), particularly for objects with lots of keys
| if (collectionKey && this.collectionData[collectionKey]) { | ||
| delete this.collectionData[collectionKey][key]; | ||
| if (collectionKey) { | ||
| affectedCollections.add(collectionKey); |
There was a problem hiding this comment.
Rather than having a separate affectedCollections variable (which is kind of confusing), could you add these directly to this.dirtyollections?
| * @param collectionKey - The collection key to rebuild | ||
| */ | ||
| private rebuildCollectionSnapshot(collectionKey: OnyxKey): void { | ||
| const oldSnapshot = this.collectionSnapshots.get(collectionKey); |
There was a problem hiding this comment.
Please rename to previousSnapshot or currentSnapshot. I don't think "old" is as helpful.
| const needsPrefixCheck = !memberKeys; | ||
|
|
||
| for (const key of keysToScan) { | ||
| if (needsPrefixCheck && OnyxKeys.getCollectionKey(key) !== collectionKey) { |
There was a problem hiding this comment.
Please add a code comment to explain why this early continue is there (I'm not sure I understand why).
| const oldSnapshot = this.collectionSnapshots.get(collectionKey); | ||
|
|
||
| const members: NonUndefined<OnyxCollection<KeyValueMapping[OnyxKey]>> = {}; | ||
| let hasChanges = false; |
There was a problem hiding this comment.
Could you rename this to be a little more descriptive? What has changes? The old snapshot?
| // Check if any members were removed (old snapshot had more keys) | ||
| if (!hasChanges && oldSnapshot) { | ||
| const oldMemberCount = Object.keys(oldSnapshot).length; | ||
| if (oldMemberCount !== newMemberCount) { |
There was a problem hiding this comment.
Is this logic safe? What if Key A was removed and Key B was added? The counts would be the same, but the snapshot would be different.
4 participants
Details
Third PR of Expensify/App#86181
E/App PR: Expensify/App#86885
Related Issues
Expensify/App#86181
Automated Tests
Unit tests were added.
Manual Tests
General testing over the app. Use Expensify/App#86885 for testing:
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
Screen.Recording.2026-04-03.at.15.07.16-compressed.mov
Android: mWeb Chrome
N/A, chrome is always crashing in my emulator
iOS: Native
Screen.Recording.2026-04-03.at.15.13.56-compressed.mov
iOS: mWeb Safari
Screen.Recording.2026-04-03.at.15.17.41-compressed.mov
MacOS: Chrome / Safari
Screen.Recording.2026-04-03.at.15.20.27-compressed.mov
Screen.Recording.2026-04-03.at.15.23.32-compressed.mov