refactor(app): Clean up notify hooks (#14562)

Partially closes RAUT-990
Opentrons · Feb 29, 2024 · ae38ff1 · ae38ff1
1 parent 6c5a6dc
commit ae38ff1
Show file tree

Hide file tree

Showing 8 changed files with 136 additions and 176 deletions.
diff --git a/app-shell-odd/src/notify.ts b/app-shell-odd/src/notify.ts
@@ -276,16 +276,19 @@ function handleDecrementSubscriptionCount(
   hostname: string,
   topic: NotifyTopic
 ): void {
-  const { client, subscriptions } = connectionStore[hostname]
-  if (topic in subscriptions) {
-    subscriptions[topic] -= 1
-    if (subscriptions[topic] <= 0) {
-      delete subscriptions[topic]
+  const host = connectionStore[hostname]
+  if (host) {
+    const { client, subscriptions } = host
+    if (topic in subscriptions) {
+      subscriptions[topic] -= 1
+      if (subscriptions[topic] <= 0) {
+        delete subscriptions[topic]
+      }
     }
-  }
 
-  if (Object.keys(subscriptions).length <= 0) {
-    client?.end()
+    if (Object.keys(subscriptions).length <= 0) {
+      client?.end()
+    }
   }
 }
 

diff --git a/app-shell/src/notify.ts b/app-shell/src/notify.ts
@@ -272,16 +272,19 @@ function handleDecrementSubscriptionCount(
   hostname: string,
   topic: NotifyTopic
 ): void {
-  const { client, subscriptions } = connectionStore[hostname]
-  if (topic in subscriptions) {
-    subscriptions[topic] -= 1
-    if (subscriptions[topic] <= 0) {
-      delete subscriptions[topic]
+  const host = connectionStore[hostname]
+  if (host) {
+    const { client, subscriptions } = host
+    if (topic in subscriptions) {
+      subscriptions[topic] -= 1
+      if (subscriptions[topic] <= 0) {
+        delete subscriptions[topic]
+      }
     }
-  }
 
-  if (Object.keys(subscriptions).length <= 0) {
-    client?.end()
+    if (Object.keys(subscriptions).length <= 0) {
+      client?.end()
+    }
   }
 }
 

diff --git a/app/src/resources/__tests__/useNotifyService.test.ts b/app/src/resources/__tests__/useNotifyService.test.ts
@@ -59,15 +59,15 @@ describe('useNotifyService', () => {
     jest.clearAllMocks()
   })
 
-  it('should trigger an HTTP refetch and subscribe action on initial mount', () => {
+  it('should trigger an HTTP refetch and subscribe action on a successful initial mount', () => {
     renderHook(() =>
       useNotifyService({
         topic: MOCK_TOPIC,
-        refetchUsingHTTP: mockHTTPRefetch,
+        setRefetchUsingHTTP: mockHTTPRefetch,
         options: MOCK_OPTIONS,
       } as any)
     )
-    expect(mockHTTPRefetch).toHaveBeenCalled()
+    expect(mockHTTPRefetch).toHaveBeenCalledWith('once')
     expect(mockDispatch).toHaveBeenCalledWith(
       notifySubscribeAction(MOCK_HOST_CONFIG.hostname, MOCK_TOPIC)
     )
@@ -81,7 +81,7 @@ describe('useNotifyService', () => {
     const { unmount } = renderHook(() =>
       useNotifyService({
         topic: MOCK_TOPIC,
-        refetchUsingHTTP: mockHTTPRefetch,
+        setRefetchUsingHTTP: mockHTTPRefetch,
         options: MOCK_OPTIONS,
       } as any)
     )
@@ -91,22 +91,11 @@ describe('useNotifyService', () => {
     )
   })
 
-  it('should return no notify error if there was a successful topic subscription', () => {
-    const { result } = renderHook(() =>
-      useNotifyService({
-        topic: MOCK_TOPIC,
-        refetchUsingHTTP: mockHTTPRefetch,
-        options: MOCK_OPTIONS,
-      } as any)
-    )
-    expect(result.current.isNotifyError).toBe(false)
-  })
-
   it('should not subscribe to notifications if forceHttpPolling is true', () => {
     renderHook(() =>
       useNotifyService({
         topic: MOCK_TOPIC,
-        refetchUsingHTTP: mockHTTPRefetch,
+        setRefetchUsingHTTP: mockHTTPRefetch,
         options: { ...MOCK_OPTIONS, forceHttpPolling: true },
       } as any)
     )
@@ -119,7 +108,7 @@ describe('useNotifyService', () => {
     renderHook(() =>
       useNotifyService({
         topic: MOCK_TOPIC,
-        refetchUsingHTTP: mockHTTPRefetch,
+        setRefetchUsingHTTP: mockHTTPRefetch,
         options: { ...MOCK_OPTIONS, enabled: false },
       } as any)
     )
@@ -132,7 +121,7 @@ describe('useNotifyService', () => {
     renderHook(() =>
       useNotifyService({
         topic: MOCK_TOPIC,
-        refetchUsingHTTP: mockHTTPRefetch,
+        setRefetchUsingHTTP: mockHTTPRefetch,
         options: { ...MOCK_OPTIONS, staleTime: Infinity },
       } as any)
     )
@@ -141,68 +130,47 @@ describe('useNotifyService', () => {
     expect(mockDispatch).not.toHaveBeenCalled()
   })
 
-  it('should log an error if hostname is null', () => {
+  it('should set HTTP refetch to always if there is an error', () => {
     mockUseHost.mockReturnValue({ hostname: null } as any)
-    const errorSpy = jest.spyOn(console, 'error')
-    errorSpy.mockImplementation(() => {})
 
     renderHook(() =>
       useNotifyService({
         topic: MOCK_TOPIC,
-        refetchUsingHTTP: mockHTTPRefetch,
+        setRefetchUsingHTTP: mockHTTPRefetch,
         options: MOCK_OPTIONS,
       } as any)
     )
-    expect(errorSpy).toHaveBeenCalledWith(
-      'NotifyService expected hostname, received null for topic:',
-      MOCK_TOPIC
-    )
-    errorSpy.mockRestore()
+    expect(mockHTTPRefetch).toHaveBeenCalledWith('always')
   })
 
-  it('should return a notify error and fire an analytics reporting event if the connection was refused', () => {
+  it('should return set HTTP refetch to always and fire an analytics reporting event if the connection was refused', () => {
     mockAppShellListener.mockImplementation((_: any, __: any, mockCb: any) => {
       mockCb('ECONNREFUSED')
     })
-    const { result, rerender } = renderHook(() =>
+    const { rerender } = renderHook(() =>
       useNotifyService({
         topic: MOCK_TOPIC,
-        refetchUsingHTTP: mockHTTPRefetch,
+        setRefetchUsingHTTP: mockHTTPRefetch,
         options: MOCK_OPTIONS,
       } as any)
     )
     expect(mockTrackEvent).toHaveBeenCalled()
     rerender()
-    expect(result.current.isNotifyError).toBe(true)
+    expect(mockHTTPRefetch).toHaveBeenCalledWith('always')
   })
 
-  it('should return a notify error if the connection failed', () => {
-    mockAppShellListener.mockImplementation((_: any, __: any, mockCb: any) => {
-      mockCb('ECONNFAILED')
-    })
-    const { result, rerender } = renderHook(() =>
-      useNotifyService({
-        topic: MOCK_TOPIC,
-        refetchUsingHTTP: mockHTTPRefetch,
-        options: MOCK_OPTIONS,
-      } as any)
-    )
-    rerender()
-    expect(result.current.isNotifyError).toBe(true)
-  })
-
-  it('should trigger an HTTP refetch if the refetch flag was returned', () => {
+  it('should trigger a single HTTP refetch if the refetch flag was returned', () => {
     mockAppShellListener.mockImplementation((_: any, __: any, mockCb: any) => {
       mockCb({ refetchUsingHTTP: true })
     })
     const { rerender } = renderHook(() =>
       useNotifyService({
         topic: MOCK_TOPIC,
-        refetchUsingHTTP: mockHTTPRefetch,
+        setRefetchUsingHTTP: mockHTTPRefetch,
         options: MOCK_OPTIONS,
       } as any)
     )
     rerender()
-    expect(mockHTTPRefetch).toHaveBeenCalled()
+    expect(mockHTTPRefetch).toHaveBeenCalledWith('once')
   })
 })
diff --git a/app/src/resources/maintenance_runs/useNotifyCurrentMaintenanceRun.ts b/app/src/resources/maintenance_runs/useNotifyCurrentMaintenanceRun.ts
@@ -1,41 +1,37 @@
 import * as React from 'react'
 
-import { useHost, useCurrentMaintenanceRun } from '@opentrons/react-api-client'
+import { useCurrentMaintenanceRun } from '@opentrons/react-api-client'
 
 import { useNotifyService } from '../useNotifyService'
 
 import type { UseQueryResult } from 'react-query'
 import type { MaintenanceRun } from '@opentrons/api-client'
-import type { QueryOptionsWithPolling } from '../useNotifyService'
+import type {
+  QueryOptionsWithPolling,
+  HTTPRefetchFrequency,
+} from '../useNotifyService'
 
 export function useNotifyCurrentMaintenanceRun(
-  options?: QueryOptionsWithPolling<MaintenanceRun, Error>
+  options: QueryOptionsWithPolling<MaintenanceRun, Error> = {}
 ): UseQueryResult<MaintenanceRun> | UseQueryResult<MaintenanceRun, Error> {
-  const host = useHost()
-  const [refetchUsingHTTP, setRefetchUsingHTTP] = React.useState(false)
+  const [
+    refetchUsingHTTP,
+    setRefetchUsingHTTP,
+  ] = React.useState<HTTPRefetchFrequency>(null)
 
-  const { isNotifyError } = useNotifyService<MaintenanceRun, Error>({
+  useNotifyService<MaintenanceRun, Error>({
     topic: 'robot-server/maintenance_runs/current_run',
-    refetchUsingHTTP: () => setRefetchUsingHTTP(true),
-    options: {
-      ...options,
-      enabled: host !== null && options?.enabled !== false,
-    },
+    setRefetchUsingHTTP,
+    options,
   })
 
-  const isNotifyEnabled =
-    !isNotifyError &&
-    !options?.forceHttpPolling &&
-    options?.staleTime !== Infinity
-
-  if (!isNotifyEnabled && !refetchUsingHTTP) setRefetchUsingHTTP(true)
-  const isHTTPEnabled =
-    host !== null && options?.enabled !== false && refetchUsingHTTP
-
   const httpQueryResult = useCurrentMaintenanceRun({
     ...options,
-    enabled: isHTTPEnabled,
-    onSettled: isNotifyEnabled ? () => setRefetchUsingHTTP(false) : undefined,
+    enabled: options?.enabled !== false && refetchUsingHTTP != null,
+    onSettled:
+      refetchUsingHTTP === 'once'
+        ? () => setRefetchUsingHTTP(null)
+        : () => null,
   })
 
   return httpQueryResult

diff --git a/app/src/resources/runs/useNotifyAllRunsQuery.ts b/app/src/resources/runs/useNotifyAllRunsQuery.ts
@@ -8,39 +8,37 @@ import type { UseQueryResult } from 'react-query'
 import type { AxiosError } from 'axios'
 import type { HostConfig, GetRunsParams, Runs } from '@opentrons/api-client'
 import type { UseAllRunsQueryOptions } from '@opentrons/react-api-client/src/runs/useAllRunsQuery'
-import type { QueryOptionsWithPolling } from '../useNotifyService'
+import type {
+  QueryOptionsWithPolling,
+  HTTPRefetchFrequency,
+} from '../useNotifyService'
 
 export function useNotifyAllRunsQuery(
   params: GetRunsParams = {},
   options: QueryOptionsWithPolling<UseAllRunsQueryOptions, AxiosError> = {},
   hostOverride?: HostConfig | null
 ): UseQueryResult<Runs, AxiosError> {
-  const [refetchUsingHTTP, setRefetchUsingHTTP] = React.useState(false)
+  const [
+    refetchUsingHTTP,
+    setRefetchUsingHTTP,
+  ] = React.useState<HTTPRefetchFrequency>(null)
 
-  const { isNotifyError } = useNotifyService<
-    UseAllRunsQueryOptions,
-    AxiosError
-  >({
+  useNotifyService<UseAllRunsQueryOptions, AxiosError>({
     topic: 'robot-server/runs',
-    refetchUsingHTTP: () => setRefetchUsingHTTP(true),
+    setRefetchUsingHTTP,
     options,
     hostOverride,
   })
 
-  const isNotifyEnabled =
-    !isNotifyError &&
-    !options?.forceHttpPolling &&
-    options?.staleTime !== Infinity
-
-  if (!isNotifyEnabled && !refetchUsingHTTP) setRefetchUsingHTTP(true)
-  const isHTTPEnabled = options?.enabled !== false && refetchUsingHTTP
-
   const httpResponse = useAllRunsQuery(
     params,
     {
       ...(options as UseAllRunsQueryOptions),
-      enabled: isHTTPEnabled,
-      onSettled: isNotifyEnabled ? () => setRefetchUsingHTTP(false) : undefined,
+      enabled: options?.enabled !== false && refetchUsingHTTP != null,
+      onSettled:
+        refetchUsingHTTP === 'once'
+          ? () => setRefetchUsingHTTP(null)
+          : () => null,
     },
     hostOverride
   )

diff --git a/app/src/resources/runs/useNotifyLastRunCommandKey.ts b/app/src/resources/runs/useNotifyLastRunCommandKey.ts
@@ -4,28 +4,33 @@ import { useNotifyService } from '../useNotifyService'
 import { useLastRunCommandKey } from '../../organisms/Devices/hooks/useLastRunCommandKey'
 
 import type { CommandsData } from '@opentrons/api-client'
-import type { QueryOptionsWithPolling } from '../useNotifyService'
+import type {
+  QueryOptionsWithPolling,
+  HTTPRefetchFrequency,
+} from '../useNotifyService'
 
 export function useNotifyLastRunCommandKey(
   runId: string,
-  options?: QueryOptionsWithPolling<CommandsData, Error>
+  options: QueryOptionsWithPolling<CommandsData, Error> = {}
 ): string | null {
-  const [refetchUsingHTTP, setRefetchUsingHTTP] = React.useState(false)
+  const [
+    refetchUsingHTTP,
+    setRefetchUsingHTTP,
+  ] = React.useState<HTTPRefetchFrequency>(null)
 
-  const { isNotifyError } = useNotifyService({
+  useNotifyService({
     topic: 'robot-server/runs/current_command',
-    refetchUsingHTTP: () => setRefetchUsingHTTP(true),
-    options: options != null ? options : {},
+    setRefetchUsingHTTP,
+    options,
   })
 
-  const isNotifyEnabled = !isNotifyError && !options?.forceHttpPolling
-  if (!isNotifyEnabled && !refetchUsingHTTP) setRefetchUsingHTTP(true)
-  const isHTTPEnabled = options?.enabled !== false && refetchUsingHTTP
-
   const httpResponse = useLastRunCommandKey(runId, {
     ...options,
-    enabled: isHTTPEnabled,
-    onSettled: isNotifyEnabled ? () => setRefetchUsingHTTP(false) : undefined,
+    enabled: options?.enabled !== false && refetchUsingHTTP != null,
+    onSettled:
+      refetchUsingHTTP === 'once'
+        ? () => setRefetchUsingHTTP(null)
+        : () => null,
   })
 
   return httpResponse