Skip to content

Commit d66152a

Browse files
elirangoshenclaude
andcommitted
fix: align multiGet cache hit check with cache.hasCacheForKey
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 (#5 (review)). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent d02382f commit d66152a

2 files changed

Lines changed: 43 additions & 3 deletions

File tree

lib/OnyxUtils.ts

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -347,9 +347,11 @@ function multiGet<TKey extends OnyxKey>(keys: CollectionKeyBase[]): Promise<Map<
347347
continue;
348348
}
349349

350-
const cacheValue = cache.get(key) as OnyxValue<TKey>;
351-
if (cacheValue) {
352-
dataMap.set(key, cacheValue);
350+
// Use hasCacheForKey, not a truthy check on the value — otherwise cached falsy values
351+
// (0, '', false, null) get treated as cache misses and re-fetched from storage, which
352+
// can overwrite the warm cached value with a stale storage value via cache.merge().
353+
if (cache.hasCacheForKey(key)) {
354+
dataMap.set(key, cache.get(key) as OnyxValue<TKey>);
353355
continue;
354356
}
355357

tests/unit/onyxUtilsTest.ts

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1091,6 +1091,44 @@ describe('OnyxUtils', () => {
10911091
});
10921092
});
10931093

1094+
describe('multiGet cache hit consistency', () => {
1095+
// Same suite-pollution guard as the pre-warm block above: capture pristine StorageMock
1096+
// references at file-load time and restore them in beforeEach. The retryOperation
1097+
// describe block above mutates StorageMock.setItem (and never restores it), so by the
1098+
// time we run, setItem may be a rejecting mock from a prior test.
1099+
const pristineSetItem = StorageMock.setItem;
1100+
const pristineMultiGet = StorageMock.multiGet;
1101+
const pristineGetItem = StorageMock.getItem;
1102+
1103+
beforeEach(() => {
1104+
StorageMock.setItem = pristineSetItem;
1105+
StorageMock.multiGet = pristineMultiGet;
1106+
StorageMock.getItem = pristineGetItem;
1107+
});
1108+
1109+
it('does not re-fetch a cached falsy value from storage', async () => {
1110+
const falsyKey = ONYXKEYS.TEST_KEY;
1111+
1112+
// Seed cache with the falsy value 0 (a number, but the same logic applies to '',
1113+
// false, and null). Using `Onyx.set` ensures the value lands in cache and storage.
1114+
await Onyx.set(falsyKey, 0);
1115+
1116+
// Spy on Storage methods to confirm multiGet does NOT round-trip to storage for
1117+
// the cached falsy value.
1118+
const multiGetSpy = jest.spyOn(StorageMock, 'multiGet');
1119+
const getItemSpy = jest.spyOn(StorageMock, 'getItem');
1120+
1121+
const result = await OnyxUtils.multiGet([falsyKey]);
1122+
1123+
// The cached value must be returned without any storage read. Before this fix,
1124+
// `if (cacheValue)` treated the cached 0 as a miss and triggered Storage.multiGet,
1125+
// which would then overwrite the warm value via cache.merge().
1126+
expect(multiGetSpy).not.toHaveBeenCalled();
1127+
expect(getItemSpy).not.toHaveBeenCalled();
1128+
expect(result.get(falsyKey)).toBe(0);
1129+
});
1130+
});
1131+
10941132
describe('storage eviction', () => {
10951133
const diskFullError = new Error('database or disk is full');
10961134

0 commit comments

Comments
 (0)