Skip to content

feat(app): Refactor labware details into labware & liquid details, accomodate stacking#17896

Merged
smb2268 merged 16 commits into
chore_release-8.4.0from
app_protocol-details-odd
Mar 28, 2025
Merged

feat(app): Refactor labware details into labware & liquid details, accomodate stacking#17896
smb2268 merged 16 commits into
chore_release-8.4.0from
app_protocol-details-odd

Conversation

@smb2268
Copy link
Copy Markdown
Contributor

@smb2268 smb2268 commented Mar 26, 2025

Fix EXEC-1220, EXEC-1223, EXEC-1303, EXEC-1304, EXEC-1305, EXEC-1092

Overview

This is a monster of a PR - sorry for the extensive changes but it's hard to break this up! At a high level, this PR does the following main things:

  1. Creates new utility getStackedItemsOnStartingDeck that utilizes the new labwareSequence field on command results to construct a starting deck state, with shape [slotName: string]: StackItem[] where StackItems are either modules or labware.
  2. Refactors both ODD and desktop protocol setup labware list view & map view to use this new utility as the source of information instead of relying on various other command parsers
  3. Combines liquid and labware details for ODD and desktop protocol setup and incorporates all related design changes
  4. Creates new modals on ODD and desktop that allow for visualization of labware in stacks

Test Plan and Hands on Testing

I've tested this with a variety of protocols with the following:

  • Stacks of TC lids loaded in both the old and new paradigms
  • Tipracks with lids on them
  • Hybrid stacks of varying labware
  • Labware with liquids
  • Hybrid stacks where some have liquids

I still need to test:

  • OT-2 labware on modules to check module in labware list item styling
  • Off deck labware

Videos of changes are here:

https://github.com/user-attachments/assets/9439eee0-eab4-4265-bf4d-64ba1bee2e11
https://github.com/user-attachments/assets/98f9bd0e-64f4-4763-b16a-197fade78094
https://github.com/user-attachments/assets/e7378773-da95-4de1-9e7c-1b6bdcd22501
https://github.com/user-attachments/assets/47ec4a6b-9f55-4ae5-b596-584461292420

Screenshots for OT-2 and Offdeck:
Screenshot 2025-03-27 at 1 08 20 PM
Screenshot 2025-03-27 at 1 04 10 PM

Changelog

  1. Creates new utility getStackedItemsOnStartingDeck that utilizes the new labwareSequence field on command results to construct a starting deck state, with shape [slotName: string]: StackItem[] where StackItems are either modules or labware.
  2. Refactor Labware list view on ODD and desktop to display labware stacks of various length and type, adding stack icon tag and liquid tag where applicable, and removing the module title from inline view
  3. Add liquids to ODD and desktop labware map view, refactor to use new utility
  4. Create new ODD screen SetupLabwareStackView which shows labware render and stacked items per slot. Open this view whenever a labware or labware line item is pressed. Move LabwareLiquidDetailModal into this folder and launch from this new view
  5. Create new desktop modal SlotDetailModal which includes both labware stack and liquid details for the given slot
  6. Remove all liquid setup screens

Review requests

Take a look at the new utility and how the screens are now organized. This was my first pass on these changes, I'm going to be cleaning things up further and consolidating views where possible as I adapt this further to accommodate stacker commands and designs

Risk assessment

Medium - changes the source of truth for starting deck state for protocol setup

NOTE: Still working on test and story updates, but this is ready for a first look if you have time!

@smb2268 smb2268 requested review from TamarZanzouri and mjhuff March 26, 2025 22:09
@smb2268 smb2268 self-assigned this Mar 26, 2025
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 26, 2025

Codecov Report

Attention: Patch coverage is 2.25564% with 1300 lines in your changes missing coverage. Please review.

Project coverage is 23.00%. Comparing base (c0cb479) to head (a476809).
Report is 14 commits behind head on chore_release-8.4.0.

Files with missing lines Patch % Lines
...s/transformations/getStackedItemsOnStartingDeck.ts 0.00% 233 Missing ⚠️
...s/ODD/ProtocolSetup/ProtocolSetupLabware/index.tsx 0.00% 180 Missing ⚠️
...vices/ProtocolRun/SetupLabware/SlotDetailModal.tsx 0.00% 162 Missing ⚠️
...tup/ProtocolSetupLabware/SetupLabwareStackView.tsx 0.00% 133 Missing ⚠️
...vices/ProtocolRun/SetupLabware/LabwareListItem.tsx 0.00% 131 Missing ⚠️
...vices/ProtocolRun/SetupLabware/SetupLabwareMap.tsx 0.00% 98 Missing ⚠️
...ices/ProtocolRun/SetupLabware/SetupLabwareList.tsx 0.00% 75 Missing ⚠️
...ProtocolSetupLabware/LabwareLiquidsDetailModal.tsx 0.00% 73 Missing ⚠️
...tocolSetup/ProtocolSetupLabware/LabwareMapView.tsx 0.00% 57 Missing ⚠️
app/src/molecules/LabwareStackContents/index.tsx 0.00% 52 Missing ⚠️
... and 12 more
Additional details and impacted files

Impacted file tree graph

@@                   Coverage Diff                   @@
##           chore_release-8.4.0   #17896      +/-   ##
=======================================================
- Coverage                24.59%   23.00%   -1.59%     
=======================================================
  Files                     2935     2945      +10     
  Lines                   225055   226728    +1673     
  Branches                 18918    18860      -58     
=======================================================
- Hits                     55344    52164    -3180     
- Misses                  169699   174553    +4854     
+ Partials                    12       11       -1     
Flag Coverage Δ
protocol-designer 18.81% <2.25%> (+0.03%) ⬆️
step-generation 4.35% <0.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...rc/molecules/LiquidDetailCard/LiquidDetailCard.tsx 0.00% <ø> (ø)
app/src/molecules/OddModal/types.ts 100.00% <ø> (ø)
...ModalContainer/modals/ConfirmMissingStepsModal.tsx 0.00% <ø> (ø)
...anisms/ODD/ProtocolSetup/ProtocolSetupSkeleton.tsx 0.00% <ø> (ø)
app/src/organisms/ODD/ProtocolSetup/index.ts 0.00% <ø> (ø)
app/src/organisms/ODD/ProtocolSetup/types.ts 100.00% <ø> (ø)
...sources/runs/hooks/useRequiredSetupStepsInOrder.ts 0.00% <ø> (ø)
app/src/redux/protocol-runs/reducer/setup.ts 0.00% <ø> (ø)
app/src/redux/protocol-runs/types/setup.ts 100.00% <ø> (ø)
components/src/atoms/ListItem/index.tsx 100.00% <100.00%> (ø)
... and 28 more

... and 99 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Copy Markdown
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good overall but I think it has a problematic reverse() in there.

Is there any chance that some of these kind of gross size constants can be less gross, or more constant?

liquidsByIdForLabware: LabwareByLiquidId
}

const HIDE_SCROLLBAR = css`
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is in here a couple times - can we put this in components somewhere?

return (
<Flex
flexDirection={DIRECTION_COLUMN}
height={isOnDevice ? '23.70375rem' : '26rem'}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a weird number... can it be a nicer rem fraction? can it be a constant?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I think the number is nicer in PX but I'll see what I can do. A lot of these constants are for fixed height modals that design has introduced like "350px" which doesn't translate nicely

Comment thread app/src/transformations/commands/transformations/getStackedItemsOnStartingDeck.ts Outdated
Comment thread app/src/transformations/commands/transformations/getStackedItemsOnStartingDeck.ts Outdated
@sfoster1
Copy link
Copy Markdown
Member

Oh also maybe better as future work but do you think we could give LabwareThumbnail a param that's like viewBoxStyle='tight'|'with-margin' or something instead of doing the viewbox math at the call site every time? that will make it easier to handle the v3 transition

height={isOnDevice ? '23.70375rem' : '26rem'}
overflowY="auto"
css={HIDE_SCROLLBAR}
minWidth="10.313rem"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one probably too

)
return labwareOnAdapter != null ? null : (
<StyledText
width="6.25rem"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a constant for this?

Comment thread app/src/organisms/Desktop/Devices/ProtocolRun/SetupLabware/SetupLabwareMap.tsx Outdated
Comment thread app/src/organisms/Desktop/Devices/ProtocolRun/SetupLabware/SetupLabwareMap.tsx Outdated
Copy link
Copy Markdown
Contributor

@TamarZanzouri TamarZanzouri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gave some feedback! over all looks good!

@smb2268 smb2268 marked this pull request as ready for review March 28, 2025 16:55
@smb2268 smb2268 requested review from a team as code owners March 28, 2025 16:55
Copy link
Copy Markdown
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing that stuff! Looks good to me!

Copy link
Copy Markdown
Contributor

@TamarZanzouri TamarZanzouri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exciting! Nice job!

@smb2268 smb2268 merged commit 8b5d76a into chore_release-8.4.0 Mar 28, 2025
66 of 67 checks passed
@smb2268 smb2268 deleted the app_protocol-details-odd branch March 28, 2025 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants