Closed m-natarajan closed 3 months ago
Triggered auto assignment to @trjExpensify (Bug
), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
Triggered auto assignment to @NickTooker (Waiting for copy
), see https://stackoverflow.com/c/expensify/questions/7025/ for more details.
Wrong message appears when the Expensify site is going down
New Feature but as discussed in the comment this is what we need to do.
Firstly, we need to fix the logical error of using the backend endpoint for determining isOffline
:
https://github.com/Expensify/App/blob/da7697734c1f759786eba0a643a062b4e39a47ad/src/libs/NetworkConnection.ts#L84-L97 and after removing the backend ping logic.
We need to add a new method that seperates the backend ping logic seperately like this :
async function testBackendReachability() {
try {
const response = await fetch(`${CONFIG.EXPENSIFY.DEFAULT_API_ROOT}api?command=Ping`);
return response.ok;
} catch {
return false;
}
}
So, that we have a seperate logic to ping backend seperately now.
Now, since we have seperated backend pinging logic seperately. Here :
https://github.com/Expensify/App/blob/da7697734c1f759786eba0a643a062b4e39a47ad/src/libs/NetworkConnection.ts#L11-L12
we can add a new boolean Onyx property called isBackendUnavailable
and initially its value will be false. So that we don't directly say that backend is unavailable.
let isBackendUnavailable = false;
Then, we need to update the setOfflineStatus
function here :
https://github.com/Expensify/App/blob/da7697734c1f759786eba0a643a062b4e39a47ad/src/libs/NetworkConnection.ts#L36-L46
to also set isBackendUnavailable
in Onyx like this :
function setOfflineStatus(isCurrentlyOffline: boolean) {
// Existing logic to set isOffline
const isBackendUp = await testBackendReachability();
isBackendUnavailable = !isBackendUp;
// Dispatch new action to set isBackendUnavailable in Onyx
}
NetInfo.addEventListener
i.e. here :
https://github.com/Expensify/App/blob/da7697734c1f759786eba0a643a062b4e39a47ad/src/libs/NetworkConnection.ts#L103-L111we need to call testBackendReachability()
on network changes like this :
NetInfo.addEventListener(state => {
await setOfflineStatus(state.isInternetReachable === false);
})
This will help us to separates the offline and backend unavailable logic, removing the dependency on the backend endpoint for determining offline status.
Also, at last in en.ts
file we can add a new key and value for saying this We might have a problem. Check out status.expensify.com
when the Expensify App is literally down.
N/A
How about:
We might have a problem. Check out status.expensify.com.
Wrong message appears when the Expensify site is going down
We need to update the Copy in Offline Indicator
here:
https://github.com/Expensify/App/blob/39a257d1aa850e875bcf604f6c7b039527234952/src/components/OfflineIndicator.tsx#L49
We will be creating a new key in src/languages
according to the approved copy:
offlineMessage: "We might have a problem. Check out "
Then use it here: https://github.com/Expensify/App/blob/39a257d1aa850e875bcf604f6c7b039527234952/src/components/OfflineIndicator.tsx#L49
We will be rendering the link in the approved copy with the TextLink
component:
<TextLink onPress={() => Link.openExternalLink('https://status.expensify.com/')}>
status.expensify.com
</TextLink>
This is the approved copy @neonbhai.
We might have a problem. Check out status.expensify.com.
Are the proposals being reviewed now? @NickTooker
Sounds like we have the copy, adding External
to get a C+ assigned.
Job added to Upwork: https://www.upwork.com/jobs/~01ed912188740cf497
Triggered auto assignment to Contributor-plus team member for initial proposal review - @cubuspl42 (External
)
Current assignee @trjExpensify is eligible for the NewFeature assigner, not assigning anyone new.
We haven't implemented this feature previously, so adding New feature
label. I've also updated the OP with the expected results copy.
@godofoutcasts94 ~Interesting, but I don't think we're looking for something this complex~
Edit: It seems that we indeed aim to implement a feature like this
~I approve the proposal by @neonbhai~
~C+ reviewed π π π~
Edit: I misunderstood the problem
Triggered auto assignment to @aldo-expensify, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
@cubuspl42 just checking, how are we distinguishing between the site being down and the user being offline to show the correct message?
Hey @trjExpensify I think the component OfflineIndicator.tsx
only deals with the situation when the user is offline as it uses useNetwork
to just show that if the device is offline or not and w.r.t it will display the message. So I believe that it won't be able to distinguish between Expensify Being Down or the user being offline.
Cool, so that's not what we want is it, @flodnv? When someone is offline or experiencing local network difficulties "You appear to be offline" is accurate. When our site is down, showing "You appear to be offline" is not accurate, and that's what we're looking to improve here somehow.
Cool, so that's not what we want is it, @flodnv? When someone is offline or experiencing local network difficulties "You appear to be offline" is accurate. When our site is down, showing "You appear to be offline" is not accurate, and that's what we're looking to improve here somehow.
Hey! What if we make some changes in useNetwork
hook like this :
export type NetworkStatus = {
isSiteDown: boolean;
};
export const STATUS_CHECK_INTERVAL = 10000;
export function useNetworkStatus(): NetworkStatus {
const [isSiteDown, setIsSiteDown] = useState(false);
useEffect(() => {
const interval = setInterval(() => {
fetch('https://status.expensify.com/')
.then(handleFetchResponse)
.catch(handleFetchError);
}, STATUS_CHECK_INTERVAL);
return () => clearInterval(interval);
}, []);
useEffect(() => {
if (isSiteDown) {
logSiteDown();
}
}, [isSiteDown]);
return { isSiteDown };
}
function handleFetchResponse(res: Response) {
if (!res.ok) {
setIsSiteDown(true);
} else {
setIsSiteDown(false);
}
}
function handleFetchError() {
setIsSiteDown(true);
}
function logSiteDown() {
console.log('Site might be down - check status.expensify.com');
}
The above code is just for reference, ofcourse it won't be displayed in the console. A seperate key in en.ts
file will be used to display about expensify down status.
You appear to be offline appears confusing the user that they have a network problem when really it's the site.
In here, we're using our own back-end endpoint as the reachabilityUrl
to validate Internet connectivity (in case of local web it will be the /
path as indicated here). This is not ideal because our endpoint itself could be down, leading to OfflineIndicator
showing for users even though the user is online and can access other sites, just that our own site is down.
This can be easily reproduced by running the web locally and Cmd + C
in the Terminal to force turn off the back-end endpoint, the OfflineIndicator
will also show in this case.
We should define a method to set back-end reachability in Network
here
function setIsBackendReachable(isBackendReachable: boolean) {
Onyx.merge(ONYXKEYS.NETWORK, {isBackendReachable});
}
And also expose the isBackendReachable
in useNetwork
here
const {isOffline, isBackendReachable} = useContext(NetworkContext) ?? CONST.DEFAULT_NETWORK_DATA;
And here
return {isOffline: isOffline ?? false, isBackendReachable: isBackendReachable ?? true};
We should turn the presumably-offline-check here to a dedicated back-end reachability check instead. The below pseudocode will use a setInterval
to do this check every 15 seconds, similar to how the @react-native-community/netinfo
itself does the check:
const BACKEND_REACHABILITY_CHECK_INTERVAL = 15000;
const backendReachabilityCheckInterval = setInterval(() => {
// If we're offline, we don't need to check for back-end reachability because it's of course not reachable
if (isOffline) {
return;
}
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);
}, BACKEND_REACHABILITY_CHECK_INTERVAL);
We're keeping the same `reachabilityUrl`, `reachabilityMethod`, `reachabilityTest`, just that we use it explicitly instead of passing to the `NetInfo`. Since `NetInfo` does not have custom configurations now, it will use the default device-specific offline checks, which will rely on the device itself or an always-up Google URL (see [here](https://www.npmjs.com/package/@react-native-community/netinfo#netinfoconfiguration)). We can be confident now that our back-end being down will lead to the user see the `You're offline` message.
Above we can also see that we'll set the back-end reachability status found by using the `NetworkActions.setIsBackendReachable` call.
3. We want to show something like `We might have a problem. Check out [status.expensify.com](http://status.expensify.com/).` as mentioned [here](https://github.com/Expensify/App/issues/37565#issuecomment-1973872087), we will render a proper text copy (using `Text` and `TextLink`) for "We might have a problem. Check out [status.expensify.com](http://status.expensify.com/)" [here](https://github.com/Expensify/App/blob/39a257d1aa850e875bcf604f6c7b039527234952/src/components/OfflineIndicator.tsx#L49) if `isBackendReachable` is strictly `false` and `isOffline` is `false`.
4. In [here](https://github.com/Expensify/App/blob/767bb3c0ffb5e47c559bd062b64678fa8538027c/src/libs/NetworkConnection.ts#L36), if `isCurrentlyOffline` is `false` (meaning the user is now online), we should also reset the `isBackendReachable` by setting it to `true`. This is to make sure if the back-end is unavailable, then the user comes offline, when they come online, we should not show `back-end is down` (because it might not be at that time), instead we'll reset `isBackendReachable` to true and wait until the next back-end reachability check where we'll know the correct state.
### What alternative solutions did you explore? (Optional)
1. We can use a `reachabilityUrl` [here](https://github.com/Expensify/App/blob/af0f6a33369d1b9b9ac7aeb9000d706fa09c0711/src/libs/NetworkConnection.ts#L84) that's independent of our back-end system. Some potential candidates are:
- [Our site Atlassian Statuspage endpoint](https://status.expensify.com/api/v2/status.json), which will be independent from our back-end system by definition
- A zero-downtime public health check endpoint from an external service like Google, Cloudflare. (Just like how we ping `8.8.8.8` to check internet connectivity in Terminal)
3. Instead of defining in a `setInterval`, we can keep using the `NetInfo.configure` and set a `reachabilityTest` such that it will additionally fetch our back-end endpoint to update the back-end reachability, if the user is not offline.
<!---
ATTN: Contributor+
You are the first line of defense in making sure every proposal has a clear and easily understood problem with a "root cause". Do not approve any proposals that lack a satisfying explanation to the first two prompts. It is CRITICALLY important that we understand the root cause at a minimum even if the solution doesn't directly address it. When we avoid this step we can end up solving the wrong problems entirely or just writing hacks and workarounds.
Instructions for how to review a proposal:
1. Address each contributor proposal one at a time and address each part of the question one at a time e.g. if a solution looks acceptable, but the stated problem is not clear then you should provide feedback and make suggestions to improve each prompt before moving on to the next. Avoid responding to all sections of a proposal at once. Move from one question to the next each time asking the contributor to "Please update your original proposal and tag me again when it's ready for review".
2. Limit excessive conversation and moderate issues to keep them on track. If someone is doing any of the following things please kindly and humbly course-correct them:
- Posting PRs.
- Posting large multi-line diffs (this is basically a PR).
- Skipping any of the required questions.
- Not using the proposal template at all.
- Suggesting that an existing issue is related to the current issue before a problem or root cause has been established.
- Excessively wordy explanations.
3. Choose the first proposal that has a reasonable answer to all the required questions.
-->
Cool, so that's not what we want is it, @flodnv? When someone is offline or experiencing local network difficulties "You appear to be offline" is accurate
@cubuspl42 The selected proposal will cause this unwanted regression (showing "We might have a problem. Check out status.expensify.com") when the user is indeed offline. Would you mind reviewing my proposal to see if it's more suitable, thanks!
Wrong message appears when the Expensify site is going down
in here (for dev, but same thing for staging/production), we utilize our internal backend endpoint as the reachabilityUrl for validating Internet connectivity. when the site is not reachable, the OfflineIndicator
displays "You appear to be offline" for users even when they are online.
in OfflineIndicator we should use an isolated instance of a network manager class to check for internet connectivity, and keep the current global instance to check for site connectivity.
for this we should use useNetInfoInstance()
hook
import { useNetInfoInstance } from "@react-native-community/netinfo";
const {netInfo} = useNetInfoInstance();
in OfflineIndicator when isOffline
is true
(representing that the site is not reachable) we should check netInfo.isInternetReachable
to determine which message we should display:
<Text style={[styles.ml3, styles.chatItemComposeSecondaryRowSubText]}>{netInfo.isInternetReachable ? translate('common.weMightHaveProblem') : translate('common.youAppearToBeOffline')}</Text>
This solution works only if the site is reachable and have problems (500 status errors):
we should add isSiteDown
to network
in Onyx
,
here add:
isSiteDown: boolean;
in this lib define isSiteDown
and connect to it from Onyx, add here this:
let isSiteDown= false;
and here add:
shouldForceOffline = Boolean(network.shouldForceOffline);
when a request fails here with one of the serviceInterruptedStatuses
, we should set isSiteDown
true
in network
in Onyx:
Onyx.merge(ONYXKEYS.NETWORK, {isSiteDown: true});
when site is back, we should add an else case here to reset isSiteDown
to false when a request succeed:
} else {
if (isSiteDown) {
Onyx.merge(ONYXKEYS.NETWORK, {isSiteDown: false});
}
}
to display the correct message we should access to Onyx network
in OfflineIndicator, and when isOffline
is true
we should check isSiteDown
to determine which message we should display:
<Text style={[styles.ml3, styles.chatItemComposeSecondaryRowSubText]}>{network?.isSiteDown ? translate('common.weMightHaveProblem') : translate('common.youAppearToBeOffline')}</Text>
I'm sorry, I misunderstood the problem.
I asked on Slack (in the original bug thread) whether we could redefine this issue to just fixing the problem where we "blame" the user for being offline when they are indeed online, without adding any new copies.
Bumped @flodnv in thread as it was his idea. :)
It seems that we're likely continue with the original plan (i.e. handling offline and site-is-down separately).
@tienifr
Your proposal looks polished and well-thought-out. It makes it clear how you want to solve the problem on the network level, but I'm unsure about the technical part.
I think that the most natural way forward would be to make the code in NetworkConnection
double-channelled.
One channel for our backend endpoint, one channel for external endpoint. We could manage two flags, isBackendAvailable
and isExternalEndpointAvailable
From these flags, we could conclude (in pseudocode):
if isBackendAvailable -> all good
else if isExternalEndpointAvailable -> site's down
else -> you are offline
Problem:
The react-native-netinfo library we use is exteremely globals-based. You can't cleanly create two plain-JavaScript instances (State
s). There's a hook-based API, but we'd prefer a pure JavaScript API in my opinion.
@rayane-djouah
You suggested to use an "isolated" hook-based instance alongside the global instance. It could work, but I really don't feel that hooks are the way to go here.
@cubuspl42 the useNetInfoInstance()
hook by default will use native reachability checks, and on platforms which do not supply internet reachability natively it will use the https://clients3.google.com/generate_204
endpoint. reference link. I think that it's a better approach as we will not need to implement our own logic to determine internet reachability, and we can customize the instance configuration as per our needs.
The react-native-netinfo library we use is exteremely globals-based. You can't cleanly create two plain-JavaScript instances (States). There's a hook-based API, but we'd prefer a pure JavaScript API in my opinion.
@cubuspl42 I think we don't really need to have 2 instances of NetInfo
, because NetInfo
does a lot of things (periodically) and can introduce overheads to the app. Also, we only need to check external endpoint
if we already see that isBackendAvailable
is false
, so there's no need to check external endpoint
every few seconds.
if isBackendAvailable -> all good else if isExternalEndpointAvailable -> site's down else -> you are offline
To achieve this, and assuming we think it's ok to detect "site being down" with just 1 request like we currently does, I think a simple way is to:
reachabilityUrl
here as isisExternalEndpointAvailable
check in the reachabilityTest
here. A http fetch
would do (we can take what NetInfo does internally for reference)isBackendAvailable
response (provided by NetInfo
) and isExternalEndpointAvailable
(by HTTP fetch), we can conclude both the outcome of offline
check and site's down
check and set the states accordingly.@tienifr
I think we don't really need to have 2 instances of NetInfo, because NetInfo does a lot of things
Yeah, after giving it a thought my suggestion to consider using NetInfo for the backend availability check might not be reasonable
(periodically) and can introduce overheads to the app.
We might want to make the hand-made solution periodic too, I think.
Keep the current reachabilityUrl here as is
...but this is the part which I'm having doubts about.
As I understand it, you suggest to...
NetInfo
to check for Expensify backend availabilityWhy not the other way around? As you mentioned, NetInfo has extra functionality (like supporting platform-native APIs for determining if we're online) that is better suited for the "offline check" task, I think.
Do the additional isExternalEndpointAvailable check in the reachabilityTest
So you'd like to affect the boolean result of reachabilityTest
based on two different endpoints? I'm unsure about it, but maybe let's put this part of discussion on hold as the earlier points are more important IMO.
@rayane-djouah
the
useNetInfoInstance()
hook by default will use native reachability checks, and on platforms which do not [...]
As I understand it, NetInfo in general behaves like you described. I just don't think that using any hooks (and the NetInfo-provided hook specifically) is a good idea in the scope of this issue, as in my opinion we're looking for a app-scoped solution that would cleanly fit into the current Expensify code (NetworkConnection
class in specific).
@godofoutcasts94
Sorry, I missed this. Please phrase solutions in the form of proposals in the future, as it makes the process easier.
Honestly, I'd like to touch useNetwork
as little as possible. It's a hook with a lot of uses, and I presume that multiple instances of this hook are often mounted at the same time. I wouldn't like to do any periodic checks on the component level. Currently, I tend toward a solution put into NetworkConnection.ts
. I think we should do everything analogically to what we do now, but...
isOffline
isBackendUnavailable
@godofoutcasts94
Sorry, I missed this. Please phrase solutions in the form of proposals in the future, as it makes the process easier.
Honestly, I'd like to touch
useNetwork
as little as possible. It's a hook with a lot of uses, and I presume that multiple instances of this hook are often mounted at the same time. I wouldn't like to do any periodic checks on the component level. Currently, I tend toward a solution put intoNetworkConnection.ts
. I think we should do everything analogically to what we do now, but...
fix the logical error of using the backend endpoint for determining
isOffline
add new logic (but analogical!) for determining a new boolean Onyx property like
isBackendUnavailable
Okay @cubuspl42 thanks for updating
Why not the other way around? As you mentioned, NetInfo has extra functionality (like supporting platform-native APIs for determining if we're online) that is better suited for the "offline check" task, I think.
@cubuspl42 Because if the offline check is successful (user is online), it does not mean the back-end is up. So we have to check the back-end every time the "network offline" check is successful. Since the "network offline" check will be more frequently successful than failed (assuming most of the time our user is connected to the internet), this will result in more network calls overall.
Meanwhile if the back-end check is successful, we know for sure that "offline check" is successful (user must be online in order to call our back-end successfully) and we don't have to do that check again. We only have to do the "offline check" if the back-end check fails in this case.
So you'd like to affect the boolean result of reachabilityTest based on two different endpoints?
@cubuspl42 not really, the reachabilityTest
will only depend on the backend reachability
. I'd phrase it more like "We additionally do the 'network offline' check only when the back-end check fails".
Back-end check
- Successful
-> We're good, we're both online, and the back-end is reachable
- Failed
-> Our back-end is not reachable
-> By the way, let's do the "network offline" check to see if user is online
- Successful: Yes, the user is online, let's show "Expensify is down" message
- Failed: No, the user is offline, let's show "You're offline" message
reachabilityUrl
The URL to call to test if the internet is reachable. Only used on platforms which do not supply internet reachability natively or if useNativeReachability is false.
Doesn't this invalidate your assumptions? We want a cross-platform solution.
@cubuspl42 I see what you mean. We have to do it the other way around then, do you have any other concern if we do it the the other way around?
Why not the other way around? As you mentioned, NetInfo has extra functionality (like supporting platform-native APIs for determining if we're online) that is better suited for the "offline check" task, I think.
@godofoutcasts94 When I say "propsal" I mean the by-the-process proposal, like the others posted.
@tienifr
I see what you mean. do you have any other concern if we do it the the other way around?
Likely not, I would encourage you to update your proposal so I can review its final shape.
Updated proposal @cubuspl42 .Hopefully now the proposal is fine for you.
Likely not, I would encourage you to update your proposal so I can review its final shape.
@cubuspl42 Sure! Proposal updated that accommodates the latest discussions.
@godofoutcasts94
In NetInfo.addEventListener i.e. here :
Do I understand correctly that you sketched a solution that checks for the backend availability only at the moment when the NetInfo
-reported Internet availability goes from offline to online or the other way around?
This doesn't sound like a good idea, the backend can go up and down independently.
I approve the proposal by @tienifr.
This is to make sure if the back-end is unavailable, then the user comes offline, when they come online, we should not show back-end is down
This is a good observation of a potential problem. We should ensure to cover this case, either using the exact suggested approach or something else that comes to us during the PR implementation.
C+ reviewed π π π
Current assignee @aldo-expensify is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.
β There was an error making the offer to @cubuspl42 for the Reviewer role. The BZ member will need to manually hire the contributor.
β There was an error making the offer to @tienifr for the Contributor role. The BZ member will need to manually hire the contributor.
If you havenβt already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Version Number: Reproducible in staging?: y Reproducible in production?: y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: @flodnv Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1709063517335079
Action Performed:
Expected Result:
When the site is down we should show:
We might have a problem. Check out status.expensify.com.
Actual Result:
You appear to be offline
appears confusing the user that they have a network problem when really it's the site.Workaround:
unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
View all open jobs on GitHub
Upwork Automation - Do Not Edit