Add guest management functionality to existing bookings#2
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>
![grapple-pr[bot]](https://avatars.githubusercontent.com/in/3205222?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 Grapple PR Review
Summary
This PR adds a guest management feature that allows users to add guests to existing bookings, including a new dialog UI, tRPC endpoint, and email notifications to both attendees and organizers. The linked ticket (#10) references an unrelated 'async import of appStore packages' issue, suggesting a mismatch between the PR's actual changes and its stated ticket context.
Changes: 15 files (+555 / -0 lines)
- 🟢 9 files added:
apps/web/components/dialog/AddGuestsDialog.tsx,packages/emails/src/templates/AttendeeAddGuestsEmail.tsx,packages/emails/src/templates/OrganizerAddGuestsEmail.tsx,packages/emails/templates/attendee-add-guests-email.ts,packages/emails/templates/organizer-add-guests-email.ts+4 more - 🟡 6 files modified:
apps/web/components/booking/BookingListItem.tsx,apps/web/public/static/locales/en/common.json,packages/emails/email-manager.ts,packages/emails/src/templates/index.ts,packages/trpc/server/routers/viewer/bookings/_router.tsx+1 more
Resolves: #10 (Async import of the appStore packages)
CI: ⏳ 0 passed, 0 failed
Impact Diagram
graph TD
pr_0[/"🔀 PR Changes: 15 files"/]
f1["🟡 BookingListItem.tsx +15/-0"]
pr_0 --> f1
f2["🟢 AddGuestsDialog.tsx +107/-0"]
pr_0 --> f2
f3["🟡 common.json +6/-0"]
pr_0 --> f3
f4["🟡 email-manager.ts +28/-0"]
pr_0 --> f4
f5["🟢 AttendeeAddGuestsEmail.tsx +10/-0"]
pr_0 --> f5
f6["🟢 OrganizerAddGuestsEmail.tsx +11/-0"]
pr_0 --> f6
f7["🟡 index.ts +2/-0"]
pr_0 --> f7
f8["🟢 attendee-add-guests-email.ts +34/-0"]
pr_0 --> f8
f9["🟢 organizer-add-guests-email.ts +38/-0"]
pr_0 --> f9
f10["🟡 _router.tsx +19/-0"]
pr_0 --> f10
f11["🟢 addGuests.handler.ts +174/-0"]
pr_0 --> f11
f12["🟢 addGuests.schema.ts +8/-0"]
pr_0 --> f12
more_13["... +3 more files"]
pr_0 --> more_13
f5 -.- f6
f6 -.- f7
f8 -.- f9
f10 -.- f11
f11 -.- f12
style f1 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f2 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f3 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f4 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f5 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f6 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f7 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f8 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f9 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f10 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f11 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f12 fill:#7c3aed,color:#fff,stroke:#7c3aed
4 critical issues found — please address before merging.
| Severity | Count |
|---|---|
| 🔴 Critical | 4 |
| 🟠 Major | 10 |
| 🟡 Minor | 9 |
| 🔵 Info | 4 |
Review details
- Reviewed at: 2026-04-07T18:02:28.217Z
- Agents used: security, logic, style, architecture, performance, verification
- Context level: full
- Execution time: 56.3s
- Low-confidence findings suppressed: 7
- Blast radius: 0 (contained)
Powered by Grapple PR — AI that understands your code.
|
|
||
| if (!isTeamAdminOrOwner && !isOrganizer && !isAttendee) { | ||
| throw new TRPCError({ code: "FORBIDDEN", message: "you_do_not_have_permission" }); | ||
| } |
There was a problem hiding this comment.
🔴 CRITICAL — Authentication/Authorization (confidence: 100%)
Authorization logic is broken: the isTeamAdminOrOwner check uses AND (&&) instead of OR (||), meaning it only grants access if the user is BOTH a team admin AND a team owner simultaneously. In practice this is almost never true, making the team-based permission path effectively dead code. More critically, any booking attendee (isAttendee) is allowed to add guests to a booking, which grants excessive privilege — attendees should not be able to modify booking guest lists.
Evidence:
- Line 48-49:
const isTeamAdminOrOwner = (await isTeamAdmin(...)) && (await isTeamOwner(...))— logical AND means the condition is only true when the user holds both roles simultaneously, which is an unusual and overly restrictive condition for a permission grant. It likely should be||. - Line 54:
const isAttendee = !!booking.attendees.find((attendee) => attendee.email === user.email)— attendees are granted full mutation access, violating least-privilege; only the organizer or admins should add guests. - The acceptance criteria explicitly states: 'The addGuests tRPC handler must verify that the requesting user is the organizer or has permission to modify the booking before adding guests.'
- No check for
booking.statusanywhere in the handler. - Acceptance criteria: 'The BookingListItem should only show the Add Guests option for bookings that are upcoming/active (not cancelled or past)' — the UI check alone is insufficient; the server must enforce this.
- Edge case: 'Adding guests to a cancelled or past booking — the handler should reject this or the UI should prevent it.'
- Line 48:
booking.eventType?.teamId ?? 0— when teamId is null, the value0is passed to isTeamAdmin/isTeamOwner, which likely returns false for all users but may produce unexpected behavior depending on those functions' implementations. - For non-team bookings, the team admin/owner path should be skipped entirely.
- Line 51-52:
(await isTeamAdmin(user.id, booking.eventType?.teamId ?? 0)) && (await isTeamOwner(user.id, booking.eventType?.teamId ?? 0)) - The intent says 'verify that the requesting user is the organizer or has permission to modify the booking'. Team admin OR team owner should satisfy this, not requiring both.
- Line 49-50:
(await isTeamAdmin(user.id, booking.eventType?.teamId ?? 0)) && (await isTeamOwner(user.id, booking.eventType?.teamId ?? 0))— AND logic means both conditions must be true - If the intent is 'team admin OR team owner', this should use
|| - Passing
teamId=0whenbooking.eventType?.teamIdis null will query for a non-existent team - Line 53:
const isAttendee = !!booking.attendees.find((attendee) => attendee.email === user.email); - Line 55-57: If
isAttendeeis true, the user passes the authorization gate and can add guests - The intent specification states: 'The addGuests tRPC handler must verify that the requesting user is the organizer or has permission to modify the booking before adding guests'
Agent: security
| } | ||
|
|
||
| return { message: "Guests added" }; | ||
| }; |
There was a problem hiding this comment.
🔴 CRITICAL — Business Logic - Missing Validation (confidence: 100%)
The handler does not check whether the booking is cancelled or in the past before allowing guests to be added. The acceptance criteria explicitly state: 'Adding guests to a cancelled or past booking — the handler should reject this or the UI should prevent it.' There is no status check (e.g., booking.status !== 'CANCELLED') and no date check (e.g., booking.startTime is in the future).
Evidence:
- The handler fetches the booking and checks authorization but never inspects booking.status or compares booking.startTime to the current time.
- Acceptance criteria: 'The BookingListItem should only show the Add Guests option for bookings that are upcoming/active (not cancelled or past)'
- Edge case: 'Adding guests to a cancelled or past booking — the handler should reject this or the UI should prevent it'
- The
prisma.booking.findFirstquery has nowherefilter onstatus - No check on
booking.statusorbooking.startTimeafter fetching - Intent specification states: 'Adding guests to a cancelled or past booking — the handler should reject this or the UI should prevent it'
Agent: logic
| guests: z.array(z.string().email()), | ||
| }); | ||
|
|
||
| export type TAddGuestsInputSchema = z.infer<typeof ZAddGuestsInputSchema>; |
There was a problem hiding this comment.
🔴 CRITICAL — Input Validation (confidence: 99%)
The addGuests schema does not enforce a non-empty array, no maximum length limit on the guests array, and no maximum length on individual email strings. An unauthenticated-but-authenticated attacker can submit an arbitrarily large array of valid email addresses, triggering bulk attendee creation and bulk email sends (DoS/resource exhaustion). The schema also does not normalize email casing before validation.
Evidence:
- Line 4-6:
guests: z.array(z.string().email())— no.min(1)to prevent empty arrays, no.max(N)to bound array size, no.transform(e => e.toLowerCase())for normalization. - The acceptance criteria states: 'The addGuests schema must validate that guest emails are properly formatted and the array is non-empty' and 'consider input length limits in the schema'.
- The edge case section notes: 'Empty email list submitted to the addGuests endpoint — schema validation should catch this' and 'Very large numbers of guests submitted at once — consider input length limits in the schema'.
- Without a max bound, a caller can trigger O(N) database writes and O(N) email sends per request.
Agent: security
| onClick: () => { | ||
| setIsOpenAddGuestsDialog(true); | ||
| }, | ||
| icon: "user-plus" as const, |
There was a problem hiding this comment.
🔴 CRITICAL — Business Logic - Missing UI Filtering (confidence: 92%)
The 'Add Guests' action is added unconditionally to the booking action list without checking if the booking is upcoming/active. The acceptance criteria explicitly require: 'The BookingListItem should only show the Add Guests option for bookings that are upcoming/active (not cancelled or past).' The action is added in the same list as other always-visible actions, with no conditional guard.
Evidence:
- The action object with id 'add_members' is pushed into the actions array unconditionally at line 192-199.
- Compare with how other conditional actions are added later in the component (e.g., ROUND_ROBIN check at line 201).
- Acceptance criteria: 'The BookingListItem should only show the Add Guests option for bookings that are upcoming/active (not cancelled or past)'
Agent: logic
| const blacklistedGuestEmails = process.env.BLACKLISTED_GUEST_EMAILS | ||
| ? process.env.BLACKLISTED_GUEST_EMAILS.split(",").map((email) => email.toLowerCase()) | ||
| : []; | ||
|
|
There was a problem hiding this comment.
🟠 MAJOR — Input Validation (confidence: 100%)
Guest email deduplication against existing attendees uses strict equality (===) without case normalization. A guest email like User@Example.com will not match an existing attendee user@example.com, allowing duplicate attendee records to be created for the same logical email address.
Evidence:
- Line 69-73:
!booking.attendees.some((attendee) => guest === attendee.email)— no.toLowerCase()normalization on either side. - Edge case from review context: 'Guest email addresses with mixed casing (e.g., User@Example.com vs user@example.com) should be normalized before deduplication checks.'
- Email addresses are case-insensitive in the local part by convention and always case-insensitive in the domain part per RFC 5321.
Agent: security
| 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.
🟡 MINOR — Data Exposure (confidence: 76%)
tRPC error messages from the server are passed directly to i18n translation and displayed to the user. The err.message field may contain internal server error details that should not be surfaced in the UI.
Evidence:
- Line 43:
const message = \${err.data?.code}: ${t(err.message)}`—err.message` from a tRPC TRPCError may contain internal database messages or stack details if not carefully controlled server-side. - While tRPC does filter some error details, relying on client-side display of raw server error messages is a data exposure risk pattern.
Agent: security
| }) | ||
| ); | ||
|
|
||
| await Promise.all(emailsToSend); |
There was a problem hiding this comment.
🔵 INFO — Code organization (confidence: 85%)
The sendAddGuestsEmails function has inconsistent email dispatch logic. It sends OrganizerAddGuestsEmail to the organizer and team members, but then sends either AttendeeScheduledEmail or AttendeeAddGuestsEmail to attendees based on whether they are in the newGuests list. This logic appears inverted—new guests should receive AttendeeScheduledEmail, while existing attendees should receive AttendeeAddGuestsEmail.
Evidence:
- Lines 536-543: Conditional checks if attendee email is in newGuests array, then sends AttendeeScheduledEmail, else sends AttendeeAddGuestsEmail
- The naming suggests AttendeeAddGuestsEmail should notify existing attendees that NEW guests were added, not that they themselves are newly scheduled
- AttendeeScheduledEmail is the standard confirmation email for newly scheduled attendees
Agent: style
|
|
||
| try { | ||
| await sendAddGuestsEmails(evt, guests); | ||
| } catch (err) { |
There was a problem hiding this comment.
🔵 INFO — Hardcoded String (confidence: 82%)
The handler returns a hardcoded English string { message: "Guests added" } rather than an i18n key. While this is a backend response and may be acceptable, the rest of the feature uses i18n keys consistently (e.g., guests_added exists in common.json).
Evidence:
- Line 169:
return { message: "Guests added" }; - The i18n key
guests_addedis defined in common.json
Agent: architecture
| ); | ||
| } | ||
|
|
||
| export default MultiEmail; |
There was a problem hiding this comment.
🔵 INFO — Naming conventions (confidence: 75%)
Component file uses non-standard naming pattern. Per the codebase pattern, UI components are named in PascalCase but the file should follow convention—compare with AddressInputLazy.tsx, PhoneInputLazy.tsx which use lazy loader pattern. The actual component here is not lazy-loaded, only the wrapper (MultiEmailLazy.tsx) is.
Evidence:
- File 'MultiEmail.tsx' exports the actual component implementation
- File 'MultiEmailLazy.tsx' is the lazy-loaded wrapper
- Similar pattern in codebase: AddressInput.tsx (actual) with AddressInputLazy.tsx (wrapper)
- Currently exported from ui/index.tsx as MultiEmailLazy which is correct, but the implementation file naming is inconsistent
Agent: style
| guests: z.array(z.string().email()), | ||
| }); | ||
|
|
||
| export type TAddGuestsInputSchema = z.infer<typeof ZAddGuestsInputSchema>; |
There was a problem hiding this comment.
🔵 INFO — Documentation (confidence: 75%)
Missing JSDoc documentation on schema and type export. The schema enforces uniqueness at the client level but the comments in acceptance criteria emphasize that server-side validation is authoritative. Schema should document this validation behavior.
Evidence:
- Acceptance criteria: 'Client-side-only validation could be bypassed; schema-level validation in addGuests.schema.ts must be the authoritative check'
- No documentation explaining what validation is performed (email format, non-empty array, etc.)
- Related schemas in the same file structure (editLocation.schema.ts, confirm.schema.ts) typically include JSDoc comments
Agent: style
There was a problem hiding this comment.
🤖 Grapple PR Review
Summary
This PR adds guest management functionality to existing bookings, allowing users to add guests to already-confirmed bookings via a dialog UI. It introduces a new tRPC endpoint, email notifications for both attendees and organizers, a reusable MultiEmail input component, and supporting UI/locale changes.
Changes: 15 files (+555 / -0 lines)
- 🟢 9 files added:
apps/web/components/dialog/AddGuestsDialog.tsx,packages/emails/src/templates/AttendeeAddGuestsEmail.tsx,packages/emails/src/templates/OrganizerAddGuestsEmail.tsx,packages/emails/templates/attendee-add-guests-email.ts,packages/emails/templates/organizer-add-guests-email.ts+4 more - 🟡 6 files modified:
apps/web/components/booking/BookingListItem.tsx,apps/web/public/static/locales/en/common.json,packages/emails/email-manager.ts,packages/emails/src/templates/index.ts,packages/trpc/server/routers/viewer/bookings/_router.tsx+1 more
Resolves: #10 (Async import of the appStore packages)
CI: ⏳ 0 passed, 0 failed
Impact Diagram
graph TD
pr_0[/"🔀 PR Changes: 15 files"/]
f1["🟡 BookingListItem.tsx +15/-0"]
pr_0 --> f1
f2["🟢 AddGuestsDialog.tsx +107/-0"]
pr_0 --> f2
f3["🟡 common.json +6/-0"]
pr_0 --> f3
f4["🟡 email-manager.ts +28/-0"]
pr_0 --> f4
f5["🟢 AttendeeAddGuestsEmail.tsx +10/-0"]
pr_0 --> f5
f6["🟢 OrganizerAddGuestsEmail.tsx +11/-0"]
pr_0 --> f6
f7["🟡 index.ts +2/-0"]
pr_0 --> f7
f8["🟢 attendee-add-guests-email.ts +34/-0"]
pr_0 --> f8
f9["🟢 organizer-add-guests-email.ts +38/-0"]
pr_0 --> f9
f10["🟡 _router.tsx +19/-0"]
pr_0 --> f10
f11["🟢 addGuests.handler.ts +174/-0"]
pr_0 --> f11
f12["🟢 addGuests.schema.ts +8/-0"]
pr_0 --> f12
more_13["... +3 more files"]
pr_0 --> more_13
f5 -.- f6
f6 -.- f7
f8 -.- f9
f10 -.- f11
f11 -.- f12
style f1 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f2 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f3 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f4 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f5 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f6 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f7 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f8 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f9 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f10 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f11 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f12 fill:#7c3aed,color:#fff,stroke:#7c3aed
3 critical issues found — please address before merging.
| Severity | Count |
|---|---|
| 🔴 Critical | 3 |
| 🟠 Major | 6 |
| 🟡 Minor | 8 |
| 🔵 Info | 3 |
Review details
- Reviewed at: 2026-04-07T18:38:24.626Z
- Agents used: security, logic, style, architecture, performance, verification
- Context level: full
- Execution time: 99.6s
- Low-confidence findings suppressed: 6
- Blast radius: 0 (contained)
Powered by Grapple PR — AI that understands your code.
| } | ||
|
|
||
| return { message: "Guests added" }; | ||
| }; |
There was a problem hiding this comment.
🔴 CRITICAL — Missing Booking State Validation (confidence: 100%)
The handler does not check the booking status before adding guests. According to the acceptance criteria, adding guests to a cancelled or past booking should be rejected. The handler allows adding guests to cancelled, rejected, or past bookings, which is a data integrity issue.
Evidence:
- No check for
booking.status(e.g., CANCELLED, REJECTED) - No check for
booking.endTime < now(past bookings) - Acceptance criteria: 'Adding a guest to a cancelled or past booking — the handler should reject or guard against this'
Agent: logic
| setIsOpenAddGuestsDialog(true); | ||
| }, | ||
| icon: "user-plus" as const, | ||
| }, |
There was a problem hiding this comment.
🔴 CRITICAL — Conditional Rendering (confidence: 100%)
The 'Add Guests' action is unconditionally added to the booking actions array for all bookings, regardless of booking status. This means the option appears for cancelled, rejected, and past bookings. The acceptance criteria explicitly states: 'BookingListItem only shows the Add Guests option for bookings in an appropriate state (e.g., upcoming, confirmed — not cancelled or past)'.
Evidence:
- The add_members action is pushed into the actions array at line 193-200 without any conditional check
- Other actions in the component (like 'Reschedule') have conditional logic based on booking state
- The handler also lacks server-side state validation, so there's no defense in depth
Agent: logic
| const isOrganizer = booking.userId === user.id; | ||
|
|
||
| const isAttendee = !!booking.attendees.find((attendee) => attendee.email === user.email); | ||
|
|
There was a problem hiding this comment.
🔴 CRITICAL — Authorization Logic (confidence: 100%)
Authorization check uses AND (&&) instead of OR (||) for team admin/owner check. The condition isTeamAdmin(...) && isTeamOwner(...) requires the user to be BOTH admin AND owner simultaneously to pass the team-level check. This means team admins who are not owners (and vice versa, though owner implies admin in most systems) will be incorrectly denied. It should be ||.
Evidence:
- Line 50:
(await isTeamAdmin(user.id, booking.eventType?.teamId ?? 0)) && (await isTeamOwner(user.id, booking.eventType?.teamId ?? 0)) - The variable is named
isTeamAdminOrOwnersuggesting OR logic, but the code uses AND - Line 50:
booking.eventType?.teamId ?? 0 - For personal (non-team) bookings, teamId would be null, and
isTeamAdmin(userId, 0)behavior is unclear - The authorization check should short-circuit for non-team bookings instead of querying with teamId=0
- Line 51:
const isTeamAdminOrOwner = (await isTeamAdmin(user.id, booking.eventType?.teamId ?? 0)) && (await isTeamOwner(user.id, booking.eventType?.teamId ?? 0)); - The variable is named
isTeamAdminOrOwnersuggesting OR semantics, but the code uses AND (&&) - A team admin who is not an owner would be denied access despite the intent to allow either role
Agent: architecture
| export const ZAddGuestsInputSchema = z.object({ | ||
| bookingId: z.number(), | ||
| guests: z.array(z.string().email()), | ||
| }); |
There was a problem hiding this comment.
🟠 MAJOR — Input Validation (confidence: 100%)
The schema allows an empty guests array (z.array(z.string().email()) with no .min(1) constraint). While the handler filters duplicates and throws if uniqueGuests.length === 0, submitting an empty array would pass schema validation and then fail with a confusing 'emails_must_be_unique_valid' error instead of a clear validation error.
Evidence:
- Edge case from spec: 'Empty guest list submission — the schema and UI should prevent submitting with no emails'
- The schema has no
.min(1)or.nonempty()on the guests array - Line 5:
guests: z.array(z.string().email())has no minimum length constraint - The intent specification states: 'Empty guest list submission — the schema and UI should prevent submitting with no emails'
Agent: logic
| const [multiEmailValue, setMultiEmailValue] = useState<string[]>([""]); | ||
| const [isInvalidEmail, setIsInvalidEmail] = useState(false); | ||
|
|
||
| const addGuestsMutation = trpc.viewer.bookings.addGuests.useMutation({ |
There was a problem hiding this comment.
🟠 MAJOR — Email Validation (confidence: 100%)
The initial state of multiEmailValue is [""] (array with one empty string). The handleAdd function checks multiEmailValue.length === 0 but never checks if the array contains empty strings. When a user opens the dialog and immediately clicks 'Add' without entering anything, the empty string fails the zod email validation and shows the 'emails_must_be_unique_valid' error. However, submitting [""] will pass the length === 0 guard and hit the validation. While this technically works, it's a confusing UX where an untouched form shows a validation error.
Evidence:
- Line 34:
const [multiEmailValue, setMultiEmailValue] = useState(['']); - Line 52:
if (multiEmailValue.length === 0) { return; }- this won't catch[''] - The cancel button resets to
['']on line 88, showing this is intentional initial state
Agent: logic
| }); | ||
|
|
||
| await eventManager.updateCalendarAttendees(evt, booking); | ||
|
|
There was a problem hiding this comment.
🟡 MINOR — Error Handling (confidence: 86%)
Email sending errors are silently swallowed with only a console.log. While the intent spec says email failures should not corrupt booking state (which is satisfied), the error message logged is not informative - it doesn't include the actual error details.
Evidence:
- Line 165:
console.log("Error sending AddGuestsEmails");- theerrvariable is caught but not logged - This makes debugging email failures in production very difficult
Agent: logic
| }) | ||
| ); | ||
|
|
||
| await Promise.all(emailsToSend); |
There was a problem hiding this comment.
🟡 MINOR — Code organization (confidence: 72%)
In sendAddGuestsEmails function, the logic for sending AttendeeAddGuestsEmail vs AttendeeScheduledEmail is based on whether the attendee is in the newGuests array. However, newly added guests should always receive AttendeeScheduledEmail (initial invitation), not AttendeeAddGuestsEmail (notification of additional guests). The current logic is inverted.
Evidence:
- Lines 541-544 show: if newGuests.includes(attendee.email) → AttendeeScheduledEmail, else → AttendeeAddGuestsEmail
- Semantically, new guests added to a booking should receive the full scheduled email (like initial attendees), not a 'guests added' notification
- Existing attendees should receive AttendeeAddGuestsEmail to notify them of new guest additions
- The logic appears backwards: newGuests should trigger AttendeeScheduledEmail, not the inverse
Agent: style
| }, | ||
| 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.
🔵 INFO — Error messages (confidence: 81%)
Error message construction concatenates err.data?.code and err.message without null checks. If either is undefined, the message will be malformed (e.g., 'undefined: undefined').
Evidence:
- Line 43: const message =
${err.data?.code}: ${t(err.message)} - If err.data is undefined or err.message is undefined, template string produces unclear output
- Should validate both values exist before constructing message
Agent: style
| text: this.getTextBody("new_guests_added"), | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
🔵 INFO — Code organization (confidence: 76%)
New email template class duplicates node-mailer payload logic from parent AttendeeScheduledEmail. Only subtitle changes (new_guests_added vs emailed_you_and_any_other_attendees). Consider if this warrants a full class or if a simpler composition pattern would reduce duplication.
Evidence:
- Lines 5-16 mirror AttendeeScheduledEmail structure with identical icalEvent, to, from, replyTo setup
- Only change is line 12: title and subtitle use 'new_guests_added' instead of standard scheduled message
- Similar duplication in OrganizerAddGuestsEmail (organizer-add-guests-email.ts)
- React component versions (src/templates/) use composition effectively but legacy TS versions duplicate code
Agent: style
| ); | ||
| } | ||
|
|
||
| export default MultiEmail; |
There was a problem hiding this comment.
🔵 INFO — Naming conventions (confidence: 74%)
Component export uses 'default export' but is imported as named export in packages/ui/index.tsx (line 153). This inconsistency could cause confusion. Either use consistent named exports or document why default is used here.
Evidence:
- MultiEmail.tsx line 96: export default MultiEmail
- MultiEmailLazy.tsx line 4: exports as typeof ...default (correct for dynamic import)
- packages/ui/index.tsx line 153: export { default as MultiEmail }
- Pattern inconsistent with other form exports like PhoneInput (line 152)
Agent: style
| const isOrganizer = booking.userId === user.id; | ||
|
|
||
| const isAttendee = !!booking.attendees.find((attendee) => attendee.email === user.email); | ||
|
|
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — architecture agent (Small fix (10 lines, 1 file))
Authorization check uses AND (
&&) instead of OR (||) for team admin/owner check. The conditionisTeamAdmin(...) && isTeamOwner(...)requires the user to be BOTH admin AND owner simultaneously to pass the team-level check. This means team admins who are not owners (and vice versa, though owner implies admin in most systems) will be incorrectly denied. It should be||.
| // Short-circuit for personal (non-team) bookings to avoid querying with teamId=0, | |
| // whose behavior is undefined. For team bookings, allow either admin OR owner access. | |
| const teamId = booking.eventType?.teamId; | |
| const isTeamAdminOrOwner = teamId | |
| ? (await isTeamAdmin(user.id, teamId)) || | |
| (await isTeamOwner(user.id, teamId)) | |
| : false; |
🤖 Grapple PR auto-fix • critical • confidence: 100%
| export const ZAddGuestsInputSchema = z.object({ | ||
| bookingId: z.number(), | ||
| guests: z.array(z.string().email()), | ||
| }); |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — logic agent (Small fix (2 lines, 1 file))
The schema allows an empty guests array (
z.array(z.string().email())with no.min(1)constraint). While the handler filters duplicates and throws ifuniqueGuests.length === 0, submitting an empty array would pass schema validation and then fail with a confusing 'emails_must_be_unique_valid' error instead of a clear validation error.
| }); | |
| guests: z.array(z.string().email()).min(1), |
🤖 Grapple PR auto-fix • major • confidence: 100%
| const [multiEmailValue, setMultiEmailValue] = useState<string[]>([""]); | ||
| const [isInvalidEmail, setIsInvalidEmail] = useState(false); | ||
|
|
||
| const addGuestsMutation = trpc.viewer.bookings.addGuests.useMutation({ |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — logic agent (Small fix (9 lines, 1 file))
The initial state of
multiEmailValueis[""](array with one empty string). ThehandleAddfunction checksmultiEmailValue.length === 0but never checks if the array contains empty strings. When a user opens the dialog and immediately clicks 'Add' without entering anything, the empty string fails the zod email validation and shows the 'emails_must_be_unique_valid' error. However, submitting[""]will pass thelength === 0guard and hit the validation. While this technically works, it's a confusing UX where an untouched form shows a validation error.
| const addGuestsMutation = trpc.viewer.bookings.addGuests.useMutation({ | |
| // Filter out empty strings since initial/reset state is [''] (one empty string), | |
| // not [] — so a plain length check would not catch an untouched form. | |
| const filteredEmails = multiEmailValue.filter((email) => email.trim() !== ""); | |
| if (filteredEmails.length === 0) { | |
| const validationResult = ZAddGuestsInputSchema.safeParse(filteredEmails); | |
| addGuestsMutation.mutate({ bookingId, guests: filteredEmails }); |
🤖 Grapple PR auto-fix • major • confidence: 100%
| const isOrganizer = booking.userId === user.id; | ||
|
|
||
| const isAttendee = !!booking.attendees.find((attendee) => attendee.email === user.email); | ||
|
|
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — architecture agent (Small fix (10 lines, 1 file))
When
booking.eventType?.teamIdis undefined/null (personal event types without a team), the code passes0toisTeamAdminandisTeamOwner. These functions will query for team membership with teamId=0, which will always return false. While this doesn't cause a security issue (the check falls through to isOrganizer/isAttendee), it results in two unnecessary database queries for every non-team booking.
| // Only query team membership when the booking is actually associated with a team. | |
| // Passing teamId=0 to isTeamAdmin/isTeamOwner would always return false but still | |
| // hit the database unnecessarily for every personal event-type booking. | |
| const teamId = booking.eventType?.teamId; | |
| const isTeamAdminOrOwner = teamId | |
| ? (await isTeamAdmin(user.id, teamId)) && (await isTeamOwner(user.id, teamId)) | |
| : false; |
🤖 Grapple PR auto-fix • major • confidence: 100%
| ); | ||
|
|
||
| await Promise.all(emailsToSend); | ||
| }; |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — logic agent (Small fix (2 lines, 1 file))
The
sendAddGuestsEmailsfunction accepts anewGuestsparameter but the checknewGuests.includes(attendee.email)compares against the originalguestsinput from the caller, not the deduplicated list. In the handler (addGuests.handler.ts line 167),guests(the raw input) is passed, notuniqueGuests. This means if a guest email was already an attendee and was filtered out as a duplicate, they could still be matched in the email logic, though they'd be in the attendees list from the DB update so it would just send them an AttendeeScheduledEmail instead of AttendeeAddGuestsEmail.
| }; | |
| await sendAddGuestsEmails(evt, uniqueGuests); |
🤖 Grapple PR auto-fix • minor • confidence: 100%
| await Promise.all(emailsToSend); | ||
| }; | ||
| export const sendFeedbackEmail = async (feedback: Feedback) => { | ||
| await sendEmail(() => new FeedbackEmail(feedback)); |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — architecture agent (Small fix (2 lines, 1 file))
The
sendAddGuestsEmailsfunction acceptsnewGuests: string[]parameter but the handler ataddGuests.handler.ts:166passesguests(the original input) rather thanuniqueGuests(the deduplicated list). This means if a guest email was already an attendee and was filtered out byuniqueGuests, the email logic insendAddGuestsEmailswill still check against the original unfiltered list, potentially sending the wrong email type to existing attendees.
| await sendEmail(() => new FeedbackEmail(feedback)); | |
| await sendAddGuestsEmails(evt, uniqueGuests); |
🤖 Grapple PR auto-fix • minor • confidence: 100%
| } | ||
| }; | ||
|
|
||
| return ( |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — architecture agent (Small fix (7 lines, 1 file))
The
handleAddfunction allows submission whenmultiEmailValuecontains empty strings. The initial state is['']and the check on line 54 only guards against an empty array, not an array containing empty strings. The client-side Zod validation will catch this, but it would show a generic 'emails must be unique' error rather than a more helpful message.
| return ( | |
| const filteredEmails = multiEmailValue.filter((email) => email.trim() !== ""); | |
| if (filteredEmails.length === 0) { | |
| const validationResult = ZAddGuestsInputSchema.safeParse(filteredEmails); | |
| addGuestsMutation.mutate({ bookingId, guests: filteredEmails }); |
🤖 Grapple PR auto-fix • minor • confidence: 98%
| @@ -0,0 +1,96 @@ | |||
| import { useLocale } from "@calcom/lib/hooks/useLocale"; | |||
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — architecture agent (Small fix (5 lines, 1 file))
MultiEmail.tsx imports from
@calcom/ui(line 1:import { Button, EmailField, Icon, Tooltip } from '@calcom/ui') while itself being part of the@calcom/uipackage (exported viapackages/ui/index.tsx). This creates a self-referencing import within the same package, which can cause issues depending on bundler behavior and module resolution.
| import { useLocale } from "@calcom/lib/hooks/useLocale"; | |
| import Button from "../components/button/Button"; | |
| import EmailField from "../components/form/inputs/EmailField"; | |
| import Icon from "../components/icon/Icon"; | |
| import Tooltip from "../components/tooltip/Tooltip"; |
🤖 Grapple PR auto-fix • minor • confidence: 93%
| }); | ||
|
|
||
| await eventManager.updateCalendarAttendees(evt, booking); | ||
|
|
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — logic agent (Small fix (2 lines, 1 file))
Email sending errors are silently swallowed with only a console.log. While the intent spec says email failures should not corrupt booking state (which is satisfied), the error message logged is not informative - it doesn't include the actual error details.
| console.error("Error sending AddGuestsEmails", err); |
🤖 Grapple PR auto-fix • minor • confidence: 86%
| }) | ||
| ); | ||
|
|
||
| await Promise.all(emailsToSend); |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — style agent (Small fix (4 lines, 1 file))
In sendAddGuestsEmails function, the logic for sending AttendeeAddGuestsEmail vs AttendeeScheduledEmail is based on whether the attendee is in the newGuests array. However, newly added guests should always receive AttendeeScheduledEmail (initial invitation), not AttendeeAddGuestsEmail (notification of additional guests). The current logic is inverted.
| await Promise.all(emailsToSend); | |
| } else { | |
| return sendEmail(() => new AttendeeScheduledEmail(calendarEvent, attendee)); |
🤖 Grapple PR auto-fix • minor • confidence: 72%
| } | ||
|
|
||
| return { message: "Guests added" }; | ||
| }; |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (17 lines, 1 file) — review recommended)
The handler does not check the booking status before adding guests. According to the acceptance criteria, adding guests to a cancelled or past booking should be rejected. The handler allows adding guests to cancelled, rejected, or past bookings, which is a data integrity issue.
--- a/packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts
+++ b/packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts
@@ -8,6 +8,7 @@
import { getUsersCredentials } from "@calcom/lib/server/getUsersCredentials";
import { isTeamAdmin, isTeamOwner } from "@calcom/lib/server/queries/teams";
import { prisma } from "@calcom/prisma";
+import { BookingStatus } from "@calcom/prisma/enums";
import type { CalendarEvent } from "@calcom/types/Calendar";
import { TRPCError } from "@trpc/server";
@@ -52,6 +53,20 @@ export const addGuestsHandler = async ({ ctx, input }: AddGuestsOptions) => {
if (!isTeamAdminOrOwner && !isOrganizer && !isAttendee) {
throw new TRPCError({ code: "FORBIDDEN", message: "you_do_not_have_permission" });
}
+
+ // Guard: do not allow adding guests to cancelled or rejected bookings
+ if (booking.status === BookingStatus.CANCELLED || booking.status === BookingStatus.REJECTED) {
+ throw new TRPCError({
+ code: "BAD_REQUEST",
+ message: "cannot_add_guests_to_cancelled_or_rejected_booking",
+ });
+ }
+
+ // Guard: do not allow adding guests to past bookings
+ if (dayjs().isAfter(dayjs(booking.endTime))) {
+ throw new TRPCError({
+ code: "BAD_REQUEST",
+ message: "cannot_add_guests_to_past_booking",
+ });
+ }
const organizer = await prisma.user.findFirstOrThrow({
where: {🤖 Grapple PR auto-fix • critical • Review this diff before applying
| setIsOpenAddGuestsDialog(true); | ||
| }, | ||
| icon: "user-plus" as const, | ||
| }, |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (25 lines, 1 file) — review recommended)
The 'Add Guests' action is unconditionally added to the booking actions array for all bookings, regardless of booking status. This means the option appears for cancelled, rejected, and past bookings. The acceptance criteria explicitly states: 'BookingListItem only shows the Add Guests option for bookings in an appropriate state (e.g., upcoming, confirmed — not cancelled or past)'.
--- a/apps/web/components/booking/BookingListItem.tsx
+++ b/apps/web/components/booking/BookingListItem.tsx
@@ -190,14 +190,16 @@ function BookingListItem(booking: BookingItemProps) {
},
icon: "map-pin" as const,
},
- {
- id: "add_members",
- label: t("additional_guests"),
- onClick: () => {
- setIsOpenAddGuestsDialog(true);
- },
- icon: "user-plus" as const,
- },
];
+ if (
+ booking.status !== BookingStatus.CANCELLED &&
+ booking.status !== BookingStatus.REJECTED &&
+ booking.status !== BookingStatus.AWAITING_HOST &&
+ booking.status !== BookingStatus.PENDING &&
+ !isPast
+ ) {
+ editBookingActions.push({
+ id: "add_members",
+ label: t("additional_guests"),
+ onClick: () => {
+ setIsOpenAddGuestsDialog(true);
+ },
+ icon: "user-plus" as const,
+ });
+ }
+
if (booking.eventType.schedulingType === SchedulingType.ROUND_ROBIN) {🤖 Grapple PR auto-fix • critical • Review this diff before applying
|
|
||
| const isTeamAdminOrOwner = | ||
| (await isTeamAdmin(user.id, booking.eventType?.teamId ?? 0)) && | ||
| (await isTeamOwner(user.id, booking.eventType?.teamId ?? 0)); |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (19 lines, 1 file) — review recommended)
The handler does not check booking status before allowing guest additions. Guests can be added to cancelled, rejected, or past bookings. The handler should verify the booking is in an appropriate state (e.g., ACCEPTED, PENDING) and that the booking's endTime is in the future.
--- a/packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts
+++ b/packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts
@@ -1,6 +1,7 @@
import EventManager from "@calcom/core/EventManager";
import dayjs from "@calcom/dayjs";
import { sendAddGuestsEmails } from "@calcom/emails";
import { parseRecurringEvent } from "@calcom/lib";
import { getTranslation } from "@calcom/lib/server";
import { getUsersCredentials } from "@calcom/lib/server/getUsersCredentials";
import { isTeamAdmin, isTeamOwner } from "@calcom/lib/server/queries/teams";
import { prisma } from "@calcom/prisma";
+import { BookingStatus } from "@calcom/prisma/enums";
import type { CalendarEvent } from "@calcom/types/Calendar";
import { TRPCError } from "@trpc/server";
@@ -42,6 +43,20 @@ export const addGuestsHandler = async ({ ctx, input }: AddGuestsOptions) => {
if (!booking) throw new TRPCError({ code: "NOT_FOUND", message: "booking_not_found" });
+ // Guard: only allow guest additions on bookings that are in an actionable state.
+ // CANCELLED and REJECTED bookings are terminal — guests should never be added to them.
+ if (booking.status === BookingStatus.CANCELLED || booking.status === BookingStatus.REJECTED) {
+ throw new TRPCError({
+ code: "BAD_REQUEST",
+ message: "cannot_add_guests_to_cancelled_or_rejected_booking",
+ });
+ }
+
+ // Guard: prevent adding guests to bookings whose end time has already passed.
+ // dayjs is already imported and used elsewhere in this file for date formatting.
+ if (dayjs(booking.endTime).isBefore(dayjs())) {
+ throw new TRPCError({
+ code: "BAD_REQUEST",
+ message: "cannot_add_guests_to_past_booking",
+ });
+ }
+
const isTeamAdminOrOwner =
(await isTeamAdmin(user.id, booking.eventType?.teamId ?? 0)) &&
(await isTeamOwner(user.id, booking.eventType?.teamId ?? 0));🤖 Grapple PR auto-fix • major • Review this diff before applying
There was a problem hiding this comment.
🤖 Grapple PR Review
Summary
This PR adds guest management functionality to existing bookings, allowing users to add guests to already-confirmed bookings via a new dialog. It introduces a tRPC handler, email notifications for attendees and organizers, a MultiEmail UI component, and an AddGuestsDialog component.
Changes: 15 files (+555 / -0 lines)
- 🟢 9 files added:
apps/web/components/dialog/AddGuestsDialog.tsx,packages/emails/src/templates/AttendeeAddGuestsEmail.tsx,packages/emails/src/templates/OrganizerAddGuestsEmail.tsx,packages/emails/templates/attendee-add-guests-email.ts,packages/emails/templates/organizer-add-guests-email.ts+4 more - 🟡 6 files modified:
apps/web/components/booking/BookingListItem.tsx,apps/web/public/static/locales/en/common.json,packages/emails/email-manager.ts,packages/emails/src/templates/index.ts,packages/trpc/server/routers/viewer/bookings/_router.tsx+1 more
Resolves: #10 (Async import of the appStore packages)
CI: ⏳ 0 passed, 0 failed
Impact Diagram
graph TD
pr_0[/"🔀 PR Changes: 15 files"/]
f1["🟡 BookingListItem.tsx +15/-0"]
pr_0 --> f1
f2["🟢 AddGuestsDialog.tsx +107/-0"]
pr_0 --> f2
f3["🟡 common.json +6/-0"]
pr_0 --> f3
f4["🟡 email-manager.ts +28/-0"]
pr_0 --> f4
f5["🟢 AttendeeAddGuestsEmail.tsx +10/-0"]
pr_0 --> f5
f6["🟢 OrganizerAddGuestsEmail.tsx +11/-0"]
pr_0 --> f6
f7["🟡 index.ts +2/-0"]
pr_0 --> f7
f8["🟢 attendee-add-guests-email.ts +34/-0"]
pr_0 --> f8
f9["🟢 organizer-add-guests-email.ts +38/-0"]
pr_0 --> f9
f10["🟡 _router.tsx +19/-0"]
pr_0 --> f10
f11["🟢 addGuests.handler.ts +174/-0"]
pr_0 --> f11
f12["🟢 addGuests.schema.ts +8/-0"]
pr_0 --> f12
more_13["... +3 more files"]
pr_0 --> more_13
f5 -.- f6
f6 -.- f7
f8 -.- f9
f10 -.- f11
f11 -.- f12
style f1 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f2 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f3 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f4 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f5 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f6 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f7 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f8 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f9 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f10 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f11 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f12 fill:#7c3aed,color:#fff,stroke:#7c3aed
1 critical issue found — please address before merging.
| Severity | Count |
|---|---|
| 🔴 Critical | 1 |
| 🟠 Major | 14 |
| 🟡 Minor | 8 |
| 🔵 Info | 7 |
Review details
- Reviewed at: 2026-04-08T02:41:03.876Z
- Agents used: security, logic, style, architecture, performance, verification
- Context level: full
- Execution time: 193.8s
- Low-confidence findings suppressed: 1
- Blast radius: 0 (contained)
Powered by Grapple PR — AI that understands your code.
|
|
||
| const guestsFullDetails = uniqueGuests.map((guest) => { | ||
| return { | ||
| name: "", |
There was a problem hiding this comment.
🔴 CRITICAL — Duplicate Attendee Prevention (confidence: 100%)
The duplicate filtering compares guest emails against existing attendees, but uses strict equality (guest === attendee.email) without normalizing case. If an attendee exists as 'User@Example.com' and the guest is submitted as 'user@example.com', the filter will not detect the duplicate, leading to a duplicate attendee record in the database.
Evidence:
- Line 82:
!booking.attendees.some((attendee) => guest === attendee.email) - Email addresses are case-insensitive per RFC 5321, but string comparison is case-sensitive
Agent: logic
|
|
||
| const isAttendee = !!booking.attendees.find((attendee) => attendee.email === user.email); | ||
|
|
||
| if (!isTeamAdminOrOwner && !isOrganizer && !isAttendee) { |
There was a problem hiding this comment.
🟠 MAJOR — Authorization Logic (confidence: 100%)
Authorization logic uses && (AND) instead of || (OR) for team admin/owner check, meaning a user must be BOTH team admin AND team owner to pass this check. This effectively makes the team-level permission check nearly useless since team owners are typically also admins but the intent is clearly to allow either role.
Evidence:
- Line 52-53:
(await isTeamAdmin(user.id, booking.eventType?.teamId ?? 0)) && (await isTeamOwner(user.id, booking.eventType?.teamId ?? 0)) - The variable is named
isTeamAdminOrOwnersuggesting OR semantics, but the code uses AND - Similar patterns in the codebase (e.g., editLocation) typically check admin OR owner
Agent: architecture
| const isTeamAdminOrOwner = | ||
| (await isTeamAdmin(user.id, booking.eventType?.teamId ?? 0)) && | ||
| (await isTeamOwner(user.id, booking.eventType?.teamId ?? 0)); | ||
|
|
There was a problem hiding this comment.
🟠 MAJOR — Missing Booking State Validation (confidence: 100%)
The handler does not check the booking's status before allowing guest addition. Guests can be added to cancelled, rejected, or past bookings. The acceptance criteria explicitly require: 'The Add Guests action is only surfaced for bookings where it is appropriate (e.g., not cancelled, not past, and the user is the organizer or has permission)'.
Evidence:
- No check on
booking.status(e.g., BookingStatus.CANCELLED, BookingStatus.REJECTED) - No check on
booking.endTimebeing in the future - The UI in BookingListItem.tsx also does not filter — the 'add_members' action is added unconditionally to the actions array at line 193-200
Agent: architecture
| setIsOpenAddGuestsDialog(true); | ||
| }, | ||
| icon: "user-plus" as const, | ||
| }, |
There was a problem hiding this comment.
🟠 MAJOR — UI - Missing Conditional Rendering (confidence: 100%)
The 'Add Guests' action is unconditionally added to the actions array for all bookings. It should only appear for active/upcoming bookings where the user has permission, similar to how other actions are conditionally rendered. Cancelled, rejected, and past bookings should not show this option.
Evidence:
- The action object is pushed into the array without any conditional check on booking status
- Other actions in this component (like reassign at line 203+) are conditionally added based on scheduling type
- Intent spec acceptance criteria: 'The Add Guests action is only surfaced for bookings where it is appropriate (e.g., not cancelled, not past, and the user is the organizer or has permission)'
Agent: architecture
| bookingId: z.number(), | ||
| guests: z.array(z.string().email()), | ||
| }); | ||
|
|
There was a problem hiding this comment.
🟠 MAJOR — Schema Validation Gap (confidence: 100%)
The schema allows an empty guests array (z.array(z.string().email()) accepts []) and does not validate that bookingId is a positive integer. The acceptance criteria explicitly require: 'addGuests.schema.ts correctly validates bookingId (as a positive integer) and a non-empty array of valid email strings'.
Evidence:
- Line 5:
guests: z.array(z.string().email())— no.nonempty()or.min(1) - Line 4:
bookingId: z.number()— no.int().positive()constraint - The handler at line 87-88 handles empty uniqueGuests but the empty input case should be caught at schema level
- A bookingId of 0, -1, or 1.5 would pass schema validation
Agent: architecture
| const { t } = useLocale(); | ||
| const ZAddGuestsInputSchema = z.array(z.string().email()).refine((emails) => { | ||
| const uniqueEmails = new Set(emails); | ||
| return uniqueEmails.size === emails.length; |
There was a problem hiding this comment.
🔵 INFO — Code organization (confidence: 91%)
Validation schema is defined inside component render scope, causing it to be recreated on every render. Should be defined outside component.
Evidence:
- Lines 23-28: ZAddGuestsInputSchema is defined inside AddGuestsDialog component
- This causes object recreation and loses referential equality across renders
- Best practice: schemas should be defined at module level or memoized
- This pattern appears in addGuests.schema.ts as a module-level export, creating inconsistency
Agent: style
|
|
||
| export const ZAddGuestsInputSchema = z.object({ | ||
| bookingId: z.number(), | ||
| guests: z.array(z.string().email()), |
There was a problem hiding this comment.
🔵 INFO — Documentation (confidence: 88%)
Missing validation on bookingId to ensure it is a positive integer. Schema allows zero or negative values.
Evidence:
- Line 3-5: bookingId is defined as z.number() without positive() constraint
- Acceptance criteria states: 'The addGuests.schema.ts correctly validates bookingId (as a positive integer)'
- Zero or negative booking IDs would fail gracefully at database level but should be rejected at schema validation for clarity
Agent: style
| const [multiEmailValue, setMultiEmailValue] = useState<string[]>([""]); | ||
| const [isInvalidEmail, setIsInvalidEmail] = useState(false); | ||
|
|
||
| const addGuestsMutation = trpc.viewer.bookings.addGuests.useMutation({ |
There was a problem hiding this comment.
🔵 INFO — Documentation (confidence: 88%)
Missing error message i18n key 'unable_to_add_guests' fallback is problematic. The error message construction attempts to translate error.message but should validate it exists.
Evidence:
- Line 43:
const message = \${err.data?.code}: ${t(err.message)}`;` assumes err.message is a valid i18n key - If backend error message doesn't match an i18n key, translation falls back to the key name itself
- Better pattern: use error.message directly or ensure backend errors map to valid i18n keys
- This could surface untranslated error codes to users
Agent: style
| @@ -0,0 +1,174 @@ | |||
| import EventManager from "@calcom/core/EventManager"; | |||
There was a problem hiding this comment.
🔵 INFO — Linked Issue Mismatch (confidence: 76%)
The PR is linked to Issue #10 ('Async import of the appStore packages') but implements guest management functionality. There are no async import changes for appStore packages in this PR. The issue linkage appears to be erroneous.
Evidence:
- Issue Async import of the appStore packages #10 title: 'Async import of the appStore packages'
- No files under packages/app-store are modified
- No async import pattern changes are present in any changed file
- The PR implements an entirely different feature (add guests to bookings)
Agent: architecture
| label: string; | ||
| setValue: (value: string[]) => void; | ||
| placeholder?: string; | ||
| } |
There was a problem hiding this comment.
🔵 INFO — Code patterns (confidence: 71%)
MultiEmail component does not handle pasting comma-separated emails (edge case mentioned in risk areas). Component only supports manual input or adding individual fields.
Evidence:
- No onPaste or clipboard event handler in component
- Edge case risk: 'MultiEmail input with leading/trailing whitespace in email addresses — should be trimmed before validation'
- User experience gap: most email input components support pasting multiple comma/semicolon-separated emails
- Current implementation requires clicking 'Add Another' button for each email
Agent: style
|
|
||
| const guestsFullDetails = uniqueGuests.map((guest) => { | ||
| return { | ||
| name: "", |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — logic agent (Small fix (4 lines, 1 file))
The duplicate filtering compares guest emails against existing attendees, but uses strict equality (
guest === attendee.email) without normalizing case. If an attendee exists as 'User@Example.com' and the guest is submitted as 'user@example.com', the filter will not detect the duplicate, leading to a duplicate attendee record in the database.
| name: "", | |
| !booking.attendees.some((attendee) => guest.toLowerCase() === attendee.email.toLowerCase()) && | |
| !blacklistedGuestEmails.includes(guest.toLowerCase()) |
🤖 Grapple PR auto-fix • critical • confidence: 100%
|
|
||
| const isAttendee = !!booking.attendees.find((attendee) => attendee.email === user.email); | ||
|
|
||
| if (!isTeamAdminOrOwner && !isOrganizer && !isAttendee) { |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — architecture agent (Small fix (4 lines, 1 file))
Authorization logic uses
&&(AND) instead of||(OR) for team admin/owner check, meaning a user must be BOTH team admin AND team owner to pass this check. This effectively makes the team-level permission check nearly useless since team owners are typically also admins but the intent is clearly to allow either role.
| if (!isTeamAdminOrOwner && !isOrganizer && !isAttendee) { | |
| (await isTeamAdmin(user.id, booking.eventType?.teamId ?? 0)) || | |
| (await isTeamOwner(user.id, booking.eventType?.teamId ?? 0)); |
🤖 Grapple PR auto-fix • major • confidence: 100%
| bookingId: z.number(), | ||
| guests: z.array(z.string().email()), | ||
| }); | ||
|
|
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — architecture agent (Small fix (4 lines, 1 file))
The schema allows an empty guests array (
z.array(z.string().email())accepts[]) and does not validate that bookingId is a positive integer. The acceptance criteria explicitly require: 'addGuests.schema.ts correctly validates bookingId (as a positive integer) and a non-empty array of valid email strings'.
| bookingId: z.number().int().positive(), | |
| guests: z.array(z.string().email()).min(1), |
🤖 Grapple PR auto-fix • major • confidence: 100%
| const eventManager = new EventManager({ | ||
| ...user, | ||
| credentials: [...credentials], | ||
| }); |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — security agent (Small fix (2 lines, 1 file))
The handler passes the original user-supplied
guestsarray tosendAddGuestsEmailsinstead ofuniqueGuests(the deduplicated, validated list). In email-manager.ts, thenewGuests.includes(attendee.email)check uses this unfiltered list. Emails that were already attendees (filtered out of DB insert) will still matchnewGuests.includes()and incorrectly receive anAttendeeScheduledEmail('you have been booked') rather thanAttendeeAddGuestsEmail. This sends misleading email to existing attendees.
| }); | |
| await sendAddGuestsEmails(evt, uniqueGuests); |
🤖 Grapple PR auto-fix • major • confidence: 100%
| : []; | ||
|
|
||
| const uniqueGuests = guests.filter( | ||
| (guest) => |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — security agent (Small fix (10 lines, 1 file))
The BLACKLISTED_GUEST_EMAILS environment variable entries are normalized to lowercase, but the incoming
guestvalues are compared without lowercasing. Submitting 'BLOCKED@EXAMPLE.COM' bypasses a blacklist entry of 'blocked@example.com'. Since email addresses are case-insensitive by RFC 5321 (local part by convention, domain always), this allows trivial bypass of the entire guest email blacklist.
| (guest) => | |
| (guest) => { | |
| const normalizedGuest = guest.toLowerCase(); | |
| return ( | |
| !booking.attendees.some((attendee) => normalizedGuest === attendee.email.toLowerCase()) && | |
| !blacklistedGuestEmails.includes(normalizedGuest) | |
| ); | |
| } |
🤖 Grapple PR auto-fix • major • confidence: 100%
| import { z } from "zod"; | ||
|
|
||
| export const ZAddGuestsInputSchema = z.object({ | ||
| bookingId: z.number(), |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — security agent (Small fix (2 lines, 1 file))
The schema uses
z.number()forbookingIdwhich accepts 0, negative integers, and non-integer floats. A value of 0 causesprisma.user.findFirstOrThrow({ where: { id: 0 } })at line 63 to potentially match no user and throw an unhandled error with an internal stack trace. Should usez.number().int().positive().
| bookingId: z.number(), | |
| bookingId: z.number().int().positive(), |
🤖 Grapple PR auto-fix • minor • confidence: 100%
|
|
||
| export const ZAddGuestsInputSchema = z.object({ | ||
| bookingId: z.number(), | ||
| guests: z.array(z.string().email()), |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — logic agent (Small fix (2 lines, 1 file))
Email inputs are not trimmed before validation. A user could submit ' user@example.com ' which would pass zod's
.email()validation in some edge cases (zod's email validator may reject leading/trailing whitespace, but this should be explicitly handled). More importantly, the emails are stored as-is in the database without trimming.
| guests: z.array(z.string().email()), | |
| guests: z.array(z.string().trim().email()), |
🤖 Grapple PR auto-fix • minor • confidence: 100%
| @@ -520,6 +522,32 @@ export const sendLocationChangeEmails = async ( | |||
|
|
|||
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — architecture agent (Small fix (2 lines, 1 file))
The
sendAddGuestsEmailsfunction acceptsnewGuests: string[]as its second parameter, but it usesnewGuestsonly viacalendarEvent.attendeesfiltering withnewGuests.includes(attendee.email). However, the caller in addGuests.handler.ts passesguests(the raw input) instead ofuniqueGuests(the filtered list), which means if a duplicate guest email was in the original input, the email logic could incorrectly identify an existing attendee as a new guest.
| // uniqueGuests already passed above — removed duplicate call with raw guests |
🤖 Grapple PR auto-fix • minor • confidence: 98%
|
|
||
| await eventManager.updateCalendarAttendees(evt, booking); | ||
|
|
||
| try { |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — architecture agent (Small fix (2 lines, 1 file))
Email sending failures are silently swallowed with only a console.log. The database has already been updated with the new guests at this point, so the mutation appears successful to the user, but no emails are sent. This could lead to guests being added without any notification. At minimum, the error should be logged with the actual error details.
| try { | |
| console.error("Error sending AddGuestsEmails", err); |
🤖 Grapple PR auto-fix • minor • confidence: 96%
| to: toAddresses.join(","), | ||
| replyTo: [this.calEvent.organizer.email, ...this.calEvent.attendees.map(({ email }) => email)], | ||
| subject: `${this.t("guests_added_event_type_subject", { | ||
| eventType: this.calEvent.type, |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — security agent (Small fix (2 lines, 1 file))
The organizer notification email sets
replyToto include ALL attendee email addresses. Email clients (Gmail, Outlook) expose these addresses in the UI when the recipient clicks Reply or Reply All. For bookings with many attendees/guests, this leaks every attendee's email address to the organizer and potentially to other recipients.
| eventType: this.calEvent.type, | |
| replyTo: this.calEvent.organizer.email, |
🤖 Grapple PR auto-fix • minor • confidence: 93%
| const isTeamAdminOrOwner = | ||
| (await isTeamAdmin(user.id, booking.eventType?.teamId ?? 0)) && | ||
| (await isTeamOwner(user.id, booking.eventType?.teamId ?? 0)); | ||
|
|
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (16 lines, 1 file) — review recommended)
The handler does not check the booking's status before allowing guest addition. Guests can be added to cancelled, rejected, or past bookings. The acceptance criteria explicitly require: 'The Add Guests action is only surfaced for bookings where it is appropriate (e.g., not cancelled, not past, and the user is the organizer or has permission)'.
--- a/packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts
+++ b/packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts
@@ -1,6 +1,7 @@
import EventManager from "@calcom/core/EventManager";
import dayjs from "@calcom/dayjs";
import { sendAddGuestsEmails } from "@calcom/emails";
import { parseRecurringEvent } from "@calcom/lib";
import { getTranslation } from "@calcom/lib/server";
import { getUsersCredentials } from "@calcom/lib/server/getUsersCredentials";
import { isTeamAdmin, isTeamOwner } from "@calcom/lib/server/queries/teams";
import { prisma } from "@calcom/prisma";
+import { BookingStatus } from "@calcom/prisma/enums";
import type { CalendarEvent } from "@calcom/types/Calendar";
import { TRPCError } from "@trpc/server";
@@ -44,6 +45,18 @@ export const addGuestsHandler = async ({ ctx, input }: AddGuestsOptions) => {
if (!booking) throw new TRPCError({ code: "NOT_FOUND", message: "booking_not_found" });
+ // Guests may only be added to active, future bookings.
+ if (booking.status === BookingStatus.CANCELLED || booking.status === BookingStatus.REJECTED) {
+ throw new TRPCError({
+ code: "BAD_REQUEST",
+ message: "cannot_add_guests_to_cancelled_or_rejected_booking",
+ });
+ }
+
+ if (dayjs(booking.endTime).isBefore(dayjs())) {
+ throw new TRPCError({
+ code: "BAD_REQUEST",
+ message: "cannot_add_guests_to_past_booking",
+ });
+ }
+
const isTeamAdminOrOwner =
(await isTeamAdmin(user.id, booking.eventType?.teamId ?? 0)) &&
(await isTeamOwner(user.id, booking.eventType?.teamId ?? 0));🤖 Grapple PR auto-fix • major • Review this diff before applying
| setIsOpenAddGuestsDialog(true); | ||
| }, | ||
| icon: "user-plus" as const, | ||
| }, |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (25 lines, 1 file) — review recommended)
The 'Add Guests' action is unconditionally added to the actions array for all bookings. It should only appear for active/upcoming bookings where the user has permission, similar to how other actions are conditionally rendered. Cancelled, rejected, and past bookings should not show this option.
--- a/apps/web/components/booking/BookingListItem.tsx
+++ b/apps/web/components/booking/BookingListItem.tsx
@@ -190,13 +190,17 @@ function BookingListItem(booking: BookingItemProps) {
},
icon: "map-pin" as const,
},
- {
- id: "add_members",
- label: t("additional_guests"),
- onClick: () => {
- setIsOpenAddGuestsDialog(true);
- },
- icon: "user-plus" as const,
- },
];
+ const isPast = booking.startTime < new Date();
+ const isActiveBooking =
+ booking.status !== BookingStatus.CANCELLED &&
+ booking.status !== BookingStatus.REJECTED &&
+ !isPast;
+
+ if (isActiveBooking) {
+ editBookingActions.push({
+ id: "add_members",
+ label: t("additional_guests"),
+ onClick: () => {
+ setIsOpenAddGuestsDialog(true);
+ },
+ icon: "user-plus" as const,
+ });
+ }
+
if (booking.eventType.schedulingType === SchedulingType.ROUND_ROBIN) {🤖 Grapple PR auto-fix • major • Review this diff before applying
| }, | ||
| }); | ||
|
|
||
| if (!booking) throw new TRPCError({ code: "NOT_FOUND", message: "booking_not_found" }); |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — security agent (Larger fix (15 lines, 1 file) — review recommended)
The handler never checks
booking.statusbefore adding guests and sending emails. A user can add guests to cancelled, rejected, or past bookings, which will: (1) create attendee records on dead bookings, (2) trigger ICS calendar invite emails to unsuspecting recipients, (3) update calendar entries for cancelled events. This is a potential email abuse vector — an authenticated user can send unsolicited meeting invitations by referencing old cancelled booking IDs.
--- a/packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts
+++ b/packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts
@@ -1,6 +1,7 @@
import EventManager from "@calcom/core/EventManager";
import dayjs from "@calcom/dayjs";
import { sendAddGuestsEmails } from "@calcom/emails";
import { parseRecurringEvent } from "@calcom/lib";
import { getTranslation } from "@calcom/lib/server";
import { getUsersCredentials } from "@calcom/lib/server/getUsersCredentials";
import { isTeamAdmin, isTeamOwner } from "@calcom/lib/server/queries/teams";
import { prisma } from "@calcom/prisma";
+import { BookingStatus } from "@calcom/prisma/enums";
import type { CalendarEvent } from "@calcom/types/Calendar";
@@ -42,6 +43,17 @@ export const addGuestsHandler = async ({ ctx, input }: AddGuestsOptions) => {
if (!booking) throw new TRPCError({ code: "NOT_FOUND", message: "booking_not_found" });
+ // Prevent adding guests to bookings that are no longer active.
+ // This guards against email abuse: an authenticated user could otherwise
+ // reference a cancelled/rejected booking to send unsolicited ICS invites.
+ if (
+ booking.status === BookingStatus.CANCELLED ||
+ booking.status === BookingStatus.REJECTED ||
+ booking.status === BookingStatus.PENDING
+ ) {
+ throw new TRPCError({
+ code: "BAD_REQUEST",
+ message: "cannot_add_guests_to_inactive_booking",
+ });
+ }
+
const isTeamAdminOrOwner =
(await isTeamAdmin(user.id, booking.eventType?.teamId ?? 0)) &&
(await isTeamOwner(user.id, booking.eventType?.teamId ?? 0));🤖 Grapple PR auto-fix • major • Review this diff before applying
| const { isOpenDialog, setIsOpenDialog, bookingId } = props; | ||
| const utils = trpc.useUtils(); | ||
| const [multiEmailValue, setMultiEmailValue] = useState<string[]>([""]); | ||
| const [isInvalidEmail, setIsInvalidEmail] = useState(false); |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (13 lines, 1 file) — review recommended)
The multiEmailValue state is initialized with
[""](an array containing one empty string). ThehandleAddfunction checksmultiEmailValue.length === 0but this will never be true since the initial state is[""]. If the user clicks 'Add' without entering any email, the empty string will be sent to zod validation which will reject it, but the real issue is that empty string entries can be submitted. Additionally, on cancel/success, the state is reset to[""]not[], so the length check on line 50 will never trigger.
--- a/apps/web/components/dialog/AddGuestsDialog.tsx
+++ b/apps/web/components/dialog/AddGuestsDialog.tsx
@@ -35,7 +35,7 @@ export const AddGuestsDialog = (props: IAddGuestsDialog) => {
const addGuestsMutation = trpc.viewer.bookings.addGuests.useMutation({
onSuccess: async () => {
showToast(t("guests_added"), "success");
setIsOpenDialog(false);
- setMultiEmailValue([""]);
+ setMultiEmailValue([]);
utils.viewer.bookings.invalidate();
},
onError: (err) => {
@@ -46,8 +46,10 @@ export const AddGuestsDialog = (props: IAddGuestsDialog) => {
const handleAdd = () => {
- if (multiEmailValue.length === 0) {
+ // Filter out empty string entries before validation — the MultiEmail component
+ // initializes with [""] so a plain length check will never be falsy on first render.
+ const filteredEmails = multiEmailValue.filter((email) => email.trim() !== "");
+ if (filteredEmails.length === 0) {
return;
}
- const validationResult = ZAddGuestsInputSchema.safeParse(multiEmailValue);
+ const validationResult = ZAddGuestsInputSchema.safeParse(filteredEmails);
if (validationResult.success) {
- addGuestsMutation.mutate({ bookingId, guests: multiEmailValue });
+ addGuestsMutation.mutate({ bookingId, guests: filteredEmails });
} else {
setIsInvalidEmail(true);
}
@@ -78,7 +80,7 @@ export const AddGuestsDialog = (props: IAddGuestsDialog) => {
<Button
onClick={() => {
- setMultiEmailValue([""]);
+ setMultiEmailValue([]);
setIsInvalidEmail(false);
setIsOpenDialog(false);
}}🤖 Grapple PR auto-fix • major • Review this diff before applying
|
|
||
| const isTeamAdminOrOwner = | ||
| (await isTeamAdmin(user.id, booking.eventType?.teamId ?? 0)) && | ||
| (await isTeamOwner(user.id, booking.eventType?.teamId ?? 0)); |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (17 lines, 1 file) — review recommended)
The handler does not check the booking's status before allowing guests to be added. Guests can be added to cancelled, rejected, or past bookings. The acceptance criteria explicitly states: 'The Add Guests action is only surfaced for bookings where it is appropriate (e.g., not cancelled, not past, and the user is the organizer or has permission)'. While the UI may not show the button, the API endpoint itself is unprotected against this.
--- a/packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts
+++ b/packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts
@@ -1,6 +1,7 @@
import EventManager from "@calcom/core/EventManager";
import dayjs from "@calcom/dayjs";
import { sendAddGuestsEmails } from "@calcom/emails";
import { parseRecurringEvent } from "@calcom/lib";
import { getTranslation } from "@calcom/lib/server";
import { getUsersCredentials } from "@calcom/lib/server/getUsersCredentials";
import { isTeamAdmin, isTeamOwner } from "@calcom/lib/server/queries/teams";
import { prisma } from "@calcom/prisma";
+import { BookingStatus } from "@calcom/prisma/enums";
import type { CalendarEvent } from "@calcom/types/Calendar";
@@ -44,6 +45,20 @@ export const addGuestsHandler = async ({ ctx, input }: AddGuestsOptions) => {
if (!booking) throw new TRPCError({ code: "NOT_FOUND", message: "booking_not_found" });
+ // Prevent adding guests to bookings that are no longer active.
+ // The UI hides the action for these cases, but the API must also enforce
+ // this to prevent direct API abuse (acceptance criteria: handler must reject
+ // cancelled, rejected, or past bookings independently of the UI).
+ if (
+ booking.status === BookingStatus.CANCELLED ||
+ booking.status === BookingStatus.REJECTED
+ ) {
+ throw new TRPCError({ code: "BAD_REQUEST", message: "booking_not_active" });
+ }
+
+ // dayjs is already imported in this file; reuse it for the past-booking check.
+ if (dayjs(booking.endTime).isBefore(dayjs())) {
+ throw new TRPCError({ code: "BAD_REQUEST", message: "booking_already_ended" });
+ }
+
const isTeamAdminOrOwner =
(await isTeamAdmin(user.id, booking.eventType?.teamId ?? 0)) &&
(await isTeamOwner(user.id, booking.eventType?.teamId ?? 0));🤖 Grapple PR auto-fix • major • Review this diff before applying
| setIsOpenAddGuestsDialog(true); | ||
| }, | ||
| icon: "user-plus" as const, | ||
| }, |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (19 lines, 1 file) — review recommended)
The 'Add Guests' action is unconditionally added to the actions array for all bookings, regardless of booking status (cancelled, rejected, past). The acceptance criteria states the action should 'only be surfaced for bookings where it is appropriate'. Other actions in this file are conditionally shown based on booking state, but this one is always present.
--- a/apps/web/components/booking/BookingListItem.tsx
+++ b/apps/web/components/booking/BookingListItem.tsx
@@ -189,13 +189,15 @@ function BookingListItem(booking: BookingItemProps) {
},
icon: "map-pin" as const,
},
- {
- id: "add_members",
- label: t("additional_guests"),
- onClick: () => {
- setIsOpenAddGuestsDialog(true);
- },
- icon: "user-plus" as const,
- },
];
+ if (booking.listingStatus === "upcoming") {
+ editBookingActions.push({
+ id: "add_members",
+ label: t("additional_guests"),
+ onClick: () => {
+ setIsOpenAddGuestsDialog(true);
+ },
+ icon: "user-plus" as const,
+ });
+ }
+
if (booking.eventType.schedulingType === SchedulingType.ROUND_ROBIN) {🤖 Grapple PR auto-fix • major • Review this diff before applying
| }); | ||
| const { isOpenDialog, setIsOpenDialog, bookingId } = props; | ||
| const utils = trpc.useUtils(); | ||
| const [multiEmailValue, setMultiEmailValue] = useState<string[]>([""]); |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (27 lines, 1 file) — review recommended)
The client-side Zod schema (
ZAddGuestsInputSchema) is redefined inside the component on every render, and its validation logic differs from the server schema. The client schema adds uniqueness validation via.refine()but the server schema does not enforce uniqueness. Additionally, the client schema allows empty arrays while validating uniqueness, creating an inconsistent validation story.
--- a/apps/web/components/dialog/AddGuestsDialog.tsx
+++ b/apps/web/components/dialog/AddGuestsDialog.tsx
@@ -1,6 +1,5 @@
import type { Dispatch, SetStateAction } from "react";
import { useState } from "react";
-import { z } from "zod";
import { useLocale } from "@calcom/lib/hooks/useLocale";
import { trpc } from "@calcom/trpc/react";
@@ -14,6 +13,13 @@ import {
showToast,
} from "@calcom/ui";
+import { z } from "zod";
+
+// Defined at module scope so it is not recreated on every render.
+// Adds client-side uniqueness validation for better UX before the network call.
+const ZAddGuestsClientSchema = z
+ .array(z.string().email())
+ .min(1)
+ .refine((emails) => new Set(emails).size === emails.length, {
+ message: "emails_must_be_unique_valid",
+ });
+
interface IAddGuestsDialog {
isOpenDialog: boolean;
setIsOpenDialog: Dispatch<SetStateAction<boolean>>;
@@ -23,11 +29,6 @@ interface IAddGuestsDialog {
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;
- });
- const { isOpenDialog, setIsOpenDialog, bookingId } = props;
+ const { isOpenDialog, setIsOpenDialog, bookingId } = props;
const utils = trpc.useUtils();
const [multiEmailValue, setMultiEmailValue] = useState<string[]>([""]);
const [isInvalidEmail, setIsInvalidEmail] = useState(false);
@@ -48,10 +49,10 @@ export const AddGuestsDialog = (props: IAddGuestsDialog) => {
const handleAdd = () => {
- if (multiEmailValue.length === 0) {
- return;
- }
- const validationResult = ZAddGuestsInputSchema.safeParse(multiEmailValue);
+ // ZAddGuestsClientSchema already enforces min(1), so the explicit
+ // length-0 guard is covered by safeParse below.
+ const validationResult = ZAddGuestsClientSchema.safeParse(
+ multiEmailValue.filter((email) => email.trim() !== "")
+ );
if (validationResult.success) {
addGuestsMutation.mutate({ bookingId, guests: multiEmailValue });
} else {🤖 Grapple PR auto-fix • minor • Review this diff before applying
| console.log("Error sending AddGuestsEmails"); | ||
| } | ||
|
|
||
| return { message: "Guests added" }; |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — style agent (Larger fix (12 lines, 1 file) — review recommended)
Silent error handling when sending emails—errors are caught but only logged to console without user feedback or proper error propagation.
--- a/packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts
+++ b/packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts
@@ -165,9 +165,16 @@
await eventManager.updateCalendarAttendees(evt, booking);
+ let emailsSent = true;
try {
await sendAddGuestsEmails(evt, guests);
} catch (err) {
- console.log("Error sending AddGuestsEmails");
+ // Email failure is non-fatal: guests have already been added to the booking
+ // and calendar. We surface the failure in the response so the caller can
+ // display a warning toast instead of silently dropping the error.
+ console.error("Error sending AddGuestsEmails", err);
+ emailsSent = false;
}
- return { message: "Guests added" };
+ return {
+ message: "Guests added",
+ emailsSent,
+ };
};🤖 Grapple PR auto-fix • minor • Review this diff before applying
|
@grapple-pr review |
There was a problem hiding this comment.
🤖 Grapple PR Review
Summary
This PR adds guest management functionality allowing users to add guests to existing bookings, including a new dialog UI, tRPC handler, email notifications for attendees and organizers, and a reusable MultiEmail input component. The linked ticket references an async import issue which appears unrelated to the actual changes.
Changes: 15 files (+555 / -0 lines)
- 🟢 9 files added:
apps/web/components/dialog/AddGuestsDialog.tsx,packages/emails/src/templates/AttendeeAddGuestsEmail.tsx,packages/emails/src/templates/OrganizerAddGuestsEmail.tsx,packages/emails/templates/attendee-add-guests-email.ts,packages/emails/templates/organizer-add-guests-email.ts+4 more - 🟡 6 files modified:
apps/web/components/booking/BookingListItem.tsx,apps/web/public/static/locales/en/common.json,packages/emails/email-manager.ts,packages/emails/src/templates/index.ts,packages/trpc/server/routers/viewer/bookings/_router.tsx+1 more
Resolves: #10 (Async import of the appStore packages)
CI: ⏳ 0 passed, 0 failed
Impact Diagram
graph TD
pr_0[/"🔀 PR Changes: 15 files"/]
f1["🟡 BookingListItem.tsx +15/-0"]
pr_0 --> f1
f2["🟢 AddGuestsDialog.tsx +107/-0"]
pr_0 --> f2
f3["🟡 common.json +6/-0"]
pr_0 --> f3
f4["🟡 email-manager.ts +28/-0"]
pr_0 --> f4
f5["🟢 AttendeeAddGuestsEmail.tsx +10/-0"]
pr_0 --> f5
f6["🟢 OrganizerAddGuestsEmail.tsx +11/-0"]
pr_0 --> f6
f7["🟡 index.ts +2/-0"]
pr_0 --> f7
f8["🟢 attendee-add-guests-email.ts +34/-0"]
pr_0 --> f8
f9["🟢 organizer-add-guests-email.ts +38/-0"]
pr_0 --> f9
f10["🟡 _router.tsx +19/-0"]
pr_0 --> f10
f11["🟢 addGuests.handler.ts +174/-0"]
pr_0 --> f11
f12["🟢 addGuests.schema.ts +8/-0"]
pr_0 --> f12
more_13["... +3 more files"]
pr_0 --> more_13
f5 -.- f6
f6 -.- f7
f8 -.- f9
f10 -.- f11
f11 -.- f12
style f1 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f2 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f3 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f4 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f5 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f6 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f7 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f8 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f9 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f10 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f11 fill:#7c3aed,color:#fff,stroke:#7c3aed
style f12 fill:#7c3aed,color:#fff,stroke:#7c3aed
2 critical issues found — please address before merging.
| Severity | Count |
|---|---|
| 🔴 Critical | 2 |
| 🟠 Major | 5 |
| 🟡 Minor | 5 |
| 🔵 Info | 2 |
Review details
- Reviewed at: 2026-04-12T02:03:25.890Z
- Agents used: security, logic, style, architecture, performance, verification
- Context level: full
- Execution time: 159.0s
- Low-confidence findings suppressed: 5
- Blast radius: 0 (contained)
Powered by Grapple PR — AI that understands your code.
| } | ||
|
|
||
| return { message: "Guests added" }; | ||
| }; |
There was a problem hiding this comment.
🔴 CRITICAL — Missing Booking Status Check (confidence: 100%)
The handler does not check the booking status before adding guests. Per the acceptance criteria, adding guests to a cancelled or past booking should be rejected. A user could add guests to a CANCELLED, REJECTED, or already-ended booking, which would result in nonsensical email notifications and database mutations.
Evidence:
- The acceptance criteria explicitly states: 'Adding guests to a cancelled or past booking — handler should check booking status and reject appropriately'
- No check for
booking.status(e.g., BookingStatus.CANCELLED, REJECTED) or whetherbooking.endTimeis in the past
Agent: logic
| (await isTeamOwner(user.id, booking.eventType?.teamId ?? 0)); | ||
|
|
||
| const isOrganizer = booking.userId === user.id; | ||
|
|
There was a problem hiding this comment.
🔴 CRITICAL — Authorization Logic (confidence: 100%)
The team admin/owner authorization check uses && (AND) instead of || (OR), meaning a user must be BOTH a team admin AND a team owner to pass this check. This effectively makes the team admin/owner path unreachable for anyone who is only an admin but not an owner. The intent was likely to allow either team admins or team owners.
Evidence:
- Line 52-53:
(await isTeamAdmin(user.id, booking.eventType?.teamId ?? 0)) && (await isTeamOwner(user.id, booking.eventType?.teamId ?? 0)) - The intent says 'must verify the caller owns or is the organizer of the booking' — team admins should also be able to add guests, and the AND condition is overly restrictive
- Line 49-50:
(await isTeamAdmin(user.id, booking.eventType?.teamId ?? 0)) && (await isTeamOwner(user.id, booking.eventType?.teamId ?? 0)) - The variable is named
isTeamAdminOrOwnersuggesting OR semantics, but the code uses AND - Compare with other authorization patterns in the codebase that typically check admin OR owner
- Line 49-50:
booking.eventType?.teamId ?? 0 - For personal (non-team) bookings, teamId will be null, and querying team admin/owner with teamId=0 is semantically incorrect
Agent: architecture
|
|
||
| if (!booking) throw new TRPCError({ code: "NOT_FOUND", message: "booking_not_found" }); | ||
|
|
||
| const isTeamAdminOrOwner = |
There was a problem hiding this comment.
🟠 MAJOR — Missing Booking Status Validation (confidence: 100%)
The handler does not check the booking status before allowing guests to be added. Guests can be added to cancelled, rejected, or past bookings. The handler should verify the booking is in an active/confirmed state and that the event hasn't already ended.
Evidence:
- No check for
booking.status(e.g., CANCELLED, REJECTED, PENDING) - No check comparing
booking.endTimeagainst the current time - The intent specification explicitly lists 'Adding guests to a cancelled or past booking' as an edge case that should be rejected
Agent: architecture
| export const ZAddGuestsInputSchema = z.object({ | ||
| bookingId: z.number(), | ||
| guests: z.array(z.string().email()), | ||
| }); |
There was a problem hiding this comment.
🟠 MAJOR — Input Validation (confidence: 100%)
The schema allows an empty guests array (z.array(z.string().email()) with no .min(1)) and has no upper bound on the number of guests. An empty array would pass schema validation and then hit the uniqueGuests.length === 0 check only after database lookups. A very large array could cause performance issues in the handler's email dispatch loop and database operations.
Evidence:
- Acceptance criteria: 'Submitting with an empty guest list — should be prevented at the schema or UI validation level'
- z.array(z.string().email()) with no
.min(1)or.nonempty()constraint - Acceptance criteria: 'Very large number of email addresses submitted at once — potential performance issue in handler or email dispatch'
- No
.max()constraint on the guests array - No
.min(1)constraint on the guests array - No
.max()constraint to prevent abuse - Intent specification lists 'Submitting with an empty guest list' and 'Very large number of email addresses' as edge cases
Agent: architecture
|
|
||
| await Promise.all(emailsToSend); | ||
| }; | ||
| export const sendFeedbackEmail = async (feedback: Feedback) => { |
There was a problem hiding this comment.
🟠 MAJOR — Email Dispatch Logic (confidence: 100%)
The sendAddGuestsEmails function accepts a newGuests parameter but then checks guests (from the outer scope of the caller) against calendarEvent.attendees. The logic sends AttendeeScheduledEmail to new guests and AttendeeAddGuestsEmail to existing attendees, but the check newGuests.includes(attendee.email) compares against the original input emails. Since the handler passes the full calendarEvent with all attendees (including pre-existing ones), every attendee gets an email — but new guests get a booking confirmation while existing attendees get a 'guests added' notification. This works but the parameter naming (newGuests vs the raw guests input which may include duplicates that were filtered) could cause incorrect email routing if the caller passes unfiltered input.
Evidence:
- In addGuests.handler.ts line 163:
await sendAddGuestsEmails(evt, guests)— passes rawguestsinput, notuniqueGuests - In email-manager.ts line 537:
if (newGuests.includes(attendee.email))— compares against raw input - Line 522: function signature accepts
newGuests: string[] - Line 536-540:
if (newGuests.includes(attendee.email))determines email type - In addGuests.handler.ts line 166:
await sendAddGuestsEmails(evt, guests)passes the originalguestsinput, notuniqueGuests - If a guest email was already an attendee (and thus filtered from DB insert), they would still match
newGuests.includes()and receive an AttendeeScheduledEmail instead of AttendeeAddGuestsEmail
Agent: architecture
| setIsOpenAddGuestsDialog(true); | ||
| }, | ||
| icon: "user-plus" as const, | ||
| }, |
There was a problem hiding this comment.
🟡 MINOR — Missing Action Visibility Control (confidence: 93%)
The 'Add Guests' action is added to the bookingActionItems array unconditionally, meaning it will appear for all booking states including cancelled, rejected, and past bookings. Other actions in this component likely have conditional visibility based on booking status. Without server-side status validation (also flagged), this creates a confusing UX where users can attempt to add guests to non-modifiable bookings.
Evidence:
- The action is added at line 193-199 without any conditional check on booking status
- The editLocation action (line 186-192) similarly appears unconditional, but other actions elsewhere in the component may have status guards
- No status check exists in the handler either, compounding the issue
Agent: architecture
| const { isOpenDialog, setIsOpenDialog, bookingId } = props; | ||
| const utils = trpc.useUtils(); | ||
| const [multiEmailValue, setMultiEmailValue] = useState<string[]>([""]); | ||
| const [isInvalidEmail, setIsInvalidEmail] = useState(false); |
There was a problem hiding this comment.
🟡 MINOR — Client-Side Validation (confidence: 89%)
The multiEmailValue is initialized with [''] (an array containing one empty string). When the user clicks 'Add' without entering anything, multiEmailValue.length is 1 (not 0), so the early return on line 51 is bypassed. The empty string then fails the .email() zod validation and shows the invalid email error. While this works functionally, it's a confusing UX — the check on line 51 is dead code given the initial state.
Evidence:
- Line 33:
useState(['']) - Line 50-52:
if (multiEmailValue.length === 0) { return; }— this never triggers because the array always has at least one element
Agent: logic
| return uniqueEmails.size === emails.length; | ||
| }); | ||
| const { isOpenDialog, setIsOpenDialog, bookingId } = props; | ||
| const utils = trpc.useUtils(); |
There was a problem hiding this comment.
🟡 MINOR — Pattern Violation (confidence: 83%)
The Zod schema ZAddGuestsInputSchema is instantiated inside the component render function, meaning it gets recreated on every render. This is a minor performance issue and a pattern violation — validation schemas should be defined outside the component or memoized, consistent with how schemas are defined in dedicated .schema.ts files elsewhere in the codebase.
Evidence:
- Line 28-31: Schema defined inside component body
- The server already has
addGuests.schema.tswith the same validation — this duplicates it client-side - Other dialogs in the codebase (e.g., EditLocationDialog) don't define Zod schemas inline
Agent: architecture
| subject="guests_added_event_type_subject" | ||
| {...props} | ||
| /> | ||
| ); |
There was a problem hiding this comment.
🔵 INFO — Unused Import/Parameter (confidence: 84%)
The React component AttendeeAddGuestsEmail in the src/templates/ directory is correctly exported from index.ts, but the actual email sending in email-manager.ts uses AttendeeScheduledEmail directly for new guests (line 537) rather than AttendeeAddGuestsEmail. The AttendeeAddGuestsEmail class from templates/attendee-add-guests-email.ts (without src/) is what's used for existing attendees. This means the React template AttendeeAddGuestsEmail is used via renderEmail('AttendeeAddGuestsEmail', ...) in the class-based template, which is the correct pattern. No issue, just noting the dual template system (class-based + React) is correctly wired.
Evidence:
- email-manager.ts imports from
./templates/attendee-add-guests-email(class) - The class in
templates/attendee-add-guests-email.tscallsrenderEmail('AttendeeAddGuestsEmail', ...)which uses the React component - Both layers are correctly connected
Agent: architecture
| to: toAddresses.join(","), | ||
| replyTo: [this.calEvent.organizer.email, ...this.calEvent.attendees.map(({ email }) => email)], | ||
| subject: `${this.t("guests_added_event_type_subject", { | ||
| eventType: this.calEvent.type, |
There was a problem hiding this comment.
🔵 INFO — Defensive Coding (confidence: 81%)
The subject line references this.calEvent.attendees[0].name without checking that the attendees array is non-empty. While in practice there should always be at least one attendee, an edge case with an empty attendees list would cause a runtime error.
Evidence:
- Line 27:
name: this.calEvent.attendees[0].name— no bounds check
Agent: logic
| (await isTeamOwner(user.id, booking.eventType?.teamId ?? 0)); | ||
|
|
||
| const isOrganizer = booking.userId === user.id; | ||
|
|
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — architecture agent (Small fix (10 lines, 1 file))
The team admin/owner authorization check uses
&&(AND) instead of||(OR), meaning a user must be BOTH a team admin AND a team owner to pass this check. This effectively makes the team admin/owner path unreachable for anyone who is only an admin but not an owner. The intent was likely to allow either team admins or team owners.
| // Only check team admin/owner when the booking belongs to a team event (teamId is non-null). | |
| // Using teamId=0 as a fallback is semantically incorrect and could yield unexpected results. | |
| // The variable name signals OR semantics: either admin OR owner should be sufficient. | |
| const teamId = booking.eventType?.teamId; | |
| const isTeamAdminOrOwner = teamId | |
| ? (await isTeamAdmin(user.id, teamId)) || (await isTeamOwner(user.id, teamId)) | |
| : false; |
🤖 Grapple PR auto-fix • critical • confidence: 100%
| export const ZAddGuestsInputSchema = z.object({ | ||
| bookingId: z.number(), | ||
| guests: z.array(z.string().email()), | ||
| }); |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — architecture agent (Small fix (8 lines, 1 file))
The schema allows an empty guests array (
z.array(z.string().email())with no.min(1)) and has no upper bound on the number of guests. An empty array would pass schema validation and then hit theuniqueGuests.length === 0check only after database lookups. A very large array could cause performance issues in the handler's email dispatch loop and database operations.
| }); | |
| // Min(1): Prevents empty arrays from passing validation and reaching the handler's | |
| // `uniqueGuests.length === 0` guard (which would otherwise trigger after DB lookups). | |
| // Max(10): Caps guest additions per request to prevent abuse in the email dispatch | |
| // loop and bulk database operations. Adjust this limit to match business requirements. | |
| const MAX_GUESTS_PER_REQUEST = 10; | |
| guests: z.array(z.string().email()).min(1).max(MAX_GUESTS_PER_REQUEST), |
🤖 Grapple PR auto-fix • major • confidence: 100%
|
|
||
| await Promise.all(emailsToSend); | ||
| }; | ||
| export const sendFeedbackEmail = async (feedback: Feedback) => { |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — architecture agent (Small fix (2 lines, 1 file))
The
sendAddGuestsEmailsfunction accepts anewGuestsparameter but then checksguests(from the outer scope of the caller) againstcalendarEvent.attendees. The logic sendsAttendeeScheduledEmailto new guests andAttendeeAddGuestsEmailto existing attendees, but the checknewGuests.includes(attendee.email)compares against the original input emails. Since the handler passes the fullcalendarEventwith all attendees (including pre-existing ones), every attendee gets an email — but new guests get a booking confirmation while existing attendees get a 'guests added' notification. This works but the parameter naming (newGuestsvs the rawguestsinput which may include duplicates that were filtered) could cause incorrect email routing if the caller passes unfiltered input.
| export const sendFeedbackEmail = async (feedback: Feedback) => { | |
| await sendAddGuestsEmails(evt, uniqueGuests); |
🤖 Grapple PR auto-fix • major • confidence: 100%
| const guestsFullDetails = uniqueGuests.map((guest) => { | ||
| return { | ||
| name: "", | ||
| email: guest, |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — logic agent (Small fix (8 lines, 1 file))
When all submitted guests are already attendees (duplicates), the handler throws a BAD_REQUEST with message 'emails_must_be_unique_valid'. Per the acceptance criteria, adding a guest who is already an attendee 'should be deduplicated without error'. The current behavior throws an error instead of silently succeeding or returning a helpful message.
| email: guest, | |
| // All submitted guests are already attendees or blacklisted — deduplicate silently | |
| // per acceptance criteria: "adding a guest who is already an attendee should be | |
| // deduplicated without error". | |
| if (uniqueGuests.length === 0) { | |
| return { message: "No new guests to add" }; | |
| } |
🤖 Grapple PR auto-fix • major • confidence: 92%
| const utils = trpc.useUtils(); | ||
| const [multiEmailValue, setMultiEmailValue] = useState<string[]>([""]); | ||
| const [isInvalidEmail, setIsInvalidEmail] = useState(false); | ||
|
|
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — logic agent (Small fix (5 lines, 1 file))
The
isInvalidEmailerror state is set totrueon validation failure but never reset when the user modifies the email input. The error message persists even after the user corrects their input until they click Cancel or successfully submit.
| setValue={(newValue) => { | |
| setIsInvalidEmail(false); | |
| setMultiEmailValue(newValue); | |
| }} |
🤖 Grapple PR auto-fix • minor • confidence: 93%
| const { isOpenDialog, setIsOpenDialog, bookingId } = props; | ||
| const utils = trpc.useUtils(); | ||
| const [multiEmailValue, setMultiEmailValue] = useState<string[]>([""]); | ||
| const [isInvalidEmail, setIsInvalidEmail] = useState(false); |
There was a problem hiding this comment.
✅ [Grapple PR] Auto-fix — logic agent (Small fix (7 lines, 1 file))
The multiEmailValue is initialized with
[''](an array containing one empty string). When the user clicks 'Add' without entering anything,multiEmailValue.lengthis 1 (not 0), so the early return on line 51 is bypassed. The empty string then fails the.email()zod validation and shows the invalid email error. While this works functionally, it's a confusing UX — the check on line 51 is dead code given the initial state.
| const [isInvalidEmail, setIsInvalidEmail] = useState(false); | |
| const nonEmptyEmails = multiEmailValue.filter((email) => email.trim() !== ""); | |
| if (nonEmptyEmails.length === 0) { | |
| const validationResult = ZAddGuestsInputSchema.safeParse(nonEmptyEmails); | |
| addGuestsMutation.mutate({ bookingId, guests: nonEmptyEmails }); |
🤖 Grapple PR auto-fix • minor • confidence: 89%
| } | ||
|
|
||
| return { message: "Guests added" }; | ||
| }; |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (20 lines, 1 file) — review recommended)
The handler does not check the booking status before adding guests. Per the acceptance criteria, adding guests to a cancelled or past booking should be rejected. A user could add guests to a CANCELLED, REJECTED, or already-ended booking, which would result in nonsensical email notifications and database mutations.
--- a/packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts
+++ b/packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts
@@ -1,6 +1,8 @@
import EventManager from "@calcom/core/EventManager";
import dayjs from "@calcom/dayjs";
import { sendAddGuestsEmails } from "@calcom/emails";
import { parseRecurringEvent } from "@calcom/lib";
import { getTranslation } from "@calcom/lib/server";
import { getUsersCredentials } from "@calcom/lib/server/getUsersCredentials";
import { isTeamAdmin, isTeamOwner } from "@calcom/lib/server/queries/teams";
import { prisma } from "@calcom/prisma";
+import { BookingStatus } from "@calcom/prisma/enums";
import type { CalendarEvent } from "@calcom/types/Calendar";
@@ -54,6 +56,19 @@ export const addGuestsHandler = async ({ ctx, input }: AddGuestsOptions) => {
if (!isTeamAdminOrOwner && !isOrganizer && !isAttendee) {
throw new TRPCError({ code: "FORBIDDEN", message: "you_do_not_have_permission" });
}
+ // Reject mutations on bookings with a terminal status — adding guests to a
+ // cancelled or rejected booking would result in nonsensical email notifications
+ // and orphaned attendee records.
+ if (booking.status === BookingStatus.CANCELLED || booking.status === BookingStatus.REJECTED) {
+ throw new TRPCError({
+ code: "BAD_REQUEST",
+ message: "cannot_add_guests_to_cancelled_or_rejected_booking",
+ });
+ }
+
+ // Reject mutations on past bookings — the event has already ended so adding
+ // guests serves no purpose and would send misleading notifications.
+ if (booking.endTime && dayjs(booking.endTime).isBefore(dayjs())) {
+ throw new TRPCError({
+ code: "BAD_REQUEST",
+ message: "cannot_add_guests_to_past_booking",
+ });
+ }
+
const organizer = await prisma.user.findFirstOrThrow({🤖 Grapple PR auto-fix • critical • Review this diff before applying
|
|
||
| if (!booking) throw new TRPCError({ code: "NOT_FOUND", message: "booking_not_found" }); | ||
|
|
||
| const isTeamAdminOrOwner = |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (22 lines, 1 file) — review recommended)
The handler does not check the booking status before allowing guests to be added. Guests can be added to cancelled, rejected, or past bookings. The handler should verify the booking is in an active/confirmed state and that the event hasn't already ended.
--- a/packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts
+++ b/packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts
@@ -1,6 +1,7 @@
import EventManager from "@calcom/core/EventManager";
import dayjs from "@calcom/dayjs";
import { sendAddGuestsEmails } from "@calcom/emails";
import { parseRecurringEvent } from "@calcom/lib";
import { getTranslation } from "@calcom/lib/server";
import { getUsersCredentials } from "@calcom/lib/server/getUsersCredentials";
import { isTeamAdmin, isTeamOwner } from "@calcom/lib/server/queries/teams";
import { prisma } from "@calcom/prisma";
+import { BookingStatus } from "@calcom/prisma/enums";
import type { CalendarEvent } from "@calcom/types/Calendar";
import { TRPCError } from "@trpc/server";
@@ -44,6 +45,22 @@ export const addGuestsHandler = async ({ ctx, input }: AddGuestsOptions) => {
if (!booking) throw new TRPCError({ code: "NOT_FOUND", message: "booking_not_found" });
+ // Reject attempts to add guests to bookings that are not in an active/confirmed state.
+ // PENDING is excluded because the booking has not yet been confirmed by the host.
+ if (
+ booking.status === BookingStatus.CANCELLED ||
+ booking.status === BookingStatus.REJECTED ||
+ booking.status === BookingStatus.PENDING
+ ) {
+ throw new TRPCError({
+ code: "BAD_REQUEST",
+ message: "cannot_add_guests_to_inactive_booking",
+ });
+ }
+
+ // Reject attempts to add guests to bookings whose event has already ended.
+ if (dayjs(booking.endTime).isBefore(dayjs())) {
+ throw new TRPCError({
+ code: "BAD_REQUEST",
+ message: "cannot_add_guests_to_past_booking",
+ });
+ }
+
const isTeamAdminOrOwner =
(await isTeamAdmin(user.id, booking.eventType?.teamId ?? 0)) &&
(await isTeamOwner(user.id, booking.eventType?.teamId ?? 0));🤖 Grapple PR auto-fix • major • Review this diff before applying
| attendees: { | ||
| createMany: { | ||
| data: guestsFullDetails, | ||
| }, |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — logic agent (Larger fix (61 lines, 1 file) — review recommended)
There is a TOCTOU race condition between checking existing attendees (line 79-84) and creating new attendees (line 88-103). If two concurrent requests to add guests to the same booking are made, both could pass the deduplication check and create duplicate attendees in the database. The read-then-write is not atomic.
--- a/packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts
+++ b/packages/trpc/server/routers/viewer/bookings/addGuests.handler.ts
@@ -69,22 +69,42 @@
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)
);
if (uniqueGuests.length === 0)
throw new TRPCError({ code: "BAD_REQUEST", message: "emails_must_be_unique_valid" });
const guestsFullDetails = uniqueGuests.map((guest) => {
return {
name: "",
email: guest,
timeZone: organizer.timeZone,
locale: organizer.locale,
};
});
- const bookingAttendees = await prisma.booking.update({
- where: {
- id: bookingId,
- },
- include: {
- attendees: true,
- },
- data: {
- attendees: {
- createMany: {
- data: guestsFullDetails,
+ // Re-fetch current attendees and perform the write atomically inside a
+ // transaction to close the TOCTOU window between the deduplication check
+ // above (which read attendees at request-start) and the actual insert.
+ // Two concurrent requests could both pass the uniqueGuests filter above and
+ // then both attempt to insert the same emails. Using skipDuplicates:true on
+ // createMany provides a database-level safety net, and wrapping in a
+ // transaction ensures the re-check + write are atomic.
+ const bookingAttendees = await prisma.$transaction(async (tx) => {
+ // Re-read the current attendees inside the transaction so we have the
+ // freshest view before writing.
+ const currentBooking = await tx.booking.findUniqueOrThrow({
+ where: { id: bookingId },
+ select: { attendees: { select: { email: true } } },
+ });
+
+ const currentEmails = new Set(currentBooking.attendees.map((a) => a.email));
+
+ // Filter out any guests that were added by a concurrent request between
+ // the outer check and now.
+ const dedupedGuests = guestsFullDetails.filter((g) => !currentEmails.has(g.email));
+
+ // If every requested guest was already added by a concurrent write, treat
+ // it as a no-op and return the current booking state rather than erroring,
+ // so the caller still gets a consistent response.
+ if (dedupedGuests.length === 0) {
+ return tx.booking.findUniqueOrThrow({
+ where: { id: bookingId },
+ include: { attendees: true },
+ });
+ }
+
+ return tx.booking.update({
+ where: {
+ id: bookingId,
+ },
+ include: {
+ attendees: true,
+ },
+ data: {
+ attendees: {
+ createMany: {
+ // skipDuplicates acts as a final database-level guard in case
+ // there is a unique constraint on (bookingId, email). This makes
+ // the insert idempotent even if our in-transaction re-check above
+ // somehow misses an edge case.
+ data: dedupedGuests,
+ skipDuplicates: true,
+ },
},
},
- },
+ });
});🤖 Grapple PR auto-fix • major • Review this diff before applying
| const [multiEmailValue, setMultiEmailValue] = useState<string[]>([""]); | ||
| const [isInvalidEmail, setIsInvalidEmail] = useState(false); | ||
|
|
||
| const addGuestsMutation = trpc.viewer.bookings.addGuests.useMutation({ |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (14 lines, 1 file) — review recommended)
The initial state of
multiEmailValueis[""](an array with one empty string). ThehandleAddfunction checksmultiEmailValue.length === 0as a guard, but since the initial value has length 1, this guard never triggers. If the user clicks 'Add' without typing anything, the empty string will fail the Zod email validation and show an error, but the UX would be better if the submit was disabled when no valid emails are entered.
--- a/apps/web/components/dialog/AddGuestsDialog.tsx
+++ b/apps/web/components/dialog/AddGuestsDialog.tsx
@@ -49,9 +49,12 @@ export const AddGuestsDialog = (props: IAddGuestsDialog) => {
const handleAdd = () => {
- if (multiEmailValue.length === 0) {
+ // Filter out empty strings before validation; initial state is [""] so
+ // length === 0 check never triggers without this normalization.
+ const nonEmptyEmails = multiEmailValue.filter((email) => email.trim() !== "");
+ if (nonEmptyEmails.length === 0) {
return;
}
- const validationResult = ZAddGuestsInputSchema.safeParse(multiEmailValue);
+ const validationResult = ZAddGuestsInputSchema.safeParse(nonEmptyEmails);
if (validationResult.success) {
- addGuestsMutation.mutate({ bookingId, guests: multiEmailValue });
+ addGuestsMutation.mutate({ bookingId, guests: nonEmptyEmails });
} else {
setIsInvalidEmail(true);
@@ -90,7 +93,10 @@ export const AddGuestsDialog = (props: IAddGuestsDialog) => {
<Button data-testid="add_members" loading={addGuestsMutation.isPending} onClick={handleAdd}>
+ <Button
+ data-testid="add_members"
+ loading={addGuestsMutation.isPending}
+ onClick={handleAdd}
+ disabled={multiEmailValue.filter((email) => email.trim() !== "").length === 0}>
{t("add")}
</Button>🤖 Grapple PR auto-fix • minor • Review this diff before applying
| setIsOpenAddGuestsDialog(true); | ||
| }, | ||
| icon: "user-plus" as const, | ||
| }, |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (38 lines, 1 file) — review recommended)
The 'Add Guests' action is added to the
bookingActionItemsarray unconditionally, meaning it will appear for all booking states including cancelled, rejected, and past bookings. Other actions in this component likely have conditional visibility based on booking status. Without server-side status validation (also flagged), this creates a confusing UX where users can attempt to add guests to non-modifiable bookings.
--- a/apps/web/components/booking/BookingListItem.tsx
+++ b/apps/web/components/booking/BookingListItem.tsx
@@ -183,19 +183,25 @@ function BookingListItem(booking: BookingItemProps) {
},
},
- {
- id: "change_location",
- label: t("edit_location"),
- onClick: () => {
- setIsOpenLocationDialog(true);
- },
- icon: "map-pin" as const,
- },
- {
- id: "add_members",
- label: t("additional_guests"),
- onClick: () => {
- setIsOpenAddGuestsDialog(true);
- },
- icon: "user-plus" as const,
- },
];
+
+ const isBookingModifiable =
+ booking.status === BookingStatus.ACCEPTED || booking.status === BookingStatus.PENDING;
+
+ if (isBookingModifiable) {
+ editBookingActions.push({
+ id: "change_location",
+ label: t("edit_location"),
+ onClick: () => {
+ setIsOpenLocationDialog(true);
+ },
+ icon: "map-pin" as const,
+ });
+ editBookingActions.push({
+ id: "add_members",
+ label: t("additional_guests"),
+ onClick: () => {
+ setIsOpenAddGuestsDialog(true);
+ },
+ icon: "user-plus" as const,
+ });
+ }
if (booking.eventType.schedulingType === SchedulingType.ROUND_ROBIN) {🤖 Grapple PR auto-fix • minor • Review this diff before applying
| return uniqueEmails.size === emails.length; | ||
| }); | ||
| const { isOpenDialog, setIsOpenDialog, bookingId } = props; | ||
| const utils = trpc.useUtils(); |
There was a problem hiding this comment.
🟡 [Grapple PR] Suggested fix — architecture agent (Larger fix (12 lines, 1 file) — review recommended)
The Zod schema
ZAddGuestsInputSchemais instantiated inside the component render function, meaning it gets recreated on every render. This is a minor performance issue and a pattern violation — validation schemas should be defined outside the component or memoized, consistent with how schemas are defined in dedicated.schema.tsfiles elsewhere in the codebase.
--- a/apps/web/components/dialog/AddGuestsDialog.tsx
+++ b/apps/web/components/dialog/AddGuestsDialog.tsx
@@ -1,6 +1,7 @@
import type { Dispatch, SetStateAction } from "react";
import { useState } from "react";
import { z } from "zod";
+
import { useLocale } from "@calcom/lib/hooks/useLocale";
import { trpc } from "@calcom/trpc/react";
import {
@@ -17,15 +18,15 @@ import {
showToast,
} from "@calcom/ui";
+// Defined at module level to avoid re-instantiation on every render.
+// Validates that all guest emails are valid and unique.
+const ZAddGuestsInputSchema = z.array(z.string().email()).refine((emails) => {
+ const uniqueEmails = new Set(emails);
+ return uniqueEmails.size === emails.length;
+});
+
interface IAddGuestsDialog {
isOpenDialog: boolean;
setIsOpenDialog: Dispatch<SetStateAction<boolean>>;
bookingId: number;
}
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;
- });
const { isOpenDialog, setIsOpenDialog, bookingId } = props;🤖 Grapple PR auto-fix • minor • Review this diff before applying
2 participants
Benchmark test PR #10 - Replicating Greptile benchmark for Grapple PR evaluation.