Fix dead-code async-catch in tryOrDegradePerformance App#784
Conversation
|
I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd82e39dc9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
|
recheck |
|
@elirangoshen I will review soon, but meanwhile:
|
|
@elirangoshen could you try with just |
Details
The
tryOrDegradePerformancehelper inlib/storage/index.tswraps storage operations in atry/catchintended to detect critical IndexedDB failures and fall back to the in-memory provider viadegradePerformance().The problem is that the
try/catchonly handles synchronous exceptions, while all storage provider operations are asynchronous.The previous implementation effectively did:
This forwards the eventual result of
fn()into the promise chain, but asynchronous rejections bypass the surroundingcatchblock entirely.As a result:
degradePerformance()Logger.logHmmm('Falling back to only using cache and dropping storage...')This PR fixes the issue by converting the synchronous
try/catchflow into proper promise rejection handling:Promise.resolve(fn())normalizes both synchronous and asynchronous return paths so.catch(...)consistently handles sync throws and async rejections.While updating this logic, the
'Internal error opening backing store for indexedDB.open'branch was removed from this function.Per the investigation in:
Expensify/App#87862 (comment)
that error indicates permanent IndexedDB corruption that cannot be recovered by switching to
MemoryOnlyProvider. A dedicated heal flow will instead handle that case in:Expensify/App#90636
The
'IDBKeyVal store could not be created'branch remains because falling back to in-memory storage is still the correct behavior for initialization failures.Related Issues
Automated Tests
Added
tests/unit/storage/tryOrDegradePerformanceTest.tswith two test cases that exercise the storage module through its public API (sincetryOrDegradePerformanceis not exported).1. Async rejection with the target error message triggers degradation
Replaces the active provider method with one that returns:
Then performs a storage operation and verifies that:
Logger.logHmmmis called with the"Falling back to only using cache..."message, confirming thatdegradePerformance()executedstorage.getStorageProvider().name === 'MemoryOnlyProvider'2. Async rejection with an unrelated error does not trigger degradation
Uses the same setup, but with:
Verifies that:
Logger.logHmmmis not calledBoth tests use
jest.isolateModules()to load a fresh instance oflib/storagefor each test, preventing leakage of the module-private provider state between runs.The tests also use:
to bypass the global mock configured in
jestSetup.js.Without this fix, the first test fails because the async rejection bypasses the previous synchronous
catchblock anddegradePerformance()is never called.Validation
npm testpasses (17 suites / 451 tests)npm run typecheckpassesManual Tests
In a local
Expensify/Appcheckout, pointreact-native-onyxto this branch (for example via local path install or a prerelease version).Launch the app on web and verify normal storage functionality works without regressions:
To trigger the fixed failure path:
OnyxDBdatabase while the app is runningAlternatively, temporarily patch
IDBKeyValProvider.setItem()(or another provider method) to return:Then reload the app.
Expected behavior
Before this fix, the underlying async error occurred but the fallback logic was never triggered, so this log message did not appear.
"Falling back to only using cache and dropping storage"message for users hitting this failure mode (currently observed 0 times, confirming the previous fallback path was effectively dead code).Author Checklist
### Related Issuessection aboveTestssectiontoggleReportand notonIconClick)myBool && <MyComponent />.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)Avataris modified, I verified thatAvataris working as expected in all cases)mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari