From 4693d04e36daec4fab9b4be1e8d77931eb661646 Mon Sep 17 00:00:00 2001 From: Jamey Huffnagle Date: Wed, 7 Aug 2024 11:14:19 -0400 Subject: [PATCH] refactor(app): Refactor intervention modal render behavior (#15898) Closes RQA-2904 The logic for rendering InterventionModal on the ODD/Desktop is a little bit different when looking at the exact conditions, and this (likely) causes the InterventionModal to render on the ODD sometimes but not on the desktop app, and vice versa. This is a good opportunity to refactor all of this logic into its own hook and use that hook where we render InterventionModal. After thinking through the render logic, there's room to simplify it a bit, too. We don't actually need stateful storage of an intervention command key. Also, I decided to separate showModal from modalProps (which lets us pass all the non-null props simply), even though we could technically just do a truthy check for modalProps for rendering InterventionModal, since this is maybe a bit more intuitive. Lastly, a few missing tests are added. To help with bug testing intervention modals, I added a couple console.warns. --- .../__tests__/InterventionModal.test.tsx | 67 ++++++- app/src/organisms/InterventionModal/index.tsx | 180 +++++++++++++----- .../__tests__/RunProgressMeter.test.tsx | 48 ++--- app/src/organisms/RunProgressMeter/index.tsx | 49 ++--- .../__tests__/RunningProtocol.test.tsx | 22 +++ app/src/pages/RunningProtocol/index.tsx | 52 ++--- 6 files changed, 262 insertions(+), 156 deletions(-) diff --git a/app/src/organisms/InterventionModal/__tests__/InterventionModal.test.tsx b/app/src/organisms/InterventionModal/__tests__/InterventionModal.test.tsx index 06f4f0a22a3..e1a6830d251 100644 --- a/app/src/organisms/InterventionModal/__tests__/InterventionModal.test.tsx +++ b/app/src/organisms/InterventionModal/__tests__/InterventionModal.test.tsx @@ -1,11 +1,13 @@ import * as React from 'react' -import { fireEvent, screen } from '@testing-library/react' +import { fireEvent, renderHook, screen } from '@testing-library/react' import { describe, it, expect, vi, beforeEach } from 'vitest' + +import { RUN_STATUS_RUNNING, RUN_STATUS_STOPPED } from '@opentrons/api-client' import { getLabwareDefURI } from '@opentrons/shared-data' -import { renderWithProviders } from '../../../__testing-utils__' +import { renderWithProviders } from '../../../__testing-utils__' +import { mockTipRackDefinition } from '../../../redux/custom-labware/__fixtures__' import { i18n } from '../../../i18n' -import { InterventionModal } from '..' import { mockPauseCommandWithoutStartTime, mockPauseCommandWithStartTime, @@ -13,9 +15,11 @@ import { mockMoveLabwareCommandFromModule, truncatedCommandMessage, } from '../__fixtures__' -import { mockTipRackDefinition } from '../../../redux/custom-labware/__fixtures__' +import { InterventionModal, useInterventionModal } from '..' import { useIsFlex } from '../../Devices/hooks' + import type { CompletedProtocolAnalysis } from '@opentrons/shared-data' +import type { RunData } from '@opentrons/api-client' const ROBOT_NAME = 'Otie' @@ -23,6 +27,61 @@ const mockOnResumeHandler = vi.fn() vi.mock('../../Devices/hooks') +describe('useInterventionModal', () => { + const defaultProps = { + runData: { id: 'run1' } as RunData, + lastRunCommand: mockPauseCommandWithStartTime, + runStatus: RUN_STATUS_RUNNING, + robotName: 'TestRobot', + analysis: null, + } + + it('should return showModal true when conditions are met', () => { + const { result } = renderHook(() => useInterventionModal(defaultProps)) + + expect(result.current.showModal).toBe(true) + expect(result.current.modalProps).not.toBeNull() + }) + + it('should return showModal false when runStatus is terminal', () => { + const props = { ...defaultProps, runStatus: RUN_STATUS_STOPPED } + + const { result } = renderHook(() => useInterventionModal(props)) + + expect(result.current.showModal).toBe(false) + expect(result.current.modalProps).toBeNull() + }) + + it('should return showModal false when lastRunCommand is null', () => { + const props = { ...defaultProps, lastRunCommand: null } + + const { result } = renderHook(() => useInterventionModal(props)) + + expect(result.current.showModal).toBe(false) + expect(result.current.modalProps).toBeNull() + }) + + it('should return showModal false when robotName is null', () => { + const props = { ...defaultProps, robotName: null } + + const { result } = renderHook(() => useInterventionModal(props)) + + expect(result.current.showModal).toBe(false) + expect(result.current.modalProps).toBeNull() + }) + + it('should return correct modalProps when showModal is true', () => { + const { result } = renderHook(() => useInterventionModal(defaultProps)) + + expect(result.current.modalProps).toEqual({ + command: mockPauseCommandWithStartTime, + run: defaultProps.runData, + robotName: 'TestRobot', + analysis: null, + }) + }) +}) + const render = (props: React.ComponentProps) => { return renderWithProviders(, { i18nInstance: i18n, diff --git a/app/src/organisms/InterventionModal/index.tsx b/app/src/organisms/InterventionModal/index.tsx index 1e7cb9e8475..6c3002ce26b 100644 --- a/app/src/organisms/InterventionModal/index.tsx +++ b/app/src/organisms/InterventionModal/index.tsx @@ -19,6 +19,12 @@ import { TYPOGRAPHY, LegacyStyledText, } from '@opentrons/components' +import { + RUN_STATUS_FAILED, + RUN_STATUS_FINISHING, + RUN_STATUS_STOPPED, + RUN_STATUS_SUCCEEDED, +} from '@opentrons/api-client' import { SmallButton } from '../../atoms/buttons' import { Modal } from '../../molecules/Modal' @@ -26,30 +32,66 @@ import { InterventionModal as InterventionModalMolecule } from '../../molecules/ import { getIsOnDevice } from '../../redux/config' import { PauseInterventionContent } from './PauseInterventionContent' import { MoveLabwareInterventionContent } from './MoveLabwareInterventionContent' +import { isInterventionCommand } from './utils' +import { useRobotType } from '../Devices/hooks' -import type { RunCommandSummary, RunData } from '@opentrons/api-client' import type { IconName } from '@opentrons/components' import type { CompletedProtocolAnalysis } from '@opentrons/shared-data' -import { useRobotType } from '../Devices/hooks' +import type { + RunCommandSummary, + RunData, + RunStatus, +} from '@opentrons/api-client' -const LEARN_ABOUT_MANUAL_STEPS_URL = - 'https://support.opentrons.com/s/article/Manual-protocol-steps' +const TERMINAL_RUN_STATUSES: RunStatus[] = [ + RUN_STATUS_STOPPED, + RUN_STATUS_FAILED, + RUN_STATUS_FINISHING, + RUN_STATUS_SUCCEEDED, +] -const CONTENT_STYLE = { - display: DISPLAY_FLEX, - flexDirection: DIRECTION_COLUMN, - alignItems: ALIGN_FLEX_START, - gridGap: SPACING.spacing24, - padding: SPACING.spacing32, - borderRadius: BORDERS.borderRadius8, -} as const +export interface UseInterventionModalProps { + runData: RunData | null + lastRunCommand: RunCommandSummary | null + runStatus: RunStatus | null + robotName: string | null + analysis: CompletedProtocolAnalysis | null +} -const FOOTER_STYLE = { - display: DISPLAY_FLEX, - width: '100%', - alignItems: ALIGN_CENTER, - justifyContent: JUSTIFY_SPACE_BETWEEN, -} as const +export type UseInterventionModalResult = + | { showModal: false; modalProps: null } + | { showModal: true; modalProps: Omit } + +// If showModal is true, modalProps are guaranteed not to be null. +export function useInterventionModal({ + runData, + lastRunCommand, + runStatus, + robotName, + analysis, +}: UseInterventionModalProps): UseInterventionModalResult { + const isValidIntervention = + lastRunCommand != null && + robotName != null && + isInterventionCommand(lastRunCommand) && + runData != null && + runStatus != null && + !TERMINAL_RUN_STATUSES.includes(runStatus) + + if (!isValidIntervention) { + return { showModal: false, modalProps: null } + } else { + return { + showModal: true, + modalProps: { + command: lastRunCommand, + run: runData, + robotName, + analysis, + }, + } + } +} export interface InterventionModalProps { robotName: string @@ -71,25 +113,28 @@ export function InterventionModal({ const robotType = useRobotType(robotName) const childContent = React.useMemo(() => { - if ( - command.commandType === 'waitForResume' || - command.commandType === 'pause' // legacy pause command - ) { - return ( - - ) - } else if (command.commandType === 'moveLabware') { - return ( - - ) - } else { - return null + switch (command.commandType) { + case 'waitForResume': + case 'pause': // legacy pause command + return ( + + ) + case 'moveLabware': + return ( + + ) + default: + console.warn( + 'Unhandled command passed to InterventionModal: ', + command.commandType + ) + return null } }, [ command.id, @@ -98,21 +143,33 @@ export function InterventionModal({ run.modules.map(m => m.id).join(), ]) - let iconName: IconName | null = null - let headerTitle = '' - let headerTitleOnDevice = '' - if ( - command.commandType === 'waitForResume' || - command.commandType === 'pause' // legacy pause command - ) { - iconName = 'pause-circle' - headerTitle = t('pause_on', { robot_name: robotName }) - headerTitleOnDevice = t('pause') - } else if (command.commandType === 'moveLabware') { - iconName = 'move-xy-circle' - headerTitle = t('move_labware_on', { robot_name: robotName }) - headerTitleOnDevice = t('move_labware') - } + const { iconName, headerTitle, headerTitleOnDevice } = (() => { + switch (command.commandType) { + case 'waitForResume': + case 'pause': + return { + iconName: 'pause-circle' as IconName, + headerTitle: t('pause_on', { robot_name: robotName }), + headerTitleOnDevice: t('pause'), + } + case 'moveLabware': + return { + iconName: 'move-xy-circle' as IconName, + headerTitle: t('move_labware_on', { robot_name: robotName }), + headerTitleOnDevice: t('move_labware'), + } + default: + console.warn( + 'Unhandled command passed to InterventionModal: ', + command.commandType + ) + return { + iconName: null, + headerTitle: '', + headerTitleOnDevice: '', + } + } + })() // TODO(bh, 2023-7-18): this is a one-off modal implementation for desktop // reimplement when design system shares a modal component between desktop/ODD @@ -171,3 +228,22 @@ export function InterventionModal({ ) } + +const LEARN_ABOUT_MANUAL_STEPS_URL = + 'https://support.opentrons.com/s/article/Manual-protocol-steps' + +const CONTENT_STYLE = { + display: DISPLAY_FLEX, + flexDirection: DIRECTION_COLUMN, + alignItems: ALIGN_FLEX_START, + gridGap: SPACING.spacing24, + padding: SPACING.spacing32, + borderRadius: BORDERS.borderRadius8, +} as const + +const FOOTER_STYLE = { + display: DISPLAY_FLEX, + width: '100%', + alignItems: ALIGN_CENTER, + justifyContent: JUSTIFY_SPACE_BETWEEN, +} as const diff --git a/app/src/organisms/RunProgressMeter/__tests__/RunProgressMeter.test.tsx b/app/src/organisms/RunProgressMeter/__tests__/RunProgressMeter.test.tsx index 0cab1ef5adb..10ecdb7bf9e 100644 --- a/app/src/organisms/RunProgressMeter/__tests__/RunProgressMeter.test.tsx +++ b/app/src/organisms/RunProgressMeter/__tests__/RunProgressMeter.test.tsx @@ -13,7 +13,10 @@ import { } from '@opentrons/api-client' import { i18n } from '../../../i18n' -import { InterventionModal } from '../../InterventionModal' +import { + useInterventionModal, + InterventionModal, +} from '../../InterventionModal' import { ProgressBar } from '../../../atoms/ProgressBar' import { useRunStatus } from '../../RunTimeControl/hooks' import { useMostRecentCompletedAnalysis } from '../../LabwarePositionCheck/useMostRecentCompletedAnalysis' @@ -27,11 +30,7 @@ import { mockUseCommandResultNonDeterministic, NON_DETERMINISTIC_COMMAND_KEY, } from '../__fixtures__' -import { - mockMoveLabwareCommandFromSlot, - mockPauseCommandWithStartTime, - mockRunData, -} from '../../InterventionModal/__fixtures__' + import { RunProgressMeter } from '..' import { renderWithProviders } from '../../../__testing-utils__' import { useLastRunCommand } from '../../Devices/hooks/useLastRunCommand' @@ -70,7 +69,7 @@ describe('RunProgressMeter', () => { beforeEach(() => { vi.mocked(ProgressBar).mockReturnValue(
MOCK PROGRESS BAR
) vi.mocked(InterventionModal).mockReturnValue( -
MOCK INTERVENTION MODAL
+
MOCK_INTERVENTION_MODAL
) vi.mocked(useRunStatus).mockReturnValue(RUN_STATUS_RUNNING) when(useMostRecentCompletedAnalysis) @@ -96,6 +95,10 @@ describe('RunProgressMeter', () => { currentStepNumber: null, hasRunDiverged: true, }) + vi.mocked(useInterventionModal).mockReturnValue({ + showModal: false, + modalProps: {} as any, + }) props = { runId: NON_DETERMINISTIC_RUN_ID, @@ -119,31 +122,18 @@ describe('RunProgressMeter', () => { screen.getByText('Not started yet') screen.getByText('Download run log') }) - it('should render an intervention modal when lastRunCommand is a pause command', () => { - vi.mocked(useNotifyAllCommandsQuery).mockReturnValue({ - data: { data: [mockPauseCommandWithStartTime], meta: { totalLength: 1 } }, - } as any) - vi.mocked(useNotifyRunQuery).mockReturnValue({ - data: { data: { labware: [] } }, - } as any) - vi.mocked(useCommandQuery).mockReturnValue({ data: null } as any) - vi.mocked(useMostRecentCompletedAnalysis).mockReturnValue({} as any) - render(props) - }) - it('should render an intervention modal when lastRunCommand is a move labware command', () => { - vi.mocked(useNotifyAllCommandsQuery).mockReturnValue({ - data: { - data: [mockMoveLabwareCommandFromSlot], - meta: { totalLength: 1 }, - }, - } as any) - vi.mocked(useNotifyRunQuery).mockReturnValue({ - data: { data: mockRunData }, - } as any) + it('should render an intervention modal when showInterventionModal is true', () => { vi.mocked(useCommandQuery).mockReturnValue({ data: null } as any) - vi.mocked(useMostRecentCompletedAnalysis).mockReturnValue({} as any) + vi.mocked(useRunStatus).mockReturnValue(RUN_STATUS_IDLE) + vi.mocked(useInterventionModal).mockReturnValue({ + showModal: true, + modalProps: {} as any, + }) + render(props) + + screen.getByText('MOCK_INTERVENTION_MODAL') }) it('should render the correct run status when run status is completed', () => { diff --git a/app/src/organisms/RunProgressMeter/index.tsx b/app/src/organisms/RunProgressMeter/index.tsx index 5f120904a41..eecf73a96f9 100644 --- a/app/src/organisms/RunProgressMeter/index.tsx +++ b/app/src/organisms/RunProgressMeter/index.tsx @@ -30,17 +30,15 @@ import { useMostRecentCompletedAnalysis } from '../LabwarePositionCheck/useMostR import { getModalPortalEl } from '../../App/portal' import { Tooltip } from '../../atoms/Tooltip' import { useRunStatus } from '../RunTimeControl/hooks' -import { InterventionModal } from '../InterventionModal' +import { InterventionModal, useInterventionModal } from '../InterventionModal' import { ProgressBar } from '../../atoms/ProgressBar' import { useDownloadRunLog, useRobotType } from '../Devices/hooks' import { InterventionTicks } from './InterventionTicks' -import { isInterventionCommand } from '../InterventionModal/utils' import { useNotifyRunQuery, useNotifyAllCommandsQuery, } from '../../resources/runs' import { useRunningStepCounts } from '../../resources/protocols/hooks' -import { TERMINAL_RUN_STATUSES } from './constants' import { useRunProgressCopy } from './hooks' interface RunProgressMeterProps { @@ -51,10 +49,6 @@ interface RunProgressMeterProps { } export function RunProgressMeter(props: RunProgressMeterProps): JSX.Element { const { runId, robotName, makeHandleJumpToStep, resumeRunHandler } = props - const [ - interventionModalCommandKey, - setInterventionModalCommandKey, - ] = React.useState(null) const { t } = useTranslation('run_details') const robotType = useRobotType(robotName) const runStatus = useRunStatus(runId) @@ -91,23 +85,6 @@ export function RunProgressMeter(props: RunProgressMeterProps): JSX.Element { const { downloadRunLog } = useDownloadRunLog(robotName, runId) - React.useEffect(() => { - if ( - lastRunCommand != null && - interventionModalCommandKey != null && - lastRunCommand.key !== interventionModalCommandKey - ) { - // set intervention modal command key to null if different from current command key - setInterventionModalCommandKey(null) - } else if ( - lastRunCommand?.key != null && - isInterventionCommand(lastRunCommand) && - interventionModalCommandKey === null - ) { - setInterventionModalCommandKey(lastRunCommand.key) - } - }, [lastRunCommand, interventionModalCommandKey]) - const onDownloadClick: React.MouseEventHandler = e => { if (downloadIsDisabled) return false e.preventDefault() @@ -115,6 +92,17 @@ export function RunProgressMeter(props: RunProgressMeterProps): JSX.Element { downloadRunLog() } + const { + showModal: showIntervention, + modalProps: interventionProps, + } = useInterventionModal({ + robotName, + runStatus, + runData, + analysis, + lastRunCommand, + }) + const { progressPercentage, stepCountStr, @@ -132,20 +120,11 @@ export function RunProgressMeter(props: RunProgressMeterProps): JSX.Element { return ( <> - {interventionModalCommandKey != null && - lastRunCommand != null && - isInterventionCommand(lastRunCommand) && - analysisCommands != null && - runStatus != null && - runData != null && - !TERMINAL_RUN_STATUSES.includes(runStatus) + {showIntervention ? createPortal( , getModalPortalEl() ) diff --git a/app/src/pages/RunningProtocol/__tests__/RunningProtocol.test.tsx b/app/src/pages/RunningProtocol/__tests__/RunningProtocol.test.tsx index 1114f4964eb..bddb00263d4 100644 --- a/app/src/pages/RunningProtocol/__tests__/RunningProtocol.test.tsx +++ b/app/src/pages/RunningProtocol/__tests__/RunningProtocol.test.tsx @@ -44,6 +44,10 @@ import { useErrorRecoveryFlows, } from '../../../organisms/ErrorRecoveryFlows' import { useLastRunCommand } from '../../../organisms/Devices/hooks/useLastRunCommand' +import { + useInterventionModal, + InterventionModal, +} from '../../../organisms/InterventionModal' import type { UseQueryResult } from 'react-query' import type { ProtocolAnalyses, RunCommandSummary } from '@opentrons/api-client' @@ -64,6 +68,7 @@ vi.mock('../../../resources/runs') vi.mock('../../../redux/config') vi.mock('../../../organisms/ErrorRecoveryFlows') vi.mock('../../../organisms/Devices/hooks/useLastRunCommand') +vi.mock('../../../organisms/InterventionModal') const RUN_ID = 'run_id' const ROBOT_NAME = 'otie' @@ -159,6 +164,13 @@ describe('RunningProtocol', () => { isERActive: false, failedCommand: {} as any, }) + vi.mocked(useInterventionModal).mockReturnValue({ + showModal: false, + modalProps: {} as any, + }) + vi.mocked(InterventionModal).mockReturnValue( +
MOCK_INTERVENTION_MODAL
+ ) }) afterEach(() => { @@ -219,6 +231,16 @@ describe('RunningProtocol', () => { screen.getByText('MOCK ERROR RECOVERY') }) + it('should render an InterventionModal appropriately', () => { + vi.mocked(useInterventionModal).mockReturnValue({ + showModal: true, + modalProps: {} as any, + }) + render(`/runs/${RUN_ID}/run`) + + screen.getByText('MOCK_INTERVENTION_MODAL') + }) + // ToDo (kj:04/04/2023) need to figure out the way to simulate swipe it.todo('should render RunningProtocolCommandList when swiping left') // const [{ getByText }] = render(`/runs/${RUN_ID}/run`) diff --git a/app/src/pages/RunningProtocol/index.tsx b/app/src/pages/RunningProtocol/index.tsx index df35c6fa846..f1ca179167d 100644 --- a/app/src/pages/RunningProtocol/index.tsx +++ b/app/src/pages/RunningProtocol/index.tsx @@ -24,14 +24,15 @@ import { import { RUN_STATUS_STOP_REQUESTED, RUN_STATUS_BLOCKED_BY_OPEN_DOOR, - RUN_STATUS_FINISHING, } from '@opentrons/api-client' import { StepMeter } from '../../atoms/StepMeter' import { useMostRecentCompletedAnalysis } from '../../organisms/LabwarePositionCheck/useMostRecentCompletedAnalysis' import { useNotifyRunQuery } from '../../resources/runs' -import { InterventionModal } from '../../organisms/InterventionModal' -import { isInterventionCommand } from '../../organisms/InterventionModal/utils' +import { + InterventionModal, + useInterventionModal, +} from '../../organisms/InterventionModal' import { useRunStatus, useRunTimestamps, @@ -89,10 +90,6 @@ export function RunningProtocol(): JSX.Element { showConfirmCancelRunModal, setShowConfirmCancelRunModal, ] = React.useState(false) - const [ - interventionModalCommandKey, - setInterventionModalCommandKey, - ] = React.useState(null) const lastAnimatedCommand = React.useRef(null) const { ref, style, swipeType, setSwipeType } = useSwipe() const robotSideAnalysis = useMostRecentCompletedAnalysis(runId) @@ -124,6 +121,16 @@ export function RunningProtocol(): JSX.Element { const robotAnalyticsData = useRobotAnalyticsData(robotName) const robotType = useRobotType(robotName) const { isERActive, failedCommand } = useErrorRecoveryFlows(runId, runStatus) + const { + showModal: showIntervention, + modalProps: interventionProps, + } = useInterventionModal({ + runStatus, + lastRunCommand, + runData: runRecord?.data ?? null, + robotName, + analysis: robotSideAnalysis, + }) React.useEffect(() => { if ( @@ -143,23 +150,6 @@ export function RunningProtocol(): JSX.Element { } }, [currentOption, swipeType, setSwipeType]) - React.useEffect(() => { - if ( - lastRunCommand != null && - interventionModalCommandKey != null && - lastRunCommand.key !== interventionModalCommandKey - ) { - // set intervention modal command key to null if different from current command key - setInterventionModalCommandKey(null) - } else if ( - lastRunCommand?.key != null && - isInterventionCommand(lastRunCommand) && - interventionModalCommandKey === null - ) { - setInterventionModalCommandKey(lastRunCommand.key) - } - }, [lastRunCommand, interventionModalCommandKey]) - return ( <> {isERActive ? ( @@ -202,18 +192,8 @@ export function RunningProtocol(): JSX.Element { isActiveRun={true} /> ) : null} - {interventionModalCommandKey != null && - runRecord?.data != null && - lastRunCommand != null && - isInterventionCommand(lastRunCommand) && - runStatus !== RUN_STATUS_FINISHING ? ( - + {showIntervention ? ( + ) : null}