Hide redundant Reimbursable rows; show "· Non-reimbursable" on Amount#90411
Conversation
… + add related tests
|
@hoangzinh 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] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef86b025e0
ℹ️ 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".
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
This happens when a report contains reimbursable expenses and reimbursable credits (negative amounts) that cancel each other out. For example: a $50 reimbursable expense and a -$50 reimbursable credit on the same report → the reimbursable total is $0. The reason this case matters is that The test on line 128 of |
|
@shawnborton Coming from your request, when user open a non-reimbursable expense in a money report (has multiple expenses), should we display the “Amount · Non-reimbursable”
|
|
I could go either way. I was thinking we'd only need this in the single expense report view for simplicity's sake but I can see an argument to use it everywhere. Any strong feelings @flaviadefaria @heyjennahay @Expensify/design @JmillsExpensify ? |
|
I’d only add it to the single expense report view, since the goal of that information was to clean up the “report reimbursable/non-reimbursable” totals, which doesn’t really apply to expenses within multi-expense reports. I’d rather stick to the original scope to avoid unnecessary repetition, as highlighted in @hoangzinh’s screenshot. |
|
That works for me! |
|
@hoangzinh can you pls check it once more? thanks |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-05-18.at.17.02.43.movAndroid: mWeb ChromeScreen.Recording.2026-05-18.at.17.01.26.moviOS: HybridAppiOS: mWeb SafariScreen.Recording.2026-05-18.at.17.08.33.movMacOS: Chrome / SafariScreen.Recording.2026-05-18.at.16.58.06.mov |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 @rlinoz 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/rlinoz in version: 9.3.76-0 🚀
|
|
No help site changes are required for this PR. The changes are a UI-only refinement to how single non-reimbursable expense reports display their total breakdown — hiding redundant "Reimbursable" / "Non-reimbursable" rows and appending "· Non-reimbursable" inline on the Amount label instead. No user-facing workflows, feature names, settings, or documented processes are affected. The existing help site articles that mention reimbursable/non-reimbursable expenses cover workspace rules, export configurations, and payment methods — none describe the report total breakdown UI. |
|
Deploy Blocker #91057 was identified to be related to this PR. |
|
Deploy Blocker #91058 was identified to be related to this PR. |
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.3.77-3 🚀
|
|
This wasn't implemented correctly - we still are showing a HR/border line at the top: Can you please follow up @mukhrr ? Thanks! |
@shawnborton what are repro steps for this? I don't see it on main branch |
|
Never mind, I found them:
cc @hoangzinh |
|
Here is PR #92042 cc @hoangzinh |
|
Thanks! |
|
@shawnborton, can you help create a new GitHub issue for this bug report, please #90411 (comment)? |
|
What do you mean? I think it would just be a follow up from the original request, no? |
|
@shawnborton Here is my honest report: Root cause: This guard was lost back in #44695 ("Remove Report Fields Beta") when Why it shows up here: for a single non-reimbursable expense, Total/Billable/Tax are hidden, so Fix: add Besides, we fix/improve another thing: removing totals from above amount row in #92042. So I think it makes sense to have, at least, an issue. What do you think? cc @hoangzinh |
|
@rlinoz can I get your opinion too? |
|
I agreed with @mukhrr comment here. During the PR for original issue #89548, we thought it's only scoped for "Non-reimbursable" single expense:
|
|
Yeah sorry if that got lost in translation, when the issue said "redundant rows" it meant to include all rows above the total, not just for reimbursable. |
|
Got it. So you are asking that we create a new isssue, which means a separate $250 bounty? If we create a new issue, I suppose we would give Melvin first dibs at solving this (proposal + sending a PR) as a heads up. |
|
@shawnborton does $250 sounds good to you? For two issues we mentioned #90411 (comment) |
|
Like I said, I think we'd make the issue and then just have Melvin try to solve it right? That's how all of our new issues go. That being said, I am not on the team that manages these issues so that's more of a question for @rlinoz I think. |
|
Oh I am okay, but I have been assigned multiple times for follow-up issues like this since I had more context about what is going one. Let's see what @rlinoz says. |
|
Hey sorry for the delay, I am back from OOO. Since this is not a regression on the PR I think we should have created a new issue and gone through our normal process. For this specific case, given the amount of work/discussion that already happened in the PR I think it is fine to continue, but it is feels like the amount should be $125 for the minor layout change. |
|
@rlinoz thanks for taking a look. Then can you pls assign me and @hoangzinh in this regard? |
|
@mukhrr just to be sure, we are fixing both things #90411 (comment) in the PR right? |
|
Yes, we do |





Explanation of Change
Fixed Issues
$ #89548
PROPOSAL: #89548 (comment)
Tests
Verify you see only one header with text “Amount · Non-reimbursable” and reimbursable amount below
Verify that no errors appear in the JS console
Offline tests
QA Steps
The 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
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari