fix: add getSnapshot memoizedShallowEqual to skip rerenders - #819
fix: add getSnapshot memoizedShallowEqual to skip rerenders#819LukasMod wants to merge 1 commit into
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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.
Should we run wider tests for this change?
|
@Julesssss Do you mean I should add more tests, or some general smoke QA tests? If there’s an option for wider test coverage, that would be great. The issue with this could be that we might rely on those unnecessary rerenders without realizing it, but that should be fixed right there instead |
|
Some additional tests here would be great, wider testing can wait for the full QA regression suite -it'll take a couple of releases due to the merge queue |
Details
useOnyxno longer changes its result identity when the shared snapshot cache holds a content-equal result from another subscriberThe snapshot cache slot is shared by every subscriber of the same
(key, selector)pair, while each subscriber's memoized selector owns a distinct output object. A subscriber that mounts later (its per-hook memoization cache is empty) computes a fresh object, content-equal but referentially new, and publishes it into the slot. Existing subscribers then adopted it blindly in thegetSnapshot()fast path, which changed their result identity and re-rendered their memoized subtrees for no reason. The fast path now keeps the hook's own result when the cached one is content-equal (memoizedShallowEqualon the value plus a status match).Why
We hit this in E/App. In a Concierge chat, every sent message and every Concierge reply re-rendered the whole
ReportActionsListcontent. React DevTools Profiler blamed thereportprop ofReportActionsListItemRenderer, and prop-diff instrumentation showed the tell-tale signature on each message:report prop (stableReport <id>): NEW REFERENCE, but every field is shallow-equal. A pure object-identity change with zero data change.Root cause: every report row (
ActionContentRouter) and the report list itself subscribe to the samereport_<id>key with the same sharedgetStableReportSelector. Every incoming message mounts a new row, which republished a fresh identity into the shared slot. That flipped thereportprop of everyReportActionsListItemRendererand re-rendered the entire chat list on each message, even though the selected data never changed. The stable-report projection exists precisely to prevent this class of re-render, and the shared slot was silently defeating it.Where the fresh object comes from
A projection selector builds a fresh object literal on every call (
return {reportID: report.reportID, ...}), so two calls with the same input produce two content-equal objects with different identities. Identity stability is never provided by the selector itself. It comes from the per-hook memoized wrapper (createMemoizedSelector), which deep-compares each recompute against itslastOutputand returns the old reference when equal. A newly mounted subscriber has nolastOutputyet, so the fresh literal becomes its output and gets published into the shared slot, where the other subscribers' fast paths adopted it.Why a per-call-site selector wrapper also "fixes" it (and why it's not the fix)
The slot key is
${key}_${selectorID}, whereselectorIDis assigned per function identity. Wrapping the shared selector in a module-level function (const stableReportSelectorForList = (report) => getStableReportSelector(report)) mints a new identity, giving that call site a private slot with a single writer and reader, which is the always-working sole-subscriber case. But that only shields one call site. All remaining subscribers of the shared selector keep flipping each other in their shared slot. The fast-path guard fixes the adoption itself, making sharing safe for every consumer without per-call-site workarounds. (The wrapper must be module-level. Defined inside a component it would mint a new identity every render, causing cache-slot churn and a selector recompute per render.)Why this implementation
memoizedShallowEqualcheck onpreviousValueRef. The fast path was the single place adopting a result without any comparison.!==pointer compare. Distinct-but-equal objects pay one top-level walk, memoized by identity pair in thememoizedShallowEqualWeakMap, so N hooks comparing the same two objects pay for one walk total. Real content changes arrive via slot invalidation (recompute path), so the guard rarely sees them.loadedresult never masks this hook'sloadingstate (and vice versa). Value equality alone isn't enough to skip adoption.useOnyxsuite is unaffected.Before:
After:
Related Issues
Expensify/App#95584
Linked E/App PR
Expensify/App#97952
Automated Tests
Manual Tests
No visible changes. Smoke tests:
Test 1: Send messages in open report
Test 2: Rapid report switching
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: mWeb
ChromeiOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Screen.Recording.2026-08-06.at.15.28.59.mov