Conversation
🦋 Changeset detectedLatest commit: bfdb0ff The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Caution Review failedThe pull request is closed. WalkthroughPublic API and internal types were unified around a new exported Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Modal as DatesheetModal
participant Timeline as DatesheetTimeline
participant Store as useDatesheet
rect rgba(230,245,255,0.9)
note right of Store: Exposes DateEntry[], addDate, editDate(index,...), removeDate(index)
end
User->>Modal: Open modal (reads dates)
Modal->>Store: get dates (DateEntry[])
Modal->>Modal: render items with uniqueId `${date.date}-${date.name}-${index}`
User->>Modal: Open item -> Modal passes currentDate & currentIndex to Form
alt Add
User->>Modal: Submit new DateEntry
Modal->>Store: addDate(entry)
Store->>Store: append & sort by date
else Edit
User->>Modal: Submit edited DateEntry
Modal->>Store: editDate(index, entry)
Store->>Store: replace at index & sort
else Delete
User->>Modal: Delete entry
Modal->>Store: removeDate(index)
Store->>Store: remove at index & sort
end
Store-->>Modal: updated dates
Store-->>Timeline: updated dates consumed
Timeline->>Timeline: renderItem(date, index) with composite key
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
akshat-OwO
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (12)
src/hooks/use-datesheet.ts (4)
4-7: Prefer stable IDs over index-based identityIndex-based edit/remove is brittle under re-sorting and concurrent mutations. Add a stable id on DateEntry to key items and target edits/removals by id.
export type DateEntry = { + id: string; name: string; date: number; };Outside this hunk (for context only):
- Generate id on add/import:
{ id: crypto.randomUUID(), ...date }.- Switch edit/remove signatures to accept
id: stringinstead ofindex: number.
33-35: Sorting after append is fine; duplicates now allowed—confirm intentPrevious uniqueness checks are gone; identical name/date pairs will be stored. If that’s not desired, add a dedupe guard.
38-45: Guard against out-of-bounds index in editDatePrevent sparse arrays or silent no-ops when index is invalid.
editDate: (index, newDate) => { set((state) => { const newDates = [...state.dates]; - newDates[index] = newDate; + if (index < 0 || index >= newDates.length) return { dates: newDates }; + newDates[index] = newDate; newDates.sort((a, b) => a.date - b.date); return { dates: newDates }; }); },
46-51: Guard against out-of-bounds index in removeDateAvoid accidental negative-index splices and clarify behavior.
removeDate: (index) => { set((state) => { const newDates = [...state.dates]; - newDates.splice(index, 1); + if (index >= 0 && index < newDates.length) { + newDates.splice(index, 1); + } return { dates: newDates }; }); },src/components/DatesheetTimeline.tsx (2)
33-38: Compute “now” once per renderMinor perf/consistency: reuse a single timestamp to avoid tiny intra-render mismatches.
export const DatesheetTimeline: React.FC<DatesheetTimelineProps> = ({ dates, interactive = false, renderItem, }) => { - return ( + const now = Date.now(); + return ( ... - "bg-primary": d.date > new Date().getTime(), + "bg-primary": d.date > now,(Apply the same replacement to other occurrences in this component.)
Also applies to: 41-45, 51-55, 63-67
89-95: Typo: text-muted-foregroundFix class name to apply the intended color.
-<span className="text-muted-foregound text-xs font-medium"> +<span className="text-muted-foreground text-xs font-medium">src/components/modals/datesheet-modal.tsx (6)
150-157: Avoid coupling to list index for editsPassing both currentDate and currentIndex can desync after re-sorts. Prefer targeting by stable id on DateEntry.
175-181: Index guard in edit submitIf index is ever undefined, no-op silently. Consider an else path to surface an error or disable Edit when index is absent.
256-263: Ensure Delete button does not submit the formExplicitly set type="button".
-<Button +<Button + type="button" variant="outline" className="flex-1" onClick={() => handleDelete()} >
314-319: Typo: text-muted-foregroundFix class name so the date subtitle has the right color.
-<span className="text-muted-foregound text-xs font-medium"> +<span className="text-muted-foreground text-xs font-medium">
281-283: Open-popover state and re-sortsBecause openPopoverId is tied to a value including index, any list mutation/re-sort will close the popover unexpectedly. Prefer a stable id for both key and openPopoverId.
Also applies to: 289-297
107-145: Optional: wire Sheet onOpenChange to the boolean valueCurrent handler always closes. If Sheet can be opened from the UI, consider accepting the boolean.
-<Sheet open={isOpen} onOpenChange={onClose}> +<Sheet open={isOpen} onOpenChange={(open) => (open ? undefined : onClose())}>Or expose
setOpen(open: boolean)in the store and pass it directly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/DatesheetTimeline.tsx(3 hunks)src/components/modals/datesheet-modal.tsx(8 hunks)src/hooks/use-datesheet.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/components/modals/datesheet-modal.tsx (3)
src/hooks/use-datesheet.ts (1)
DateEntry(4-7)src/components/DatesheetTimeline.tsx (1)
DatesheetTimeline(16-110)src/lib/utils.ts (1)
cn(8-10)
src/components/DatesheetTimeline.tsx (1)
src/hooks/use-datesheet.ts (1)
DateEntry(4-7)
🔇 Additional comments (2)
src/components/modals/datesheet-modal.tsx (2)
88-91: LGTM: button layout tweakMoving width styling to the Button simplifies the layout.
1-347: Fix typo: replace "text-muted-foregound" → "text-muted-foreground"Found in:
- src/components/DatesheetTimeline.tsx:89
- src/components/modals/datesheet-modal.tsx:314
Repo-wide scans for key={index} usages and div-as-trigger checks failed due to ripgrep errors (unrecognized file type "tsx" and PCRE2 pattern error) — re-run those checks to verify there are no problematic keys or trigger elements.
| <div | ||
| key={`${d.date}-${d.name}-${index}`} | ||
| className="flex items-center" | ||
| > |
There was a problem hiding this comment.
Avoid index in React keys
Including index makes keys unstable across re-ordering and can remount items unexpectedly. Use a stable id (preferred) or fall back to content-only composite if ids are not yet available.
- <div
- key={`${d.date}-${d.name}-${index}`}
- className="flex items-center"
- >
+ <div
+ key={`${d.date}-${d.name}`}
+ className="flex items-center"
+ >Best: key={d.id} once DateEntry carries an id.
📝 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.
| <div | |
| key={`${d.date}-${d.name}-${index}`} | |
| className="flex items-center" | |
| > | |
| <div | |
| key={`${d.date}-${d.name}`} | |
| className="flex items-center" | |
| > |
🤖 Prompt for AI Agents
In src/components/DatesheetTimeline.tsx around lines 25 to 28, the key uses the
array index which can cause unstable reconciliation; replace it with a stable
identifier: modify the DateEntry data shape to include an id and use key={d.id}.
If adding id isn't yet possible, use a stable composite that doesn't include the
index such as key={`${d.date}-${d.name}`} to avoid index-based keys.
| const [currDate, setCurrDate] = useState<DateEntry | undefined>( | ||
| currentDate | ||
| ); | ||
| const form = useForm<TDatesheetSchema>({ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused currDate state; rely on react-hook-form
currDate isn’t read and the DateTimePicker’s setCurrDate does nothing user-visible. Simplify and avoid stale state.
-const [currDate, setCurrDate] = useState<DateEntry | undefined>(currentDate);
const form = useForm<TDatesheetSchema>({
resolver: zodResolver(datesheetSchema),
defaultValues: {
- name: currDate?.name || "",
- date: currDate?.date ? new Date(currDate.date) : new Date(),
+ name: currentDate?.name || "",
+ date: currentDate?.date ? new Date(currentDate.date) : new Date(),
},
});
...
-<DateTimePicker
+<DateTimePicker
hourCycle={12}
value={field.value}
- onChange={(e) => {
- setCurrDate((prev) => {
- if (prev) {
- const timestamp = e?.getTime();
- if (timestamp) {
- return { ...prev, date: timestamp };
- }
- }
- return prev;
- });
- field.onChange(e);
- }}
+ onChange={field.onChange}
/>Also applies to: 166-168, 220-239
🤖 Prompt for AI Agents
In src/components/modals/datesheet-modal.tsx around lines 160-163 (and also
touching 166-168 and 220-239), remove the unused currDate useState and any
setCurrDate calls and instead rely on react-hook-form for the date value;
initialize the form with currentDate as the defaultValue for the date field,
replace any local state updates with form.setValue or the Controller on the
DateTimePicker so the picker reads/writes the RHF field directly, and remove
related unused variables and handlers so there is no stale duplicate state.
| renderItem={(date, index) => { | ||
| const uniqueId = `${date.date}-${date.name}-${index}`; | ||
| return ( | ||
| <Popover | ||
| key={uniqueId} | ||
| open={openPopoverId === uniqueId} | ||
| onOpenChange={(isOpen) => | ||
| setOpenPopoverId(isOpen ? uniqueId : null) | ||
| } | ||
| > | ||
| <PopoverTrigger asChild> | ||
| <div | ||
| className={cn( | ||
| "group h-fit w-full whitespace-normal rounded-md border py-1.5 pl-3 pr-2", | ||
| { | ||
| "cursor-pointer transition-colors hover:bg-secondary/50": | ||
| true, | ||
| } | ||
| )} | ||
| > | ||
| <div className="flex flex-1 items-center justify-between"> | ||
| <div className="flex flex-col"> | ||
| <p className="line-clamp-1 text-sm font-semibold"> | ||
| {date.name} | ||
| </p> | ||
| <span className="text-muted-foregound text-xs font-medium"> | ||
| {format( | ||
| new Date(date.date), | ||
| "do MMMM yyyy hh:mm aaa" | ||
| )} | ||
| </span> | ||
| </div> | ||
| <div className="relative inline-flex items-center justify-center rounded-md border border-input p-1.5"> | ||
| <CalendarIcon className="h-8 w-8" /> | ||
| <span className="pointer-events-none absolute bottom-2.5 left-1/2 -translate-x-1/2 text-xs font-semibold"> | ||
| {new Date(date.date).getDate()} | ||
| </span> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </PopoverTrigger> | ||
| <PopoverContent align="start"> | ||
| <DatesheetModal.Form | ||
| isEditForm | ||
| currentDate={{ | ||
| name: date.name, | ||
| date: date.date, | ||
| }} | ||
| onFormAction={() => setOpenPopoverId(null)} | ||
| /> | ||
| </PopoverContent> | ||
| </Popover> | ||
| )} | ||
| </PopoverTrigger> | ||
| <PopoverContent align="start"> | ||
| <DatesheetModal.Form | ||
| isEditForm | ||
| currentDate={date} | ||
| currentIndex={index} | ||
| onFormAction={() => setOpenPopoverId(null)} | ||
| /> | ||
| </PopoverContent> | ||
| </Popover> | ||
| ); | ||
| }} |
There was a problem hiding this comment.
Popover trigger should be an interactive element; stabilize identity
- Accessibility: Using a div as PopoverTrigger breaks keyboard navigation. Use a button or add appropriate role/tabIndex/keypress handlers.
- Identity: uniqueId includes index; it will change after re-sorts, breaking open state. Prefer a stable id.
-<PopoverTrigger asChild>
- <div
+<PopoverTrigger asChild>
+ <button
+ type="button"
className={cn(
"group h-fit w-full whitespace-normal rounded-md border py-1.5 pl-3 pr-2",
{ "cursor-pointer transition-colors hover:bg-secondary/50": true }
)}
+ aria-haspopup="dialog"
>
...
- </div>
+ </button>
</PopoverTrigger>And, if/when DateEntry has id:
-const uniqueId = `${date.date}-${date.name}-${index}`;
+const uniqueId = date.id;📝 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.
| renderItem={(date, index) => { | |
| const uniqueId = `${date.date}-${date.name}-${index}`; | |
| return ( | |
| <Popover | |
| key={uniqueId} | |
| open={openPopoverId === uniqueId} | |
| onOpenChange={(isOpen) => | |
| setOpenPopoverId(isOpen ? uniqueId : null) | |
| } | |
| > | |
| <PopoverTrigger asChild> | |
| <div | |
| className={cn( | |
| "group h-fit w-full whitespace-normal rounded-md border py-1.5 pl-3 pr-2", | |
| { | |
| "cursor-pointer transition-colors hover:bg-secondary/50": | |
| true, | |
| } | |
| )} | |
| > | |
| <div className="flex flex-1 items-center justify-between"> | |
| <div className="flex flex-col"> | |
| <p className="line-clamp-1 text-sm font-semibold"> | |
| {date.name} | |
| </p> | |
| <span className="text-muted-foregound text-xs font-medium"> | |
| {format( | |
| new Date(date.date), | |
| "do MMMM yyyy hh:mm aaa" | |
| )} | |
| </span> | |
| </div> | |
| <div className="relative inline-flex items-center justify-center rounded-md border border-input p-1.5"> | |
| <CalendarIcon className="h-8 w-8" /> | |
| <span className="pointer-events-none absolute bottom-2.5 left-1/2 -translate-x-1/2 text-xs font-semibold"> | |
| {new Date(date.date).getDate()} | |
| </span> | |
| </div> | |
| </div> | |
| </div> | |
| </div> | |
| </PopoverTrigger> | |
| <PopoverContent align="start"> | |
| <DatesheetModal.Form | |
| isEditForm | |
| currentDate={{ | |
| name: date.name, | |
| date: date.date, | |
| }} | |
| onFormAction={() => setOpenPopoverId(null)} | |
| /> | |
| </PopoverContent> | |
| </Popover> | |
| )} | |
| </PopoverTrigger> | |
| <PopoverContent align="start"> | |
| <DatesheetModal.Form | |
| isEditForm | |
| currentDate={date} | |
| currentIndex={index} | |
| onFormAction={() => setOpenPopoverId(null)} | |
| /> | |
| </PopoverContent> | |
| </Popover> | |
| ); | |
| }} | |
| renderItem={(date, index) => { | |
| const uniqueId = date.id; | |
| return ( | |
| <Popover | |
| key={uniqueId} | |
| open={openPopoverId === uniqueId} | |
| onOpenChange={(isOpen) => | |
| setOpenPopoverId(isOpen ? uniqueId : null) | |
| } | |
| > | |
| <PopoverTrigger asChild> | |
| <button | |
| type="button" | |
| className={cn( | |
| "group h-fit w-full whitespace-normal rounded-md border py-1.5 pl-3 pr-2", | |
| { "cursor-pointer transition-colors hover:bg-secondary/50": true } | |
| )} | |
| aria-haspopup="dialog" | |
| > | |
| <div className="flex flex-1 items-center justify-between"> | |
| <div className="flex flex-col"> | |
| <p className="line-clamp-1 text-sm font-semibold"> | |
| {date.name} | |
| </p> | |
| <span className="text-muted-foregound text-xs font-medium"> | |
| {format( | |
| new Date(date.date), | |
| "do MMMM yyyy hh:mm aaa" | |
| )} | |
| </span> | |
| </div> | |
| <div className="relative inline-flex items-center justify-center rounded-md border border-input p-1.5"> | |
| <CalendarIcon className="h-8 w-8" /> | |
| <span className="pointer-events-none absolute bottom-2.5 left-1/2 -translate-x-1/2 text-xs font-semibold"> | |
| {new Date(date.date).getDate()} | |
| </span> | |
| </div> | |
| </div> | |
| </button> | |
| </PopoverTrigger> | |
| <PopoverContent align="start"> | |
| <DatesheetModal.Form | |
| isEditForm | |
| currentDate={date} | |
| currentIndex={index} | |
| onFormAction={() => setOpenPopoverId(null)} | |
| /> | |
| </PopoverContent> | |
| </Popover> | |
| ); | |
| }} |
🤖 Prompt for AI Agents
In src/components/modals/datesheet-modal.tsx around lines 289 to 340, the
PopoverTrigger currently uses a div and the uniqueId includes the mutable index;
replace the div trigger with a real interactive element (a <button
type="button">) preserving the same classes and asChild usage so keyboard
focus/activation works, and make the stable id/key use a persistent identifier
(prefer date.id if available, otherwise use a deterministic composite like
`${date.date}-${date.name}` without the index); update key, openPopoverId
assignment/comparison, and setOpenPopoverId to use that stableId so open state
doesn't break on reorders.
| dates: DateEntry[]; | ||
| addDate: (date: { name: string; date: number }) => void; | ||
| editDate: ( | ||
| date: { name: string; date: number }, | ||
| newDate: { name: string; date: number } | ||
| ) => void; | ||
| removeDate: (date: { name: string; date: number }) => void; | ||
| editDate: (index: number, newDate: { name: string; date: number }) => void; | ||
| removeDate: (index: number) => void; | ||
| importDatesheet: (dates: Array<{ name: string; date: number }>) => void; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unify function signatures on DateEntry
Use the exported DateEntry type consistently; also prefer DateEntry[] for import.
dates: DateEntry[];
- addDate: (date: { name: string; date: number }) => void;
- editDate: (index: number, newDate: { name: string; date: number }) => void;
- removeDate: (index: number) => void;
- importDatesheet: (dates: Array<{ name: string; date: number }>) => void;
+ addDate: (date: DateEntry | Omit<DateEntry, "id">) => void;
+ editDate: (index: number, newDate: DateEntry | Omit<DateEntry, "id">) => void;
+ removeDate: (index: number) => void;
+ importDatesheet: (dates: DateEntry[]) => void;If you add ids as suggested above, change edit/remove to (id: string, newDate: DateEntry) and (id: string).
📝 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.
| dates: DateEntry[]; | |
| addDate: (date: { name: string; date: number }) => void; | |
| editDate: ( | |
| date: { name: string; date: number }, | |
| newDate: { name: string; date: number } | |
| ) => void; | |
| removeDate: (date: { name: string; date: number }) => void; | |
| editDate: (index: number, newDate: { name: string; date: number }) => void; | |
| removeDate: (index: number) => void; | |
| importDatesheet: (dates: Array<{ name: string; date: number }>) => void; | |
| }; | |
| dates: DateEntry[]; | |
| addDate: (date: DateEntry | Omit<DateEntry, "id">) => void; | |
| editDate: (index: number, newDate: DateEntry | Omit<DateEntry, "id">) => void; | |
| removeDate: (index: number) => void; | |
| importDatesheet: (dates: DateEntry[]) => void; | |
| }; |
🤖 Prompt for AI Agents
In src/hooks/use-datesheet.ts around lines 15 to 20, the exported DateEntry type
should be used consistently in all function signatures: change addDate to accept
a DateEntry, change importDatesheet to accept DateEntry[] instead of
Array<{...}>, and (if you add ids to DateEntry as suggested) change editDate and
removeDate to use the id signature: editDate(id: string, newDate: DateEntry) and
removeDate(id: string). Update any call sites and types accordingly to match
these signatures.
2 participants
Closing #152 issue
Summary by CodeRabbit
Refactor
Bug Fixes
Style