Add guest management functionality to existing bookings#3
Open
ShashankFC wants to merge 1 commit into
Open
Conversation
* feat: ability to add guests via app.cal.com/bookings * fix: some update * fix: minor issue * fix: final update * update * update * add requested changes * fix type error * small update * final update * fix type error * fix location * update calender event --------- Co-authored-by: Somay Chauhan <somaychauhan98@gmail.com>
Author
|
@cubic-dev-ai review this pull request |
Contributor
@ShashankFC I have started the AI code review. It will take a few minutes to complete. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
9 issues found across 15 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="apps/web/components/dialog/AddGuestsDialog.tsx">
<violation number="1" location="apps/web/components/dialog/AddGuestsDialog.tsx:52">
P1: Empty strings in the email array are not filtered before validation. The `MultiEmail` component initializes new fields with `""`, so users who click "Add another" but don't fill in the extra field will get a validation error. Filter out empty strings before parsing and submitting.</violation>
<violation number="2" location="apps/web/components/dialog/AddGuestsDialog.tsx:54">
P2: `isInvalidEmail` is not reset when validation succeeds on a subsequent attempt. If a user first triggers a validation error, then fixes the emails and clicks Add, the error message remains visible during the mutation. Add `setIsInvalidEmail(false)` before calling `mutate`.</violation>
</file>
<file name="packages/emails/email-manager.ts">
<violation number="1" location="packages/emails/email-manager.ts:525">
P1: Missing `eventTypeMetadata` checks — this function will send emails even when the event type has standard emails disabled for host/attendees. Every other comparable function in this file (e.g., `sendLocationChangeEmails`, `sendRequestRescheduleEmail`, `sendScheduledEmails`) respects these settings. The function should accept an optional `eventTypeMetadata` parameter and wrap the organizer and attendee email sends in the corresponding disable checks.</violation>
</file>
<file name="packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts">
<violation number="1" location="packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts:47">
P1: Bug: `&&` should be `||`. The variable is named `isTeamAdminOrOwner` but uses AND logic, meaning a user must be both admin AND owner to pass. Team admins who aren't owners will be denied access.</violation>
<violation number="2" location="packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts:77">
P1: Blacklist bypass: guest emails are not lowercased before comparison against the lowercased blacklist. `"Blocked@Example.com"` would bypass `"blocked@example.com"` in the blacklist.</violation>
<violation number="3" location="packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts:168">
P1: Bug: `sendAddGuestsEmails` is called with the original `guests` array instead of `uniqueGuests`. This will send email notifications for guests that were already attendees or were blacklisted.</violation>
<violation number="4" location="packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts:170">
P2: The caught error `err` is silently discarded and only a generic string is logged via `console.log`. Include the error object in the log and consider using the project's logger instead of `console.log`.</violation>
</file>
<file name="packages/emails/templates/organizer-add-guests-email.ts">
<violation number="1" location="packages/emails/templates/organizer-add-guests-email.ts:23">
P2: Use `EMAIL_FROM_NAME` instead of `APP_NAME` for the email `from` field, consistent with the parent class `OrganizerScheduledEmail`. `EMAIL_FROM_NAME` respects the `process.env.EMAIL_FROM_NAME` override (falling back to `APP_NAME`), so using `APP_NAME` directly bypasses that configuration.</violation>
</file>
<file name="packages/ui/form/MultiEmail.tsx">
<violation number="1" location="packages/ui/form/MultiEmail.tsx:26">
P2: Using `key={index}` on a list where items can be added/removed causes React reconciliation bugs. After deleting an email from the middle of the list, subsequent input fields may display wrong values because React mismatches component instances. Use a stable unique identifier (e.g., a generated ID) instead of the array index.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if (multiEmailValue.length === 0) { | ||
| return; | ||
| } | ||
| const validationResult = ZAddGuestsInputSchema.safeParse(multiEmailValue); |
Contributor
There was a problem hiding this comment.
P1: Empty strings in the email array are not filtered before validation. The MultiEmail component initializes new fields with "", so users who click "Add another" but don't fill in the extra field will get a validation error. Filter out empty strings before parsing and submitting.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/components/dialog/AddGuestsDialog.tsx, line 52:
<comment>Empty strings in the email array are not filtered before validation. The `MultiEmail` component initializes new fields with `""`, so users who click "Add another" but don't fill in the extra field will get a validation error. Filter out empty strings before parsing and submitting.</comment>
<file context>
@@ -0,0 +1,107 @@
+ if (multiEmailValue.length === 0) {
+ return;
+ }
+ const validationResult = ZAddGuestsInputSchema.safeParse(multiEmailValue);
+ if (validationResult.success) {
+ addGuestsMutation.mutate({ bookingId, guests: multiEmailValue });
</file context>
Comment on lines
+525
to
+546
| export const sendAddGuestsEmails = async (calEvent: CalendarEvent, newGuests: string[]) => { | ||
| const calendarEvent = formatCalEvent(calEvent); | ||
|
|
||
| const emailsToSend: Promise<unknown>[] = []; | ||
| emailsToSend.push(sendEmail(() => new OrganizerAddGuestsEmail({ calEvent: calendarEvent }))); | ||
|
|
||
| if (calendarEvent.team?.members) { | ||
| for (const teamMember of calendarEvent.team.members) { | ||
| emailsToSend.push( | ||
| sendEmail(() => new OrganizerAddGuestsEmail({ calEvent: calendarEvent, teamMember })) | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| emailsToSend.push( | ||
| ...calendarEvent.attendees.map((attendee) => { | ||
| if (newGuests.includes(attendee.email)) { | ||
| return sendEmail(() => new AttendeeScheduledEmail(calendarEvent, attendee)); | ||
| } else { | ||
| return sendEmail(() => new AttendeeAddGuestsEmail(calendarEvent, attendee)); | ||
| } | ||
| }) |
Contributor
There was a problem hiding this comment.
P1: Missing eventTypeMetadata checks — this function will send emails even when the event type has standard emails disabled for host/attendees. Every other comparable function in this file (e.g., sendLocationChangeEmails, sendRequestRescheduleEmail, sendScheduledEmails) respects these settings. The function should accept an optional eventTypeMetadata parameter and wrap the organizer and attendee email sends in the corresponding disable checks.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/emails/email-manager.ts, line 525:
<comment>Missing `eventTypeMetadata` checks — this function will send emails even when the event type has standard emails disabled for host/attendees. Every other comparable function in this file (e.g., `sendLocationChangeEmails`, `sendRequestRescheduleEmail`, `sendScheduledEmails`) respects these settings. The function should accept an optional `eventTypeMetadata` parameter and wrap the organizer and attendee email sends in the corresponding disable checks.</comment>
<file context>
@@ -520,6 +522,32 @@ export const sendLocationChangeEmails = async (
await Promise.all(emailsToSend);
};
+export const sendAddGuestsEmails = async (calEvent: CalendarEvent, newGuests: string[]) => {
+ const calendarEvent = formatCalEvent(calEvent);
+
</file context>
Suggested change
| export const sendAddGuestsEmails = async (calEvent: CalendarEvent, newGuests: string[]) => { | |
| const calendarEvent = formatCalEvent(calEvent); | |
| const emailsToSend: Promise<unknown>[] = []; | |
| emailsToSend.push(sendEmail(() => new OrganizerAddGuestsEmail({ calEvent: calendarEvent }))); | |
| if (calendarEvent.team?.members) { | |
| for (const teamMember of calendarEvent.team.members) { | |
| emailsToSend.push( | |
| sendEmail(() => new OrganizerAddGuestsEmail({ calEvent: calendarEvent, teamMember })) | |
| ); | |
| } | |
| } | |
| emailsToSend.push( | |
| ...calendarEvent.attendees.map((attendee) => { | |
| if (newGuests.includes(attendee.email)) { | |
| return sendEmail(() => new AttendeeScheduledEmail(calendarEvent, attendee)); | |
| } else { | |
| return sendEmail(() => new AttendeeAddGuestsEmail(calendarEvent, attendee)); | |
| } | |
| }) | |
| export const sendAddGuestsEmails = async ( | |
| calEvent: CalendarEvent, | |
| newGuests: string[], | |
| eventTypeMetadata?: EventTypeMetadata | |
| ) => { | |
| const calendarEvent = formatCalEvent(calEvent); | |
| const emailsToSend: Promise<unknown>[] = []; | |
| if (!eventTypeDisableHostEmail(eventTypeMetadata)) { | |
| emailsToSend.push(sendEmail(() => new OrganizerAddGuestsEmail({ calEvent: calendarEvent }))); | |
| if (calendarEvent.team?.members) { | |
| for (const teamMember of calendarEvent.team.members) { | |
| emailsToSend.push( | |
| sendEmail(() => new OrganizerAddGuestsEmail({ calEvent: calendarEvent, teamMember })) | |
| ); | |
| } | |
| } | |
| } | |
| if (!eventTypeDisableAttendeeEmail(eventTypeMetadata)) { | |
| emailsToSend.push( | |
| ...calendarEvent.attendees.map((attendee) => { | |
| if (newGuests.includes(attendee.email)) { | |
| return sendEmail(() => new AttendeeScheduledEmail(calendarEvent, attendee)); | |
| } else { | |
| return sendEmail(() => new AttendeeAddGuestsEmail(calendarEvent, attendee)); | |
| } | |
| }) | |
| ); | |
| } | |
| await Promise.all(emailsToSend); | |
| }; |
| const uniqueGuests = guests.filter( | ||
| (guest) => | ||
| !booking.attendees.some((attendee) => guest === attendee.email) && | ||
| !blacklistedGuestEmails.includes(guest) |
Contributor
There was a problem hiding this comment.
P1: Blacklist bypass: guest emails are not lowercased before comparison against the lowercased blacklist. "Blocked@Example.com" would bypass "blocked@example.com" in the blacklist.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts, line 77:
<comment>Blacklist bypass: guest emails are not lowercased before comparison against the lowercased blacklist. `"Blocked@Example.com"` would bypass `"blocked@example.com"` in the blacklist.</comment>
<file context>
@@ -0,0 +1,174 @@
+ const uniqueGuests = guests.filter(
+ (guest) =>
+ !booking.attendees.some((attendee) => guest === attendee.email) &&
+ !blacklistedGuestEmails.includes(guest)
+ );
+
</file context>
| await eventManager.updateCalendarAttendees(evt, booking); | ||
|
|
||
| try { | ||
| await sendAddGuestsEmails(evt, guests); |
Contributor
There was a problem hiding this comment.
P1: Bug: sendAddGuestsEmails is called with the original guests array instead of uniqueGuests. This will send email notifications for guests that were already attendees or were blacklisted.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts, line 168:
<comment>Bug: `sendAddGuestsEmails` is called with the original `guests` array instead of `uniqueGuests`. This will send email notifications for guests that were already attendees or were blacklisted.</comment>
<file context>
@@ -0,0 +1,174 @@
+ await eventManager.updateCalendarAttendees(evt, booking);
+
+ try {
+ await sendAddGuestsEmails(evt, guests);
+ } catch (err) {
+ console.log("Error sending AddGuestsEmails");
</file context>
| if (!booking) throw new TRPCError({ code: "NOT_FOUND", message: "booking_not_found" }); | ||
|
|
||
| const isTeamAdminOrOwner = | ||
| (await isTeamAdmin(user.id, booking.eventType?.teamId ?? 0)) && |
Contributor
There was a problem hiding this comment.
P1: Bug: && should be ||. The variable is named isTeamAdminOrOwner but uses AND logic, meaning a user must be both admin AND owner to pass. Team admins who aren't owners will be denied access.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts, line 47:
<comment>Bug: `&&` should be `||`. The variable is named `isTeamAdminOrOwner` but uses AND logic, meaning a user must be both admin AND owner to pass. Team admins who aren't owners will be denied access.</comment>
<file context>
@@ -0,0 +1,174 @@
+ if (!booking) throw new TRPCError({ code: "NOT_FOUND", message: "booking_not_found" });
+
+ const isTeamAdminOrOwner =
+ (await isTeamAdmin(user.id, booking.eventType?.teamId ?? 0)) &&
+ (await isTeamOwner(user.id, booking.eventType?.teamId ?? 0));
+
</file context>
| } | ||
| const validationResult = ZAddGuestsInputSchema.safeParse(multiEmailValue); | ||
| if (validationResult.success) { | ||
| addGuestsMutation.mutate({ bookingId, guests: multiEmailValue }); |
Contributor
There was a problem hiding this comment.
P2: isInvalidEmail is not reset when validation succeeds on a subsequent attempt. If a user first triggers a validation error, then fixes the emails and clicks Add, the error message remains visible during the mutation. Add setIsInvalidEmail(false) before calling mutate.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/components/dialog/AddGuestsDialog.tsx, line 54:
<comment>`isInvalidEmail` is not reset when validation succeeds on a subsequent attempt. If a user first triggers a validation error, then fixes the emails and clicks Add, the error message remains visible during the mutation. Add `setIsInvalidEmail(false)` before calling `mutate`.</comment>
<file context>
@@ -0,0 +1,107 @@
+ }
+ const validationResult = ZAddGuestsInputSchema.safeParse(multiEmailValue);
+ if (validationResult.success) {
+ addGuestsMutation.mutate({ bookingId, guests: multiEmailValue });
+ } else {
+ setIsInvalidEmail(true);
</file context>
| try { | ||
| await sendAddGuestsEmails(evt, guests); | ||
| } catch (err) { | ||
| console.log("Error sending AddGuestsEmails"); |
Contributor
There was a problem hiding this comment.
P2: The caught error err is silently discarded and only a generic string is logged via console.log. Include the error object in the log and consider using the project's logger instead of console.log.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts, line 170:
<comment>The caught error `err` is silently discarded and only a generic string is logged via `console.log`. Include the error object in the log and consider using the project's logger instead of `console.log`.</comment>
<file context>
@@ -0,0 +1,174 @@
+ try {
+ await sendAddGuestsEmails(evt, guests);
+ } catch (err) {
+ console.log("Error sending AddGuestsEmails");
+ }
+
</file context>
| }), | ||
| method: "REQUEST", | ||
| }, | ||
| from: `${APP_NAME} <${this.getMailerOptions().from}>`, |
Contributor
There was a problem hiding this comment.
P2: Use EMAIL_FROM_NAME instead of APP_NAME for the email from field, consistent with the parent class OrganizerScheduledEmail. EMAIL_FROM_NAME respects the process.env.EMAIL_FROM_NAME override (falling back to APP_NAME), so using APP_NAME directly bypasses that configuration.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/emails/templates/organizer-add-guests-email.ts, line 23:
<comment>Use `EMAIL_FROM_NAME` instead of `APP_NAME` for the email `from` field, consistent with the parent class `OrganizerScheduledEmail`. `EMAIL_FROM_NAME` respects the `process.env.EMAIL_FROM_NAME` override (falling back to `APP_NAME`), so using `APP_NAME` directly bypasses that configuration.</comment>
<file context>
@@ -0,0 +1,38 @@
+ }),
+ method: "REQUEST",
+ },
+ from: `${APP_NAME} <${this.getMailerOptions().from}>`,
+ to: toAddresses.join(","),
+ replyTo: [this.calEvent.organizer.email, ...this.calEvent.attendees.map(({ email }) => email)],
</file context>
| </label> | ||
| <ul> | ||
| {value.map((field, index) => ( | ||
| <li key={index}> |
Contributor
There was a problem hiding this comment.
P2: Using key={index} on a list where items can be added/removed causes React reconciliation bugs. After deleting an email from the middle of the list, subsequent input fields may display wrong values because React mismatches component instances. Use a stable unique identifier (e.g., a generated ID) instead of the array index.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/ui/form/MultiEmail.tsx, line 26:
<comment>Using `key={index}` on a list where items can be added/removed causes React reconciliation bugs. After deleting an email from the middle of the list, subsequent input fields may display wrong values because React mismatches component instances. Use a stable unique identifier (e.g., a generated ID) instead of the array index.</comment>
<file context>
@@ -0,0 +1,96 @@
+ </label>
+ <ul>
+ {value.map((field, index) => (
+ <li key={index}>
+ <EmailField
+ disabled={readOnly}
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Test 10nn
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.
nn---n*Replicated from [ai-code-review-evaluation/cal.com-coderabbit#10](https://github.com/ai-code-review-evaluation/cal.com-coderabbit/pull/10)*