fix - Report - Amount is not updated smoothly after deleting expenses from report.#68484
Conversation
|
@dukenv0307 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] |
src/libs/actions/IOU.ts
Outdated
| if (typeof updatedIOUReport.total === 'number' && currency === iouReport?.currency && canEditTotal) { | ||
| // Because of the Expense reports are stored as negative values, we add the total from the amount | ||
| const amountDiff = getAmount(transaction, true); | ||
| const amountDiff = getAmount(transaction, true) + (transactionPendingDelete?.reduce((prev, curr) => prev + getAmount(curr, true), 0) ?? 0); |
There was a problem hiding this comment.
we can precompute the transactionAmounts (id: string, amount: number) before calling prepareToCleanUpMoneyRequest and reuse it
There was a problem hiding this comment.
@dukenv0307 prepareToCleanUpMoneyRequest is handling the total updating logic inside it by considering the type of the iou report etc I don't think taking that logic out of the function is a good idea as a function should serve a single purpose if possible.
There was a problem hiding this comment.
Don't you see that you've called getAmount too many times? And you also do it in the 2 nested loops, at least you can precompute them at the begining of prepareToCleanUpMoneyRequest
|
@FitseTLT Please let me know if it's ready for review. Thanks |
|
Bump @FitseTLT |
| const hasPendingAction = useMemo(() => { | ||
| return transactions.some(getTransactionPendingAction); | ||
| }, [transactions]); | ||
| return hasPendingDeletionTransaction || transactions.some(getTransactionPendingAction); |
There was a problem hiding this comment.
Can you please explain why we need this condition? Doesn't transactions.some(getTransactionPendingAction) cover pending delete?
There was a problem hiding this comment.
@dukenv0307 we are already filtering out deleted transactions here
App/src/pages/home/ReportScreen.tsx
Line 306 in f54a2c1
There was a problem hiding this comment.
but we just need to grey out only if the total can't be recalculated, right?
There was a problem hiding this comment.
@trjExpensify is suggesting to follow the pattern we use for new expense creation and we grey it out until the BE clears the pending action currently, no matter the condition of the recalculation.
There was a problem hiding this comment.
Sorry @FitseTLT, I'm still quite confused.
Based on the confirmation here, in offline mode, we want to grey out the amount only when the total can't be recalculated.

But with the current implementation, we always grey out even though the amount is updated (IOU currency is the same as expense currency)
There was a problem hiding this comment.
@dukenv0307 I am trying to follow the current pattern we are using. On main, when adding a new expense with the same currency as the iou report it is greyed out even online until the BE response clears the pending field.
|
The amount remains greyed even when all API calls are successful Screen.Recording.2025-08-28.at.10.04.14.mov |
This is caused by another RCA from to delete transactions because we can't get the previous reportID from a deleted transacion(undefined) we save the previousTransacion value that is set hereFor our case when the first transaction is deleted transactionsToProcess will only include the first transaction although the transaction_ collection value includes the second deleted transaction set as undefined so our previousTransaction will not include the second transaction so when the second transaction is updated previousReportID will not be available and the logic to delete it from reportTransactionsAndViolations doesn't work.Solution:
I prefer (1) WDYT @dukenv0307 ? |
|
@FitseTLT Thanks for your response. Actually, the RCA is not clear to me
Why is that? Is it the issue with Onyx?
I agree with you, and I think we have 2 approaches now:
I think we should go with the first option. Please let me know what you think |
I think we should go with (1) and handle it separately. |
|
@trjExpensify What do you think: #68484 (comment)? |
This looks wrong to me. When you delete the expenses on the report while offline, they should display in the "pending delete" state from offline pattern B, meaning greyed out (50% opacity) and with strikethrough. In your video, they seem to completely disappear from the report. |
|
@FitseTLT We're filtering out the pending delete transactions, right? |
Yep It is not linked to this pr @trjExpensify After #65247 we are filtering out even pending delete traansactions offline. 😭 |
|
@TMisiukiewicz @mountiny can you weigh in here? Was this actually intentional? It goes against the established offline pattern B for pending delete actions where we show the UI element at 50% opacity with strikethrough. |
|
I don't think that was intentional. I am not super close to that PR, but maybe it was a side effect of trying to improve performance. Could you create an issue for us to follow up on there? |
|
Well I kinda' worry that is what is driving this bug, if you look at the proposals on the issue. So I think we should probably pause this PR until we iron that out. |
|
Makes sense, I believe @TMisiukiewicz will be ooo until the end of the week though |
|
Asked if someone could look into this while Tomek is ooo |
|
cc @TMisiukiewicz could you please check out the last couple of comments here? |
|
Definitely it was not intentional, however I am not much familiar with this feature so it's a bit difficult for me to help you out there |
|
Okay, are these the next steps here?
|
|
I don’t think reverting the PR makes sense, since it clearly improves performance when reading report-related transactions and violations. I had a moment to take a closer look on the issue, and I think it's not the problem of the derived value itself. Transactions with a pending delete status are still returned there. But later in App/src/pages/home/ReportScreen.tsx Lines 307 to 314 in 79f111b App/src/libs/MoneyRequestReportUtils.ts Lines 74 to 82 in 79f111b That function filters out deleted transactions entirely, including pending deletes. Possible solutions I can think of:
|
Yeah, I don't mean revert the whole PR, just unwind whatever broke the pending delete offline pattern.
I'll let @mountiny @dukenv0307 @FitseTLT chime in on which approach sounds best. |
|
I think we can go with the first solution
|
Yep will update it on this pr |
|
Applied the changes. Result: 2025-09-24.16-07-00.mp4 |
|
reviewing now |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2025-09-29.at.10.04.35.movAndroid: mWeb ChromeScreen.Recording.2025-09-29.at.10.00.06.moviOS: HybridAppScreen.Recording.2025-09-29.at.10.06.00.moviOS: mWeb SafariScreen.Recording.2025-09-29.at.09.58.36.movMacOS: Chrome / SafariScreen.Recording.2025-09-29.at.09.56.21.movMacOS: DesktopScreen.Recording.2025-09-29.at.10.07.17.mov |
|
All yours @mollfpr can you please do a final review? |
|
✋ 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/mollfpr in version: 9.2.21-0 🚀
|
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.2.21-4 🚀
|
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.2.21-4 🚀
|

Details
Fixed Issues
$ #67641
PROPOSAL: #67641 (comment)
Tests
Prerequisite: Account has at least one workspace.
Offline tests
Same as above
QA Steps
Same as above
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel so 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
2025-08-15.16-23-39.mp4
Android: mWeb Chrome
2025-08-15.17-55-31.mp4
iOS: mWeb Safari
2025-08-14.01-38-44.mp4
MacOS: Chrome / Safari
2025-08-14.01-37-29.mp4
MacOS: Desktop
2025-08-14.01-38-04.mp4