refactor: booking-list-state-management#24047
Conversation
Walkthrough
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
|
@hemantmm is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/platform/atoms/src/components/ui/dialog.tsx (1)
54-71: Wire upenableOverflowand avoid hardcoded "Close" (add prop).
enableOverflowis declared but unused. Also, the "Close" label should be localized or configurable.Apply this diff:
const DialogContent = React.forwardRef< React.ElementRef<typeof DialogPrimitives.Content>, - React.ComponentPropsWithoutRef<typeof DialogPrimitives.Content> & { enableOverflow?: boolean } ->(({ className, children, ...props }, ref) => ( + React.ComponentPropsWithoutRef<typeof DialogPrimitives.Content> & { + enableOverflow?: boolean; + closeAriaLabel?: string; + } +>(({ className, children, enableOverflow, closeAriaLabel, ...props }, ref) => ( <> <DialogPortal> <div className="calcom-atoms"> <DialogOverlay /> <DialogPrimitives.Content ref={ref} className={cn( - "bg-background data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 data-[state=closed]:zoom-out-95 data-[state=open]:zoom-in-95 data-[state=closed]:slide-out-to-left-1/2 data-[state=closed]:slide-out-to-top-[48%] data-[state=open]:slide-in-from-left-1/2 data-[state=open]:slide-in-from-top-[48%] fixed left-[50%] top-[50%] z-50 grid w-full max-w-lg translate-x-[-50%] translate-y-[-50%] gap-4 border p-6 shadow-lg duration-200 sm:rounded-lg", + "bg-background data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 data-[state=closed]:zoom-out-95 data-[state=open]:zoom-in-95 data-[state=closed]:slide-out-to-left-1/2 data-[state=closed]:slide-out-to-top-[48%] data-[state=open]:slide-in-from-left-1/2 data-[state=open]:slide-in-from-top-[48%] fixed left-[50%] top-[50%] z-50 grid w-full max-w-lg translate-x-[-50%] translate-y-[-50%] gap-4 border p-6 shadow-lg duration-200 sm:rounded-lg", + enableOverflow && "max-h-[85vh] overflow-y-auto", className )} {...props}> {children} <DialogPrimitives.Close className="ring-offset-background focus:ring-ring data-[state=open]:bg-accent data-[state=open]:text-muted-foreground absolute right-4 top-4 rounded-sm opacity-70 transition-opacity hover:opacity-100 focus:outline-none focus:ring-2 focus:ring-offset-2 disabled:pointer-events-none"> <Icon name="x" className="h-4 w-4" /> - <span className="sr-only">Close</span> + <span className="sr-only">{closeAriaLabel ?? "Close"}</span> </DialogPrimitives.Close> </DialogPrimitives.Content> </div> </DialogPortal> </> ));apps/web/components/booking/BookingListItem.tsx (1)
291-297: Bug: Closing Edit Location dialog uses a dead local setter.
setIsOpenLocationDialog(false)is disconnected from the actual dialog; the dialog won’t close after save.- const [, setIsOpenLocationDialog] = useState(false); - // const [setIsOpenLocationDialog] = useState(false); const setLocationMutation = trpc.viewer.bookings.editLocation.useMutation({ onSuccess: () => { showToast(t("location_updated"), "success"); - setIsOpenLocationDialog(false); + closeDialog("editLocation"); utils.viewer.bookings.invalidate(); },
🧹 Nitpick comments (5)
apps/web/components/dialog/ChargeCardDialog.tsx (1)
25-31: Localize the success toast message.Avoid hardcoded strings; use
t().- showToast("Charge successful", "success"); + showToast(t("charge_successful"), "success");apps/web/components/booking/hooks/useBookingItemState.ts (2)
1-1: ImportuseCallbackfor stable handlers.-import { useState } from "react"; +import { useState, useCallback } from "react";
3-14: DRY initial state and stabilize handlers (useCallback).Prevents duplication drift and reduces needless re-renders when passing callbacks.
export type DialogState = { reschedule: boolean; reassign: boolean; editLocation: boolean; addGuests: boolean; chargeCard: boolean; viewRecordings: boolean; meetingSessionDetails: boolean; noShowAttendees: boolean; rejection: boolean; reroute: boolean; }; +const initialDialogState: DialogState = { + reschedule: false, + reassign: false, + editLocation: false, + addGuests: false, + chargeCard: false, + viewRecordings: false, + meetingSessionDetails: false, + noShowAttendees: false, + rejection: false, + reroute: false, +}; + export function useBookingItemState() { const [rejectionReason, setRejectionReason] = useState<string>(""); - const [dialogState, setDialogState] = useState<DialogState>({ - reschedule: false, - reassign: false, - editLocation: false, - addGuests: false, - chargeCard: false, - viewRecordings: false, - meetingSessionDetails: false, - noShowAttendees: false, - rejection: false, - reroute: false, - }); + const [dialogState, setDialogState] = useState<DialogState>(initialDialogState); - const openDialog = (dialog: keyof DialogState) => { + const openDialog = useCallback((dialog: keyof DialogState) => { setDialogState((prevState) => ({ ...prevState, [dialog]: true, })); - }; + }, []); - const closeDialog = (dialog: keyof DialogState) => { + const closeDialog = useCallback((dialog: keyof DialogState) => { setDialogState((prevState) => ({ ...prevState, [dialog]: false, })); - }; + }, []); - const toggleDialog = (dialog: keyof DialogState) => { + const toggleDialog = useCallback((dialog: keyof DialogState) => { setDialogState((prevState) => ({ ...prevState, [dialog]: !prevState[dialog], })); - }; + }, []); // Helper function to reset all dialogs (useful when one action should close others) - const resetDialogs = () => { - setDialogState({ - reschedule: false, - reassign: false, - editLocation: false, - addGuests: false, - chargeCard: false, - viewRecordings: false, - meetingSessionDetails: false, - noShowAttendees: false, - rejection: false, - reroute: false, - }); - }; + const resetDialogs = useCallback(() => { + setDialogState(initialDialogState); + }, []);Also applies to: 16-29, 31-50, 52-66, 68-77
apps/web/components/booking/BookingListItem.tsx (2)
825-827: Avoid string-based date comparisons in recurring tooltip.Comparing
toString()is brittle; compare timestamps.- !booking.recurringInfo?.bookings[BookingStatus.CANCELLED] - .map((date: { toString: () => any }) => date.toString()) - .includes(recurringDate.toString()) + !booking.recurringInfo?.bookings[BookingStatus.CANCELLED].some( + (date) => new Date(date).getTime() === recurringDate.getTime() + )- booking.recurringInfo?.bookings[BookingStatus.CANCELLED] - .map((date: { toString: () => any }) => date.toString()) - .includes(aDate.toString()); + booking.recurringInfo?.bookings[BookingStatus.CANCELLED].some( + (date) => new Date(date).getTime() === aDate.getTime() + );Also applies to: 843-845
472-477: Localize "(Optional)".Hardcoded text should use
t().- label={ - <> - {t("rejection_reason")} - <span className="text-subtle font-normal"> (Optional)</span> - </> - } + label={ + <> + {t("rejection_reason")} + <span className="text-subtle font-normal"> ({t("optional")})</span> + </> + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
apps/web/components/booking/BookingListItem.tsx(15 hunks)apps/web/components/booking/hooks/useBookingItemState.ts(1 hunks)apps/web/components/dialog/ChargeCardDialog.tsx(1 hunks)packages/platform/atoms/src/components/ui/dialog.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
apps/web/components/dialog/ChargeCardDialog.tsxpackages/platform/atoms/src/components/ui/dialog.tsxapps/web/components/booking/BookingListItem.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
apps/web/components/dialog/ChargeCardDialog.tsxpackages/platform/atoms/src/components/ui/dialog.tsxapps/web/components/booking/hooks/useBookingItemState.tsapps/web/components/booking/BookingListItem.tsx
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
apps/web/components/dialog/ChargeCardDialog.tsxpackages/platform/atoms/src/components/ui/dialog.tsxapps/web/components/booking/hooks/useBookingItemState.tsapps/web/components/booking/BookingListItem.tsx
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
apps/web/components/booking/hooks/useBookingItemState.ts
🧠 Learnings (1)
📚 Learning: 2025-09-08T09:34:46.661Z
Learnt from: CarinaWolli
PR: calcom/cal.com#23421
File: packages/features/ee/workflows/components/AddActionDialog.tsx:171-174
Timestamp: 2025-09-08T09:34:46.661Z
Learning: In packages/features/ee/workflows/components/AddActionDialog.tsx, the actionOptions prop is guaranteed to never be empty according to maintainer CarinaWolli, so defensive checks for empty arrays are not necessary.
Applied to files:
apps/web/components/booking/BookingListItem.tsx
🧬 Code graph analysis (1)
apps/web/components/booking/BookingListItem.tsx (4)
apps/web/components/booking/hooks/useBookingItemState.ts (1)
useBookingItemState(16-77)apps/web/components/dialog/ReassignDialog.tsx (1)
ReassignDialog(63-328)apps/web/components/dialog/ChargeCardDialog.tsx (1)
ChargeCardDialog(19-79)packages/features/ee/video/MeetingSessionDetailsDialog.tsx (1)
MeetingSessionDetailsDialog(172-201)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Type check / check-types
- GitHub Check: Tests / Unit
🔇 Additional comments (3)
apps/web/components/booking/BookingListItem.tsx (2)
133-135: Nice state centralization.Replacing scattered
useStatewithuseBookingItemStateimproves cohesion and testability.
159-164: Close rejection dialog on success: good.This aligns the mutation side-effect with the new state model.
apps/web/components/dialog/ChargeCardDialog.tsx (1)
11-14: Prop rename and setter signature change look good; callsites updated.
ChargeCardDialog now expects (isOpen: boolean) => void; apps/web/components/booking/BookingListItem.tsx passes a matching (open) => (open ? openDialog("chargeCard") : closeDialog("chargeCard")). No other ChargeCardDialog usages found.
| {isBookingFromRoutingForm && ( | ||
| <RerouteDialog | ||
| isOpenDialog={rerouteDialogIsOpen} | ||
| setIsOpenDialog={setRerouteDialogIsOpen} | ||
| isOpenDialog={dialogState.reroute} | ||
| setIsOpenDialog={() => closeDialog("reroute")} | ||
| booking={{ ...parsedBooking, eventType: parsedBooking.eventType }} | ||
| /> | ||
| )} |
There was a problem hiding this comment.
RerouteDialog setter ignores the boolean param.
Align with other dialogs to handle both open and close paths.
- <RerouteDialog
- isOpenDialog={dialogState.reroute}
- setIsOpenDialog={() => closeDialog("reroute")}
- booking={{ ...parsedBooking, eventType: parsedBooking.eventType }}
- />
+ <RerouteDialog
+ isOpenDialog={dialogState.reroute}
+ setIsOpenDialog={(open) => (open ? openDialog("reroute") : closeDialog("reroute"))}
+ booking={{ ...parsedBooking, eventType: parsedBooking.eventType }}
+ />📝 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.
| {isBookingFromRoutingForm && ( | |
| <RerouteDialog | |
| isOpenDialog={rerouteDialogIsOpen} | |
| setIsOpenDialog={setRerouteDialogIsOpen} | |
| isOpenDialog={dialogState.reroute} | |
| setIsOpenDialog={() => closeDialog("reroute")} | |
| booking={{ ...parsedBooking, eventType: parsedBooking.eventType }} | |
| /> | |
| )} | |
| {isBookingFromRoutingForm && ( | |
| <RerouteDialog | |
| isOpenDialog={dialogState.reroute} | |
| setIsOpenDialog={(open) => (open ? openDialog("reroute") : closeDialog("reroute"))} | |
| booking={{ ...parsedBooking, eventType: parsedBooking.eventType }} | |
| /> | |
| )} |
🤖 Prompt for AI Agents
In apps/web/components/booking/BookingListItem.tsx around lines 730-736, the
RerouteDialog prop setIsOpenDialog currently ignores the boolean parameter;
change it to accept the boolean and forward that value to the dialog state
updater (i.e., call the existing closeDialog or dialog setter with the dialog
name and the boolean, or directly set the reroute boolean on dialog state) so
the handler supports both open and close paths consistent with the other
dialogs.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/platform/atoms/src/components/ui/dialog.tsx (2)
54-67: Do not forward unknownenableOverflowprop to DOM; also implement its behavior.
enableOverflowis being passed through toDialogPrimitives.Content, which likely renders a DOM node. React will warn about unknown DOM attributes. Destructure and use it to toggle overflow classes; don’t spread it to the DOM.Apply this diff:
-const DialogContent = React.forwardRef< - React.ElementRef<typeof DialogPrimitives.Content>, - React.ComponentPropsWithoutRef<typeof DialogPrimitives.Content> & { enableOverflow?: boolean } ->(({ className, children, ...props }, ref) => ( +const DialogContent = React.forwardRef< + React.ElementRef<typeof DialogPrimitives.Content>, + React.ComponentPropsWithoutRef<typeof DialogPrimitives.Content> & { + enableOverflow?: boolean; + closeLabel?: string; + } +>(({ className, children, enableOverflow, closeLabel, ...props }, ref) => ( <> <DialogPortal> <div className="calcom-atoms"> <DialogOverlay /> <DialogPrimitives.Content ref={ref} className={cn( - "bg-background data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 data-[state=closed]:zoom-out-95 data-[state=open]:zoom-in-95 data-[state=closed]:slide-out-to-left-1/2 data-[state=closed]:slide-out-to-top-[48%] data-[state=open]:slide-in-from-left-1/2 data-[state=open]:slide-in-from-top-[48%] fixed left-[50%] top-[50%] z-50 grid w-full max-w-lg translate-x-[-50%] translate-y-[-50%] gap-4 border p-6 shadow-lg duration-200 sm:rounded-lg", + "bg-background data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 data-[state=closed]:zoom-out-95 data-[state=open]:zoom-in-95 data-[state=closed]:slide-out-to-left-1/2 data-[state=closed]:slide-out-to-top-[48%] data-[state=open]:slide-in-from-left-1/2 data-[state=open]:slide-in-from-top-[48%] fixed left-[50%] top-[50%] z-50 grid w-full max-w-lg translate-x-[-50%] translate-y-[-50%] gap-4 border p-6 shadow-lg duration-200 sm:rounded-lg", + enableOverflow ? "overflow-visible" : "overflow-auto", className )} {...props}> {children} <DialogPrimitives.Close className="ring-offset-background focus:ring-ring data-[state=open]:bg-accent data-[state=open]:text-muted-foreground absolute right-4 top-4 rounded-sm opacity-70 transition-opacity hover:opacity-100 focus:outline-none focus:ring-2 focus:ring-offset-2 disabled:pointer-events-none"> <Icon name="x" className="h-4 w-4" /> - <span className="sr-only">Close</span> + <span className="sr-only">{closeLabel ?? "Close"}</span> </DialogPrimitives.Close> </DialogPrimitives.Content> </div> </DialogPortal> </> ));
84-90: Prevent leakingnoStickyto DOM (unknown prop).
DialogFootercurrently accepts arbitrary div props and will forwardnoStickyto the DOM (see usage in BookingListItem). Destructure and drop it.Apply this diff:
-const DialogFooter = ({ className, ...props }: React.HTMLAttributes<HTMLDivElement>) => ( +const DialogFooter = ( + { className, noSticky, ...props }: React.HTMLAttributes<HTMLDivElement> & { noSticky?: boolean } +) => ( <div className={cn("flex flex-col-reverse sm:flex-row sm:justify-end sm:space-x-2", className)} {...props} /> );apps/web/components/booking/BookingListItem.tsx (2)
291-299: Bug: dialog won’t close after location update (stale local setter).
setIsOpenLocationDialog(false)only updates an unused local state. The actual dialog is controlled bydialogState.editLocation. CallcloseDialog("editLocation")instead and remove the local state.Apply this diff:
- const [, setIsOpenLocationDialog] = useState(false); - // const [setIsOpenLocationDialog] = useState(false); const setLocationMutation = trpc.viewer.bookings.editLocation.useMutation({ onSuccess: () => { showToast(t("location_updated"), "success"); - setIsOpenLocationDialog(false); + closeDialog("editLocation"); utils.viewer.bookings.invalidate(); },
331-336: Possible runtime error when recurring arrays are undefined.Calling
.concaton a possibly undefined array will throw. Use nullish coalescing.Apply this diff:
- const recurringDates = booking.recurringInfo?.bookings[BookingStatus.ACCEPTED] - .concat(booking.recurringInfo?.bookings[BookingStatus.CANCELLED]) - .concat(booking.recurringInfo?.bookings[BookingStatus.PENDING]) - .sort((date1: Date, date2: Date) => date1.getTime() - date2.getTime()); + const recurringDates = [ + ...(booking.recurringInfo?.bookings[BookingStatus.ACCEPTED] ?? []), + ...(booking.recurringInfo?.bookings[BookingStatus.CANCELLED] ?? []), + ...(booking.recurringInfo?.bookings[BookingStatus.PENDING] ?? []), + ].sort((d1: Date, d2: Date) => d1.getTime() - d2.getTime());
🧹 Nitpick comments (5)
packages/platform/atoms/src/components/ui/dialog.tsx (1)
68-71: Localize accessible “Close” label.Per guidelines, avoid hardcoded text. With the diff above, pass a localized
closeLabelfrom callers (e.g., t("close")).apps/web/components/dialog/ChargeCardDialog.tsx (1)
26-31: Localize success toast string.Replace hardcoded “Charge successful” with a localized string.
Apply this diff:
- showToast("Charge successful", "success"); + showToast(t("charge_successful"), "success");Ensure the i18n key exists.
apps/web/components/booking/hooks/useBookingItemState.ts (1)
31-51: Memoize handlers to reduce re-renders in children.
openDialog/closeDialog/toggleDialogare recreated on each render; if passed to many children, wrap withuseCallback.apps/web/components/booking/BookingListItem.tsx (2)
474-476: Localize “(Optional)”.Avoid hardcoded text; use a localized label or compose it via i18n.
Apply this diff:
- <span className="text-subtle font-normal"> (Optional)</span> + <span className="text-subtle font-normal"> ({t("optional")})</span>Ensure the key exists.
825-827: Use time values instead oftoString()for date comparisons.Stringifying dates is locale-dependent and brittle. Compare timestamps.
Apply this diff:
- !booking.recurringInfo?.bookings[BookingStatus.CANCELLED] - .map((date: { toString: () => any }) => date.toString()) - .includes(recurringDate.toString()) + !new Set( + (booking.recurringInfo?.bookings[BookingStatus.CANCELLED] ?? []).map((d: Date) => d.getTime()) + ).has(recurringDate.getTime())- booking.recurringInfo?.bookings[BookingStatus.CANCELLED] - .map((date: { toString: () => any }) => date.toString()) - .includes(aDate.toString()); + new Set( + (booking.recurringInfo?.bookings[BookingStatus.CANCELLED] ?? []).map((d: Date) => d.getTime()) + ).has(aDate.getTime());Also update the temporary types to
Date.Also applies to: 843-845
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
apps/web/components/booking/BookingListItem.tsx(15 hunks)apps/web/components/booking/hooks/useBookingItemState.ts(1 hunks)apps/web/components/dialog/ChargeCardDialog.tsx(1 hunks)packages/platform/atoms/src/components/ui/dialog.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Always use
t()for text localization in frontend code; direct text embedding should trigger a warning
Files:
packages/platform/atoms/src/components/ui/dialog.tsxapps/web/components/booking/BookingListItem.tsxapps/web/components/dialog/ChargeCardDialog.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/platform/atoms/src/components/ui/dialog.tsxapps/web/components/booking/hooks/useBookingItemState.tsapps/web/components/booking/BookingListItem.tsxapps/web/components/dialog/ChargeCardDialog.tsx
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/platform/atoms/src/components/ui/dialog.tsxapps/web/components/booking/hooks/useBookingItemState.tsapps/web/components/booking/BookingListItem.tsxapps/web/components/dialog/ChargeCardDialog.tsx
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
apps/web/components/booking/hooks/useBookingItemState.ts
🧠 Learnings (1)
📚 Learning: 2025-09-08T09:34:46.661Z
Learnt from: CarinaWolli
PR: calcom/cal.com#23421
File: packages/features/ee/workflows/components/AddActionDialog.tsx:171-174
Timestamp: 2025-09-08T09:34:46.661Z
Learning: In packages/features/ee/workflows/components/AddActionDialog.tsx, the actionOptions prop is guaranteed to never be empty according to maintainer CarinaWolli, so defensive checks for empty arrays are not necessary.
Applied to files:
apps/web/components/booking/BookingListItem.tsx
🧬 Code graph analysis (1)
apps/web/components/booking/BookingListItem.tsx (4)
apps/web/components/booking/hooks/useBookingItemState.ts (1)
useBookingItemState(16-77)apps/web/components/dialog/ReassignDialog.tsx (1)
ReassignDialog(63-328)apps/web/components/dialog/ChargeCardDialog.tsx (1)
ChargeCardDialog(19-79)packages/features/ee/video/MeetingSessionDetailsDialog.tsx (1)
MeetingSessionDetailsDialog(172-201)
🔇 Additional comments (3)
apps/web/components/booking/hooks/useBookingItemState.ts (1)
16-29: Centralized dialog state: good direction.API is clear and replaces scattered useState effectively.
Consider if only one dialog should be open at a time. If yes, we could store
activeDialog: keyof DialogState | nullinstead of many booleans, or haveopenDialogauto-close others.apps/web/components/booking/BookingListItem.tsx (2)
133-135: Nice adoption of the centralized hook.Clear migration away from scattered state.
465-468: Unknown prop leakage:noStickyon DialogFooter (from atoms).This will become an unknown DOM attribute unless atoms drops it. After applying the atoms fix to destructure
noSticky, this is fine.
|
@anikdhabal, can you review the PR? |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 4 files
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="packages/platform/atoms/src/components/ui/dialog.tsx">
<violation number="1" location="packages/platform/atoms/src/components/ui/dialog.tsx:54">
Adding enableOverflow to the public props but leaving the signature as ({ className, children, ...props }) forwards the flag straight to DialogPrimitives.Content without any handling, so consumers get neither overflow behavior nor a safe DOM attribute. Please implement the overflow behavior (or strip the prop) before exposing it.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| const DialogContent = React.forwardRef< | ||
| React.ElementRef<typeof DialogPrimitives.Content>, | ||
| React.ComponentPropsWithoutRef<typeof DialogPrimitives.Content> | ||
| React.ComponentPropsWithoutRef<typeof DialogPrimitives.Content> & { enableOverflow?: boolean } |
There was a problem hiding this comment.
Adding enableOverflow to the public props but leaving the signature as ({ className, children, ...props }) forwards the flag straight to DialogPrimitives.Content without any handling, so consumers get neither overflow behavior nor a safe DOM attribute. Please implement the overflow behavior (or strip the prop) before exposing it.
Prompt for AI agents
Address the following comment on packages/platform/atoms/src/components/ui/dialog.tsx at line 54:
<comment>Adding enableOverflow to the public props but leaving the signature as ({ className, children, ...props }) forwards the flag straight to DialogPrimitives.Content without any handling, so consumers get neither overflow behavior nor a safe DOM attribute. Please implement the overflow behavior (or strip the prop) before exposing it.</comment>
<file context>
@@ -51,7 +51,7 @@ DialogOverlay.displayName = DialogPrimitives.Overlay.displayName;
const DialogContent = React.forwardRef<
React.ElementRef<typeof DialogPrimitives.Content>,
- React.ComponentPropsWithoutRef<typeof DialogPrimitives.Content>
+ React.ComponentPropsWithoutRef<typeof DialogPrimitives.Content> & { enableOverflow?: boolean }
>(({ className, children, ...props }, ref) => (
<>
</file context>
dhairyashiil
left a comment
dhairyashiil
left a comment
There was a problem hiding this comment.
can you resolve merge conflicts? making it draft until then
There was a problem hiding this comment.
1 issue found across 4 files
Prompt for AI agents (all 1 issues)
Understand the root cause of the following 1 issues and fix them.
<file name="packages/platform/atoms/src/components/ui/dialog.tsx">
<violation number="1" location="packages/platform/atoms/src/components/ui/dialog.tsx:54">
Adding the enableOverflow prop without consuming it breaks parity with the existing DialogContent—dialogs that previously scrolled now overflow because the prop is forwarded instead of toggling overflow classes. Please handle enableOverflow in the atoms implementation the same way as before (e.g., toggle overflow-y-auto) or drop the prop entirely.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| const DialogContent = React.forwardRef< | ||
| React.ElementRef<typeof DialogPrimitives.Content>, | ||
| React.ComponentPropsWithoutRef<typeof DialogPrimitives.Content> | ||
| React.ComponentPropsWithoutRef<typeof DialogPrimitives.Content> & { enableOverflow?: boolean } |
There was a problem hiding this comment.
Adding the enableOverflow prop without consuming it breaks parity with the existing DialogContent—dialogs that previously scrolled now overflow because the prop is forwarded instead of toggling overflow classes. Please handle enableOverflow in the atoms implementation the same way as before (e.g., toggle overflow-y-auto) or drop the prop entirely.
Prompt for AI agents
Address the following comment on packages/platform/atoms/src/components/ui/dialog.tsx at line 54:
<comment>Adding the enableOverflow prop without consuming it breaks parity with the existing DialogContent—dialogs that previously scrolled now overflow because the prop is forwarded instead of toggling overflow classes. Please handle enableOverflow in the atoms implementation the same way as before (e.g., toggle overflow-y-auto) or drop the prop entirely.</comment>
<file context>
@@ -51,7 +51,7 @@ DialogOverlay.displayName = DialogPrimitives.Overlay.displayName;
const DialogContent = React.forwardRef<
React.ElementRef<typeof DialogPrimitives.Content>,
- React.ComponentPropsWithoutRef<typeof DialogPrimitives.Content>
+ React.ComponentPropsWithoutRef<typeof DialogPrimitives.Content> & { enableOverflow?: boolean }
>(({ className, children, ...props }, ref) => (
<>
</file context>
pallava-joshi
left a comment
pallava-joshi
left a comment
There was a problem hiding this comment.
please fix the failing type checks and merge conflicts
|
also attach a loom video or a screenshot. marking this PR draft until then. |
3 participants
closes: #22214
What does this PR do?
✅ Extracted useDialogState hook
✅ Extracted useBookingFormState hook
✅ Replaced scattered useState calls
✅ Improved separation of concerns
✅ Enhanced testability
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Checklist