fix: land new account on initially opened room#92834
Conversation
1202a6a to
d2ff7e8
Compare
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
b85d0d6 to
169b7ec
Compare
169b7ec to
1201f47
Compare
|
@bernhardoj 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] |
|
@marcochavezf 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] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1201f47709
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@FitseTLT kind ping here. Thanks! |
JmillsExpensify
left a comment
There was a problem hiding this comment.
I don't quite get how this particular state happens, though the sign-in on the RHP seems fine to me.
|
@JmillsExpensify Yes, agreed that SignIn on the RHP itself is fine. The issue is the transient navigation state created by opening From the frontend debug logs, right when [ So the report route is already the fullscreen base and SignIn is the RHP on top. The bug was that the onboarding REDIRECT reset replaced that base with Home. This PR preserves that existing fullscreen report base under @FitseTLT Do you agree with this as well? |
|
@JmillsExpensify This case is when you sign in while on a public room. |
JmillsExpensify
left a comment
There was a problem hiding this comment.
Ah, thanks. LGTM then.
|
Can u merge main @KJ21-ENG |
…sed-onboarding-deeplink-fix
|
@FitseTLT merged latest upstream/main into this PR branch and pushed it. |
|
@FitseTLT Kind ping here. Thanks! |
…sed-onboarding-deeplink-fix # Conflicts: # tests/unit/navigateAfterOnboardingTest.ts
Explanation of Change
This PR fixes issue #85242: when a signed-out user opens a report URL like
/r/<reportID>, signs in/signs up with a new account, and completes onboarding, the app should land on the initially opened report. Currentmainlands on Home instead.The accepted proposal identified the first destructive step in
RootStackRouter: whenOnboardingGuardreturns a REDIRECT to onboarding,handleNavigationGuardsresets the whole root stack using the adapted onboarding state. That adapted state has Home as the base route, so the report route that was revealed after dismissing SignIn is replaced by[Home, OnboardingModalNavigator].While validating the fix locally, frontend logging showed two additional Home navigations that could still overwrite the restored report after the router preserved it:
SignInModaldismisses the SignIn RHP and then navigates to Home.So the final fix is intentionally narrow:
OnboardingModalNavigatorinstead of replacing it with Home./concierge.Fixed Issues
$ #85242
PROPOSAL: #85242 (comment)
Tests
Manual recordings have been added below.
Original issue #85242:
https://staging.new.expensify.com/r/7075912447943023.Regression #86258:
https://staging.new.expensify.com/r/7075912447943023.Regression #90303:
https://staging.new.expensify.com/conciergefrom an external app such as Notes or the browser.Automated checks run:
npx prettier --check src/components/SidePanel/RHPVariantTest/index.ts src/libs/Navigation/AppNavigator/createRootStackNavigator/GetStateForActionHandlers.ts src/libs/Navigation/AppNavigator/createRootStackNavigator/RootStackRouter.ts src/pages/signin/SignInModal.tsx tests/unit/Navigation/guards/handleNavigationGuardRedirect.test.tsnpx eslint --cache --cache-location=node_modules/.cache/eslint --cache-strategy content --quiet --concurrency=auto --no-warn-ignored src/components/SidePanel/RHPVariantTest/index.ts src/libs/Navigation/AppNavigator/createRootStackNavigator/GetStateForActionHandlers.ts src/libs/Navigation/AppNavigator/createRootStackNavigator/RootStackRouter.ts src/pages/signin/SignInModal.tsx tests/unit/Navigation/guards/handleNavigationGuardRedirect.test.tsnpm run react-compiler-compliance-check check src/components/SidePanel/RHPVariantTest/index.ts src/libs/Navigation/AppNavigator/createRootStackNavigator/GetStateForActionHandlers.ts src/libs/Navigation/AppNavigator/createRootStackNavigator/RootStackRouter.ts src/pages/signin/SignInModal.tsx tests/unit/Navigation/guards/handleNavigationGuardRedirect.test.tsnpm test -- tests/unit/Navigation/guards/handleNavigationGuardRedirect.test.ts --runInBandgit diff --check upstream/main..HEADnpm run typecheck-tsgowas also attempted. It failed in unrelated local/native files:src/components/QRShare/QRShareWithDownload/index.native.tsxsrc/pages/settings/Profile/Avatar/AvatarCapture/index.native.tsxOffline tests
Not applicable. This change affects sign-in/onboarding navigation and requires online authentication/onboarding APIs.
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
ezyZip.15.mp4
Android: mWeb Chrome
ezyZip.14.mp4
iOS: Native
N/A
iOS: mWeb Safari
ezyZip.16.mp4
MacOS: Chrome / Safari
ezyZip.17.mp4