[WIP][Structural Sharing] PR 4: Reference equality in notification and subscription paths#774
Conversation
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5abe9f855d
ℹ️ 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".
| } | ||
| // Not using waitForCollectionCallback — notify per changed key | ||
| for (const dataKey of changedMemberKeys) { | ||
| if (cachedCollection[dataKey] === previousCollection[dataKey]) { |
There was a problem hiding this comment.
Keep deep-equality check for collection member diffs
This === comparison assumes unchanged collection members keep the same reference, but setCollectionWithRetry() and partialSetCollection() populate updates via cache.set(...), which replaces object references even when values are deep-equal. In those paths, unchanged members are now treated as changed and subscriber callbacks fire unnecessarily, which can trigger extra side effects and render work compared to prior behavior.
Useful? React with 👍 / 👎.
| } else { | ||
| areValuesEqual = shallowEqual(previousValueRef.current ?? undefined, newValueRef.current); | ||
| } | ||
| const areValuesEqual = (previousValueRef.current ?? undefined) === (newValueRef.current ?? undefined); |
There was a problem hiding this comment.
Preserve shallow-equality fast path for non-selector snapshots
Switching non-selector comparison to strict reference equality means shallow-equal objects are now considered changed. When Onyx emits a new object reference with equivalent top-level fields (for example after collection rewrites or forced cache updates), useOnyx will now update resultRef and re-render consumers that previously stayed stable, creating avoidable render churn in production screens without selectors.
Useful? React with 👍 / 👎.
Details
Fourth PR of Expensify/App#86181
E/App PR: Expensify/App#88458
Related Issues
GH_LINK
Automated Tests
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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari