Skip to content

Make mergeCollectionWithPatches cache-first [No QA]#91160

Draft
elirangoshen wants to merge 2 commits into
Expensify:mainfrom
callstack-internal:elirangoshen/fix/90634-mergeCollectionWithPatches-cache-first
Draft

Make mergeCollectionWithPatches cache-first [No QA]#91160
elirangoshen wants to merge 2 commits into
Expensify:mainfrom
callstack-internal:elirangoshen/fix/90634-mergeCollectionWithPatches-cache-first

Conversation

@elirangoshen

@elirangoshen elirangoshen commented May 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Explanation of Change

Bumps react-native-onyx to the head of Expensify/react-native-onyx#787, which makes mergeCollectionWithPatches cache-first (matching every other Onyx write method — setWithRetry, applyMerge, setCollectionWithRetry, partialSetCollection, clear).

Before: mergeCollectionWithPatches pushed Storage.multiMerge / Storage.multiSet first and bundled the cache.merge + keysChanged update into a previousCollectionPromise.then(...) chain that ran in parallel with storage. If the cache update lost the race to a storage failure, subscribers could observe a state inconsistent with the merge that was logically applied — the only Onyx write path where a storage error could miss the cache.

After: get() first pre-warms the cache from storage for cache-miss existing keys (sync no-op for cache hits), then cache.merge + keysChanged run synchronously before the storage promises are issued. Subscribers receive the merged data before any storage write is attempted; storage failure can no longer race the cache update.

This is the only diff in this PR — just the package.json and package-lock.json SHA bump (5 lines total). The actual code change lives in the linked Onyx PR.

Fixed Issues

$ #90634
PROPOSAL:

Tests

These steps exercise mergeCollectionWithPatches end-to-end across the App's most common callers (Search, IOU/Hold, Report/MarkAllMessageAsRead, Report/index, Policy/Policy, MoneyRequest), plus every Onyx.update batch that includes a MERGE_COLLECTION op (LHN refresh, Pusher events, etc.).

