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 routing inconsistencies after a run #15966

Merged
merged 2 commits into from
Aug 12, 2024

Conversation

mjhuff
Copy link
Contributor

@mjhuff mjhuff commented Aug 12, 2024

Closes RQA-2922, RQA-2886, RQA-2944, and RQA-2950

Overview

We were on the right track in #15875 (see this PR for more context) for how to handle the routing issues after completing a run, but it turns out we have to set the caches to empty for BOTH of the endpoints used by useCurrentRoute to get proper behavior, which makes sense in retrospect.

The attached tickets are either directly solved by this PR are solved by this PR in conjunction with #15943.

Test Plan and Hands on Testing

  • Verified that after a run, clicking "return to dashboard" on the ODD doesn't do anything unexpected.
  • Verified expected behavior for "run again" on the ODD.
  • Verified the desktop app/ODD are in sync after getting into states described in the linked tickets.

Changelog

  • Fixed routing issues on the ODD.
  • Fixed the desktop app thinking the robot is busy when it's not busy.

Risk assessment

low

@mjhuff mjhuff requested a review from a team as a code owner August 12, 2024 15:00
@mjhuff mjhuff changed the title fix routing inconsistencies after a run fix(app): routing inconsistencies after a run Aug 12, 2024
@mjhuff mjhuff changed the title fix(app): routing inconsistencies after a run fix(app): fix routing inconsistencies after a run Aug 12, 2024
Copy link
Contributor

@SyntaxColoring SyntaxColoring left a comment

Choose a reason for hiding this comment

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

This makes sense conceptually, but should this clearing of the query cache live in useDismissCurrentRunMutation instead? We're already doing stuff there with [host, 'runs'] and [host, 'runs', runId].

@mjhuff
Copy link
Contributor Author

mjhuff commented Aug 12, 2024

This makes sense conceptually, but should this clearing of the query cache live in useDismissCurrentRunMutation instead? We're already doing stuff there with [host, 'runs'] and [host, 'runs', runId].

Great Q. Seth and I talked a bit indirectly about this last week. While there's a lot we could and should do with the routing logic (and the ideal fix isn't even clearing caches like we do here - see the TODO a few lines above the fix), the intention now is to keep the blast radius as small as possible. That solution is better for the immediate problem, but it increases the likelihood of introducing issues elsewhere (ex, ProtocolRunHeader). The client-side run context closing logic is complex, and it's quite difficult to reason through all the permutations of that change.

Copy link
Contributor

@TamarZanzouri TamarZanzouri left a comment

Choose a reason for hiding this comment

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

Thank you for fixing this!

@mjhuff mjhuff merged commit 0cb4832 into chore_release-8.0.0 Aug 12, 2024
20 checks passed
@mjhuff mjhuff deleted the app_fix-post-run-routing branch August 12, 2024 16:24
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