-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Changes from 6 commits
cc7a59e
d89951a
03e64f6
8052227
18552c2
b965cdd
50a8bb1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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; | ||||||||||||||||||
return; | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
|
@@ -167,7 +169,6 @@ Onyx.connect({ | |||||||||||||||||
}); | ||||||||||||||||||
|
||||||||||||||||||
const allReports: OnyxCollection<Report> = {}; | ||||||||||||||||||
let conciergeChatReportID: string | undefined; | ||||||||||||||||||
const typingWatchTimers: Record<string, NodeJS.Timeout> = {}; | ||||||||||||||||||
|
||||||||||||||||||
let reportIDDeeplinkedFromOldDot: string | undefined; | ||||||||||||||||||
|
@@ -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) { | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Which was the PR adding this code? Won't There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was added in #29122.
No. App/src/libs/actions/Report.ts Line 170 in 6914b13
App/src/libs/actions/Report.ts Lines 1092 to 1098 in 6914b13
|
||||||||||||||||||
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; | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above (https://github.com/Expensify/App/pull/36216/files#r1484132324) |
||||||||||||||||||
} | ||||||||||||||||||
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)); | ||||||||||||||||||
} | ||||||||||||||||||
} | ||||||||||||||||||
|
@@ -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; | ||||||||||||||||||
} | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This prevents us to deep-link to ConciergePage when logged out. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there any issue caused by this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, the issue is in my previous comment. To reproduce it:
|
||||||||||||||||||
|
||||||||||||||||||
if (route && Session.isAnonymousUser() && !Session.canAccessRouteByAnonymousUser(route)) { | ||||||||||||||||||
Session.signOutAndRedirectToSignIn(true); | ||||||||||||||||||
return; | ||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is the change necessary? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
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'; | ||
|
||
|
@@ -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; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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'; | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.