From 4b897fcb894b057e72030495b2e1c670abf95dd7 Mon Sep 17 00:00:00 2001 From: Jamey Huffnagle Date: Thu, 22 Aug 2024 09:51:37 -0400 Subject: [PATCH] fix(app): fix Recent Run Protocol Card onClick routing behavior (#16094) Closes RQA-3038 This PR fixes a longstanding issue (although the severity and presentation of the underlying issue has varied over time) in which clicking on a RecentRunProtocolCard often leads to unexpected UI updates, full screen re-renders, and routing problems. See #16084 for more explanation on why routing issues are more prevalent this release (and why QA is probably now filing these existing issues more aggressively). The RecentRunProtocolCard actively navigates to a page managed by TopLevelRedirects, and the code that caused the active navigation existed before MQTT effectively made these redirects instantaneous (and deprecating the need for active redirects to pages managed by TopLevelRedirects). In other words, we really don't have to do any navigation here at all, but that leads to two questions: What's actually causing this bug? What happens if we just let TopLevelRedirects do the navigation? First, it's difficult to pinpoint exactly what's causing the unexpected routing behavior, however, it very much appears that: It's almost certainly not TopLevelRedirects itself. I verified this with extensive console.logging. Note that this logic doesn't do anything when the app thinks we should still be on the dashboard. It's almost certainly some extra render cycles occurring somewhere in the app as a result of the cloneRun function, which does a good bit of query invalidation. It's quite difficult to pinpoint exactly what's causing the full on dashboard re-rendering to occur (see the current behavior video), because we don't actually have React Devtools on the physical ODD (yeah, I should probably look into this). So what does a good fix look like? If we just let TopLevelRedirects passively navigate us, we unfortunately get some wonky, but expected UI. The protocol cards are sorted based on timestamps, so the cards shuffle around when you click a card, the timestamps change a few times, and THEN we see the navigation occur cleanly. A much simpler and effective solution is to actively navigate to a fake RunSetup page. This solves the above problem (users don't see crazy shuffling cards), and it plays well with TopLevelRedirects (this component doesn't do anything until the real setup route is valid, and then it redirects us to that route cleanly). --- .../RobotDashboard/RecentRunProtocolCard.tsx | 10 +++++----- .../__tests__/RecentRunProtocolCard.test.tsx | 7 ++++--- app/src/organisms/ProtocolUpload/hooks/useCloneRun.ts | 11 +++++++---- 3 files changed, 16 insertions(+), 12 deletions(-) diff --git a/app/src/organisms/OnDeviceDisplay/RobotDashboard/RecentRunProtocolCard.tsx b/app/src/organisms/OnDeviceDisplay/RobotDashboard/RecentRunProtocolCard.tsx index 2fb5f75efb2..e39e46ef62f 100644 --- a/app/src/organisms/OnDeviceDisplay/RobotDashboard/RecentRunProtocolCard.tsx +++ b/app/src/organisms/OnDeviceDisplay/RobotDashboard/RecentRunProtocolCard.tsx @@ -38,7 +38,7 @@ import { useMissingProtocolHardware } from '../../../pages/Protocols/hooks' import { useCloneRun } from '../../ProtocolUpload/hooks' import { useRerunnableStatusText } from './hooks' -import type { Run, RunData, RunStatus } from '@opentrons/api-client' +import type { RunData, RunStatus } from '@opentrons/api-client' import type { ProtocolResource } from '@opentrons/shared-data' interface RecentRunProtocolCardProps { @@ -88,10 +88,7 @@ export function ProtocolWithLastRun({ const trackEvent = useTrackEvent() // TODO(BC, 08/29/23): reintroduce this analytics event when we refactor the hook to fetch data lazily (performance concern) // const { trackProtocolRunEvent } = useTrackProtocolRunEvent(runData.id) - const onResetSuccess = (createRunResponse: Run): void => { - navigate(`runs/${createRunResponse.data.id}/setup`) - } - const { cloneRun } = useCloneRun(runData.id, onResetSuccess) + const { cloneRun } = useCloneRun(runData.id) const [showSpinner, setShowSpinner] = React.useState(false) const protocolName = @@ -143,6 +140,9 @@ export function ProtocolWithLastRun({ navigate(`/protocols/${protocolId}`) } else { cloneRun() + // Navigate to a dummy setup skeleton until TopLevelRedirects routes to the proper setup page. Doing so prevents + // needing to manage complex UI state updates for protocol cards, overzealous dashboard rendering, and potential navigation pitfalls. + navigate('/runs/1234/setup') trackEvent({ name: ANALYTICS_PROTOCOL_PROCEED_TO_RUN, properties: { sourceLocation: 'RecentRunProtocolCard' }, diff --git a/app/src/organisms/OnDeviceDisplay/RobotDashboard/__tests__/RecentRunProtocolCard.test.tsx b/app/src/organisms/OnDeviceDisplay/RobotDashboard/__tests__/RecentRunProtocolCard.test.tsx index 006bf2dc43e..d9d4959814d 100644 --- a/app/src/organisms/OnDeviceDisplay/RobotDashboard/__tests__/RecentRunProtocolCard.test.tsx +++ b/app/src/organisms/OnDeviceDisplay/RobotDashboard/__tests__/RecentRunProtocolCard.test.tsx @@ -158,9 +158,10 @@ describe('RecentRunProtocolCard', () => { when(useTrackProtocolRunEvent).calledWith(RUN_ID, ROBOT_NAME).thenReturn({ trackProtocolRunEvent: mockTrackProtocolRunEvent, }) - when(useCloneRun) - .calledWith(RUN_ID, expect.anything()) - .thenReturn({ cloneRun: mockCloneRun, isLoading: false }) + vi.mocked(useCloneRun).mockReturnValue({ + cloneRun: mockCloneRun, + isLoading: false, + }) }) afterEach(() => { diff --git a/app/src/organisms/ProtocolUpload/hooks/useCloneRun.ts b/app/src/organisms/ProtocolUpload/hooks/useCloneRun.ts index 0132adf7298..41277f47e86 100644 --- a/app/src/organisms/ProtocolUpload/hooks/useCloneRun.ts +++ b/app/src/organisms/ProtocolUpload/hooks/useCloneRun.ts @@ -36,10 +36,13 @@ export function useCloneRun( 'protocols', protocolKey, ]) - Promise.all([invalidateRuns, invalidateProtocols]).catch((e: Error) => { - console.error(`error invalidating runs query: ${e.message}`) - }) - if (onSuccessCallback != null) onSuccessCallback(response) + Promise.all([invalidateRuns, invalidateProtocols]) + .then(() => { + onSuccessCallback?.(response) + }) + .catch((e: Error) => { + console.error(`error invalidating runs query: ${e.message}`) + }) }, }) const { createProtocolAnalysis } = useCreateProtocolAnalysisMutation(