Use backward navigation pattern for reverse-split deletion#85425
Use backward navigation pattern for reverse-split deletion#85425
Conversation
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. |
Co-authored-by: Abdelrahman Khattab <abzokhattab@users.noreply.github.com>
|
Fixed the failing |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
🚧 @Valforte has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
still examining the selected approach ... worked on it abit today but i am still not sure about it .. need more time to look into it .. will check it today in the morning |
|
Please re-state the problem that we are trying to solve in this issue. On Android (narrow layout), after splitting an expense, creating a report from one split, removing the remaining split (triggering a reverse-split), and saving, the user is navigated to the Workspace Chat. However, pressing the hardware Back button from the WS chat shows a "Not here" page instead of normal back navigation. Further back presses create a loop between the WS chat and the "Not here" page. This is a regression introduced by #84656. What is the root cause of that problem? PR #84656 fixed the original "Not here" page issue (#84612) by making On narrow layout, The The fundamental issue is that What changes do you think we should make in order to solve the problem? In if (isLastTransactionInReport && fallbackReportID) {
const backRoute = ROUTES.REPORT_WITH_ID.getRoute(fallbackReportID);
Navigation.dismissToSuperWideRHP();
Navigation.isNavigationReady().then(() => {
Navigation.goBack(backRoute);
});
return;
}How this works:
Both callbacks are The existing The non-deletion path (normal split updates) keeps using can you check this solution @MelvinBot is it better than the solution you proposed ? |
|
Great analysis, ComparisonCurrent PR approach (mine)Uses Your proposed approachUses
Verdict: Your approach is betterFollows established patterns. Addresses the root cause directly. The fundamental issue is using forward navigation (replace) for a deletion context. Your approach uses backward navigation (pop), which naturally cleans the stack instead of creating new routes and then cleaning up stale ones. Less fragile. The current PR's cleanup relies on:
Your approach avoids all of this — Simpler code. Replaces ~15 lines of cleanup logic with ~4 lines that mirror an existing pattern. I'd recommend updating the PR to use your proposed approach. The |
|
@MelvinBot implement the new approach and adapt the tests and the pr description |
…reverse-split deletion Replace dismissModalWithReport with the established deletion navigation pattern used by navigateBackOnDeleteTransaction. On narrow layout, dismissModalWithReport navigates forward (replace), leaving stale screens in the back stack. The backward pattern (dismissToSuperWideRHP + goBack) naturally pops stale screens instead. Co-authored-by: Abdelrahman Khattab <abzokhattab@users.noreply.github.com>
|
Done! I've updated the PR to implement the new approach: Code changes (
Test changes (
PR description: Updated to reflect the new approach. The relevant test passes locally. CI will validate the full suite. |
|
@MelvinBot mark the pr as ready |
|
I'm not able to mark PRs as ready for review — this is a safety guardrail to ensure a human reviews the PR state before it goes out for review. You can mark it ready yourself by clicking the "Ready for review" button on the PR page, or running: |
Co-authored-by: Abdelrahman Khattab <abzokhattab@users.noreply.github.com>
|
Fixed the failing ESLint check: inverted the |
|
@MelvinBot mark the PR as ready |
|
@ChavdaSachin 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] |
|
Done — PR is now marked as ready for review! ✅ |
|
Does this need my review? |
src/libs/actions/IOU/Split.ts
Outdated
| // stale screens from the stack, matching the pattern in navigateBackOnDeleteTransaction. | ||
| if (isLastTransactionInReport && fallbackReportID) { | ||
| const backRoute = ROUTES.REPORT_WITH_ID.getRoute(fallbackReportID); | ||
| Navigation.dismissToSuperWideRHP(); |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
The navigation logic added here (Navigation.dismissToSuperWideRHP() followed by Navigation.isNavigationReady().then(() => Navigation.goBack(backRoute))) duplicates the core logic of the existing navigateBackOnDeleteTransaction function in src/libs/ReportUtils.ts, which is already exported. The PR description even acknowledges this match: "matching the approach used by navigateBackOnDeleteTransaction".
Since the search fullscreen case is already handled earlier in this function (line 1730), calling navigateBackOnDeleteTransaction(backRoute) would skip the SEARCH_FULLSCREEN_NAVIGATOR check and execute the same dismissToSuperWideRHP + goBack sequence.
Reuse the existing function instead of duplicating its logic:
if (isLastTransactionInReport && fallbackReportID) {
const backRoute = ROUTES.REPORT_WITH_ID.getRoute(fallbackReportID);
navigateBackOnDeleteTransaction(backRoute);
// Remove the transaction thread report screen to avoid navigating back to a removed report
requestAnimationFrame(() => {
if (!transactionThreadReportScreen?.key) {
return;
}
Navigation.removeScreenByKey(transactionThreadReportScreen.key);
});
return;
}Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Good catch — this is a valid suggestion. The inlined dismissToSuperWideRHP() + isNavigationReady().then(() => goBack(backRoute)) is an exact duplicate of the non-search path in navigateBackOnDeleteTransaction (ReportUtils.ts:6225-6228).
The search fullscreen check inside navigateBackOnDeleteTransaction is harmless here because updateSplitTransactionsFromSplitExpensesFlow already handles the search case with an early return at line 1730, so by the time we reach this block we're guaranteed to not be on the search fullscreen navigator.
I'll update the code to reuse navigateBackOnDeleteTransaction(backRoute) as suggested, keeping the requestAnimationFrame cleanup block that follows (since that part is specific to this call site and not in navigateBackOnDeleteTransaction).
|
@ChavdaSachin i will be reviewing it .. i dont think Melvin PRs need 2 reviewers |
Replace the inlined dismissToSuperWideRHP + goBack pattern with a call to the existing navigateBackOnDeleteTransaction function from ReportUtils, addressing the CONSISTENCY-3 review feedback. Co-authored-by: Abdelrahman Khattab <abzokhattab@users.noreply.github.com>
|
@dukenv0307 @Valforte One of you needs to 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: 5faac692bb
ℹ️ 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".
src/libs/actions/IOU/Split.ts
Outdated
| if (isLastTransactionInReport && fallbackReportID) { | ||
| const backRoute = ROUTES.REPORT_WITH_ID.getRoute(fallbackReportID); | ||
| Navigation.dismissToSuperWideRHP(); | ||
| Navigation.isNavigationReady().then(() => { | ||
| Navigation.goBack(backRoute); |
There was a problem hiding this comment.
Set submit follow-up action before early return
When a reverse split deletes the last transaction, this new early-return branch skips the setPendingSubmitFollowUpAction(...) call that still runs for the non-deletion path. If SPAN_SUBMIT_TO_DESTINATION_VISIBLE is active, the destination ReportScreen/search visibility handlers rely on that pending action to end the span, so this path now leaves submit-to-visible telemetry untracked for the delete flow and can skew experiment/performance metrics.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The Codex review is technically correct that the early return skips setPendingSubmitFollowUpAction, but it's not a real issue. Here's why:
-
The span is unlikely to be active here.
SPAN_SUBMIT_TO_DESTINATION_VISIBLEis started during expense creation/submission flows (IOURequestStepConfirmation,SendInvoice,SendMoney).updateSplitTransactionsFromSplitExpensesFlowis an update flow for existing splits — different user flow entirely. -
Even if it were active, setting it would be worse. The
targetReportIDin the non-deletion path isparams.expenseReport?.reportID— the expense report being deleted. CallingsetPendingSubmitFollowUpAction(DISMISS_MODAL_AND_OPEN_REPORT, deletedReportID)would tell the system to end the span when that report becomes visible, but a deleted report will never become visible — leaving the span stuck forever, which is worse than not setting it. -
No other deletion flow sets this.
navigateBackOnDeleteTransaction(used byReportDetailsPage),MoneyReportHeader, andMoneyRequestHeaderall skipsetPendingSubmitFollowUpActionduring deletion. This PR follows the same pattern. -
The search early return already skips it too. The pre-existing search path (line ~1730) also returns without calling
setPendingSubmitFollowUpAction— same situation, shipping without issues.
The review can be safely dismissed.
JmillsExpensify
left a comment
There was a problem hiding this comment.
Yeah, would love to get rid of all these "Hmm not here" cases. Seems to be happening a lot right now. Product approved.
|
Waiting on you @abzokhattab to review and signal the issue is resolved |
|
@MelvinBot change this step After it navigates you to WS chat, click on Back navigation now it navigates to the the report chat not the ws chat |
|
Updated test step 11 from "After it navigates you to WS chat" to "After it navigates you to the report chat" to reflect the current navigation behavior. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-03-24.at.03.36.00.movAndroid: mWeb ChromeScreen.Recording.2026-03-24.at.03.38.13.moviOS: HybridAppScreen.Recording.2026-03-24.at.03.33.20.moviOS: mWeb SafariScreen.Recording.2026-03-24.at.03.40.45.movMacOS: Chrome / SafariScreen.Recording.2026-03-24.at.03.30.54.mov |
|
🚧 @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. |
Explanation of Change
On Android (narrow layout), after a reverse-split deletes the last transaction in an expense report, the previous approach used
dismissModalWithReportto navigate to the parent chat. However,dismissModalWithReporton narrow layout usesnavigate(reportRoute, {forceReplace: true})which replaces the focused root-level route (the RHP) with a newReportsSplitNavigatorfor the parent chat, while the oldReportsSplitNavigator(containing the now-deleted expense report screen) remains in the navigation back stack. This caused the "Not here" page to appear when pressing the hardware Back button.This PR switches the reverse-split deletion navigation to use the established backward navigation pattern (
dismissToSuperWideRHP+goBack), matching the approach used bynavigateBackOnDeleteTransaction. On narrow layout,dismissToSuperWideRHPcallsdismissModal()which pops the RHP, thengoBack(backRoute)pops the stale expense report screen from the RSN's internal state — naturally cleaning the stack without leaving stale routes behind.The non-deletion path (normal split updates where the expense report still exists) continues to use
dismissModalWithReportunchanged.Fixed Issues
$ #85408
Tests
Offline tests
N/A - This is a navigation-only fix that runs synchronously after the split save operation.
QA Steps
Same as tests above, performed on Android.
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.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