Fix: Display MoneyRequestReport as single transaction after deleting others#80107
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@ZhenjaHorbach 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] |
Code Review CompleteI've reviewed PR #80107 and found 1 potential issue that should be addressed: Issue FoundFile: CONSISTENCY-5: Outdated ESLint Disable Justification The ESLint disable comment states "We don't want this hook to re-run on the every report change", but after adding Recommendation: Update the comment to accurately reflect the new behavior, such as: "We limit dependencies to avoid unnecessary re-runs, but include oneTransactionID to handle transaction deletions" Overall Assessment: |
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp2026-01-21.17.19.01.movAndroid: mWeb Chrome2026-01-21.17.20.15.moviOS: HybridApp2026-01-21.17.19.01.moviOS: mWeb Safari2026-01-21.17.20.15.movMacOS: Chrome / Safari2026-01-21.17.13.27.mov |
|
LGTM! |
|
@collectioneur Won't this change cause openReport to be fetched even more frequently (exactly the same one)? |
|
@LukasMod Yes, unfortunately, it will trigger every time transactions are added or removed. The key point here is that the report needs to be fetched when the second-to-last transaction is deleted, right after oneTransactionID changes. I haven't figured out a way to reduce the number of fetches yet. We could add |
|
@collectioneur Not really. Yesterday, I worked on a way to reduce duplicated requests on the Reports tab when we open report, but after some testing I realized that these requests are not always duplicates and that there is a high risk of regression (openReport uses multiple onyx.connect calls internally). Context: #77173 (comment) The overall idea was to keep openReport calls to a minimum, as there are many Onyx actions involved and they will trigger rerender cascades. I just wanted to point out that with this change there is a higher risk of such request duplicates, but I don’t know a simple way to determine when a call is redundant and when it isn’t. Small guard would be always something 👍 |
|
🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
mountiny
left a comment
There was a problem hiding this comment.
Please add a comment to the dependency array
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
@mountiny added a comment with an explanation |
|
✋ 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/mountiny in version: 9.3.7-0 🚀
|
|
Some deploy blockers found, testing on the revert |
Looks like the original issue is worse than the regressions |
|
@mountiny I’ll try to fix all the regressions today, but I think we should revert, as the bugs are quite serious |
|
Oh, sorry. I didn't see that QA had already verified the revert. In that case, a revert isn't needed since the bugs aren't related to this PR 😄 |
|
🚀 Deployed to production by https://github.com/Beamanator in version: 9.3.7-3 🚀
|
|
Correct sorry for not updating here! |
Explanation of Change
Previously, if multiple transactions were added to a report and then all but one were deleted without being opened, MoneyRequestReportPage failed to display the remaining transaction. This happened because oneTransactionID still referenced a deleted transaction ID while the deletion was in progress.
I've added oneTransactionID to the useEffect dependency array. This ensures that when transactions are deleted, the effect re-runs to correctly create a transactionThread for the final remaining transaction.
Fixed Issues
$ #79732
Tests
Offline tests
QA Steps
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
Screen.Recording.2026-01-21.at.13.55.15.mov