Add frontend validation for distance expense amounts exceeding backend limit#82971
Add frontend validation for distance expense amounts exceeding backend limit#82971
Conversation
This comment has been minimized.
This comment has been minimized.
…d limit Extreme odometer/distance values can produce amounts exceeding the backend WAF's 12-digit limit (999,999,999,999 cents). When this happens, the WAF silently strips the amount parameter, causing null amount exceptions during transaction merge. This adds client-side validation to all distance expense entry points: - Odometer page (IOURequestStepDistanceOdometer) - Manual distance page (IOURequestStepDistanceManual) - Confirmation list (MoneyRequestConfirmationList) for map/GPS/all types Also adds a new isDistanceAmountWithinLimit utility in DistanceRequestUtils and localized error messages in all 10 language files.
Changes single-quoted string with escaped apostrophe to double-quoted string in the distanceAmountTooLarge translation, matching Prettier's formatting rules. Co-authored-by: Neil Marcellini <neil-marcellini@users.noreply.github.com>
7504140 to
b21e2a5
Compare
|
Fixed the two failing checks:
|
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.
|
|
The previously failing checks ( All completed checks on the current run are green:
The remaining pending checks are platform builds and perf-tests, which are still running but are infrastructure-level and unrelated to the earlier failures. |
|
@MelvinBot can you pls cite the key points of our philosophy on form validation. Does the UX you build here conform to that? Explain why and if not, update the code so that it does. |
Form Validation Philosophy — Key PointsThe Expensify form validation philosophy is documented in
Assessment of This PRThe distance expense pages ( What conforms ✅
What does NOT fully conform
|
|
@MelvinBot there's a gap in this PR. I tested out the update flow and encountered a problem. If I take a manual distance expense with a low distance and rate value, then update the distance to a very large number, then update to a large rate like $10 / mile, the update is made with no validation error preventing it. Please fix the update flows for distance and rate to prevent this. |
When editing the rate on an existing distance expense, validate that the new rate combined with the existing distance does not exceed the backend WAF's 12-digit limit (999,999,999,999 cents). Shows the same distanceAmountTooLarge error message used in the creation flow. Co-authored-by: Neil Marcellini <neil-marcellini@users.noreply.github.com>
|
Good catch — the rate edit flow was missing validation entirely. Here's what I found and fixed: The gap:
The fix (commit af8f3cd): Added validation in Note on the waypoint/map edit flow: ESLint passes on the changed file. Full |
Co-authored-by: Neil Marcellini <neil-marcellini@users.noreply.github.com>
|
Fixed the failing |
|
@MelvinBot thanks for that. The UX is still a little bit off unfortunately. When you select a new rate that causes the amount to be over the limit the error message displays, but then the previous rate (the one that is acceptable) shows as selected, so it's a confusing situation. Instead I think it should work like this. When you select the problematic rate it displays the error message, leaves you on that page, and leaves the problematic rate selected. When you select a valid rate it should clear the error message and navigate you back to show the updated expense page. If you leave the error message on the page without fixing it and hit the back button, the expense page should show the previously selected rate that is stored on the server. Pls make those changes. 2026-02-20_16-07-26.mp4 |
…idation error When selecting a rate that causes the distance amount to exceed the backend limit, the page now keeps the problematic rate shown as selected (via local pendingRateID state) instead of reverting to the previously stored rate. This makes the error context clear to the user. Selecting a valid rate clears the error and navigates back normally. Hitting back without fixing leaves the server-stored rate unchanged. Also moved the validation check before tax state updates so that an invalid rate selection does not produce side effects on tax amount/code. Co-authored-by: Neil Marcellini <neil-marcellini@users.noreply.github.com>
|
Pushed the fix in 867d5f1. Here's what changed in Problem: When a user selected a rate that would exceed the amount limit, the error message appeared but the radio button reverted to showing the previous (valid) rate as selected — confusing because the error says the amount is too large but the selected rate looks fine. Fix — 3 changes:
Behavior now matches the requested UX:
|
|
@MelvinBot cool thanks Melv. The flow on the error page seems to be working well. I did notice another thing, when updating the rate or distance of and existing expense, the error message instructs you to change the distance or rate, but if you're on the distance page it doesn't make sense to mention changing the rate in that error message, and visa-versa on the rate page. pls update. |
On distance pages (odometer/manual), show "Reduce the distance" only. On the rate page, show "Lower the rate" only. The confirmation page keeps the general message mentioning both. Co-authored-by: Neil Marcellini <neil-marcellini@users.noreply.github.com>
|
Updated the error messages to be context-specific:
All 10 language files have been updated with the corresponding translations. See commit a0d0779. |
…Validation Co-authored-by: Neil Marcellini <neil-marcellini@users.noreply.github.com>
|
Fixed the failing Root cause: The branch was based on an older version of Fix: Merged current |
|
@MelvinBot I noticed when testing distance updates for odometer expenses that the input has no max value. Can we please make the max value match the manual distance max value? Or a reasonable max value for a car's odometer? I think they probably don't go over 999,999 so that seems like a good limit. |
The odometer start/end TextInput fields had no character length limit, allowing users to enter unreasonably large values. Add CONST.IOU.ODOMETER_MAX_LENGTH (10 chars, supporting up to 9999999.99) and apply it to both odometer TextInput fields. Co-authored-by: Neil Marcellini <neil-marcellini@users.noreply.github.com>
|
Added a What changed:
This follows the same pattern used in Pushed in commit 95d35f9. |
maxLength alone is a character count limit, not a numeric limit. 9999999.99 is 10 characters but far exceeds 999,999. Add an explicit numeric check against ODOMETER_MAX_VALUE (999,999) in handleNext() and reduce ODOMETER_MAX_LENGTH from 10 to 9 as a UX guardrail. Co-authored-by: Cursor <cursoragent@cursor.com>
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Cursor <cursoragent@cursor.com>
…smatch Co-authored-by: Neil Marcellini <neil-marcellini@users.noreply.github.com>
isOdometerInputValid and parseOdometerReading previously used different stripping logic — the validator stripped all non-numeric characters while the parser only stripped commas. This meant pasted text like "1 234.5" would pass validation but parseFloat would return 1 instead of 1234.5. Extract a single stripOdometerText function that both use, so the stripping logic can never diverge. Co-authored-by: Neil Marcellini <neil-marcellini@users.noreply.github.com>
Co-authored-by: Neil Marcellini <neil-marcellini@users.noreply.github.com>
This comment was marked as off-topic.
This comment was marked as off-topic.
|
All code-related CI checks are now passing (ESLint, Prettier, typecheck, all 8 test jobs, Storybook, perf-tests, spellcheck, verifySignedCommits, etc.). The only failing check is |
|
The only failing check is The reviewer checklist check will pass once a reviewer copies and posts the reviewer checklist template as a comment on this PR. |
|
@codex pls review again |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 97dc8d2dcd
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@MelvinBot pls fix the latest review comments |
…y over-max odometer values Use the selected rate's unit (not the transaction's stale stored unit) when validating that distance × rate stays within the backend limit. Also allow users to edit down pre-populated odometer readings that already exceed the max value, while still preventing new input that increases beyond the limit. Co-authored-by: Neil Marcellini <neil-marcellini@users.noreply.github.com>
…odometer values above max Co-authored-by: Neil Marcellini <neil-marcellini@users.noreply.github.com>
|
The only failing check is All code-related checks (ESLint, Prettier, builds, typecheck, CLA, verifySignedCommits) are passing. Tests, spellcheck, and a few other checks are still pending but none have failed. |
Co-authored-by: Neil Marcellini <neil-marcellini@users.noreply.github.com>
|
The only failing check is the PR Reviewer Checklist — it fails because no reviewer has posted the reviewer checklist comment yet. This is a process check that requires a human reviewer to complete, not a code issue. All code-related checks that have completed are passing: ESLint, Prettier, typecheck, all builds, spellcheck, generateTranslations, unused styles check, verifySignedCommits, and Snyk security/license scans. The remaining pending checks (tests, perf-tests, Changed files ESLint, Storybook, dryRun) are still running but there are no prior code failures on this branch. |
|
The only failing check is the PR Reviewer Checklist ( All code-related checks are passing: ESLint, typecheck, Prettier, builds (Android/iOS/desktop), tests, spellcheck, CLA, and verifySignedCommits. |
|
@codex pls review again |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c8b6656af
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/libs/DistanceRequestUtils.ts
Outdated
| * @returns true if the amount is within limits, false if it would exceed the backend limit | ||
| */ | ||
| function isDistanceAmountWithinLimit(distance: number, rate: number): boolean { | ||
| const amount = Math.abs(Math.round(distance * rate)); |
There was a problem hiding this comment.
Align limit check with rounded distance amount math
isDistanceAmountWithinLimit() computes Math.round(distance * rate) on the raw distance, but distance amounts are actually derived via getDistanceRequestAmount(), which rounds converted distance to 2 decimals before multiplying. In the rate-change flow (IOURequestStepDistanceRate), unit conversion produces distances with more than 2 decimals, so this mismatch creates boundary off-by-one cases where we can either pass an over-limit amount (still tripping the backend/WAF) or reject a valid one near MAX_SAFE_AMOUNT; the limit check should mirror the same 2-decimal rounding path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid catch. isDistanceAmountWithinLimit was doing Math.round(distance * rate) directly, while getDistanceRequestAmount first rounds the converted distance to 2 decimal places (toFixed(2)) before multiplying by rate. This mismatch meant unit-converted distances with more than 2 decimals could produce off-by-one boundary results — either letting an over-limit amount through or rejecting a valid one near MAX_SAFE_AMOUNT.
Fixed by adding the same 2-decimal rounding step before multiplication, so the limit check now mirrors the actual amount computation path. Pushed in b6d8e10.
The limit check was using Math.round(distance * rate) directly, but getDistanceRequestAmount rounds the converted distance to 2 decimal places before multiplying by rate. This mismatch could produce off-by-one boundary cases where the limit check passes but the actual amount exceeds MAX_SAFE_AMOUNT, or vice versa. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Fixed the Codex P1 review comment about Added |
…stAmount Round distance to 2 decimal places before multiplying by rate in the limit check, matching the rounding path in getDistanceRequestAmount(). Without this, unit-converted distances with more than 2 decimals could produce off-by-one boundary results — either passing an over-limit amount or rejecting a valid one near MAX_SAFE_AMOUNT. Co-authored-by: Neil Marcellini <neil-marcellini@users.noreply.github.com>
Both local and remote fixed the same 2-decimal rounding issue. Resolved by keeping Math.abs on distance input and the descriptive comment from the remote version. Co-authored-by: Neil Marcellini <neil-marcellini@users.noreply.github.com>
|
The only failing check is PR Reviewer Checklist ( All code-related checks that have completed are passing: ESLint, Prettier, typecheck, spellcheck, builds (all 4), Storybook tests, generateTranslations, verifySignedCommits, CLA, and dryRun. The test jobs (1-8) and perf-tests are still pending/queued (waiting for CI runners), but there are no code-related failures to fix. |
|
The only failing check is PR Reviewer Checklist ( |
|
(Neil's AI agent) Closing this PR in favor of #83170, which contains the identical final code squashed into a single clean commit. The duplicate MelvinBot dispatches (tracked in https://github.com/Expensify/Expensify/issues/602760) caused repeated duplicate commits and merge conflicts on this branch, making the commit history difficult to review. NeilAll that is correct except I'm not squashing to one commit, just removing duplicates. Melvin went a bit off the rails on this one with the duplicates. I'll report in Slack. |
Explanation of Change
Extreme odometer or manual distance values can produce expense amounts that exceed the backend WAF's 12-digit limit (999,999,999,999 cents). When this happens, the WAF silently strips the
amountparameter from the API request, causing anull amountexception duringTransaction_Merge(ExpException atapi.php:1686).This PR adds client-side validation to prevent users from submitting distance expenses with amounts that would exceed the backend limit.
Amount validation (distance × rate):
MAX_SAFE_AMOUNT: 999999999999constant toCONST.IOUmatching the backend WAF regex limitisDistanceAmountWithinLimit(distance, rate)utility inDistanceRequestUtilsOdometer reading limits:
ODOMETER_MAX_VALUE: 9999999.9— max reading of 9,999,999.9 to support commercial vehiclesLocale-aware input parsing:
replaceAllDigits+fromLocaleDigitso European formats (e.g., German1.234.567,9) are handled correctlynumberFormatto display the max value in the user's localeRate edit UX:
pendingRateIDstate)Localization:
odometerReadingTooLargeuses a function with locale-formatted max value instead of hardcoded numbersFixed Issues
$ https://github.com/Expensify/Expensify/issues/601062
Tests
Create flow
2026-02-19_17-53-02.mp4
Update flow
2026-02-20_18-16-11.mp4
2026-02-20_18-20-48.mp4
Test with odometer expenses too
2026-02-22_08-49-44.mp4
Offline tests
No offline-specific behavior changes — validation is purely client-side before any API call.
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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
No platform specific changes. See the videos in the tests section.
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari