fix Add Responsive Pagination to People Page Contributors List#258
fix Add Responsive Pagination to People Page Contributors List#258kiransatdive wants to merge 5 commits into
Conversation
❌ Deploy Preview for cv-community-dashboard failed.
|
WalkthroughAdds internal helpers to LeaderboardCard and ContributorCard to build and open GitHub search URLs, wires onClick handlers to activity chips (making them clickable) and adds cursor-pointer/hover/transition styles. Tweaks Clear/Reset button hover/active styling in LeaderboardView and removes an explicit color class from the GitFork icon in home-dashboard. Implements client-side, URL-synced pagination in app/people/page.tsx including page-size selector, navigation helpers, and dynamic pagination UI; preserves existing data fetching and exported signatures. Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 `@components/Leaderboard/LeaderboardCard.tsx`:
- Around line 165-198: getGitHubSearchUrl and openGitHubSearch are duplicated
across LeaderboardCard.tsx and ContributorCard.tsx and are recreated on every
render; extract them to a shared module (e.g., lib/github-search.ts) at module
scope, move the username regex (USERNAME_RE) there, implement and export
getGitHubSearchUrl(activityName: string, username: string): string | null and
openGitHubSearch(activityName: string, username: string): void, ensure the same
activityName -> query mappings are used in the shared function (including "PR
merged", "PR opened", "Issue opened", "Issue closed", "Issue assigned", "Issue
labeled", "Review submitted"), update both LeaderboardCard and ContributorCard
to import and call these exported functions instead of defining them inline, and
keep window.open behavior in openGitHubSearch so callers simply invoke it.
- Around line 287-294: The clickable activity items in LeaderboardCard use
onClick on a <div> (the element that calls openGitHubSearch with activityName
and entry.username and uses cn/style.bgColor/style.borderColor) which is not
keyboard-accessible; replace each such interactive <div> with a semantic
<button> (respecting existing classes/styling) or add role="button",
tabIndex={0}, and an onKeyDown handler that calls the same openGitHubSearch when
Enter or Space is pressed, and ensure focus styles remain visible; update all
similar occurrences in this component where onClick is used for interactivity.
- Around line 184-185: In the LeaderboardCard component's switch handling for
case "Issue labeled", replace the invalid GitHub search qualifier `labeler:`
with the correct `label:` if the goal is to search by label name (update the
returned URL in the case "Issue labeled" inside LeaderboardCard.tsx), or if the
real intent is to find issues labeled by a specific user (actor), change the
implementation to call the GitHub API (e.g., via REST search/issues or GraphQL)
and filter by events/actor instead, since GitHub search does not support
filtering by the user who applied a label.
🧹 Nitpick comments (1)
components/people/ContributorCard.tsx (1)
52-77: Subset of activity types handled vsLeaderboardCard.This version of
getGitHubSearchUrlonly handles 3 activity types while the copy inLeaderboardCard.tsxhandles 7. If new activity types are displayed here in the future, the search links will silently fail. This is another reason to consolidate into a shared utility as suggested in theLeaderboardCardreview.
Git-HimanshuRathi
left a comment
Git-HimanshuRathi
left a comment
There was a problem hiding this comment.
@kiransatdive can u fill the PR template and also mention which issue your are fixing ?
- Implement pagination for contributor cards (12, 24, 48, 96, All options) - Add URL parameter handling for page and limit (?page=2&limit=48) - Add pagination controls with previous/next buttons and page numbers - Add intelligent ellipsis for large page counts - Maintain search functionality with page reset - Ensure responsive design and accessibility - Consistent with Leaderboard page pagination behavior Fixes issue with ~278 contributors showing in single continuous list
- Resolved merge conflict in app/people/page.tsx - Kept pagination implementation from main branch - Ready to push pagination feature to feat/kiran-satdive branch
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 (1)
app/people/page.tsx (1)
52-63:⚠️ Potential issue | 🟡 Minor
res.json()returned withoutawaitin anasyncfunction.Line 59 returns the raw
Promisefromres.json()without awaiting it. While this works (the caller awaits the outer async function), ifres.json()throws a parse error, it will reject the returned promise rather than being caught by the surroundingtry/catchblock. This means a malformed JSON response bypasses the graceful error handling and instead surfaces as an unhandled rejection in theloadDataeffect's catch.Suggested fix
- return res.json(); + return await res.json();
🤖 Fix all issues with AI agents
In `@app/people/page.tsx`:
- Around line 397-411: The JSX block for the contributors section (the outer
<div className="mb-8"> containing the TITLE h2 and DESCRIPTION p and its closing
</div>) has inconsistent indentation; reformat that entire block to match the
surrounding file's JSX indentation style (use the same 2-space or 4-space
nesting used in the rest of app/people/page.tsx), ensuring each child element
(the inner <div className="mb-4">, <h2>, <span>s, and <p
className="text-lg...">) is indented consistently and the closing </div> aligns
with its opening tag.
- Around line 201-220: The pagination local state is being redundantly synced
with URL params causing divergence and extra renders; remove the two useEffect
syncs that read searchParams and call setCurrentPage/setPageSize, instead derive
currentPage and pageSize directly from searchParams using useMemo (or compute
inline) so the URL is the single source of truth, and update the helpers
updatePage and updatePageSize to only call router.replace(...) (or router.push)
to update searchParams without calling setState; keep references to
searchParams, setCurrentPage, setPageSize, updatePage, and updatePageSize so you
can locate and remove the state writes and replace them with URL-only updates
and memoized reads.
- Around line 229-234: The effect that currently resets to page 1 when
currentPage > totalPages should instead clamp the page to the last valid page;
update the useEffect (watching currentPage and totalPages) to call
updatePage(totalPages) when currentPage > totalPages && totalPages > 0 so that
pages bookmarked beyond the dataset are corrected to the last page rather than
silently jumping to page 1; keep the existing guards (totalPages > 0) and
reference useEffect, updatePage, currentPage, totalPages, and filteredPeople
when making the change.
- Around line 222-227: The useEffect that resets pagination uses updatePage,
currentPage, and pageSize but only lists searchQuery in its deps, causing a
stale closure; update the effect to either inline the URL-update logic (read and
set searchParams/pathname/router directly) or ensure updatePage is stable by
wrapping it in useCallback and include updatePage, currentPage, and pageSize
(and any values it closes over such as searchParams, pathname, router) in the
dependency array; similarly fix the other effect at lines 229–234 so all
referenced symbols (updatePage, currentPage, pageSize, searchParams, pathname,
router) are properly included or the logic is inlined to make the URL the single
source of truth.
🧹 Nitpick comments (1)
app/people/page.tsx (1)
413-482: Inconsistent indentation continues in the main content area.Lines 413–414 jump to 12-space indentation while the surrounding contributor title/description block (lines 397–411) used a different indentation scheme. This suggests the two blocks were authored or pasted separately. Please normalize to a single consistent indent level throughout the returned JSX.
| // Sync pagination state with URL params | ||
| useEffect(() => { | ||
| const page = searchParams.get('page'); | ||
| const parsedPage = page ? parseInt(page, 10) : 1; | ||
| if (parsedPage > 0) { | ||
| setCurrentPage(parsedPage); | ||
| } | ||
| }, [searchParams]); | ||
|
|
||
| useEffect(() => { | ||
| const limit = searchParams.get('limit'); | ||
| if (limit === 'all') { | ||
| setPageSize(Infinity); | ||
| } else { | ||
| const parsedLimit = limit ? parseInt(limit, 10) : 24; | ||
| if ([12, 24, 48, 96].includes(parsedLimit)) { | ||
| setPageSize(parsedLimit); | ||
| } | ||
| } | ||
| }, [searchParams]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Redundant bidirectional sync between URL and local state is fragile.
There are two sources of truth for currentPage and pageSize: local useState and URL search params. The helpers (updatePage, updatePageSize) write to both, and these useEffect hooks read from URL back into local state. This creates:
- A brief window each render where local state and URL can diverge.
- Unnecessary re-renders (state is set in the helper, then the effect sets it again from the URL).
- Hard-to-trace bugs when external navigation (browser back/forward) changes the URL.
Consider deriving pagination values directly from searchParams via useMemo, making the URL the single source of truth:
Sketch: derive from URL instead of syncing
- const [pageSize, setPageSize] = useState<number>(() => { ... });
- const [currentPage, setCurrentPage] = useState<number>(() => { ... });
+ const pageSize = useMemo(() => {
+ const limit = searchParams.get('limit');
+ if (limit === 'all') return Infinity;
+ const parsed = limit ? parseInt(limit, 10) : 24;
+ return [12, 24, 48, 96].includes(parsed) ? parsed : 24;
+ }, [searchParams]);
+
+ const currentPage = useMemo(() => {
+ const page = searchParams.get('page');
+ const parsed = page ? parseInt(page, 10) : 1;
+ return parsed > 0 ? parsed : 1;
+ }, [searchParams]);Then the updatePage / updatePageSize helpers only need to call router.replace(...) — no setState calls, no sync effects.
🤖 Prompt for AI Agents
In `@app/people/page.tsx` around lines 201 - 220, The pagination local state is
being redundantly synced with URL params causing divergence and extra renders;
remove the two useEffect syncs that read searchParams and call
setCurrentPage/setPageSize, instead derive currentPage and pageSize directly
from searchParams using useMemo (or compute inline) so the URL is the single
source of truth, and update the helpers updatePage and updatePageSize to only
call router.replace(...) (or router.push) to update searchParams without calling
setState; keep references to searchParams, setCurrentPage, setPageSize,
updatePage, and updatePageSize so you can locate and remove the state writes and
replace them with URL-only updates and memoized reads.
| // Reset to page 1 when search query changes | ||
| useEffect(() => { | ||
| if (currentPage !== 1 && pageSize !== Infinity) { | ||
| updatePage(1); | ||
| } | ||
| }, [searchQuery]); |
There was a problem hiding this comment.
Missing dependencies in useEffect — stale closure risk.
updatePage is called inside this effect but is not listed in the dependency array. Since updatePage closes over searchParams, pathname, and router, a stale closure can occur when URL state changes between renders without searchQuery changing. Additionally, currentPage and pageSize are read but also not declared as dependencies.
The React exhaustive-deps rule would flag this. The same issue applies to the effect at lines 229–234.
Suggested fix: inline the URL update or wrap helpers in useCallback
- // Reset to page 1 when search query changes
- useEffect(() => {
- if (currentPage !== 1 && pageSize !== Infinity) {
- updatePage(1);
- }
- }, [searchQuery]);
+ // Reset to page 1 when search query changes
+ useEffect(() => {
+ if (currentPage !== 1 && pageSize !== Infinity) {
+ const params = new URLSearchParams(searchParams.toString());
+ params.delete("page");
+ setCurrentPage(1);
+ router.replace(`${pathname}?${params.toString()}`, { scroll: false });
+ }
+ // eslint-disable-next-line react-hooks/exhaustive-deps
+ }, [searchQuery]);A cleaner long-term approach: derive currentPage and pageSize directly from searchParams (making the URL the single source of truth) instead of duplicating into local state and syncing via effects. This eliminates the entire class of stale-closure and out-of-sync bugs.
🤖 Prompt for AI Agents
In `@app/people/page.tsx` around lines 222 - 227, The useEffect that resets
pagination uses updatePage, currentPage, and pageSize but only lists searchQuery
in its deps, causing a stale closure; update the effect to either inline the
URL-update logic (read and set searchParams/pathname/router directly) or ensure
updatePage is stable by wrapping it in useCallback and include updatePage,
currentPage, and pageSize (and any values it closes over such as searchParams,
pathname, router) in the dependency array; similarly fix the other effect at
lines 229–234 so all referenced symbols (updatePage, currentPage, pageSize,
searchParams, pathname, router) are properly included or the logic is inlined to
make the URL the single source of truth.
| // Reset to page 1 when filtered results change significantly | ||
| useEffect(() => { | ||
| if (currentPage > totalPages && totalPages > 0) { | ||
| updatePage(1); | ||
| } | ||
| }, [currentPage, totalPages]); |
There was a problem hiding this comment.
This safety-net effect can trigger an unnecessary navigation on initial load.
When data first loads, filteredPeople goes from [] to a populated array. Before data arrives, totalPages is 0 (from Math.ceil(0 / pageSize)), and if the URL had ?page=2, currentPage would be 2. The condition currentPage > totalPages && totalPages > 0 guards against totalPages === 0, so this specific scenario is safe. However, if a user bookmarks ?page=5 but the dataset only has 3 pages, this resets silently to page 1 instead of the last valid page — a minor UX concern.
Consider clamping to totalPages instead of resetting to 1:
Suggested fix
useEffect(() => {
if (currentPage > totalPages && totalPages > 0) {
- updatePage(1);
+ updatePage(totalPages);
}
}, [currentPage, totalPages]);📝 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.
| // Reset to page 1 when filtered results change significantly | |
| useEffect(() => { | |
| if (currentPage > totalPages && totalPages > 0) { | |
| updatePage(1); | |
| } | |
| }, [currentPage, totalPages]); | |
| // Reset to page 1 when filtered results change significantly | |
| useEffect(() => { | |
| if (currentPage > totalPages && totalPages > 0) { | |
| updatePage(totalPages); | |
| } | |
| }, [currentPage, totalPages]); |
🤖 Prompt for AI Agents
In `@app/people/page.tsx` around lines 229 - 234, The effect that currently resets
to page 1 when currentPage > totalPages should instead clamp the page to the
last valid page; update the useEffect (watching currentPage and totalPages) to
call updatePage(totalPages) when currentPage > totalPages && totalPages > 0 so
that pages bookmarked beyond the dataset are corrected to the last page rather
than silently jumping to page 1; keep the existing guards (totalPages > 0) and
reference useEffect, updatePage, currentPage, totalPages, and filteredPeople
when making the change.
| <div className="mb-8"> | ||
| {/* TITLE */} | ||
| <div className="mb-4"> | ||
| <h2 className="text-3xl font-bold"> | ||
| <span className="text-black dark:text-white">Community </span> | ||
| <span className="text-[#42B883]">Contributors</span> | ||
| </h2> | ||
| </div> | ||
|
|
||
| <p className="text-lg text-muted-foreground max-w-3xl mb-6"> | ||
| Amazing community members who contribute to CircuitVerse through | ||
| code, documentation, and more. | ||
| </p> | ||
| <h2 className="text-3xl font-bold"> | ||
| <span className="text-black dark:text-white">Community </span> | ||
| <span className="text-[#42B883]">Contributors</span> | ||
| </h2> | ||
| </div> | ||
|
|
||
| <div className="flex flex-col gap-4"> | ||
| <PeopleStats | ||
| contributors={filteredPeople} | ||
| allContributors={people} | ||
| onContributorClick={handleContributorClick} | ||
| /> | ||
|
|
||
| <div className="flex items-center justify-between gap-4 py-8"> | ||
| <div className="flex items-center gap-2"> | ||
| <Users className="w-5 h-5 text-muted-foreground" /> | ||
| <span className="text-2xl font-bold text-foreground"> | ||
| {filteredPeople.length}{' '} | ||
| <span className="text-[#42B883]"> | ||
| {filteredPeople.length === 1 ? 'Contributor' : 'Contributors'} | ||
| </span> | ||
| {searchQuery && <span className="text-foreground"> found</span>} | ||
| </span> | ||
| </div> | ||
|
|
||
| <div className="relative w-full sm:w-72"> | ||
| <Search className="absolute left-3 top-1/2 -translate-y-1/2 h-4 w-4 text-muted-foreground" /> | ||
| <Input | ||
| type="text" | ||
| placeholder="Search contributors..." | ||
| value={searchQuery} | ||
| onChange={(e) => setSearchQuery(e.target.value)} | ||
| className="pl-9 h-10" | ||
| /> | ||
| </div> | ||
| </div> | ||
|
|
||
| <PeopleGrid | ||
| contributors={filteredPeople} | ||
| onContributorClick={handleContributorClick} | ||
| viewMode="grid" | ||
| loading={false} | ||
| /> | ||
| </div> | ||
| </section> | ||
| {/* DESCRIPTION */} | ||
| <p className="text-lg text-muted-foreground max-w-3xl mb-6"> | ||
| Amazing community members who contribute to CircuitVerse through | ||
| code, documentation, and more. | ||
| </p> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Severely inconsistent indentation in the contributors section.
This block mixes 0, 2, 4, and 8-space indentation levels across siblings and parent-child elements. For example, the closing </div> at line 411 is at column 0 while its opening tag at line 397 is indented. This makes the JSX structure very hard to follow and maintain.
Please re-indent this block to match the surrounding code's indentation style (the rest of the file uses consistent 2-space or 4-space nesting within the JSX tree).
🤖 Prompt for AI Agents
In `@app/people/page.tsx` around lines 397 - 411, The JSX block for the
contributors section (the outer <div className="mb-8"> containing the TITLE h2
and DESCRIPTION p and its closing </div>) has inconsistent indentation; reformat
that entire block to match the surrounding file's JSX indentation style (use the
same 2-space or 4-space nesting used in the rest of app/people/page.tsx),
ensuring each child element (the inner <div className="mb-4">, <h2>, <span>s,
and <p className="text-lg...">) is indented consistently and the closing </div>
aligns with its opening tag.
okay |
kiransatdive
changed the title
kiransatdive
changed the title
kiransatdive
changed the title
3 participants
Description
This PR fixes issues related to the People Page Contributors List by adding responsive, client-side pagination and improving contributor interactions. The pagination is optimized for different screen sizes and keeps the page state in sync with the URL for better navigation and sharing.
fixes Issue #255
Type of change
What Was Fixed / Improved
-Added responsive pagination to the contributors list