Skip to content

Commit

Permalink
fix(app): fix Recent Run Protocol Card onClick routing behavior (#16094)
Browse files Browse the repository at this point in the history
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).
  • Loading branch information
mjhuff authored Aug 22, 2024
1 parent dbb1353 commit 4b897fc
Show file tree
Hide file tree
Showing 3 changed files with 16 additions and 12 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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<boolean>(false)

const protocolName =
Expand Down Expand Up @@ -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' },
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(() => {
Expand Down
11 changes: 7 additions & 4 deletions app/src/organisms/ProtocolUpload/hooks/useCloneRun.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down

0 comments on commit 4b897fc

Please sign in to comment.