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

fix(app): App support for new lid commands and fix lid error boundary trigger #17386

Merged
merged 6 commits into from
Jan 30, 2025

Conversation

smb2268
Copy link
Contributor

@smb2268 smb2268 commented Jan 30, 2025

fix EXEC-1042, RABR-712

Overview

This PR adds the new loadLid and loadLidStack commands to the typescript command schema, makes a few necessary utils aware that labware can be loaded through these commands, and adds some null protection where we were hitting an error boundary in the recovery flow due to missing labware

Test Plan and Hands on Testing

On this branch, run a protocol that loads a lid, trigger a stall/collision, and see that you no longer hit an error boundary
I tested this out on desktop and it works as expected! Error recover is now launched and you can address the gripper failure

Changelog

  1. adds the new loadLid and loadLidStack commands to the typescript command schema
  2. Add prelim support for these in the run log
  3. Make a few necessary utils aware that labware can be loaded through these commands
  4. Adds some null protection where we were hitting an error boundary in the recovery flow due to missing labware, even though it should no longer be missing

Review requests

Look over code changes and test this out if you can

Risk assessment

Low

@smb2268 smb2268 requested review from vegano1 and mjhuff January 30, 2025 19:55
@smb2268 smb2268 self-assigned this Jan 30, 2025
@smb2268 smb2268 requested a review from CaseyBatten January 30, 2025 19:55
@smb2268 smb2268 marked this pull request as ready for review January 30, 2025 19:58
@smb2268 smb2268 requested review from a team as code owners January 30, 2025 19:58
Copy link
Contributor

@mjhuff mjhuff left a comment

Choose a reason for hiding this comment

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

I'm unable to test it, but the fix makes sense given the stack trace. Thank you!

@@ -287,7 +287,7 @@ export function getFailedCmdRelevantLabware(
const failedLWURI = runRecord?.data.labware.find(
labware => labware.id === recentRelevantFailedLabwareCmd?.params.labwareId
)?.definitionUri
if (failedLWURI != null) {
if (failedLWURI != null && Object.keys(lwDefsByURI).includes(failedLWURI)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

That will do it!

Comment on lines +45 to +47
(commandType === 'loadLabware' ||
commandType === 'loadLid' ||
commandType === 'loadLidStack') &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe not in this PR, but could we add a TODO for adding some sort of constant to shared data that contains a list of all possible load commands, and then implement that in the app?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, definitely - we won't want to reference all three of these everywhere we search for loadLabware (we'll want to exclude these from LPC, for example) but it would be good to have this as a type

Copy link
Contributor

@vegano1 vegano1 left a comment

Choose a reason for hiding this comment

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

sweet, thank you!

@vegano1
Copy link
Contributor

vegano1 commented Jan 30, 2025

Works as expected, I tested it on a flex with both the ODD and Desktop app.

@smb2268 smb2268 merged commit 18da739 into edge Jan 30, 2025
68 of 79 checks passed
@smb2268 smb2268 deleted the app_lid-commands-ts branch January 30, 2025 20:52
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