[Structural Sharing] PR 2: Reference-stable fastMerge and removeNestedNullValues#765
Conversation
Add hasChanged tracking to mergeObject() and removeNestedNullValues() so they return the original target/value reference when nothing actually changed. This avoids unnecessary object allocations and enables downstream consumers to use reference equality checks to skip redundant re-renders and cache invalidations. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca1a65d173
ℹ️ 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".
| } | ||
|
|
||
| return result as TValue; | ||
| return hasChanged ? (result as TValue) : value; |
There was a problem hiding this comment.
Keep removeNestedNullValues from returning caller-owned object
Returning value unchanged when hasChanged is false makes Onyx retain the exact object reference passed by the caller. setWithRetry() and storage-preparation paths use removeNestedNullValues() and then cache/store that result directly, so later in-place mutations of the original input can silently mutate cached state without going through Onyx update/broadcast flow (and hasValueChanged can miss it because cache and input now alias). Before this commit, this helper always produced a fresh object/array container, which prevented that reference leakage.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Before removeNestedNullValues only returned a new shallow container for objects — nested references were still shared with the caller. So any deep mutations in E/App or Onyx would have leaked through regardless.
This change doesn't make the situation worse, it just avoids a redundant shallow copy when nothing was removed.
|
ready to review @tgolen @Julesssss |
Details
Second PR of Expensify/App#86181
E/App PR: Expensify/App#86636
Related Issues
Expensify/App#86181
Automated Tests
Unit tests were added.
Manual Tests
General testing over the app. Use Expensify/App#86636 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-03-30.at.11.49.35-compressed.mov
Android: mWeb Chrome
N/A, Chrome always crash in my emulator.
iOS: Native
Screen.Recording.2026-03-30.at.11.55.44-compressed.mov
iOS: mWeb Safari
Screen.Recording.2026-03-30.at.12.03.10-compressed.mov
MacOS: Chrome / Safari
Screen.Recording.2026-03-30.at.12.05.33-compressed.mov
Screen.Recording.2026-03-30.at.12.08.11-compressed.mov