Conversation
|
Warning Rate limit exceeded@prxt6529 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 27 minutes and 11 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (5)
WalkthroughRemoved legacy Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Browser
participant Route as Next Route
participant Layout as LayoutWrapper (ErrorBoundary)
participant Extract as extractErrorDetails
participant ErrorComp as ErrorComponent
participant Page as app/error/page.tsx
participant GlobalErr as app/global-error.tsx
User->>Browser: navigate /page or /error?stack=...
Browser->>Route: render route
Route->>Layout: render children (wrapped in ErrorBoundary)
alt route throws during render
Layout->>Extract: extractErrorDetails(error)
Extract-->>ErrorComp: details string
ErrorComp-->>User: show fallback UI (toggle, copy, digest)
else explicit error route (/error)
User->>Page: request with searchParams
Page->>ErrorComp: pass stackTrace from searchParams
ErrorComp-->>User: render error UI with toggleable stack & copy
else global runtime error
Route->>GlobalErr: render global error component
GlobalErr->>Extract: extractErrorDetails(error, "GLOBAL_ERROR")
Extract-->>ErrorComp: details string
ErrorComp-->>User: render global error UI
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Focus points:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
components/not-found/NotFound.tsx (1)
8-19: Consider memoizingtitleLabelto prevent unnecessary re-renders.The
titleLabelis computed on every render, causing theuseEffectto run each time even whenlabelhasn't changed. WhilesetTitlelikely deduplicates, this still creates unnecessary work.Apply this diff to memoize the computed title:
+"use client"; + +import { useTitle } from "@/contexts/TitleContext"; +import Image from "next/image"; +import { useEffect, useMemo } from "react"; + export default function NotFound({ label }: { readonly label?: string }) { - let titleLabel = "PAGE NOT FOUND"; - if (label) { - titleLabel = `${label} OR ${titleLabel}`; - } - - titleLabel = `404 | ${titleLabel}`; + const titleLabel = useMemo(() => { + let computed = "PAGE NOT FOUND"; + if (label) { + computed = `${label} OR ${computed}`; + } + return `404 | ${computed}`; + }, [label]); const { setTitle } = useTitle();app/global-error.tsx (1)
12-12: Consider using theresetprop or removing it.The
resetparameter is defined but never used. If recovery functionality is not needed in the global error handler, the prop can be removed from the signature. Otherwise, consider adding a retry button that callsreset().app/error/page.tsx (1)
2-7: AlignsearchParamsoptionality with runtime expectations
ErrorPagePropsallowssearchParamsto beundefined, but the implementation unconditionally doesawait searchParams, which would throw if it were ever omitted (e.g., in non‑Next usage/tests). In a real Next app router page this is probably always defined, so this is more of a defensive mismatch between the type and the implementation than a bug today.You could either:
- Make
searchParamsrequired in the type, or- Guard the await:
export default async function ErrorPage({ searchParams }: ErrorPageProps) { const resolvedSearchParams = searchParams ? await searchParams : undefined; const stackTraceParam = resolvedSearchParams?.stack ?? null; return ( <main className={styles.main}> <Suspense fallback={null}> <ErrorComponent stackTrace={stackTraceParam} /> </Suspense> </main> ); }This keeps the prop type and runtime behavior in sync while still passing the resolved
stackTracedown.Please confirm this pattern matches the
searchParamscontract for the Next.js version used in this repo.Also applies to: 9-10, 15-15
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
__tests__/components/error/Error.test.tsx(4 hunks)app/[user]/not-found.tsx(1 hunks)app/error-page.tsx(0 hunks)app/error/page.tsx(1 hunks)app/global-error.tsx(1 hunks)app/layout.tsx(1 hunks)app/not-found.tsx(1 hunks)components/error/Error.tsx(1 hunks)components/not-found/NotFound.tsx(1 hunks)components/providers/LayoutErrorFallback.tsx(1 hunks)components/providers/LayoutWrapper.tsx(3 hunks)utils/error-extractor.ts(1 hunks)
💤 Files with no reviewable changes (1)
- app/error-page.tsx
🧰 Additional context used
🧬 Code graph analysis (8)
components/providers/LayoutErrorFallback.tsx (2)
utils/error-extractor.ts (1)
extractErrorDetails(1-66)components/error/Error.tsx (1)
ErrorComponent(14-95)
components/providers/LayoutWrapper.tsx (2)
components/providers/LayoutErrorFallback.tsx (1)
LayoutErrorFallback(8-16)FooterWrapper.tsx (1)
FooterWrapper(18-68)
app/layout.tsx (1)
components/providers/LayoutWrapper.tsx (1)
LayoutWrapper(15-96)
components/not-found/NotFound.tsx (1)
contexts/TitleContext.tsx (1)
useTitle(186-192)
__tests__/components/error/Error.test.tsx (1)
components/error/Error.tsx (1)
ErrorComponent(14-95)
app/error/page.tsx (1)
components/error/Error.tsx (1)
ErrorComponent(14-95)
components/error/Error.tsx (1)
contexts/TitleContext.tsx (1)
useTitle(186-192)
app/global-error.tsx (2)
utils/error-extractor.ts (1)
extractErrorDetails(1-66)components/error/Error.tsx (1)
ErrorComponent(14-95)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (10)
components/providers/LayoutWrapper.tsx (1)
88-93: LGTM! ErrorBoundary integration is well-implemented.The ErrorBoundary correctly wraps the children and FooterWrapper, with pathname as a reset key to clear errors on navigation. This provides localized error handling within the layout.
app/layout.tsx (1)
55-55: LGTM! Error boundary correctly moved to LayoutWrapper.The removal of the ErrorBoundary wrapper here aligns with the refactoring goal, as LayoutWrapper now manages its own error boundary internally via LayoutErrorFallback.
utils/error-extractor.ts (1)
1-66: LGTM! Comprehensive and defensive error extraction utility.The implementation correctly extracts error details with:
- Coverage of standard Error properties (message, stack, name, cause)
- Support for Next.js digest property
- Dynamic handling of additional properties
- Defensive fallback chain for stringification
- Optional context-based logging for debugging
This provides a solid foundation for the enhanced error handling across the application.
components/providers/LayoutErrorFallback.tsx (1)
8-13: ErrorComponent correctly handles null stackTrace—no fix required.The verification confirms that
ErrorComponentgracefully handles a nullstackTraceprop:
- The prop is typed as
string | null(line 11 incomponents/error/Error.tsx)- Line 30 uses nullish coalescing to fall back to
stackTraceFromQueryifstackTraceis null- Line 66 conditionally renders the stacktrace UI only if
hasStackTraceis true- No errors or undefined behavior occurs when
stackTraceis null__tests__/components/error/Error.test.tsx (3)
1-32: Solid test setup for title + search params mockingThe combination of
mockSearchParamswith theuseSearchParamsmock and resetting it inbeforeEachgives each test a clean, controllable URL state while still checking the title behavior viasetTitleMock. This looks correct and isolated.
43-52: Prop‑driven stack trace path is well coveredThis test nicely asserts the happy path where a
stackTraceprop is provided: it verifies the toggle label and that the stack content appears specifically inside a<pre>when expanded. This directly guards the new prop API onErrorComponent.
54-63: URL‑driven stack trace fallback behavior is exercised correctlyBy overriding
mockSearchParamsbefore rendering and omitting thestackTraceprop, this test validates the fallback resolution from thestackquery parameter and that it only appears after toggling the button. Good coverage of the second resolution path.components/error/Error.tsx (3)
10-12: Stack trace resolution logic is clear and extensibleThe new
ErrorComponentPropsplusconst stackTraceFromQuery = useMemo( () => searchParams?.get("stack") ?? "", [searchParams] ); const resolvedStackTrace = stackTrace ?? stackTraceFromQuery; const hasStackTrace = Boolean(resolvedStackTrace);give a clear precedence (prop first, then URL) while allowing
nullto explicitly suppress display. This matches the new tests and keeps the component flexible for both page‑level and global error use.Also applies to: 14-21, 26-31
34-64: Improved error page UX and messagingThe centered layout, dedicated error headline (“Page of Doom”), emoji, and support mailto link make the error state much clearer without adding complexity to the component. All of this sits behind the existing title‑setting effect, so behavior is preserved while the UI is upgraded.
66-91: Accessible stacktrace toggle is well implementedThe collapsible stacktrace block uses local state, a stable
useIdforaria-controls, andaria-expandedon the button, plus a<pre>for the stack content. This is a solid, accessible pattern and cleanly encapsulates the new “Show/Hide stacktrace” behavior.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/not-found.tsx (3)
8-14: Unreachable code due to throw statement on line 6.This entire return block is dead code and will never execute because of the
throwstatement on line 6. Once the throw is removed, this code will become reachable again.
17-19: Incorrect metadata pattern for Next.js App Router.In Next.js App Router, metadata cannot be defined as a static property on the component. Instead, you should export a
metadataobject or usegenerateMetadatafunction.Apply this diff to fix the metadata export:
-NotFoundPage.metadata = { - title: "404 - NOT FOUND", -}; +export const metadata = { + title: "404 - NOT FOUND", +};
10-12: Remove unnecessary Suspense wrapper.The
NotFoundcomponent uses only synchronous client-side logic (useEffectand context) with no async operations oruse()hooks. The Suspense wrapper can be removed.
♻️ Duplicate comments (1)
app/global-error.tsx (1)
49-58: Global error rendering is correct, but consider dedicated styles.The rendering structure is correct for a Next.js global error boundary:
lang="en"attribute is present (addresses the past review comment)- ErrorComponent correctly receives error details
However, using
styles.mainfromHome.module.scss(line 52) is semantically odd for a global error page. Consider creating dedicated error page styles or using inline Tailwind classes for consistency with the ErrorComponent's styling.
🧹 Nitpick comments (5)
components/error/Error.tsx (4)
26-31: Consider returningnullinstead of empty string for consistency.Line 27 returns an empty string (
"") when thestackquery parameter is not present, but the prop type isstring | null. For consistency with the type definition, consider returningnull:const stackTraceFromQuery = useMemo(() => { - return searchParams?.get("stack") ?? ""; + return searchParams?.get("stack") ?? null; }, [searchParams]);This would make the fallback behavior more predictable and type-consistent.
36-45: Remove redundantloading="eager"prop.When
priorityis set totrue, Next.js automatically uses eager loading. The explicitloading="eager"on line 39 is redundant.<Image unoptimized priority - loading="eager" width="0" height="0" style={{ height: "auto", width: "100px" }} src="/SummerGlasses.svg" alt="SummerGlasses" />
50-54: Add explicit dimensions to emoji image for better performance.The emoji
<img>element lacks explicitwidthandheightattributes, which can cause layout shift. Consider adding them:<img src="/emojis/sgt_grimacing.webp" alt="sgt_grimacing" + width="32" + height="32" className="tw-w-8 tw-h-8" />
66-91: Excellent accessibility implementation for collapsible stacktrace.The stacktrace toggle is well-implemented with:
- Proper ARIA attributes (
aria-expanded,aria-controls)- Semantic button with
type="button"- Unique ID via
useId()for proper association- Appropriate styling for code display (mono font, pre-wrap)
Consider adding
tw-cursor-pointerto the button className (line 71) to improve UX by indicating the button is clickable.app/global-error.tsx (1)
13-48: Consider exposing theresetcallback to users.The
resetprop (line 10, line 13) is provided by Next.js to allow retrying after an error, but it's currently unused. Consider adding a "Try Again" button in the UI that callsreset()to give users a way to recover from the error.Additionally, the extensive logging in this
useEffect(lines 14-45) combined with callingextractErrorDetails(error, "GLOBAL_ERROR")on line 47 results in duplicate logging, sinceextractErrorDetailsalso logs when a context is provided. Consider either:
- Removing the
useEffectlogging and relying solely onextractErrorDetails- Not passing the context to
extractErrorDetailsto avoid duplication- const errorDetails = extractErrorDetails(error, "GLOBAL_ERROR"); + const errorDetails = extractErrorDetails(error);Then keep the detailed logging in the
useEffectif needed for global error debugging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
app/global-error.tsx(1 hunks)app/not-found.tsx(1 hunks)components/error/Error.tsx(1 hunks)package.json(3 hunks)utils/error-extractor.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
components/error/Error.tsx (1)
contexts/TitleContext.tsx (1)
useTitle(186-192)
app/global-error.tsx (2)
utils/error-extractor.ts (1)
extractErrorDetails(1-138)components/error/Error.tsx (1)
ErrorComponent(14-95)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
package.json (1)
74-76: Reown package versions pinned to v1.8.14.The three Reown/AppKit packages are updated to the same patch version (v1.8.14). Ensure all three are compatible and that your AppKit initialization code remains compatible with this version.
Verify there are no compatibility warnings or breaking changes in the AppKit v1.8.14 release.
utils/error-extractor.ts (1)
27-137: Comprehensive error extraction with good defensive coding.The error detail extraction logic is well-structured with thorough type checking and error handling. The fallback mechanisms (lines 133-137) ensure the function always returns something useful.
However, consider that the extracted details may include sensitive information (stack traces, error properties, etc.) that should not be exposed to end users in production. Ensure this output is used appropriately based on the environment.
app/global-error.tsx (1)
1-11: Correct setup for Next.js global error boundary.The component structure follows Next.js conventions:
"use client"directive is required for global error boundaries- Props type correctly includes
errorandresetparameters- Imports are appropriate
prxt6529
force-pushed
the
better-error-handlers-2
branch
from
b4a8ab1 to
e7c2de9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/[user]/not-found.tsx (1)
6-7: This test artifact must still be removed before merging.The debug throw will cause the user not-found page to error on every access, making the return statement and NotFound component unreachable. This breaks production functionality.
Apply this diff to remove the test error:
export default function UserNotFoundPage() { - throw new Error("Test error"); - return (
🧹 Nitpick comments (2)
components/error/Error.tsx (2)
14-16: Consider simplifying the component signature.The default parameter
= {}works but may be unnecessary sincestackTraceis already optional in the type definition. React 19 components typically receive props directly without requiring empty object defaults.Consider this simpler signature:
-export default function ErrorComponent({ - stackTrace, -}: ErrorComponentProps = {}) { +export default function ErrorComponent({ stackTrace }: ErrorComponentProps) {This is cleaner and achieves the same behavior since
stackTraceis already optional inErrorComponentProps.
36-45: Consider refining Image component props.Several minor optimization opportunities:
- Both
priorityandloading="eager"are set, which is redundant—priorityalready implies eager loading- The pattern
width="0" height="0"with a style override is unconventional; consider using actual dimensionsSuggested refinement:
<Image unoptimized priority - loading="eager" - width="0" - height="0" - style={{ height: "auto", width: "100px" }} + width={100} + height={100} src="/SummerGlasses.svg" alt="SummerGlasses" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/[user]/not-found.tsx(1 hunks)components/error/Error.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
components/error/Error.tsx (1)
contexts/TitleContext.tsx (1)
useTitle(186-192)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
components/error/Error.tsx (2)
26-31: LGTM!The stacktrace resolution logic is well-structured:
- Correctly prioritizes the
stackTraceprop over the URL query parameter- Uses
useMemoappropriately to avoid re-computing on every render- The fallback to empty string and subsequent
Booleancheck ensures proper conditional rendering
66-91: Excellent accessibility implementation!The collapsible stacktrace section demonstrates best practices:
- Proper ARIA attributes (
aria-expanded,aria-controls) linked to unique IDs viauseId- Semantic button with
type="button"to prevent unintended form submission- Clear visual feedback with chevron icons
- Proper formatting preservation with
tw-whitespace-pre-wrap
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
utils/error-extractor.ts (1)
52-103: Consider extracting the stringification logic into a helper function.The type-checking and stringification pattern is duplicated between cause handling (lines 54-68) and additional properties handling (lines 83-97). Extracting this into a helper function would improve maintainability and adhere to DRY principles.
Example refactor:
function safeStringify(value: unknown): string { if (value === null || value === undefined) { return String(value); } if (typeof value === "object") { return JSON.stringify(value, null, 2); } if ( typeof value === "string" || typeof value === "number" || typeof value === "boolean" || typeof value === "bigint" || typeof value === "symbol" ) { return String(value); } return JSON.stringify(value, null, 2); }Then simplify the cause handling:
if (err.cause) { - let causeString: string; - if (err.cause !== null && typeof err.cause === "object") { - causeString = JSON.stringify(err.cause, null, 2); - } else if ( - typeof err.cause === "string" || - typeof err.cause === "number" || - typeof err.cause === "boolean" || - typeof err.cause === "bigint" || - typeof err.cause === "symbol" || - err.cause === null || - err.cause === undefined - ) { - causeString = String(err.cause); - } else { - causeString = JSON.stringify(err.cause, null, 2); - } + const causeString = safeStringify(err.cause); parts.push(`\n\nCause: ${causeString}`); }And the additional properties loop:
try { const value = (err as unknown as Record<string, unknown>)[prop]; - let valueString: string; - if (value === null || value === undefined) { - valueString = String(value); - } else if (typeof value === "object") { - valueString = JSON.stringify(value, null, 2); - } else if ( - typeof value === "string" || - typeof value === "number" || - typeof value === "boolean" || - typeof value === "bigint" || - typeof value === "symbol" - ) { - valueString = String(value); - } else { - valueString = JSON.stringify(value, null, 2); - } + const valueString = safeStringify(value); parts.push(` ${prop}: ${valueString}`); } catch { parts.push(` ${prop}: [unable to stringify]`); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/global-error.tsx(1 hunks)components/error/Error.tsx(1 hunks)utils/error-extractor.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/global-error.tsx
- components/error/Error.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
utils/error-extractor.ts (2)
7-14: Production logging guard implemented correctly.The conditional logging block appropriately prevents verbose error details from appearing in production logs while preserving debugging capability in development. This addresses the previous review concern.
16-114: Comprehensive and robust error extraction implementation.The function handles error details thoroughly with thoughtful considerations:
- Special handling for generic Next.js server errors with digest guidance
- Stack trace deduplication to avoid redundant output
- Graceful fallbacks at multiple levels (catch blocks, final stringify attempt)
- Filters standard properties to highlight additional context
The implementation is production-ready and provides valuable debugging information while maintaining code safety.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/[user]/not-found.tsx (1)
6-7: Remove the debug throw so the NotFound page can render.
throw new Error("test");runs unconditionally, so the function never reaches the JSXreturnand the user 404 UI is never displayed. This looks like a leftover debug artifact and effectively reintroduces the same issue previously flagged on this file.Suggested fix:
export default function UserNotFoundPage() { - throw new Error("test"); - return (
🧹 Nitpick comments (1)
components/error/Error.tsx (1)
36-45: Minor: Redundantloadingprop when usingpriority.When
priorityis set, Next.js automatically uses eager loading, making the explicitloading="eager"prop redundant (though harmless).Apply this diff to remove the redundant prop:
<Image unoptimized priority - loading="eager" width="0" height="0" style={{ height: "auto", width: "100px" }} src="/SummerGlasses.svg" alt="SummerGlasses" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/[user]/not-found.tsx(1 hunks)app/global-error.tsx(1 hunks)components/error/Error.tsx(1 hunks)utils/error-extractor.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/global-error.tsx
- utils/error-extractor.ts
🧰 Additional context used
🧬 Code graph analysis (1)
components/error/Error.tsx (1)
contexts/TitleContext.tsx (1)
useTitle(186-192)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
components/error/Error.tsx (5)
1-16: LGTM! Clean component structure and type safety.The component signature correctly accepts optional props with a sensible default, and the imports follow Next.js 15 App Router patterns appropriately.
17-31: Solid hook usage and stack trace resolution logic.The defensive optional chaining on line 27 appropriately handles edge cases where
searchParamsmight be null during prerendering. The precedence logic (props over query params) is clear and correct.
46-64: Excellent UX: Clear messaging and actionable support contact.The error message provides users with a clear path forward and direct support contact. The use of a regular
<img>tag for the small emoji is a reasonable optimization choice.
66-91: Excellent accessibility implementation for collapsible stacktrace.The stacktrace UI demonstrates best practices:
- Proper ARIA attributes (
aria-expanded,aria-controls) for screen reader support- Unique ID generation via
useId()for robust element relationships- Safe text rendering (React automatically escapes content, preventing XSS)
- Clear visual affordances with toggle labels and chevron icons
33-34: LGTM! Standard centered layout pattern.The section wrapper provides proper full-viewport centering with appropriate semantic HTML.
Also applies to: 92-94
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/[user]/not-found.tsx (1)
6-7: 🚨 CRITICAL: Remove debug throw statement before merging.This
throw new Error("test");statement will crash the user not-found page on every access, making the return statement unreachable. This was previously flagged and marked as addressed, but it still appears in the current code.Apply this diff to remove the debug artifact:
export default function UserNotFoundPage() { - throw new Error("test"); - return (
🧹 Nitpick comments (1)
components/not-found/NotFound.tsx (1)
8-15: Consider simplifying the title construction logic.While the current implementation works correctly, the two-step mutation of
titleLabelcould be streamlined for better readability.Apply this diff for a more declarative approach:
- let titleLabel; - if (label) { - titleLabel = label.toUpperCase(); - } else { - titleLabel = "PAGE"; - } - - titleLabel = `404 | ${titleLabel} NOT FOUND`; + const titleLabel = `404 | ${label ? label.toUpperCase() : "PAGE"} NOT FOUND`;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
__tests__/components/error/Error.test.tsx(4 hunks)__tests__/components/not-found/NotFound.test.tsx(2 hunks)__tests__/moreStaticPages.test.tsx(1 hunks)app/[user]/not-found.tsx(1 hunks)components/distribution-plan-tool/create-snapshots/form/CreateSnapshotFormSearchCollectionMemesModal.tsx(1 hunks)components/error/Error.tsx(1 hunks)components/not-found/NotFound.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
components/error/Error.tsx (1)
contexts/TitleContext.tsx (1)
useTitle(186-192)
__tests__/components/error/Error.test.tsx (1)
components/error/Error.tsx (1)
ErrorComponent(14-95)
components/not-found/NotFound.tsx (1)
contexts/TitleContext.tsx (1)
useTitle(186-192)
app/[user]/not-found.tsx (1)
components/not-found/NotFound.tsx (1)
NotFound(7-51)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (16)
components/distribution-plan-tool/create-snapshots/form/CreateSnapshotFormSearchCollectionMemesModal.tsx (1)
89-90: Text color change looks good and consistentUsing
tw-text-whiteon the heading paragraph matches the legend styling below and keeps the modal copy visually consistent. No functional impact.__tests__/moreStaticPages.test.tsx (1)
90-103: LGTM - Test simplification aligns with component changes.The test correctly focuses on content verification after the removal of navigation elements from the NotFound component. The assertions appropriately cover the main 404 message and visual assets.
components/not-found/NotFound.tsx (1)
26-35: LGTM - Image optimization is appropriate.The use of
priorityandeagerloading for the above-the-fold image is correct for a 404 page, ensuring the visual feedback appears immediately.app/[user]/not-found.tsx (1)
11-11: LGTM - Label update improves clarity.The change from "USER" to "USER OR PAGE" better describes what might be missing when this not-found page is triggered.
__tests__/components/not-found/NotFound.test.tsx (2)
45-50: LGTM - Test expectations correctly updated.The test expectations properly reflect the component's new title format using "|" instead of "-", maintaining appropriate coverage for the default label scenario.
60-66: LGTM - Custom label test updated correctly.The test properly verifies title uppercasing and the new delimiter format for custom labels.
__tests__/components/error/Error.test.tsx (5)
31-31: LGTM - Proper test isolation.Initializing a fresh
URLSearchParamsinstance inbeforeEachensures each test starts with a clean state, preventing test interdependencies.
43-52: LGTM - Stack trace prop test is well-structured.The test correctly verifies that a stack trace provided via props is displayed when toggled, covering the primary use case.
54-62: LGTM - URL parameter test validates fallback behavior.This test properly verifies that the component reads stack traces from URL query parameters when no prop is provided, covering the secondary data source.
64-70: LGTM - Negative test case improves robustness.Verifying that the stack trace UI is not rendered when no stack trace is available prevents unnecessary UI elements and improves user experience.
72-88: LGTM - Toggle behavior thoroughly validated.This test comprehensively verifies the show/hide cycle, checking both the visibility of the stack trace content and the button label changes. The assertions properly cover the complete user interaction flow.
components/error/Error.tsx (5)
10-16: LGTM - Clean type definition and signature.The component signature properly accepts an optional
stackTraceprop with appropriate typing and default parameter handling.
18-20: LGTM - Appropriate hooks for the use case.Using
useSearchParamsfor query parameter access,useStatefor toggle control, anduseIdfor ARIA relationship is the correct approach.
26-31: LGTM - Robust stack trace resolution.The implementation correctly:
- Uses
useMemoto prevent unnecessary query param re-reads- Handles null/undefined
searchParamswith optional chaining- Prioritizes prop over query parameter with nullish coalescing
- Derives boolean flag for conditional rendering
68-82: LGTM - Accessible toggle button.The toggle button properly implements:
- ARIA expanded state tracking
- ARIA controls relationship via unique ID
- Dynamic label based on expansion state
- Visual indicator with chevron icons
- Proper button semantics with
type="button"
83-89: LGTM - Well-styled stack trace display.The stack trace container appropriately uses:
- Monospace font for code readability
- Scrollable container with max-height
- Pre-wrap for preserving formatting
- Proper ARIA relationship via
idattribute- Conditional rendering based on expansion state
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
components/not-found/NotFound.tsx (1)
8-15: Consider simplifying the title label computation.The current multi-step assignment can be condensed into a single expression for better readability.
Apply this diff to simplify:
- let titleLabel; - if (label) { - titleLabel = label.toUpperCase(); - } else { - titleLabel = "PAGE"; - } - - titleLabel = `404 | ${titleLabel} NOT FOUND`; + const titleLabel = `404 | ${label ? label.toUpperCase() : "PAGE"} NOT FOUND`;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/global-error.tsx(1 hunks)components/error/Error.tsx(1 hunks)components/not-found/NotFound.tsx(1 hunks)components/providers/LayoutErrorFallback.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- components/providers/LayoutErrorFallback.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
app/global-error.tsx (2)
utils/error-extractor.ts (1)
extractErrorDetails(3-25)components/error/Error.tsx (1)
ErrorComponent(15-98)
components/error/Error.tsx (1)
contexts/TitleContext.tsx (1)
useTitle(186-192)
components/not-found/NotFound.tsx (1)
contexts/TitleContext.tsx (1)
useTitle(186-192)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
components/not-found/NotFound.tsx (1)
24-50: UI refactoring looks good, pending removal of the test throw.The new centered layout with image, title, and descriptive text provides a clean and user-friendly 404 experience. The removal of navigation buttons in favor of a simpler presentation is appropriate. However, full approval depends on removing the test throw in the useEffect (line 21).
components/error/Error.tsx (1)
70-84: LGTM! Good accessibility implementation.The collapsible stacktrace toggle button correctly implements accessibility features with
aria-expandedandaria-controlsattributes, along with clear visual indicators (chevron icons) for the expanded/collapsed state. The use ofuseId()for generating a uniquestacktraceContentIdensures proper ARIA relationships even with multiple instances.app/global-error.tsx (1)
16-16: LGTM! Accessibility improvement addressed.The
lang="en"attribute has been correctly added to the<html>element, addressing the previous review comment and ensuring proper accessibility and consistency with the root layout.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
components/not-found/NotFound.tsx (1)
19-22: CRITICAL: Remove the test error throw before merging.Line 20 contains a deliberate
throw new Error("test")that will crash the NotFound component on every render. This remains unresolved from previous reviews and is a production-breaking blocker that must be removed before this PR can be merged.Apply this diff to remove the test throw:
useEffect(() => { - throw new Error("test"); setTitle(titleLabel); }, [titleLabel, setTitle]);
🧹 Nitpick comments (3)
components/not-found/NotFound.tsx (1)
8-15: Optional: Simplify title label construction.The multi-step title label construction can be simplified into a single expression for improved readability.
Apply this diff to simplify the logic:
- let titleLabel; - if (label) { - titleLabel = label.toUpperCase(); - } else { - titleLabel = "PAGE"; - } - - titleLabel = `404 | ${titleLabel} NOT FOUND`; + const titleLabel = `404 | ${label?.toUpperCase() ?? "PAGE"} NOT FOUND`;components/error/Error.tsx (1)
48-155: Copy-to-clipboard behavior and collapsible stacktrace UI are well-structuredThe
isCopiedtimeout/reset effect, disabled state, and copy payload (including digest when present) all line up with the tests, and the accessible toggle (aria-expanded/aria-controlswithuseId) plusAnimatePresence/motion.divusage are sound. The only minor UX nit is that in digest-only cases the toggle label still says “Show Stacktrace”; if product/design ever wants finer wording, you could consider a more generic label like “Show error details,” but this isn’t a blocker.__tests__/components/error/Error.test.tsx (1)
65-187: Consider extracting small helpers to reduce repeated test boilerplateSeveral tests repeat the same “render + toggle Show Stacktrace before asserting” pattern; extracting helpers like
renderWithStackTraceandexpandStacktrace()could tighten the suite and make intent clearer without changing behavior. This is purely a readability/maintainability tweak.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
__tests__/components/error/Error.test.tsx(4 hunks)app/[user]/not-found.tsx(1 hunks)app/global-error.tsx(1 hunks)components/error/Error.tsx(1 hunks)components/not-found/NotFound.tsx(1 hunks)components/providers/LayoutErrorFallback.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- app/[user]/not-found.tsx
- components/providers/LayoutErrorFallback.tsx
- app/global-error.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
components/error/Error.tsx (1)
contexts/TitleContext.tsx (1)
useTitle(186-192)
__tests__/components/error/Error.test.tsx (2)
__mocks__/react-use.js (1)
React(2-2)components/error/Error.tsx (1)
ErrorComponent(22-159)
components/not-found/NotFound.tsx (1)
contexts/TitleContext.tsx (1)
useTitle(186-192)
🪛 Biome (2.1.2)
components/not-found/NotFound.tsx
[error] 21-21: This code will never be reached ...
... because this statement will throw an exception beforehand
(lint/correctness/noUnreachable)
🔇 Additional comments (4)
components/not-found/NotFound.tsx (1)
24-50: LGTM: UI structure and content are well-implemented.The centered layout, image optimization settings, and user-friendly messaging are appropriate for a 404 page. The conditional text handling based on the label prop is correct.
components/error/Error.tsx (1)
16-47: Stack trace resolution and digest handling look correct and robustThe
ErrorComponentPropsshape and theresolvedStackTrace/resolvedStackTraceWithDigestlogic correctly prioritize the explicitstackTraceprop over the URL param, while still supporting URL-only and digest-only scenarios, and the(hasStackTrace || digest)guard matches the new test expectations. No functional issues spotted here.__tests__/components/error/Error.test.tsx (2)
1-54: Test scaffolding and coverage align well with the new ErrorComponent behaviorThe mocks for
TitleContext,useSearchParams,next/image,react-use, andframer-motionare clean and isolated, and the suite exercises all key behaviors: prop vs URL stack resolution, digest-only rendering, toggle visibility, copy payload, “Copied” state reset via fake timers, andonResethandling. This should give good confidence around the new error UI and interactions.Also applies to: 65-187
189-206: Try Again button tests correctly gate ononResetThe positive/negative cases around the Try Again button accurately reflect the
onResetprop contract and ensure no stray button is rendered when the callback isn’t provided. No changes needed here.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
app/error.tsx (1)
7-13: Avoid shadowing the globalErrorsymbol (Biome warning)The component name
Errorshadows the globalErrorconstructor and triggers Biome’snoShadowRestrictedNamesrule. Renaming the local type and component improves clarity and resolves the lint issue without affecting Next.js behavior (default export name is not special).You can apply a rename like this:
-type ErrorProps = { +type RouteErrorProps = { readonly error: Error & { digest?: string }; readonly reset: () => void; }; -export default function Error({ error, reset }: ErrorProps) { +export default function RouteError({ error, reset }: RouteErrorProps) { const errorDetails = extractErrorDetails(error, "ROUTE_ERROR");components/the-memes/TheMemes.module.scss (1)
85-91: Minor nit:z-indexon a static element is ineffectiveWith
.fullScreen { position: static; ... z-index: 100; }, thez-indexno longer has any effect. Not harmful, but you can drop it (or make the iconposition: relativeif you still rely on stacking) for clarity.components/memelab/MemeLab.module.scss (1)
88-94: Redundantz-indexon static.fullScreeniconSame as in TheMemes styles: with
position: static, thez-indexhas no effect. It’s harmless but could be removed (or.fullScreenmadeposition: relative) for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/[user]/not-found.tsx(1 hunks)app/error.tsx(1 hunks)components/memelab/MemeLab.module.scss(2 hunks)components/memelab/MemeLabPage.tsx(3 hunks)components/not-found/NotFound.tsx(1 hunks)components/the-memes/MemePageArt.tsx(3 hunks)components/the-memes/TheMemes.module.scss(2 hunks)next.config.mjs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/[user]/not-found.tsx
- components/not-found/NotFound.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
app/error.tsx (2)
utils/error-extractor.ts (1)
extractErrorDetails(3-25)components/error/Error.tsx (1)
ErrorComponent(22-159)
components/memelab/MemeLabPage.tsx (2)
helpers/Helpers.ts (1)
enterArtFullScreen(172-195)components/nft-image/NFTImage.tsx (1)
NFTImage(21-53)
components/the-memes/MemePageArt.tsx (2)
helpers/Helpers.ts (1)
enterArtFullScreen(172-195)components/nft-image/NFTImage.tsx (1)
NFTImage(21-53)
🪛 Biome (2.1.2)
app/error.tsx
[error] 12-12: 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)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (7)
app/error.tsx (1)
1-24: Route error handler wiring looks solidThis correctly follows the Next.js error boundary contract, uses
extractErrorDetailswith a clear"ROUTE_ERROR"context, and passesstackTrace,digest, andonResetcleanly into the sharedErrorComponentwhile reusing the existingHome.module.scsslayout.components/the-memes/TheMemes.module.scss (2)
64-83: Art header layout looks consistent and flexibleThe new
.artHeader*structure (relative container + flex row + centered label) should work well with the fullscreen icon and different viewport widths; no issues from a layout/stacking perspective.
146-174: Carousel control overrides are scoped and look safeAnchoring controls with
.memesCarousel { position: relative; }and absolute :global() rules for prev/next buttons/icons should give predictable centering and hit areas without leaking to other carousels, given the.memesCarouselprefix. No functional concerns.components/the-memes/MemePageArt.tsx (1)
72-88: Header-based format label and fullscreen integration look goodThe shared header (
artHeader+artHeaderContent+artFormatLabel+ fullscreen icon) for both animated and static paths is clean and consistent, and pairing it withshowBalance={false}in this context avoids wallet noise in the pure “art view”. Structurally this matches the updated styles and looks sound.Also applies to: 119-141
components/memelab/MemeLabPage.tsx (1)
752-799: Art header + carousel restructuring is coherent and matches TheMemesThe new header above the carousel (format label + fullscreen toggle) and the corresponding non-animation branch, combined with
showBalance={false}, give a clean “art-first” presentation and mirror the TheMemes implementation. Structure and props wiring throughNFTImagelook correct.Also applies to: 801-825
components/memelab/MemeLab.module.scss (2)
67-86: Shared art header styles are consistent with TheMemesThe
.artHeader*additions mirror TheMemes styles, giving a unified header/flex layout across meme/memelab views; no issues spotted.
149-177: Carousel control positioning overrides look correct and scopedUsing
.memesCarousel { position: relative; }plus the prefixed :global carousel-control rules should centralize prev/next buttons without affecting other carousels. The explicit icon sizing is also clear. No functional concerns here.
|
2 participants
Summary by CodeRabbit
New Features
UI Changes
Tests