Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(app, react-api-client): add "retry failed command" during Error Recovery #15170

Merged
merged 7 commits into from
May 21, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions app/src/assets/localization/en/error_recovery.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,5 +15,6 @@
"run_will_resume": "The run will resume from the point at which the error occurred. Take any necessary actions to correct the problem first. If the step is completed successfully, the protocol continues.",
"stand_back": "Stand back, robot is in motion",
"stand_back_resuming": "Stand back, resuming current step",
"stand_back_retrying": "Stand back, retrying current command",
"view_recovery_options": "View recovery options"
}
Original file line number Diff line number Diff line change
Expand Up @@ -278,10 +278,12 @@ describe('ProtocolRunHeader', () => {
pause: () => {},
stop: () => {},
reset: () => {},
resumeFromRecovery: () => {},
isPlayRunActionLoading: false,
isPauseRunActionLoading: false,
isStopRunActionLoading: false,
isResetRunLoading: false,
isResumeRunFromRecoveryActionLoading: false,
})
when(vi.mocked(useRunStatus)).calledWith(RUN_ID).thenReturn(RUN_STATUS_IDLE)
when(vi.mocked(useRunTimestamps)).calledWith(RUN_ID).thenReturn({
Expand Down Expand Up @@ -777,10 +779,12 @@ describe('ProtocolRunHeader', () => {
pause: () => {},
stop: () => {},
reset: () => {},
resumeFromRecovery: () => {},
isPlayRunActionLoading: false,
isPauseRunActionLoading: false,
isStopRunActionLoading: false,
isResetRunLoading: true,
isResumeRunFromRecoveryActionLoading: false,
})
render()

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,10 +82,12 @@ describe('HistoricalProtocolRunOverflowMenu', () => {
pause: () => {},
stop: () => {},
reset: () => {},
resumeFromRecovery: () => {},
isPlayRunActionLoading: false,
isPauseRunActionLoading: false,
isStopRunActionLoading: false,
isResetRunLoading: false,
isResumeRunFromRecoveryActionLoading: false,
})
when(useAllCommandsQuery)
.calledWith(
Expand Down
133 changes: 133 additions & 0 deletions app/src/organisms/ErrorRecoveryFlows/ErrorRecoveryWizard.tsx
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Diff this against the old index.tsx to see changes.

The differences are the new route for the "retrying command" view, the useInitialPipetteHome, and the new name, ErrorRecoveryWizard.

Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
import * as React from 'react'
import { createPortal } from 'react-dom'
import { useSelector } from 'react-redux'

import {
BORDERS,
COLORS,
DIRECTION_COLUMN,
Flex,
POSITION_ABSOLUTE,
} from '@opentrons/components'

import { getIsOnDevice } from '../../redux/config'
import { getTopPortalEl } from '../../App/portal'
import { BeforeBeginning } from './BeforeBeginning'
import { SelectRecoveryOption, ResumeRun } from './RecoveryOptions'
import { ErrorRecoveryHeader } from './ErrorRecoveryHeader'
import { RecoveryInProgress } from './RecoveryInProgress'
import { getErrorKind, useRouteUpdateActions } from './utils'
import { useRecoveryCommands } from './useRecoveryCommands'
import { RECOVERY_MAP } from './constants'

import type { FailedCommand, IRecoveryMap, RecoveryContentProps } from './types'

export interface ErrorRecoveryFlowsProps {
runId: string
failedCommand: FailedCommand | null
}

export function ErrorRecoveryWizard({
runId,
failedCommand,
}: ErrorRecoveryFlowsProps): JSX.Element {
/**
* Recovery Route: A logically-related collection of recovery steps or a single step if unrelated to any existing recovery route.
* Recovery Step: Analogous to a "step" in other wizard flows.
*/
const [recoveryMap, setRecoveryMap] = React.useState<IRecoveryMap>({
route: RECOVERY_MAP.ROBOT_IN_MOTION.ROUTE, // Initialize the route to "in motion", since wizard always start with a home command.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we expect the robot to already be in motion when we start this wizard?
Or does the robot not start homing until the useInitialPipetteHome call like 15 lines down?

If it is already in motion, then this makes sense, if it is not, should useInitialPipetteHome be the thing that adds the ROBOT_IN_MOTION step to the route?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good Q - doing it this way prevents us initially rendering some other screen for one render cycle before the in-motion screen. This problem is really only noticeable if you test it on the physical ODD, and it looks bad enough IMO that I'd rather eagerly set the robot to 'in motion' immediately and not have the screen immediately changing as soon as we launch ER.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DerekMaggio After further investigation, I found a better React way of doing this while initializing the route to something more sensible, so thank you for calling this out!

step: RECOVERY_MAP.ROBOT_IN_MOTION.STEPS.IN_MOTION,
})

const errorKind = getErrorKind(failedCommand?.error?.errorType)
const isOnDevice = useSelector(getIsOnDevice)
const routeUpdateActions = useRouteUpdateActions({
recoveryMap,
setRecoveryMap,
})
const recoveryCommands = useRecoveryCommands({
runId,
failedCommand,
})

useInitialPipetteHome(recoveryCommands, routeUpdateActions)

return (
<ErrorRecoveryComponent
failedCommand={failedCommand}
errorKind={errorKind}
isOnDevice={isOnDevice}
recoveryMap={recoveryMap}
routeUpdateActions={routeUpdateActions}
recoveryCommands={recoveryCommands}
/>
)
}

function ErrorRecoveryComponent(props: RecoveryContentProps): JSX.Element {
return createPortal(
<Flex
flexDirection={DIRECTION_COLUMN}
width="992px"
height="568px"
left="14.5px"
top="16px"
borderRadius={BORDERS.borderRadius12}
position={POSITION_ABSOLUTE}
backgroundColor={COLORS.white}
>
<ErrorRecoveryHeader errorKind={props.errorKind} />
<ErrorRecoveryContent {...props} />
</Flex>,
getTopPortalEl()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably wants to be getModalPortalEl if this is on the desktop

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yep, great point. Given hi-fi designs, it looks like we may have some modal-within-modals as well, so we'd want the lower zIndex for ODD as well.

)
}

export function ErrorRecoveryContent(props: RecoveryContentProps): JSX.Element {
const buildBeforeBeginning = (): JSX.Element => {
return <BeforeBeginning {...props} />
}

const buildSelectRecoveryOption = (): JSX.Element => {
return <SelectRecoveryOption {...props} />
}

const buildRecoveryInProgress = (): JSX.Element => {
return <RecoveryInProgress {...props} />
}

const buildResumeRun = (): JSX.Element => {
return <ResumeRun {...props} />
}

switch (props.recoveryMap.route) {
case RECOVERY_MAP.BEFORE_BEGINNING.ROUTE:
return buildBeforeBeginning()
case RECOVERY_MAP.OPTION_SELECTION.ROUTE:
return buildSelectRecoveryOption()
case RECOVERY_MAP.RESUME.ROUTE:
return buildResumeRun()
case RECOVERY_MAP.ROBOT_IN_MOTION.ROUTE:
case RECOVERY_MAP.ROBOT_RESUMING.ROUTE:
case RECOVERY_MAP.ROBOT_RETRYING_COMMAND.ROUTE:
return buildRecoveryInProgress()
default:
return buildSelectRecoveryOption()
}
}

// Home the Z-axis of all attached pipettes on Error Recovery launch.
export function useInitialPipetteHome(
recoveryCommands: ReturnType<typeof useRecoveryCommands>,
routeUpdateActions: ReturnType<typeof useRouteUpdateActions>
): void {
const { homePipetteZAxes } = recoveryCommands
const { setRobotInMotion } = routeUpdateActions

React.useEffect(() => {
void setRobotInMotion(true)
.then(() => homePipetteZAxes())
.then(() => setRobotInMotion(false))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably wants to be .finally()

}, [])
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,11 @@ import type { RobotMovingRoute, RecoveryContentProps } from './types'
export function RecoveryInProgress({
recoveryMap,
}: RecoveryContentProps): JSX.Element {
const { ROBOT_IN_MOTION, ROBOT_RESUMING } = RECOVERY_MAP
const {
ROBOT_IN_MOTION,
ROBOT_RESUMING,
ROBOT_RETRYING_COMMAND,
} = RECOVERY_MAP
const { t } = useTranslation('error_recovery')
const { route } = recoveryMap

Expand All @@ -19,6 +23,8 @@ export function RecoveryInProgress({
return t('stand_back')
case ROBOT_RESUMING.ROUTE:
return t('stand_back_resuming')
case ROBOT_RETRYING_COMMAND.ROUTE:
return t('stand_back_retrying')
default:
return t('stand_back')
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,17 +11,27 @@ import {
StyledText,
} from '@opentrons/components'

import { RECOVERY_MAP } from '../constants'
import { RecoveryFooterButtons } from './shared'

import type { RecoveryContentProps } from '../types'

export function ResumeRun({
isOnDevice,
onComplete,
routeUpdateActions,
recoveryCommands,
}: RecoveryContentProps): JSX.Element | null {
const { ROBOT_RETRYING_COMMAND } = RECOVERY_MAP
const { t } = useTranslation('error_recovery')
const { goBackPrevStep } = routeUpdateActions

const { retryFailedCommand, resumeRun } = recoveryCommands
const { goBackPrevStep, setRobotInMotion } = routeUpdateActions

const primaryBtnOnClick = (): Promise<void> => {
return setRobotInMotion(true, ROBOT_RETRYING_COMMAND.ROUTE) // Show the "retrying" motion screen while exiting ER.
.then(() => retryFailedCommand())
.then(() => resumeRun())
}

if (isOnDevice) {
return (
Expand Down Expand Up @@ -50,7 +60,7 @@ export function ResumeRun({
</Flex>
<RecoveryFooterButtons
isOnDevice={isOnDevice}
primaryBtnOnClick={onComplete}
primaryBtnOnClick={primaryBtnOnClick}
secondaryBtnOnClick={goBackPrevStep}
primaryBtnTextOverride={t('confirm')}
/>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import * as React from 'react'
import { vi, describe, it, expect, beforeEach } from 'vitest'
import { screen, fireEvent } from '@testing-library/react'
import { screen, fireEvent, waitFor } from '@testing-library/react'

import { renderWithProviders } from '../../../../__testing-utils__'
import { i18n } from '../../../../i18n'
Expand All @@ -16,20 +16,19 @@ const render = (props: React.ComponentProps<typeof ResumeRun>) => {
}

describe('RecoveryFooterButtons', () => {
const { RESUME } = RECOVERY_MAP
const { RESUME, ROBOT_RETRYING_COMMAND } = RECOVERY_MAP
let props: React.ComponentProps<typeof ResumeRun>
let mockOnComplete: Mock
let mockGoBackPrevStep: Mock

beforeEach(() => {
mockOnComplete = vi.fn()
mockGoBackPrevStep = vi.fn()
const mockRouteUpdateActions = { goBackPrevStep: mockGoBackPrevStep } as any

props = {
isOnDevice: true,
recoveryCommands: {} as any,
failedCommand: {} as any,
errorKind: ERROR_KINDS.GENERAL_ERROR,
onComplete: mockOnComplete,
routeUpdateActions: mockRouteUpdateActions,
recoveryMap: {
route: RESUME.ROUTE,
Expand All @@ -38,21 +37,68 @@ describe('RecoveryFooterButtons', () => {
}
})

it('renders appropriate copy and click behavior', () => {
it('renders appropriate copy and click behavior', async () => {
render(props)

screen.getByText('Are you sure you want to resume?')
screen.queryByText(
'The run will resume from the point at which the error occurred.'
)

const primaryBtn = screen.getByRole('button', { name: 'Confirm' })
const secondaryBtn = screen.getByRole('button', { name: 'Go back' })

fireEvent.click(primaryBtn)
fireEvent.click(secondaryBtn)

expect(mockOnComplete).toHaveBeenCalled()
expect(mockGoBackPrevStep).toHaveBeenCalled()
})

it('should call commands in the correct order for the primaryOnClick callback', async () => {
const setRobotInMotionMock = vi.fn(() => Promise.resolve())
const retryFailedCommandMock = vi.fn(() => Promise.resolve())
const resumeRunMock = vi.fn()

const mockRecoveryCommands = {
retryFailedCommand: retryFailedCommandMock,
resumeRun: resumeRunMock,
} as any

const mockRouteUpdateActions = {
setRobotInMotion: setRobotInMotionMock,
} as any

render({
...props,
recoveryCommands: mockRecoveryCommands,
routeUpdateActions: mockRouteUpdateActions,
})

const primaryBtn = screen.getByRole('button', { name: 'Confirm' })
fireEvent.click(primaryBtn)

await waitFor(() => {
expect(setRobotInMotionMock).toHaveBeenCalledTimes(1)
})
await waitFor(() => {
expect(setRobotInMotionMock).toHaveBeenCalledWith(
true,
ROBOT_RETRYING_COMMAND.ROUTE
)
})
await waitFor(() => {
expect(retryFailedCommandMock).toHaveBeenCalledTimes(1)
})
await waitFor(() => {
expect(resumeRunMock).toHaveBeenCalledTimes(1)
})

expect(setRobotInMotionMock.mock.invocationCallOrder[0]).toBeLessThan(
retryFailedCommandMock.mock.invocationCallOrder[0]
)

await waitFor(() => {
expect(retryFailedCommandMock.mock.invocationCallOrder[0]).toBeLessThan(
resumeRunMock.mock.invocationCallOrder[0]
)
})
})
})
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@ describe('SelectRecoveryOption', () => {
props = {
isOnDevice: true,
errorKind: ERROR_KINDS.GENERAL_ERROR,
onComplete: vi.fn(),
failedCommand: {} as any,
recoveryCommands: {} as any,
routeUpdateActions: mockRouteUpdateActions,
recoveryMap: {
route: RESUME.ROUTE,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,9 @@ describe('BeforeBeginning', () => {

props = {
isOnDevice: true,
recoveryCommands: {} as any,
failedCommand: {} as any,
errorKind: ERROR_KINDS.GENERAL_ERROR,
onComplete: vi.fn(),
routeUpdateActions: mockRouteUpdateActions,
recoveryMap: {
route: BEFORE_BEGINNING.ROUTE,
Expand Down
Loading
Loading