Add guest management functionality to existing bookings

[akshayutture-augment]

Test 10

---

Replicated from ai-code-review-evaluation/cal.com-greptile#10

[akshayutture-augment]

@claude review this PR

[akshayutture-augment]

@claude review this PR

[claude]

Claude finished @akshayutture-augment's task —— View job

---

PR Review Complete ✅

I've completed a thorough review of this PR that adds guest management functionality to existing bookings. Overall, this is a well-structured implementation, but there are several important issues that should be addressed.

---

🔴 Critical Issues

1. Authorization Logic Flaw in addGuests.handler.ts:46-48

const isTeamAdminOrOwner =
  (await isTeamAdmin(user.id, booking.eventType?.teamId ?? 0)) &&
  (await isTeamOwner(user.id, booking.eventType?.teamId ?? 0));

Problem: This uses && (AND) operator, meaning a user must be BOTH a team admin AND a team owner to pass this check. This is likely incorrect - it should use || (OR) since being either an admin OR an owner should grant permission.

Impact: Team admins who are not owners will be incorrectly denied access.

Suggested fix:

const isTeamAdminOrOwner =
  (await isTeamAdmin(user.id, booking.eventType?.teamId ?? 0)) ||
  (await isTeamOwner(user.id, booking.eventType?.teamId ?? 0));

---

🟡 High Priority Issues

2. Missing Email Validation on Server Side (addGuests.handler.ts)

The handler relies only on the Zod schema validation for email format. While the schema validates emails, there's no additional sanitization or normalization (e.g., trimming whitespace, lowercasing).

Recommendation: Add email normalization:

const uniqueGuests = guests
  .map(email => email.trim().toLowerCase())
  .filter(/* existing filter logic */);

3. Race Condition: Duplicate Guest Prevention (addGuests.handler.ts:74-78)

The code checks for duplicate guests before insertion, but there's a potential race condition if multiple requests add the same guest simultaneously.

Recommendation: Consider using database constraints or handling unique constraint violations gracefully.

4. Insufficient Error Handling in Email Sending (addGuests.handler.ts:167-171)

try {
  await sendAddGuestsEmails(evt, guests);
} catch (err) {
  console.log("Error sending AddGuestsEmails");
}

Issues:

• Uses console.log instead of proper logging
• Swallows the error without notifying the user
• Guest records are already created in DB, but emails failed silently

Recommendation: Use proper logger and consider notifying users of email failures.

5. Missing Input Validation in AddGuestsDialog.tsx:49-51

if (multiEmailValue.length === 0) {
  return;
}

This checks if the array is empty but doesn't check if the array contains only empty strings (e.g., ["", ""]).

Recommendation: Add validation:

const nonEmptyEmails = multiEmailValue.filter(email => email.trim() !== "");
if (nonEmptyEmails.length === 0) {
  return;
}

6. Empty Name for New Guests (addGuests.handler.ts:84-86)

const guestsFullDetails = uniqueGuests.map((guest) => {
  return {
    name: "",  // ← Always empty
    email: guest,
    timeZone: organizer.timeZone,
    locale: organizer.locale,
  };
});

New guests are always created with an empty name, which may cause display issues in the UI.

---

🟠 Medium Priority Issues

7. Inconsistent Validation Between Client and Server

• Client (AddGuestsDialog.tsx:26-29): Validates uniqueness within the new guests being added
• Server (addGuests.handler.ts:74-78): Validates uniqueness against existing attendees + blacklist

The client should also check against existing attendees to provide better UX feedback before submission.

8. Missing Translation Keys

The error handling assumes translation keys exist, but I can see these are added in common.json. Good! However, verify all keys are properly defined:

• guests_added ✅
• unable_to_add_guests ❓ (not seen in the diff)
• emails_must_be_unique_valid ✅
• new_guests_added ✅
• guests_added_event_type_subject ✅

9. No Loading State Reset on Error (AddGuestsDialog.tsx:42-46)

If the mutation fails, the dialog closes and the error shows, but if the user reopens the dialog, they'll see the same invalid state. The multiEmailValue should be reset on dialog close, not just on success.

10. Calendar Update May Fail Silently (addGuests.handler.ts:165)

await eventManager.updateCalendarAttendees(evt, booking);

This can fail but there's no error handling. If calendar integration fails, guests are added to Cal.com but not to the external calendar.

---

🟢 Low Priority / Style Issues

11. Hardcoded Blacklist Environment Variable (addGuests.handler.ts:70-72)

The blacklist feature is good for security, but it's not documented. Consider adding:

• Documentation about BLACKLISTED_GUEST_EMAILS env var
• A comment explaining the security purpose

12. Component Props Interface Naming (AddGuestsDialog.tsx:18-22)

interface IAddGuestsDialog {

The I prefix is an outdated TypeScript convention. Modern practice uses AddGuestsDialogProps.

13. Unused Import (MultiEmail.tsx)

The component doesn't seem to use all its imports. Verify if everything is needed.

14. Missing Component Test ID (AddGuestsDialog.tsx)

The "Add" button has data-testid="add_members" which seems misnamed (should be add_guests for consistency).

---

🧪 Testing Considerations

Missing test coverage for:

1. Authorization edge cases (team admin but not owner, vice versa)
2. Race conditions with duplicate guests
3. Email sending failures
4. Calendar integration failures
5. Empty string handling in MultiEmail component
6. Blacklisted email handling
7. Maximum guest limits (is there a limit?)

---

🎯 Security Review

Findings:

1. ✅ Email validation present
2. ✅ Authorization checks in place (but with logic bug feat: add calendar cache status and actions (#22532) #1)
3. ✅ Blacklist functionality for preventing spam
4. ⚠️ No rate limiting (users could spam add guests repeatedly)
5. ⚠️ No maximum guest limit check
6. ✅ TRPC procedures properly authenticated

---

📝 Code Quality

Positive aspects:

• Good separation of concerns (UI, API, email templates)
• Proper use of TRPC for type safety
• Internationalization properly implemented
• Email templates reuse existing components
• Calendar integration properly updated

Areas for improvement:

• Error handling and logging
• Input validation consistency
• Race condition handling

---

✅ Recommendations Summary

Must fix before merge:

1. Fix authorization logic bug (issue feat: add calendar cache status and actions (#22532) #1)
2. Add proper email normalization (issue Add guest management functionality to existing bookings #2)
3. Improve error handling for email sending (issue OAuth credential sync and app integration enhancements #4)
4. Fix empty string validation (issue Advanced date override handling and timezone compatibility improvements #5)

Should fix:
5. Address race condition potential (issue #3)
6. Handle empty names for guests (issue #6)
7. Improve client-side validation (issue #7)
8. Add proper error handling for calendar updates (issue #10)

Nice to have:
9. Add rate limiting
10. Add maximum guest limits
11. Improve test coverage
12. Update component naming conventions

---

[github-actions]

This PR is being marked as stale due to inactivity.