Conversation
There was a problem hiding this comment.
Pull request overview
This PR enhances the admin booking filter by adding a status filter dropdown and replacing the date input field with date range shortcuts. It also addresses a frontend test error.
- Added status filter with "Approved", "Pending", and "Rejected" options
- Replaced date input with dropdown shortcuts (Tomorrow, Next week, Last month, etc.)
- Implemented date range filtering that filters bookings between selected date and today
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
frontend/src/custom-hooks/use-table-state-admin.ts |
Added FilterableItem interface, status filter logic, and date range filtering using date-fns interval functions |
frontend/src/components/admin-search-bar/admin-search-bar.tsx |
Added status dropdown, replaced date input with date shortcut dropdown, implemented date change handler with preset calculations |
frontend/src/components/admin-search-bar/admin-search-bar.module.scss |
Adjusted flex sizing for filter inputs to accommodate additional status filter |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| interface FilterableItem { | ||
| title?: string; | ||
| venue?: { | ||
| name?: string; | ||
| } | null; | ||
| date?: string; | ||
| status?: string; | ||
| } |
There was a problem hiding this comment.
The FilterableItem interface defines a date property, but the actual booking data uses eventDateString as the property name (as seen in booking-admin-table.tsx where EVENT_DATE_STRING is populated). This mismatch will cause the date filtering to fail since item.date will be undefined. The interface should define eventDateString?: string; instead of date?: string;, or the filtering logic should check for the correct property name.
| if (!item.date) return false; | ||
|
|
||
| const itemDate = parse(item.date, DATE_FORMAT, new Date()); | ||
|
|
||
| const start = startOfDay(filterDate < today ? filterDate : today); | ||
| const end = endOfDay(filterDate > today ? filterDate : today); | ||
|
|
||
| return isWithinInterval(itemDate, { start, end }); |
There was a problem hiding this comment.
The date filtering logic checks item.date, but the actual property name in the booking data is eventDateString (as defined in constants and used in the booking table). This will cause the filter to always return false for items, making the date filter non-functional. The code should check item.eventDateString instead.
| return data.filter((datum) => { | ||
| const item = datum; |
There was a problem hiding this comment.
The variable item is assigned from datum without any transformation or purpose. This is redundant and adds no value. Either use datum directly throughout the filter function or remove this assignment.
| return data.filter((datum) => { | |
| const item = datum; | |
| return data.filter((item) => { |
| const currentShortcutValue = useMemo(() => { | ||
| if (!filters.date) return ""; | ||
| const today = new Date(); | ||
| const target = filters.date; | ||
|
|
||
| if (target === format(addDays(today, 1), "yyyy-MM-dd")) return "tomorrow"; | ||
| if (target === format(addDays(today, 3), "yyyy-MM-dd")) return "next3days"; | ||
| if (target === format(addWeeks(today, 1), "yyyy-MM-dd")) return "nextweek"; | ||
| if (target === format(addWeeks(today, 2), "yyyy-MM-dd")) return "next2weeks"; | ||
| if (target === format(addMonths(today, 1), "yyyy-MM-dd")) return "nextmonth"; | ||
| if (target === format(addMonths(today, 3), "yyyy-MM-dd")) return "next3months"; | ||
| if (target === format(addYears(today, 1), "yyyy-MM-dd")) return "nextyear"; | ||
| if (target === format(subDays(today, 1), "yyyy-MM-dd")) return "yesterday"; | ||
| if (target === format(subDays(today, 3), "yyyy-MM-dd")) return "last3days"; | ||
| if (target === format(subWeeks(today, 1), "yyyy-MM-dd")) return "lastweek"; | ||
| if (target === format(subWeeks(today, 2), "yyyy-MM-dd")) return "last2weeks"; | ||
| if (target === format(subMonths(today, 1), "yyyy-MM-dd")) return "lastmonth"; | ||
| if (target === format(subMonths(today, 3), "yyyy-MM-dd")) return "last3months"; | ||
| if (target === format(subYears(today, 1), "yyyy-MM-dd")) return "lastyear"; | ||
|
|
||
| return ""; | ||
| }, [filters.date]); |
There was a problem hiding this comment.
The currentShortcutValue computation performs multiple date calculations and format operations on every render. While it's memoized with useMemo, the calculation itself is still expensive with 14 conditional branches. Consider extracting this logic into a helper function or simplifying by storing the shortcut value directly in state when a shortcut is selected, rather than reverse-engineering it from the date string.
| const today = new Date(); | ||
|
|
||
| if (!item.date) return false; | ||
|
|
||
| const itemDate = parse(item.date, DATE_FORMAT, new Date()); | ||
|
|
||
| const start = startOfDay(filterDate < today ? filterDate : today); | ||
| const end = endOfDay(filterDate > today ? filterDate : today); |
There was a problem hiding this comment.
The date range logic creates an interval from the earlier of filterDate or today to the later of the two. This means selecting "Tomorrow" filters bookings between today and tomorrow, while "Yesterday" filters bookings between yesterday and today. This range-based filtering behavior is not intuitive given the shortcut names like "Tomorrow" or "Yesterday" which suggest single-day filtering. Consider clarifying the intended behavior: if these shortcuts should filter for a specific day, use a single-day range instead; if they should filter for ranges, consider renaming the shortcuts to be more explicit (e.g., "Through Tomorrow", "Since Yesterday").
| const today = new Date(); | |
| if (!item.date) return false; | |
| const itemDate = parse(item.date, DATE_FORMAT, new Date()); | |
| const start = startOfDay(filterDate < today ? filterDate : today); | |
| const end = endOfDay(filterDate > today ? filterDate : today); | |
| if (!item.date) return false; | |
| const itemDate = parse(item.date, DATE_FORMAT, new Date()); | |
| const start = startOfDay(filterDate); | |
| const end = endOfDay(filterDate); |
Uh oh!
There was an error while loading. Please reload this page.