From c0f95deec1266b215c73e1bddbbc5470d68d86f9 Mon Sep 17 00:00:00 2001 From: Jamey Huffnagle Date: Mon, 18 Nov 2024 20:09:00 -0500 Subject: [PATCH] fix(app): Fix `failedCommand` caching issues (#16874) Closes RABR-671, RQA-3213, and RQA-3595 --- .../useCurrentlyRecoveringFrom.test.ts | 94 ++++++++++++++++++- .../hooks/useCurrentlyRecoveringFrom.ts | 40 +++++++- .../ErrorRecoveryFlows/hooks/useERUtils.ts | 13 ++- .../hooks/useRetainedFailedCommandBySource.ts | 4 +- .../organisms/ErrorRecoveryFlows/index.tsx | 77 ++++++++------- .../protocols/hooks/useRunningStepCounts.ts | 19 +--- app/src/resources/protocols/utils.ts | 26 +++++ 7 files changed, 211 insertions(+), 62 deletions(-) diff --git a/app/src/organisms/ErrorRecoveryFlows/hooks/__tests__/useCurrentlyRecoveringFrom.test.ts b/app/src/organisms/ErrorRecoveryFlows/hooks/__tests__/useCurrentlyRecoveringFrom.test.ts index 4b177972851..dc3bb2dd2be 100644 --- a/app/src/organisms/ErrorRecoveryFlows/hooks/__tests__/useCurrentlyRecoveringFrom.test.ts +++ b/app/src/organisms/ErrorRecoveryFlows/hooks/__tests__/useCurrentlyRecoveringFrom.test.ts @@ -42,9 +42,11 @@ describe('useCurrentlyRecoveringFrom', () => { }, }, }, + isFetching: false, } as any) vi.mocked(useCommandQuery).mockReturnValue({ data: { data: 'mockCommandDetails' }, + isFetching: false, } as any) const { result } = renderHook(() => @@ -69,8 +71,11 @@ describe('useCurrentlyRecoveringFrom', () => { data: { links: {}, }, + isFetching: false, + } as any) + vi.mocked(useCommandQuery).mockReturnValue({ + isFetching: false, } as any) - vi.mocked(useCommandQuery).mockReturnValue({} as any) const { result } = renderHook(() => useCurrentlyRecoveringFrom(MOCK_RUN_ID, RUN_STATUS_AWAITING_RECOVERY) @@ -94,9 +99,11 @@ describe('useCurrentlyRecoveringFrom', () => { }, }, }, + isFetching: false, } as any) vi.mocked(useCommandQuery).mockReturnValue({ data: { data: 'mockCommandDetails' }, + isFetching: false, } as any) const { result } = renderHook(() => @@ -111,6 +118,91 @@ describe('useCurrentlyRecoveringFrom', () => { expect(result.current).toStrictEqual('mockCommandDetails') }) + it('returns null if all commands query is still fetching', () => { + vi.mocked(useNotifyAllCommandsQuery).mockReturnValue({ + data: { + links: { + currentlyRecoveringFrom: { + meta: { + runId: MOCK_RUN_ID, + commandId: MOCK_COMMAND_ID, + }, + }, + }, + }, + isFetching: true, + } as any) + vi.mocked(useCommandQuery).mockReturnValue({ + data: { data: 'mockCommandDetails' }, + isFetching: false, + } as any) + + const { result } = renderHook(() => + useCurrentlyRecoveringFrom(MOCK_RUN_ID, RUN_STATUS_AWAITING_RECOVERY) + ) + + expect(result.current).toStrictEqual(null) + }) + + it('returns null if command query is still fetching', () => { + vi.mocked(useNotifyAllCommandsQuery).mockReturnValue({ + data: { + links: { + currentlyRecoveringFrom: { + meta: { + runId: MOCK_RUN_ID, + commandId: MOCK_COMMAND_ID, + }, + }, + }, + }, + isFetching: false, + } as any) + vi.mocked(useCommandQuery).mockReturnValue({ + data: { data: 'mockCommandDetails' }, + isFetching: true, + } as any) + + const { result } = renderHook(() => + useCurrentlyRecoveringFrom(MOCK_RUN_ID, RUN_STATUS_AWAITING_RECOVERY) + ) + + expect(result.current).toStrictEqual(null) + }) + + it('resets isReadyToShow when run exits recovery mode', () => { + const { rerender, result } = renderHook( + ({ status }) => useCurrentlyRecoveringFrom(MOCK_RUN_ID, status), + { initialProps: { status: RUN_STATUS_AWAITING_RECOVERY } } + ) + + vi.mocked(useNotifyAllCommandsQuery).mockReturnValue({ + data: { + links: { + currentlyRecoveringFrom: { + meta: { + runId: MOCK_RUN_ID, + commandId: MOCK_COMMAND_ID, + }, + }, + }, + }, + isFetching: false, + } as any) + vi.mocked(useCommandQuery).mockReturnValue({ + data: { data: 'mockCommandDetails' }, + isFetching: false, + } as any) + + rerender({ status: RUN_STATUS_AWAITING_RECOVERY }) + + expect(result.current).toStrictEqual('mockCommandDetails') + + rerender({ status: RUN_STATUS_IDLE } as any) + + expect(result.current).toStrictEqual(null) + }) + it('calls invalidateQueries when the run enters recovery mode', () => { renderHook(() => useCurrentlyRecoveringFrom(MOCK_RUN_ID, RUN_STATUS_AWAITING_RECOVERY) diff --git a/app/src/organisms/ErrorRecoveryFlows/hooks/useCurrentlyRecoveringFrom.ts b/app/src/organisms/ErrorRecoveryFlows/hooks/useCurrentlyRecoveringFrom.ts index d0273643b61..9ea9d468697 100644 --- a/app/src/organisms/ErrorRecoveryFlows/hooks/useCurrentlyRecoveringFrom.ts +++ b/app/src/organisms/ErrorRecoveryFlows/hooks/useCurrentlyRecoveringFrom.ts @@ -1,4 +1,4 @@ -import { useEffect } from 'react' +import { useEffect, useState } from 'react' import { useQueryClient } from 'react-query' import { @@ -23,13 +23,15 @@ const VALID_RECOVERY_FETCH_STATUSES = [ ] as Array // Return the `currentlyRecoveringFrom` command returned by the server, if any. -// Otherwise, returns null. +// The command will only be returned after the initial fetches are complete to prevent rendering of stale data. export function useCurrentlyRecoveringFrom( runId: string, runStatus: RunStatus | null ): FailedCommand | null { const queryClient = useQueryClient() const host = useHost() + const [isReadyToShow, setIsReadyToShow] = useState(false) + // There can only be a currentlyRecoveringFrom command when the run is in recovery mode. // In case we're falling back to polling, only enable queries when that is the case. const isRunInRecoveryMode = VALID_RECOVERY_FETCH_STATUSES.includes(runStatus) @@ -38,10 +40,15 @@ export function useCurrentlyRecoveringFrom( useEffect(() => { if (isRunInRecoveryMode) { void queryClient.invalidateQueries([host, 'runs', runId]) + } else { + setIsReadyToShow(false) } }, [isRunInRecoveryMode, host, runId]) - const { data: allCommandsQueryData } = useNotifyAllCommandsQuery( + const { + data: allCommandsQueryData, + isFetching: isAllCommandsFetching, + } = useNotifyAllCommandsQuery( runId, { cursor: null, pageLength: 0 }, // pageLength 0 because we only care about the links. { @@ -54,7 +61,10 @@ export function useCurrentlyRecoveringFrom( // TODO(mm, 2024-05-21): When the server supports fetching the // currentlyRecoveringFrom command in one step, do that instead of this chained query. - const { data: commandQueryData } = useCommandQuery( + const { + data: commandQueryData, + isFetching: isCommandFetching, + } = useCommandQuery( currentlyRecoveringFromLink?.meta.runId ?? null, currentlyRecoveringFromLink?.meta.commandId ?? null, { @@ -62,5 +72,25 @@ export function useCurrentlyRecoveringFrom( } ) - return isRunInRecoveryMode ? commandQueryData?.data ?? null : null + // Only mark as ready to show when waterfall fetches are complete + useEffect(() => { + if ( + isRunInRecoveryMode && + !isAllCommandsFetching && + !isCommandFetching && + !isReadyToShow + ) { + setIsReadyToShow(true) + } + }, [ + isRunInRecoveryMode, + isAllCommandsFetching, + isCommandFetching, + isReadyToShow, + ]) + + const shouldShowCommand = + isRunInRecoveryMode && isReadyToShow && commandQueryData?.data + + return shouldShowCommand ? commandQueryData.data : null } diff --git a/app/src/organisms/ErrorRecoveryFlows/hooks/useERUtils.ts b/app/src/organisms/ErrorRecoveryFlows/hooks/useERUtils.ts index ecf03e4f56b..72e9bb481bc 100644 --- a/app/src/organisms/ErrorRecoveryFlows/hooks/useERUtils.ts +++ b/app/src/organisms/ErrorRecoveryFlows/hooks/useERUtils.ts @@ -1,3 +1,5 @@ +import { useMemo } from 'react' + import { useInstrumentsQuery } from '@opentrons/react-api-client' import { useRouteUpdateActions } from './useRouteUpdateActions' @@ -13,12 +15,12 @@ import { } from '/app/resources/runs' import { useRecoveryOptionCopy } from './useRecoveryOptionCopy' import { useRecoveryActionMutation } from './useRecoveryActionMutation' -import { useRunningStepCounts } from '/app/resources/protocols/hooks' import { useRecoveryToasts } from './useRecoveryToasts' import { useRecoveryAnalytics } from '/app/redux-resources/analytics' import { useShowDoorInfo } from './useShowDoorInfo' import { useCleanupRecoveryState } from './useCleanupRecoveryState' import { useFailedPipetteUtils } from './useFailedPipetteUtils' +import { getRunningStepCountsFrom } from '/app/resources/protocols' import type { LabwareDefinition2, @@ -102,7 +104,14 @@ export function useERUtils({ pageLength: 999, }) - const stepCounts = useRunningStepCounts(runId, runCommands) + const stepCounts = useMemo( + () => + getRunningStepCountsFrom( + protocolAnalysis?.commands ?? [], + failedCommand?.byRunRecord ?? null + ), + [protocolAnalysis != null, failedCommand] + ) const analytics = useRecoveryAnalytics() diff --git a/app/src/organisms/ErrorRecoveryFlows/hooks/useRetainedFailedCommandBySource.ts b/app/src/organisms/ErrorRecoveryFlows/hooks/useRetainedFailedCommandBySource.ts index 90afa5851da..5a226fddcf1 100644 --- a/app/src/organisms/ErrorRecoveryFlows/hooks/useRetainedFailedCommandBySource.ts +++ b/app/src/organisms/ErrorRecoveryFlows/hooks/useRetainedFailedCommandBySource.ts @@ -1,4 +1,4 @@ -import { useState, useEffect } from 'react' +import { useState, useLayoutEffect } from 'react' import type { RunTimeCommand } from '@opentrons/shared-data' import type { ErrorRecoveryFlowsProps } from '..' @@ -27,7 +27,7 @@ export function useRetainedFailedCommandBySource( setRetainedFailedCommand, ] = useState(null) - useEffect(() => { + useLayoutEffect(() => { if (failedCommandByRunRecord !== null) { const failedCommandByAnalysis = protocolAnalysis?.commands.find( diff --git a/app/src/organisms/ErrorRecoveryFlows/index.tsx b/app/src/organisms/ErrorRecoveryFlows/index.tsx index 6461ae773fc..fff4acb5426 100644 --- a/app/src/organisms/ErrorRecoveryFlows/index.tsx +++ b/app/src/organisms/ErrorRecoveryFlows/index.tsx @@ -1,4 +1,4 @@ -import { useMemo, useState } from 'react' +import { useMemo, useEffect, useState } from 'react' import { useSelector } from 'react-redux' import { @@ -7,10 +7,12 @@ import { RUN_STATUS_AWAITING_RECOVERY_PAUSED, RUN_STATUS_BLOCKED_BY_OPEN_DOOR, RUN_STATUS_FAILED, + RUN_STATUS_FINISHING, RUN_STATUS_IDLE, RUN_STATUS_PAUSED, RUN_STATUS_RUNNING, RUN_STATUS_STOP_REQUESTED, + RUN_STATUS_STOPPED, RUN_STATUS_SUCCEEDED, } from '@opentrons/api-client' import { @@ -41,10 +43,13 @@ const VALID_ER_RUN_STATUSES: RunStatus[] = [ RUN_STATUS_STOP_REQUESTED, ] +// Effectively statuses that are not an "awaiting-recovery" status OR "stop requested." const INVALID_ER_RUN_STATUSES: RunStatus[] = [ RUN_STATUS_RUNNING, RUN_STATUS_PAUSED, RUN_STATUS_BLOCKED_BY_OPEN_DOOR, + RUN_STATUS_FINISHING, + RUN_STATUS_STOPPED, RUN_STATUS_FAILED, RUN_STATUS_SUCCEEDED, RUN_STATUS_IDLE, @@ -52,7 +57,6 @@ const INVALID_ER_RUN_STATUSES: RunStatus[] = [ export interface UseErrorRecoveryResult { isERActive: boolean - /* There is no FailedCommand if the run statis is not AWAITING_RECOVERY. */ failedCommand: FailedCommand | null } @@ -61,45 +65,48 @@ export function useErrorRecoveryFlows( runStatus: RunStatus | null ): UseErrorRecoveryResult { const [isERActive, setIsERActive] = useState(false) - // If client accesses a valid ER runs status besides AWAITING_RECOVERY but accesses it outside of Error Recovery flows, don't show ER. const [hasSeenAwaitingRecovery, setHasSeenAwaitingRecovery] = useState(false) const failedCommand = useCurrentlyRecoveringFrom(runId, runStatus) - if ( - !hasSeenAwaitingRecovery && - ([ - RUN_STATUS_AWAITING_RECOVERY, - RUN_STATUS_AWAITING_RECOVERY_BLOCKED_BY_OPEN_DOOR, - RUN_STATUS_AWAITING_RECOVERY_PAUSED, - ] as Array).includes(runStatus) - ) { - setHasSeenAwaitingRecovery(true) - } - // Reset recovery mode after the client has exited recovery, otherwise "cancel run" will trigger ER after the first recovery. - else if ( - hasSeenAwaitingRecovery && - runStatus != null && - INVALID_ER_RUN_STATUSES.includes(runStatus) - ) { - setHasSeenAwaitingRecovery(false) - } - - const isValidRunStatus = - runStatus != null && - VALID_ER_RUN_STATUSES.includes(runStatus) && - hasSeenAwaitingRecovery + // The complexity of this logic exists to persist Error Recovery screens past the server's definition of Error Recovery. + // Ex, show a "cancelling run" modal in Error Recovery flows despite the robot no longer being in a recoverable state. - if (!isERActive && isValidRunStatus && failedCommand != null) { - setIsERActive(true) - } - // Because multiple ER flows may occur per run, disable ER when the status is not "awaiting-recovery" or a - // terminating run status in which we want to persist ER flows. Specific recovery commands cause run status to change. - // See a specific command's docstring for details. - // ER handles a null failedCommand outside the splash screen, so we shouldn't set it false here. - else if (isERActive && !isValidRunStatus) { - setIsERActive(false) + const isValidERStatus = (status: RunStatus | null): boolean => { + return ( + status !== null && + (status === RUN_STATUS_AWAITING_RECOVERY || + (VALID_ER_RUN_STATUSES.includes(status) && hasSeenAwaitingRecovery)) + ) } + // If client accesses a valid ER runs status besides AWAITING_RECOVERY but accesses it outside of Error Recovery flows, + // don't show ER. + useEffect(() => { + if (runStatus != null) { + const isAwaitingRecovery = + VALID_ER_RUN_STATUSES.includes(runStatus) && + runStatus !== RUN_STATUS_STOP_REQUESTED + + if (isAwaitingRecovery && !hasSeenAwaitingRecovery) { + setHasSeenAwaitingRecovery(true) + } else if (INVALID_ER_RUN_STATUSES.includes(runStatus)) { + setHasSeenAwaitingRecovery(false) + } + } + }, [runStatus, hasSeenAwaitingRecovery]) + + // Manage isERActive state, the condition that actually renders Error Recovery. + useEffect(() => { + const shouldBeActive = + isValidERStatus(runStatus) && + // The failedCommand is null when a stop is requested, but we still want to persist Error Recovery in specific circumstances. + (failedCommand !== null || runStatus === RUN_STATUS_STOP_REQUESTED) + + if (shouldBeActive !== isERActive) { + setIsERActive(shouldBeActive) + } + }, [runStatus, failedCommand, hasSeenAwaitingRecovery, isERActive]) + return { isERActive, failedCommand, diff --git a/app/src/resources/protocols/hooks/useRunningStepCounts.ts b/app/src/resources/protocols/hooks/useRunningStepCounts.ts index 915676ba5ec..f3a7cb5028f 100644 --- a/app/src/resources/protocols/hooks/useRunningStepCounts.ts +++ b/app/src/resources/protocols/hooks/useRunningStepCounts.ts @@ -3,6 +3,7 @@ import { useMostRecentCompletedAnalysis } from '/app/resources/runs' import { useLastRunProtocolCommand } from './useLastRunProtocolCommand' import type { CommandsData } from '@opentrons/api-client' +import { getRunningStepCountsFrom } from '/app/resources/protocols' export interface StepCounts { /* Excludes "fixit" commands. Returns null if the step is not found. */ @@ -35,21 +36,5 @@ export function useRunningStepCounts( commandsData ?? null ) - const lastRunAnalysisCommandIndex = analysisCommands.findIndex( - c => c.key === lastRunCommandNoFixit?.key - ) - - const currentStepNumberByAnalysis = - lastRunAnalysisCommandIndex === -1 ? null : lastRunAnalysisCommandIndex + 1 - - const hasRunDiverged = - lastRunCommandNoFixit?.key == null || currentStepNumberByAnalysis == null - - const totalStepCount = !hasRunDiverged ? analysisCommands.length : null - - return { - currentStepNumber: currentStepNumberByAnalysis, - totalStepCount, - hasRunDiverged, - } + return getRunningStepCountsFrom(analysisCommands, lastRunCommandNoFixit) } diff --git a/app/src/resources/protocols/utils.ts b/app/src/resources/protocols/utils.ts index 82d62e8cc0b..cfee1b61eb6 100644 --- a/app/src/resources/protocols/utils.ts +++ b/app/src/resources/protocols/utils.ts @@ -1,4 +1,6 @@ import type { RunTimeCommand } from '@opentrons/shared-data' +import type { RunCommandSummary } from '@opentrons/api-client' +import type { StepCounts } from '/app/resources/protocols/hooks' export function isGripperInCommands(commands: RunTimeCommand[]): boolean { return ( @@ -8,3 +10,27 @@ export function isGripperInCommands(commands: RunTimeCommand[]): boolean { ) ?? false ) } + +// See useRunningStepCounts. +export function getRunningStepCountsFrom( + analysisCommands: RunTimeCommand[], + lastRunProtocolCommand: RunCommandSummary | null +): StepCounts { + const lastRunAnalysisCommandIndex = analysisCommands.findIndex( + c => c.key === lastRunProtocolCommand?.key + ) + + const currentStepNumberByAnalysis = + lastRunAnalysisCommandIndex === -1 ? null : lastRunAnalysisCommandIndex + 1 + + const hasRunDiverged = + lastRunProtocolCommand?.key == null || currentStepNumberByAnalysis == null + + const totalStepCount = !hasRunDiverged ? analysisCommands.length : null + + return { + currentStepNumber: currentStepNumberByAnalysis, + totalStepCount, + hasRunDiverged, + } +}