fix(app): dont ignore errors in redux robotapi #16128
Conversation
More recent app code uses the api-client direct fetch interface through react-api-client hooks to talk to the robot, but older app code like that in the desktop wifi connection uses what came before, an rxjs based api that you interact with through redux. This API doesn't seem to properly handle the new errors from the USB interface, so fix that. This happens in a pretty ugly way because the type interface doesn't really have the concept of requests that fail without having an http status code, so we fake one. Closes RQA-3087
| ok: inRange(response?.status, 200, 300), | ||
| ok: result.isError | ||
| ? false | ||
| : inRange(result?.response?.status, 200, 300), |
There was a problem hiding this comment.
upper bound should probably be 400 not 300
There was a problem hiding this comment.
and presumably lower bound should be 100 lol. I'm a bit scared to touch though
There was a problem hiding this comment.
oh oops you didnt actually change this code, i read the diff wrong
app/src/redux/robot-api/http.ts
Outdated
| body: result?.response?.data, | ||
| // FIXME(sf) this doesn't seem right, but also the type interface isn't written to allow for request | ||
| // failures that don't come from valid connections | ||
| status: result?.response?.status ?? -1, |
There was a problem hiding this comment.
up to you but falling back to a 444 rather than -1 seems less likely to cause problems. but actually first, why might we get a rejected promise here? does it mean the connection was closed?
There was a problem hiding this comment.
re: 444 - oh didn't know about that. That's apparently a specific nginx thing? I suppose I'm fine with it
re: why rejected promise - for some reason, maybe something isn't getting passed over ipc correctly, the node-side axios seems to reject on more errors than the browser does. if we're passing in a validator, maybe that's not firing properly. That said, even if axios rejects, the error is very rich and actually has a status code already. Line 82 here should pick up a status from the error if present - that's what the reshaping of the results in the promise then and catch is for, to make sure that both cases have the same shape - but since the error type technically allows the status to be empty in the case of a failed connection, we need some valid default.
we added the ability for the USB interface to actually pass on communication errors to the browser side, but only some of the things that create those requests on the browser handled it properly. The old redux robot api client didn't. Now it does, at least on USB.
This sort of fixes RQA-3087 in that it will no longer be in the spinner forever and correctly expose the error that happened on the robot, but it doesn't fix it in that you'll now get an error and then a moment later the robot will say it's connected anyway.
Closes RQA-3087 (under the above constraint)