-
Notifications
You must be signed in to change notification settings - Fork 15
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
feat: add error boundary #319
Conversation
This adds an error boundary for the _auth routes. By adding this, errors are handled in a more consistent manner in a way that is interactable for the user. In addition this removes the sonners that would be shown when API errors occurred. If they are blocking errors, the user will now be directed to the error boundary. We'll want things to be more specific in the future, but this is a great start. Signed-off-by: tylerslaton <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couple things to clean up regarding window.href, but other than that looks great!
<Button | ||
className="w-full" | ||
variant="secondary" | ||
onClick={() => window.location.reload()} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should be using the useNavigate
hook to perform routing options. It will handle some other stuff that Remix needs to manage it's router
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would you recommend using useNavigate here? I tried a couple of things but nothing worked quite like this. I Know it reloads the whole page but that was desired by me.
className="w-full" | ||
variant="secondary" | ||
onClick={() => { | ||
window.location.href = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here. The useNavigate
hook is preferred over setting the window.location
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is intentional since the Remix router will route to this with /admin
prefixed. We do not want this here.
switch (true) { | ||
case error instanceof AxiosError: | ||
if ( | ||
error.response?.status === 403 && | ||
me.username && | ||
me.username !== AuthDisabledUsername | ||
) { | ||
return <Unauthorized />; | ||
} | ||
return <SignIn />; | ||
case isRouteErrorResponse(error): | ||
return <RouteError error={error} />; | ||
default: | ||
return <Error error={error as Error} />; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't know this was possible with JS switches 😮
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah classic switch(true)!
variant="secondary" | ||
className="w-full" | ||
onClick={() => { | ||
window.location.href = "/oauth2/start?rd=/admin/"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above ^ intentional since it won't work with navigate or link.
This adds an error boundary for the _auth routes. By adding this, errors are handled in a more consistent manner in a way that is interactable for the user.
In addition this removes the sonners that would be shown when API errors occurred. If they are blocking errors, the user will now be directed to the error boundary. We'll want things to be more specific in the future, but this is a great start.
Screen.Recording.2024-10-24.at.10.21.42.PM.mov