feat: reschedule calendar modal on inbox #518
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several enhancements to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (3)
apps/frontend/src/lib/@types/Items/Cycle.ts (1)
11-11: Document the expected date string format.To ensure consistent date handling, consider adding JSDoc documentation to specify the expected format for date strings (e.g., ISO 8601).
+ /** Due date for the item. When provided as string, should be in ISO 8601 format */ dueDate: Date | string | nullapps/frontend/src/components/Inbox/RescheduleCalendar/Calendar.tsx (1)
12-17: Consider adding prop validation for critical props.While the props are well-structured, consider adding runtime validation for critical props using PropTypes or a schema validation library like Zod, especially if this component will be used across different parts of the application.
Example with PropTypes:
import PropTypes from 'prop-types'; Calendar.propTypes = { className: PropTypes.string, classNames: PropTypes.object, showOutsideDays: PropTypes.bool, selected: PropTypes.instanceOf(Date), onSelect: PropTypes.func };apps/frontend/src/components/Inbox/InboxItems.tsx (1)
Line range hint
1-238: Consider splitting component responsibilitiesThe
InboxItemscomponent is handling multiple responsibilities:
- Inbox item rendering
- Item status management
- Rescheduling functionality
- Modal management
This could make the component harder to maintain as it grows.
Consider:
- Extracting the rescheduling modal into a separate component
- Creating a custom hook for inbox item management
- Moving the date handling logic into a separate service
Example structure:
// useInboxItems.ts export const useInboxItems = () => { // Item management logic } // RescheduleModal.tsx export const RescheduleModal = ({ isOpen, onClose, itemId, onReschedule }) => { // Modal and calendar logic } // InboxItems.tsx export const InboxItems = () => { const { items, handleDone } = useInboxItems() // Rendering logic only }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
apps/frontend/src/components/Inbox/InboxItems.tsx(6 hunks)apps/frontend/src/components/Inbox/RescheduleCalendar/Calendar.tsx(1 hunks)apps/frontend/src/components/Inbox/RescheduleCalendar/DateSelectionButton.tsx(1 hunks)apps/frontend/src/components/Inbox/RescheduleCalendar/RescheduleCalendar.tsx(1 hunks)apps/frontend/src/lib/@types/Items/Cycle.ts(1 hunks)
🔇 Additional comments (6)
apps/frontend/src/components/Inbox/RescheduleCalendar/DateSelectionButton.tsx (1)
1-11: LGTM! Well-structured props interface.
The component props are well-typed and have clear, descriptive names. The use of React.ElementType for the icon prop is the correct approach for component props.
apps/frontend/src/lib/@types/Items/Cycle.ts (1)
11-11: LGTM! Verify date handling across the application.
The type change to support Date | string | null is appropriate for the calendar rescheduling feature. However, ensure consistent date handling throughout the application.
Let's verify the date handling implementation:
✅ Verification successful
Date handling is consistent and well-managed across the codebase
The verification shows a robust and consistent date handling implementation:
- Frontend uses TypeScript's
Dateobjects with proper formatting viadate-fnslibrary functions - Backend consistently handles dates with proper UTC considerations and ISO string conversions
- Date serialization/deserialization is handled properly:
- Frontend converts to ISO strings when sending to backend (
toISOString()) - Backend properly parses string dates using
new Date() - Clear date range handling with start/end of day/week/month utilities
- Frontend converts to ISO strings when sending to backend (
The type change to Date | string | null aligns with the existing date handling patterns, where dates are manipulated as Date objects internally and serialized to ISO strings for storage/transmission.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for date parsing/formatting consistency
# Look for date handling patterns across the codebase
# Search for date parsing/formatting
rg -A 2 "new Date\(|Date\.|toISOString|toLocaleDateString"
# Look for potential date-related utility functions
ast-grep --pattern 'function $_(date: Date) { $$$ }'
Length of output: 20141
apps/frontend/src/components/Inbox/RescheduleCalendar/Calendar.tsx (2)
1-10: LGTM! Clean imports and type definition.
The imports are well-organized, and the type definition properly extends DayPicker props for type safety.
69-71: LGTM! Clean component export.
The component export follows React best practices by setting the displayName property.
apps/frontend/src/components/Inbox/RescheduleCalendar/RescheduleCalendar.tsx (1)
30-49: 🛠️ Refactor suggestion
Reduce code duplication in date setter functions.
The date setter functions follow a similar pattern and could be optimized using a single handler.
Consider consolidating the functions:
+ const handleDateChange = (newDate: Date | null) => {
+ setDate(newDate)
+ setDateChanged(true)
+ }
+
- const setToday = () => {
- setNoDateFlag(false)
- setDate(new Date())
- setDateChanged(true)
- }
+ const setToday = () => handleDateChange(new Date())
- const setTomorrow = () => {
- setNoDateFlag(false)
- setDate(addDays(new Date(), 1))
- setDateChanged(true)
- }
+ const setTomorrow = () => handleDateChange(addDays(new Date(), 1))
- const setNextWeek = () => {
- setNoDateFlag(false)
- setDate(addDays(new Date(), 7))
- setDateChanged(true)
- }
+ const setNextWeek = () => handleDateChange(addDays(new Date(), 7))
- const setNoDate = () => {
- setNoDateFlag(true)
- setDate(null)
- setDateChanged(true)
- }
+ const setNoDate = () => handleDateChange(null)Likely invalid or redundant comment.
apps/frontend/src/components/Inbox/InboxItems.tsx (1)
5-5: LGTM: Clean state management setup
The new imports and state variables are well-structured and properly typed for the rescheduling functionality.
Also applies to: 8-8, 21-26
| <button | ||
| className="hover-bg hover-text flex w-full cursor-pointer items-center justify-between gap-4 rounded-lg p-2" | ||
| onClick={onClick} | ||
| > | ||
| <span className="flex items-center gap-2"> | ||
| <Icon size={24} /> {label} | ||
| </span> | ||
| <span className="text-sm">{formattedDate}</span> | ||
| </button> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility and keyboard interaction.
While the basic implementation is solid, consider these improvements for better accessibility and user experience:
<button
className="hover-bg hover-text flex w-full cursor-pointer items-center justify-between gap-4 rounded-lg p-2"
onClick={onClick}
+ type="button"
+ aria-label={`Schedule for ${formattedDate}`}
+ onKeyDown={(e) => {
+ if (e.key === 'Enter' || e.key === ' ') {
+ e.preventDefault();
+ onClick();
+ }
+ }}
>Also consider making the icon size configurable through props instead of hardcoding it to 24:
- <Icon size={24} /> {label}
+ <Icon size={iconSize ?? 24} /> {label}And update the props interface:
}: {
label: string
icon: React.ElementType
formattedDate: string
onClick: () => void
+ iconSize?: number
})Committable suggestion skipped: line range outside the PR's diff.
| <DayPicker | ||
| showOutsideDays={showOutsideDays} | ||
| className={cn("p-3", className)} | ||
| classNames={{ | ||
| months: "flex flex-col sm:flex-row space-y-4 sm:space-x-4 sm:space-y-0", | ||
| month: "space-y-4 text-primary-foreground", | ||
| caption: "flex justify-center pt-1 relative items-center", | ||
| caption_label: "text-sm font-medium", | ||
| nav: "space-x-1 flex items-center", | ||
| nav_button: cn( | ||
| "h-7 w-7 text-center bg-transparent p-1 text-secondary-foreground hover-text" | ||
| ), | ||
| nav_button_previous: "absolute left-1", | ||
| nav_button_next: "absolute right-1", | ||
| table: "w-full border-collapse space-y-1", | ||
| head_row: "flex", | ||
| head_cell: | ||
| "text-secondary-foreground rounded-md w-8 font-normal text-[0.8rem]", | ||
| row: "flex w-full mt-2", | ||
| cell: cn( | ||
| "relative p-0 text-center text-sm focus-within:relative focus-within:z-20 [&:has([aria-selected])]:bg-accent [&:has([aria-selected].day-outside)]:bg-accent/50 [&:has([aria-selected].day-range-end)]:rounded-r-md", | ||
| props.mode === "range" | ||
| ? "[&:has(>.day-range-end)]:rounded-r-md [&:has(>.day-range-start)]:rounded-l-md first:[&:has([aria-selected])]:rounded-l-md last:[&:has([aria-selected])]:rounded-r-md" | ||
| : "[&:has([aria-selected])]:rounded-md" | ||
| ), | ||
| day: cn( | ||
| "h-8 w-8 p-0 font-normal aria-selected:opacity-100 text-secondary-foreground hover-bg hover-text rounded-lg" | ||
| ), | ||
| day_range_start: "day-range-start", | ||
| day_range_end: "day-range-end", | ||
| day_selected: | ||
| "bg-background-hover border border-border !text-foreground", | ||
| day_today: "bg-background-hover text-foreground", | ||
| day_outside: | ||
| "day-outside text-muted-foreground opacity-50 aria-selected:bg-accent/50 aria-selected:text-muted-foreground aria-selected:opacity-30", | ||
| day_disabled: "text-muted-foreground opacity-50", | ||
| day_range_middle: | ||
| "aria-selected:bg-accent aria-selected:text-accent-foreground", | ||
| day_hidden: "invisible", | ||
|
|
||
| ...classNames, | ||
| }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider extracting class names and enhancing accessibility.
While the styling implementation is comprehensive, consider these improvements:
- Extract commonly used class names into constants
- Add ARIA labels for better screen reader support
- Consider adding keyboard navigation instructions
Example refactor:
const CALENDAR_CLASSES = {
BASE: "p-3",
MONTHS: "flex flex-col sm:flex-row space-y-4 sm:space-x-4 sm:space-y-0",
// ... other class definitions
} as const;
// In the component
<DayPicker
showOutsideDays={showOutsideDays}
className={cn(CALENDAR_CLASSES.BASE, className)}
aria-label="Date picker calendar"
// ... other props
/>| components={{ | ||
| IconLeft: ({ ...props }) => <ChevronLeftIcon className="size-4" />, | ||
| IconRight: ({ ...props }) => <ChevronRightIcon className="size-4" />, | ||
| }} |
There was a problem hiding this comment.
Enhance navigation accessibility and remove unused props.
The navigation implementation can be improved:
- Remove unused props spread
- Add aria-labels for better accessibility
Apply this diff:
components={{
- IconLeft: ({ ...props }) => <ChevronLeftIcon className="size-4" />,
- IconRight: ({ ...props }) => <ChevronRightIcon className="size-4" />,
+ IconLeft: () => (
+ <ChevronLeftIcon className="size-4" aria-label="Previous month" />
+ ),
+ IconRight: () => (
+ <ChevronRightIcon className="size-4" aria-label="Next month" />
+ ),
}}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| components={{ | |
| IconLeft: ({ ...props }) => <ChevronLeftIcon className="size-4" />, | |
| IconRight: ({ ...props }) => <ChevronRightIcon className="size-4" />, | |
| }} | |
| components={{ | |
| IconLeft: () => ( | |
| <ChevronLeftIcon className="size-4" aria-label="Previous month" /> | |
| ), | |
| IconRight: () => ( | |
| <ChevronRightIcon className="size-4" aria-label="Next month" /> | |
| ), | |
| }} |
| export function RescheduleCalendar({ | ||
| date, | ||
| setDate, | ||
| dateChanged, | ||
| setDateChanged, | ||
| ...props | ||
| }: RescheduleCalendarProps) { | ||
| const [noDate, setNoDateFlag] = React.useState<boolean>(false) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize state management by removing redundant state.
The noDate state can be derived from the date prop, making it unnecessary to maintain as separate state.
Replace the state with a computed value:
- const [noDate, setNoDateFlag] = React.useState<boolean>(false)
+ const noDate = date === nullThen update the setter functions to remove setNoDateFlag calls since they're no longer needed.
Committable suggestion skipped: line range outside the PR's diff.
| interface RescheduleCalendarProps { | ||
| date: Date | null | ||
| setDate: (date: Date | null) => void | ||
| dateChanged: boolean | ||
| setDateChanged: (state: boolean) => void | ||
| icon?: React.ReactNode | ||
| [key: string]: any | ||
| } |
There was a problem hiding this comment.
Improve type safety of the RescheduleCalendarProps interface.
The interface has two potential issues:
- The catch-all index signature
[key: string]: anyreduces type safety by allowing any additional props - The
iconprop is defined but never used in the component
Consider this improved interface definition:
interface RescheduleCalendarProps {
date: Date | null
setDate: (date: Date | null) => void
dateChanged: boolean
setDateChanged: (state: boolean) => void
- icon?: React.ReactNode
- [key: string]: any
+ className?: string // Add specific props you need instead of [key: string]: any
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| interface RescheduleCalendarProps { | |
| date: Date | null | |
| setDate: (date: Date | null) => void | |
| dateChanged: boolean | |
| setDateChanged: (state: boolean) => void | |
| icon?: React.ReactNode | |
| [key: string]: any | |
| } | |
| interface RescheduleCalendarProps { | |
| date: Date | null | |
| setDate: (date: Date | null) => void | |
| dateChanged: boolean | |
| setDateChanged: (state: boolean) => void | |
| className?: string // Add specific props you need instead of [key: string]: any | |
| } |
| const todayFormatted = format(new Date(), "eee") | ||
| const tomorrowFormatted = format(addDays(new Date(), 1), "eee") | ||
| const nextWeekFormatted = format(addDays(new Date(), 7), "eee MMM dd") | ||
|
|
||
| const dueDateFormatted = noDate | ||
| ? "No Date Assigned" | ||
| : date | ||
| ? format(date, "eee MMM dd") | ||
| : "No Date Assigned" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve date formatting maintainability.
The date format strings should be constants, and the due date formatting logic can be simplified.
Consider these improvements:
+ const DATE_FORMATS = {
+ dayOfWeek: "eee",
+ fullDate: "eee MMM dd"
+ } as const
+
- const todayFormatted = format(new Date(), "eee")
- const tomorrowFormatted = format(addDays(new Date(), 1), "eee")
- const nextWeekFormatted = format(addDays(new Date(), 7), "eee MMM dd")
+ const todayFormatted = format(new Date(), DATE_FORMATS.dayOfWeek)
+ const tomorrowFormatted = format(addDays(new Date(), 1), DATE_FORMATS.dayOfWeek)
+ const nextWeekFormatted = format(addDays(new Date(), 7), DATE_FORMATS.fullDate)
- const dueDateFormatted = noDate
- ? "No Date Assigned"
- : date
- ? format(date, "eee MMM dd")
- : "No Date Assigned"
+ const dueDateFormatted = date
+ ? format(date, DATE_FORMATS.fullDate)
+ : "No Date Assigned"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const todayFormatted = format(new Date(), "eee") | |
| const tomorrowFormatted = format(addDays(new Date(), 1), "eee") | |
| const nextWeekFormatted = format(addDays(new Date(), 7), "eee MMM dd") | |
| const dueDateFormatted = noDate | |
| ? "No Date Assigned" | |
| : date | |
| ? format(date, "eee MMM dd") | |
| : "No Date Assigned" | |
| const DATE_FORMATS = { | |
| dayOfWeek: "eee", | |
| fullDate: "eee MMM dd" | |
| } as const | |
| const todayFormatted = format(new Date(), DATE_FORMATS.dayOfWeek) | |
| const tomorrowFormatted = format(addDays(new Date(), 1), DATE_FORMATS.dayOfWeek) | |
| const nextWeekFormatted = format(addDays(new Date(), 7), DATE_FORMATS.fullDate) | |
| const dueDateFormatted = date | |
| ? format(date, DATE_FORMATS.fullDate) | |
| : "No Date Assigned" |
| return ( | ||
| <div | ||
| className="flex flex-row-reverse overflow-hidden rounded-lg bg-background text-secondary-foreground" | ||
| {...props} | ||
| > | ||
| <div className="p-4 text-sm"> | ||
| <div className="mb-2 text-primary-foreground"> | ||
| Due Date:{" "} | ||
| <strong className="text-foreground">{dueDateFormatted}</strong> | ||
| </div> | ||
| <div> | ||
| <DateSelectionButton | ||
| label="Today" | ||
| icon={CalendarDot} | ||
| formattedDate={todayFormatted} | ||
| onClick={setToday} | ||
| /> | ||
| <DateSelectionButton | ||
| label="Tomorrow" | ||
| icon={SquareChevronRightIcon} | ||
| formattedDate={tomorrowFormatted} | ||
| onClick={setTomorrow} | ||
| /> | ||
| <DateSelectionButton | ||
| label="Next Week" | ||
| icon={CalendarDots} | ||
| formattedDate={nextWeekFormatted} | ||
| onClick={setNextWeek} | ||
| /> | ||
| <DateSelectionButton | ||
| label="No Date" | ||
| icon={CalendarX} | ||
| formattedDate={""} | ||
| onClick={setNoDate} | ||
| /> | ||
| </div> | ||
| </div> | ||
| <Calendar | ||
| mode="single" | ||
| selected={date || undefined} | ||
| onSelect={(selectedDate) => { | ||
| setNoDateFlag(false) | ||
| setDate(selectedDate || null) | ||
| setDateChanged(true) | ||
| }} | ||
| initialFocus | ||
| /> | ||
| </div> |
There was a problem hiding this comment.
Enhance accessibility and prop handling.
The component needs accessibility improvements and better prop handling for the Calendar component.
Consider these improvements:
- Add proper ARIA labels:
<div
className="flex flex-row-reverse overflow-hidden rounded-lg bg-background text-secondary-foreground"
+ role="dialog"
+ aria-label="Reschedule calendar"
{...props}
>- Avoid undefined fallback:
<Calendar
mode="single"
- selected={date || undefined}
+ selected={date}
onSelect={(selectedDate) => {- Add error boundary and loading states:
+ if (error) {
+ return <div>Error loading calendar</div>
+ }
+
+ if (loading) {
+ return <div>Loading calendar...</div>
+ }Committable suggestion skipped: line range outside the PR's diff.
| {reschedulingItemId !== null && ( | ||
| <div> | ||
| <div | ||
| className="fixed inset-0 z-50 cursor-default bg-black/80" | ||
| role="button" | ||
| onClick={() => setReschedulingItemId(null)} | ||
| onKeyDown={(e) => { | ||
| if (e.key === "Escape" || e.key === "Esc") { | ||
| setReschedulingItemId(null) | ||
| } | ||
| }} | ||
| tabIndex={0} | ||
| ></div> | ||
| <div className="fixed left-1/2 top-1/2 z-50 -translate-x-1/2 -translate-y-1/2 rounded-md bg-background shadow-lg"> | ||
| <RescheduleCalendar | ||
| date={date} | ||
| setDate={setDate} | ||
| dateChanged={dateChanged} | ||
| setDateChanged={setDateChanged} | ||
| /> | ||
| </div> | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
Enhance modal accessibility
The modal implementation needs accessibility improvements for better keyboard navigation and screen reader support.
Consider these accessibility enhancements:
{reschedulingItemId !== null && (
- <div>
+ <div role="dialog" aria-modal="true" aria-label="Reschedule Item">
<div
className="fixed inset-0 z-50 cursor-default bg-black/80"
role="button"
+ aria-label="Close modal"
onClick={() => setReschedulingItemId(null)}
onKeyDown={(e) => {
if (e.key === "Escape" || e.key === "Esc") {
setReschedulingItemId(null)
}
}}
tabIndex={0}
/>
- <div className="fixed left-1/2 top-1/2 z-50 -translate-x-1/2 -translate-y-1/2 rounded-md bg-background shadow-lg">
+ <div
+ className="fixed left-1/2 top-1/2 z-50 -translate-x-1/2 -translate-y-1/2 rounded-md bg-background shadow-lg"
+ role="document"
+ >
<RescheduleCalendar
date={date}
setDate={setDate}
dateChanged={dateChanged}
setDateChanged={setDateChanged}
/>
</div>
</div>
)}Consider also:
- Adding a focus trap to keep focus within the modal
- Restoring focus to the trigger element when the modal closes
- Adding aria-describedby for additional context
Committable suggestion skipped: line range outside the PR's diff.
| useEffect(() => { | ||
| if (dateChanged && reschedulingItemId && date) { | ||
| updateItem(session, { dueDate: date }, reschedulingItemId) | ||
| setReschedulingItemId(null) | ||
| setDateChanged(false) | ||
| setUpdateInbox(!updateInbox) | ||
| } | ||
| }, [date, updateItem, session, reschedulingItemId, dateChanged, updateInbox]) | ||
|
|
||
| useEffect(() => { | ||
| fetchInbox(session) | ||
| }, [updateInbox, fetchInbox, session]) |
There was a problem hiding this comment.
Potential race conditions and unnecessary refetches in useEffect hooks
Several issues need attention:
- The first useEffect combines multiple state updates which could lead to race conditions
- Using
updateInboxtoggle in dependencies could cause extra renders - The second useEffect might trigger unnecessary refetches
Consider this refactor:
useEffect(() => {
if (dateChanged && reschedulingItemId && date) {
- updateItem(session, { dueDate: date }, reschedulingItemId)
- setReschedulingItemId(null)
- setDateChanged(false)
- setUpdateInbox(!updateInbox)
+ updateItem(session, { dueDate: date }, reschedulingItemId).then(() => {
+ setReschedulingItemId(null)
+ setDateChanged(false)
+ fetchInbox(session)
+ })
}
- }, [date, updateItem, session, reschedulingItemId, dateChanged, updateInbox])
+ }, [date, updateItem, session, reschedulingItemId, dateChanged, fetchInbox])
- useEffect(() => {
- fetchInbox(session)
- }, [updateInbox, fetchInbox, session])This change:
- Ensures state updates happen after the API call completes
- Removes the need for the
updateInboxtoggle - Eliminates the second useEffect to prevent duplicate fetches
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| if (dateChanged && reschedulingItemId && date) { | |
| updateItem(session, { dueDate: date }, reschedulingItemId) | |
| setReschedulingItemId(null) | |
| setDateChanged(false) | |
| setUpdateInbox(!updateInbox) | |
| } | |
| }, [date, updateItem, session, reschedulingItemId, dateChanged, updateInbox]) | |
| useEffect(() => { | |
| fetchInbox(session) | |
| }, [updateInbox, fetchInbox, session]) | |
| useEffect(() => { | |
| if (dateChanged && reschedulingItemId && date) { | |
| updateItem(session, { dueDate: date }, reschedulingItemId).then(() => { | |
| setReschedulingItemId(null) | |
| setDateChanged(false) | |
| fetchInbox(session) | |
| }) | |
| } | |
| }, [date, updateItem, session, reschedulingItemId, dateChanged, fetchInbox]) |
| const handleRescheduleCalendar = ( | ||
| e: React.MouseEvent, | ||
| id: string, | ||
| dueDate: Date | string | null | ||
| ) => { | ||
| e.stopPropagation() | ||
|
|
||
| setReschedulingItemId(id) | ||
| setDate(dueDate) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance type safety and error handling in handleRescheduleCalendar
While the function works, it could benefit from improved type safety and validation.
Consider this enhancement:
const handleRescheduleCalendar = (
e: React.MouseEvent,
id: string,
- dueDate: Date | string | null
+ dueDate: Date | string | null,
) => {
e.stopPropagation()
+
+ // Ensure string dates are parsed correctly
+ const parsedDate = dueDate instanceof Date
+ ? dueDate
+ : dueDate
+ ? new Date(dueDate)
+ : new Date()
+
+ // Validate the parsed date
+ if (isNaN(parsedDate.getTime())) {
+ console.error('Invalid date provided')
+ return
+ }
setReschedulingItemId(id)
- setDate(dueDate)
+ setDate(parsedDate)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleRescheduleCalendar = ( | |
| e: React.MouseEvent, | |
| id: string, | |
| dueDate: Date | string | null | |
| ) => { | |
| e.stopPropagation() | |
| setReschedulingItemId(id) | |
| setDate(dueDate) | |
| } | |
| const handleRescheduleCalendar = ( | |
| e: React.MouseEvent, | |
| id: string, | |
| dueDate: Date | string | null, | |
| ) => { | |
| e.stopPropagation() | |
| // Ensure string dates are parsed correctly | |
| const parsedDate = dueDate instanceof Date | |
| ? dueDate | |
| : dueDate | |
| ? new Date(dueDate) | |
| : new Date() | |
| // Validate the parsed date | |
| if (isNaN(parsedDate.getTime())) { | |
| console.error('Invalid date provided') | |
| return | |
| } | |
| setReschedulingItemId(id) | |
| setDate(parsedDate) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
apps/frontend/src/components/Inbox/InboxItems.tsx(6 hunks)apps/frontend/src/lib/store/cycle.store.ts(2 hunks)
🔇 Additional comments (5)
apps/frontend/src/components/Inbox/InboxItems.tsx (4)
5-5: LGTM: State management and imports are well-structured.
The new imports and state variables are properly organized and typed for the rescheduling functionality.
Also applies to: 8-8, 21-25
57-63: Skipping comment as past review is still valid.
A previous review comment already addresses the race conditions and state management issues in this useEffect hook.
132-141: Skipping comment as past review is still valid.
A previous review comment already addresses the type safety improvements needed in this function.
210-232: Skipping comment as past review is still valid.
A previous review comment already addresses the accessibility improvements needed in the modal implementation.
apps/frontend/src/lib/store/cycle.store.ts (1)
422-425: Consider handling race conditions in optimistic updates.
While the implementation correctly updates the inbox view state, there's a potential race condition between the optimistic update and the server response. If multiple updates are made in quick succession, the final state might not reflect the correct order of operations.
Let's verify the potential for race conditions:
Consider these approaches to handle race conditions:
- Implement request queuing to serialize updates
- Track pending updates and merge them with server responses
- Add version numbers to items and validate them server-side
| onClick={(e) => | ||
| handleRescheduleCalendar(e, item._id, item.dueDate) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider memoizing the click handler for better performance.
The inline arrow function creates a new function instance on each render. Consider memoizing it with useCallback.
+ const handleCalendarClick = useCallback(
+ (id: string, dueDate: Date | string | null) => (e: React.MouseEvent) => {
+ handleRescheduleCalendar(e, id, dueDate)
+ },
+ [handleRescheduleCalendar]
+ )
// In JSX:
- onClick={(e) => handleRescheduleCalendar(e, item._id, item.dueDate)}
+ onClick={handleCalendarClick(item._id, item.dueDate)}Committable suggestion skipped: line range outside the PR's diff.
| const updateItemsInViewInbox = (items: CycleItem[]) => { | ||
| // remove item from inbox if dueDate is set (not null) | ||
| if (updates.dueDate) { | ||
| return items.filter((item) => item._id !== id) | ||
| } | ||
| // otherwise, just update the item in inbox | ||
| return items.map((item) => | ||
| item._id === id ? { ...item, ...updates } : item | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing type safety and edge cases in updateItemsInViewInbox.
The function correctly implements the inbox view update logic, removing items when a due date is set and updating them otherwise. However, consider these improvements:
- Add explicit handling for when
dueDateis set to null - Use type narrowing for better type safety
Consider this enhanced implementation:
const updateItemsInViewInbox = (items: CycleItem[]) => {
- // remove item from inbox if dueDate is set (not null)
- if (updates.dueDate) {
+ // Remove item from inbox if dueDate is set (not null)
+ if (updates.dueDate !== undefined && updates.dueDate !== null) {
return items.filter((item) => item._id !== id)
}
- // otherwise, just update the item in inbox
+ // Otherwise, just update the item in inbox
return items.map((item) =>
item._id === id ? { ...item, ...updates } : item
)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const updateItemsInViewInbox = (items: CycleItem[]) => { | |
| // remove item from inbox if dueDate is set (not null) | |
| if (updates.dueDate) { | |
| return items.filter((item) => item._id !== id) | |
| } | |
| // otherwise, just update the item in inbox | |
| return items.map((item) => | |
| item._id === id ? { ...item, ...updates } : item | |
| ) | |
| } | |
| const updateItemsInViewInbox = (items: CycleItem[]) => { | |
| // Remove item from inbox if dueDate is set (not null) | |
| if (updates.dueDate !== undefined && updates.dueDate !== null) { | |
| return items.filter((item) => item._id !== id) | |
| } | |
| // Otherwise, just update the item in inbox | |
| return items.map((item) => | |
| item._id === id ? { ...item, ...updates } : item | |
| ) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
apps/frontend/src/components/DropDownItems.tsx (2)
96-96: LGTM! Consider simplifying the null check.The date handling changes look good and properly handle the new Date object type. However, the null coalescing operator (??) is redundant since an empty string is already provided as a fallback.
- {getOverdueText(item.dueDate?.toISOString() || "") ?? ""} + {getOverdueText(item.dueDate?.toISOString() || "")}
Line range hint
1-1: Remove unused and incorrect import.The
formatimport from the 'path' module appears to be unused and incorrect. The 'path' module is for handling file paths, not date formatting.-import { format } from "path"If date formatting is needed, consider using a date formatting library like
date-fns:import { format } from 'date-fns'apps/frontend/src/components/Inbox/InboxItems.tsx (2)
132-141: Add date validation in handleRescheduleCalendar.The function should validate the dueDate parameter before setting it as state to prevent invalid dates.
Consider this enhancement:
const handleRescheduleCalendar = ( e: React.MouseEvent, id: string, dueDate: Date | null ) => { e.stopPropagation() + + // Validate date if provided + if (dueDate && isNaN(dueDate.getTime())) { + console.error('Invalid date provided') + return + } setReschedulingItemId(id) setDate(dueDate) }
210-232: Add focus management to the modal.The modal should manage focus properly to enhance user experience.
Consider using a focus trap library like
focus-trap-react:+ import { useFocusTrap } from '@focus-trap/react' // Inside component: + const focusTrap = useFocusTrap() + const [lastFocusedElement, setLastFocusedElement] = useState<HTMLElement | null>(null) const handleRescheduleCalendar = (e: React.MouseEvent, id: string, dueDate: Date | null) => { e.stopPropagation() + setLastFocusedElement(document.activeElement as HTMLElement) setReschedulingItemId(id) setDate(dueDate) } + const handleModalClose = () => { + setReschedulingItemId(null) + lastFocusedElement?.focus() + } // In JSX: {reschedulingItemId !== null && ( <div> <div className="fixed inset-0 z-50 cursor-default bg-black/80" role="button" - onClick={() => setReschedulingItemId(null)} + onClick={handleModalClose} onKeyDown={(e) => { if (e.key === "Escape" || e.key === "Esc") { - setReschedulingItemId(null) + handleModalClose() } }} tabIndex={0} /> - <div className="fixed left-1/2 top-1/2 z-50 -translate-x-1/2 -translate-y-1/2 rounded-md bg-background shadow-lg"> + <div + ref={focusTrap} + className="fixed left-1/2 top-1/2 z-50 -translate-x-1/2 -translate-y-1/2 rounded-md bg-background shadow-lg" + >apps/frontend/src/components/ThisWeek/CustomKanban.tsx (3)
Line range hint
79-94: LGTM! Consider adding error handling for date operations.The change from ISO string to Date object is correct and aligns with the interface updates. However, consider adding error handling for date operations to make the code more robust.
- const today = new Date() + try { + const today = new Date() + if (isNaN(today.getTime())) { + throw new Error('Invalid date') + } + } catch (error) { + console.error('Error creating date:', error) + // Fallback to current timestamp + const today = new Date() + }
286-286: LGTM! Consider adding error handling for date creation.The change from ISO string to Date object is correct. However, consider adding error handling for date creation to ensure robustness.
- data.dueDate = new Date() + try { + const date = new Date() + if (isNaN(date.getTime())) { + throw new Error('Invalid date') + } + data.dueDate = date + } catch (error) { + console.error('Error creating date:', error) + // Fallback to current timestamp + data.dueDate = new Date() + }
Line range hint
1-388: Consider extracting reusable components and hooks.The implementation is solid, but there are opportunities to improve code organization and reusability:
- The drag-and-drop logic could be extracted into a custom hook (e.g.,
useDragAndDrop)- The
CardandColumncomponents could be moved to separate files- Date handling logic could be centralized in a utility function
These changes would make the code more maintainable and easier to test.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
apps/frontend/src/components/DropDownItems.tsx(1 hunks)apps/frontend/src/components/Inbox/InboxItems.tsx(7 hunks)apps/frontend/src/components/ThisWeek/CustomKanban.tsx(2 hunks)apps/frontend/src/lib/@types/Items/Cycle.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/frontend/src/lib/@types/Items/Cycle.ts
| useEffect(() => { | ||
| if (dateChanged && reschedulingItemId && date) { | ||
| updateItem(session, { dueDate: date, status: "todo" }, reschedulingItemId) | ||
| setReschedulingItemId(null) | ||
| setDateChanged(false) | ||
| } | ||
| }, [date, updateItem, session, reschedulingItemId, dateChanged]) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to the date update effect.
The useEffect should handle potential failures in the updateItem call to prevent silent failures and provide feedback to users.
Consider this enhancement:
useEffect(() => {
if (dateChanged && reschedulingItemId && date) {
- updateItem(session, { dueDate: date, status: "todo" }, reschedulingItemId)
- setReschedulingItemId(null)
- setDateChanged(false)
+ updateItem(session, { dueDate: date, status: "todo" }, reschedulingItemId)
+ .then(() => {
+ setReschedulingItemId(null)
+ setDateChanged(false)
+ })
+ .catch((error) => {
+ // Handle error appropriately (e.g., show error toast)
+ console.error('Failed to update date:', error)
+ })
}
}, [date, updateItem, session, reschedulingItemId, dateChanged])Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
apps/frontend/src/components/DropDownItems.tsx(2 hunks)apps/frontend/src/utils/datetime.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/frontend/src/components/DropDownItems.tsx
🔇 Additional comments (1)
apps/frontend/src/utils/datetime.ts (1)
88-88: Verify the impact of the breaking change.
The parameter type change from string | null to Date | null is a breaking change that requires updates to all callers.
Let's verify all usages of this function:
| export const getOverdueText = (dueDate: Date | null): string => { | ||
| if (!dueDate) { | ||
| return "" // or some default text like "No due date" | ||
| } | ||
|
|
||
| const due = new Date(dueDate) | ||
| const now = new Date() | ||
| const diffInDays = differenceInDays(now, due) | ||
| const diffInDays = differenceInDays(now, dueDate) | ||
|
|
||
| return `since ${diffInDays} ${diffInDays === 1 ? "day" : "days"}` |
There was a problem hiding this comment.
Fix overdue calculation and improve clarity.
The current implementation has several issues:
- The overdue calculation
differenceInDays(now, dueDate)returns positive days for overdue items, which is counterintuitive. Users typically expect negative numbers for overdue items. - The text "since X days" doesn't clearly indicate if an item is overdue or due in the future.
Consider this improved implementation:
export const getOverdueText = (dueDate: Date | null): string => {
if (!dueDate) {
- return "" // or some default text like "No due date"
+ return "No due date"
}
const now = new Date()
- const diffInDays = differenceInDays(now, dueDate)
+ const diffInDays = differenceInDays(dueDate, now)
- return `since ${diffInDays} ${diffInDays === 1 ? "day" : "days"}`
+ if (diffInDays < 0) {
+ const days = Math.abs(diffInDays)
+ return `overdue by ${days} ${days === 1 ? "day" : "days"}`
+ }
+ if (diffInDays === 0) {
+ return "due today"
+ }
+ return `due in ${diffInDays} ${diffInDays === 1 ? "day" : "days"}`
}This implementation:
- Returns negative days for overdue items by swapping the order of dates in
differenceInDays - Provides clear text indicating whether items are overdue or upcoming
- Handles the special case of items due today
- Uses a more meaningful default text for null dates
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const getOverdueText = (dueDate: Date | null): string => { | |
| if (!dueDate) { | |
| return "" // or some default text like "No due date" | |
| } | |
| const due = new Date(dueDate) | |
| const now = new Date() | |
| const diffInDays = differenceInDays(now, due) | |
| const diffInDays = differenceInDays(now, dueDate) | |
| return `since ${diffInDays} ${diffInDays === 1 ? "day" : "days"}` | |
| export const getOverdueText = (dueDate: Date | null): string => { | |
| if (!dueDate) { | |
| return "No due date" | |
| } | |
| const now = new Date() | |
| const diffInDays = differenceInDays(dueDate, now) | |
| if (diffInDays < 0) { | |
| const days = Math.abs(diffInDays) | |
| return `overdue by ${days} ${days === 1 ? "day" : "days"}` | |
| } | |
| if (diffInDays === 0) { | |
| return "due today" | |
| } | |
| return `due in ${diffInDays} ${diffInDays === 1 ? "day" : "days"}` | |
| } |
No description provided.