Fix distance rate not selected after creating new rate from self DM#84850
Fix distance rate not selected after creating new rate from self DM#84850aldo-expensify merged 1 commit intomainfrom
Conversation
Remove isMovingTransactionFromTrackExpense guard from the auto-rate-update effect so lastSelectedRate can restore the user's chosen rate in the self DM submit flow, matching the existing behavior in the FAB flow. Co-authored-by: Joel Davies <jjcoffee@users.noreply.github.com>
|
@MelvinBot Could you give your reasoning for why you feel it is safe to remove the |
|
Good question. Here's the full context: Why the guard was originally addedThe There are two useEffects in this component that both set the distance rate:
The guard was added to effect #2 to prevent it from competing with effect #1 during the move-from-track-expense flow. Without it, both effects would fire: #2 would blindly set Why it's safe to removeThe guard was too broad. It blocked all In that scenario:
Removing the guard is safe because effect #2 still has strong preconditions — it only fires when:
After effect #2 sets In short: the guard prevented a legitimate use case (restoring a user's newly-created rate) without adding safety, because the validation effect at line 492 already validates whatever rate ends up being set. |
|
@MelvinBot That PR didn't introduce it, it just moved existing code around. The source is #74662. |
|
Thanks for the correction — you're right, PR #82148 just reorganized the code. The guard was originally introduced in PR #74662 by Why the guard was originally added (PR #74662)The original bug: when a user is newly invited to a workspace, PR #74662 fixed this by expanding the condition to also check for From
Why it's safe to remove for issue #82629The guard prevents all
The dedicated handler at lines 492-528 doesn't help here either — it checks if Removing the guard lets the auto-rate useEffect fire during the move flow when the rate is fake/unset. This is safe because:
So the validation effect still acts as the safety net. The guard was preventing a legitimate case (restoring a user-created rate) without adding protection that isn't already provided by the validation effect. |
|
@MelvinBot Thanks! Mark this as ready for review, please. |
|
Done — PR is now marked as ready for review. 🤖 |
melvin-bot
bot
requested review from
heyjennahay
and removed request for
a team
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid-app-2026-03-12_15.06.12.mp4Android: mWeb Chromeandroid-chrome-2026-03-12_15.07.41.mp4iOS: HybridAppios-app-2026-03-12_15.01.03.mp4iOS: mWeb Safariios-safari-2026-03-12_15.03.08.mp4MacOS: Chrome / Safaridesktop-chrome-2026-03-12_14.57.37.mp4 |
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #82629 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
![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: f832719df3
ℹ️ 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".
| !isPolicyExpenseChat || | ||
| !transactionID || | ||
| !lastSelectedRate || | ||
| isMovingTransactionFromTrackExpense || | ||
| !selectedParticipants.some((participant) => participant.policyID === policy?.id) |
There was a problem hiding this comment.
By removing the isMovingTransactionFromTrackExpense guard here, this effect now runs during the move-from-track-expense flow and can overwrite the rate chosen by the earlier move-specific effect (useEffect around line 492) in the same render cycle. In cases where the moved transaction’s matched policy rate differs from lastSelectedRate (e.g., user previously used a different workspace rate), the final customUnitRateID becomes lastSelectedRate, which changes the calculated distance amount and submits the expense with the wrong rate.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@MelvinBot What do you think about this? Can you provide test steps to verify.
There was a problem hiding this comment.
The concern is technically valid but narrow in practice. Here's the full breakdown:
The Overwrite Scenario
Both effects run in the same React commit phase (declaration order). On the first render during a move-from-track-expense flow:
- Effect 1 (line 492): Sees
customUnitRateID = FAKE_P2P_ID, searches policy rates by rate+unit, finds a match (rateID_A), callssetCustomUnitRateID(rateID_A)→ writes to Onyx but re-render hasn't happened yet. - Effect 2 (line 694): Still sees
customUnitRateID = FAKE_P2P_ID(stale), so its guard passes. CallssetCustomUnitRateID(lastSelectedRate)→ overwritesrateID_AwithlastSelectedRate.
On the second render, customUnitRateID is lastSelectedRate. Effect 1 re-runs, sees it's a valid policy rate, clears errors. Stable.
When this matters: Only when lastSelectedRate differs from the rate matched by rate+unit — meaning the user previously selected a non-default rate on this policy, AND the tracked expense's P2P rate+unit matches a different policy rate. This requires a policy with multiple mileage rates.
When it doesn't matter (the common case): Most policies have one rate. lastSelectedRate falls back to defaultMileageRate, and the rate-validation effect would match the same rate by rate+unit. Both effects write the same value → no observable difference.
My Assessment
For the specific bug being fixed (issue #82629 — creating a new rate via upgrade flow in self DM), removing the guard is the right fix. The new rate IS lastSelectedRate, and the validation effect can't match it by rate+unit because it was just created.
However, for the general move-from-track-expense case with policies that have multiple rates, the overwrite could silently change the rate. A more targeted fix would be to only skip the guard when coming from the upgrade flow, or to have the auto-rate effect check whether the validation effect already handled it (e.g., via a ref).
Test Steps to Verify
Test 1: Original bug fix (must pass)
- Go to Self DM
- Create a distance expense
- Click "Submit it to someone" → enter email → select user
- On confirm page, click Rate → Upgrade → Got it, thanks
- Enter a new rate and click Save
- Verify: The new rate is selected on the confirm page (not the previous default)
Test 2: Move-from-track with single-rate policy (regression — should still work)
- Go to Self DM
- Create a distance expense (uses P2P rate)
- Click "Submit it to someone" → select a user on a workspace with ONE mileage rate
- Verify: The policy's rate is correctly applied on the confirm page
- Verify: No
invalidRateerror appears
Test 3: Move-from-track with multi-rate policy (the edge case to watch)
- Set up a workspace with two or more custom mileage rates (e.g., "Standard" at $0.67/mi and "Premium" at $1.00/mi)
- Create a distance expense from FAB on that workspace, select the "Premium" rate, and submit it (this stores "Premium" as
lastSelectedRatefor that policy) - Go to Self DM
- Create a new distance expense (it gets the P2P rate, which likely matches "Standard" by rate+unit)
- Click "Submit it to someone" → select a user on the same workspace
- Check: Which rate is applied on the confirm page?
- With the guard removed (this PR):
lastSelectedRatewins → "Premium" rate is applied - With the guard present (old behavior): Rate-validation effect matches by rate+unit → "Standard" rate is applied
- With the guard removed (this PR):
- Question for product: Which behavior is correct? Should it match by rate+unit (preserving the tracked expense's economics) or use the user's last-selected rate?
Test 3 is the key one to decide whether this approach is acceptable or needs a more targeted condition.
There was a problem hiding this comment.
@MelvinBot For Test 3 there is a P2P rate (which is not the same as the default rate on any workspace), this rate is maintained on the Submit it to someone page but shows with a Rate not valid for this workspace... error (as it's a P2P rate).
If you instead chose to upgrade when creating the expense on the self-DM, this just creates a new workspace each time from what I can tell, so there wouldn't be any risk of multiple rates there.
There was a problem hiding this comment.
You're right on both points, and this effectively resolves my Test 3 concern.
On the P2P rate: I verified in the code that the P2P rate for USD is hardcoded at 72.5 cents/mi (CONST.CURRENCY_TO_DEFAULT_MILEAGE_RATE at src/CONST/index.ts:6974), which doesn't match typical workspace mileage rates. So the validation effect's rate+unit match at line 508 fails, and it falls through to setFormError('iou.error.invalidRate'). The scenario I was concerned about — effect 1 successfully matching a policy rate that effect 2 then overwrites — doesn't actually happen because effect 1 never successfully matches.
On the upgrade flow: Since upgrading from self-DM creates a fresh workspace with a single rate, there's no multi-rate conflict. lastSelectedRate is the only rate on that workspace.
So the guard removal is safe: in the move-from-track-expense case, the validation effect doesn't match any rate (it shows an error), and with the guard removed, the auto-rate effect can set lastSelectedRate which the validation effect then re-checks. In the upgrade flow (the actual bug), lastSelectedRate is the newly created rate and gets correctly restored. Both paths work correctly.
My Test 3 concern was based on an incorrect assumption that the P2P rate would match a workspace rate by rate+unit. Thanks for the correction.
|
@aldo-expensify All yours, this doesn't seem to be an issue. |
|
🚧 @aldo-expensify 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! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/aldo-expensify in version: 9.3.41-0 🚀
Bundle Size Analysis (Sentry): |
|
🚀 Deployed to production by https://github.com/cristipaval in version: 9.3.41-4 🚀
|
4 participants
Explanation of Change
When creating a distance expense from a self DM and submitting it to someone, the user can go through an upgrade flow to create a new custom rate. After saving the new rate and returning to the confirmation page, the newly created rate is not selected — the previous default rate persists instead.
This happens because of a race condition between two
useEffecthooks inMoneyRequestConfirmationList.tsx:lastSelectedRate— but it was blocked for the self DM submit flow by anisMovingTransactionFromTrackExpenseguard.The fix removes the
isMovingTransactionFromTrackExpenseguard from the auto-rate-update effect so thatlastSelectedRatecan restore the user's chosen rate in the self DM submit flow, matching the existing behavior in the FAB flow.Fixed Issues
$ #82629
PROPOSAL: #82629 (comment)
Tests
Offline tests
N/A — This fix only affects rate selection logic during an upgrade flow that requires network connectivity.
QA Steps
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