[#368] Club Data Dash improper values#372
Merged
DVidal1205 merged 5 commits intomainfrom Feb 17, 2026
Merged
Conversation
Removed Confetti Component references from raffle-draw since it was removed in the "Component Refactor" PR Ran Format, Lint, Typecheck
jesusthecreator017
added
Bug
|
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:
📝 WalkthroughWalkthroughRemoves confetti from the raffle UI, drops an ESLint-disable comment on a form responses page, introduces a union-style input and fast-path to return all members in Changes
Sequence Diagram(s)sequenceDiagram
participant Client as MemberDemographics (client)
participant API as getMembers (server/router)
participant DB as Database
Client->>API: getMembers({ fetchAll: true }) or getMembers({...})
alt fetchAll true OR no filters/pagination/search provided
API->>DB: findMany({ orderBy: { id: asc } })
DB-->>API: all members
API-->>Client: return all members
else paginated/filtered request
API->>DB: build WHERE, ORDER BY, OFFSET, LIMIT
DB-->>API: paginated/filtered members
API-->>Client: return members + paging metadata
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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: 1
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@apps/blade/src/app/_components/admin/banquet-raffle/raffle-draw.tsx`:
- Around line 1-7: This file has broad ESLint/TS disables; clean up the specific
unsafe usages by replacing the (window as any).webkitAudioContext access with a
proper type guard or narrowing (e.g., check for 'webkitAudioContext' in window
and cast to specific AudioContext type) and remove the use of 'any', and replace
the non-null assertions a[j]! (seen where arrays are indexed around the raffle
draw logic) with conditional checks or optional chaining and early returns to
handle undefined values safely; do this inside the raffle-draw component
functions that reference window.webkitAudioContext and the array indexed
variables so you can gradually remove the no-explicit-any and
no-non-null-assertion disables.
In `@packages/api/src/routers/member.ts`:
- Around line 356-364: The early-return in the members query uses only
pagination/filter checks and ignores sort params (input?.sortField,
input?.sortOrder), causing db.query.Member.findMany() to return unsorted results
when only sorting is requested; update the condition to also check for undefined
sortField and sortOrder so the early-return only triggers when no sort is
present, or alternately, handle sorting in that branch by passing an orderBy to
db.query.Member.findMany({ orderBy: { [input.sortField]: input.sortOrder } })
when input.sortField/input.sortOrder are provided; adjust the logic around the
existing input checks and the db.query.Member.findMany() call accordingly.
🧹 Nitpick comments (1)
🤖 Fix all nitpicks with AI agents
Verify each finding against the current code and only fix it if needed. In `@apps/blade/src/app/_components/admin/banquet-raffle/raffle-draw.tsx`: - Around line 1-7: This file has broad ESLint/TS disables; clean up the specific unsafe usages by replacing the (window as any).webkitAudioContext access with a proper type guard or narrowing (e.g., check for 'webkitAudioContext' in window and cast to specific AudioContext type) and remove the use of 'any', and replace the non-null assertions a[j]! (seen where arrays are indexed around the raffle draw logic) with conditional checks or optional chaining and early returns to handle undefined values safely; do this inside the raffle-draw component functions that reference window.webkitAudioContext and the array indexed variables so you can gradually remove the no-explicit-any and no-non-null-assertion disables.apps/blade/src/app/_components/admin/banquet-raffle/raffle-draw.tsx (1)
1-7: Pre-existing ESLint disables remain—consider cleaning up in a follow-up.The file disables several TypeScript rules including
no-explicit-anyandno-non-null-assertion. While these predate this PR, they conflict with the coding guidelines. Consider addressing them incrementally:
- Line 48:
(window as any).webkitAudioContextcould use a type guard or a more specific type- Lines 140, 167: Non-null assertions (
a[j]!) could use conditional checksNot blocking, but worth tracking as tech debt.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/blade/src/app/_components/admin/banquet-raffle/raffle-draw.tsx` around lines 1 - 7, This file has broad ESLint/TS disables; clean up the specific unsafe usages by replacing the (window as any).webkitAudioContext access with a proper type guard or narrowing (e.g., check for 'webkitAudioContext' in window and cast to specific AudioContext type) and remove the use of 'any', and replace the non-null assertions a[j]! (seen where arrays are indexed around the raffle draw logic) with conditional checks or optional chaining and early returns to handle undefined values safely; do this inside the raffle-draw component functions that reference window.webkitAudioContext and the array indexed variables so you can gradually remove the no-explicit-any and no-non-null-assertion disables.
There was a problem hiding this comment.
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@packages/api/src/routers/member.ts`:
- Around line 356-366: The early-return branch returns members without the
pagination path's default ordering, causing inconsistent result order; update
the early-return (the call to Member.findMany()) to include the same default
ordering used in the paginated branch (the orderBy asc on Member.id) so both
paths return results in the same deterministic order.
🧹 Nitpick comments (1)
🤖 Fix all nitpicks with AI agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/routers/member.ts`: - Around line 356-366: The early-return branch returns members without the pagination path's default ordering, causing inconsistent result order; update the early-return (the call to Member.findMany()) to include the same default ordering used in the paginated branch (the orderBy asc on Member.id) so both paths return results in the same deterministic order.packages/api/src/routers/member.ts (1)
356-366: Early-return logic correctly addresses the pagination bug.This fix properly returns all members when no filters/pagination/sort options are provided. The condition now includes
sortFieldandsortByTime, addressing the prior review feedback.One minor inconsistency: the early-return path returns members in database-default order, while the paginated path (line 430) defaults to
orderBy(asc(Member.id)). Consider adding consistent default ordering for predictable results:🔧 Optional: add consistent default ordering
input?.sortByTime === undefined ) { - return db.query.Member.findMany(); + return db.query.Member.findMany({ + orderBy: (members, { asc }) => asc(members.id), + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/routers/member.ts` around lines 356 - 366, The early-return branch returns members without the pagination path's default ordering, causing inconsistent result order; update the early-return (the call to Member.findMany()) to include the same default ordering used in the paginated branch (the orderBy asc on Member.id) so both paths return results in the same deterministic order.
Added option to pass in Infinity for page size to get all members (for alex)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/blade/src/app/_components/admin/club/data/MemberDemographics.tsx`:
- Around line 16-18: The call to api.member.getMembers.useQuery uses pageSize:
Infinity which breaks JSON serialization; change the argument to the backend's
sentinel for "all" (use pageSize: -1) or the new boolean flag if the API exposed
one (e.g., { all: true }); update the call in MemberDemographics.tsx (the
api.member.getMembers.useQuery invocation) to use the sentinel/flag and adjust
any local types or usages of the query result if needed so the code compiles
with the updated API shape.
Ran format, lint, typecheck
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/api/src/routers/member.ts (1)
425-438: Default sort order whensortFieldis provided.
Right now,sortFieldwithoutsortOrderfalls back toidordering, which can surprise callers. Default to"asc"(or enforcesortOrderin the schema).♻️ Suggested update
// Sorting + const sortOrder = input.sortOrder ?? "asc"; if (input.sortByTime) { query = query.orderBy( - input.sortOrder === "desc" + sortOrder === "desc" ? desc(Member.dateCreated) : asc(Member.dateCreated), - input.sortOrder === "desc" + sortOrder === "desc" ? desc(Member.timeCreated) : asc(Member.timeCreated), ) as typeof query; - } else if (input.sortField && input.sortOrder) { + } else if (input.sortField) { const sortColumn = Member[input.sortField]; query = query.orderBy( - input.sortOrder === "asc" ? asc(sortColumn) : desc(sortColumn), + sortOrder === "asc" ? asc(sortColumn) : desc(sortColumn), ) as typeof query; } else { query = query.orderBy(asc(Member.id)) as typeof query; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/routers/member.ts` around lines 425 - 438, The current branch in member router uses input.sortField but if input.sortOrder is falsy it silently falls back to id ordering; update the ordering logic in the Member sorting block (the code that accesses input.sortField, input.sortOrder, Member, and calls query.orderBy) to default sortOrder to "asc" when it is not provided (or alternatively validate/enforce sortOrder in the input schema), and then call query.orderBy with asc or desc on the resolved sortColumn accordingly so callers receive a predictable ascending default.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/api/src/routers/member.ts`:
- Around line 362-376: The early-return guard that returns
db.query.Member.findMany(...) incorrectly treats requests that only include
sortOrder as "no inputs", bypassing pagination; update the conditional around
input?.fetchAll / the null-input check in routers/member.ts to also check
input?.sortOrder (or change the logic to require sortField and sortOrder as a
pair), so that a request with only sortOrder will not take the early-return path
and will fall through to the paginated query; ensure you update the same
conditional that currently checks input?.currentPage, input?.pageSize,
input?.searchTerm, input?.schoolFilter, input?.majorFilter, input?.sortField,
and input?.sortByTime so it includes input?.sortOrder (or enforces
sortField+sortOrder together) before calling Member.findMany.
---
Nitpick comments:
In `@packages/api/src/routers/member.ts`:
- Around line 425-438: The current branch in member router uses input.sortField
but if input.sortOrder is falsy it silently falls back to id ordering; update
the ordering logic in the Member sorting block (the code that accesses
input.sortField, input.sortOrder, Member, and calls query.orderBy) to default
sortOrder to "asc" when it is not provided (or alternatively validate/enforce
sortOrder in the input schema), and then call query.orderBy with asc or desc on
the resolved sortColumn accordingly so callers receive a predictable ascending
default.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
After my pagination PR was merged an error in the club stats occured, this is because the getMembers endpoint was forcing pagination on any reference (hence why it was only showing 10 members). This PR aims to solve this issue
What
Issue(s): #368
Since my member pagination PR, the getMembers endpoint forced pagination on any other file that used getMembers, now it doesnt force get members by just checking if there are no optional inputs and return all members.
Changes:
Test Plan
Visually see that the stats are back to normal based on the current data I have available (Refer to images)
Images
Checklist
db:pushbefore mergingSummary by CodeRabbit
Refactor
Chores