Fix useOnyx subscription to collection member key after Onyx.clear()#722
Conversation
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.mp4Android: mWeb Chromeandroid.mweb.mp4iOS: HybridAppios.mp4iOS: mWeb Safariios.mweb.mp4MacOS: Chrome / Safariweb.mp4 |
bernhardoj
left a comment
There was a problem hiding this comment.
Recording showing Expensify/App#79436 is fixed
web.mp4
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. ℹ️ 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". |
mountiny
left a comment
There was a problem hiding this comment.
Minor comment but I dont think we need to hold on that
|
|
||
| return resultRef.current; | ||
| }, [options?.initWithStoredValues, options?.allowStaleData, options?.canBeMissing, key, memoizedSelector, cacheKey]); | ||
| }, [options?.initWithStoredValues, options?.allowStaleData, options?.canBeMissing, key, memoizedSelector, cacheKey, previousKey]); |
There was a problem hiding this comment.
Is there measurable performance change now that we added another dependency that can change? @fabioh8010
There was a problem hiding this comment.
I don't think it will, but the change is kinda unavoidable as we are now using this property in this useCallback because of this PR.
|
|
||
| return resultRef.current; | ||
| }, [options?.initWithStoredValues, options?.allowStaleData, options?.canBeMissing, key, memoizedSelector, cacheKey]); | ||
| }, [options?.initWithStoredValues, options?.allowStaleData, options?.canBeMissing, key, memoizedSelector, cacheKey, previousKey]); |
There was a problem hiding this comment.
I don't think it will, but the change is kinda unavoidable as we are now using this property in this useCallback because of this PR.
Details
This PR fixes a bug that was discovered in Expensify/App#79436. Basically the
useOnyxworks correctly with regular key subscriptions afterOnyx.clear(), but not with collection member key subscriptions. The subscriber receives stale data instead of being properly notified of the change.The problem happens because, for collection subscribers, we need data in cache in order to notify them but
Onyx.clear()clears it before we start notifying and the subscribers end up not receiving updated data. The fix here will store the old values during clearing process before cache is cleared, and then send this data toscheduleNotifyCollectionSubscriberswhere it will be used to correctly notifying subscribers.Related Issues
Expensify/App#80105
Automated Tests
An unit test was added to cover this bug.
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
Screen.Recording.2026-01-22.at.12.10.02-compressed.mov
Android: mWeb Chrome
Unfortunately I was having constant fatal crashes when opening the website in Chrome Android and I wasn't able to record videos.
iOS: Native
Screen.Recording.2026-01-22.at.12.21.36-compressed.mov
iOS: mWeb Safari
Screen.Recording.2026-01-22.at.12.26.31-compressed.mov
MacOS: Chrome / Safari
Screen.Recording.2026-01-22.at.12.31.40-compressed.mov
Screen.Recording.2026-01-22.at.12.33.28-compressed.mov
MacOS: Desktop
N/A