Skip to content

Commit

Permalink
refactor(app): Update ignore error route in Error Recovery (#16511)
Browse files Browse the repository at this point in the history
Partially closes EXEC-776

As the "ignore error and skip/retry" routing no longer applies only to one recovery option, it's worth revisiting the way we structure routing/copy. More importantly, we probably should avoid sending any sort of "ignore all errors" policy updates to the server until the user exits error recovery, because sending policy changes earlier could provide unexpected behavior if the user selects the "ignore all option" but then clicks "go back".

If the PUT request for the policy updates fail, route the user to our "recovery failed" modal and let the user select a different (or same) option.
  • Loading branch information
mjhuff authored Oct 17, 2024
1 parent d7d7315 commit 9938d4f
Show file tree
Hide file tree
Showing 13 changed files with 241 additions and 70 deletions.
4 changes: 3 additions & 1 deletion app/src/assets/localization/en/error_recovery.json
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,13 @@
"homing_pipette_dangerous": "Homing the <bold>{{mount}} pipette</bold> with liquid in the tips may damage it. You must remove all tips before using the pipette again.",
"if_issue_persists_gripper_error": " If the issue persists, cancel the run and rerun gripper calibration",
"if_issue_persists_overpressure": " If the issue persists, cancel the run and make the necessary changes to the protocol",
"if_issue_persists_tip_not_detected": " If the issue persists, cancel the run and initiate Labware Position Check",
"if_issue_persists_tip_not_detected": " If the issue persists, cancel the run and perform Labware Position Check",
"if_tips_are_attached": "If tips are attached, you can choose to blow out any aspirated liquid and drop tips before the run is terminated.",
"ignore_all_errors_of_this_type": "Ignore all errors of this type",
"ignore_error_and_skip": "Ignore error and skip to next step",
"ignore_only_this_error": "Ignore only this error",
"ignore_similar_errors_later_in_run": "Ignore similar errors later in the run?",
"inspect_the_robot": "<block>First, inspect the robot to ensure it's prepared to continue the run from the next step.</block><block>Then, close the robot door before proceeding.</block>",
"labware_released_from_current_height": "The labware will be released from its current height.",
"launch_recovery_mode": "Launch Recovery Mode",
"manually_fill_liquid_in_well": "Manually fill liquid in well {{well}}",
Expand Down Expand Up @@ -70,6 +71,7 @@
"resume": "Resume",
"retry_dropping_tip": "Retry dropping tip",
"retry_now": "Retry now",
"retry_picking_up_tip": "Retry picking up tip",
"retry_step": "Retry step",
"retry_with_new_tips": "Retry with new tips",
"retry_with_same_tips": "Retry with same tips",
Expand Down
4 changes: 2 additions & 2 deletions app/src/organisms/ErrorRecoveryFlows/ErrorRecoveryWizard.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ export function ErrorRecoveryContent(props: RecoveryContentProps): JSX.Element {
return <RecoveryInProgress {...props} />
}

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

Expand Down Expand Up @@ -246,7 +246,7 @@ export function ErrorRecoveryContent(props: RecoveryContentProps): JSX.Element {
case RECOVERY_MAP.ERROR_WHILE_RECOVERING.ROUTE:
return buildRecoveryError()
case RECOVERY_MAP.RETRY_STEP.ROUTE:
return buildResumeRun()
return buildRetryStep()
case RECOVERY_MAP.CANCEL_RUN.ROUTE:
return buildCancelRun()
case RECOVERY_MAP.DROP_TIP_FLOWS.ROUTE:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,6 @@ export function SkipToNextStep(
const { ROBOT_SKIPPING_STEP, IGNORE_AND_SKIP } = RECOVERY_MAP
const { t } = useTranslation('error_recovery')

// TODO(jh, 06-18-24): EXEC-569
const secondaryBtnOnClick = (): void => {
if (selectedRecoveryOption === IGNORE_AND_SKIP.ROUTE) {
void proceedToRouteAndStep(IGNORE_AND_SKIP.ROUTE)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import * as React from 'react'
import { useState, useEffect } from 'react'
import head from 'lodash/head'
import { css } from 'styled-components'
import { useTranslation } from 'react-i18next'
Expand All @@ -17,12 +17,14 @@ import {
RECOVERY_MAP,
ODD_ONLY,
DESKTOP_ONLY,
ERROR_KINDS,
} from '../constants'
import { SelectRecoveryOption } from './SelectRecoveryOption'
import {
RecoveryFooterButtons,
RecoverySingleColumnContentWrapper,
RecoveryRadioGroup,
SkipStepInfo,
} from '../shared'

import type { RecoveryContentProps } from '../types'
Expand All @@ -36,6 +38,8 @@ export function IgnoreErrorSkipStep(props: RecoveryContentProps): JSX.Element {
switch (step) {
case IGNORE_AND_SKIP.STEPS.SELECT_IGNORE_KIND:
return <IgnoreErrorStepHome {...props} />
case IGNORE_AND_SKIP.STEPS.SKIP_STEP:
return <SkipStepInfo {...props} />
default:
console.warn(`${step} in ${route} not explicitly handled. Rerouting.`)
return <SelectRecoveryOption {...props} />
Expand All @@ -48,34 +52,56 @@ export function IgnoreErrorSkipStep(props: RecoveryContentProps): JSX.Element {
export function IgnoreErrorStepHome({
recoveryCommands,
routeUpdateActions,
errorKind,
}: RecoveryContentProps): JSX.Element | null {
const { t } = useTranslation('error_recovery')
const { MANUAL_FILL_AND_SKIP } = RECOVERY_MAP
const { ignoreErrorKindThisRun } = recoveryCommands
const { proceedToRouteAndStep, goBackPrevStep } = routeUpdateActions
const {
proceedNextStep,
proceedToRouteAndStep,
goBackPrevStep,
} = routeUpdateActions

const [selectedOption, setSelectedOption] = React.useState<IgnoreOption>(
const [selectedOption, setSelectedOption] = useState<IgnoreOption>(
head(IGNORE_OPTIONS_IN_ORDER) as IgnoreOption
)

// It's safe to hard code the routing here, since only one route currently
// utilizes ignoring. In the future, we may have to check the selectedRecoveryOption
// and route appropriately.
// Reset client choice to ignore all errors whenever navigating back to this view. This prevents unexpected
// behavior after pressing "go back" and ending up on this screen.
useEffect(() => {
void ignoreErrorKindThisRun(false)
}, [])

// In order to keep routing linear, all extended "skip" flows should be kept as separate recovery options with
// go back functionality that routes to this view. Those "skip" views encapsulate the generic "skip" view.
// See the "manually fill well and skip" recovery option for an example.
const ignoreOnce = (): void => {
void proceedToRouteAndStep(
MANUAL_FILL_AND_SKIP.ROUTE,
MANUAL_FILL_AND_SKIP.STEPS.SKIP
)
switch (errorKind) {
case ERROR_KINDS.NO_LIQUID_DETECTED:
void proceedToRouteAndStep(
RECOVERY_MAP.MANUAL_FILL_AND_SKIP.ROUTE,
RECOVERY_MAP.MANUAL_FILL_AND_SKIP.STEPS.SKIP
)
break
default:
void proceedNextStep()
}
}

// See ignoreOnce comment.
const ignoreAlways = (): void => {
void ignoreErrorKindThisRun().then(() =>
proceedToRouteAndStep(
MANUAL_FILL_AND_SKIP.ROUTE,
MANUAL_FILL_AND_SKIP.STEPS.SKIP
)
)
void ignoreErrorKindThisRun(true).then(() => {
switch (errorKind) {
case ERROR_KINDS.NO_LIQUID_DETECTED:
void proceedToRouteAndStep(
RECOVERY_MAP.MANUAL_FILL_AND_SKIP.ROUTE,
RECOVERY_MAP.MANUAL_FILL_AND_SKIP.STEPS.SKIP
)
break
default:
void proceedNextStep()
}
})
}

const primaryOnClick = (): void => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,19 +10,23 @@ import {
IgnoreErrorStepHome,
IgnoreOptions,
} from '../IgnoreErrorSkipStep'
import { RECOVERY_MAP } from '../../constants'
import { ERROR_KINDS, RECOVERY_MAP } from '../../constants'
import { SelectRecoveryOption } from '../SelectRecoveryOption'
import { clickButtonLabeled } from '../../__tests__/util'
import { SkipStepInfo } from '/app/organisms/ErrorRecoveryFlows/shared'

import type { Mock } from 'vitest'

vi.mock('../shared', async () => {
const actual = await vi.importActual('../shared')
vi.mock('/app/organisms/ErrorRecoveryFlows/shared', async () => {
const actual = await vi.importActual(
'/app/organisms/ErrorRecoveryFlows/shared'
)
return {
...actual,
RecoverySingleColumnContentWrapper: vi.fn(({ children }) => (
<div>{children}</div>
)),
SkipStepInfo: vi.fn(),
}
})
vi.mock('../SelectRecoveryOption')
Expand All @@ -47,11 +51,13 @@ describe('IgnoreErrorSkipStep', () => {
beforeEach(() => {
props = {
...mockRecoveryContentProps,
recoveryCommands: { ignoreErrorKindThisRun: vi.fn() } as any,
}

vi.mocked(SelectRecoveryOption).mockReturnValue(
<div>MOCK_SELECT_RECOVERY_OPTION</div>
)
vi.mocked(SkipStepInfo).mockReturnValue(<div>MOCK_SKIP_STEP_INFO</div>)
})

it(`renders IgnoreErrorStepHome when step is ${RECOVERY_MAP.IGNORE_AND_SKIP.STEPS.SELECT_IGNORE_KIND}`, () => {
Expand All @@ -66,6 +72,18 @@ describe('IgnoreErrorSkipStep', () => {
screen.getByText('Ignore similar errors later in the run?')
})

it(`renders SkipStepInfo when step is ${RECOVERY_MAP.IGNORE_AND_SKIP.STEPS.SKIP_STEP}`, () => {
props = {
...props,
recoveryMap: {
...props.recoveryMap,
step: RECOVERY_MAP.IGNORE_AND_SKIP.STEPS.SKIP_STEP,
},
}
render(props)
screen.getByText('MOCK_SKIP_STEP_INFO')
})

it('renders SelectRecoveryOption as a fallback', () => {
props = {
...props,
Expand All @@ -84,26 +102,30 @@ describe('IgnoreErrorStepHome', () => {
let mockIgnoreErrorKindThisRun: Mock
let mockProceedToRouteAndStep: Mock
let mockGoBackPrevStep: Mock
let mockProceedNextStep: Mock

beforeEach(() => {
mockIgnoreErrorKindThisRun = vi.fn(() => Promise.resolve())
mockProceedToRouteAndStep = vi.fn()
mockGoBackPrevStep = vi.fn()
mockProceedNextStep = vi.fn()

props = {
...mockRecoveryContentProps,
isOnDevice: true,
errorKind: ERROR_KINDS.NO_LIQUID_DETECTED,
recoveryCommands: {
ignoreErrorKindThisRun: mockIgnoreErrorKindThisRun,
} as any,
routeUpdateActions: {
proceedToRouteAndStep: mockProceedToRouteAndStep,
goBackPrevStep: mockGoBackPrevStep,
proceedNextStep: mockProceedNextStep,
} as any,
}
})

it('calls ignoreOnce when "ignore_only_this_error" is selected and primary button is clicked', async () => {
it(`ignoreOnce correctly routes "ignore_only_this_error" is clicked and the errorKind is ${ERROR_KINDS.NO_LIQUID_DETECTED}`, async () => {
renderIgnoreErrorStepHome(props)
fireEvent.click(screen.queryAllByText('Ignore only this error')[0])
clickButtonLabeled('Continue')
Expand All @@ -115,7 +137,19 @@ describe('IgnoreErrorStepHome', () => {
})
})

it('calls ignoreAlways when "ignore_all_errors_of_this_type" is selected and primary button is clicked', async () => {
it(`ignoreOnce correctly routes "ignore_only_this_error" is clicked and the errorKind not explicitly handled`, async () => {
renderIgnoreErrorStepHome({
...props,
errorKind: ERROR_KINDS.GENERAL_ERROR,
})
fireEvent.click(screen.queryAllByText('Ignore only this error')[0])
clickButtonLabeled('Continue')
await waitFor(() => {
expect(mockProceedNextStep).toHaveBeenCalled()
})
})

it(`ignoreAlways correctly routes when "ignore_all_errors_of_this_type" is clicked and the errorKind is ${ERROR_KINDS.NO_LIQUID_DETECTED}`, async () => {
renderIgnoreErrorStepHome(props)
fireEvent.click(screen.queryAllByText('Ignore all errors of this type')[0])
clickButtonLabeled('Continue')
Expand All @@ -130,6 +164,18 @@ describe('IgnoreErrorStepHome', () => {
})
})

it(`ignoreAlways correctly routes "ignore_all_errors_of_this_type" is clicked and the errorKind not explicitly handled`, async () => {
renderIgnoreErrorStepHome({
...props,
errorKind: ERROR_KINDS.GENERAL_ERROR,
})
fireEvent.click(screen.queryAllByText('Ignore all errors of this type')[0])
clickButtonLabeled('Continue')
await waitFor(() => {
expect(mockProceedNextStep).toHaveBeenCalled()
})
})

it('calls goBackPrevStep when secondary button is clicked', () => {
renderIgnoreErrorStepHome(props)
clickButtonLabeled('Go back')
Expand Down
8 changes: 6 additions & 2 deletions app/src/organisms/ErrorRecoveryFlows/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ export const RECOVERY_MAP = {
},
IGNORE_AND_SKIP: {
ROUTE: 'ignore-and-skip-step',
STEPS: { SELECT_IGNORE_KIND: 'select-ignore' },
STEPS: { SELECT_IGNORE_KIND: 'select-ignore', SKIP_STEP: 'skip-step' },
},
MANUAL_FILL_AND_SKIP: {
ROUTE: 'manual-fill-well-and-skip',
Expand Down Expand Up @@ -248,7 +248,10 @@ export const STEP_ORDER: StepOrder = {
DROP_TIP_FLOWS.STEPS.CHOOSE_TIP_DROP,
],
[REFILL_AND_RESUME.ROUTE]: [],
[IGNORE_AND_SKIP.ROUTE]: [IGNORE_AND_SKIP.STEPS.SELECT_IGNORE_KIND],
[IGNORE_AND_SKIP.ROUTE]: [
IGNORE_AND_SKIP.STEPS.SELECT_IGNORE_KIND,
IGNORE_AND_SKIP.STEPS.SKIP_STEP,
],
[CANCEL_RUN.ROUTE]: [CANCEL_RUN.STEPS.CONFIRM_CANCEL],
[MANUAL_FILL_AND_SKIP.ROUTE]: [
MANUAL_FILL_AND_SKIP.STEPS.MANUAL_FILL,
Expand Down Expand Up @@ -343,6 +346,7 @@ export const RECOVERY_MAP_METADATA: RecoveryRouteStepMetadata = {
[IGNORE_AND_SKIP.STEPS.SELECT_IGNORE_KIND]: {
allowDoorOpen: false,
},
[IGNORE_AND_SKIP.STEPS.SKIP_STEP]: { allowDoorOpen: false },
},
[MANUAL_FILL_AND_SKIP.ROUTE]: {
[MANUAL_FILL_AND_SKIP.STEPS.MANUAL_FILL]: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ describe('useRecoveryCommands', () => {
const mockChainRunCommands = vi.fn().mockResolvedValue([])
const mockReportActionSelectedResult = vi.fn()
const mockReportRecoveredRunResult = vi.fn()
const mockUpdateErrorRecoveryPolicy = vi.fn()
const mockUpdateErrorRecoveryPolicy = vi.fn(() => Promise.resolve())

const props = {
runId: mockRunId,
Expand All @@ -70,7 +70,7 @@ describe('useRecoveryCommands', () => {
chainRunCommands: mockChainRunCommands,
} as any)
vi.mocked(useUpdateErrorRecoveryPolicy).mockReturnValue({
updateErrorRecoveryPolicy: mockUpdateErrorRecoveryPolicy,
mutateAsync: mockUpdateErrorRecoveryPolicy,
} as any)
})

Expand Down Expand Up @@ -302,12 +302,18 @@ describe('useRecoveryCommands', () => {
failedCommandByRunRecord: mockFailedCommandWithError,
}

const { result } = renderHook(() => useRecoveryCommands(testProps))
const { result, rerender } = renderHook(() =>
useRecoveryCommands(testProps)
)

await act(async () => {
await result.current.ignoreErrorKindThisRun()
await result.current.ignoreErrorKindThisRun(true)
})

rerender()

result.current.skipFailedCommand()

const expectedPolicyRules = buildIgnorePolicyRules(
'aspirateInPlace',
'mockErrorType'
Expand All @@ -318,16 +324,33 @@ describe('useRecoveryCommands', () => {
)
})

it('should reject with an error when failedCommand or error is null', async () => {
it('should call proceedToRouteAndStep with ERROR_WHILE_RECOVERING route when updateErrorRecoveryPolicy rejects', async () => {
const mockFailedCommandWithError = {
...mockFailedCommand,
commandType: 'aspirateInPlace',
error: {
errorType: 'mockErrorType',
},
}

const testProps = {
...props,
failedCommand: null,
failedCommandByRunRecord: mockFailedCommandWithError,
}

mockUpdateErrorRecoveryPolicy.mockRejectedValueOnce(
new Error('Update policy failed')
)

const { result } = renderHook(() => useRecoveryCommands(testProps))

await expect(result.current.ignoreErrorKindThisRun()).rejects.toThrow(
'Could not execute command. No failed command.'
await act(async () => {
await result.current.ignoreErrorKindThisRun(true)
})

expect(mockUpdateErrorRecoveryPolicy).toHaveBeenCalled()
expect(mockProceedToRouteAndStep).toHaveBeenCalledWith(
RECOVERY_MAP.ERROR_WHILE_RECOVERING.ROUTE
)
})
})
Loading

0 comments on commit 9938d4f

Please sign in to comment.