Automated Test: guest-management-enhanced #360
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>
📝 WalkthroughWalkthroughThis pull request introduces a complete "add guests to booking" feature. It includes a frontend dialog for email collection, a TRPC backend endpoint with validation and access control, email templates for organizers and attendees, UI components for multi-email input, translation keys, and handler logic that validates guests, updates the database, syncs calendar attendees, and sends notifications. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as AddGuestsDialog
participant API as TRPC Router
participant Handler as addGuestsHandler
participant DB as Database
participant Calendar as EventManager
participant Email as Email Service
User->>UI: Opens dialog & enters emails
User->>UI: Clicks Add
UI->>API: viewer.bookings.addGuests({bookingId, guests})
API->>Handler: Process request
Handler->>DB: Fetch booking with attendees/event
Handler->>Handler: Validate access (admin/organizer/attendee)
Handler->>Handler: Filter guests (unique, non-blacklisted, not existing)
Handler->>DB: Create new attendees (createMany)
Handler->>Calendar: Initialize EventManager
Calendar->>Calendar: Update calendar with new attendees
Handler->>Email: sendAddGuestsEmails(calEvent, newGuests)
Email->>Email: Compose OrganizerAddGuestsEmail
Email->>Email: Compose AttendeeAddGuestsEmail (for new guests)
Email->>Email: Compose AttendeeAddGuestsEmail (for existing attendees)
Email->>Email: Send all via Promise.all
Handler->>API: Return success
API->>UI: Success response
UI->>UI: Show success toast
UI->>UI: Close dialog & reset
UI->>User: Dialog closed, guests added
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Fix all issues with AI agents
In `@apps/web/components/dialog/AddGuestsDialog.tsx`:
- Around line 42-45: The onError handler builds message with `${err.data?.code}:
${t(err.message)}` which yields "undefined: ..." and prevents the fallback; fix
by checking err.data?.code before interpolating and only prepend
`${err.data.code}: ` when it exists (otherwise use just t(err.message) or fall
back to t("unable_to_add_guests")), then pass that guarded value to showToast;
update the onError function, the message variable, and the showToast call
accordingly.
In `@packages/emails/email-manager.ts`:
- Around line 539-547: The includes check is case-sensitive and can misclassify
attendees; normalize case before comparing by ensuring the newGuests collection
is lowercased (or build a Set of lowercased emails) and compare against
attendee.email.toLowerCase() when deciding between AttendeeScheduledEmail and
AttendeeAddGuestsEmail in the block that calls sendEmail for each attendee of
calendarEvent; update where newGuests is prepared or change the map callback to
use a lowercased comparison so matches are reliable across cases.
- Around line 525-550: The sendAddGuestsEmails function currently ignores
eventTypeMetadata gating; update sendAddGuestsEmails(calendarEvent:
CalendarEvent, newGuests: string[], eventTypeMetadata?) to accept the
eventTypeMetadata param (or retrieve it like other send* functions), call
formatCalEvent as before, and before pushing OrganizerAddGuestsEmail or per-team
OrganizerAddGuestsEmail use the eventTypeDisableHostEmail flag to skip
host/organizer emails; likewise, when mapping calendarEvent.attendees use
eventTypeDisableAttendeeEmail to skip attendee emails (but still send
AttendeeScheduledEmail for newly scheduled guests if that logic is intended),
relying on the existing sendEmail, formatCalEvent, OrganizerAddGuestsEmail,
AttendeeAddGuestsEmail and AttendeeScheduledEmail symbols to gate sends
consistently with the other send* functions.
In `@packages/emails/templates/organizer-add-guests-email.ts`:
- Around line 26-30: The subject construction uses
this.calEvent.attendees[0].name which will throw if attendees is empty; update
the code that builds subject (the template using this.t with eventType:
this.calEvent.type, name: this.calEvent.attendees[0].name, date:
this.getFormattedDate()) to defensively access the first attendee (e.g. optional
chaining or a fallback like a generic name/empty string) and ensure attendees
exists and has length before reading [0].name so the subject generation never
throws.
In `@packages/trpc/server/routers/viewer/bookings/_router.tsx`:
- Around line 79-95: The authorization check in addGuestsHandler incorrectly
requires both admin and owner; locate the isTeamAdminOrOwner assignment (uses
isTeamAdmin and isTeamOwner with booking.eventType?.teamId) and change the
logical operator from && to || so isTeamAdminOrOwner is true when the user is
either a team admin or a team owner; keep the rest of the authorization flow
intact.
In `@packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts`:
- Around line 46-48: The variable isTeamAdminOrOwner is incorrectly computed
with a logical AND; change the check to use OR so a user who is either a team
admin or a team owner passes. Locate the expression that assigns
isTeamAdminOrOwner (it calls isTeamAdmin(...) and isTeamOwner(...) with
booking.eventType?.teamId) and replace the && between those awaits with ||,
preserving the await calls and the teamId fallback.
- Line 168: The code calls sendAddGuestsEmails(evt, guests) using the original
unfiltered guests list, so emails may be sent to blacklisted or already-existing
addresses; change the call to use the filtered uniqueGuests variable instead
(i.e., call sendAddGuestsEmails(evt, uniqueGuests)) so only the
deduplicated/validated guest list is emailed, and ensure no other downstream
uses still reference the unfiltered guests list.
- Around line 70-78: The filter comparing guest emails against the blacklist and
existing attendees doesn't normalize guest emails, so mixed-case addresses can
bypass checks; update the uniqueGuests computation to compare lowercase forms by
mapping or lowercasing each guest (guests) before comparing against
blacklistedGuestEmails (already lowercased) and booking.attendees' emails
(lowercase attendee.email or compare attendee.email.toLowerCase()), ensuring
both blacklist and attendee checks use guest.toLowerCase() when evaluating
membership in blacklistedGuestEmails and booking.attendees.
- Around line 165-171: Wrap the call to
eventManager.updateCalendarAttendees(evt, booking) in a try/catch like
sendAddGuestsEmails so a calendar API failure doesn’t throw after DB changes; in
the catch log the error object (include err or error.stack) and return/emit a
partial-success result or flag so the caller knows attendees were saved but
calendar update failed (refer to updateCalendarAttendees, sendAddGuestsEmails,
evt, booking, guests). Ensure the catch does not swallow the error
silently—replace the generic console.log("Error sending AddGuestsEmails") with a
detailed log including the caught error for both the calendar update and email
send paths.
In `@packages/ui/form/MultiEmail.tsx`:
- Line 37: The empty label fragment in MultiEmail (label={<></>}) removes
accessible labeling for each EmailField—replace the empty fragment with an
aria-label on the EmailField (e.g., aria-label={t("guest_email")} or a unique
aria-label per input) so screen readers can identify each input; update the
EmailField props where label={<></>} is used in the MultiEmail component to pass
aria-label instead and ensure the shared label remains for visual UI only.
- Around line 25-55: The list uses index as the React key in value.map which
causes focus/cursor bugs when you splice items; change the value state from an
array of strings to an array of objects with a stable id (e.g., {id, email})
when creating new entries, then update the map to use that id as the key, update
EmailField to read/write the email property (handlers in the onChange and
onClickAddon should locate the item by id and update/remove it via setValue),
and ensure any add-new-entry code generates a unique id for each item so keys
remain stable across reorders/removals.
🧹 Nitpick comments (4)
packages/ui/form/MultiEmail.tsx (1)
12-14: Nit: Prefer a localconstover reassigning the parameter.Line 14 reassigns the
valueparameter. Using a local const is clearer and avoids parameter mutation.♻️ Suggested refactor
-function MultiEmail({ value, readOnly, label, setValue, placeholder }: MultiEmailProps) { +function MultiEmail({ value: valueProp, readOnly, label, setValue, placeholder }: MultiEmailProps) { const { t } = useLocale(); - value = value || []; + const value = valueProp || [];apps/web/components/dialog/AddGuestsDialog.tsx (2)
26-29: MoveZAddGuestsInputSchemaoutside the component.This Zod schema is invariant and gets recreated on every render. Defining it at module scope avoids unnecessary allocations.
♻️ Proposed fix
+const ZAddGuestsInputSchema = z.array(z.string().email()).refine((emails) => { + const uniqueEmails = new Set(emails); + return uniqueEmails.size === emails.length; +}); + export const AddGuestsDialog = (props: IAddGuestsDialog) => { const { t } = useLocale(); - const ZAddGuestsInputSchema = z.array(z.string().email()).refine((emails) => { - const uniqueEmails = new Set(emails); - return uniqueEmails.size === emails.length; - });
48-58:isInvalidEmailis never cleared when the user modifies the email list.Once the validation error flag is set, it persists until the user clicks Cancel. Consider clearing it in
handleAddbefore re-validating, or whensetMultiEmailValueis called, so the error disappears as soon as the user corrects the input.♻️ Proposed fix — clear on re-attempt
const handleAdd = () => { + setIsInvalidEmail(false); if (multiEmailValue.length === 0) { return; }packages/trpc/server/routers/viewer/bookings/addGuests.schema.ts (1)
3-6: Server schema lacks the uniqueness and minimum-length constraints present on the client.The client-side
ZAddGuestsInputSchemaenforces unique emails via.refine(), but the server schema doesn't. This means duplicate or empty guest arrays pass server-side validation, shifting that responsibility entirely to the handler.Consider adding
.min(1)and a uniqueness refinement here so the tRPC layer rejects obviously invalid payloads before the handler runs.♻️ Proposed fix
export const ZAddGuestsInputSchema = z.object({ bookingId: z.number(), - guests: z.array(z.string().email()), + guests: z + .array(z.string().email()) + .min(1) + .refine((emails) => new Set(emails).size === emails.length, { + message: "emails_must_be_unique_valid", + }), });
| onError: (err) => { | ||
| const message = `${err.data?.code}: ${t(err.message)}`; | ||
| showToast(message || t("unable_to_add_guests"), "error"); | ||
| }, |
There was a problem hiding this comment.
Error toast always renders the template literal, bypassing the fallback.
When err.data?.code is undefined, the message becomes "undefined: <translated>", which is always truthy, so the || t("unable_to_add_guests") fallback never activates. Guard the code before interpolating.
🐛 Proposed fix
onError: (err) => {
- const message = `${err.data?.code}: ${t(err.message)}`;
- showToast(message || t("unable_to_add_guests"), "error");
+ const message = err.data?.code ? `${err.data.code}: ${t(err.message)}` : t("unable_to_add_guests");
+ showToast(message, "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.
| onError: (err) => { | |
| const message = `${err.data?.code}: ${t(err.message)}`; | |
| showToast(message || t("unable_to_add_guests"), "error"); | |
| }, | |
| onError: (err) => { | |
| const message = err.data?.code ? `${err.data.code}: ${t(err.message)}` : t("unable_to_add_guests"); | |
| showToast(message, "error"); | |
| }, |
🤖 Prompt for AI Agents
In `@apps/web/components/dialog/AddGuestsDialog.tsx` around lines 42 - 45, The
onError handler builds message with `${err.data?.code}: ${t(err.message)}` which
yields "undefined: ..." and prevents the fallback; fix by checking
err.data?.code before interpolating and only prepend `${err.data.code}: ` when
it exists (otherwise use just t(err.message) or fall back to
t("unable_to_add_guests")), then pass that guarded value to showToast; update
the onError function, the message variable, and the showToast call accordingly.
| 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)); | ||
| } | ||
| }) | ||
| ); | ||
|
|
||
| await Promise.all(emailsToSend); | ||
| }; |
There was a problem hiding this comment.
Missing disableStandardEmails checks, unlike every other send function in this file.
All sibling functions (sendScheduledEmails, sendLocationChangeEmails, sendCancelledEmails, etc.) accept eventTypeMetadata and conditionally skip host/attendee emails via eventTypeDisableHostEmail / eventTypeDisableAttendeeEmail. This function skips those checks, so organizers and attendees who opted out of standard emails will still receive "guests added" notifications.
🐛 Proposed fix — add metadata gating
-export const sendAddGuestsEmails = async (calEvent: CalendarEvent, newGuests: string[]) => {
+export const sendAddGuestsEmails = async (
+ calEvent: CalendarEvent,
+ newGuests: string[],
+ eventTypeMetadata?: EventTypeMetadata
+) => {
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 }))
- );
+ 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 }))
+ );
+ }
}
}
- emailsToSend.push(
- ...calendarEvent.attendees.map((attendee) => {
- if (newGuests.includes(attendee.email)) {
- return sendEmail(() => new AttendeeScheduledEmail(calendarEvent, attendee));
- } else {
- return sendEmail(() => new AttendeeAddGuestsEmail(calendarEvent, attendee));
- }
- })
- );
+ 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);
};📝 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.
| 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)); | |
| } | |
| }) | |
| ); | |
| await Promise.all(emailsToSend); | |
| }; | |
| 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); | |
| }; |
🤖 Prompt for AI Agents
In `@packages/emails/email-manager.ts` around lines 525 - 550, The
sendAddGuestsEmails function currently ignores eventTypeMetadata gating; update
sendAddGuestsEmails(calendarEvent: CalendarEvent, newGuests: string[],
eventTypeMetadata?) to accept the eventTypeMetadata param (or retrieve it like
other send* functions), call formatCalEvent as before, and before pushing
OrganizerAddGuestsEmail or per-team OrganizerAddGuestsEmail use the
eventTypeDisableHostEmail flag to skip host/organizer emails; likewise, when
mapping calendarEvent.attendees use eventTypeDisableAttendeeEmail to skip
attendee emails (but still send AttendeeScheduledEmail for newly scheduled
guests if that logic is intended), relying on the existing sendEmail,
formatCalEvent, OrganizerAddGuestsEmail, AttendeeAddGuestsEmail and
AttendeeScheduledEmail symbols to gate sends consistently with the other send*
functions.
| emailsToSend.push( | ||
| ...calendarEvent.attendees.map((attendee) => { | ||
| if (newGuests.includes(attendee.email)) { | ||
| return sendEmail(() => new AttendeeScheduledEmail(calendarEvent, attendee)); | ||
| } else { | ||
| return sendEmail(() => new AttendeeAddGuestsEmail(calendarEvent, attendee)); | ||
| } | ||
| }) | ||
| ); |
There was a problem hiding this comment.
Case-sensitive email comparison may miss matches.
newGuests.includes(attendee.email) is case-sensitive, but email local-parts are often stored/entered in different cases. A mismatch means a new guest would receive the "guests added" notification (for existing attendees) instead of the "scheduled" email (for new guests).
🛡️ Proposed fix — normalize to lowercase
+ const newGuestsLower = new Set(newGuests.map((e) => e.toLowerCase()));
+
emailsToSend.push(
...calendarEvent.attendees.map((attendee) => {
- if (newGuests.includes(attendee.email)) {
+ if (newGuestsLower.has(attendee.email.toLowerCase())) {
return sendEmail(() => new AttendeeScheduledEmail(calendarEvent, attendee));
} else {
return sendEmail(() => new AttendeeAddGuestsEmail(calendarEvent, attendee));
}
})
);📝 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.
| emailsToSend.push( | |
| ...calendarEvent.attendees.map((attendee) => { | |
| if (newGuests.includes(attendee.email)) { | |
| return sendEmail(() => new AttendeeScheduledEmail(calendarEvent, attendee)); | |
| } else { | |
| return sendEmail(() => new AttendeeAddGuestsEmail(calendarEvent, attendee)); | |
| } | |
| }) | |
| ); | |
| const newGuestsLower = new Set(newGuests.map((e) => e.toLowerCase())); | |
| emailsToSend.push( | |
| ...calendarEvent.attendees.map((attendee) => { | |
| if (newGuestsLower.has(attendee.email.toLowerCase())) { | |
| return sendEmail(() => new AttendeeScheduledEmail(calendarEvent, attendee)); | |
| } else { | |
| return sendEmail(() => new AttendeeAddGuestsEmail(calendarEvent, attendee)); | |
| } | |
| }) | |
| ); |
🤖 Prompt for AI Agents
In `@packages/emails/email-manager.ts` around lines 539 - 547, The includes check
is case-sensitive and can misclassify attendees; normalize case before comparing
by ensuring the newGuests collection is lowercased (or build a Set of lowercased
emails) and compare against attendee.email.toLowerCase() when deciding between
AttendeeScheduledEmail and AttendeeAddGuestsEmail in the block that calls
sendEmail for each attendee of calendarEvent; update where newGuests is prepared
or change the map callback to use a lowercased comparison so matches are
reliable across cases.
| subject: `${this.t("guests_added_event_type_subject", { | ||
| eventType: this.calEvent.type, | ||
| name: this.calEvent.attendees[0].name, | ||
| date: this.getFormattedDate(), | ||
| })}`, |
There was a problem hiding this comment.
Potential crash if attendees is empty.
this.calEvent.attendees[0].name will throw a TypeError if the attendees array is empty. While unlikely in the normal "add guests" flow, a defensive guard (or optional chaining) prevents a hard failure in edge cases.
🛡️ Proposed fix
subject: `${this.t("guests_added_event_type_subject", {
eventType: this.calEvent.type,
- name: this.calEvent.attendees[0].name,
+ name: this.calEvent.attendees[0]?.name ?? "",
date: this.getFormattedDate(),
})}`,📝 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.
| subject: `${this.t("guests_added_event_type_subject", { | |
| eventType: this.calEvent.type, | |
| name: this.calEvent.attendees[0].name, | |
| date: this.getFormattedDate(), | |
| })}`, | |
| subject: `${this.t("guests_added_event_type_subject", { | |
| eventType: this.calEvent.type, | |
| name: this.calEvent.attendees[0]?.name ?? "", | |
| date: this.getFormattedDate(), | |
| })}`, |
🤖 Prompt for AI Agents
In `@packages/emails/templates/organizer-add-guests-email.ts` around lines 26 -
30, The subject construction uses this.calEvent.attendees[0].name which will
throw if attendees is empty; update the code that builds subject (the template
using this.t with eventType: this.calEvent.type, name:
this.calEvent.attendees[0].name, date: this.getFormattedDate()) to defensively
access the first attendee (e.g. optional chaining or a fallback like a generic
name/empty string) and ensure attendees exists and has length before reading
[0].name so the subject generation never throws.
| addGuests: authedProcedure.input(ZAddGuestsInputSchema).mutation(async ({ input, ctx }) => { | ||
| if (!UNSTABLE_HANDLER_CACHE.addGuests) { | ||
| UNSTABLE_HANDLER_CACHE.addGuests = await import("./addGuests.handler").then( | ||
| (mod) => mod.addGuestsHandler | ||
| ); | ||
| } | ||
|
|
||
| // Unreachable code but required for type safety | ||
| if (!UNSTABLE_HANDLER_CACHE.addGuests) { | ||
| throw new Error("Failed to load handler"); | ||
| } | ||
|
|
||
| return UNSTABLE_HANDLER_CACHE.addGuests({ | ||
| ctx, | ||
| input, | ||
| }); | ||
| }), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find the files we need to inspect
echo "=== Looking for bookingsProcedure definition ==="
find packages/trpc/server/routers/viewer/bookings -name "*.ts" -o -name "*.tsx" | head -20Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 1398
🏁 Script executed:
#!/bin/bash
# Find util.ts or similar files that define procedures
find packages/trpc/server/routers/viewer/bookings -type f -name "*util*" -o -name "*procedure*" 2>/dev/nullRepository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 142
🏁 Script executed:
#!/bin/bash
# Find addGuests.handler.ts
find packages -type f -name "addGuests.handler.ts" 2>/dev/nullRepository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 155
🏁 Script executed:
#!/bin/bash
# Also search for where bookingsProcedure is defined
rg "bookingsProcedure\s*=" packages/trpc/server/routers/viewer/bookings --type ts --type tsx -A 5Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 117
🏁 Script executed:
#!/bin/bash
# Read util.ts to see procedure definitions
cat -n packages/trpc/server/routers/viewer/bookings/util.tsRepository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 2657
🏁 Script executed:
#!/bin/bash
# Read addGuests.handler.ts to check authorization
cat -n packages/trpc/server/routers/viewer/bookings/addGuests.handler.tsRepository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 6529
🏁 Script executed:
#!/bin/bash
# Read the _router.tsx to see the procedures used and context
cat -n packages/trpc/server/routers/viewer/bookings/_router.tsx | head -100Repository: admin-coderabbit/coderabbit-cal_dot_com-mirror
Length of output: 4603
Fix authorization logic bug in addGuestsHandler: line 47 should use || instead of &&.
The handler properly enforces authorization checks (lines 46-56), which validates your original concern. However, there's a critical logic error: the condition checking team permissions uses AND (&&) instead of OR (||).
Line 47 currently reads:
const isTeamAdminOrOwner =
(await isTeamAdmin(user.id, booking.eventType?.teamId ?? 0)) &&
(await isTeamOwner(user.id, booking.eventType?.teamId ?? 0));This requires the user to be BOTH admin AND owner, but the variable name and logic in line 54 suggest the intent is to allow users who are admin OR owner. Change && to || on line 47 to match the intended authorization pattern.
🤖 Prompt for AI Agents
In `@packages/trpc/server/routers/viewer/bookings/_router.tsx` around lines 79 -
95, The authorization check in addGuestsHandler incorrectly requires both admin
and owner; locate the isTeamAdminOrOwner assignment (uses isTeamAdmin and
isTeamOwner with booking.eventType?.teamId) and change the logical operator from
&& to || so isTeamAdminOrOwner is true when the user is either a team admin or a
team owner; keep the rest of the authorization flow intact.
| const blacklistedGuestEmails = process.env.BLACKLISTED_GUEST_EMAILS | ||
| ? process.env.BLACKLISTED_GUEST_EMAILS.split(",").map((email) => email.toLowerCase()) | ||
| : []; | ||
|
|
||
| const uniqueGuests = guests.filter( | ||
| (guest) => | ||
| !booking.attendees.some((attendee) => guest === attendee.email) && | ||
| !blacklistedGuestEmails.includes(guest) | ||
| ); |
There was a problem hiding this comment.
Blacklist bypass: guest emails are not lowercased before comparison.
The blacklist is lowercased (Line 71), but guest emails are compared as-is (Line 77). A guest email like "Blocked@Example.COM" would bypass the blacklist check. Normalize guest emails to lowercase before filtering.
🐛 Proposed fix
const uniqueGuests = guests.filter(
(guest) =>
- !booking.attendees.some((attendee) => guest === attendee.email) &&
- !blacklistedGuestEmails.includes(guest)
+ !booking.attendees.some((attendee) => guest.toLowerCase() === attendee.email.toLowerCase()) &&
+ !blacklistedGuestEmails.includes(guest.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 blacklistedGuestEmails = process.env.BLACKLISTED_GUEST_EMAILS | |
| ? process.env.BLACKLISTED_GUEST_EMAILS.split(",").map((email) => email.toLowerCase()) | |
| : []; | |
| const uniqueGuests = guests.filter( | |
| (guest) => | |
| !booking.attendees.some((attendee) => guest === attendee.email) && | |
| !blacklistedGuestEmails.includes(guest) | |
| ); | |
| const blacklistedGuestEmails = process.env.BLACKLISTED_GUEST_EMAILS | |
| ? process.env.BLACKLISTED_GUEST_EMAILS.split(",").map((email) => email.toLowerCase()) | |
| : []; | |
| const uniqueGuests = guests.filter( | |
| (guest) => | |
| !booking.attendees.some((attendee) => guest.toLowerCase() === attendee.email.toLowerCase()) && | |
| !blacklistedGuestEmails.includes(guest.toLowerCase()) | |
| ); |
🤖 Prompt for AI Agents
In `@packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts` around
lines 70 - 78, The filter comparing guest emails against the blacklist and
existing attendees doesn't normalize guest emails, so mixed-case addresses can
bypass checks; update the uniqueGuests computation to compare lowercase forms by
mapping or lowercasing each guest (guests) before comparing against
blacklistedGuestEmails (already lowercased) and booking.attendees' emails
(lowercase attendee.email or compare attendee.email.toLowerCase()), ensuring
both blacklist and attendee checks use guest.toLowerCase() when evaluating
membership in blacklistedGuestEmails and booking.attendees.
| await eventManager.updateCalendarAttendees(evt, booking); | ||
|
|
||
| try { | ||
| await sendAddGuestsEmails(evt, guests); | ||
| } catch (err) { | ||
| console.log("Error sending AddGuestsEmails"); | ||
| } |
There was a problem hiding this comment.
Calendar update failure leaves DB in inconsistent state; error details are swallowed.
updateCalendarAttendees (Line 165) is not wrapped in a try/catch. If the calendar API fails, the handler throws even though attendees were already persisted to the DB. Consider wrapping it similarly to the email call so the user still gets a success response (or at minimum a partial-success indication).
Also, Line 170 logs a generic string without the actual err, making failures hard to diagnose.
🛡️ Proposed fix
- await eventManager.updateCalendarAttendees(evt, booking);
-
try {
- await sendAddGuestsEmails(evt, guests);
+ await eventManager.updateCalendarAttendees(evt, booking);
} catch (err) {
- console.log("Error sending AddGuestsEmails");
+ console.error("Error updating calendar attendees", err);
+ }
+
+ try {
+ await sendAddGuestsEmails(evt, uniqueGuests);
+ } catch (err) {
+ console.error("Error sending AddGuestsEmails", err);
}🤖 Prompt for AI Agents
In `@packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts` around
lines 165 - 171, Wrap the call to eventManager.updateCalendarAttendees(evt,
booking) in a try/catch like sendAddGuestsEmails so a calendar API failure
doesn’t throw after DB changes; in the catch log the error object (include err
or error.stack) and return/emit a partial-success result or flag so the caller
knows attendees were saved but calendar update failed (refer to
updateCalendarAttendees, sendAddGuestsEmails, evt, booking, guests). Ensure the
catch does not swallow the error silently—replace the generic console.log("Error
sending AddGuestsEmails") with a detailed log including the caught error for
both the calendar update and email send paths.
| await eventManager.updateCalendarAttendees(evt, booking); | ||
|
|
||
| try { | ||
| await sendAddGuestsEmails(evt, guests); |
There was a problem hiding this comment.
Bug: Sends emails to raw guests instead of filtered uniqueGuests.
sendAddGuestsEmails(evt, guests) uses the original unfiltered input. This means emails will be sent to addresses that were filtered out — including blacklisted emails and already-existing attendees. This should use uniqueGuests.
🐛 Proposed fix
- await sendAddGuestsEmails(evt, guests);
+ await sendAddGuestsEmails(evt, uniqueGuests);📝 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.
| await sendAddGuestsEmails(evt, guests); | |
| await sendAddGuestsEmails(evt, uniqueGuests); |
🤖 Prompt for AI Agents
In `@packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts` at line
168, The code calls sendAddGuestsEmails(evt, guests) using the original
unfiltered guests list, so emails may be sent to blacklisted or already-existing
addresses; change the call to use the filtered uniqueGuests variable instead
(i.e., call sendAddGuestsEmails(evt, uniqueGuests)) so only the
deduplicated/validated guest list is emailed, and ensure no other downstream
uses still reference the unfiltered guests list.
| {value.map((field, index) => ( | ||
| <li key={index}> | ||
| <EmailField | ||
| disabled={readOnly} | ||
| value={field} | ||
| className={inputClassName} | ||
| onChange={(e) => { | ||
| const updatedValue = [...value]; | ||
| updatedValue[index] = e.target.value; | ||
| setValue(updatedValue); | ||
| }} | ||
| placeholder={placeholder} | ||
| label={<></>} | ||
| required | ||
| onClickAddon={() => { | ||
| const updatedValue = [...value]; | ||
| updatedValue.splice(index, 1); | ||
| setValue(updatedValue); | ||
| }} | ||
| addOnSuffix={ | ||
| !readOnly ? ( | ||
| <Tooltip content="Remove email"> | ||
| <button className="m-1" type="button"> | ||
| <Icon name="x" width={12} className="text-default" /> | ||
| </button> | ||
| </Tooltip> | ||
| ) : null | ||
| } | ||
| /> | ||
| </li> | ||
| ))} |
There was a problem hiding this comment.
Using key={index} on a mutable list can cause stale input state and focus bugs.
When an item is removed via splice (line 41), indices shift and React may misidentify which DOM node corresponds to which email. Since EmailField is controlled via the value prop, data correctness is preserved, but focus, cursor position, and any uncontrolled internal state within EmailField can behave unexpectedly.
Consider using a stable identifier (e.g., a unique ID generated when adding each entry) instead of the array index.
🤖 Prompt for AI Agents
In `@packages/ui/form/MultiEmail.tsx` around lines 25 - 55, The list uses index as
the React key in value.map which causes focus/cursor bugs when you splice items;
change the value state from an array of strings to an array of objects with a
stable id (e.g., {id, email}) when creating new entries, then update the map to
use that id as the key, update EmailField to read/write the email property
(handlers in the onChange and onClickAddon should locate the item by id and
update/remove it via setValue), and ensure any add-new-entry code generates a
unique id for each item so keys remain stable across reorders/removals.
| setValue(updatedValue); | ||
| }} | ||
| placeholder={placeholder} | ||
| label={<></>} |
There was a problem hiding this comment.
Empty label fragment reduces accessibility — consider aria-label.
label={<></>} suppresses the visible label, but screen readers won't associate a label with each input. While there's a shared <label> above (line 21), it doesn't use htmlFor tied to individual inputs. Consider adding aria-label={t("guest_email")} or similar to each EmailField.
🤖 Prompt for AI Agents
In `@packages/ui/form/MultiEmail.tsx` at line 37, The empty label fragment in
MultiEmail (label={<></>}) removes accessible labeling for each
EmailField—replace the empty fragment with an aria-label on the EmailField
(e.g., aria-label={t("guest_email")} or a unique aria-label per input) so screen
readers can identify each input; update the EmailField props where label={<></>}
is used in the MultiEmail component to pass aria-label instead and ensure the
shared label remains for visual UI only.
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit
Release Notes