From c8777422ceb3793208e3d8382cb44a3f7efdef2f Mon Sep 17 00:00:00 2001 From: Elias Nahum Date: Thu, 22 Nov 2018 10:25:08 -0300 Subject: [PATCH] Fix network indicator layout and reconnect logic (#2351) * Fix network indicator layout and reconnect logic * Reconnection logic to cover more use cases * Check for internet connectivity when bringing the app to the foreground --- .../network_indicator/network_indicator.js | 165 ++++++++++-------- app/mattermost.js | 5 + app/store/index.js | 2 - app/utils/network.js | 34 ++-- assets/base/i18n/en.json | 2 +- package-lock.json | 4 +- package.json | 2 +- 7 files changed, 121 insertions(+), 93 deletions(-) diff --git a/app/components/network_indicator/network_indicator.js b/app/components/network_indicator/network_indicator.js index 64ea491d8..f4267a3c6 100644 --- a/app/components/network_indicator/network_indicator.js +++ b/app/components/network_indicator/network_indicator.js @@ -9,9 +9,9 @@ import { Alert, Animated, AppState, + NetInfo, Platform, StyleSheet, - TouchableOpacity, View, } from 'react-native'; import IonIcon from 'react-native-vector-icons/Ionicons'; @@ -28,7 +28,7 @@ import {RequestStatus} from 'mattermost-redux/constants'; const HEIGHT = 38; const MAX_WEBSOCKET_RETRIES = 3; -const CONNECTION_RETRY_SECONDS = 30; +const CONNECTION_RETRY_SECONDS = 5; const CONNECTION_RETRY_TIMEOUT = 1000 * CONNECTION_RETRY_SECONDS; // 30 seconds const { ANDROID_TOP_LANDSCAPE, @@ -71,6 +71,7 @@ export default class NetworkIndicator extends PureComponent { this.top = new Animated.Value(navBar - HEIGHT); this.backgroundColor = new Animated.Value(0); + this.firstRun = true; this.networkListener = networkConnectionListener(this.handleConnectionChange); } @@ -82,8 +83,14 @@ export default class NetworkIndicator extends PureComponent { } componentDidUpdate(prevProps) { - const {websocketStatus: previousWebsocketStatus} = prevProps; - const {websocketErrorCount, websocketStatus} = this.props; + const {isLandscape: prevIsLandscape, websocketStatus: previousWebsocketStatus} = prevProps; + const {isLandscape, websocketErrorCount, websocketStatus} = this.props; + + if (isLandscape !== prevIsLandscape) { + const navBar = this.getNavBarHeight(isLandscape); + const initialTop = websocketErrorCount || previousWebsocketStatus === RequestStatus.FAILURE || previousWebsocketStatus === RequestStatus.NOT_STARTED ? 0 : HEIGHT; + this.top.setValue(navBar - initialTop); + } if (this.props.isOnline) { if (previousWebsocketStatus === RequestStatus.STARTED && websocketStatus === RequestStatus.SUCCESS) { @@ -93,10 +100,10 @@ export default class NetworkIndicator extends PureComponent { } else if (previousWebsocketStatus === RequestStatus.STARTED && websocketStatus === RequestStatus.FAILURE && websocketErrorCount > MAX_WEBSOCKET_RETRIES) { this.handleWebSocket(false); this.handleReconnect(); - } else if (websocketStatus === RequestStatus.FAILURE && websocketErrorCount > 1) { + } else if (websocketStatus === RequestStatus.FAILURE) { this.show(); } - } else if (prevProps.isOnline && !this.props.isOnline) { + } else { this.offline(); } } @@ -116,17 +123,28 @@ export default class NetworkIndicator extends PureComponent { clearTimeout(this.connectionRetryTimeout); } - connect = async () => { + connect = (displayBar = false) => { clearTimeout(this.connectionRetryTimeout); - const result = await checkConnection(this.props.isOnline); + NetInfo.isConnected.fetch().then(async (isConnected) => { + const {hasInternet, serverReachable} = await checkConnection(isConnected); - if (result) { - this.props.actions.connection(true); - this.initializeWebSocket(); - } else { - this.handleWebSocket(false); - } + this.props.actions.connection(hasInternet); + if (serverReachable) { + this.initializeWebSocket(); + } else { + this.handleWebSocket(false); + + if (displayBar) { + this.show(); + } + + if (hasInternet) { + // try to reconnect cause we have internet + this.handleReconnect(); + } + } + }); }; connected = () => { @@ -192,21 +210,37 @@ export default class NetworkIndicator extends PureComponent { const {currentChannelId} = this.props; const active = appState === 'active'; - this.handleWebSocket(active); + if (active) { + this.connect(true); - if (active && currentChannelId) { - PushNotifications.clearChannelNotifications(currentChannelId); + if (currentChannelId) { + PushNotifications.clearChannelNotifications(currentChannelId); + } + } else { + this.handleWebSocket(false); } }; - handleConnectionChange = (isConnected) => { + handleConnectionChange = ({hasInternet, serverReachable}) => { const {connection} = this.props.actions; + // On first run always initialize the WebSocket + // if we have internet connection + if (hasInternet && this.firstRun) { + this.initializeWebSocket(); + this.firstRun = false; + return; + } + // Prevent for being called more than once. - if (this.isConnected !== isConnected) { - this.isConnected = isConnected; - this.handleWebSocket(isConnected); - connection(isConnected); + if (this.hasInternet !== hasInternet) { + this.hasInternet = hasInternet; + connection(hasInternet); + } + + if (this.serverReachable !== serverReachable) { + this.serverReachable = serverReachable; + this.handleWebSocket(serverReachable); } }; @@ -230,7 +264,7 @@ export default class NetworkIndicator extends PureComponent { certificate = await mattermostBucket.getPreference('cert', LocalConfig.AppGroupId); } - initWebSocket(platform, null, null, null, {certificate}).catch(() => { + initWebSocket(platform, null, null, null, {certificate, forceConnection: true}).catch(() => { // we should dispatch a failure and show the app as disconnected Alert.alert( formatMessage({id: 'mobile.authentication_error.title', defaultMessage: 'Authentication Error'}), @@ -252,6 +286,10 @@ export default class NetworkIndicator extends PureComponent { }; offline = () => { + if (this.connectionRetryTimeout) { + clearTimeout(this.connectionRetryTimeout); + } + this.show(); }; @@ -265,7 +303,7 @@ export default class NetworkIndicator extends PureComponent { }; render() { - const {websocketErrorCount, websocketStatus} = this.props; + const {isOnline, websocketStatus} = this.props; const background = this.backgroundColor.interpolate({ inputRange: [0, 1], outputRange: ['#939393', '#629a41'], @@ -276,53 +314,40 @@ export default class NetworkIndicator extends PureComponent { let values; let action; - const currentWebsocketStatus = (websocketStatus === RequestStatus.FAILURE && websocketErrorCount <= MAX_WEBSOCKET_RETRIES) ? RequestStatus.STARTED : websocketStatus; - - switch (currentWebsocketStatus) { - case (RequestStatus.NOT_STARTED): - case (RequestStatus.FAILURE): + if (isOnline) { + switch (websocketStatus) { + case RequestStatus.NOT_STARTED: + case RequestStatus.FAILURE: + case RequestStatus.STARTED: + i18nId = t('mobile.offlineIndicator.connecting'); + defaultMessage = 'Connecting...'; + action = ( + + + + ); + break; + case RequestStatus.SUCCESS: + default: + i18nId = t('mobile.offlineIndicator.connected'); + defaultMessage = 'Connected'; + action = ( + + + + ); + break; + } + } else { i18nId = t('mobile.offlineIndicator.offline'); - defaultMessage = 'Cannot connect to the server. Retrying in {seconds} seconds'; - values = {seconds: CONNECTION_RETRY_SECONDS}; - action = ( - - - - ); - break; - case RequestStatus.STARTED: - i18nId = t('mobile.offlineIndicator.connecting'); - defaultMessage = 'Connecting...'; - action = ( - - - - ); - break; - case RequestStatus.SUCCESS: - default: - i18nId = t('mobile.offlineIndicator.connected'); - defaultMessage = 'Connected'; - action = ( - - - - ); - break; + defaultMessage = 'No internet connection'; } return ( diff --git a/app/mattermost.js b/app/mattermost.js index a42d1e6e4..1faf37a39 100644 --- a/app/mattermost.js +++ b/app/mattermost.js @@ -137,6 +137,11 @@ const resetBadgeAndVersion = () => { }; const handleLogout = () => { + // Because we can logout while being offline we reset + // the Client online flag to true cause the network handler + // is not available at this point + Client4.setOnline(true); + app.setAppStarted(true); app.clearNativeCache(); deleteFileCache(); diff --git a/app/store/index.js b/app/store/index.js index 91ad88290..e50d74054 100644 --- a/app/store/index.js +++ b/app/store/index.js @@ -14,7 +14,6 @@ import EventEmitter from 'mattermost-redux/utils/event_emitter'; import {NavigationTypes, ViewTypes} from 'app/constants'; import appReducer from 'app/reducers'; import {throttle} from 'app/utils/general'; -import networkConnectionListener from 'app/utils/network'; import {createSentryMiddleware} from 'app/utils/sentry/middleware'; import mattermostBucket from 'app/mattermost_bucket'; @@ -141,7 +140,6 @@ export default function configureAppStore(initialState) { return effect(); }, - detectNetwork: (callback) => networkConnectionListener(callback), persist: (store, options) => { const persistor = persistStore(store, {storage: AsyncStorage, ...options}, () => { store.dispatch({ diff --git a/app/utils/network.js b/app/utils/network.js index cd5fee7f3..dfe1d45e0 100644 --- a/app/utils/network.js +++ b/app/utils/network.js @@ -10,18 +10,17 @@ import {Client4} from 'mattermost-redux/client'; import mattermostBucket from 'app/mattermost_bucket'; import LocalConfig from 'assets/config'; -const PING_TIMEOUT = 10000; +const PING_TIMEOUT = 3000; let certificate = ''; -let checking = false; +let previousState; export async function checkConnection(isConnected) { - if (!Client4.getBaseRoute().startsWith('http') || checking) { + if (!Client4.getBaseRoute().startsWith('http')) { // If we don't have a server yet, return the default implementation - return isConnected; + return {hasInternet: isConnected, serverReachable: false}; } // Ping the Mattermost server to detect if the we have network connection even if the websocket cannot connect - checking = true; const server = `${Client4.getBaseRoute()}/system/ping?time=${Date.now()}`; const config = { @@ -36,32 +35,33 @@ export async function checkConnection(isConnected) { try { await RNFetchBlob.config(config).fetch('GET', server); - checking = false; - return true; + return {hasInternet: isConnected, serverReachable: true}; } catch (error) { - checking = false; - return false; + return {hasInternet: isConnected, serverReachable: false}; } } function handleConnectionChange(onChange) { return async (isConnected) => { - // Set device internet connectivity immediately - onChange(isConnected); + if (isConnected !== previousState) { + previousState = isConnected; - // Check if connected to server - const result = await checkConnection(isConnected); - onChange(result); + // Check if connected to server + const result = await checkConnection(isConnected); + onChange(result); + } }; } export default function networkConnectionListener(onChange) { const connectionChanged = handleConnectionChange(onChange); - NetInfo.isConnected.addEventListener('connectionChange', connectionChanged); - NetInfo.isConnected.fetch().then(connectionChanged); + NetInfo.isConnected.fetch().then((isConnected) => { + NetInfo.isConnected.addEventListener('connectionChange', connectionChanged); + connectionChanged(isConnected); + }); - const removeEventListener = () => NetInfo.isConnected.removeEventListener('connectionChange', connectionChanged); // eslint-disable-line + const removeEventListener = () => NetInfo.isConnected.removeEventListener('connectionChange', connectionChanged); return { removeEventListener, diff --git a/assets/base/i18n/en.json b/assets/base/i18n/en.json index 5456b84ae..46cb47185 100644 --- a/assets/base/i18n/en.json +++ b/assets/base/i18n/en.json @@ -325,7 +325,7 @@ "mobile.notification.in": " in ", "mobile.offlineIndicator.connected": "Connected", "mobile.offlineIndicator.connecting": "Connecting...", - "mobile.offlineIndicator.offline": "Cannot connect to the server. Retrying in {seconds} seconds", + "mobile.offlineIndicator.offline": "No internet connection", "mobile.open_dm.error": "We couldn't open a direct message with {displayName}. Please check your connection and try again.", "mobile.open_gm.error": "We couldn't open a group message with those users. Please check your connection and try again.", "mobile.open_unknown_channel.error": "Unable to join the channel. Please reset the cache and try again.", diff --git a/package-lock.json b/package-lock.json index a4e067975..293da7379 100644 --- a/package-lock.json +++ b/package-lock.json @@ -8229,8 +8229,8 @@ "integrity": "sha1-izqsWIuKZuSXXjzepn97sylgH6w=" }, "mattermost-redux": { - "version": "github:mattermost/mattermost-redux#f815f6bdc56003e9feac1e82f3d4ae52656eb8bd", - "from": "github:mattermost/mattermost-redux#f815f6bdc56003e9feac1e82f3d4ae52656eb8bd", + "version": "github:mattermost/mattermost-redux#b1f68abe9ee70540aeee5ffc755d94248c358feb", + "from": "github:mattermost/mattermost-redux#b1f68abe9ee70540aeee5ffc755d94248c358feb", "requires": { "deep-equal": "1.0.1", "eslint-plugin-header": "1.2.0", diff --git a/package.json b/package.json index 7545943ae..4945681e2 100644 --- a/package.json +++ b/package.json @@ -17,7 +17,7 @@ "intl": "1.2.5", "jail-monkey": "1.0.0", "jsc-android": "236355.1.0", - "mattermost-redux": "github:mattermost/mattermost-redux#f815f6bdc56003e9feac1e82f3d4ae52656eb8bd", + "mattermost-redux": "github:mattermost/mattermost-redux#b1f68abe9ee70540aeee5ffc755d94248c358feb", "mime-db": "1.37.0", "moment-timezone": "0.5.23", "prop-types": "15.6.2",