fix: handle collective multiple host on destinationCalendar#9
Open
ShashankFC wants to merge 1 commit into
Open
Conversation
Author
|
@cubic-dev-ai review this pull request |
Contributor
@ShashankFC I have started the AI code review. It will take a few minutes to complete. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
10 issues found across 22 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/features/bookings/lib/handleCancelBooking.ts">
<violation number="1" location="packages/features/bookings/lib/handleCancelBooking.ts:449">
P1: Bug: Recurring event cancellation duplicates deletions and misses non-organizer hosts' calendars.
The recurring event branch (lines 449–461) iterates `bookingToDelete.user.credentials` for every outer `reference` in the loop. Since that inner logic is independent of the outer `reference`, this causes:
1. **Duplicate deletions** — the same calendar events are deleted once per outer reference.
2. **Missed deletions** — credentials fetched from DB (for non-organizer hosts) are never used in the recurring path because the inner loop only checks `bookingToDelete.user.credentials`.
Consider using the already-resolved `calendarCredential` (which may have been fetched from DB) for the delete call, and restructuring so the recurring path runs per-reference rather than iterating all user credentials.</violation>
<violation number="2" location="packages/features/bookings/lib/handleCancelBooking.ts:454">
P2: Inconsistency: `updBooking.references.find()` only gets the first calendar reference, but the PR's intent is to handle multiple calendar references.
The outer code was changed from `.find()` to `.filter()` to support multiple destination calendars. However, this inner lookup within the recurring event path still uses `.find()`, so it will only delete the first calendar reference per updated booking, missing other hosts' calendar events.</violation>
</file>
<file name="apps/web/playwright/webhook.e2e.ts">
<violation number="1" location="apps/web/playwright/webhook.e2e.ts:249">
P1: Inconsistent `destinationCalendar` expectation across webhook tests. This test now expects `[]` but the BOOKING_REQUESTED test at line 373 (which uses the same opt-in event flow via `bookOptinEvent`) and the BOOKING_CREATED test at line 119 still expect `null`. If the underlying payload shape changed to an array, those tests will fail; if it didn't, this one will fail.</violation>
</file>
<file name="packages/core/CalendarManager.ts">
<violation number="1" location="packages/core/CalendarManager.ts:240">
P3: Typo in comment: "witch" should be "with" ("doesn't match **with** certain credential").</violation>
</file>
<file name="packages/app-store/googlecalendar/lib/CalendarService.ts">
<violation number="1" location="packages/app-store/googlecalendar/lib/CalendarService.ts:256">
P1: Bug: The fallback branch of this ternary is dead code. When `externalCalendarId` is falsy (else branch), the `.find()` compares `cal.externalId === externalCalendarId` where `externalCalendarId` is already known to be falsy — so it will never match a valid calendar. `selectedCalendar` will always be `undefined` in the fallback case, which may cause the Google Calendar API call to fail or target the wrong calendar.
Consider using a similar approach to `createEvent`, which uses `credentialId` for the lookup, or fall back to the first destination calendar's `externalId`.</violation>
</file>
<file name="packages/trpc/server/routers/viewer/organizations/create.handler.ts">
<violation number="1" location="packages/trpc/server/routers/viewer/organizations/create.handler.ts:151">
P0: Bug: Logic inversion — `slug` is now set when billing IS enabled, instead of when it's NOT enabled. The original `!IS_TEAM_BILLING_ENABLED` was negated, but the ternary replacement dropped the negation. This means organizations will get their slug immediately even when team billing is enabled (bypassing the requestedSlug approval flow), and will never get a slug when billing is disabled.</violation>
</file>
<file name="packages/core/EventManager.ts">
<violation number="1" location="packages/core/EventManager.ts:119">
P1: Accessing `.integration` on `mainHostDestinationCalendar` will throw a `TypeError` when `evt.destinationCalendar` is nullish or empty (destructuring yields `undefined`). Add an optional chaining guard or check that `mainHostDestinationCalendar` is defined before accessing its properties.</violation>
<violation number="2" location="packages/core/EventManager.ts:548">
P1: `updateEvent` is called with a potentially `undefined` credential. If the credential is not found in the in-memory list AND the DB lookup fails (no record or no `app.slug`), `credential` remains `undefined`. Add a guard like the one used in `createAllCalendarEvents`: `if (credential) { result.push(...) }`.</violation>
</file>
<file name="packages/features/bookings/lib/handleNewBooking.ts">
<violation number="1" location="packages/features/bookings/lib/handleNewBooking.ts:737">
P1: The Prisma `select` for dynamic users no longer includes `organization: { select: { slug: true } }`, which was present in the old code. Downstream code (and the `IsFixedAwareUser` type) expects `organization.slug` to be present. This will cause a runtime TypeError for dynamic bookings when the organization field is accessed.</violation>
<violation number="2" location="packages/features/bookings/lib/handleNewBooking.ts:761">
P1: The catch block swallows the specific validation `Error` messages thrown inside `loadUsers` (e.g., `"dynamicUserList is not properly defined or empty."` and `"eventType.hosts is not properly defined."`). Since these are plain `Error` instances—not `HttpError` or `PrismaClientKnownRequestError`—they fall through to the generic `throw new HttpError({ statusCode: 500, message: "Unable to load users" })`. This loses the descriptive error message and returns a 500 instead of a 400.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| create: { | ||
| name, | ||
| ...(!IS_TEAM_BILLING_ENABLED && { slug }), | ||
| ...(IS_TEAM_BILLING_ENABLED ? { slug } : {}), |
Contributor
There was a problem hiding this comment.
P0: Bug: Logic inversion — slug is now set when billing IS enabled, instead of when it's NOT enabled. The original !IS_TEAM_BILLING_ENABLED was negated, but the ternary replacement dropped the negation. This means organizations will get their slug immediately even when team billing is enabled (bypassing the requestedSlug approval flow), and will never get a slug when billing is disabled.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/trpc/server/routers/viewer/organizations/create.handler.ts, line 151:
<comment>Bug: Logic inversion — `slug` is now set when billing IS enabled, instead of when it's NOT enabled. The original `!IS_TEAM_BILLING_ENABLED` was negated, but the ternary replacement dropped the negation. This means organizations will get their slug immediately even when team billing is enabled (bypassing the requestedSlug approval flow), and will never get a slug when billing is disabled.</comment>
<file context>
@@ -148,9 +148,9 @@ export const createHandler = async ({ input, ctx }: CreateOptions) => {
create: {
name,
- ...(!IS_TEAM_BILLING_ENABLED && { slug }),
+ ...(IS_TEAM_BILLING_ENABLED ? { slug } : {}),
metadata: {
- ...(IS_TEAM_BILLING_ENABLED && { requestedSlug: slug }),
</file context>
Suggested change
| ...(IS_TEAM_BILLING_ENABLED ? { slug } : {}), | |
| ...(!IS_TEAM_BILLING_ENABLED ? { slug } : {}), |
| bookingToDelete.recurringEventId && | ||
| allRemainingBookings | ||
| ) { | ||
| const promises = bookingToDelete.user.credentials |
Contributor
There was a problem hiding this comment.
P1: Bug: Recurring event cancellation duplicates deletions and misses non-organizer hosts' calendars.
The recurring event branch (lines 449–461) iterates bookingToDelete.user.credentials for every outer reference in the loop. Since that inner logic is independent of the outer reference, this causes:
- Duplicate deletions — the same calendar events are deleted once per outer reference.
- Missed deletions — credentials fetched from DB (for non-organizer hosts) are never used in the recurring path because the inner loop only checks
bookingToDelete.user.credentials.
Consider using the already-resolved calendarCredential (which may have been fetched from DB) for the delete call, and restructuring so the recurring path runs per-reference rather than iterating all user credentials.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/features/bookings/lib/handleCancelBooking.ts, line 449:
<comment>Bug: Recurring event cancellation duplicates deletions and misses non-organizer hosts' calendars.
The recurring event branch (lines 449–461) iterates `bookingToDelete.user.credentials` for every outer `reference` in the loop. Since that inner logic is independent of the outer `reference`, this causes:
1. **Duplicate deletions** — the same calendar events are deleted once per outer reference.
2. **Missed deletions** — credentials fetched from DB (for non-organizer hosts) are never used in the recurring path because the inner loop only checks `bookingToDelete.user.credentials`.
Consider using the already-resolved `calendarCredential` (which may have been fetched from DB) for the delete call, and restructuring so the recurring path runs per-reference rather than iterating all user credentials.</comment>
<file context>
@@ -411,58 +415,71 @@ async function handler(req: CustomRequest) {
+ bookingToDelete.recurringEventId &&
+ allRemainingBookings
+ ) {
+ const promises = bookingToDelete.user.credentials
+ .filter((credential) => credential.type.endsWith("_calendar"))
+ .map(async (credential) => {
</file context>
| ], | ||
| location: "[redacted/dynamic]", | ||
| destinationCalendar: null, | ||
| destinationCalendar: [], |
Contributor
There was a problem hiding this comment.
P1: Inconsistent destinationCalendar expectation across webhook tests. This test now expects [] but the BOOKING_REQUESTED test at line 373 (which uses the same opt-in event flow via bookOptinEvent) and the BOOKING_CREATED test at line 119 still expect null. If the underlying payload shape changed to an array, those tests will fail; if it didn't, this one will fail.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/playwright/webhook.e2e.ts, line 249:
<comment>Inconsistent `destinationCalendar` expectation across webhook tests. This test now expects `[]` but the BOOKING_REQUESTED test at line 373 (which uses the same opt-in event flow via `bookOptinEvent`) and the BOOKING_CREATED test at line 119 still expect `null`. If the underlying payload shape changed to an array, those tests will fail; if it didn't, this one will fail.</comment>
<file context>
@@ -246,7 +246,7 @@ test.describe("BOOKING_REJECTED", async () => {
],
location: "[redacted/dynamic]",
- destinationCalendar: null,
+ destinationCalendar: [],
// hideCalendarNotes: false,
requiresConfirmation: "[redacted/dynamic]",
</file context>
| const selectedCalendar = externalCalendarId | ||
| ? externalCalendarId | ||
| : event.destinationCalendar?.externalId; | ||
| : event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId; |
Contributor
There was a problem hiding this comment.
P1: Bug: The fallback branch of this ternary is dead code. When externalCalendarId is falsy (else branch), the .find() compares cal.externalId === externalCalendarId where externalCalendarId is already known to be falsy — so it will never match a valid calendar. selectedCalendar will always be undefined in the fallback case, which may cause the Google Calendar API call to fail or target the wrong calendar.
Consider using a similar approach to createEvent, which uses credentialId for the lookup, or fall back to the first destination calendar's externalId.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/app-store/googlecalendar/lib/CalendarService.ts, line 256:
<comment>Bug: The fallback branch of this ternary is dead code. When `externalCalendarId` is falsy (else branch), the `.find()` compares `cal.externalId === externalCalendarId` where `externalCalendarId` is already known to be falsy — so it will never match a valid calendar. `selectedCalendar` will always be `undefined` in the fallback case, which may cause the Google Calendar API call to fail or target the wrong calendar.
Consider using a similar approach to `createEvent`, which uses `credentialId` for the lookup, or fall back to the first destination calendar's `externalId`.</comment>
<file context>
@@ -244,7 +253,7 @@ export default class GoogleCalendarService implements Calendar {
const selectedCalendar = externalCalendarId
? externalCalendarId
- : event.destinationCalendar?.externalId;
+ : event.destinationCalendar?.find((cal) => cal.externalId === externalCalendarId)?.externalId;
calendar.events.update(
</file context>
| (credential) => credential.type === reference?.type | ||
| ); | ||
| for (const credential of credentials) { | ||
| result.push(updateEvent(credential, event, bookingRefUid, calenderExternalId)); |
Contributor
There was a problem hiding this comment.
P1: updateEvent is called with a potentially undefined credential. If the credential is not found in the in-memory list AND the DB lookup fails (no record or no app.slug), credential remains undefined. Add a guard like the one used in createAllCalendarEvents: if (credential) { result.push(...) }.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/EventManager.ts, line 548:
<comment>`updateEvent` is called with a potentially `undefined` credential. If the credential is not found in the in-memory list AND the DB lookup fails (no record or no `app.slug`), `credential` remains `undefined`. Add a guard like the one used in `createAllCalendarEvents`: `if (credential) { result.push(...) }`.</comment>
<file context>
@@ -468,33 +493,62 @@ export default class EventManager {
+ (credential) => credential.type === reference?.type
+ );
+ for (const credential of credentials) {
+ result.push(updateEvent(credential, event, bookingRefUid, calenderExternalId));
+ }
}
</file context>
| if (evt.location === MeetLocationType && evt.destinationCalendar?.integration !== "google_calendar") { | ||
| // @NOTE: destinationCalendar it's an array now so as a fallback we will only check the first one | ||
| const [mainHostDestinationCalendar] = evt.destinationCalendar ?? []; | ||
| if (evt.location === MeetLocationType && mainHostDestinationCalendar.integration !== "google_calendar") { |
Contributor
There was a problem hiding this comment.
P1: Accessing .integration on mainHostDestinationCalendar will throw a TypeError when evt.destinationCalendar is nullish or empty (destructuring yields undefined). Add an optional chaining guard or check that mainHostDestinationCalendar is defined before accessing its properties.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/EventManager.ts, line 119:
<comment>Accessing `.integration` on `mainHostDestinationCalendar` will throw a `TypeError` when `evt.destinationCalendar` is nullish or empty (destructuring yields `undefined`). Add an optional chaining guard or check that `mainHostDestinationCalendar` is defined before accessing its properties.</comment>
<file context>
@@ -114,7 +114,9 @@ export default class EventManager {
- if (evt.location === MeetLocationType && evt.destinationCalendar?.integration !== "google_calendar") {
+ // @NOTE: destinationCalendar it's an array now so as a fallback we will only check the first one
+ const [mainHostDestinationCalendar] = evt.destinationCalendar ?? [];
+ if (evt.location === MeetLocationType && mainHostDestinationCalendar.integration !== "google_calendar") {
evt["location"] = "integrations:daily";
}
</file context>
Suggested change
| if (evt.location === MeetLocationType && mainHostDestinationCalendar.integration !== "google_calendar") { | |
| if (evt.location === MeetLocationType && mainHostDestinationCalendar?.integration !== "google_calendar") { |
| select: { | ||
| ...userSelect.select, | ||
| credentials: true, // Don't leak to client | ||
| credentials: true, |
Contributor
There was a problem hiding this comment.
P1: The Prisma select for dynamic users no longer includes organization: { select: { slug: true } }, which was present in the old code. Downstream code (and the IsFixedAwareUser type) expects organization.slug to be present. This will cause a runtime TypeError for dynamic bookings when the organization field is accessed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/features/bookings/lib/handleNewBooking.ts, line 737:
<comment>The Prisma `select` for dynamic users no longer includes `organization: { select: { slug: true } }`, which was present in the old code. Downstream code (and the `IsFixedAwareUser` type) expects `organization.slug` to be present. This will cause a runtime TypeError for dynamic bookings when the organization field is accessed.</comment>
<file context>
@@ -722,31 +721,46 @@ async function handler(
select: {
...userSelect.select,
- credentials: true, // Don't leak to client
+ credentials: true,
metadata: true,
- organization: {
</file context>
Suggested change
| credentials: true, | |
| credentials: true, | |
| organization: { | |
| select: { | |
| slug: true, | |
| }, | |
| }, |
| if (error instanceof HttpError || error instanceof Prisma.PrismaClientKnownRequestError) { | ||
| throw new HttpError({ statusCode: 400, message: error.message }); | ||
| } | ||
| throw new HttpError({ statusCode: 500, message: "Unable to load users" }); |
Contributor
There was a problem hiding this comment.
P1: The catch block swallows the specific validation Error messages thrown inside loadUsers (e.g., "dynamicUserList is not properly defined or empty." and "eventType.hosts is not properly defined."). Since these are plain Error instances—not HttpError or PrismaClientKnownRequestError—they fall through to the generic throw new HttpError({ statusCode: 500, message: "Unable to load users" }). This loses the descriptive error message and returns a 500 instead of a 400.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/features/bookings/lib/handleNewBooking.ts, line 761:
<comment>The catch block swallows the specific validation `Error` messages thrown inside `loadUsers` (e.g., `"dynamicUserList is not properly defined or empty."` and `"eventType.hosts is not properly defined."`). Since these are plain `Error` instances—not `HttpError` or `PrismaClientKnownRequestError`—they fall through to the generic `throw new HttpError({ statusCode: 500, message: "Unable to load users" })`. This loses the descriptive error message and returns a 500 instead of a 400.</comment>
<file context>
@@ -722,31 +721,46 @@ async function handler(
+ if (error instanceof HttpError || error instanceof Prisma.PrismaClientKnownRequestError) {
+ throw new HttpError({ statusCode: 400, message: error.message });
+ }
+ throw new HttpError({ statusCode: 500, message: "Unable to load users" });
+ }
+ };
</file context>
| .map(async (credential) => { | ||
| const calendar = await getCalendar(credential); | ||
| for (const updBooking of updatedBookings) { | ||
| const bookingRef = updBooking.references.find((ref) => ref.type.includes("_calendar")); |
Contributor
There was a problem hiding this comment.
P2: Inconsistency: updBooking.references.find() only gets the first calendar reference, but the PR's intent is to handle multiple calendar references.
The outer code was changed from .find() to .filter() to support multiple destination calendars. However, this inner lookup within the recurring event path still uses .find(), so it will only delete the first calendar reference per updated booking, missing other hosts' calendar events.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/features/bookings/lib/handleCancelBooking.ts, line 454:
<comment>Inconsistency: `updBooking.references.find()` only gets the first calendar reference, but the PR's intent is to handle multiple calendar references.
The outer code was changed from `.find()` to `.filter()` to support multiple destination calendars. However, this inner lookup within the recurring event path still uses `.find()`, so it will only delete the first calendar reference per updated booking, missing other hosts' calendar events.</comment>
<file context>
@@ -411,58 +415,71 @@ async function handler(req: CustomRequest) {
+ .map(async (credential) => {
+ const calendar = await getCalendar(credential);
+ for (const updBooking of updatedBookings) {
+ const bookingRef = updBooking.references.find((ref) => ref.type.includes("_calendar"));
+ if (bookingRef) {
+ const { uid, externalCalendarId } = bookingRef;
</file context>
| .catch(async (error: { code: number; calError: string }) => { | ||
| success = false; | ||
| /** | ||
| * There is a time when selectedCalendar externalId doesn't match witch certain credential |
Contributor
There was a problem hiding this comment.
P3: Typo in comment: "witch" should be "with" ("doesn't match with certain credential").
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/CalendarManager.ts, line 240:
<comment>Typo in comment: "witch" should be "with" ("doesn't match **with** certain credential").</comment>
<file context>
@@ -226,29 +227,31 @@ export const createEvent = async (
+ .catch(async (error: { code: number; calError: string }) => {
+ success = false;
+ /**
+ * There is a time when selectedCalendar externalId doesn't match witch certain credential
+ * so google returns 404.
+ * */
</file context>
Suggested change
| * There is a time when selectedCalendar externalId doesn't match witch certain credential | |
| * There is a time when selectedCalendar externalId doesn't match with certain credential |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Test 4nn
Summary by CodeRabbit
Release Notes
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.
nn---n*Replicated from [ai-code-review-evaluation/cal.com-coderabbit#4](https://github.com/ai-code-review-evaluation/cal.com-coderabbit/pull/4)*