feat(event-types): add team members as optional guests#22651
feat(event-types): add team members as optional guests#22651husniabad wants to merge 4 commits intocalcom:mainfrom
Conversation
|
@husniabad is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
|
|
WalkthroughAdds support for selecting team members as optional guests and propagates that selection across DB schema, API, UI, booking flow, and calendar adapters. Introduces a Prisma many-to-many relation and migration, extends TRPC input/handler and repository projections to accept/persist optionalGuestTeamMembers, adds UI controls (OptionalTeamGuestSetting) and localization, updates form/defaults and CalendarEventBuilder to include optional guests, and updates multiple calendar services to add optional attendees and avoid duplicating them as required. Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ 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
$25
community
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
apps/web/public/static/locales/en/common.json (1)
1497-1498: Grammar & clarity – adjust pluralization and streamline the description
The current copy mixes singular (“an optional guest”) with plural (“team members”) and is a bit verbose. Refining it improves readability and remains consistent with neighbouring strings.- "optional_guest_team_members": "Add team members as optional guests", - "optional_guest_team_members_description": "Adding team members as an optional guest will always send an optional invite, but not check their availability.", + "optional_guest_team_members": "Add team members as optional guests", + "optional_guest_team_members_description": "Selected team members are invited as optional guests without checking their availability.",packages/features/eventtypes/components/tabs/advanced/OptionalTeamGuestSetting.tsx (3)
26-28: Consider removing redundant useEffect.The
useEffectupdatingisOptionalTeamGuestEnabledappears redundant since the initial state already derives fromeventType.optionalGuestTeamMembers.length > 0. The component will re-render when props change, making this effect unnecessary.- useEffect(() => { - setIsOptionalTeamGuestEnabled(eventType.optionalGuestTeamMembers.length > 0); - }, [eventType.optionalGuestTeamMembers]);
42-42: Ensure consistent email fallback values.The email fallback values are inconsistent:
- Line 42:
"No Email"- Line 89:
""Use consistent fallback values to prevent display inconsistencies.
- label: option.user.email || "No Email", + label: option.user.email || "",Also applies to: 89-89
82-83: Remove unnecessary null check for onChange.The
onChangefunction is provided by react-hook-form's Controller and will always be defined. The null check is unnecessary.- if (!onChange) return; onChange(options.map((option) => ({ id: parseInt(option.value) })));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
apps/web/public/static/locales/en/common.json(1 hunks)packages/app-store/exchange2013calendar/lib/CalendarService.ts(3 hunks)packages/app-store/exchange2016calendar/lib/CalendarService.ts(3 hunks)packages/app-store/exchangecalendar/lib/CalendarService.ts(4 hunks)packages/app-store/feishucalendar/lib/CalendarService.ts(1 hunks)packages/app-store/googlecalendar/lib/CalendarService.ts(2 hunks)packages/app-store/larkcalendar/lib/CalendarService.ts(1 hunks)packages/app-store/zohocalendar/lib/CalendarService.ts(2 hunks)packages/features/CalendarEventBuilder.ts(1 hunks)packages/features/bookings/lib/handleNewBooking.ts(1 hunks)packages/features/bookings/lib/handleNewBooking/getEventTypesFromDB.ts(1 hunks)packages/features/eventtypes/components/tabs/advanced/EventAdvancedTab.tsx(2 hunks)packages/features/eventtypes/components/tabs/advanced/OptionalTeamGuestSetting.tsx(1 hunks)packages/features/eventtypes/lib/types.ts(1 hunks)packages/lib/defaultEvents.ts(1 hunks)packages/lib/server/repository/eventType.ts(1 hunks)packages/platform/atoms/event-types/hooks/useEventTypeForm.ts(1 hunks)packages/prisma/migrations/20250716153028_add_team_member_as_optional_guest/migration.sql(1 hunks)packages/prisma/schema.prisma(2 hunks)packages/trpc/server/routers/viewer/eventTypes/types.ts(1 hunks)packages/trpc/server/routers/viewer/eventTypes/update.handler.ts(4 hunks)packages/types/Calendar.d.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (16)
📓 Common learnings
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/prisma/schema.prisma:891-891
Timestamp: 2025-07-18T08:47:01.255Z
Learning: In Cal.com's calendar integration, both Google Calendar and Outlook Calendar are designed to allow multiple eventTypeIds to share the same subscription ID (googleChannelId or outlookSubscriptionId). This is an intentional design pattern to reuse existing subscriptions for efficiency rather than creating separate subscriptions for each event type. Therefore, unique constraints like `@@unique([outlookSubscriptionId, eventTypeId])` should not be added as they would break this subscription sharing functionality.
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/prisma/schema.prisma:891-891
Timestamp: 2025-07-18T08:47:01.256Z
Learning: The Outlook Calendar integration in Cal.com intentionally reuses subscription IDs across multiple event types for efficiency. The `upsertSelectedCalendarsForEventTypeIds` method creates separate SelectedCalendar records for each eventTypeId, all sharing the same outlookSubscriptionId. This subscription sharing pattern means that unique constraints like `@@unique([outlookSubscriptionId, eventTypeId])` should not be applied as they would prevent this intended functionality.
packages/features/eventtypes/lib/types.ts (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.
packages/platform/atoms/event-types/hooks/useEventTypeForm.ts (3)
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.
Learnt from: alishaz-polymath
PR: calcom/cal.com#22304
File: packages/features/eventtypes/components/MultiplePrivateLinksController.tsx:92-94
Timestamp: 2025-07-16T06:42:27.001Z
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: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/prisma/schema.prisma:891-891
Timestamp: 2025-07-18T08:47:01.255Z
Learning: In Cal.com's calendar integration, both Google Calendar and Outlook Calendar are designed to allow multiple eventTypeIds to share the same subscription ID (googleChannelId or outlookSubscriptionId). This is an intentional design pattern to reuse existing subscriptions for efficiency rather than creating separate subscriptions for each event type. Therefore, unique constraints like `@@unique([outlookSubscriptionId, eventTypeId])` should not be added as they would break this subscription sharing functionality.
packages/features/bookings/lib/handleNewBooking/getEventTypesFromDB.ts (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.
packages/features/bookings/lib/handleNewBooking.ts (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.
packages/trpc/server/routers/viewer/eventTypes/types.ts (1)
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/prisma/schema.prisma:891-891
Timestamp: 2025-07-18T08:47:01.255Z
Learning: In Cal.com's calendar integration, both Google Calendar and Outlook Calendar are designed to allow multiple eventTypeIds to share the same subscription ID (googleChannelId or outlookSubscriptionId). This is an intentional design pattern to reuse existing subscriptions for efficiency rather than creating separate subscriptions for each event type. Therefore, unique constraints like `@@unique([outlookSubscriptionId, eventTypeId])` should not be added as they would break this subscription sharing functionality.
packages/app-store/feishucalendar/lib/CalendarService.ts (1)
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/app-store/office365calendar/api/webhook.ts:120-123
Timestamp: 2025-07-18T17:57:16.370Z
Learning: The office365calendar webhook handler in packages/app-store/office365calendar/api/webhook.ts is specifically designed for Office365 calendar integration, not as a generic webhook handler. Therefore, it's safe to assume that fetchAvailabilityAndSetCache method will be implemented in the Office365CalendarService, making explicit validation checks unnecessary.
packages/app-store/exchange2013calendar/lib/CalendarService.ts (3)
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/app-store/office365calendar/api/webhook.ts:120-123
Timestamp: 2025-07-18T17:57:16.370Z
Learning: The office365calendar webhook handler in packages/app-store/office365calendar/api/webhook.ts is specifically designed for Office365 calendar integration, not as a generic webhook handler. Therefore, it's safe to assume that fetchAvailabilityAndSetCache method will be implemented in the Office365CalendarService, making explicit validation checks unnecessary.
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/prisma/schema.prisma:891-891
Timestamp: 2025-07-18T08:47:01.256Z
Learning: The Outlook Calendar integration in Cal.com intentionally reuses subscription IDs across multiple event types for efficiency. The `upsertSelectedCalendarsForEventTypeIds` method creates separate SelectedCalendar records for each eventTypeId, all sharing the same outlookSubscriptionId. This subscription sharing pattern means that unique constraints like `@@unique([outlookSubscriptionId, eventTypeId])` should not be applied as they would prevent this intended functionality.
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/prisma/schema.prisma:891-891
Timestamp: 2025-07-18T08:47:01.255Z
Learning: In Cal.com's calendar integration, both Google Calendar and Outlook Calendar are designed to allow multiple eventTypeIds to share the same subscription ID (googleChannelId or outlookSubscriptionId). This is an intentional design pattern to reuse existing subscriptions for efficiency rather than creating separate subscriptions for each event type. Therefore, unique constraints like `@@unique([outlookSubscriptionId, eventTypeId])` should not be added as they would break this subscription sharing functionality.
packages/app-store/exchangecalendar/lib/CalendarService.ts (3)
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/app-store/office365calendar/api/webhook.ts:120-123
Timestamp: 2025-07-18T17:57:16.370Z
Learning: The office365calendar webhook handler in packages/app-store/office365calendar/api/webhook.ts is specifically designed for Office365 calendar integration, not as a generic webhook handler. Therefore, it's safe to assume that fetchAvailabilityAndSetCache method will be implemented in the Office365CalendarService, making explicit validation checks unnecessary.
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/prisma/schema.prisma:891-891
Timestamp: 2025-07-18T08:47:01.256Z
Learning: The Outlook Calendar integration in Cal.com intentionally reuses subscription IDs across multiple event types for efficiency. The `upsertSelectedCalendarsForEventTypeIds` method creates separate SelectedCalendar records for each eventTypeId, all sharing the same outlookSubscriptionId. This subscription sharing pattern means that unique constraints like `@@unique([outlookSubscriptionId, eventTypeId])` should not be applied as they would prevent this intended functionality.
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/prisma/schema.prisma:891-891
Timestamp: 2025-07-18T08:47:01.255Z
Learning: In Cal.com's calendar integration, both Google Calendar and Outlook Calendar are designed to allow multiple eventTypeIds to share the same subscription ID (googleChannelId or outlookSubscriptionId). This is an intentional design pattern to reuse existing subscriptions for efficiency rather than creating separate subscriptions for each event type. Therefore, unique constraints like `@@unique([outlookSubscriptionId, eventTypeId])` should not be added as they would break this subscription sharing functionality.
packages/app-store/larkcalendar/lib/CalendarService.ts (1)
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/app-store/office365calendar/api/webhook.ts:120-123
Timestamp: 2025-07-18T17:57:16.370Z
Learning: The office365calendar webhook handler in packages/app-store/office365calendar/api/webhook.ts is specifically designed for Office365 calendar integration, not as a generic webhook handler. Therefore, it's safe to assume that fetchAvailabilityAndSetCache method will be implemented in the Office365CalendarService, making explicit validation checks unnecessary.
packages/app-store/exchange2016calendar/lib/CalendarService.ts (3)
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/app-store/office365calendar/api/webhook.ts:120-123
Timestamp: 2025-07-18T17:57:16.370Z
Learning: The office365calendar webhook handler in packages/app-store/office365calendar/api/webhook.ts is specifically designed for Office365 calendar integration, not as a generic webhook handler. Therefore, it's safe to assume that fetchAvailabilityAndSetCache method will be implemented in the Office365CalendarService, making explicit validation checks unnecessary.
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/prisma/schema.prisma:891-891
Timestamp: 2025-07-18T08:47:01.256Z
Learning: The Outlook Calendar integration in Cal.com intentionally reuses subscription IDs across multiple event types for efficiency. The `upsertSelectedCalendarsForEventTypeIds` method creates separate SelectedCalendar records for each eventTypeId, all sharing the same outlookSubscriptionId. This subscription sharing pattern means that unique constraints like `@@unique([outlookSubscriptionId, eventTypeId])` should not be applied as they would prevent this intended functionality.
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/prisma/schema.prisma:891-891
Timestamp: 2025-07-18T08:47:01.255Z
Learning: In Cal.com's calendar integration, both Google Calendar and Outlook Calendar are designed to allow multiple eventTypeIds to share the same subscription ID (googleChannelId or outlookSubscriptionId). This is an intentional design pattern to reuse existing subscriptions for efficiency rather than creating separate subscriptions for each event type. Therefore, unique constraints like `@@unique([outlookSubscriptionId, eventTypeId])` should not be added as they would break this subscription sharing functionality.
packages/app-store/googlecalendar/lib/CalendarService.ts (1)
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/app-store/office365calendar/api/webhook.ts:120-123
Timestamp: 2025-07-18T17:57:16.370Z
Learning: The office365calendar webhook handler in packages/app-store/office365calendar/api/webhook.ts is specifically designed for Office365 calendar integration, not as a generic webhook handler. Therefore, it's safe to assume that fetchAvailabilityAndSetCache method will be implemented in the Office365CalendarService, making explicit validation checks unnecessary.
packages/app-store/zohocalendar/lib/CalendarService.ts (1)
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/app-store/office365calendar/api/webhook.ts:120-123
Timestamp: 2025-07-18T17:57:16.370Z
Learning: The office365calendar webhook handler in packages/app-store/office365calendar/api/webhook.ts is specifically designed for Office365 calendar integration, not as a generic webhook handler. Therefore, it's safe to assume that fetchAvailabilityAndSetCache method will be implemented in the Office365CalendarService, making explicit validation checks unnecessary.
packages/features/eventtypes/components/tabs/advanced/OptionalTeamGuestSetting.tsx (2)
Learnt from: alishaz-polymath
PR: calcom/cal.com#22304
File: packages/features/eventtypes/components/MultiplePrivateLinksController.tsx:92-94
Timestamp: 2025-07-16T06:42:27.001Z
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: 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>
packages/features/eventtypes/components/tabs/advanced/EventAdvancedTab.tsx (2)
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.
Learnt from: alishaz-polymath
PR: calcom/cal.com#22304
File: packages/features/eventtypes/components/MultiplePrivateLinksController.tsx:92-94
Timestamp: 2025-07-16T06:42:27.001Z
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.
🧬 Code Graph Analysis (2)
packages/app-store/exchangecalendar/lib/CalendarService.ts (2)
packages/trpc/server/routers/publicViewer/procedures/event.ts (1)
event(8-12)packages/types/Calendar.d.ts (1)
Person(33-45)
packages/trpc/server/routers/viewer/eventTypes/update.handler.ts (1)
packages/platform/libraries/index.ts (2)
TRPCError(56-56)SchedulingType(30-30)
⏰ 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: required
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Security Check
🔇 Additional comments (32)
packages/features/eventtypes/lib/types.ts (1)
168-168: LGTM! Type definition is appropriate for the feature.The
optionalGuestTeamMembersproperty is correctly defined as an array of objects with numeric IDs, which is suitable for storing references to selected team members in the form state.packages/lib/defaultEvents.ts (1)
147-147: LGTM! Proper default initialization.The empty array default for
optionalGuestTeamMembersis appropriate and follows the established pattern in the commons object for initializing optional array properties.packages/types/Calendar.d.ts (1)
224-227: LGTM! Well-structured type definition for calendar events.The
optionalGuestTeamMembersproperty is appropriately defined with:
- Required
- Optional
namefield (allowing null)- Optional property for backward compatibility
The structure correctly differs from the form type (using email/name instead of just id) as calendar events need actual contact information rather than just references.
packages/trpc/server/routers/viewer/eventTypes/types.ts (1)
90-93: LGTM! Proper schema validation definition.The
optionalGuestTeamMembersschema is well-defined with:
- Appropriate Zod validation for array of objects with numeric IDs
- Optional and nullable for backward compatibility
- Consistent structure with the form type definitions
The validation ensures type safety while maintaining flexibility for different input states.
packages/features/bookings/lib/handleNewBooking.ts (1)
1163-1163: LGTM! Clean integration with the CalendarEventBuilder pattern.The addition follows the established builder pattern used throughout the file and includes appropriate null-safety with the empty array fallback.
packages/platform/atoms/event-types/hooks/useEventTypeForm.ts (1)
83-83: LGTM! Proper form field initialization.The addition follows the existing pattern for initializing optional array fields in the form's default values, ensuring consistent form state management.
packages/features/bookings/lib/handleNewBooking/getEventTypesFromDB.ts (1)
175-181: LGTM! Well-structured Prisma relation selection.The addition follows the established pattern for relation selections in this query and appropriately selects the necessary fields (name, email, id) for optional guest team member functionality.
packages/lib/server/repository/eventType.ts (1)
520-524: LGTM! Well-integrated addition following established patterns.The addition of
optionalGuestTeamMembersto the select object follows the same pattern as other relations in the codebase, appropriately selecting only theidfield. This change aligns with the PR objective to support optional guest team members functionality.packages/features/CalendarEventBuilder.ts (1)
265-276: LGTM! Clean builder method implementation.The
withOptionalGuestTeamMembersmethod follows the established builder pattern perfectly, with appropriate TypeScript typing and safe defaults. The method signature aligns with the CalendarEvent interface and provides a clean API for setting optional guest team members.packages/prisma/schema.prisma (1)
206-209: Rename the new relation field for clarity & to avoid naming collisionsThe field name
EventTypeis too generic, starts with a capital letter (inconsistent with the rest of the model), and clashes semantically with the already-existingeventTypes/ownedEventTypesrelations. This will generate an awkward Prisma Client API (user.EventType) that gives no hint that it represents “optional guest” event types.- optionalGuestTeamMembers User[] @relation("optional_guest_team_members") + optionalGuestTeamMembers User[] @relation("optional_guest_team_members")No change is needed on the
EventTypeside, but onUserthe mirror field should be renamed to something descriptive, e.g.optionalGuestForEventTypes.Likely an incorrect or invalid review comment.
packages/features/eventtypes/components/tabs/advanced/EventAdvancedTab.tsx (2)
66-66: LGTM: Clean import addition.The import is properly placed in alphabetical order and follows the established pattern for local component imports.
1205-1207: LGTM: Well-implemented conditional rendering.The component is properly conditionally rendered with appropriate checks for team existence and members. The props passed (
teamandeventType) align with the component's requirements, and the placement before the round-robin setting makes logical sense.packages/app-store/larkcalendar/lib/CalendarService.ts (1)
402-448: Excellent refactor for attendee handling.The restructured
translateAttendeesmethod demonstrates several best practices:
- Efficient lookup: Using a
Setfor optional guest emails provides O(1) lookup performance- Clear separation: The 4-step approach with descriptive comments makes the logic easy to follow
- Duplication prevention: Properly filters required team members to exclude those marked as optional guests
- Proper flagging: Correctly sets
is_optionalbased on attendee typeThis refactor aligns well with the broader pattern across calendar services for handling optional guest team members.
packages/app-store/exchange2013calendar/lib/CalendarService.ts (3)
72-96: Well-implemented attendee handling in createEvent.The changes properly implement the optional guest team member feature:
- Efficient lookup: Set-based approach for checking optional guest emails
- Proper segregation: Required team members are filtered to exclude optional guests, preventing duplication
- Correct API usage: Uses Exchange's
RequiredAttendeesandOptionalAttendeescollections appropriately- Clear logic flow: Well-commented steps make the intent obvious
128-155: Comprehensive updateEvent implementation.The changes ensure proper attendee management during updates:
- Prevents accumulation: Clearing existing attendees before adding new ones avoids stale entries
- Consistent logic: Uses the same efficient attendee handling as
createEvent- Complete coverage: Handles both required and optional attendees consistently
This thorough approach ensures the update operation maintains data integrity.
104-104: Good improvement: Added descriptive type field.Setting the
typeto"exchange2013_calendar"instead of an empty string provides better service identification and debugging capabilities.packages/app-store/googlecalendar/lib/CalendarService.ts (3)
104-107: LGTM! Efficient email lookup implementation.The use of a
Setwith lowercase conversion provides optimal O(1) lookup performance and ensures case-insensitive email matching.
111-116: Good deduplication logic.The enhanced filter correctly prevents team members from being added as both required and optional attendees, maintaining clean attendee lists.
130-147: Excellent implementation of optional guest handling.The code correctly sets
optional: trueandresponseStatus: "needsAction"for optional attendees, with proper deduplication logic to prevent conflicts with existing attendees.packages/app-store/exchange2016calendar/lib/CalendarService.ts (4)
73-90: Consistent implementation with proper deduplication.The optional guest handling follows the established pattern from Google Calendar integration, correctly preventing team members from being added as both required and optional attendees.
93-97: Correct use of Exchange API for optional attendees.The code properly utilizes
appointment.OptionalAttendees.Add()to distinguish optional guests from required attendees in the Exchange calendar system.
129-156: Good practice of clearing existing attendees in updates.The implementation correctly clears both required and optional attendees before adding new ones, preventing stale data accumulation. The attendee handling logic consistently mirrors the
createEventmethod.
105-105: Good addition of explicit type identifier.Setting the
typefield to"exchange2016_calendar"improves clarity and makes it easier to identify the calendar integration used for event creation.packages/app-store/exchangecalendar/lib/CalendarService.ts (3)
63-87: Consistent implementation with proper TypeScript typing.The optional guest handling follows the established pattern with correct type annotations. The deduplication logic properly prevents conflicts between required and optional attendees.
117-143: Proper attendee management in updates.The implementation correctly clears existing attendees before reconstruction, preventing data inconsistencies. The logic consistently mirrors the
createEventmethod pattern.
95-95: Good addition of explicit type identifiers.Setting the
typefield to"exchange_calendar"in both methods improves consistency and makes it easier to identify the calendar integration used.Also applies to: 154-154
packages/app-store/feishucalendar/lib/CalendarService.ts (2)
404-423: Excellent refactoring with improved deduplication.The introduction of the
addUniqueAttendeehelper function and Set-based deduplication significantly improves code organization and prevents duplicate attendees. The case-insensitive email tracking is a good practice.
424-444: Well-structured 4-step attendee processing.The systematic approach with clear step documentation makes the attendee handling logic easy to understand and maintain. The filtering correctly prevents duplication and excludes the current user from team member invitations.
packages/prisma/migrations/20250716153028_add_team_member_as_optional_guest/migration.sql (1)
1-18: Well-structured many-to-many relationship migration.The migration follows Prisma conventions with proper indexing and CASCADE constraints. The unique index on
(A, B)prevents duplicate relationships while the index onBoptimizes queries from the user side.packages/app-store/zohocalendar/lib/CalendarService.ts (1)
470-470: Good refactoring to extract attendee logic.Moving the attendee construction logic to a dedicated method improves code organization and maintainability.
packages/trpc/server/routers/viewer/eventTypes/update.handler.ts (2)
394-418: Well-implemented validation for optional guest team members.The validation logic properly handles:
- Duplicate detection using Set for efficiency
- Team membership verification to ensure data integrity
- Clear error messages for different failure scenarios
- Correct Prisma relation update pattern with set/connect
432-471: Good defensive programming for host updates.The added conditional check
if (hosts && Array.isArray(hosts))prevents unintended modifications to the hosts relation when the field is not included in the update request. This is an important improvement for API robustness.
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (07/20/25)1 reviewer was added to this PR based on Keith Williams's automation. "Add community label" took an action on this PR • (07/20/25)1 label was added to this PR based on Keith Williams's automation. |
husniabad
force-pushed
the
feature/add-team-member-as-optional-guest
branch
from
2f2e97f to
ad8e8a6
Compare
Devanshusharma2005
left a comment
Devanshusharma2005
left a comment
There was a problem hiding this comment.
typecheck issues are failing. Can you please address them.
husniabad
force-pushed
the
feature/add-team-member-as-optional-guest
branch
from
ad8e8a6 to
bf05524
Compare
Issue fixed, please check again |
|
This PR is being marked as stale due to inactivity. |
kart1ka
left a comment
kart1ka
left a comment
There was a problem hiding this comment.
Hey @husniabad, Could you pls resolve the merge conflicts?
husniabad
force-pushed
the
feature/add-team-member-as-optional-guest
branch
from
bf05524 to
336a5da
Compare
|
Hi @kart1ka I have updated the PR, could you please review |
husniabad
force-pushed
the
feature/add-team-member-as-optional-guest
branch
from
2f0cb60 to
f87a6d7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/web/lib/csp.ts (1)
39-50: Document the magic number.22 bytes is a good choice (base64 encodes to 32 chars). Add a quick inline note so future edits don’t accidentally change CSP expectations.
- const BYTE_LEN = 22; + const BYTE_LEN = 22; // 22 bytes → 32 base64 chars (no padding) for CSP nonce
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
apps/web/lib/csp.ts(1 hunks)apps/web/public/static/locales/en/common.json(1 hunks)packages/app-store/exchange2013calendar/lib/CalendarService.ts(3 hunks)packages/app-store/exchange2016calendar/lib/CalendarService.ts(3 hunks)packages/app-store/exchangecalendar/lib/CalendarService.ts(4 hunks)packages/app-store/feishucalendar/lib/CalendarService.ts(1 hunks)packages/app-store/googlecalendar/lib/CalendarService.ts(2 hunks)packages/app-store/larkcalendar/lib/CalendarService.ts(1 hunks)packages/app-store/zohocalendar/lib/CalendarService.ts(2 hunks)packages/features/CalendarEventBuilder.ts(1 hunks)packages/features/bookings/lib/handleNewBooking.ts(1 hunks)packages/features/bookings/lib/handleNewBooking/getEventTypesFromDB.ts(1 hunks)packages/features/eventtypes/components/tabs/advanced/EventAdvancedTab.tsx(2 hunks)packages/features/eventtypes/components/tabs/advanced/OptionalTeamGuestSetting.tsx(1 hunks)packages/features/eventtypes/lib/types.ts(1 hunks)packages/lib/defaultEvents.ts(1 hunks)packages/lib/server/repository/eventTypeRepository.ts(2 hunks)packages/platform/atoms/event-types/hooks/useEventTypeForm.ts(1 hunks)packages/prisma/migrations/20250716153028_add_team_member_as_optional_guest/migration.sql(1 hunks)packages/prisma/schema.prisma(2 hunks)packages/trpc/server/routers/viewer/eventTypes/types.ts(1 hunks)packages/trpc/server/routers/viewer/eventTypes/update.handler.ts(3 hunks)packages/types/Calendar.d.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (21)
- packages/features/eventtypes/lib/types.ts
- packages/lib/defaultEvents.ts
- packages/features/bookings/lib/handleNewBooking/getEventTypesFromDB.ts
- packages/platform/atoms/event-types/hooks/useEventTypeForm.ts
- packages/features/CalendarEventBuilder.ts
- packages/trpc/server/routers/viewer/eventTypes/types.ts
- packages/app-store/larkcalendar/lib/CalendarService.ts
- packages/features/eventtypes/components/tabs/advanced/EventAdvancedTab.tsx
- packages/app-store/exchangecalendar/lib/CalendarService.ts
- packages/features/bookings/lib/handleNewBooking.ts
- packages/app-store/exchange2016calendar/lib/CalendarService.ts
- packages/types/Calendar.d.ts
- packages/prisma/schema.prisma
- packages/app-store/exchange2013calendar/lib/CalendarService.ts
- packages/app-store/feishucalendar/lib/CalendarService.ts
- packages/prisma/migrations/20250716153028_add_team_member_as_optional_guest/migration.sql
- packages/features/eventtypes/components/tabs/advanced/OptionalTeamGuestSetting.tsx
- apps/web/public/static/locales/en/common.json
- packages/app-store/googlecalendar/lib/CalendarService.ts
- packages/app-store/zohocalendar/lib/CalendarService.ts
- packages/trpc/server/routers/viewer/eventTypes/update.handler.ts
🧰 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/lib/csp.tspackages/lib/server/repository/eventTypeRepository.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/lib/csp.tspackages/lib/server/repository/eventTypeRepository.ts
**/*Repository.ts
📄 CodeRabbit Inference Engine (.cursor/rules/review.mdc)
Repository files must include
Repositorysuffix, prefix with technology if applicable (e.g.,PrismaAppRepository.ts), and use PascalCase matching the exported class
Files:
packages/lib/server/repository/eventTypeRepository.ts
🧠 Learnings (9)
📓 Common learnings
Learnt from: sean-brydon
PR: calcom/cal.com#22618
File: packages/trpc/server/routers/viewer/eventTypes/utils/transformUtils.ts:113-113
Timestamp: 2025-08-05T07:42:06.335Z
Learning: In Cal.com's getUserEventGroups handler refactor (PR #22618), the membershipCount field for team event groups is intentionally set to 0 in the new createTeamEventGroup function, as confirmed by sean-brydon (PR author). This preserves the same behavior as the old implementation that was being refactored, maintaining backward compatibility. While other parts of the codebase may use actual member counts, this specific implementation maintains the previous behavior.
Learnt from: sean-brydon
PR: calcom/cal.com#22618
File: packages/trpc/server/routers/viewer/eventTypes/utils/transformUtils.ts:113-113
Timestamp: 2025-08-05T07:42:06.335Z
Learning: In Cal.com's event type system, the membershipCount field for team event groups is intentionally set to 0, as confirmed by sean-brydon (PR author). This behavior was preserved during the refactor from the old getUserEventGroups.handler.ts implementation to the new createTeamEventGroup function in transformUtils.ts. This is not a bug but the intended behavior that maintains consistency with the previous implementation.
Learnt from: sean-brydon
PR: calcom/cal.com#22618
File: packages/trpc/server/routers/viewer/eventTypes/utils/transformUtils.ts:113-113
Timestamp: 2025-08-05T07:42:06.335Z
Learning: In Cal.com's event type system, the membershipCount field for team event groups is intentionally hard-coded to 0, as confirmed by sean-brydon. This behavior was preserved during the refactor from the old getUserEventGroups.handler.ts implementation to the new createTeamEventGroup function in transformUtils.ts. This is not a bug but the intended behavior.
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/prisma/schema.prisma:891-891
Timestamp: 2025-07-18T08:47:01.264Z
Learning: In Cal.com's calendar integration, both Google Calendar and Outlook Calendar are designed to allow multiple eventTypeIds to share the same subscription ID (googleChannelId or outlookSubscriptionId). This is an intentional design pattern to reuse existing subscriptions for efficiency rather than creating separate subscriptions for each event type. Therefore, unique constraints like `@unique([outlookSubscriptionId, eventTypeId])` should not be added as they would break this subscription sharing functionality.
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/prisma/schema.prisma:891-891
Timestamp: 2025-07-18T08:47:01.264Z
Learning: The Outlook Calendar integration in Cal.com intentionally reuses subscription IDs across multiple event types for efficiency. The `upsertSelectedCalendarsForEventTypeIds` method creates separate SelectedCalendar records for each eventTypeId, all sharing the same outlookSubscriptionId. This subscription sharing pattern means that unique constraints like `@unique([outlookSubscriptionId, eventTypeId])` should not be applied as they would prevent this intended functionality.
📚 Learning: 2025-07-28T11:50:23.946Z
Learnt from: CR
PR: calcom/cal.com#0
File: .cursor/rules/review.mdc:0-0
Timestamp: 2025-07-28T11:50:23.946Z
Learning: Applies to **/*.ts : Ensure the `credential.key` field is never returned from tRPC endpoints or APIs
Applied to files:
apps/web/lib/csp.ts
📚 Learning: 2025-08-05T07:42:06.335Z
Learnt from: sean-brydon
PR: calcom/cal.com#22618
File: packages/trpc/server/routers/viewer/eventTypes/utils/transformUtils.ts:113-113
Timestamp: 2025-08-05T07:42:06.335Z
Learning: In Cal.com's getUserEventGroups handler refactor (PR #22618), the membershipCount field for team event groups is intentionally set to 0 in the new createTeamEventGroup function, as confirmed by sean-brydon (PR author). This preserves the same behavior as the old implementation that was being refactored, maintaining backward compatibility. While other parts of the codebase may use actual member counts, this specific implementation maintains the previous behavior.
Applied to files:
packages/lib/server/repository/eventTypeRepository.ts
📚 Learning: 2025-08-05T07:42:06.335Z
Learnt from: sean-brydon
PR: calcom/cal.com#22618
File: packages/trpc/server/routers/viewer/eventTypes/utils/transformUtils.ts:113-113
Timestamp: 2025-08-05T07:42:06.335Z
Learning: In Cal.com's event type system, the membershipCount field for team event groups is intentionally set to 0, as confirmed by sean-brydon (PR author). This behavior was preserved during the refactor from the old getUserEventGroups.handler.ts implementation to the new createTeamEventGroup function in transformUtils.ts. This is not a bug but the intended behavior that maintains consistency with the previous implementation.
Applied to files:
packages/lib/server/repository/eventTypeRepository.ts
📚 Learning: 2025-08-05T07:42:06.335Z
Learnt from: sean-brydon
PR: calcom/cal.com#22618
File: packages/trpc/server/routers/viewer/eventTypes/utils/transformUtils.ts:113-113
Timestamp: 2025-08-05T07:42:06.335Z
Learning: In Cal.com's event type system, the membershipCount field for team event groups is intentionally hard-coded to 0, as confirmed by sean-brydon. This behavior was preserved during the refactor from the old getUserEventGroups.handler.ts implementation to the new createTeamEventGroup function in transformUtils.ts. This is not a bug but the intended behavior.
Applied to files:
packages/lib/server/repository/eventTypeRepository.ts
📚 Learning: 2025-07-28T11:50:23.946Z
Learnt from: CR
PR: calcom/cal.com#0
File: .cursor/rules/review.mdc:0-0
Timestamp: 2025-07-28T11:50:23.946Z
Learning: Applies to **/*.ts : For Prisma queries, only select data you need; never use `include`, always use `select`
Applied to files:
packages/lib/server/repository/eventTypeRepository.ts
📚 Learning: 2025-08-08T09:27:23.870Z
Learnt from: Udit-takkar
PR: calcom/cal.com#22919
File: packages/features/calAIPhone/providers/retellAI/services/AgentService.ts:195-216
Timestamp: 2025-08-08T09:27:23.870Z
Learning: In PR calcom/cal.com#22919, file packages/features/calAIPhone/providers/retellAI/services/AgentService.ts, the updateAgentConfiguration method intentionally does not persist the optional `name` parameter to the repository for now, per maintainer (Udit-takkar). Future reviews should not flag this unless requirements change.
Applied to files:
packages/lib/server/repository/eventTypeRepository.ts
📚 Learning: 2025-08-07T18:42:34.056Z
Learnt from: Udit-takkar
PR: calcom/cal.com#22919
File: packages/lib/server/repository/PrismaPhoneNumberRepository.ts:412-417
Timestamp: 2025-08-07T18:42:34.056Z
Learning: In Cal.com codebase, the coding guideline requiring explicit `select` clauses instead of `include` for Prisma queries applies to read operations but not to update operations. Update operations don't need explicit select clauses.
Applied to files:
packages/lib/server/repository/eventTypeRepository.ts
📚 Learning: 2025-08-08T09:29:11.665Z
Learnt from: Udit-takkar
PR: calcom/cal.com#22919
File: packages/features/calAIPhone/interfaces/AIPhoneService.interface.ts:118-143
Timestamp: 2025-08-08T09:29:11.665Z
Learning: In calcom/cal.com PR #22919, packages/features/calAIPhone/interfaces/AIPhoneService.interface.ts (TypeScript), the AIPhoneServiceAgentListItem is required to include user.email in listAgents responses (per maintainer Udit-takkar). Future reviews should not flag this as unnecessary PII unless requirements change.
Applied to files:
packages/lib/server/repository/eventTypeRepository.ts
⏰ 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). (1)
- GitHub Check: Install dependencies / Yarn install & cache
🔇 Additional comments (4)
packages/lib/server/repository/eventTypeRepository.ts (3)
302-306: AI summary vs code: minimal-data method does not include optionalGuestTeamMembersThe AI summary mentions adding optionalGuestTeamMembers to findAllByUpIdWithMinimalData, but this select currently includes only eventTypeSelect and hashedLink. If minimal views/UI need to show existing optional guests, consider adding the id-only selection here; otherwise update the PR summary for accuracy.
Proposed change:
const select = { ...eventTypeSelect, hashedLink: hashedLinkSelect, + optionalGuestTeamMembers: { select: { id: true } }, };To ensure we don’t duplicate what eventTypeSelect might already include, check its contents:
#!/bin/bash # Locate and inspect eventTypeSelect definition and callers fd -a 'eventTypeSelect.ts' | while read -r f; do echo "==> $f" rg -n 'optionalGuestTeamMembers|export const eventTypeSelect' "$f" -n -A 3 done # Find usages of findAllByUpIdWithMinimalData to confirm consumer needs rg -n --hidden --glob '!node_modules' 'findAllByUpIdWithMinimalData' -A 3Likely an incorrect or invalid review comment.
866-870: Approve minimal id-only select for optionalGuestTeamMembers
Selecting only id in eventTypeRepository keeps the payload minimal and aligns with how the viewer and update routes consume this data. Downstream booking flows (via getEventTypesFromDB) still fetch email and name for calendar-provider integrations, and the UI maps the returned IDs to full member details in the OptionalTeamGuestSetting component. No further changes needed.
565-569: optionalGuestTeamMembers select is safe as implementedAll downstream consumers either only need an
idor explicitly fetch additional fields:
- UI (OptionalTeamGuestSetting.tsx) only reads
.lengthand IDs to toggle settings.- tRPC schema (
viewer/eventTypes/types.ts) definesoptionalGuestTeamMembersasarray<{id: number}>.- Booking flow (
getEventTypesFromDB.ts) uses its own query selectingid,name, andNo changes required.
apps/web/lib/csp.ts (1)
41-48: No client-side bundling risk detectedWe verified that
getCspNonceis only imported inapps/web/middleware.ts(server‐only), and there are no imports ofapps/web/lib/cspin any client components, pages, or hooks. The runtime guard aroundcrypto/node:cryptocovers both environments, so no further changes are needed here.
husniabad
force-pushed
the
feature/add-team-member-as-optional-guest
branch
2 times, most recently
from
55a564f to
c2da2f1
Compare
|
Hi @kart1ka I updated the PR and fixed merge conflicts as well I addressed the test failure The test failure was not related to my PR, most probably due to the previous merged PR, I fixed it in this file: |
| const oldHostsSet = new Set(eventType.hosts.map((oldHost) => oldHost.userId)); | ||
| const newHostsSet = new Set(hosts.map((oldHost) => oldHost.userId)); | ||
| // weights were already enabled or are enabled now | ||
| const isWeightsEnabled = |
apps/web/lib/csp.ts
Outdated
| const BYTE_LEN = 22; | ||
| const bytes = new Uint8Array(BYTE_LEN); | ||
|
|
||
| return nonce; | ||
| // Prefer Web Crypto in both browser and modern Node runtimes | ||
| const hasWebCrypto = | ||
| typeof (globalThis as any).crypto !== "undefined" && | ||
| typeof (globalThis as any).crypto.getRandomValues === "function"; | ||
|
|
||
| if (hasWebCrypto) { | ||
| (globalThis as any).crypto.getRandomValues(bytes); | ||
| } else { | ||
| // Node.js environment (including tests) | ||
| // eslint-disable-next-line @typescript-eslint/no-var-requires | ||
| const nodeCrypto = require("node:crypto") as typeof import("node:crypto"); | ||
| if (nodeCrypto.webcrypto?.getRandomValues) { | ||
| nodeCrypto.webcrypto.getRandomValues(bytes); | ||
| } else { | ||
| nodeCrypto.randomFillSync(bytes); | ||
| } | ||
| } | ||
|
|
||
| return buildNonce(bytes); |
There was a problem hiding this comment.
This one has nothing to do with the implementation feature; I added it because tests were failing due to another merged PR the time I pushed
husniabad
force-pushed
the
feature/add-team-member-as-optional-guest
branch
from
b03394d to
fd33b44
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/app-store/exchange2016calendar/lib/CalendarService.ts (1)
129-161: Await Update() to ensure persistence and error propagation.
Appointment.Update(...)returns a promise; withoutawait, the function can exit early and errors won’t be caught in this try/catch. This is especially important after clearing and rebuilding attendees.Apply this diff:
- appointment.Update( + await appointment.Update( ConflictResolutionMode.AlwaysOverwrite, SendInvitationsOrCancellationsMode.SendToAllAndSaveCopy );
🧹 Nitpick comments (6)
packages/app-store/exchange2016calendar/lib/CalendarService.ts (3)
78-96: De-duplicate attendees and prevent conflicts between required and optional lists.As written, duplicates can slip in (e.g., repeated attendee emails in input, overlap between
event.attendeesand team members, or between required and optional). EWS can end up with duplicate recipients or ambiguous attendee state. Normalize and track to avoid duplicates.Proposed focused refactor within this block:
- // Add the main booker as required - for (let i = 0; i < event.attendees.length; i++) { - appointment.RequiredAttendees.Add(new Attendee(event.attendees[i].email)); - } + // Add the main booker as required (de-duped) + const seenRequired = new Set<string>(); + for (const a of event.attendees ?? []) { + const email = a?.email?.trim(); + if (!email) continue; + const l = email.toLowerCase(); + if (seenRequired.has(l)) continue; + appointment.RequiredAttendees.Add(new Attendee(email)); + seenRequired.add(l); + } // Add team members as required, ONLY if they aren't optional if (event.team?.members) { - event.team.members - .filter((member) => member.email && !optionalGuestEmails.has(member.email.toLowerCase())) - .forEach((member) => { - appointment.RequiredAttendees.Add(new Attendee(member.email)); - }); + for (const m of event.team.members ?? []) { + const email = m?.email?.trim(); + if (!email) continue; + const l = email.toLowerCase(); + if (optionalGuestEmails.has(l) || seenRequired.has(l)) continue; + appointment.RequiredAttendees.Add(new Attendee(email)); + seenRequired.add(l); + } } - // Add optional members to the optional list - if (event.optionalGuestTeamMembers) { - event.optionalGuestTeamMembers.forEach((member) => { - appointment.OptionalAttendees.Add(new Attendee(member.email)); - }); - } + // Add optional members to the optional list (skip any already required, and de-dupe) + if (event.optionalGuestTeamMembers) { + const seenOptional = new Set<string>(); + for (const m of event.optionalGuestTeamMembers) { + const email = m?.email?.trim(); + if (!email) continue; + const l = email.toLowerCase(); + if (seenOptional.has(l) || seenRequired.has(l)) continue; + appointment.OptionalAttendees.Add(new Attendee(email)); + seenOptional.add(l); + } + }
137-156: De-duplicate required/optional attendees during updates as well.Parity with createEvent avoids drift and duplicate recipients after edits.
Minimal localized changes:
- // Add the main booker as required - for (let i = 0; i < event.attendees.length; i++) { - appointment.RequiredAttendees.Add(new Attendee(event.attendees[i].email)); - } + // Add the main booker as required (de-duped) + const seenRequired = new Set<string>(); + for (const a of event.attendees ?? []) { + const email = a?.email?.trim(); + if (!email) continue; + const l = email.toLowerCase(); + if (seenRequired.has(l)) continue; + appointment.RequiredAttendees.Add(new Attendee(email)); + seenRequired.add(l); + } // Add team members as required, ONLY if they aren't optional if (event.team?.members) { - event.team.members - .filter((member) => member.email && !optionalGuestEmails.has(member.email.toLowerCase())) - .forEach((member) => { - appointment.RequiredAttendees.Add(new Attendee(member.email)); - }); + for (const m of event.team.members ?? []) { + const email = m?.email?.trim(); + if (!email) continue; + const l = email.toLowerCase(); + if (optionalGuestEmails.has(l) || seenRequired.has(l)) continue; + appointment.RequiredAttendees.Add(new Attendee(email)); + seenRequired.add(l); + } } // Add optional members to the optional list - if (event.optionalGuestTeamMembers) { - event.optionalGuestTeamMembers.forEach((member) => { - appointment.OptionalAttendees.Add(new Attendee(member.email)); - }); - } + if (event.optionalGuestTeamMembers) { + const seenOptional = new Set<string>(); + for (const m of event.optionalGuestTeamMembers) { + const email = m?.email?.trim(); + if (!email) continue; + const l = email.toLowerCase(); + if (seenOptional.has(l) || seenRequired.has(l)) continue; + appointment.OptionalAttendees.Add(new Attendee(email)); + seenOptional.add(l); + } + }
36-36: Prefer named export to comply with repository guidelines.Coding guideline recommends avoiding default exports in TS modules; consider
export class ExchangeCalendarService ...and updating imports accordingly. This can be a follow-up if other calendar modules also default-export.I can scan the repo to assess breakage risk and outline a safe migration plan if you want.
packages/app-store/feishucalendar/lib/CalendarService.ts (3)
428-435: Exclude organizer and compare emails case-insensitively to avoid self-invites.Right now you exclude only this.credential.user?.email with a case-sensitive check. If the organizer differs from the credential user (service accounts) or casing differs, they can be added as a required attendee. Normalize and exclude both:
- event.team?.members.forEach((member) => { - if ( - member.email && - member.email !== this.credential.user?.email && - !optionalGuestEmails.has(member.email.toLowerCase()) - ) { - addUniqueAttendee(member.email, false); - } - }); + const currentUserEmailLc = this.credential.user?.email?.toLowerCase(); + const organizerEmailLc = event.organizer?.email?.toLowerCase(); + event.team?.members.forEach((member) => { + const memberEmailLc = member.email?.toLowerCase(); + if ( + memberEmailLc && + memberEmailLc !== currentUserEmailLc && + memberEmailLc !== organizerEmailLc && + !optionalGuestEmails.has(memberEmailLc) + ) { + addUniqueAttendee(member.email, false); + } + });
404-444: Optional: prefer internal user IDs when available for better native behavior.Feishu treats internal “user” attendees (user_id) differently from third_party emails (richer signals like is_external, org visibility). If you have Feishu user IDs for team members in event.team.members, consider sending type: "user" with user_id for those, falling back to third_party_email otherwise. This can improve consistency with in-tenant meetings. (pkg.go.dev)
I can draft a helper that maps team members to Feishu user attendees when an id field (e.g., feishuOpenId) is present, with graceful fallback.
404-444: Add targeted tests for attendee translation.Recommend tests covering: (1) dedup across attendees/team/optionals (case-insensitive), (2) organizer/current user exclusion, (3) “required wins” precedence, and (4) optional guests flagged correctly. Since translateAttendees is private, you can extract a small pure helper (e.g., buildFeishuAttendees(event, credentialUserEmail)) for unit testing.
Happy to provide a test-ready helper + tests if you want.
📜 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 (22)
apps/web/public/static/locales/en/common.json(1 hunks)packages/app-store/exchange2013calendar/lib/CalendarService.ts(3 hunks)packages/app-store/exchange2016calendar/lib/CalendarService.ts(3 hunks)packages/app-store/exchangecalendar/lib/CalendarService.ts(4 hunks)packages/app-store/feishucalendar/lib/CalendarService.ts(1 hunks)packages/app-store/googlecalendar/lib/CalendarService.ts(2 hunks)packages/app-store/larkcalendar/lib/CalendarService.ts(1 hunks)packages/app-store/zohocalendar/lib/CalendarService.ts(2 hunks)packages/features/CalendarEventBuilder.ts(1 hunks)packages/features/bookings/lib/handleNewBooking.ts(1 hunks)packages/features/bookings/lib/handleNewBooking/getEventTypesFromDB.ts(1 hunks)packages/features/eventtypes/components/tabs/advanced/EventAdvancedTab.tsx(2 hunks)packages/features/eventtypes/components/tabs/advanced/OptionalTeamGuestSetting.tsx(1 hunks)packages/features/eventtypes/lib/types.ts(1 hunks)packages/lib/defaultEvents.ts(1 hunks)packages/lib/server/repository/eventTypeRepository.ts(2 hunks)packages/platform/atoms/event-types/hooks/useEventTypeForm.ts(1 hunks)packages/prisma/migrations/20250716153028_add_team_member_as_optional_guest/migration.sql(1 hunks)packages/prisma/schema.prisma(2 hunks)packages/trpc/server/routers/viewer/eventTypes/types.ts(1 hunks)packages/trpc/server/routers/viewer/eventTypes/update.handler.ts(3 hunks)packages/types/Calendar.d.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (19)
- packages/lib/defaultEvents.ts
- packages/app-store/exchangecalendar/lib/CalendarService.ts
- packages/platform/atoms/event-types/hooks/useEventTypeForm.ts
- packages/prisma/migrations/20250716153028_add_team_member_as_optional_guest/migration.sql
- packages/features/bookings/lib/handleNewBooking.ts
- packages/app-store/exchange2013calendar/lib/CalendarService.ts
- packages/types/Calendar.d.ts
- packages/app-store/googlecalendar/lib/CalendarService.ts
- packages/trpc/server/routers/viewer/eventTypes/types.ts
- packages/prisma/schema.prisma
- apps/web/public/static/locales/en/common.json
- packages/features/eventtypes/lib/types.ts
- packages/app-store/zohocalendar/lib/CalendarService.ts
- packages/lib/server/repository/eventTypeRepository.ts
- packages/features/bookings/lib/handleNewBooking/getEventTypesFromDB.ts
- packages/features/eventtypes/components/tabs/advanced/EventAdvancedTab.tsx
- packages/features/CalendarEventBuilder.ts
- packages/features/eventtypes/components/tabs/advanced/OptionalTeamGuestSetting.tsx
- packages/app-store/larkcalendar/lib/CalendarService.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*Service.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Service files must include
Servicesuffix, use PascalCase matching exported class, and avoid generic names (e.g.,MembershipService.ts)
Files:
packages/app-store/exchange2016calendar/lib/CalendarService.tspackages/app-store/feishucalendar/lib/CalendarService.ts
**/*.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:
packages/app-store/exchange2016calendar/lib/CalendarService.tspackages/app-store/feishucalendar/lib/CalendarService.tspackages/trpc/server/routers/viewer/eventTypes/update.handler.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:
packages/app-store/exchange2016calendar/lib/CalendarService.tspackages/app-store/feishucalendar/lib/CalendarService.tspackages/trpc/server/routers/viewer/eventTypes/update.handler.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:
packages/app-store/exchange2016calendar/lib/CalendarService.tspackages/app-store/feishucalendar/lib/CalendarService.tspackages/trpc/server/routers/viewer/eventTypes/update.handler.ts
🧠 Learnings (3)
📚 Learning: 2025-08-05T12:04:29.037Z
Learnt from: din-prajapati
PR: calcom/cal.com#21854
File: packages/app-store/office365calendar/__tests__/unit_tests/SubscriptionManager.test.ts:0-0
Timestamp: 2025-08-05T12:04:29.037Z
Learning: In packages/app-store/office365calendar/lib/CalendarService.ts, the fetcher method in Office365CalendarService class is public, not private. It was specifically changed from private to public in this PR to support proper testing and external access patterns.
Applied to files:
packages/app-store/exchange2016calendar/lib/CalendarService.ts
📚 Learning: 2025-08-17T22:00:16.329Z
Learnt from: Udit-takkar
PR: calcom/cal.com#22995
File: packages/trpc/server/routers/viewer/aiVoiceAgent/_router.ts:117-126
Timestamp: 2025-08-17T22:00:16.329Z
Learning: In calcom/cal.com PR #22995, packages/trpc/server/routers/viewer/aiVoiceAgent/_router.ts, the enabled input parameter in the update endpoint is intentionally not forwarded to aiService.updateAgentConfiguration() as the enabled/disabled agent functionality is not required at the moment (per maintainer Udit-takkar). Future reviews should not flag this as missing functionality unless requirements change.
Applied to files:
packages/trpc/server/routers/viewer/eventTypes/update.handler.ts
📚 Learning: 2025-08-21T13:44:06.784Z
Learnt from: supalarry
PR: calcom/cal.com#23217
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/services/output-event-types.service.ts:93-94
Timestamp: 2025-08-21T13:44:06.784Z
Learning: In apps/api/v2/src/ee/event-types/event-types_2024_06_14/event-types.repository.ts, repository functions that use explicit Prisma select clauses (like getEventTypeWithSeats) are used for specific purposes and don't need to include all EventType fields like bookingRequiresAuthentication. These methods don't feed into the general OutputEventTypesService_2024_06_14 flow.
Applied to files:
packages/trpc/server/routers/viewer/eventTypes/update.handler.ts
🧬 Code graph analysis (2)
packages/app-store/exchange2016calendar/lib/CalendarService.ts (1)
packages/trpc/server/routers/publicViewer/procedures/event.ts (1)
event(8-12)
packages/trpc/server/routers/viewer/eventTypes/update.handler.ts (1)
packages/platform/libraries/index.ts (2)
TRPCError(56-56)SchedulingType(30-30)
⏰ 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). (3)
- GitHub Check: Tests / Unit
- GitHub Check: Linters / lint
- GitHub Check: Type check / check-types
🔇 Additional comments (7)
packages/app-store/exchange2016calendar/lib/CalendarService.ts (1)
105-106: Return “type” metadata looks good and aligns with integrationName.Helps downstream consumers disambiguate provider type. Please ensure client-side discriminated unions include "exchange2016_calendar".
If you want, I can generate a quick search script to confirm all consumers handle this type.
packages/app-store/feishucalendar/lib/CalendarService.ts (3)
404-417: Solid dedup/accumulation approach for attendees.Using a Set with a small helper to prevent duplicates is clean and ensures consistent is_optional flags. The order (required first, then optional) preserves intent when the same person appears in both collections.
411-414: Field mapping to Feishu API looks correct.Feishu attendees accept type: "third_party" with third_party_email and the is_optional boolean, which aligns with the payload you build here. (apifox.com)
438-441: Order guarantees “required wins” over “optional.”Adding optionals after required team members and main attendees means explicit invites remain required if duplicated. That matches the product intent.
packages/trpc/server/routers/viewer/eventTypes/update.handler.ts (3)
450-453: LGTM: team guard and membership lookup entrypointEntering the team-scoped branch and fetching accepted member IDs up-front is the right shape for the validations that follow.
478-482: LGTM: host membership guard only when hosts are providedThe conditional membership guard keyed on
hostsavoids unnecessary checks. The sub-team bypass remains intact.
681-684: Good move: use Prisma.validator forselecttypingThis improves type-safety and keeps the selection minimal (slug and schedulingType only), which aligns with the “select-only what you need” guideline.
| // Create a set of optional guest emails for easy lookup. | ||
| const optionalGuestEmails = new Set( | ||
| event.optionalGuestTeamMembers?.map((guest) => guest.email.toLowerCase()) ?? [] | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden optional guest email normalization (avoid runtime TypeError) and add typing.
If any optionalGuestTeamMembers entries are missing email, .toLowerCase() will throw. Also, type the Set to string to keep TS strictness.
Apply this diff:
- // Create a set of optional guest emails for easy lookup.
- const optionalGuestEmails = new Set(
- event.optionalGuestTeamMembers?.map((guest) => guest.email.toLowerCase()) ?? []
- );
+ // Create a set of optional guest emails for easy lookup.
+ const optionalGuestEmails = new Set<string>(
+ (event.optionalGuestTeamMembers ?? [])
+ .map((guest) => guest?.email?.trim())
+ .filter((e): e is string => !!e)
+ .map((e) => e.toLowerCase())
+ );📝 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.
| // Create a set of optional guest emails for easy lookup. | |
| const optionalGuestEmails = new Set( | |
| event.optionalGuestTeamMembers?.map((guest) => guest.email.toLowerCase()) ?? [] | |
| ); | |
| // Create a set of optional guest emails for easy lookup. | |
| const optionalGuestEmails = new Set<string>( | |
| (event.optionalGuestTeamMembers ?? []) | |
| .map((guest) => guest?.email?.trim()) | |
| .filter((e): e is string => !!e) | |
| .map((e) => e.toLowerCase()) | |
| ); |
🤖 Prompt for AI Agents
In packages/app-store/exchange2016calendar/lib/CalendarService.ts around lines
73 to 76, the code builds optionalGuestEmails without typing and calls
.toLowerCase() on possibly undefined emails which can throw; change the Set to
be typed Set<string> and only include defined emails after normalizing to
lowercase. Specifically, replace the map with a guarded transform such as
filtering out entries without an email (or using flatMap) and then pushing
guest.email.toLowerCase() into the Set so TypeScript knows the values are
strings and runtime errors are avoided.
| const optionalGuestEmails = new Set( | ||
| event.optionalGuestTeamMembers?.map((guest) => guest.email.toLowerCase()) ?? [] | ||
| ); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mirror the createEvent hardening: filter undefined emails and add Set typing.
Same risk here for .toLowerCase() on undefined.
Apply this diff:
- const optionalGuestEmails = new Set(
- event.optionalGuestTeamMembers?.map((guest) => guest.email.toLowerCase()) ?? []
- );
+ const optionalGuestEmails = new Set<string>(
+ (event.optionalGuestTeamMembers ?? [])
+ .map((guest) => guest?.email?.trim())
+ .filter((e): e is string => !!e)
+ .map((e) => e.toLowerCase())
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const optionalGuestEmails = new Set( | |
| event.optionalGuestTeamMembers?.map((guest) => guest.email.toLowerCase()) ?? [] | |
| ); | |
| const optionalGuestEmails = new Set<string>( | |
| (event.optionalGuestTeamMembers ?? []) | |
| .map((guest) => guest?.email?.trim()) | |
| .filter((e): e is string => !!e) | |
| .map((e) => e.toLowerCase()) | |
| ); |
🤖 Prompt for AI Agents
In packages/app-store/exchange2016calendar/lib/CalendarService.ts around lines
133 to 136, the current construction of optionalGuestEmails calls toLowerCase()
on guest.email without guarding against undefined; update this to mirror
createEvent hardening by first extracting emails, filtering out
undefined/null/empty values, then mapping to lower-case, and declare the
variable as a Set<string> to enforce typing; ensure the filter runs before
toLowerCase so no undefined is passed to the method.
| const optionalGuestEmails = new Set( | ||
| event.optionalGuestTeamMembers?.map((guest) => guest.email.toLowerCase()) ?? [] | ||
| ); |
There was a problem hiding this comment.
Guard against undefined emails to avoid runtime errors; also normalize/trim before dedup.
If any optionalGuestTeamMembers entry lacks email, calling toLowerCase() will throw. Additionally, trimming avoids duplicate entries that differ only by whitespace. Apply this small hardening:
- const optionalGuestEmails = new Set(
- event.optionalGuestTeamMembers?.map((guest) => guest.email.toLowerCase()) ?? []
- );
+ const optionalGuestEmails = new Set(
+ (event.optionalGuestTeamMembers ?? [])
+ .map((guest) => guest.email?.trim())
+ .filter((e): e is string => !!e)
+ .map((e) => e.toLowerCase())
+ );And make addUniqueAttendee robust to stray whitespace and compute lowercased value once:
- const addUniqueAttendee = (email: string, is_optional: boolean) => {
- if (email && !addedEmails.has(email.toLowerCase())) {
- attendeeArray.push({
- type: "third_party",
- is_optional,
- third_party_email: email,
- });
- addedEmails.add(email.toLowerCase());
- }
- };
+ const addUniqueAttendee = (email: string, is_optional: boolean) => {
+ const trimmed = email?.trim();
+ const lc = trimmed?.toLowerCase();
+ if (lc && !addedEmails.has(lc)) {
+ attendeeArray.push({
+ type: "third_party",
+ is_optional,
+ third_party_email: trimmed,
+ });
+ addedEmails.add(lc);
+ }
+ };Also applies to: 408-417
🤖 Prompt for AI Agents
In packages/app-store/feishucalendar/lib/CalendarService.ts around lines
408-422, guard against undefined/empty emails and normalize/trim before dedup:
when building optionalGuestEmails skip entries with no email, trim the email and
then compute a single lowercased value to insert into the Set (e.g. const e =
guest.email?.trim(); if (!e) continue;
optionalGuestEmails.add(e.toLowerCase())). Also update the addUniqueAttendee
helper to trim its input, skip empty strings, compute the lowercased key once,
and use that key for deduplication so stray whitespace or undefined values
cannot cause runtime errors or duplicate entries.
packages/trpc/server/routers/viewer/eventTypes/update.handler.ts
Outdated
Show resolved
Hide resolved
| // Only process hosts if they are provided | ||
| if (hosts && Array.isArray(hosts)) { | ||
| const oldHostsSet = new Set(eventType.hosts.map((oldHost) => oldHost.userId)); | ||
| const newHostsSet = new Set(hosts.map((oldHost) => oldHost.userId)); | ||
|
|
||
| const existingHosts = hosts.filter((newHost) => oldHostsSet.has(newHost.userId)); | ||
| const newHosts = hosts.filter((newHost) => !oldHostsSet.has(newHost.userId)); | ||
| const removedHosts = eventType.hosts.filter((oldHost) => !newHostsSet.has(oldHost.userId)); | ||
| const existingHosts = hosts.filter((newHost) => oldHostsSet.has(newHost.userId)); | ||
| const newHosts = hosts.filter((newHost) => !oldHostsSet.has(newHost.userId)); | ||
| const removedHosts = eventType.hosts.filter((oldHost) => !newHostsSet.has(oldHost.userId)); | ||
|
|
||
| data.hosts = { | ||
| deleteMany: { | ||
| OR: removedHosts.map((host) => ({ | ||
| userId: host.userId, | ||
| eventTypeId: id, | ||
| })), | ||
| }, | ||
| create: newHosts.map((host) => { | ||
| return { | ||
| ...host, | ||
| isFixed: data.schedulingType === SchedulingType.COLLECTIVE || host.isFixed, | ||
| priority: host.priority ?? 2, | ||
| weight: host.weight ?? 100, | ||
| groupId: host.groupId, | ||
| }; | ||
| }), | ||
| update: existingHosts.map((host) => ({ | ||
| where: { | ||
| userId_eventTypeId: { | ||
| data.hosts = { | ||
| deleteMany: { | ||
| OR: removedHosts.map((host) => ({ | ||
| userId: host.userId, | ||
| eventTypeId: id, | ||
| }, | ||
| }, | ||
| data: { | ||
| isFixed: data.schedulingType === SchedulingType.COLLECTIVE || host.isFixed, | ||
| priority: host.priority ?? 2, | ||
| weight: host.weight ?? 100, | ||
| scheduleId: host.scheduleId ?? null, | ||
| groupId: host.groupId, | ||
| })), | ||
| }, | ||
| })), | ||
| }; | ||
| create: newHosts.map((host) => { | ||
| return { | ||
| ...host, | ||
| isFixed: data.schedulingType === SchedulingType.COLLECTIVE || host.isFixed, | ||
| priority: host.priority ?? 2, | ||
| weight: host.weight ?? 100, | ||
| groupId: host.groupId, | ||
| }; | ||
| }), | ||
| update: existingHosts.map((host) => ({ | ||
| where: { | ||
| userId_eventTypeId: { | ||
| userId: host.userId, | ||
| eventTypeId: id, | ||
| }, | ||
| }, | ||
| data: { | ||
| isFixed: data.schedulingType === SchedulingType.COLLECTIVE || host.isFixed, | ||
| priority: host.priority ?? 2, | ||
| weight: host.weight ?? 100, | ||
| scheduleId: host.scheduleId ?? null, | ||
| groupId: host.groupId, | ||
| }, | ||
| })), | ||
| }; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix schedulingType dependency, include scheduleId on create, and guard deleteMany when empty
- Bug:
isFixedis computed fromdata.schedulingType, which may be undefined when the request doesn’t update schedulingType. Use the effective scheduling type from input or the existing eventType. - Inconsistency:
scheduleIdis set on update but not on create. - Robustness:
deleteManywithOR: []can error. Add the clause conditionally.
Apply this diff to the hosts block:
- // Only process hosts if they are provided
- if (hosts && Array.isArray(hosts)) {
- const oldHostsSet = new Set(eventType.hosts.map((oldHost) => oldHost.userId));
- const newHostsSet = new Set(hosts.map((oldHost) => oldHost.userId));
+ // Only process hosts if they are provided
+ if (hosts && Array.isArray(hosts)) {
+ // Use the effective schedulingType (from input if provided, otherwise current eventType)
+ const effectiveSchedulingType = (input.schedulingType ?? eventType.schedulingType) as SchedulingType;
+ const oldHostsSet = new Set(eventType.hosts.map((h) => h.userId));
+ const newHostsSet = new Set(hosts.map((h) => h.userId));
@@
- data.hosts = {
- deleteMany: {
- OR: removedHosts.map((host) => ({
- userId: host.userId,
- eventTypeId: id,
- })),
- },
- create: newHosts.map((host) => {
- return {
- ...host,
- isFixed: data.schedulingType === SchedulingType.COLLECTIVE || host.isFixed,
- priority: host.priority ?? 2,
- weight: host.weight ?? 100,
- groupId: host.groupId,
- };
- }),
- update: existingHosts.map((host) => ({
- where: {
- userId_eventTypeId: {
- userId: host.userId,
- eventTypeId: id,
- },
- },
- data: {
- isFixed: data.schedulingType === SchedulingType.COLLECTIVE || host.isFixed,
- priority: host.priority ?? 2,
- weight: host.weight ?? 100,
- scheduleId: host.scheduleId ?? null,
- groupId: host.groupId,
- },
- })),
- };
+ const hostUpdates: Prisma.EventTypeUpdateInput["hosts"] = {
+ create: newHosts.map((host) => ({
+ ...host,
+ isFixed: effectiveSchedulingType === SchedulingType.COLLECTIVE || host.isFixed,
+ priority: host.priority ?? 2,
+ weight: host.weight ?? 100,
+ scheduleId: host.scheduleId ?? null,
+ groupId: host.groupId,
+ })),
+ update: existingHosts.map((host) => ({
+ where: {
+ userId_eventTypeId: {
+ userId: host.userId,
+ eventTypeId: id,
+ },
+ },
+ data: {
+ isFixed: effectiveSchedulingType === SchedulingType.COLLECTIVE || host.isFixed,
+ priority: host.priority ?? 2,
+ weight: host.weight ?? 100,
+ scheduleId: host.scheduleId ?? null,
+ groupId: host.groupId,
+ },
+ })),
+ ...(removedHosts.length > 0
+ ? {
+ deleteMany: {
+ OR: removedHosts.map((host) => ({
+ userId: host.userId,
+ eventTypeId: id,
+ })),
+ },
+ }
+ : {}),
+ };
+ data.hosts = hostUpdates;
}Additional change required outside this hunk (to support eventType.schedulingType in the effective type calculation):
// In the eventType.findUniqueOrThrow({ select: { ... } }) above, add:
schedulingType: true,This keeps isFixed consistent even when schedulingType isn’t updated in the same request.
To confirm the above after applying the patch:
-
Verify schedulingType is selected:
-
Sanity-check that optional guests can be cleared (look for
set:withoptionalGuestTeamMembers):
🏁 Script executed:
rg -nC2 -P 'findUniqueOrThrow\([\\s\\S]*?select:[\\s\\S]*?schedulingType:\s*true' packages/trpc/server/routers/viewer/eventTypes/update.handler.tsLength of output: 148
🏁 Script executed:
rg -n 'optionalGuestTeamMembers.+set:' packages/trpc/server/routers/viewer/eventTypes/update.handler.ts -C2Length of output: 107
🏁 Script executed:
#!/bin/bash
# Verify schedulingType is selected in the initial findUniqueOrThrow call
rg -nC5 'findUniqueOrThrow' packages/trpc/server/routers/viewer/eventTypes/update.handler.ts
# Inspect hosts block to confirm scheduleId on create and unconditional deleteMany
rg -nC5 'data\.hosts' packages/trpc/server/routers/viewer/eventTypes/update.handler.tsLength of output: 946
Ensure schedulingType is selected, include scheduleId on create, and guard deleteMany
The current implementation still lacks the changes needed to keep host updates correct and robust:
- The
findUniqueOrThrowcall (around line 102) does not selectschedulingType, soeventType.schedulingTypeisn’t available when the request omitsinput.schedulingType. - In the hosts block (starting at line 488),
deleteManyis always emitted, causing an error ifremovedHostsis empty (OR: []). - The
createclause for new hosts doesn’t includescheduleId, even though theupdateclause does.
Please apply the following adjustments to the hosts block and the preceding selection:
--- a/packages/trpc/server/routers/viewer/eventTypes/update.handler.ts
+++ b/packages/trpc/server/routers/viewer/eventTypes/update.handler.ts
@@ -102,8 +102,9 @@ const eventType = await ctx.prisma.eventType.findUniqueOrThrow({
where: { id },
select: {
title: true,
- locations: true,
- description: true,
+ locations: true,
+ description: true,
+ schedulingType: true,
hostGroups: true,
optionalGuestTeamMembers: true,
// …other fields
@@ -490,18 +491, Thirty
- data.hosts = {
- deleteMany: {
- OR: removedHosts.map((host) => ({
- userId: host.userId,
- eventTypeId: id,
- })),
- },
- create: newHosts.map((host) => {
- return {
- ...host,
- isFixed: data.schedulingType === SchedulingType.COLLECTIVE || host.isFixed,
- priority: host.priority ?? 2,
- weight: host.weight ?? 100,
- groupId: host.groupId,
- };
- }),
- update: existingHosts.map((host) => ({
+ const effectiveSchedulingType = (input.schedulingType ?? eventType.schedulingType) as SchedulingType;
+ const hostUpdates: Prisma.EventTypeUpdateInput["hosts"] = {
+ create: newHosts.map((host) => ({
+ ...host,
+ isFixed: effectiveSchedulingType === SchedulingType.COLLECTIVE || host.isFixed,
+ priority: host.priority ?? 2,
+ weight: host.weight ?? 100,
+ scheduleId: host.scheduleId ?? null,
+ groupId: host.groupId,
+ })),
+ update: existingHosts.map((host) => ({
where: {
userId_eventTypeId: {
userId: host.userId,
eventTypeId: id,
},
},
+ data: {
+ isFixed: effectiveSchedulingType === SchedulingType.COLLECTIVE || host.isFixed,
+ priority: host.priority ?? 2,
+ weight: host.weight ?? 100,
+ scheduleId: host.scheduleId ?? null,
+ groupId: host.groupId,
+ },
+ })),
+ ...(removedHosts.length > 0
+ ? {
+ deleteMany: {
+ OR: removedHosts.map((host) => ({
+ userId: host.userId,
+ eventTypeId: id,
+ })),
+ },
+ }
+ : {}),
+ };
+ data.hosts = hostUpdates;These changes will:
- Guarantee
eventType.schedulingTypeis fetched for computingisFixed. - Include
scheduleIdon both create and update to keep data consistent. - Only emit
deleteManywhen there are hosts to remove, avoiding an empty-OR error.
📝 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.
| // Only process hosts if they are provided | |
| if (hosts && Array.isArray(hosts)) { | |
| const oldHostsSet = new Set(eventType.hosts.map((oldHost) => oldHost.userId)); | |
| const newHostsSet = new Set(hosts.map((oldHost) => oldHost.userId)); | |
| const existingHosts = hosts.filter((newHost) => oldHostsSet.has(newHost.userId)); | |
| const newHosts = hosts.filter((newHost) => !oldHostsSet.has(newHost.userId)); | |
| const removedHosts = eventType.hosts.filter((oldHost) => !newHostsSet.has(oldHost.userId)); | |
| const existingHosts = hosts.filter((newHost) => oldHostsSet.has(newHost.userId)); | |
| const newHosts = hosts.filter((newHost) => !oldHostsSet.has(newHost.userId)); | |
| const removedHosts = eventType.hosts.filter((oldHost) => !newHostsSet.has(oldHost.userId)); | |
| data.hosts = { | |
| deleteMany: { | |
| OR: removedHosts.map((host) => ({ | |
| userId: host.userId, | |
| eventTypeId: id, | |
| })), | |
| }, | |
| create: newHosts.map((host) => { | |
| return { | |
| ...host, | |
| isFixed: data.schedulingType === SchedulingType.COLLECTIVE || host.isFixed, | |
| priority: host.priority ?? 2, | |
| weight: host.weight ?? 100, | |
| groupId: host.groupId, | |
| }; | |
| }), | |
| update: existingHosts.map((host) => ({ | |
| where: { | |
| userId_eventTypeId: { | |
| data.hosts = { | |
| deleteMany: { | |
| OR: removedHosts.map((host) => ({ | |
| userId: host.userId, | |
| eventTypeId: id, | |
| }, | |
| }, | |
| data: { | |
| isFixed: data.schedulingType === SchedulingType.COLLECTIVE || host.isFixed, | |
| priority: host.priority ?? 2, | |
| weight: host.weight ?? 100, | |
| scheduleId: host.scheduleId ?? null, | |
| groupId: host.groupId, | |
| })), | |
| }, | |
| })), | |
| }; | |
| create: newHosts.map((host) => { | |
| return { | |
| ...host, | |
| isFixed: data.schedulingType === SchedulingType.COLLECTIVE || host.isFixed, | |
| priority: host.priority ?? 2, | |
| weight: host.weight ?? 100, | |
| groupId: host.groupId, | |
| }; | |
| }), | |
| update: existingHosts.map((host) => ({ | |
| where: { | |
| userId_eventTypeId: { | |
| userId: host.userId, | |
| eventTypeId: id, | |
| }, | |
| }, | |
| data: { | |
| isFixed: data.schedulingType === SchedulingType.COLLECTIVE || host.isFixed, | |
| priority: host.priority ?? 2, | |
| weight: host.weight ?? 100, | |
| scheduleId: host.scheduleId ?? null, | |
| groupId: host.groupId, | |
| }, | |
| })), | |
| }; | |
| } | |
| // … around line 102 in update.handler.ts … | |
| const eventType = await ctx.prisma.eventType.findUniqueOrThrow({ | |
| where: { id }, | |
| select: { | |
| title: true, | |
| locations: true, | |
| description: true, | |
| schedulingType: true, // ← newly added | |
| hostGroups: true, | |
| optionalGuestTeamMembers: true, | |
| // …other fields as before | |
| }, | |
| }); | |
| // … later, inside the `if (hosts && Array.isArray(hosts)) { … }` block … | |
| const oldHostsSet = new Set(eventType.hosts.map(oldHost => oldHost.userId)); | |
| const newHostsSet = new Set(hosts.map(newHost => newHost.userId)); | |
| const existingHosts = hosts.filter(newHost => oldHostsSet.has(newHost.userId)); | |
| const newHosts = hosts.filter(newHost => !oldHostsSet.has(newHost.userId)); | |
| const removedHosts = eventType.hosts.filter(oldHost => !newHostsSet.has(oldHost.userId)); | |
| // Compute the schedulingType to use when input.schedulingType is omitted | |
| const effectiveSchedulingType = (input.schedulingType ?? eventType.schedulingType) as SchedulingType; | |
| // Build a single hosts-update payload instead of inline data.hosts = { … } | |
| const hostUpdates: Prisma.EventTypeUpdateInput["hosts"] = { | |
| create: newHosts.map(host => ({ | |
| ...host, | |
| isFixed: effectiveSchedulingType === SchedulingType.COLLECTIVE || host.isFixed, | |
| priority: host.priority ?? 2, | |
| weight: host.weight ?? 100, | |
| scheduleId: host.scheduleId ?? null, // ← newly included | |
| groupId: host.groupId, | |
| })), | |
| update: existingHosts.map(host => ({ | |
| where: { | |
| userId_eventTypeId: { | |
| userId: host.userId, | |
| eventTypeId: id, | |
| }, | |
| }, | |
| data: { | |
| isFixed: effectiveSchedulingType === SchedulingType.COLLECTIVE || host.isFixed, | |
| priority: host.priority ?? 2, | |
| weight: host.weight ?? 100, | |
| scheduleId: host.scheduleId ?? null, | |
| groupId: host.groupId, | |
| }, | |
| })), | |
| // Only emit deleteMany when there are hosts to remove | |
| ...(removedHosts.length > 0 | |
| ? { | |
| deleteMany: { | |
| OR: removedHosts.map(host => ({ | |
| userId: host.userId, | |
| eventTypeId: id, | |
| })), | |
| }, | |
| } | |
| : {}), | |
| }; | |
| data.hosts = hostUpdates; | |
| - // original inline assignment; remove this entire block: | |
| - data.hosts = { | |
| - deleteMany: { | |
| - OR: removedHosts.map(host => ({ | |
| - userId: host.userId, | |
| - eventTypeId: id, | |
| - })), | |
| - }, | |
| - create: newHosts.map(host => ({ | |
| - … // as before, without scheduleId | |
| - })), | |
| - update: existingHosts.map(host => ({ | |
| - … // as before | |
| - })), | |
| - }; |
husniabad
force-pushed
the
feature/add-team-member-as-optional-guest
branch
from
fd33b44 to
3f546c4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
packages/trpc/server/routers/viewer/eventTypes/update.handler.ts (4)
102-193: Select schedulingType in the initial eventType fetch to support correct isFixed computationYou compute isFixed based on the effective scheduling type, but schedulingType isn’t selected here. Without it, effectiveSchedulingType falls back incorrectly and can yield wrong isFixed flags.
Apply this diff:
select: { title: true, locations: true, description: true, + schedulingType: true, seatsPerTimeSlot: true, recurringEvent: true,
98-99: Don’t default optionalGuestTeamMembers; it blocks “clear all” and conflates no-op vs. replaceUndefined must mean “no change”, and [] must mean “clear all”. The current default to [] prevents clients from clearing previously saved optional guests.
Apply this diff:
- hostGroups, - optionalGuestTeamMembers = [], + hostGroups, + optionalGuestTeamMembers, ...rest
453-474: Enable clearing, avoid double-writes, and tighten team membership check for optional guests
- Gate should run when the field is provided (even if empty) so clients can clear all.
- Use a Set for O(1) membership checks.
- Replace entire relation in one go with set: [...] (handles both replace and clear) instead of set: [] + connect: [...].
This fixes a functional bug (cannot clear) and reduces redundant writes.
Apply this diff:
- if (optionalGuestTeamMembers && optionalGuestTeamMembers.length > 0) { - // Deduplicate IDs to prevent duplicate connections - const ids = optionalGuestTeamMembers.map(({ id }) => id); - const uniqueIds = Array.from(new Set(ids)); - if (uniqueIds.length !== ids.length) { - throw new TRPCError({ - code: "BAD_REQUEST", - message: "Duplicate optional guest team members are not allowed.", - }); - } - if (optionalGuestTeamMembers.every((each) => teamMemberIds.includes(each.id))) { - data.optionalGuestTeamMembers = { - set: [], - connect: uniqueIds.map((id) => ({ id })), - }; - } else { - throw new TRPCError({ - code: "FORBIDDEN", - message: "You can only assign team members as optional guests.", - }); - } - } + if (optionalGuestTeamMembers !== undefined) { + // Deduplicate IDs to prevent duplicate connections + const ids = optionalGuestTeamMembers.map(({ id }) => id); + const uniqueIds = Array.from(new Set(ids)); + if (uniqueIds.length !== ids.length) { + throw new TRPCError({ + code: "BAD_REQUEST", + message: "Duplicate optional guest team members are not allowed.", + }); + } + const teamMemberIdSet = new Set(teamMemberIds); + if (!uniqueIds.every((id) => teamMemberIdSet.has(id))) { + throw new TRPCError({ + code: "FORBIDDEN", + message: "You can only assign team members as optional guests.", + }); + } + // Replace the entire set; also clears when uniqueIds is [] + data.optionalGuestTeamMembers = { + set: uniqueIds.map((id) => ({ id })), + }; + }
484-525: Fix isFixed calculation, include scheduleId on create, and guard deleteMany when empty
- isFixed currently derives from data.schedulingType, which may be undefined if schedulingType isn’t updated in this request, yielding wrong values. Use the effective type: input.schedulingType ?? eventType.schedulingType.
- Missing scheduleId on create (present on update) creates inconsistent host rows.
- deleteMany with OR: [] can error on some Prisma versions; add conditionally.
These are correctness issues that can break host assignment.
Apply this diff:
- // Only process hosts if they are provided - if (hosts && Array.isArray(hosts)) { - const oldHostsSet = new Set(eventType.hosts.map((oldHost) => oldHost.userId)); - const newHostsSet = new Set(hosts.map((oldHost) => oldHost.userId)); - - const existingHosts = hosts.filter((newHost) => oldHostsSet.has(newHost.userId)); - const newHosts = hosts.filter((newHost) => !oldHostsSet.has(newHost.userId)); - const removedHosts = eventType.hosts.filter((oldHost) => !newHostsSet.has(oldHost.userId)); - - data.hosts = { - deleteMany: { - OR: removedHosts.map((host) => ({ - userId: host.userId, - eventTypeId: id, - })), - }, - create: newHosts.map((host) => { - return { - ...host, - isFixed: data.schedulingType === SchedulingType.COLLECTIVE || host.isFixed, - priority: host.priority ?? 2, - weight: host.weight ?? 100, - groupId: host.groupId, - }; - }), - update: existingHosts.map((host) => ({ - where: { - userId_eventTypeId: { - userId: host.userId, - eventTypeId: id, - }, - }, - data: { - isFixed: data.schedulingType === SchedulingType.COLLECTIVE || host.isFixed, - priority: host.priority ?? 2, - weight: host.weight ?? 100, - scheduleId: host.scheduleId ?? null, - groupId: host.groupId, - }, - })), - }; - } + // Only process hosts if they are provided + if (hosts && Array.isArray(hosts)) { + const effectiveSchedulingType = (input.schedulingType ?? eventType.schedulingType) as SchedulingType; + const oldHostsSet = new Set(eventType.hosts.map((h) => h.userId)); + const newHostsSet = new Set(hosts.map((h) => h.userId)); + + const existingHosts = hosts.filter((h) => oldHostsSet.has(h.userId)); + const newHosts = hosts.filter((h) => !oldHostsSet.has(h.userId)); + const removedHosts = eventType.hosts.filter((h) => !newHostsSet.has(h.userId)); + + const hostUpdates: Prisma.EventTypeUpdateInput["hosts"] = { + create: newHosts.map((host) => ({ + ...host, + isFixed: effectiveSchedulingType === SchedulingType.COLLECTIVE || host.isFixed, + priority: host.priority ?? 2, + weight: host.weight ?? 100, + scheduleId: host.scheduleId ?? null, + groupId: host.groupId, + })), + update: existingHosts.map((host) => ({ + where: { + userId_eventTypeId: { + userId: host.userId, + eventTypeId: id, + }, + }, + data: { + isFixed: effectiveSchedulingType === SchedulingType.COLLECTIVE || host.isFixed, + priority: host.priority ?? 2, + weight: host.weight ?? 100, + scheduleId: host.scheduleId ?? null, + groupId: host.groupId, + }, + })), + ...(removedHosts.length > 0 + ? { + deleteMany: { + OR: removedHosts.map((host) => ({ + userId: host.userId, + eventTypeId: id, + })), + }, + } + : {}), + }; + data.hosts = hostUpdates; + }
🧹 Nitpick comments (4)
packages/app-store/googlecalendar/lib/__tests__/CalendarService.test.ts (3)
1318-1333: Good shift to “functional equivalence” for chunking; add DST-crossing test cases for stronger justificationThe reframing aligns with our guidance to prefer native Date in hot paths. To fully justify the 1-hour tolerance, consider adding at least one case that actually crosses a DST boundary with non-UTC offsets (e.g., America/New_York: 2024-03-10 and 2024-11-03). That ensures we’re not masking issues and proves why the tolerance exists.
Apply this focused augmentation to the test cases array:
const testCases = [ { dateFrom: "2024-01-01T00:00:00Z", dateTo: "2024-04-01T00:00:00Z", // 91 days - requires chunking name: "91 days (minimal chunking)", }, { dateFrom: "2024-01-01T00:00:00Z", dateTo: "2024-07-01T00:00:00Z", // 182 days - multiple chunks name: "182 days (multiple chunks)", }, + // Cross a US DST transition (spring forward) + { + dateFrom: "2024-02-15T00:00:00-05:00", + dateTo: "2024-05-15T00:00:00-04:00", + name: "Spans DST start (America/New_York)", + }, + // Cross a US DST transition (fall back) + { + dateFrom: "2024-09-01T00:00:00-04:00", + dateTo: "2024-12-15T00:00:00-05:00", + name: "Spans DST end (America/New_York)", + }, ];
1400-1401: Prefer debug-level logging in tests to reduce noiseInfo-level logs add noise to CI output. Drop this to debug so failures remain readable.
- log.info(`${testCase.name} - Generated ${newChunks.length} functionally equivalent chunks`); + log.debug(`${testCase.name} - Generated ${newChunks.length} functionally equivalent chunks`);
1384-1398: Refine chunking test tolerance and update sanity-check commandThe 1-hour blanket tolerance may conceal genuine off-by-one-hour issues when working purely with “Z” (UTC) timestamps. Instead:
- Compute the tolerance dynamically:
- If all start/end strings end with “Z” (Zulu), require exact equality (0 ms tolerance).
- Otherwise allow up to 1 hour (60 * 60 * 1000 ms) for genuine offset/DST differences.
- Additionally assert there are no unintended gaps or overlaps between consecutive chunks.
Targeted diff (inside
packages/app-store/googlecalendar/lib/__tests__/CalendarService.test.tsaround lines 1384–1398):// Verify functionally equivalent chunking results expect(newChunks).toHaveLength(oldChunks.length); - for (let i = 0; i < oldChunks.length; i++) { - // Allow for small timezone differences (up to 1 hour) due to DST handling differences - const oldStart = new Date(oldChunks[i].start).getTime(); - const newStart = new Date(newChunks[i].start).getTime(); - const oldEnd = new Date(oldChunks[i].end).getTime(); - const newEnd = new Date(newChunks[i].end).getTime(); - const startDiff = Math.abs(oldStart - newStart); - const endDiff = Math.abs(oldEnd - newEnd); - // Allow up to 1 hour difference for timezone/DST handling - expect(startDiff).toBeLessThanOrEqual(60 * 60 * 1000); // 1 hour in ms - expect(endDiff).toBeLessThanOrEqual(60 * 60 * 1000); // 1 hour in ms - } + let prevNewEnd: number | null = null; + const oneHourMs = 60 * 60 * 1000; + const oneMinuteMs = 60 * 1000; + + for (let i = 0; i < oldChunks.length; i++) { + const oldStart = new Date(oldChunks[i].start).getTime(); + const newStart = new Date(newChunks[i].start).getTime(); + const oldEnd = new Date(oldChunks[i].end).getTime(); + const newEnd = new Date(newChunks[i].end).getTime(); + + // If all timestamps use Zulu (“Z”), require exact match; otherwise allow up to 1 hour + const isZulu = + oldChunks[i].start.endsWith("Z") && + oldChunks[i].end.endsWith("Z") && + newChunks[i].start.endsWith("Z") && + newChunks[i].end.endsWith("Z"); + const toleranceMs = isZulu ? 0 : oneHourMs; + + expect(Math.abs(oldStart - newStart)).toBeLessThanOrEqual(toleranceMs); + expect(Math.abs(oldEnd - newEnd)).toBeLessThanOrEqual(toleranceMs); + + // Ensure no overlaps and no gaps larger than the intended 1-minute resolution + if (prevNewEnd !== null) { + const gap = newStart - prevNewEnd; + expect(gap).toBeGreaterThanOrEqual(0); + expect(gap).toBeLessThanOrEqual(oneMinuteMs); + } + prevNewEnd = newEnd; + }You can sanity-check this locally by running the package’s tests directly. For example:
pnpm --filter @calcom/app-store-googlecalendar run test -- -t "chunking logic should produce functionally equivalent results"If this command needs adjustment in your workspace setup (e.g., using
pnpm -Foryarn workspace), please adapt accordingly and confirm all assertions pass without unexpected DST-related discrepancies.packages/trpc/server/routers/viewer/eventTypes/update.handler.ts (1)
450-451: Membership checks look correct; minor perf/readability nitUsing includes() repeatedly over an array is fine here, but a Set for teamMemberIds would be more consistent with the optional guest block and O(1). Optional tweak only.
Example:
- const teamMemberIds = await membershipRepo.listAcceptedTeamMemberIds({ teamId }); + const teamMemberIds = await membershipRepo.listAcceptedTeamMemberIds({ teamId }); + const teamMemberIdSet = new Set(teamMemberIds); @@ - if (hosts && !hosts.every((host) => teamMemberIds.includes(host.userId)) && !eventType.team?.parentId) { + if (hosts && !hosts.every((host) => teamMemberIdSet.has(host.userId)) && !eventType.team?.parentId) { throw new TRPCError({ code: "FORBIDDEN", }); }Also applies to: 478-482
📜 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 (3)
packages/app-store/googlecalendar/lib/__tests__/CalendarService.test.ts(2 hunks)packages/features/eventtypes/components/tabs/advanced/OptionalTeamGuestSetting.tsx(1 hunks)packages/trpc/server/routers/viewer/eventTypes/update.handler.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/features/eventtypes/components/tabs/advanced/OptionalTeamGuestSetting.tsx
🧰 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:
packages/app-store/googlecalendar/lib/__tests__/CalendarService.test.tspackages/trpc/server/routers/viewer/eventTypes/update.handler.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:
packages/app-store/googlecalendar/lib/__tests__/CalendarService.test.tspackages/trpc/server/routers/viewer/eventTypes/update.handler.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:
packages/app-store/googlecalendar/lib/__tests__/CalendarService.test.tspackages/trpc/server/routers/viewer/eventTypes/update.handler.ts
🧠 Learnings (2)
📚 Learning: 2025-07-28T11:50:23.946Z
Learnt from: CR
PR: calcom/cal.com#0
File: .cursor/rules/review.mdc:0-0
Timestamp: 2025-07-28T11:50:23.946Z
Learning: Applies to **/*.{ts,tsx} : Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js `.utc()` in hot paths like loops
Applied to files:
packages/app-store/googlecalendar/lib/__tests__/CalendarService.test.ts
📚 Learning: 2025-08-21T13:44:06.784Z
Learnt from: supalarry
PR: calcom/cal.com#23217
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/services/output-event-types.service.ts:93-94
Timestamp: 2025-08-21T13:44:06.784Z
Learning: In apps/api/v2/src/ee/event-types/event-types_2024_06_14/event-types.repository.ts, repository functions that use explicit Prisma select clauses (like getEventTypeWithSeats) are used for specific purposes and don't need to include all EventType fields like bookingRequiresAuthentication. These methods don't feed into the general OutputEventTypesService_2024_06_14 flow.
Applied to files:
packages/trpc/server/routers/viewer/eventTypes/update.handler.ts
🧬 Code graph analysis (1)
packages/trpc/server/routers/viewer/eventTypes/update.handler.ts (1)
packages/platform/libraries/index.ts (2)
TRPCError(56-56)SchedulingType(30-30)
🔇 Additional comments (1)
packages/trpc/server/routers/viewer/eventTypes/update.handler.ts (1)
681-684: Good: Minimal select via Prisma.validator keeps return payload leanSelecting only slug and schedulingType is appropriate and uses select (not include) per our Prisma query guidelines.
| optionalGuestTeamMembers = [], | ||
| ...rest |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Quick self-check script: verify field gating and set semantics are applied
Run this to sanity-check that defaults are removed, the guard uses !== undefined, and set: is used for optionalGuestTeamMembers.
Also applies to: 453-474, 484-525
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Ensure no default assignment remains
rg -nP 'optionalGuestTeamMembers\s*=\s*\[\]' packages/trpc/server/routers/viewer/eventTypes/update.handler.ts || echo "OK: no default []"
# 2) Ensure guard is '!== undefined'
rg -n 'optionalGuestTeamMembers\s*!==\s*undefined' packages/trpc/server/routers/viewer/eventTypes/update.handler.ts
# 3) Ensure prisma write uses a single set:
rg -n 'optionalGuestTeamMembers.+set:' -C2 packages/trpc/server/routers/viewer/eventTypes/update.handler.ts
# 4) Ensure hosts block computes effectiveSchedulingType and guards deleteMany:
rg -n 'effectiveSchedulingType' -C2 packages/trpc/server/routers/viewer/eventTypes/update.handler.ts
rg -n 'deleteMany' -n -C2 packages/trpc/server/routers/viewer/eventTypes/update.handler.tsLength of output: 287
Remove default assignment for optionalGuestTeamMembers
Your snippet still has a default of = [] on line 98, so the change to remove default values hasn’t been applied. This must be addressed before merging:
- In packages/trpc/server/routers/viewer/eventTypes/update.handler.ts at line 98, remove the
= []default on theoptionalGuestTeamMembersparameter. - After removing the default, verify that you guard updates with a strict undefined check, i.e.
if (optionalGuestTeamMembers !== undefined) { // prisma update using { set: optionalGuestTeamMembers } }
- Ensure your Prisma update only uses a single
set:operation foroptionalGuestTeamMembers. - Likewise, confirm in the hosts block that you compute
effectiveSchedulingTypecorrectly and guard anydeleteManycalls behind a check (e.g.,if (effectiveSchedulingType === …)).
Please apply these fixes to align with the intended field-gating and set semantics.
🤖 Prompt for AI Agents
In packages/trpc/server/routers/viewer/eventTypes/update.handler.ts around lines
98-99, remove the default assignment "= []" from the optionalGuestTeamMembers
parameter and instead treat it as possibly undefined; update the handler so any
Prisma update for optionalGuestTeamMembers is only executed when
optionalGuestTeamMembers !== undefined, using a single set: operation inside
that guarded update; also in the hosts block compute effectiveSchedulingType
before making changes and wrap any deleteMany calls behind a strict check of
effectiveSchedulingType (e.g., only call deleteMany when effectiveSchedulingType
matches the condition) so the update only modifies fields when explicitly
provided.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/trpc/server/routers/viewer/eventTypes/update.handler.ts (2)
102-193: Select schedulingType in the initial eventType query to support effective isFixed computation
eventType.schedulingTypeis needed for the effective calculation wheninput.schedulingTypeis omitted. Add it to the select.const eventType = await ctx.prisma.eventType.findUniqueOrThrow({ where: { id }, select: { title: true, locations: true, description: true, + schedulingType: true, seatsPerTimeSlot: true, recurringEvent: true,
493-534: Compute effective schedulingType, include scheduleId on create, and guard deleteMany to avoid OR: []
isFixedis derived fromdata.schedulingType, which may be undefined when schedulingType isn’t updated in this request. Also,deleteManywithOR: []may error, andscheduleIdis set on update but not on create.Apply this diff:
- // Only process hosts if they are provided - if (hosts && Array.isArray(hosts)) { - const oldHostsSet = new Set(eventType.hosts.map((oldHost) => oldHost.userId)); - const newHostsSet = new Set(hosts.map((oldHost) => oldHost.userId)); + // Only process hosts if they are provided + if (hosts && Array.isArray(hosts)) { + const oldHostsSet = new Set(eventType.hosts.map((oldHost) => oldHost.userId)); + const newHostsSet = new Set(hosts.map((oldHost) => oldHost.userId)); @@ - data.hosts = { - deleteMany: { - OR: removedHosts.map((host) => ({ - userId: host.userId, - eventTypeId: id, - })), - }, - create: newHosts.map((host) => { - return { - ...host, - isFixed: data.schedulingType === SchedulingType.COLLECTIVE || host.isFixed, - priority: host.priority ?? 2, - weight: host.weight ?? 100, - groupId: host.groupId, - }; - }), - update: existingHosts.map((host) => ({ - where: { - userId_eventTypeId: { - userId: host.userId, - eventTypeId: id, - }, - }, - data: { - isFixed: data.schedulingType === SchedulingType.COLLECTIVE || host.isFixed, - priority: host.priority ?? 2, - weight: host.weight ?? 100, - scheduleId: host.scheduleId ?? null, - groupId: host.groupId, - }, - })), - }; + const effectiveSchedulingType = (input.schedulingType ?? eventType.schedulingType) as SchedulingType; + const hostUpdates: Prisma.EventTypeUpdateInput["hosts"] = { + create: newHosts.map((host) => ({ + ...host, + isFixed: effectiveSchedulingType === SchedulingType.COLLECTIVE || host.isFixed, + priority: host.priority ?? 2, + weight: host.weight ?? 100, + scheduleId: host.scheduleId ?? null, + groupId: host.groupId, + })), + update: existingHosts.map((host) => ({ + where: { + userId_eventTypeId: { + userId: host.userId, + eventTypeId: id, + }, + }, + data: { + isFixed: effectiveSchedulingType === SchedulingType.COLLECTIVE || host.isFixed, + priority: host.priority ?? 2, + weight: host.weight ?? 100, + scheduleId: host.scheduleId ?? null, + groupId: host.groupId, + }, + })), + ...(removedHosts.length > 0 + ? { + deleteMany: { + OR: removedHosts.map((host) => ({ + userId: host.userId, + eventTypeId: id, + })), + }, + } + : {}), + }; + data.hosts = hostUpdates;
🧹 Nitpick comments (1)
packages/trpc/server/routers/viewer/eventTypes/update.handler.ts (1)
583-589: Remove console.log noise (and potential PII) from server pathUse
logger.debugwith minimal structured fields or drop entirely. Avoid logging arrays of links or full payloads.- console.log("multiplePrivateLinks", multiplePrivateLinks); + // logger.debug({ eventTypeId: id, multiplePrivateLinksCount: multiplePrivateLinks?.length ?? 0 }, "multiplePrivateLinks update"); @@ - console.log("connectedLinks", connectedLinks); + // logger.debug({ eventTypeId: id, connectedLinksCount: connectedLinks.length }, "connected links fetched");
📜 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/trpc/server/routers/viewer/eventTypes/update.handler.ts(3 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:
packages/trpc/server/routers/viewer/eventTypes/update.handler.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:
packages/trpc/server/routers/viewer/eventTypes/update.handler.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:
packages/trpc/server/routers/viewer/eventTypes/update.handler.ts
🧠 Learnings (2)
📚 Learning: 2025-08-17T22:00:16.329Z
Learnt from: Udit-takkar
PR: calcom/cal.com#22995
File: packages/trpc/server/routers/viewer/aiVoiceAgent/_router.ts:117-126
Timestamp: 2025-08-17T22:00:16.329Z
Learning: In calcom/cal.com PR #22995, packages/trpc/server/routers/viewer/aiVoiceAgent/_router.ts, the enabled input parameter in the update endpoint is intentionally not forwarded to aiService.updateAgentConfiguration() as the enabled/disabled agent functionality is not required at the moment (per maintainer Udit-takkar). Future reviews should not flag this as missing functionality unless requirements change.
Applied to files:
packages/trpc/server/routers/viewer/eventTypes/update.handler.ts
📚 Learning: 2025-08-21T13:44:06.784Z
Learnt from: supalarry
PR: calcom/cal.com#23217
File: apps/api/v2/src/ee/event-types/event-types_2024_06_14/services/output-event-types.service.ts:93-94
Timestamp: 2025-08-21T13:44:06.784Z
Learning: In apps/api/v2/src/ee/event-types/event-types_2024_06_14/event-types.repository.ts, repository functions that use explicit Prisma select clauses (like getEventTypeWithSeats) are used for specific purposes and don't need to include all EventType fields like bookingRequiresAuthentication. These methods don't feed into the general OutputEventTypesService_2024_06_14 flow.
Applied to files:
packages/trpc/server/routers/viewer/eventTypes/update.handler.ts
🧬 Code graph analysis (1)
packages/trpc/server/routers/viewer/eventTypes/update.handler.ts (1)
packages/platform/libraries/index.ts (2)
TRPCError(56-56)SchedulingType(30-30)
⏰ 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). (1)
- GitHub Check: Install dependencies / Yarn install & cache
🔇 Additional comments (2)
packages/trpc/server/routers/viewer/eventTypes/update.handler.ts (2)
98-99: Good extraction: keeps optionalGuestTeamMembers out of ...restDestructuring
optionalGuestTeamMembersseparately prevents unintended persistence via...rest. LGTM.
690-693: Typed select via Prisma.validator is a nice improvementThis keeps the
updateprojection type-safe and includesschedulingTypefor downstream logic. LGTM.
packages/trpc/server/routers/viewer/eventTypes/update.handler.ts
Outdated
Show resolved
Hide resolved
husniabad
force-pushed
the
feature/add-team-member-as-optional-guest
branch
from
b6c02ee to
1c59531
Compare
|
Hi @kart1ka I have removed the changes done by me on the file apps/web/lib/csp.ts since it has been fixed from the source, Also I fixed some bugs including CodeRabbitAi suggestion |
|
This PR is being marked as stale due to inactivity. |
Adds a setting to event types to automatically invite selected team members as optional attendees to meetings. This does not check their availability. Closes CAL-5091
husniabad
force-pushed
the
feature/add-team-member-as-optional-guest
branch
from
1c59531 to
1865404
Compare
Hi @kart1ka and @emrysal this PR is ready, and I resolved merge conflicts again, could you please review it! |
4 participants
Adds a setting to event types to automatically invite selected team members as optional attendees to meetings. This does not check their availability. Closes CAL-5091
What does this PR do?
Visual Demo (For contributors especially)
A visual demonstration is strongly recommended, for both the original and new change (video / image - any one).
Video Demo (if applicable):
Add team member as optional guest - Watch Video
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
/claim [CAL-5091] additional settings: "add team member as optional guest #18947
Checklist