-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[HOLD for payment 2024-10-29] [$250] Sign in-User remains in first step of Onboarding when tapping an option after closing the app #50064
Comments
Triggered auto assignment to @stephanieelliott ( |
We think that this bug might be related to #wave-collect - Release 1 |
@stephanieelliott FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors |
ProposalPlease re-state the problem that we are trying to solve in this issue.The onboarding modal shows again from the first step after selecting an option if we reopen the app. What is the root cause of that problem?When we open the app, the onboarding page is shown by Lines 207 to 210 in bad93d5
Inside the function, we check whether the user already completed the onboarding or not. It's to prevent the user from deep-linking to another page if the onboarding hasn't been completed yet. App/src/libs/actions/Report.ts Lines 2683 to 2721 in bad93d5
But, we only execute it if the protected routes are ready. So, the checking is currently pending. The function will wait for the navigation to be ready first, however, even after the nav is ready, the state is undefined. App/src/libs/Navigation/Navigation.ts Lines 391 to 404 in bad93d5
The ready promise is resolved here, which doesn't guarantee that the state is ready. App/src/libs/Navigation/NavigationRoot.tsx Line 181 in bad93d5
We also resolve the promise here whenever the state is changed.
We can remove the first resolve from App/src/libs/Navigation/Navigation.ts Lines 369 to 375 in bad93d5
Only after we complete the first step by selecting any option, the protected route becomes available, and the pending onboarding flow check is executed, which triggers the 2nd What changes do you think we should make in order to solve the problem?We actually have a logic to ignore the onboarding start if the onboarding is currently in progress, App/src/libs/actions/Welcome/index.ts Lines 41 to 56 in 837152e
however, it uses a local variable which is only updated within the We can create a global variable to hold it, but I think a cleaner way to check if an onboarding is in progress is to check the focused route. If it's an onboarding modal navigator, then the onboarding is in progress.
In openReportFromDeepLink, we can early return if |
This has been labelled "Needs Reproduction". Follow the steps here: https://stackoverflowteams.com/c/expensify/questions/16989 |
Is this still happening? I'm not able to repro this on either platform -- RPReplay_Final1727995452.MP4@IuliiaHerets if it's still reproducible for you, can you please confirm the repro steps in the issue body? |
This is still happening to me ios.mp4 |
@stephanieelliott Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
Ok, this is a critical flow and the issue makes sense given the proposal, so we should move forward with fixing it. Applying labels. |
Job added to Upwork: https://www.upwork.com/jobs/~021843801370397066465 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @dukenv0307 ( |
@bernhardoj Thanks for your proposal, do you know why this issue doesn't happen on web? |
This happen on web too. web.mp4 |
@stephanieelliott, @dukenv0307 Huh... This is 4 days overdue. Who can take care of this? |
Hey @dukenv0307 were you able to review the proposal here? Any thoughts on the solution that @bernhardoj is proposing? |
I'm reviewing... |
Checking the focused route is not a good idea. We can update |
We can definitely do that, but App/src/libs/actions/Welcome/index.ts Lines 41 to 54 in a333676
If we perform:
What happened is, that when we deep link to A, page A is open, and App/src/libs/actions/Report.ts Lines 2714 to 2718 in a333676
It's actually a different issue with how we handle the deep link, but it shows that the Do you have any concerns with checking the focused route? |
ProposalPlease re-state the problem that we are trying to solve in this issue.User remains on the same page. What is the root cause of that problem?When users open App from Deeplink, the logic here is executed Lines 208 to 210 in ec3e940
That should execute the startOnboarding flow App/src/libs/actions/Report.ts Lines 2717 to 2721 in bad93d5
But it’s not triggered since App/src/libs/Navigation/Navigation.ts Line 393 in ec3e940
That means App/src/libs/Navigation/Navigation.ts Line 375 in ec3e940
Unfortunately, the
And we get And What changes do you think we should make in order to solve the problem?We have the same place to resetState in
And the
Or just add What alternative solutions did you explore? (Optional)
App/src/libs/Navigation/Navigation.ts Line 392 in ec3e940
Because
|
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
Thanks for your proposals. @bernhardoj Using @truph01 explains the details and correct RCA. We can go with @truph01's main solution. 🎀👀🎀 C+ reviewed |
Triggered auto assignment to @lakchote, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
📣 @dukenv0307 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app! |
📣 @truph01 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
@lakchote @stephanieelliott @dukenv0307 @truph01 this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks! |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.51-4 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2024-10-29. 🎊 For reference, here are some details about the assignees on this issue:
|
BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
|
BugZero Checklist:
Regression test:
Do we 👍 or 👎 |
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Version Number: v9.0.42-3
Reproducible in staging?: Y
Reproducible in production?: Y
Issue reported by: Applause Internal Team
Action Performed:
Expected Result:
The next step of onboarding flow should be displayed.
Actual Result:
User remains on the same page.
Workaround:
Unknown
Platforms:
Screenshots/Videos
Bug6622099_1727873782030.Record_2024-10-02-14-55-13.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @stephanieelliottThe text was updated successfully, but these errors were encountered: