fix(app): fix infinitely re-rendering/never rendering firmware success toasts (#14981)

Closes RQA-2588

There are two issues with rendering firmware update toasts.

First, the toasts depend on a request id stored as state within the ModuleCard, but ModuleCards most often unrender during the firmware update process, so this state is lost. This causes the toast never to render.

The second issue is that sometimes, given the timing of the polling for attached modules, the module is always attached during the firmware update, thereby causing the module card not to unrender. When this happens, the useEffect hook responsible for making the success toast has conditional logic that is always true after an update, causing the toast to render infinitely.

The solution to is to lift the request id state out of the module card itself and then abstract away the storage/retrieval via a utility hook, which is utilized by all parent components of ModuleCard. Also, the shouldRenderToast logic should be calculated only on the initial render.

Author: mjhuff

app/src/assets/localization/en/device_details.json

@@ -58,7 +58,7 @@
   "firmware_update_needed": "Instrument firmware update needed. Start the update on the robot's touchscreen.",
   "firmware_update_available": "Firmware update available.",
   "firmware_update_failed": "Failed to update module firmware",
-  "firmware_update_installation_successful": "Installation successful",
+  "firmware_updated_successfully": "Firmware updated successfully",
   "firmware_update_occurring": "Firmware update in progress...",
   "fixture": "Fixture",
   "have_not_run_description": "After you run some protocols, they will appear here.",

app/src/organisms/Devices/InstrumentsAndModules.tsx

@@ -33,6 +33,7 @@ import { PipetteCard } from './PipetteCard'
 import { FlexPipetteCard } from './PipetteCard/FlexPipetteCard'
 import { GripperCard } from '../GripperCard'
 import { useIsEstopNotDisengaged } from '../../resources/devices/hooks/useIsEstopNotDisengaged'
+import { useModuleApiRequests } from '../ModuleCard/utils'
 
 import type {
   BadGripper,
@@ -62,6 +63,7 @@ export function InstrumentsAndModules({
   const currentRunId = useCurrentRunId()
   const { isRunTerminal, isRunRunning } = useRunStatuses()
   const isEstopNotDisengaged = useIsEstopNotDisengaged(robotName)
+  const [getLatestRequestId, handleModuleApiRequests] = useModuleApiRequests()
 
   const { data: attachedInstruments } = useInstrumentsQuery({
     refetchInterval: EQUIPMENT_POLL_MS,
@@ -218,6 +220,8 @@ export function InstrumentsAndModules({
                   attachPipetteRequired={attachPipetteRequired}
                   calibratePipetteRequired={calibratePipetteRequired}
                   updatePipetteFWRequired={updatePipetteFWRequired}
+                  latestRequestId={getLatestRequestId(module.serialNumber)}
+                  handleModuleApiRequests={handleModuleApiRequests}
                 />
               ))}
             </Flex>
@@ -267,6 +271,8 @@ export function InstrumentsAndModules({
                   attachPipetteRequired={attachPipetteRequired}
                   calibratePipetteRequired={calibratePipetteRequired}
                   updatePipetteFWRequired={updatePipetteFWRequired}
+                  latestRequestId={getLatestRequestId(module.serialNumber)}
+                  handleModuleApiRequests={handleModuleApiRequests}
                 />
               ))}
             </Flex>

app/src/organisms/Devices/ProtocolRun/ProtocolRunModuleControls.tsx

@@ -10,6 +10,8 @@ import {
 } from '@opentrons/components'
 import { ModuleCard } from '../../ModuleCard'
 import { useModuleRenderInfoForProtocolById } from '../hooks'
+import { useModuleApiRequests } from '../../ModuleCard/utils'
+
 import type { BadPipette, PipetteData } from '@opentrons/api-client'
 
 interface PipetteStatus {
@@ -77,6 +79,7 @@ export const ProtocolRunModuleControls = ({
     calibratePipetteRequired,
     updatePipetteFWRequired,
   } = usePipetteIsReady()
+  const [getLatestRequestId, handleModuleApiRequests] = useModuleApiRequests()
 
   const moduleRenderInfoForProtocolById = useModuleRenderInfoForProtocolById(
     runId,
@@ -120,6 +123,10 @@ export const ProtocolRunModuleControls = ({
               attachPipetteRequired={attachPipetteRequired}
               calibratePipetteRequired={calibratePipetteRequired}
               updatePipetteFWRequired={updatePipetteFWRequired}
+              latestRequestId={getLatestRequestId(
+                module.attachedModuleMatch.serialNumber
+              )}
+              handleModuleApiRequests={handleModuleApiRequests}
             />
           ) : null
         )}
@@ -141,6 +148,10 @@ export const ProtocolRunModuleControls = ({
               attachPipetteRequired={attachPipetteRequired}
               calibratePipetteRequired={calibratePipetteRequired}
               updatePipetteFWRequired={updatePipetteFWRequired}
+              latestRequestId={getLatestRequestId(
+                module.attachedModuleMatch.serialNumber
+              )}
+              handleModuleApiRequests={handleModuleApiRequests}
             />
           ) : null
         )}

app/src/organisms/ModuleCard/__tests__/ModuleCard.test.tsx

@@ -24,7 +24,6 @@ import {
   getRequestById,
   PENDING,
   SUCCESS,
-  useDispatchApiRequest,
 } from '../../../redux/robot-api'
 import { useCurrentRunStatus } from '../../RunTimeControl/hooks'
 import { useToaster } from '../../ToasterOven'
@@ -43,7 +42,7 @@ import type {
   MagneticModule,
   ThermocyclerModule,
 } from '../../../redux/modules/types'
-import type { DispatchApiRequestType } from '../../../redux/robot-api'
+import type { Mock } from 'vitest'
 
 vi.mock('../ErrorInfo')
 vi.mock('../MagneticModuleData')
@@ -182,32 +181,33 @@ const mockMakeSnackbar = vi.fn()
 const mockMakeToast = vi.fn()
 const mockEatToast = vi.fn()
 
+const MOCK_LATEST_REQUEST_ID = '1234'
+
 const render = (props: React.ComponentProps<typeof ModuleCard>) => {
   return renderWithProviders(<ModuleCard {...props} />, {
     i18nInstance: i18n,
   })[0]
 }
 
 describe('ModuleCard', () => {
-  let dispatchApiRequest: DispatchApiRequestType
   let props: React.ComponentProps<typeof ModuleCard>
+  let mockHandleModuleApiRequests: Mock
 
   beforeEach(() => {
+    mockHandleModuleApiRequests = vi.fn()
+
     props = {
       module: mockMagneticModule,
       robotName: mockRobot.name,
       isLoadedInRun: false,
       attachPipetteRequired: false,
       calibratePipetteRequired: false,
       updatePipetteFWRequired: false,
+      handleModuleApiRequests: mockHandleModuleApiRequests,
+      latestRequestId: MOCK_LATEST_REQUEST_ID,
     }
 
-    dispatchApiRequest = vi.fn()
     vi.mocked(ErrorInfo).mockReturnValue(null)
-    vi.mocked(useDispatchApiRequest).mockReturnValue([
-      dispatchApiRequest,
-      ['id'],
-    ])
     vi.mocked(MagneticModuleData).mockReturnValue(
       <div>Mock Magnetic Module Data</div>
     )

app/src/organisms/ModuleCard/__tests__/utils.test.ts

@@ -1,4 +1,5 @@
-import { describe, expect, it } from 'vitest'
+import { describe, expect, it, vi, beforeEach } from 'vitest'
+import { renderHook, act } from '@testing-library/react'
 
 import {
   mockHeaterShaker,
@@ -9,7 +10,10 @@ import {
   mockThermocycler,
   mockThermocyclerGen2,
 } from '../../../redux/modules/__fixtures__'
-import { getModuleCardImage } from '../utils'
+import { getModuleCardImage, useModuleApiRequests } from '../utils'
+import { useDispatchApiRequest } from '../../../redux/robot-api'
+
+vi.mock('../../../redux/robot-api')
 
 const mockThermocyclerGen2ClosedLid = {
   id: 'thermocycler_id2',
@@ -83,3 +87,29 @@ describe('getModuleCardImage', () => {
     )
   })
 })
+
+const updateModuleAction = { meta: { requestId: '12345' } }
+const MOCK_ROBOT_NAME = 'MOCK_ROBOT'
+const MOCK_SERIAL_NUMBER = '1234'
+const mockDispatchApiRequest = () => updateModuleAction
+
+describe('useModuleApiRequests', () => {
+  beforeEach(() => {
+    vi.mocked(useDispatchApiRequest).mockReturnValue([
+      mockDispatchApiRequest,
+    ] as any)
+  })
+
+  it('should dispatch an API request and update requestIdsBySerial on handleModuleApiRequests', () => {
+    const { result } = renderHook(() => useModuleApiRequests())
+
+    act(() => {
+      result.current[1](MOCK_ROBOT_NAME, MOCK_SERIAL_NUMBER)
+    })
+
+    expect(result.current[0](MOCK_SERIAL_NUMBER)).toEqual(
+      updateModuleAction.meta.requestId
+    )
+    expect(result.current[0]('NON_EXISTENT_SERIAL')).toBeNull()
+  })
+})

app/src/organisms/ModuleCard/index.tsx

@@ -1,7 +1,6 @@
 import * as React from 'react'
 import { Trans, useTranslation } from 'react-i18next'
 import { useDispatch, useSelector } from 'react-redux'
-import last from 'lodash/last'
 import { useHistory } from 'react-router-dom'
 
 import {
@@ -31,9 +30,7 @@ import {
 import { RUN_STATUS_FINISHING, RUN_STATUS_RUNNING } from '@opentrons/api-client'
 
 import { OverflowBtn } from '../../atoms/MenuList/OverflowBtn'
-import { updateModule } from '../../redux/modules'
 import {
-  useDispatchApiRequest,
   getRequestById,
   PENDING,
   FAILURE,
@@ -85,6 +82,8 @@ interface ModuleCardProps {
   attachPipetteRequired: boolean
   calibratePipetteRequired: boolean
   updatePipetteFWRequired: boolean
+  latestRequestId: string | null
+  handleModuleApiRequests: (robotName: string, serialNumber: string) => void
   runId?: string
   slotName?: string
 }
@@ -100,6 +99,8 @@ export const ModuleCard = (props: ModuleCardProps): JSX.Element | null => {
     attachPipetteRequired,
     calibratePipetteRequired,
     updatePipetteFWRequired,
+    latestRequestId,
+    handleModuleApiRequests,
   } = props
   const dispatch = useDispatch<Dispatch>()
   const {
@@ -115,13 +116,12 @@ export const ModuleCard = (props: ModuleCardProps): JSX.Element | null => {
   const [hasSecondary, setHasSecondary] = React.useState(false)
   const [showAboutModule, setShowAboutModule] = React.useState(false)
   const [showTestShake, setShowTestShake] = React.useState(false)
-  const [showHSWizard, setShowHSWizard] = React.useState<boolean>(false)
-  const [showFWBanner, setShowFWBanner] = React.useState<boolean>(true)
-  const [showCalModal, setShowCalModal] = React.useState<boolean>(false)
+  const [showHSWizard, setShowHSWizard] = React.useState(false)
+  const [showFWBanner, setShowFWBanner] = React.useState(true)
+  const [showCalModal, setShowCalModal] = React.useState(false)
 
   const [targetProps, tooltipProps] = useHoverTooltip()
   const history = useHistory()
-  const [dispatchApiRequest, requestIds] = useDispatchApiRequest()
   const runStatus = useCurrentRunStatus({
     onSettled: data => {
       if (data == null) {
@@ -138,29 +138,31 @@ export const ModuleCard = (props: ModuleCardProps): JSX.Element | null => {
     (!attachPipetteRequired ?? false) &&
     (!calibratePipetteRequired ?? false) &&
     (!updatePipetteFWRequired ?? false)
-  const latestRequestId = last(requestIds)
+
   const latestRequest = useSelector<State, RequestState | null>(state =>
-    latestRequestId ? getRequestById(state, latestRequestId) : null
+    latestRequestId != null ? getRequestById(state, latestRequestId) : null
   )
-  const isEstopNotDisengaged = useIsEstopNotDisengaged(robotName)
 
-  const handleCloseErrorModal = (): void => {
-    if (latestRequestId != null) {
-      dispatch(dismissRequest(latestRequestId))
-    }
+  const hasUpdated =
+    !module.hasAvailableUpdate && latestRequest?.status === SUCCESS
+  const [showFirmwareToast, setShowFirmwareToast] = React.useState(hasUpdated)
+  const { makeToast } = useToaster()
+  if (showFirmwareToast) {
+    makeToast(t('firmware_updated_successfully'), SUCCESS_TOAST)
+    setShowFirmwareToast(false)
   }
 
   const handleFirmwareUpdateClick = (): void => {
-    robotName &&
-      dispatchApiRequest(updateModule(robotName, module.serialNumber))
+    robotName && handleModuleApiRequests(robotName, module.serialNumber)
   }
 
-  const { makeToast } = useToaster()
-  React.useEffect(() => {
-    if (!module.hasAvailableUpdate && latestRequest?.status === SUCCESS) {
-      makeToast(t('firmware_update_installation_successful'), SUCCESS_TOAST)
+  const isEstopNotDisengaged = useIsEstopNotDisengaged(robotName)
+
+  const handleCloseErrorModal = (): void => {
+    if (latestRequestId != null) {
+      dispatch(dismissRequest(latestRequestId))
     }
-  }, [module.hasAvailableUpdate, latestRequest?.status, makeToast, t])
+  }
 
   const isPending = latestRequest?.status === PENDING
   const hotToTouch: IconProps = { name: 'ot-hot-to-touch' }

app/src/organisms/ModuleCard/utils.ts

@@ -1,10 +1,17 @@
+import * as React from 'react'
+import last from 'lodash/last'
+
+import { useDispatchApiRequest } from '../../redux/robot-api'
+import { updateModule } from '../../redux/modules'
+
 import magneticModule from '../../assets/images/magnetic_module_gen_2_transparent.png'
 import temperatureModule from '../../assets/images/temp_deck_gen_2_transparent.png'
 import thermoModuleGen1Closed from '../../assets/images/thermocycler_closed.png'
 import thermoModuleGen1Opened from '../../assets/images/thermocycler_open_transparent.png'
 import heaterShakerModule from '../../assets/images/heater_shaker_module_transparent.png'
 import thermoModuleGen2Closed from '../../assets/images/thermocycler_gen_2_closed.png'
 import thermoModuleGen2Opened from '../../assets/images/thermocycler_gen_2_opened.png'
+
 import type { AttachedModule } from '../../redux/modules/types'
 
 export function getModuleCardImage(attachedModule: AttachedModule): string {
@@ -35,3 +42,58 @@ export function getModuleCardImage(attachedModule: AttachedModule): string {
       return 'unknown module model, this is an error'
   }
 }
+
+type RequestIdsBySerialNumber = Record<string, string[]>
+type HandleModuleApiRequestsType = (robotName: string, moduleId: string) => void
+type GetLatestRequestIdType = (moduleId: string) => string | null
+
+export function useModuleApiRequests(): [
+  GetLatestRequestIdType,
+  HandleModuleApiRequestsType
+] {
+  const [dispatchApiRequest] = useDispatchApiRequest()
+  const [
+    requestIdsBySerial,
+    setRequestIdsBySerial,
+  ] = React.useState<RequestIdsBySerialNumber>({})
+
+  const handleModuleApiRequests = (
+    robotName: string,
+    serialNumber: string
+  ): void => {
+    const action = dispatchApiRequest(updateModule(robotName, serialNumber))
+    const { requestId } = action.meta
+
+    if (requestId != null) {
+      if (serialNumber in requestIdsBySerial) {
+        setRequestIdsBySerial((prevState: RequestIdsBySerialNumber) => {
+          const existingRequestIds = prevState[serialNumber] || []
+          return {
+            ...prevState,
+            [serialNumber]: [...existingRequestIds, requestId],
+          }
+        })
+      } else {
+        setRequestIdsBySerial(prevState => {
+          return {
+            ...prevState,
+            [serialNumber]: [requestId],
+          }
+        })
+      }
+    }
+  }
+
+  const getLatestRequestId = React.useCallback(
+    (serialNumber: string): string | null => {
+      if (serialNumber in requestIdsBySerial) {
+        return last(requestIdsBySerial[serialNumber]) ?? null
+      } else {
+        return null
+      }
+    },
+    [requestIdsBySerial]
+  )
+
+  return [getLatestRequestId, handleModuleApiRequests]
+}