Add skeleton screens and route-level loading states across all pages (including Ideas section)#594
Conversation
📝 WalkthroughWalkthroughIntroduces a comprehensive skeleton loading system with new Skeleton components and updates pages to use client-side data fetching with loading states. Adds SuspenseWrapper for suspense boundaries, implements route-change loading indicators in the app wrapper, and migrates ideas pages from static generation to dynamic client-side data fetching. Includes a minor SVG icon update. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Router as Next.js Router
participant Page as Ideas Page
participant API as Data API
participant UI as UI (Skeletons/Content)
User->>Router: Navigate to /ideas
Router->>Page: Render component
Page->>Page: Initialize state (articles=[], isLoading=true)
Page->>UI: Render SkeletonGrid
UI-->>User: Show skeleton placeholders
Page->>API: useEffect: fetch ideas data
API-->>Page: Return articles data
Page->>Page: Update state (articles, isLoading=false)
Page->>UI: Re-render with fetched data
UI-->>User: Display article cards
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
|
I’ve implemented consistent skeleton loading states across all pages, including the Ideas section (converted to client-side fetching to enable loading placeholders). The project builds successfully and has been tested locally (including slow network simulation). Please let me know if you'd prefer retaining SSG for the Ideas section — I’m happy to adjust the implementation if needed. Looking forward to your feedback. Thanks! |
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/pages/ideas/[year]/[slug].jsx (2)
64-72:⚠️ Potential issue | 🟡 Minor
router.isFallbackcheck is dead code after removinggetStaticPaths.
router.isFallbackis onlytrueduring SSG fallback rendering, which requiresgetStaticPathswithfallback: trueor'blocking'. SincegetStaticPathswas removed in this PR, this branch can never be reached and should be deleted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/ideas/`[year]/[slug].jsx around lines 64 - 72, Remove the now-dead SSG fallback branch: delete the router.isFallback conditional and its JSX return (the Container block that renders "Loading...") since getStaticPaths was removed and this branch can never be reached; update any imports or surrounding code in the [year]/[slug].jsx component if they become unused after removing the router.isFallback block.
195-206:⚠️ Potential issue | 🟠 MajorThe
inlineprop on thecodecomponent was removed in react-markdown v9.0.0 (not v8.0.0), and it does not exist in v10.1.0. As a result,inlineis alwaysundefinedand every code snippet—including single-backtick inline code—renders using block-level styles instead of inline styles.The modern approach is to override
prefor code blocks andcodefor inline code:Recommended fix
-code: ({ node, inline, ...props }) => - inline ? ( - <code - className="bg-zinc-100 dark:bg-zinc-800 text-green-600 dark:text-yellow-400 px-2 py-1 rounded text-sm font-mono" - {...props} - /> - ) : ( - <code - className="block bg-zinc-100 dark:bg-zinc-800 text-zinc-800 dark:text-zinc-200 p-4 rounded-lg overflow-x-auto text-sm font-mono my-4" - {...props} - /> - ), +pre: ({ node, ...props }) => ( + <pre className="bg-zinc-100 dark:bg-zinc-800 p-4 rounded-lg overflow-x-auto my-4" {...props} /> +), +code: ({ node, ...props }) => ( + <code + className="bg-zinc-100 dark:bg-zinc-800 text-green-600 dark:text-yellow-400 px-2 py-1 rounded text-sm font-mono" + {...props} + /> +),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/ideas/`[year]/[slug].jsx around lines 195 - 206, The current custom renderer uses the removed inline prop on the code component so every snippet renders as a block; replace that logic by providing both a pre renderer for block-level code and a code renderer for inline code inside the ReactMarkdown components prop: implement components.pre to render the block wrapper (using the block classes you already have: "block bg-zinc-100 ... font-mono my-4" and forward children/props), and implement components.code to render inline code (using the inline classes "bg-zinc-100 ... text-green-600 ... font-mono" and forward className/children/props); remove reliance on the inline argument and ensure className and children are forwarded correctly in both pre and code renderers so fenced code blocks get the pre styling and single-backtick inline code gets the inline styling.src/pages/_app.jsx (1)
37-62:⚠️ Potential issue | 🟠 MajorDouble-loading pattern creates jarring UX — current page unmounts immediately on navigation.
When a route change begins,
loadingflips totrueand the current<Component>unmounts right away, showing only<PageLoader>. Once the route transition completes, the incoming page mounts with its ownisLoading = truestate and then runs the 500ms timer. The user therefore sees:current page disappears → full-page spinner (route transition) → per-page skeleton (500ms) → real content
That's three sequential states every time a link is clicked. A more user-friendly approach is to keep the current page visible and use a non-blocking top-bar progress indicator (e.g.
nprogress) while the route is loading, and remove the artificial per-page timers for pages that have no real async data dependency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/_app.jsx` around lines 37 - 62, The current useEffect toggles loading and unmounts the current page by switching from rendering <Component> to <PageLoader>; instead, keep <Component previousPathname={previousPathname} {...pageProps} /> mounted and render the route loader as a non-blocking indicator (e.g., integrate nprogress or a top-bar progress component) so routeChangeStart triggers nprogress.start() and routeChangeComplete/routeChangeError call nprogress.done(); update the JSX to always render <Component /> and conditionally render PageLoader only as an overlay/progress indicator (not full-page replacement), and remove any artificial per-page 500ms timers in page-level components that rely on isLoading so pages with no async data don’t double-load.
🧹 Nitpick comments (10)
src/components/Skeletons/Skeleton.jsx (1)
4-12: MovevariantStylesmap outside the component.It's a static constant recreated on every render. Hoisting it to module scope avoids unnecessary object allocations.
♻️ Proposed refactor
+const variantStyles = { + rectangular: 'rounded-md', + circular: 'rounded-full', + text: 'rounded', +} + const Skeleton = forwardRef(function Skeleton( { className, variant = 'rectangular', width, height, ...props }, ref ) { - const variantStyles = { - rectangular: 'rounded-md', - circular: 'rounded-full', - text: 'rounded', - } - return (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Skeletons/Skeleton.jsx` around lines 4 - 12, The variantStyles object is recreated on every render inside the forwardRef Skeleton component; hoist it to module scope as a top-level constant (e.g., VARIANT_STYLES) and have the Skeleton function reference that constant instead of declaring variantStyles inline so you avoid repeated allocations; update any usages in the Skeleton component to read from the new top-level VARIANT_STYLES.src/pages/apply.jsx (1)
11-31:itemsarray values are defined but never read inTimelineSkeleton.The
mapcallback uses(_, index)— theitemsentries (with theirtitle/descriptionfields) are ignored. Replace the array with a plainArray.from({ length: 5 }).♻️ Proposed refactor
function TimelineSkeleton() { - const items = [ - { title: 'Join us on Discord', description: '' }, - { title: 'Start Contributing', description: '' }, - { title: 'Write a Draft Application', description: '' }, - { title: 'Discuss with Mentors', description: '' }, - { title: 'Submit Final Application', description: '' }, - ]; - return ( <ol className="relative border-l-2 border-gray-200 dark:border-gray-700"> - {items.map((_, index) => ( + {Array.from({ length: 5 }).map((_, index) => ( <li key={index} className="mb-10 ml-6">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/apply.jsx` around lines 11 - 31, TimelineSkeleton defines a constant items = [...] but the map callback ignores each entry (uses (_, index)), so the array contents are unused; fix by either using the defined items inside the map (e.g., map((item, index) => ...) and render item.title/item.description) or replace the items array with a simple Array.from({ length: 5 }) and keep map((_, index) => ...); update the map callback in TimelineSkeleton accordingly so the code no longer defines unused values.src/components/SuspenseWrapper.jsx (1)
14-18: Prefer nullish coalescing over||for the fallback check.
fallback || defaultFallbackwill discard valid falsy fallbacks (e.g.,0,false,""). Use??to only substitute when the caller passesnullorundefined.♻️ Proposed fix
- <Suspense fallback={fallback || defaultFallback}> + <Suspense fallback={fallback ?? defaultFallback}>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/SuspenseWrapper.jsx` around lines 14 - 18, The Suspense fallback expression currently uses fallback || defaultFallback which will incorrectly override valid falsy values; update the JSX in the SuspenseWrapper component to use the nullish coalescing operator (fallback ?? defaultFallback) so only null or undefined fallbacks are replaced—adjust the expression passed to the Suspense fallback prop (referencing fallback, defaultFallback, and Suspense).src/pages/_app.jsx (1)
1-10: Remove unusedSuspenseandSkeletonimports.Neither
Suspense(Line 3) norSkeleton(Line 10) are referenced anywhere in this file.♻️ Proposed fix
-import { useEffect, useRef, useState } from 'react' +import { useEffect, useRef, useState } from 'react' import { useRouter } from 'next/router' -import { Suspense } from 'react' ... -import { Skeleton } from '@/components/Skeletons'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/_app.jsx` around lines 1 - 10, Remove the unused imports to clean up the module: delete the unused React symbol Suspense from the named imports and remove the unused Skeleton import from '@/components/Skeletons' so only used symbols (useEffect, useRef, useState, useRouter, Header, Footer) remain; update the import statements at the top of the module to drop Suspense and the Skeleton import line.src/pages/about.jsx (2)
174-188: Loading branch duplicatesstyles.containeras an inline object.The outer
<div>at Line 176 re-declares the same style values already captured in thestyles.containerobject (Lines 102-109). Usestyles.containerfor consistency.♻️ Proposed fix
- <div style={{ fontFamily: 'Arial, sans-serif', padding: '20px', textAlign: 'center', maxWidth: '1000px', margin: '0 auto' }}> + <div style={styles.container}>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/about.jsx` around lines 174 - 188, The loading branch duplicates inline container styles; replace the inline style object on the outer <div> in the isLoading return with the existing styles.container constant to avoid duplication and ensure consistency (update the JSX that renders AboutSkeleton, Banner and Container.Outer to use styles.container instead of the inline style).
50-55: Artificial 500ms delay on a fully-static page degrades UX.The
Aboutpage has no async data:isDarkModeis a synchronous DOM read and all content is hardcoded. The timer adds 500ms of unnecessary skeleton display on every visit without any benefit.The skeleton pattern makes sense only for pages with genuine async dependencies (e.g.
ideas/index.jsxfetching from GitHub). For pages that are entirely static, drop the timer and theisLoadingstate entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/about.jsx` around lines 50 - 55, Remove the artificial 500ms loading delay and the now-unnecessary isLoading state: delete the useEffect timer block that calls setIsLoading, remove the isLoading state variable and any conditional rendering that shows the skeleton when isLoading is true (in src/pages/about.jsx), and render the About page content directly (still respecting isDarkMode where used). Ensure any references to setIsLoading or isLoading are cleaned up so there are no unused state variables or dead branches left.src/pages/index.jsx (1)
92-98:setIsLoading(true)at the top of the effect is redundant.
isLoadingis already initialized totrue, and the effect runs only once on mount, so this call is a no-op.♻️ Proposed fix
useEffect(() => { - setIsLoading(true) const timer = setTimeout(() => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/index.jsx` around lines 92 - 98, The initial setIsLoading(true) inside the useEffect is redundant because isLoading is initialized to true and the effect runs only on mount; remove the top-level setIsLoading(true) call in the useEffect that contains setRandomProjects and setIsLoading(false), leaving the timeout logic (setRandomProjects(projects.sort(() => 0.5 - Math.random()).slice(0, 3)) and setIsLoading(false)) intact and keep the cleanup return () => clearTimeout(timer) so the effect still runs once and clears the timer properly.src/pages/ideas/[year]/[slug].jsx (1)
11-11:getCurrentYearis imported but never used in this file.♻️ Proposed fix
-import { fetchIdeaContent, getCurrentYear } from '@/helper/fetchGitHubIdeas'; +import { fetchIdeaContent } from '@/helper/fetchGitHubIdeas';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/ideas/`[year]/[slug].jsx at line 11, Remove the unused getCurrentYear import from the import statement (leave fetchIdeaContent) or, if you intended to use it, actually call getCurrentYear where the current year is needed (e.g., for defaulting or validating the [year] route param); update the import to only import fetchIdeaContent if you choose removal and ensure there are no other references to getCurrentYear in this component.src/pages/projects.jsx (2)
15-16: Consolidate two imports from the same module into one statement.♻️ Proposed fix
-import { Skeleton } from '@/components/Skeletons'; -import { SkeletonText } from '@/components/Skeletons'; +import { Skeleton, SkeletonText } from '@/components/Skeletons';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/projects.jsx` around lines 15 - 16, Consolidate the two separate imports into a single import statement by combining Skeleton and SkeletonText: replace the two lines importing Skeleton and SkeletonText from '@/components/Skeletons' with one import that imports both symbols (Skeleton, SkeletonText) from the same module to remove the duplicate import.
185-192: Artificial 500ms delay for synchronous data.Same pattern as
about.jsx: all project content comes from a synchronous local import, so the 500ms timer only delays the page render with no real async work behind it. Consider removing the timer entirely and rendering content immediately, keeping the skeleton pattern only for pages with genuine async dependencies.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/projects.jsx` around lines 185 - 192, The 500ms artificial delay in Projects page is unnecessary because data is synchronous; remove the useEffect timer and the isLoading state (useState/setIsLoading) or set isLoading to false immediately so the component renders content without delay; update render logic that checks isLoading to show content directly (or keep the skeleton only for true async pages) and delete the timer cleanup code referencing timer/clearTimeout.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/Skeletons/SkeletonGrid.jsx`:
- Around line 6-10: The wrapper <span> around each <SkeletonCard /> produces
invalid HTML because SkeletonCard renders a <div>; remove the <span> and apply
the key directly to the <SkeletonCard /> inside the Array.from map in
SkeletonGrid.jsx (i.e., change the mapping from <span key={index}><SkeletonCard
/></span> to <SkeletonCard key={index} /> or wrap in a fragment if needed).
In `@src/components/SuspenseWrapper.jsx`:
- Around line 21-31: The outer wrapper in PageLoadingSkeleton currently uses the
animate-pulse class which duplicates the animation already applied inside each
Skeleton, causing compounded opacity; remove animate-pulse from the outer
wrapper div (leave other classes like spacing/background intact) so only the
Skeleton components control the pulse animation and restore the intended 50%
keyframe opacity.
In `@src/pages/apply.jsx`:
- Around line 35-42: Remove the artificial 500ms delay by deleting the isLoading
state, its setter setIsLoading, and the useEffect that creates the timer
(setTimeout/clearTimeout); then remove the conditional render that checks
isLoading and render the page content directly (replace the isLoading ? skeleton
branch with the actual content) so the apply page displays immediately without
synthetic latency.
In `@src/pages/ideas/2024/index.jsx`:
- Around line 127-130: Add a new error state (e.g., const [error, setError] =
useState(null)) and update the data-fetching catch block in the getAllIdeas flow
to call setError(error) (or setError(error.message)) before the finally that
calls setIsLoading(false); ensure setArticles is only set on success. Then
update the JSX where the articles grid renders to conditionally show a
user-facing error message when error is truthy (and hide the empty grid), while
preserving the existing loading indicator controlled by setIsLoading.
- Around line 120-135: Calling getAllIdeas() inside the client-side useEffect is
invalid because getAllIdeas (and fast-glob/path usage) relies on Node.js
filesystem APIs and will break in the browser; revert the data load back to
getStaticProps to keep the MDX ideas fetched at build time, or move the
server-only logic into a new API route (e.g., /api/ideas) that calls getAllIdeas
server-side and have the useEffect fetch that endpoint; also add an error state
alongside setIsLoading and setArticles (update state setters referenced in the
component such as setArticles and setIsLoading) so failures are surfaced to the
user instead of silently logging with console.error.
In `@src/pages/ideas/index.jsx`:
- Around line 119-136: Replace the unused state for currentYear with a stable
value (use const currentYear = getCurrentYear() or useMemo) so you can remove
currentYear from the useEffect dependency list; add an error state (e.g., const
[error, setError] = useState(null)) and in fetchData's catch call
setError(error.message || String(error)) and optionally setArticles([]) as
appropriate; keep setIsLoading(false) in finally; update the component render to
show a visible error branch (e.g., when error is truthy render a centered red
message) so users see failures from fetchIdeasFromGitHub instead of an empty
Grid.
In `@src/pages/index.jsx`:
- Around line 94-96: The current timer callback uses projects.sort(...), which
mutates the imported module array; change it to sort a shallow copy instead
(e.g., use [...projects] or projects.slice() before .sort) inside the timer
callback that calls setRandomProjects and setIsLoading so the module-level
projects array is not mutated across consumers or HMR reloads.
---
Outside diff comments:
In `@src/pages/_app.jsx`:
- Around line 37-62: The current useEffect toggles loading and unmounts the
current page by switching from rendering <Component> to <PageLoader>; instead,
keep <Component previousPathname={previousPathname} {...pageProps} /> mounted
and render the route loader as a non-blocking indicator (e.g., integrate
nprogress or a top-bar progress component) so routeChangeStart triggers
nprogress.start() and routeChangeComplete/routeChangeError call
nprogress.done(); update the JSX to always render <Component /> and
conditionally render PageLoader only as an overlay/progress indicator (not
full-page replacement), and remove any artificial per-page 500ms timers in
page-level components that rely on isLoading so pages with no async data don’t
double-load.
In `@src/pages/ideas/`[year]/[slug].jsx:
- Around line 64-72: Remove the now-dead SSG fallback branch: delete the
router.isFallback conditional and its JSX return (the Container block that
renders "Loading...") since getStaticPaths was removed and this branch can never
be reached; update any imports or surrounding code in the [year]/[slug].jsx
component if they become unused after removing the router.isFallback block.
- Around line 195-206: The current custom renderer uses the removed inline prop
on the code component so every snippet renders as a block; replace that logic by
providing both a pre renderer for block-level code and a code renderer for
inline code inside the ReactMarkdown components prop: implement components.pre
to render the block wrapper (using the block classes you already have: "block
bg-zinc-100 ... font-mono my-4" and forward children/props), and implement
components.code to render inline code (using the inline classes "bg-zinc-100 ...
text-green-600 ... font-mono" and forward className/children/props); remove
reliance on the inline argument and ensure className and children are forwarded
correctly in both pre and code renderers so fenced code blocks get the pre
styling and single-backtick inline code gets the inline styling.
---
Nitpick comments:
In `@src/components/Skeletons/Skeleton.jsx`:
- Around line 4-12: The variantStyles object is recreated on every render inside
the forwardRef Skeleton component; hoist it to module scope as a top-level
constant (e.g., VARIANT_STYLES) and have the Skeleton function reference that
constant instead of declaring variantStyles inline so you avoid repeated
allocations; update any usages in the Skeleton component to read from the new
top-level VARIANT_STYLES.
In `@src/components/SuspenseWrapper.jsx`:
- Around line 14-18: The Suspense fallback expression currently uses fallback ||
defaultFallback which will incorrectly override valid falsy values; update the
JSX in the SuspenseWrapper component to use the nullish coalescing operator
(fallback ?? defaultFallback) so only null or undefined fallbacks are
replaced—adjust the expression passed to the Suspense fallback prop (referencing
fallback, defaultFallback, and Suspense).
In `@src/pages/_app.jsx`:
- Around line 1-10: Remove the unused imports to clean up the module: delete the
unused React symbol Suspense from the named imports and remove the unused
Skeleton import from '@/components/Skeletons' so only used symbols (useEffect,
useRef, useState, useRouter, Header, Footer) remain; update the import
statements at the top of the module to drop Suspense and the Skeleton import
line.
In `@src/pages/about.jsx`:
- Around line 174-188: The loading branch duplicates inline container styles;
replace the inline style object on the outer <div> in the isLoading return with
the existing styles.container constant to avoid duplication and ensure
consistency (update the JSX that renders AboutSkeleton, Banner and
Container.Outer to use styles.container instead of the inline style).
- Around line 50-55: Remove the artificial 500ms loading delay and the
now-unnecessary isLoading state: delete the useEffect timer block that calls
setIsLoading, remove the isLoading state variable and any conditional rendering
that shows the skeleton when isLoading is true (in src/pages/about.jsx), and
render the About page content directly (still respecting isDarkMode where used).
Ensure any references to setIsLoading or isLoading are cleaned up so there are
no unused state variables or dead branches left.
In `@src/pages/apply.jsx`:
- Around line 11-31: TimelineSkeleton defines a constant items = [...] but the
map callback ignores each entry (uses (_, index)), so the array contents are
unused; fix by either using the defined items inside the map (e.g., map((item,
index) => ...) and render item.title/item.description) or replace the items
array with a simple Array.from({ length: 5 }) and keep map((_, index) => ...);
update the map callback in TimelineSkeleton accordingly so the code no longer
defines unused values.
In `@src/pages/ideas/`[year]/[slug].jsx:
- Line 11: Remove the unused getCurrentYear import from the import statement
(leave fetchIdeaContent) or, if you intended to use it, actually call
getCurrentYear where the current year is needed (e.g., for defaulting or
validating the [year] route param); update the import to only import
fetchIdeaContent if you choose removal and ensure there are no other references
to getCurrentYear in this component.
In `@src/pages/index.jsx`:
- Around line 92-98: The initial setIsLoading(true) inside the useEffect is
redundant because isLoading is initialized to true and the effect runs only on
mount; remove the top-level setIsLoading(true) call in the useEffect that
contains setRandomProjects and setIsLoading(false), leaving the timeout logic
(setRandomProjects(projects.sort(() => 0.5 - Math.random()).slice(0, 3)) and
setIsLoading(false)) intact and keep the cleanup return () =>
clearTimeout(timer) so the effect still runs once and clears the timer properly.
In `@src/pages/projects.jsx`:
- Around line 15-16: Consolidate the two separate imports into a single import
statement by combining Skeleton and SkeletonText: replace the two lines
importing Skeleton and SkeletonText from '@/components/Skeletons' with one
import that imports both symbols (Skeleton, SkeletonText) from the same module
to remove the duplicate import.
- Around line 185-192: The 500ms artificial delay in Projects page is
unnecessary because data is synchronous; remove the useEffect timer and the
isLoading state (useState/setIsLoading) or set isLoading to false immediately so
the component renders content without delay; update render logic that checks
isLoading to show content directly (or keep the skeleton only for true async
pages) and delete the timer cleanup code referencing timer/clearTimeout.
| {Array.from({ length: count }).map((_, index) => ( | ||
| <span key={index}> | ||
| <SkeletonCard /> | ||
| </span> | ||
| ))} |
There was a problem hiding this comment.
<span> wrapping a block <div> is invalid HTML.
SkeletonCard renders a <div>, so wrapping it in a <span> produces an invalid block-in-inline nesting. The wrapper is also unnecessary since key can be applied directly to <SkeletonCard />.
🐛 Proposed fix
- {Array.from({ length: count }).map((_, index) => (
- <span key={index}>
- <SkeletonCard />
- </span>
- ))}
+ {Array.from({ length: count }).map((_, index) => (
+ <SkeletonCard key={index} />
+ ))}📝 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.
| {Array.from({ length: count }).map((_, index) => ( | |
| <span key={index}> | |
| <SkeletonCard /> | |
| </span> | |
| ))} | |
| {Array.from({ length: count }).map((_, index) => ( | |
| <SkeletonCard key={index} /> | |
| ))} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Skeletons/SkeletonGrid.jsx` around lines 6 - 10, The wrapper
<span> around each <SkeletonCard /> produces invalid HTML because SkeletonCard
renders a <div>; remove the <span> and apply the key directly to the
<SkeletonCard /> inside the Array.from map in SkeletonGrid.jsx (i.e., change the
mapping from <span key={index}><SkeletonCard /></span> to <SkeletonCard
key={index} /> or wrap in a fragment if needed).
| export function PageLoadingSkeleton() { | ||
| return ( | ||
| <div className="animate-pulse"> | ||
| <div className="h-16 bg-gray-200 dark:bg-gray-700 mb-8" /> | ||
| <div className="space-y-4"> | ||
| <Skeleton height="3rem" width="60%" className="mx-auto" /> | ||
| <Skeleton height="1.5rem" width="80%" className="mx-auto" /> | ||
| <Skeleton height="1.5rem" width="70%" className="mx-auto" /> | ||
| </div> | ||
| </div> | ||
| ) |
There was a problem hiding this comment.
Redundant animate-pulse on outer wrapper causes compounded opacity animation.
Skeleton already applies animate-pulse internally. The wrapper div at Line 23 also applies it, so at the 50% keyframe the child Skeleton elements render at ~25% opacity (0.5 × 0.5) instead of the intended 50%. Remove animate-pulse from the outer container.
🐛 Proposed fix
export function PageLoadingSkeleton() {
return (
- <div className="animate-pulse">
- <div className="h-16 bg-gray-200 dark:bg-gray-700 mb-8" />
+ <div>
+ <div className="animate-pulse h-16 bg-gray-200 dark:bg-gray-700 mb-8" />
<div className="space-y-4">📝 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.
| export function PageLoadingSkeleton() { | |
| return ( | |
| <div className="animate-pulse"> | |
| <div className="h-16 bg-gray-200 dark:bg-gray-700 mb-8" /> | |
| <div className="space-y-4"> | |
| <Skeleton height="3rem" width="60%" className="mx-auto" /> | |
| <Skeleton height="1.5rem" width="80%" className="mx-auto" /> | |
| <Skeleton height="1.5rem" width="70%" className="mx-auto" /> | |
| </div> | |
| </div> | |
| ) | |
| export function PageLoadingSkeleton() { | |
| return ( | |
| <div> | |
| <div className="animate-pulse h-16 bg-gray-200 dark:bg-gray-700 mb-8" /> | |
| <div className="space-y-4"> | |
| <Skeleton height="3rem" width="60%" className="mx-auto" /> | |
| <Skeleton height="1.5rem" width="80%" className="mx-auto" /> | |
| <Skeleton height="1.5rem" width="70%" className="mx-auto" /> | |
| </div> | |
| </div> | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/SuspenseWrapper.jsx` around lines 21 - 31, The outer wrapper
in PageLoadingSkeleton currently uses the animate-pulse class which duplicates
the animation already applied inside each Skeleton, causing compounded opacity;
remove animate-pulse from the outer wrapper div (leave other classes like
spacing/background intact) so only the Skeleton components control the pulse
animation and restore the intended 50% keyframe opacity.
| useEffect(() => { | ||
| const fetchData = async () => { | ||
| setIsLoading(true); | ||
| try { | ||
| const ideas = await getAllIdeas(); | ||
| const mappedIdeas = ideas.map(({ component, ...meta }) => meta); | ||
| setArticles(mappedIdeas); | ||
| } catch (error) { | ||
| console.error('Error fetching ideas:', error); | ||
| } finally { | ||
| setIsLoading(false); | ||
| } | ||
| }; | ||
|
|
||
| fetchData(); | ||
| }, []); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and examine getAllIdeas function
fd -e js -e ts getAllIdeas --exec cat {}Repository: AOSSIE-Org/website
Length of output: 2237
🏁 Script executed:
#!/bin/bash
# Also check the full file to understand context
cat -n src/pages/ideas/2024/index.jsx | head -150Repository: AOSSIE-Org/website
Length of output: 6605
🏁 Script executed:
#!/bin/bash
# Search for getAllIdeas import and usage
rg -B2 -A2 "getAllIdeas" src/pages/ideas/2024/index.jsxRepository: AOSSIE-Org/website
Length of output: 518
getAllIdeas() is a Node.js-only function and will break at runtime when called client-side.
fast-glob traverses the filesystem using Node.js APIs, and getAllIdeas also calls path.join(process.cwd(), ...), both of which are Node.js-only. When moved from getStaticProps into useEffect, this function will execute in the browser, where process.cwd() and filesystem I/O are not available. The module cannot be tree-shaken out of the client bundle when imported directly into a component.
Additionally, the error is silently swallowed (line 128: console.error only) with no error state. On failure, setArticles is never called, leaving an empty list displayed to the user with no indication of what went wrong.
Options (in preference order):
- Revert to
getStaticProps— the ideas list is static MDX content; there is no need to fetch it at runtime. - Create an API route (
/api/ideas) that callsgetAllIdeasserver-side and fetch from it inuseEffect.
If keeping useEffect, also add an error state to communicate failures to the user.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/ideas/2024/index.jsx` around lines 120 - 135, Calling getAllIdeas()
inside the client-side useEffect is invalid because getAllIdeas (and
fast-glob/path usage) relies on Node.js filesystem APIs and will break in the
browser; revert the data load back to getStaticProps to keep the MDX ideas
fetched at build time, or move the server-only logic into a new API route (e.g.,
/api/ideas) that calls getAllIdeas server-side and have the useEffect fetch that
endpoint; also add an error state alongside setIsLoading and setArticles (update
state setters referenced in the component such as setArticles and setIsLoading)
so failures are surfaced to the user instead of silently logging with
console.error.
| } catch (error) { | ||
| console.error('Error fetching ideas:', error); | ||
| } finally { | ||
| setIsLoading(false); |
There was a problem hiding this comment.
Missing error state leaves users staring at an empty grid on fetch failure.
When getAllIdeas() throws, setArticles is never called (stays []), isLoading flips to false via finally, and the page silently renders an empty grid. Add an error state and display a user-facing message.
🐛 Proposed fix
export default function Ideas() {
const [articles, setArticles] = useState([]);
const [isLoading, setIsLoading] = useState(true);
+ const [error, setError] = useState(null);
useEffect(() => {
const fetchData = async () => {
setIsLoading(true);
+ setError(null);
try {
const ideas = await getAllIdeas();
const mappedIdeas = ideas.map(({ component, ...meta }) => meta);
setArticles(mappedIdeas);
} catch (error) {
console.error('Error fetching ideas:', error);
+ setError('Failed to load ideas. Please try again later.');
} finally {
setIsLoading(false);
}
};
fetchData();
}, []);Then render the error where the grid appears:
- {articles.map((article) => (
+ {error ? (
+ <p className="text-red-500 font-mono">{error}</p>
+ ) : articles.map((article) => (
<Article key={article.slug} article={article} />
- ))}
+ ))}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/ideas/2024/index.jsx` around lines 127 - 130, Add a new error state
(e.g., const [error, setError] = useState(null)) and update the data-fetching
catch block in the getAllIdeas flow to call setError(error) (or
setError(error.message)) before the finally that calls setIsLoading(false);
ensure setArticles is only set on success. Then update the JSX where the
articles grid renders to conditionally show a user-facing error message when
error is truthy (and hide the empty grid), while preserving the existing loading
indicator controlled by setIsLoading.
| const [currentYear, setCurrentYear] = useState(getCurrentYear()); | ||
| const [isLoading, setIsLoading] = useState(true); | ||
|
|
||
| useEffect(() => { | ||
| const fetchData = async () => { | ||
| setIsLoading(true); | ||
| try { | ||
| const ideas = await fetchIdeasFromGitHub(currentYear); | ||
| setArticles(ideas); | ||
| } catch (error) { | ||
| console.error('Error fetching ideas:', error); | ||
| } finally { | ||
| setIsLoading(false); | ||
| } | ||
| }; | ||
|
|
||
| fetchData(); | ||
| }, [currentYear]); |
There was a problem hiding this comment.
setCurrentYear is never called, and fetch errors produce an empty grid with no user feedback.
Two issues here:
-
Dead setter:
currentYearis initialized once fromgetCurrentYear()andsetCurrentYearis never invoked. The year never changes, souseStateis unnecessary — a plainconst currentYear = getCurrentYear()(oruseMemo) is sufficient, and dropping the state variable removescurrentYearfrom theuseEffectdependency array. -
Silent failure: When
fetchIdeasFromGitHubrejects (API rate-limit, network error, etc.), thecatchblock only logs the error and leavesarticlesas[]. The rendered output is an empty<Grid>with no error message, which gives users no indication that something went wrong.
🐛 Proposed fix
- const [currentYear, setCurrentYear] = useState(getCurrentYear());
+ const currentYear = getCurrentYear();
const [isLoading, setIsLoading] = useState(true);
+ const [error, setError] = useState(null);
useEffect(() => {
const fetchData = async () => {
setIsLoading(true);
+ setError(null);
try {
const ideas = await fetchIdeasFromGitHub(currentYear);
setArticles(ideas);
} catch (error) {
console.error('Error fetching ideas:', error);
+ setError('Failed to load ideas. Please try again later.');
} finally {
setIsLoading(false);
}
};
fetchData();
- }, [currentYear]);
+ }, []);Then add an error branch to the render:
{error && (
<p className="text-center text-red-600 dark:text-red-400">{error}</p>
)}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/ideas/index.jsx` around lines 119 - 136, Replace the unused state
for currentYear with a stable value (use const currentYear = getCurrentYear() or
useMemo) so you can remove currentYear from the useEffect dependency list; add
an error state (e.g., const [error, setError] = useState(null)) and in
fetchData's catch call setError(error.message || String(error)) and optionally
setArticles([]) as appropriate; keep setIsLoading(false) in finally; update the
component render to show a visible error branch (e.g., when error is truthy
render a centered red message) so users see failures from fetchIdeasFromGitHub
instead of an empty Grid.
src/pages/index.jsx
Outdated
| const timer = setTimeout(() => { | ||
| setRandomProjects(projects.sort(() => 0.5 - Math.random()).slice(0, 3)) | ||
| setIsLoading(false) |
There was a problem hiding this comment.
projects.sort() mutates the shared imported module array in place.
Array.prototype.sort mutates the original array. Since projects is a module-level import, this permanently reorders it for every consumer in the same module graph (other pages, future effects, HMR reloads, etc.).
🐛 Proposed fix — sort a shallow copy
- setRandomProjects(projects.sort(() => 0.5 - Math.random()).slice(0, 3))
+ setRandomProjects([...projects].sort(() => 0.5 - Math.random()).slice(0, 3))📝 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.
| const timer = setTimeout(() => { | |
| setRandomProjects(projects.sort(() => 0.5 - Math.random()).slice(0, 3)) | |
| setIsLoading(false) | |
| const timer = setTimeout(() => { | |
| setRandomProjects([...projects].sort(() => 0.5 - Math.random()).slice(0, 3)) | |
| setIsLoading(false) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/pages/index.jsx` around lines 94 - 96, The current timer callback uses
projects.sort(...), which mutates the imported module array; change it to sort a
shallow copy instead (e.g., use [...projects] or projects.slice() before .sort)
inside the timer callback that calls setRandomProjects and setIsLoading so the
module-level projects array is not mutated across consumers or HMR reloads.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/pages/ideas/[year]/[slug].jsx (2)
64-72:⚠️ Potential issue | 🟡 Minor
router.isFallbackis dead code —getStaticPathswas removed in this PR.
router.isFallbackis only evertrueduring on-demand ISR generation whengetStaticPathsexportsfallback: true. SincegetStaticPathswas deleted as part of this migration, this branch can never execute and can be safely removed.🐛 Proposed fix
- if (router.isFallback) { - return ( - <Container className="mt-16 mb-20"> - <div className="text-center"> - <p className="text-lg text-zinc-600 dark:text-zinc-400">Loading...</p> - </div> - </Container> - ); - } - if (isLoading) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/ideas/`[year]/[slug].jsx around lines 64 - 72, The router.isFallback branch in the component is dead code because getStaticPaths was removed; remove the entire conditional block that checks router.isFallback (and the accompanying Loading... JSX) and any now-unused references to router/useRouter in this file (e.g., the useRouter() call and its import) so the component no longer contains an unreachable fallback rendering path.
190-201:⚠️ Potential issue | 🟠 Major
inlineprop was removed in react-markdown v9 — inline code is always rendered as a block.The
codecomponent no longer acceptsinlineas a prop. Prior to 9.0.0, react-markdown exposed theinlineprop to code components; this functionality was removed in 9.0.0. With react-markdown v10.1.0,inlinewill always beundefined, so the ternary at line 191 always falls into the block-code branch, meaning backtick-quoted inline code (e.g.,`foo`) will be rendered as a block element.The idiomatic v9+/v10 fix is to check
classNamefor alanguage-*pattern to distinguish fenced code from inline:🐛 Proposed fix
-code: ({ node, inline, ...props }) => - inline ? ( - <code - className="bg-zinc-100 dark:bg-zinc-800 text-green-600 dark:text-yellow-400 px-2 py-1 rounded text-sm font-mono" - {...props} - /> - ) : ( - <code - className="block bg-zinc-100 dark:bg-zinc-800 text-zinc-800 dark:text-zinc-200 p-4 rounded-lg overflow-x-auto text-sm font-mono my-4" - {...props} - /> - ), +code: ({ node, className, children, ...props }) => { + const isBlock = /language-(\w+)/.exec(className || ''); + return isBlock ? ( + <code + className={`block bg-zinc-100 dark:bg-zinc-800 text-zinc-800 dark:text-zinc-200 p-4 rounded-lg overflow-x-auto text-sm font-mono my-4 ${className}`} + {...props} + > + {children} + </code> + ) : ( + <code + className="bg-zinc-100 dark:bg-zinc-800 text-green-600 dark:text-yellow-400 px-2 py-1 rounded text-sm font-mono" + {...props} + > + {children} + </code> + ); +},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/ideas/`[year]/[slug].jsx around lines 190 - 201, The code renderer passed to react-markdown still checks the removed "inline" prop; update the code renderer (the code: ({ node, inline, ...props }) => ...) to instead destructure className (e.g. code: ({ node, className, ...props }) => { const isCodeBlock = className && /language-/.test(className); return isCodeBlock ? /* block code markup with fenced styling */ : /* inline code markup with inline styling */ }). Replace the ternary that uses inline with a regex test on className (/language-/) to distinguish fenced code blocks from inline backtick code and apply the corresponding className/markup.
🧹 Nitpick comments (2)
src/pages/ideas/[year]/[slug].jsx (1)
48-59: Component-levelcatchis unreachable —fetchIdeaContentswallows all errors internally.
fetchIdeaContent(insrc/helper/fetchGitHubIdeas.js) catches every error and returnsnull, so thecatchblock here never executes.setIdea(null)is also a no-op sincenullis the initial state. Consider either removing the redundanttry/catchor havingfetchIdeaContentre-throw so the component can surface fetch failures to the user.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/ideas/`[year]/[slug].jsx around lines 48 - 59, The component-level try/catch in fetchData is ineffective because fetchIdeaContent swallows errors and returns null; either remove the redundant try/catch in fetchData (inside the component where fetchData sets setIsLoading, setIdea) or change fetchIdeaContent (in src/helper/fetchGitHubIdeas.js) to re-throw after logging so the catch in fetchData can run — specifically update fetchIdeaContent to throw the caught error (or return a result/throw pattern) and keep fetchData's try/catch to setIsLoading(false) and setIdea(null)/show an error state, or remove the try/catch in fetchData and rely on fetchIdeaContent's null return and handle that case after awaiting the call.src/pages/ideas/2024/index.jsx (1)
78-114:ArticleSkeletonandIdeasGridSkeletonare duplicated verbatim insrc/pages/ideas/index.jsx.Consider extracting both into
src/components/Skeletons/(alongside the existingSkeletonCard/SkeletonGrid) so they can be shared.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/ideas/2024/index.jsx` around lines 78 - 114, The ArticleSkeleton and IdeasGridSkeleton components are duplicated; extract both functions into a shared component module (e.g., a new Skeletons component alongside the existing SkeletonCard/SkeletonGrid), export them (ArticleSkeleton and IdeasGridSkeleton), replace the inline definitions in this file and the other duplicate file with imports of those exports, and update usages to render the imported ArticleSkeleton and IdeasGridSkeleton so the duplicated JSX is centralized and reused.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/pages/ideas/`[year]/[slug].jsx:
- Around line 64-72: The router.isFallback branch in the component is dead code
because getStaticPaths was removed; remove the entire conditional block that
checks router.isFallback (and the accompanying Loading... JSX) and any
now-unused references to router/useRouter in this file (e.g., the useRouter()
call and its import) so the component no longer contains an unreachable fallback
rendering path.
- Around line 190-201: The code renderer passed to react-markdown still checks
the removed "inline" prop; update the code renderer (the code: ({ node, inline,
...props }) => ...) to instead destructure className (e.g. code: ({ node,
className, ...props }) => { const isCodeBlock = className &&
/language-/.test(className); return isCodeBlock ? /* block code markup with
fenced styling */ : /* inline code markup with inline styling */ }). Replace the
ternary that uses inline with a regex test on className (/language-/) to
distinguish fenced code blocks from inline backtick code and apply the
corresponding className/markup.
---
Duplicate comments:
In `@src/pages/ideas/2024/index.jsx`:
- Around line 127-130: The fetch error is currently only console.logged in the
catch block causing an empty grid with no user feedback; add an error state
(e.g., error / setError) in the component, setError(error) inside the catch of
the fetch routine (the catch around "Error fetching ideas:"), and update the
render logic to show a user-facing error message or call-to-action when error is
non-null (instead of just rendering the empty articles grid); keep
setIsLoading(false) in finally so loading behaves the same.
- Around line 120-135: getAllIdeas uses Node-only APIs and must not be invoked
from the browser; replace the direct client-side call inside useEffect/fetchData
with a server-side data fetch (e.g., move the getAllIdeas call into
getStaticProps or getServerSideProps and pass the mapped articles as props to
the component) or expose a server API route that calls getAllIdeas and have
fetchData call that API endpoint instead; update the component to consume the
props or the API response and remove any direct use of process.cwd()/fast-glob
from client-side code (look for getAllIdeas, useEffect, and fetchData to apply
the change).
In `@src/pages/ideas/index.jsx`:
- Around line 118-135: The currentYear state (useState/getCurrentYear and
setCurrentYear) is never updated so keeping setCurrentYear and listing
currentYear in useEffect dependencies is dead code; change to a plain constant
(const currentYear = getCurrentYear()) and remove currentYear from the
dependency array of the useEffect that defines fetchData (which calls
fetchIdeasFromGitHub and setArticles), leaving the effect to run only on mount
(or add any real changing deps if intended).
- Around line 125-130: fetchIdeasFromGitHub currently swallows errors so the
component never surfaces failures; update the page component to detect and
surface load failures by adding an error state (e.g., setError) and, after
calling fetchIdeasFromGitHub, check if the returned ideas array is empty or null
and set that error state (and optionally log the underlying problem) before
calling setArticles; also keep the existing catch in place for future re-throws
from fetchIdeasFromGitHub so you cover both cases—reference symbols:
fetchIdeasFromGitHub, setArticles, setIsLoading, and add setError (or similar)
to the component and use it to render an error UI instead of an empty grid.
---
Nitpick comments:
In `@src/pages/ideas/`[year]/[slug].jsx:
- Around line 48-59: The component-level try/catch in fetchData is ineffective
because fetchIdeaContent swallows errors and returns null; either remove the
redundant try/catch in fetchData (inside the component where fetchData sets
setIsLoading, setIdea) or change fetchIdeaContent (in
src/helper/fetchGitHubIdeas.js) to re-throw after logging so the catch in
fetchData can run — specifically update fetchIdeaContent to throw the caught
error (or return a result/throw pattern) and keep fetchData's try/catch to
setIsLoading(false) and setIdea(null)/show an error state, or remove the
try/catch in fetchData and rely on fetchIdeaContent's null return and handle
that case after awaiting the call.
In `@src/pages/ideas/2024/index.jsx`:
- Around line 78-114: The ArticleSkeleton and IdeasGridSkeleton components are
duplicated; extract both functions into a shared component module (e.g., a new
Skeletons component alongside the existing SkeletonCard/SkeletonGrid), export
them (ArticleSkeleton and IdeasGridSkeleton), replace the inline definitions in
this file and the other duplicate file with imports of those exports, and update
usages to render the imported ArticleSkeleton and IdeasGridSkeleton so the
duplicated JSX is centralized and reused.
Addressed Issues:
Fixes #277
Recordings:
Screen.Recording.2026-02-23.025129.mp4
Additional Notes:
This PR introduces consistent skeleton screens and route-level loading states across the entire application to improve perceived performance and eliminate layout shifts.
Key Changes:
_app.jsxIdeas Section Refactor:
getStaticPropsandgetStaticPathsuseState+useEffectIf maintaining static generation for SEO is preferred, I’m happy to adjust the implementation accordingly.
All changes were tested locally (including slow network simulation and dark mode).
Summary by CodeRabbit
New Features
Bug Fixes