diff --git a/API-INTERNAL.md b/API-INTERNAL.md index bc8c0b467..49226736e 100644 --- a/API-INTERNAL.md +++ b/API-INTERNAL.md @@ -111,10 +111,11 @@ provider) once so it's visible, then bounded retry without eviction.
broadcastUpdate()

Notifies subscribers and writes current value to cache

-
prepareKeyValuePairsForStorage() ⇒
+
prepareKeyValuePairsForStorage()

Storage expects array like: [["@MyApp_user", value_1], ["@MyApp_key", value_2]] This method transforms an object like {'@MyApp_user': myUserValue, '@MyApp_key': myKeyValue} -to an array of key-value pairs in the above format and removes key-value pairs that are being set to null

+to an array of key-value pairs in the above format, and collects the keys of null values into +keysToRemove for the caller to delete as one batch (cache drop + notification + batched storage removal).

mergeChanges(changes, existingValue)

Merges an array of changes with an existing value or creates a single change.

@@ -376,13 +377,13 @@ Notifies subscribers and writes current value to cache **Kind**: global function -## prepareKeyValuePairsForStorage() ⇒ +## prepareKeyValuePairsForStorage() Storage expects array like: [["@MyApp_user", value_1], ["@MyApp_key", value_2]] This method transforms an object like {'@MyApp_user': myUserValue, '@MyApp_key': myKeyValue} -to an array of key-value pairs in the above format and removes key-value pairs that are being set to null +to an array of key-value pairs in the above format, and collects the keys of null values into +`keysToRemove` for the caller to delete as one batch (cache drop + notification + batched storage removal). **Kind**: global function -**Returns**: an array of key - value pairs <[key, value]> ## mergeChanges(changes, existingValue) @@ -524,7 +525,6 @@ that this internal function allows passing an additional `mergeReplaceNullPatche | params.collectionKey | e.g. `ONYXKEYS.COLLECTION.REPORT` | | params.collection | Object collection keyed by individual collection member keys and values | | params.mergeReplaceNullPatches | Record where the key is a collection member key and the value is a list of tuples that we'll use to replace the nested objects of that collection member record with something else. | -| params.isProcessingCollectionUpdate | whether this is part of a collection update operation. | | retryAttempt | retry attempt | diff --git a/lib/Onyx.ts b/lib/Onyx.ts index d79836f85..0a0d8abc9 100644 --- a/lib/Onyx.ts +++ b/lib/Onyx.ts @@ -324,7 +324,7 @@ function merge(key: TKey, changes: OnyxMergeInput): * @param collection Object collection keyed by individual collection member keys and values */ function mergeCollection(collectionKey: TKey, collection: OnyxMergeCollectionInput): Promise { - return OnyxUtils.afterInit(() => OnyxUtils.mergeCollectionWithPatches({collectionKey, collection, isProcessingCollectionUpdate: true})); + return OnyxUtils.afterInit(() => OnyxUtils.mergeCollectionWithPatches({collectionKey, collection})); } /** @@ -565,7 +565,6 @@ function update(data: Array>): Promise, mergeReplaceNullPatches: batchedCollectionUpdates.mergeReplaceNullPatches, - isProcessingCollectionUpdate: true, }), ); } diff --git a/lib/OnyxUtils.ts b/lib/OnyxUtils.ts index 31c50c38e..400cfbb2b 100644 --- a/lib/OnyxUtils.ts +++ b/lib/OnyxUtils.ts @@ -64,6 +64,12 @@ function resetDiskPressureLogThrottle(): void { type OnyxMethod = ValueOf; +/** Result of `prepareKeyValuePairsForStorage`: pairs to write and keys whose `null` value marks them for removal. */ +type PreparedKeyValuePairs = { + pairs: StorageKeyValuePair[]; + keysToRemove: OnyxKey[]; +}; + // Key/value store of Onyx key and arrays of values to merge let mergeQueue: Record>> = {}; let mergeQueuePromise: Record> = {}; @@ -623,12 +629,7 @@ function keysChanged( /** * When a key change happens, search for any callbacks matching the key or collection key and trigger those callbacks */ -function keyChanged( - key: TKey, - value: OnyxValue, - canUpdateSubscriber: (subscriber?: CallbackToStateMapping) => boolean = () => true, - isProcessingCollectionUpdate = false, -): void { +function keyChanged(key: TKey, value: OnyxValue, canUpdateSubscriber: (subscriber?: CallbackToStateMapping) => boolean = () => true): void { // Add or remove this key from the recentlyAccessedKeys list if (value !== null && value !== undefined) { cache.addLastAccessedKey(key, OnyxKeys.isCollectionKey(key)); @@ -672,11 +673,6 @@ function keyChanged( } if (OnyxKeys.isCollectionKey(subscriber.key)) { - // Skip individual key changes during collection updates to prevent duplicate - // callbacks - the collection update will handle this properly. - if (isProcessingCollectionUpdate) { - continue; - } // Cache once per dispatch to ensure all subscribers see a consistent snapshot // even if a previous callback synchronously wrote to the same collection. let cachedCollection = cachedCollections[subscriber.key]; @@ -755,9 +751,9 @@ function getCollectionDataAndSendAsObject(matchingKeys: Co /** * Remove a key from Onyx and update the subscribers */ -function remove(key: TKey, isProcessingCollectionUpdate?: boolean): Promise { +function remove(key: TKey): Promise { cache.drop(key); - keyChanged(key, undefined as OnyxValue, undefined, isProcessingCollectionUpdate); + keyChanged(key, undefined as OnyxValue); if (OnyxKeys.isRamOnlyKey(key)) { return Promise.resolve(); @@ -919,21 +915,20 @@ function hasPendingMergeForKey(key: OnyxKey): boolean { /** * Storage expects array like: [["@MyApp_user", value_1], ["@MyApp_key", value_2]] * This method transforms an object like {'@MyApp_user': myUserValue, '@MyApp_key': myKeyValue} - * to an array of key-value pairs in the above format and removes key-value pairs that are being set to null - * - * @return an array of key - value pairs <[key, value]> + * to an array of key-value pairs in the above format, and collects the keys of null values into + * `keysToRemove` for the caller to delete as one batch (cache drop + notification + batched storage removal). */ function prepareKeyValuePairsForStorage( data: Record>, shouldRemoveNestedNulls?: boolean, replaceNullPatches?: MultiMergeReplaceNullPatches, - isProcessingCollectionUpdate?: boolean, -): StorageKeyValuePair[] { +): PreparedKeyValuePairs { const pairs: StorageKeyValuePair[] = []; + const keysToRemove: OnyxKey[] = []; for (const [key, value] of Object.entries(data)) { if (value === null) { - remove(key, isProcessingCollectionUpdate); + keysToRemove.push(key); continue; } @@ -944,7 +939,7 @@ function prepareKeyValuePairsForStorage( } } - return pairs; + return {pairs, keysToRemove}; } /** @@ -1408,7 +1403,12 @@ function multiSetWithRetry(data: OnyxMultiSetInput, retryAttempt?: number): Prom }, {}); } - const keyValuePairsToSet = OnyxUtils.prepareKeyValuePairsForStorage(newData, true); + const {pairs: keyValuePairsToSet, keysToRemove: removalCandidates} = OnyxUtils.prepareKeyValuePairsForStorage(newData, true); + + // Removals of keys that are neither cached nor persisted are no-ops and skipped. When the key + // index has not been loaded yet (empty set), keep the removal to be safe. + const persistedKeys = cache.getAllKeys(); + const keysToRemove = removalCandidates.filter((key) => cache.get(key) !== undefined || persistedKeys.size === 0 || persistedKeys.has(key)); // Group collection members by their parent collection key so each collection can be notified // via a single batched keysChanged() call instead of one keyChanged() per member. For each @@ -1456,6 +1456,27 @@ function multiSetWithRetry(data: OnyxMultiSetInput, retryAttempt?: number): Prom } } + // Null keys join the same per-collection batches (as undefined) and are deleted from storage + // in one batched call below, so cross-tab sync raises a single event instead of one per key. + for (const key of keysToRemove) { + const previousValue = cache.get(key); + cache.drop(key); + + const collectionKey = OnyxKeys.getCollectionKey(key); + if (collectionKey && OnyxKeys.isCollectionMemberKey(collectionKey, key)) { + let batch = collectionBatches.get(collectionKey); + if (!batch) { + batch = {partial: {}, previous: {}}; + collectionBatches.set(collectionKey, batch); + } + batch.partial[key] = undefined; + batch.previous[key] = previousValue; + } else if (!retryAttempt) { + // Skip subscriber notification on retry — already notified on attempt 0. + keyChanged(key, undefined); + } + } + // One keysChanged() per collection — fires each collection-level subscriber once and lets // keysChanged() internally decide which individual member subscribers need notification. // Skip on retry — already notified on attempt 0 (see same-reason comment above). @@ -1470,10 +1491,17 @@ function multiSetWithRetry(data: OnyxMultiSetInput, retryAttempt?: number): Prom // Filter out the RAM-only key value pairs, as they should not be saved to storage return !OnyxKeys.isRamOnlyKey(key); }); + const keysToRemoveFromStorage = keysToRemove.filter((key) => !OnyxKeys.isRamOnlyKey(key)); const inFlightKeys = new Set(keyValuePairsToSet.map(([key]) => key)); - return Storage.multiSet(keyValuePairsToStore) + // A failed removal is logged, not retried — keysToRemove cannot be re-derived after the cache update. + const storagePromises = [Storage.multiSet(keyValuePairsToStore)]; + if (keysToRemoveFromStorage.length > 0) { + storagePromises.push(Storage.removeItems(keysToRemoveFromStorage).catch((error) => Logger.logAlert(`multiSet failed to remove keys from storage. Error: ${error}`))); + } + + return Promise.all(storagePromises) .then(() => StorageCircuitBreaker.recordWriteSuccess()) .catch((error) => OnyxUtils.retryOperation(error, multiSetWithRetry, newData, retryAttempt, inFlightKeys)) .then(() => { @@ -1534,15 +1562,21 @@ function setCollectionWithRetry({collectionKey, mutableCollection[key] = null; } - const keyValuePairs = OnyxUtils.prepareKeyValuePairsForStorage(mutableCollection, true, undefined, true); + const {pairs: keyValuePairs, keysToRemove: removalCandidates} = OnyxUtils.prepareKeyValuePairsForStorage(mutableCollection, true); + // Removals of keys that are neither cached nor persisted are no-ops and skipped. + const keysToRemove = removalCandidates.filter((key) => cache.get(key) !== undefined || persistedKeys.has(key)); + // Snapshot before cache mutations so keysChanged() can diff removed members. const previousCollection = OnyxUtils.getCachedCollection(collectionKey); for (const [key, value] of keyValuePairs) cache.set(key, value); + for (const key of keysToRemove) cache.drop(key); // Skip subscriber notification on retry — already notified on attempt 0. // Collection-root subscribers re-fire on every keysChanged by contract. if (!retryAttempt) { - keysChanged(collectionKey, mutableCollection, previousCollection); + // Removed members are notified as undefined, matching mergeCollection/multiSet. + const partialForNotify = Object.fromEntries(Object.entries(mutableCollection).map(([key, value]) => [key, value ?? undefined])); + keysChanged(collectionKey, partialForNotify, previousCollection); } // RAM-only keys are not supposed to be saved to storage @@ -1553,7 +1587,14 @@ function setCollectionWithRetry({collectionKey, const inFlightKeys = new Set(keyValuePairs.map(([key]) => key)); - return Storage.multiSet(keyValuePairs) + // One batched removal = one cross-tab sync event instead of one per key. A failed removal is + // logged, not retried — keysToRemove cannot be re-derived after the cache update. + const storagePromises = [Storage.multiSet(keyValuePairs)]; + if (keysToRemove.length > 0) { + storagePromises.push(Storage.removeItems(keysToRemove).catch((error) => Logger.logAlert(`setCollection failed to remove keys from storage. Error: ${error}`))); + } + + return Promise.all(storagePromises) .then(() => StorageCircuitBreaker.recordWriteSuccess()) .catch((error) => OnyxUtils.retryOperation(error, setCollectionWithRetry, {collectionKey, collection}, retryAttempt, inFlightKeys)) .then(() => { @@ -1572,11 +1613,10 @@ function setCollectionWithRetry({collectionKey, * @param params.collection Object collection keyed by individual collection member keys and values * @param params.mergeReplaceNullPatches Record where the key is a collection member key and the value is a list of * tuples that we'll use to replace the nested objects of that collection member record with something else. - * @param params.isProcessingCollectionUpdate whether this is part of a collection update operation. * @param retryAttempt retry attempt */ function mergeCollectionWithPatches( - {collectionKey, collection, mergeReplaceNullPatches, isProcessingCollectionUpdate = false}: MergeCollectionWithPatchesParams, + {collectionKey, collection, mergeReplaceNullPatches}: MergeCollectionWithPatchesParams, retryAttempt?: number, ): Promise { if (!isValidNonEmptyCollectionForMerge(collection)) { @@ -1612,15 +1652,34 @@ function mergeCollectionWithPatches( return getAllKeys() .then((persistedKeys) => { - // Split to keys that exist in storage and keys that don't + // Split to keys that exist in storage and keys that don't. Null members are collected + // for one batched removal below; nulls that are neither cached nor persisted are no-ops and skipped. + const keysToRemove: OnyxKey[] = []; const keys = resultCollectionKeys.filter((key) => { if (resultCollection[key] === null) { - remove(key, isProcessingCollectionUpdate); + if (cache.get(key) !== undefined || persistedKeys.has(key)) { + keysToRemove.push(key); + } return false; } return true; }); + // Drop removed members before the pre-warm await below, so a concurrent write to one of + // these keys during the pre-warm is not wiped out by a late drop. + const removedPreviousValues: OnyxInputKeyValueMapping = {}; + for (const key of keysToRemove) { + removedPreviousValues[key] = cache.get(key); + cache.drop(key); + } + + // One batched removal = one cross-tab sync event instead of one per key. Issued at drop time + // so a concurrent later write to a removed key persists after the removal. + const removalPromise = + !OnyxKeys.isRamOnlyKey(collectionKey) && keysToRemove.length > 0 + ? Storage.removeItems(keysToRemove).catch((error) => Logger.logAlert(`mergeCollection failed to remove keys from storage. Error: ${error}`)) + : undefined; + const existingKeys = keys.filter((key) => persistedKeys.has(key)); const cachedCollectionForExistingKeys = getCachedCollection(collectionKey, existingKeys); @@ -1658,11 +1717,11 @@ function mergeCollectionWithPatches( // When (multi-)merging the values with the existing values in storage, // we don't want to remove nested null values from the data that we pass to the storage layer, // because the storage layer uses them to remove nested keys from storage natively. - const keyValuePairsForExistingCollection = prepareKeyValuePairsForStorage(existingKeyCollection, false, mergeReplaceNullPatches); + const {pairs: keyValuePairsForExistingCollection} = prepareKeyValuePairsForStorage(existingKeyCollection, false, mergeReplaceNullPatches); // We can safely remove nested null values when using (multi-)set, // because we will simply overwrite the existing values in storage. - const keyValuePairsForNewCollection = prepareKeyValuePairsForStorage(newCollection, true); + const {pairs: keyValuePairsForNewCollection} = prepareKeyValuePairsForStorage(newCollection, true); // finalMergedCollection contains all the keys that were merged, without the keys of incompatible updates const finalMergedCollection = { @@ -1688,15 +1747,24 @@ function mergeCollectionWithPatches( // ensuring subscribers still reflect the merged data even if the subsequent storage // write fails. const previousCollection = getCachedCollection(collectionKey, existingKeys); + cache.merge(finalMergedCollection); // Skip subscriber notification on retry — already notified on attempt 0. // Collection-root subscribers re-fire on every keysChanged by contract. if (!retryAttempt) { - keysChanged(collectionKey, finalMergedCollection, previousCollection); + const partialForNotify = keysToRemove.length > 0 ? {...finalMergedCollection, ...Object.fromEntries(keysToRemove.map((key) => [key, undefined]))} : finalMergedCollection; + const previousForNotify = keysToRemove.length > 0 ? {...previousCollection, ...removedPreviousValues} : previousCollection; + if (Object.keys(partialForNotify).length > 0) { + keysChanged(collectionKey, partialForNotify, previousForNotify); + } } const promises = []; + if (removalPromise) { + promises.push(removalPromise); + } + // New keys go through multiSet and existing keys through multiMerge. multiMerge on a // missing key stores the value just like multiSet across all backends; splitting them lets // multiSet strip nested nulls (the merge layer keeps them to delete nested storage keys). @@ -1722,7 +1790,6 @@ function mergeCollectionWithPatches( collectionKey, collection: resultCollection as OnyxMergeCollectionInput, mergeReplaceNullPatches, - isProcessingCollectionUpdate, }, retryAttempt, inFlightKeys, @@ -1777,15 +1844,21 @@ function partialSetCollection({collectionKey, co return getAllKeys().then((persistedKeys) => { const mutableCollection: OnyxInputKeyValueMapping = {...resultCollection}; const existingKeys = resultCollectionKeys.filter((key) => persistedKeys.has(key)); + const {pairs: keyValuePairs, keysToRemove: removalCandidates} = prepareKeyValuePairsForStorage(mutableCollection, true); + // Removals of keys that are neither cached nor persisted are no-ops and skipped. + const keysToRemove = removalCandidates.filter((key) => cache.get(key) !== undefined || persistedKeys.has(key)); + // Snapshot before cache mutations so keysChanged() can diff removed members. const previousCollection = getCachedCollection(collectionKey, existingKeys); - const keyValuePairs = prepareKeyValuePairsForStorage(mutableCollection, true, undefined, true); for (const [key, value] of keyValuePairs) cache.set(key, value); + for (const key of keysToRemove) cache.drop(key); // Skip subscriber notification on retry — already notified on attempt 0. // Collection-root subscribers re-fire on every keysChanged by contract. if (!retryAttempt) { - keysChanged(collectionKey, mutableCollection, previousCollection); + // Removed members are notified as undefined, matching mergeCollection/multiSet. + const partialForNotify = Object.fromEntries(Object.entries(mutableCollection).map(([key, value]) => [key, value ?? undefined])); + keysChanged(collectionKey, partialForNotify, previousCollection); } if (OnyxKeys.isRamOnlyKey(collectionKey)) { @@ -1795,7 +1868,14 @@ function partialSetCollection({collectionKey, co const inFlightKeys = new Set(keyValuePairs.map(([key]) => key)); - return Storage.multiSet(keyValuePairs) + // One batched removal = one cross-tab sync event instead of one per key. A failed removal is + // logged, not retried — keysToRemove cannot be re-derived after the cache update. + const storagePromises = [Storage.multiSet(keyValuePairs)]; + if (keysToRemove.length > 0) { + storagePromises.push(Storage.removeItems(keysToRemove).catch((error) => Logger.logAlert(`setCollection failed to remove keys from storage. Error: ${error}`))); + } + + return Promise.all(storagePromises) .then(() => StorageCircuitBreaker.recordWriteSuccess()) .catch((error) => retryOperation(error, partialSetCollection, {collectionKey, collection}, retryAttempt, inFlightKeys)) .then(() => { diff --git a/lib/storage/InstanceSync/index.web.ts b/lib/storage/InstanceSync/index.web.ts index b79491cab..640adcf38 100644 --- a/lib/storage/InstanceSync/index.web.ts +++ b/lib/storage/InstanceSync/index.web.ts @@ -100,6 +100,10 @@ const InstanceSync = { init: (onStorageKeysChanged: OnStorageKeysChanged, store: StorageProvider) => { storage = store; + // Coalesce storage events into one dispatch per tick: a per-key sender would otherwise re-run + // the whole notification pipeline once per key and can flood the receiving tab into unresponsiveness. + let pendingSyncKeys: Set | null = null; + // This listener will only be triggered by events coming from other tabs global.addEventListener('storage', (event) => { // Ignore events that don't originate from the SYNC_ONYX logic @@ -109,7 +113,19 @@ const InstanceSync = { const onyxKeys = parseSyncOnyxStorageEventValue(event.newValue); - storage.multiGet(onyxKeys).then((pairs) => onStorageKeysChanged(pairs)); + if (pendingSyncKeys) { + for (const onyxKey of onyxKeys) { + pendingSyncKeys.add(onyxKey); + } + return; + } + + pendingSyncKeys = new Set(onyxKeys); + setTimeout(() => { + const keys = Array.from(pendingSyncKeys ?? []); + pendingSyncKeys = null; + storage.multiGet(keys).then((pairs) => onStorageKeysChanged(pairs)); + }, 0); }); }, setItem: raiseStorageSyncEvent, diff --git a/lib/types.ts b/lib/types.ts index f2b5b8014..96f130813 100644 --- a/lib/types.ts +++ b/lib/types.ts @@ -342,7 +342,6 @@ type MergeCollectionWithPatchesParams = { collectionKey: TKey; collection: OnyxMergeCollectionInput; mergeReplaceNullPatches?: MultiMergeReplaceNullPatches; - isProcessingCollectionUpdate?: boolean; }; type RetriableOnyxOperation = diff --git a/tests/unit/onyxTest.ts b/tests/unit/onyxTest.ts index 938a64038..a36c79ec2 100644 --- a/tests/unit/onyxTest.ts +++ b/tests/unit/onyxTest.ts @@ -2981,6 +2981,194 @@ describe('Onyx', () => { }); }); + describe('batched collection member removals', () => { + const routeA = `${ONYX_KEYS.COLLECTION.ROUTES}A`; + const routeB = `${ONYX_KEYS.COLLECTION.ROUTES}B`; + + it('mergeCollection deletes null members from cache and storage via one batched removeItems call', async () => { + await Onyx.mergeCollection(ONYX_KEYS.COLLECTION.ROUTES, { + [routeA]: {name: 'Route A'}, + [routeB]: {name: 'Route B'}, + } as GenericCollection); + + (StorageMock.removeItem as jest.Mock).mockClear(); + (StorageMock.removeItems as jest.Mock).mockClear(); + + await Onyx.mergeCollection(ONYX_KEYS.COLLECTION.ROUTES, { + [routeA]: null, + [routeB]: {name: 'Route B v2'}, + } as GenericCollection); + + // Per-key removals would raise one cross-tab sync event per member; the batch must persist in one call. + expect(StorageMock.removeItem).not.toHaveBeenCalled(); + expect(StorageMock.removeItems).toHaveBeenCalledTimes(1); + expect(StorageMock.removeItems).toHaveBeenCalledWith([routeA]); + + expect(cache.get(routeA)).toBeUndefined(); + const keys = await OnyxUtils.getAllKeys(); + expect(keys.has(routeA)).toBe(false); + expect(keys.has(routeB)).toBe(true); + }); + + it('mergeCollection notifies member subscribers about batched removals', async () => { + await Onyx.mergeCollection(ONYX_KEYS.COLLECTION.ROUTES, { + [routeA]: {name: 'Route A'}, + } as GenericCollection); + + let received: unknown = 'sentinel'; + connection = Onyx.connect({ + key: routeA, + callback: (value) => (received = value), + }); + await waitForPromisesToResolve(); + expect(received).toEqual({name: 'Route A'}); + + await Onyx.mergeCollection(ONYX_KEYS.COLLECTION.ROUTES, { + [routeA]: null, + } as GenericCollection); + + expect(received).toBeUndefined(); + }); + + it('mergeCollection skips removals of members that are neither cached nor persisted', async () => { + const routeMissing = `${ONYX_KEYS.COLLECTION.ROUTES}Missing`; + + await Onyx.mergeCollection(ONYX_KEYS.COLLECTION.ROUTES, { + [routeA]: {name: 'Route A'}, + } as GenericCollection); + + (StorageMock.removeItem as jest.Mock).mockClear(); + (StorageMock.removeItems as jest.Mock).mockClear(); + + // Nulling a member that was never stored must not raise any storage removal. + await Onyx.mergeCollection(ONYX_KEYS.COLLECTION.ROUTES, { + [routeMissing]: null, + [routeA]: {name: 'Route A v2'}, + } as GenericCollection); + + expect(StorageMock.removeItem).not.toHaveBeenCalled(); + expect(StorageMock.removeItems).not.toHaveBeenCalled(); + }); + + it('setCollection deletes missing members via one batched removeItems call', async () => { + await Onyx.mergeCollection(ONYX_KEYS.COLLECTION.ROUTES, { + [routeA]: {name: 'Route A'}, + [routeB]: {name: 'Route B'}, + } as GenericCollection); + + (StorageMock.removeItem as jest.Mock).mockClear(); + (StorageMock.removeItems as jest.Mock).mockClear(); + + await Onyx.setCollection(ONYX_KEYS.COLLECTION.ROUTES, { + [routeA]: {name: 'New Route A'}, + } as GenericCollection); + + expect(StorageMock.removeItem).not.toHaveBeenCalled(); + expect(StorageMock.removeItems).toHaveBeenCalledTimes(1); + expect(StorageMock.removeItems).toHaveBeenCalledWith([routeB]); + + const keys = await OnyxUtils.getAllKeys(); + expect(keys.has(routeB)).toBe(false); + }); + + it('notifies member subscribers when a cached-only (RAM-only) member is removed via a batched set', async () => { + const ramKey = `${ONYX_KEYS.COLLECTION.RAM_ONLY_COLLECTION}removal`; + + await Onyx.mergeCollection(ONYX_KEYS.COLLECTION.RAM_ONLY_COLLECTION, { + [ramKey]: {name: 'RAM member'}, + } as GenericCollection); + + let received: unknown = 'sentinel'; + connection = Onyx.connect({ + key: ramKey, + callback: (value) => (received = value), + }); + await waitForPromisesToResolve(); + expect(received).toEqual({name: 'RAM member'}); + + // Two set updates on members of the same collection are batched into partialSetCollection, + // where the removed member exists only in cache (RAM-only keys are never persisted). + const ramKeyOther = `${ONYX_KEYS.COLLECTION.RAM_ONLY_COLLECTION}other`; + await Onyx.update([ + {onyxMethod: Onyx.METHOD.SET, key: ramKey, value: null}, + {onyxMethod: Onyx.METHOD.SET, key: ramKeyOther, value: {name: 'other'}}, + ]); + + expect(received).toBeUndefined(); + }); + + it('multiSet deletes null keys via one batched removeItems call', async () => { + await Onyx.multiSet({[ONYX_KEYS.OTHER_TEST]: 42}); + + (StorageMock.removeItem as jest.Mock).mockClear(); + (StorageMock.removeItems as jest.Mock).mockClear(); + + await Onyx.multiSet({[ONYX_KEYS.OTHER_TEST]: null}); + + expect(StorageMock.removeItem).not.toHaveBeenCalled(); + expect(StorageMock.removeItems).toHaveBeenCalledTimes(1); + expect(StorageMock.removeItems).toHaveBeenCalledWith([ONYX_KEYS.OTHER_TEST]); + expect(cache.get(ONYX_KEYS.OTHER_TEST)).toBeUndefined(); + }); + + it('multiSet skips removals of keys that are neither cached nor persisted', async () => { + const routeMissing = `${ONYX_KEYS.COLLECTION.ROUTES}Missing`; + await Onyx.multiSet({[ONYX_KEYS.OTHER_TEST]: 42}); + + (StorageMock.removeItem as jest.Mock).mockClear(); + (StorageMock.removeItems as jest.Mock).mockClear(); + + // Nulling a key that was never stored must not raise any storage removal. + await Onyx.multiSet({[routeMissing]: null, [ONYX_KEYS.OTHER_TEST]: 43}); + + expect(StorageMock.removeItem).not.toHaveBeenCalled(); + expect(StorageMock.removeItems).not.toHaveBeenCalled(); + }); + + it('mergeCollection removal does not wipe out a concurrent write issued during the pre-warm read', async () => { + await Onyx.mergeCollection(ONYX_KEYS.COLLECTION.ROUTES, { + [routeA]: {a: 1}, + [routeB]: {b: 1}, + } as GenericCollection); + + // Evict both members' values (their keys stay indexed) so the merge below takes the slow + // pre-warm path and awaits a real storage read before applying. + cache.set(routeA, undefined); + cache.set(routeB, undefined); + + const removal = Onyx.mergeCollection(ONYX_KEYS.COLLECTION.ROUTES, { + [routeA]: null, + [routeB]: {b: 2}, + } as GenericCollection); + const concurrent = Onyx.merge(routeA, {y: 2}); + await Promise.all([removal, concurrent]); + + // The merge was issued after the removal, so it must win in both cache and storage. + expect(cache.get(routeA)).toEqual(expect.objectContaining({y: 2})); + const persisted = await StorageMock.getItem(routeA); + expect(persisted).toEqual(expect.objectContaining({y: 2})); + }); + + it('does not re-run the whole write when only the batched removal fails', async () => { + await Onyx.mergeCollection(ONYX_KEYS.COLLECTION.ROUTES, { + [routeA]: {name: 'Route A'}, + [routeB]: {name: 'Route B'}, + } as GenericCollection); + + (StorageMock.multiSet as jest.Mock).mockClear(); + (StorageMock.removeItems as jest.Mock).mockImplementationOnce(() => Promise.reject(new Error('storage removal failed'))); + + // routeB is missing from the new collection, so its removal is attempted and fails. + await Onyx.setCollection(ONYX_KEYS.COLLECTION.ROUTES, { + [routeA]: {name: 'New Route A'}, + } as GenericCollection); + + // The failed removal must not trigger a retry of the (already successful) multiSet. + expect(StorageMock.multiSet).toHaveBeenCalledTimes(1); + expect(cache.get(routeB)).toBeUndefined(); + }); + }); + describe('clear', () => { it('should handle RAM-only keys with defaults correctly during clear', async () => { // Set a value for RAM-only key diff --git a/tests/unit/storage/instanceSyncWebTest.ts b/tests/unit/storage/instanceSyncWebTest.ts index c27d5a85f..0dc750494 100644 --- a/tests/unit/storage/instanceSyncWebTest.ts +++ b/tests/unit/storage/instanceSyncWebTest.ts @@ -109,6 +109,34 @@ describe('InstanceSync (web)', () => { expect(multiGet).toHaveBeenCalledWith(['123']); }); + it('coalesces a burst of storage events into one multiGet and one dispatch', async () => { + // A tab running an older bundle emits one event per key; the burst must collapse into one batch. + storageEventHandler({key: SYNC_ONYX, newValue: 'test_1'}); + storageEventHandler({key: SYNC_ONYX, newValue: JSON.stringify(['test_2', 'test_3'])}); + storageEventHandler({key: SYNC_ONYX, newValue: 'test_2'}); + await waitForPromisesToResolve(); + + expect(multiGet).toHaveBeenCalledTimes(1); + expect(multiGet).toHaveBeenCalledWith(['test_1', 'test_2', 'test_3']); + expect(onStorageKeysChanged).toHaveBeenCalledTimes(1); + expect(onStorageKeysChanged).toHaveBeenCalledWith([ + ['test_1', 'value_of_test_1'], + ['test_2', 'value_of_test_2'], + ['test_3', 'value_of_test_3'], + ]); + }); + + it('dispatches separate batches for separate bursts', async () => { + storageEventHandler({key: SYNC_ONYX, newValue: 'test_1'}); + await waitForPromisesToResolve(); + storageEventHandler({key: SYNC_ONYX, newValue: 'test_2'}); + await waitForPromisesToResolve(); + + expect(multiGet).toHaveBeenCalledTimes(2); + expect(multiGet).toHaveBeenNthCalledWith(1, ['test_1']); + expect(multiGet).toHaveBeenNthCalledWith(2, ['test_2']); + }); + it('ignores storage events that are not SYNC_ONYX', async () => { storageEventHandler({key: 'someOtherKey', newValue: 'test_1'}); storageEventHandler({key: SYNC_ONYX, newValue: null});