detect offline client #129

Blazing-Mike · 2024-02-26T12:43:30Z

Context (Problem, Motivation, Solution)

Link related issues!
i ran into git conflict so i had to pull. open another PR
#126 #35

Describe Your Changes

Checklist

I have performed a self-review of my code
I ran make check and fixed resulting issues
I ran the relevant tests and they all pass
I wrote tests for my new features, or added regression tests for the bug I fixed

Testing

I implemented a modal to display when our app is offline. so how did i test this, i tested this by throttling the network in my devtools and also disconneCTING my device from wifi and this modal appears on the index.tsx and play.tsx page. when i toggle back or reconnect, the modal disaappears

CC: @norswap

norswap · 2024-02-26T23:25:08Z

packages/webapp/src/pages/index.tsx

@@ -37,6 +38,8 @@ const Home: FablePage = ({ isHydrated }) => {
  const isRightNetwork = !notConnected && chainSupported
  const isWrongNetwork = !notConnected && !chainSupported

+    useOfflineCheck();
+


Sorry I forgot this one is the last review, could you add this to _app.tsx instead, so it applies to all pages?

norswap · 2024-02-26T23:28:11Z

packages/webapp/src/hooks/useOfflineCheck.ts

+    });
+    return response.ok;
+  } catch (error) {
+    throw error; // Rethrow for retries


This is redundant, you could remove the whole try-catch statement and it would work the same.

norswap · 2024-02-26T23:32:54Z

packages/webapp/src/hooks/useOfflineCheck.ts

+        }, retryDelay);
+      } else {
+        setIsOffline(true);
+        toast.error("App is offline. Please check your internet connection.");


Add an option object here with { dismissible: false, duration: Infinity } which prevents the toast from closing.

I'm wondering if we need this at all — can't we just handle the "online" and "offline" events? On "online" do what you do in handleOnline, on "offline" do setIsOffline and the toast. What's the reasoning here? Is there some documentation around the behaviour and reliability of these events?

packages/webapp/src/hooks/useOfflineCheck.ts

norswap · 2024-02-26T23:48:15Z

packages/webapp/src/hooks/useOfflineCheck.ts

+      .catch(() => {
+        setIsOffline(true);
+        toast.error("App is offline. Please check your internet connection.");
+      });


The toast is triggered many times, which we should fix.

I'm wondering if this isn't the problem, as this is rerun every time useOfflineCheck is rerun, which is every time that the page is rendered.

Another possible source of the issue is that the "offline" event is fired multiple times? Is that the case?

i think i figured why the toast is rendering multiple times, it is being called both in the performNetworkCheck() function and the handleOffline.

Did you try the codebase and shut down your wifi to see if this works correctly?

norswap · 2024-02-26T23:49:25Z

packages/webapp/src/hooks/useOfflineCheck.ts

+function useOfflineCheck(options: any = {}) {
+  const {
+    // Optional configuration properties:
+    threshold = 1000, // Minimum interval between offline checks (ms)


This is never used.

norswap · 2024-02-26T23:50:03Z

packages/webapp/src/hooks/useOfflineCheck.ts

+          performNetworkCheck()
+            .then(() => handleOnline()) // online
+            .catch(() => {
+              setRetries(retries + 1); // Retry if error occurs


I'm quite dubious on the retry logic here, as we don't actually enqueue any retry! The only way retries can be attempted is if the "offline" event fire multiple times. Is that the intention here? But if so, the retryDelay variable is a little bit misleading, as it simply adds to the delay between multiple firings of "offline".

for the retry logic, we only want to show our app is offline if `retries === maxRetries. so the reason for that.

Blazing-Mike · 2024-03-02T23:11:04Z

@norswap please i need a review on this PR.

norswap · 2024-03-05T11:19:45Z

Hey, time is a bit tight right now, but I'll get to it soon. Don't worry, this'll be considered for ODHack.

Blazing-Mike · 2024-03-05T18:49:00Z

I understand. Thank you.

norswap · 2024-03-05T23:03:48Z

packages/webapp/src/hooks/useOfflineCheck.ts

+      } 
+    };
+
+    window.addEventListener("online", handleOffline);


It seems unecessary to recheck the network state when receiving "online".

this is checked to dismiss the toast.

norswap · 2024-03-05T23:06:21Z

packages/webapp/src/hooks/useOfflineCheck.ts

+            });
+        }, retryDelay);
+      } 
+    };


The retry logic still doesn't make sense.

handleOffline is called

performNetworkCheck is called

in case of failure (catch) the retry count is incremented by 1

??? — nothing in the code makes performNetworkCheck or handleOffline be called again

Do we really need retry logic in the first place? We could just assume offline after a single failure.

The risk of assuming after single failure is that it's a fluke and the offline event and subsequent online events are never sent, in which case we stay in offline mode forever.

Do we even need performNetworkCheck at all? Can't we only rely on online and offline?

It's possible that these are not always reliable and it's recommend to perform explicit checks, but I'd like to hear your reasoning about this + some documentation showing that indeed online / offline is not to be relied on.

Hmmmm, it might be complicating things. The first option I opted for was using the online and offline , but seeing some documentation that suggests online/offline might not be relied on that's why I opted for doing a network check and retrying the request if it fails the first time.

In my opinion, the unreliability of theonline / offline event might be rare and we can tackle it with listening for both online and offline. I implemented a version that uses the online / offline and they both work. I will push the changes for review.

Cc: @norswap

norswap · 2024-03-05T23:09:26Z

packages/webapp/src/hooks/useOfflineCheck.ts

+      .catch(() => {
+        setIsOffline(true);
+        toast.error("App is offline. Please check your internet connection.");
+      });


Did you try the codebase and shut down your wifi to see if this works correctly?

norswap · 2024-03-12T18:03:03Z

Hey, any updates on this?

Blazing-Mike · 2024-03-12T18:05:06Z

Yess. I have fixed the implementation to just rely on offline and online events. And it works when I switched off WiFi and turned it on.

Blazing-Mike · 2024-03-13T20:47:43Z

cc @norswap

norswap · 2024-03-15T05:16:54Z

Sorry, I missed that extra commit. Looks good, could you rebase on top of master and make sure make check passes? (You'll want to run make format first.)

Blazing-Mike · 2024-03-15T09:24:51Z

Sorry, I missed that extra commit. Looks good, could you rebase on top of master and make sure make check passes? (You'll want to run make format first.)

Done ✅. make check passes.

Cc: @norswap

Blazing-Mike · 2024-03-18T15:41:23Z

Hi norswap.Just reminding you of this PR, make check have passed.

norswap · 2024-03-20T03:50:47Z

Hey, I see you merged instead of rebasing, do you think you could get that fixed? If not, it's okay, just let me know and I'll perform the rebase myself :)

Blazing-Mike · 2024-03-20T04:04:20Z

I am running into some issues with it. could you help please? thank you.

Blazing-Mike · 2024-03-24T19:18:30Z

it's me again 🙂...just wanna check if the or is merged. Have a productive week ahead.

Cc: @norswap

norswap · 2024-03-26T07:56:48Z

Hey, I'm unfortunately super buy and hadn't time to deal with this, but I'll get it sorted!

norswap

Let's go! 🚀

Blazing-Mike · 2024-04-02T06:27:26Z

finally 😃!!I feel so elated looking forward to contributing more to this project.

norswap requested changes Feb 26, 2024

View reviewed changes

Blazing-Mike requested a review from norswap February 27, 2024 08:36

norswap requested changes Mar 5, 2024

View reviewed changes

Blazing-Mike requested a review from norswap March 6, 2024 12:50

offline client detection

4680398

norswap force-pushed the detect-offline-client branch from 8fc4b62 to 4680398 Compare April 2, 2024 05:31

norswap approved these changes Apr 2, 2024

View reviewed changes

norswap merged commit a037159 into 0xFableOrg:master Apr 2, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

detect offline client #129

detect offline client #129

Blazing-Mike commented Feb 26, 2024

norswap Feb 26, 2024

Blazing-Mike Feb 27, 2024

norswap Feb 26, 2024

norswap Feb 26, 2024

norswap Feb 26, 2024

norswap Feb 26, 2024

Blazing-Mike Feb 27, 2024

norswap Mar 5, 2024

norswap Feb 26, 2024

norswap Feb 26, 2024

Blazing-Mike Feb 27, 2024

Blazing-Mike commented Mar 2, 2024

norswap commented Mar 5, 2024

Blazing-Mike commented Mar 5, 2024

norswap Mar 5, 2024

Blazing-Mike Mar 6, 2024

norswap Mar 5, 2024

norswap Mar 5, 2024

Blazing-Mike Mar 6, 2024 •

edited

Loading

norswap Mar 5, 2024

norswap commented Mar 12, 2024

Blazing-Mike commented Mar 12, 2024

Blazing-Mike commented Mar 13, 2024

norswap commented Mar 15, 2024

Blazing-Mike commented Mar 15, 2024

Blazing-Mike commented Mar 18, 2024

norswap commented Mar 20, 2024

Blazing-Mike commented Mar 20, 2024 •

edited

Loading

Blazing-Mike commented Mar 24, 2024

norswap commented Mar 26, 2024

norswap left a comment

Blazing-Mike commented Apr 2, 2024

detect offline client #129

detect offline client #129

Conversation

Blazing-Mike commented Feb 26, 2024

Context (Problem, Motivation, Solution)

Describe Your Changes

Checklist

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Blazing-Mike commented Mar 2, 2024

norswap commented Mar 5, 2024

Blazing-Mike commented Mar 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Blazing-Mike Mar 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

norswap commented Mar 12, 2024

Blazing-Mike commented Mar 12, 2024

Blazing-Mike commented Mar 13, 2024

norswap commented Mar 15, 2024

Blazing-Mike commented Mar 15, 2024

Blazing-Mike commented Mar 18, 2024

norswap commented Mar 20, 2024

Blazing-Mike commented Mar 20, 2024 • edited Loading

Blazing-Mike commented Mar 24, 2024

norswap commented Mar 26, 2024

norswap left a comment

Choose a reason for hiding this comment

Blazing-Mike commented Apr 2, 2024

Blazing-Mike Mar 6, 2024 •

edited

Loading

Blazing-Mike commented Mar 20, 2024 •

edited

Loading