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): Fix persistent "run in progress" settings banner after run cancel #17235

Open
wants to merge 3 commits into
base: chore_release-8.3.0
Choose a base branch
from

Conversation

mjhuff
Copy link
Contributor

@mjhuff mjhuff commented Jan 9, 2025

Closes RQA-3838

Overview

This PR addresses the underlying issue of the run not un-currenting when expected. This issue manifests in three separate bugs that have been a part of relatively recent refactors.

Cancelling a Run before Run Start

40d08aa

We implicitly close the run context if the run has been cancelled, and no tips are confirmed to be attached. Additionally, if you cancel a run before starting the run, no run commands have occurred.

On the desktop app, the tip detection logic currently runs only if there has been at least one command, meaning that we never implicitly close the run context when canceling a run before starting it. This results in the run always being current, which results in the bug mentioned in the ticket.

This "check the commands" logic is a holdover from when tip detection actually used the protocol record to do tip detection, which we removed after 8.2 now that we use the currentState endpoint for tip detection. This could should have been removed along with that refactor.

Disabling Error Recovery in App Settings

9b10793

Doing so does not properly invoke the post-run tip detection logic, which is dependent on whether the run has entered error recovery. The lastRunCommandPromptedErrorRecovery is not a sufficient check for this condition, because the user can disable error recovery in settings, and this utility only looks at the previous non-fixit command.

After giving the tip detection gating more thought: gating tip detection logic is no longer a hard requirement, because the app isn't doing any expensive compute like it did 8.2 and earlier. Simplifying the tip detection logic comes with the reasonable tradeoff of running a few network requests once post-run. That seems ok.

/runCommandErrors Refactors

4eb337b

Because the endpoint now supports historical errors, we can now implicitly uncurrent cancelled runs that have undergone error recovery and don't have tips attached and preserve the banner.

Test Plan and Hands on Testing

  • On chore_release, start protocol setup and then cancel the run (on the desktop app). Note on the robot settings page that the "run in progress" banner is present.
  • On this branch, repeat the above. The run is now properly un-currented, and the banner is no longer present.
  • Validated correct behavior when disabling error recovery in settings.
  • Validated a cancelled run that has undergone error recovery is properly uncurrented.

Changelog

  • Fixed the desktop app settings menu being inaccessible if a run was cancelled before being started.

Risk assessment

low

@mjhuff mjhuff requested a review from a team as a code owner January 9, 2025 22:14
@mjhuff mjhuff force-pushed the app_fix-persistent-settings-banner branch from 4eb337b to 2dc62af Compare January 9, 2025 22:14
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.

1 participant