fix(app): fix infinitely hanging return to dashboard

[mjhuff]

Closes RABR-672

Overview

We gate the "return to dashboard" by a success close run response, but we probably should do this just onSettled.

Test Plan and Hands on Testing

• Ran protocols on the ODD and clicked return to dashboard.

Review requests

• does this seem reasonable?

Risk assessment

low

[sfoster1]

Yeah it makes sense to me. I'd be a bit worried that we'd orphan runs though, do we know why the close failed?

[mjhuff]

Yeah it makes sense to me. I'd be a bit worried that we'd orphan runs though, do we know why the close failed?

No, I don't. I'll discuss with @SyntaxColoring and dig around a bit myself before merging!

EDIT: Ah...

[SyntaxColoring]

Hm, so you're suggesting that the close request raced against something else that was also closing the run, and lost, so the button never "successfully closed the run," and that's what made the spinner spin infinitely?

If so, it kind of doesn't sound like we want to gate this on just a success response or just onSettled. Ideally, we'd want to gate it on <success 200 response> || <409 with RunNotCurrentError>?

[SyntaxColoring]

Or the server should return a 2xx for that PATCH instead of a 409. Setting current: false on a run that's already current: false is just as much a successful no-op as it is a conflict.

[mjhuff]

Hm, so you're suggesting that the close request raced against something else that was also closing the run, and lost, so the button never "successfully closed the run," and that's what made the spinner spin infinitely?

Yeah, it seems quite likely that's the case since the desktop app implicitly closes a run in one circumstance.

If so, it kind of doesn't sound like we want to gate this on just a success response or just onSettled. Ideally, we'd want to gate it on <success 200 response> || <409 with RunNotCurrentError>?

I actually don't think we want to gate on error response, since any error blocks users from doing anything on the ODD.

Or the server should return a 2xx for that PATCH instead of a 409. Setting current: false on a run that's already current: false is just as much a successful no-op as it is a conflict.

I think that's reasonable. I still think we shouldn't block the client on error here, though.