[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
|
@Julesssss @tgolen ready to review! |
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".
Julesssss
left a comment
There was a problem hiding this comment.
Could you add some before/after metrics to this PR please. Also I generated an AdHoc build for the linked App PR and will pass to QA to for some initial testing.
|
baseline delta Worth to note that I'm essentially comparing Onyx from 3.0.58 (merged in E/App main) to the last commit here, which includes more changes so measurements aren't exclusive of the changes here (end result is good though) |
|
Holding briefly, I asked QA to test the associated App AdHoc build. |
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