Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix other chat is shown briefly when navigating to concierge page #36216

Merged
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 14 additions & 11 deletions src/libs/actions/Report.ts
Original file line number Diff line number Diff line change
Expand Up @@ -85,12 +85,14 @@ type ActionSubscriber = {
callback: SubscriberCallback;
};

let conciergeChatReportID: string | undefined;
let currentUserAccountID = -1;
Onyx.connect({
key: ONYXKEYS.SESSION,
callback: (value) => {
// When signed out, val is undefined
if (!value?.accountID) {
conciergeChatReportID = undefined;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if it is the correct place to make the change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want to clear it when signed out and I think this is the place to know whether the user is signed out or not.

return;
}

Expand Down Expand Up @@ -167,7 +169,6 @@ Onyx.connect({
});

const allReports: OnyxCollection<Report> = {};
let conciergeChatReportID: string | undefined;
const typingWatchTimers: Record<string, NodeJS.Timeout> = {};

let reportIDDeeplinkedFromOldDot: string | undefined;
Expand Down Expand Up @@ -1681,24 +1682,29 @@ function updateWriteCapabilityAndNavigate(report: Report, newValue: WriteCapabil

/**
* Navigates to the 1:1 report with Concierge
*
* @param ignoreConciergeReportID - Flag to ignore conciergeChatReportID during navigation. The default behavior is to not ignore.
*/
function navigateToConciergeChat(ignoreConciergeReportID = false, shouldDismissModal = false) {
function navigateToConciergeChat(shouldDismissModal = false, shouldPopCurrentScreen = false, checkIfCurrentPageActive = () => true) {
// If conciergeChatReportID contains a concierge report ID, we navigate to the concierge chat using the stored report ID.
// Otherwise, we would find the concierge chat and navigate to it.
// Now, when user performs sign-out and a sign-in again, conciergeChatReportID may contain a stale value.
// In order to prevent navigation to a stale value, we use ignoreConciergeReportID to forcefully find and navigate to concierge chat.
if (!conciergeChatReportID || ignoreConciergeReportID) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ignoreConciergeReportID is only used in https://github.com/Expensify/App/pull/36216/files#r1484170212. The reason behind this param is explained by the comment itself. To properly fix it, I clear the concierge ID when the user is logged out

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which was the PR adding this code? Won't conciergeChatReportID get removed when singed out along with other onyx data?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was added in #29122.

Won't conciergeChatReportID get removed when singed out along with other onyx data?

No. conciergeChatReportID is a global(?) variable that is assigned when we receive the onyx data.

let conciergeChatReportID: string | undefined;

App/src/libs/actions/Report.ts

Lines 1092 to 1098 in 6914b13

if (allReports && report?.reportID) {
allReports[report.reportID] = report;
if (ReportUtils.isConciergeChatReport(report)) {
conciergeChatReportID = report.reportID;
}
}

if (!conciergeChatReportID) {
// 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
if (!checkIfCurrentPageActive()) {
return;
Copy link
Contributor Author

@bernhardoj bernhardoj Feb 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}
if (shouldPopCurrentScreen && !shouldDismissModal) {
Navigation.goBack();
}
navigateToAndOpenReport([CONST.EMAIL.CONCIERGE], shouldDismissModal);
});
} else if (shouldDismissModal) {
Navigation.dismissModal(conciergeChatReportID);
} else {
if (shouldPopCurrentScreen) {
Navigation.goBack();
}
Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(conciergeChatReportID));
}
}
Expand Down Expand Up @@ -2148,10 +2154,7 @@ function openReportFromDeepLink(url: string, isAuthenticated: boolean) {
Session.waitForUserSignIn().then(() => {
Navigation.waitForProtectedRoutes().then(() => {
const route = ReportUtils.getRouteFromLink(url);
if (route === ROUTES.CONCIERGE) {
navigateToConciergeChat(true);
return;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This prevents us to deep-link to ConciergePage when logged out.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any issue caused by this?

Copy link
Contributor Author

@bernhardoj bernhardoj Feb 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the issue is in my previous comment.

To reproduce it:

  1. Sign out
  2. Deep link to /concierge
  3. Sign in
  4. The LHN is shown instead of ConciergePage because ConciergePage is never added to the nav stack. A concierge chat will be shown eventually because we call navigateToConciergeChat, but ConciergePage utility page is never shown. (native)


if (route && Session.isAnonymousUser() && !Session.canAccessRouteByAnonymousUser(route)) {
Session.signOutAndRedirectToSignIn(true);
return;
Expand Down
2 changes: 1 addition & 1 deletion src/libs/actions/Session/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -874,7 +874,7 @@ const canAccessRouteByAnonymousUser = (route: string) => {
if (route.startsWith('/')) {
routeRemovedReportId = routeRemovedReportId.slice(1);
}
const routesCanAccessByAnonymousUser = [ROUTES.SIGN_IN_MODAL, ROUTES.REPORT_WITH_ID_DETAILS.route, ROUTES.REPORT_WITH_ID_DETAILS_SHARE_CODE.route];
const routesCanAccessByAnonymousUser = [ROUTES.SIGN_IN_MODAL, ROUTES.REPORT_WITH_ID_DETAILS.route, ROUTES.REPORT_WITH_ID_DETAILS_SHARE_CODE.route, ROUTES.CONCIERGE];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the change necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To allow anonymous user to open ConciergePage utility page.


if ((routesCanAccessByAnonymousUser as string[]).includes(routeRemovedReportId)) {
return true;
Expand Down
36 changes: 30 additions & 6 deletions src/pages/ConciergePage.tsx
Original file line number Diff line number Diff line change
@@ -1,14 +1,18 @@
import {useFocusEffect} from '@react-navigation/native';
import type {StackScreenProps} from '@react-navigation/stack';
import React from 'react';
import React, {useEffect, useRef} from 'react';
import {View} from 'react-native';
import type {OnyxEntry} from 'react-native-onyx';
import {withOnyx} from 'react-native-onyx';
import FullScreenLoadingIndicator from '@components/FullscreenLoadingIndicator';
import ReportActionsSkeletonView from '@components/ReportActionsSkeletonView';
import ReportHeaderSkeletonView from '@components/ReportHeaderSkeletonView';
import ScreenWrapper from '@components/ScreenWrapper';
import useThemeStyles from '@hooks/useThemeStyles';
import Navigation from '@libs/Navigation/Navigation';
import type {AuthScreensParamList} from '@libs/Navigation/types';
import * as App from '@userActions/App';
import * as Report from '@userActions/Report';
import ONYXKEYS from '@src/ONYXKEYS';
import ROUTES from '@src/ROUTES';
import type SCREENS from '@src/SCREENS';
import type {Session} from '@src/types/onyx';

Expand All @@ -25,19 +29,39 @@ type ConciergePageProps = ConciergePageOnyxProps & StackScreenProps<AuthScreensP
* - Else re-route to the login page
*/
function ConciergePage({session}: ConciergePageProps) {
const styles = useThemeStyles();
const isUnmounted = useRef(false);

useFocusEffect(() => {
if (session && 'authToken' in session) {
App.confirmReadyToOpenApp();
// Pop the concierge loading page before opening the concierge report.
Navigation.isNavigationReady().then(() => {
Navigation.goBack(ROUTES.HOME);
Report.navigateToConciergeChat();
if (isUnmounted.current) {
return;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We return early here if the component is unmounted to prevent the user from being navigated to the concierge chat even after closing the concierge page

Report.navigateToConciergeChat(undefined, true, () => !isUnmounted.current);
});
} else {
Navigation.navigate();
}
});

return <FullScreenLoadingIndicator />;
useEffect(
() => () => {
isUnmounted.current = true;
},
[],
);

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

ConciergePage.displayName = 'ConciergePage';
Expand Down
2 changes: 1 addition & 1 deletion src/pages/ReimbursementAccount/ValidationStep.js
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,7 @@ function ValidationStep({reimbursementAccount, translate, onBackButtonPress, acc
<Text>{translate('validationStep.letsChatText')}</Text>
<Button
text={translate('validationStep.letsChatCTA')}
onPress={() => Report.navigateToConciergeChat(false, true)}
onPress={() => Report.navigateToConciergeChat(true)}
icon={Expensicons.ChatBubble}
style={[styles.mt4]}
iconStyles={[styles.buttonCTAIcon]}
Expand Down
Loading