[$500] Web - On login with concierge, different chat is displayed before redirecting to concierge

[kbecciv]

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

---

Version Number: 1.4.27-0
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
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Action Performed:

1. Open the app
2. Logout if logged in
3. Visit concierge link eg: https://staging.new.expensify.com/concierge
4. Login with user with many chats
5. Observe that after login, different chat is displayed first before redirecting to concierge page

Expected Result:

App should display concierge page on login with concierge link

Actual Result:

App displays different chat first before redirecting to concierge page on login with concierge link

Workaround:

Unknown

Platforms:

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

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6346840_1705601525095.34701_-_login_with_concierge_issue_staging.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01206911560f412311
• Upwork Job ID: 1748064944878907392
• Last Price Increase: 2024-02-01
• Automatic offers:
• bernhardoj | Contributor | 0

[melvin-bot]

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

[melvin-bot]

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

[melvin-bot]

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

[kbecciv]

@Quintar - #vip-vsb

[dummy-1111]

Proposal

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

When login with concierge, different chat is displayed before redirecting to concierge

What is the root cause of that problem?

When deep linking, openReportFromDeepLink function is called. After signing in, navigates to the deep link url

App/src/libs/actions/Report.ts

Lines 2057 to 2078 in e6b0761

	const route = ReportUtils.getRouteFromLink(url);
	if (route === ROUTES.CONCIERGE) {
	navigateToConciergeChat(true);
	return;
	}
	if (Session.isAnonymousUser() && !Session.canAccessRouteByAnonymousUser(route)) {
	Session.signOutAndRedirectToSignIn(true);
	return;
	}
	// We don't want to navigate to the exitTo route when creating a new workspace from a deep link,
	// because we already handle creating the optimistic policy and navigating to it in App.setUpPoliciesAndNavigate,
	// which is already called when AuthScreens mounts.
	if (new URL(url).searchParams.get('exitTo') === ROUTES.WORKSPACE_NEW) {
	return;
	}
	if (shouldSkipDeepLinkNavigation(route)) {
	return;
	}
	Navigation.navigate(route as Route, CONST.NAVIGATION.ACTION_TYPE.PUSH);

When navigating to concierge chat, we wait for openApp or reconnectApp API call and navigate to the concierge chat

App/src/libs/actions/Report.ts

Lines 1582 to 1585 in e6b0761

	Welcome.serverDataIsReadyPromise().then(() => {
	// If we don't have a chat with Concierge then create it
	navigateToAndOpenReport([CONST.EMAIL.CONCIERGE], false);
	});

but when navigating to other chat, we just go to the report

Meanwhile, we have ReportScreenIDSetter to go to the last accessed report.

App/src/libs/Navigation/AppNavigator/ReportScreenIDSetter.ts

Lines 58 to 77 in e6b0761

	if (route?.params?.reportID) {
	const reportActionID = route?.params?.reportActionID;
	const regexValidReportActionID = new RegExp(/^\d*$/);
	if (reportActionID && !regexValidReportActionID.test(reportActionID)) {
	navigation.setParams({reportActionID: ''});
	}
	App.confirmReadyToOpenApp();
	return;
	}
	// If there is no reportID in route, try to find last accessed and use it for setParams
	const reportID = getLastAccessedReportID(reports, !canUseDefaultRooms, policies, isFirstTimeNewExpensifyUser, !!reports?.params?.openOnAdminRoom, reportMetadata);
	// It's possible that reports aren't fully loaded yet
	// in that case the reportID is undefined
	if (reportID) {
	navigation.setParams({reportID: String(reportID)});
	} else {
	App.confirmReadyToOpenApp();
	}

As you can see above, route?.params?.reportID is empty and last accessed report exists, we call navigation.setParams({reportID: String(reportID)}). When deep linking to concierge chat, route?.params?.reportID is empty and so navigation.setParams is called, but when linking to other chat, route?.params?.reportID is not empty and navigation.setParams isn't called

This is the root cause

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

We shouldn't call navigation.setParams when deep linking to concierge chat

Insert the following code here

let deepLinkRoute: string = '';
Linking.getInitialURL().then((url) => {
    deepLinkRoute = ReportUtils.getRouteFromLink(url);
});

Update

App/src/libs/Navigation/AppNavigator/ReportScreenIDSetter.ts

Line 74 in e6b0761

navigation.setParams({reportID: String(reportID)});

to

            if (deepLinkRoute !== ROUTES.CONCIERGE) {
                navigation.setParams({reportID: String(reportID)});
            }

This works as expected

Result

34755.mp4

What alternative solutions did you explore? (Optional)

[bernhardoj]

Proposal

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

When going to /concierge and login, the app shows other report briefly before the concierge chat.

What is the root cause of that problem?

When we navigate to /concierge, we actually being navigated to ConciergePage.

App/src/pages/ConciergePage.tsx

Lines 27 to 41 in 991a4e2

	function ConciergePage({session}: ConciergePageProps) {
	useFocusEffect(() => {
	if (session && 'authToken' in session) {
	// Pop the concierge loading page before opening the concierge report.
	Navigation.isNavigationReady().then(() => {
	Navigation.goBack(ROUTES.HOME);
	Report.navigateToConciergeChat();
	});
	} else {
	Navigation.navigate();
	}
	});
	return <FullScreenLoadingIndicator />;
	}

Before going to the concierge chat, it will pop the current screen, that is the ConciergePage itself. But navigateToConciergeChat will wait until the initial load of the app to complete.

App/src/libs/actions/Report.ts

Lines 1579 to 1585 in 991a4e2

	if (!conciergeChatReportID || ignoreConciergeReportID) {
	// In order to avoid creating concierge repeatedly,
	// we need to ensure that the server data has been successfully pulled
	Welcome.serverDataIsReadyPromise().then(() => {
	// If we don't have a chat with Concierge then create it
	navigateToAndOpenReport([CONST.EMAIL.CONCIERGE], false);
	});

So, the ConciergePage utility page is gone, but we are still not navigated to the concierge page yet.

Then, we have a logic to have at least one report screen on the stack which will show the last accessed report.

App/src/libs/Navigation/AppNavigator/createCustomStackNavigator/CustomRouter.ts

Lines 68 to 74 in 991a4e2

	if (!isAtLeastOneCentralPaneNavigatorInState(partialState) && !options.getIsSmallScreenWidth()) {
	// If we added a route we need to make sure that the state.stale is true to generate new key for this route
	// eslint-disable-next-line no-param-reassign
	(partialState.stale as boolean) = true;
	addCentralPaneNavigatorRoute(partialState);
	}

So, the nav stack will be: [Home, ReportScreen], and when navigateToConciergeChat is done, concierge chat is added to the stack: [Home, ReportScreen, ReportScreen (Concierge)].

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

First, we should wait until navigateToConciergeChat is complete before popping out the ConciergePage. But as we are not allowed to do promise chaining in a component, we can pass a new param to navigateToConciergeChat that will pop the current screen after navigating to concierge chat, or just replace the current screen with concierge chat.

Report.navigateToConciergeChat(false, true <- shouldPopCurrentScreen); or
Report.navigateToConciergeChat(false, CONST.NAVIGATION.TYPE.FORCED_UP);

With this change, we will see ConciergePage (which shows a loading screen) instead of a report screen.

If we want to show a report skeleton on the concierge page, we can add ReportHeaderSkeletonView and ReportActionsSkeletonView to the component like below:

return (
    <>
        <View style={[styles.borderBottom]}>
            <ReportHeaderSkeletonView onBackButtonPress={Navigation.goBack} />
        </View>
        <ReportActionsSkeletonView />
    </>
);

Here is what it looks like:

Screen.Recording.2024-02-06.at.17.23.26.mov

But the report screen added by CustomRouter still exists on the nav stack: [Home, ReportScreen, ReporScreen (Concierge)]. To prevent this, we can avoid adding it if ConciergePage is in the stack.

App/src/libs/Navigation/AppNavigator/createCustomStackNavigator/CustomRouter.ts

Line 68 in 991a4e2

if (!isAtLeastOneCentralPaneNavigatorInState(partialState) && !options.getIsSmallScreenWidth()) {

const isConciergePageInState = (state: State): boolean => !!state.routes.find((route) => route.name === SCREENS.CONCIERGE);

&& !isConciergePageInState(partialState)

Last, we need to call App.confirmReadyToOpenApp() in ConciergePage inside the if. App.confirmReadyToOpenApp() is previously called in ReportScreenIDSetter, but since we don't add report screen to the stack anymore, we need to call it in ConciergePage.

App/src/pages/ConciergePage.tsx

Lines 29 to 31 in 991a4e2

	if (session && 'authToken' in session) {
	// Pop the concierge loading page before opening the concierge report.
	Navigation.isNavigationReady().then(() => {

[adelekennedy]

@sobitneupane two proposals above to review!

[sobitneupane]

I am working partially this week. I will try to review the proposals before weekend.

[melvin-bot]

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

[melvin-bot]

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

[adelekennedy]

we have proposals - @sobitneupane will review when back

[melvin-bot]

@sobitneupane @adelekennedy 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!

[sobitneupane]

i will review the proposals by EOD.

[melvin-bot]

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

[sobitneupane]

Thanks for the proposal everyone.

Special thanks to @bernhardoj for detailed RCA. Overall, your proposal looks good to me.

With this change, we will see ConciergePage (which shows a loading screen) instead of a report screen.

@bernhardoj Should we show SkeletonView instead of loading screen?

[bernhardoj]

I think it's fine to show the loading screen as it's just an intermediary screen and only noticeable when opening it while the app is still loading.

[sobitneupane]

I believe we have replace the loading screen by SkeletonView in most of the places. And it's good to do have the change here as well. Is there any reason to not have SkeletonView here?

[bernhardoj]

I don't really have a strong reason, but I see all intermediary pages only show a loading screen (LogOutPreviousUserPage, demo page - not exist anymore), so I think we can just leave it as it is.

If we want to add a skeleton, what would it look like? Do we want to show the header and report skeleton without the composer?

[melvin-bot]

@sobitneupane @jasperhuangg @adelekennedy this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

[adelekennedy]

@jasperhuangg will you check the approved proposal?

[jasperhuangg]

On it, thanks for the bump. Been OOO the last two days.

[jasperhuangg]

@bernhardoj's proposal also makes sense to me, great explanation!

[melvin-bot]

📣 @bernhardoj 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[bernhardoj]

PR is ready

cc: @sobitneupane

[melvin-bot]

This issue has not been updated in over 15 days. @sobitneupane, @jasperhuangg, @bernhardoj, @adelekennedy eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[jasperhuangg]

PR was deployed to prod a few days ago without any regressions!

[jasperhuangg]

Seems there's been a problem with melvin adding the awaiting payment labels

[jasperhuangg]

It looks like we need to pay out:
@bernhardoj for opening the PR
@sobitneupane for reviewing the PR

@adelekennedy can you send them offers?

[adelekennedy]

Payouts due:

Contributor: $500 @bernhardoj Upwork
Contributor+: $500 @sobitneupane NewDot

[adelekennedy]

made upwork payment and posted bz payment summary above

[JmillsExpensify]

$500 approved for @sobitneupane based on summary.