[#332] Blade/member pagination & search #358
Conversation
Ran ui-add for shadcn pagination component Fixed any issues from the generated pagination.tsx file Exported ButtonProps from button.tsx Updated members-table to include the optional inputs
Implemented Generic Pagination to members-table component
Removed logic rendering total amount of member
Updated getMembers and getMemberCount endpoints Moved filtering logic from frontend to backend Commented out a lot of logic for the moment
Added compatability to search via full name
(This endpoint is getting large :p)
Ran format, lint and typecheck
jesusthecreator017
added
Onboarding
📝 WalkthroughWalkthroughIntroduces server-driven pagination, sorting, and filtering for the members management feature. Adds CustomPagination and CustomPaginationSelect components, a useDebounce hook, enhanced API procedures with filtering/sorting logic, and new pagination UI library components. Changes
Sequence DiagramsequenceDiagram
participant User
participant MemberTable as Members Table
participant Router as Next Router
participant API as Member API
participant DB as Database
User->>MemberTable: Change page/size or search
MemberTable->>Router: Update query params (page, pageSize, searchTerm)
Router->>API: Call getMembers(currentPage, pageSize, searchTerm, sortField, sortOrder)
API->>DB: Query members with WHERE clause (ILIKE), ORDER BY, LIMIT/OFFSET
DB-->>API: Return filtered & sorted member rows
API->>DB: Query getMemberCount(searchTerm)
DB-->>API: Return total matching count
API-->>MemberTable: Return paginated results + totalCount
MemberTable->>MemberTable: Render table rows + pagination controls
MemberTable-->>User: Display updated members list with pagination UI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 5
🤖 Fix all issues with AI agents
In `@apps/blade/src/app/admin/_components/CustomPagination.tsx`:
- Around line 81-86: The PaginationPrevious/PaginationNext controls still fire
changePage when at bounds because aria-disabled only informs AT; update the
CustomPagination component to also prevent activation: set the control's
disabled prop (or remove/replace the onClick) when currentPage <= 1 for
PaginationPrevious and when currentPage >= totalPages for PaginationNext, and
keep aria-disabled in sync; additionally guard the changePage invocation inside
the onClick handlers (no-op if out of range) so clicking or programmatic
activation cannot change pages when disabled.
In `@apps/blade/src/app/admin/_components/CustomPaginationSelect.tsx`:
- Around line 1-8: Add the Next.js client directive to this component so event
handlers like onValueChange work on the client: insert the string "use client"
as the very first line of the CustomPaginationSelect component file (before any
imports) so the React component and handlers (e.g., Select, SelectTrigger,
onValueChange usage) run on the client rather than the server.
In `@apps/blade/src/app/admin/club/members/_components/members-table.tsx`:
- Line 49: The currentPage computed in members-table.tsx (const currentPage =
Number(searchParams.get("page") || 1)) isn't validated and can become NaN or
negative; update the logic that derives currentPage (used by the MembersTable
pagination) to parse the value (e.g., parseInt), treat non-numeric results as 1,
clamp the result to a minimum of 1, and if you have a known totalPages or
pageCount available in this component further clamp to that upper bound before
using it for rendering or navigation.
In `@packages/api/src/routers/member.ts`:
- Around line 374-393: The time-based sorting branch currently calls
query.orderBy(...) without reassigning the result and mixes
Member.dateCreated/timeCreated inconsistently; change the sort-by-time branch to
assign the result back (query = query.orderBy(...)) and use the correct columns
consistently (both clauses should reference Member.timeCreated, using asc when
input.sortOrder === "asc" and desc when "desc") so that sorting by time is
actually applied; keep the other branches (sortField and default) unchanged.
- Around line 400-407: The getMemberCount endpoint is currently exposed as
publicProcedure which allows unauthenticated enumeration via the searchTerm
filter; change its declaration from publicProcedure to permProcedure and apply
the same permission check used by getMembers (controlPerms.or(["READ_MEMBERS",
"READ_CLUB_DATA"], ctx)) so only authorized users can call getMemberCount, and
keep the existing searchTerm input handling; alternatively, if public access is
required, remove the searchTerm filtering entirely from getMemberCount so it
only returns an unfiltered total count.
🧹 Nitpick comments (6)
apps/blade/src/app/admin/_hooks/debounce.ts (1)
3-19: Clean debounce implementation!The hook correctly handles cleanup to prevent stale updates. One tiny typo on line 7: "timout" → "timeout".
- // Set a timout to update the debounced value after the short delay + // Set a timeout to update the debounced value after the short delaypackages/ui/src/pagination.tsx (1)
44-62: Consider keyboard accessibility forPaginationLink.
PaginationLinkrenders an<a>tag, but it's used withonClickhandlers (withouthref) inCustomPagination. Anchor tags withouthrefaren't keyboard-focusable by default.Two options:
- Add
role="button"andtabIndex={0}when nohrefis provided- Use a
<button>element whenonClickis the primary interaction♻️ Suggested enhancement
const PaginationLink = ({ className, isActive, size = "icon", + href, ...props }: PaginationLinkProps) => ( <a aria-current={isActive ? "page" : undefined} + role={href ? undefined : "button"} + tabIndex={href ? undefined : 0} className={cn( buttonVariants({ variant: isActive ? "outline" : "ghost", size, }), className, )} + href={href} {...props} /> );packages/api/src/routers/member.ts (1)
359-372: Consider extracting the shared filter logic.The ILIKE filter conditions are duplicated between
getMembers(lines 359-372) andgetMemberCount(lines 414-424). Extracting this to a helper would improve maintainability.♻️ Example refactor
// At the top of the file or in a shared utils const buildMemberSearchFilter = (searchPattern: string) => or( ilike(Member.firstName, searchPattern), ilike(Member.lastName, searchPattern), ilike(Member.email, searchPattern), ilike(Member.discordUser, searchPattern), ilike(Member.company, searchPattern), sql`CONCAT(${Member.firstName}, ' ', ${Member.lastName}) ILIKE ${searchPattern}`, );Also applies to: 409-424
apps/blade/src/app/admin/_components/CustomPagination.tsx (1)
16-21: Minor: Interface name doesn't match component purpose.
MemberPaginationPropssuggests member-specific usage, butCustomPaginationis generic. Consider renaming toCustomPaginationPropsfor consistency.-interface MemberPaginationProps { +interface CustomPaginationProps { itemCount: number; pageSize: number; currentPage: number; className?: string; }apps/blade/src/app/admin/club/members/_components/members-table.tsx (1)
52-63: Consider adding loading and error states.The component renders immediately with empty data (
members ?? [],totalCount ?? 0) without indicating loading or errors. Per repo patterns, gating rendering until all required data succeeds provides a more coherent UX.♻️ Suggested pattern
const { data: members, isLoading: membersLoading, error: membersError } = api.member.getMembers.useQuery({...}); const { data: totalCount, isLoading: countLoading } = api.member.getMemberCount.useQuery({...}); const { data: duesPayingStatus, isLoading: duesLoading } = api.member.getDuesPayingMembers.useQuery(); const isLoading = membersLoading || countLoading || duesLoading; if (isLoading) return <LoadingSpinner />; if (membersError) return <ErrorMessage error={membersError} />;Based on learnings: "gating rendering should occur only when all required data fetches succeed...implement a unified loading/state or error handling."
apps/blade/src/app/admin/_components/CustomPaginationSelect.tsx (1)
28-30: "Members" label is hardcoded — limits reusability.If you plan to reuse this component elsewhere (e.g., for events or hackers), consider adding a
labelprop.interface CustomPaginationSelectProps { pageSize: number; onPageSizeChange: (pageSize: number) => void; options?: number[]; className?: string; + label?: string; } // In the component: - <SelectValue>{pageSize} Members</SelectValue> + <SelectValue>{pageSize} {label ?? "Members"}</SelectValue>
| <PaginationItem> | ||
| <PaginationPrevious | ||
| onClick={() => changePage(currentPage - 1)} | ||
| aria-disabled={currentPage <= 1} | ||
| /> | ||
| </PaginationItem> |
There was a problem hiding this comment.
aria-disabled doesn't prevent click events.
Users can still click "Previous" on page 1 or "Next" on the last page. The aria-disabled attribute only communicates state to assistive tech—it doesn't stop the handler.
🛡️ Proposed fix
<PaginationItem>
<PaginationPrevious
- onClick={() => changePage(currentPage - 1)}
+ onClick={() => currentPage > 1 && changePage(currentPage - 1)}
aria-disabled={currentPage <= 1}
+ className={currentPage <= 1 ? "pointer-events-none opacity-50" : ""}
/>
</PaginationItem> <PaginationItem>
<PaginationNext
- onClick={() => changePage(currentPage + 1)}
+ onClick={() => currentPage < pageCount && changePage(currentPage + 1)}
aria-disabled={currentPage >= pageCount}
+ className={currentPage >= pageCount ? "pointer-events-none opacity-50" : ""}
/>
</PaginationItem>Also applies to: 102-107
🤖 Prompt for AI Agents
In `@apps/blade/src/app/admin/_components/CustomPagination.tsx` around lines 81 -
86, The PaginationPrevious/PaginationNext controls still fire changePage when at
bounds because aria-disabled only informs AT; update the CustomPagination
component to also prevent activation: set the control's disabled prop (or
remove/replace the onClick) when currentPage <= 1 for PaginationPrevious and
when currentPage >= totalPages for PaginationNext, and keep aria-disabled in
sync; additionally guard the changePage invocation inside the onClick handlers
(no-op if out of range) so clicking or programmatic activation cannot change
pages when disabled.
| import { | ||
| Select, | ||
| SelectContent, | ||
| SelectGroup, | ||
| SelectItem, | ||
| SelectTrigger, | ||
| SelectValue, | ||
| } from "@forge/ui/select"; |
There was a problem hiding this comment.
Missing "use client" directive.
This component uses event handlers (onValueChange), so it needs the client directive for Next.js App Router.
+"use client";
+
import {
Select,
SelectContent,🤖 Prompt for AI Agents
In `@apps/blade/src/app/admin/_components/CustomPaginationSelect.tsx` around lines
1 - 8, Add the Next.js client directive to this component so event handlers like
onValueChange work on the client: insert the string "use client" as the very
first line of the CustomPaginationSelect component file (before any imports) so
the React component and handlers (e.g., Select, SelectTrigger, onValueChange
usage) run on the client rather than the server.
|
|
||
| const searchParams = useSearchParams(); | ||
| const router = useRouter(); | ||
| const currentPage = Number(searchParams.get("page") || 1); |
There was a problem hiding this comment.
Potential issue: currentPage from URL isn't validated.
If someone navigates to ?page=abc or ?page=-5, Number() returns NaN or a negative value. Consider clamping to valid range.
- const currentPage = Number(searchParams.get("page") || 1);
+ const currentPage = Math.max(1, Number(searchParams.get("page")) || 1);📝 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 currentPage = Number(searchParams.get("page") || 1); | |
| const currentPage = Math.max(1, Number(searchParams.get("page")) || 1); |
🤖 Prompt for AI Agents
In `@apps/blade/src/app/admin/club/members/_components/members-table.tsx` at line
49, The currentPage computed in members-table.tsx (const currentPage =
Number(searchParams.get("page") || 1)) isn't validated and can become NaN or
negative; update the logic that derives currentPage (used by the MembersTable
pagination) to parse the value (e.g., parseInt), treat non-numeric results as 1,
clamp the result to a minimum of 1, and if you have a known totalPages or
pageCount available in this component further clamp to that upper bound before
using it for rendering or navigation.
| // Add the sorting here | ||
| if (input.sortByTime) { | ||
| query.orderBy( | ||
| input.sortOrder === "desc" | ||
| ? desc(Member.timeCreated) | ||
| : asc(Member.dateCreated), | ||
| input.sortOrder === "desc" | ||
| ? desc(Member.timeCreated) | ||
| : asc(Member.timeCreated), | ||
| ) as typeof query; | ||
| } else if (input.sortField && input.sortOrder) { | ||
| const sortColumn = Member[input.sortField]; | ||
| query = query.orderBy( | ||
| input.sortOrder === "asc" ? asc(sortColumn) : desc(sortColumn), | ||
| ) as typeof query; | ||
| } else { | ||
| // Default sort by ID | ||
| query = query.orderBy(asc(Member.id)) as typeof query; | ||
| } | ||
|
|
There was a problem hiding this comment.
Critical: Time-based sorting is not applied to the query.
The query.orderBy(...) result on line 376 is not reassigned to query, so the sorting is silently discarded. Also, when sortOrder === "desc", the date/time logic appears inconsistent.
🐛 Proposed fix
// Add the sorting here
if (input.sortByTime) {
- query.orderBy(
+ query = query.orderBy(
input.sortOrder === "desc"
- ? desc(Member.timeCreated)
- : asc(Member.dateCreated),
+ ? desc(Member.dateCreated)
+ : asc(Member.dateCreated),
input.sortOrder === "desc"
? desc(Member.timeCreated)
: asc(Member.timeCreated),
) as typeof query;🤖 Prompt for AI Agents
In `@packages/api/src/routers/member.ts` around lines 374 - 393, The time-based
sorting branch currently calls query.orderBy(...) without reassigning the result
and mixes Member.dateCreated/timeCreated inconsistently; change the sort-by-time
branch to assign the result back (query = query.orderBy(...)) and use the
correct columns consistently (both clauses should reference Member.timeCreated,
using asc when input.sortOrder === "asc" and desc when "desc") so that sorting
by time is actually applied; keep the other branches (sortField and default)
unchanged.
| getMemberCount: publicProcedure | ||
| .input( | ||
| z | ||
| .object({ | ||
| searchTerm: z.string().optional(), | ||
| }) | ||
| .optional(), | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the getMemberCount implementation
cat packages/api/src/routers/member.ts | head -500 | tail -150Repository: KnightHacks/forge
Length of output: 4775
🏁 Script executed:
# Check how publicProcedure and permProcedure are defined
rg -n "publicProcedure|permProcedure" packages/api/src --type ts -B 2 -A 2 | head -80Repository: KnightHacks/forge
Length of output: 4480
🏁 Script executed:
# Check getMembers implementation for comparison
rg -n "getMembers:" packages/api/src/routers/member.ts -A 10Repository: KnightHacks/forge
Length of output: 423
🏁 Script executed:
# Look at other endpoints using publicProcedure to understand the pattern
rg -n "publicProcedure" packages/api/src/routers --type ts -A 5 | grep -E "(publicProcedure|^\s+\w+:|\.output)" | head -100Repository: KnightHacks/forge
Length of output: 2383
Restrict getMemberCount to authenticated users with appropriate permissions.
The endpoint currently allows unauthenticated access with searchable filters (email, name, company, Discord user, full names), enabling membership enumeration attacks. An attacker could systematically query whether specific individuals are members—defeating any privacy intention.
Contrast with getMembers (line 326), which correctly uses permProcedure with controlPerms.or(["READ_MEMBERS", "READ_CLUB_DATA"], ctx). The inconsistency suggests this public access is unintentional.
Recommendation: Change publicProcedure to permProcedure and add the same permission check:
- getMemberCount: publicProcedure
+ getMemberCount: permProcedure
.input(
z.object({
searchTerm: z.string().optional(),
}).optional(),
)
- .query(async ({ input }) => {
+ .query(async ({ input, ctx }) => {
+ controlPerms.or(["READ_MEMBERS", "READ_CLUB_DATA"], ctx);If public access is truly needed, remove the search filter capability entirely (e.g., return only total count without searchTerm filtering).
🤖 Prompt for AI Agents
In `@packages/api/src/routers/member.ts` around lines 400 - 407, The
getMemberCount endpoint is currently exposed as publicProcedure which allows
unauthenticated enumeration via the searchTerm filter; change its declaration
from publicProcedure to permProcedure and apply the same permission check used
by getMembers (controlPerms.or(["READ_MEMBERS", "READ_CLUB_DATA"], ctx)) so only
authorized users can call getMemberCount, and keep the existing searchTerm input
handling; alternatively, if public access is required, remove the searchTerm
filtering entirely from getMemberCount so it only returns an unfiltered total
count.
1 participant
Why
The site had NO pagination at all, which made it difficult to navigate all 70 something members listed. Additionally, the search bar functionality was limited. This PR aimed at fixing these issues.
What
Issue(s): #332
Test Plan
Images
Checklist
db:pushbefore mergingSummary by CodeRabbit
New Features
Chores