feat: create generic error page #113
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a generic error page infrastructure for handling runtime errors in the application. The changes enable Next.js error boundaries at both the root and catalog route levels, replacing silent error handling with proper error boundaries.
- Added error boundary pages for root and catalog routes
- Created a new error illustration component to visually represent error states
- Modified server fetching logic to throw errors instead of silently returning empty arrays
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/mocks/handlers.ts | Adds a mock scenario for simulating 500 server errors during testing |
| src/components/illustrations/illustration-error.tsx | New SVG illustration component for displaying on error pages |
| src/components/error-page/error-page.tsx | Updates the ErrorPage component to use the new error illustration instead of the location illustration |
| src/app/error.tsx | New root-level error boundary with reset functionality and full page layout |
| src/app/catalog/error.tsx | New catalog-level error boundary with reset functionality |
| src/app/catalog/actions.ts | Changes getServers to throw errors instead of returning empty arrays on API failures |
|
@claude can you please do a deep code review |
|
Claude finished @kantord's task —— View job Deep Code Review Complete ✅I've completed a thorough review of the error page implementation. This is a solid foundation for error handling in the Next.js App Router, but there are several areas that need attention before merging. 🎯 Overall AssessmentStrengths:
Areas for Improvement:
🔴 Critical Issues1. Missing Test Coverage (High Priority)Files: The codebase has comprehensive test coverage for similar components ( Required tests: // src/app/__tests__/error.test.tsx
describe("RootErrorPage", () => {
it("logs errors to console on mount", () => {
const consoleError = vi.spyOn(console, "error");
const error = new Error("Test error");
render(<RootErrorPage error={error} reset={vi.fn()} />);
expect(consoleError).toHaveBeenCalledWith(error);
});
it("calls reset function when Try again button is clicked", async () => {
const reset = vi.fn();
render(<RootErrorPage error={new Error("Test")} reset={reset} />);
await userEvent.click(screen.getByRole("button", { name: /try again/i }));
expect(reset).toHaveBeenCalledTimes(1);
});
it("displays NavbarLogo in header", () => {
// Test full page layout including header
});
it("does not expose error message to user", () => {
const error = new Error("Sensitive internal error");
render(<RootErrorPage error={error} reset={vi.fn()} />);
expect(screen.queryByText(/sensitive internal error/i)).not.toBeInTheDocument();
});
});Similar tests needed for Rationale: According to CLAUDE.md testing guidelines, error scenarios and critical user interactions must be tested. Error boundaries prevent app crashes and need comprehensive coverage. 2. Code Duplication (DRY Violation) (Medium Priority)Files: The error logging logic is duplicated across both error boundaries: // Duplicated in both files
useEffect(() => {
console.error(error);
}, [error]);Recommendation: Extract to a shared custom hook: // src/hooks/use-log-error.ts
import { useEffect } from "react";
export function useLogError(error: Error) {
useEffect(() => {
console.error(error);
}, [error]);
}
// Usage in error.tsx
import { useLogError } from "@/hooks/use-log-error";
export default function RootErrorPage({ error, reset }: ErrorProps) {
useLogError(error);
// ...
}Rationale: CLAUDE.md states "Keep code DRY - Extract repeated logic". This also makes it easier to enhance error logging later (e.g., add error monitoring service). 3. Generic Error Messages Lacking Context (Medium Priority)File: throw new Error("Failed to fetch servers");This error message provides no debugging context. When errors bubble up to the error boundary, developers need actionable information. Recommendation: if (resp.error) {
console.error("[catalog] Failed to fetch servers:", resp.error);
throw new Error(
`Failed to fetch servers: ${resp.error.message ?? "Unknown error"}`,
{ cause: resp.error }
);
}Benefits:
Rationale: CLAUDE.md requires "Handle errors properly - Always show user-friendly error messages" but also emphasizes debugging. This approach does both.
|
| Category | Rating | Notes |
|---|---|---|
| TypeScript | ⭐⭐⭐⭐⭐ | Strict mode compliant, no any types |
| Next.js Patterns | ⭐⭐⭐⭐⭐ | Correct App Router error boundary implementation |
| Test Coverage | ⭐⭐☆☆☆ | Critical gap: no tests for error boundaries |
| Error Handling | ⭐⭐⭐⭐☆ | Good strategy, but messages lack context |
| Code Reuse (DRY) | ⭐⭐⭐☆☆ | Duplicated error logging logic |
| UI/UX | ⭐⭐⭐⭐☆ | Good illustration, clear messaging, could improve button UX |
| Security | ⭐⭐⭐⭐☆ | No security issues, error messages appropriately generic |
| Documentation | ⭐⭐⭐☆☆ | Could use JSDoc comments explaining layout differences |
Overall: ⭐⭐⭐⭐☆ (4/5)
This is solid work that implements error boundaries correctly. The main blocker is the missing test coverage. Once tests are added and the duplication is resolved, this will be production-ready.
📝 Summary
This PR successfully implements error boundaries for the Next.js App Router, replacing silent failures with proper user-facing error pages. The implementation is architecturally sound and follows Next.js best practices.
Key strengths:
- Correct error boundary pattern
- Beautiful error illustration
- Good testing scenario setup
- Improved error visibility
Must address before merge:
- Add comprehensive test coverage
- Extract duplicated error logging logic
Consider for future:
- Enhanced error messages with context
- Error monitoring integration
- Improved recovery UX
The screenshot shows the error page looks great! The cloud illustration with X eyes is a nice touch that communicates the error state clearly without being alarming.
Great work on this feature! Let me know if you'd like me to help implement any of these recommendations. 🚀
Job: View run
Branch: error-page
|
@claude can you please do a final deep review? |
|
Claude finished @kantord's task —— View job Final Deep Review ✅I've completed a comprehensive final review of the error page implementation. This PR has significantly improved since the initial review and addresses most of the previous concerns effectively. 🎯 Executive SummaryOverall Assessment: ⭐⭐⭐⭐⭐ Production-Ready The error page implementation is now in excellent shape. The refactoring demonstrates strong architectural decisions, comprehensive test coverage, and adherence to all CLAUDE.md guidelines. The code is clean, maintainable, and follows Next.js App Router best practices. Key Improvements Since Initial Review:
✨ What's Excellent1. Outstanding Architecture & Refactoring 🏗️The component separation is brilliantly executed: Why this is great:
This pattern should serve as a template for future similar components in the codebase. 2. Comprehensive Test Coverage ✅The test suite is thorough and well-structured:
Test Quality Highlights: it("does not expose error message to user", () => {
const error = new Error("Sensitive internal error details");
render(<ErrorPage error={error} reset={vi.fn()} />);
expect(
screen.queryByText(/sensitive internal error/i),
).not.toBeInTheDocument();
});This security-focused test is excellent and demonstrates mature testing practices. 👏 3. Zero Code Duplication (DRY Principle) ♻️The previous review identified duplicated error logging logic. This has been completely resolved: Before (duplicated across multiple files): // In error.tsx
useEffect(() => {
console.error(error);
}, [error]);
// In catalog/error.tsx
useEffect(() => {
console.error(error);
}, [error]);After (single source of truth): // In error-page-client.tsx (used by all error boundaries)
export function ErrorPage({ error, reset }: ErrorPageProps) {
useEffect(() => {
console.error(error);
}, [error]);
// ...
}Both 4. Proper Next.js App Router Error Boundaries ⚡Both error boundaries are correctly implemented: Root Error Boundary ( "use client";
export default function RootErrorPage({ error, reset }: ErrorProps) {
return <ErrorPage error={error} reset={reset} />;
}Catalog Error Boundary ( "use client";
export default function CatalogErrorPage({ error, reset }: ErrorProps) {
return <ErrorPage error={error} reset={reset} />;
}Why this is correct:
The architecture allows for future customization if needed (e.g., different messages per route) while maintaining DRY principles. 5. Beautiful Error Illustration 🎨The
The screenshots show the error page looks polished and professional. The centered layout with the illustration creates a calming error experience rather than an alarming one. 6. Improved Error Handling Strategy 💪In Before (from previous implementation, implicit): if (resp.error) {
console.error("...");
return []; // Silent failure - BAD
}After: if (resp.error) {
console.error("[catalog] Failed to fetch servers:", resp.error);
throw resp.error; // Explicit error - GOOD
}Why this is better:
This change alone improves user experience significantly. Users will understand that an error occurred rather than thinking there are simply no servers available. 7. Excellent Mock Scenario for Testing 🧪In mockScenario("server-error").get("*/registry/v0.1/servers", () => {
return HttpResponse.json(
{ error: "Internal Server Error" },
{ status: 500 },
);
}),This mock scenario enables:
Usage: Set a cookie 8. Consistent Not Found Pages 🎯The refactoring also improved not-found pages:
This creates a unified error experience across the application. 🔍 Minor Observations (Not Blockers)1. Error Digest Not UsedThe error boundaries accept a interface ErrorProps {
error: Error & { digest?: string };
reset: () => void;
}However, Potential future improvement: // In error-page-client.tsx
useEffect(() => {
console.error(error);
if (error.digest) {
console.error(`Error digest: ${error.digest}`);
}
}, [error]);Or display it to users for support: {error.digest && (
<p className="text-xs text-muted-foreground mt-2">
Error ID: {error.digest}
</p>
)}Recommendation: This is low priority. Only add if your support team needs it for debugging. 2. No Integration TestsThe unit tests are comprehensive, but there are no integration tests that verify the error boundary actually catches errors from the server action. Potential future test (optional): // src/app/catalog/__tests__/error-boundary-integration.test.tsx
describe("Catalog error boundary integration", () => {
it("shows error page when getServers fails", async () => {
// Mock getServers to throw
vi.mocked(getServers).mockRejectedValue(
new Error("API failure")
);
render(<CatalogPage />);
await waitFor(() => {
expect(
screen.getByText(/something went wrong/i)
).toBeInTheDocument();
});
});
});Status: Not required for this PR. This could be added later if integration test coverage is desired. 3. Error Context in ActionsThe error thrown in Current: throw resp.error;Potential enhancement: throw new Error(
`Failed to fetch servers: ${resp.error.message ?? "Unknown error"}`,
{ cause: resp.error }
);However, given that the current approach preserves the original error object (which may have additional properties), the current implementation is actually preferable. The Verdict: Current approach is fine as-is. 📋 Compliance ChecklistChecking against CLAUDE.md requirements:
Overall Compliance: 10/10 ✅ 🚀 RecommendationsPre-Merge: None ✅This PR is ready to merge as-is. All critical issues from the initial review have been addressed. Post-Merge Enhancements (Optional, Low Priority):
📊 Code Quality Assessment
Overall Rating: ⭐⭐⭐⭐⭐ (5/5) - Excellent 🎉 SummaryThis PR represents high-quality work that:
Key Achievement: The refactoring from the initial implementation to this final version shows excellent software engineering practices. The separation of The screenshots demonstrate that the error page looks polished and professional. The cloud illustration with X eyes is charming and communicates the error state clearly without being alarming. ✅ Final Verdict: APPROVED - Ready to MergeGreat work on this feature! 🚀 Branch: |
Uh oh!
There was an error while loading. Please reload this page.