Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(app): Wire up lpc redesign #17850

Merged
merged 38 commits into from
Mar 21, 2025
Merged

feat(app): Wire up lpc redesign #17850

merged 38 commits into from
Mar 21, 2025

Conversation

mjhuff
Copy link
Contributor

@mjhuff mjhuff commented Mar 21, 2025

Closes EXEC-1297, EXEC-1307, and EXEC-1306.

Partially closes EXEC-1354

Overview

This PR wires up the redesigned LPC flows to the app, both the desktop and ODD, removing the FF. There may or may not be several bug fixes included given time constraints. Given the size of this PR (sorry), I think it's best to view it commit by commit. Explanations provided for each commit. See Review Requests.

Screen.Recording.2025-03-21.at.1.12.02.PM.mov
Screen.Recording.2025-03-21.at.1.18.56.PM.mov

3e85337 - don't force users to LPC if they have all LS offsets already. I start the work here, and this is fixed when I QA'd it in later commits.

06324f6 - Ignore this commit. The implementation is bad, and I greatly simplify this later. I do make it worse first, though in some follow up commits.

03707c7 - Add selectors that are required for some run setup copy/behavior. These are expanded and fixed in later commits.

0b2dd17 - Quick type fix.

7267cce - Begins adding the scaffolding for applying offsets. I decide to include some fixes for 06324f6 here too for some reason, but it's still broken at this point.

5eb830e - Ignore, after clarifying with Design, we disable the "LPC" button entirely if we don't need to LPC.

7288573 - More selectors for run setup.

04ef356 - If a user decides they want to use the cloned run offsets instead of the database offsets, inject those into the store.

ce9d8ad - Make the LabwareOffsetsTable a shared app component. Just the ODD here, desktop later.

16d227c - Implements the database offsets in quick transfer. No more run scraping.

b83257d - The big one. Removes the feature flag and ensures we only scrape offsets for the OT-2.

a0efcac - test fixing/linting

d002c6b - Greatly improve the component structure of the labware offsets conflict modal, and fixes some ODD copy issues in the modal.

2427475 - Refactor QT a bit. We don't actually need to inject offsets at all, since the user has to confirm offsets, anyway.

7d198f6 - Correct some of the selector counting logic utilized by copy/render behavior.

d37bd91 - Fixes the OT-2 not launching LPC and run setup not popping the setup confirmation modal for the OT-2 if offsets weren't confirmed.

50adb3e - Automatically apply offsets if there's no LPC-able labware.

e7c9fbe - There isn't any Figma-specced loading state in the Designs to handle all the network requests we're doing, but after talking to Design, we'll just show the skeleton on run setup for the ODD until those requests have settled.

6fc3ed7 - Ok, now the offsets timestamp logic is functional and easier to follow. It still gets refined later.

bd73279 - We only want to show one entry for a labwareUri, locationSequence combo, not as many as there are labware.

c372509 - This isn't specced in Figma, but the historical run log needed updating. We now show offsets that have 0,0,0 vector values too, since setting those offsets is a deliberate choice.

caf67dd - Clean up the dispatch logic for the deck config.

0f5ed78 - Woops, looking at a historical run could pop the offsets conflict modal.

126f53a - Fixes some odd bugs by making the code a bit DRYer.

742044f - Added copy support for passing in modules to the old-style of display text.

b243c2c - Fixes a logic issue.

04152e6 - Dispatching the conflict timestamp can update the store's conflict resolution (potentially) given the outcome.

592637d - When users migrate to 8.4, we don't want to make them LPC for default offsets if they have all the location offsets already.

b52ba45 - See ODD video. The launch LPC button shouldn't clip the offsets table, and the vector labels should never overflow.

Test Plan and Hands on Testing

Ran a bunch of different protocols, covering the following test cases:

  • Offset state correctly reflects a conflict when run-initialized offsets conflict with the current offsets.
  • The offset conflict modal renders appropriately.
  • The run starts with appropriate offsets if there's an offset conflict.
  • Navigating away and back from run setup on the desktop app reflects correct LPC state.
  • No pipettes or LPC-able labware = no need to LPC.
  • If all offsets are hardcoded, apply them correctly.
  • As many permutations of location-specific, default, and hardcoded copy and design do the right thing... at least as many as I could conceive....so many.
  • Network requests that shouldn't happen on a historical run don't.
  • Quick Transfer works with the new offsets.
  • The OT-2 labware tables and copy still work.
  • The OT-2 LPC flows still work.
  • The "apply historical run" option on the ODD is gone.
  • The "apply offsets" reticle/behavior is removed for the Flex but preserved for the OT-2.
  • You don't have to configure default offsets if you have all location-specific offsets ready (the case when upgrading to 8.4 and cloning a run).
  • Labware renders on the deck as expected during LPC ows.
  • Hardcoded offsets aren't added to the run record.

I do need to add extensive testing, and it's on my TODO list after a few higher priority tickets, see EXEC-1352.

Changelog

  • Wired up the Redesigned LPC to the desktop and ODD.
  • Removed FFs.
  • The historical runs offset tabs now sorts offsets and supports more informative deck labels.

