[HOLD for payment 2024-11-29] [Regression] [$125] Search - Onboarding is dismissed after using search

[IuliiaHerets]

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.50-0
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers): dave0123seife@gmail.com
Issue reported by: Applause Internal Team

Action Performed:

1. Login with a new user
2. When the Onboarding page is shown press on cmd + k
3. Click on self chat (you)

Expected Result:

Onboarding is not dismissed

Actual Result:

Onboarding is dismissed and chat is opened.

Workaround:

Unknown

Platforms:

• Android: Standalone
• Android: HybridApp
• Android: mWeb Chrome
• iOS: Standalone
• iOS: HybridApp
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Bug6637238_1729137558420.Screen_Recording_2024-10-17_at_6.58.02_in_the_morning.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021846862688780448155
• Upwork Job ID: 1846862688780448155
• Last Price Increase: 2024-11-29
• Automatic offers:
• dukenv0307 | Reviewer | 104562576
• daledah | Contributor | 104562577

Issue Owner

Current Issue Owner: @stitesExpensify

[melvin-bot]

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

[IuliiaHerets]

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

[daledah]

Edited by proposal-police: This proposal was edited at 2024-10-17 08:35:35 UTC.

Proposal

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

Onboarding is dismissed and chat is opened.

What is the root cause of that problem?

We don't have option to prevent user from opening the search modal

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

Lines 358 to 360 in a0ebba0

	Session.checkIfActionIsAllowed(() => {
	toggleSearchRouter();
	})();

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

we should add the condition to check if isOnboardingCompleted is false or not

    const isOnboardingCompletedRef = useRef(isOnboardingCompleted)

    useEffect(()=>{
        isOnboardingCompletedRef.current=isOnboardingCompleted
    },[isOnboardingCompleted])
...
Session.checkIfActionIsAllowed(() => {
                    isOnboardingCompletedRef.current && toggleSearchRouter();
                })();

What alternative solutions did you explore? (Optional)

We can update checkIfActionIsAllowed function to return () => void if onboarding flow is not completed

We should consider other keyboard shortcut too

[melvin-bot]

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

[melvin-bot]

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

[Krishna2323]

Edited by proposal-police: This proposal was edited at 2024-10-17 14:08:14 UTC.

Proposal


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

Search - Onboarding is dismissed after using search

What is the root cause of that problem?

• We aren't preventing users from opening search modal and other modals.

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

Lines 306 to 385 in 62302d5

	const unsubscribeOnyxModal = onyxSubscribe({
	key: ONYXKEYS.MODAL,
	callback: (modalArg) => {
	if (modalArg === null || typeof modalArg !== 'object') {
	return;
	}
	modal.current = modalArg;
	},
	});
	const shortcutConfig = CONST.KEYBOARD_SHORTCUTS.ESCAPE;
	const unsubscribeEscapeKey = KeyboardShortcut.subscribe(
	shortcutConfig.shortcutKey,
	() => {
	if (modal.current.willAlertModalBecomeVisible) {
	return;
	}
	if (modal.current.disableDismissOnEscape) {
	return;
	}
	Navigation.dismissModal();
	},
	shortcutConfig.descriptionKey,
	shortcutConfig.modifiers,
	true,
	true,
	);
	// Listen to keyboard shortcuts for opening certain pages
	const unsubscribeShortcutsOverviewShortcut = KeyboardShortcut.subscribe(
	shortcutsOverviewShortcutConfig.shortcutKey,
	() => {
	Modal.close(() => {
	if (Navigation.isActiveRoute(ROUTES.KEYBOARD_SHORTCUTS)) {
	return;
	}
	return Navigation.navigate(ROUTES.KEYBOARD_SHORTCUTS);
	});
	},
	shortcutsOverviewShortcutConfig.descriptionKey,
	shortcutsOverviewShortcutConfig.modifiers,
	true,
	);
	// Listen for the key K being pressed so that focus can be given to
	// Search Router, or new group chat
	// based on the key modifiers pressed and the operating system
	const unsubscribeSearchShortcut = KeyboardShortcut.subscribe(
	searchShortcutConfig.shortcutKey,
	() => {
	Session.checkIfActionIsAllowed(() => {
	toggleSearchRouter();
	})();
	},
	shortcutsOverviewShortcutConfig.descriptionKey,
	shortcutsOverviewShortcutConfig.modifiers,
	true,
	);
	const unsubscribeChatShortcut = KeyboardShortcut.subscribe(
	chatShortcutConfig.shortcutKey,
	() => {
	Modal.close(Session.checkIfActionIsAllowed(() => Navigation.navigate(ROUTES.NEW)));
	},
	chatShortcutConfig.descriptionKey,
	chatShortcutConfig.modifiers,
	true,
	);
	const unsubscribeDebugShortcut = KeyboardShortcut.subscribe(
	debugShortcutConfig.shortcutKey,
	() => {
	toggleTestToolsModal();
	},
	debugShortcutConfig.descriptionKey,
	debugShortcutConfig.modifiers,
	true,
	);

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


• We can use the isOnboardingCompleted variable to check if the onboarding process is completed or not and if it not completed then we can early return without opening the modal.
  

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

  Line 241 in 62302d5

  const {isOnboardingCompleted} = useOnboardingFlowRouter();

NOTE: We might not want to disabled all the modals so it can be discussed in the PR.

What alternative solutions did you explore? (Optional)

• We can also import OnboardingRefManager and call OnboardingRefManager.handleOuterClick(); when a shortcut is pressed and the onboarding flow is not completed.

    // We can also use useCallback and the call handleOuterClick
    const handleOuterClick = useCallback(() => {
        OnboardingRefManager.handleOuterClick();
    }, []);

