refetch after paint and not in render

Author: janiejaffe

app/src/resources/camera/useNotifyCamera.ts

@@ -1,3 +1,5 @@
+import { useEffect } from 'react'
+
 import { useCamera } from '@opentrons/react-api-client'
 
 import { useNotifyDataReady } from '../useNotifyDataReady'
@@ -16,9 +18,14 @@ export function useNotifyCamera(
 
   const httpQueryResult = useCamera(queryOptionsNotify)
 
-  if (shouldRefetch) {
-    void httpQueryResult.refetch()
-  }
+  useEffect(() => {
+    if (shouldRefetch) {
+      void httpQueryResult.refetch()
+    }
+
+    // refetch is stable, the result object is not
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [shouldRefetch])
 
   return httpQueryResult
 }

app/src/resources/client_data/audit/useClientDataLogDeletion.ts

@@ -1,4 +1,4 @@
-import { useCallback } from 'react'
+import { useCallback, useEffect } from 'react'
 
 import { useClientData, useUpdateClientData } from '@opentrons/react-api-client'
 
@@ -27,9 +27,14 @@ export function useClientDataLogDeletion(
     queryOptionsNotify
   )
 
-  if (shouldRefetch) {
-    void httpQueryResult.refetch()
-  }
+  useEffect(() => {
+    if (shouldRefetch) {
+      void httpQueryResult.refetch()
+    }
+
+    // refetch is stable, the result object is not
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [shouldRefetch])
 
   const { data } = httpQueryResult
 

app/src/resources/client_data/encryptionKeys/useNotifyClientDataEncryptionKeys.ts

@@ -1,3 +1,5 @@
+import { useEffect } from 'react'
+
 import { useClientData } from '@opentrons/react-api-client'
 
 import { useNotifyDataReady } from '../../useNotifyDataReady'
@@ -24,9 +26,14 @@ export function useNotifyClientDataEncryptionKeys(
     queryOptionsNotify
   )
 
-  if (shouldRefetch) {
-    void httpQueryResult.refetch()
-  }
+  useEffect(() => {
+    if (shouldRefetch) {
+      void httpQueryResult.refetch()
+    }
+
+    // refetch is stable, the result object is not
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [shouldRefetch])
 
   return httpQueryResult
 }

app/src/resources/client_data/lpc/useNotifyClientDataLPC.ts

@@ -1,3 +1,5 @@
+import { useEffect } from 'react'
+
 import { useClientData } from '@opentrons/react-api-client'
 
 import { useNotifyDataReady } from '../../useNotifyDataReady'
@@ -25,9 +27,14 @@ export function useNotifyClientDataLPC(
     queryOptionsNotify
   )
 
-  if (shouldRefetch) {
-    void httpQueryResult.refetch()
-  }
+  useEffect(() => {
+    if (shouldRefetch) {
+      void httpQueryResult.refetch()
+    }
+
+    // refetch is stable, the result object is not
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [shouldRefetch])
 
   return httpQueryResult
 }

app/src/resources/client_data/recovery/useNotifyClientDataRecovery.ts

@@ -1,3 +1,5 @@
+import { useEffect } from 'react'
+
 import { useClientData } from '@opentrons/react-api-client'
 
 import { useNotifyDataReady } from '../../useNotifyDataReady'
@@ -24,9 +26,14 @@ export function useNotifyClientDataRecovery(
     queryOptionsNotify
   )
 
-  if (shouldRefetch) {
-    void httpQueryResult.refetch()
-  }
+  useEffect(() => {
+    if (shouldRefetch) {
+      void httpQueryResult.refetch()
+    }
+
+    // refetch is stable, the result object is not
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [shouldRefetch])
 
   return httpQueryResult
 }

app/src/resources/dataFiles/useNotifyImageFileQuery.ts

@@ -1,3 +1,5 @@
+import { useEffect } from 'react'
+
 import { useImageFileQuery } from '@opentrons/react-api-client'
 
 import { useNotifyDataReady } from '../useNotifyDataReady'
@@ -17,9 +19,14 @@ export function useNotifyImageFileQuery(
 
   const httpQueryResult = useImageFileQuery(runId, queryOptionsNotify)
 
-  if (shouldRefetch) {
-    void httpQueryResult.refetch()
-  }
+  useEffect(() => {
+    if (shouldRefetch) {
+      void httpQueryResult.refetch()
+    }
+
+    // refetch is stable, the result object is not
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [shouldRefetch])
 
   return httpQueryResult
 }

app/src/resources/deck_configuration/useNotifyDeckConfigurationQuery.ts

@@ -1,3 +1,5 @@
+import { useEffect } from 'react'
+
 import { useDeckConfigurationQuery } from '@opentrons/react-api-client'
 
 import { useNotifyDataReady } from '../useNotifyDataReady'
@@ -16,9 +18,14 @@ export function useNotifyDeckConfigurationQuery(
 
   const httpQueryResult = useDeckConfigurationQuery(queryOptionsNotify)
 
-  if (shouldRefetch) {
-    void httpQueryResult.refetch()
-  }
+  useEffect(() => {
+    if (shouldRefetch) {
+      void httpQueryResult.refetch()
+    }
+
+    // refetch is stable, the result object is not
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [shouldRefetch])
 
   return httpQueryResult
 }

app/src/resources/labware_offsets/useNotifySearchLabwareOffsets.ts

@@ -1,3 +1,5 @@
+import { useEffect } from 'react'
+
 import { useSearchLabwareOffsets } from '@opentrons/react-api-client'
 
 import { useNotifyDataReady } from '../useNotifyDataReady'
@@ -24,9 +26,14 @@ export function useNotifySearchLabwareOffsets(
 
   const httpQueryResult = useSearchLabwareOffsets(request, queryOptionsNotify)
 
-  if (shouldRefetch) {
-    void httpQueryResult.refetch()
-  }
+  useEffect(() => {
+    if (shouldRefetch) {
+      void httpQueryResult.refetch()
+    }
+
+    // refetch is stable, the result object is not
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [shouldRefetch])
 
   return httpQueryResult
 }

app/src/resources/maintenance_runs/notifications/useNotifyCurrentMaintenanceRun.ts

@@ -1,3 +1,5 @@
+import { useEffect } from 'react'
+
 import { useCurrentMaintenanceRun } from '@opentrons/react-api-client'
 
 import { useNotifyDataReady } from '../../useNotifyDataReady'
@@ -16,9 +18,14 @@ export function useNotifyCurrentMaintenanceRun(
 
   const httpQueryResult = useCurrentMaintenanceRun(queryOptionsNotify)
 
-  if (shouldRefetch) {
-    void httpQueryResult.refetch()
-  }
+  useEffect(() => {
+    if (shouldRefetch) {
+      void httpQueryResult.refetch()
+    }
+
+    // refetch is stable, the result object is not
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, [shouldRefetch])
 
   return httpQueryResult
 }

app/src/resources/runs/__tests__/useNotifyRunQuery.test.tsx

@@ -0,0 +1,108 @@
+import { renderHook } from '@testing-library/react'
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
+
+import { useRunQuery } from '@opentrons/react-api-client'
+
+import { useNotifyDataReady } from '/app/resources/useNotifyDataReady'
+
+import { useNotifyRunQuery } from '../useNotifyRunQuery'
+
+vi.mock('@opentrons/react-api-client')
+vi.mock('/app/resources/useNotifyDataReady')
+
+const MOCK_RUN_ID = 'run-123'
+const MOCK_OPTIONS = { staleTime: 5000 }
+
+describe('useNotifyRunQuery', () => {
+  const refetch = vi.fn()
+  const queryOptionsNotify = { ...MOCK_OPTIONS, refetchInterval: false }
+
+  beforeEach(() => {
+    vi.mocked(useNotifyDataReady).mockReturnValue({
+      shouldRefetch: false,
+      queryOptionsNotify,
+    } as any)
+    vi.mocked(useRunQuery).mockReturnValue({
+      refetch,
+    } as any)
+  })
+
+  afterEach(() => {
+    vi.clearAllMocks()
+  })
+
+  it('should pass notify options through to useRunQuery', () => {
+    renderHook(() => useNotifyRunQuery(MOCK_RUN_ID, MOCK_OPTIONS))
+
+    expect(useNotifyDataReady).toHaveBeenCalledWith({
+      topic: `robot-server/runs/${MOCK_RUN_ID}`,
+      options: MOCK_OPTIONS,
+      hostOverride: undefined,
+    })
+    expect(useRunQuery).toHaveBeenCalledWith(
+      MOCK_RUN_ID,
+      queryOptionsNotify,
+      undefined
+    )
+  })
+
+  it('should refetch in an effect when shouldRefetch is true, not during render', () => {
+    vi.mocked(useNotifyDataReady).mockReturnValue({
+      shouldRefetch: true,
+      queryOptionsNotify,
+    } as any)
+    vi.mocked(useRunQuery).mockImplementation(() => {
+      // useRunQuery runs during render; refetch must not have been called yet.
+      expect(refetch).not.toHaveBeenCalled()
+      return { refetch } as any
+    })
+
+    renderHook(() => useNotifyRunQuery(MOCK_RUN_ID, MOCK_OPTIONS))
+
+    expect(refetch).toHaveBeenCalledTimes(1)
+  })
+
+  it('should not refetch when shouldRefetch is false', () => {
+    renderHook(() => useNotifyRunQuery(MOCK_RUN_ID, MOCK_OPTIONS))
+
+    expect(refetch).not.toHaveBeenCalled()
+  })
+
+  it('should not refetch when runId is null', () => {
+    vi.mocked(useNotifyDataReady).mockReturnValue({
+      shouldRefetch: true,
+      queryOptionsNotify,
+    } as any)
+
+    renderHook(() => useNotifyRunQuery(null, MOCK_OPTIONS))
+
+    expect(refetch).not.toHaveBeenCalled()
+  })
+
+  it('should not refetch when runId is the string "null"', () => {
+    vi.mocked(useNotifyDataReady).mockReturnValue({
+      shouldRefetch: true,
+      queryOptionsNotify,
+    } as any)
+
+    renderHook(() => useNotifyRunQuery('null', MOCK_OPTIONS))
+
+    expect(refetch).not.toHaveBeenCalled()
+  })
+
+  it('should refetch when shouldRefetch becomes true after a rerender', () => {
+    const { rerender } = renderHook(() =>
+      useNotifyRunQuery(MOCK_RUN_ID, MOCK_OPTIONS)
+    )
+
+    expect(refetch).not.toHaveBeenCalled()
+
+    vi.mocked(useNotifyDataReady).mockReturnValue({
+      shouldRefetch: true,
+      queryOptionsNotify,
+    } as any)
+    rerender()
+
+    expect(refetch).toHaveBeenCalledTimes(1)
+  })
+})