Review requests

  • None really. It's a lot of code, but it's pretty straightforward and I tested it extensively. I had to make personal decisions about error handling when network requests failed, and I always defaulted to not applying offsets and popping a snackbar. This would block users from starting a run, but given the spirit of the Redesign, it seems appropriate not to permit the run starting if the offsets aren't correct. Let me know if you disagree!

Risk assessment

11/10. Everyone gets to enjoy the new LPC now!

mjhuff added 30 commits March 20, 2025 20:12
@mjhuff mjhuff requested review from sfoster1, SyntaxColoring and a team March 21, 2025 17:24
@mjhuff mjhuff requested a review from a team as a code owner March 21, 2025 17:24
@mjhuff mjhuff requested review from smb2268 and removed request for a team March 21, 2025 17:24
Copy link
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.

Love the implementation but can't we get the database ids of offsets from run record?

Copy link

codecov bot commented Mar 21, 2025

Codecov Report

Attention: Patch coverage is 0.17162% with 1745 lines in your changes missing coverage. Please review.

Project coverage is 23.27%. Comparing base (fa3180d) to head (b52ba45).
Report is 8 commits behind head on edge.

Files with missing lines Patch % Lines
...dux/protocol-runs/selectors/lpc/labware/offsets.ts 0.00% 144 Missing ⚠️
...rc/organisms/LabwareOffsetsConflictModal/index.tsx 0.00% 128 Missing ⚠️
...tocolRun/SetupLabwarePositionCheck/OT2SetupLPC.tsx 0.00% 110 Missing ⚠️
...rc/redux/protocol-runs/selectors/lpc/transforms.ts 0.00% 110 Missing ⚠️
...sitionCheck/LPCFlows/useOffsetConflictTimestamp.ts 0.00% 109 Missing ⚠️
...arePositionCheck/FlexSetupLPC/LPCSetupFlexBtns.tsx 0.00% 89 Missing ⚠️
...s/Desktop/Devices/ProtocolRun/ProtocolRunSetup.tsx 0.00% 74 Missing ⚠️
...reducer/transforms/lpc/updateLPCLabwareInfoFrom.ts 0.00% 71 Missing ⚠️
app/src/redux/protocol-runs/reducer/lpc.ts 0.00% 69 Missing ⚠️
...lSetup/ProtocolSetupOffsets/SetupOffsetsHeader.tsx 0.00% 65 Missing ⚠️
... and 63 more
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #17850      +/-   ##
==========================================
- Coverage   23.44%   23.27%   -0.17%     
==========================================
  Files        2892     2913      +21     
  Lines      222553   223882    +1329     
  Branches    18939    19118     +179     
==========================================
- Hits        52171    52109      -62     
- Misses     170371   171762    +1391     
  Partials       11       11              
Flag Coverage Δ
protocol-designer 18.79% <0.17%> (-0.09%) ⬇️
step-generation 4.33% <0.05%> (-0.06%) ⬇️

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

Files with missing lines Coverage Δ
app/src/organisms/Desktop/Devices/hooks/index.ts 0.00% <ø> (ø)
...nisms/LabwarePositionCheck/LPCContentContainer.tsx 0.00% <ø> (ø)
...ganisms/LabwarePositionCheck/LPCFlows/LPCFlows.tsx 0.00% <ø> (ø)
...p/src/organisms/LabwarePositionCheck/OffsetTag.tsx 0.00% <ø> (ø)
...LabwarePositionCheck/hooks/useLPCCommands/index.ts 0.00% <ø> (ø)
.../LPCLabwareDetails/DefaultLocationOffset/index.tsx 0.00% <ø> (ø)
...rotocolSetupParameters/ProtocolSetupParameters.tsx 0.00% <ø> (ø)
...s/ODD/RobotSettingsDashboard/RobotSettingsList.tsx 0.00% <ø> (ø)
app/src/redux/config/actions.ts 0.00% <ø> (ø)
app/src/redux/config/constants.ts 5.55% <ø> (+0.29%) ⬆️
... and 81 more

... and 8 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.

@mjhuff
Copy link
Contributor Author

mjhuff commented Mar 21, 2025

Love the implementation but can't we get the database ids of offsets from run record?

Yea we certainly can!

EDIT: Woops, chatting with @SyntaxColoring , it seems like we can't. These are different ids.

@mjhuff mjhuff merged commit 394188c into edge Mar 21, 2025
55 of 56 checks passed
@mjhuff mjhuff deleted the app_wire-up-lpc-redesign branch March 21, 2025 19:57
@mjhuff mjhuff changed the title App wire up lpc redesign feat(app): Wire up lpc redesign Mar 21, 2025
SyntaxColoring added a commit that referenced this pull request Mar 21, 2025
…17832)

## Overview

The client-side half of EXEC-1342.

## Test Plan and Hands on Testing

* [x] I guess at this point we can turn on the feature flag and try this
out by entering LPC on one client and changing offsets on another?
  * Tested with a merge of this PR, #17853, #17817, and #17850.

## Changelog

The client was polling `POST /labwareOffsets/searches`. This switches it
to only happen when the client receives a notification from the server
that offsets have changed, with a polling fallback. I'm entirely
cargo-culting the patterns from the existing `useNotify___()` hooks.

## Risk assessment

Medium. I don't think we have a good way to integration-test this kind
of thing, so there are risks of things like missed updates, or
inadvertent polling.
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.

2 participants