Repository navigation
[WIP] Make Onyx.update and Onyx.multiSet apply atomically - #858
fabioh8010 wants to merge 4 commits into
Conversation
Onyx.update wrote its keys to the cache over three microtasks (sets, then single-key merges, then grouped collections), notifying subscribers after each wave, so subscribers and React renders could see a half-applied batch. Onyx.update now loads anything it needs from storage first (a pending clear, the persisted key index, merge targets evicted from the cache), then writes every key to the cache in one synchronous pass and notifies once. An Onyx.merge still queued for one of its keys is applied first, keeping call order. Onyx.multiSet notifies the same way. Notifications are held by onyxSubscriptionManager.batch(), which delivers each key once with the value the cache holds at delivery time. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
When the update applied in separate microtask waves, individual sets landed first, then snapshot merges, setCollection, partial collection sets and single-key merges, and grouped collection merges last. That order decides which members a collection gains first, so the key order code sees when reading a collection. Apply the writes in that same order and lock it with a test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Applying the update inside the call changed what code right after Onyx.update() reads. Actions that write in quick succession read module state that subscribers keep in sync, and started seeing each other's optimistic writes, which their rollback data was not written for. Some App tests also stopped finishing. The update now applies one microtask after the call, as before, still in one synchronous pass with one round of notifications. An Onyx.merge called earlier has its own microtask scheduled first, so it lands before the update without being folded in. An Onyx.merge still queued when the update applies was called after it, so it is left to apply on top. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An update that has to read from storage first (a key evicted from the cache, or a running Onyx.clear) applies later than one with nothing to load. A later update in the same tick could then apply first, and the earlier one would overwrite it. While an update is waiting, updates called after it now wait behind it. An update with nothing to wait for, and nothing waiting before it, still applies a microtask after the call. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
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. |
![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: 82bc5700e0
ℹ️ 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".
| const applied = Promise.all([previousDelayedUpdate, preparation]).then(() => { | ||
| storageWrites = applyUpdate(); |
There was a problem hiding this comment.
If the store has no cached keys or this update must prewarm an evicted merge target, preparation remains pending while an Onyx.merge() invoked immediately afterward can drain its queue before this continuation runs. Once preparation finishes, mergeInUpdate() applies the older update on top and overwrites overlapping fields (for example, update {value: 1} followed by merge {value: 2} can finish as 1), so Onyx.merge() also needs to respect this delayed-update barrier.
Useful? React with 👍 / 👎.
| }, | ||
| [OnyxUtils.METHOD.CLEAR]: () => { | ||
| clearPromise = clear(); | ||
| clear(); |
There was a problem hiding this comment.
When an update payload contains CLEAR alongside SET or MERGE entries, this starts the normal asynchronous clear, which emits reset notifications before pendingClear lets applyUpdate enter onyxSubscriptionManager.batch. Existing subscribers therefore observe the empty/default state and are notified again with the final update, allowing callbacks to act on a partial update despite the new atomicity guarantee; the clear notifications need to be held with the rest of this update.
Useful? React with 👍 / 👎.
|
Tag me when it's ready |
2 participants
Onyx.update wrote its keys to the cache over three microtasks (sets, then single-key merges, then grouped collections), notifying subscribers after each wave, so subscribers and React renders could see a half-applied batch.
Onyx.update now loads anything it needs from storage first (a pending clear, the persisted key index, merge targets evicted from the cache), then writes every key to the cache in one synchronous pass and notifies once. An Onyx.merge still queued for one of its keys is applied first, keeping call order. Onyx.multiSet notifies the same way.
Notifications are held by onyxSubscriptionManager.batch(), which delivers each key once with the value the cache holds at delivery time.
Details
Related Issues
Expensify/App#101801
Linked E/App PR
Expensify/App#104308
Automated Tests
Manual Tests
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