From 8df42571d29ca702b66ad20867375587f6d275e8 Mon Sep 17 00:00:00 2001 From: Brian Cooper Date: Thu, 15 Oct 2020 17:45:46 -0400 Subject: [PATCH 1/3] fix(app): debounce calibraiton jog requests while others are in flight In order to prevent jog commands from queueing up, which can be sometimes considerable given network latency, do not register jog commands that are fired while another is still bending on the robot. Note: at the moment this is not attached to a visual change in the ui, as the requests generally clear in under 2 seconds, and it is often considered bad UX to change the visual for any action that takes less than two seconds. --- .../CalibratePipetteOffset/index.js | 4 +-- .../CalibratePipetteOffset/types.js | 1 + .../useCalibratePipetteOffset.js | 29 +++++++++++++++---- 3 files changed, 27 insertions(+), 7 deletions(-) diff --git a/app/src/components/CalibratePipetteOffset/index.js b/app/src/components/CalibratePipetteOffset/index.js index 674f39d5a7e..2bc7e5ddd20 100644 --- a/app/src/components/CalibratePipetteOffset/index.js +++ b/app/src/components/CalibratePipetteOffset/index.js @@ -97,7 +97,7 @@ const PANEL_STYLE_PROPS_BY_STEP: { export function CalibratePipetteOffset( props: CalibratePipetteOffsetParentProps ): React.Node { - const { session, robotName, dispatchRequests, showSpinner } = props + const { session, robotName, dispatchRequests, showSpinner, isJogging } = props const { currentStep, instrument, labware } = session?.details || {} const { @@ -120,7 +120,7 @@ export function CalibratePipetteOffset( }, [instrument]) function sendCommands(...commands: Array) { - if (session?.id) { + if (session?.id && !isJogging) { const sessionCommandActions = commands.map(c => Sessions.createSessionCommand(robotName, session.id, { command: c.command, diff --git a/app/src/components/CalibratePipetteOffset/types.js b/app/src/components/CalibratePipetteOffset/types.js index 3724c4565e8..0a7572f5571 100644 --- a/app/src/components/CalibratePipetteOffset/types.js +++ b/app/src/components/CalibratePipetteOffset/types.js @@ -16,6 +16,7 @@ export type CalibratePipetteOffsetParentProps = {| ...Array<{ ...Action, meta: { requestId: string } }> ) => void, showSpinner: boolean, + isJogging: boolean, |} export type CalibratePipetteOffsetChildProps = {| diff --git a/app/src/components/CalibratePipetteOffset/useCalibratePipetteOffset.js b/app/src/components/CalibratePipetteOffset/useCalibratePipetteOffset.js index 223055ec33e..d23a6c674c1 100644 --- a/app/src/components/CalibratePipetteOffset/useCalibratePipetteOffset.js +++ b/app/src/components/CalibratePipetteOffset/useCalibratePipetteOffset.js @@ -33,9 +33,10 @@ export function useCalibratePipetteOffset( ): [Invoker, React.Node | null] { const [showWizard, setShowWizard] = React.useState(false) - const trackedRequestId = React.useRef(null) const deleteRequestId = React.useRef(null) const createRequestId = React.useRef(null) + const jogRequestId = React.useRef(null) + const spinnerRequestId = React.useRef(null) const pipOffsetCalSession: PipetteOffsetCalibrationSession | null = useSelector( (state: State) => { @@ -45,6 +46,7 @@ export function useCalibratePipetteOffset( const [dispatchRequests] = RobotApi.useDispatchApiRequests( dispatchedAction => { + console.log('dispatched') if (dispatchedAction.type === Sessions.ENSURE_SESSION) { createRequestId.current = dispatchedAction.meta.requestId } else if ( @@ -52,21 +54,27 @@ export function useCalibratePipetteOffset( pipOffsetCalSession?.id === dispatchedAction.payload.sessionId ) { deleteRequestId.current = dispatchedAction.meta.requestId + } else if ( + dispatchedAction.type === Sessions.CREATE_SESSION_COMMAND && + dispatchedAction.payload.command.command === + Sessions.sharedCalCommands.JOG + ) { + jogRequestId.current = dispatchedAction.meta.requestId } else if ( dispatchedAction.type !== Sessions.CREATE_SESSION_COMMAND || !spinnerCommandBlockList.includes( dispatchedAction.payload.command.command ) ) { - trackedRequestId.current = dispatchedAction.meta.requestId + spinnerRequestId.current = dispatchedAction.meta.requestId } } ) const showSpinner = useSelector(state => - trackedRequestId.current - ? RobotApi.getRequestById(state, trackedRequestId.current) + spinnerRequestId.current + ? RobotApi.getRequestById(state, spinnerRequestId.current) : null )?.status === RobotApi.PENDING @@ -84,6 +92,13 @@ export function useCalibratePipetteOffset( : null )?.status === RobotApi.SUCCESS + const isJogging = + useSelector((state: State) => + jogRequestId.current + ? RobotApi.getRequestById(state, jogRequestId.current) + : null + )?.status === RobotApi.PENDING + const closeWizard = React.useCallback(() => { onComplete && onComplete() setShowWizard(false) @@ -106,7 +121,10 @@ export function useCalibratePipetteOffset( hasCalibrationBlock = false, tipRackDefinition = null, } = sessionParams - const handleStartPipOffsetCalSession: Invoker = overrideParams => { + const handleStartPipOffsetCalSession: Invoker = ( + overrideParams: $Shape | void = {} + ) => { + console.log(overrideParams) dispatchRequests( Sessions.ensureSession( robotName, @@ -132,6 +150,7 @@ export function useCalibratePipetteOffset( closeWizard={closeWizard} showSpinner={showSpinner} dispatchRequests={dispatchRequests} + isJogging={isJogging} /> ) : null, From 81fad7f5c59efe1191c36fe34c3fa9936b88225a Mon Sep 17 00:00:00 2001 From: Brian Cooper Date: Thu, 15 Oct 2020 18:27:09 -0400 Subject: [PATCH 2/3] extend test coverage for debounce and add similar logic to tlc --- .../__tests__/CalibratePipetteOffset.test.js | 56 ++++++++++++++++--- .../useCalibratePipetteOffset.js | 2 - .../__tests__/CalibrateTipLength.test.js | 49 +++++++++++++++- .../components/CalibrateTipLength/index.js | 3 +- .../components/CalibrateTipLength/types.js | 1 + .../Calibrate/CalibrateTipLengthControl.js | 16 ++++++ 6 files changed, 115 insertions(+), 12 deletions(-) diff --git a/app/src/components/CalibratePipetteOffset/__tests__/CalibratePipetteOffset.test.js b/app/src/components/CalibratePipetteOffset/__tests__/CalibratePipetteOffset.test.js index 5fe639a053f..896af13c198 100644 --- a/app/src/components/CalibratePipetteOffset/__tests__/CalibratePipetteOffset.test.js +++ b/app/src/components/CalibratePipetteOffset/__tests__/CalibratePipetteOffset.test.js @@ -42,10 +42,8 @@ describe('CalibratePipetteOffset', () => { let mockStore let render let dispatch - let mockPipOffsetCalSession: Sessions.PipetteOffsetCalibrationSession = { - id: 'fake_session_id', - ...mockPipetteOffsetCalibrationSessionAttributes, - } + let dispatchRequests + let mockPipOffsetCalSession: Sessions.PipetteOffsetCalibrationSession const getExitButton = wrapper => wrapper.find({ title: 'exit' }).find('button') @@ -72,6 +70,7 @@ describe('CalibratePipetteOffset', () => { beforeEach(() => { dispatch = jest.fn() + dispatchRequests = jest.fn() mockStore = { subscribe: () => {}, getState: () => ({ @@ -87,14 +86,19 @@ describe('CalibratePipetteOffset', () => { } render = (props = {}) => { - const { showSpinner = false } = props + const { + showSpinner = false, + isJogging = false, + session = mockPipOffsetCalSession, + } = props return mount( , { wrappingComponent: Provider, @@ -147,4 +151,42 @@ describe('CalibratePipetteOffset', () => { const wrapper = render({ showSpinner: true }) expect(wrapper.find('SpinnerModalPage').exists()).toBe(true) }) + + it('does dispatch jog requests when not isJogging', () => { + const session = { + id: 'fake_session_id', + ...mockPipetteOffsetCalibrationSessionAttributes, + details: { + ...mockPipetteOffsetCalibrationSessionAttributes.details, + currentStep: Sessions.PIP_OFFSET_STEP_PREPARING_PIPETTE, + }, + } + const wrapper = render({ isJogging: false, session }) + wrapper.find('button[title="forward"]').invoke('onClick')() + expect(dispatchRequests).toHaveBeenCalledWith( + Sessions.createSessionCommand('robot-name', session.id, { + command: Sessions.sharedCalCommands.JOG, + data: { vector: [0, -0.1, 0] }, + }) + ) + }) + + it('does not dispatch jog requests when isJogging', () => { + const session = { + id: 'fake_session_id', + ...mockPipetteOffsetCalibrationSessionAttributes, + details: { + ...mockPipetteOffsetCalibrationSessionAttributes.details, + currentStep: Sessions.PIP_OFFSET_STEP_PREPARING_PIPETTE, + }, + } + const wrapper = render({ isJogging: true, session }) + wrapper.find('button[title="forward"]').invoke('onClick')() + expect(dispatchRequests).not.toHaveBeenCalledWith( + Sessions.createSessionCommand('robot-name', session.id, { + command: Sessions.sharedCalCommands.JOG, + data: { vector: [0, -0.1, 0] }, + }) + ) + }) }) diff --git a/app/src/components/CalibratePipetteOffset/useCalibratePipetteOffset.js b/app/src/components/CalibratePipetteOffset/useCalibratePipetteOffset.js index d23a6c674c1..6b2a2512e06 100644 --- a/app/src/components/CalibratePipetteOffset/useCalibratePipetteOffset.js +++ b/app/src/components/CalibratePipetteOffset/useCalibratePipetteOffset.js @@ -46,7 +46,6 @@ export function useCalibratePipetteOffset( const [dispatchRequests] = RobotApi.useDispatchApiRequests( dispatchedAction => { - console.log('dispatched') if (dispatchedAction.type === Sessions.ENSURE_SESSION) { createRequestId.current = dispatchedAction.meta.requestId } else if ( @@ -124,7 +123,6 @@ export function useCalibratePipetteOffset( const handleStartPipOffsetCalSession: Invoker = ( overrideParams: $Shape | void = {} ) => { - console.log(overrideParams) dispatchRequests( Sessions.ensureSession( robotName, diff --git a/app/src/components/CalibrateTipLength/__tests__/CalibrateTipLength.test.js b/app/src/components/CalibrateTipLength/__tests__/CalibrateTipLength.test.js index 00539501312..c3e31fd594e 100644 --- a/app/src/components/CalibrateTipLength/__tests__/CalibrateTipLength.test.js +++ b/app/src/components/CalibrateTipLength/__tests__/CalibrateTipLength.test.js @@ -42,6 +42,7 @@ describe('CalibrateTipLength', () => { let mockStore let render let dispatch + let dispatchRequests let mockTipLengthSession: Sessions.TipLengthCalibrationSession = { id: 'fake_session_id', ...mockTipLengthCalibrationSessionAttributes, @@ -72,6 +73,7 @@ describe('CalibrateTipLength', () => { beforeEach(() => { dispatch = jest.fn() + dispatchRequests = jest.fn() mockStore = { subscribe: () => {}, getState: () => ({ @@ -87,14 +89,19 @@ describe('CalibrateTipLength', () => { } render = (props = {}) => { - const { showSpinner = false } = props + const { + showSpinner = false, + isJogging = false, + session = mockTipLengthSession, + } = props return mount( {}} dispatchRequests={jest.fn()} showSpinner={showSpinner} + isJogging={isJogging} />, { wrappingComponent: Provider, @@ -147,4 +154,42 @@ describe('CalibrateTipLength', () => { const wrapper = render({ showSpinner: true }) expect(wrapper.find('SpinnerModalPage').exists()).toBe(true) }) + + it('does dispatch jog requests when not isJogging', () => { + const session = { + id: 'fake_session_id', + ...mockTipLengthCalibrationSessionAttributes, + details: { + ...mockTipLengthCalibrationSessionAttributes.details, + currentStep: Sessions.TIP_LENGTH_STEP_PREPARING_PIPETTE, + }, + } + const wrapper = render({ isJogging: false, session }) + wrapper.find('button[title="forward"]').invoke('onClick')() + expect(dispatchRequests).toHaveBeenCalledWith( + Sessions.createSessionCommand('robot-name', session.id, { + command: Sessions.sharedCalCommands.JOG, + data: { vector: [0, -0.1, 0] }, + }) + ) + }) + + it('does not dispatch jog requests when isJogging', () => { + const session = { + id: 'fake_session_id', + ...mockTipLengthCalibrationSessionAttributes, + details: { + ...mockTipLengthCalibrationSessionAttributes.details, + currentStep: Sessions.TIP_LENGTH_STEP_PREPARING_PIPETTE, + }, + } + const wrapper = render({ isJogging: true, session }) + wrapper.find('button[title="forward"]').invoke('onClick')() + expect(dispatchRequests).not.toHaveBeenCalledWith( + Sessions.createSessionCommand('robot-name', session.id, { + command: Sessions.sharedCalCommands.JOG, + data: { vector: [0, -0.1, 0] }, + }) + ) + }) }) diff --git a/app/src/components/CalibrateTipLength/index.js b/app/src/components/CalibrateTipLength/index.js index 589c092af6d..85b6f891731 100644 --- a/app/src/components/CalibrateTipLength/index.js +++ b/app/src/components/CalibrateTipLength/index.js @@ -99,6 +99,7 @@ export function CalibrateTipLength( closeWizard, showSpinner, dispatchRequests, + isJogging, } = props const { currentStep, instrument, labware } = session?.details || {} @@ -114,7 +115,7 @@ export function CalibrateTipLength( : null function sendCommands(...commands: Array) { - if (session?.id) { + if (session?.id && !isJogging) { const sessionCommandActions = commands.map(c => Sessions.createSessionCommand(robotName, session.id, { command: c.command, diff --git a/app/src/components/CalibrateTipLength/types.js b/app/src/components/CalibrateTipLength/types.js index 39c5be30b57..5d3e8c56ff4 100644 --- a/app/src/components/CalibrateTipLength/types.js +++ b/app/src/components/CalibrateTipLength/types.js @@ -11,4 +11,5 @@ export type CalibrateTipLengthParentProps = {| ...Array<{ ...Action, meta: { requestId: string } }> ) => void, showSpinner: boolean, + isJogging: boolean, |} diff --git a/app/src/pages/Calibrate/CalibrateTipLengthControl.js b/app/src/pages/Calibrate/CalibrateTipLengthControl.js index 292eaf0a1a4..115cd042199 100644 --- a/app/src/pages/Calibrate/CalibrateTipLengthControl.js +++ b/app/src/pages/Calibrate/CalibrateTipLengthControl.js @@ -58,6 +58,7 @@ export function CalibrateTipLengthControl({ const trackedRequestId = React.useRef(null) const deleteRequestId = React.useRef(null) const createRequestId = React.useRef(null) + const jogRequestId = React.useRef(null) const [dispatchRequests] = RobotApi.useDispatchApiRequests( dispatchedAction => { @@ -68,6 +69,12 @@ export function CalibrateTipLengthControl({ calibrationSession?.id === dispatchedAction.payload.sessionId ) { deleteRequestId.current = dispatchedAction.meta.requestId + } else if ( + dispatchedAction.type === Sessions.CREATE_SESSION_COMMAND && + dispatchedAction.payload.command.command === + Sessions.sharedCalCommands.JOG + ) { + jogRequestId.current = dispatchedAction.meta.requestId } else if ( dispatchedAction.type !== Sessions.CREATE_SESSION_COMMAND || !spinnerCommandBlockList.includes( @@ -142,6 +149,13 @@ export function CalibrateTipLengthControl({ : null )?.status === RobotApi.SUCCESS + const isJogging = + useSelector((state: State) => + jogRequestId.current + ? RobotApi.getRequestById(state, jogRequestId.current) + : null + )?.status === RobotApi.PENDING + React.useEffect(() => { if (shouldOpen) { setShowWizard(true) @@ -201,6 +215,7 @@ export function CalibrateTipLengthControl({ closeWizard={handleCloseWizard} showSpinner={showSpinner} dispatchRequests={dispatchRequests} + isJogging={isJogging} /> ) : ( )} From b78194f131a35fef48404c44b1a6f01d8edb8b42 Mon Sep 17 00:00:00 2001 From: Brian Cooper Date: Thu, 15 Oct 2020 18:35:20 -0400 Subject: [PATCH 3/3] extend same functionality to deck calibration --- .../__tests__/CalibrateDeck.test.js | 50 +++++++++++++++++-- app/src/components/CalibrateDeck/index.js | 3 +- app/src/components/CalibrateDeck/types.js | 1 + .../__tests__/CalibrateTipLength.test.js | 2 +- .../RobotSettings/DeckCalibrationControl.js | 15 ++++++ 5 files changed, 66 insertions(+), 5 deletions(-) diff --git a/app/src/components/CalibrateDeck/__tests__/CalibrateDeck.test.js b/app/src/components/CalibrateDeck/__tests__/CalibrateDeck.test.js index ae0cf1a7849..24f9b5f0541 100644 --- a/app/src/components/CalibrateDeck/__tests__/CalibrateDeck.test.js +++ b/app/src/components/CalibrateDeck/__tests__/CalibrateDeck.test.js @@ -42,6 +42,7 @@ describe('CalibrateDeck', () => { let mockStore let render let dispatch + let dispatchRequests let mockDeckCalSession: Sessions.DeckCalibrationSession = { id: 'fake_session_id', ...mockDeckCalibrationSessionAttributes, @@ -74,6 +75,7 @@ describe('CalibrateDeck', () => { beforeEach(() => { dispatch = jest.fn() + dispatchRequests = jest.fn() mockStore = { subscribe: () => {}, getState: () => ({ @@ -89,14 +91,19 @@ describe('CalibrateDeck', () => { } render = (props = {}) => { - const { showSpinner = false } = props + const { + showSpinner = false, + isJogging = false, + session = mockDeckCalSession, + } = props return mount( {}} - dispatchRequests={jest.fn()} + dispatchRequests={dispatchRequests} showSpinner={showSpinner} + isJogging={isJogging} />, { wrappingComponent: Provider, @@ -149,4 +156,41 @@ describe('CalibrateDeck', () => { const wrapper = render({ showSpinner: true }) expect(wrapper.find('SpinnerModalPage').exists()).toBe(true) }) + it('does dispatch jog requests when not isJogging', () => { + const session = { + id: 'fake_session_id', + ...mockDeckCalibrationSessionAttributes, + details: { + ...mockDeckCalibrationSessionAttributes.details, + currentStep: Sessions.DECK_STEP_PREPARING_PIPETTE, + }, + } + const wrapper = render({ isJogging: false, session }) + wrapper.find('button[title="forward"]').invoke('onClick')() + expect(dispatchRequests).toHaveBeenCalledWith( + Sessions.createSessionCommand('robot-name', session.id, { + command: Sessions.sharedCalCommands.JOG, + data: { vector: [0, -0.1, 0] }, + }) + ) + }) + + it('does not dispatch jog requests when isJogging', () => { + const session = { + id: 'fake_session_id', + ...mockDeckCalibrationSessionAttributes, + details: { + ...mockDeckCalibrationSessionAttributes.details, + currentStep: Sessions.DECK_STEP_PREPARING_PIPETTE, + }, + } + const wrapper = render({ isJogging: true, session }) + wrapper.find('button[title="forward"]').invoke('onClick')() + expect(dispatchRequests).not.toHaveBeenCalledWith( + Sessions.createSessionCommand('robot-name', session.id, { + command: Sessions.sharedCalCommands.JOG, + data: { vector: [0, -0.1, 0] }, + }) + ) + }) }) diff --git a/app/src/components/CalibrateDeck/index.js b/app/src/components/CalibrateDeck/index.js index 7d00e306f00..ad150ba4d50 100644 --- a/app/src/components/CalibrateDeck/index.js +++ b/app/src/components/CalibrateDeck/index.js @@ -98,6 +98,7 @@ export function CalibrateDeck(props: CalibrateDeckParentProps): React.Node { closeWizard, dispatchRequests, showSpinner, + isJogging, } = props const { currentStep, instrument, labware } = session?.details || {} @@ -115,7 +116,7 @@ export function CalibrateDeck(props: CalibrateDeckParentProps): React.Node { }, [instrument]) function sendCommands(...commands: Array) { - if (session?.id) { + if (session?.id && !isJogging) { const sessionCommandActions = commands.map(c => Sessions.createSessionCommand(robotName, session.id, { command: c.command, diff --git a/app/src/components/CalibrateDeck/types.js b/app/src/components/CalibrateDeck/types.js index 3294fa8594d..19c6e14efed 100644 --- a/app/src/components/CalibrateDeck/types.js +++ b/app/src/components/CalibrateDeck/types.js @@ -10,4 +10,5 @@ export type CalibrateDeckParentProps = {| ...Array<{ ...Action, meta: { requestId: string } }> ) => void, showSpinner: boolean, + isJogging: boolean, |} diff --git a/app/src/components/CalibrateTipLength/__tests__/CalibrateTipLength.test.js b/app/src/components/CalibrateTipLength/__tests__/CalibrateTipLength.test.js index c3e31fd594e..0bf14a2e60e 100644 --- a/app/src/components/CalibrateTipLength/__tests__/CalibrateTipLength.test.js +++ b/app/src/components/CalibrateTipLength/__tests__/CalibrateTipLength.test.js @@ -99,7 +99,7 @@ describe('CalibrateTipLength', () => { robotName="robot-name" session={session} closeWizard={() => {}} - dispatchRequests={jest.fn()} + dispatchRequests={dispatchRequests} showSpinner={showSpinner} isJogging={isJogging} />, diff --git a/app/src/components/RobotSettings/DeckCalibrationControl.js b/app/src/components/RobotSettings/DeckCalibrationControl.js index 9b2e064e65a..2d1ee512adb 100644 --- a/app/src/components/RobotSettings/DeckCalibrationControl.js +++ b/app/src/components/RobotSettings/DeckCalibrationControl.js @@ -82,6 +82,7 @@ export function DeckCalibrationControl(props: Props): React.Node { const trackedRequestId = React.useRef(null) const deleteRequestId = React.useRef(null) const createRequestId = React.useRef(null) + const jogRequestId = React.useRef(null) const [dispatchRequests] = RobotApi.useDispatchApiRequests( dispatchedAction => { @@ -92,6 +93,12 @@ export function DeckCalibrationControl(props: Props): React.Node { deckCalibrationSession?.id === dispatchedAction.payload.sessionId ) { deleteRequestId.current = dispatchedAction.meta.requestId + } else if ( + dispatchedAction.type === Sessions.CREATE_SESSION_COMMAND && + dispatchedAction.payload.command.command === + Sessions.sharedCalCommands.JOG + ) { + jogRequestId.current = dispatchedAction.meta.requestId } else if ( dispatchedAction.type !== Sessions.CREATE_SESSION_COMMAND || !spinnerCommandBlockList.includes( @@ -124,6 +131,13 @@ export function DeckCalibrationControl(props: Props): React.Node { : null )?.status === RobotApi.SUCCESS + const isJogging = + useSelector((state: State) => + jogRequestId.current + ? RobotApi.getRequestById(state, jogRequestId.current) + : null + )?.status === RobotApi.PENDING + React.useEffect(() => { if (shouldOpen) { setShowWizard(true) @@ -214,6 +228,7 @@ export function DeckCalibrationControl(props: Props): React.Node { closeWizard={() => setShowWizard(false)} dispatchRequests={dispatchRequests} showSpinner={showSpinner} + isJogging={isJogging} /> )}