feat: Add email input dialog to /booking/uid page #22542
Conversation
|
@iharsh02 is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the """ WalkthroughThe changes introduce an email verification step before allowing booking cancellation or viewing booking details when the current user is not authenticated and no valid email parameter is present in the URL. The Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes were found. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes ✨ 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
|
|
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Details: |
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (07/16/25)1 reviewer was added to this PR based on Keith Williams's automation. "Add community label" took an action on this PR • (07/16/25)1 label was added to this PR based on Keith Williams's automation. |
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
apps/web/modules/bookings/views/bookings-single-view.tsx (1)
865-872: Avoid unsafe cast & ensure fallbacks for optional fields
bookingInfo?.user?.emailis optional. Casting it tostringhides theundefinedcase at compile-time but the runtime value can still beundefined, which will break the new e-mail-verification flow insideCancelBooking.
Likewise,bookingInfo?.attendeescan beundefined, yetCancelBookingpresumably iterates over it.- attendees: bookingInfo?.attendees, - userEmail: bookingInfo?.user?.email as string, + attendees: bookingInfo.attendees ?? [], + userEmail: + bookingInfo?.user?.email ?? + bookingInfo?.userPrimaryEmail ?? + "", // empty string handled inside CancelBookingAdjust the
CancelBookingprop types if they currently mark these fields as required.
This prevents runtime errors and keeps the types honest.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/web/components/booking/CancelBooking.tsx(6 hunks)apps/web/modules/bookings/views/bookings-single-view.tsx(1 hunks)apps/web/playwright/booking-seats.e2e.ts(2 hunks)apps/web/public/static/locales/en/common.json(1 hunks)packages/features/schedules/components/Schedule.tsx(0 hunks)
💤 Files with no reviewable changes (1)
- packages/features/schedules/components/Schedule.tsx
🧰 Additional context used
🧠 Learnings (3)
apps/web/modules/bookings/views/bookings-single-view.tsx (1)
Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.341Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
apps/web/public/static/locales/en/common.json (1)
undefined
<retrieved_learning>
Learnt from: bandhan-majumder
PR: #22359
File: packages/lib/server/locales/en/common.json:1336-1339
Timestamp: 2025-07-14T16:31:45.201Z
Learning: When making localization changes for new features, it's often safer to add new strings rather than modify existing ones to avoid breaking existing functionality that depends on the original strings. This approach allows for feature-specific customization while maintaining backward compatibility.
</retrieved_learning>
apps/web/components/booking/CancelBooking.tsx (1)
Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.341Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
🧬 Code Graph Analysis (1)
apps/web/components/booking/CancelBooking.tsx (1)
packages/ui/components/form/index.ts (1)
Input(21-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Install dependencies / Yarn install & cache
- GitHub Check: Security Check
🔇 Additional comments (5)
apps/web/public/static/locales/en/common.json (1)
2504-2506: Colon alignment & style inconsistency—remove leading spaces before:Across this i18n file, key-value pairs are written without a space before the colon.
The two new entriesproceed_with_cancellation_descriptionandproceed_with_cancellation_errorintroduce a leading space before the colon, breaking the prevailing style and creating noisy diffs later.-"proceed_with_cancellation_description" : "Please enter the email address used for this booking to proceed with cancellation", -"proceed_with_cancellation_error" : "Email doesn't match the booking email", +"proceed_with_cancellation_description": "Please enter the email address used for this booking to proceed with cancellation", +"proceed_with_cancellation_error": "Email doesn't match the booking email",[ suggest_nitpick ]
apps/web/playwright/booking-seats.e2e.ts (1)
127-170: LGTM! Test correctly implements the new email verification flow.The test properly handles the email verification requirement by:
- Destructuring the
userobject to access the email- Following the correct sequence: confirm → verify email → submit
This aligns with the new
CancelBookingcomponent behavior.apps/web/components/booking/CancelBooking.tsx (3)
12-13: Imports and type updates look good.The Dialog components and Input are correctly imported, and the booking type additions support the email verification functionality.
Also applies to: 80-81
124-126: Well-structured refactoring with proper state management.The extraction of cancellation logic into
handleCancellationimproves code organization. TheverifyAndCancelfunction correctly implements conditional verification based on whethercurrentUserEmailis available (logged-in users skip verification).Also applies to: 137-183
286-324: Well-implemented verification dialog.The Dialog component is properly structured with:
- Appropriate test IDs that match the e2e tests
- Clear error handling and display
- Proper state cleanup on cancel
- Accessible form inputs
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
apps/web/modules/bookings/views/bookings-single-view.tsx (1)
423-432: Consider enhancing email obfuscation for better privacy.The current implementation might reveal too much information for short email usernames. Consider:
- Ensuring minimum obfuscation even for short usernames
- Partially obfuscating the domain for additional privacy
const obfuscateEmail = (email: string) => { if (!email || !email.includes("@")) return obfuscate(email, 2); const [user, domain] = email.split("@"); - return `${obfuscate(user, 2)}@${domain}`; + // Ensure at least 3 dots for privacy + const obfuscatedUser = user.length <= 2 ? user[0] + "•••" : obfuscate(user, 2); + // Optionally obfuscate domain for additional privacy + const [domainName, ...domainParts] = domain.split("."); + const obfuscatedDomain = domainParts.length > 0 + ? `${obfuscate(domainName, 2)}.${domainParts.join(".")}` + : domain; + return `${obfuscatedUser}@${obfuscatedDomain}`; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/web/components/booking/CancelBooking.tsx(4 hunks)apps/web/modules/bookings/views/bookings-single-view.tsx(9 hunks)apps/web/playwright/booking-seats.e2e.ts(5 hunks)apps/web/public/static/locales/en/common.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/web/public/static/locales/en/common.json
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/playwright/booking-seats.e2e.ts
- apps/web/components/booking/CancelBooking.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code. Functions like .add, .diff, .isBefore, and .isAfter are slow, especially in timezone mode. Prefer .utc() for better performance. Where possible, replace with native Date and direct .valueOf() comparisons for faster execution. Recommend using native methods or Day.js .utc() consistently in hot paths like loops.
Files:
apps/web/modules/bookings/views/bookings-single-view.tsx
🧠 Learnings (2)
📓 Common learnings
Learnt from: Anshumancanrock
PR: calcom/cal.com#22570
File: apps/web/modules/signup-view.tsx:253-253
Timestamp: 2025-07-21T21:33:23.371Z
Learning: In signup-view.tsx, when checking if redirectUrl contains certain strings, using explicit && checks (redirectUrl && redirectUrl.includes()) is preferred over optional chaining (redirectUrl?.includes()) to ensure the result is always a boolean rather than potentially undefined. This approach provides cleaner boolean contracts for downstream conditional logic.
Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.389Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
apps/web/modules/bookings/views/bookings-single-view.tsx (2)
Learnt from: eunjae-lee
PR: #22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.389Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
Learnt from: Anshumancanrock
PR: #22570
File: apps/web/modules/signup-view.tsx:253-253
Timestamp: 2025-07-21T21:33:23.371Z
Learning: In signup-view.tsx, when checking if redirectUrl contains certain strings, using explicit && checks (redirectUrl && redirectUrl.includes()) is preferred over optional chaining (redirectUrl?.includes()) to ensure the result is always a boolean rather than potentially undefined. This approach provides cleaner boolean contracts for downstream conditional logic.
⏰ Context from checks skipped due to timeout of 90000ms. 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 (7)
apps/web/modules/bookings/views/bookings-single-view.tsx (7)
8-8: LGTM! Import statements are appropriate for the new functionality.The added hooks (useCallback, useMemo) and UI components (Dialog, Input) are correctly imported for implementing the email verification feature.
Also applies to: 53-53, 56-56
86-89: Well-structured email validation schema.Good use of Zod for input validation with clear error messages.
158-158: Proper state management for authentication and verification flow.Good addition of session status and well-organized state variables for the email verification dialog.
Also applies to: 198-201
434-442: Well-implemented email validation logic.Good use of useCallback for performance optimization and proper case-insensitive email comparison.
455-462: Clean implementation of URL parameter update.Good use of router.replace with scroll preservation.
464-484: Robust email verification handler with proper error handling.Good use of try-finally to ensure loading state cleanup. The validation flow is well-structured.
704-737: Comprehensive PII protection implementation.Excellent consistent application of obfuscation across all sensitive fields including names, emails, and phone numbers.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
apps/web/modules/bookings/views/bookings-single-view.tsx (5)
434-442: Email validation logic is correctly implemented.The case-insensitive email comparison and proper memoization with
bookingInfodependency are appropriate.
444-453: Effect logic is correct but has dependency issues.The verification dialog display logic properly checks session state and email validity.
486-493: PII visibility logic is correct but has duplication.The logic properly determines when to show personally identifiable information based on session or valid email.
963-964: Props correctly passed with type safety concern.The attendees prop is properly passed. The userEmail type casting issue has been previously identified.
1240-1240: Good improvements but missing loading state.The dialog now uses translations for the title, but the verify button still needs the loading prop as suggested in the past review.
Also applies to: 1257-1259
🧹 Nitpick comments (1)
apps/web/modules/bookings/views/bookings-single-view.tsx (1)
423-426: Good implementation with minor improvement needed.The obfuscation function works well, but consider using
Math.minto prevent potential issues whenvisibleCharsexceeds string length:const obfuscate = (value: string, visibleChars = 1) => { if (!value) return ""; - return value.substring(0, visibleChars) + "•".repeat(Math.max(0, value.length - visibleChars)); + const chars = Math.min(visibleChars, value.length); + return value.substring(0, chars) + "•".repeat(Math.max(0, value.length - chars)); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/modules/bookings/views/bookings-single-view.tsx(9 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code. Functions like .add, .diff, .isBefore, and .isAfter are slow, especially in timezone mode. Prefer .utc() for better performance. Where possible, replace with native Date and direct .valueOf() comparisons for faster execution. Recommend using native methods or Day.js .utc() consistently in hot paths like loops.
Files:
apps/web/modules/bookings/views/bookings-single-view.tsx
🧠 Learnings (2)
📓 Common learnings
Learnt from: Anshumancanrock
PR: calcom/cal.com#22570
File: apps/web/modules/signup-view.tsx:253-253
Timestamp: 2025-07-21T21:33:23.371Z
Learning: In signup-view.tsx, when checking if redirectUrl contains certain strings, using explicit && checks (redirectUrl && redirectUrl.includes()) is preferred over optional chaining (redirectUrl?.includes()) to ensure the result is always a boolean rather than potentially undefined. This approach provides cleaner boolean contracts for downstream conditional logic.
Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.389Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
apps/web/modules/bookings/views/bookings-single-view.tsx (5)
Learnt from: eunjae-lee
PR: #22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.389Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
Learnt from: Anshumancanrock
PR: #22570
File: apps/web/modules/signup-view.tsx:253-253
Timestamp: 2025-07-21T21:33:23.371Z
Learning: In signup-view.tsx, when checking if redirectUrl contains certain strings, using explicit && checks (redirectUrl && redirectUrl.includes()) is preferred over optional chaining (redirectUrl?.includes()) to ensure the result is always a boolean rather than potentially undefined. This approach provides cleaner boolean contracts for downstream conditional logic.
Learnt from: alishaz-polymath
PR: #22304
File: packages/features/eventtypes/components/MultiplePrivateLinksController.tsx:92-94
Timestamp: 2025-07-16T06:42:27.024Z
Learning: In the MultiplePrivateLinksController component (packages/features/eventtypes/components/MultiplePrivateLinksController.tsx), the currentLink.maxUsageCount ?? 1 fallback in the openSettingsDialog function is intentional. Missing maxUsageCount values indicate old/legacy private links that existed before the expiration feature was added, and they should default to single-use behavior (1) for backward compatibility.
Learnt from: eunjae-lee
PR: #22702
File: packages/lib/server/service/insightsBooking.ts:120-124
Timestamp: 2025-07-24T08:39:06.185Z
Learning: In the InsightsBookingService (packages/lib/server/service/insightsBooking.ts), the constructor stores null for invalid options or filters but this is handled safely through null checks in buildFilterConditions() and buildAuthorizationConditions() methods. The service uses defensive programming to return safe fallback conditions (null or NOTHING_CONDITION) rather than throwing errors on invalid inputs.
Learnt from: CR
PR: calcom/cal.com#0
File: .cursor/rules/review.mdc:0-0
Timestamp: 2025-07-21T13:54:11.770Z
Learning: Applies to frontend/**/*.{js,jsx,ts,tsx} : Always use t() for text localization in frontend code; direct text embedding should trigger a warning.
⏰ Context from checks skipped due to timeout of 90000ms. 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 (10)
apps/web/modules/bookings/views/bookings-single-view.tsx (10)
8-8: LGTM!The addition of
useCallback,useEffect,useMemo, anduseStatehooks is appropriate for the new email verification functionality.
53-53: LGTM!The Dialog and Input components are correctly imported for the email verification dialog implementation.
Also applies to: 56-56
86-88: LGTM!The email validation schema with proper error messages is well-defined and follows best practices for form validation.
158-158: LGTM!Good addition of
sessionStatusto prevent race conditions when checking session state.
198-201: LGTM!The email verification state variables are properly initialized with appropriate types and default values.
428-432: LGTM!The email obfuscation preserves the domain for better user recognition while protecting the username portion.
455-462: LGTM!The URL update logic correctly preserves existing parameters and prevents unwanted scrolling.
464-484: LGTM!The verification handler properly validates input, manages loading state, and provides clear error feedback.
701-714: LGTM!Host information is properly obfuscated when PII cannot be shown, maintaining UI consistency while protecting privacy.
719-736: LGTM!Attendee information obfuscation is consistently applied across all PII fields (name, phone, email).
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
apps/web/modules/bookings/views/bookings-single-view.tsx (4)
444-454: Well-structured dialog visibility logic.The effect correctly handles the loading state and uses functional setState to optimize re-renders. The logic appropriately shows the verification dialog only when needed.
486-494: Efficient PII visibility computation.Good use of
useMemowith proper dependencies. The logic correctly determines when to show personally identifiable information.
963-965: Props correctly updated for CancelBooking.The additional props are properly passed to support the email verification flow in the cancellation process.
1231-1263: Fix critical issues in dialog implementation.There are several issues that need to be addressed:
- Line 1234 has a critical bug -
setShowVerificationDialogis not being called- Dialog title should use translation instead of hardcoded text
- Missing loading state on the verify button
Apply these fixes:
<Dialog open={showVerificationDialog} onOpenChange={() => { - setShowVerificationDialog; + // Prevent closing - handled by onPointerDownOutside and onEscapeKeyDown }} data-testid="verify-email-dialog"> <DialogContent onPointerDownOutside={(e) => e.preventDefault()} onEscapeKeyDown={(e) => e.preventDefault()}> - <DialogHeader title="Verify your email" /> + <DialogHeader title={t("verify_email")} /> <div className="space-y-4 py-4"> <p className="text-default text-sm">{t("verification_email_dialog_description")}</p> <Input data-testid="verify-email-input" type="email" placeholder={t("verification_email_input_placeholder")} value={verificationEmail} onChange={(e) => setVerificationEmail(e.target.value)} className="mb-2" /> {verificationError && ( <p data-testid="verify-email-error" className="text-error text-sm"> {verificationError} </p> )} <div className="flex justify-end space-x-2"> - <Button onClick={handleVerification} disabled={isVerifying} data-testid="verify-email-trigger"> + <Button onClick={handleVerification} disabled={isVerifying} loading={isVerifying} data-testid="verify-email-trigger"> {t("verify")} </Button> </div> </div> </DialogContent> </Dialog>
🧹 Nitpick comments (1)
apps/web/modules/bookings/views/bookings-single-view.tsx (1)
423-433: Well-implemented PII obfuscation utilities.The obfuscation logic properly masks sensitive information while maintaining some readability. Good defensive programming with the null checks.
Consider extracting the bullet character as a constant for consistency:
+const OBFUSCATION_CHAR = "•"; + const obfuscate = (value: string, visibleChars = 1) => { if (!value) return ""; - return value.substring(0, visibleChars) + "•".repeat(Math.max(0, value.length - visibleChars)); + return value.substring(0, visibleChars) + OBFUSCATION_CHAR.repeat(Math.max(0, value.length - visibleChars)); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/modules/bookings/views/bookings-single-view.tsx(9 hunks)apps/web/public/static/locales/en/common.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/public/static/locales/en/common.json
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code. Functions like .add, .diff, .isBefore, and .isAfter are slow, especially in timezone mode. Prefer .utc() for better performance. Where possible, replace with native Date and direct .valueOf() comparisons for faster execution. Recommend using native methods or Day.js .utc() consistently in hot paths like loops.
Files:
apps/web/modules/bookings/views/bookings-single-view.tsx
🧠 Learnings (2)
📓 Common learnings
Learnt from: Anshumancanrock
PR: calcom/cal.com#22570
File: apps/web/modules/signup-view.tsx:253-253
Timestamp: 2025-07-21T21:33:23.371Z
Learning: In signup-view.tsx, when checking if redirectUrl contains certain strings, using explicit && checks (redirectUrl && redirectUrl.includes()) is preferred over optional chaining (redirectUrl?.includes()) to ensure the result is always a boolean rather than potentially undefined. This approach provides cleaner boolean contracts for downstream conditional logic.
Learnt from: eunjae-lee
PR: calcom/cal.com#22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.389Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
apps/web/modules/bookings/views/bookings-single-view.tsx (6)
Learnt from: eunjae-lee
PR: #22106
File: packages/features/insights/components/FailedBookingsByField.tsx:65-71
Timestamp: 2025-07-15T12:59:34.389Z
Learning: In the FailedBookingsByField component (packages/features/insights/components/FailedBookingsByField.tsx), although routingFormId is typed as optional in useInsightsParameters, the system automatically enforces a routing form filter, so routingFormId is always present in practice. This means the data always contains only one entry, making the single-entry destructuring approach safe.
Learnt from: Anshumancanrock
PR: #22570
File: apps/web/modules/signup-view.tsx:253-253
Timestamp: 2025-07-21T21:33:23.371Z
Learning: In signup-view.tsx, when checking if redirectUrl contains certain strings, using explicit && checks (redirectUrl && redirectUrl.includes()) is preferred over optional chaining (redirectUrl?.includes()) to ensure the result is always a boolean rather than potentially undefined. This approach provides cleaner boolean contracts for downstream conditional logic.
Learnt from: alishaz-polymath
PR: #22304
File: packages/features/eventtypes/components/MultiplePrivateLinksController.tsx:92-94
Timestamp: 2025-07-16T06:42:27.024Z
Learning: In the MultiplePrivateLinksController component (packages/features/eventtypes/components/MultiplePrivateLinksController.tsx), the currentLink.maxUsageCount ?? 1 fallback in the openSettingsDialog function is intentional. Missing maxUsageCount values indicate old/legacy private links that existed before the expiration feature was added, and they should default to single-use behavior (1) for backward compatibility.
Learnt from: eunjae-lee
PR: #22702
File: packages/lib/server/service/insightsBooking.ts:120-124
Timestamp: 2025-07-24T08:39:06.185Z
Learning: In the InsightsBookingService (packages/lib/server/service/insightsBooking.ts), the constructor stores null for invalid options or filters but this is handled safely through null checks in buildFilterConditions() and buildAuthorizationConditions() methods. The service uses defensive programming to return safe fallback conditions (null or NOTHING_CONDITION) rather than throwing errors on invalid inputs.
Learnt from: CR
PR: calcom/cal.com#0
File: .cursor/rules/review.mdc:0-0
Timestamp: 2025-07-21T13:54:11.770Z
Learning: Applies to frontend/**/*.{js,jsx,ts,tsx} : Always use t() for text localization in frontend code; direct text embedding should trigger a warning.
Learnt from: CarinaWolli
PR: #22296
File: packages/lib/bookings/filterHostsBySameRoundRobinHost.ts:41-42
Timestamp: 2025-07-22T11:42:47.623Z
Learning: The filterHostsBySameRoundRobinHost function in packages/lib/bookings/filterHostsBySameRoundRobinHost.ts has a known limitation where it doesn't work correctly with fixed hosts or round robin groups. This is pre-existing technical debt that was already broken before the round robin groups feature. CarinaWolli has documented this in Linear issue CAL-6134 for future fix.
⏰ Context from checks skipped due to timeout of 90000ms. 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 (6)
apps/web/modules/bookings/views/bookings-single-view.tsx (6)
8-8: LGTM! Proper imports and validation schema.The added React hooks and UI component imports are appropriate for the email verification feature. The Zod schema correctly validates email format with clear error messages.
Also applies to: 53-53, 56-56, 86-89
158-158: Good state management setup.Properly tracking session loading status and initializing all necessary state variables for the email verification flow.
Also applies to: 198-201
434-443: Correctly implemented email validation.Good use of
useCallbackwith proper dependencies. The case-insensitive email comparison is appropriate and the validation logic correctly checks all relevant emails.
455-463: Clean URL parameter handling.Good practice using
router.replacewithscroll: falseto update the URL without affecting browser history or scroll position.
464-485: Robust email verification handler.Excellent error handling with Zod validation, proper loading state management using try/finally, and clear error messaging.
701-735: Consistent PII protection throughout the UI.All personally identifiable information is properly obfuscated when
canShowPIIis false, including names, emails, and phone numbers. Good consistent implementation pattern.
Changes done :
Preview :feat-cal-5045.1.mp4 |
As per the comment by @CarinaWolli we should not display the booking page at all and not just obfuscate the booking details. In your video I could see that booking page is still being displayed before the dialog box opens. |
cal.mp4 |
|
The dialog looks good now 🙏 @kart1ka could you do another code review? |
I will review. |
|
Hey @kart1ka can we please prioritize a review here and get this over the line? |
| const isValidBookingEmail = useCallback( | ||
| (email: string) => { | ||
| return ( | ||
| bookingInfo?.attendees?.some((attendee) => attendee.email.toLowerCase() === email.toLowerCase()) || | ||
| email.toLowerCase() === bookingInfo?.user?.email.toLowerCase() | ||
| ); | ||
| }, | ||
| [bookingInfo] | ||
| ); |
There was a problem hiding this comment.
We should do a server side validation instead of checking the email client side.
| }, [session, sessionStatus, isValidBookingEmail, emailParam]); | ||
|
|
||
| const updateSearchParams = useCallback( | ||
| async (email: string) => { |
There was a problem hiding this comment.
| async (email: string) => { | |
| (email: string) => { |
| async (email: string) => { | ||
| const params = new URLSearchParams(searchParams.toString()); | ||
| params.set("email", email); | ||
| await router.replace(`?${params.toString()}`, { scroll: false }); |
There was a problem hiding this comment.
| await router.replace(`?${params.toString()}`, { scroll: false }); | |
| router.replace(`?${params.toString()}`, { scroll: false }); |
|
This PR is being marked as stale due to inactivity. |
What does this PR do?
Follow-up PR - #18770
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Checklist
cal-5045.mp4