feat:improve-error-dir-handling #2679
Conversation
|
@admirsaheta is attempting to deploy a commit to the Onlook Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughIntroduces security headers and image/experimental settings in Next.js config, removes devIndicators. Overhauls the custom error page to a typed, UI-rich component with status-aware messaging, server/client context handling, development-only details, and updated getInitialProps returning extended error metadata. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant R as Router/Next.js
participant S as Server
participant E as Error Page (_error.tsx)
U->>R: Navigate/request page
alt Server-side error
R->>S: Render request
S-->>R: err/statusCode
R->>E: getInitialProps({ res, err })
E-->>R: { statusCode, hasGetInitialPropsRun, err }
R-->>U: Render Error UI (SSR)
else Client-side error
R->>E: getInitialProps({ err })
E-->>R: { statusCode, hasGetInitialPropsRun, err }
R-->>U: Render Error UI (CSR)
end
U->>R: Click "Go back home" or "Try again"
R-->>U: Navigate/reload
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–20 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 3
🧹 Nitpick comments (3)
apps/web/client/next.config.ts (2)
14-35: Solid baseline headers; suggest adding HSTS (prod-only) and modern COOP/CORP/Permissions-PolicyGreat start with X-Frame-Options, X-Content-Type-Options, and Referrer-Policy. To harden further without undue risk, consider:
- Add Strict-Transport-Security in production only.
- Add Cross-Origin-Opener-Policy and Cross-Origin-Resource-Policy to reduce XS-Leaks.
- Add a conservative Permissions-Policy.
Apply this diff to extend your headers while keeping HSTS gated to production:
- async headers() { - return [ - { - source: '/(.*)', - headers: [ - { - key: 'X-Frame-Options', - value: 'DENY', - }, - { - key: 'X-Content-Type-Options', - value: 'nosniff', - }, - { - key: 'Referrer-Policy', - value: 'strict-origin-when-cross-origin', - }, - ], - }, - ]; - }, + async headers() { + const securityHeaders: Array<{ key: string; value: string }> = [ + { key: 'X-Frame-Options', value: 'DENY' }, + { key: 'X-Content-Type-Options', value: 'nosniff' }, + { key: 'Referrer-Policy', value: 'strict-origin-when-cross-origin' }, + { key: 'Cross-Origin-Opener-Policy', value: 'same-origin' }, + { key: 'Cross-Origin-Resource-Policy', value: 'same-origin' }, + { key: 'Permissions-Policy', value: 'camera=(), microphone=(), geolocation=()' }, + ]; + if (process.env.NODE_ENV === 'production') { + securityHeaders.push({ + key: 'Strict-Transport-Security', + value: 'max-age=63072000; includeSubDomains; preload', + }); + } + return [ + { + source: '/(.*)', + headers: securityHeaders, + }, + ]; + },Please confirm that DENY framing won’t break any legitimate embedding use-cases (docs, admin portals, etc.). If embedding is needed, switch to a CSP with frame-ancestors instead.
42-44: Use of optimizePackageImports is fine; validate package compatibilityThis experimental flag works well for lucide-react. For @onlook/ui, confirm it’s ESM-friendly and safe for import slicing (no unexpected side effects from the package’s barrel). If you see unexpected behavior, consider per-package modularizeImports instead.
apps/web/client/src/pages/_error.tsx (1)
and ARIA where appropriate
27-41: Optional: Improve semantics by usingFor better semantics, consider:
- Replace the outer container with .
- Add role="alert" to the message block for assistive tech emphasis.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
apps/web/client/next.config.ts(1 hunks)apps/web/client/src/pages/_error.tsx(1 hunks)
🧰 Additional context used
🪛 Biome (2.1.2)
apps/web/client/src/pages/_error.tsx
[error] 10-10: Do not shadow the global "Error" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🔇 Additional comments (3)
apps/web/client/next.config.ts (1)
37-40: Images config LGTM; verify remote image sources are configuredUsing AVIF/WebP and a modest TTL is good. If you serve remote images, ensure images.domains or images.remotePatterns cover them; otherwise Next/Image will error at runtime.
apps/web/client/src/pages/_error.tsx (2)
27-41: Nice UX and accessibility improvements
- Clear status-aware messaging, prominent actions, and sensible defaults.
- Button-only reload avoids SSR “window” pitfalls.
- Tailwind usage is consistent and readable.
No changes requested.
Also applies to: 43-57, 68-69
58-67: Dev-only error details are appropriately gatedGood job constraining stack traces to development only, reducing PII leakage risk in production.
| interface ErrorProps { | ||
| statusCode: number; | ||
| hasGetInitialPropsRun?: boolean; | ||
| err?: Error; | ||
| } | ||
|
|
There was a problem hiding this comment.
Type mismatch: err.statusCode access is not on the base Error type
Accessing err.statusCode is not valid on the standard Error interface and may fail TS checks depending on Next’s typings. Introduce a narrowed type and use it in getInitialProps.
interface ErrorProps {
statusCode: number;
hasGetInitialPropsRun?: boolean;
- err?: Error;
+ err?: Error & { statusCode?: number };
}-ErrorPage.getInitialProps = ({ res, err }: NextPageContext): ErrorProps => {
- const statusCode = res?.statusCode ?? err?.statusCode ?? 404;
+ErrorPage.getInitialProps = ({ res, err }: NextPageContext): ErrorProps => {
+ const e = err as (Error & { statusCode?: number }) | undefined;
+ const statusCode = res?.statusCode ?? e?.statusCode ?? 404;
return {
statusCode,
hasGetInitialPropsRun: true,
- err: err || undefined
+ err: e
};Also applies to: 73-79
🤖 Prompt for AI Agents
In apps/web/client/src/pages/_error.tsx around lines 4-9 (and also update lines
73-79), the code accesses err.statusCode but the base Error type doesn't include
statusCode; define a narrowed error type (e.g., interface NextError extends
Error { statusCode?: number }) and use that type for ErrorProps.err and in
getInitialProps cast or type-guard the incoming err to NextError before reading
statusCode; update all err usages to use the narrowed type (or optional
chaining) so TypeScript accepts accessing statusCode.
| err?: Error; | ||
| } | ||
|
|
||
| function Error({ statusCode, hasGetInitialPropsRun, err }: ErrorProps) { |
There was a problem hiding this comment.
Rename component to avoid shadowing global Error (linter error)
Biome flags shadowing of the global Error constructor. Rename the component and its static getInitialProps.
-function Error({ statusCode, hasGetInitialPropsRun, err }: ErrorProps) {
+function ErrorPage({ statusCode, hasGetInitialPropsRun, err }: ErrorProps) {-Error.getInitialProps = ({ res, err }: NextPageContext): ErrorProps => {
+ErrorPage.getInitialProps = ({ res, err }: NextPageContext): ErrorProps => {-export default Error;
+export default ErrorPage;Also applies to: 73-79, 82-82
🧰 Tools
🪛 Biome (2.1.2)
[error] 10-10: Do not shadow the global "Error" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🤖 Prompt for AI Agents
In apps/web/client/src/pages/_error.tsx around line 10 (and also affecting lines
73-79 and 82), the component is named "Error" which shadows the global Error
constructor and triggers a linter flag; rename the component (for example to
"ErrorPage") and update the static getInitialProps method to attach to the new
component name, update the default export to the new name, and adjust any
internal references or prop types that reference the old name so all occurrences
(declaration, static method, and export) are consistently renamed.
| function Error({ statusCode, hasGetInitialPropsRun, err }: ErrorProps) { | ||
| const getErrorMessage = (code: number) => { | ||
| switch (code) { | ||
| case 404: | ||
| return 'This page could not be found'; | ||
| case 500: | ||
| return 'Internal Server Error'; | ||
| case 403: | ||
| return 'Access Forbidden'; | ||
| case 401: | ||
| return 'Authentication Required'; | ||
| default: | ||
| return 'An unexpected error has occurred'; | ||
| } | ||
| }; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use hasGetInitialPropsRun to rethrow client-side errors (Next.js-recommended pattern)
The prop is defined but unused. Add the rethrow to ensure client-side errors get surfaced to the runtime/monitoring.
-function ErrorPage({ statusCode, hasGetInitialPropsRun, err }: ErrorProps) {
- const getErrorMessage = (code: number) => {
+function ErrorPage({ statusCode, hasGetInitialPropsRun, err }: ErrorProps) {
+ if (!hasGetInitialPropsRun && err) {
+ // Let Next.js handle this (useful for error tracking integrations)
+ throw err;
+ }
+ const getErrorMessage = (code: number) => {Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (2.1.2)
[error] 10-10: Do not shadow the global "Error" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🤖 Prompt for AI Agents
In apps/web/client/src/pages/_error.tsx around lines 10 to 25, the
hasGetInitialPropsRun prop is defined but unused; implement the
Next.js-recommended client-side rethrow so runtime/monitoring surface client
errors: after computing or inside the component, if hasGetInitialPropsRun is
false and err is present, rethrow err (throw err) to let the error propagate on
the client; keep the existing error message logic intact so server-rendered
paths still return the friendly message.
Description
Related Issues
Type of Change
This PR implements user experience and security improvements to the web client, focusing on enhanced error handling and security configurations:
Enhanced Error Page : Completely redesigned the error page ( _error.tsx ) with:
Security Headers : Added essential security headers in Next.js config:
Testing
Tested with Bun runtime:
Screenshots (if applicable)
Additional Notes
Important
Improves error handling and security in the web client by redesigning the error page and adding security headers in
next.config.ts._error.tsxwith Tailwind CSS styling, specific messages for HTTP status codes (404, 500, 403, 401), and action buttons.NextPageContext.next.config.ts:X-Frame-Options: DENY,X-Content-Type-Options: nosniff,Referrer-Policy: strict-origin-when-cross-origin.next.config.ts.@onlook/uiandlucide-reactinnext.config.ts.devIndicators.buildActivitysetting innext.config.ts.This description was created by
for 8652ec9. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit