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

fix(app): fix recovery takeover state cleanup #16694

Merged
merged 2 commits into from
Nov 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ describe('useCleanupRecoveryState', () => {
beforeEach(() => {
mockSetRM = vi.fn()
props = {
isTakeover: false,
isActiveUser: false,
stashedMapRef: {
current: {
route: RECOVERY_MAP.MANUAL_FILL_AND_SKIP.ROUTE,
Expand All @@ -22,7 +22,7 @@ describe('useCleanupRecoveryState', () => {
}
})

it('does not modify state when isTakeover is false', () => {
it('does not modify state when user was never active', () => {
renderHook(() => useCleanupRecoveryState(props))

expect(props.stashedMapRef.current).toEqual({
Expand All @@ -32,10 +32,26 @@ describe('useCleanupRecoveryState', () => {
expect(mockSetRM).not.toHaveBeenCalled()
})

it('resets state when isTakeover is true', () => {
props.isTakeover = true
it('does not modify state when user becomes active', () => {
props.isActiveUser = true

renderHook(() => useCleanupRecoveryState(props))

expect(props.stashedMapRef.current).toEqual({
route: RECOVERY_MAP.MANUAL_FILL_AND_SKIP.ROUTE,
step: RECOVERY_MAP.MANUAL_FILL_AND_SKIP.STEPS.SKIP,
})
expect(mockSetRM).not.toHaveBeenCalled()
})

it('resets state when user becomes inactive after being active', () => {
const { rerender } = renderHook(
({ isActiveUser }) => useCleanupRecoveryState({ ...props, isActiveUser }),
{ initialProps: { isActiveUser: true } }
)

rerender({ isActiveUser: false })

expect(props.stashedMapRef.current).toBeNull()
expect(mockSetRM).toHaveBeenCalledWith({
route: RECOVERY_MAP.OPTION_SELECTION.ROUTE,
Expand All @@ -44,9 +60,13 @@ describe('useCleanupRecoveryState', () => {
})

it('handles case when stashedMapRef.current is already null', () => {
props.isTakeover = true
const { rerender } = renderHook(
({ isActiveUser }) => useCleanupRecoveryState({ ...props, isActiveUser }),
{ initialProps: { isActiveUser: true } }
)

props.stashedMapRef.current = null
renderHook(() => useCleanupRecoveryState(props))
rerender({ isActiveUser: false })

expect(props.stashedMapRef.current).toBeNull()
expect(mockSetRM).toHaveBeenCalledWith({
Expand All @@ -55,24 +75,47 @@ describe('useCleanupRecoveryState', () => {
})
})

it('does not reset state when isTakeover changes from true to false', () => {
it('does not reset state on subsequent inactive states', () => {
const { rerender } = renderHook(
({ isTakeover }) => useCleanupRecoveryState({ ...props, isTakeover }),
{ initialProps: { isTakeover: true } }
({ isActiveUser }) => useCleanupRecoveryState({ ...props, isActiveUser }),
{ initialProps: { isActiveUser: true } }
)

rerender({ isActiveUser: false })
mockSetRM.mockClear()

props.stashedMapRef.current = {
route: RECOVERY_MAP.MANUAL_FILL_AND_SKIP.ROUTE,
step: RECOVERY_MAP.MANUAL_FILL_AND_SKIP.STEPS.SKIP,
}

rerender({ isTakeover: false })
rerender({ isActiveUser: false })

expect(props.stashedMapRef.current).toEqual({
route: RECOVERY_MAP.MANUAL_FILL_AND_SKIP.ROUTE,
step: RECOVERY_MAP.MANUAL_FILL_AND_SKIP.STEPS.SKIP,
})
expect(mockSetRM).not.toHaveBeenCalled()
})

it('resets state only after a full active->inactive cycle', () => {
const { rerender } = renderHook(
({ isActiveUser }) => useCleanupRecoveryState({ ...props, isActiveUser }),
{ initialProps: { isActiveUser: false } }
)

rerender({ isActiveUser: true })
expect(mockSetRM).not.toHaveBeenCalled()
expect(props.stashedMapRef.current).toEqual({
route: RECOVERY_MAP.MANUAL_FILL_AND_SKIP.ROUTE,
step: RECOVERY_MAP.MANUAL_FILL_AND_SKIP.STEPS.SKIP,
})

rerender({ isActiveUser: false })
expect(props.stashedMapRef.current).toBeNull()
expect(mockSetRM).toHaveBeenCalledWith({
route: RECOVERY_MAP.OPTION_SELECTION.ROUTE,
step: RECOVERY_MAP.OPTION_SELECTION.STEPS.SELECT,
})
})
})
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { useEffect } from 'react'
import { useState } from 'react'

import { RECOVERY_MAP } from '../constants'

Expand All @@ -9,25 +9,28 @@ import type {
} from '../hooks'

export interface UseCleanupProps {
isTakeover: ERUtilsProps['showTakeover']
isActiveUser: ERUtilsProps['isActiveUser']
stashedMapRef: UseRouteUpdateActionsResult['stashedMapRef']
setRM: UseRecoveryRoutingResult['setRM']
}

// When certain events (ex, a takeover) occur, reset state that needs to be reset.
// When certain events (ex, someone terminates this app's recovery session) occur, reset state that needs to be reset.
export function useCleanupRecoveryState({
isTakeover,
isActiveUser,
stashedMapRef,
setRM,
}: UseCleanupProps): void {
useEffect(() => {
if (isTakeover) {
stashedMapRef.current = null
const [wasActiveUser, setWasActiveUser] = useState(false)

setRM({
route: RECOVERY_MAP.OPTION_SELECTION.ROUTE,
step: RECOVERY_MAP.OPTION_SELECTION.STEPS.SELECT,
})
}
}, [isTakeover])
if (isActiveUser && !wasActiveUser) {
setWasActiveUser(true)
} else if (!isActiveUser && wasActiveUser) {
setWasActiveUser(false)

stashedMapRef.current = null
setRM({
route: RECOVERY_MAP.OPTION_SELECTION.ROUTE,
step: RECOVERY_MAP.OPTION_SELECTION.STEPS.SELECT,
})
}
}
6 changes: 3 additions & 3 deletions app/src/organisms/ErrorRecoveryFlows/hooks/useERUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ export type ERUtilsProps = Omit<ErrorRecoveryFlowsProps, 'failedCommand'> & {
isOnDevice: boolean
robotType: RobotType
failedCommand: ReturnType<typeof useRetainedFailedCommandBySource>
showTakeover: boolean
isActiveUser: UseRecoveryTakeoverResult['isActiveUser']
allRunDefs: LabwareDefinition2[]
labwareDefinitionsByUri: LabwareDefinitionsByUri | null
}
Expand Down Expand Up @@ -85,7 +85,7 @@ export function useERUtils({
isOnDevice,
robotType,
runStatus,
showTakeover,
isActiveUser,
allRunDefs,
unvalidatedFailedCommand,
labwareDefinitionsByUri,
Expand Down Expand Up @@ -193,7 +193,7 @@ export function useERUtils({
)

useCleanupRecoveryState({
isTakeover: showTakeover,
isActiveUser,
setRM,
stashedMapRef: routeUpdateActions.stashedMapRef,
})
Expand Down
2 changes: 1 addition & 1 deletion app/src/organisms/ErrorRecoveryFlows/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ export function ErrorRecoveryFlows(
toggleERWizAsActiveUser,
isOnDevice,
robotType,
showTakeover,
isActiveUser,
failedCommand: failedCommandBySource,
allRunDefs,
labwareDefinitionsByUri,
Expand Down
Loading