Fix tax amount input overwriting in-progress edits in new manual expense flow#94538
Conversation
…nse flow Co-authored-by: thelullabyy <thelullabyy@users.noreply.github.com>
|
@MelvinBot You are using the wrong PR description template. Please use this one https://raw.githubusercontent.com/Expensify/App/main/.github/PULL_REQUEST_TEMPLATE.md After that replace all Finally, open PR for review |
|
@codex review |
|
@thelullabyy Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
🤖 Done,
|
| // tax rate changed and the amount was recalculated externally). | ||
| const currentInput = numberFormRef.current?.getNumber(); | ||
| if (currentInput !== undefined) { | ||
| const currentBackendAmount = currentInput.trim() === '' ? 0 : convertToBackendAmount(Number.parseFloat(currentInput)); |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
This expression currentInput.trim() === '' ? 0 : convertToBackendAmount(Number.parseFloat(currentInput)) duplicates the exact string-to-backend-amount conversion already used in handleTaxAmountChange (newAmount.trim() === '' ? 0 : convertToBackendAmount(Number.parseFloat(newAmount))). Both apply the same empty-string-to-zero guard before converting. Consolidating keeps the parsing rules in one place so the two sites can't drift apart.
Extract a small local helper and call it in both spots:
const toBackendTaxAmount = (input: string) => (input.trim() === '' ? 0 : convertToBackendAmount(Number.parseFloat(input)));
// in handleTaxAmountChange
const taxAmountInSmallestCurrencyUnits = toBackendTaxAmount(newAmount);
// in the effect
const currentBackendAmount = toBackendTaxAmount(currentInput);Reviewed at: e29f18f | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Good catch — addressed in 9750f66. Extracted a local toBackendTaxAmount helper and used it in both handleTaxAmountChange and the sync effect so the empty-string-to-zero guard and conversion live in one place.
|
@MelvinBot Please check this comment #94538 (comment) |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-06-26.at.00.38.26.movScreen.Recording.2026-06-26.at.00.59.02.mov |
Co-authored-by: thelullabyy <thelullabyy@users.noreply.github.com>
|
@puneetlath Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
trjExpensify
left a comment
There was a problem hiding this comment.
Another one bites the dust! 👍
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 puneetlath has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/puneetlath in version: 9.4.21-2 🚀
|
Explanation of Change
The inline tax amount input in the new manual expense flow re-added padding digits (
.00) while the user was typing, because a force-syncuseEffectinTaxFields.tsxoverwrote the field on every keystroke.The effect's guard compared raw strings: the in-progress text returned by
getNumber()(e.g."5.0") was compared against the re-paddedtaxAmountInput("5.00"). Because the strings differ, the guard failed andupdateNumber("5.00")overwrote the user's edit. This produced all three reported symptoms:"5.00"→"5.0"got re-padded back to"5.00"(BUG 1/2).10stored1000, which got re-padded to"10.00"(BUG 3).The fix changes the guard to compare the numeric value instead of the formatted string. The current input is parsed to its backend amount (treating an empty field as
0) and compared against the storedtaxAmount. An in-progress edit like"5.0"(≡"5.00"≡ backend500) is now recognized as already in-sync and is left untouched. The field is only refreshed viaupdateNumberwhen the stored tax amount genuinely differs — e.g. when the tax rate is changed and the amount is recalculated externally.Fixed Issues
$ #94264
PROPOSAL: #94264 (comment)
Tests
Requires the new manual expense flow + Taxes enabled on the workspace.
+→ Create expense → Manual.0(BUG 1).00(BUG 2).10..00is not auto-appended (BUG 3).Offline tests
Same as tests.
QA Steps
Requires the new manual expense flow + Taxes enabled on the workspace.
+→ Create expense → Manual.0(BUG 1).00(BUG 2).10..00is not auto-appended (BUG 3).PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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.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