From a1638ea1800a795057abd395938a42df81f7ab5f Mon Sep 17 00:00:00 2001 From: VickyStash Date: Mon, 23 Mar 2026 15:56:01 +0100 Subject: [PATCH 1/3] Revert "Merge pull request #758 from Expensify/revert-724-VickyStash/poc/get-rid-of-macro-micro-tasks" This reverts commit 381c36a51ef7f3644362b417e375729f041f2b38, reversing changes made to 1ab73fed35611b97a8bc110f5abcfd22ef87e9c6. --- lib/Onyx.ts | 22 ++--- lib/OnyxMerge/index.native.ts | 5 +- lib/OnyxMerge/index.ts | 5 +- lib/OnyxMerge/types.ts | 1 - lib/OnyxUtils.ts | 131 ++++++++----------------- tests/perf-test/OnyxUtils.perf-test.ts | 63 +----------- tests/unit/collectionHydrationTest.ts | 85 ++++++++++++++++ tests/unit/onyxTest.ts | 5 +- 8 files changed, 143 insertions(+), 174 deletions(-) create mode 100644 tests/unit/collectionHydrationTest.ts diff --git a/lib/Onyx.ts b/lib/Onyx.ts index 42a3cd57c..3da6a4dee 100644 --- a/lib/Onyx.ts +++ b/lib/Onyx.ts @@ -266,9 +266,8 @@ function merge(key: TKey, changes: OnyxMergeInput): return Promise.resolve(); } - return OnyxMerge.applyMerge(key, existingValue, validChanges).then(({mergedValue, updatePromise}) => { + return OnyxMerge.applyMerge(key, existingValue, validChanges).then(({mergedValue}) => { OnyxUtils.sendActionToDevTools(OnyxUtils.METHOD.MERGE, key, changes, mergedValue); - return updatePromise; }); } catch (error) { Logger.logAlert(`An error occurred while applying merge for key: ${key}, Error: ${error}`); @@ -380,16 +379,6 @@ function clear(keysToPreserve: OnyxKey[] = []): Promise { keysToBeClearedFromStorage.push(key); } - const updatePromises: Array> = []; - - // Notify the subscribers for each key/value group so they can receive the new values - for (const [key, value] of Object.entries(keyValuesToResetIndividually)) { - updatePromises.push(OnyxUtils.scheduleSubscriberUpdate(key, value)); - } - for (const [key, value] of Object.entries(keyValuesToResetAsCollection)) { - updatePromises.push(OnyxUtils.scheduleNotifyCollectionSubscribers(key, value.newValues, value.oldValues)); - } - // Exclude RAM-only keys to prevent them from being saved to storage const defaultKeyValuePairs = Object.entries( Object.keys(defaultKeyStates) @@ -408,7 +397,14 @@ function clear(keysToPreserve: OnyxKey[] = []): Promise { .then(() => Storage.multiSet(defaultKeyValuePairs)) .then(() => { DevTools.clearState(keysToPreserve); - return Promise.all(updatePromises); + + // Notify the subscribers for each key/value group so they can receive the new values + for (const [key, value] of Object.entries(keyValuesToResetIndividually)) { + OnyxUtils.keyChanged(key, value); + } + for (const [key, value] of Object.entries(keyValuesToResetAsCollection)) { + OnyxUtils.keysChanged(key, value.newValues, value.oldValues); + } }); }) .then(() => undefined); diff --git a/lib/OnyxMerge/index.native.ts b/lib/OnyxMerge/index.native.ts index ec8c242e3..5e56bf49a 100644 --- a/lib/OnyxMerge/index.native.ts +++ b/lib/OnyxMerge/index.native.ts @@ -26,21 +26,20 @@ const applyMerge: ApplyMerge = , hasChanged); + OnyxUtils.broadcastUpdate(key, mergedValue as OnyxValue, hasChanged); const shouldSkipStorageOperations = !hasChanged || OnyxUtils.isRamOnlyKey(key); // If the value has not changed, calling Storage.setItem() would be redundant and a waste of performance, so return early instead. // If the key is marked as RAM-only, it should not be saved nor updated in the storage. if (shouldSkipStorageOperations) { - return Promise.resolve({mergedValue, updatePromise}); + return Promise.resolve({mergedValue}); } // For native platforms we use `mergeItem` that will take advantage of JSON_PATCH and JSON_REPLACE SQL operations to // merge the object in a performant way. return Storage.mergeItem(key, batchedChanges as OnyxValue, replaceNullPatches).then(() => ({ mergedValue, - updatePromise, })); }; diff --git a/lib/OnyxMerge/index.ts b/lib/OnyxMerge/index.ts index 7eac789cb..ef92293d3 100644 --- a/lib/OnyxMerge/index.ts +++ b/lib/OnyxMerge/index.ts @@ -18,20 +18,19 @@ const applyMerge: ApplyMerge = , hasChanged); + OnyxUtils.broadcastUpdate(key, mergedValue as OnyxValue, hasChanged); const shouldSkipStorageOperations = !hasChanged || OnyxUtils.isRamOnlyKey(key); // If the value has not changed, calling Storage.setItem() would be redundant and a waste of performance, so return early instead. // If the key is marked as RAM-only, it should not be saved nor updated in the storage. if (shouldSkipStorageOperations) { - return Promise.resolve({mergedValue, updatePromise}); + return Promise.resolve({mergedValue}); } // For web platforms we use `setItem` since the object was already merged with its changes before. return Storage.setItem(key, mergedValue as OnyxValue).then(() => ({ mergedValue, - updatePromise, })); }; diff --git a/lib/OnyxMerge/types.ts b/lib/OnyxMerge/types.ts index c59b7892a..e53d8ff32 100644 --- a/lib/OnyxMerge/types.ts +++ b/lib/OnyxMerge/types.ts @@ -2,7 +2,6 @@ import type {OnyxInput, OnyxKey} from '../types'; type ApplyMergeResult = { mergedValue: TValue; - updatePromise: Promise; }; type ApplyMerge = | undefined, TChange extends OnyxInput | null>( diff --git a/lib/OnyxUtils.ts b/lib/OnyxUtils.ts index f203870da..1a9b11bb8 100644 --- a/lib/OnyxUtils.ts +++ b/lib/OnyxUtils.ts @@ -1,4 +1,4 @@ -import {deepEqual} from 'fast-equals'; +import {deepEqual, shallowEqual} from 'fast-equals'; import type {ValueOf} from 'type-fest'; import _ from 'underscore'; import DevTools from './DevTools'; @@ -74,9 +74,6 @@ type OnyxMethod = ValueOf; let mergeQueue: Record>> = {}; let mergeQueuePromise: Record> = {}; -// Used to schedule subscriber update to the macro task queue -let nextMacrotaskPromise: Promise | null = null; - // Holds a mapping of all the React components that want their state subscribed to a store key let callbackToStateMapping: Record> = {}; @@ -709,6 +706,8 @@ function keysChanged( // 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) { subscriber.callback(cachedCollection, subscriber.key, partialCollection); continue; @@ -808,6 +807,7 @@ function keyChanged( } cachedCollection[key] = value; + lastConnectionCallbackData.set(subscriber.subscriptionID, cachedCollection); subscriber.callback(cachedCollection, subscriber.key, {[key]: value}); continue; } @@ -826,24 +826,34 @@ function keyChanged( /** * Sends the data obtained from the keys to the connection. */ -function sendDataToConnection(mapping: CallbackToStateMapping, value: OnyxValue | null, matchedKey: TKey | undefined): void { +function sendDataToConnection(mapping: CallbackToStateMapping, matchedKey: TKey | undefined): void { // If the mapping no longer exists then we should not send any data. // This means our subscriber was disconnected. if (!callbackToStateMapping[mapping.subscriptionID]) { return; } + // Always read the latest value from cache to avoid stale or duplicate data. + // For collection subscribers with waitForCollectionCallback, read the full collection. + // For individual key subscribers, read just that key's value. + let value: OnyxValue | undefined; + if (isCollectionKey(mapping.key) && mapping.waitForCollectionCallback) { + const collection = getCachedCollection(mapping.key); + value = Object.keys(collection).length > 0 ? (collection as OnyxValue) : undefined; + } else { + value = cache.get(matchedKey ?? mapping.key) as OnyxValue; + } + // For regular callbacks, we never want to pass null values, but always just undefined if a value is not set in cache or storage. - const valueToPass = value === null ? undefined : value; + value = value === null ? undefined : value; const lastValue = lastConnectionCallbackData.get(mapping.subscriptionID); - lastConnectionCallbackData.get(mapping.subscriptionID); // If the value has not changed we do not need to trigger the callback - if (lastConnectionCallbackData.has(mapping.subscriptionID) && valueToPass === lastValue) { + if (lastConnectionCallbackData.has(mapping.subscriptionID) && shallowEqual(lastValue, value)) { return; } - (mapping as DefaultConnectOptions).callback?.(valueToPass, matchedKey as TKey); + (mapping as DefaultConnectOptions).callback?.(value, matchedKey as TKey); } /** @@ -866,63 +876,17 @@ function addKeyToRecentlyAccessedIfNeeded(key: TKey): void * Gets the data for a given an array of matching keys, combines them into an object, and sends the result back to the subscriber. */ function getCollectionDataAndSendAsObject(matchingKeys: CollectionKeyBase[], mapping: CallbackToStateMapping): void { - multiGet(matchingKeys).then((dataMap) => { - const data = Object.fromEntries(dataMap.entries()) as OnyxValue; - sendDataToConnection(mapping, data, mapping.key); + multiGet(matchingKeys).then(() => { + sendDataToConnection(mapping, mapping.key); }); } -/** - * Delays promise resolution until the next macrotask to prevent race condition if the key subscription is in progress. - * - * @param callback The keyChanged/keysChanged callback - * */ -function prepareSubscriberUpdate(callback: () => void): Promise { - if (!nextMacrotaskPromise) { - nextMacrotaskPromise = new Promise((resolve) => { - setTimeout(() => { - nextMacrotaskPromise = null; - resolve(); - }, 0); - }); - } - return Promise.all([nextMacrotaskPromise, Promise.resolve().then(callback)]).then(); -} - -/** - * Schedules an update that will be appended to the macro task queue (so it doesn't update the subscribers immediately). - * - * @example - * scheduleSubscriberUpdate(key, value, subscriber => subscriber.initWithStoredValues === false) - */ -function scheduleSubscriberUpdate( - key: TKey, - value: OnyxValue, - canUpdateSubscriber: (subscriber?: CallbackToStateMapping) => boolean = () => true, - isProcessingCollectionUpdate = false, -): Promise { - return prepareSubscriberUpdate(() => keyChanged(key, value, canUpdateSubscriber, isProcessingCollectionUpdate)); -} - -/** - * This method is similar to scheduleSubscriberUpdate but it is built for working specifically with collections - * so that keysChanged() is triggered for the collection and not keyChanged(). If this was not done, then the - * subscriber callbacks receive the data in a different format than they normally expect and it breaks code. - */ -function scheduleNotifyCollectionSubscribers( - key: TKey, - value: OnyxCollection, - previousValue?: OnyxCollection, -): Promise { - return prepareSubscriberUpdate(() => keysChanged(key, value, previousValue)); -} - /** * Remove a key from Onyx and update the subscribers */ function remove(key: TKey, isProcessingCollectionUpdate?: boolean): Promise { cache.drop(key); - scheduleSubscriberUpdate(key, undefined as OnyxValue, undefined, isProcessingCollectionUpdate); + keyChanged(key, undefined as OnyxValue, undefined, isProcessingCollectionUpdate); if (isRamOnlyKey(key)) { return Promise.resolve(); @@ -993,7 +957,7 @@ function retryOperation(error: Error, on /** * Notifies subscribers and writes current value to cache */ -function broadcastUpdate(key: TKey, value: OnyxValue, hasChanged?: boolean): Promise { +function broadcastUpdate(key: TKey, value: OnyxValue, hasChanged?: boolean): void { // Update subscribers if the cached value has changed, or when the subscriber specifically requires // all updates regardless of value changes (indicated by initWithStoredValues set to false). if (hasChanged) { @@ -1002,7 +966,7 @@ function broadcastUpdate(key: TKey, value: OnyxValue cache.addToAccessedKeys(key); } - return scheduleSubscriberUpdate(key, value, (subscriber) => hasChanged || subscriber?.initWithStoredValues === false).then(() => undefined); + keyChanged(key, value, (subscriber) => hasChanged || subscriber?.initWithStoredValues === false); } function hasPendingMergeForKey(key: OnyxKey): boolean { @@ -1219,7 +1183,7 @@ function subscribeToKey(connectOptions: ConnectOptions(connectOptions: ConnectOptions { - for (const [key, val] of values.entries()) { - sendDataToConnection(mapping, val as OnyxValue, key as TKey); + multiGet(matchingKeys).then(() => { + for (const key of matchingKeys) { + sendDataToConnection(mapping, key as TKey); } }); return; } // If we are not subscribed to a collection key then there's only a single key to send an update for. - get(mapping.key).then((val) => sendDataToConnection(mapping, val as OnyxValue, mapping.key)); + get(mapping.key).then(() => sendDataToConnection(mapping, mapping.key)); return; } @@ -1405,24 +1369,23 @@ function setWithRetry({key, value, options}: SetParams OnyxUtils.retryOperation(error, setWithRetry, {key, value: valueWithoutNestedNullValues, options}, retryAttempt)) .then(() => { OnyxUtils.sendActionToDevTools(OnyxUtils.METHOD.SET, key, valueWithoutNestedNullValues); - return updatePromise; }); } @@ -1456,17 +1419,17 @@ function multiSetWithRetry(data: OnyxMultiSetInput, retryAttempt?: number): Prom const keyValuePairsToSet = OnyxUtils.prepareKeyValuePairsForStorage(newData, true); - const updatePromises = keyValuePairsToSet.map(([key, value]) => { + for (const [key, value] of keyValuePairsToSet) { // When we use multiSet to set a key we want to clear the current delta changes from Onyx.merge that were queued // before the value was set. If Onyx.merge is currently reading the old value from storage, it will then not apply the changes. if (OnyxUtils.hasPendingMergeForKey(key)) { delete OnyxUtils.getMergeQueue()[key]; } - // Update cache and optimistically inform subscribers on the next tick + // Update cache and optimistically inform subscribers cache.set(key, value); - return OnyxUtils.scheduleSubscriberUpdate(key, value); - }); + keyChanged(key, value); + } const keyValuePairsToStore = keyValuePairsToSet.filter((keyValuePair) => { const [key] = keyValuePair; @@ -1478,9 +1441,7 @@ function multiSetWithRetry(data: OnyxMultiSetInput, retryAttempt?: number): Prom .catch((error) => OnyxUtils.retryOperation(error, multiSetWithRetry, newData, retryAttempt)) .then(() => { OnyxUtils.sendActionToDevTools(OnyxUtils.METHOD.MULTI_SET, undefined, newData); - return Promise.all(updatePromises); - }) - .then(() => undefined); + }); } /** @@ -1541,19 +1502,18 @@ function setCollectionWithRetry({collectionKey, for (const [key, value] of keyValuePairs) cache.set(key, value); - const updatePromise = OnyxUtils.scheduleNotifyCollectionSubscribers(collectionKey, mutableCollection, previousCollection); + keysChanged(collectionKey, mutableCollection, previousCollection); // RAM-only keys are not supposed to be saved to storage if (isRamOnlyKey(collectionKey)) { OnyxUtils.sendActionToDevTools(OnyxUtils.METHOD.SET_COLLECTION, undefined, mutableCollection); - return updatePromise; + return; } return Storage.multiSet(keyValuePairs) .catch((error) => OnyxUtils.retryOperation(error, setCollectionWithRetry, {collectionKey, collection}, retryAttempt)) .then(() => { OnyxUtils.sendActionToDevTools(OnyxUtils.METHOD.SET_COLLECTION, undefined, mutableCollection); - return updatePromise; }); }); } @@ -1685,7 +1645,7 @@ function mergeCollectionWithPatches( // and update all subscribers const promiseUpdate = previousCollectionPromise.then((previousCollection) => { cache.merge(finalMergedCollection); - return scheduleNotifyCollectionSubscribers(collectionKey, finalMergedCollection, previousCollection); + keysChanged(collectionKey, finalMergedCollection, previousCollection); }); return Promise.all(promises) @@ -1751,18 +1711,17 @@ function partialSetCollection({collectionKey, co for (const [key, value] of keyValuePairs) cache.set(key, value); - const updatePromise = scheduleNotifyCollectionSubscribers(collectionKey, mutableCollection, previousCollection); + keysChanged(collectionKey, mutableCollection, previousCollection); if (isRamOnlyKey(collectionKey)) { sendActionToDevTools(METHOD.SET_COLLECTION, undefined, mutableCollection); - return updatePromise; + return; } return Storage.multiSet(keyValuePairs) .catch((error) => retryOperation(error, partialSetCollection, {collectionKey, collection}, retryAttempt)) .then(() => { sendActionToDevTools(METHOD.SET_COLLECTION, undefined, mutableCollection); - return updatePromise; }); }); } @@ -1810,8 +1769,6 @@ const OnyxUtils = { sendDataToConnection, getCollectionKey, getCollectionDataAndSendAsObject, - scheduleSubscriberUpdate, - scheduleNotifyCollectionSubscribers, remove, reportStorageQuota, retryOperation, @@ -1868,10 +1825,6 @@ GlobalSettings.addGlobalSettingsChangeListener(({enablePerformanceMetrics}) => { // @ts-expect-error Reassign sendDataToConnection = decorateWithMetrics(sendDataToConnection, 'OnyxUtils.sendDataToConnection'); // @ts-expect-error Reassign - scheduleSubscriberUpdate = decorateWithMetrics(scheduleSubscriberUpdate, 'OnyxUtils.scheduleSubscriberUpdate'); - // @ts-expect-error Reassign - scheduleNotifyCollectionSubscribers = decorateWithMetrics(scheduleNotifyCollectionSubscribers, 'OnyxUtils.scheduleNotifyCollectionSubscribers'); - // @ts-expect-error Reassign remove = decorateWithMetrics(remove, 'OnyxUtils.remove'); // @ts-expect-error Reassign reportStorageQuota = decorateWithMetrics(reportStorageQuota, 'OnyxUtils.reportStorageQuota'); diff --git a/tests/perf-test/OnyxUtils.perf-test.ts b/tests/perf-test/OnyxUtils.perf-test.ts index 5a00d910e..8677fc37d 100644 --- a/tests/perf-test/OnyxUtils.perf-test.ts +++ b/tests/perf-test/OnyxUtils.perf-test.ts @@ -367,7 +367,6 @@ describe('OnyxUtils', () => { subscriptionID, callback: jest.fn(), }, - mockedReportActionsMap, undefined, ), { @@ -431,66 +430,6 @@ describe('OnyxUtils', () => { }); }); - describe('scheduleSubscriberUpdate', () => { - test('10k calls scheduling updates', async () => { - const subscriptionMap = new Map(); - - const changedReportActions = Object.fromEntries( - Object.entries(mockedReportActionsMap).map(([k, v]) => [k, createRandomReportAction(Number(v.reportActionID))] as const), - ) as GenericCollection; - - await measureAsyncFunction(() => Promise.all(Object.entries(changedReportActions).map(([key, value]) => OnyxUtils.scheduleSubscriberUpdate(key, value))), { - beforeEach: async () => { - await Onyx.multiSet(mockedReportActionsMap); - for (const key of mockedReportActionsKeys) { - const id = OnyxUtils.subscribeToKey({key, callback: jest.fn(), initWithStoredValues: false}); - subscriptionMap.set(key, id); - } - }, - afterEach: async () => { - for (const key of mockedReportActionsKeys) { - const id = subscriptionMap.get(key); - if (id) { - OnyxUtils.unsubscribeFromKey(id); - } - } - subscriptionMap.clear(); - await clearOnyxAfterEachMeasure(); - }, - }); - }); - }); - - describe('scheduleNotifyCollectionSubscribers', () => { - test('one call with 10k heavy objects to update 10k subscribers', async () => { - const subscriptionMap = new Map(); - - const changedReportActions = Object.fromEntries( - Object.entries(mockedReportActionsMap).map(([k, v]) => [k, createRandomReportAction(Number(v.reportActionID))] as const), - ) as GenericCollection; - - await measureAsyncFunction(() => OnyxUtils.scheduleNotifyCollectionSubscribers(collectionKey, changedReportActions, mockedReportActionsMap), { - beforeEach: async () => { - await Onyx.multiSet(mockedReportActionsMap); - for (const key of mockedReportActionsKeys) { - const id = OnyxUtils.subscribeToKey({key, callback: jest.fn(), initWithStoredValues: false}); - subscriptionMap.set(key, id); - } - }, - afterEach: async () => { - for (const key of mockedReportActionsKeys) { - const id = subscriptionMap.get(key); - if (id) { - OnyxUtils.unsubscribeFromKey(id); - } - } - subscriptionMap.clear(); - await clearOnyxAfterEachMeasure(); - }, - }); - }); - }); - describe('remove', () => { test('10k calls', async () => { await measureAsyncFunction(() => Promise.all(mockedReportActionsKeys.map((key) => OnyxUtils.remove(key))), { @@ -534,7 +473,7 @@ describe('OnyxUtils', () => { const reportAction = mockedReportActionsMap[`${collectionKey}0`]; const changedReportAction = createRandomReportAction(Number(reportAction.reportActionID)); - await measureAsyncFunction(() => OnyxUtils.broadcastUpdate(key, changedReportAction, true), { + await measureFunction(() => OnyxUtils.broadcastUpdate(key, changedReportAction, true), { beforeEach: async () => { await Onyx.set(key, reportAction); }, diff --git a/tests/unit/collectionHydrationTest.ts b/tests/unit/collectionHydrationTest.ts new file mode 100644 index 000000000..a36f53db1 --- /dev/null +++ b/tests/unit/collectionHydrationTest.ts @@ -0,0 +1,85 @@ +import StorageMock from '../../lib/storage'; +import Onyx from '../../lib'; +import waitForPromisesToResolve from '../utils/waitForPromisesToResolve'; + +const ONYX_KEYS = { + COLLECTION: { + TEST_KEY: 'test_', + }, +}; + +describe('Collection hydration with connect() followed by immediate set()', () => { + beforeEach(async () => { + // ===== Session 1 ===== + // Data is written to persistent storage (simulates a previous app session). + await StorageMock.setItem(`${ONYX_KEYS.COLLECTION.TEST_KEY}1`, {id: 1, title: 'Test One'}); + await StorageMock.setItem(`${ONYX_KEYS.COLLECTION.TEST_KEY}2`, {id: 2, title: 'Test Two'}); + await StorageMock.setItem(`${ONYX_KEYS.COLLECTION.TEST_KEY}3`, {id: 3, title: 'Test Three'}); + + // ===== Session 2 ===== + // App restarts. Onyx.init() calls getAllKeys() which populates storageKeys + // with all 3 keys, but their values are NOT read into cache yet. + Onyx.init({keys: ONYX_KEYS}); + }); + + afterEach(() => Onyx.clear()); + + test('waitForCollectionCallback=true should deliver full collection from storage', async () => { + const mockCallback = jest.fn(); + + // A component connects to the collection (starts async hydration via multiGet). + Onyx.connect({ + key: ONYX_KEYS.COLLECTION.TEST_KEY, + waitForCollectionCallback: true, + callback: mockCallback, + }); + + Onyx.set(`${ONYX_KEYS.COLLECTION.TEST_KEY}1`, {id: 1, title: 'Updated Test One'}); + + await waitForPromisesToResolve(); + + // The subscriber should eventually receive ALL collection members. + // The async hydration reads test_2 and test_3 from storage. + const lastCall = mockCallback.mock.calls[mockCallback.mock.calls.length - 1][0]; + expect(lastCall).toHaveProperty(`${ONYX_KEYS.COLLECTION.TEST_KEY}1`); + expect(lastCall).toHaveProperty(`${ONYX_KEYS.COLLECTION.TEST_KEY}2`); + expect(lastCall).toHaveProperty(`${ONYX_KEYS.COLLECTION.TEST_KEY}3`); + + // Verify the updated value is present (not stale) + expect(lastCall[`${ONYX_KEYS.COLLECTION.TEST_KEY}1`]).toEqual({id: 1, title: 'Updated Test One'}); + }); + + test('waitForCollectionCallback=false should deliver all collection members from storage', async () => { + const mockCallback = jest.fn(); + + // A component connects to the collection (callback fires per key, not batched). + Onyx.connect({ + key: ONYX_KEYS.COLLECTION.TEST_KEY, + waitForCollectionCallback: false, + callback: mockCallback, + }); + + Onyx.set(`${ONYX_KEYS.COLLECTION.TEST_KEY}1`, {id: 1, title: 'Updated Test One'}); + + await waitForPromisesToResolve(); + + // With waitForCollectionCallback=false, the callback fires per key individually. + // Collect all keys that were delivered across all calls. + const deliveredKeys = new Set(); + for (const call of mockCallback.mock.calls) { + const [, key] = call; + if (key) { + deliveredKeys.add(key); + } + } + + expect(deliveredKeys).toContain(`${ONYX_KEYS.COLLECTION.TEST_KEY}1`); + expect(deliveredKeys).toContain(`${ONYX_KEYS.COLLECTION.TEST_KEY}2`); + expect(deliveredKeys).toContain(`${ONYX_KEYS.COLLECTION.TEST_KEY}3`); + + // Verify the updated value is present (not stale) by finding the last call for key 1 + const key1Calls = mockCallback.mock.calls.filter((call) => call[1] === `${ONYX_KEYS.COLLECTION.TEST_KEY}1`); + const lastKey1Value = key1Calls[key1Calls.length - 1][0]; + expect(lastKey1Value).toEqual({id: 1, title: 'Updated Test One'}); + }); +}); diff --git a/tests/unit/onyxTest.ts b/tests/unit/onyxTest.ts index 390f1a165..bfafd0768 100644 --- a/tests/unit/onyxTest.ts +++ b/tests/unit/onyxTest.ts @@ -1552,10 +1552,9 @@ describe('Onyx', () => { return waitForPromisesToResolve(); }) .then(() => { - expect(collectionCallback).toHaveBeenCalledTimes(3); + expect(collectionCallback).toHaveBeenCalledTimes(2); expect(collectionCallback).toHaveBeenNthCalledWith(1, {[cat]: initialValue}, ONYX_KEYS.COLLECTION.ANIMALS, {[cat]: initialValue}); - expect(collectionCallback).toHaveBeenNthCalledWith(2, {[cat]: initialValue}, ONYX_KEYS.COLLECTION.ANIMALS, undefined); - expect(collectionCallback).toHaveBeenNthCalledWith(3, collectionDiff, ONYX_KEYS.COLLECTION.ANIMALS, {[cat]: initialValue, [dog]: {name: 'Rex'}}); + expect(collectionCallback).toHaveBeenNthCalledWith(2, collectionDiff, ONYX_KEYS.COLLECTION.ANIMALS, {[cat]: initialValue, [dog]: {name: 'Rex'}}); // Cat hasn't changed from its original value, expect only the initial connect callback expect(catCallback).toHaveBeenCalledTimes(1); From a19cc41cd08ae19e6785b48199c4bfa11f32b8f6 Mon Sep 17 00:00:00 2001 From: VickyStash Date: Tue, 24 Mar 2026 08:04:16 +0100 Subject: [PATCH 2/3] Track matchedKey in lastConnectionCallbackData to prevent false dedup of collection member callbacks --- lib/OnyxUtils.ts | 21 ++++++++------- tests/unit/collectionHydrationTest.ts | 37 +++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 9 deletions(-) diff --git a/lib/OnyxUtils.ts b/lib/OnyxUtils.ts index 1a9b11bb8..fff17b8ad 100644 --- a/lib/OnyxUtils.ts +++ b/lib/OnyxUtils.ts @@ -87,7 +87,7 @@ let onyxKeyToSubscriptionIDs = new Map(); let defaultKeyStates: Record> = {}; // Used for comparison with a new update to avoid invoking the Onyx.connect callback with the same data. -let lastConnectionCallbackData = new Map>(); +let lastConnectionCallbackData = new Map; matchedKey: OnyxKey | undefined}>(); let snapshotKey: OnyxKey | null = null; @@ -706,7 +706,7 @@ function keysChanged( // 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); + lastConnectionCallbackData.set(subscriber.subscriptionID, {value: cachedCollection, matchedKey: subscriber.key}); if (subscriber.waitForCollectionCallback) { subscriber.callback(cachedCollection, subscriber.key, partialCollection); @@ -735,7 +735,7 @@ function keysChanged( const subscriberCallback = subscriber.callback as DefaultConnectCallback; subscriberCallback(cachedCollection[subscriber.key], subscriber.key as TKey); - lastConnectionCallbackData.set(subscriber.subscriptionID, cachedCollection[subscriber.key]); + lastConnectionCallbackData.set(subscriber.subscriptionID, {value: cachedCollection[subscriber.key], matchedKey: subscriber.key}); continue; } @@ -789,7 +789,8 @@ function keyChanged( // Subscriber is a regular call to connect() and provided a callback if (typeof subscriber.callback === 'function') { - if (lastConnectionCallbackData.has(subscriber.subscriptionID) && lastConnectionCallbackData.get(subscriber.subscriptionID) === value) { + const lastData = lastConnectionCallbackData.get(subscriber.subscriptionID); + if (lastData && lastData.matchedKey === key && lastData.value === value) { continue; } @@ -807,7 +808,7 @@ function keyChanged( } cachedCollection[key] = value; - lastConnectionCallbackData.set(subscriber.subscriptionID, cachedCollection); + lastConnectionCallbackData.set(subscriber.subscriptionID, {value: cachedCollection, matchedKey: subscriber.key}); subscriber.callback(cachedCollection, subscriber.key, {[key]: value}); continue; } @@ -815,7 +816,7 @@ function keyChanged( const subscriberCallback = subscriber.callback as DefaultConnectCallback; subscriberCallback(value, key); - lastConnectionCallbackData.set(subscriber.subscriptionID, value); + lastConnectionCallbackData.set(subscriber.subscriptionID, {value, matchedKey: key}); continue; } @@ -846,10 +847,12 @@ function sendDataToConnection(mapping: CallbackToStateMapp // For regular callbacks, we never want to pass null values, but always just undefined if a value is not set in cache or storage. value = value === null ? undefined : value; - const lastValue = lastConnectionCallbackData.get(mapping.subscriptionID); + const lastData = lastConnectionCallbackData.get(mapping.subscriptionID); - // If the value has not changed we do not need to trigger the callback - if (lastConnectionCallbackData.has(mapping.subscriptionID) && shallowEqual(lastValue, value)) { + // If the value has not changed for the same key we do not need to trigger the callback. + // We compare matchedKey to avoid suppressing callbacks for different collection members + // that happen to have shallow-equal values (e.g. during hydration racing with set()). + if (lastData && lastData.matchedKey === matchedKey && shallowEqual(lastData.value, value)) { return; } diff --git a/tests/unit/collectionHydrationTest.ts b/tests/unit/collectionHydrationTest.ts index a36f53db1..50e9a81a8 100644 --- a/tests/unit/collectionHydrationTest.ts +++ b/tests/unit/collectionHydrationTest.ts @@ -49,6 +49,43 @@ describe('Collection hydration with connect() followed by immediate set()', () = expect(lastCall[`${ONYX_KEYS.COLLECTION.TEST_KEY}1`]).toEqual({id: 1, title: 'Updated Test One'}); }); + test('waitForCollectionCallback=false should deliver all shallow-equal collection members when set() races with hydration', async () => { + // Clear existing storage and set up shallow-equal values for all members + await StorageMock.clear(); + await StorageMock.setItem(`${ONYX_KEYS.COLLECTION.TEST_KEY}1`, {status: 'active'}); + await StorageMock.setItem(`${ONYX_KEYS.COLLECTION.TEST_KEY}2`, {status: 'active'}); + await StorageMock.setItem(`${ONYX_KEYS.COLLECTION.TEST_KEY}3`, {status: 'active'}); + // Re-init so Onyx picks up the new storage keys + Onyx.init({keys: ONYX_KEYS}); + + const mockCallback = jest.fn(); + + Onyx.connect({ + key: ONYX_KEYS.COLLECTION.TEST_KEY, + waitForCollectionCallback: false, + callback: mockCallback, + }); + + // set() with the same shallow-equal value — this fires keyChanged synchronously, + // populating lastConnectionCallbackData before the hydration multiGet resolves. + Onyx.set(`${ONYX_KEYS.COLLECTION.TEST_KEY}1`, {status: 'active'}); + + await waitForPromisesToResolve(); + + const deliveredKeys = new Set(); + for (const call of mockCallback.mock.calls) { + const [, key] = call; + if (key) { + deliveredKeys.add(key); + } + } + + // ALL three members must be delivered, even though their values are shallow-equal. + expect(deliveredKeys).toContain(`${ONYX_KEYS.COLLECTION.TEST_KEY}1`); + expect(deliveredKeys).toContain(`${ONYX_KEYS.COLLECTION.TEST_KEY}2`); + expect(deliveredKeys).toContain(`${ONYX_KEYS.COLLECTION.TEST_KEY}3`); + }); + test('waitForCollectionCallback=false should deliver all collection members from storage', async () => { const mockCallback = jest.fn(); From f63331e99fc0da31579f7efa2347e2d5ee0264d2 Mon Sep 17 00:00:00 2001 From: VickyStash Date: Tue, 24 Mar 2026 09:18:45 +0100 Subject: [PATCH 3/3] Wrap keyChanged/keysChanged with function-level try/catch to prevent subscriber errors from blocking storage updates --- lib/OnyxUtils.ts | 216 ++++++++++++++++++++++++----------------------- 1 file changed, 112 insertions(+), 104 deletions(-) diff --git a/lib/OnyxUtils.ts b/lib/OnyxUtils.ts index fff17b8ad..c26df02f0 100644 --- a/lib/OnyxUtils.ts +++ b/lib/OnyxUtils.ts @@ -669,78 +669,82 @@ function keysChanged( partialCollection: OnyxCollection, partialPreviousCollection: OnyxCollection | undefined, ): void { - // We prepare the "cached collection" which is the entire collection + the new partial data that - // was merged in via mergeCollection(). - const cachedCollection = getCachedCollection(collectionKey); + try { + // We prepare the "cached collection" which is the entire collection + the new partial data that + // was merged in via mergeCollection(). + const cachedCollection = getCachedCollection(collectionKey); - const previousCollection = partialPreviousCollection ?? {}; + const previousCollection = partialPreviousCollection ?? {}; - // We are iterating over all subscribers similar to keyChanged(). However, we are looking for subscribers who are subscribing to either a collection key or - // individual collection key member for the collection that is being updated. It is important to note that the collection parameter cane be a PARTIAL collection - // and does not represent all of the combined keys and values for a collection key. It is just the "new" data that was merged in via mergeCollection(). - const stateMappingKeys = Object.keys(callbackToStateMapping); + // We are iterating over all subscribers similar to keyChanged(). However, we are looking for subscribers who are subscribing to either a collection key or + // individual collection key member for the collection that is being updated. It is important to note that the collection parameter cane be a PARTIAL collection + // and does not represent all of the combined keys and values for a collection key. It is just the "new" data that was merged in via mergeCollection(). + const stateMappingKeys = Object.keys(callbackToStateMapping); - for (const stateMappingKey of stateMappingKeys) { - const subscriber = callbackToStateMapping[stateMappingKey]; - if (!subscriber) { - continue; - } + for (const stateMappingKey of stateMappingKeys) { + const subscriber = callbackToStateMapping[stateMappingKey]; + if (!subscriber) { + continue; + } - // Skip iteration if we do not have a collection key or a collection member key on this subscriber - if (!Str.startsWith(subscriber.key, collectionKey)) { - continue; - } + // Skip iteration if we do not have a collection key or a collection member key on this subscriber + if (!Str.startsWith(subscriber.key, collectionKey)) { + continue; + } + + /** + * e.g. Onyx.connect({key: ONYXKEYS.COLLECTION.REPORT, callback: ...}); + */ + const isSubscribedToCollectionKey = subscriber.key === collectionKey; + + /** + * e.g. Onyx.connect({key: `${ONYXKEYS.COLLECTION.REPORT}{reportID}`, callback: ...}); + */ + const isSubscribedToCollectionMemberKey = isCollectionMemberKey(collectionKey, subscriber.key); + + // Regular Onyx.connect() subscriber found. + if (typeof subscriber.callback === 'function') { + // 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, {value: cachedCollection, matchedKey: subscriber.key}); + + if (subscriber.waitForCollectionCallback) { + subscriber.callback(cachedCollection, subscriber.key, partialCollection); + continue; + } - /** - * e.g. Onyx.connect({key: ONYXKEYS.COLLECTION.REPORT, callback: ...}); - */ - const isSubscribedToCollectionKey = subscriber.key === collectionKey; - - /** - * e.g. Onyx.connect({key: `${ONYXKEYS.COLLECTION.REPORT}{reportID}`, callback: ...}); - */ - const isSubscribedToCollectionMemberKey = isCollectionMemberKey(collectionKey, subscriber.key); - - // Regular Onyx.connect() subscriber found. - if (typeof subscriber.callback === 'function') { - // 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, {value: cachedCollection, matchedKey: subscriber.key}); - - if (subscriber.waitForCollectionCallback) { - subscriber.callback(cachedCollection, subscriber.key, partialCollection); + // 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); + } 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])) { + // And if the subscriber is specifically only tracking a particular collection member key then we will + // notify them with the cached data for that key only. + if (isSubscribedToCollectionMemberKey) { + if (deepEqual(cachedCollection[subscriber.key], previousCollection[subscriber.key])) { continue; } - subscriber.callback(cachedCollection[dataKey], dataKey); - } - continue; - } - - // And if the subscriber is specifically only tracking a particular collection member key then we will - // notify them with the cached data for that key only. - if (isSubscribedToCollectionMemberKey) { - if (deepEqual(cachedCollection[subscriber.key], previousCollection[subscriber.key])) { + const subscriberCallback = subscriber.callback as DefaultConnectCallback; + subscriberCallback(cachedCollection[subscriber.key], subscriber.key as TKey); + lastConnectionCallbackData.set(subscriber.subscriptionID, {value: cachedCollection[subscriber.key], matchedKey: subscriber.key}); continue; } - const subscriberCallback = subscriber.callback as DefaultConnectCallback; - subscriberCallback(cachedCollection[subscriber.key], subscriber.key as TKey); - lastConnectionCallbackData.set(subscriber.subscriptionID, {value: cachedCollection[subscriber.key], matchedKey: subscriber.key}); continue; } - - continue; } + } catch (error) { + Logger.logAlert(`[OnyxUtils.keysChanged] Subscriber callback threw an error for key '${collectionKey}': ${error}`); } } @@ -756,71 +760,75 @@ function keyChanged( canUpdateSubscriber: (subscriber?: CallbackToStateMapping) => boolean = () => true, isProcessingCollectionUpdate = false, ): void { - // Add or remove this key from the recentlyAccessedKeys lists - if (value !== null) { - cache.addLastAccessedKey(key, isCollectionKey(key)); - } else { - cache.removeLastAccessedKey(key); - } - - // We get the subscribers interested in the key that has just changed. If the subscriber's key is a collection key then we will - // notify them if the key that changed is a collection member. Or if it is a regular key notify them when there is an exact match. - // Given the amount of times this function is called we need to make sure we are not iterating over all subscribers every time. On the other hand, we don't need to - // do the same in keysChanged, because we only call that function when a collection key changes, and it doesn't happen that often. - // For performance reason, we look for the given key and later if don't find it we look for the collection key, instead of checking if it is a collection key first. - let stateMappingKeys = onyxKeyToSubscriptionIDs.get(key) ?? []; - const collectionKey = getCollectionKey(key); - - if (collectionKey) { - // Getting the collection key from the specific key because only collection keys were stored in the mapping. - stateMappingKeys = [...stateMappingKeys, ...(onyxKeyToSubscriptionIDs.get(collectionKey) ?? [])]; - if (stateMappingKeys.length === 0) { - return; + try { + // Add or remove this key from the recentlyAccessedKeys lists + if (value !== null) { + cache.addLastAccessedKey(key, isCollectionKey(key)); + } else { + cache.removeLastAccessedKey(key); } - } - - const cachedCollections: Record> = {}; - for (const stateMappingKey of stateMappingKeys) { - const subscriber = callbackToStateMapping[stateMappingKey]; - if (!subscriber || !isKeyMatch(subscriber.key, key) || !canUpdateSubscriber(subscriber)) { - continue; + // We get the subscribers interested in the key that has just changed. If the subscriber's key is a collection key then we will + // notify them if the key that changed is a collection member. Or if it is a regular key notify them when there is an exact match. + // Given the amount of times this function is called we need to make sure we are not iterating over all subscribers every time. On the other hand, we don't need to + // do the same in keysChanged, because we only call that function when a collection key changes, and it doesn't happen that often. + // For performance reason, we look for the given key and later if don't find it we look for the collection key, instead of checking if it is a collection key first. + let stateMappingKeys = onyxKeyToSubscriptionIDs.get(key) ?? []; + const collectionKey = getCollectionKey(key); + + if (collectionKey) { + // Getting the collection key from the specific key because only collection keys were stored in the mapping. + stateMappingKeys = [...stateMappingKeys, ...(onyxKeyToSubscriptionIDs.get(collectionKey) ?? [])]; + if (stateMappingKeys.length === 0) { + return; + } } - // Subscriber is a regular call to connect() and provided a callback - if (typeof subscriber.callback === 'function') { - const lastData = lastConnectionCallbackData.get(subscriber.subscriptionID); - if (lastData && lastData.matchedKey === key && lastData.value === value) { + const cachedCollections: Record> = {}; + + for (const stateMappingKey of stateMappingKeys) { + const subscriber = callbackToStateMapping[stateMappingKey]; + if (!subscriber || !isKeyMatch(subscriber.key, key) || !canUpdateSubscriber(subscriber)) { continue; } - if (isCollectionKey(subscriber.key) && subscriber.waitForCollectionCallback) { - // Skip individual key changes for collection callbacks during collection updates - // to prevent duplicate callbacks - the collection update will handle this properly - if (isProcessingCollectionUpdate) { + // Subscriber is a regular call to connect() and provided a callback + if (typeof subscriber.callback === 'function') { + const lastData = lastConnectionCallbackData.get(subscriber.subscriptionID); + if (lastData && lastData.matchedKey === key && lastData.value === value) { continue; } - let cachedCollection = cachedCollections[subscriber.key]; - if (!cachedCollection) { - cachedCollection = getCachedCollection(subscriber.key); - cachedCollections[subscriber.key] = cachedCollection; + if (isCollectionKey(subscriber.key) && subscriber.waitForCollectionCallback) { + // Skip individual key changes for collection callbacks during collection updates + // to prevent duplicate callbacks - the collection update will handle this properly + if (isProcessingCollectionUpdate) { + continue; + } + let cachedCollection = cachedCollections[subscriber.key]; + + if (!cachedCollection) { + cachedCollection = getCachedCollection(subscriber.key); + cachedCollections[subscriber.key] = cachedCollection; + } + + cachedCollection[key] = value; + lastConnectionCallbackData.set(subscriber.subscriptionID, {value: cachedCollection, matchedKey: subscriber.key}); + subscriber.callback(cachedCollection, subscriber.key, {[key]: value}); + continue; } - cachedCollection[key] = value; - lastConnectionCallbackData.set(subscriber.subscriptionID, {value: cachedCollection, matchedKey: subscriber.key}); - subscriber.callback(cachedCollection, subscriber.key, {[key]: value}); + const subscriberCallback = subscriber.callback as DefaultConnectCallback; + subscriberCallback(value, key); + + lastConnectionCallbackData.set(subscriber.subscriptionID, {value, matchedKey: key}); continue; } - const subscriberCallback = subscriber.callback as DefaultConnectCallback; - subscriberCallback(value, key); - - lastConnectionCallbackData.set(subscriber.subscriptionID, {value, matchedKey: key}); - continue; + console.error('Warning: Found a matching subscriber to a key that changed, but no callback could be found.'); } - - console.error('Warning: Found a matching subscriber to a key that changed, but no callback could be found.'); + } catch (error) { + Logger.logAlert(`[OnyxUtils.keyChanged] Subscriber callback threw an error for key '${key}': ${error}`); } }