fix: cancel event button for each booking on bookings page#23479
fix: cancel event button for each booking on bookings page#23479anikdhabal merged 5 commits intocalcom:mainfrom
Conversation
|
@ShashwatPS is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe change updates apps/web/components/booking/BookingListItem.tsx so that the Cancel dropdown item does not set an href when the action is disabled. Specifically, href is now conditional: href={cancelEventAction.disabled ? undefined : cancelEventAction.href}. Other props, including onClick and disabled, are unchanged. No exports or public APIs were altered. Assessment against linked issues
Possibly related PRs
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
github-actions
bot
added
Low priority
dosubot
bot
added
the
bookings
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
packages/ui/components/dropdown/Dropdown.tsx (1)
148-159: Prevent accidental form submissions: default button type="button".When
isLinkis false, this renders a<button>without an explicittype, which defaults tosubmitinside forms. Add an explicittype="button"and avoid passingdisabledonto<a>.Apply:
- const ButtonOrLink = isLink ? "a" : "button"; - - const content = <ButtonOrLink {...props} />; + // Avoid passing `disabled` to <a>, and ensure buttons don't submit forms. + const { disabled, ...restProps } = props; + const content = isLink + ? <a {...restProps} /> + : <button {...props} type={props.type ?? "button"} />;
🧹 Nitpick comments (6)
packages/ui/components/dropdown/Dropdown.tsx (6)
165-171: Improve disabled styling to truly “mute” and block hovers.Current classes only set
disabled:cursor-not-allowed; hover styles still apply visually. Add pointer-event and hover overrides.- className={classNames( - "hover:text-emphasis text-default inline-flex w/full items-center space-x-1 rounded-lg p-2 disabled:cursor-not-allowed", + className={classNames( + "hover:text-emphasis text-default inline-flex w-full items-center space-x-1 rounded-lg p-2 disabled:cursor-not-allowed disabled:pointer-events-none disabled:opacity-50 disabled:hover:bg-transparent disabled:hover:text-default", color === "destructive" ? "hover:bg-error hover:text-error text-error" : "hover:bg-subtle", props.className )}
144-159: Optional: modernize Link usage.If/when you drop
legacyBehaviorproject-wide, render the final element directly as Link’s child-free API and pass props to Link instead of<a>. Non-blocking here.
62-71: Type correctness: use Item props, not CheckboxItem.
DropdownMenuItemrendersDropdownMenuPrimitive.Itembut its props type isCheckboxItem. Align the types to avoid subtle prop mismatches.-type DropdownMenuItemProps = ComponentProps<(typeof DropdownMenuPrimitive)["CheckboxItem"]>; +type DropdownMenuItemProps = ComponentProps<(typeof DropdownMenuPrimitive)["Item"]>;
88-105: Localize a11y labels (“Active/Not active”).Per repo guidelines, wrap user-facing strings with
t():Active/Not activeshould be localized.Example (pseudo):
aria-label={typeof children === "string" ? t("notActive", { label: children }) : undefined}aria-label={typeof children === "string" ? t("active", { label: children }) : undefined}
197-197: Prefer named exports over default.Guideline: avoid default exports. Consider removing
export default Dropdown;and rely on the named export.-export default Dropdown; +// Prefer named export; update imports accordingly where used.
161-181: Add an automated test to lock this behavior.A lightweight RTL test: render
DropdownItemwith{href:"/x", disabled:true}and assert it renders a<button disabled>and not an<a>, and that hover styles don’t change. I can draft it if helpful.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/ui/components/dropdown/Dropdown.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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/ui/components/dropdown/Dropdown.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/ui/components/dropdown/Dropdown.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/ui/components/dropdown/Dropdown.tsx
🔇 Additional comments (2)
packages/ui/components/dropdown/Dropdown.tsx (2)
145-156: Good fix: don’t render a link when disabled.Gating
isLinkwith!props.disabledresolves the UI/UX bug where disabled items withhrefwere still navigable. This aligns the UI with server-side cancellation rules.
145-146: No usages ofDropdownItemorButtonOrLinkwith bothhrefanddisabledfound; removing disabled‐link behavior is safe.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/web/components/booking/bookingActions.ts (1)
66-66: Avoid appending seatReferenceUid=undefined; compute once and guard.If
getSeatReferenceUid()returnsundefinedwhileseatsReferences.lengthis truthy, the URL will containseatReferenceUid=undefined. Compute once and append only if defined.- const isDisabled = isActionDisabled("cancel", context); + const isDisabled = isActionDisabled("cancel", context); + const seatRefUid = getSeatReferenceUid(); - href: isDisabled - ? undefined - : `/booking/${booking.uid}?cancel=true${ - isTabRecurring && isRecurring ? "&allRemainingBookings=true" : "" - }${booking.seatsReferences.length ? `&seatReferenceUid=${getSeatReferenceUid()}` : ""}`, + href: isDisabled + ? undefined + : `/booking/${booking.uid}?cancel=true${ + isTabRecurring && isRecurring ? "&allRemainingBookings=true" : "" + }${seatRefUid ? `&seatReferenceUid=${seatRefUid}` : ""}`,Also applies to: 71-75
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/web/components/booking/bookingActions.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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/bookingActions.ts
**/*.{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/booking/bookingActions.ts
**/*.{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/booking/bookingActions.ts
⏰ 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). (7)
- GitHub Check: Production builds / Build Web App
- GitHub Check: Production builds / Build API v2
- GitHub Check: Production builds / Build Atoms
- GitHub Check: Production builds / Build API v1
- GitHub Check: Tests / Unit
- GitHub Check: Linters / lint
- GitHub Check: Type check / check-types
🔇 Additional comments (2)
apps/web/components/booking/bookingActions.ts (2)
66-66: Good fix: cancel is now truly disabled (no link + disabled state).Using
isActionDisabled("cancel", ...)and droppinghrefwhen disabled prevents navigation and aligns UI with server constraints. Looks correct and matches the stated objective.Also applies to: 71-75, 78-78
66-66: VerifyDropdownItemlink detection andActionType.hrefoptionality
Confirm that inDropdownItem’sisLinklogic you’re gatinghrefby!props.disabled, and thatActionTypedefineshrefas an optional field (href?: string).
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 (1)
apps/web/components/booking/BookingListItem.tsx (1)
691-699: Standardize disabled+href logic across cancel entry points. In BookingListItem.tsx, the dropdown guard (href={cancelEventAction.disabled ? undefined : cancelEventAction.href}) is needed because neither DropdownItem nor TableActions strip href whendisabled. Apply the same pattern to the<TableActions actions={[cancelEventAction]}>call (or update TableActions to unsethrefwhendisabled) so disabled cancel actions never render as active links.
🧹 Nitpick comments (1)
apps/web/components/booking/BookingListItem.tsx (1)
691-699: Also gate onClick when disabled (defense in depth).Even though disabled should block interaction, explicitly removing the handler avoids edge cases and keeps semantics clear.
- <DropdownItem + <DropdownItem type="button" color={cancelEventAction.color} StartIcon={cancelEventAction.icon} - href={cancelEventAction.disabled ? undefined : cancelEventAction.href} - onClick={cancelEventAction.onClick} + href={cancelEventAction.disabled ? undefined : cancelEventAction.href} + onClick={cancelEventAction.disabled ? undefined : cancelEventAction.onClick} disabled={cancelEventAction.disabled} data-bookingid={cancelEventAction.bookingId} data-testid={cancelEventAction.id} className={cancelEventAction.disabled ? "text-muted" : undefined}>
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/web/components/booking/BookingListItem.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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/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/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/booking/BookingListItem.tsx
🧠 Learnings (1)
📓 Common learnings
Learnt from: supalarry
PR: calcom/cal.com#23364
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/transformers/internal-to-api/internal-to-api.spec.ts:295-296
Timestamp: 2025-08-27T13:32:46.887Z
Learning: In calcom/cal.com, when transforming booking fields from internal to API format, tests in organizations-event-types.e2e-spec.ts already expect name field label and placeholder to be empty strings ("") rather than undefined. PR changes that set these to explicit empty strings are typically fixing implementation to match existing test expectations rather than breaking changes.
⏰ 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: Install dependencies / Yarn install & cache
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (1)
apps/web/components/booking/BookingListItem.tsx (1)
694-694: LGTM: preventing navigation when disabled.Setting href to undefined when the action is disabled avoids accidental navigation and matches the objective.
E2E results are ready! |
2 participants
What does this PR do?
Booking cancellation is already handled internally when the
Disable Cancellingoption is enabled in the advanced setting ( as shown in the screenshot ) . However, the cancel button was still appearing, even when the prop to disable it was being passed to the component.The issue was traced to the
DropdownItemcomponent, which was not correctly respecting thedisabledprop for elements that also had anhrefattribute. A small change was made to fix this.This change ensures the component respects the
disabledprop, which mutes the button's text and makes it unclickable.Visual Demo (After Fix)
https://www.loom.com/share/cc2f3ead2a884e6c827c7f1e1aba5886?sid=2f0a0f3c-c755-43c6-ad5d-c5bff28c18f0
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Checklist