Open
Conversation
This commit: - Replaces the incremental migration with a fresh table creation migration to avoid constraint conflicts - Updates event service to use correct method names and improve whitelist checking logic - Adds validation improvements to whitelist DTOs - Fixes frontend whitelist component to handle null data and improve accessibility - Updates user service to separate user retrieval with/without social accounts - Refactors database query in removeEventAdmin to use query builder with INNER JOIN for pessimistic locking compatibility
📝 WalkthroughWalkthroughThis PR introduces event whitelist functionality to control access to private events. It adds a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 4
🧹 Nitpick comments (3)
api/src/event/dtos/whitelistDto.ts (1)
5-11: Consider adding@IsNotEmpty()to prevent empty username strings.The
@IsString()decorator allows empty strings. Adding@IsNotEmpty()ensures usernames are meaningful.Proposed fix
+import { ArrayNotEmpty, IsArray, IsEnum, IsNotEmpty, IsString, IsUUID, ValidateNested } from "class-validator"; -import { ArrayNotEmpty, IsArray, IsEnum, IsString, IsUUID, ValidateNested } from "class-validator"; import { Type } from "class-transformer"; import { WhitelistPlatform } from "../entities/event-whitelist.entity"; export class WhitelistEntryDto { `@IsString`() + `@IsNotEmpty`() username: string; `@IsEnum`(WhitelistPlatform) platform: WhitelistPlatform; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/event/dtos/whitelistDto.ts` around lines 5 - 11, Add a non-empty validation to the username field on WhitelistEntryDto: update the WhitelistEntryDto class so the username property is decorated with `@IsNotEmpty`() in addition to `@IsString`(), ensuring empty strings are rejected by validation; keep the existing `@IsEnum`(WhitelistPlatform) on platform unchanged.frontend/app/events/[id]/dashboard/components/WhitelistManagement.tsx (1)
77-119: Don't widen mutation errors toany.These three
onErrorhandlers throw away the type information coming fromuseMutationand make.messageunchecked again. Please verify this against the installed@tanstack/react-querytypings and keep the inferred type, or narrow before reading the message.As per coding guidelines, `**/*.{ts,tsx}`: Use strict TypeScript typing and avoid any type where possible, using interfaces/types for DTOs and props instead.Type-safe error handling
- onError: (e: any) => toast.error(e.message), + onError: (error) => + toast.error( + error instanceof Error ? error.message : "An unexpected error occurred", + ),Apply the same change to the other two mutations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/events/`[id]/dashboard/components/WhitelistManagement.tsx around lines 77 - 119, The onError handlers for addMutation, removeMutation, and bulkRemoveMutation currently accept e: any and lose type information; change each onError to accept e: unknown (or the specific error type from `@tanstack/react-query` if available) and narrow it before reading message — e.g., check if e is an instance of Error and use e.message, otherwise render a safe fallback like String(e) or a generic error string; update the onError signatures for useMutation calls in WhitelistManagement.tsx (the addMutation, removeMutation, and bulkRemoveMutation handlers) accordingly.frontend/app/actions/event.ts (1)
253-284: Parameterize the whitelist Axios calls.
handleErrorinfers its generic from the Axios promise, and these helpers currently pass untypedget/post/deletecalls, so the newServerActionResponse<WhitelistEntry[]>/ServerActionResponse<void>contracts collapse toany. Please verify the payloads against the backend routes and set the Axios generics here so contract drift is caught by TypeScript.As per coding guidelines, `**/*.{ts,tsx}`: Use strict TypeScript typing and avoid any type where possible, using interfaces/types for DTOs and props instead.Preserve the declared response types
export async function getEventWhitelist( eventId: string, ): Promise<ServerActionResponse<WhitelistEntry[]>> { - return await handleError(axiosInstance.get(`event/${eventId}/whitelist`)); + return await handleError( + axiosInstance.get<WhitelistEntry[]>(`event/${eventId}/whitelist`), + ); } export async function addToWhitelist( eventId: string, entries: { username: string; platform: "GITHUB" | "FORTYTWO" }[], ): Promise<ServerActionResponse<WhitelistEntry[]>> { return await handleError( - axiosInstance.post(`event/${eventId}/whitelist`, { entries }), + axiosInstance.post<WhitelistEntry[]>(`event/${eventId}/whitelist`, { entries }), ); } export async function removeFromWhitelist( eventId: string, whitelistId: string, ): Promise<ServerActionResponse<void>> { return await handleError( - axiosInstance.delete(`event/${eventId}/whitelist/${whitelistId}`), + axiosInstance.delete<void>(`event/${eventId}/whitelist/${whitelistId}`), ); } export async function bulkRemoveFromWhitelist( eventId: string, ids: string[], ): Promise<ServerActionResponse<void>> { return await handleError( - axiosInstance.post(`event/${eventId}/whitelist/bulk-delete`, { ids }), + axiosInstance.post<void>(`event/${eventId}/whitelist/bulk-delete`, { ids }), ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/actions/event.ts` around lines 253 - 284, The axios calls in getEventWhitelist, addToWhitelist, removeFromWhitelist, and bulkRemoveFromWhitelist are untyped so handleError can't infer the declared ServerActionResponse<...> results; update each axiosInstance call to include the correct response generics (e.g. axiosInstance.get<ServerActionResponse<WhitelistEntry[]>>(...) and axiosInstance.post<ServerActionResponse<WhitelistEntry[]>>(...) / post<ServerActionResponse<void>> / delete<ServerActionResponse<void>>), and also type the request payloads for the POSTs (ensure the entries and ids match the backend DTO types) so TypeScript will catch contract drift when compiling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/src/event/event.controller.ts`:
- Around line 176-194: When handling private events in the event join flow, gate
the whitelist check behind a presence check so an empty whitelist does not block
joins: call this.eventService.hasWhitelist(eventId) first and only call
this.eventService.isUserWhitelisted(...) (after fetching the user via
this.userService.getUserWithSocialAccounts(userId) and extracting
fortyTwoAccount) if hasWhitelist returns true; if hasWhitelist is false, allow
link-only access and do not throw the UnauthorizedException for an empty
whitelist.
In `@api/src/event/event.service.ts`:
- Around line 697-723: The current read-then-save flow around
existingEntries/newEntries races the DB unique constraint (existingSet check
then whitelistRepository.save) so concurrent requests can still cause a
duplicate-key error; fix by performing an insert that ignores conflicts or by
catching the duplicate-key error and treating it as idempotent. Replace the save
path for whitelistEntries (where newEntries are created) with a single bulk
insert that uses your ORM's conflict-ignore/upsert behavior (e.g.,
whitelistRepository.createQueryBuilder().insert().values(...).orIgnore() or
equivalent) so duplicates are handled at insert time, or wrap
whitelistRepository.save(...) in a try/catch that detects the DB
unique-constraint error (e.g., Postgres 23505) and returns as if no-op; refer to
whitelistRepository.save, whitelistRepository.create, newEntries,
uniqueInputEntries and existingSet to locate and update the code.
In `@frontend/app/events/`[id]/dashboard/components/WhitelistManagement.tsx:
- Around line 92-104: The onSuccess handlers for whitelist mutations (e.g.,
removeMutation and the bulk delete mutation referenced around lines 151-168)
only invalidate the query but do not update the local selectedIds state, leaving
deleted IDs in selectedIds and causing stale bulk UI and failed subsequent
deletes; after a successful mutation, update selectedIds by removing the deleted
id(s) (for removeMutation drop the single whitelistId; for bulk delete drop all
returned/removed ids or intersect selectedIds with the refreshed list), or
compute selectedIds = selectedIds.filter(id => refreshedList.includes(id))
before or after invalidation so the selection reflects the new list and the bulk
toolbar/count disappears appropriately.
In `@local-setup/local-values/api.yaml`:
- Line 30: The GitHub OAuth callback in auth.controller.ts references the old
env key OAUTH_SUCCESS_REDIRECT_URL which no longer exists; update the GitHub
callback handler (in api/src/auth/auth.controller.ts, the code that calls
getOrThrow for the redirect URL) to use the renamed
OAUTH_42_SUCCESS_REDIRECT_URL or alternatively add and use a clearer
OAUTH_GITHUB_SUCCESS_REDIRECT_URL; ensure the getOrThrow call and any variable
names in the GitHub callback method are updated to match the chosen env key so
the value resolves at runtime.
---
Nitpick comments:
In `@api/src/event/dtos/whitelistDto.ts`:
- Around line 5-11: Add a non-empty validation to the username field on
WhitelistEntryDto: update the WhitelistEntryDto class so the username property
is decorated with `@IsNotEmpty`() in addition to `@IsString`(), ensuring empty
strings are rejected by validation; keep the existing `@IsEnum`(WhitelistPlatform)
on platform unchanged.
In `@frontend/app/actions/event.ts`:
- Around line 253-284: The axios calls in getEventWhitelist, addToWhitelist,
removeFromWhitelist, and bulkRemoveFromWhitelist are untyped so handleError
can't infer the declared ServerActionResponse<...> results; update each
axiosInstance call to include the correct response generics (e.g.
axiosInstance.get<ServerActionResponse<WhitelistEntry[]>>(...) and
axiosInstance.post<ServerActionResponse<WhitelistEntry[]>>(...) /
post<ServerActionResponse<void>> / delete<ServerActionResponse<void>>), and also
type the request payloads for the POSTs (ensure the entries and ids match the
backend DTO types) so TypeScript will catch contract drift when compiling.
In `@frontend/app/events/`[id]/dashboard/components/WhitelistManagement.tsx:
- Around line 77-119: The onError handlers for addMutation, removeMutation, and
bulkRemoveMutation currently accept e: any and lose type information; change
each onError to accept e: unknown (or the specific error type from
`@tanstack/react-query` if available) and narrow it before reading message — e.g.,
check if e is an instance of Error and use e.message, otherwise render a safe
fallback like String(e) or a generic error string; update the onError signatures
for useMutation calls in WhitelistManagement.tsx (the addMutation,
removeMutation, and bulkRemoveMutation handlers) accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d17d4ed1-5fc5-476b-8acd-d34d0f62554c
⛔ Files ignored due to path filters (1)
frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
api/db/migrations/1775336799887-event-whitelist.tsapi/src/auth/auth.controller.tsapi/src/event/dtos/whitelistDto.tsapi/src/event/entities/event-whitelist.entity.tsapi/src/event/entities/event.entity.tsapi/src/event/event.controller.tsapi/src/event/event.module.tsapi/src/event/event.service.tsapi/src/user/user.service.tsfrontend/app/actions/event.tsfrontend/app/events/[id]/dashboard/components/WhitelistManagement.tsxfrontend/app/events/[id]/dashboard/dashboard.tsxfrontend/components/event-info-notice.tsxfrontend/components/ui/checkbox.tsxfrontend/package.jsonlocal-setup/local-values/api.yaml
Comment on lines
+697
to
+723
| const existingEntries = await this.whitelistRepository.find({ | ||
| where: { | ||
| event: { id: eventId }, | ||
| }, | ||
| }); | ||
|
|
||
| const existingSet = new Set( | ||
| existingEntries.map((e) => `${e.username}:${e.platform}`), | ||
| ); | ||
|
|
||
| const newEntries = uniqueInputEntries.filter( | ||
| (entry) => !existingSet.has(`${entry.username}:${entry.platform}`), | ||
| ); | ||
|
|
||
| if (newEntries.length === 0) { | ||
| return []; | ||
| } | ||
|
|
||
| const whitelistEntries = newEntries.map((entry) => | ||
| this.whitelistRepository.create({ | ||
| event, | ||
| username: entry.username, | ||
| platform: entry.platform, | ||
| }), | ||
| ); | ||
|
|
||
| return this.whitelistRepository.save(whitelistEntries); |
There was a problem hiding this comment.
The duplicate filter still races the unique constraint.
This path does a read of existing rows and then a separate save(). Two concurrent adds of the same username can both pass the existingSet filter, and one request will then fail on the unique key from api/db/migrations/1775336799887-event-whitelist.ts Line 7 instead of behaving like a no-op. Please handle conflicts at insert time, or catch the duplicate-key case and treat it as idempotent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/src/event/event.service.ts` around lines 697 - 723, The current
read-then-save flow around existingEntries/newEntries races the DB unique
constraint (existingSet check then whitelistRepository.save) so concurrent
requests can still cause a duplicate-key error; fix by performing an insert that
ignores conflicts or by catching the duplicate-key error and treating it as
idempotent. Replace the save path for whitelistEntries (where newEntries are
created) with a single bulk insert that uses your ORM's conflict-ignore/upsert
behavior (e.g.,
whitelistRepository.createQueryBuilder().insert().values(...).orIgnore() or
equivalent) so duplicates are handled at insert time, or wrap
whitelistRepository.save(...) in a try/catch that detects the DB
unique-constraint error (e.g., Postgres 23505) and returns as if no-op; refer to
whitelistRepository.save, whitelistRepository.create, newEntries,
uniqueInputEntries and existingSet to locate and update the code.
Comment on lines
+92
to
+104
| const removeMutation = useMutation({ | ||
| mutationFn: async (whitelistId: string) => { | ||
| const result = await removeFromWhitelist(eventId, whitelistId); | ||
| if (isActionError(result)) | ||
| throw new Error(result.error); | ||
| return result; | ||
| }, | ||
| onSuccess: () => { | ||
| toast.success("User removed from whitelist"); | ||
| queryClient.invalidateQueries({ queryKey: ["event", eventId, "whitelist"] }); | ||
| }, | ||
| onError: (e: any) => toast.error(e.message), | ||
| }); |
There was a problem hiding this comment.
Prune selectedIds after whitelist mutations.
removeMutation.onSuccess only invalidates the query. If the deleted row was selected, selectedIds still contains it after the refetch, so the bulk toolbar/count stay visible and the next bulk delete resends a dead id. Intersect the local selection with the refreshed list, or drop the removed id in onSuccess.
Also applies to: 151-168
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/app/events/`[id]/dashboard/components/WhitelistManagement.tsx around
lines 92 - 104, The onSuccess handlers for whitelist mutations (e.g.,
removeMutation and the bulk delete mutation referenced around lines 151-168)
only invalidate the query but do not update the local selectedIds state, leaving
deleted IDs in selectedIds and causing stale bulk UI and failed subsequent
deletes; after a successful mutation, update selectedIds by removing the deleted
id(s) (for removeMutation drop the single whitelistId; for bulk delete drop all
returned/removed ids or intersect selectedIds with the refreshed list), or
compute selectedIds = selectedIds.filter(id => refreshedList.includes(id))
before or after invalidation so the selection reflects the new list and the bulk
toolbar/count disappears appropriately.
| AUTH_COOKIE_DOMAIN: localhost | ||
| CALLBACK_GITHUB_URL: http://localhost:4000/auth/github/callback | ||
| OAUTH_SUCCESS_REDIRECT_URL: http://localhost:3000/auth/sso | ||
| OAUTH_42_SUCCESS_REDIRECT_URL: http://localhost:3000/auth/sso |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify which OAuth handlers reference which environment variables
rg -n "OAUTH.*SUCCESS_REDIRECT" --type tsRepository: 42core-team/website
Length of output: 210
Update the GitHub OAuth handler to use the renamed variable.
The GitHub callback handler in api/src/auth/auth.controller.ts (line 42) references OAUTH_SUCCESS_REDIRECT_URL, but the config file now only defines OAUTH_42_SUCCESS_REDIRECT_URL. GitHub login will fail at runtime when getOrThrow cannot find the variable. Update line 42 to use OAUTH_42_SUCCESS_REDIRECT_URL or create a separate OAUTH_GITHUB_SUCCESS_REDIRECT_URL variable for clarity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@local-setup/local-values/api.yaml` at line 30, The GitHub OAuth callback in
auth.controller.ts references the old env key OAUTH_SUCCESS_REDIRECT_URL which
no longer exists; update the GitHub callback handler (in
api/src/auth/auth.controller.ts, the code that calls getOrThrow for the redirect
URL) to use the renamed OAUTH_42_SUCCESS_REDIRECT_URL or alternatively add and
use a clearer OAUTH_GITHUB_SUCCESS_REDIRECT_URL; ensure the getOrThrow call and
any variable names in the GitHub callback method are updated to match the chosen
env key so the value resolves at runtime.
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
1 participant
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.
Summary by CodeRabbit
Release Notes
New Features
Improvements