[$250] Fix onboarding modal in macOS Chrome/Safari

[carlosmiceli]

The onboarding modal seems to have issues being displayed in macOS in Chrome and Safari (conversation, GH comment). Seems like there is an issue with screen size and/or refresh. Please fix it and ensure it's always displayed in all possible platforms. We found this while working on this issue: #53326

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021865160679237341916
• Upwork Job ID: 1865160679237341916
• Last Price Increase: 2024-12-06

Issue Owner

Current Issue Owner: @hungvu193

[melvin-bot]

Triggered auto assignment to @slafortune (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.

[melvin-bot]

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

[melvin-bot]

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

[Shahidullah-Muffakir]

Coming from #53326 (comment). I’ll work on this, thanks!

[melvin-bot]

📣 @Shahidullah-Muffakir You have been assigned to this job!
Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs!
Keep in mind: Code of Conduct | Contributing 📖

[Shahidullah-Muffakir]

@carlosmiceli as per #53284 (comment) and after testing with same steps, the onboarding modal works as expected in staging when signing up through staging.expensify.com, after redirecting to staging.new.expensify.com the onboarding flow modal appears as expected. However, the issue persists in prod.
Considering this, should we wait for the next production deploy to see if the issue resolves? Thank you.

[carlosmiceli]

should we wait for the next production deploy to see if the issue resolves?

Yes, that sounds like a good idea.

[carlosmiceli]

Not overdue.

[melvin-bot]

@carlosmiceli, @slafortune, @hungvu193, @Shahidullah-Muffakir Whoops! This issue is 2 days overdue. Let's get this updated quick!

[carlosmiceli]

Still discussing details in the PR.

[carlosmiceli]

@Shahidullah-Muffakir is this still occurring?

[melvin-bot]

@carlosmiceli, @slafortune, @hungvu193, @Shahidullah-Muffakir Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[hungvu193]

little bump @Shahidullah-Muffakir

[Shahidullah-Muffakir]

@Shahidullah-Muffakir is this still occurring?

Yes, still occurring.

[Shahidullah-Muffakir]

little bump @Shahidullah-Muffakir

PR will be ready by tomorrow.

[Shahidullah-Muffakir]

Proposal

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

Fix onboarding modal in macOS Chrome/Safari.

What is the root cause of that problem?

1. In the hasCompletedGuidedSetupFlowSelector function, when the onboardingFlow object is empty or hasCompletedGuidedSetupFlowSelector is undefined, we are returning true:
   

   App/src/libs/onboardingSelectors.ts

   Lines 14 to 20 in 1466c02

   	if (Array.isArray(onboarding) || isEmptyObject(onboarding)) {
   	return true;
   	}
   	if (!isEmptyObject(onboarding) && onboarding?.hasCompletedGuidedSetupFlow === undefined) {
   	return true;
   	}

2. This results in the useOnboardingFlowRouter hook returning true:
   

   App/src/libs/Navigation/AppNavigator/AuthScreens.tsx

   Line 243 in 1466c02

   const {isOnboardingCompleted} = useOnboardingFlowRouter();

   
   and as the isOnboardingCompleted is true, this condition fails:
   

   App/src/libs/Navigation/AppNavigator/AuthScreens.tsx

   Lines 538 to 542 in 1466c02

   	{isOnboardingCompleted === false && (
   	<RootStack.Screen
   	name={NAVIGATORS.ONBOARDING_MODAL_NAVIGATOR}
   	options={{...rootNavigatorOptions.basicModalNavigator, gestureEnabled: false}}
   	component={OnboardingModalNavigator}

   
   Hence, the Onboarding Modal is not shown.

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

The same way we return false when the onboarding object is empty in the case of:

App/src/libs/onboardingSelectors.ts

Lines 51 to 54 in 1466c02

	function hasSeenTourSelector(onboarding: OnyxValue<typeof ONYXKEYS.NVP_ONBOARDING>): boolean | undefined {
	if (Array.isArray(onboarding) || isEmptyObject(onboarding)) {
	return false;
	}

,
we should return false in the case of hasCompletedGuidedSetupFlowSelector as well if the onboarding is undefined.
as:

function hasCompletedGuidedSetupFlowSelector(onboarding: OnyxValue<typeof ONYXKEYS.NVP_ONBOARDING>): boolean | undefined {
    // Onboarding is an empty object for old accounts and accounts migrated from OldDot
    if (Array.isArray(onboarding) || isEmptyObject(onboarding)) {
-         return true;
+        return false;
    }

    if (!isEmptyObject(onboarding) && onboarding?.hasCompletedGuidedSetupFlow === undefined) {
-        return true;
+       return false;
    }

@hungvu193 Before making the PR, could you review the changes I proposed, thanks.

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

We need to updated these tests as well, to return false instead of true:

App/tests/unit/OnboardingSelectorsTest.ts

Lines 10 to 22 in 1466c02

	// It might be the case that backend returns an empty array if the NVP is not defined on this particular account
	it('Should return true if onboarding NVP is an array', () => {
	const onboarding = [] as OnyxValue<typeof ONYXKEYS.NVP_ONBOARDING>;
	expect(hasCompletedGuidedSetupFlowSelector(onboarding)).toBe(true);
	});
	it('Should return true if onboarding NVP is an empty object', () => {
	const onboarding = {} as OnyxValue<typeof ONYXKEYS.NVP_ONBOARDING>;
	expect(hasCompletedGuidedSetupFlowSelector(onboarding)).toBe(true);
	});
	it('Should return true if onboarding NVP contains only signupQualifier', () => {
	const onboarding = {signupQualifier: CONST.ONBOARDING_SIGNUP_QUALIFIERS.VSB} as OnyxValue<typeof ONYXKEYS.NVP_ONBOARDING>;
	expect(hasCompletedGuidedSetupFlowSelector(onboarding)).toBe(true);
	});

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[mountiny]

@Shahidullah-Muffakir We cannot return false there, its intentional that we do not show the onboarding modal for users that do not have the nvp or of the nvp is an empty object

[Shahidullah-Muffakir]

We cannot return false there, its intentional that we do not show the onboarding modal for users that do not have the nvp or of the nvp is an empty object

Got it. Thanks for confirming

[mountiny]

Lets close this in favour of #54322