Fix "Tried to use NavigationService before appContainerRef was set".

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,
         }}
       >
-        <HideIfNotHydrated PlaceholderComponent={LoadingScreen}>
+        <HideIfNotHydrated PlaceholderComponent={FullScreenLoading}>
           <AppEventHandlers>
             <AppDataFetcher>
               <TranslationProvider>

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));
 };
 

src/start/LoadingScreen.js → 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 (

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<typeof HomeScreen>,
@@ -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 <FullScreenLoading />;
   }
 
   return (
@@ -119,3 +128,22 @@ export default function MainTabsScreen(props: Props) {
     </View>
   );
 }
+
+// `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);

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,

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<typeof ChatScreen>,
   'dev-auth': RouteParamsOf<typeof DevAuthScreen>,
   'emoji-picker': RouteParamsOf<typeof EmojiPickerScreen>,
-  loading: RouteParamsOf<typeof LoadingScreen>,
   'main-tabs': RouteParamsOf<typeof MainTabsScreen>,
   'message-reactions': RouteParamsOf<typeof MessageReactionsScreen>,
   'password-auth': RouteParamsOf<typeof PasswordAuthScreen>,
@@ -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) {
       <Stack.Screen name="chat" component={ChatScreen} />
       <Stack.Screen name="dev-auth" component={DevAuthScreen} />
       <Stack.Screen name="emoji-picker" component={EmojiPickerScreen} />
-      <Stack.Screen name="loading" component={LoadingScreen} />
-      <Stack.Screen
-        name="main-tabs"
-        component={MainTabsScreen}
-        options={{
-          // So we don't show a transition animation between 'loading'
-          // and 'main'.
-          animationEnabled: false,
-        }}
-      />
+      <Stack.Screen name="main-tabs" component={MainTabsScreen} />
       <Stack.Screen name="message-reactions" component={MessageReactionsScreen} />
       <Stack.Screen name="password-auth" component={PasswordAuthScreen} />
       <Stack.Screen

src/nav/__tests__/navSelectors-test.js

@@ -78,7 +78,7 @@ describe('getSameRoutesCount', () => {
   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' } },

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' };
 };

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' }] });