What alternative solutions did you explore? (Optional 2)

• We can use useKeyboardShortcut for every keyboard shortcuts and call handleOuterClick like we do for escape key.
  

  App/src/libs/Navigation/AppNavigator/Navigators/OnboardingModalNavigator.tsx

  Lines 27 to 31 in 62302d5

  	const handleOuterClick = useCallback(() => {
  	OnboardingRefManager.handleOuterClick();
  	}, []);
  	useKeyboardShortcut(CONST.KEYBOARD_SHORTCUTS.ESCAPE, handleOuterClick, {shouldBubble: true});

Result

[Krishna2323]

PROPOSAL UPDATED

• Added alternative

[Krishna2323]

PROPOSAL UPDATED

• Added alternative 2

[Anaslancer]

Proposal

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

Onboarding is dismissed and chat is opened.

What is the root cause of that problem?

Seems like there is no logic for checking function if user selected one option at the onboarding modal or not, before opening the Chat Finder modal (Ctrl + K).
But your team made the logic for Esc button at here.

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

We should make the logic in here.
We can use this value, isOnboardingCompleted for checking.

What alternative solutions did you explore? (Optional)

We should add this logic for the New Chat modal (Ctrl + Shift + K) at here for solving the other error.

Contributor details

Expensify account email: anasup1995@gmail.com
Upwork Profile Link: https://www.upwork.com/freelancers/~01aff093c9a804b145

[melvin-bot]

📣 @Anaslancer! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Make sure you've read and understood the contributing guidelines.
2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[dukenv0307]

@daledah's main proposal looks good to me.
Using isOnboardingCompleted is not enough and we shouldn't open search modal only when onboarding flow is not completed.

🎀👀🎀 C+ reviewed

[melvin-bot]

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

[Krishna2323]

@dukenv0307, is isOnboardingCompleted not working? When I tested, it was working as expected.

[Anaslancer]

we shouldn't open search modal only when onboarding flow is not completed.

I think this is the best workflow;
when onboarding flow is not completed, -> the modal shouldn't be opened.
when onboarding flow is completed, -> the modal should be opened.
We can use isOnboardingCompleted for this case.
so the callback function will have this code;

if (!isOnboardingCompleted) {
    return;
}

[dukenv0307]

@Krishna2323 @Anaslancer did you open search page after completing the onboarding flow?

[Anaslancer]

@dukenv0307 Yes, It works fine on my side after adding the logic

[dukenv0307]

@Anaslancer yah, but we have to pass the value to dependencies and re-create the listener, right?

[Anaslancer]

@dukenv0307 No, Just add the above 3 lines into here. That's it.

[Anaslancer]

@dukenv0307 It's just for the Ctrl+K dialog, we should fix for the Ctrl+Shift+K modal as well.

[dukenv0307]

@Anaslancer I can't understand why your solution can work if you don't pass isOnboardingCompleted to useEffect's dependency.

Here's the result

web-resize.mp4

If you want to use isOnboardingCompleted, we have to add isOnboardingCompleted in here to call unsubscribeSearchShortcut and re-subscribe it again.

But it can cause other listeners to unsubscribe and re-subscribe again.

-> We can separate it in other hook

Overall, using isOnboardingCompletedRef can make code cleaner and simpler to me

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.65-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• fix: prevent open search on onboarding route #52534

If no regressions arise, payment will be issued on 2024-11-29. 🎊

For reference, here are some details about the assignees on this issue:

• @dukenv0307 requires payment automatic offer (Reviewer)
• @daledah requires payment automatic offer (Contributor)

[melvin-bot]

@dukenv0307 @sonialiap @dukenv0307 The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

[melvin-bot]

Upwork job price has been updated to $125

[sonialiap]

Payment summary:
Due to regression, decreasing the payment from 250 to 125

• @dukenv0307 $125, please complete the checklist - payment via ND
• @daledah $125 paid via upwork ✔️

[melvin-bot]

@sonialiap, @stitesExpensify, @dukenv0307, @daledah Whoops! This issue is 2 days overdue. Let's get this updated quick!

[sonialiap]

@dukenv0307 bumping for the checklist #50981 (comment)

[stitesExpensify]

Not overdue, waiting on @dukenv0307

[dukenv0307]

BugZero Checklist:

• [Contributor] Classify the bug:

Bug classification

Source of bug:

• 1a. Result of the original design (eg. a case wasn't considered)
• 1b. Mistake during implementation
• 1c. Backend bug
• 1z. Other:

Where bug was reported:

• 2a. Reported on production
• 2b. Reported on staging (deploy blocker)
• 2c. Reported on a PR
• 2z. Other:

Who reported the bug:

• 3a. Expensify user
• 3b. Expensify employee
• 3c. Contributor
• 3d. QA
• 3z. Other:

• [Contributor] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake.

  Link to comment: https://github.com/Expensify/App/pull/49984/files#r1868668182

• [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner.

  Link to discussion: N/A

• [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again. Yes

Regression Test Proposal Template

• [BugZero Assignee] Create a GH issue for creating/updating the regression test once above steps have been agreed upon.

  Link to issue: https://github.com/Expensify/Expensify/issues/450237

Regression Test Proposal

Test:

1. Sign in with a new account
2. On onboarding step, try to press Cmd + K
3. Verify that: the search modal is not opened.

Do we agree 👍 or 👎

[dukenv0307]

@sonialiap We're good to go. Thank you

[sonialiap]

Thanks @dukenv0307!

[JmillsExpensify]

$125 approved for @dukenv0307