feat: added whitelist so that only specific people can join event#607
Open
PaulicStudios wants to merge 1 commit intodevfrom
Open
feat: added whitelist so that only specific people can join event#607PaulicStudios wants to merge 1 commit intodevfrom
PaulicStudios wants to merge 1 commit intodevfrom
Conversation
📝 WalkthroughWalkthroughIntroduces event whitelist functionality enabling private event access control. Adds database migration, TypeORM entity, service methods, and REST API endpoints for whitelist management. Updates private event join logic to enforce whitelist checks. Implements frontend server actions and WhitelistManagement component with add/remove/bulk-delete UI. Adds Checkbox component dependency. Changes
Sequence DiagramssequenceDiagram
participant User as User (Browser)
participant Frontend as Frontend App
participant API as Event API
participant Service as EventService
participant DB as Database
User->>Frontend: Click "Join Event"
Frontend->>API: PUT /event/{id}/join
API->>Service: isEventPrivate(eventId)
Service->>DB: Query EventEntity
DB-->>Service: Event data
alt Private Event
API->>Service: getUserById(userId)
Service->>DB: Query User + socialAccounts
DB-->>Service: User with GitHub/42 usernames
API->>Service: isUserWhitelistedForEvent(eventId, usernames)
Service->>DB: Query EventWhitelistEntity
alt User Whitelisted
DB-->>Service: Whitelist entry exists
Service-->>API: true
API->>Service: registerUserForEvent(eventId, userId)
Service->>DB: Create UserEventRegistration
DB-->>Service: Success
API-->>Frontend: 200 OK
else User Not Whitelisted
DB-->>Service: No whitelist entry
Service-->>API: false
API-->>Frontend: 403 Unauthorized
end
else Public Event
API->>Service: registerUserForEvent(eventId, userId)
Service->>DB: Create UserEventRegistration
DB-->>Service: Success
API-->>Frontend: 200 OK
end
Frontend-->>User: Success/Error notification
sequenceDiagram
participant Admin as Event Admin
participant Frontend as Dashboard UI
participant API as Event API
participant Service as EventService
participant DB as Database
Admin->>Frontend: View Whitelist Tab
Frontend->>API: GET /event/{id}/whitelist
API->>Service: Check isEventAdmin(eventId, userId)
Service->>DB: Query permissions
DB-->>Service: Admin confirmed
Service->>API: getWhitelist(eventId)
Service->>DB: Query EventWhitelistEntity
DB-->>Service: Whitelist entries
API-->>Frontend: WhitelistEntry[]
Frontend-->>Admin: Display whitelist table
Admin->>Frontend: Add users (textarea + platform select)
Frontend->>API: POST /event/{id}/whitelist
Note over API: { entries: [{username, platform}, ...] }
API->>Service: Check isEventAdmin
Service->>DB: Verify admin
DB-->>Service: Confirmed
Service->>API: addToWhitelist(eventId, entries)
Service->>DB: INSERT new EventWhitelistEntity rows
DB-->>Service: Created entries
API-->>Frontend: WhitelistEntry[]
Frontend-->>Admin: Success toast + refresh table
Admin->>Frontend: Click delete on entry
Frontend->>API: DELETE /event/{id}/whitelist/{whitelistId}
API->>Service: Check isEventAdmin
Service->>DB: Verify admin
DB-->>Service: Confirmed
Service->>API: removeFromWhitelist(eventId, whitelistId)
Service->>DB: DELETE EventWhitelistEntity by id
DB-->>Service: Success
API-->>Frontend: 204 No Content
Frontend-->>Admin: Success toast + remove row
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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: 7
🧹 Nitpick comments (2)
api/src/event/entities/event.entity.ts (1)
126-129: Remove redundantonDeleteconfiguration from@OneToManyrelation.The owning
@ManyToOneside inapi/src/event/entities/event-whitelist.entity.tsalready correctly includesonDelete: "CASCADE". Having it on the inverse@OneToManyside is redundant since TypeORM only applies cascade delete rules at the FK-owning side. You can safely removeonDelete: "CASCADE"from the@OneToManydecorator here (lines 127-128).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/src/event/entities/event.entity.ts` around lines 126 - 129, The `@OneToMany` decorator on the Event entity currently includes a redundant onDelete: "CASCADE" option; remove the onDelete configuration from the `@OneToMany`(() => EventWhitelistEntity, (whitelist) => whitelist.event, ...) declaration (the whitelists property) so cascade delete is only defined on the owning `@ManyToOne` side in EventWhitelistEntity.frontend/app/events/[id]/dashboard/components/WhitelistManagement.tsx (1)
74-74: Prefer a typed error here.
anysidesteps the mutation error contract and makese.messageunchecked.unknown+ narrowing, or a concrete error type, keeps the toast path type-safe without changing behavior.As per coding guidelines, "Use strict TypeScript typing and avoid any type where possible, using interfaces/types for DTOs and props instead."
Also applies to: 88-88, 103-103
🤖 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 at line 74, Replace the loose any error typing in the onError callbacks in WhitelistManagement (the mutation config callbacks around the onError handlers) with unknown and narrow before reading .message; e.g., change (e: any) => toast.error(e.message) to (e: unknown) => { if (e instanceof Error) toast.error(e.message); else toast.error(String(e)); } or extract a small helper like handleMutationError(error: unknown) used by the onError handlers to perform the instanceof Error check and provide a safe fallback.
🤖 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/db/migrations/1775081823814-event-whitelist.ts`:
- Around line 9-10: The migration is making eventId nullable which allows
duplicate (NULL, username, platform) rows; revert that by removing or undoing
the ALTER TABLE ... DROP NOT NULL so "eventId" remains NOT NULL and keep the
UNIQUE constraint addition, and update the entity decorator in
api/src/event/entities/event-whitelist.entity.ts by setting the `@ManyToOne`
relation on the EventWhitelist entity (the event property) to nullable: false so
TypeORM will not regenerate the column as nullable in future migrations.
In `@api/src/event/dtos/whitelistDto.ts`:
- Around line 13-24: Add `@ArrayNotEmpty`() to AddToWhitelistDto.entries and to
BulkDeleteWhitelistDto.ids to reject empty arrays, and tighten
BulkDeleteWhitelistDto.ids validation by replacing `@IsString`({ each: true })
with `@IsUUID`('4', { each: true }) so each id is validated as a UUID; update
imports to include ArrayNotEmpty and IsUUID (and keep Type/ValidateNested for
AddToWhitelistDto.entries) so validation runs at the DTO boundary.
In `@api/src/event/event.service.ts`:
- Around line 758-780: The current logic builds a `conditions` array but then
reduces it to only the first entry, so the `fortyTwoUsername` is never checked;
update the check used in `whitelistRepository.existsBy` (the block that builds
the query using `conditions.reduce`) to test any of the conditions instead of
only the first. Replace the reduce-based single-pair merge with logic that
queries for event id plus an OR/any match over `conditions` (or call `existsBy`
for each condition and short-circuit), ensuring both `githubUsername` and
`fortyTwoUsername` (if present) are honored; adjust the query construction where
`conditions`, `WhitelistPlatform`, `githubUsername`, `fortyTwoUsername`, and
`whitelistRepository.existsBy` are referenced.
- Around line 793-809: The current query built by
whitelistRepository.createQueryBuilder("whitelist") applies the eventId only to
the first branch, allowing a FORTYTWO whitelist from another event to match;
update the logic in event.service.ts (the queryBuilder creation and conditions
around createQueryBuilder/getExists) so the eventId predicate scopes both
platform checks—either by combining into a single where that wraps the ORed
platform clauses under the same "whitelist.eventId = :eventId" condition or by
adding a grouped AND around the github/fortyTwo .orWhere clause—so both github
and FORTYTWO matches must also satisfy whitelist.eventId = :eventId.
In `@api/src/user/user.service.ts`:
- Around line 114-117: getUserById currently eager-loads socialAccounts via
userRepository.findOneOrFail({ relations: ["socialAccounts"] }); remove the
relations so getUserById returns only the user by id (minimal join), then add a
new method (e.g., getUserWithSocialAccounts or getUserForWhitelist) that calls
userRepository.findOneOrFail with relations: ["socialAccounts"] for the
whitelist/auth flows that need it; update any callers that require
socialAccounts (such as the auth "/me" flow) to use the new method while leaving
other callers using getUserById unchanged.
In `@frontend/app/events/`[id]/dashboard/components/WhitelistManagement.tsx:
- Around line 52-60: The query currently defaults data to [] so failures are
rendered as an empty whitelist; update the useQuery call in
WhitelistManagement.tsx (the useQuery invocation that calls getEventWhitelist
and uses isActionError) to stop defaulting data and instead check useQuery's
isError and error before rendering the empty-state; return undefined/null for
data on error, surface a loading/error UI when isLoading or isError, and only
render the "No users whitelisted" copy when data is a non-error empty array.
Apply the same fix to the other useQuery instance around the second whitelist
block (the one referenced at lines 182-186).
- Around line 171-174: The select-all Checkbox and each row's controls lack
accessible names—update the Checkbox component used for the header (checked prop
using selectedIds/whitelist and onCheckedChange={toggleSelectAll}) to include
aria-label="Select all whitelisted users", add aria-label={`Select
${entry.username}`} to the per-row Checkbox (the one bound to entry and
selectedIds), and add aria-label={`Remove ${entry.username} from whitelist`} to
the icon-only delete button (the remove/delete handler for the whitelist entry)
so screen readers get contextual labels.
---
Nitpick comments:
In `@api/src/event/entities/event.entity.ts`:
- Around line 126-129: The `@OneToMany` decorator on the Event entity currently
includes a redundant onDelete: "CASCADE" option; remove the onDelete
configuration from the `@OneToMany`(() => EventWhitelistEntity, (whitelist) =>
whitelist.event, ...) declaration (the whitelists property) so cascade delete is
only defined on the owning `@ManyToOne` side in EventWhitelistEntity.
In `@frontend/app/events/`[id]/dashboard/components/WhitelistManagement.tsx:
- Line 74: Replace the loose any error typing in the onError callbacks in
WhitelistManagement (the mutation config callbacks around the onError handlers)
with unknown and narrow before reading .message; e.g., change (e: any) =>
toast.error(e.message) to (e: unknown) => { if (e instanceof Error)
toast.error(e.message); else toast.error(String(e)); } or extract a small helper
like handleMutationError(error: unknown) used by the onError handlers to perform
the instanceof Error check and provide a safe fallback.
🪄 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: 1231511d-0295-4d2d-b158-fbed522003ea
⛔ Files ignored due to path filters (1)
frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
api/db/migrations/1775081823814-event-whitelist.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.json
Comment on lines
+9
to
+10
| await queryRunner.query(`ALTER TABLE "event_whitelists" ALTER COLUMN "eventId" DROP NOT NULL`); | ||
| await queryRunner.query(`ALTER TABLE "event_whitelists" ADD CONSTRAINT "UQ_1e66e3e9750244ea3f6563a6d49" UNIQUE ("eventId", "username", "platform")`); |
There was a problem hiding this comment.
Keep eventId non-nullable.
A whitelist row without an event is invalid, and in PostgreSQL this new unique key will still allow duplicate (NULL, username, platform) rows. Please keep eventId as NOT NULL here and mark the @ManyToOne in api/src/event/entities/event-whitelist.entity.ts as nullable: false so TypeORM does not regenerate this relaxation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/db/migrations/1775081823814-event-whitelist.ts` around lines 9 - 10, The
migration is making eventId nullable which allows duplicate (NULL, username,
platform) rows; revert that by removing or undoing the ALTER TABLE ... DROP NOT
NULL so "eventId" remains NOT NULL and keep the UNIQUE constraint addition, and
update the entity decorator in api/src/event/entities/event-whitelist.entity.ts
by setting the `@ManyToOne` relation on the EventWhitelist entity (the event
property) to nullable: false so TypeORM will not regenerate the column as
nullable in future migrations.
Comment on lines
+13
to
+24
| export class AddToWhitelistDto { | ||
| @IsArray() | ||
| @ValidateNested({ each: true }) | ||
| @Type(() => WhitelistEntryDto) | ||
| entries: WhitelistEntryDto[]; | ||
| } | ||
|
|
||
| export class BulkDeleteWhitelistDto { | ||
| @IsArray() | ||
| @IsString({ each: true }) | ||
| ids: string[]; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify whitelist id type
fd -i 'event-whitelist.entity.ts' | xargs -r rg -n -C2 'PrimaryGeneratedColumn|id'
# Verify whether service/controller already short-circuit empty arrays
fd -i 'event.service.ts' | xargs -r rg -n -C2 'whitelist|entries|ids|bulk'
fd -i 'event.controller.ts' | xargs -r rg -n -C2 'whitelist|bulk-delete'Repository: 42core-team/website
Length of output: 5360
🏁 Script executed:
cat -n api/src/event/dtos/whitelistDto.tsRepository: 42core-team/website
Length of output: 799
Add @ArrayNotEmpty() and validate IDs as UUIDs in whitelist DTOs.
Both entries and ids currently accept empty arrays, which cause no-op requests. Additionally, ids are only validated as strings but are actually UUID-backed in the entity. Tighten validation at the DTO boundary.
Suggested DTO update
-import { IsArray, IsEnum, IsString, ValidateNested } from "class-validator";
+import {
+ ArrayNotEmpty,
+ IsArray,
+ IsEnum,
+ IsString,
+ IsUUID,
+ ValidateNested,
+} from "class-validator";
export class AddToWhitelistDto {
`@IsArray`()
+ `@ArrayNotEmpty`()
`@ValidateNested`({ each: true })
`@Type`(() => WhitelistEntryDto)
entries: WhitelistEntryDto[];
}
export class BulkDeleteWhitelistDto {
`@IsArray`()
- `@IsString`({ each: true })
+ `@ArrayNotEmpty`()
+ `@IsUUID`("4", { each: true })
ids: string[];
}🤖 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 13 - 24, Add
`@ArrayNotEmpty`() to AddToWhitelistDto.entries and to BulkDeleteWhitelistDto.ids
to reject empty arrays, and tighten BulkDeleteWhitelistDto.ids validation by
replacing `@IsString`({ each: true }) with `@IsUUID`('4', { each: true }) so each id
is validated as a UUID; update imports to include ArrayNotEmpty and IsUUID (and
keep Type/ValidateNested for AddToWhitelistDto.entries) so validation runs at
the DTO boundary.
Comment on lines
+758
to
+780
| const conditions: { username: string; platform: WhitelistPlatform }[] = [ | ||
| { username: githubUsername.toLowerCase(), platform: WhitelistPlatform.GITHUB }, | ||
| ]; | ||
|
|
||
| if (fortyTwoUsername) { | ||
| conditions.push({ | ||
| username: fortyTwoUsername.toLowerCase(), | ||
| platform: WhitelistPlatform.FORTYTWO, | ||
| }); | ||
| } | ||
|
|
||
| return this.whitelistRepository.existsBy({ | ||
| event: { id: eventId }, | ||
| ...conditions.reduce( | ||
| (acc, cond, idx) => { | ||
| if (idx === 0) { | ||
| return { username: cond.username, platform: cond.platform }; | ||
| } | ||
| return acc; | ||
| }, | ||
| {} as { username?: string; platform?: WhitelistPlatform }, | ||
| ), | ||
| }); |
There was a problem hiding this comment.
This helper never checks the 42 username.
The reduce keeps only the first condition, so once a FORTYTWO username is present the query still uses only the GitHub pair. A user who is whitelisted via 42 but not GitHub will be reported as not whitelisted.
🤖 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 758 - 780, The current logic
builds a `conditions` array but then reduces it to only the first entry, so the
`fortyTwoUsername` is never checked; update the check used in
`whitelistRepository.existsBy` (the block that builds the query using
`conditions.reduce`) to test any of the conditions instead of only the first.
Replace the reduce-based single-pair merge with logic that queries for event id
plus an OR/any match over `conditions` (or call `existsBy` for each condition
and short-circuit), ensuring both `githubUsername` and `fortyTwoUsername` (if
present) are honored; adjust the query construction where `conditions`,
`WhitelistPlatform`, `githubUsername`, `fortyTwoUsername`, and
`whitelistRepository.existsBy` are referenced.
Comment on lines
+793
to
+809
| const queryBuilder = this.whitelistRepository | ||
| .createQueryBuilder("whitelist") | ||
| .where("whitelist.eventId = :eventId", { eventId }) | ||
| .andWhere( | ||
| "(whitelist.username = :githubUsername AND whitelist.platform = :githubPlatform)", | ||
| { githubUsername: githubUsername.toLowerCase(), githubPlatform: WhitelistPlatform.GITHUB }, | ||
| ); | ||
|
|
||
| if (fortyTwoUsername) { | ||
| queryBuilder.orWhere( | ||
| "(whitelist.username = :fortyTwoUsername AND whitelist.platform = :fortyTwoPlatform)", | ||
| { | ||
| fortyTwoUsername: fortyTwoUsername.toLowerCase(), | ||
| fortyTwoPlatform: WhitelistPlatform.FORTYTWO, | ||
| }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Scope both whitelist branches to the current event.
This predicate is effectively (eventId AND githubMatch) OR fortyTwoMatch, so a 42 whitelist entry from any other event can satisfy the check and let the user join this private event. Group the GitHub/42 clauses under the same eventId filter before calling getExists().
🐛 Proposed fix
-import {
- DataSource,
- In,
- IsNull,
- LessThanOrEqual,
- MoreThanOrEqual,
- Repository,
- UpdateResult,
-} from "typeorm";
+import {
+ Brackets,
+ DataSource,
+ In,
+ IsNull,
+ LessThanOrEqual,
+ MoreThanOrEqual,
+ Repository,
+ UpdateResult,
+} from "typeorm"; const queryBuilder = this.whitelistRepository
.createQueryBuilder("whitelist")
.where("whitelist.eventId = :eventId", { eventId })
- .andWhere(
- "(whitelist.username = :githubUsername AND whitelist.platform = :githubPlatform)",
- { githubUsername: githubUsername.toLowerCase(), githubPlatform: WhitelistPlatform.GITHUB },
- );
-
- if (fortyTwoUsername) {
- queryBuilder.orWhere(
- "(whitelist.username = :fortyTwoUsername AND whitelist.platform = :fortyTwoPlatform)",
- {
- fortyTwoUsername: fortyTwoUsername.toLowerCase(),
- fortyTwoPlatform: WhitelistPlatform.FORTYTWO,
- },
- );
- }
+ .andWhere(
+ new Brackets((qb) => {
+ qb.where(
+ "whitelist.username = :githubUsername AND whitelist.platform = :githubPlatform",
+ {
+ githubUsername: githubUsername.toLowerCase(),
+ githubPlatform: WhitelistPlatform.GITHUB,
+ },
+ );
+
+ if (fortyTwoUsername) {
+ qb.orWhere(
+ "whitelist.username = :fortyTwoUsername AND whitelist.platform = :fortyTwoPlatform",
+ {
+ fortyTwoUsername: fortyTwoUsername.toLowerCase(),
+ fortyTwoPlatform: WhitelistPlatform.FORTYTWO,
+ },
+ );
+ }
+ }),
+ );📝 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.
Suggested change
| const queryBuilder = this.whitelistRepository | |
| .createQueryBuilder("whitelist") | |
| .where("whitelist.eventId = :eventId", { eventId }) | |
| .andWhere( | |
| "(whitelist.username = :githubUsername AND whitelist.platform = :githubPlatform)", | |
| { githubUsername: githubUsername.toLowerCase(), githubPlatform: WhitelistPlatform.GITHUB }, | |
| ); | |
| if (fortyTwoUsername) { | |
| queryBuilder.orWhere( | |
| "(whitelist.username = :fortyTwoUsername AND whitelist.platform = :fortyTwoPlatform)", | |
| { | |
| fortyTwoUsername: fortyTwoUsername.toLowerCase(), | |
| fortyTwoPlatform: WhitelistPlatform.FORTYTWO, | |
| }, | |
| ); | |
| } | |
| import { | |
| Brackets, | |
| DataSource, | |
| In, | |
| IsNull, | |
| LessThanOrEqual, | |
| MoreThanOrEqual, | |
| Repository, | |
| UpdateResult, | |
| } from "typeorm"; | |
| // ... other code ... | |
| const queryBuilder = this.whitelistRepository | |
| .createQueryBuilder("whitelist") | |
| .where("whitelist.eventId = :eventId", { eventId }) | |
| .andWhere( | |
| new Brackets((qb) => { | |
| qb.where( | |
| "whitelist.username = :githubUsername AND whitelist.platform = :githubPlatform", | |
| { | |
| githubUsername: githubUsername.toLowerCase(), | |
| githubPlatform: WhitelistPlatform.GITHUB, | |
| }, | |
| ); | |
| if (fortyTwoUsername) { | |
| qb.orWhere( | |
| "whitelist.username = :fortyTwoUsername AND whitelist.platform = :fortyTwoPlatform", | |
| { | |
| fortyTwoUsername: fortyTwoUsername.toLowerCase(), | |
| fortyTwoPlatform: WhitelistPlatform.FORTYTWO, | |
| }, | |
| ); | |
| } | |
| }), | |
| ); |
🤖 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 793 - 809, The current query
built by whitelistRepository.createQueryBuilder("whitelist") applies the eventId
only to the first branch, allowing a FORTYTWO whitelist from another event to
match; update the logic in event.service.ts (the queryBuilder creation and
conditions around createQueryBuilder/getExists) so the eventId predicate scopes
both platform checks—either by combining into a single where that wraps the ORed
platform clauses under the same "whitelist.eventId = :eventId" condition or by
adding a grouped AND around the github/fortyTwo .orWhere clause—so both github
and FORTYTWO matches must also satisfy whitelist.eventId = :eventId.
Comment on lines
+114
to
117
| return this.userRepository.findOneOrFail({ | ||
| where: { id: userId }, | ||
| relations: ["socialAccounts"], | ||
| }); |
There was a problem hiding this comment.
Avoid eager-loading socialAccounts in the generic getUserById path.
This method is reused in auth/team flows that don’t need socialAccounts, so this adds unnecessary joins and can leak extra relation data via /me (api/src/auth/auth.controller.ts Lines 133-139). Please keep getUserById minimal and add a dedicated method for whitelist use-cases.
Suggested refactor
- getUserById(userId: string) {
- return this.userRepository.findOneOrFail({
- where: { id: userId },
- relations: ["socialAccounts"],
- });
- }
+ getUserById(userId: string) {
+ return this.userRepository.findOneOrFail({
+ where: { id: userId },
+ });
+ }
+
+ getUserByIdWithSocialAccounts(userId: string) {
+ return this.userRepository.findOneOrFail({
+ where: { id: userId },
+ relations: ["socialAccounts"],
+ });
+ }📝 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.
Suggested change
| return this.userRepository.findOneOrFail({ | |
| where: { id: userId }, | |
| relations: ["socialAccounts"], | |
| }); | |
| getUserById(userId: string) { | |
| return this.userRepository.findOneOrFail({ | |
| where: { id: userId }, | |
| }); | |
| } | |
| getUserByIdWithSocialAccounts(userId: string) { | |
| return this.userRepository.findOneOrFail({ | |
| where: { id: userId }, | |
| relations: ["socialAccounts"], | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/src/user/user.service.ts` around lines 114 - 117, getUserById currently
eager-loads socialAccounts via userRepository.findOneOrFail({ relations:
["socialAccounts"] }); remove the relations so getUserById returns only the user
by id (minimal join), then add a new method (e.g., getUserWithSocialAccounts or
getUserForWhitelist) that calls userRepository.findOneOrFail with relations:
["socialAccounts"] for the whitelist/auth flows that need it; update any callers
that require socialAccounts (such as the auth "/me" flow) to use the new method
while leaving other callers using getUserById unchanged.
Comment on lines
+52
to
+60
| const { data: whitelist = [], isLoading } = useQuery({ | ||
| queryKey: ["event", eventId, "whitelist"], | ||
| queryFn: async () => { | ||
| const result = await getEventWhitelist(eventId); | ||
| if (isActionError(result)) | ||
| throw new Error(result.error); | ||
| return result; | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Don’t render load failures as an empty whitelist.
Defaulting data to [] means any query error falls through to the “No users whitelisted” copy, which is misleading on an admin screen that controls private-event access. Handle isError/error explicitly before showing the empty-state message.
Also applies to: 182-186
🤖 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 52 - 60, The query currently defaults data to [] so failures are rendered
as an empty whitelist; update the useQuery call in WhitelistManagement.tsx (the
useQuery invocation that calls getEventWhitelist and uses isActionError) to stop
defaulting data and instead check useQuery's isError and error before rendering
the empty-state; return undefined/null for data on error, surface a
loading/error UI when isLoading or isError, and only render the "No users
whitelisted" copy when data is a non-error empty array. Apply the same fix to
the other useQuery instance around the second whitelist block (the one
referenced at lines 182-186).
Comment on lines
+171
to
+174
| <Checkbox | ||
| checked={whitelist.length > 0 && selectedIds.size === whitelist.length} | ||
| onCheckedChange={toggleSelectAll} | ||
| /> |
There was a problem hiding this comment.
Add accessible names to the row controls.
The select-all checkbox, row checkboxes, and the icon-only delete button are all unlabeled, so screen readers will announce generic controls with no context. Add aria-labels such as “Select all whitelisted users”, Select ${entry.username}, and Remove ${entry.username} from whitelist.
Also applies to: 194-197, 206-214
🤖 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 171 - 174, The select-all Checkbox and each row's controls lack accessible
names—update the Checkbox component used for the header (checked prop using
selectedIds/whitelist and onCheckedChange={toggleSelectAll}) to include
aria-label="Select all whitelisted users", add aria-label={`Select
${entry.username}`} to the per-row Checkbox (the one bound to entry and
selectedIds), and add aria-label={`Remove ${entry.username} from whitelist`} to
the icon-only delete button (the remove/delete handler for the whitelist entry)
so screen readers get contextual labels.
Peu77
reviewed
Apr 2, 2026
| public async up(queryRunner: QueryRunner): Promise<void> { | ||
| await queryRunner.query(`ALTER TABLE "event_whitelists" DROP CONSTRAINT "FK_event_whitelists_event"`); | ||
| await queryRunner.query(`DROP INDEX "public"."UQ_event_whitelists_event_username_platform"`); | ||
| await queryRunner.query(`ALTER TABLE "event_whitelists" ALTER COLUMN "eventId" DROP NOT NULL`); |
Member
There was a problem hiding this comment.
why did you make eventId nullable?
Member
There was a problem hiding this comment.
shouldn't event_whitelists always have an event?
Peu77
reviewed
Apr 2, 2026
| @PrimaryGeneratedColumn("uuid") | ||
| id: string; | ||
|
|
||
| @Column({ type: "enum", enum: WhitelistPlatform }) |
Member
There was a problem hiding this comment.
It's better when we just store a text type in the db instead of a enum
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.
Summary by CodeRabbit
New Features
UI Improvements