fix(app): fix ODD IntersectionObserver reference cycle (#16743)

Works toward EXEC-807

We have this code in the ODD that runs every render cycle on a lot of the "idle" views that manages the scrollbar. We instantiate a new observer on every render, but the old observer is never cleaned up. This commit ensures that we only ever instantiate one observer, and we properly remove it on component derender.

app/src/local-resources/dom-utils/hooks/__tests__/useScrollPosition.test.ts

@@ -0,0 +1,76 @@
+import { renderHook, act } from '@testing-library/react'
+import { describe, it, expect, vi, beforeEach } from 'vitest'
+
+import { useScrollPosition } from '../useScrollPosition'
+
+describe('useScrollPosition', () => {
+  const mockObserve = vi.fn()
+  const mockDisconnect = vi.fn()
+  let intersectionCallback: (entries: IntersectionObserverEntry[]) => void
+
+  beforeEach(() => {
+    vi.stubGlobal(
+      'IntersectionObserver',
+      vi.fn(callback => {
+        intersectionCallback = callback
+        return {
+          observe: mockObserve,
+          disconnect: mockDisconnect,
+          unobserve: vi.fn(),
+        }
+      })
+    )
+  })
+
+  it('should return initial state and ref', () => {
+    const { result } = renderHook(() => useScrollPosition())
+
+    expect(result.current.isScrolled).toBe(false)
+    expect(result.current.scrollRef).toBeDefined()
+    expect(result.current.scrollRef.current).toBe(null)
+  })
+
+  it('should observe when ref is set', async () => {
+    const { result } = renderHook(() => useScrollPosition())
+
+    const div = document.createElement('div')
+
+    await act(async () => {
+      // @ts-expect-error we're forcibly setting readonly ref
+      result.current.scrollRef.current = div
+
+      const observer = new IntersectionObserver(intersectionCallback)
+      observer.observe(div)
+    })
+
+    expect(mockObserve).toHaveBeenCalledWith(div)
+  })
+
+  it('should update isScrolled when intersection changes for both scrolled and unscrolled cases', () => {
+    const { result } = renderHook(() => useScrollPosition())
+
+    act(() => {
+      intersectionCallback([
+        { isIntersecting: false } as IntersectionObserverEntry,
+      ])
+    })
+
+    expect(result.current.isScrolled).toBe(true)
+
+    act(() => {
+      intersectionCallback([
+        { isIntersecting: true } as IntersectionObserverEntry,
+      ])
+    })
+
+    expect(result.current.isScrolled).toBe(false)
+  })
+
+  it('should disconnect observer on unmount', () => {
+    const { unmount } = renderHook(() => useScrollPosition())
+
+    unmount()
+
+    expect(mockDisconnect).toHaveBeenCalled()
+  })
+})

app/src/local-resources/dom-utils/hooks/index.ts

@@ -0,0 +1 @@
+export * from './useScrollPosition'

app/src/local-resources/dom-utils/hooks/useScrollPosition.ts

@@ -0,0 +1,27 @@
+import { useRef, useState, useEffect } from 'react'
+
+import type { RefObject } from 'react'
+
+export function useScrollPosition(): {
+  scrollRef: RefObject<HTMLDivElement>
+  isScrolled: boolean
+} {
+  const scrollRef = useRef<HTMLDivElement>(null)
+  const [isScrolled, setIsScrolled] = useState<boolean>(false)
+
+  useEffect(() => {
+    const observer = new IntersectionObserver(([entry]) => {
+      setIsScrolled(!entry.isIntersecting)
+    })
+
+    if (scrollRef.current != null) {
+      observer.observe(scrollRef.current)
+    }
+
+    return () => {
+      observer.disconnect()
+    }
+  }, [])
+
+  return { scrollRef, isScrolled }
+}

app/src/local-resources/dom-utils/index.ts

@@ -0,0 +1 @@
+export * from './hooks'

app/src/organisms/ODD/Navigation/__tests__/Navigation.test.tsx

@@ -10,31 +10,15 @@ import { mockConnectedRobot } from '/app/redux/discovery/__fixtures__'
 import { useNetworkConnection } from '/app/resources/networking/hooks/useNetworkConnection'
 import { NavigationMenu } from '../NavigationMenu'
 import { Navigation } from '..'
+import { useScrollPosition } from '/app/local-resources/dom-utils'
 
 vi.mock('/app/resources/networking/hooks/useNetworkConnection')
 vi.mock('/app/redux/discovery')
 vi.mock('../NavigationMenu')
+vi.mock('/app/local-resources/dom-utils')
 
 mockConnectedRobot.name = '12345678901234567'
 
-class MockIntersectionObserver {
-  observe = vi.fn()
-  disconnect = vi.fn()
-  unobserve = vi.fn()
-}
-
-Object.defineProperty(window, 'IntersectionObserver', {
-  writable: true,
-  configurable: true,
-  value: MockIntersectionObserver,
-})
-
-Object.defineProperty(global, 'IntersectionObserver', {
-  writable: true,
-  configurable: true,
-  value: MockIntersectionObserver,
-})
-
 const render = (props: React.ComponentProps<typeof Navigation>) => {
   return renderWithProviders(
     <MemoryRouter>
@@ -56,6 +40,10 @@ describe('Navigation', () => {
       isUsbConnected: false,
       connectionStatus: 'Not connected',
     })
+    vi.mocked(useScrollPosition).mockReturnValue({
+      isScrolled: false,
+      scrollRef: {} as any,
+    })
   })
   it('should render text and they have attribute', () => {
     render(props)

app/src/organisms/ODD/Navigation/index.tsx

@@ -27,6 +27,7 @@ import {
   TYPOGRAPHY,
 } from '@opentrons/components'
 import { ODD_FOCUS_VISIBLE } from '/app/atoms/buttons/constants'
+import { useScrollPosition } from '/app/local-resources/dom-utils'
 
 import { useNetworkConnection } from '/app/resources/networking/hooks/useNetworkConnection'
 import { getLocalRobot } from '/app/redux/discovery'
@@ -92,15 +93,7 @@ export function Navigation(props: NavigationProps): JSX.Element {
     setShowNavMenu(openMenu)
   }
 
-  const scrollRef = React.useRef<HTMLDivElement>(null)
-  const [isScrolled, setIsScrolled] = React.useState<boolean>(false)
-
-  const observer = new IntersectionObserver(([entry]) => {
-    setIsScrolled(!entry.isIntersecting)
-  })
-  if (scrollRef.current != null) {
-    observer.observe(scrollRef.current)
-  }
+  const { scrollRef, isScrolled } = useScrollPosition()
 
   const navBarScrollRef = React.useRef<HTMLDivElement>(null)
   React.useEffect(() => {

app/src/pages/ODD/ProtocolDetails/__tests__/ProtocolDetails.test.tsx

@@ -24,21 +24,10 @@ import { Deck } from '../Deck'
 import { Hardware } from '../Hardware'
 import { Labware } from '../Labware'
 import { Parameters } from '../Parameters'
+import { useScrollPosition } from '/app/local-resources/dom-utils'
 
 import type { HostConfig } from '@opentrons/api-client'
 
-// Mock IntersectionObserver
-class IntersectionObserver {
-  observe = vi.fn()
-  disconnect = vi.fn()
-  unobserve = vi.fn()
-}
-
-Object.defineProperty(window, 'IntersectionObserver', {
-  writable: true,
-  configurable: true,
-  value: IntersectionObserver,
-})
 vi.mock(
   '/app/organisms/ODD/ProtocolSetup/ProtocolSetupParameters/ProtocolSetupParameters'
 )
@@ -55,6 +44,7 @@ vi.mock('../Hardware')
 vi.mock('../Labware')
 vi.mock('../Parameters')
 vi.mock('/app/redux/config')
+vi.mock('/app/local-resources/dom-utils')
 
 const MOCK_HOST_CONFIG = {} as HostConfig
 const mockCreateRun = vi.fn((id: string) => {})
@@ -119,6 +109,10 @@ describe('ODDProtocolDetails', () => {
     vi.mocked(getProtocol).mockResolvedValue({
       data: { links: { referencingRuns: [{ id: '1' }, { id: '2' }] } },
     } as any)
+    vi.mocked(useScrollPosition).mockReturnValue({
+      isScrolled: false,
+      scrollRef: {} as any,
+    })
   })
   afterEach(() => {
     vi.resetAllMocks()

app/src/pages/ODD/ProtocolDetails/index.tsx

@@ -1,4 +1,4 @@
-import { useState, useRef } from 'react'
+import { useState } from 'react'
 import last from 'lodash/last'
 import { useTranslation } from 'react-i18next'
 import { useQueryClient } from 'react-query'
@@ -56,6 +56,7 @@ import { Hardware } from './Hardware'
 import { Labware } from './Labware'
 import { Liquids } from './Liquids'
 import { formatTimeWithUtcLabel } from '/app/resources/runs'
+import { useScrollPosition } from '/app/local-resources/dom-utils'
 
 import type { Protocol } from '@opentrons/api-client'
 import type { OddModalHeaderBaseProps } from '/app/molecules/OddModal/types'
@@ -335,14 +336,7 @@ export function ProtocolDetails(): JSX.Element | null {
   })
 
   // Watch for scrolling to toggle dropshadow
-  const scrollRef = useRef<HTMLDivElement>(null)
-  const [isScrolled, setIsScrolled] = useState<boolean>(false)
-  const observer = new IntersectionObserver(([entry]) => {
-    setIsScrolled(!entry.isIntersecting)
-  })
-  if (scrollRef.current != null) {
-    observer.observe(scrollRef.current)
-  }
+  const { scrollRef, isScrolled } = useScrollPosition()
 
   let pinnedProtocolIds = useSelector(getPinnedProtocolIds) ?? []
   const pinned = pinnedProtocolIds.includes(protocolId)

app/src/pages/ODD/ProtocolSetup/__tests__/ProtocolSetup.test.tsx

@@ -64,22 +64,11 @@ import {
 } from '/app/resources/runs'
 import { mockConnectableRobot } from '/app/redux/discovery/__fixtures__'
 import { mockRunTimeParameterData } from '/app/organisms/ODD/ProtocolSetup/__fixtures__'
+import { useScrollPosition } from '/app/local-resources/dom-utils'
 
 import type { UseQueryResult } from 'react-query'
 import type * as SharedData from '@opentrons/shared-data'
 import type { NavigateFunction } from 'react-router-dom'
-// Mock IntersectionObserver
-class IntersectionObserver {
-  observe = vi.fn()
-  disconnect = vi.fn()
-  unobserve = vi.fn()
-}
-
-Object.defineProperty(window, 'IntersectionObserver', {
-  writable: true,
-  configurable: true,
-  value: IntersectionObserver,
-})
 
 let mockNavigate = vi.fn()
 
@@ -125,6 +114,7 @@ vi.mock('../ConfirmSetupStepsCompleteModal')
 vi.mock('/app/redux-resources/analytics')
 vi.mock('/app/redux-resources/robots')
 vi.mock('/app/resources/modules')
+vi.mock('/app/local-resources/dom-utils')
 
 const render = (path = '/') => {
   return renderWithProviders(
@@ -334,6 +324,10 @@ describe('ProtocolSetup', () => {
     when(vi.mocked(useTrackProtocolRunEvent))
       .calledWith(RUN_ID, ROBOT_NAME)
       .thenReturn({ trackProtocolRunEvent: mockTrackProtocolRunEvent })
+    vi.mocked(useScrollPosition).mockReturnValue({
+      isScrolled: false,
+      scrollRef: {} as any,
+    })
   })
 
   it('should render text, image, and buttons', () => {

app/src/pages/ODD/ProtocolSetup/index.tsx

@@ -81,6 +81,7 @@ import {
   useModuleCalibrationStatus,
   useProtocolAnalysisErrors,
 } from '/app/resources/runs'
+import { useScrollPosition } from '/app/local-resources/dom-utils'
 
 import type { Run } from '@opentrons/api-client'
 import type { CutoutFixtureId, CutoutId } from '@opentrons/shared-data'
@@ -129,14 +130,7 @@ function PrepareToRun({
   const { t, i18n } = useTranslation(['protocol_setup', 'shared'])
   const navigate = useNavigate()
   const { makeSnackbar } = useToaster()
-  const scrollRef = React.useRef<HTMLDivElement>(null)
-  const [isScrolled, setIsScrolled] = React.useState<boolean>(false)
-  const observer = new IntersectionObserver(([entry]) => {
-    setIsScrolled(!entry.isIntersecting)
-  })
-  if (scrollRef.current != null) {
-    observer.observe(scrollRef.current)
-  }
+  const { scrollRef, isScrolled } = useScrollPosition()
 
   const protocolId = runRecord?.data?.protocolId ?? null
   const { data: protocolRecord } = useProtocolQuery(protocolId, {

app/src/pages/ODD/QuickTransferDetails/__tests__/QuickTransferDetails.test.tsx

@@ -26,21 +26,10 @@ import { QuickTransferDetails } from '..'
 import { Deck } from '../Deck'
 import { Hardware } from '../Hardware'
 import { Labware } from '../Labware'
+import { useScrollPosition } from '/app/local-resources/dom-utils'
 
 import type { HostConfig } from '@opentrons/api-client'
 
-// Mock IntersectionObserver
-class IntersectionObserver {
-  observe = vi.fn()
-  disconnect = vi.fn()
-  unobserve = vi.fn()
-}
-
-Object.defineProperty(window, 'IntersectionObserver', {
-  writable: true,
-  configurable: true,
-  value: IntersectionObserver,
-})
 vi.mock('/app/organisms/ODD/ProtocolSetup/ProtocolSetupParameters')
 vi.mock('@opentrons/api-client')
 vi.mock('@opentrons/react-api-client')
@@ -55,6 +44,7 @@ vi.mock('../Deck')
 vi.mock('../Hardware')
 vi.mock('../Labware')
 vi.mock('/app/redux/config')
+vi.mock('/app/local-resources/dom-utils')
 
 const MOCK_HOST_CONFIG = {} as HostConfig
 const mockCreateRun = vi.fn((id: string) => {})
@@ -125,6 +115,10 @@ describe('ODDQuickTransferDetails', () => {
       },
     } as any)
     when(vi.mocked(useHost)).calledWith().thenReturn(MOCK_HOST_CONFIG)
+    vi.mocked(useScrollPosition).mockReturnValue({
+      isScrolled: false,
+      scrollRef: {} as any,
+    })
   })
   afterEach(() => {
     vi.resetAllMocks()

app/src/pages/ODD/QuickTransferDetails/index.tsx

@@ -1,4 +1,4 @@
-import { useState, useEffect, useRef } from 'react'
+import { useState, useEffect } from 'react'
 import last from 'lodash/last'
 import { useTranslation } from 'react-i18next'
 import { useQueryClient } from 'react-query'
@@ -57,6 +57,7 @@ import { Deck } from './Deck'
 import { Hardware } from './Hardware'
 import { Labware } from './Labware'
 import { formatTimeWithUtcLabel } from '/app/resources/runs'
+import { useScrollPosition } from '/app/local-resources/dom-utils'
 
 import type { Protocol } from '@opentrons/api-client'
 import type { Dispatch } from '/app/redux/types'
@@ -321,14 +322,7 @@ export function QuickTransferDetails(): JSX.Element | null {
   })
 
   // Watch for scrolling to toggle dropshadow
-  const scrollRef = useRef<HTMLDivElement>(null)
-  const [isScrolled, setIsScrolled] = useState<boolean>(false)
-  const observer = new IntersectionObserver(([entry]) => {
-    setIsScrolled(!entry.isIntersecting)
-  })
-  if (scrollRef.current != null) {
-    observer.observe(scrollRef.current)
-  }
+  const { scrollRef, isScrolled } = useScrollPosition()
 
   let pinnedTransferIds = useSelector(getPinnedQuickTransferIds) ?? []
   const pinned = pinnedTransferIds.includes(transferId)