Skip to content

[Onyx] Fix dead-code async-catch in tryOrDegradePerformance#90768

Draft
elirangoshen wants to merge 2 commits into
Expensify:mainfrom
callstack-internal:elirangoshen/fix/90632-tryOrDegradePerformance-async-catch
Draft

[Onyx] Fix dead-code async-catch in tryOrDegradePerformance#90768
elirangoshen wants to merge 2 commits into
Expensify:mainfrom
callstack-internal:elirangoshen/fix/90632-tryOrDegradePerformance-async-catch

Conversation

@elirangoshen
Copy link
Copy Markdown
Contributor

Explanation of Change

This is a draft testing branch that pulls in the react-native-onyx fix for #90632 into New Expensify so reviewers and CI can validate it end-to-end before the onyx PR merges and gets a published release.

What the onyx PR fixes

tryOrDegradePerformance in lib/storage/index.ts was supposed to detect known IndexedDB failures (e.g. 'IDBKeyVal store could not be created') and gracefully fall back to an in-memory provider via degradePerformance(). It wrapped every storage call in a synchronous try/catch, but every storage provider method returns a Promise and the helper did resolve(fn()) (no await). Any rejection from fn() propagated through the promise chain and never re-entered the catch. Result: the fallback branch has been dead code and the Logger.logHmmm("Falling back to only using cache…") message has never fired in production.

The fix converts the synchronous wrapper into a proper async .catch(...) so rejections are actually caught:

function tryOrDegradePerformance<T>(
  fn: () => Promise<T> | T,
  waitForInitialization = true,
): Promise<T> {
  const initialization = waitForInitialization
    ? initPromise
    : Promise.resolve();
  return initialization.then(() =>
    Promise.resolve(fn()).catch((error: unknown) => {
      if (
        error instanceof Error &&
        error.message.includes("IDBKeyVal store could not be created")
      ) {
        degradePerformance(error);
      }
      return Promise.reject(error);
    }),
  );
}

What this PR contains

  1. package.json — bumps react-native-onyx to the head commit of Expensify/react-native-onyx#784 via the standard git-hash trick:
    "react-native-onyx": "git+https://github.com/Expensify/react-native-onyx.git#dd82e39dc9784d59f8237e143771a68707083e2b"
    
  2. Removed patches/react-native-onyx/react-native-onyx+3.0.69.patch. That patch's hasMountedRef change ([Due for payment 2026-04-28] [$250] [Onyx Audit] Fix useOnyx state not resetting on key change in getSnapshot() #85416 — one fewer useOnyx render on initial mount) is already merged upstream in onyx 3.0.74; the same hasMountedRef ref + guarded reset block are present in node_modules/react-native-onyx/dist/useOnyx.js. Keeping a 3.0.69-targeted patch on top of newer onyx source breaks npm install (patch-package fails on the diverged file), so it has to go here.

The proper version-bump PR (pinning to a published onyx release instead of a PR commit) will be opened once Expensify/react-native-onyx#784 lands and a new onyx version is released.

Fixed Issues

$ #90632
PROPOSAL: N/A — task issue assigned directly.

Tests

The fix targets the IndexedDB code path. IndexedDB is web-only — mobile platforms use SQLiteProvider and never reach the changed branch. So the tests that actually exercise the fix are all web. Mobile testing here is purely a non-regression check.

  1. npm install and npm run web. Confirm the App boots and behaves identically to main on the happy path: log in, view reports, open a chat, send a message, refresh the page.
  2. Verify in the JS console that Logger.logHmmm("Falling back to only using cache and dropping storage") is not logged during normal use.
  3. To exercise the fixed degrade path, induce an IDB failure. Easiest approach:
    • Open DevTools → Sources, set a breakpoint inside IDBKeyValProvider.setItem (or any other provider method) in node_modules/react-native-onyx/dist/storage/providers/IDBKeyValProvider.js.
    • When the breakpoint hits, use the debugger console to throw / reject with new Error('IDBKeyVal store could not be created').
  4. Continue and observe the App. Expected with this fix:
    • The App keeps working for the rest of the session (in-memory storage only, persistence dropped — same UX as today).
    • Console shows: [Onyx] Error while using IDBKeyValProvider. Falling back to only using cache and dropping storage. plus the error stack.
  5. On main (without this PR), step 4's log message never appears even though the same underlying IDB error fires — confirming the dead-code bug.
  6. Spot-check useOnyx-heavy screens (Reports list, Money Request flows) on web, iOS, and Android. The deleted patch's effect (one fewer render on initial mount per useOnyx) is now provided by upstream onyx 3.0.74, so behavior should be identical to current main.
  • Verify that no errors appear in the JS console

Offline tests

Onyx's job is offline-first storage, so the offline path is the most important non-regression check:

  1. Boot the App on web while online and log in.
  2. Disable network (DevTools → Network → Offline).
  3. Navigate around: open reports, view chat history, scroll lists. Previously-fetched data should remain readable from Onyx's cache.
  4. Send a chat message while offline — confirm it's queued optimistically.
  5. Re-enable network — confirm the queued message gets flushed.
  6. Reload while offline and confirm prior state persists from the storage provider (IndexedDB on web).
  7. Behavior should be identical to main — this PR does not change any happy-path storage behavior, only the (previously dead) error-fallback branch.

QA Steps

Same as Tests above. The fix only meaningfully changes behavior in a degraded IndexedDB-failure state, which is impractical to reproduce in QA without manual instrumentation. Primary QA value here is verifying no regressions in normal storage flows on web, since this PR routes every Onyx storage call through a rewritten helper.

  1. Log into a high-traffic account on web (staging).
  2. Verify: reports load, search works, sending/receiving messages works, refresh maintains state, navigating between workspaces works.
  3. Reload the App several times; confirm Onyx state is correctly restored from IndexedDB.
  4. On native (iOS and Android), confirm normal Onyx behavior is unchanged — this PR is a no-op on native at the storage level.
  • 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
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android: Native
    • Android: mWeb Chrome
    • iOS: Native
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.ts or at the top of the file that uses the constant) are defined as such
  • I verified that if a function's arguments changed that all usages have also been updated correctly
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari

@github-actions
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. ⚠️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant