From 88b0a71e07c767b5f9631b8b9b80b686a61bdb65 Mon Sep 17 00:00:00 2001
From: Chris Bobbe <cbobbe@zulip.com>
Date: Tue, 3 Aug 2021 16:41:41 -0400
Subject: [PATCH] OfflineNotice: Show an alert if we stay in an unknown state
 too long.

And log what we know from NetInfo to Sentry.
---
 src/boot/AppEventHandlers.js         |  9 +++-
 src/common/OfflineNotice.js          | 61 +++++++++++++++++++++++-----
 static/translations/messages_en.json |  1 +
 3 files changed, 60 insertions(+), 11 deletions(-)

diff --git a/src/boot/AppEventHandlers.js b/src/boot/AppEventHandlers.js
index 2e1fedb969f..6e4bfd6d912 100644
--- a/src/boot/AppEventHandlers.js
+++ b/src/boot/AppEventHandlers.js
@@ -86,7 +86,8 @@ class AppEventHandlersInner extends PureComponent<Props> {
         //
         // 1. A request loop is started. A HEAD request is made to
         //    https://clients3.google.com/generate_204, with a timeout of
-        //    15s, to see if the Internet is reachable.
+        //    15s (`reachabilityRequestTimeout`), to see if the Internet is
+        //    reachable.
         //    - If the `fetch` succeeds and a 204 is received, this will be
         //      made `true`. We'll then sleep for 60s before making the
         //      request again.
@@ -130,7 +131,13 @@ class AppEventHandlersInner extends PureComponent<Props> {
     handleInitialNotification(dispatch);
     handleInitialShare(dispatch);
 
+    NetInfo.configure({
+      // This is the default, as of 6.0.0, but `OfflineNotice` depends on this
+      // value being stable.
+      reachabilityRequestTimeout: 15 * 1000,
+    });
     this.netInfoDisconnectCallback = NetInfo.addEventListener(this.handleConnectivityChange);
+
     AppState.addEventListener('change', this.handleAppStateChange);
     AppState.addEventListener('memoryWarning', this.handleMemoryWarning);
     ScreenOrientation.addOrientationChangeListener(this.handleOrientationChange);
diff --git a/src/common/OfflineNotice.js b/src/common/OfflineNotice.js
index 8f3ba3296a4..4e14d2e1c9a 100644
--- a/src/common/OfflineNotice.js
+++ b/src/common/OfflineNotice.js
@@ -1,10 +1,13 @@
 /* @flow strict-local */
 
-import React from 'react';
+import React, { useEffect } from 'react';
 import type { Node } from 'react';
 import { View } from 'react-native';
+import NetInfo from '@react-native-community/netinfo';
 
+import * as logging from '../utils/logging';
 import { createStyleSheet, HALF_COLOR } from '../styles';
+import { useHasStayedTrueForMs } from '../reactUtils';
 import { useSelector } from '../react-redux';
 import { getSession } from '../selectors';
 import Label from './Label';
@@ -25,18 +28,56 @@ const styles = createStyleSheet({
 type Props = $ReadOnly<{||}>;
 
 /**
- * Displays a notice that the app is working in offline mode.
- * Not rendered if state is 'online'.
+ * Shows a notice if the app is working in offline mode.
+ *
+ * Shows a different notice if we've taken longer than we expect to
+ * determine Internet reachability. IOW, if the user sees this, there's a
+ * bug.
+ *
+ * Shows nothing if the Internet is reachable.
  */
 export default function OfflineNotice(props: Props): Node {
   const isOnline = useSelector(state => getSession(state).isOnline);
-  if (isOnline === true || isOnline === null) {
-    return null;
-  }
 
-  return (
-    <View style={styles.block}>
-      <Label style={styles.text} text="No Internet connection" />
-    </View>
+  const shouldShowUncertaintyNotice = useHasStayedTrueForMs(
+    // See note in `SessionState` for what this means.
+    isOnline === null,
+
+    // A decently long time, much longer than it takes to send `true` or
+    // `false` over the RN bridge.
+    //
+    // Also, one second longer than what we set for
+    // `reachabilityRequestTimeout` in NetInfo's config (15s), which is the
+    // longest `isOnline` can be `null` on iOS in an expected case. For
+    // details, see the comment where we dispatch the action to change
+    // `isOnline`.
+    //
+    // If this time passes and `isOnline` is still `null`, we should treat
+    // it as a bug and investigate.
+    16 * 1000,
   );
+
+  useEffect(() => {
+    if (shouldShowUncertaintyNotice) {
+      NetInfo.fetch().then(state => {
+        logging.warn('Failed to determine Internet reachability in a reasonable time', state);
+      });
+    }
+  }, [shouldShowUncertaintyNotice]);
+
+  if (shouldShowUncertaintyNotice) {
+    return (
+      <View style={styles.block}>
+        <Label style={styles.text} text="Please check your Internet connection" />
+      </View>
+    );
+  } else if (isOnline === false) {
+    return (
+      <View style={styles.block}>
+        <Label style={styles.text} text="No Internet connection" />
+      </View>
+    );
+  } else {
+    return null;
+  }
 }
diff --git a/static/translations/messages_en.json b/static/translations/messages_en.json
index fb2c5a78fe6..93455bfcebd 100644
--- a/static/translations/messages_en.json
+++ b/static/translations/messages_en.json
@@ -64,6 +64,7 @@
   "Mute stream": "Mute stream",
   "Unmute stream": "Unmute stream",
   "No Internet connection": "No Internet connection",
+  "Please check your Internet connection": "Please check your Internet connection",
   "{realm} is running Zulip Server {serverVersion}, which is unsupported. Please upgrade your server as soon as possible.": "{realm} is running Zulip Server {serverVersion}, which is unsupported. Please upgrade your server as soon as possible.",
   "{realm} is running Zulip Server {serverVersion}, which is unsupported. Please contact your administrator about upgrading.": "{realm} is running Zulip Server {serverVersion}, which is unsupported. Please contact your administrator about upgrading.",
   "Dismiss": "Dismiss",