diff --git a/src/ZulipMobile.js b/src/ZulipMobile.js index a1f9e496877..911418473b5 100644 --- a/src/ZulipMobile.js +++ b/src/ZulipMobile.js @@ -15,7 +15,7 @@ import AppEventHandlers from './boot/AppEventHandlers'; import AppDataFetcher from './boot/AppDataFetcher'; import BackNavigationHandler from './nav/BackNavigationHandler'; import { initializeSentry } from './sentry'; -import LoadingScreen from './start/LoadingScreen'; +import FullScreenLoading from './common/FullScreenLoading'; initializeSentry(); @@ -44,7 +44,7 @@ export default (): React$Node => ( backgroundColor: BRAND_COLOR, }} > - + diff --git a/src/account/accountActions.js b/src/account/accountActions.js index e0647b4bad7..29501000c15 100644 --- a/src/account/accountActions.js +++ b/src/account/accountActions.js @@ -8,7 +8,7 @@ import { LOGIN_SUCCESS, LOGOUT, } from '../actionConstants'; -import { resetToAccountPicker, resetToLoading } from '../nav/navActions'; +import { resetToAccountPicker, resetToMainTabs } from '../nav/navActions'; import type { ZulipVersion } from '../utils/zulipVersion'; const accountSwitchPlain = (index: number): Action => ({ @@ -17,7 +17,7 @@ const accountSwitchPlain = (index: number): Action => ({ }); export const accountSwitch = (index: number) => (dispatch: Dispatch, getState: GetState) => { - NavigationService.dispatch(resetToLoading()); + NavigationService.dispatch(resetToMainTabs()); dispatch(accountSwitchPlain(index)); }; @@ -48,7 +48,7 @@ export const loginSuccess = (realm: URL, email: string, apiKey: string) => ( dispatch: Dispatch, getState: GetState, ) => { - NavigationService.dispatch(resetToLoading()); + NavigationService.dispatch(resetToMainTabs()); dispatch(loginSuccessPlain(realm, email, apiKey)); }; diff --git a/src/start/LoadingScreen.js b/src/common/FullScreenLoading.js similarity index 53% rename from src/start/LoadingScreen.js rename to src/common/FullScreenLoading.js index a9014e785d2..4ae63bc83bd 100644 --- a/src/start/LoadingScreen.js +++ b/src/common/FullScreenLoading.js @@ -3,10 +3,8 @@ import React from 'react'; import { View } from 'react-native'; import { useSafeAreaInsets } from 'react-native-safe-area-context'; -import type { RouteProp } from '../react-navigation'; -import type { AppNavigationProp } from '../nav/AppNavigator'; import { BRAND_COLOR, createStyleSheet } from '../styles'; -import { LoadingIndicator, ZulipStatusBar } from '../common'; +import { LoadingIndicator, ZulipStatusBar } from '.'; const componentStyles = createStyleSheet({ center: { @@ -17,17 +15,12 @@ const componentStyles = createStyleSheet({ }, }); -type Props = $ReadOnly<{| - // Since we've put this screen in AppNavigator's route config, but - // we do invoke it from one other place, which is not a navigator - // (see ZulipMobile.js), it might or might not get the `navigation` - // prop (with the particular shape for this route) and the `route` - // prop for free. - navigation?: AppNavigationProp<'loading'>, - route?: RouteProp<'loading', void>, -|}>; +type Props = $ReadOnly<{||}>; -export default function LoadingScreen(props: Props) { +/** + * Meant to be used to cover the whole screen. + */ +export default function FullScreenLoading(props: Props) { const insets = useSafeAreaInsets(); return ( diff --git a/src/main/MainTabsScreen.js b/src/main/MainTabsScreen.js index 0c5a25b3c40..2e1c1f6938f 100644 --- a/src/main/MainTabsScreen.js +++ b/src/main/MainTabsScreen.js @@ -8,6 +8,7 @@ import { import { useSafeAreaInsets } from 'react-native-safe-area-context'; import type { RouteProp, RouteParamsOf } from '../react-navigation'; +import type { Dispatch } from '../types'; import type { AppNavigationProp } from '../nav/AppNavigator'; import type { GlobalParamList } from '../nav/globalTypes'; import { bottomTabNavigatorConfig } from '../styles/tabs'; @@ -19,9 +20,10 @@ import { IconInbox, IconSettings, IconStream } from '../common/Icons'; import { OwnAvatar, OfflineNotice, ZulipStatusBar } from '../common'; import IconUnreadConversations from '../nav/IconUnreadConversations'; import ProfileScreen from '../account-info/ProfileScreen'; -import { useSelector } from '../react-redux'; +import { connect } from '../react-redux'; import { getHaveServerData } from '../selectors'; import styles, { ThemeContext } from '../styles'; +import FullScreenLoading from '../common/FullScreenLoading'; export type MainTabsNavigatorParamList = {| home: RouteParamsOf, @@ -41,26 +43,33 @@ const Tab = createBottomTabNavigator< MainTabsNavigationProp<>, >(); +type SelectorProps = $ReadOnly<{| + haveServerData: boolean, +|}>; + type Props = $ReadOnly<{| navigation: AppNavigationProp<'main-tabs'>, route: RouteProp<'main-tabs', void>, + + dispatch: Dispatch, + ...SelectorProps, |}>; -export default function MainTabsScreen(props: Props) { +function MainTabsScreen(props: Props) { const { backgroundColor } = useContext(ThemeContext); - const haveServerData = useSelector(getHaveServerData); + const { haveServerData } = props; const insets = useSafeAreaInsets(); if (!haveServerData) { - // This can happen if the user has just logged out; this screen - // is still visible for the duration of the nav transition, and - // it's legitimate for its `render` to get called again. - // See our #4275. + // Show a full-screen loading indicator while waiting for the + // initial fetch to complete, if we don't have potentially stale + // data to show instead. Also show it for the duration of the nav + // transition just after the user logs out (see our #4275). // - // Avoid rendering any of our main UI in this case, to maintain - // the guarantee that it can all rely on server data existing. - return null; + // And avoid rendering any of our main UI, to maintain the + // guarantee that it can all rely on server data existing. + return ; } return ( @@ -119,3 +128,22 @@ export default function MainTabsScreen(props: Props) { ); } + +// `connect` does something useful for us that `useSelector` doesn't +// do: it interposes a new `ReactReduxContext.Provider` component, +// which proxies subscriptions so that the descendant components only +// rerender if this one continues to say their subtree should be kept +// around. See +// https://github.com/zulip/zulip-mobile/pull/4454#discussion_r578140524 +// and some discussion around +// https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/converting.20to.20Hooks/near/1111970 +// where we describe some limits of our understanding. +// +// We found these things out while investigating an annoying crash: we +// found that `mapStateToProps` on a descendant of `MainTabsScreen` +// was running -- and throwing an uncaught error -- on logout, and +// `MainTabsScreen`'s early return on `!haveServerData` wasn't +// preventing that from happening. +export default connect(state => ({ + haveServerData: getHaveServerData(state), +}))(MainTabsScreen); diff --git a/src/message/fetchActions.js b/src/message/fetchActions.js index aef9abc7305..a3c0d97eb7b 100644 --- a/src/message/fetchActions.js +++ b/src/message/fetchActions.js @@ -1,10 +1,8 @@ /* @flow strict-local */ -import * as NavigationService from '../nav/NavigationService'; import type { Narrow, Dispatch, GetState, GlobalState, Message, Action, UserId } from '../types'; import type { ApiResponseServerSettings } from '../api/settings/getServerSettings'; import type { InitialData } from '../api/initialDataTypes'; import * as api from '../api'; -import { resetToMainTabs } from '../actions'; import { isClientError } from '../api/apiErrors'; import { getAuth, @@ -13,7 +11,6 @@ import { getLastMessageId, getCaughtUpForNarrow, getFetchingForNarrow, - getNavigationRoutes, } from '../selectors'; import config from '../config'; import { @@ -167,26 +164,10 @@ const initialFetchStart = (): Action => ({ type: INITIAL_FETCH_START, }); -const initialFetchCompletePlain = (): Action => ({ +const initialFetchComplete = (): Action => ({ type: INITIAL_FETCH_COMPLETE, }); -export const initialFetchComplete = () => async (dispatch: Dispatch, getState: GetState) => { - if (!getNavigationRoutes().some(navigationRoute => navigationRoute.name === 'main')) { - // If we're anywhere in the normal UI of the app, then remain - // where we are. Only reset the nav state if we're elsewhere, - // and in that case, go to the main screen. - // - // TODO: "elsewhere" is probably just a way of saying "on the - // loading screen", but we're not sure. We could adjust the - // conditional accordingly, if we found out we're not depending on - // the more general condition; see - // https://github.com/zulip/zulip-mobile/pull/4274#discussion_r505941875 - NavigationService.dispatch(resetToMainTabs()); - } - dispatch(initialFetchCompletePlain()); -}; - /** Private; exported only for tests. */ export const isFetchNeededAtAnchor = ( state: GlobalState, diff --git a/src/nav/AppNavigator.js b/src/nav/AppNavigator.js index 0d42b9c23a9..50f0f311ccf 100644 --- a/src/nav/AppNavigator.js +++ b/src/nav/AppNavigator.js @@ -9,7 +9,7 @@ import { import type { RouteParamsOf } from '../react-navigation'; import { useSelector } from '../react-redux'; -import { hasAuth as getHasAuth, getAccounts, getHaveServerData } from '../selectors'; +import { hasAuth as getHasAuth, getAccounts } from '../selectors'; import getInitialRouteInfo from './getInitialRouteInfo'; import type { GlobalParamList } from './globalTypes'; import AccountPickScreen from '../account/AccountPickScreen'; @@ -23,7 +23,6 @@ import GroupDetailsScreen from '../chat/GroupDetailsScreen'; import SearchMessagesScreen from '../search/SearchMessagesScreen'; import UsersScreen from '../users/UsersScreen'; import ChatScreen from '../chat/ChatScreen'; -import LoadingScreen from '../start/LoadingScreen'; import LanguageScreen from '../settings/LanguageScreen'; import PasswordAuthScreen from '../start/PasswordAuthScreen'; import DebugScreen from '../settings/DebugScreen'; @@ -52,7 +51,6 @@ export type AppNavigatorParamList = {| chat: RouteParamsOf, 'dev-auth': RouteParamsOf, 'emoji-picker': RouteParamsOf, - loading: RouteParamsOf, 'main-tabs': RouteParamsOf, 'message-reactions': RouteParamsOf, 'password-auth': RouteParamsOf, @@ -89,12 +87,10 @@ type Props = $ReadOnly<{||}>; export default function AppNavigator(props: Props) { const hasAuth = useSelector(getHasAuth); const accounts = useSelector(getAccounts); - const haveServerData = useSelector(getHaveServerData); const { initialRouteName, initialRouteParams } = getInitialRouteInfo({ hasAuth, accounts, - haveServerData, }); return ( @@ -115,16 +111,7 @@ export default function AppNavigator(props: Props) { - - + { test('if last route differs from routes the count of same routes is 0', () => { NavigationService.getState = jest.fn().mockReturnValue( deepFreeze({ - routes: [{ name: 'main' }, { name: 'chat' }], + routes: [{ name: 'main-tabs' }, { name: 'chat' }], }), ); @@ -92,7 +92,7 @@ describe('getSameRoutesCount', () => { deepFreeze({ routes: [ { name: 'login' }, - { name: 'main' }, + { name: 'main-tabs' }, { name: 'chat', params: { key: 'value' } }, { name: 'chat', params: { key: 'another value' } }, { name: 'chat', params: { anotherKey: 'some value' } }, diff --git a/src/nav/getInitialRouteInfo.js b/src/nav/getInitialRouteInfo.js index 2349a721cc4..8cf3597d41b 100644 --- a/src/nav/getInitialRouteInfo.js +++ b/src/nav/getInitialRouteInfo.js @@ -7,9 +7,8 @@ import type { Account } from '../types'; export default (args: {| hasAuth: boolean, accounts: Account[], - haveServerData: boolean, |}): {| initialRouteName: string, initialRouteParams?: ScreenParams |} => { - const { hasAuth, accounts, haveServerData } = args; + const { hasAuth, accounts } = args; // If the active account is not logged in, bring the user as close // as we can to AuthScreen, the place where they can log in. @@ -39,14 +38,11 @@ export default (args: {| } } - // If there's an active, logged-in account but no server data, then behave - // like `ACCOUNT_SWITCH`: show loading screen. Crucially, `sessionReducer` - // will have set `needsInitialFetch`, too, so we really will be loading. - if (!haveServerData) { - return { initialRouteName: 'loading' }; - } - - // Great: we have an active, logged-in account, and server data for it. - // Show the main UI. + // Show the main UI screen. + // + // If we don't have server data yet, that screen will show a loading + // indicator until the data is loaded. Crucially, `sessionReducer` + // will have set `needsInitialFetch`, too, so we really will be + // loading. return { initialRouteName: 'main-tabs' }; }; diff --git a/src/nav/navActions.js b/src/nav/navActions.js index a7f6157e9a5..789a15946cb 100644 --- a/src/nav/navActions.js +++ b/src/nav/navActions.js @@ -15,12 +15,6 @@ export const navigateBack = (): GenericNavigationAction => StackActions.pop(getS * "Reset" actions, to explicitly prohibit back navigation. */ -export const resetToLoading = (): GenericNavigationAction => - CommonActions.reset({ - index: 0, - routes: [{ name: 'loading' }], - }); - export const resetToAccountPicker = (): GenericNavigationAction => CommonActions.reset({ index: 0, routes: [{ name: 'account-pick' }] });