feat: add ability to delete events and add confirmation dialogs #35
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
HJyup
changed the title
There was a problem hiding this comment.
Pull request overview
This PR adds event deletion functionality with confirmation dialogs for both delete and publish actions. The changes improve user experience by requiring confirmation before destructive operations and extracting button logic into reusable components.
- Adds event deletion capability with a confirmation dialog
- Refactors publish and register buttons into separate components with confirmation flows
- Updates the API endpoint for event updates from POST to PUT (following REST conventions)
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
pnpm-lock.yaml |
Moves tw-animate-css from dependencies to devDependencies |
apps/web/package.json |
Moves tw-animate-css from dependencies to devDependencies |
apps/web/src/styles.css |
Imports tw-animate-css and updates popover color, removes custom scrollbar styles |
apps/web/src/routes/events/$eventId/index.tsx |
Refactors event page to use new button components for delete, publish, and register actions |
apps/web/src/routes/events/$eventId/edit.tsx |
Adds cancel button handler for edit form |
apps/web/src/lib/hooks/use-delete-event.tsx |
New hook for deleting events with mutation handling |
apps/web/src/lib/data/event.ts |
Adds deleteEvent server function and changes update endpoint from POST to PUT |
apps/web/src/config/mocks.ts |
Removes mock data file (no longer needed) |
apps/web/src/config/event-form.ts |
New config file with default form fields for event registration |
apps/web/src/components/forms/modify-event-form.tsx |
Refactors to use centralized default fields, adds cancel button, removes URL prefix handling |
apps/web/src/components/controlls/register-event-button.tsx |
New component for event registration button with form dialog |
apps/web/src/components/controlls/publish-event-button.tsx |
New component for publishing events with confirmation dialog |
apps/web/src/components/controlls/delete-event-button.tsx |
New component for deleting events with confirmation dialog |
apps/api/src/modules/events/route.ts |
Changes update endpoint from POST to PUT method |
apps/api/src/modules/events/route.test.ts |
Updates tests to reflect PUT method for event updates |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,49 @@ | |||
| import { useNavigate } from '@tanstack/react-router' | |||
There was a problem hiding this comment.
The directory name "controlls" appears to be misspelled. The correct spelling should be "controls" (with one 'l'). This affects the import paths in other files and should be renamed for consistency with standard English spelling.
| id: `field-${Date.now()}-1`, | ||
| type: 'select', | ||
| label: 'University Year', | ||
| required: true, | ||
| options: ['1', '2', '3', '4', 'Masters', 'PhD'], | ||
| }, | ||
| { | ||
| id: `field-${Date.now()}-2`, |
There was a problem hiding this comment.
The DEFAULT_FIELDS array contains duplicate IDs. The first field has id field-${Date.now()}-1 (line 5), and the second field also has id field-${Date.now()}-1 (line 11). Since Date.now() is evaluated once at module load time, both fields will have identical IDs, which can cause issues with form field management and React key props. Each field should have a unique ID suffix.
| id: `field-${Date.now()}-1`, | |
| type: 'select', | |
| label: 'University Year', | |
| required: true, | |
| options: ['1', '2', '3', '4', 'Masters', 'PhD'], | |
| }, | |
| { | |
| id: `field-${Date.now()}-2`, | |
| id: `field-${Date.now()}-2`, | |
| type: 'select', | |
| label: 'University Year', | |
| required: true, | |
| options: ['1', '2', '3', '4', 'Masters', 'PhD'], | |
| }, | |
| { | |
| id: `field-${Date.now()}-3`, |
| data: { eventId }, | ||
| }), | ||
| onSuccess: (event) => { | ||
| void queryClient.invalidateQueries({ queryKey: ['events', eventId] }) |
There was a problem hiding this comment.
After deleting an event, only the single event query is invalidated with queryKey ['events', eventId]. However, the events list queries (used on the homepage and draft pages) use queryKeys like ['events', { state: 'published' }] and ['events', { state: 'draft' }]. These list queries will not be invalidated, so the deleted event will still appear in the list until the page is refreshed. Consider invalidating all events queries using { queryKey: ['events'] } to ensure list views are updated.
| void queryClient.invalidateQueries({ queryKey: ['events', eventId] }) | |
| void queryClient.invalidateQueries({ queryKey: ['events'] }) |
| function PublishEventButton({ eventId }: { eventId: string }) { | ||
| const { publishEvent, isPublishing } = usePublishEvent(eventId) | ||
|
|
||
| return ( |
There was a problem hiding this comment.
After publishing an event (which changes its state from 'draft' to 'published'), only the single event query is invalidated with queryKey ['events', eventId]. However, the events list queries use queryKeys like ['events', { state: 'published' }] and ['events', { state: 'draft' }]. These list queries will not be invalidated, so the published event will still appear in the draft list and won't appear in the published list until the page is refreshed. Consider invalidating all events queries using { queryKey: ['events'] } to ensure list views are updated.
| <Dialog> | ||
| <DialogTrigger asChild> | ||
| <Button>Publish</Button> | ||
| </DialogTrigger> | ||
| <DialogContent> | ||
| <DialogHeader> | ||
| <DialogTitle>Publish Event</DialogTitle> | ||
| <DialogDescription> | ||
| Are you sure you want to publish this event? Once published, it will | ||
| be visible in the search results and students will be able to | ||
| register. | ||
| </DialogDescription> | ||
| </DialogHeader> | ||
| <div className="flex justify-start gap-3 mt-4"> | ||
| <Button onClick={() => publishEvent()} disabled={isPublishing}> | ||
| {isPublishing ? 'Publishing' : 'Publish'} | ||
| </Button> | ||
| </div> | ||
| </DialogContent> | ||
| </Dialog> |
There was a problem hiding this comment.
The Dialog doesn't automatically close after the publish action is successful. The button triggers publishEvent() but there's no mechanism to close the dialog after a successful publish. Consider using controlled Dialog state with open/onOpenChange props and closing the dialog in the onSuccess callback of the mutation, or add a DialogClose component wrapper around the confirmation button.
| <Dialog> | ||
| <DialogTrigger asChild> | ||
| <Button variant="secondary">Delete</Button> | ||
| </DialogTrigger> | ||
| <DialogContent> | ||
| <DialogHeader> | ||
| <DialogTitle>Delete Event</DialogTitle> | ||
| <DialogDescription> | ||
| Are you sure you want to delete this event? This action is permanent | ||
| and will remove all associated registrations and data. | ||
| </DialogDescription> | ||
| </DialogHeader> | ||
| <div className="flex justify-start gap-3 mt-4"> | ||
| <Button | ||
| disabled={isDeleting} | ||
| onClick={() => { | ||
| deleteEvent(undefined, { | ||
| onSuccess: () => { | ||
| void navigate({ to: '/', replace: true }) | ||
| }, | ||
| }) | ||
| }} | ||
| > | ||
| {isDeleting ? 'Deleting...' : 'Confirm Delete'} | ||
| </Button> | ||
| </div> | ||
| </DialogContent> | ||
| </Dialog> |
There was a problem hiding this comment.
The Dialog doesn't automatically close after the delete action completes. Although the user is navigated away on success, if an error occurs or if there's a delay, the dialog remains open with no way to dismiss it except by clicking outside. Consider using controlled Dialog state with open/onOpenChange props and closing the dialog in both the onSuccess callback (before navigation) and potentially in the onError callback.
…elete, publish