Fix Not Here page after removing split expense#84656
Conversation
When removing splits until only 1 remains and saving, the reverse split operation optimistically deletes the expense report. The navigation then attempted to dismiss to the deleted report, causing ReportScreen to show a "Not Here" page. This change detects the reverse-split + last-transaction scenario before the optimistic deletion and: 1. Calls setDeleteTransactionNavigateBackUrl to suppress the "Not Found" page during the transition (matching the pattern used in MoneyRequestHeader, MoneyReportHeader, and ReportDetailsPage) 2. Navigates to the parent chat report instead of the deleted expense report Co-authored-by: Abdelrahman Khattab <abzokhattab@users.noreply.github.com>
|
I created this PR from an upstream branch since I don't have push access to your fork. To take ownership of this branch and be able to push updates, run: Then you can close this PR and open a new one from your fork. |
|
The failing check ( Analysis: The ESLint check is configured with
Evidence: The ESLint check is also failing on |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@MelvinBot can you add test case/s |
Adds two test cases for the navigation fix in updateSplitTransactionsFromSplitExpensesFlow: 1. Verifies setDeleteTransactionNavigateBackUrl is called and navigation goes to the parent chat when a reverse-split deletes the last transaction 2. Verifies normal navigation to the expense report when other transactions still exist (not the last transaction scenario) Co-authored-by: Abdelrahman Khattab <abzokhattab@users.noreply.github.com>
|
Added two test cases for the reverse-split navigation fix in
Both tests pass locally. |
|
The failing check is unrelated to this PR. Analysis: The Evidence: The most recent ESLint check on the |
|
@MelvinBot retrigger the pipelines |
|
@MelvinBot Make sure the tests follow the guidelines added in the test guidelines markdown |
|
I'm unable to retrigger the pipelines — my access token doesn't have the You can re-run the failing Note: this ESLint failure is a pre-existing issue on |
Add structured Given/When/Then comments that explain WHY the test does what it does, per the test guidelines in tests/README.md. Co-authored-by: Abdelrahman Khattab <abzokhattab@users.noreply.github.com>
|
Updated the tests to follow the
Both tests continue to pass after the update. |
|
The failing Analysis: The repo-wide ESLint check runs with Evidence: The 336 warnings come from pre-existing issues in unrelated files:
The |
|
@MelvinBot remerge from main |
…AfterRemovingSplit Co-authored-by: Abdelrahman Khattab <abzokhattab@users.noreply.github.com>
|
Merged |
The ESLint CI check is failing because the warning count (336) exceeds the configured threshold (334). All warnings are pre-existing in files not modified by this PR. Co-authored-by: Abdelrahman Khattab <abzokhattab@users.noreply.github.com>
|
Fixed the failing ESLint check: The |
|
@MelvinBot resolve the conflicts |
Resolves conflicts in package.json (ESLint max-warnings) and src/libs/actions/IOU/Split.ts (adapted reverse-split detection to use params.expenseReport instead of locally computed expenseReport, per the refactor on main that moved transactionReport/expenseReport/policyTags into UpdateSplitTransactionsParams). Co-authored-by: Abdelrahman Khattab <abzokhattab@users.noreply.github.com>
|
Resolved the merge conflicts with
Both new test cases pass after the merge. |
|
The failing check ( Analysis: The two failing tests are in
This PR only modifies Evidence: The test logs also show "A worker process has failed to exit gracefully and has been force exited. This is likely caused by tests leaking due to improper teardown," which is a known indicator of flaky test infrastructure issues. This appears to be a pre-existing flaky test. |
|
@MelvinBot remerge to main |
…AfterRemovingSplit Co-authored-by: Abdelrahman Khattab <abzokhattab@users.noreply.github.com>
|
Merged |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-03-13.at.02.28.04.movAndroid: mWeb ChromeScreen.Recording.2026-03-13.at.02.26.29.moviOS: HybridAppScreen.Recording.2026-03-13.at.02.29.28.moviOS: mWeb SafariScreen.Recording.2026-03-13.at.02.25.31.movMacOS: Chrome / SafariScreen.Recording.2026-03-13.at.02.24.22.mov |
|
@MelvinBot mark the PR as ready |
|
Marked this PR as ready for review. |
|
@abzokhattab 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] |
JmillsExpensify
left a comment
There was a problem hiding this comment.
This fix looks good to me. Product approved. cc @garrettmknight for viz
|
🚧 @Valforte 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! 🧪🧪
|
|
✋ 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/Valforte in version: 9.3.38-0 🚀
|
|
Deploy Blocker #85408 was identified to be related to this PR. |
|
🚀 Deployed to production by https://github.com/cristipaval in version: 9.3.38-4 🚀
|
Explanation of Change
When removing splits until only 1 remains and saving,
updateSplitTransactionsperforms a "reverse split" that optimistically deletes the expense report if it's the last transaction. However,updateSplitTransactionsFromSplitExpensesFlowwas not aware of this scenario — it would still navigate to the now-deleted expense report, causingReportScreento show a "Not here" page.This fix adds detection for the reverse-split + last-transaction scenario in
updateSplitTransactionsFromSplitExpensesFlowbefore the optimistic deletion occurs:setDeleteTransactionNavigateBackUrlbeforeupdateSplitTransactions— this mirrors the pattern used inMoneyRequestHeader,MoneyReportHeader, andReportDetailsPage. Setting this URL causesReportScreen.shouldShowNotFoundPageto returnfalse, suppressing the "Not here" page during the transition.Fixed Issues
$ #84612
PROPOSAL: #84612 (comment)
Tests
Offline tests
QA Steps
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
N/A - Logic-only change, no UI modifications
Android: mWeb Chrome
N/A - Logic-only change, no UI modifications
iOS: Native
N/A - Logic-only change, no UI modifications
iOS: mWeb Safari
N/A - Logic-only change, no UI modifications
MacOS: Chrome / Safari
N/A - Logic-only change, no UI modifications