Skip to content

fix(app): fix accumulating offsets on run record #17969

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.

Sign up for GitHub

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
mjhuff merged 1 commit into from
Apr 2, 2025
+143 −7
Merged

fix(app): fix accumulating offsets on run record #17969

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
129 changes: 124 additions & 5 deletions app/src/resources/runs/__tests__/useCloneRun.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,15 @@ import { useCloneRun } from '../useCloneRun'
import { useNotifyRunQuery } from '../useNotifyRunQuery'

import type { FunctionComponent, ReactNode } from 'react'
import type { HostConfig } from '@opentrons/api-client'
import type { HostConfig, LabwareOffset } from '@opentrons/api-client'

vi.mock('@opentrons/react-api-client')
vi.mock('/app/resources/runs/useNotifyRunQuery')

const HOST_CONFIG: HostConfig = { hostname: 'localhost' }
const RUN_ID_NO_RTP: string = 'run_id_no_rtp'
const RUN_ID_RTP: string = 'run_id_rtp'
const RUN_ID_DUPLICATE_OFFSETS: string = 'run_id_duplicate_offsets'

describe('useCloneRun hook', () => {
let wrapper: FunctionComponent<{ children: ReactNode }>
Expand All @@ -34,7 +35,13 @@ describe('useCloneRun hook', () => {
data: {
id: RUN_ID_NO_RTP,
protocolId: 'protocolId',
labwareOffsets: 'someOffset',
labwareOffsets: [
{
definitionUri: 'uri1',
locationSequence: [1, 2],
offset: { x: 1, y: 1, z: 1 },
},
],
runTimeParameters: [
{
type: 'int',
Expand All @@ -59,7 +66,13 @@ describe('useCloneRun hook', () => {
data: {
id: RUN_ID_RTP,
protocolId: 'protocolId',
labwareOffsets: 'someOffset',
labwareOffsets: [
{
definitionUri: 'uri1',
locationSequence: [1, 2],
offset: { x: 1, y: 1, z: 1 },
},
],
runTimeParameters: [
{
type: 'int',
Expand All @@ -82,6 +95,38 @@ describe('useCloneRun hook', () => {
},
},
} as any)

const duplicateOffsets: LabwareOffset[] = [
{
definitionUri: 'uri1',
locationSequence: [1, 2],
offset: { x: 1, y: 1, z: 1 },
},
{
definitionUri: 'uri2',
locationSequence: [3, 4],
offset: { x: 2, y: 2, z: 2 },
},
{
definitionUri: 'uri1',
locationSequence: [1, 2],
offset: { x: 3, y: 3, z: 3 },
},
] as any

when(vi.mocked(useNotifyRunQuery))
.calledWith(RUN_ID_DUPLICATE_OFFSETS)
.thenReturn({
data: {
data: {
id: RUN_ID_DUPLICATE_OFFSETS,
protocolId: 'protocolId',
labwareOffsets: duplicateOffsets,
runTimeParameters: [],
},
},
} as any)

when(vi.mocked(useCreateRunMutation))
.calledWith(expect.anything())
.thenReturn({ createRun: vi.fn() } as any)
Expand Down Expand Up @@ -111,11 +156,18 @@ describe('useCloneRun hook', () => {
result.current && result.current.cloneRun()
expect(mockCreateRun).toHaveBeenCalledWith({
protocolId: 'protocolId',
labwareOffsets: 'someOffset',
labwareOffsets: [
{
definitionUri: 'uri1',
locationSequence: [1, 2],
offset: { x: 1, y: 1, z: 1 },
},
],
runTimeParameterValues: {},
runTimeParameterFiles: {},
})
})

it('should return a function that when called, calls createRun run with runTimeParameterValues overrides', async () => {
const mockCreateRun = vi.fn()
vi.mocked(useCreateRunMutation).mockReturnValue({
Expand All @@ -126,7 +178,74 @@ describe('useCloneRun hook', () => {
result.current && result.current.cloneRun()
expect(mockCreateRun).toHaveBeenCalledWith({
protocolId: 'protocolId',
labwareOffsets: 'someOffset',
labwareOffsets: [
{
definitionUri: 'uri1',
locationSequence: [1, 2],
offset: { x: 1, y: 1, z: 1 },
},
],
runTimeParameterValues: {
number_param: 2,
boolean_param: false,
},
runTimeParameterFiles: {
file_param: 'fileId_123',
},
})
})

it('should filter duplicate labware offsets and keep only the most recent ones', async () => {
const mockCreateRun = vi.fn()
vi.mocked(useCreateRunMutation).mockReturnValue({
createRun: mockCreateRun,
} as any)

const { result } = renderHook(() => useCloneRun(RUN_ID_DUPLICATE_OFFSETS), {
wrapper,
})
result.current && result.current.cloneRun()

const expectedOffsets = [
{
definitionUri: 'uri2',
locationSequence: [3, 4],
offset: { x: 2, y: 2, z: 2 },
},
{
definitionUri: 'uri1',
locationSequence: [1, 2],
offset: { x: 3, y: 3, z: 3 },
},
]

expect(mockCreateRun).toHaveBeenCalledWith({
protocolId: 'protocolId',
labwareOffsets: expectedOffsets,
runTimeParameterValues: {},
runTimeParameterFiles: {},
})
})

it('should handle analysis trigger when specified', async () => {
const mockCreateRun = vi.fn()
const mockCreateProtocolAnalysis = vi.fn()

vi.mocked(useCreateRunMutation).mockReturnValue({
createRun: mockCreateRun,
} as any)
vi.mocked(useCreateProtocolAnalysisMutation).mockReturnValue({
createProtocolAnalysis: mockCreateProtocolAnalysis,
} as any)

const { result } = renderHook(
() => useCloneRun(RUN_ID_RTP, undefined, true),
{ wrapper }
)
result.current && result.current.cloneRun()

expect(mockCreateProtocolAnalysis).toHaveBeenCalledWith({
protocolKey: 'protocolId',
runTimeParameterValues: {
number_param: 2,
boolean_param: false,
Expand Down
21 changes: 19 additions & 2 deletions app/src/resources/runs/useCloneRun.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ import {
getRunTimeParameterFilesForRun,
} from '/app/transformations/runs'

import type { Run } from '@opentrons/api-client'
import type { LabwareOffset, Run } from '@opentrons/api-client'
import isEqual from 'lodash/isEqual'

interface UseCloneRunResult {
cloneRun: () => void
Expand Down Expand Up @@ -70,7 +71,7 @@ export function useCloneRun(
}
createRun({
protocolId,
labwareOffsets,
labwareOffsets: mostRecentUniqueLabwareOffsets(labwareOffsets),
runTimeParameterValues,
runTimeParameterFiles,
})
Expand All @@ -81,3 +82,19 @@ export function useCloneRun(

return { cloneRun, isLoadingRun, isCloning }
}

// Returns the most recent, unique offsets for each labware uri + location pair.
// Assumes the most recent labware offsets are appended to the end of the list.
function mostRecentUniqueLabwareOffsets(
offsets: LabwareOffset[] | undefined
): LabwareOffset[] | undefined {
return offsets?.filter((offset, index, array) => {
return (
array.findLastIndex(
firstOffset =>
isEqual(firstOffset.locationSequence, offset.locationSequence) &&
isEqual(firstOffset.definitionUri, offset.definitionUri)
) === index
Comment on lines +93 to +97
Copy link
Copy Markdown
Contributor

@SyntaxColoring SyntaxColoring Apr 2, 2025

Choose a reason for hiding this comment

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

Because you're only comparing locationSequence, until #17946 is merged, I think this will return wrong results when used on old runs. (Offsets will be dropped erroneously because the old offsets, without the new locationSequence field, will erroneously compare equal to each other.)

I'm fine solving this with #17946 if you are, but if you wanted to solve it here, I guess you'd replace

isEqual(firstOffset.locationSequence, offset.locationSequence)

with

isEqual(firstOffset.locationSequence ?? firstOffset.location, offset.locationSequence ?? offset.location)

Copy link
Copy Markdown
Contributor Author

@mjhuff mjhuff Apr 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Ah thanks for noting, I meant to mention this in the PR description. Yes, this will load the wrong offsets on runs containing legacy offset data. I'm fine with #17946 solving this entirely.

)
})
}
Loading