Fix infinite loading after cross-report expense merge#84540
Draft
Fix infinite loading after cross-report expense merge#84540
Conversation
When merging expenses across different reports, the target transaction's reportID was not being optimistically updated to the destination report. This caused the UI to navigate to a report where the transaction data hadn't been moved yet, triggering the skeleton guard in MoneyRequestView indefinitely. Additionally, if the source report was a single-expense report that happened to be the merge destination, it would be optimistically deleted even though the backend would keep it (with the merged expense). This caused the destination report to appear as null in Onyx. Two changes: 1. Pass newTransactionReportID to getUpdateMoneyRequestParams during merge so the target transaction's reportID is optimistically updated to the destination report. 2. Skip optimistic deletion of the source report when it is the merge destination. Co-authored-by: Situ Chandra Shil <situchan@users.noreply.github.com>
7 tasks
Contributor
|
@MelvinBot close PR. Do you know which PR fixed the original bug? |
Contributor
Author
|
Closing this PR as requested — the linked issue (#79717) is no longer reproducible (confirmed by QA on build 9.3.43-1) and was closed by Which PR fixed the original bug? There is no single PR that directly targeted #79717. The bug was most likely resolved as a side effect of one or more of these merge-related PRs that landed between when the issue was filed and when QA confirmed it was fixed:
My best guess is #72990 since it fixed the same symptom (skeleton/infinite loading after merge) in a closely related scenario. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Explanation of Change
When merging expenses across different reports (cross-report merge), the target transaction's
reportIDwas not being optimistically updated to the destination report. After the merge, the UI navigated to the destination report, but from Onyx's perspective the transaction still belonged to its original report. This causedMoneyRequestViewto find no validtransactionThreadReport, triggering its skeleton guard atMoneyRequestView.tsx:789and displaying infinite loading.Additionally, if the source report was a single-expense report that also happened to be the merge destination, it would be optimistically deleted (SET to null) even though the backend would keep it with the merged expense. This compounded the issue by making the destination report appear as null in Onyx.
This PR makes two changes to
src/libs/actions/MergeTransaction.ts:Pass
newTransactionReportIDtogetUpdateMoneyRequestParamswhen the destination report differs from the target transaction's current report. This leverages the existingnewTransactionReportIDparameter (already used in the Split flow) to optimistically update the target transaction'sreportIDto the destination report, so the derived data and component chain reflect the correct state immediately.Skip optimistic deletion of the source report when it is the merge destination. The backend will move the merged transaction to that report, so it should not be deleted optimistically.
Fixed Issues
$ #79717
PROPOSAL: #79717 (comment)
Tests
Offline tests
QA Steps
Same as Tests above. The key verification is step 17 — the second cross-report merge should no longer cause infinite loading.
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