Skip to content

feat(app): Remove labware logic from deck conflict resolution checker #18676

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

Merged
merged 5 commits into from
Jun 24, 2025

Conversation

smb2268
Copy link
Contributor

@smb2268 smb2268 commented Jun 18, 2025

fix EXEC-1470

Overview

This PR refactors the deck conflict resolution logic in Protocol Setup Deck Hardware section to no longer find the labware that will eventually need to be placed in an empty slot. Instead, we'll display the specific slot (left, right or center) that needs to be present if there is a conflict.

Test Plan and Hands on Testing

Look at protocol setup for a variety of configurations when your deck config does and doesn't match what is needed for a protocol. Especially:

  1. Having various things configured in a slot that you will load or move labware to at some point in the protocol
  2. Having Flex Stacker related conflicts

Changelog

  1. Removes getLabwareInSlots util which is not very accurate and stop showing that info in the deck hardware section since labware can be handled in the next setup tab
  2. Removes logic that sometimes showed the current fixture instead of the required fixture in the deck hardware list
  3. Stop doubly showing Flex Stacker in the deck hardware section, which was happening because stacker provides multiple addressable areas
  4. Adds some null protection to getStackedItemsOnStartingDeck that isn't super relevant to these changes but I noticed the issue when testing them

Needed to address in a follow-up:
Adding images for these new slot fixtures once we have them from design

Review requests

Look over the changes and test this out if you can! I've done a good amount of testing but deck config has a lot of possible variations so the more testing the better.

Risk assessment

Medium

@smb2268 smb2268 requested a review from a team as a code owner June 18, 2025 17:57
@smb2268 smb2268 requested review from jerader and removed request for a team June 18, 2025 17:57
@smb2268 smb2268 requested review from sfoster1, TamarZanzouri and CaseyBatten and removed request for jerader June 18, 2025 17:58
Copy link

codecov bot commented Jun 18, 2025

Codecov Report

Attention: Patch coverage is 0% with 56 lines in your changes missing coverage. Please review.

Project coverage is 25.65%. Comparing base (642f5ff) to head (0182371).
Report is 69 commits behind head on edge.

Files with missing lines Patch % Lines
...rotocolRun/SetupModuleAndDeck/SetupFixtureList.tsx 0.00% 15 Missing ⚠️
app/src/resources/deck_configuration/utils.ts 0.00% 14 Missing ⚠️
...Setup/ProtocolSetupModulesAndDeck/FixtureTable.tsx 0.00% 11 Missing ⚠️
...s/transformations/getStackedItemsOnStartingDeck.ts 0.00% 6 Missing ⚠️
shared-data/js/fixtures.ts 0.00% 6 Missing ⚠️
...op/Devices/ProtocolRun/SetupModuleAndDeck/utils.ts 0.00% 3 Missing ⚠️
...ms/LocationConflictModal/LocationConflictModal.tsx 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #18676      +/-   ##
==========================================
- Coverage   25.72%   25.65%   -0.08%     
==========================================
  Files        3280     3284       +4     
  Lines      283602   285655    +2053     
  Branches    28598    29729    +1131     
==========================================
+ Hits        72964    73291     +327     
- Misses     210611   212337    +1726     
  Partials       27       27              
Flag Coverage Δ
protocol-designer 19.07% <0.00%> (-0.03%) ⬇️
step-generation 5.28% <0.00%> (+0.42%) ⬆️

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

Files with missing lines Coverage Δ
...p/Devices/ProtocolRun/SetupModuleAndDeck/index.tsx 0.00% <ø> (ø)
...uration/hooks/useDeckConfigurationCompatibility.ts 0.00% <ø> (ø)
app/src/transformations/analysis/index.ts 0.00% <ø> (ø)
...ms/LocationConflictModal/LocationConflictModal.tsx 0.00% <0.00%> (ø)
...op/Devices/ProtocolRun/SetupModuleAndDeck/utils.ts 0.00% <0.00%> (ø)
...s/transformations/getStackedItemsOnStartingDeck.ts 0.00% <0.00%> (ø)
shared-data/js/fixtures.ts 49.24% <0.00%> (-13.52%) ⬇️
...Setup/ProtocolSetupModulesAndDeck/FixtureTable.tsx 0.00% <0.00%> (ø)
app/src/resources/deck_configuration/utils.ts 0.00% <0.00%> (ø)
...rotocolRun/SetupModuleAndDeck/SetupFixtureList.tsx 0.00% <0.00%> (ø)

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

@smb2268 smb2268 requested a review from vegano1 June 23, 2025 16:25
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.

Looks like good simplification overall, though there's a couple things around checking for exactly-one conflicts that makes me raise my eyebrow a bit

cutoutFixtureId === THERMOCYCLER_V2_FRONT_FIXTURE &&
missingLabwareDisplayName != null
compatibleCutoutFixtureIds.length === 1 &&
Copy link
Member

Choose a reason for hiding this comment

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

this feels a bit fragile, does it need to be exactly 1? or does it need to be 1 or more?

Copy link
Member

Choose a reason for hiding this comment

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

like could it be compatibleCutoutFixtureIds.some(fixtureId => SINGLE_SLOT_FIXTURES.includes(fixtureId)) instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe that would also work, but I do think it will always be exactly 1 and this is closer to the logic that was being used to create that missingLabwareDisplayName variable. I can change it though and verify it still works as expected!

@@ -614,6 +615,12 @@ export function getFixtureDisplayName(
FLEX_STACKER_MODULE_V1
)} in USB-${usbPortNumber} and magnetic block`
: `${getModuleDisplayName(FLEX_STACKER_MODULE_V1)} and magnetic block`
case SINGLE_CENTER_SLOT_FIXTURE:
return 'Center slot'
Copy link
Member

Choose a reason for hiding this comment

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

do these need translations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should talk to Anthony about this before release. We'd made a call when doing the first iteration of translation work that deck config should not be translated, but maybe we want to revisit that

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.

Looks good to me!

@smb2268 smb2268 merged commit a60acd2 into edge Jun 24, 2025
45 of 46 checks passed
@smb2268 smb2268 deleted the working-deck-config-help branch June 25, 2025 15:22
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