fix: add getSnapshot memoizedShallowEqual to skip rerenders - #819
Open
LukasMod wants to merge 1 commit into
Open
fix: add getSnapshot memoizedShallowEqual to skip rerenders#819LukasMod wants to merge 1 commit into
LukasMod wants to merge 1 commit into
Conversation
Author
|
@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". |
This was referenced Aug 5, 2026
LukasMod
marked this pull request as ready for review
August 7, 2026 06:13
arosiclair
approved these changes
Aug 7, 2026
Julesssss
approved these changes
Aug 7, 2026
Julesssss
left a comment
Contributor
There was a problem hiding this comment.
Should we run wider tests for this change?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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