From fbf2a4c66d99af63793d40068381ec1733b795c5 Mon Sep 17 00:00:00 2001 From: Jamey Huffnagle Date: Tue, 23 Apr 2024 12:09:41 -0400 Subject: [PATCH] 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. --- .../localization/en/device_details.json | 2 +- .../Devices/InstrumentsAndModules.tsx | 6 ++ .../ProtocolRun/ProtocolRunModuleControls.tsx | 11 ++++ .../ModuleCard/__tests__/ModuleCard.test.tsx | 16 ++--- .../ModuleCard/__tests__/utils.test.ts | 34 +++++++++- app/src/organisms/ModuleCard/index.tsx | 44 ++++++------- app/src/organisms/ModuleCard/utils.ts | 62 +++++++++++++++++++ 7 files changed, 143 insertions(+), 32 deletions(-) diff --git a/app/src/assets/localization/en/device_details.json b/app/src/assets/localization/en/device_details.json index df0d7c743e2..d3fdab0b04c 100644 --- a/app/src/assets/localization/en/device_details.json +++ b/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.", diff --git a/app/src/organisms/Devices/InstrumentsAndModules.tsx b/app/src/organisms/Devices/InstrumentsAndModules.tsx index 07b78af63cb..04068e8e21c 100644 --- a/app/src/organisms/Devices/InstrumentsAndModules.tsx +++ b/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} /> ))} @@ -267,6 +271,8 @@ export function InstrumentsAndModules({ attachPipetteRequired={attachPipetteRequired} calibratePipetteRequired={calibratePipetteRequired} updatePipetteFWRequired={updatePipetteFWRequired} + latestRequestId={getLatestRequestId(module.serialNumber)} + handleModuleApiRequests={handleModuleApiRequests} /> ))} diff --git a/app/src/organisms/Devices/ProtocolRun/ProtocolRunModuleControls.tsx b/app/src/organisms/Devices/ProtocolRun/ProtocolRunModuleControls.tsx index fa9aad2e7d1..4930efee2d3 100644 --- a/app/src/organisms/Devices/ProtocolRun/ProtocolRunModuleControls.tsx +++ b/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 )} diff --git a/app/src/organisms/ModuleCard/__tests__/ModuleCard.test.tsx b/app/src/organisms/ModuleCard/__tests__/ModuleCard.test.tsx index 74ca18bef61..8c6dbcfd025 100644 --- a/app/src/organisms/ModuleCard/__tests__/ModuleCard.test.tsx +++ b/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,6 +181,8 @@ const mockMakeSnackbar = vi.fn() const mockMakeToast = vi.fn() const mockEatToast = vi.fn() +const MOCK_LATEST_REQUEST_ID = '1234' + const render = (props: React.ComponentProps) => { return renderWithProviders(, { i18nInstance: i18n, @@ -189,10 +190,12 @@ const render = (props: React.ComponentProps) => { } describe('ModuleCard', () => { - let dispatchApiRequest: DispatchApiRequestType let props: React.ComponentProps + let mockHandleModuleApiRequests: Mock beforeEach(() => { + mockHandleModuleApiRequests = vi.fn() + props = { module: mockMagneticModule, robotName: mockRobot.name, @@ -200,14 +203,11 @@ describe('ModuleCard', () => { 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(
Mock Magnetic Module Data
) diff --git a/app/src/organisms/ModuleCard/__tests__/utils.test.ts b/app/src/organisms/ModuleCard/__tests__/utils.test.ts index 311c9676da0..5798efeb827 100644 --- a/app/src/organisms/ModuleCard/__tests__/utils.test.ts +++ b/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() + }) +}) diff --git a/app/src/organisms/ModuleCard/index.tsx b/app/src/organisms/ModuleCard/index.tsx index c2c42151eda..52f3ed99f65 100644 --- a/app/src/organisms/ModuleCard/index.tsx +++ b/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() 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(false) - const [showFWBanner, setShowFWBanner] = React.useState(true) - const [showCalModal, setShowCalModal] = React.useState(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 => - 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' } diff --git a/app/src/organisms/ModuleCard/utils.ts b/app/src/organisms/ModuleCard/utils.ts index c80cfa2c4fe..dfd136bfcfc 100644 --- a/app/src/organisms/ModuleCard/utils.ts +++ b/app/src/organisms/ModuleCard/utils.ts @@ -1,3 +1,9 @@ +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' @@ -5,6 +11,7 @@ import thermoModuleGen1Opened from '../../assets/images/thermocycler_open_transp 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 +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({}) + + 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] +}