[BATCH 2] Migrate Navigation usage, part 1#88538
Conversation
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.
|
|
|
||
| onCloseCompleteRef.current = options?.afterTransition; | ||
| setIsSidePanelTransitionEnded(false); | ||
| SidePanelActions.closeSidePanel(!isExtraLargeScreenWidth || shouldUpdateNarrow); |
There was a problem hiding this comment.
I don't get why we no longer need shouldUpdateNarrow here 🤔
There was a problem hiding this comment.
I noticed that we never use this flag when calling closeSidePanel, so I just removed it
| if (isInNarrowPaneModal) { | ||
| Navigation.navigateBackToLastSuperWideRHPScreen({afterTransition: afterDelete}); | ||
| return; | ||
| } | ||
| onBackButtonPress(false, {afterTransition: afterDelete}); |
There was a problem hiding this comment.
Why do we change the logic here?
There was a problem hiding this comment.
I probably messed up resolving the merge conflicts 😓 , I'll double-check
| TransitionTracker.runAfterTransitions({ | ||
| callback: () => { | ||
| waitForUserSignIn().then(() => { | ||
| Navigation.waitForProtectedRoutes().then(() => { |
There was a problem hiding this comment.
We wait 3 times here 😓 Can't this logic be simplified a bit?
There was a problem hiding this comment.
I simplified it with async/await, looks like the best approach here
| @@ -24,10 +23,11 @@ function DeleteTransactionNavigateBackHandler() { | |||
| return; | |||
| } | |||
| // Clear the URL only after we navigate away to avoid a brief Not Found flash. | |||
There was a problem hiding this comment.
that sounds like the use-case for afterTransition. Do we have to use transition tracker here?
| @@ -102,10 +101,11 @@ function SearchMoneyRequestReportPage({route}: SearchMoneyRequestPageProps) { | |||
| return; | |||
| } | |||
| // Clear the URL only after we navigate away to avoid a brief Not Found flash. | |||
There was a problem hiding this comment.
I feel like doing it that way in these two cases might be a bit tricky. Since the report has quite a few flows where a transaction can be deleted, we'd have to track down every single goBack/other navigation call triggered after a deletion and manually insert the same afterTransition everywhere.
Also, the report data gets deleted before the navigation starts. Because of this, using a useEffect with transitionTracker works better here. It catches the exact moment when the data is gone and the screen loses focus, and only then queues up the callback (which safely runs after the navigation completes). Plus, the navigation method after a deletion can vary quite a bit (dismissModal, goBack, etc), so this approach handles all those cases more cleanly imo
| Navigation.dismissModal(); | ||
| if (backTo) { | ||
| Navigation.navigate(backTo); | ||
| Navigation.dismissModal(); |
There was a problem hiding this comment.
I wonder if we should put the next navigate in afterTransition callback here 🤔
There was a problem hiding this comment.
The navigation already has internal mechanisms that put navigation calls onto the stack, so there will be no conflicts. React Navigation will dismiss the modal first and then navigate using the afterTransition callback
|
on it now |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-05-27.at.14.05.21.movAndroid: mWeb Chromeam.moviOS: HybridAppios.moviOS: mWeb Safariios-mweb.movMacOS: Chrome / SafariTest 1web-1.movTest 2web-2.movTest 3web-3.mov |
There was a problem hiding this comment.
flushCallbacks() calls callback() without await, so the try/catch block won't catch errors from async callbacks. I think we should consider making flushCallbacks async-aware
There was a problem hiding this comment.
Added error handling for async callbacks, thanks for the suggestion!
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #83062 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
|
🚧 @luacmartins 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/luacmartins in version: 9.3.89-1 🚀
Bundle Size Analysis (Sentry): |
|
🤖 Help site review: No changes required. This PR is an internal refactor — it migrates 15 There are no user-facing changes:
No corresponding |
|
@collectioneur This PR is failing in Test 2 and Test 3 because of a regression issue #92036
The issue is reproducible in: Web Bug7165846_1780008223260.Report_disappears.mp4 |
|
@miratachim I'm already looking into it. I'll try to release a fix today 🙌 |
|
@mitarachim after investigating, I discovered that the 2.movAlso, when the transition starts, an unrelated bug occurs (the overlay's opacity doesn't animate with the transition). This also contributes to the illusion that the transaction deletes before 1.mov |


Explanation of Change
Migrates 15
InteractionManager.runAfterInteractionsusages toTransitionTracker-based APIs. All entries are navigation-related, replaces fire-and-forget post-navigation callbacks with structuredafterTransitionoptions onNavigation.goBack,Navigation.dismissModal,Navigation.navigate, andcloseSidePanel. Ensures cleanup callbacks run only after transitions complete, preventing stale UI flashes.Fixed Issues
$ #83062
Tests
Test 1: Delete a single-expense report from Search
Test 2: Delete an entire report with multiple expenses from Search
Test 3: Delete a single transaction from within a transaction thread
Offline tests
N/A
QA Steps
Same as tests
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
Test 1
Screen.Recording.2026-05-21.at.14.41.22.mov
Test 2
Screen.Recording.2026-05-21.at.14.41.51.mov
Test 3
Screen.Recording.2026-05-21.at.14.42.49.mov