Offline detection is now more deeply integrated #697

joshwingreene · 2023-01-25T01:15:05Z

Changes

Added setupListeners to store.ts in order to make fetched data be refetched automatically when the user's internet connection is restored
Added the same refetching code that is used for the other api files to notificationApi
Refactored out the internet connection code in NetworkDetector into its own hook
Refactored out the infinite loading functionality in useLiveNotifications into its own hook and made it generic. The original useInfiniteLoading hook has been replaced by this hook.

Purpose

Offline detection is now more deeply integrated

Notes

Multiple duplicate key warnings will be shown for the unread notifications view if the user reconnects and they received notifications while they were offline. However, the user won't experience any issues since the clear method inside of the useInfiniteLoading hook will eventually be called.
I found that the resetApiState() call was only necessary for the notification functionality. So, that's why the useInfiniteLoading hook has an optional resetApiStateFunction argument.

Testing Steps

Pull in the changes to your local copy of this branch and run it alongside the dj_starter_demo repo.
Use Developer Tools in order to make the app go offline
Create another admin and log in via a separate browser
Create a new farm
Reconnect the original app instance to the internet. Data should be refetched and you shouldn't experience any visual issues.

Demo

Old Demo

improve-offline-integration.mov

joshwingreene · 2023-01-25T17:18:25Z

Offline detection should be more deeply integrated into the system.

DropsOfSerenity

This is definitely on the right track! Nice work getting this going. There are a few clarifications needed to get this in.

DropsOfSerenity · 2023-02-22T16:14:02Z

src/app/redux/store.ts

@@ -30,6 +31,8 @@ export const createAppStore = (options?: ConfigureStoreOptions['preloadedState']

 export const store = createAppStore();

+setupListeners(store.dispatch);


Nice job finding this, looks like this enables rtk-query's focus and reconnect tracking.

DropsOfSerenity · 2023-02-22T16:16:33Z

src/common/hooks/useInfiniteLoading.ts

+export interface WithNumberIdentifier {
+  id?: number;
+}


This won't be typed correctly for string identifiers (which many people like to use, such as uuid's).

Also the naming here is a little confusing, WithIdentifier but then saying id is optional?

The reason why it is optional is that there are some models that don't have an id property. For example, the HistoricalRecord's id property is historyId. At the moment, the changelog functionality doesn't need the remove case in the useInfiniteLoading hook's reducer. However, I still needed to add the id property somehow since that reducer case makes use of an id property. Should we implement something that extends these type of models so that their custom id property is used to fill in the value for the standard id property? For example, when fetching an agent's history, I could use the useMemo hook in order to make all fetched history items be transformed into an object that contains an id property whose value comes from the historyId property. The newly created objects wouldn't include the historyId property.

DropsOfSerenity · 2023-02-22T16:21:48Z

src/common/hooks/useInfiniteLoading.ts

