Skip to content
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

Feature: Display backend unreachability message #38377

Merged
merged 33 commits into from
May 14, 2024
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
37cf997
display backend unreachability message
tienifr Mar 15, 2024
4b24bd9
show status page hostname only
tienifr Mar 15, 2024
7e8cf7f
adjust polling timeout and status page link style
tienifr Mar 15, 2024
c7b627d
update comment
tienifr Mar 15, 2024
3da4432
Merge branch 'main' of https://github.com/tienifr/App into fix/37565
tienifr Mar 18, 2024
f3be100
update spanish copy
tienifr Mar 18, 2024
6fa6d52
Merge branch 'main' of https://github.com/tienifr/App into fix/37565
tienifr Mar 18, 2024
d3faf92
Update Espanol copy
tienifr Mar 18, 2024
8f84a24
Merge branch 'main' of https://github.com/tienifr/App into fix/37565
tienifr Mar 19, 2024
aae3f37
update comments
tienifr Mar 19, 2024
eb5094d
Merge branch 'main' into fix/37565
tienifr Mar 20, 2024
4c84f89
refactor: extract subscribeToBackendReachability and modify minor com…
tienifr Mar 20, 2024
0b5dec8
refactor: add returns doc
tienifr Mar 20, 2024
b7be2f2
refactor: return cleanup interval function
tienifr Mar 20, 2024
292656e
Merge branch 'main' of https://github.com/tienifr/App into fix/37565
tienifr Mar 29, 2024
ddc17c8
Merge branch 'main' of https://github.com/tienifr/App into fix/37565
tienifr Apr 1, 2024
4967911
remove redundant comment
tienifr Apr 1, 2024
4213ccf
fix typecheck
tienifr Apr 1, 2024
5c86046
rename constant
tienifr Apr 1, 2024
5de949b
Modify comment
tienifr Apr 2, 2024
3f1cf05
Merge branch 'main' of https://github.com/tienifr/App into fix/37565
tienifr Apr 5, 2024
2f8e345
Merge branch 'main' of https://github.com/tienifr/App into fix/37565
tienifr Apr 12, 2024
bffd5f0
implement internet reachability for android
tienifr Apr 12, 2024
01aafb0
modify fetch logic
tienifr Apr 12, 2024
c851a49
modify comment
tienifr Apr 12, 2024
045047f
Merge branch 'main' of https://github.com/tienifr/App into fix/37565
tienifr Apr 23, 2024
17b6aad
Merge branch 'main' into fix/37565
tienifr May 2, 2024
3103489
set network status
tienifr May 2, 2024
ab835c9
Merge branch 'main' into fix/37565
tienifr May 6, 2024
3122072
fix isBackendReachable flickering
tienifr May 6, 2024
17a5965
modify comment
tienifr May 6, 2024
fb78adc
Merge branch 'main' of https://github.com/tienifr/App into fix/37565
tienifr May 8, 2024
ce38e99
Merge branch 'main' into fix/37565
tienifr May 14, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/CONST.ts
Original file line number Diff line number Diff line change
Expand Up @@ -521,6 +521,7 @@ const CONST = {
EMPTY_ARRAY,
EMPTY_OBJECT,
USE_EXPENSIFY_URL,
STATUS_EXPENSIFY_URL: 'https://status.expensify.com',
GOOGLE_MEET_URL_ANDROID: 'https://meet.google.com',
GOOGLE_DOC_IMAGE_LINK_MATCH: 'googleusercontent.com',
IMAGE_BASE64_MATCH: 'base64',
Expand Down Expand Up @@ -950,13 +951,14 @@ const CONST = {
MAX_RETRY_WAIT_TIME_MS: 10 * 1000,
PROCESS_REQUEST_DELAY_MS: 1000,
MAX_PENDING_TIME_MS: 10 * 1000,
REACHABILITY_TIMEOUT_MS: 60 * 1000,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, why timeout? Timeout is the time until we give up some request or operation.

What about BACKEND_REACHABILITY_CHECK_INTERVAL_MS ? If this name feels too long, it could be shortened to BACKEND_CHECK_INTERVAL_MS.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tienifr Bump on this minor comment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot to push the changes. Updated.

MAX_REQUEST_RETRIES: 10,
},
WEEK_STARTS_ON: 1, // Monday
DEFAULT_TIME_ZONE: {automatic: true, selected: 'America/Los_Angeles'},
DEFAULT_ACCOUNT_DATA: {errors: null, success: '', isLoading: false},
DEFAULT_CLOSE_ACCOUNT_DATA: {errors: null, success: '', isLoading: false},
DEFAULT_NETWORK_DATA: {isOffline: false},
DEFAULT_NETWORK_DATA: {isOffline: false, isBackendReachable: true},
FORMS: {
LOGIN_FORM: 'LoginForm',
VALIDATE_CODE_FORM: 'ValidateCodeForm',
Expand Down
6 changes: 4 additions & 2 deletions src/Expensify.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -138,8 +138,10 @@ function Expensify({
// Initialize this client as being an active client
ActiveClientManager.init();

// Used for the offline indicator appearing when someone is offline
NetworkConnection.subscribeToNetInfo();
// Used for the offline indicator appearing when someone is offline or backend is unreachable
const unsubscribeNetInfo = NetworkConnection.subscribeToNetInfo();

return () => unsubscribeNetInfo();
}, []);

useEffect(() => {
Expand Down
26 changes: 23 additions & 3 deletions src/components/OfflineIndicator.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,11 @@ import useTheme from '@hooks/useTheme';
import useThemeStyles from '@hooks/useThemeStyles';
import useWindowDimensions from '@hooks/useWindowDimensions';
import variables from '@styles/variables';
import CONST from '@src/CONST';
import Icon from './Icon';
import * as Expensicons from './Icon/Expensicons';
import Text from './Text';
import TextLink from './TextLink';

type OfflineIndicatorProps = {
/** Optional styles for container element that will override the default styling for the offline indicator */
Expand All @@ -23,7 +25,7 @@ function OfflineIndicator({style, containerStyles}: OfflineIndicatorProps) {
const theme = useTheme();
const styles = useThemeStyles();
const {translate} = useLocalize();
const {isOffline} = useNetwork();
const {isOffline, isBackendReachable} = useNetwork();
const {isSmallScreenWidth} = useWindowDimensions();

const computedStyles = useMemo((): StyleProp<ViewStyle> => {
Expand All @@ -34,7 +36,7 @@ function OfflineIndicator({style, containerStyles}: OfflineIndicatorProps) {
return isSmallScreenWidth ? styles.offlineIndicatorMobile : styles.offlineIndicator;
}, [containerStyles, isSmallScreenWidth, styles.offlineIndicatorMobile, styles.offlineIndicator]);

if (!isOffline) {
if (!isOffline && isBackendReachable) {
return null;
}

Expand All @@ -46,7 +48,25 @@ function OfflineIndicator({style, containerStyles}: OfflineIndicatorProps) {
width={variables.iconSizeSmall}
height={variables.iconSizeSmall}
/>
<Text style={[styles.ml3, styles.chatItemComposeSecondaryRowSubText]}>{translate('common.youAppearToBeOffline')}</Text>
<Text style={[styles.ml3, styles.chatItemComposeSecondaryRowSubText]}>
{
// If we reversed the ternary, unreachability message would always show even when offline
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't like this comment. Unless you feel very strongly about it, I would just remove it. It's normal that you can't just flip the ternary expression and expect it to work.

If I really wanted to make this logic more clear, I'd consider extracting the whole markup to a separate component, like BaseOfflineIndicator, which accepts children.

Then, we could update the return logic of the OfflineIndicator:

if (isOffline) {
  return <BaseOfflineIndicator>{translate('common.youAppearToBeOffline')}</BaseOfflineIndicator>
} else if (!isBackendReachable) {
  return (
    <BaseOfflineIndicator>
        {translate('common.weMightHaveProblem')}
        <TextLink
            href={CONST.STATUS_EXPENSIFY_URL}
            style={[styles.chatItemComposeSecondaryRowSubText, styles.link]}
        >
            {new URL(CONST.STATUS_EXPENSIFY_URL).host}
        </TextLink>
        .
    </BaseOfflineIndicator>
  );
} else {
  return null;
}

This is clear without comments.

I'll leave it to you: either just remove the comment, or (if you like the idea) you can also do the suggested refactoring to make the logic clear without any comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the comment.

isOffline ? (
translate('common.youAppearToBeOffline')
) : (
<>
{translate('common.weMightHaveProblem')}
<TextLink
href={CONST.STATUS_EXPENSIFY_URL}
style={[styles.chatItemComposeSecondaryRowSubText, styles.link]}
>
{new URL(CONST.STATUS_EXPENSIFY_URL).host}
</TextLink>
.
</>
)
}
</Text>
</View>
);
}
Expand Down
6 changes: 3 additions & 3 deletions src/hooks/useNetwork.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,13 @@ type UseNetworkProps = {
onReconnect?: () => void;
};

type UseNetwork = {isOffline: boolean};
type UseNetwork = {isOffline: boolean; isBackendReachable: boolean};

export default function useNetwork({onReconnect = () => {}}: UseNetworkProps = {}): UseNetwork {
const callback = useRef(onReconnect);
callback.current = onReconnect;

const {isOffline} = useContext(NetworkContext) ?? CONST.DEFAULT_NETWORK_DATA;
const {isOffline, isBackendReachable} = useContext(NetworkContext) ?? CONST.DEFAULT_NETWORK_DATA;
const prevOfflineStatusRef = useRef(isOffline);
useEffect(() => {
// If we were offline before and now we are not offline then we just reconnected
Expand All @@ -29,5 +29,5 @@ export default function useNetwork({onReconnect = () => {}}: UseNetworkProps = {
prevOfflineStatusRef.current = isOffline;
}, [isOffline]);

return {isOffline: isOffline ?? false};
return {isOffline: isOffline ?? false, isBackendReachable: isBackendReachable ?? true};
}
1 change: 1 addition & 0 deletions src/languages/en.ts
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,7 @@ export default {
conciergeHelp: 'Please reach out to Concierge for help.',
maxParticipantsReached: ({count}: MaxParticipantsReachedParams) => `You've selected the maximum number (${count}) of participants.`,
youAppearToBeOffline: 'You appear to be offline.',
weMightHaveProblem: 'We might have a problem. Check out ',
thisFeatureRequiresInternet: 'This feature requires an active internet connection to be used.',
areYouSure: 'Are you sure?',
verify: 'Verify',
Expand Down
1 change: 1 addition & 0 deletions src/languages/es.ts
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,7 @@ export default {
conciergeHelp: 'Por favor, contacta con Concierge para obtener ayuda.',
maxParticipantsReached: ({count}: MaxParticipantsReachedParams) => `Has seleccionado el número máximo (${count}) de participantes.`,
youAppearToBeOffline: 'Parece que estás desconectado.',
weMightHaveProblem: 'Peude que te tengamos un problema. Echa un vistazo a ',
thisFeatureRequiresInternet: 'Esta función requiere una conexión a Internet activa para ser utilizada.',
areYouSure: '¿Estás seguro?',
verify: 'Verifique',
Expand Down
65 changes: 38 additions & 27 deletions src/libs/NetworkConnection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ function setOfflineStatus(isCurrentlyOffline: boolean): void {
// When reconnecting, ie, going from offline to online, all the reconnection callbacks
// are triggered (this is usually Actions that need to re-download data from the server)
if (isOffline && !isCurrentlyOffline) {
NetworkActions.setIsBackendReachable(true);
triggerReconnectionCallbacks('offline status changed');
}

Expand Down Expand Up @@ -68,46 +69,56 @@ Onyx.connect({
});

/**
* Set up the event listener for NetInfo to tell whether the user has
* internet connectivity or not. This is more reliable than the Pusher
* `disconnected` event which takes about 10-15 seconds to emit.
* Monitor internet connectivity and perform periodic backend reachability checks
*/
function subscribeToNetInfo(): void {
// Note: We are disabling the configuration for NetInfo when using the local web API since requests can get stuck in a 'Pending' state and are not reliable indicators for "offline".
function subscribeToNetInfo() {
tienifr marked this conversation as resolved.
Show resolved Hide resolved
let backendReachabilityCheckInterval: NodeJS.Timeout;
tienifr marked this conversation as resolved.
Show resolved Hide resolved
// Note: We are disabling the reachability check when using the local web API since requests can get stuck in a 'Pending' state and are not reliable indicators for "offline".
// If you need to test the "recheck" feature then switch to the production API proxy server.
if (!CONFIG.IS_USING_LOCAL_WEB) {
// Calling NetInfo.configure (re)checks current state. We use it to force a recheck whenever we (re)subscribe
NetInfo.configure({
// By default, NetInfo uses `/` for `reachabilityUrl`
// When App is served locally (or from Electron) this address is always reachable - even offline
// Set interval to periodically (re)check backend status
backendReachabilityCheckInterval = setInterval(() => {
// Offline status also implies backend unreachability
if (isOffline) {
return;
}
// Using the API url ensures reachability is tested over internet
reachabilityUrl: `${CONFIG.EXPENSIFY.DEFAULT_API_ROOT}api?command=Ping`,
reachabilityMethod: 'GET',
reachabilityTest: (response) => {
if (!response.ok) {
return Promise.resolve(false);
}
return response
.json()
.then((json) => Promise.resolve(json.jsonCode === 200))
.catch(() => Promise.resolve(false));
},

// If a check is taking longer than this time we're considered offline
reachabilityRequestTimeout: CONST.NETWORK.MAX_PENDING_TIME_MS,
});
fetch(`${CONFIG.EXPENSIFY.DEFAULT_API_ROOT}api?command=Ping`, {
method: 'GET',
cache: 'no-cache',
})
.then((response) => {
if (!response.ok) {
return Promise.resolve(false);
}
return response
.json()
.then((json) => Promise.resolve(json.jsonCode === 200))
.catch(() => Promise.resolve(false));
})
.then(NetworkActions.setIsBackendReachable)
.catch(() => NetworkActions.setIsBackendReachable(false));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure that .then / .catch / ... operators are more readable than the async/await notation?

We're good with the async syntax in .ts files.

Copy link
Contributor Author

@tienifr tienifr Mar 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're good with the async syntax in .ts files.

Oh that's new to me. AFAIK, async/await is forbidden in Style guidelines. Turned out it's fine for TS.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cubuspl42 Seems like we cannot use async await, except for workflow and test files:

App/.eslintrc.js

Lines 270 to 272 in 73ecb3f

files: ['workflow_tests/**/*.{js,jsx,ts,tsx}', 'tests/**/*.{js,jsx,ts,tsx}', '.github/**/*.{js,jsx,ts,tsx}'],
rules: {
'@lwc/lwc/no-async-await': 'off',

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad. I asked on Slack about it. But of course, it's not blocking us.

}, CONST.NETWORK.REACHABILITY_TIMEOUT_MS);
}

// Subscribe to the state change event via NetInfo so we can update
// whether a user has internet connectivity or not.
NetInfo.addEventListener((state) => {
/**
* Set up the event listener for NetInfo to tell whether the user has
* internet connectivity or not. This is more reliable than the Pusher
* `disconnected` event which takes about 10-15 seconds to emit.
*/
tienifr marked this conversation as resolved.
Show resolved Hide resolved
const unsubscribeNetInfo = NetInfo.addEventListener((state) => {
tienifr marked this conversation as resolved.
Show resolved Hide resolved
Log.info('[NetworkConnection] NetInfo state change', false, {...state});
if (shouldForceOffline) {
Log.info('[NetworkConnection] Not setting offline status because shouldForceOffline = true');
return;
}
setOfflineStatus((state.isInternetReachable ?? false) === false);
});

return () => {
clearInterval(backendReachabilityCheckInterval);
unsubscribeNetInfo();
};
}

function listenForReconnect() {
Expand Down
6 changes: 5 additions & 1 deletion src/libs/actions/Network.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
import Onyx from 'react-native-onyx';
import ONYXKEYS from '@src/ONYXKEYS';

function setIsBackendReachable(isBackendReachable: boolean) {
Onyx.merge(ONYXKEYS.NETWORK, {isBackendReachable});
}

function setIsOffline(isOffline: boolean) {
Onyx.merge(ONYXKEYS.NETWORK, {isOffline});
}
Expand All @@ -20,4 +24,4 @@ function setShouldFailAllRequests(shouldFailAllRequests: boolean) {
Onyx.merge(ONYXKEYS.NETWORK, {shouldFailAllRequests});
}

export {setIsOffline, setShouldForceOffline, setShouldFailAllRequests, setTimeSkew};
export {setIsBackendReachable, setIsOffline, setShouldForceOffline, setShouldFailAllRequests, setTimeSkew};
2 changes: 2 additions & 0 deletions src/types/onyx/Network.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ type Network = {
/** Is the network currently offline or not */
isOffline: boolean;

isBackendReachable: boolean;

/** Should the network be forced offline */
shouldForceOffline?: boolean;

Expand Down
Loading