handle api errors in error boundary #17086
Conversation
| started the job as gitpod-build-bmh-top-level-error-boundary.1 because the annotations in the pull request description changed |
| <QueryErrorResetBoundary> | ||
| {({ reset }) => ( | ||
| // Passing reset to onReset so any queries know to retry after boundary is reset | ||
| <ErrorBoundary FallbackComponent={ExpectedQueryErrorsFallback} onReset={reset}> |
There was a problem hiding this comment.
passing reset into here will tell react-query to reset any queries once the boundary has been reset.
There was a problem hiding this comment.
Ah, nice! Wondered before how this happens/if it's the default/hard-wired behavior 💡 🧡
| } | ||
| } | ||
| | ||
| return <Login onLoggedIn={resetErrorBoundary} />; |
There was a problem hiding this comment.
It's important to reset the error boundary once we've gotten into a state that shouldn't produce it anymore. This tells react (and react query) to reset state, and try the render again.
| </Subheading> | ||
| <div> | ||
| <button onClick={resetErrorBoundary}>Reload</button> | ||
| <button onClick={handleReset}>Reload</button> |
There was a problem hiding this comment.
I this case we don't actually want to ask React to try and re-render, we want to just have the user reload the page. We may want to have a route-level error boundary that does attempt a re-render on reset though as a future improvement.
| } finally { | ||
| trackLocation(!!user); | ||
| } | ||
| const user = await getGitpodService().server.getLoggedInUser(); |
There was a problem hiding this comment.
Wow. This looking really clean now! 🎉
There was a problem hiding this comment.
Actually, I think this in combination with the QueryErrorBoundary, could serve as one example on how to use react-query.
WDYT @selfcontained?
| | ||
| // Wrap the App in an ErrorBoundary to catch User/Org loading errors | ||
| // This will also catch any errors that happen to bubble all the way up to the top | ||
| const AppWithErrorBoundary: FC = () => { |
There was a problem hiding this comment.
This is superfluous now, right? Or am I missing sth? (asking to understand)
geropl left a comment
geropl left a comment There was a problem hiding this comment.
The code LGTM, tested and works. ✔️
I have some minor clarification questions.
/hold But I think it's worth discussion if we want to keep main clean during the offsite, and have a "merge freeze" (see slack)
exhaling roboquat... 😞 |
This reverts commit 8ddad67.
| /reviewpad robin prompt can you find any issues with this implementation? |
| After carefully reviewing the provided git patches, I found some potential issues and improvements:
To fix this, you could either: a. Wrap the Other than these issues, the overall implementation seems fine. The code is well-structured and modular, and the patches are organized well into meaningful segments. |
* handle api errors in error boundary * handling resetting the error boundary * splitting out error boundary types
* handle api errors in error boundary * handling resetting the error boundary * splitting out error boundary types
roboquat added deployed: webapp 
Description
Adding a new
<QueryErrorBoundary>to handle expected api errors that queries may throw. These will currently only come from the 2 queries we've enabled theuseErrorBoundaryoption on.getLoggedInUser()- codelistTeams()- codeWhen these queries throw, they're caught first by the
<QueryErrorBoundary>, which will handle any errors wherecodeis one ofNOT_AUTHENTICATEDorSETUP_REQUIRED(we can add more in the future as we need). If it's not one of those errors, we re-throw it, and let it get caught by our default error boundary, now renamed to<ReloadPageErrorBoundary>for clarity. This one continues to report the error, and render a view to ask the user to reload the page. This should only be reached by unanticipated errors.A nice side effect is it cleans up a bunch of the rendering in the main
<App/>component.Related Issue(s)
Fixes WEB-70
How to test
Preview Env: https://bmh-top-le869d9d8254.preview.gitpod-dev.com/workspaces
Other tests to exercise the error boundary are harder to test, as you have to simulate errors in different spots.
Release Notes
Documentation
Build Options:
Run the build with werft instead of GHA
Run Leeway with
--dont-testPublish Options
Installer Options
Add desired feature flags to the end of the line above, space separated
Preview Environment Options:
If enabled this will build
install/previewIf enabled this will create the environment on GCE infra
Valid options are
all,workspace,webapp,ide,jetbrains,vscode,ssh