feat(router): add global error boundary for route errors #2198
Conversation
- Use React Router’s useRouteError to get error details. - Create an ErrorPage component in pages/public/error.tsx. - Show useful error messages based on status codes. Add a button to help users go back or retry.
📝 WalkthroughWalkthroughThe pull request introduces a new Changes
Sequence DiagramsequenceDiagram
participant User
participant Router
participant ErrorPage
User->>Router: Trigger route error
Router->>ErrorPage: Render error component
ErrorPage->>ErrorPage: Retrieve error details
ErrorPage-->>User: Display error message
User->>ErrorPage: Click home button
ErrorPage->>Router: Navigate to home page
Possibly related issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/client/src/pages/public/error.tsx (3)
1-9: LGTM! Consider running autofix for import sorting.The RouterError type definition is well-structured and includes all necessary fields for handling route errors.
Run ESLint autofix to sort the imports:
-import { useRouteError } from "react-router"; -import { Button } from "@reactive-resume/ui"; +import { Button } from "@reactive-resume/ui"; +import { useRouteError } from "react-router";🧰 Tools
🪛 ESLint
[error] 1-2: Run autofix to sort these imports!
(simple-import-sort/imports)
11-13: Consider safer error type handling.The type assertion
as RouterErrorcould fail if the error doesn't match the expected shape. Consider adding type guards for safer error handling.- const error = useRouteError() as RouterError; + const routeError = useRouteError(); + const error: RouterError = { + status: 'status' in routeError ? (routeError.status as number) : 500, + statusText: 'statusText' in routeError ? (routeError.statusText as string) : undefined, + data: 'data' in routeError ? String(routeError.data) : 'Unknown error', + message: 'message' in routeError ? String(routeError.message) : undefined, + };
28-37: Enhance navigation options and add internationalization.Consider adding a "Go back" option and internationalizing the button text.
+import { useNavigate } from "react-router"; + export const ErrorPage = () => { + const navigate = useNavigate(); + const handleGoBack = () => navigate(-1); + // ... rest of the component <div className="pt-2"> + <div className="space-x-4"> + <Button onClick={handleGoBack}> + <Trans>Go back</Trans> + </Button> <a href={process.env.PUBLIC_URL} className="inline-block" > <Button> - Go to home + <Trans>Go to home</Trans> </Button> </a> + </div> </div>🧰 Tools
🪛 ESLint
[error] 33-35: String not marked for translation. Wrap it with .
(lingui/no-unlocalized-strings)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/client/src/pages/public/error.tsx(1 hunks)apps/client/src/router/index.tsx(1 hunks)
🧰 Additional context used
🪛 ESLint
apps/client/src/pages/public/error.tsx
[error] 1-2: Run autofix to sort these imports!
(simple-import-sort/imports)
[error] 18-19: String not marked for translation. Wrap it with .
(lingui/no-unlocalized-strings)
[error] 33-35: String not marked for translation. Wrap it with .
(lingui/no-unlocalized-strings)
🔇 Additional comments (1)
apps/client/src/router/index.tsx (1)
23-23: LGTM! Error boundary is correctly implemented.The ErrorPage is properly integrated as a global error boundary at the root level, which will catch and handle all routing errors in the application.
Also applies to: 26-26
🧰 Tools
🪛 ESLint
[error] 1-23: Run autofix to sort these imports!
(simple-import-sort/imports)
| return ( | ||
| <main className="flex items-center justify-center min-h-screen p-4"> | ||
| <div className="w-full max-w-sm space-y-6"> | ||
| <h4 className="flex flex-col text-4xl font-bold text-white"> | ||
| <span> | ||
| Error {error.status} | ||
| </span> | ||
| {error.statusText && <span className="text-base font-normal">{error.statusText}</span>} | ||
| </h4> | ||
|
|
||
| <p className="text-sm text-gray-500 break-words"> | ||
| {error.data || error.message} | ||
| </p> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add internationalization support and specific error messages.
The error display should support internationalization and provide specific messages for different HTTP status codes.
+import { Trans } from "@lingui/macro";
+
+const getErrorMessage = (status: number) => {
+ switch (status) {
+ case 404:
+ return "The page you're looking for doesn't exist.";
+ case 403:
+ return "You don't have permission to access this page.";
+ case 500:
+ return "An internal server error occurred.";
+ default:
+ return "An unexpected error occurred.";
+ }
+};
<h4 className="flex flex-col text-4xl font-bold text-white">
<span>
- Error {error.status}
+ <Trans>Error {error.status}</Trans>
</span>
- {error.statusText && <span className="text-base font-normal">{error.statusText}</span>}
+ {error.statusText && <span className="text-base font-normal"><Trans>{error.statusText}</Trans></span>}
</h4>
<p className="text-sm text-gray-500 break-words">
- {error.data || error.message}
+ <Trans>{error.data || error.message || getErrorMessage(error.status)}</Trans>
</p>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return ( | |
| <main className="flex items-center justify-center min-h-screen p-4"> | |
| <div className="w-full max-w-sm space-y-6"> | |
| <h4 className="flex flex-col text-4xl font-bold text-white"> | |
| <span> | |
| Error {error.status} | |
| </span> | |
| {error.statusText && <span className="text-base font-normal">{error.statusText}</span>} | |
| </h4> | |
| <p className="text-sm text-gray-500 break-words"> | |
| {error.data || error.message} | |
| </p> | |
| return ( | |
| <main className="flex items-center justify-center min-h-screen p-4"> | |
| <div className="w-full max-w-sm space-y-6"> | |
| <h4 className="flex flex-col text-4xl font-bold text-white"> | |
| <span> | |
| <Trans>Error {error.status}</Trans> | |
| </span> | |
| {error.statusText && <span className="text-base font-normal"><Trans>{error.statusText}</Trans></span>} | |
| </h4> | |
| <p className="text-sm text-gray-500 break-words"> | |
| <Trans>{error.data || error.message || getErrorMessage(error.status)}</Trans> | |
| </p> |
🧰 Tools
🪛 ESLint
[error] 18-19: String not marked for translation. Wrap it with .
(lingui/no-unlocalized-strings)
…ustom messages - Add LocaleProvider wrapper for i18n support - Replace a tag with React Router Link components - Add custom error messages for different HTTP status codes - Implement proper translation support using Trans and t components
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/client/src/pages/public/error.tsx (1)
14-29:⚠️ Potential issueAdd internationalization support and fix switch case style.
The error messages need to be internationalized, and the switch cases should follow a consistent style with braces.
const getErrorMessage = (status: number) => { switch (status) { - case 404: - return "The page you're looking for doesn't exist."; - case 403: - return "You don't have permission to access this page."; - case 500: - return "An internal server error occurred."; - case 401: - return "You are not authorized to access this page."; - case 400: - return "The request was invalid."; - default: - return "An unexpected error occurred."; + case 404: { + return t`The page you're looking for doesn't exist.`; + } + case 403: { + return t`You don't have permission to access this page.`; + } + case 500: { + return t`An internal server error occurred.`; + } + case 401: { + return t`You are not authorized to access this page.`; + } + case 400: { + return t`The request was invalid.`; + } + default: { + return t`An unexpected error occurred.`; + } } };🧰 Tools
🪛 ESLint
[error] 16-16: Missing braces in case clause.
(unicorn/switch-case-braces)
[error] 17-17: String not marked for translation. Wrap it with t
, <Trans>, or msg.(lingui/no-unlocalized-strings)
[error] 18-18: Missing braces in case clause.
(unicorn/switch-case-braces)
[error] 19-19: String not marked for translation. Wrap it with t
, <Trans>, or msg.(lingui/no-unlocalized-strings)
[error] 20-20: Missing braces in case clause.
(unicorn/switch-case-braces)
[error] 21-21: String not marked for translation. Wrap it with t
, <Trans>, or msg.(lingui/no-unlocalized-strings)
[error] 22-22: Missing braces in case clause.
(unicorn/switch-case-braces)
[error] 23-23: String not marked for translation. Wrap it with t
, <Trans>, or msg.(lingui/no-unlocalized-strings)
[error] 24-24: Missing braces in case clause.
(unicorn/switch-case-braces)
[error] 25-25: String not marked for translation. Wrap it with t
, <Trans>, or msg.(lingui/no-unlocalized-strings)
[error] 26-26: Missing braces in case clause.
(unicorn/switch-case-braces)
[error] 27-27: String not marked for translation. Wrap it with t
, <Trans>, or msg.(lingui/no-unlocalized-strings)
🧹 Nitpick comments (2)
apps/client/src/pages/public/error.tsx (2)
7-12: Consider making the data property optional in RouterError type.The
dataproperty might not always be present in the error object, making it safer to mark it as optional.type RouterError = { status: number; statusText?: string; - data: string; + data?: string; message?: string; } & Error;
46-46: Remove commented code.Remove the commented-out code as it's no longer needed and can cause confusion.
- {/* {error.data || error.message} */}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
-
apps/client/src/pages/public/error.tsx(1 hunks)
🧰 Additional context used
🪛 ESLint
apps/client/src/pages/public/error.tsx
[error] 2-5: Run autofix to sort these imports!
(simple-import-sort/imports)
[error] 16-16: Missing braces in case clause.
(unicorn/switch-case-braces)
[error] 17-17: String not marked for translation. Wrap it with t, <Trans>, or msg.
(lingui/no-unlocalized-strings)
[error] 18-18: Missing braces in case clause.
(unicorn/switch-case-braces)
[error] 19-19: String not marked for translation. Wrap it with t, <Trans>, or msg.
(lingui/no-unlocalized-strings)
[error] 20-20: Missing braces in case clause.
(unicorn/switch-case-braces)
[error] 21-21: String not marked for translation. Wrap it with t, <Trans>, or msg.
(lingui/no-unlocalized-strings)
[error] 22-22: Missing braces in case clause.
(unicorn/switch-case-braces)
[error] 23-23: String not marked for translation. Wrap it with t, <Trans>, or msg.
(lingui/no-unlocalized-strings)
[error] 24-24: Missing braces in case clause.
(unicorn/switch-case-braces)
[error] 25-25: String not marked for translation. Wrap it with t, <Trans>, or msg.
(lingui/no-unlocalized-strings)
[error] 26-26: Missing braces in case clause.
(unicorn/switch-case-braces)
[error] 27-27: String not marked for translation. Wrap it with t, <Trans>, or msg.
(lingui/no-unlocalized-strings)
[error] 40-40: Should be ${variable}, not ${object.property} or ${myFunction()}
(lingui/no-expression-in-message)
[error] 47-47: You couldn't translate just a variable, remove Trans or add some text inside
(lingui/no-single-variables-to-translate)
[error] 47-47: should not wrap a single element unnecessarily
(lingui/no-single-tag-to-translate)
[error] 47-47: Should be ${variable}, not ${object.property} or ${myFunction()}
(lingui/no-expression-in-message)
|
@omimouni Thank you so much for your work on this PR! Do you perhaps have a screenshot of the error page so I can check it out before we merge this to main? |
|
Hi @AmruthPillai, here are screenshot of the error page on both desktop & mobile 
|
|
Looks good, thank you for the quick response! |
2 participants
Summary by CodeRabbit
New Features
Improvements