[CP Staging] fix(navigation): stop goUp at a pop prevented by a beforeRemove guard by WojtekBoman · Pull Request #102022 · Expensify/App

WojtekBoman · 2026-09-23T10:36:46Z

Explanation of Change

goUp kept sending pops after a beforeRemove guard prevented one, so the discard-changes guard saw a second pop and closed its own prompt. dispatch now detects a dropped action via the noop __unsafe_action__ event, stops goUp there, and disarms the focus-restore skip. Tests added in GoBackTests.tsx.

Fixed Issues

$ #102010
PROPOSAL:

Tests

Test 1 - Discard prompt shows on RHP Back (Description)

Open any expense report and open an expense you can edit
Click the Description field to open the edit page in the RHP
Type some text without saving
Click the Back button in the RHP header
Verify that the discard-changes confirmation prompt appears and stays open
Click "Cancel" in the prompt
Verify that the prompt closes, the Description page stays open, and the typed text is still there

Test 2 - Discard prompt shows on RHP Back (Merchant) and Discard leaves the page

Open an editable expense
Click the Merchant field to open the edit page in the RHP
Change the merchant without saving
Click the Back button in the RHP header
Verify that the discard-changes confirmation prompt appears and stays open
Click "Discard"
Verify that the RHP goes back to the expense details, and the merchant is unchanged

Test 3 - Save still goes back normally

Open an editable expense and click the Description field
Change the description and click "Save"
Verify that the RHP goes back to the expense details with no prompt, and the new description is shown
Press Back again (or Esc on web)
Verify that the RHP closes normally and focus returns to the element that opened it

Verify that no errors appear in the JS console

Offline tests

N/A - navigation-only change, no API calls or Onyx writes are affected.

QA Steps

Same as tests

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

WojtekBoman · 2026-09-23T10:36:57Z

@codex review

codecov · 2026-09-23T10:52:07Z

Codecov Report

❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.

Files with missing lines	Coverage Δ
src/libs/NavigationFocusReturn/index.native.ts	`96.03% <100.00%> (+0.03%)`	⬆️
src/libs/Navigation/Navigation.ts	`68.93% <93.33%> (+0.70%)`	⬆️
src/libs/NavigationFocusReturn/index.ts	`94.91% <0.00%> (-0.52%)`	⬇️
... and 32 files with indirect coverage changes

WojtekBoman · 2026-09-23T11:07:17Z

@MelvinBot review this

MelvinBot · 2026-09-23T11:13:06Z

Review

The approach is right — bailing out of goUp at the first prevented pop is what stops the second dispatch from reaching the guard over its own open prompt and closing it. Three things I'd want resolved before merge, one of them likely user-visible.

1. After the user confirms "Discard", the rest of the back navigation is dropped

useDiscardChangesConfirmation stores only the single prevented action (src/hooks/useDiscardChangesConfirmation/index.ts:124) and replays only that one on confirm (index.ts:55-62).

Before this PR, goUp dispatched every pop in its plan. Now it stops at the first prevented one, so on confirm the user gets just that pop — not the remaining pops, the jumpTo, or the final pop/replace. In the exact scenario your own test 2 models (modal over two workspace splits), "back" after discarding lands one screen short of backToRoute.

For the single-pop flow in Web - Expense - Back button does not trigger discard change modal on Description this is fine. Can you confirm the multi-pop case still lands on the requested route, or note it as a known follow-up?

2. `noop: true` means "no state change", not "prevented"

React Navigation emits noop: true from two places in useOnAction: when a beforeRemove listener prevents the action, and when the router returned no state change. A targeted StackActions.pop that StackRouter refuses (currentIndex === 0 → returns null → coerced back to state because action.target === state.key) lands in the second branch.

In that case Navigation.ts:555-557 now returns early and goUp silently does nothing, where before it fell through to the final pop/replace. It's reachable if a non-preventing beforeRemove listener changes state mid-loop and invalidates a pop that was pre-computed from the pre-loop state — narrow, but the comment at Navigation.ts:532-533 states the implication in only one direction. At minimum the comment should say noop also covers a plain no-op.

3. `shouldSkipFocusRestore` leaks on the prevented path

dispatch arms skipNextFocusRestore() before dispatching, and that's a module-level one-shot latch consumed by the next back navigation (src/libs/NavigationFocusReturn/index.ts:168-170).

