Remove waitForCollectionCallback from Onyx#799
Conversation
Collection-root subscriptions (via Onyx.connect or useOnyx) now always deliver the whole collection snapshot to the callback. The legacy per-member delivery mode for collection-root subscribers is removed; consumers needing per-member processing subscribe to a single member key. Collapses the DefaultConnectOptions/CollectionConnectOptions discriminated union into one ConnectOptions shape (callback typed on TKey), simplifies generateConnectionID and the keyChanged/keysChanged dispatch, and drops the option from useOnyx's internal connect. Per-member-mode tests were migrated to assert snapshot delivery rather than deleted. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 01704211bf
ℹ️ 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".
| if (OnyxKeys.isCollectionKey(subscriber.key)) { | ||
| // Collection-root subscribers always receive the whole collection snapshot. | ||
| // Skip individual key changes during collection updates to prevent duplicate | ||
| // callbacks - the collection update will handle this properly. | ||
| if (isProcessingCollectionUpdate) { |
There was a problem hiding this comment.
On web with multiple tabs, InstanceSync raises storage events per member key and Onyx.ts calls keyChanged(..., isKeyCollectionMember) with true for those collection-member updates. With this broadened collection-root branch, the receiving tab hits the isProcessingCollectionUpdate continue, and no keysChanged() runs afterward in that tab, so Onyx.connect({key: COLLECTION...}) subscribers never receive the required collection snapshot when another tab changes the collection. Please allow the storage-sync path to notify collection-root subscribers instead of treating it like a local batched collection update.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 9263ec9. Basically this flow was already flawed in main – the collection key callbacks with waitForCollectionCallback: true were not notified during tab syncs, only collection key callbacks without waitForCollectionCallback: true (per-member mode) or regular/collection member keys.
The solution here is to batch the writes across tab syncs instead of treating each synced member as an independent update. InstanceSync now raises a single SYNC_ONYX event carrying the JSON array of all keys changed in one write (instead of one event per key), and the receiving tab reads them with a single multiGet and hands Onyx the whole batch via a new batched onStorageKeysChanged(pairs) callback (with a single-key fallback for backwards compatibility).
Onyx then processes that batch synchronously — grouping members by their parent collection and firing exactly one keysChanged() per affected collection (plus keyChanged() for individual keys) — which mirrors how a local mergeCollection works. This makes a cross-tab collection update notify each collection-root subscriber (Onyx.connect/useOnyx on a collection key) once, in O(N), rather than re-delivering the whole collection once per member (O(N^2), which can crash the receiving tab depending on the payload) or never delivering it at all.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9263ec964d
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3da566e848
ℹ️ 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".
fabioh8010
changed the title
|
I caught one bug with the tab sync logic when clearing Onyx cache, investigating it. |
1 participant
Details
Removes
waitForCollectionCallback— the per-member collection subscription mode — so a collection-key subscription always receives the whole collection. Removing it exposed a latent multi-tab sync bug, which this PR also fixes.Cross-tab sync fix
Now that every collection subscriber is a whole-collection subscriber, multi-instance (web, multi-tab) sync needed fixing.
InstanceSyncraised onelocalStorageevent per changed key and the receiving tab read each with its own asyncgetItem, so amergeCollectionof N members arrived as N independent events across N tasks. That meant collection-root subscribers either received nothing cross-tab, or — if notified per member — had the whole collection re-delivered N times (O(N²)), crashing the receiving tab on large collections.The fix preserves the write's batch boundary across tabs:
InstanceSyncraises a singleSYNC_ONYXevent carrying the JSON array of all keys changed in a write (falls back to a single key for the previous format).multiGetand hands the whole batch to Onyx via a new batchedonStorageKeysChanged(pairs)callback.keysChanged()per collection (pluskeyChanged()for individual keys) — mirroring a localmergeCollection.Result: a cross-tab collection update notifies each collection-root subscriber once (O(N)) instead of N times or never.
Related Issues
Expensify/App#94331
Linked E/App PR
Expensify/App#93436
Automated Tests
Tests were changed to accomodate the changes.
Manual Tests
We are basically just removing
waitForCollectionCallbackusage in Expensify/App#93436, functionality should stay the same so we can test only the tab sync issues that are present onmainand are being fixed here. Tests are web-only.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 Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Screen.Recording.2026-06-26.at.14.09.06.mov
Screen.Recording.2026-06-26.at.14.12.04.mov