Skip to content

Fix distance preview mismatch with expense details header - #101068

Open
nabi-ebrahimi wants to merge 47 commits into
Expensify:mainfrom
nabi-ebrahimi:fix-distance-preview-header-mismatch-100188
Open

nabi-ebrahimi wants to merge 47 commits into
Expensify:mainfrom
nabi-ebrahimi:fix-distance-preview-header-mismatch-100188

Conversation

@nabi-ebrahimi

@nabi-ebrahimi nabi-ebrahimi commented Sep 14, 2026 •

Copy link
Copy Markdown
Contributor

Explanation of Change

This PR fixes the distance expense preview mismatch after a distance expense with commuter exclusion is removed from a report and shown in the self-DM.
The transaction preview now uses the same display normalization logic as the expense details/header flow. For distance requests, it resolves the original policy for the selected distance rate when needed, then passes a display-safe transaction into getTransactionDetails().
This prevents the preview card from showing commuter-exclusion-adjusted amount/distance values when the expense is no longer in a policy expense chat, while preserving the existing commuter exclusion behavior for real workspace expenses.

Fixed Issues

$#100188
PROPOSAL:

Tests

  1. Open the app and sign in.
  2. Go to a workspace with distance rates enabled.
  3. Enable commuter exclusion for the workspace distance rate.
  4. Create a distance expense using that workspace distance rate.
  5. Submit the distance expense to the workspace report.
  6. Open the expense details and confirm the header, Amount row, and Distance row are consistent.
  7. Remove the expense from the report.
  8. Go to the self-DM/personal space where the tracked expense appears.
  9. Verify the expense preview card amount/distance matches the expense details header.
  10. Open the expense details from the preview.
  11. Verify the header, Amount row, Distance row, and preview card all show consistent full-route values.
  • Verify that no errors appear in the JS console

Offline tests

  1. Complete QA steps 1-6 while online.
  2. After the distance expense is created and the values are visible, go offline.
  3. Remove the expense from the report while offline.
  4. Go to the self-DM/personal space where the tracked expense appears.
  5. Verify the expense preview card amount/distance matches the expense details header.
  6. Open the expense details from the preview.
  7. Verify the header, Amount row, Distance row, and preview card all show consistent full-route values.
  8. Go back online and verify the values remain consistent after sync.

QA Steps

// TODO: These must be filled out, or the issue title must include "[No QA]."
Same as tests

  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android: Native
    • Android: mWeb Chrome
    • iOS: Native
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
  • If new assets were added or existing ones were modified, I verified that:
    • The assets are optimized and compressed (for SVG files, run npm run compress-svg)
    • The assets load correctly across all supported platforms.
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
    • If the PR adds or modifies the UI:
  • I asked an AI agent to review the changes for accessibility issues and addressed its findings.
  • I tested with a screen reader (VoiceOver on macOS) and verified all new/changed elements are reachable with a logical focus order.
  • I verified all new/changed elements have meaningful accessible names and roles.
  • I verified state changes are announced (e.g. checked/unchecked, expanded/collapsed, selected).
  • I added unit tests for any new feature or bug fix in this PR to help automatically prevent regressions in this user flow.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Screencast.From.2026-09-14.14-00-21.mp4

@nabi-ebrahimi
nabi-ebrahimi marked this pull request as ready for review September 14, 2026 10:18
@nabi-ebrahimi
nabi-ebrahimi requested review from a team as code owners September 14, 2026 10:18
@melvin-bot
melvin-bot Bot requested review from flaviadefaria and marufsharifi and removed request for a team September 14, 2026 10:18
@melvin-bot

melvin-bot Bot commented Sep 14, 2026

Copy link
Copy Markdown

@marufsharifi 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]

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 526afbe6da

ℹ️ 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".

Comment thread src/components/ReportActionItem/TransactionPreview/TransactionPreviewContent.tsx Outdated

@marufsharifi marufsharifi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No regression test added for the normalization this PR introduces.

