Remove macro/micro tasks during subscriber update#724
Remove macro/micro tasks during subscriber update#724VickyStash wants to merge 22 commits intoExpensify:mainfrom
Conversation
# Conflicts: # lib/Onyx.ts # lib/OnyxMerge/index.native.ts
# Conflicts: # lib/Onyx.ts
|
LGTM |
VickyStash
changed the title
![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: 6318f5ef65
ℹ️ 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 (lastConnectionCallbackData.has(mapping.subscriptionID) && valueToPass === lastValue) { | ||
| // If the subscriber was already notified (e.g. by a synchronous keyChanged call), | ||
| // skip the initial data delivery to prevent duplicate callbacks. | ||
| if (lastConnectionCallbackData.has(mapping.subscriptionID)) { |
There was a problem hiding this comment.
Keep sendDataToConnection() from returning solely on lastConnectionCallbackData.has(...); this now drops the initial hydration callback whenever any synchronous keyChanged/keysChanged ran first. In the common race where Onyx.connect() is followed by an immediate Onyx.set() in the same tick, the subscription gets marked as "already notified" and the later storage-backed init payload is skipped even if it contains additional data (especially for collection subscribers), leaving subscribers with a partial state until a future update arrives.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
In this case, by the time the hydration promise resolves, it would only have equal or older data, making the skip correct.
There was a problem hiding this comment.
@VickyStash I asked Claude about this comment and according to it it's valid, here's a unit test it designed for me
diff --git a/tests/unit/onyxTest.ts b/tests/unit/onyxTest.ts
index 64caec5..94aff05 100644
--- a/tests/unit/onyxTest.ts
+++ b/tests/unit/onyxTest.ts
@@ -140,6 +140,51 @@ describe('Onyx', () => {
});
});
+ it('should deliver full collection data when connect() is followed by immediate set() of a single member in the same tick', () => {
+ const mockCallback = jest.fn();
+ const collectionKey = ONYX_KEYS.COLLECTION.TEST_CONNECT_COLLECTION;
+
+ // Write collection members directly to storage, bypassing Onyx cache values.
+ // This simulates data that exists in persistent storage but hasn't been loaded into cache yet
+ // (e.g. from a previous session).
+ return StorageMock.setItem(`${collectionKey}1`, {ID: 1, value: 'one'})
+ .then(() => StorageMock.setItem(`${collectionKey}2`, {ID: 2, value: 'two'}))
+ .then(() => StorageMock.setItem(`${collectionKey}3`, {ID: 3, value: 'three'}))
+ .then(() => {
+ // Register the keys in Onyx's key cache so getAllKeys() can discover them.
+ // We intentionally do NOT add values to the cache — only keys — to simulate
+ // data that is known to exist but whose values haven't been hydrated yet.
+ cache.addKey(`${collectionKey}1`);
+ cache.addKey(`${collectionKey}2`);
+ cache.addKey(`${collectionKey}3`);
+
+ // Connect to the collection — this starts an async storage read (microtask)
+ connection = Onyx.connect({
+ key: collectionKey,
+ waitForCollectionCallback: true,
+ callback: mockCallback,
+ });
+
+ // Immediately set a single member in the same synchronous tick.
+ // This triggers synchronous keyChanged() which calls the subscriber with a partial
+ // collection (just _1 from the cache). This sets lastConnectionCallbackData for this
+ // subscriber. The async hydration from subscribeToKey should still deliver the full
+ // collection afterwards, since the data is different.
+ Onyx.set(`${collectionKey}1`, {ID: 1, value: 'updated'});
+
+ // Wait for all async operations (storage reads from subscribeToKey) to complete
+ return waitForPromisesToResolve();
+ })
+ .then(() => {
+ // The subscriber's final state should contain ALL collection members,
+ // including _2 and _3 that were only in storage (not cache) at the time of the synchronous keyChanged call.
+ const lastCall = mockCallback.mock.calls[mockCallback.mock.calls.length - 1][0];
+ expect(lastCall).toHaveProperty(`${collectionKey}1`);
+ expect(lastCall).toHaveProperty(`${collectionKey}2`);
+ expect(lastCall).toHaveProperty(`${collectionKey}3`);
+ });
+ });
+
it('should merge an object with another object', () => {
let testKeyValue: unknown;
Could you validate if it makes sense?
|
Reviewing... |
|
|
|
@VickyStash I believe this might be related to the changes. Could you please take a look? Thanks! 
|
Yeah, it looks so. |
|
I've run checks with Claude and I haven't found other places like that, though it's not 100% guarantee Summary Problem
Why it's a problem • React may re-render components multiple times, discard renders, or pause them (Strict Mode, concurrent features). Side effects in the render body can execute unpredictably. Why it wasn't caught before The code was always technically wrong, but previously Scope of the issue After searching the entire codebase across HOCs, custom hooks, providers, and all components importing from Fix Wrap the call in useEffect: |
VickyStash
changed the title
|
I asked @VickyStash to hold this PR because of this plan to handle the canBeMissing situation -> Expensify/App#80095 (comment) |
VickyStash
changed the title
|
Not holded anymore! |
|
@Krishna2323 can you please test this pr with App? |
|
Will be on it in an hour or two. |
|
Reviewing... |
|
@Krishna2323 Please, take another look! |
|
@VickyStash could you please check this comment? Sorry about the wrong suggestion. 🙏🏻 |
|
I’ll try to finish the review by tomorrow — I had to step out for a doctor’s appointment. Thanks for the patience. |
| if (isSubscribedToCollectionKey) { | ||
| if (subscriber.waitForCollectionCallback) { | ||
| lastConnectionCallbackData.set(subscriber.subscriptionID, cachedCollection); | ||
| subscriber.callback(cachedCollection, subscriber.key, partialCollection); | ||
| continue; | ||
| } | ||
|
|
||
| // If they are not using waitForCollectionCallback then we notify the subscriber with | ||
| // the new merged data but only for any keys in the partial collection. | ||
| const dataKeys = Object.keys(partialCollection ?? {}); | ||
| for (const dataKey of dataKeys) { | ||
| if (deepEqual(cachedCollection[dataKey], previousCollection[dataKey])) { | ||
| continue; | ||
| } | ||
|
|
||
| subscriber.callback(cachedCollection[dataKey], dataKey); | ||
| } | ||
| lastConnectionCallbackData.set(subscriber.subscriptionID, cachedCollection); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
diff --git a/lib/OnyxUtils.ts b/lib/OnyxUtils.ts
index dd08e6b..06d17c1 100644
--- a/lib/OnyxUtils.ts
+++ b/lib/OnyxUtils.ts
@@ -686,8 +686,9 @@ function keysChanged<TKey extends CollectionKeyBase>(
// If they are subscribed to the collection key and using waitForCollectionCallback then we'll
// send the whole cached collection.
if (isSubscribedToCollectionKey) {
+ lastConnectionCallbackData.set(subscriber.subscriptionID, cachedCollection);
+
if (subscriber.waitForCollectionCallback) {
- lastConnectionCallbackData.set(subscriber.subscriptionID, cachedCollection);
subscriber.callback(cachedCollection, subscriber.key, partialCollection);
continue;
}
@@ -702,7 +703,6 @@ function keysChanged<TKey extends CollectionKeyBase>(
subscriber.callback(cachedCollection[dataKey], dataKey);
}
- lastConnectionCallbackData.set(subscriber.subscriptionID, cachedCollection);
continue;
}
Maybe we can just do lastConnectionCallbackData.set(subscriber.subscriptionID, cachedCollection); right inside if (isSubscribedToCollectionKey) { since it will be executed in all situations inside this condition?
| if (lastConnectionCallbackData.has(mapping.subscriptionID) && valueToPass === lastValue) { | ||
| // If the subscriber was already notified (e.g. by a synchronous keyChanged call), | ||
| // skip the initial data delivery to prevent duplicate callbacks. | ||
| if (lastConnectionCallbackData.has(mapping.subscriptionID)) { |
There was a problem hiding this comment.
@VickyStash I asked Claude about this comment and according to it it's valid, here's a unit test it designed for me
diff --git a/tests/unit/onyxTest.ts b/tests/unit/onyxTest.ts
index 64caec5..94aff05 100644
--- a/tests/unit/onyxTest.ts
+++ b/tests/unit/onyxTest.ts
@@ -140,6 +140,51 @@ describe('Onyx', () => {
});
});
+ it('should deliver full collection data when connect() is followed by immediate set() of a single member in the same tick', () => {
+ const mockCallback = jest.fn();
+ const collectionKey = ONYX_KEYS.COLLECTION.TEST_CONNECT_COLLECTION;
+
+ // Write collection members directly to storage, bypassing Onyx cache values.
+ // This simulates data that exists in persistent storage but hasn't been loaded into cache yet
+ // (e.g. from a previous session).
+ return StorageMock.setItem(`${collectionKey}1`, {ID: 1, value: 'one'})
+ .then(() => StorageMock.setItem(`${collectionKey}2`, {ID: 2, value: 'two'}))
+ .then(() => StorageMock.setItem(`${collectionKey}3`, {ID: 3, value: 'three'}))
+ .then(() => {
+ // Register the keys in Onyx's key cache so getAllKeys() can discover them.
+ // We intentionally do NOT add values to the cache — only keys — to simulate
+ // data that is known to exist but whose values haven't been hydrated yet.
+ cache.addKey(`${collectionKey}1`);
+ cache.addKey(`${collectionKey}2`);
+ cache.addKey(`${collectionKey}3`);
+
+ // Connect to the collection — this starts an async storage read (microtask)
+ connection = Onyx.connect({
+ key: collectionKey,
+ waitForCollectionCallback: true,
+ callback: mockCallback,
+ });
+
+ // Immediately set a single member in the same synchronous tick.
+ // This triggers synchronous keyChanged() which calls the subscriber with a partial
+ // collection (just _1 from the cache). This sets lastConnectionCallbackData for this
+ // subscriber. The async hydration from subscribeToKey should still deliver the full
+ // collection afterwards, since the data is different.
+ Onyx.set(`${collectionKey}1`, {ID: 1, value: 'updated'});
+
+ // Wait for all async operations (storage reads from subscribeToKey) to complete
+ return waitForPromisesToResolve();
+ })
+ .then(() => {
+ // The subscriber's final state should contain ALL collection members,
+ // including _2 and _3 that were only in storage (not cache) at the time of the synchronous keyChanged call.
+ const lastCall = mockCallback.mock.calls[mockCallback.mock.calls.length - 1][0];
+ expect(lastCall).toHaveProperty(`${collectionKey}1`);
+ expect(lastCall).toHaveProperty(`${collectionKey}2`);
+ expect(lastCall).toHaveProperty(`${collectionKey}3`);
+ });
+ });
+
it('should merge an object with another object', () => {
let testKeyValue: unknown;
Could you validate if it makes sense?
|
Thanks @Krishna2323 feel free to take time off too |
6 participants
Details
Check the issue description for details.
Related Issues
$ Expensify/App#82871
Automated Tests
Should be covered by existing tests
Manual Tests
The E/App should work the same way as before. Let's verify basic test steps:
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
android.mp4
Android: mWeb Chrome
android_web.mp4
iOS: Native
ios.mov
iOS: mWeb Safari
ios_web.mov
MacOS: Chrome / Safari
web.mp4