feat: add analytics #40
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This pull request adds a comprehensive analytics feature for event registrations, including visualization charts, data tables for registration management, and backend API endpoints to support analytics data aggregation. The changes include new UI components (charts, tables, dropdowns, checkboxes), reorganization of React hooks into domain-specific folders, and extensive backend modifications to compute and serve analytics data.
Key Changes
- Added analytics dashboard with charts showing registration composition by status, trends over time, and form response breakdowns
- Implemented interactive data table with bulk actions for managing registrations (accept, reject, waitlist)
- Introduced new API endpoints for batch operations and analytics data retrieval
- Reorganized React hooks from flat structure into domain-specific folders (events/, registrations/)
Reviewed changes
Copilot reviewed 31 out of 37 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Added dependencies for charts (recharts), tables (@tanstack/react-table), and UI components (@radix-ui components) |
| apps/web/src/styles.css | Added CSS variables for chart colors and status-specific styling |
| apps/web/src/routes/events/$eventId/analytics.tsx | Implemented analytics dashboard with charts and registration table |
| apps/web/src/lib/hooks/registrations/* | New hooks for registration CRUD operations and batch updates |
| apps/web/src/lib/hooks/events/* | Moved event-related hooks to dedicated folder |
| apps/web/src/lib/data/registration.ts | Added functions for fetching registrations list, analytics, and batch operations |
| apps/web/src/components/ui/* | New reusable UI components for table, dropdown-menu, checkbox, chart, and card |
| apps/web/src/components/charts/* | Chart components for visualizing registration data |
| apps/web/src/components/tables/registration-table/* | Data table with filtering, sorting, and bulk actions |
| apps/web/src/components/controlls/* | Updated button components with corrected imports and new batch accept button |
| apps/shared/src/registrations/* | Updated schemas and types for batch operations and analytics response |
| apps/api/src/modules/registration/store.ts | Added getAnalytics method and modified queries to join user data |
| apps/api/src/modules/registration/service.ts | Removed promote-from-waitlist, modified batch update logic, added analytics service |
| apps/api/src/modules/registration/schema.ts | Consolidated batch update schemas and removed pagination from query filter |
| apps/api/src/modules/registration/route.ts | Removed promote-from-waitlist endpoint, added analytics endpoint |
| apps/api/src/modules/registration/route.test.ts | Comprehensive test coverage for new analytics endpoint |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| userId: UserIdSchema.shape.id.optional(), | ||
| page: z.coerce.number().min(1).default(1), | ||
| limit: z.coerce.number().min(1).max(100).default(20), | ||
| status: z.enum(RegistrationStatus).optional(), |
There was a problem hiding this comment.
The removal of pagination parameters (page and limit) from RegistrationsQueryFilterSchema changes the API behavior. The get method in the store no longer respects pagination, which could cause performance issues if an event has thousands of registrations. Consider if this removal is intentional or if pagination should be maintained.
| status: z.enum(RegistrationStatus).optional(), | |
| status: z.enum(RegistrationStatus).optional(), | |
| page: z.number().int().positive().optional(), | |
| limit: z.number().int().positive().optional(), |
| 'use client' | ||
|
|
There was a problem hiding this comment.
The 'use client' directive at the top of this file is not needed for TanStack Start/Router applications. This directive is specific to React Server Components in Next.js.
| 'use client' |
| ) | ||
|
|
||
| const isAcceptanceDisabled = | ||
| event.capacity == analytics.countByStatus['accepted'] || |
There was a problem hiding this comment.
The comparison should use optional chaining and nullish coalescing for safer property access. Since analytics.countByStatus might not have an 'accepted' key, accessing it directly could return undefined. Consider using analytics.countByStatus['accepted'] ?? 0 for the comparison.
| } catch (err) { | ||
| console.error(err) | ||
|
|
||
| throw new Error('Failed to load an event') |
There was a problem hiding this comment.
Generic error message "Failed to load an event" should be more specific - this is loading registrations, not an event. Consider changing to "Failed to load registrations".
| } catch (err) { | ||
| console.error(err) | ||
|
|
||
| throw new Error('Failed to load an event') |
There was a problem hiding this comment.
Generic error message "Failed to load an event" should be more specific - this is updating a registration, not loading an event. Consider changing to "Failed to update registration".
| throw new Error('Failed to load an event') | |
| throw new Error('Failed to update registration') |
| @@ -95,7 +95,6 @@ describe("Registration route", () => { | |||
| expect(response.statusCode).toBe(201); | |||
| const data = response.json(); | |||
|
|
|||
There was a problem hiding this comment.
The test comment says "CRITICAL CHANGE: Status should be pending, not waitlist" was removed. This suggests there was an important behavioral change related to registration status that should be documented. The removal of this comment loses important context about why this assertion exists.
| // CRITICAL CHANGE: Registrations should now be created with status "pending" | |
| // (even when the event is at or over capacity). Previously this was "waitlist"; | |
| // this assertion protects that behavior from being accidentally reverted. |
| sql`CASE | ||
| WHEN ${registrationsTable.status} = 'pending' THEN 0 | ||
| WHEN ${registrationsTable.status} = 'waitlist' THEN 1 | ||
| END`, |
There was a problem hiding this comment.
The SQL case statement has inconsistent indentation. The WHEN clauses should be aligned with CASE for better readability.
| sql`CASE | |
| WHEN ${registrationsTable.status} = 'pending' THEN 0 | |
| WHEN ${registrationsTable.status} = 'waitlist' THEN 1 | |
| END`, | |
| sql`CASE | |
| WHEN ${registrationsTable.status} = 'pending' THEN 0 | |
| WHEN ${registrationsTable.status} = 'waitlist' THEN 1 | |
| END`, |
| const isAcceptanceDisabled = | ||
| event.capacity == analytics.countByStatus['accepted'] || | ||
| event.capacity === null |
There was a problem hiding this comment.
The logic here seems incorrect. The condition checks if capacity equals accepted count OR if capacity is null, then disables accepting. However, if capacity is null (unlimited), accepting should be enabled, not disabled. The condition should be event.capacity !== null && event.capacity === analytics.countByStatus['accepted'].
| } catch (err) { | ||
| console.error(err) | ||
|
|
||
| throw new Error('Failed to load an event') |
There was a problem hiding this comment.
Generic error message "Failed to load an event" should be more specific - this is loading analytics data, not an event. Consider changing to "Failed to load registration analytics".
| throw new Error('Failed to load an event') | |
| throw new Error('Failed to load registration analytics') |
| 'use client' | ||
|
|
There was a problem hiding this comment.
The 'use client' directive at the top of this file is not needed for TanStack Start/Router applications. This directive is specific to React Server Components in Next.js. TanStack Start handles client/server boundaries differently.
| 'use client' |
No description provided.