8000 fix: include a link and more useful error details for 403 response codes by brettkolodny · Pull Request #16644 · coder/coder · GitHub
[go: up one dir, main page]
Skip to content

fix: include a link and more useful error details for 403 response codes #16644

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Feb 21, 2025
Merged

fix: include a link and more useful error details for 403 response codes #16644

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
196ac96
fix: include link and more useful message for 403 errors
brettkolodny Feb 20, 2025
b6f2e5a
fix: fmt
brettkolodny Feb 20, 2025
3a7dd2d
Merge branch 'main' into brett/improve-erroralert-to-include-a-redire…
brettkolodny Feb 20, 2025
0f47a34
Merge branch 'main' into brett/improve-erroralert-to-include-a-redire…
brettkolodny Feb 21, 2025
50404ee
fix: move Language object to it's own file so playwright doesn't cry
brettkolodny Feb 21, 2025
1b8cf2a
fix: add details to API response on backend
brettkolodny Feb 21, 2025
94907f2
fix: error detail copy
brettkolodny Feb 21, 2025
169d41c
fix: move body to ternary
brettkolodny Feb 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
fix: include link and more useful message for 403 errors
  • Loading branch information
@brettkolodny
brettkolodny committed Feb 20, 2025
commit 196ac96a093a731ea5afdc7d3b3f41e1a8c94e0d
8 changes: 8 additions & 0 deletions site/src/api/errors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,14 @@ export const getErrorDetail = (error: unknown): string | undefined => {
return undefined;
};

export const getErrorStatus = (error: unknown): number | undefined => {
if (isApiError(error)) {
return error.status;
}

return undefined;
};

export class DetailedError extends Error {
constructor(
message: string,
Expand Down
42 changes: 35 additions & 7 deletions site/src/components/Alert/ErrorAlert.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import AlertTitle from "@mui/material/AlertTitle";
import { getErrorDetail, getErrorMessage } from "api/errors";
import { getErrorDetail, getErrorMessage, getErrorStatus } from "api/errors";
import { Link } from "components/Link/Link";
import type { FC } from "react";
import { Alert, AlertDetail, type AlertProps } from "./Alert";

Expand All @@ -8,21 +9,48 @@ export const ErrorAlert: FC<
> = ({ error, ...alertProps }) => {
const message = getErrorMessage(error, "Something went wrong.");
const detail = getErrorDetail(error);
const status = getErrorStatus(error);

// For some reason, the message and detail can be the same on the BE, but does
// not make sense in the FE to showing them duplicated
const shouldDisplayDetail = message !== detail;

return (
<Alert severity="error" {...alertProps}>
{detail ? (
const body = () => {
// When the error is a Forbidden response we include a link for the user to
// go back to a known viewable page.
// Additionally since the error messages and details from the server can be
// missing or confusing for an end user we render a friendlier message
// regardless of the response from the server.
if (status === 403) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm okay with this as a baseline solution, but do you know if there are ever cases where it makes sense to redirect the user to a page more relevant to what they were trying to access?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I agree, and I'm actually working on a PR right now that will make RequirePermissions a bit more user friendly in that way. But this change is still relevant in the cases where we don't wrap a portion of the page in RequirePermissions

return (
<>
<AlertTitle>You don't have permission to view this page</AlertTitle>
<AlertDetail>
If you believe this is a mistake, please contact your administrator
or try signing in with different credentials.{" "}
<Link href="/workspaces" className="w-fit">
Go to workspaces
</Link>
</AlertDetail>
</>
);
}

if (detail) {
return (
<>
<AlertTitle>{message}</AlertTitle>
{shouldDisplayDetail && <AlertDetail>{detail}</AlertDetail>}
</>
) : (
message
)}
);
}

return message;
};

return (
<Alert severity="error" {...alertProps}>
{body()}
Copy link
Member
@Parkreiner Parkreiner Feb 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I don't know your opinion on ternaries, but I feel like the typical React way of doing this would be to inline everything into a single return statement:

return (
  <Alert severity="error" {...alterProps}>
    {status === 403
      ? (
        <-- JSX -->
        )
      : detail ?
        (
        <-- JSX -->
        )
      : (
        message
      )
  </Alert>
);

Prettier/Biome should be able to format the ternaries to make them more readable than this, but you get the idea

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm usually pro ternary but I've been trying to match other general vibes of the code base. Generally though I think I've started to learn towards if the logic is more than an "if/else" it's more readable being broken out

</Alert>
);
};
Loading
0