refactor(shared-data): handle multiple liquids in one well by combining them into a third mixed liquid #20359
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## edge #20359 +/- ##
==========================================
+ Coverage 56.76% 57.14% +0.37%
==========================================
Files 3658 3658
Lines 304224 305071 +847
Branches 42798 43566 +768
==========================================
+ Hits 172698 174323 +1625
+ Misses 131312 130532 -780
- Partials 214 216 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
mjhuff
left a comment
mjhuff
left a comment
There was a problem hiding this comment.
The implementation looks good to me. The way we visualize liquid combos is more of a Design-level decision, and if we do this in PD, we should keep the pattern consistent anyway.
A few cleanup comments - nice work!
|
|
||
| import type { LoadLiquidRunTimeCommand, RunTimeCommand } from '../../command' | ||
|
|
||
| export const consolidateSharedWells = ( |
There was a problem hiding this comment.
| export const consolidateSharedWells = ( | |
| const consolidateSharedWells = ( |
Let's move this function beneath the major exported util, too.
| interface WellVolumeEntry { | ||
| labwareId: string | ||
| volume: number | ||
| liquidId: string | ||
| } |
There was a problem hiding this comment.
We keep our type interfaces at module scope.
| for (const [liquidId, labwareArray] of Object.entries( | ||
| liquidsByIdForLabware | ||
| )) { | ||
| for (const labware of labwareArray) { | ||
| for (const [well, volume] of Object.entries(labware.volumeByWell)) { | ||
| if (!wellToLabwareVolumes[well]) wellToLabwareVolumes[well] = [] | ||
| wellToLabwareVolumes[well].push({ | ||
| labwareId: labware.labwareId, | ||
| volume, | ||
| liquidId, | ||
| }) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
It's a bit tricky parsing nested for...const blocks. We generally use forEach and the JS helper methods, so this would perhaps be a bit more JS semantic...I think this is equivalent:
Object.entries(liquidsByIdForLabware).forEach(([liquidId, labwareArray]) => {
labwareArray.forEach(labware => {
Object.entries(labware.volumeByWell).forEach(([well, volume]) => {
wellToLabwareVolumes[well] ??= []
wellToLabwareVolumes[well].push({ labwareId: labware.labwareId, volume, liquidId })
})
})
})
| const overlappingWells = Object.entries(wellToLabwareVolumes).filter( | ||
| ([, entries]) => entries.length > 1 | ||
| ) | ||
| const consolidatedWells: LabwareByLiquidId = {} | ||
|
|
||
| for (const [well, entries] of overlappingWells) { | ||
| let totalVolume = 0 | ||
| let labwareIdName = '' | ||
| const liquidIds = entries.map(e => e.liquidId).sort() | ||
| const mixedLiquidId = `mixed-${liquidIds.join('-')}` | ||
|
|
||
| for (const entry of entries) { | ||
| totalVolume += entry.volume | ||
| labwareIdName = entry.labwareId | ||
| } | ||
| if (!consolidatedWells[mixedLiquidId]) { | ||
| consolidatedWells[mixedLiquidId] = [ | ||
| { | ||
| labwareId: labwareIdName, | ||
| volumeByWell: {}, | ||
| }, | ||
| ] | ||
| } | ||
| const existingEntry = consolidatedWells[mixedLiquidId][0] | ||
| existingEntry.volumeByWell[well] = totalVolume | ||
| } |
There was a problem hiding this comment.
Ditto here, this avoids the let bindings, too.
const overlappingWells = Object.entries(wellToLabwareVolumes).filter(
([, entries]) => entries.length > 1
)
const consolidatedWells: LabwareByLiquidId = {}
overlappingWells.forEach(([well, entries]) => {
const totalVolume = entries.reduce((sum, entry) => sum + entry.volume, 0)
const labwareIdName = entries[0].labwareId
const liquidIds = entries.map(e => e.liquidId).sort()
const mixedLiquidId = `mixed-${liquidIds.join('-')}`
consolidatedWells[mixedLiquidId] ??= [{ labwareId: labwareIdName, volumeByWell: {} }]
consolidatedWells[mixedLiquidId][0].volumeByWell[well] = totalVolume
})
| const cleanedOriginal: LabwareByLiquidId = {} | ||
|
|
||
| for (const [liquidId, labwareArray] of Object.entries( | ||
| liquidsByIdForLabware | ||
| )) { | ||
| for (const labware of labwareArray) { | ||
| const remainingWells: Record<string, number> = {} | ||
|
|
||
| for (const [well, volume] of Object.entries(labware.volumeByWell)) { | ||
| const isOverlapping = overlappingWells.some(([ow]) => ow === well) | ||
| if (!isOverlapping) { | ||
| remainingWells[well] = volume | ||
| } | ||
| } | ||
| if (Object.keys(remainingWells).length > 0) { | ||
| if (!cleanedOriginal[liquidId]) cleanedOriginal[liquidId] = [] | ||
| cleanedOriginal[liquidId].push({ | ||
| labwareId: labware.labwareId, | ||
| volumeByWell: remainingWells, | ||
| }) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
const cleanedOriginal: LabwareByLiquidId = {}
const overlappingWellSet = new Set(overlappingWells.map(([well]) => well))
Object.entries(liquidsByIdForLabware).forEach(([liquidId, labwareArray]) => {
labwareArray.forEach(labware => {
const remainingWells = Object.fromEntries(
Object.entries(labware.volumeByWell).filter(([well]) => !overlappingWellSet.has(well))
)
if (Object.keys(remainingWells).length > 0) {
cleanedOriginal[liquidId] ??= []
cleanedOriginal[liquidId].push({
labwareId: labware.labwareId,
volumeByWell: remainingWells,
})
}
})
})
| }> | ||
| } | ||
|
|
||
| // Updated getLabwareInfoByLiquidId |
There was a problem hiding this comment.
| // Updated getLabwareInfoByLiquidId |
| if (!seenLiquids.has(liquidId)) { | ||
| seenLiquids.add(liquidId) | ||
| const liquid = loadedLiquids.find(l => l.id === liquidId) | ||
| if (liquid) acc.push(liquid) |
There was a problem hiding this comment.
| if (liquid) acc.push(liquid) | |
| if (liquid) { | |
| acc.push(liquid) | |
| } |
|
|
||
| mixedMap.set(key, { | ||
| id: mixedId, | ||
| displayColor: '#737578', |
There was a problem hiding this comment.
| displayColor: '#737578', | |
| displayColor: MIXED_WELL_COLOR, |
Yeah, think we should mimic PD here and pull this into a constant.
| mixedMap.set(key, { | ||
| id: mixedId, | ||
| displayColor: '#737578', | ||
| displayName: `${liquidNames.length} liquids`, |
There was a problem hiding this comment.
This string has to be i18n'd, and we can't i18n in shared-data, so we should return something like a totalLiquids here and then have the app take that value and do a proper "{{totalLiquids}} liquids" translation.
mjhuff
left a comment
mjhuff
left a comment
There was a problem hiding this comment.
Coming along! I think the abstraction is getting a bit leaky by making displayName return a number though, see comment. Also, it seems the i18n keys aren't quite in the right spot:
There was a problem hiding this comment.
Is this file intentionally targeted? I think we need protocol_setup for sure, but maybe want it here too?
| mixedMap.set(key, { | ||
| id: mixedId, | ||
| displayColor: MIXED_WELL_COLOR, | ||
| displayName: totalLiquids, |
There was a problem hiding this comment.
I think we should avoid assigning something to a parameter called displayName that isn't the actual displayName. Maybe what we can do is return null for displayName for multiple liquids, and now return a totalLiquids property.
Then, where ever we do this name rendering, we can either check the totalLiquids and displayName properties and do our rendering based on that.
Overview
When multiple liquids are loaded into the same well, our current behavior is to layer them on top of each other. This creates an unclear UI because the shared well is always highlighted.
This PR creates a third liquid by combining the liquids in one well into a command seperated list. The total volume is displayed.
Test Plan and Hands on Testing
Changelog
Screen.Recording.2025-12-10.at.5.55.50.PM.mov
Review requests
Risk assessment