-
Notifications
You must be signed in to change notification settings - Fork 19
Add logout button to error page #2244
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
e2a6652 to
6820db6
Compare
6820db6 to
b2ac4f4
Compare
app/components/ErrorBoundary.tsx
Outdated
| <p className="text-tertiary"> | ||
| Please try again. If the problem persists, contact your administrator. | ||
| </p> | ||
| <SignOutButton className="!mt-4" /> |
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.
What if instead of showing in all non-404 errors, we show it only on 403 errors where it is most relevant. Don't want people hitting a 500 and thinking they need to logout to resolve.
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.
👍 will attempt
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 can do it, but it makes me nervous because of the way this stuff works. When we get the error in the fallback, it's not actually the original error from the loader request that's bubbling up — RQ prefetchQuery eats all errors, and in order to get the error from the loader, we'd have to use fetchQuery instead. Which we could do, but I want to think it through. So instead we have to tell the usePrefetchApiQuery call to throw on 403s instead of eating it (currently we only do it for 404s). That gives us an error we can detect in the ErrorBoundary to only show the sign out button when we see a 403, but like I said it's such a weird approach that it makes me nervous.
small diff showing how that is done
diff --git a/app/api/hooks.ts b/app/api/hooks.ts
index a6c464f4..75216f35 100644
--- a/app/api/hooks.ts
+++ b/app/api/hooks.ts
@@ -169,7 +169,7 @@ export const getUsePrefetchedApiQuery =
// we can say Not Found. If you need to allow a 404 and want it to show
// up as `error` state instead, pass `useErrorBoundary: false` as an
// option from the calling component and it will override this
- throwOnError: (err) => err.statusCode === 404,
+ throwOnError: (err) => err.statusCode === 404 || err.statusCode === 403,
...options,
})
invariant(
diff --git a/app/components/ErrorBoundary.tsx b/app/components/ErrorBoundary.tsx
index 95b6cf95..114d7f49 100644
--- a/app/components/ErrorBoundary.tsx
+++ b/app/components/ErrorBoundary.tsx
@@ -24,13 +24,15 @@ function ErrorFallback({ error }: Props) {
return <NotFound />
}
+ const showSignOut = 'statusCode' in error && error.statusCode === 403
+
return (
<ErrorPage>
<h1 className="text-sans-2xl">Something went wrong</h1>
<p className="text-tertiary">
Please try again. If the problem persists, contact your administrator.
</p>
- <SignOutButton className="!mt-4" />
+ {showSignOut && <SignOutButton className="!mt-4" />}
</ErrorPage>
)
}
I'd like to postpone being smart and instead solve your concern by moving the button into the corner so it doesn't give the impression that it's an action specific to the error you're seeing.
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.
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.
You have my blessing, thanks for looking into it.
oxidecomputer/console@078d171...a228b75 * [a228b75b](oxidecomputer/console@a228b75b) bump omicron (only one tiny diff in validators) * [fc91ec1e](oxidecomputer/console@fc91ec1e) oxidecomputer/console#2256 * [39b4491e](oxidecomputer/console@39b4491e) oxidecomputer/console#2230 * [e4e912ca](oxidecomputer/console@e4e912ca) oxidecomputer/console#2247 * [dcf09ec9](oxidecomputer/console@dcf09ec9) oxidecomputer/console#2217 * [c36b3d63](oxidecomputer/console@c36b3d63) oxidecomputer/console#2238 * [a8eb7745](oxidecomputer/console@a8eb7745) oxidecomputer/console#2251 * [9b20b7c9](oxidecomputer/console@9b20b7c9) oxidecomputer/console#2248 * [f20a5bcb](oxidecomputer/console@f20a5bcb) oxidecomputer/console#2245 * [b815dd8f](oxidecomputer/console@b815dd8f) oxidecomputer/console#2244 * [8c7b2946](oxidecomputer/console@8c7b2946) add node_modules to eslint ignore patterns * [90e78dbb](oxidecomputer/console@90e78dbb) oxidecomputer/console#2237 * [b603d2dd](oxidecomputer/console@b603d2dd) oxidecomputer/console#2242 * [bfce37c7](oxidecomputer/console@bfce37c7) upgrade @oxide/openapi-gen-ts to 0.2.2 * [efceb17d](oxidecomputer/console@efceb17d) oxidecomputer/console#2236 * [1aa46459](oxidecomputer/console@1aa46459) oxidecomputer/console#2235 * [b400ae78](oxidecomputer/console@b400ae78) oxidecomputer/console#2225 * [7bb3bbf7](oxidecomputer/console@7bb3bbf7) oxidecomputer/console#2229 * [c56a9ec5](oxidecomputer/console@c56a9ec5) oxidecomputer/console#2228 * [cd9d1f99](oxidecomputer/console@cd9d1f99) oxidecomputer/console#2227 * [ee269bd9](oxidecomputer/console@ee269bd9) oxidecomputer/console#2223
Highlights: soft image validation, logout button on error pages to help deal with auth-related errors. oxidecomputer/console@078d171...a228b75 * [a228b75b](oxidecomputer/console@a228b75b) bump omicron (only one tiny diff in validators) * [fc91ec1e](oxidecomputer/console@fc91ec1e) oxidecomputer/console#2256 * [39b4491e](oxidecomputer/console@39b4491e) oxidecomputer/console#2230 * [e4e912ca](oxidecomputer/console@e4e912ca) oxidecomputer/console#2247 * [dcf09ec9](oxidecomputer/console@dcf09ec9) oxidecomputer/console#2217 * [c36b3d63](oxidecomputer/console@c36b3d63) oxidecomputer/console#2238 * [a8eb7745](oxidecomputer/console@a8eb7745) oxidecomputer/console#2251 * [9b20b7c9](oxidecomputer/console@9b20b7c9) oxidecomputer/console#2248 * [f20a5bcb](oxidecomputer/console@f20a5bcb) oxidecomputer/console#2245 * [b815dd8f](oxidecomputer/console@b815dd8f) oxidecomputer/console#2244 * [8c7b2946](oxidecomputer/console@8c7b2946) add node_modules to eslint ignore patterns * [90e78dbb](oxidecomputer/console@90e78dbb) oxidecomputer/console#2237 * [b603d2dd](oxidecomputer/console@b603d2dd) oxidecomputer/console#2242 * [bfce37c7](oxidecomputer/console@bfce37c7) upgrade @oxide/openapi-gen-ts to 0.2.2 * [efceb17d](oxidecomputer/console@efceb17d) oxidecomputer/console#2236 * [1aa46459](oxidecomputer/console@1aa46459) oxidecomputer/console#2235 * [b400ae78](oxidecomputer/console@b400ae78) oxidecomputer/console#2225 * [7bb3bbf7](oxidecomputer/console@7bb3bbf7) oxidecomputer/console#2229 * [c56a9ec5](oxidecomputer/console@c56a9ec5) oxidecomputer/console#2228 * [cd9d1f99](oxidecomputer/console@cd9d1f99) oxidecomputer/console#2227 * [ee269bd9](oxidecomputer/console@ee269bd9) oxidecomputer/console#2223

Closes #1876
I thought about also checking specifically for the situation where nothing works, maybe by trying to hit
/session/meand seeing if that fails, but I'd rather do that in a followup. Also the error message wouldn't be that different.