Setup

  1. Check out this branch and run npm install under Node 20.20.0 (the repo's pinned version) to fetch the new Onyx SHA
  2. Run npm run web and open https://dev.new.expensify.com:8082/ in Chrome with DevTools open
  3. Sign in to a test account

1. Initial hydration — the post-auth Pusher payload and OpenApp response trigger a large batched Onyx.update that includes MERGE_COLLECTION ops for reports, policies, and personal details.

  1. After login, verify the LHN reports list populates within a few seconds
  2. Verify the workspace list populates in the workspace switcher
  3. Expected: all reports/workspaces visible, no console errors, no missing rows compared to a baseline session on main

2. Send a chat message — optimistic merge into the report's reportActions_ collection.

  1. Open any chat report
  2. Type and send a message
  3. Expected: message appears immediately (optimistic), then confirms via Pusher with no flicker
  4. Reload the page → message is still there

3. Mark all as read (Report/MarkAllMessageAsRead.tsx → MERGE_COLLECTION)

  1. With at least one unread chat in LHN, click the option to mark all as read
  2. Expected: all unread badges clear immediately. Reload → still cleared.

4. Search & filter (Search.ts → MERGE_COLLECTION for snapshot results)

  1. Open Search (top nav)
  2. Apply any filter (date, type, status, etc.)
  3. Expected: results populate within a few seconds and update live as filters change. No duplicate rows. No console errors.

5. Hold / unhold an expense (IOU/Hold.ts → MERGE_COLLECTION)

  1. Open a report with at least one expense
  2. Hold the expense from its actions menu
  3. Expected: hold badge appears immediately; persists after reload
  4. Unhold → badge clears immediately

6. Create a money request (MoneyRequest.ts → direct Onyx.mergeCollection call)

  1. From the FAB → Submit expense → enter amount → submit
  2. Expected: expense appears in the report immediately, no duplicate, persists after reload

7. Switch workspaces (Policy/Policy.ts → MERGE_COLLECTION)

  1. Open the workspace switcher
  2. Switch to a different workspace
  3. Expected: LHN reports filter to the new workspace's chats within a few seconds

8. Storage-failure simulation — the core regression guard for this fix

The fix protects against a race where a failing Storage.multiMerge could leave the cache without the merge's update (and therefore leave subscribers stale). With the fix in place, cache.merge and keysChanged always fire before the storage write, so subscribers stay correct regardless of storage outcome.

  1. With the App running and authenticated, open Chrome DevTools → Application tab → IndexedDB
  2. Find the database used by Onyx (typically named OnyxDB — only one or two databases listed)
  3. Right-click the database → Delete database while the App is still running and connected (do NOT reload)
  4. Immediately trigger a mergeCollection-driven action — switch workspaces, send a chat message, or apply a search filter
  5. Expected with this fix: the UI updates correctly (new workspace's reports appear, message lands, search results populate). Console shows storage error logs but no white screen, no stale UI, no data loss within the session.
  6. (Regression check) Reverting just the Onyx PR locally and re-running this step should produce timing-sensitive misses where the UI doesn't reflect the action.

Evidence

A short screen recording of tests 1–4 (functional smoke), 8 (IDB-failure simulation), and 9 (cold-cache) is sufficient evidence. The race-condition tests (8, 9) are the ones reviewers should focus on since the rest are general mergeCollection smoke that would surface in any e2e run.

  • Verify that no errors appear in the JS console

Offline tests

The fix improves offline / storage-failure resilience — but it is not directly testable from the offline simulator since the race manifests only when storage operations themselves fail (e.g. IDB corruption / quota errors). The Onyx-side automated test mocks Storage.multiMerge rejection to cover this path.

For best-effort offline verification:

  1. Open the app and authenticate
  2. Toggle offline mode in DevTools
  3. Open a report and send a message — verify the optimistic update appears
  4. Switch back online — verify the message syncs and the optimistic UI remains stable

QA Steps

[No QA] — this fix targets a race condition that surfaces only when underlying storage (Storage.multiMerge) fails. The failure mode (IDB corruption / non-retriable IndexedDB errors) is not reproducible in staging through manual interaction, so there is no meaningful manual QA path. Verification is via automated tests in the companion Onyx PR.

  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the `### Fixed Issues` section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the `Tests` section
    • I added steps for the expected offline behavior in the `Offline steps` section
    • I added steps for Staging and/or Production testing in the `QA steps` section — marked [No QA] with justification
    • I added steps to cover failure scenarios — the failure scenario (storage rejection) is only reproducible in unit tests; covered by the Onyx PR's regression test
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior
    • I tested this PR with a High Traffic account — not applicable for a pure SHA bump; behavior changes are covered by Onyx unit tests
  • I included screenshots or videos for tests on all platforms — N/A, data-layer change with no UI surface
  • I ran the tests on all platforms & verified they passed on — N/A, no UI surface; Onyx unit tests cover behavior across platforms
    • Android: Native
    • Android: mWeb Chrome
    • iOS: Native
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
  • I verified there are no console errors
  • I followed proper code patterns
    • I verified that any callback methods that were added or modified are named for what the method does
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what"
    • I verified any copy / text shown in the product is localized — N/A, no copy changes
      • If any non-english text was added/modified, I used JaimeGPT — N/A
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods — N/A
    • I verified any copy / text that was added to the app is grammatically correct in English — N/A
    • I verified proper file naming conventions were followed for any new files or renamed files — N/A, no new files
    • I verified the JSDocs style guidelines were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers — N/A, no new patterns
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes — covered by the full Onyx unit test suite (452 tests)
  • I verified all code is DRY
  • I verified any variables that can be defined as constants are defined as such
  • I verified that if a function's arguments changed that all usages have also been updated correctly — function signatures unchanged
  • If any new file was added I verified that — N/A, no new files
  • If a new CSS style is added I verified that — N/A, no CSS changes
  • If new assets were added or existing ones were modified — N/A, no asset changes
  • If the PR modifies code that runs when editing or sending messages — N/A, no messaging code changes
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages — covered by Onyx unit tests
  • If the PR modifies a component related to any of the existing Storybook stories — N/A
  • If the PR modifies a component or page that can be accessed by a direct deeplink — N/A
  • If the PR modifies the UI — N/A, no UI changes
    • I verified that all the inputs inside a form are aligned with each other — N/A
    • I added Design label and/or tagged @Expensify/design — N/A
  • If a new page is added — N/A
  • I added unit tests for any new feature or bug fix — Onyx PR Make unread chats bold in chat switcher #787 includes the regression test
  • If the `main` branch was merged into this PR after a review, I tested again — N/A on first push

Screenshots/Videos

Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Screen.Recording.2026-05-20.at.14.53.27.mov
Screen.Recording.2026-05-20.at.14.54.02.mov
Screen.Recording.2026-05-20.at.15.00.50.mov
Screen.Recording.2026-05-20.at.15.06.56.mov
Screen.Recording.2026-05-20.at.15.10.33.mov
Screen.Recording.2026-05-20.at.15.12.19.mov
Screen.Recording.2026-05-20.at.15.14.31.mov
Screen.Recording.2026-05-20.at.15.16.13.mov
Screen.Recording.2026-05-20.at.15.16.36.mov
Screen.Recording.2026-05-20.at.15.17.07.mov
Screen.Recording.2026-05-20.at.15.19.23.mov
Screen.Recording.2026-05-20.at.15.27.05.mov

@elirangoshen
Pin react-native-onyx to PR Expensify#787 head for cache-first mergeC…
a800988 
…ollectionWithPatches

Bumps react-native-onyx to the head of Expensify/react-native-onyx#787,
which makes mergeCollectionWithPatches cache-first (matches every other
Onyx write method): cache + subscribers are updated synchronously before
the storage write is issued, so subscribers still reflect the merged data
when the storage write fails (e.g. IDB corruption).

Fixes Expensify#90634
@github-actions

github-actions Bot commented May 20, 2026

Copy link
Copy Markdown
Contributor

⚠️ This PR is possibly changing native code and/or updating libraries, it may cause problems with HybridApp. Please check if any patch updates are required in the HybridApp repo and run an AdHoc build to verify that HybridApp will not break. Ask Contributor Plus for help if you are not sure how to handle this. ⚠️

@elirangoshen elirangoshen mentioned this pull request May 20, 2026
Merged
41 tasks
@elirangoshen
Remove obsolete react-native-onyx patch reverting Onyx Expensify#770
9fd1811 
The patch reverted Onyx PR Expensify#770 locally. Onyx PR Expensify#785 merged the
canonical upstream revert, and the SHA bump in this PR
(42b796d64f92b8122c1e545adb6aac587df7a787) is branched off Onyx main
after Expensify#785 -- so the lines the patch tries to delete are already
gone, causing patch-package to fail on apply.

Deletes the patch file and clears its details.md entry. Behavior is
unchanged: the same revert is now in upstream Onyx.

Related: Expensify#86181
@elirangoshen elirangoshen mentioned this pull request May 25, 2026
Draft
51 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@elirangoshen elirangoshen

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@elirangoshen