If the pop is prevented and the user then cancels the prompt, nothing ever consumes the latch and it eats the focus restore on the next unrelated Back/Esc. The comment directly above — "no window between 'set flag' and dispatch for an early-return to leak it" (Navigation.ts:531) — is declaring exactly the invariant the new early returns break, in a new shape. Arming the flag only after the dispatch is known to have applied would close it.

Smaller points

The new guard on the tab-pop is untested. Both new tests only exercise the popsToNavigator loop (test 2's 2 dispatches are both loop pops). The underlyingTabNavIndex branch at Navigation.ts:580-584 has no prevented-pop coverage.
goUp's return value is dead. The only caller ignores it (Navigation.ts:670), so the "return true" comments read as if they suppress a fallback that doesn't exist. Harmless, but misleading.
Test 1's state assertion is close to tautological. GoBackTests.tsx:826 compares getRootState() to a reference captured earlier; with no state change React Navigation hands back the same object. Asserting route names the way test 2 does is stronger.
navigationRef.current?.dispatch → navigationContainer.dispatch drops the per-call null check. Equivalent given the guard at the top of goUp, just flagging the behavior change if the container unmounts mid-loop.
Reference equality on event.data.action does hold — useNavigationHelpers passes the action object straight through to onAction, and the listener is subscribed and unsubscribed synchronously around the dispatch. No issue there.
Using __unsafe_action__ for app logic is documented as debug-only upstream, but runAfterPredictedTransition.ts already does it, so this is consistent with existing practice rather than a new pattern. Its global listener skips noop actions, so there's no interaction with the prediction window here.
PR body: Tests and QA Steps are empty and the author checklist is unticked. Worth filling in given this is on a DeployBlockerCash issue.

All substantive CI checks are green (the two failures are the checklist and independent-approval jobs).

I reviewed this statically — browser verification wasn't available in this run, and I couldn't execute the Jest suite locally either, so I'm relying on CI for the test results.

collectioneur · 2026-09-23T13:24:16Z

-        dispatch(popToNavigator);
+        // A prevented pop hands control to its guard: going on would change screens under its prompt or re-send the pop,
+        // which closes the prompt. On confirm the guard replays only this pop, which usually is the whole way back.
+        if (!dispatch(popToNavigator)) {


Can there be any cases in the app where we should continue after a no-op?

afaik not today: goUp's pops are all pop(n > 0) computed from the current state, so the only noop they can produce is a beforeRemove guard preventing them, and the dispatches after the loop are goUp's last step anyway

collectioneur · 2026-09-23T13:25:36Z

        }
-        navigationRef.current?.dispatch(actionToDispatch);
+        let wasApplied = true;
+        const unsubscribe = navigationContainer.addListener('__unsafe_action__', (event) => {


unsafe_action is synchronous. Are you sure that the navigation events will happen before the listener is triggered, but after it's registered?

It's the order: dispatch -> onAction -> beforeRemove -> emit('__unsafe_action__') all run synchronously inside the dispatch() call, so the event always should land between addListener and unsubscribe()

melvin-bot · 2026-09-23T15:25:16Z

@huult 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]

huult · 2026-09-23T15:31:25Z

Reviewer Checklist

Screenshots/Videos

Android: HybridApp

Screen.Recording.2026-09-23.at.22.49.02.mov

Android: mWeb Chrome

Screen.Recording.2026-09-23.at.22.50.28.mov

iOS: HybridApp

Screen.Recording.2026-09-23.at.22.51.52.mov

iOS: mWeb Safari

Screen.Recording.2026-09-23.at.22.52.21.mov

MacOS: Chrome / Safari

Screen.Recording.2026-09-23.at.22.47.09.mov

huult

LGTM.

lakchote

LGTM. Thoughts @Julesssss?

Julesssss · 2026-09-23T16:45:58Z

+            expect(skipSpy).toHaveBeenCalledTimes(1);
+            expect(cancelSkipSpy).toHaveBeenCalledTimes(1);


It would be better to test the outcome of cancelSkipSpy being called, this would still pass if it's behaviour changed.

Julesssss

1 comment about test but NAB as this resolves a blocker

…-pop-on-prevented-remove [CP Staging] fix(navigation): stop goUp at a pop prevented by a beforeRemove guard (cherry picked from commit 6dd93ed) (cherry-picked to staging by lakchote)

github-actions · 2026-09-23T16:55:12Z

🚧 Julesssss has triggered a test Expensify/App build. You can view the workflow run here.

OSBotify · 2026-09-23T16:56:05Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

github-actions · 2026-09-23T16:59:50Z

🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
Built from App PR #102022.

Android 🤖	iOS 🍎
https://ad-hoc-expensify-cash.s3.us-east-1.amazonaws.com/rock-artifacts/ad-hoc/rock-android-Adhoc-6dd93ed-ad3e58a-6fc9bc666dd00ecc414352a2bc4ac17dff85310c/index.html	https://ad-hoc-expensify-cash.s3.us-east-1.amazonaws.com/rock-artifacts/ad-hoc/rock-ios-device-AdHoc-6dd93ed-ad3e58a-f772a7db7c6014a543b2ad39d3445b4395bcfc8e/index.html

Web 🕸️
https://102022.pr-testing.expensify.com

👀 View the workflow run that generated this build 👀

os-botify · 2026-09-23T17:14:17Z

🔁 Filed a Staging retest request for deploy blockers #102010 after this PR was cherry-picked to staging.

OSBotify · 2026-09-23T17:14:47Z

🚀 Cherry-picked to staging by https://github.com/lakchote in version: 9.4.92-5 🚀

platform	result
🕸 web 🕸	success ✅
🤖 android 🤖	success ✅
🍎 iOS 🍎	success ✅

MelvinBot · 2026-09-23T17:16:15Z

Help site review: no changes required

No draft PR created — nothing under App/docs/articles needs updating for this PR.

Why: this is an internal navigation fix. It changes how goUp sequences its dispatches when a beforeRemove guard prevents a pop, so the discard-changes prompt stays open instead of being closed by a follow-up dispatch. It restores the behavior the app was always meant to have — it doesn't add, remove, or rename any feature, tab, setting, or button.

What I checked:

No help article documents the discard-changes prompt or back-button behavior. The only "discard" hits in new-expensify are about discarding a GPS trip in Distance-Expenses.md and discarding duplicate expenses in How-to-Find-and-Resolve-Flagged-Duplicate-Expenses.md — neither is this prompt.
Edit-Expenses.md is the article covering the flow in Web - Expense - Back button does not trigger discard change modal on Description. It lists which fields you can edit, including Description and Merchant, but says nothing about what happens when you leave an edit with unsaved changes. Nothing in it becomes wrong because of this PR.
No UI strings, labels, or navigation entry points change in the diff — only src/libs/Navigation/Navigation.ts and two test files.

If you'd rather the discard-changes prompt be documented as behavior in its own right, that's a net-new article request rather than a fix follow-up — say the word and I'll draft it.

I reached this from static review of the diff and a search of App/docs/articles. Browser verification wasn't available in this run, so I couldn't confirm the on-screen copy of the prompt against the live UI.

@WojtekBoman, since no help site changes are needed, there's no linked docs PR to review — please confirm you agree with that conclusion, or point me at an article you think should cover this and I'll open the draft PR.

Julesssss · 2026-09-23T17:21:43Z

Requested QA retest here.

OSBotify · 2026-09-24T12:34:04Z

🚀 Deployed to production by https://github.com/lakchote in version: 9.4.92-8 🚀

platform	result
🕸 web 🕸	success ✅
🤖 android 🤖	success ✅
🍎 iOS 🍎	success ✅

Bundle Size Analysis (Sentry):

Android

OSBotify · 2026-09-24T12:53:40Z

🚀 Cherry-picked to staging by https://github.com/lakchote in version: 9.4.93-0 🚀

platform	result
🕸 web 🕸	success ✅
🤖 android 🤖	success ✅
🍎 iOS 🍎	success ✅

MelvinBot · 2026-09-24T12:56:58Z

No help site changes required

This PR is an internal navigation fix, not a product behavior change. No files under docs/articles need updating, so I did not create a draft docs PR.

Why:

The change is limited to navigation plumbing: src/libs/Navigation/Navigation.ts stops goUp when a beforeRemove guard drops a pop, and src/libs/NavigationFocusReturn/index.ts gains a matching way to disarm the focus-restore skip. The rest of the diff is tests.
It restores the expected behavior described in [Due for payment 2026-10-01] Web - Expense - Back button does not trigger discard change modal on Description — the discard-changes prompt appearing when you press Back with unsaved edits. Nothing about the documented product changes.
No new or renamed features, tabs, settings labels, or buttons. No copy changes.
I searched docs/articles for any article that documents the discard-changes prompt, unsaved edits, or Back button behavior in the right-hand panel. There are no matches, so there is no existing article to correct either.

@WojtekBoman, please confirm you agree that no help site update is needed. If you think an article should cover this behavior, tell me which one and I'll draft the docs PR.

view run

OSBotify · 2026-09-25T13:24:24Z

🚀 Deployed to production by https://github.com/lakchote in version: 9.4.93-3 🚀

platform	result
🕸 web 🕸	success ✅
🤖 android 🤖	success ✅
🍎 iOS 🍎	success ✅

Bundle Size Analysis (Sentry):

Android

fix(navigation): stop goUp at a pop prevented by a beforeRemove guard

e946c4c

melvin-bot Bot assigned WojtekBoman Sep 23, 2026

fix(navigation): disarm focus-restore skip when goUp pop is prevented

f5dd859

collectioneur reviewed Sep 23, 2026

View reviewed changes

WojtekBoman marked this pull request as ready for review September 23, 2026 15:25

WojtekBoman requested review from a team as code owners September 23, 2026 15:25

melvin-bot Bot requested review from a team, flaviadefaria and huult and removed request for a team September 23, 2026 15:25

melvin-bot Bot removed the request for review from a team September 23, 2026 15:25

huult approved these changes Sep 23, 2026

View reviewed changes

melvin-bot Bot requested review from Julesssss and blimpich September 23, 2026 16:02

blimpich removed their request for review September 23, 2026 16:04

lakchote changed the title ~~fix(navigation): stop goUp at a pop prevented by a beforeRemove guard~~ [CP Staging] fix(navigation): stop goUp at a pop prevented by a beforeRemove guard Sep 23, 2026

lakchote approved these changes Sep 23, 2026

View reviewed changes

Julesssss reviewed Sep 23, 2026

View reviewed changes

Julesssss approved these changes Sep 23, 2026

View reviewed changes

Julesssss merged commit 6dd93ed into Expensify:main Sep 23, 2026
42 of 51 checks passed

OSBotify added the CP Staging marks PRs that have been CP'd to staging label Sep 23, 2026

Julesssss mentioned this pull request Sep 23, 2026

[Due for payment 2026-10-01] Web - Expense - Back button does not trigger discard change modal on Description #102010

Closed

1 of 7 tasks

OSBotify mentioned this pull request Sep 23, 2026

Deploy Checklist: New Expensify 2026-09-23 #101971

Closed

		expect(skipSpy).toHaveBeenCalledTimes(1);
		expect(cancelSkipSpy).toHaveBeenCalledTimes(1);

Conversation

WojtekBoman commented Sep 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Explanation of Change

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

WojtekBoman commented Sep 23, 2026

Uh oh!

codecov Bot commented Sep 23, 2026 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

WojtekBoman commented Sep 23, 2026

Uh oh!

MelvinBot commented Sep 23, 2026

Review

1. After the user confirms "Discard", the rest of the back navigation is dropped

2. noop: true means "no state change", not "prevented"

3. shouldSkipFocusRestore leaks on the prevented path

Uh oh!

collectioneur Sep 23, 2026

Choose a reason for hiding this comment

Uh oh!

WojtekBoman Sep 23, 2026

Choose a reason for hiding this comment

Uh oh!

collectioneur Sep 23, 2026

Choose a reason for hiding this comment

Uh oh!

WojtekBoman Sep 23, 2026

Choose a reason for hiding this comment

Uh oh!

melvin-bot Bot commented Sep 23, 2026

Uh oh!

huult commented Sep 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviewer Checklist

Screenshots/Videos

Uh oh!

huult left a comment

Choose a reason for hiding this comment

Uh oh!

lakchote left a comment

Choose a reason for hiding this comment

Uh oh!

Julesssss Sep 23, 2026

Choose a reason for hiding this comment

Uh oh!

Julesssss left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions Bot commented Sep 23, 2026

Uh oh!

OSBotify commented Sep 23, 2026

Uh oh!

github-actions Bot commented Sep 23, 2026

Uh oh!

os-botify Bot commented Sep 23, 2026

Uh oh!

OSBotify commented Sep 23, 2026

Uh oh!

MelvinBot commented Sep 23, 2026

Help site review: no changes required

Uh oh!

Julesssss commented Sep 23, 2026

Uh oh!

OSBotify commented Sep 24, 2026

Uh oh!

OSBotify commented Sep 24, 2026

Uh oh!

MelvinBot commented Sep 24, 2026

No help site changes required

Uh oh!

OSBotify commented Sep 25, 2026

WojtekBoman commented Sep 23, 2026 •

edited

Loading

codecov Bot commented Sep 23, 2026 •

edited by github-actions Bot

Loading

2. `noop: true` means "no state change", not "prevented"

3. `shouldSkipFocusRestore` leaks on the prevented path

huult commented Sep 23, 2026 •

edited

Loading