Guard getErrorParts against non-JSON-safe thrown values - #826
Conversation
JSON.stringify circular objects and BigInt before classifyError so storage error normalization cannot throw.
tsc targets below ES2020, so use BigInt(1) instead of 1n.
Keep the undefined-stringify fallback via ?? so BigInt still works.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 953364834c
ℹ️ 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".
| */ | ||
| function serializeThrownValue(error: unknown): string { | ||
| try { | ||
| return JSON.stringify(error) ?? String(error); |
There was a problem hiding this comment.
Preserve messages from cross-realm errors
When an Error or DOMException originates in another realm, such as an iframe, both instanceof checks are false and its name and message properties are non-enumerable, so this returns {}. The previous String(error) path retained text such as Error: QuotaExceededError; losing it now causes classifyIDBError to return UNKNOWN instead of CAPACITY (and similarly hides other recognizable messages). Read error-like name/message fields structurally or fall back to String when JSON serialization erases them.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Cross-realm instanceof is a real JS thing, but it does not apply here.
This classifier only sees throws from our own IndexedDB / SQLite providers in the same window. Those are same-realm Error / DOMException, so the existing instanceof checks still match. We do not load Onyx inside an iframe that would produce a foreign-realm error.
Even in the hypothetical miss, this is not a regression vs main: main already used String(error) only after instanceof failed. JSON.stringify of an Error is {}, so we would lose name/message either way. classifyIDBError matching CAPACITY from QuotaExceededError still goes through the instanceof path.
Not changing this.
Details
getErrorPartsusedString(error)for non-Error throws. That produces[object Object]for plain objects, andJSON.stringifyon a circular object or BigInt can throw beforeclassifyErrorruns.Serialize non-Error throws with a try/fallback so circular objects, BigInt, functions, and symbols cannot crash storage error normalization. Plain objects keep their JSON so classifiers can still match fields like
message.Split out of #802.
Related Issues
For #802
Linked E/App PR
Expensify/App#99224
Automated Tests
Added
tests/unit/storage/getErrorPartsTest.tscovering Error instances, strings, null/undefined, plain objects, circular objects, and BigInt. ExistingclassifyErrorTeststill passes.Manual Tests
None.
Author Checklist
### Related Issuessection above### Linked E/App PRsection above, and verified this change against it (E/App CI passed and manual testing completed)TestssectiontoggleReportand 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