-
Notifications
You must be signed in to change notification settings - Fork 0
feat: polish applications #42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request refactors and polishes the application, focusing on improving code organization, type safety, error handling, and authentication/authorization patterns across both frontend and backend.
Changes:
- Centralized authentication/authorization logic using preHandler middleware instead of inline checks
- Improved error messages and type safety across registration hooks and components
- Refactored analytics data fetching to use parallel queries for better performance
- Enhanced test coverage with more comprehensive test cases for user, event, and registration routes
- Cleaned up redundant code and improved maintainability
Reviewed changes
Copilot reviewed 25 out of 28 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/web/src/routes/events/$eventId/analytics.tsx | Parallelized data loading with Promise.all; added loading state handling for actions |
| apps/web/src/lib/hooks/registrations/use-create-registration.tsx | Fixed error messages to accurately describe registration operations |
| apps/web/src/lib/hooks/registrations/use-batch-update-registrations.tsx | Improved error message to be more generic |
| apps/web/src/lib/data/registration.ts | Updated query keys and improved schema validation |
| apps/web/src/components/tables/registration-table/data-table.tsx | Added isActionsDisabled prop and proper type imports |
| apps/shared/src/registrations/types.ts | Cleaned up whitespace |
| apps/shared/src/registrations/schemas.ts | Removed unnecessary default value from optional field |
| apps/api/tests/setup.ts | Updated mock auth import path |
| apps/api/tests/mock-auth.ts | Moved mock auth utilities to tests directory |
| apps/api/src/types/globals.d.ts | Added FastifyRequest user interface and improved type definitions |
| apps/api/src/server.ts | Updated error handler import path |
| apps/api/src/modules/users/store.ts | Refactored to use shared RegistrationStoreSelection |
| apps/api/src/modules/users/service.ts | Removed nullable types for authenticated user parameters |
| apps/api/src/modules/users/route.ts | Migrated to preHandler authentication pattern |
| apps/api/src/modules/users/route.test.ts | Significantly expanded test coverage with more comprehensive scenarios |
| apps/api/src/modules/registration/store.ts | Refactored analytics into focused, parallelizable functions |
| apps/api/src/modules/registration/service.ts | Removed inline role checks (moved to middleware) |
| apps/api/src/modules/registration/schema.ts | Centralized store selection; improved type definitions |
| apps/api/src/modules/registration/route.ts | Migrated to preHandler authentication pattern |
| apps/api/src/modules/registration/route.test.ts | Updated tests for new auth behavior; removed obsolete endpoint tests |
| apps/api/src/modules/events/service.ts | Removed inline authorization checks (moved to middleware) |
| apps/api/src/modules/events/schema.ts | Extracted shared QueryFilterSchema |
| apps/api/src/modules/events/route.ts | Migrated to preHandler authentication for protected endpoints |
| apps/api/src/modules/events/route.test.ts | Comprehensive test improvements with better coverage |
| apps/api/src/modules/core/schema.ts | New shared schema for pagination |
| apps/api/src/lib/error-handler.ts | Moved error handler to dedicated file |
| apps/api/src/lib/auth-guard.ts | New centralized authentication middleware |
| apps/api/src/db/schema.ts | Added NOT NULL constraints to registration timestamps and status |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 34 out of 38 changed files in this pull request and generated 5 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }); | ||
|
|
||
| if (!event || (role !== "committee" && event.state === "draft")) { | ||
| if (event.state === "draft") { |
Copilot
AI
Jan 31, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eventStore.findById can return undefined, but this code dereferences event.state without a null check and only throws NotFoundError when state === "draft". If the event does not exist, this will cause a runtime error instead of returning a controlled 404-style response; consider restoring a guard for a missing event (e.g. if (!event || event.state === "draft")) before accessing its properties.
| if (event.state === "draft") { | |
| if (!event || event.state === "draft") { |
|
|
||
| const requireCommittee = async (request: FastifyRequest, reply: FastifyReply) => { | ||
| await requireAuth(request, reply); | ||
|
|
Copilot
AI
Jan 31, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
requireCommittee calls requireAuth but then unconditionally reads request.user.role; if requireAuth has already sent a 401 response (because userId or role was missing), request.user will be undefined and this line will throw at runtime. You should short-circuit when authentication fails (for example by checking reply.sent or having requireAuth throw) so that the role check is only executed when request.user has been populated.
| if (reply.sent || !request.user) { | |
| return; | |
| } |
| role: null, | ||
| requesterId: null, |
Copilot
AI
Jan 31, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the user.updated branch you call userService.updateUser with role and requesterId set to null, but updateUser enforces that either the requester is a committee member or is updating their own record. This means webhook-driven profile updates will always fail with UnauthorizedError; consider treating this webhook as a trusted system actor (e.g. pass a committee role or add a bypass path) so that Clerk updates can successfully sync users.
| role: null, | |
| requesterId: null, | |
| role: "committee", | |
| requesterId: data.id, |
|
|
||
| export const RegistrationContractSchema = z.object({ | ||
| answers: RegistrationAnswerSchema.optional().default({}), | ||
| answers: RegistrationAnswerSchema.optional(), |
Copilot
AI
Jan 31, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By removing the default {} from RegistrationContractSchema.answers, new registrations submitted without any answers will now flow through the create pipeline with answers undefined/null, while consumers such as RegistrationResponseSchema and the registration table columns assume answers is always an object to index into. To avoid Zod parse failures and runtime row.answers[...] errors, either restore the {} default on this field or normalise missing answers to an empty object before persisting and returning registrations.
| answers: RegistrationAnswerSchema.optional(), | |
| answers: RegistrationAnswerSchema.default({}), |
| export const clerkWebhookRoutes = async (server: FastifyInstance) => { | ||
| server.post( | ||
| "/clerk", | ||
| { | ||
| config: { | ||
| rawBody: true, | ||
| }, | ||
| }, | ||
| async (request: FastifyRequest, reply: FastifyReply) => { | ||
| const webhookSecret = process.env.CLERK_WEBHOOK_SECRET; | ||
|
|
||
| if (!webhookSecret) { | ||
| server.log.error("CLERK_WEBHOOK_SECRET is not set"); | ||
| return reply.status(500).send({ error: "Webhook secret not configured" }); | ||
| } | ||
|
|
||
| const svixId = request.headers["svix-id"] as string; | ||
| const svixTimestamp = request.headers["svix-timestamp"] as string; | ||
| const svixSignature = request.headers["svix-signature"] as string; | ||
|
|
||
| if (!svixId || !svixTimestamp || !svixSignature) { | ||
| return reply.status(400).send({ error: "Missing svix headers" }); | ||
| } | ||
|
|
||
| const wh = new Webhook(webhookSecret); | ||
| let event: ClerkWebhookEvent; | ||
|
|
||
| try { | ||
| event = wh.verify(request.rawBody!, { | ||
| "svix-id": svixId, | ||
| "svix-timestamp": svixTimestamp, | ||
| "svix-signature": svixSignature, | ||
| }) as ClerkWebhookEvent; | ||
| } catch { | ||
| return reply.status(400).send({ error: "Invalid webhook signature" }); | ||
| } | ||
|
|
||
| const { type, data } = event; | ||
|
|
||
| server.log.info(`Received Clerk webhook: ${type}`); | ||
|
|
||
| try { | ||
| switch (type) { | ||
| case "user.created": { | ||
| const primaryEmail = data.email_addresses.find( | ||
| (email) => email.id === data.primary_email_address_id | ||
| ); | ||
|
|
||
| if (!primaryEmail) { | ||
| return reply.status(400).send({ error: "No primary email found" }); | ||
| } | ||
|
|
||
| await userService.createUser({ | ||
| db: server.db, | ||
| data: { | ||
| id: data.id, | ||
| email: primaryEmail.email_address, | ||
| firstName: data.first_name || "", | ||
| lastName: data.last_name || "", | ||
| }, | ||
| }); | ||
|
|
||
| server.log.info(`Created user: ${data.id}`); | ||
| break; | ||
| } | ||
|
|
||
| case "user.updated": { | ||
| const primaryEmail = data.email_addresses.find( | ||
| (email) => email.id === data.primary_email_address_id | ||
| ); | ||
|
|
||
| if (!primaryEmail) { | ||
| return reply.status(400).send({ error: "No primary email found" }); | ||
| } | ||
|
|
||
| await userService.updateUser({ | ||
| db: server.db, | ||
| data: { | ||
| id: data.id, | ||
| email: primaryEmail.email_address, | ||
| firstName: data.first_name || "", | ||
| lastName: data.last_name || "", | ||
| }, | ||
| role: null, | ||
| requesterId: null, | ||
| }); | ||
|
|
||
| server.log.info(`Updated user: ${data.id}`); | ||
| break; | ||
| } | ||
|
|
||
| case "user.deleted": { | ||
| await userService.deleteUser({ | ||
| db: server.db, | ||
| data: { id: data.id }, | ||
| role: "committee", | ||
| requesterId: data.id, | ||
| }); | ||
|
|
||
| server.log.info(`Deleted user: ${data.id}`); | ||
| break; | ||
| } | ||
|
|
||
| default: | ||
| server.log.info(`Unhandled webhook type: ${type}`); | ||
| } | ||
| } catch { | ||
| return reply.status(500).send({ error: "Error processing webhook" }); | ||
| } | ||
|
|
||
| return reply.status(200).send({ received: true }); |
Copilot
AI
Jan 31, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These new Clerk webhook routes perform signature verification and user create/update/delete side effects but currently have no automated tests. Given the importance of correctly handling signed webhooks, consider adding tests that cover valid and invalid signatures and the user.created/user.updated/user.deleted flows to prevent regressions.
No description provided.