fix(app): debounce calibration jog requests while others are in flight

[b-cooper]

Overview

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.

Changelog

• debounce jog requests in TLC, POC, and Deck Calibration such that any jog click/keypress will be ignored if there is already an unresolved (active) jog request in progress on the robot.

Review requests

• Try out all panels that involve jogging in TLC, POC, and Deck Calibration. You shouldn't be able to queue up jog requests.
• Does the jogging experience still work for you?

Risk assessment

medium, effect the feeling of many calibration flows

[sfoster1]

This absolutely rules and I suspect addresses a really common mistake that users make. One thing that we might want to do in a followup though is make a hook api or something (I have bought a new hammer and now everything looks like a nail) because there's some duplicated logic here

[amitlissack]

Do you think that perhaps it would be possible to solve the multiple inflight jog commands on the backend using ThreadedAsyncLock.forbid?

Way back we added a feature to the ThreadedAsyncLock on the backend. You can use forbid method which will raise an exception if another thread is trying to enter the critical section. This was done to try and eliminate the queuing and various deadlocks in RPC.

[sfoster1]

Way back we added a feature to the ThreadedAsyncLock on the backend. You can use forbid method which will raise an exception if another thread is trying to enter the critical section. This was done to try and eliminate the queuing and various deadlocks in RPC.
Do you think that perhaps it would be useful to solve the multiple inflight jog commands on the backend?

I think it wouldn't hurt to also do that; but I think legitimately the client is the right place to implement this from a UX perspective, because when implemented in the client we get to lock out the input for the full duration of the request including the internal-to-the-app parts, and it means the requests just never get sent. Implementing it in the server means the app would continue to send all the requests, and the server would respond with 429s or whatever. I think that's a good backup, but the client is a good place for the primary implementation.

[mcous]

Does this logic belong in the UI components? Could it be a part of createSessionCommandEpic instead?

Also agree with Amit that "don't accept multiple jog requests" is a server concern in the long run. The client should make a best effort to avoid that, but relying on the client to avoid creating an "error" condition on the server is inherently less reliable

[sfoster1]

Does this logic belong in the UI components? Could it be a part of createSessionCommandEpic instead?

Also agree with Amit that "don't accept multiple jog requests" is a server concern in the long run. The client should make a best effort to avoid that, but relying on the client to avoid creating an "error" condition on the server is inherently less reliable

In terms of reliability, the server absolutely should also verify this to avoid the error condition of queuing up multiple jogs, but I think from a responsiveness standpoint the client can absolutely consider it as well. Consider for instance if we want to add ui for the button disabling - that would absolutely need the client's request state to be as responsive as possible.

[b-cooper]

@mcous it could absolutely be a part of the createSessionCommandEpic. I think that could be the next step; collect the commands that we would want to debounce (movement related) and have the epic handle them accordingly.

As far as this logic being a part of a UI component, I agree that it's tangential to purely UI concerns, so I could also see it handled in a hook as a way of partitioning it into its own block of functionality and keeping the component files cleaner.

For now though, the proximate question is, does this UX feel like an improvement? We're going to need to touch this code very soon (next week) with the Cal Check updates, and I think that would be a perfect time to solidify this functionality into a cleaner abstraction, whether that's in the epic or a hook.

[mcous]

I can't find any tests for this logic, should we have some? Seems like the crux of the whole thing.

I was trying to think if there's a way to make a getIsJogging(state, robotName) selector, which I think would be a more testable / straightforward solution, but I'm not sure there's quite enough info in state at the moment to do that nicely

[mcous]

Code looks like it will work, but it's sorta setting off alarms in my head because the actual logic that determines if the robot is jogging isn't covered by tests and seems more appropriate in epic(s), selector(s), or hook(s).

Approved due to the "imminent" reworking in case there's an immediate need for this functionality. If, however, there isn't an immediate need for this, I wonder if it would make sense to skip right to the "rework"

[b-cooper]

@mcous I completely agree that this feature will work better in the epic(s), selector(s), hook(s). I think that given a little bit of spiked out planning, we may even be able to make that abstraction work for us in more cases than just jogging within a calibration flow.

This first, albeit not-very-abstract or elegant solution, allows us to conduct our user tests with this wanted functionality despite having a better abstraction in the short-term cooker (instapot?).

I think with the "imminent" reworking next week, we should be able to sculpt a more robustly tested, better-organized, and reusable abstraction that will wash out the code smell that we're all aware of with this PR.