Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.56k stars 2.9k forks source link

[$250] Onboarding - The app freezes if a FAB menu is clicked before the onboarding video shows #46531

Closed lanitochka17 closed 2 months ago

lanitochka17 commented 3 months ago

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: 9.0.14-2 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Issue reported by: Applause - Internal Team

Action Performed:

  1. Navigate to https://staging.new.expensify.com/
  2. Log in with a new Gmail account
  3. Input anything for "First name" and click on the "Continue" button
  4. Quickly click on the FAB and on any button from the dropdown menu
  5. Click anywhere to close the RHP
  6. Click on any interactable element

Expected Result:

I should be able to interach with the app

Actual Result:

The app freezes if a FAB menu is quickly clicked before the onboarding video shows

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/520f6634-bc2d-4d98-873c-969380bcd6d4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01ab200147805fc6ce
  • Upwork Job ID: 1818775055281225355
  • Last Price Increase: 2024-08-21
Issue OwnerCurrent Issue Owner: @MonilBhavsar
melvin-bot[bot] commented 3 months ago

Triggered auto assignment to @mallenexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

lanitochka17 commented 3 months ago

@mallenexpensify 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

melvin-bot[bot] commented 3 months ago

Job added to Upwork: https://www.upwork.com/jobs/~01ab200147805fc6ce

melvin-bot[bot] commented 3 months ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @eVoloshchak (External)

mallenexpensify commented 3 months ago

Able to repro, def seems like something we want to fix. You have to click the FAB then another button quickly so it's def an edge case

@eVoloshchak , comment if you don't think this can be external. Thx

https://github.com/user-attachments/assets/82dc6975-91fd-401d-8c11-af8636eba89c

ravindra-encoresky commented 3 months ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

On onboarding if we click FAB button and select an option before welcome video, app freezes.

What is the root cause of that problem?

Welcome video modal not really dismissing due to change in routes before it show. Because we are using Navigation.goBack() to dismiss it.

What changes do you think we should make in order to solve the problem?

On closeModal function of FeatureTrainingModal we are just using Navigation.goBack(). This works fine if there is no change in navigation before modal shows https://github.com/Expensify/App/blob/d69b203c298a879e609ebff4be249f550456f98e/src/components/FeatureTrainingModal.tsx#L168

but if there is change in navigation before FeatureTrainingModal show Navigation.goBack() not really dismiss the modal. we need to dismiss modal using dismissModal global function.

 const closeModal = useCallback(() => {
        if (!willShowAgain) {
            User.dismissTrackTrainingModal();
        }
        setIsModalVisible(false);
        Navigation.goBack();
        Navigation.dismissModal(); // we need to add this
    }, [willShowAgain]);

Also need to add navigator in global dismiss modal function dismissModal.ts

case NAVIGATORS.WELCOME_VIDEO_MODAL_NAVIGATOR:

https://github.com/user-attachments/assets/9b5d5785-5f41-4630-864f-9fafafc2f77e

What alternative solutions did you explore? (Optional)

Suggestion - 2 seconds delay in showing welcome video modal causing others issues as well. If it is not very very necessary we can consider removing it

ravindra-encoresky commented 3 months ago

@eVoloshchak @mallenexpensify @lanitochka17 please have a look at the above proposal. thanks.

melvin-bot[bot] commented 3 months ago

@eVoloshchak, @mallenexpensify Huh... This is 4 days overdue. Who can take care of this?

ravindra-encoresky commented 3 months ago

hi @eVoloshchak @mallenexpensify please have a look at the above proposal.

eVoloshchak commented 3 months ago

@ravindra-encoresky's proposal looks good to me!

🎀👀🎀 C+ reviewed!

2 seconds delay in showing welcome video modal causing others issues as well. If it is not very very necessary we can consider removing it

Could you please elaborate on what other issues it is causing?

melvin-bot[bot] commented 3 months ago

Triggered auto assignment to @MonilBhavsar, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

melvin-bot[bot] commented 3 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

ravindra-encoresky commented 3 months ago

Could you please elaborate on what other issues it is causing?

if we navigate to profile tab before welcome video modal appear, we get this issue https://github.com/Expensify/App/issues/46548 also in current issue if we navigate to menu in fab button.

There are possibilities of other issues if navigate to other screens before welcome video modal appears.

ravindra-encoresky commented 3 months ago

@MonilBhavsar please have a look at the above proposal. thanks.

trjExpensify commented 3 months ago

@anmurali @danielrvidal aren't we going to kill this welcome video in favour of videos in setup tasks? Trying to assess whether this is worth fixing.

ravindra-encoresky commented 3 months ago

@trjExpensify so what's for me in this now ?

MonilBhavsar commented 3 months ago

Waiting on clarification https://github.com/Expensify/App/issues/46531#issuecomment-2276938077 if it's worth fixing

danielrvidal commented 3 months ago

Asking for clarification here, I'll report back when I have a direction

https://expensify.enterprise.slack.com/archives/C036QM0SLJK/p1723511235420929

melvin-bot[bot] commented 3 months ago

@eVoloshchak @mallenexpensify @MonilBhavsar 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!

melvin-bot[bot] commented 3 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 3 months ago

@eVoloshchak, @mallenexpensify, @MonilBhavsar Whoops! This issue is 2 days overdue. Let's get this updated quick!

mallenexpensify commented 3 months ago

@danielrvidal what's the take away from the (long ass) thread discussion?

MonilBhavsar commented 3 months ago

Latest^

MonilBhavsar commented 3 months ago

We're still discussing if we want to fix it cc @danielrvidal

danielrvidal commented 3 months ago

I'm not sure, I think we're still planning to remove it but I asked in that thread.

melvin-bot[bot] commented 2 months ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 2 months ago

@eVoloshchak, @mallenexpensify, @MonilBhavsar Whoops! This issue is 2 days overdue. Let's get this updated quick!

MonilBhavsar commented 2 months ago

Yea, I think we’d remove it and see then measure if more people are then engaging with the onboarding tasks.

https://expensify.slack.com/archives/C036QM0SLJK/p1724187631995739?thread_ts=1723511235.420929&cid=C036QM0SLJK

It does seem like we are removing the video and testing. So we can close it for now. cc @danielrvidal @mallenexpensify