Gracefully handle malformed currency codes instead of crashing#91114
Open
mountiny wants to merge 6 commits into
Open
Gracefully handle malformed currency codes instead of crashing#91114mountiny wants to merge 6 commits into
mountiny wants to merge 6 commits into
Conversation
When a workspace policy or transaction has an invalid currency code stored in the backend, Intl.NumberFormat throws a RangeError that crashes the expense amount screen. Add two-layer protection: 1. sanitizeCurrencyCode helper in CurrencyUtils.ts: validates the format (ISO 4217 = exactly 3 uppercase ASCII letters) and falls back to USD with a Log.warn when the code is malformed. Applied in getLocalizedCurrencySymbol, convertToDisplayString, convertToDisplayStringWithoutCurrency, convertToShortDisplayString, and convertAmountToDisplayString. 2. try/catch safety net in NumberFormatUtils/index.ts: catches any RangeError from Intl.NumberFormat caused by a bad currency option that slips past layer 1, retries with USD, and logs a warning. Also sanitizes currency in TotalCell.tsx before its direct formatToParts call. Fixes #91113 Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
Contributor
There was a problem hiding this comment.
Pull request overview
Addresses a production crash caused by malformed currency codes reaching Intl.NumberFormat by sanitizing inputs and adding a defensive fallback in number-formatting utilities.
Changes:
- Added
sanitizeCurrencyCode()to validate (and fallback) currency codes before formatting. - Wrapped
Intl.NumberFormatusage inNumberFormatUtilswith aRangeErrorcatch + USD retry. - Sanitized currency prior to a direct
formatToPartsusage inTotalCell.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| src/libs/NumberFormatUtils/index.ts | Adds try/catch fallback around Intl.NumberFormat formatting/parts generation and logs warnings on malformed currency input. |
| src/libs/CurrencyUtils.ts | Introduces sanitizeCurrencyCode and applies it to currency-formatting helpers to prevent Intl.NumberFormat crashes. |
| src/components/TransactionItemRow/DataCells/TotalCell.tsx | Sanitizes currency before calling formatToParts directly to avoid crashes in the transaction total cell. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…currency - Use relative './Log' / '@libs/Log' instead of '@src/libs/Log' to satisfy @dword-design/import-alias/prefer-alias rule for files inside src/libs. - Extract isValidCurrencyCode helper from sanitizeCurrencyCode in CurrencyUtils.ts and export it for callers that need to gate behavior on currency validity rather than transparently fall back to USD. - Use isValidCurrencyCode in Formula.formatAmount to keep the original semantic of returning an empty string (which surfaces the formula placeholder upstream) when the source or display currency is malformed, instead of silently formatting as USD. Restores the 'invalid source currency - should return placeholder' Jest expectation in FormulaTest that previously relied on Intl.NumberFormat throwing. Co-authored-by: Cursor <cursoragent@cursor.com>
- Normalize whitespace + case before validating in sanitizeCurrencyCode so common backend malformations like 'eur' and ' USD ' display the intended currency instead of incorrectly falling back to USD. - Throttle the invalid-currency Log.warn to fire at most once per unique malformed value using a module-level Set, avoiding log spam on hot paths like getLocalizedCurrencySymbol. - Loosen sanitizeCurrencyCode/isValidCurrencyCode parameter typing to unknown so non-string inputs from JS callers are handled safely without unsafe-type lint errors. - Use 'currency' in options (instead of truthiness) in the NumberFormatUtils try/catch fallback so Intl.NumberFormat throws triggered by an empty-string currency are still recovered to USD. - Add unit tests for isValidCurrencyCode, sanitizeCurrencyCode, and the malformed-currency behavior of convertToDisplayString and getLocalizedCurrencySymbol. Co-authored-by: Cursor <cursoragent@cursor.com>
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
|
🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
CurrencyUtilsTest additions:
- isValidCurrencyCode: non-string inputs (number, boolean, object, array).
- sanitizeCurrencyCode: whitespace/case normalization edge cases
(tab/newline/internal-spaces, mixed case), explicit non-string inputs,
log throttling (warn fires once per unique malformed value), and
no-warn assertion when normalization recovers a valid code.
- convertToShortDisplayString, convertAmountToDisplayString, and
convertToDisplayStringWithoutCurrency now have malformed-currency
fallback coverage matching convertToDisplayString.
- convertToDisplayString: covers the shouldUseLocalCurrencySymbol branch
with a malformed currency, and the undefined currency default path.
New NumberFormatUtilsTest covers the try/catch safety net directly:
- format/formatToParts succeed for valid currencies without warning.
- format/formatToParts fall back to USD without throwing for malformed
currencies (including the empty-string case via the 'currency' in
options guard).
- format/formatToParts do not swallow RangeErrors when the failing
option is not currency (e.g. an unknown unit), preserving normal
error propagation for unrelated bugs.
- Verifies Intl's native case-insensitive handling of lowercase ISO
codes ('usd') does not trigger the fallback path.
Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
Author
|
Tested in the Adhoc and I can supportal to this customer account and access the page, they have one unreported expense which looks like has no currency set |
melvin-bot
Bot
requested review from
FitseTLT and
trjExpensify
and removed request for
a team
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
- Extract createFormatter helper in NumberFormatUtils/index.ts so format and formatToParts share a single try/catch + USD fallback path (addresses the CONSISTENCY-3 bot review on discussion_r3274381328). - Apply sanitizeCurrencyCode to the two remaining direct callers that bypassed layer-1 validation: the convertToDisplayString and convertToDisplayStringWithoutCurrency closures in CurrencyListContextProvider, and the formatToParts call in SearchChartView. These previously only had the anonymous layer-2 catch as a safety net and missed normalization (e.g. 'eur' rendering as USD). - Add resetInvalidCurrencyWarningsForTesting in CurrencyUtils.ts so Log.warn-asserting tests can clear the module-level throttle Set in beforeEach instead of relying on globally-unique fixture strings. - Add a cross-helper throttle test asserting the shared Set deduplicates warnings across convertToDisplayString, getLocalizedCurrencySymbol, and convertToShortDisplayString for the same malformed code. - Add malformed-currency test matrix for convertToDisplayStringWithExplicitCurrency, including the falsy branch that delegates to convertToDisplayStringWithoutCurrency. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
|
Is this ready for review? |
Contributor
Author
|
@FitseTLT Yes, can you work on the checklist please? thank you |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f88c5d14c
ℹ️ 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".
Contributor
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb Chrome2026-05-21.00-05-07.mp4iOS: HybridApp2026-05-21.00-17-11.mp4iOS: mWeb Safari2026-05-21.00-02-26.mp4MacOS: Chrome / Safari2026-05-20.23-58-10.mp4 |
FitseTLT
reviewed
May 20, 2026
|
|
||
| const firstItem = data.at(0); | ||
| const currency = firstItem?.currency ?? 'USD'; | ||
| const currency = sanitizeCurrencyCode(firstItem?.currency ?? 'USD'); |
Contributor
There was a problem hiding this comment.
It would be nice to replace literal USD with const value.
| /** | ||
| * Build an Intl.NumberFormat with a USD fallback for malformed currency codes. | ||
| * Intl throws RangeError for empty/malformed currency values; check presence of the option (not truthiness) | ||
| * so we still recover when currency is '' rather than rethrowing and crashing the screen. |
Contributor
There was a problem hiding this comment.
I think you need to fix the comment around "when currency is ''"
FitseTLT
reviewed
May 20, 2026
Comment on lines
+21
to
+22
| if (e instanceof RangeError && options && 'currency' in options) { | ||
| Log.warn('NumberFormatUtils: malformed currency code, falling back to USD', {currency: options.currency}); |
Contributor
There was a problem hiding this comment.
Should we not track malformed currencies here to avoid duplicate warning?
trjExpensify
approved these changes
May 21, 2026
Contributor
trjExpensify
left a comment
trjExpensify
left a comment
There was a problem hiding this comment.
I've seen this be reported three times today. 👍
|
|
||
| const firstItem = data.at(0); | ||
| const currency = firstItem?.currency ?? 'USD'; | ||
| const currency = sanitizeCurrencyCode(firstItem?.currency ?? 'USD'); |
Contributor
There was a problem hiding this comment.
❌ CONSISTENCY-2 (docs)
The string literal 'USD' is used as a fallback currency code, but the constant CONST.CURRENCY.USD is already available and imported in this file (line 9). Using the named constant improves readability and ensures consistency if the default currency value ever changes.
Replace the magic string with the existing constant:
const currency = sanitizeCurrencyCode(firstItem?.currency ?? CONST.CURRENCY.USD);Reviewed at: 8f88c5d | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Explanation of Change
When a workspace policy or transaction has an invalid/malformed currency code stored in the backend,
Intl.NumberFormatthrows aRangeError: Malformed currency codethat crashes the entire expense amount entry screen. This has been occurring since Feb 28, 2026 — 632 occurrences across 210 users (Sentry APP-6MR).Crash path:
Two-layer fix:
sanitizeCurrencyCodeinCurrencyUtils.ts— validates the currency format (ISO 4217 = exactly 3 uppercase ASCII letters) and returnsCONST.CURRENCY.USDwith aLog.warnwhen invalid. Applied in all functions that pass a currency toIntl.NumberFormat:getLocalizedCurrencySymbol,convertToDisplayString,convertToDisplayStringWithoutCurrency,convertToShortDisplayString,convertAmountToDisplayString.Try/catch safety net in
NumberFormatUtils/index.ts— catches anyRangeErrorfromIntl.NumberFormatwith a bad currency option that slips past layer 1, retries with USD, and logs a warning. This protects all current and future callers.Also sanitized currency in
TotalCell.tsxbefore its directformatToPartscall.The backend root cause (malformed currency code stored in a policy or transaction) should be addressed separately.
Fixed Issues
$ #91113
PROPOSAL:
Tests
Verify that no errors appear in the JS console
Supportal to the account of this user https://github.com/Expensify/Expensify/issues/638330 and make sure you can access the spend page just fine
Offline tests
No offline-specific behavior changed.
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.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