The diff touches only these 2 source files, yet the author checklist marks "I added unit tests." tests/ui/TransactionPreviewContentTest.tsx already exists and is the natural home for:

  • a distance expense w/ commuter exclusion, once removed from its policy-expense chat (shown in self-DM), renders full-route amount/distance in the preview and matches the details header; and
  • a distance expense still in its policy-expense chat keeps the commuter-adjusted values (normalization must NOT apply).

For a subtle amount-normalization fix, this assertion is what stops the bug from silently returning. Test coverage of the diff is currently 0%.

Comment thread src/components/ReportActionItem/TransactionPreview/index.tsx Outdated
Comment thread src/components/ReportActionItem/TransactionPreview/index.tsx Outdated
Comment thread src/components/ReportActionItem/TransactionPreview/TransactionPreviewContent.tsx Outdated
const transactionDetails = useMemo<Partial<TransactionDetails>>(
() => getTransactionDetails(transaction, undefined, policy, isParentPolicyExpenseChat) ?? {},
[transaction, policy, isParentPolicyExpenseChat],
() => getTransactionDetails(displayTransaction, undefined, policy, isParentPolicyExpenseChat) ?? {},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ Same allowNegativeAmount concern as index.tsx:90 (pre-existing here).

getTransactionDetails(displayTransaction, undefined, policy, isParentPolicyExpenseChat) passes the policy-expense-chat flag into the allowNegativeAmount slot (ReportUtils.ts:5010). Only transaction → displayTransaction changed on this line, so the questionable arg is pre-existing — but since you're already touching it, either switch to a real derived allowNegative (like MoneyRequestView.tsx:338) or add a clarifying comment so it isn't "fixed" into a regression later.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This argument is pre-existing and was intentionally added in eeaa81f to support negative transaction preview values. This PR only changes the input from transaction to displayTransaction. I’m leaving the existing behavior unchanged to avoid introducing an unrelated regression; deriving allowNegativeAmount through shouldEnableNegative() can be handled separately with dedicated coverage.

Comment thread src/components/ReportActionItem/TransactionPreview/TransactionPreviewContent.tsx Outdated
Comment thread src/components/ReportActionItem/TransactionPreview/index.tsx Outdated
Comment thread src/components/ReportActionItem/TransactionPreview/TransactionPreviewContent.tsx Outdated
Comment thread src/components/ReportActionItem/TransactionPreview/index.tsx Outdated
Comment thread src/components/ReportActionItem/TransactionPreview/index.tsx Outdated
Comment thread src/components/ReportActionItem/TransactionPreview/TransactionPreviewContent.tsx Outdated
@marufsharifi

Copy link
Copy Markdown
Contributor

@nabi-ebrahimi, Gentle bump. thanks.

@nabi-ebrahimi

Copy link
Copy Markdown
Contributor Author

Hi,@marufsharifi ,Thank you for taking the time to review this. I’ve addressed all your feedback and thoroughly tested the changes to confirm that everything works as expected. Could you please review the latest changes again when you have a chance?

@nabi-ebrahimi

Copy link
Copy Markdown
Contributor Author

Hi @Gonals , while testing the issue, I found another related bug that also reproduces on main.

With distance rates enabled and a commuter exclusion configured:

Move/remove a distance expense from a workspace to Self DM.
Open the expense and confirm the correct distance and amount are shown.
Select More → Split.
On the Split page, the commuter exclusion is applied again, reducing both the distance and the amount.

For example, with a 2-mile commuter exclusion, 19.34 mi / TJS 19.34 becomes 17.34 mi / TJS 17.34.

I also tested this with different distance-rate values, and the same behavior occurs.

Should this be fixed as part of the current issue/PR, or would you prefer to track it in a separate issue?

Screencast.From.2026-09-25.11-47-38.mp4

@marufsharifi

Copy link
Copy Markdown
Contributor

@Gonals, could you please take a look at #101068 (comment) when you get a chance. thanks.

@marufsharifi

Copy link
Copy Markdown
Contributor

@Gonals, Gentle bump #101068 (comment). thanks.

@Gonals

Gonals commented Oct 5, 2026

Copy link
Copy Markdown
Contributor

Should this be fixed as part of the current issue/PR, or would you prefer to track it in a separate issue?

Let's create a different issue for it

@nabi-ebrahimi

Copy link
Copy Markdown
Contributor Author

@marufsharifi, gentle bump. Thank you!

@nabi-ebrahimi

Copy link
Copy Markdown
Contributor Author

Hi @marufsharifi, I’ve addressed all of your feedback. Could you please review the latest changes when you have a chance? Thanks!

@nabi-ebrahimi

Copy link
Copy Markdown
Contributor Author

Hi @marufsharifi, during testing, I found and addressed two issues: changing the workspace distance rate could affect an existing expense’s amount in Self DM, and switching the app language could also change that amount. The fixes preserve the saved rate and keep the restored amount consistent, with regression tests added.

Could you please review the latest changes? Thanks.

Comment thread src/libs/TransactionUtils/index.ts Outdated
Comment thread src/hooks/useDisplayTransaction.ts Outdated
Comment thread src/hooks/useDisplayTransaction.ts Outdated
Comment thread src/libs/TransactionUtils/index.ts Outdated
Comment thread src/libs/TransactionUtils/index.ts Outdated
Comment thread src/hooks/useDisplayTransaction.ts
Comment thread src/libs/TransactionUtils/getUpdatedTransaction.ts
@nabi-ebrahimi

Copy link
Copy Markdown
Contributor Author

Hi @marufsharifi, I have addressed all of your feedback and added the requested test coverage. Could you please review the latest changes? Thank you for your guidance.

@nabi-ebrahimi

Copy link
Copy Markdown
Contributor Author

Hi @marufsharifi, during testing, I found that changing the workspace distance rate could cause an expense in Self DM to display its commuter-reduced amount again. I addressed this by validating both saved amounts against the historical rate and added regression tests. Could you please review the latest changes? Thank you.

@marufsharifi

Copy link
Copy Markdown
Contributor

BUG

Steps to reproduce:

Open the workspace distance expense created at $1.00/mile.
In the recording, its original distance is 13.53 miles.
After the one-mile deduction, it shows 12.53 miles / $12.53.
Select Report → Remove from report.
Open the expense in your personal chat.
Enable Settings → About → Troubleshoot → Force offline.
Return to the personal expense and change its rate to $2.00/mile.
Select Report and move it back to the original workspace report.
Open the expense in that report while still offline.

Expected result:

The workspace’s one-mile commuter deduction applies consistently to both distance and amount:

13.53 − 1.00 = 12.53 miles × $2.00 = $25.06

The expense header, report row, and Amount field should reflect $25.06.

Actual result:

The Distance field shows 12.53 miles and “Removed 1.00 commuter mile”, but the Amount field, expense header, and report row show $27.06.

The amount is calculated using the full 13.53 miles, despite the displayed deduction.

RF-1.mov

@nabi-ebrahimi

Copy link
Copy Markdown
Contributor Author

BUG

Steps to reproduce:

Open the workspace distance expense created at $1.00/mile. In the recording, its original distance is 13.53 miles. After the one-mile deduction, it shows 12.53 miles / $12.53. Select Report → Remove from report. Open the expense in your personal chat. Enable Settings → About → Troubleshoot → Force offline. Return to the personal expense and change its rate to $2.00/mile. Select Report and move it back to the original workspace report. Open the expense in that report while still offline.

Expected result:

The workspace’s one-mile commuter deduction applies consistently to both distance and amount:

13.53 − 1.00 = 12.53 miles × $2.00 = $25.06

The expense header, report row, and Amount field should reflect $25.06.

Actual result:

The Distance field shows 12.53 miles and “Removed 1.00 commuter mile”, but the Amount field, expense header, and report row show $27.06.

The amount is calculated using the full 13.53 miles, despite the displayed deduction.

RF-1.mov

Thanks for the feedback, @marufsharifi . I’ve addressed it and added regression tests for the offline move and fractional rates. Could you please review it again?

Click to show video
Screencast.From.2026-10-23.12-18-33.mp4

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants