From 32956378f65742fc361bb976a229602ab69b69cf Mon Sep 17 00:00:00 2001 From: eliran goshen Date: Thu, 21 May 2026 12:19:14 +0200 Subject: [PATCH 1/4] perf: batch cold-cache pre-warm in mergeCollectionWithPatches via multiGet MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaces the unconditional Promise.all(existingKeys.map(get)) pre-warm with a hybrid: - Fast path (every existingKey is already in cache): use a sync- resolved Promise — no extra microtask hops, preserving the original promise-chain depth and subscriber-callback timing that dependent tests rely on (Onyx.update batch tests broadcast a single merged callback rather than an `undefined` initial followed by the merged result). - Slow path (at least one cache-miss existingKey): use multiGet — one Storage.multiGet round-trip for the missing keys instead of N parallel get() invocations. Net result: same correctness as before, fewer storage operations on cold-cache merges, identical broadcast timing for warm-cache merges. Addresses follow-up from Expensify/react-native-onyx#787 review. --- lib/OnyxUtils.ts | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/lib/OnyxUtils.ts b/lib/OnyxUtils.ts index 06e56f8c6..aa2b8258f 100644 --- a/lib/OnyxUtils.ts +++ b/lib/OnyxUtils.ts @@ -1597,12 +1597,20 @@ function mergeCollectionWithPatches( // finalMergedCollection contains all the keys that were merged, without the keys of incompatible updates const finalMergedCollection = {...existingKeyCollection, ...newCollection}; - // Pre-warm cache for any existing storage keys that aren't yet in cache. get() is a no-op - // (sync-resolved) for cache hits, and on a cache miss it reads from storage and writes the - // value back to cache. This is required so the subsequent cache.merge() merges the new delta - // into the real previous storage value (rather than starting from `undefined` and dropping - // the existing keys). - return Promise.all(existingKeys.map((key) => get(key))).then(() => { + // Pre-warm cache for any existing storage keys that aren't yet in cache so the subsequent + // cache.merge() merges the new delta into the real previous storage value (rather than + // starting from `undefined` and dropping the existing keys). + // + // Fast path: when every existingKey is already in cache, skip the pre-warm entirely. This + // preserves the original promise-chain depth and the subscriber-callback timing that + // dependent tests rely on. + // + // Slow path: when at least one existingKey is a cache miss, use multiGet — it batches the + // missing keys into a single Storage.multiGet call (vs. N parallel get() invocations) and + // writes the storage values back to cache before resolving. + const hasColdExistingKey = existingKeys.some((key) => !cache.hasCacheForKey(key)); + const prewarmPromise = hasColdExistingKey ? multiGet(existingKeys) : Promise.resolve(); + return prewarmPromise.then(() => { // Snapshot previous values from the (now-warm) cache for keysChanged's diff, then update // cache and notify subscribers synchronously BEFORE issuing storage writes. This matches // the cache-first / storage-second invariant followed by every other Onyx write method From d02382fdfec65d862a9f3948d40b9b9be7dd9d1b Mon Sep 17 00:00:00 2001 From: eliran goshen Date: Tue, 26 May 2026 10:26:18 +0200 Subject: [PATCH 2/4] test: cover mergeCollectionWithPatches multiGet pre-warm fast/slow paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a new describe('mergeCollection pre-warm', ...) block with 5 tests: 1. Fast path: skips storage reads entirely when every existing key is warm in cache (spies on StorageMock.multiGet and StorageMock.getItem, asserts both are called 0 times). 2. Slow path: batches cold existing keys into a single Storage.multiGet, with no individual getItem calls. 3. Slow path: cold-cache merge layers the new delta on top of existing storage data (no field drops) — guards the correctness invariant the in-code comment specifically calls out. 4. Warm cache: subscriber receives a single merged broadcast for an Onyx.update batch (no transient undefined) — guards promise-chain depth. 5. Equivalence: warm-path and cold-path produce the same final cache state for the same merge. Includes a suite-pollution fix (capture pristine StorageMock refs and restore them in beforeEach so seeding via Onyx.set isn't intercepted by mocks leaking from the earlier retryOperation describe block), and an evictFromCache helper that uses for…of to satisfy unicorn/no-array-for-each. Co-Authored-By: Claude Opus 4.7 (1M context) --- tests/unit/onyxUtilsTest.ts | 171 ++++++++++++++++++++++++++++++++++++ 1 file changed, 171 insertions(+) diff --git a/tests/unit/onyxUtilsTest.ts b/tests/unit/onyxUtilsTest.ts index a545275c5..e0cbc1929 100644 --- a/tests/unit/onyxUtilsTest.ts +++ b/tests/unit/onyxUtilsTest.ts @@ -920,6 +920,177 @@ describe('OnyxUtils', () => { }); }); + describe('mergeCollection pre-warm', () => { + // The retryOperation describe block above mutates StorageMock.setItem (and never restores it), + // so by the time we run, setItem may be a rejecting mock from a prior test. Capture pristine + // references at file-load time and restore them in beforeEach so our seeding via Onyx.set + // actually reaches the in-memory storage provider. + const pristineSetItem = StorageMock.setItem; + const pristineMultiSet = StorageMock.multiSet; + const pristineMultiGet = StorageMock.multiGet; + const pristineGetItem = StorageMock.getItem; + const pristineMultiMerge = StorageMock.multiMerge; + + beforeEach(() => { + StorageMock.setItem = pristineSetItem; + StorageMock.multiSet = pristineMultiSet; + StorageMock.multiGet = pristineMultiGet; + StorageMock.getItem = pristineGetItem; + StorageMock.multiMerge = pristineMultiMerge; + }); + + // Make a key "cold" — value evicted from cache but the key is still tracked as persisted. + // OnyxCache.drop also removes the key from `storageKeys`, which would cause getAllKeys() to + // miss it (unless the entire storageKeys set is empty, in which case the function falls back + // to Storage.getAllKeys). To reliably hit the cold-but-persisted state regardless of how many + // other keys remain in cache, we re-register the key as known after dropping its value. + const evictFromCache = (...keys: string[]) => { + for (const key of keys) { + OnyxCache.drop(key); + OnyxCache.addKey(key); + } + }; + + it('fast path: skips storage reads entirely when every existing key is warm in cache', async () => { + const collectionKey = ONYXKEYS.COLLECTION.TEST_KEY; + const existingKey1 = `${collectionKey}1`; + const existingKey2 = `${collectionKey}2`; + + // Seed both members so they are both warm in cache and present in storage. + await Onyx.set(existingKey1, {value: 'initial-1'}); + await Onyx.set(existingKey2, {value: 'initial-2'}); + + const multiGetSpy = jest.spyOn(StorageMock, 'multiGet'); + const getItemSpy = jest.spyOn(StorageMock, 'getItem'); + + await Onyx.mergeCollection(collectionKey, { + [existingKey1]: {value: 'merged-1'}, + [existingKey2]: {value: 'merged-2'}, + } as GenericCollection); + + // With every existingKey warm, the diff swaps Promise.all(get) for Promise.resolve(), + // so no storage reads should happen during the pre-warm. + expect(multiGetSpy).not.toHaveBeenCalled(); + expect(getItemSpy).not.toHaveBeenCalled(); + + // Cache still reflects the merge. + const cached = OnyxCache.getCollectionData(collectionKey); + expect(cached?.[existingKey1]).toEqual({value: 'merged-1'}); + expect(cached?.[existingKey2]).toEqual({value: 'merged-2'}); + }); + + it('slow path: batches cold existing keys into a single Storage.multiGet, with no individual getItem calls', async () => { + const collectionKey = ONYXKEYS.COLLECTION.TEST_KEY; + const coldKey1 = `${collectionKey}1`; + const coldKey2 = `${collectionKey}2`; + const warmKey = `${collectionKey}3`; + + // Seed all three in storage, then evict two from cache so they are cold-but-persisted. + await Onyx.set(coldKey1, {value: 'persisted-1'}); + await Onyx.set(coldKey2, {value: 'persisted-2'}); + await Onyx.set(warmKey, {value: 'persisted-3'}); + evictFromCache(coldKey1, coldKey2); + + // Reset spies AFTER seeding so we only count calls made during mergeCollection itself. + const multiGetSpy = jest.spyOn(StorageMock, 'multiGet').mockClear(); + const getItemSpy = jest.spyOn(StorageMock, 'getItem').mockClear(); + + await Onyx.mergeCollection(collectionKey, { + [coldKey1]: {value: 'merged-1'}, + [coldKey2]: {value: 'merged-2'}, + [warmKey]: {value: 'merged-3'}, + } as GenericCollection); + + // OnyxUtils.multiGet filters to cache-missing keys before issuing Storage.multiGet, so we + // expect exactly one batched read containing only the cold keys (the warm key is skipped). + expect(multiGetSpy).toHaveBeenCalledTimes(1); + const requestedKeys = multiGetSpy.mock.calls[0][0] as string[]; + expect(requestedKeys.sort()).toEqual([coldKey1, coldKey2].sort()); + + // No individual Storage.getItem calls during pre-warm. Old code path would have fired one + // get() per existing key, each potentially landing in Storage.getItem on cache miss. + expect(getItemSpy).not.toHaveBeenCalled(); + }); + + it('slow path: cold-cache merge layers the new delta on top of existing storage data (no field drops)', async () => { + const collectionKey = ONYXKEYS.COLLECTION.TEST_KEY; + const coldKey = `${collectionKey}1`; + + // Seed an object with multiple fields in storage, then evict from cache so the merge base + // must come from a storage read — not from `undefined`. + await Onyx.set(coldKey, {a: 1, b: 2}); + evictFromCache(coldKey); + + await Onyx.mergeCollection(collectionKey, { + [coldKey]: {c: 3}, + } as GenericCollection); + + // If the pre-warm did NOT populate the cache from storage, fastMerge would treat the + // previous value as undefined and the result would drop {a:1, b:2}. With the pre-warm + // running multiGet on the cold key, the merge layers {c:3} on top of {a:1, b:2}. + const cached = OnyxCache.getCollectionData(collectionKey); + expect(cached?.[coldKey]).toEqual({a: 1, b: 2, c: 3}); + }); + + it('warm cache: subscriber receives a single merged broadcast for an Onyx.update batch (no transient undefined)', async () => { + const collectionKey = ONYXKEYS.COLLECTION.TEST_KEY; + const existingKey = `${collectionKey}1`; + + await Onyx.set(existingKey, {value: 'initial'}); + + const collectionCallback = jest.fn(); + Onyx.connect({ + key: collectionKey, + waitForCollectionCallback: true, + callback: collectionCallback, + }); + await waitForPromisesToResolve(); + collectionCallback.mockClear(); + + await Onyx.update([ + { + onyxMethod: Onyx.METHOD.MERGE_COLLECTION, + key: collectionKey, + value: { + [existingKey]: {value: 'merged'}, + } as GenericCollection, + }, + ]); + + // The fast path resolves the pre-warm synchronously (Promise.resolve()), preserving the + // original promise-chain depth. The Onyx.update batch must therefore broadcast exactly + // one merged value — not undefined first and the merged value on a later microtask. + const broadcasts = collectionCallback.mock.calls.map((c) => c[0]); + expect(broadcasts).toHaveLength(1); + expect(broadcasts[0]?.[existingKey]).toEqual({value: 'merged'}); + }); + + it('equivalence: warm-path and cold-path produce the same final cache state for the same merge', async () => { + const collectionKey = ONYXKEYS.COLLECTION.TEST_KEY; + const memberKey = `${collectionKey}1`; + const delta = {value: 'after'} as const; + + // Warm-path run. + await Onyx.set(memberKey, {value: 'before', extra: 'kept'}); + await Onyx.mergeCollection(collectionKey, { + [memberKey]: delta, + } as GenericCollection); + const warmResult = OnyxCache.getCollectionData(collectionKey)?.[memberKey]; + + // Reset and replay with a cold cache before the merge. + await Onyx.clear(); + await Onyx.set(memberKey, {value: 'before', extra: 'kept'}); + evictFromCache(memberKey); + await Onyx.mergeCollection(collectionKey, { + [memberKey]: delta, + } as GenericCollection); + const coldResult = OnyxCache.getCollectionData(collectionKey)?.[memberKey]; + + expect(warmResult).toEqual(coldResult); + expect(coldResult).toEqual({value: 'after', extra: 'kept'}); + }); + }); + describe('storage eviction', () => { const diskFullError = new Error('database or disk is full'); From d66152a3be15bd9830334a78496973714a5b77ae Mon Sep 17 00:00:00 2001 From: eliran goshen Date: Tue, 26 May 2026 10:28:17 +0200 Subject: [PATCH 3/4] fix: align multiGet cache hit check with cache.hasCacheForKey MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Inside OnyxUtils.multiGet, the cached-value path was guarded by a truthy check (`if (cacheValue)`), which treats cached falsy values (0, '', false, null) as cache misses. Those keys fell through to missingKeys → Storage.multiGet → cache.merge(temp), which would overwrite the warm cached value with whatever stale value sat in storage. This mattered most after the new perf hybrid pre-warm in mergeCollectionWithPatches started routing cold-cache merges through multiGet — the outer check (`!cache.hasCacheForKey(key)`) and the inner check disagreed for falsy values. Concrete case: `Onyx.set('coll_1', 0)` updates cache to `0` but the storage write is still in flight; a mergeCollection over the same collection key would re-fetch `coll_1` from storage and clobber the cached `0`. Aligns the inner check with hasCacheForKey so both sides agree, and adds a regression test that seeds cache with `0` and asserts multiGet returns it without touching Storage.multiGet / Storage.getItem. Addresses fabioh8010's review on PR #5 (https://github.com/callstack-internal/react-native-onyx/pull/5#pullrequestreview-4361502804). Co-Authored-By: Claude Opus 4.7 (1M context) --- lib/OnyxUtils.ts | 8 +++++--- tests/unit/onyxUtilsTest.ts | 38 +++++++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 3 deletions(-) diff --git a/lib/OnyxUtils.ts b/lib/OnyxUtils.ts index aa2b8258f..776395797 100644 --- a/lib/OnyxUtils.ts +++ b/lib/OnyxUtils.ts @@ -347,9 +347,11 @@ function multiGet(keys: CollectionKeyBase[]): Promise; - if (cacheValue) { - dataMap.set(key, cacheValue); + // Use hasCacheForKey, not a truthy check on the value — otherwise cached falsy values + // (0, '', false, null) get treated as cache misses and re-fetched from storage, which + // can overwrite the warm cached value with a stale storage value via cache.merge(). + if (cache.hasCacheForKey(key)) { + dataMap.set(key, cache.get(key) as OnyxValue); continue; } diff --git a/tests/unit/onyxUtilsTest.ts b/tests/unit/onyxUtilsTest.ts index e0cbc1929..e1b61fd43 100644 --- a/tests/unit/onyxUtilsTest.ts +++ b/tests/unit/onyxUtilsTest.ts @@ -1091,6 +1091,44 @@ describe('OnyxUtils', () => { }); }); + describe('multiGet cache hit consistency', () => { + // Same suite-pollution guard as the pre-warm block above: capture pristine StorageMock + // references at file-load time and restore them in beforeEach. The retryOperation + // describe block above mutates StorageMock.setItem (and never restores it), so by the + // time we run, setItem may be a rejecting mock from a prior test. + const pristineSetItem = StorageMock.setItem; + const pristineMultiGet = StorageMock.multiGet; + const pristineGetItem = StorageMock.getItem; + + beforeEach(() => { + StorageMock.setItem = pristineSetItem; + StorageMock.multiGet = pristineMultiGet; + StorageMock.getItem = pristineGetItem; + }); + + it('does not re-fetch a cached falsy value from storage', async () => { + const falsyKey = ONYXKEYS.TEST_KEY; + + // Seed cache with the falsy value 0 (a number, but the same logic applies to '', + // false, and null). Using `Onyx.set` ensures the value lands in cache and storage. + await Onyx.set(falsyKey, 0); + + // Spy on Storage methods to confirm multiGet does NOT round-trip to storage for + // the cached falsy value. + const multiGetSpy = jest.spyOn(StorageMock, 'multiGet'); + const getItemSpy = jest.spyOn(StorageMock, 'getItem'); + + const result = await OnyxUtils.multiGet([falsyKey]); + + // The cached value must be returned without any storage read. Before this fix, + // `if (cacheValue)` treated the cached 0 as a miss and triggered Storage.multiGet, + // which would then overwrite the warm value via cache.merge(). + expect(multiGetSpy).not.toHaveBeenCalled(); + expect(getItemSpy).not.toHaveBeenCalled(); + expect(result.get(falsyKey)).toBe(0); + }); + }); + describe('storage eviction', () => { const diskFullError = new Error('database or disk is full'); From 7fb447dc7e755f6b5983227d761ba8d5abb4e794 Mon Sep 17 00:00:00 2001 From: eliran goshen Date: Tue, 26 May 2026 11:59:32 +0200 Subject: [PATCH 4/4] fix: preserve cache-first merge when multiGet pre-warm rejects MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The new hybrid pre-warm in mergeCollectionWithPatches replaced `Promise.all(existingKeys.map(get))` with `multiGet(existingKeys)` on the slow path. The two have different failure semantics: - get() catches storage read errors per-key and resolves with undefined (see Logger.logInfo catch at the bottom of get()). - multiGet() has no .catch, so Storage.multiGet rejections propagate all the way up. In the cold-key path this meant a transient IndexedDB read error rejected before cache.merge() and keysChanged() ran — subscribers missed the in-memory merge and the outer Onyx.mergeCollection / Onyx.update promise rejected, regressing the cache-first invariant established in PR #787 (Expensify/react-native-onyx#787). Adds a .catch at the call site that swallows the rejection (logging via Logger.logInfo for visibility) so cache.merge + keysChanged still fire when pre-warm reads fail. Doesn't modify multiGet itself — other callers may legitimately depend on rejection visibility. Adds a regression test that mocks Storage.multiGet to reject on the pre-warm read and asserts: 1. Onyx.mergeCollection resolves (doesn't reject up to caller). 2. The waitForCollectionCallback subscriber sees the merged delta. Confirmed: test fails on pre-fix code with "Received: [Error: Transient IndexedDB read error]" on `expect(outerRejected).toBeNull()`. Addresses chatgpt-codex-connector review on PR #793 (https://github.com/Expensify/react-native-onyx/pull/793#discussion_r3302454987). Co-Authored-By: Claude Opus 4.7 (1M context) --- lib/OnyxUtils.ts | 9 +++++- tests/unit/onyxUtilsTest.ts | 56 +++++++++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 1 deletion(-) diff --git a/lib/OnyxUtils.ts b/lib/OnyxUtils.ts index 776395797..91b45560d 100644 --- a/lib/OnyxUtils.ts +++ b/lib/OnyxUtils.ts @@ -1611,7 +1611,14 @@ function mergeCollectionWithPatches( // missing keys into a single Storage.multiGet call (vs. N parallel get() invocations) and // writes the storage values back to cache before resolving. const hasColdExistingKey = existingKeys.some((key) => !cache.hasCacheForKey(key)); - const prewarmPromise = hasColdExistingKey ? multiGet(existingKeys) : Promise.resolve(); + // Swallow pre-warm read failures the same way the previous get()-based pre-warm did + // (see get() catch at the bottom of its definition). Without this, a transient + // Storage.multiGet rejection would skip cache.merge() + keysChanged() below and + // regress the cache-first invariant established in #787 — subscribers would miss + // the merge and Onyx.mergeCollection would reject up to the caller. + const prewarmPromise = hasColdExistingKey + ? multiGet(existingKeys).catch((err) => Logger.logInfo(`mergeCollectionWithPatches pre-warm failed; proceeding with cache-only merge. Error: ${err}`)) + : Promise.resolve(); return prewarmPromise.then(() => { // Snapshot previous values from the (now-warm) cache for keysChanged's diff, then update // cache and notify subscribers synchronously BEFORE issuing storage writes. This matches diff --git a/tests/unit/onyxUtilsTest.ts b/tests/unit/onyxUtilsTest.ts index e1b61fd43..3840cc9d3 100644 --- a/tests/unit/onyxUtilsTest.ts +++ b/tests/unit/onyxUtilsTest.ts @@ -1089,6 +1089,62 @@ describe('OnyxUtils', () => { expect(warmResult).toEqual(coldResult); expect(coldResult).toEqual({value: 'after', extra: 'kept'}); }); + it('preserves cache-first invariant when Storage.multiGet rejects on the slow path', async () => { + // Regression for codex review #3302454987 / PR #793. Before the .catch() at the + // pre-warm call site, a Storage.multiGet rejection would propagate up and skip + // cache.merge() + keysChanged() entirely — subscribers would miss the merge and + // Onyx.mergeCollection would reject. The previous get()-based pre-warm swallowed + // these errors per-key inside get()'s own .catch(), so the cache-first invariant + // from #787 held even on a flaky read. + const collectionKey = ONYXKEYS.COLLECTION.TEST_KEY; + const coldMemberKey = `${collectionKey}1`; + const newMemberKey = `${collectionKey}2`; + + // Seed an existing member, then evict it from cache so it's "tracked but unloaded" — + // the slow path will try to multiGet it. + await Onyx.set(coldMemberKey, {value: 'persisted'}); + evictFromCache(coldMemberKey); + + // Connect the subscriber and flush its initial data load FIRST, before installing + // the rejecting mock. Otherwise the connect's getCollectionDataAndSendAsObject path + // (whose multiGet call has no .catch) would consume the mockRejectedValueOnce and + // leak an unhandled rejection — not the merge pre-warm we're actually exercising. + const collectionCallback = jest.fn(); + Onyx.connect({ + key: collectionKey, + waitForCollectionCallback: true, + callback: collectionCallback, + }); + await waitForPromisesToResolve(); + collectionCallback.mockClear(); + + // The subscriber's connect re-populated cache, so re-evict to force the merge into + // the slow (cold-key) path. Then reject the next Storage.multiGet so the pre-warm + // read fails. + evictFromCache(coldMemberKey); + const transientError = new Error('Transient IndexedDB read error'); + StorageMock.multiGet = jest.fn(pristineMultiGet).mockRejectedValueOnce(transientError); + + // Outer promise must resolve, not reject, even when the pre-warm read fails. + let outerRejected: unknown = null; + const result = await Onyx.mergeCollection(collectionKey, { + [coldMemberKey]: {merged: true}, + [newMemberKey]: {value: 'new'}, + } as GenericCollection).catch((e: unknown) => { + outerRejected = e; + }); + expect(outerRejected).toBeNull(); + expect(result).toBeUndefined(); + + // cache.merge() + keysChanged() must still have fired so subscribers see the merge. + // Use toMatchObject for the cold key because a successful concurrent read may have + // re-populated the persisted value into cache; what we care about is that the new + // {merged: true} delta is applied. + expect(collectionCallback).toHaveBeenCalled(); + const lastBroadcast = collectionCallback.mock.calls.at(-1)?.[0] as Record | undefined; + expect(lastBroadcast?.[coldMemberKey]).toMatchObject({merged: true}); + expect(lastBroadcast?.[newMemberKey]).toEqual({value: 'new'}); + }); }); describe('multiGet cache hit consistency', () => {