[$1000] Android - Workspace - User is returned to 'Hmm... it's not here' page from workspace chat

[izarutskaya]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Open app from the deep link staging.new.expensify.com/r/saastr
2. Log in with a new account.
3. Swipe back to return to LHN.

Expected Result:

User is returned to LHN.

Actual Result:

User is returned to 'Hmm... it's not here' page.

Workaround:

Unknown

Platforms:

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

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.64-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:

Email or phone of affected tester (no customers):

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

Notes/Photos/Videos: Any additional supporting documentation

Bug6190352_26844_Android.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/~0148a4f4b16e9811e6
• Upwork Job ID: 1699443740675543040
• Last Price Increase: 2023-09-06

[melvin-bot]

Triggered auto assignment to @CortneyOfstad (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[melvin-bot]

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

[melvin-bot]

Current assignee @CortneyOfstad is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

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

[melvin-bot]

⚠️ Failed to update price automatically. The BZ team member will need to update the price manually in Upwork. cc @thienlnam

[dukenv0307]

Proposal

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

Android - Workspace - User is returned to 'Hmm... it's not here' page from workspace chat

What is the root cause of that problem?

We goBack to the sidebar screen with shouldPopToTop as true to clear all previous pages in the stack navigator

App/src/pages/home/ReportScreen.js

Line 165 in 1eafc7d

onNavigationMenuButtonClicked={() => Navigation.goBack(ROUTES.HOME, false, true)}

In goBack function, we only do this action if both shouldPopToTop and shouldPopAllStateOnUP are true

App/src/libs/Navigation/Navigation.js

Lines 105 to 111 in 1eafc7d

	if (shouldPopToTop) {
	if (shouldPopAllStateOnUP) {
	shouldPopAllStateOnUP = false;
	navigationRef.current.dispatch(StackActions.popToTop());
	return;
	}
	}

But shouldPopAllStateOnUP is only true when we don't start on the small screen the screen size is changed

App/src/libs/Navigation/NavigationRoot.js

Lines 60 to 72 in 1eafc7d

	useEffect(() => {
	if (firstRenderRef.current) {
	// we don't want to make the report back button go back to LHN if the user
	// started on the small screen so we don't set it on the first render
	// making it only work on consecutive changes of the screen size
	firstRenderRef.current = false;
	return;
	}
	if (!isSmallScreenWidth) {
	return;
	}
	Navigation.setShouldPopAllStateOnUP();
	}, [isSmallScreenWidth]);

That makes back button in Header doesn't work properly

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

We should update the condition here to clear all previous pages in the stack navigator if shouldPopToTop is true

if (shouldPopToTop || shouldPopAllStateOnUP) {
    if (shouldPopAllStateOnUP) {
        shouldPopAllStateOnUP = false;
    }
    navigationRef.current.dispatch(StackActions.popToTop());
    return;
}

App/src/libs/Navigation/Navigation.js

Lines 105 to 111 in 1eafc7d

	if (shouldPopToTop) {
	if (shouldPopAllStateOnUP) {
	shouldPopAllStateOnUP = false;
	navigationRef.current.dispatch(StackActions.popToTop());
	return;
	}
	}

What alternative solutions did you explore? (Optional)

Before we call goBack here we can call Navigation.setShouldPopAllStateOnUP(); to set shouldPopAllStateOnUP as true

App/src/pages/home/ReportScreen.js

Line 165 in 1eafc7d

onNavigationMenuButtonClicked={() => Navigation.goBack(ROUTES.HOME, false, true)}

[bernhardoj]

Proposal

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

Open r/saastr by deep link will open the demo chat but when going back, a not found page will be shown.

What is the root cause of that problem?

When we deep link to r/saastr, it will open a chat/report screen with the report ID of saastr. Because the report ID is invalid, we will have the not found page in the navigation stack.

Navigation stack: [LHN, Not found chat]

In addition to the not found chat, we will also be navigated to the demo chat (SAASTR). This is because inside runDemoByURL (which is called if there is a deep link), it checks whether the deep link URL ends with the demo chat routes.

App/src/libs/actions/DemoActions.js

Lines 67 to 86 in 1eafc7d

	function runDemoByURL(url = '') {
	const cleanUrl = (url || '').toLowerCase();
	if (cleanUrl.endsWith(ROUTES.SAASTR)) {
	Onyx.set(ONYXKEYS.DEMO_INFO, {
	saastr: {
	isBeginningDemo: true,
	},
	});
	} else if (cleanUrl.endsWith(ROUTES.SBE)) {
	Onyx.set(ONYXKEYS.DEMO_INFO, {
	sbe: {
	isBeginningDemo: true,
	},
	});
	} else {
	// No demo is being run, so clear out demo info
	Onyx.set(ONYXKEYS.DEMO_INFO, null);
	}
	}

In our case, it is true because r/saastr ends with SAASTR which sets the isBeginningDemo to true which in turn navigates us to the demo chat.

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

Lines 166 to 171 in c2dab6e

	// Check if we should be running any demos immediately after signing in.
	if (lodashGet(this.props.demoInfo, 'saastr.isBeginningDemo', false)) {
	Navigation.navigate(ROUTES.SAASTR, CONST.NAVIGATION.TYPE.FORCED_UP);
	} else if (lodashGet(this.props.demoInfo, 'sbe.isBeginningDemo', false)) {
	Navigation.navigate(ROUTES.SBE, CONST.NAVIGATION.TYPE.FORCED_UP);
	}

Navigation stack: [LHN, Not found chat, Demo chat (SAASTR)]

So, when we go back, we will arrive on the not found chat.

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

We shouldn't check whether the deep link URL ends with the demo route (SAASTR/SBE) because we can put anything in the URL and put SAASTR/SBE (e.g., /abcdefghsaastr) at the end of the link and our logic will think we want to navigate to demo chat. To have a better check of the route, we can first extract the path/route from the full deep link URL (using ReportUtils.getRouteFromLink).

From: new-expensify://r/saastr
To: r/saastr

Then, check if the route/path is equal to either SAASTR or SBE.

DemoActions

runDemoByURL(url = '') {
    const route = ReportUtils.getRouteFromLink((url || '').toLowerCase());

    if (route === ROUTES.SAASTR) // set the onyx
    else if (route === ROUTES.SBE) // set the onyx
    else ...
}

[abdulrahuman5196]

@mountiny Is this issue like a deploy blocker or something priority?

[mountiny]

@abdulrahuman5196 not a blocker, but would be good to get it merged tomorrow

[abdulrahuman5196]

It seems even in the video, NotFound page comes first then comes the Saastr page.

On @dukenv0307 's proposal here, it doesn't have information on why the NotFound page comes and stays in navigation on the first place, which I would like to investigate. The proposal targets to fix not showing the page on back press.

@bernhardoj You root cause is good. But the navigation is FORCED_UP type so it will replace the NotFound page right? So shouldn't it end up like Navigation stack: [LHN, Demo chat (SAASTR)]?

Navigation.navigate(ROUTES.SAASTR, CONST.NAVIGATION.TYPE.FORCED_UP);

And I am unable to understand the solution properly.

Kindly do let me know if I am missing something

[dukenv0307]

@abdulrahuman5196 The NotFoundPage comes first because the URL of the sbe and saastr report is incorrect. The URL '/r/sbe' is the URL of the ReportScreen so it knows sbe is the reportID and then not found page appears because the reportID is invalid.

[bernhardoj]

But the navigation is FORCED_UP type

Oh, you are right, I didn't notice this one. After doing some investigation, I found that the navigation to the demo page by deep link failed because the navigation container was not ready yet.

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

Lines 166 to 171 in d60c13e

	// Check if we should be running any demos immediately after signing in.
	if (lodashGet(this.props.demoInfo, 'saastr.isBeginningDemo', false)) {
	Navigation.navigate(ROUTES.SAASTR, CONST.NAVIGATION.TYPE.FORCED_UP);
	} else if (lodashGet(this.props.demoInfo, 'sbe.isBeginningDemo', false)) {
	Navigation.navigate(ROUTES.SBE, CONST.NAVIGATION.TYPE.FORCED_UP);
	}

If it's not ready, it's stored in a pending route. However, the pending route only stores the route, but not the type.

App/src/libs/Navigation/Navigation.js

Line 83 in d60c13e

pendingRoute = route;

So, when the navigation is ready, we navigate to the pending route with no type. I tried to fix it by saving the type too, but now the navigation stack becomes: [LHN, Demo chat (SAASTR), Not found report]

We still have the not found report in the stack because doing a deep link to a report screen will navigate it twice. So, the first not found page is replaced by demo chat and the second one is appended to the stack at the top.

---

@abdulrahuman5196 Updated my solution to include more explanation

[rojiphil]

Proposal

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

User is returned to ‘Hmm… Its not here’ page from workspace chat when app is launched via deep link staging.new.expensify.com/r/saastr and swiped back.

What is the root cause of that problem?

The root cause of the problem is that when the code reaches at this location the demo chat is the immediately previous one in navigation stack which has no chat page. And, hence the problem.

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

To solve the problem, we need to replace the demo chat with the demo workspace chat report. We can do this using FORCED_UP by replacing the code here like this:

        if (Navigation.getActiveRoute().includes(`/r/saastr`) || Navigation.getActiveRoute().includes(`/r/sbe`)) {        
            Navigation.navigate(ROUTES.getReportRoute(demoWorkspaceChatReportID),CONST.NAVIGATION.TYPE.FORCED_UP);
        }
        else{
            Navigation.navigate(ROUTES.getReportRoute(demoWorkspaceChatReportID));
        }

What alternative solutions did you explore? (Optional)

[abdulrahuman5196]

@abdulrahuman5196 The NotFoundPage comes first because the URL of the sbe and saastr report is incorrect. The URL '/r/sbe' is the URL of the ReportScreen so it knows sbe is the reportID and then not found page appears because the reportID is invalid.

Got it. @dukenv0307 I understand this. But I would like to fix the issue of NotFoundPage itself. Like NotFound page itself should not become visible at first place.

[abdulrahuman5196]

@bernhardoj
With your proposal I am only seeing NotFoundPage view,

We still have the not found report in the stack because doing a deep link to a report screen will #23441. So, the first not found page is replaced by demo chat and the second one is appended to the stack at the top.

We already have a PR for that issue. I tried your proposal alongside with its fix. But still I am seeing this issue.

[bernhardoj]

Yes, that's supposed to be the expected result. When the user goes to r/saastr, a not found page will be shown. The correct demo page is /saastr or /sbe.

Currently, it pushes both not found and demo chat.

[abdulrahuman5196]

Yes, that's supposed to be the expected result. When the user goes to r/saastr, a not found page will be shown. The correct demo page is /saastr or /sbe.

Currently, it pushes both not found and demo chat.

Sorry I don't understand this. Could you provide more information. @bernhardoj

[dukenv0307]

@abdulrahuman5196 As @bernhardoj mentioned, because the URL /r/sbe is invalid reportID and then the not found page display is the expected behavior. So I think we should confirm the expected we want to fix here:

1. Prevent Saastr and sbe report open when open with the wrong deeplink. The correct link should be domain/saastr
2. Fix back button to always go back to LHN in native

[abdulrahuman5196]

Got it @dukenv0307 Thank you.

I was under the impression, deeplink urls ending with /saastr is always valid one. I could be wrong as well.

@mountiny Could you kindly clarify this? In the video we have one NotFoundPage followed by the saastr page opening for r/saastr url

[abdulrahuman5196]

Anyways just to note: @rojiphil 's solution here #26881 (comment) is working fine. Like it opens the saastr page and on back navigates to LHN.

[rojiphil]

Proposal

Updated
@abdulrahuman5196 Thanks for your feedback. Have updated proposal with a safety condition as we want to replace only when the route includes /r/saastr or /r/sbe

[mountiny]

@abdulrahuman5196 no the r/sbe is invalid route and that is not supported, is this entire issue based on this wrong testing assumption? 😢 does this not occur when navigating to /sbe? If so we dont have to do anything. Sorry this was not caught earlier

[mountiny]

@mvtglobally @izarutskaya @kavimuru the links for the conferences do not have the r/ in it, not sure if your team tested it with those links. should be only /sbe or /saastr

[dukenv0307]

@mountiny If we visit with the correct link /saastr it works as well.

[bernhardoj]

@mountiny yep, it works well if we visit /saastr, but we still need to fix the issue where visiting /r/saastr (or any link ending with saastr) will open the demo chat too (which I believe cause the confusion).

[mountiny]

These links will be removed after the conferences and they wont be used so i am hesitant if this is needed, basically this is a wrong link noone would normally encounter from QR code or from a link shared somewhere, we dont have to care about this.

[mountiny]

I am sorry for wasting your time on this one, this was missed on QA and my side that I havent checked the report better, sorry

[izarutskaya]

@mountiny The problem was discovered while executing PR #26844

[mountiny]

I see that the qa steps mention /sbe and /saastr routes but that probably should could have been made clearer with a full route.