Repository navigation
Deep compare arrays on merge and return old array if unchanged - #860
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@blimpich opened it instead of draft, no review required yet. Sorry for noise |
|
I have read the CLA Document and I hereby sign the CLA |
|
@jmusial could you merge branch with |
Updated, sorry for missing that part. One thing that after |
@jmusial I'm unable to merge because of this. Can you fix this? |
b858d5d to
6845eac
Compare
a50ecc4 to
71c972c
Compare
done |
Details
cache.mergenow keeps the cached array when the incoming one is deep-equal, extending #765's reference stability to arrays. Resent, unchanged data becomes a no-op (#768'smerged === existing), so subscribers aren't notified anduseOnyxdoesn't re-render.shouldKeepEqualArraysmerge option, on only incache.merge.Why: on Expensify/App cold start,
SearchForTodosresends ~1,350 cached transaction violations (arrays), which made App recompute ~1,000 reports (381 ms).Proposal on slack here
Perf (reassure): renders on a deep-equal collection merge 2 → 1; existing merge/update/init benchmarks within noise.
Related Issues
Expensify/App#104725
Linked E/App PR
Expensify/App#104984
Automated Tests
The change is convered by unit tests and perf tests. Details in the code.
Manual Tests
Tested on E/App.
Use an account with a few hundred expenses that have violations. Compare this PR's E/App build with
main, 5 runs each, using the medians. Spans appear in the JS console as[Sentry][<span name>] Ending span (<N>ms).Web (desktop and mobile browsers)
Ending span.OnyxDerivedCompute_reportAttributeslogged after theSearchForTodosrequest finished.ManualNavigateToInboxTab.Native (iOS and Android)
Ending span.OnyxDerivedCompute_reportAttributeslogged after theSearchForTodosrequest finished.ManualNavigateToInboxTab.Author Checklist
### Related Issuessection above### Linked E/App PRsection above, and verified this change against it (E/App CI passed and manual testing completed)TestssectiontoggleReportand 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-native-pr860.mp4
Android: mWeb Chrome
android-chrome-pr860.mp4
iOS: Native
ios-native-pr860.mp4
iOS: mWeb Safari
ios-safari-pr860.mp4
MacOS: Chrome / Safari
web-chrome-pr860.mp4