feat: add responsive pagination to contributors list#256
feat: add responsive pagination to contributors list#256Omkarchavan2527 wants to merge 2 commits into
Conversation
❌ Deploy Preview for cv-community-dashboard failed.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe People page was simplified: all client-side data fetching, local state, effects, per-contributor navigation, and inline rendering for Core Team, Alumni, and Contributors were removed. The page now renders a single client-side Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@app/people/page.tsx`:
- Around line 224-233: The current updatePage function (and similar functions
updatePageSize and the reset effect) uses window.history.replaceState which
bypasses Next.js routing; import useRouter from "next/navigation", call const
router = useRouter() in the component, and replace window.history.replaceState
calls with router.replace(...) so Next's internal navigation state updates
correctly; when building the replacement URL use params.toString() and omit the
trailing "?" when it's an empty string (i.e., use pathname alone when
params.toString() === "") to avoid URLs like "/people?".
- Around line 197-207: The effect that resets currentPage (useEffect checking
currentPage > totalPages) causes a render where paginatedPeople is empty;
instead ensure the effective page is computed or reset synchronously when the
search changes: in the search onChange handler (where
filteredPeople/searchParams are updated) call setCurrentPageState(1) immediately
(or update the URL params to remove page) so filteredPeople, totalPages and
currentPage remain consistent within the same render; alternatively compute an
effectivePage variable used by pagination logic (e.g., const effectivePage =
Math.min(currentPage, Math.max(1, totalPages)) or default to 1) and use
effectivePage for deriving paginatedPeople so you avoid the intermediate empty
render—apply the change around the search input handler and pagination code that
derives paginatedPeople.
- Line 74: The page currently calls useSearchParams() inside the "use client"
PeoplePage which forces full client rendering; extract the search-params logic
into a small Client Component (e.g., create a new component named PeopleSearch
that includes "use client" and calls useSearchParams()), then import and render
<PeopleSearch/> inside the server component and wrap it with React.Suspense
(e.g., <Suspense fallback={...}><PeopleSearch/></Suspense>); update PeoplePage
to remove useSearchParams() usage so the rest of PeoplePage remains a
server/static component and only PeopleSearch is client-side.
🧹 Nitpick comments (2)
app/people/page.tsx (2)
81-81: MovePAGE_SIZE_OPTIONSto module scope.This array is recreated on every render. Since it's a constant, declare it outside the component.
+const PAGE_SIZE_OPTIONS = [24, 48, 96, 192] as const; + export default function PeoplePage() { - const PAGE_SIZE_OPTIONS = [24, 48, 96, 192] as const;
207-207: Non-idiomatic dependency array expression:pathname ?? "".Inline expressions in useEffect deps can confuse the React exhaustive-deps lint rule and other developers.
pathnamefromusePathname()is always a string in a page component—just usepathnamedirectly.- }, [totalPages, currentPage, searchParamsString, pathname ?? ""]); + }, [totalPages, currentPage, searchParamsString, pathname]);
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/people/PeopleSearch.tsx`:
- Around line 52-63: The fetchPeople function currently returns res.json()
without awaiting it, which allows JSON parsing errors to escape the local
try/catch; update fetchPeople so you await the JSON parse (await res.json())
before returning, ensuring any exceptions from res.json() are caught by the
try/catch and the fallback result ({ updatedAt: 0, people: [], coreTeam: [],
alumni: [] }) is returned on error.
🧹 Nitpick comments (4)
app/people/PeopleSearch.tsx (3)
65-178: Consider breaking this ~570-line component into smaller pieces.
PeopleSearchhandles data fetching, URL sync, search/filter, pagination logic, scroll management, error/loading states, contributor detail view, and the full render tree. This is a lot of responsibility for a single component, making it harder to test and maintain.Extracting focused subcomponents or custom hooks (e.g., a
usePeoplePaginationhook for pagination/URL sync, aContributorsSectioncomponent for the grid + pagination UI) would improve readability without changing behavior.
25-27: Nit: Extra blank lines.There are three consecutive blank lines (25–27) between the imports and the interface declaration. One blank line is sufficient.
140-151: Hash-based scroll effect doesn't clean up ifloadingnever transitions.The effect only runs when
loadingchanges. If the component mounts withwindow.location.hash === "#contributors"but data loads very quickly (or is cached), the 50mssetTimeoutraces with the DOM being ready. This is a minor edge case since thesetTimeoutprovides a small buffer, but worth noting.app/people/page.tsx (1)
1-9: Provide a meaningful Suspense fallback instead ofnull.
fallback={null}renders a blank page while the client component suspends (e.g., during static pre-rendering or initial hydration whenuseSearchParamsisn't available yet). SincePeopleSearchalready has a detailed loading skeleton, consider extracting that skeleton into a shared component and using it here, or at minimum show a lightweight spinner:Quick improvement
+import { Loader2 } from "lucide-react"; import { Suspense } from "react"; import PeopleSearch from "./PeopleSearch"; export default function PeoplePage() { return ( - <Suspense fallback={null}> + <Suspense fallback={ + <div className="flex items-center justify-center min-h-screen"> + <Loader2 className="w-12 h-12 animate-spin text-green-600" /> + </div> + }> <PeopleSearch /> </Suspense> ); }
ffe2b08 to
8e37c4a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/people/page.tsx`:
- Around line 6-8: The Suspense fallback currently returns null, causing a blank
page while the PeopleSearch client bundle loads; extract the skeleton markup
(from PeopleSearch.tsx lines 302–394) into a new shared server component (e.g.,
PeopleSearchSkeleton), export it, import it into app/people/page.tsx, and
replace fallback={null} with <PeopleSearchSkeleton /> so the lightweight loading
UI displays until the client component mounts; ensure the skeleton contains only
server-renderable markup/props and no client-only hooks.
🧹 Nitpick comments (5)
app/people/PeopleSearch.tsx (5)
203-211: Missingrouterin the effect dependency array.
router.replaceis called inside this effect butrouterisn't listed in the dependency array. Whilerouteris practically stable in Next.js, the exhaustive-deps lint rule will flag this. Same applies tobuildUrl(which closes overpathname).Proposed fix
- }, [totalPages, currentPage, searchParamsString, pathname ?? ""]); + }, [totalPages, currentPage, searchParamsString, pathname, router]);
86-138:PAGE_SIZE_OPTIONSis recreated every render and referenced in effects — hoist it to module scope.Since it's a constant array, defining it inside the component body means a new reference each render. Hoisting avoids this and makes the effects cleaner.
Proposed fix
+const PAGE_SIZE_OPTIONS = [24, 48, 96, 192] as const; + export default function PeopleSearch() { ... - const PAGE_SIZE_OPTIONS = [24, 48, 96, 192] as const;
302-394: Consider extracting the loading skeleton into a separate component.This ~90-line skeleton block adds significant visual noise to an already large component (~574 lines). Extracting it (e.g.
PeopleLoadingSkeleton) would improve readability and make it reusable as the Suspense fallback inpage.tsx(instead offallback={null}).
509-551: Extract pagination page-number generation into a helper.The inline IIFE that builds the truncated page list adds cognitive load. A small
getPageNumbers(currentPage, totalPages)utility would be easier to read and unit-test independently.
228-234:updatePagereadssearchParamsdirectly — prefer the stablesearchParamsString.
searchParams?.toString()on Line 229 reads the hook object, whilesearchParamsString(Line 84) was explicitly created as a stable primitive for effect deps. Using both patterns is inconsistent and can lead to subtle staleness bugs ifsearchParamsreference hasn't updated yet. The same applies toupdatePageSize(Line 238).Proposed fix
const updatePage = (page: number) => { - const params = new URLSearchParams(searchParams?.toString() ?? ""); + const params = new URLSearchParams(searchParamsString);
| <Suspense fallback={null}> | ||
| <PeopleSearch /> | ||
| </Suspense> |
There was a problem hiding this comment.
fallback={null} shows a blank page while the client bundle loads.
Until the PeopleSearch client component JS is downloaded, parsed, and mounted, the user sees nothing. Provide a lightweight loading skeleton as the fallback for better perceived performance. This is especially important on mobile / slow connections.
Proposed fix
+import { PeopleLoadingSkeleton } from "./PeopleLoadingSkeleton";
+
export default function PeoplePage() {
return (
- <Suspense fallback={null}>
+ <Suspense fallback={<PeopleLoadingSkeleton />}>
<PeopleSearch />
</Suspense>
);
}Extract the skeleton from PeopleSearch.tsx Lines 302–394 into a shared server component so it can be used here.
🤖 Prompt for AI Agents
In `@app/people/page.tsx` around lines 6 - 8, The Suspense fallback currently
returns null, causing a blank page while the PeopleSearch client bundle loads;
extract the skeleton markup (from PeopleSearch.tsx lines 302–394) into a new
shared server component (e.g., PeopleSearchSkeleton), export it, import it into
app/people/page.tsx, and replace fallback={null} with <PeopleSearchSkeleton />
so the lightweight loading UI displays until the client component mounts; ensure
the skeleton contains only server-renderable markup/props and no client-only
hooks.
Heyy-Himanshuu
left a comment
Heyy-Himanshuu
left a comment
There was a problem hiding this comment.
would suggest you to keep the number like 50 , 100 to maintain the consistency accross the website as other pages also uses the same format .
8e37c4a to
f96b65c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@app/people/PeopleSearch.tsx`:
- Around line 86-126: The initializer for pageSize (useState for pageSize)
returns 10 when no limit param, but the sync useEffect sets pageSize to 50 by
default, causing a layout shift; make the default consistent (pick one, e.g.,
10) by updating the useEffect branch that currently does setPageSizeState(50)
and the else branch to set the same chosen default (or change the initializer
instead if you prefer 50), ensuring PAGE_SIZE_OPTIONS, pageSize, and
setPageSizeState are all using the same default value.
🧹 Nitpick comments (2)
app/people/PeopleSearch.tsx (2)
509-551: Consider extracting the page-number generation into a helper function.The IIFE inside JSX (Lines 509–551) computes page numbers with ellipsis logic and renders buttons — this is ~40 lines of inline logic that would be easier to read and test as a standalone
getPageNumbers(currentPage, totalPages)utility. Optional cleanup.
302-394: Extract loading skeleton into a shared component.This ~90-line skeleton block is a good candidate for extraction into a standalone
PeopleLoadingSkeletonserver component. It could then also be used as the<Suspense fallback>inpage.tsx(which currently passesnull, showing a blank page while the client bundle loads).
f96b65c to
ec63ebf
Compare
|
CI is still red . |
ec63ebf to
b562bbc
Compare
2 participants
Description
This PR adds responsive pagination to the People page contributors list. Instead of rendering all contributor cards in a single long list, contributors are now displayed in smaller pages to improve performance, reduce excessive scrolling, and provide a more structured browsing experience on both desktop and mobile.
Pagination state is also synced with the page URL so users can reload or share the same view.
Related Issue
Fixes #255
Type of change
Checklist
Screenshots (if applicable)
before :
people-CircuitVerse.mp4
After:
Summary by CodeRabbit
New Features
UX Improvements