Show migrated user welcome modal immediately and navigate to Home on dismiss#86733
Show migrated user welcome modal immediately and navigate to Home on dismiss#86733
Conversation
…me on dismiss The migrated user welcome modal guard was purely passive — it only evaluated during navigation actions. When nvp_tryNewDot was updated via Onyx, the guard condition became true silently but no navigation was dispatched, so the modal only appeared on the next user-initiated navigation. Added proactive navigation in the Onyx callbacks: when session is authenticated, app is loaded, and nudge migration is active, navigate directly to the modal route. Also added Navigation.navigate(ROUTES.HOME) in MigratedUserWelcomeModal's onClose handler so dismissing the modal navigates the user to the Reports page as expected. Co-authored-by: Eric Han <eh2077@users.noreply.github.com>
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.
|
|
@MelvinBot mark this ready for review |
|
Done! PR has been marked as ready for review. ✅ |
melvin-bot
bot
requested review from
eh2077 and
heyjennahay
and removed request for
a team
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b76596f180
ℹ️ 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".
| hasRedirectedToMigratedUserModal = true; | ||
| Navigation.navigate(ROUTES.MIGRATED_USER_WELCOME_MODAL.getRoute()); |
There was a problem hiding this comment.
hasRedirectedToMigratedUserModal is flipped to true before attempting the proactive navigation. When this callback runs before the navigator is ready, the navigate call can be deferred and later replaced by another startup navigation, but this guard will still treat the modal as already redirected and return ALLOW on subsequent actions. In that case eligible users can miss the migrated-user modal for the rest of the session because the redirect gate is permanently closed even though the modal never opened.
Useful? React with 👍 / 👎.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-04-20.at.9.28.31.PM.mov |
| const onClose = () => { | ||
| Log.hmmm('[MigratedUserWelcomeModal] onClose called, dismissing product training'); | ||
| dismissProductTraining(CONST.MIGRATED_USER_WELCOME_MODAL); | ||
| Navigation.navigate(ROUTES.HOME); |
There was a problem hiding this comment.
@MelvinBot I think we should navigate to SEARCH_ROOT according to the original bug report #86083
There was a problem hiding this comment.
Updated — now navigating to SEARCH_ROOT (default expense search) instead of HOME on modal dismiss, matching the original issue's expected behavior.
Co-authored-by: Eric Han <eh2077@users.noreply.github.com>
…ression The guard had its own connectWithoutView for SESSION and IS_LOADING_APP, duplicating the ones in guards/index.ts. The extra Onyx connections caused an additional render in the SidebarLinks perf test (render count +1). Instead, guards/index.ts now calls onSessionOrLoadingAppChanged() in its existing callbacks to pass the values through to the guard. Co-authored-by: Eric Han <eh2077@users.noreply.github.com>
|
Fixed the failing perf-test check: removed duplicate |
|
|
| const onClose = () => { | ||
| Log.hmmm('[MigratedUserWelcomeModal] onClose called, dismissing product training'); | ||
| dismissProductTraining(CONST.MIGRATED_USER_WELCOME_MODAL); | ||
| Navigation.navigate(ROUTES.SEARCH_ROOT.getRoute({query: buildCannedSearchQuery()})); |
There was a problem hiding this comment.
@MelvinBot Update this
| Navigation.navigate(ROUTES.SEARCH_ROOT.getRoute({query: buildCannedSearchQuery()})); | |
| Navigation.navigate(ROUTES.SEARCH_ROOT.getRoute({query: buildCannedSearchQuery({type: CONST.SEARCH.DATA_TYPES.EXPENSE_REPORT})})); |
There was a problem hiding this comment.
Updated — now passing CONST.SEARCH.DATA_TYPES.EXPENSE_REPORT to buildCannedSearchQuery so the modal dismisses to the expense report search view.
Pass EXPENSE_REPORT type to buildCannedSearchQuery per review feedback. Co-authored-by: Eric Han <eh2077@users.noreply.github.com>
The navigateToMigratedUserWelcomeModalIfReady() calls were running synchronously inside Onyx connectWithoutView callbacks, which interfered with Onyx batch processing and caused an extra render cycle in SidebarLinks perf tests. Deferring the check to the next event loop tick via setTimeout(0) with debouncing ensures it doesn't affect the current batch while still navigating promptly when conditions are met. Co-authored-by: Eric Han <eh2077@users.noreply.github.com>
…om/Expensify/App into claude-fixMigratedUserWelcomeModal Co-authored-by: Eric Han <eh2077@users.noreply.github.com>
|
Fixed the failing perf-tests check. The Fix: Deferred the navigation check to the next event loop tick using The other failing check ( |
|
MelvinBot has made 6 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
|
@MelvinBot try merge with main please |
…rWelcomeModal Co-authored-by: Eric Han <eh2077@users.noreply.github.com>
|
Merged |
| /** | ||
| * Schedules a deferred navigation check. The check is deferred to the next event loop tick | ||
| * so it doesn't run synchronously inside Onyx callbacks, which would interfere with | ||
| * Onyx batch processing and cause extra component renders. | ||
| */ | ||
| function scheduleNavigationCheck() { | ||
| if (pendingNavigationCheck !== undefined) { | ||
| return; | ||
| } | ||
| pendingNavigationCheck = setTimeout(() => { | ||
| pendingNavigationCheck = undefined; | ||
| navigateToMigratedUserWelcomeModalIfReady(); | ||
| }, 0); | ||
| } |
There was a problem hiding this comment.
Removed the scheduleNavigationCheck / setTimeout deferral — Onyx callbacks now call navigateToMigratedUserWelcomeModalIfReady() directly.
Co-authored-by: Eric Han <eh2077@users.noreply.github.com>
2 participants
Explanation of Change
The migrated user welcome modal had two bugs:
Modal didn't appear immediately — The
MigratedUserWelcomeModalGuardwas purely passive and only evaluated during user-initiated navigation actions. Whennvp_tryNewDotwas updated via Onyx (e.g., via DevTools console), the guard condition became true silently but no navigation was dispatched, so the modal only appeared on the next user-initiated navigation (e.g., clicking another LHN tab).No navigation to Reports after dismissal — Clicking "Got it" on the modal closed it via
Navigation.goBack()but there was no explicit navigation to the Reports/Home page, so users ended up wherever they were before.Changes:
navigateToMigratedUserWelcomeModalIfReady()function inMigratedUserWelcomeModalGuard.tsthat proactively navigates to the modal route when all conditions are met (session authenticated, app loaded, nudge migration active, modal not dismissed). This function is called fromconnectWithoutViewcallbacks forSESSION,IS_LOADING_APP, andNVP_TRY_NEW_DOT, so the modal appears immediately when the migration flag is set.Navigation.navigate(ROUTES.HOME)inMigratedUserWelcomeModal.tsx'sonClosehandler so dismissing the modal navigates the user to the Reports page.Fixed Issues
$ #86083
PROPOSAL: #86083 (comment)
Tests
Onyx.merge('nvp_tryNewDot', {nudgeMigration: {timestamp: Date.now()}})Offline tests
QA Steps
Onyx.merge('nvp_tryNewDot', {nudgeMigration: {timestamp: Date.now()}})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