Feature: Display backend unreachability message #38377
Conversation
|
@cubuspl42 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
src/languages/es.ts
Outdated
| @@ -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: 'We might have a problem. Check out ', | |||
There was a problem hiding this comment.
Copy was verified here.
There was a problem hiding this comment.
I think you forgot to actually use that copy!
|
Currently I found no way to manually block specific network requests on native apps the way we did on web with the support of DevTools. My only solution was to hard-code the reachability URL to make it fail. |
|
@tienifr If there's no better way, we can test it this way on Native. In rare cases, we test things by applying a small code change. Please specify this technique in the "Tests" steps. |
src/languages/es.ts
Outdated
| @@ -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: 'We might have a problem. Check out ', | |||
There was a problem hiding this comment.
I think you forgot to actually use that copy!
src/libs/NetworkConnection.ts
Outdated
| 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)); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@cubuspl42 Seems like we cannot use async await, except for workflow and test files:
Lines 270 to 272 in 73ecb3f
There was a problem hiding this comment.
My bad. I asked on Slack about it. But of course, it's not blocking us.
|
@cubuspl42 I updated all the comments and replied to your feedbacks above. |
|
@cubuspl42 I extracted local |
src/libs/NetworkConnection.ts
Outdated
| reachabilityMethod: 'GET', | ||
| reachabilityTest: (response) => { | ||
| function subscribeToBackendReachability() { | ||
| return setInterval(() => { |
There was a problem hiding this comment.
Maybe we could return a function like...
const intervalId = setInterval(...);
return () => {
clearInterval(intervalId);
};We'd call our util like this...
const unsubscribeFromBackendReachability = !CONFIG.IS_USING_LOCAL_WEB ? subscribeToBackendReachability() : undefined;This would be symmetric with how we handle NetInfo, and also cleanly abstract the specific API we use in the implementation.
There was a problem hiding this comment.
unsubscribeFromBackendReachability could be called like this:
unsubscribeFromBackendReachability?.();
src/libs/NetworkConnection.ts
Outdated
| // 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. |
There was a problem hiding this comment.
state and are not reliable indicators for "offline".
This is outdated 🙁
src/CONST.ts
Outdated
| @@ -965,13 +966,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, | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@tienifr Bump on this minor comment
There was a problem hiding this comment.
Forgot to push the changes. Updated.
By these points, do you mean we implement our own logic for internet, connectivity and backend reachability and ignore I myself think the current logic is more readable and understandable. It's just simple: Backend check >> Internet check (if backend failed). We don't have redundant requests, only one |
|
Taking over as C+ |
|
Conflicts to resolve here! Assigned you as the reviewer @DylanDylann. |
| }) | ||
| .catch(() => { | ||
| checkInternetReachability().then((isInternetReachable: boolean) => { | ||
| setOfflineStatus(!isInternetReachable); |
There was a problem hiding this comment.
@tienifr We implemented subscribeToNetworkStatus to detect the onl/off status by using NetInfo.addEventListener. Why do we need to call setOfflineStatus here?
There was a problem hiding this comment.
@DylanDylann The reason for this check is here: #38377 (comment). Please raise questions if any.
There was a problem hiding this comment.
I believe that you might wonder whether react-native-netinfo's event listener collided with our own custom checkInternetReachability. Note that we only run this check on Android and we do not trigger it if the system is already offline here.
|
Whoops, I can test on the emulator |
|
@tienifr BUG: Flicker when going online. When going online the indicator message turns into Screen.Recording.2024-05-02.at.16.06.02.mov |
|
Thanks @DylanDylann I found the root cause and solution for this, but need time to retest the flow on all platforms. I'll push the update today. |
|
@DylanDylann That issue happened when the BE was previously being unreachable, then when we turned back online, the Lines 32 to 33 in 8383d80 Now My solution is that if network status is unknown, we should treat it as if we're online and backend is reachable here. That's what we already did with |
|
Reviewing today |
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2024-05-08.at.15.42.50.mp4Android: mWeb ChromeScreen.Recording.2024-05-08.at.15.39.12.moviOS: NativeScreen.Recording.2024-05-08.at.15.34.14.moviOS: mWeb Safarisafari.movMacOS: Chrome / SafariScreen.Recording.2024-05-08.at.14.59.56.movMacOS: DesktopScreen.Recording.2024-05-08.at.15.14.20.mov |
|
Hi @aldo-expensify, merge freeze is over, I think we're good to proceed this. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/aldo-expensify in version: 1.4.74-0 🚀
|
|
🚀 Deployed to production by https://github.com/chiragsalian in version: 1.4.74-6 🚀
|
There was a problem hiding this comment.
I had some question in this slack thread: https://expensify.slack.com/archives/C05LX9D6E07/p1717631386394629 and found some things in this PR that I didn't quite understand.
| import type InternetReachabilityCheck from './types'; | ||
|
|
||
| export default function checkInternetReachability(): InternetReachabilityCheck { | ||
| return Promise.resolve(true); |
There was a problem hiding this comment.
Why is this a no-op for all other platforms? Shouldn't this at least be doing a NetInfo.fetch()?
There was a problem hiding this comment.
We only need to manually check for internet reachability on Android. This is due to a limitation on Android OS:
App/src/libs/checkInternetReachability/index.android.ts
Lines 4 to 7 in e9dc2ef
Other platforms does not have that problem so we use NetInfo's own check:
App/src/libs/NetworkConnection.ts
Line 147 in e9dc2ef
| checkInternetReachability().then((isInternetReachable: boolean) => { | ||
| setOfflineStatus(!isInternetReachable); | ||
| setNetWorkStatus(isInternetReachable); | ||
| NetworkActions.setIsBackendReachable(false); |
There was a problem hiding this comment.
Why is this inside the promise for checkInternetReachability()? It doesn't do anything with the isInternetReachable value.
There was a problem hiding this comment.
You meant the setIsBackendReachable, didn't you?
Backend unreachability might mean internet unreachability so we need to check the internet first to clarify whether the root cause is internet or backend failure.
If we move the setIsBackendReachable out of the promise, we would have We might have problem ... appear briefly before You appear to be offline when the internet was down.
Details
When user is offline, we display offline message (
You appear to be offline.), when user is online but our backend is unreachable, we displayWe might have a problem. Check out status.expensify.commessage.Fixed Issues
$ #37565
PROPOSAL: #37565 (comment)
Tests
Network Devtools
Web: Open DevTools >> Network
Native: Toggle RN dev menu by
CMD + D>> Open Element Inspector >> NetworkBlock network request
Chrome
Open DevTools >> More tools >> Network request blocking >> Enable network request blocking >> Add network request blocking pattern as
https://dev.new.expensify.com:8082/apiSafari
Blocktype and URL ashttps://dev.new.expensify.com:8082/apiwith Regular Expression enabledNative
Hard-code the reachability URL here to an invalid URL.
Pingcommand is called every 60 seconds (see Network Devtools)https://dev.new.expensify.com:8082/apirequest to make backend unreachable (see Block network request)We might have a problem. Check out status.expensify.com.message appears and the status page URL can be opened=====
You appear to be offline.message appears=====
Offline tests
Same as Tests
QA Steps
NA
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Screen.Recording.2024-03-18.at.18.30.51-compressed.mov
Android: mWeb Chrome
Screen.Recording.2024-03-18.at.18.16.09-compressed.mov
iOS: Native
Screen.Recording.2024-03-18.at.18.22.31-compressed.mov
iOS: mWeb Safari
Untitled.mov
MacOS: Chrome / Safari
Untitled.2.mov
MacOS: Desktop
Untitled.2.mov