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): Delineate failed command source #15933

Merged
merged 2 commits into from
Aug 8, 2024

Conversation

mjhuff
Copy link
Contributor

@mjhuff mjhuff commented Aug 8, 2024

Closes RQA-2913

Overview

One tricky problem of the mono repo's typing for a run command derived from protocol analysis and a run command derived from a runRecord is they both share the same type, RunTimeCommand, but they actually aren't equivalent. They share the same property names and value types, but the exact values of those types often differs, such as key and id. If you need information from the run itself, you need the run command, and if you need information from analysis, you need the analysis command, but typing doesn't catch this misue.

When a dev wants to find some information about a command that is only available from protocol analysis, such as exact details about the labware/well/the next command/the previous command/etc., they need to match a command that they do have with one from analysis based on some property of the supplied command with one from analysis. When this command is actually from the run and not protocol analysis, these APIs often return null, because properties like id are actually different between the two commands, and a lot of the utility function APIs the app uses unfortunately filters on the protocol analysis command's id.

At least how I see it, there are several viable ways to tackle this problem (with varying degrees of successfully solving the root issue), listed from greatest to least amount of effort:

  1. Have the server return all the information the app could ever dream about relating to a command, so the app never has to filter the protocol analysis.
  2. Update the types to represent reality, that RunTimeCommand is distinctly unique from AnalysisDerivedCommand, and the two are not equivalent.
  3. Make it pretty clear in Error Recovery that there two separate versions of the command (here, the failed command). This at least makes it easier for devs working in Error Recovery that we need to decide which command we actually need.
  4. Quickly pass in the corrected command type and move on.

Admittedly, I've chosen door 4 during a lot of the development of error recovery, but it's time to pay for my mistakes. I've decided to choose door 3 this time. While door 2 may seem feasible, it's actually not, because:

  • It doesn't solve the actual bug that's occurring right now. And more importantly...
  • Using RunTimeCommand for protocol analysis commands is such a systemic thing in the app that we should probably go with what is likely best solution at that point (which I think is Error in Documentation #1, but tbd).

There's a lot of noise in this PR with testing. There are really just two files (CategorizedStepContent and useRetainedFailedCommandBySource) that fix several bugs, including:

  • The ticket, linked above (the run command vs protocol command issue).
  • If you have a FailedStepNextStep render but you are actually on the penultimate step or actual last step, we don't render empty "next steps" anymore.
  • Retain the failed command so you can show failed command details the app cares about (see comments in new hook).

Test Plan and Hands on Testing

  • Smoke tested ER to ensure everything looks/acts correctly.
  • We are well covered by lint and unit testing here, so I feel pretty ok by making a change that may initially seem to have a wide blast radius.

Changelog

  • Fixed the "next command" not always displaying proper copy.
  • Fixed displaying "next step(s)" when there aren't any next step(s).
  • Cancelling a run in Error Recovery and clicking "View error details" now displays the correct copy.

Risk assessment

low, see test plan

@mjhuff mjhuff requested a review from a team August 8, 2024 00:06
@mjhuff mjhuff requested a review from a team as a code owner August 8, 2024 00:06
@mjhuff mjhuff requested review from smb2268 and removed request for a team and smb2268 August 8, 2024 00:06
@mjhuff mjhuff force-pushed the app_delineate-failed-command-source branch from bb5d91d to 3a1081b Compare August 8, 2024 13:30
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.

Okay, yeah, this looks good. I would love to do the big typing change in the future but I think this is the right level of change for now.

@mjhuff mjhuff merged commit 9f758bc into chore_release-8.0.0 Aug 8, 2024
30 checks passed
@mjhuff mjhuff deleted the app_delineate-failed-command-source branch August 8, 2024 22:40
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