Create navigation guards + implement Onboarding Guard#79898
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.
|
TMisiukiewicz
force-pushed
the
navigation-guards
branch
from
8608177 to
3f08445
Compare
melvin-bot
Bot
requested review from
eh2077 and
joekaufmanexpensify
and removed request for
a team
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
@TMisiukiewicz conflicts @codex review |
![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: 282d6fff38
ℹ️ 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".
Code Review SummaryI've done a thorough review of this PR. The Navigation Guards architecture is well-designed and provides a clean, extensible pattern. Here are the key issues I identified: Critical Issues1. Missing Authentication Check in OnboardingGuardThe guard doesn't check const shouldSkipOnboarding = context.isLoading || isTransitioning || isOnboardingCompleted || isMigratedUser || isSingleEntry || needsExplanationModal || isInvitedOrGroupMember;The original code in if (!CONFIG.IS_HYBRID_APP && !hasNonPersonalPolicy && !isOnboardingCompleted && !wasInvitedToNewDot && authenticated) {Risk: Unauthenticated users could potentially be redirected to onboarding screens. Recommendation: Add 2. Test Drive Modal Deep Link BypassThe OnboardingGuard only checks The temporary workaround in // Temporary solution to navigate to onboarding when trying to access the app
// Should be removed once Test Drive modal route has its own navigation guard
if (hasCompletedGuidedSetupFlowSelector(onboardingValues) && onboardingValues?.testDriveModalDismissed === false) {Risk: Users who completed onboarding but haven't dismissed the test drive modal could bypass it via deep links that don't go through Recommendation: Either factor 3. Verify BLOCK Result Works with syncBrowserHistoryThe Recommendation: Manually verify the test plan from #50719 still works:
Overall AssessmentThe architecture is solid and will make it easier to add future navigation guards. The code reduction (~100+ lines removed) and centralization of logic is a clear improvement. The issues above should be addressed before merge to prevent regressions. |
No need to do that because guards are being called in
test drive modal will be a separate guard (next one right after merging that). This PR covers the temporary logic to display it, tested above
Verified it, works fine conflicts resolved ✅ |
|
nice! reviewing this, will update in an hour or two |
@TMisiukiewicz hmm, it works on web but native app Screen.Recording.2026-02-02.at.6.08.59.PM.mov |
|
Bug: mobile browser - test-drive modal pops up twice when redirect from workspaces page
|
hmm looks like the same thing happens on Screen.Recording.2026-02-02.at.11.35.15.mov |
Hmm interesting, I tried a couple of times but I couldn't reproduce it 👀 ios.res.mov |
|
I see, I can't reproduce either. Screen.Recording.2026-02-02.at.7.04.35.PM.mov |
eh2077
left a comment
eh2077
left a comment
There was a problem hiding this comment.
Apart from #79898 (comment), then all good!
|
Since the bug mentioned doesn't seem to be strictly related to this PR, I'll continue with merging |
|
✋ 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/mjasikowski in version: 9.3.11-25 🚀
|
|
@mjasikowski @TMisiukiewicz Could this please be tested internally? The QA team is unable to validate it on native devices, as it requires running logs in the terminal or any workaround for this? |
|
@m-natarajan yup I think it might be difficult to test deep link scenarios for QAs unless they have Android and iOS environment set up and able to connect the native devices with macbook |
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.3.12-1 🚀
|
|
This PR introduced the following issue:
The problem occurs because this PR adds an onboarding guard, which unintentionally blocks the deeplink navigation during login |
11 participants
Explanation of Change
Fixed Issues
$ #79974
PROPOSAL:
Tests
xcrun simctl openurl booted "new-expensify://workspaces"in terminaladb shell am start -W -a android.intent.action.VIEW -d "new-expensify://workspaces"xcrun simctl openurl booted "new-expensify://workspaces"in terminaladb shell am start -W -a android.intent.action.VIEW -d "new-expensify://workspaces"xcrun simctl openurl booted "new-expensify://onboarding/purpose"in terminaladb shell am start -W -a android.intent.action.VIEW -d "new-expensify://onboarding/purpose"Offline tests
n/a
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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.mov
Android: mWeb Chrome
iOS: Native
ios.mov
iOS: mWeb Safari
ios-web.mov
MacOS: Chrome / Safari
web.mov