From a57a998516e40119f83b3395c8a45248fe213a80 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Tue, 21 May 2024 14:49:21 -0400 Subject: [PATCH 1/9] Do light refactors on `RunsPublisher`. * Mark `RunHooks` and `EngineStateSlice` as private. * Rename `initialize()` to `start_publishing_for_run()`, because "initialize" was reading to me like a one-time thing for the instance. * Remove unused attributes. * Use "new" instead of "current" in variable names, because "current" already has a different meaning in "current command" and "currently recovering from." --- .../robot_server/runs/run_data_manager.py | 2 +- .../publishers/runs_publisher.py | 36 ++++++++----------- .../publishers/test_runs_publisher.py | 12 ++++--- 3 files changed, 23 insertions(+), 27 deletions(-) diff --git a/robot-server/robot_server/runs/run_data_manager.py b/robot-server/robot_server/runs/run_data_manager.py index 5f69ba5a960..ce59805e1c7 100644 --- a/robot-server/robot_server/runs/run_data_manager.py +++ b/robot-server/robot_server/runs/run_data_manager.py @@ -194,7 +194,7 @@ async def create( created_at=created_at, protocol_id=protocol.protocol_id if protocol is not None else None, ) - await self._runs_publisher.initialize( + await self._runs_publisher.start_publishing_for_run( get_current_command=self.get_current_command, get_state_summary=self._get_good_state_summary, run_id=run_id, diff --git a/robot-server/robot_server/service/notifications/publishers/runs_publisher.py b/robot-server/robot_server/service/notifications/publishers/runs_publisher.py index 6a5b3544f01..382849c32b5 100644 --- a/robot-server/robot_server/service/notifications/publishers/runs_publisher.py +++ b/robot-server/robot_server/service/notifications/publishers/runs_publisher.py @@ -1,4 +1,3 @@ -import asyncio from fastapi import Depends from dataclasses import dataclass from typing import Callable, Optional @@ -16,7 +15,7 @@ @dataclass -class RunHooks: +class _RunHooks: """Generated during a protocol run. Utilized by RunsPublisher.""" run_id: str @@ -25,7 +24,7 @@ class RunHooks: @dataclass -class EngineStateSlice: +class _EngineStateSlice: """Protocol Engine state relevant to RunsPublisher.""" current_command: Optional[CommandPointer] = None @@ -40,18 +39,15 @@ def __init__( ) -> None: """Returns a configured Runs Publisher.""" self._client = client - self._publisher_notifier = publisher_notifier - self._run_data_manager_polling = asyncio.Event() - self._poller: Optional[asyncio.Task[None]] = None # Variables and callbacks related to PE state changes. - self._run_hooks: Optional[RunHooks] = None - self._engine_state_slice: Optional[EngineStateSlice] = None + self._run_hooks: Optional[_RunHooks] = None + self._engine_state_slice: Optional[_EngineStateSlice] = None - self._publisher_notifier.register_publish_callbacks( + publisher_notifier.register_publish_callbacks( [self._handle_current_command_change, self._handle_engine_status_change] ) - async def initialize( + async def start_publishing_for_run( self, run_id: str, get_current_command: Callable[[str], Optional[CommandPointer]], @@ -64,12 +60,12 @@ async def initialize( get_current_command: Callback to get the currently executing command, if any. get_state_summary: Callback to get the current run's state summary, if any. """ - self._run_hooks = RunHooks( + self._run_hooks = _RunHooks( run_id=run_id, get_current_command=get_current_command, get_state_summary=get_state_summary, ) - self._engine_state_slice = EngineStateSlice() + self._engine_state_slice = _EngineStateSlice() await self._publish_runs_advise_refetch_async(run_id=run_id) @@ -116,31 +112,29 @@ async def publish_pre_serialized_commands_notification(self, run_id: str) -> Non async def _handle_current_command_change(self) -> None: """Publish a refetch flag if the current command has changed.""" if self._run_hooks is not None and self._engine_state_slice is not None: - current_command = self._run_hooks.get_current_command( + new_current_command = self._run_hooks.get_current_command( self._run_hooks.run_id ) - if self._engine_state_slice.current_command != current_command: + if self._engine_state_slice.current_command != new_current_command: await self._publish_current_command() - self._engine_state_slice.current_command = current_command + self._engine_state_slice.current_command = new_current_command async def _handle_engine_status_change(self) -> None: """Publish a refetch flag if the engine status has changed.""" if self._run_hooks is not None and self._engine_state_slice is not None: - current_state_summary = self._run_hooks.get_state_summary( + new_state_summary = self._run_hooks.get_state_summary( self._run_hooks.run_id ) if ( - current_state_summary is not None + new_state_summary is not None and self._engine_state_slice.state_summary_status - != current_state_summary.status + != new_state_summary.status ): await self._publish_runs_advise_refetch_async( run_id=self._run_hooks.run_id ) - self._engine_state_slice.state_summary_status = ( - current_state_summary.status - ) + self._engine_state_slice.state_summary_status = new_state_summary.status _runs_publisher_accessor: AppStateAccessor[RunsPublisher] = AppStateAccessor[ diff --git a/robot-server/tests/service/notifications/publishers/test_runs_publisher.py b/robot-server/tests/service/notifications/publishers/test_runs_publisher.py index 5d22f8c8428..38c256cea60 100644 --- a/robot-server/tests/service/notifications/publishers/test_runs_publisher.py +++ b/robot-server/tests/service/notifications/publishers/test_runs_publisher.py @@ -61,7 +61,9 @@ async def test_initialize( get_current_command = AsyncMock() get_state_summary = AsyncMock() - await runs_publisher.initialize(run_id, get_current_command, get_state_summary) + await runs_publisher.start_publishing_for_run( + run_id, get_current_command, get_state_summary + ) assert runs_publisher._run_hooks assert runs_publisher._run_hooks.run_id == run_id @@ -82,7 +84,7 @@ async def test_clean_up_current_run( runs_publisher: RunsPublisher, notification_client: AsyncMock ) -> None: """It should publish to appropriate topics at the end of a run.""" - await runs_publisher.initialize("1234", AsyncMock(), AsyncMock()) + await runs_publisher.start_publishing_for_run("1234", AsyncMock(), AsyncMock()) await runs_publisher.clean_up_run(run_id="1234") @@ -106,7 +108,7 @@ async def test_handle_current_command_change( runs_publisher: RunsPublisher, notification_client: AsyncMock ) -> None: """It should handle command changes appropriately.""" - await runs_publisher.initialize( + await runs_publisher.start_publishing_for_run( "1234", lambda _: mock_curent_command("command1"), AsyncMock() ) @@ -135,7 +137,7 @@ async def test_handle_engine_status_change( runs_publisher: RunsPublisher, notification_client: AsyncMock ) -> None: """It should handle engine status changes appropriately.""" - await runs_publisher.initialize( + await runs_publisher.start_publishing_for_run( "1234", lambda _: mock_curent_command("command1"), AsyncMock() ) @@ -168,7 +170,7 @@ async def test_publish_pre_serialized_commannds_notif( runs_publisher: RunsPublisher, notification_client: AsyncMock ) -> None: """It should send out a notification for pre serialized commands.""" - await runs_publisher.initialize( + await runs_publisher.start_publishing_for_run( "1234", lambda _: mock_curent_command("command1"), AsyncMock() ) From c67595712d5bbfadd24d9d28a78a02104b4d436d Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Tue, 21 May 2024 14:46:42 -0400 Subject: [PATCH 2/9] Rework `current_command` topic as `commands_links`. WIP --- app/src/redux/shell/types.ts | 2 +- .../runs/useNotifyAllCommandsQuery.ts | 8 +++- .../robot_server/runs/run_data_manager.py | 1 + .../publishers/runs_publisher.py | 38 ++++++++++++++++--- .../service/notifications/topics.py | 2 +- .../publishers/test_runs_publisher.py | 4 +- 6 files changed, 44 insertions(+), 11 deletions(-) diff --git a/app/src/redux/shell/types.ts b/app/src/redux/shell/types.ts index 8f485e24bd7..9696827ae60 100644 --- a/app/src/redux/shell/types.ts +++ b/app/src/redux/shell/types.ts @@ -138,7 +138,7 @@ export interface RobotMassStorageDeviceRemoved { export type NotifyTopic = | 'ALL_TOPICS' | 'robot-server/maintenance_runs/current_run' - | 'robot-server/runs/current_command' + | 'robot-server/runs/commands_links' | 'robot-server/runs' | `robot-server/runs/${string}` | 'robot-server/deck_configuration' diff --git a/app/src/resources/runs/useNotifyAllCommandsQuery.ts b/app/src/resources/runs/useNotifyAllCommandsQuery.ts index c4624313d00..11e1e880c68 100644 --- a/app/src/resources/runs/useNotifyAllCommandsQuery.ts +++ b/app/src/resources/runs/useNotifyAllCommandsQuery.ts @@ -11,8 +11,14 @@ export function useNotifyAllCommandsQuery( params?: GetCommandsParams | null, options: QueryOptionsWithPolling = {} ): UseQueryResult { + // Assume the useAllCommandsQuery() response can only change when the command links change. + // + // TODO(mm, 2024-05-21): I don't think this is correct. If a command goes from + // running to succeeded, that may change the useAllCommandsQuery response, but it + // will not necessarily change the command links. We might need an MQTT topic + // covering "any change in `GET /runs/{id}/commands`". const { notifyOnSettled, isNotifyEnabled } = useNotifyService({ - topic: 'robot-server/runs/current_command', // only updates to the current command cause all commands to change + topic: 'robot-server/runs/commands_links', options, }) diff --git a/robot-server/robot_server/runs/run_data_manager.py b/robot-server/robot_server/runs/run_data_manager.py index ce59805e1c7..2fe67a48c27 100644 --- a/robot-server/robot_server/runs/run_data_manager.py +++ b/robot-server/robot_server/runs/run_data_manager.py @@ -196,6 +196,7 @@ async def create( ) await self._runs_publisher.start_publishing_for_run( get_current_command=self.get_current_command, + get_recovery_target_command=self.get_recovery_target_command, get_state_summary=self._get_good_state_summary, run_id=run_id, ) diff --git a/robot-server/robot_server/service/notifications/publishers/runs_publisher.py b/robot-server/robot_server/service/notifications/publishers/runs_publisher.py index 382849c32b5..bcb54133b3e 100644 --- a/robot-server/robot_server/service/notifications/publishers/runs_publisher.py +++ b/robot-server/robot_server/service/notifications/publishers/runs_publisher.py @@ -20,6 +20,7 @@ class _RunHooks: run_id: str get_current_command: Callable[[str], Optional[CommandPointer]] + get_recovery_target_command: Callable[[str], Optional[CommandPointer]] get_state_summary: Callable[[str], Optional[StateSummary]] @@ -28,6 +29,7 @@ class _EngineStateSlice: """Protocol Engine state relevant to RunsPublisher.""" current_command: Optional[CommandPointer] = None + recovery_target_command: Optional[CommandPointer] = None state_summary_status: Optional[EngineStatus] = None @@ -44,13 +46,18 @@ def __init__( self._engine_state_slice: Optional[_EngineStateSlice] = None publisher_notifier.register_publish_callbacks( - [self._handle_current_command_change, self._handle_engine_status_change] + [ + self._handle_current_command_change, + self._handle_recovery_target_command_change, + self._handle_engine_status_change, + ] ) async def start_publishing_for_run( self, run_id: str, get_current_command: Callable[[str], Optional[CommandPointer]], + get_recovery_target_command: Callable[[str], Optional[CommandPointer]], get_state_summary: Callable[[str], Optional[StateSummary]], ) -> None: """Initialize RunsPublisher with necessary information derived from the current run. @@ -63,6 +70,7 @@ async def start_publishing_for_run( self._run_hooks = _RunHooks( run_id=run_id, get_current_command=get_current_command, + get_recovery_target_command=get_recovery_target_command, get_state_summary=get_state_summary, ) self._engine_state_slice = _EngineStateSlice() @@ -74,10 +82,14 @@ async def clean_up_run(self, run_id: str) -> None: await self._publish_runs_advise_refetch_async(run_id=run_id) await self._publish_runs_advise_unsubscribe_async(run_id=run_id) - async def _publish_current_command(self) -> None: - """Publishes the equivalent of GET /runs/:runId/commands?cursor=null&pageLength=1.""" + async def _publish_command_links(self) -> None: + """Publish an update to the run's command links. + + Corresponds to the `links` field in `GET /runs/:runId/commands` + (regardless of query parameters). + """ await self._client.publish_advise_refetch_async( - topic=Topics.RUNS_CURRENT_COMMAND + topic=Topics.RUNS_COMMANDS_LINKS ) async def _publish_runs_advise_refetch_async(self, run_id: str) -> None: @@ -96,7 +108,7 @@ async def _publish_runs_advise_unsubscribe_async(self, run_id: str) -> None: topic=f"{Topics.RUNS}/{run_id}" ) await self._client.publish_advise_unsubscribe_async( - topic=Topics.RUNS_CURRENT_COMMAND + topic=Topics.RUNS_COMMANDS_LINKS ) await self._client.publish_advise_unsubscribe_async( topic=f"{Topics.RUNS_PRE_SERIALIZED_COMMANDS}/{run_id}" @@ -116,9 +128,23 @@ async def _handle_current_command_change(self) -> None: self._run_hooks.run_id ) if self._engine_state_slice.current_command != new_current_command: - await self._publish_current_command() + await self._publish_command_links() self._engine_state_slice.current_command = new_current_command + async def _handle_recovery_target_command_change(self) -> None: + if self._run_hooks is not None and self._engine_state_slice is not None: + new_recovery_target_command = self._run_hooks.get_recovery_target_command( + self._run_hooks.run_id + ) + if ( + self._engine_state_slice.recovery_target_command + != new_recovery_target_command + ): + await self._publish_command_links() + self._engine_state_slice.recovery_target_command = ( + new_recovery_target_command + ) + async def _handle_engine_status_change(self) -> None: """Publish a refetch flag if the engine status has changed.""" if self._run_hooks is not None and self._engine_state_slice is not None: diff --git a/robot-server/robot_server/service/notifications/topics.py b/robot-server/robot_server/service/notifications/topics.py index f8a6ecaf701..bb21d7e6760 100644 --- a/robot-server/robot_server/service/notifications/topics.py +++ b/robot-server/robot_server/service/notifications/topics.py @@ -12,7 +12,7 @@ class Topics(str, Enum): """ MAINTENANCE_RUNS_CURRENT_RUN = f"{_TOPIC_BASE}/maintenance_runs/current_run" - RUNS_CURRENT_COMMAND = f"{_TOPIC_BASE}/runs/current_command" + RUNS_COMMANDS_LINKS = f"{_TOPIC_BASE}/runs/commands_links" RUNS = f"{_TOPIC_BASE}/runs" DECK_CONFIGURATION = f"{_TOPIC_BASE}/deck_configuration" RUNS_PRE_SERIALIZED_COMMANDS = f"{_TOPIC_BASE}/runs/pre_serialized_commands" diff --git a/robot-server/tests/service/notifications/publishers/test_runs_publisher.py b/robot-server/tests/service/notifications/publishers/test_runs_publisher.py index 38c256cea60..815d0a2283c 100644 --- a/robot-server/tests/service/notifications/publishers/test_runs_publisher.py +++ b/robot-server/tests/service/notifications/publishers/test_runs_publisher.py @@ -96,7 +96,7 @@ async def test_clean_up_current_run( topic=f"{Topics.RUNS}/1234" ) notification_client.publish_advise_unsubscribe_async.assert_any_await( - topic=Topics.RUNS_CURRENT_COMMAND + topic=Topics.RUNS_COMMANDS_LINKS ) notification_client.publish_advise_unsubscribe_async.assert_any_await( topic=f"{Topics.RUNS_PRE_SERIALIZED_COMMANDS}/1234" @@ -128,7 +128,7 @@ async def test_handle_current_command_change( await runs_publisher._handle_current_command_change() notification_client.publish_advise_refetch_async.assert_any_await( - topic=Topics.RUNS_CURRENT_COMMAND + topic=Topics.RUNS_COMMANDS_LINKS ) From dbb16808e1a0d7639ec130c4cf0519bb2f8c9353 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Tue, 21 May 2024 15:51:08 -0400 Subject: [PATCH 3/9] Update RunsPublisher tests. --- .../publishers/test_runs_publisher.py | 124 ++++++++++++------ 1 file changed, 86 insertions(+), 38 deletions(-) diff --git a/robot-server/tests/service/notifications/publishers/test_runs_publisher.py b/robot-server/tests/service/notifications/publishers/test_runs_publisher.py index 815d0a2283c..fe71f322f59 100644 --- a/robot-server/tests/service/notifications/publishers/test_runs_publisher.py +++ b/robot-server/tests/service/notifications/publishers/test_runs_publisher.py @@ -1,14 +1,17 @@ """Tests for runs publisher.""" import pytest from datetime import datetime -from unittest.mock import MagicMock, AsyncMock +from unittest.mock import AsyncMock, MagicMock, Mock + +from opentrons.protocol_engine import CommandPointer, EngineStatus from robot_server.service.notifications import RunsPublisher, Topics -from opentrons.protocol_engine import CommandPointer, EngineStatus, StateSummary +from robot_server.service.notifications.notification_client import NotificationClient +from robot_server.service.notifications.publisher_notifier import PublisherNotifier -def mock_curent_command(command_id: str) -> CommandPointer: - """Create a mock CommandPointer.""" +def make_command_pointer(command_id: str) -> CommandPointer: + """Create a dummy CommandPointer.""" return CommandPointer( command_id=command_id, command_key="1", @@ -17,34 +20,21 @@ def mock_curent_command(command_id: str) -> CommandPointer: ) -def mock_state_summary(run_id: str) -> StateSummary: - return StateSummary.construct( - status=EngineStatus.FAILED, - errors=[], - labware=[], - pipettes=[], - modules=[], - labwareOffsets=[], - startedAt=None, - completedAt=datetime(year=2021, month=1, day=1), - ) - - @pytest.fixture -def notification_client() -> AsyncMock: +def notification_client() -> Mock: """Mocked notification client.""" - return AsyncMock() + return Mock(spec_set=NotificationClient) @pytest.fixture -def publisher_notifier() -> AsyncMock: +def publisher_notifier() -> Mock: """Mocked publisher notifier.""" - return AsyncMock() + return Mock(spec_set=PublisherNotifier) @pytest.fixture async def runs_publisher( - notification_client: AsyncMock, publisher_notifier: AsyncMock + notification_client: Mock, publisher_notifier: Mock ) -> RunsPublisher: """Instantiate RunsPublisher.""" return RunsPublisher( @@ -52,25 +42,32 @@ async def runs_publisher( ) -@pytest.mark.asyncio async def test_initialize( - runs_publisher: RunsPublisher, notification_client: AsyncMock + runs_publisher: RunsPublisher, notification_client: Mock ) -> None: """It should initialize the runs_publisher with required parameters and callbacks.""" run_id = "1234" get_current_command = AsyncMock() + get_recovery_target_command = AsyncMock() get_state_summary = AsyncMock() await runs_publisher.start_publishing_for_run( - run_id, get_current_command, get_state_summary + run_id, get_current_command, get_recovery_target_command, get_state_summary ) + # todo(mm, 2024-05-21): We should test through the public interface of the subject, + # not through its private attributes. assert runs_publisher._run_hooks assert runs_publisher._run_hooks.run_id == run_id assert runs_publisher._run_hooks.get_current_command == get_current_command + assert ( + runs_publisher._run_hooks.get_recovery_target_command + == get_recovery_target_command + ) assert runs_publisher._run_hooks.get_state_summary == get_state_summary assert runs_publisher._engine_state_slice assert runs_publisher._engine_state_slice.current_command is None + assert runs_publisher._engine_state_slice.recovery_target_command is None assert runs_publisher._engine_state_slice.state_summary_status is None notification_client.publish_advise_refetch_async.assert_any_await(topic=Topics.RUNS) @@ -79,12 +76,13 @@ async def test_initialize( ) -@pytest.mark.asyncio async def test_clean_up_current_run( - runs_publisher: RunsPublisher, notification_client: AsyncMock + runs_publisher: RunsPublisher, notification_client: Mock ) -> None: """It should publish to appropriate topics at the end of a run.""" - await runs_publisher.start_publishing_for_run("1234", AsyncMock(), AsyncMock()) + await runs_publisher.start_publishing_for_run( + "1234", AsyncMock(), AsyncMock(), AsyncMock() + ) await runs_publisher.clean_up_run(run_id="1234") @@ -103,25 +101,31 @@ async def test_clean_up_current_run( ) -@pytest.mark.asyncio async def test_handle_current_command_change( - runs_publisher: RunsPublisher, notification_client: AsyncMock + runs_publisher: RunsPublisher, notification_client: Mock ) -> None: """It should handle command changes appropriately.""" await runs_publisher.start_publishing_for_run( - "1234", lambda _: mock_curent_command("command1"), AsyncMock() + run_id="1234", + get_current_command=lambda _: make_command_pointer("command1"), + get_recovery_target_command=AsyncMock(), + get_state_summary=AsyncMock(), ) + # todo(mm, 2024-05-21): We should test through the public interface of the subject, + # not through its private attributes. assert runs_publisher._run_hooks assert runs_publisher._engine_state_slice - runs_publisher._engine_state_slice.current_command = mock_curent_command("command1") + runs_publisher._engine_state_slice.current_command = make_command_pointer( + "command1" + ) await runs_publisher._handle_current_command_change() assert notification_client.publish_advise_refetch_async.call_count == 2 - runs_publisher._run_hooks.get_current_command = lambda _: mock_curent_command( + runs_publisher._run_hooks.get_current_command = lambda _: make_command_pointer( "command2" ) @@ -132,15 +136,54 @@ async def test_handle_current_command_change( ) -@pytest.mark.asyncio +async def test_handle_recovery_target_command_change( + runs_publisher: RunsPublisher, notification_client: Mock +) -> None: + """It should handle command changes appropriately.""" + await runs_publisher.start_publishing_for_run( + run_id="1234", + get_current_command=AsyncMock(), + get_recovery_target_command=lambda _: make_command_pointer("command1"), + get_state_summary=AsyncMock(), + ) + + # todo(mm, 2024-05-21): We should test through the public interface of the subject, + # not through its private attributes. + assert runs_publisher._run_hooks + assert runs_publisher._engine_state_slice + + runs_publisher._engine_state_slice.recovery_target_command = make_command_pointer( + "command1" + ) + + await runs_publisher._handle_recovery_target_command_change() + + assert notification_client.publish_advise_refetch_async.call_count == 2 + + runs_publisher._run_hooks.get_recovery_target_command = ( + lambda _: make_command_pointer("command2") + ) + + await runs_publisher._handle_recovery_target_command_change() + + notification_client.publish_advise_refetch_async.assert_any_await( + topic=Topics.RUNS_COMMANDS_LINKS + ) + + async def test_handle_engine_status_change( - runs_publisher: RunsPublisher, notification_client: AsyncMock + runs_publisher: RunsPublisher, notification_client: Mock ) -> None: """It should handle engine status changes appropriately.""" await runs_publisher.start_publishing_for_run( - "1234", lambda _: mock_curent_command("command1"), AsyncMock() + run_id="1234", + get_current_command=lambda _: make_command_pointer("command1"), + get_recovery_target_command=AsyncMock(), + get_state_summary=AsyncMock(), ) + # todo(mm, 2024-05-21): We should test through the public interface of the subject, + # not through its private attributes. assert runs_publisher._run_hooks assert runs_publisher._engine_state_slice @@ -167,13 +210,18 @@ async def test_handle_engine_status_change( async def test_publish_pre_serialized_commannds_notif( - runs_publisher: RunsPublisher, notification_client: AsyncMock + runs_publisher: RunsPublisher, notification_client: Mock ) -> None: """It should send out a notification for pre serialized commands.""" await runs_publisher.start_publishing_for_run( - "1234", lambda _: mock_curent_command("command1"), AsyncMock() + run_id="1234", + get_current_command=lambda _: make_command_pointer("command1"), + get_recovery_target_command=AsyncMock(), + get_state_summary=AsyncMock(), ) + # todo(mm, 2024-05-21): We should test through the public interface of the subject, + # not through its private attributes. assert runs_publisher._run_hooks assert runs_publisher._engine_state_slice assert notification_client.publish_advise_refetch_async.call_count == 2 From 6ab76306fe2daf8a526415410347d937d8a4c0e9 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Tue, 21 May 2024 13:20:33 -0400 Subject: [PATCH 4/9] Rename `useCurrentlyFailedRunCommand` -> `useCurrentlyRecoveringFrom()`. For symmetry with how the field is named in the HTTP response. --- .../__tests__/ErrorRecoveryFlows.test.tsx | 6 ++---- .../ErrorRecoveryFlows/__tests__/utils.test.ts | 10 +++++----- app/src/organisms/ErrorRecoveryFlows/index.tsx | 4 ++-- app/src/organisms/ErrorRecoveryFlows/utils.ts | 2 +- 4 files changed, 10 insertions(+), 12 deletions(-) diff --git a/app/src/organisms/ErrorRecoveryFlows/__tests__/ErrorRecoveryFlows.test.tsx b/app/src/organisms/ErrorRecoveryFlows/__tests__/ErrorRecoveryFlows.test.tsx index d44e3d4b333..065a9279920 100644 --- a/app/src/organisms/ErrorRecoveryFlows/__tests__/ErrorRecoveryFlows.test.tsx +++ b/app/src/organisms/ErrorRecoveryFlows/__tests__/ErrorRecoveryFlows.test.tsx @@ -11,7 +11,7 @@ import { renderWithProviders } from '../../../__testing-utils__' import { i18n } from '../../../i18n' import { ErrorRecoveryFlows, useErrorRecoveryFlows } from '..' import { ErrorRecoveryWizard } from '../ErrorRecoveryWizard' -import { useCurrentlyFailedRunCommand } from '../utils' +import { useCurrentlyRecoveringFrom } from '../utils' import type { RunStatus } from '@opentrons/api-client' @@ -20,9 +20,7 @@ vi.mock('../utils') describe('useErrorRecovery', () => { beforeEach(() => { - vi.mocked(useCurrentlyFailedRunCommand).mockReturnValue( - 'mockCommand' as any - ) + vi.mocked(useCurrentlyRecoveringFrom).mockReturnValue('mockCommand' as any) }) it('should have initial state of isEREnabled as false', () => { diff --git a/app/src/organisms/ErrorRecoveryFlows/__tests__/utils.test.ts b/app/src/organisms/ErrorRecoveryFlows/__tests__/utils.test.ts index b91ad453591..e0f1b07b095 100644 --- a/app/src/organisms/ErrorRecoveryFlows/__tests__/utils.test.ts +++ b/app/src/organisms/ErrorRecoveryFlows/__tests__/utils.test.ts @@ -11,7 +11,7 @@ import { getErrorKind, getRecoveryRouteNavigation, useRouteUpdateActions, - useCurrentlyFailedRunCommand, + useCurrentlyRecoveringFrom, } from '../utils' import { useNotifyAllCommandsQuery } from '../../../resources/runs' @@ -165,7 +165,7 @@ describe('useCurrentlyFailedRunCommand', () => { it('returns null on initial render when the run status is not "awaiting-recovery"', () => { const { result } = renderHook(() => - useCurrentlyFailedRunCommand(MOCK_RUN_ID, RUN_STATUS_RUNNING) + useCurrentlyRecoveringFrom(MOCK_RUN_ID, RUN_STATUS_RUNNING) ) expect(result.current).toBeNull() @@ -174,7 +174,7 @@ describe('useCurrentlyFailedRunCommand', () => { it('sets recentFailedCommand correctly when runStatus is "awaiting-recovery" and there is no recent failed command', () => { const { result, rerender } = renderHook( // @ts-expect-error this works - props => useCurrentlyFailedRunCommand(...props), + props => useCurrentlyRecoveringFrom(...props), { initialProps: [MOCK_RUN_ID, RUN_STATUS_RUNNING], } @@ -194,7 +194,7 @@ describe('useCurrentlyFailedRunCommand', () => { it('always returns the failed protocol run command that caused the run to enter "awaiting-recovery"', () => { const { result, rerender } = renderHook( // @ts-expect-error this works - props => useCurrentlyFailedRunCommand(...props), + props => useCurrentlyRecoveringFrom(...props), { initialProps: [MOCK_RUN_ID, RUN_STATUS_AWAITING_RECOVERY], } @@ -216,7 +216,7 @@ describe('useCurrentlyFailedRunCommand', () => { it('sets recentFailedCommand to null when runStatus is not "awaiting-recovery"', () => { const { result, rerender } = renderHook( // @ts-expect-error this works - props => useCurrentlyFailedRunCommand(...props), + props => useCurrentlyRecoveringFrom(...props), { initialProps: ['runId', 'awaiting-recovery'], } diff --git a/app/src/organisms/ErrorRecoveryFlows/index.tsx b/app/src/organisms/ErrorRecoveryFlows/index.tsx index b7da1f4caaf..1957e231e50 100644 --- a/app/src/organisms/ErrorRecoveryFlows/index.tsx +++ b/app/src/organisms/ErrorRecoveryFlows/index.tsx @@ -3,7 +3,7 @@ import * as React from 'react' import { RUN_STATUS_AWAITING_RECOVERY } from '@opentrons/api-client' import { ErrorRecoveryWizard } from './ErrorRecoveryWizard' -import { useCurrentlyFailedRunCommand } from './utils' +import { useCurrentlyRecoveringFrom } from './utils' import type { RunStatus } from '@opentrons/api-client' import type { FailedCommand } from './types' @@ -19,7 +19,7 @@ export function useErrorRecoveryFlows( runStatus: RunStatus | null ): UseErrorRecoveryResult { const [isERActive, setIsERActive] = React.useState(false) - const failedCommand = useCurrentlyFailedRunCommand(runId, runStatus) + const failedCommand = useCurrentlyRecoveringFrom(runId, runStatus) const toggleER = (): void => { setIsERActive(!isERActive) diff --git a/app/src/organisms/ErrorRecoveryFlows/utils.ts b/app/src/organisms/ErrorRecoveryFlows/utils.ts index 749c314d7fb..fd32556e757 100644 --- a/app/src/organisms/ErrorRecoveryFlows/utils.ts +++ b/app/src/organisms/ErrorRecoveryFlows/utils.ts @@ -26,7 +26,7 @@ import type { const ALL_COMMANDS_POLL_MS = 5000 // TODO(jh, 05-20-24): Update the logic for returning the failed run command once EXEC-458 merges. -export function useCurrentlyFailedRunCommand( +export function useCurrentlyRecoveringFrom( runId: string, runStatus: RunStatus | null ): FailedCommand | null { From 27fd52fdb0f7081b4673dc494906dde837af3647 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Tue, 21 May 2024 13:57:32 -0400 Subject: [PATCH 5/9] Attempt React hook updates. --- app/src/organisms/ErrorRecoveryFlows/utils.ts | 46 +++++++------------ 1 file changed, 17 insertions(+), 29 deletions(-) diff --git a/app/src/organisms/ErrorRecoveryFlows/utils.ts b/app/src/organisms/ErrorRecoveryFlows/utils.ts index fd32556e757..e46eb70d46f 100644 --- a/app/src/organisms/ErrorRecoveryFlows/utils.ts +++ b/app/src/organisms/ErrorRecoveryFlows/utils.ts @@ -2,9 +2,8 @@ import * as React from 'react' import { useTranslation } from 'react-i18next' import head from 'lodash/head' import last from 'lodash/last' -import findLast from 'lodash/findLast' -import { RUN_STATUS_AWAITING_RECOVERY } from '@opentrons/api-client' +import { useCommandQuery } from '@opentrons/react-api-client' import { useNotifyAllCommandsQuery } from '../../resources/runs' import { RECOVERY_MAP, ERROR_KINDS, INVALID, STEP_ORDER } from './constants' @@ -21,46 +20,35 @@ import type { // TODO(jh, 05-09-24): Migrate utils, useRecoveryCommands.ts, and respective tests to a utils dir, and make each util a separate file. -// While the run is "awaiting-recovery", return the most recently failed run command with a protocol intent. -// Otherwise, returns null. const ALL_COMMANDS_POLL_MS = 5000 -// TODO(jh, 05-20-24): Update the logic for returning the failed run command once EXEC-458 merges. +// Return the `currentlyRecoveringFrom` command returned by the server, if any. +// Otherwise, returns null. export function useCurrentlyRecoveringFrom( runId: string, runStatus: RunStatus | null ): FailedCommand | null { - const [ - recentFailedCommand, - setRecentFailedCommand, - ] = React.useState(null) - // The most recently failed protocol command causes the run to enter "awaiting-recovery", therefore only check - // for a newly failed command when the run first enters "awaiting-recovery." - const isRunStatusAwaitingRecovery = runStatus === RUN_STATUS_AWAITING_RECOVERY - const { data: allCommandsQueryData } = useNotifyAllCommandsQuery( runId, - null, + { cursor: null, pageLength: 0 }, // pageLength 0 because we only care about the links. { - enabled: isRunStatusAwaitingRecovery && recentFailedCommand == null, refetchInterval: ALL_COMMANDS_POLL_MS, } ) + const currentlyRecoveringFromLink = + allCommandsQueryData?.links.currentlyRecoveringFrom + + // One-shot query for the full details of the currentlyRecoveringFrom command. + // + // 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( + currentlyRecoveringFromLink?.meta.runId ?? null, + currentlyRecoveringFromLink?.meta.commandId ?? null, + { enabled: currentlyRecoveringFromLink != null, staleTime: Infinity } + ) - React.useEffect(() => { - if (isRunStatusAwaitingRecovery && recentFailedCommand == null) { - const failedCommand = - findLast( - allCommandsQueryData?.data, - command => command.status === 'failed' && command.intent !== 'fixit' - ) ?? null - setRecentFailedCommand(failedCommand) - } else if (!isRunStatusAwaitingRecovery && recentFailedCommand != null) { - setRecentFailedCommand(null) - } - }, [isRunStatusAwaitingRecovery, recentFailedCommand, allCommandsQueryData]) - - return recentFailedCommand + return commandQueryData?.data ?? null } export function useErrorName(errorKind: ErrorKind): string { From 4f5eae66f944af983ff568bc2c4af87553546d66 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Wed, 22 May 2024 15:36:20 -0400 Subject: [PATCH 6/9] Attempt test updates for React hook updates. --- .../__tests__/utils.test.ts | 113 ++++++------------ .../organisms/ErrorRecoveryFlows/index.tsx | 2 +- app/src/organisms/ErrorRecoveryFlows/utils.ts | 4 +- 3 files changed, 36 insertions(+), 83 deletions(-) diff --git a/app/src/organisms/ErrorRecoveryFlows/__tests__/utils.test.ts b/app/src/organisms/ErrorRecoveryFlows/__tests__/utils.test.ts index e0f1b07b095..737d973bebc 100644 --- a/app/src/organisms/ErrorRecoveryFlows/__tests__/utils.test.ts +++ b/app/src/organisms/ErrorRecoveryFlows/__tests__/utils.test.ts @@ -1,10 +1,5 @@ import { vi, describe, it, expect, beforeEach } from 'vitest' -import { renderHook, act } from '@testing-library/react' - -import { - RUN_STATUS_AWAITING_RECOVERY, - RUN_STATUS_RUNNING, -} from '@opentrons/api-client' +import { renderHook } from '@testing-library/react' import { ERROR_KINDS, INVALID, RECOVERY_MAP } from '../constants' import { @@ -17,7 +12,9 @@ import { useNotifyAllCommandsQuery } from '../../../resources/runs' import type { Mock } from 'vitest' import type { GetRouteUpdateActionsParams } from '../utils' +import { useCommandQuery } from '@opentrons/react-api-client' +vi.mock('@opentrons/react-api-client') vi.mock('../../../resources/runs') describe('getErrorKind', () => { @@ -145,87 +142,45 @@ describe('useRouteUpdateActions', () => { }) }) -const MOCK_COMMANDS_QUERY = { - data: { - data: [ - { status: 'failed', intent: 'fixit', id: '0' }, - { status: 'failed', intent: 'protocol', id: '111' }, - { status: 'failed', intent: 'protocol', id: '123' }, - { status: 'success', intent: 'fixit', id: '1' }, - ], - }, -} as any - const MOCK_RUN_ID = 'runId' +const MOCK_COMMAND_ID = 'commandId' -describe('useCurrentlyFailedRunCommand', () => { - beforeEach(() => { - vi.mocked(useNotifyAllCommandsQuery).mockReturnValue(MOCK_COMMANDS_QUERY) - }) - - it('returns null on initial render when the run status is not "awaiting-recovery"', () => { - const { result } = renderHook(() => - useCurrentlyRecoveringFrom(MOCK_RUN_ID, RUN_STATUS_RUNNING) - ) - - expect(result.current).toBeNull() - }) - - it('sets recentFailedCommand correctly when runStatus is "awaiting-recovery" and there is no recent failed command', () => { - const { result, rerender } = renderHook( - // @ts-expect-error this works - props => useCurrentlyRecoveringFrom(...props), - { - initialProps: [MOCK_RUN_ID, RUN_STATUS_RUNNING], - } - ) +describe('useCurrentlyRecoveringFrom', () => { + it('returns null if there is no currentlyRecoveringFrom command', () => { + vi.mocked(useNotifyAllCommandsQuery).mockReturnValue({ + data: { + links: {}, + }, + } as any) + vi.mocked(useCommandQuery).mockReturnValue({} as any) - act(() => { - rerender([MOCK_RUN_ID, RUN_STATUS_AWAITING_RECOVERY]) - }) + const { result } = renderHook(() => useCurrentlyRecoveringFrom(MOCK_RUN_ID)) - expect(result.current).toEqual({ - status: 'failed', - intent: 'protocol', - id: '123', - }) + expect(result.current).toStrictEqual(null) + // TODO(mm, 2024-05-22): Figure out how to assert that useCommandQuery was + // called with `enabled: false`. }) - it('always returns the failed protocol run command that caused the run to enter "awaiting-recovery"', () => { - const { result, rerender } = renderHook( - // @ts-expect-error this works - props => useCurrentlyRecoveringFrom(...props), - { - initialProps: [MOCK_RUN_ID, RUN_STATUS_AWAITING_RECOVERY], - } - ) - + it('fetches and returns the currentlyRecoveringFrom command, given that there is one', () => { vi.mocked(useNotifyAllCommandsQuery).mockReturnValue({ - ...MOCK_COMMANDS_QUERY, - ...{ status: 'failed', intent: 'protocol', id: '124' }, - }) - rerender([MOCK_RUN_ID, RUN_STATUS_AWAITING_RECOVERY]) - - expect(result.current).toEqual({ - status: 'failed', - intent: 'protocol', - id: '123', - }) - }) - - it('sets recentFailedCommand to null when runStatus is not "awaiting-recovery"', () => { - const { result, rerender } = renderHook( - // @ts-expect-error this works - props => useCurrentlyRecoveringFrom(...props), - { - initialProps: ['runId', 'awaiting-recovery'], - } - ) + data: { + links: { + currentlyRecoveringFrom: { + meta: { + commandId: MOCK_COMMAND_ID, + }, + }, + }, + }, + } as any) + vi.mocked(useCommandQuery).mockReturnValue({ + data: { data: 'mockCommandDetails' }, + } as any) - act(() => { - rerender([MOCK_RUN_ID, RUN_STATUS_RUNNING]) - }) + const { result } = renderHook(() => useCurrentlyRecoveringFrom(MOCK_RUN_ID)) - expect(result.current).toBeNull() + expect(result.current).toStrictEqual('mockCommandDetails') + // TODO(mm, 2024-05-22): Figure out how to assert that useCommandQuery was + // called with `enabled: true`. }) }) diff --git a/app/src/organisms/ErrorRecoveryFlows/index.tsx b/app/src/organisms/ErrorRecoveryFlows/index.tsx index 1957e231e50..93fa79dfa1c 100644 --- a/app/src/organisms/ErrorRecoveryFlows/index.tsx +++ b/app/src/organisms/ErrorRecoveryFlows/index.tsx @@ -19,7 +19,7 @@ export function useErrorRecoveryFlows( runStatus: RunStatus | null ): UseErrorRecoveryResult { const [isERActive, setIsERActive] = React.useState(false) - const failedCommand = useCurrentlyRecoveringFrom(runId, runStatus) + const failedCommand = useCurrentlyRecoveringFrom(runId) const toggleER = (): void => { setIsERActive(!isERActive) diff --git a/app/src/organisms/ErrorRecoveryFlows/utils.ts b/app/src/organisms/ErrorRecoveryFlows/utils.ts index e46eb70d46f..d63ce573cdb 100644 --- a/app/src/organisms/ErrorRecoveryFlows/utils.ts +++ b/app/src/organisms/ErrorRecoveryFlows/utils.ts @@ -8,7 +8,6 @@ import { useCommandQuery } from '@opentrons/react-api-client' import { useNotifyAllCommandsQuery } from '../../resources/runs' import { RECOVERY_MAP, ERROR_KINDS, INVALID, STEP_ORDER } from './constants' -import type { RunStatus } from '@opentrons/api-client' import type { RouteStep, IRecoveryMap, @@ -25,8 +24,7 @@ const ALL_COMMANDS_POLL_MS = 5000 // Return the `currentlyRecoveringFrom` command returned by the server, if any. // Otherwise, returns null. export function useCurrentlyRecoveringFrom( - runId: string, - runStatus: RunStatus | null + runId: string ): FailedCommand | null { const { data: allCommandsQueryData } = useNotifyAllCommandsQuery( runId, From 05efb84270dc9e8701348e49b1f3672bcaa0f6f1 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Fri, 24 May 2024 10:38:17 -0400 Subject: [PATCH 7/9] Delete `staleTime: Infinity`. --- app/src/organisms/ErrorRecoveryFlows/utils.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/src/organisms/ErrorRecoveryFlows/utils.ts b/app/src/organisms/ErrorRecoveryFlows/utils.ts index d63ce573cdb..5049fdbf8c6 100644 --- a/app/src/organisms/ErrorRecoveryFlows/utils.ts +++ b/app/src/organisms/ErrorRecoveryFlows/utils.ts @@ -43,7 +43,7 @@ export function useCurrentlyRecoveringFrom( const { data: commandQueryData } = useCommandQuery( currentlyRecoveringFromLink?.meta.runId ?? null, currentlyRecoveringFromLink?.meta.commandId ?? null, - { enabled: currentlyRecoveringFromLink != null, staleTime: Infinity } + { enabled: currentlyRecoveringFromLink != null } ) return commandQueryData?.data ?? null From 713d43caaf7e92b7e246b5ce833e550ecdcc13f0 Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Fri, 24 May 2024 10:41:21 -0400 Subject: [PATCH 8/9] Assert that `useCommandQuery()` is called with the correct `enabled` value. --- .../ErrorRecoveryFlows/__tests__/utils.test.ts | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/app/src/organisms/ErrorRecoveryFlows/__tests__/utils.test.ts b/app/src/organisms/ErrorRecoveryFlows/__tests__/utils.test.ts index 737d973bebc..7516e355770 100644 --- a/app/src/organisms/ErrorRecoveryFlows/__tests__/utils.test.ts +++ b/app/src/organisms/ErrorRecoveryFlows/__tests__/utils.test.ts @@ -156,9 +156,10 @@ describe('useCurrentlyRecoveringFrom', () => { const { result } = renderHook(() => useCurrentlyRecoveringFrom(MOCK_RUN_ID)) + expect(vi.mocked(useCommandQuery)).toHaveBeenCalledWith(null, null, { + enabled: false, + }) expect(result.current).toStrictEqual(null) - // TODO(mm, 2024-05-22): Figure out how to assert that useCommandQuery was - // called with `enabled: false`. }) it('fetches and returns the currentlyRecoveringFrom command, given that there is one', () => { @@ -167,6 +168,7 @@ describe('useCurrentlyRecoveringFrom', () => { links: { currentlyRecoveringFrom: { meta: { + runId: MOCK_RUN_ID, commandId: MOCK_COMMAND_ID, }, }, @@ -179,8 +181,11 @@ describe('useCurrentlyRecoveringFrom', () => { const { result } = renderHook(() => useCurrentlyRecoveringFrom(MOCK_RUN_ID)) + expect(vi.mocked(useCommandQuery)).toHaveBeenCalledWith( + MOCK_RUN_ID, + MOCK_COMMAND_ID, + { enabled: true } + ) expect(result.current).toStrictEqual('mockCommandDetails') - // TODO(mm, 2024-05-22): Figure out how to assert that useCommandQuery was - // called with `enabled: true`. }) }) From 0b9ede490c5f9570532398e2d5441c8ca69c360e Mon Sep 17 00:00:00 2001 From: Max Marrone Date: Fri, 24 May 2024 11:25:35 -0400 Subject: [PATCH 9/9] Only enable queries when run status is 'awaiting-recovery'. --- .../__tests__/utils.test.ts | 46 ++++++++++++++++++- .../organisms/ErrorRecoveryFlows/index.tsx | 2 +- app/src/organisms/ErrorRecoveryFlows/utils.ts | 19 ++++++-- 3 files changed, 59 insertions(+), 8 deletions(-) diff --git a/app/src/organisms/ErrorRecoveryFlows/__tests__/utils.test.ts b/app/src/organisms/ErrorRecoveryFlows/__tests__/utils.test.ts index 7516e355770..33f4cd529a0 100644 --- a/app/src/organisms/ErrorRecoveryFlows/__tests__/utils.test.ts +++ b/app/src/organisms/ErrorRecoveryFlows/__tests__/utils.test.ts @@ -13,6 +13,10 @@ import { useNotifyAllCommandsQuery } from '../../../resources/runs' import type { Mock } from 'vitest' import type { GetRouteUpdateActionsParams } from '../utils' import { useCommandQuery } from '@opentrons/react-api-client' +import { + RUN_STATUS_AWAITING_RECOVERY, + RUN_STATUS_IDLE, +} from '@opentrons/api-client' vi.mock('@opentrons/react-api-client') vi.mock('../../../resources/runs') @@ -146,6 +150,40 @@ const MOCK_RUN_ID = 'runId' const MOCK_COMMAND_ID = 'commandId' describe('useCurrentlyRecoveringFrom', () => { + it('disables all queries if the run is not awaiting-recovery', () => { + vi.mocked(useNotifyAllCommandsQuery).mockReturnValue({ + data: { + links: { + currentlyRecoveringFrom: { + meta: { + runId: MOCK_RUN_ID, + commandId: MOCK_COMMAND_ID, + }, + }, + }, + }, + } as any) + vi.mocked(useCommandQuery).mockReturnValue({ + data: { data: 'mockCommandDetails' }, + } as any) + + const { result } = renderHook(() => + useCurrentlyRecoveringFrom(MOCK_RUN_ID, RUN_STATUS_IDLE) + ) + + expect(vi.mocked(useNotifyAllCommandsQuery)).toHaveBeenCalledWith( + MOCK_RUN_ID, + { cursor: null, pageLength: 0 }, + { enabled: false, refetchInterval: 5000 } + ) + expect(vi.mocked(useCommandQuery)).toHaveBeenCalledWith( + MOCK_RUN_ID, + MOCK_COMMAND_ID, + { enabled: false } + ) + expect(result.current).toStrictEqual(null) + }) + it('returns null if there is no currentlyRecoveringFrom command', () => { vi.mocked(useNotifyAllCommandsQuery).mockReturnValue({ data: { @@ -154,7 +192,9 @@ describe('useCurrentlyRecoveringFrom', () => { } as any) vi.mocked(useCommandQuery).mockReturnValue({} as any) - const { result } = renderHook(() => useCurrentlyRecoveringFrom(MOCK_RUN_ID)) + const { result } = renderHook(() => + useCurrentlyRecoveringFrom(MOCK_RUN_ID, RUN_STATUS_AWAITING_RECOVERY) + ) expect(vi.mocked(useCommandQuery)).toHaveBeenCalledWith(null, null, { enabled: false, @@ -179,7 +219,9 @@ describe('useCurrentlyRecoveringFrom', () => { data: { data: 'mockCommandDetails' }, } as any) - const { result } = renderHook(() => useCurrentlyRecoveringFrom(MOCK_RUN_ID)) + const { result } = renderHook(() => + useCurrentlyRecoveringFrom(MOCK_RUN_ID, RUN_STATUS_AWAITING_RECOVERY) + ) expect(vi.mocked(useCommandQuery)).toHaveBeenCalledWith( MOCK_RUN_ID, diff --git a/app/src/organisms/ErrorRecoveryFlows/index.tsx b/app/src/organisms/ErrorRecoveryFlows/index.tsx index 93fa79dfa1c..1957e231e50 100644 --- a/app/src/organisms/ErrorRecoveryFlows/index.tsx +++ b/app/src/organisms/ErrorRecoveryFlows/index.tsx @@ -19,7 +19,7 @@ export function useErrorRecoveryFlows( runStatus: RunStatus | null ): UseErrorRecoveryResult { const [isERActive, setIsERActive] = React.useState(false) - const failedCommand = useCurrentlyRecoveringFrom(runId) + const failedCommand = useCurrentlyRecoveringFrom(runId, runStatus) const toggleER = (): void => { setIsERActive(!isERActive) diff --git a/app/src/organisms/ErrorRecoveryFlows/utils.ts b/app/src/organisms/ErrorRecoveryFlows/utils.ts index 5049fdbf8c6..e201d031334 100644 --- a/app/src/organisms/ErrorRecoveryFlows/utils.ts +++ b/app/src/organisms/ErrorRecoveryFlows/utils.ts @@ -3,11 +3,13 @@ import { useTranslation } from 'react-i18next' import head from 'lodash/head' import last from 'lodash/last' +import { RUN_STATUS_AWAITING_RECOVERY } from '@opentrons/api-client' import { useCommandQuery } from '@opentrons/react-api-client' import { useNotifyAllCommandsQuery } from '../../resources/runs' import { RECOVERY_MAP, ERROR_KINDS, INVALID, STEP_ORDER } from './constants' +import type { RunStatus } from '@opentrons/api-client' import type { RouteStep, IRecoveryMap, @@ -24,29 +26,36 @@ const ALL_COMMANDS_POLL_MS = 5000 // Return the `currentlyRecoveringFrom` command returned by the server, if any. // Otherwise, returns null. export function useCurrentlyRecoveringFrom( - runId: string + runId: string, + runStatus: RunStatus | null ): FailedCommand | null { + // There can only be a currentlyRecoveringFrom command when the run is awaiting-recovery. + // In case we're falling back to polling, only enable queries when that is the case. + const isRunStatusAwaitingRecovery = runStatus === RUN_STATUS_AWAITING_RECOVERY + const { data: allCommandsQueryData } = useNotifyAllCommandsQuery( runId, { cursor: null, pageLength: 0 }, // pageLength 0 because we only care about the links. { + enabled: isRunStatusAwaitingRecovery, refetchInterval: ALL_COMMANDS_POLL_MS, } ) const currentlyRecoveringFromLink = allCommandsQueryData?.links.currentlyRecoveringFrom - // One-shot query for the full details of the currentlyRecoveringFrom command. - // // 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( currentlyRecoveringFromLink?.meta.runId ?? null, currentlyRecoveringFromLink?.meta.commandId ?? null, - { enabled: currentlyRecoveringFromLink != null } + { + enabled: + currentlyRecoveringFromLink != null && isRunStatusAwaitingRecovery, + } ) - return commandQueryData?.data ?? null + return isRunStatusAwaitingRecovery ? commandQueryData?.data ?? null : null } export function useErrorName(errorKind: ErrorKind): string {