Add guest management functionality to existing bookings#9
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>
|
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Details: |
|
Claude finished @ShashankFC's task in 2m 37s —— View job Code Review Complete ✓Tasks
OverviewThis PR adds functionality to add guests to existing bookings. The implementation includes a UI dialog, tRPC endpoint, email notifications, and calendar event updates. Overall, the feature is well-structured, but there are several critical security issues, bugs, and code quality concerns that need to be addressed. 🔴 Critical Issues1. Authorization Logic Bug (Security)File: const isTeamAdminOrOwner =
(await isTeamAdmin(user.id, booking.eventType?.teamId ?? 0)) &&
(await isTeamOwner(user.id, booking.eventType?.teamId ?? 0));Problem: Uses Fix: Should be 2. Missing Email Validation in Frontend SchemaFile: const ZAddGuestsInputSchema = z.array(z.string().email()).refine((emails) => {
const uniqueEmails = new Set(emails);
return uniqueEmails.size === emails.length;
});Problem: The frontend validates uniqueness but doesn't filter out empty strings before validation. The initial state is Impact: Line 49 checks 🟡 Important Issues3. Poor Error Handling in Email SendingFile: try {
await sendAddGuestsEmails(evt, guests);
} catch (err) {
console.log("Error sending AddGuestsEmails");
}Problems:
Suggestion: Either propagate the error or log properly with the logger that's already imported but unused. 4. Unused ImportFile: The 5. Case-Sensitive Email Filtering IssueFile: const uniqueGuests = guests.filter(
(guest) =>
!booking.attendees.some((attendee) => guest === attendee.email) &&
!blacklistedGuestEmails.includes(guest)
);Problem: The Note: Blacklisted emails are lowercased (line 71), but incoming guests are not lowercased before comparison. 6. Calendar Update Error HandlingFile: await eventManager.updateCalendarAttendees(evt, booking);Problem: No error handling. If calendar provider (Google Calendar, etc.) fails, the database will have the guests added, but the calendar event won't be updated. This creates data inconsistency. Suggestion: Wrap in try-catch and decide whether to rollback the database changes or just log/warn the user. 🔵 Code Quality Issues7. Missing Required TranslationsFile: New translation keys added:
Problem: Only English translations are added. Other locales need these keys too, or they'll fall back to the key name for non-English users. 8. Inconsistent Error State ResetFile: The cancel button resets 9. Potential Race ConditionFile: The success handler calls Low priority but consider awaiting the invalidation. 10. Empty Name for New GuestsFile: const guestsFullDetails = uniqueGuests.map((guest) => {
return {
name: "",
email: guest,
timeZone: organizer.timeZone,
locale: organizer.locale,
};
});All new guests get an empty string for name. While this might be intentional (since we only have emails), it could cause display issues in UIs that expect names. Consider using the email username part or "Guest" as a fallback. 11. Missing Accessibility LabelFile: <Tooltip content="Remove email">The remove button inside the tooltip doesn't have an 12. PR Title Doesn't Follow ConventionAs noted by the bot, the PR title should follow Conventional Commits. It should be: ✅ Positive Aspects
SummaryMust Fix Before Merge:
Should Fix:
Nice to Have:
|
|
This PR is being marked as stale due to inactivity. |
2 participants
Test 10nn---nReplicated from ai-code-review-evaluation/cal.com-greptile#10