refactor:statically generate icon detail pages and remove client-side…#49
refactor:statically generate icon detail pages and remove client-side…#49Abhijit-Jha merged 4 commits intomasterfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughIcon content fetching moved from the IconDetailContent component to the page layer; pages now generate static params and fetch icon code, passing a required Changes
Sequence DiagramsequenceDiagram
participant Page as Page ([slug])
participant Service as getIconsContent()
participant Component as IconDetailContent
rect rgb(240,248,255)
note over Page,Service: Static params + server fetch
Page->>Service: getIconsContent(slug)
Service-->>Page: code (string) or null
alt code present
Page->>Component: render IconDetailContent(slug, code)
Component->>Component: initialize state from code prop
else not found
Page->>Page: notFound()
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @app/icons/[slug]/page.tsx:
- Around line 40-42: Wrap the getIconsContent(slug) call in a try-catch (or
check for a null/undefined result) inside the page component that returns
<IconDetailContent>; on any error or missing content call notFound() from
next/navigation to render a 404 instead of letting the exception bubble.
Specifically, update the code around getIconsContent and the return of
IconDetailContent so that failed/absent icon content triggers notFound() and
only successful fetches render IconDetailContent.
🧹 Nitpick comments (3)
icons/accessibility-icon.tsx (1)
59-59: CSS class fix looks good.The className correction from
"cursor - pointer"to"cursor-pointer"properly fixes the CSS class name.Optional: Remove trailing space.
There's a trailing space after
${className}that could be removed for cleaner formatting.🔎 Suggested cleanup
- className={`cursor-pointer ${className} `} + className={`cursor-pointer ${className}`}app/icons/[slug]/icon-detail-content.tsx (2)
29-29: Remove unusedsetIconCodeand simplify state management.Since
codeis now passed via props and never updated client-side, theiconCodestate and its setter are unnecessary. Use thecodeprop directly.🔎 Proposed fix
- const [iconCode, setIconCode] = React.useState<string>(code);Then replace all occurrences of
iconCodewithcodethroughout the component (lines 39, 225, 245, 248).
245-257: Dead code: "Loading code..." fallback is unreachable.Since
codeis now provided server-side via props and will always have a value, this loading state branch will never execute. Consider removing it for cleaner code.🔎 Proposed simplification
<div className="max-h-[500px] overflow-auto"> - {iconCode ? ( - <pre className="p-4 text-sm"> - <code className="text-foreground/90"> - {iconCode} - </code> - </pre> - ) : ( - <div className="flex h-40 items-center justify-center"> - <span className="text-muted-foreground text-sm"> - Loading code... - </span> - </div> - )} + <pre className="p-4 text-sm"> + <code className="text-foreground/90">{code}</code> + </pre> </div>
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/icons/[slug]/icon-detail-content.tsxapp/icons/[slug]/page.tsxicons/accessibility-icon.tsxicons/arrow-big-down-dash-icon.tsxicons/arrow-big-down-icon.tsxpublic/r/accessibility-icon.jsonpublic/r/arrow-big-down-dash-icon.jsonpublic/r/arrow-big-down-icon.jsonpublic/r/coin-bitcoin-icon.json
🧰 Additional context used
🧬 Code graph analysis (1)
app/icons/[slug]/page.tsx (2)
actions/get-icons-content.ts (1)
getIconsContent(6-10)app/icons/[slug]/icon-detail-content.tsx (1)
IconDetailContent(21-316)
🪛 GitHub Check: build
app/icons/[slug]/icon-detail-content.tsx
[warning] 29-29:
'setIconCode' is assigned a value but never used
🔇 Additional comments (8)
public/r/coin-bitcoin-icon.json (1)
10-10: LGTM! Formatting improvement.The newline formatting update (CRLF to LF) improves consistency without affecting functionality.
public/r/accessibility-icon.json (1)
10-10: LGTM! Consistent with source component.The content field correctly reflects the className fix from
icons/accessibility-icon.tsx, updating from"cursor - pointer"to"cursor-pointer".icons/arrow-big-down-icon.tsx (1)
10-10: Animation simplification looks good.The change from
{ y: [0, 4, 0] }to{ y: [0, 4] }simplifies the animation by removing the automatic return to the origin. The icon now stays at positiony: 4untilstop()is called, which is appropriate since the hover handlers correctly triggerstop()on hover end.icons/arrow-big-down-dash-icon.tsx (1)
10-11: Animation simplifications look good.Both animations have been simplified from keyframe sequences to fixed target values:
- Arrow
yposition:[0, 4, 0]→4- Dash opacity:
[1, 0.5, 1]→0.5This matches the pattern in
arrow-big-down-icon.tsxand properly relies onstop()to return elements to their original state. The hover handlers ensure correct animation lifecycle.public/r/arrow-big-down-icon.json (1)
10-10: LGTM!The animation simplification from a three-point keyframe sequence (
y: [0, 4, 0]) to a single target (y: [0, 4]) is appropriate. Thestop()function still handles returning toy: 0, so the hover-in/hover-out behavior remains intact with cleaner separation of concerns.public/r/arrow-big-down-dash-icon.json (1)
10-10: LGTM!Consistent animation simplification with
arrow-big-down-icon.json. Both the arrow movement and dash opacity now animate to single target values, withstop()handling the restoration. The parallel animation calls for.arrowand.dashare appropriate for synchronized effects.app/icons/[slug]/page.tsx (1)
11-28: LGTM on the static params generation approach.The
generateStaticParamsfunction correctly extracts icon slugs at build time. Using synchronousfs.readFileSynchere is acceptable since this only runs during the build phase.app/icons/[slug]/icon-detail-content.tsx (1)
21-27: Good refactoring to accept code via props.The simplification from client-side fetching to server-provided props aligns well with the SSG approach and Next.js App Router best practices for data flow.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @app/icons/[slug]/page.tsx:
- Around line 48-49: The catch block declares an unused error parameter; remove
it by changing the catch clause from `catch (error)` to a parameterless `catch`
so the `notFound()` call remains but the unused `error` variable is eliminated
(update the catch in the code path that calls notFound() in the page component
handling icons/[slug]).
- Around line 12-29: The generateStaticParams function lacks error handling and
incorrectly filters icon names by dash; wrap the file read and regex parsing in
a try/catch around fs.readFileSync(indexPath, "utf-8") and namePattern.exec
usage to throw or log a clear, descriptive error if icons/index.ts is missing or
malformed, and remove the name.includes("-") check so every extracted name (from
namePattern) is pushed into the names array; keep the dedupe and return mapping
as-is.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/icons/[slug]/page.tsx
🧰 Additional context used
🪛 GitHub Check: build
app/icons/[slug]/page.tsx
[warning] 48-48:
'error' is defined but never used
🔇 Additional comments (2)
app/icons/[slug]/page.tsx (2)
1-6: LGTM!Imports are appropriate for a Next.js App Router server component. Using
fsandpathingenerateStaticParamsis valid since it runs at build time.
39-53: Clean implementation of server-side fetching with proper error handling.The try-catch pattern with
notFound()correctly handles both fetch failures and missing icon content. This aligns well with Next.js App Router best practices for static generation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/icons/[slug]/page.tsx (1)
12-34: Good implementation with proper error handling.The error handling and deduplication logic are solid. The regex-based extraction is a pragmatic approach to avoid importing client components into the server build context.
One consideration: the regex
/name:\s*["']([^"']+)["']/gassumes a specific format inicons/index.ts. If the format changes (e.g., multi-line properties or computed names), this could silently miss entries. Consider adding a build-time assertion that compares the extracted count against an expected minimum.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/icons/[slug]/page.tsx
🔇 Additional comments (4)
app/icons/[slug]/page.tsx (4)
1-6: LGTM!The imports are appropriate for static generation with filesystem access at build time, and all are used in the code.
8-10: LGTM!The
Promise<{ slug: string }>type correctly reflects Next.js 15's async params API.
36-42: LGTM!Metadata generation correctly uses the async params API and produces appropriate SEO-friendly titles and descriptions.
44-58: Solid error handling with clean 404 fallback.The try-catch with
notFound()correctly handles both missing content and fetch failures. The parameterlesscatchon line 53 properly addresses the previous static analysis warning about unused variables.Minor note: The
let code = ""initialization on line 46 is technically redundant sincenotFound()throws and the empty string path is never reached. You could useconst code = await getIconsContent(slug)directly inside the try block, but the current approach is clear and works correctly.
This change reduces unnecessary edge and serverless requests by converting
/icon/[slug] pages from request-time rendering to build-time static generation.
What changed:
client components into the server context
Benefits:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.