[$1000] Deeplink - No prompt to open in app when navigating to deep link as logged-out web user

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Issue found when executing PR #24638

Action Performed:

Go to staging.new.expensify.com
As signed in user on web, copy code to room and paste it on web browser and navigate to the link
As signed out user on web, repeat Step 2

Expected Result:

Whether user is signed in or signed out on web browser, navigate to new expensify deeplink on web should show a prompt to open ND in app

Actual Result:

When user is signed out on web and navigates to deep link, there is no prompt to open in desktop app

Workaround:

Unknown

Platforms:

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

Version Number: 1.3.56-18

Reproducible in staging?: Yes

Reproducible in production?: No

If this was caught during regression testing, add the test name, ID and link from TestRail:

Email or phone of affected tester (no customers):

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

Bug6174773_20230823_221451.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01f534d2e9aa904030
Upwork Job ID: 1694458731220623360
Last Price Increase: 2023-08-30

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

Identify the pull request that introduced this issue and revert it.
Find someone who can quickly fix the issue.
Fix the issue yourself.

@yuwenmemon

Triggered auto assignment to @yuwenmemon (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

I QA'd #24638 - it worked fine and I don't believe what we're seeing here is a deploy blocker. I'm going to reassign this to the bug zero team for triage.

@ArekChr

@ArekChr I would think we wouldn't want to close this, no? It seems like a bug that we're not triggering the desktop prompt when logged out on staging.

Sure, I thought that was intentional.

Proposal

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

When a user is signed out on the web and attempts to navigate to a deep link, there is no prompt to open the link in the desktop app.

What is the root cause of that problem?

The root cause of this problem is that we are not handling deep links when users are logged out.
You can find the relevant code snippet here:

App/src/components/DeeplinkWrapper/index.website.js

Lines 78 to 94 in 34d4e54

    
           if (isAuthenticated) { 
        
               promptToOpenInDesktopApp(); 
        
               setHasShownPrompt(true); 
        
           } else { 
        
               // Navigation state is not set up yet, we're unsure if we should show the deep link prompt or not 
        
               if (currentScreen === undefined || isAuthenticated === false) { 
        
                   return; 
        
               } 
        
               const preventPrompt = shouldPreventDeeplinkPrompt(currentScreen); 
        
               if (preventPrompt === true) { 
        
                   return; 
        
               } 
        
               promptToOpenInDesktopApp(); 
        
               setHasShownPrompt(true); 
        
           }

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

To address this issue, I recommend the following changes:

Update Conditional Expression
We need to modify the code by removing the conditional expression that checks for isAuthenticated === false. Since the existing else statement implies that isAuthenticated is false, the conditional is unnecessary.

App/src/components/DeeplinkWrapper/index.website.js

Line 83 in 34d4e54

if (currentScreen === undefined || isAuthenticated === false) {

Adjust Denylist for Opening Desktop App
We need to modify the code by either removing HOME from the DEEPLINK_PROMPT_DENYLIST, or by incorporating the condition navigationRef.current.getState().routes.length === 1. This indicates that these changes are specifically related to the DEEPLINK and do not affect HOME. This condition establishes that if the route length is greater than 1, it signifies a deep link; otherwise, it is not a deep link. These changes are aimed at enabling the desktop app to show a prompt for opening when a user, logged out on the web, navigates to a deep link.

App/src/components/DeeplinkWrapper/index.website.js

Lines 87 to 88 in 34d4e54

    
           const preventPrompt = shouldPreventDeeplinkPrompt(currentScreen); 
        
           if (preventPrompt === true) {

App/src/CONST.js

Line 444 in 34d4e54

    
           DEEPLINK_PROMPT_DENYLIST: [SCREENS.HOME, SCREENS.SIGN_IN_WITH_APPLE_DESKTOP, SCREENS.SIGN_IN_WITH_GOOGLE_DESKTOP],

Modification of Conditional Code Block Adjust the code as follows:

if (currentScreen === undefined) {
  return;
}

const preventPrompt = shouldPreventDeeplinkPrompt(currentScreen);

// navigationRef.current is not null as currentScreen is not undefined. Therefore, we can utilize navigationRef.current.
if (preventPrompt === true && navigationRef.current.getState().routes.length === 1) {
  return;
}

This modification ensures proper handling of the prompt prevention based on the conditions mentioned.

What alternative solutions did you explore? (Optional)

We can remove HOME from the DEEPLINK_PROMPT_DENYLIST, but this might disrupt the original functionality.

P.S.

The existing solution has a potential problem of causing an infinite loop due to a specific section of code. This issue arises from the usage of the useEffect hooks in certain parts of the codebase, where each hook has a dependency on the hasShownPrompt variable. Consequently, when any of these dependencies change, it triggers the useEffect hooks and leads to an infinite loop.

App/src/components/DeeplinkWrapper/index.website.js

Lines 44 to 95 in 34d4e54

    
           useEffect(() => { 
        
               // If we've shown the prompt and still have a listener registered, 
        
               // remove the listener and reset its ref to undefined 
        
               if (hasShownPrompt && removeListener.current !== undefined) { 
        
                   removeListener.current(); 
        
                   removeListener.current = undefined; 
        
               } 
        
               if (isAuthenticated === false) { 
        
                   setHasShownPrompt(false); 
        
                   Navigation.isNavigationReady().then(() => { 
        
                       // Get initial route 
        
                       const initialRoute = navigationRef.current.getCurrentRoute(); 
        
                       setCurrentScreen(initialRoute.name); 
        
                       removeListener.current = navigationRef.current.addListener('state', (event) => { 
        
                           setCurrentScreen(Navigation.getRouteNameFromStateEvent(event)); 
        
                       }); 
        
                   }); 
        
               } 
        
           }, [hasShownPrompt, isAuthenticated]); 
        
           useEffect(() => { 
        
               // According to the design, we don't support unlink in Desktop app https://github.com/Expensify/App/issues/19681#issuecomment-1610353099 
        
               const isUnsupportedDeeplinkRoute = _.some([CONST.REGEX.ROUTES.UNLINK_LOGIN], (unsupportRouteRegex) => { 
        
                   const routeRegex = new RegExp(unsupportRouteRegex); 
        
                   return routeRegex.test(window.location.pathname); 
        
               }); 
        
               // Making a few checks to exit early before checking authentication status 
        
               if (!isMacOSWeb() || isUnsupportedDeeplinkRoute || CONFIG.ENVIRONMENT === CONST.ENVIRONMENT.DEV || hasShownPrompt) { 
        
                   return; 
        
               } 
        
               // We want to show the prompt immediately if the user is already authenticated. 
        
               // Otherwise, we want to wait until the navigation state is set up 
        
               // and we know the user is on a screen that supports deeplinks. 
        
               if (isAuthenticated) { 
        
                   promptToOpenInDesktopApp(); 
        
                   setHasShownPrompt(true); 
        
               } else { 
        
                   // Navigation state is not set up yet, we're unsure if we should show the deep link prompt or not 
        
                   if (currentScreen === undefined || isAuthenticated === false) { 
        
                       return; 
        
                   } 
        
                   const preventPrompt = shouldPreventDeeplinkPrompt(currentScreen); 
        
                   if (preventPrompt === true) { 
        
                       return; 
        
                   } 
        
                   promptToOpenInDesktopApp(); 
        
                   setHasShownPrompt(true); 
        
               } 
        
           }, [currentScreen, hasShownPrompt, isAuthenticated]);

App/src/components/DeeplinkWrapper/index.website.js

Line 53 in 34d4e54

setHasShownPrompt(false);

App/src/components/DeeplinkWrapper/index.website.js

Line 80 in 34d4e54

setHasShownPrompt(true);

To address this problem, two potential solutions have been identified:

Option 1: Removing a specific line of code
In this option, the problematic line in the codebase is removed. However, it's worth noting that this action could potentially disrupt the logic, which is designed to restrict the opening of the desktop app to a single instance.

App/src/components/DeeplinkWrapper/index.website.js

Line 80 in 34d4e54

setHasShownPrompt(true);

Result:

expensify.issue.1.mp4

Option 2: Removing another specific line of code
Alternatively, this option suggests removing a different line of code. Similarly, this approach might impact the logic that allows the opening of the desktop app after a user has logged in.

App/src/components/DeeplinkWrapper/index.website.js

Line 53 in 34d4e54

setHasShownPrompt(false);

Result:

expensify.issue.2.mp4

Ultimately, the chosen option will depend on the priorities of the project and the desired behavior of the application.

Proposal

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

No prompt to open in app when navigating to deep link as logged-out web user

What is the root cause of that problem?

App/src/components/DeeplinkWrapper/index.website.js

Line 83 in 34d4e54

if (currentScreen === undefined || isAuthenticated === false) {

App/src/components/DeeplinkWrapper/index.website.js

Line 52 in 34d4e54

if (isAuthenticated === false) {

Both places we are blocking for non-auth people

This Pr is add this #23673 when social sign-in is enabled

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

we should remove isAuthenticated === false both places and
we should remove SCREENS.HOME as well because SCREENS.HOME is a root path for this allow the path

 const preventPrompt = shouldPreventDeeplinkPrompt(currentScreen);

and
we should update

App/src/components/DeeplinkWrapper/index.website.js

Line 52 in 34d4e54

if (isAuthenticated === false) {

    + if (!hasShownPrompt) { // update this one
   - setHasShownPrompt(false); and remove this
            Navigation.isNavigationReady().then(() => {
                // Get initial route
                const initialRoute = navigationRef.current.getCurrentRoute();
                setCurrentScreen(initialRoute.name);

                removeListener.current = navigationRef.current.addListener('state', (event) => {
                    setCurrentScreen(Navigation.getRouteNameFromStateEvent(event));
                });
            });
        }

App/src/components/DeeplinkWrapper/index.website.js

Line 64 in 34d4e54

}, [hasShownPrompt, isAuthenticated]);

because when ever hasShownPrompt changes and isAuthenticated === false means it will update again hasShownPrompt as false and again it will ask the popup to open desktop so it will goes to loop.

@ArekChr

Sure, I thought that was intentional.

@ArekChr did you see discussion that said this somewhere? The reason I thought this was not intentional is that when I was testing it the other day, we were not showing the "switch to desktop" prompt to non-authenticated users on staging, but we were on production.

That inconsistency made me think this was likely a regression introduced at recently (unless we made the active decision to change this somewhere).

Not overdue

@ArekChr

@ArekChr mind sharing your thoughts on this when you have a sec?

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

Still pending C+ review

@joekaufmanexpensify

@joekaufmanexpensify It Looks like this functionality is intended. There were discussions about not prompting redirects when users are logged out on the web app, a related thread here
cc: @cdanwards

Ah cool. Yep, confirmed here. Closing as this is intended!

@kbecciv

Hi @kbecciv I see you linked to this comment from here, mentioning that the PR wasn't passing tests. I think we are good now, but could you please just take a look to confirm? thanks!

Navigation Menu

Repository navigation

[$1000] Deeplink - No prompt to open in app when navigating to deep link as logged-out web user #25790

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

19 remaining items

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

P.S.

Proposal

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

What is the root cause of that problem?

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

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	if (isAuthenticated) {
	promptToOpenInDesktopApp();
	setHasShownPrompt(true);
	} else {
	// Navigation state is not set up yet, we're unsure if we should show the deep link prompt or not
	if (currentScreen === undefined \|\| isAuthenticated === false) {
	return;
	}

	const preventPrompt = shouldPreventDeeplinkPrompt(currentScreen);
	if (preventPrompt === true) {
	return;
	}

	promptToOpenInDesktopApp();
	setHasShownPrompt(true);
	}

	useEffect(() => {
	// If we've shown the prompt and still have a listener registered,
	// remove the listener and reset its ref to undefined
	if (hasShownPrompt && removeListener.current !== undefined) {
	removeListener.current();
	removeListener.current = undefined;
	}

	if (isAuthenticated === false) {
	setHasShownPrompt(false);
	Navigation.isNavigationReady().then(() => {
	// Get initial route
	const initialRoute = navigationRef.current.getCurrentRoute();
	setCurrentScreen(initialRoute.name);

	removeListener.current = navigationRef.current.addListener('state', (event) => {
	setCurrentScreen(Navigation.getRouteNameFromStateEvent(event));
	});
	});
	}
	}, [hasShownPrompt, isAuthenticated]);
	useEffect(() => {
	// According to the design, we don't support unlink in Desktop app https://github.com/Expensify/App/issues/19681#issuecomment-1610353099
	const isUnsupportedDeeplinkRoute = _.some([CONST.REGEX.ROUTES.UNLINK_LOGIN], (unsupportRouteRegex) => {
	const routeRegex = new RegExp(unsupportRouteRegex);
	return routeRegex.test(window.location.pathname);
	});
	// Making a few checks to exit early before checking authentication status
	if (!isMacOSWeb() \|\| isUnsupportedDeeplinkRoute \|\| CONFIG.ENVIRONMENT === CONST.ENVIRONMENT.DEV \|\| hasShownPrompt) {
	return;
	}
	// We want to show the prompt immediately if the user is already authenticated.
	// Otherwise, we want to wait until the navigation state is set up
	// and we know the user is on a screen that supports deeplinks.
	if (isAuthenticated) {
	promptToOpenInDesktopApp();
	setHasShownPrompt(true);
	} else {
	// Navigation state is not set up yet, we're unsure if we should show the deep link prompt or not
	if (currentScreen === undefined \|\| isAuthenticated === false) {
	return;
	}

	const preventPrompt = shouldPreventDeeplinkPrompt(currentScreen);
	if (preventPrompt === true) {
	return;
	}

	promptToOpenInDesktopApp();
	setHasShownPrompt(true);
	}
	}, [currentScreen, hasShownPrompt, isAuthenticated]);

[$1000] Deeplink - No prompt to open in app when navigating to deep link as logged-out web user #25790

Description

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Activity

OSBotify commented on Aug 23, 2023

melvin-bot commented on Aug 23, 2023

yuwenmemon commented on Aug 23, 2023

19 remaining items

joekaufmanexpensify commented on Aug 24, 2023

ArekChr commented on Aug 25, 2023

napster125 commented on Aug 25, 2023

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

P.S.

pradeepmdk commented on Aug 25, 2023

Proposal

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

What is the root cause of that problem?

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

joekaufmanexpensify commented on Aug 25, 2023

joekaufmanexpensify commented on Aug 28, 2023

joekaufmanexpensify commented on Aug 29, 2023

melvin-bot commented on Aug 30, 2023

joekaufmanexpensify commented on Aug 30, 2023

ArekChr commented on Aug 31, 2023

joekaufmanexpensify commented on Aug 31, 2023

Julesssss commented on Jun 6, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions