refactor(app): Refactor LPC flows in preparation for LPC redesign #17279
+4,637
−33
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… iterating over commands
Move redux out of the presentation layer and introduce more of the commonly used boilerplate. The intention is to shift more and more state into redux.
A first rough pass at splitting data from presentation. This requires a good bit of typing clean up as well. A couple params from legacy lpc flows are made optional.
…lows Because the Flex/OT-2 flows are controlled outside of the LPC presentation layer, we don't need any of the conditional logic here.
TerseOffsetTable is used in other places in the app outside of LPC, so it should be an organism.
We already have a global util that does the same thing.
We iterate over the same command set a lot to get labware definitions. We can just do this once.
About half the commands were injected into the presentation and half of them were not, so now we inject all of them. All commands are injected via a singular hook, useLPCCommands. There's a good bit of cleanup still left to do, but this commit is the first step.
Why do many when one does the trick?
The commands utilized by the LPC handlers are largely shared and should be consolidated and given structure so it's easier to reason about them, since we'll continually need to support new modules in LPC. This commit inadvertently fixes a bug in which LPC assumes certain axes were homed before they may actually be in practice.
Some LPC steps contained pseudo components that really should be their own components. This helps React Fiber properly optimize renders, too.
A couple components are now not shared in order to keep render control flow simpler. This just moves them out of the shared dir and cleans up a bit of their CSS styling in the process.
There are no longer any base components that are shared between steps, so we can nix the directory.
General utils are no longer generally used and should be relocated appropriately for better namespacing.
We don't actually use tip pickup offset data in the Flex LPC flows, since we use a probe (and don't pick up tips).
Doing so gives us more granular control over when to show robot in motion views.
mjhuff
force-pushed
the
app_split-data-presentation-lpc
branch
from
f413fb5 to
47e1ac7
Compare
mjhuff
force-pushed
the
app_split-data-presentation-lpc
branch
from
47e1ac7 to
3ee2308
Compare
sfoster1
reviewed
Jan 15, 2025
| @@ -77,7 +75,7 @@ export function ProtocolSetupOffsets({ | |||
| const nonIdentityOffsets = getLatestCurrentOffsets(sortedOffsets) | |||
| return ( | |||
| <> | |||
| {isNewLpc ? null : LPCWizard} | |||
| {LPCWizard} | |||
There was a problem hiding this comment.
these seem like a weird couple lines, can we collapse this into either no logic or explicit alternation logic, like {LPCWizard ?? (...)}
| export interface LPCWizardFlexProps extends Omit<LPCFlowsProps, 'robotType'> {} | ||
|
|
||
| export function LPCWizardFlex(props: LPCWizardFlexProps): JSX.Element { | ||
| const initialState = useLPCInitialState({ ...props }) |
There was a problem hiding this comment.
For the debuggability that we're using redux for, IMO this really wants to go in the global store keyed by run id or something. Having this owned by the react component itself makes it a lot less useful.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes EXEC-1070
Overview
This PR effectively rewrites
LabwarePositionCheck, separating the data layer from the presentation layer and re-architecting both layers to be more in line with the direction Design is heading with the LPC Redesign.Some Disclaimers
First and foremost, this PR intentionally separates the OT-2 from the Flex LPC flows, and it effectively does not touch the OT-2 flows. All changes to LPC are made for the Flex flows entirely. There is one very very small change to the Legacy flows, see "Risk assessment".
Note that no functional changes are intended to be introduced, but in practice, a few bugs I noticed in the old code were fixed along the way as a side effect of rewriting the code. Also, this PR is not meant to polish all the existing LPC logic or be a final architectural product. Some old decisions are kept that should definitely change (ex, the role of
stepsin the old/this version of LPC), but it's probably not worth investing the time until designs are more concrete. As a last note, there are intentionally no tests (yet!!), because the presentation layer is very likely to change, and I'm not yet convinced the data layer is solidified enough to add testing. It's a lot of time to invest for little payoff.High Level Overview of Changes
To best reason about the new LPC, it's probably best to start with
LPCFlows, the touch point for of the new LPC API. Below are the high-level, TLDR major changes followed by some additional explanation for the more salient changes/thoughts.On a high-level, here are the major ways the new code differs from the old code:
LPC is split into the OT-2 flows,
LegacyLabwarePositionCheck, and the Flex flows,LabwarePositionCheck. Effectively no changes are made to the OT-2 flows.All state and commands are injected into LPC components.
Almost all state, including all initial state, is contained in Redux.
We get labware definitions from an HTTP resource as opposed to scanning commands and building it ourselves.
Protocol command scanning is eliminated except for one instance.
Component render control flow is consolidated, and no component returns
nullnow.Each LPC step view renders what's on the tin. There's no more conditional rendering of different views in each step.
N A M E S P A C I N G.
Increased encapsulation around components.
An introduction of encapsulation for commands and module commands. This seems important, because we need a clear pattern when it comes to supporting new modules in LPC.
Lots of removed cruft. No more unused variables, local utilities that duplicate utilities in
local-resources, redeclaration of variables/subroutines for functionality that exists elsewhere, and miscellaneous duplicated logic.Error handling is consolidated.
Jogging is now debounced! No more breaking labware by overjogging!
More comments on things that I felt needed comments.
Some React Fiber/general rendering optimizations.
Eliminated all robot commands that could occur as side effects.
A couple bug fixes:
Some Things Worth Mentioning in More Detail
Everything Goes into Redux
One of the nice and also not nice things about React is that it's very unopinionated, especially around issues of state management. We have Redux in the app, we also have React context, we have local
useState, we have prop drilling, we have hooks, and we have hook wrappers around React Query. While there are some quasi-objective instances in which one state management solution is more correct than another, that isn't always the case.One thing that worked well in Error Recovery and Drop Tip wizard was the use of a single object to pass around state. However, there's a good argument that if you probably want Redux for some of that state, should all your state go into Redux? There's arguments on both sides, but I think doing so for LPC does make working the API a bit simpler (see next paragraph), and keeping things memoized becomes the job of a third party library instead of needing to roll our own solution (and potentially introduce odd bugs because of dependency lists including/missing certain things). This PR does not add the LPC store to the app Redux store, but it could, see review comments.
The major advantage of using the store will become more apparent with the redesign: it will be simpler to promote labware to first-class citizens, storing, updating, and deriving all sorts of data associated with a specific labware URI. Additionally, debugging will be made significantly easier.
No Commands Emitted as Side-Effects
Commands emitted as side effects often lead to unexpected interactions, especially once flows start to get more complicated. When looking at other wizard flows, the ones that execute commands purely through CTAs lead to less unexpected behavior, especially as the flows evolve, since keeping on top of interactions is much more straightforward. This does lead to the commands API being a bit more complex than the alternative (state driven conditional logic is more of the responsibility of the commands themselves than the presentation layer), but I think the benefits are worth it.
Stepsare Part of the Old Data Model (and Make the Refactor Awkward)It seems that the original intention of
stepswas to act as a top-level shared prop object, but it's at odds with the new approach to prop drilling.Stepstreats the "step" as the first-class citizen, which conflicts with where we want to end up by the end of the redesign, with "labware" as the first class citizen. It would be a serious functional refactor (it's probably the most important change of the redesign from a FE perspective) to include it in this PR, so it's excluded.All of this is to say that there's a lot of weird
stepinjection, especially into commands, that feels awkward because it is. It's where the old LPC data model meets the new one, and a lot of this will feel much better when labware becomes the first-class citizen.Test Plan and Hands on Testing
Review requests
Risk assessment
very very low - see the diff for files in
LegacyLPCFlows. It's just marking a couple props as optional, so the adapter that injects necessary props in the modern LPC flows component works.