feat: integrate web and api for an event service #33
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR integrates Clerk authentication with TanStack Start and connects the web application to the API backend. The changes enable server-side authentication, event management functionality (create, read, update, publish), and improved UI consistency.
Key Changes:
- Integration of
@clerk/tanstack-react-startfor SSR-compatible authentication with middleware - Full API connectivity for events with create, update, and publish operations
- Form enhancements including priority selection and improved validation
- Dependency updates across the stack (TanStack Router, React Query, Zod, TypeScript ESLint)
Reviewed changes
Copilot reviewed 36 out of 37 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Updated dependencies including TanStack packages, Zod, TypeScript ESLint, and added Clerk TanStack integration |
| apps/web/vite.config.ts | Reordered plugins and added module aliases for compatibility |
| apps/web/src/start.ts | New file creating Start instance with Clerk middleware |
| apps/web/src/routes/*.tsx | Updated routes to fetch real data from API and integrate new Clerk hooks |
| apps/web/src/lib/data/event.ts | Expanded to include create, update operations with authentication |
| apps/web/src/components/forms/modify-event-form.tsx | Enhanced form with priority field and improved request transformation |
| apps/shared/src/**/*.ts | Updated imports to use explicit .js extensions |
| apps/api/src/modules/events/route.ts | Changed update endpoint from PUT to POST |
| apps/api/src/db/schema.ts | Fixed inconsistent field name (locationUrl → locationURL) |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| capacity: form.capacity ? parseInt(form.capacity, 10) : null, | ||
| date: date.toISOString(), | ||
| aboutMarkdown: form.aboutMarkdown || null, | ||
| locationURL: form.locationURL ? `https://${form.locationURL}` : null, |
There was a problem hiding this comment.
The locationURL transformation prepends 'https://' to the URL, but this doesn't validate if the URL already has a protocol or is a valid URL format. This could result in malformed URLs like 'https://https://example.com'. Consider validating the URL format first or checking if it already has a protocol.
| locationURL: form.locationURL ? `https://${form.locationURL}` : null, | |
| locationURL: form.locationURL | |
| ? (/^[a-zA-Z][a-zA-Z\d+\-.]*:\/\//.test(form.locationURL) | |
| ? form.locationURL | |
| : `https://${form.locationURL}`) | |
| : null, |
| } catch (err) { | ||
| console.error(err) | ||
| throw new Error('Failed to load events') | ||
| } |
There was a problem hiding this comment.
The error handling here logs to console.error and then throws a generic error message. The original error information is lost to the caller. Consider including the error details in the thrown error or re-throwing the original error for better debugging.
| }); | ||
|
|
||
| server.put("/:id", async (request, reply) => { | ||
| server.post("/:id", async (request, reply) => { |
There was a problem hiding this comment.
The HTTP method for updating an event should typically be PUT or PATCH, not POST. Using POST for updates is not RESTful and can be confusing. Consider changing this to PUT to follow REST conventions.
| server.post("/:id", async (request, reply) => { | |
| server.put("/:id", async (request, reply) => { |
| const data = UpdateEventSchema.parse({ | ||
| id, | ||
| ...dto, | ||
| id, | ||
| ...(dto.date && { date: new Date(dto.date) }), |
There was a problem hiding this comment.
The order of object spreading here could lead to the 'id' being overwritten if it exists in 'dto'. The id should be applied after spreading dto to ensure it takes precedence, or extract it from dto first to avoid conflicts.
| export const updateEvent = createServerFn({ method: 'POST' }) | ||
| .inputValidator((data: UpdateEventRequest & Pick<Event, 'id'>) => { | ||
| return UpdateEventContractSchema.extend({ | ||
| id: z.string(), | ||
| }).parse(data) | ||
| }) | ||
| .handler(async ({ data }) => { | ||
| const eventId = data.id | ||
| const authObj = await auth() | ||
| const token = await authObj.getToken() | ||
|
|
||
| const baseUrl = process.env.API_BASE_URL | ||
| if (!baseUrl) { | ||
| throw new Error('API_BASE_URL is not defined') | ||
| } | ||
|
|
||
| const { data: event } = await axios.post<Event>( |
There was a problem hiding this comment.
The HTTP method was changed from PUT to POST, but this route is for updating an existing resource. The client code in lib/data/event.ts also uses POST for updates, which is inconsistent with REST conventions. Consider using PUT for updates to follow REST best practices.
No description provided.