+  const itemProviderValue = useMemo(() => {
+    const result = {
+      items: [...items, ...oldItems] as T[],
+      count: items.length + count, // Justin: I'm not so sure this is right.


Hmm yeah, I think count should be tracked within the reducer itself so we don't have to do strange math here.

I'm not so sure this is right either.

Yeah. I'll go ahead and move it.

By the way, the "I'm not so sure this is right." comment was originally from you. That's why I added the "Justin: " to it. So far, I haven't encountered any issues with the count. So, we could go ahead and remove that comment.

Okay. I was able to refactor this. Check out how I refactored the cases in the reducer.

DropsOfSerenity · 2023-02-22T16:22:14Z

src/features/network-detector/components/NetworkDetector.tsx

-  }, [isDisconnected]);
-
-  return <>{children}</>;
+  const { isDisconnected } = useNetworkDetection();


nice simplification!

DropsOfSerenity · 2023-02-22T16:22:28Z

src/features/network-detector/hooks/useNetworkConnection.ts

+  return new Date().valueOf().toString();
+};
+
+export const useNetworkDetection = () => {


Love how you extracted this to it's own hook.

Rebase. This commit includes the following commits: feat(various): Created a hook out of the NetworkDetector component feat(various): Refactored the NetworkDetector component so that it shows a barrier if the user goes offline; Moved the use of the useNetworkDetection hook to the NetworkDetector component feat(notificationApi): Made the notificationApi file consistent with the other api files when it comes to refetching data feat(store): Data is now being refetched when the user's connection is restored feat(useReducerInfiniteLoading): In the middle of creating a version of useInfiniteLoading that uses the reducer from useLiveNotifications feat(various): Added the useNewLiveNotifications hook for testing purposes and resolves some issues feat(useReducerInfiniteLoading): The user's unread notifications are now cleared before refetching in the internet reconnection scenario. fix(useReducerInfiniteLoading): Resolved the issue that was causing the notifications to be cleared when more notifications are fetched fix(various): Removed the clearing that was happening when the notification dropdown was closed. This is already handled by the useReducerInfiniteLoading hook. refactor(various): Renamed notification to item in the useReducerInfiniteLoading hook refactor(useReducerInfiniteLoading): Implemented a better solution for the duplicate item issue when the user's internet connection is restored refactor(useReducerInfiniteLoading): Removed some unnecessary code and added some console logs in order to help identify why this hook keeps executing fix(useReducerInfiniteLoading): Resolves the infinite re-rendering of the notification listview fix(useReducerInfiniteLoading): Corrected the useReducerInfiniteLoading hook so that it returns the right type for the items and made it be compatible with the pages that use the useInfiniteLoading hook refactor(various): Now using the useReducerInfiniteLoading hook in all of the places where the useInfiniteLoading hook was being used fix(useReducerInfiniteLoading): Forget to add the error value to the useMemo dependency list fix(useReducerInfiniteLoading): The nextItemUrl wasn't being set correctly when the reducer state was reset. fix(various): I found that the resetApiState call would cause the infinite loading functionality to just refresh the whole page instead of working as you would aspect. Removing it fixed this issue. Based on my testing, I found it wasn't necessary for the notification functionality. fix(various): It was incorrect to remove the resetApiState function call for the notification functionality. refactor(various): Removed the old versions of useLiveNotifications and useInfiniteLoading and replaced them with the new ones refactored(useLiveNotifications): Removed some commented out code refactor(NetworkDetector): Removed the InteractionBarrier since the service worker PR will make this functionality unnecessary refactor(various): Renamed WithNumberIdentifier to WithIdentifier and made its id property accept string values as well refactor(various): In the middle of making the infinite loading functionality simpler refactor(various): Mostly everything is working. Just need to get the count on the NotificationListView to update when a notification is removed. feat(various): The count on the NotificationListView is now being updated correctly. refactor(various): Renamed the addOne and addMultiple cases refactor(useInfinteLoading): Need to implement a better solution for updating the count on NotificationListView refactor(various): The count is now being updated correctly again. refactor(various): Removed unnecessary properties and console logs; Resolved eslint issues

joshwingreene requested a review from a team January 25, 2023 01:15

joshwingreene requested review from DropsOfSerenity and fluturecode as code owners January 25, 2023 01:15

joshwingreene requested review from Fleury14, goseinthemachine and aleon510 and removed request for a team January 25, 2023 01:15

DropsOfSerenity suggested changes Feb 22, 2023

View reviewed changes

joshwingreene requested review from DropsOfSerenity and removed request for aleon510 March 6, 2023 22:55

coreyshuman removed the request for review from DropsOfSerenity April 3, 2023 23:37

coreyshuman marked this pull request as draft April 4, 2023 18:30

goseinthemachine removed their request for review May 2, 2023 23:12

joshwingreene force-pushed the jgg-improve-offline-detection-integration branch from 95ce07f to 49c1d9e Compare May 17, 2023 00:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Offline detection is now more deeply integrated #697

Offline detection is now more deeply integrated #697

joshwingreene commented Jan 25, 2023 •

edited

Loading

joshwingreene commented Jan 25, 2023

DropsOfSerenity left a comment

DropsOfSerenity Feb 22, 2023

DropsOfSerenity Feb 22, 2023

joshwingreene Mar 2, 2023

DropsOfSerenity Feb 22, 2023

joshwingreene Feb 22, 2023 •

edited

Loading

joshwingreene Mar 6, 2023

DropsOfSerenity Feb 22, 2023

DropsOfSerenity Feb 22, 2023

		@@ -30,6 +31,8 @@ export const createAppStore = (options?: ConfigureStoreOptions['preloadedState']

		export const store = createAppStore();

		setupListeners(store.dispatch);

Offline detection is now more deeply integrated #697

Are you sure you want to change the base?

Offline detection is now more deeply integrated #697

Conversation

joshwingreene commented Jan 25, 2023 • edited Loading

Changes

Purpose

Notes

Testing Steps

Demo

Old Demo

joshwingreene commented Jan 25, 2023

DropsOfSerenity left a comment

Choose a reason for hiding this comment

DropsOfSerenity Feb 22, 2023

Choose a reason for hiding this comment

DropsOfSerenity Feb 22, 2023

Choose a reason for hiding this comment

joshwingreene Mar 2, 2023

Choose a reason for hiding this comment

DropsOfSerenity Feb 22, 2023

Choose a reason for hiding this comment

joshwingreene Feb 22, 2023 • edited Loading

Choose a reason for hiding this comment

joshwingreene Mar 6, 2023

Choose a reason for hiding this comment

DropsOfSerenity Feb 22, 2023

Choose a reason for hiding this comment

DropsOfSerenity Feb 22, 2023

Choose a reason for hiding this comment

joshwingreene commented Jan 25, 2023 •

edited

Loading

joshwingreene Feb 22, 2023 •

edited

Loading