Skip to content

fix: handle collective multiple host on destinationCalendar#8

Open
zaibkhan wants to merge 1 commit into
enhance-collective-scheduling-foundationfrom
fix/handle-collective-multiple-host-destinations
Open

fix: handle collective multiple host on destinationCalendar#8
zaibkhan wants to merge 1 commit into
enhance-collective-scheduling-foundationfrom
fix/handle-collective-multiple-host-destinations

Conversation

@zaibkhan

Copy link
Copy Markdown

No description provided.

@codoki-ai codoki-ai deleted a comment from codoki-pr-intelligence Bot Sep 10, 2025
@codoki-ai codoki-ai deleted a comment from codoki-pr-intelligence Bot Sep 10, 2025
@codoki-ai codoki-ai deleted a comment from codoki-pr-intelligence Bot Sep 10, 2025
@codoki-ai codoki-ai deleted a comment from codoki-pr-intelligence Bot Sep 10, 2025
@codoki-pr-intelligence

codoki-pr-intelligence Bot commented Sep 10, 2025

Copy link
Copy Markdown

Codoki PR Review

Summary: Support multiple destination calendars, update types
What’s good: Type updates align with the PR goal: destinationCalendar now supports multiple hosts, and schedulingType and externalId add clarity for downstream consumers.
Review Status: ❌ Requires changes
Overall Priority: High
Review Update:
• Coverage: Reviewed all 22 files across 3 batches

Issues (Critical & High only)

Severity Issue Why it matters
High Correctness — Null access on mainHostDestinationCalendar.integraton cau... …/core/EventManager.ts
mainHostDestinationCalendar can be undefined when there is no destinationCalendar; accessing mainHostDestinationCalendar.integration below will throw at runtime, causing booking creation to fail. Guard this access before comparing against 'google_calendar'.
High Correctness — Missing calendarId fallback in Google updateEvent leads t... …/lib/CalendarService.ts
When externalCalendarId is null/undefined, this expression resolves to undefined (find() compares against null/undefined), causing calendarId to be undefined in the update call and leading to Google API errors. Provide a safe fallback to the main host's calendar or 'primary'.

Showing top 2 issues. Critical: 0, High: 2. See inline suggestions for more.

Key Feedback (click to expand)
  • Needs improvement: Changing Calendar.createEvent to require credentialId and making destinationCalendar an array are breaking changes; consider a backward-compatible path (e.g., optional parameter, overloads, or transitional typedefs) and ensure all implementations are updated.
  • Testing: Add tests covering: (1) createEvent with multiple destination calendars to ensure all hosts receive events, (2) regression where a single destination calendar is passed (ensure array handling or validation), and (3) createEvent callers that omit credentialId (if any legacy paths exist) to fail fast or auto-infer correctly.
  • Documentation: Document the migration: destinationCalendar is now an array and Calendar.createEvent now requires credentialId; include examples and a short guide for updating integrators (Google/Lark/etc.).
  • Compatibility: These type changes are potentially breaking for all Calendar implementations and callsites. Provide an interim compatibility layer (optional param, overload, or wrapper) or a codemod guide and a clear version note.
  • Performance: No clear performance impact from the type changes; if createAllCalendarEvents fans out to multiple destination calendars, ensure batching/rate-limiting in implementations to avoid provider throttling.
  • Security: No new security surface evident in the provided diffs. Verify that any expanded fan-out to multiple calendars preserves existing permission checks per credential.
  • Open questions: Should credentialId live on CalendarEvent (to simplify callsites) or remain a separate param? Do we guarantee all CalendarService implementations now accept destinationCalendar[] and not a single object?

Confidence: 3/5 — Needs work before merge (2 high · status: Requires changes · scope: large PR (22 files))

React with 👍 or 👎 if you found this review useful.

// Fallback to Cal Video if Google Meet is selected w/o a Google Cal
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 ?? [];

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ High: mainHostDestinationCalendar can be undefined when there is no destinationCalendar; accessing mainHostDestinationCalendar.integration below will throw at runtime, causing booking creation to fail. Guard this access before comparing against 'google_calendar'.

Suggested change
const [mainHostDestinationCalendar] = evt.destinationCalendar ?? [];
if (evt.location === MeetLocationType && (mainHostDestinationCalendar?.integration !== "google_calendar")) {
evt["location"] = "integrations:daily";
}

@@ -244,7 +253,7 @@ export default class GoogleCalendarService implements Calendar {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ High: When externalCalendarId is null/undefined, this expression resolves to undefined (find() compares against null/undefined), causing calendarId to be undefined in the update call and leading to Google API errors. Provide a safe fallback to the main host's calendar or 'primary'.

Suggested change
const [mainHostDestinationCalendar] = event.destinationCalendar ?? [];
const selectedCalendar = externalCalendarId || mainHostDestinationCalendar?.externalId || "primary";

calendarReference = newBooking?.references.length
? newBooking.references.find((reference) => reference.type.includes("_calendar"))
: booking.references.find((reference) => reference.type.includes("_calendar"));
? newBooking.references.filter((reference) => reference.type.includes("_calendar"))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔷 Medium: updateAllCalendarEvents builds an array of Promises via result.push(updateEvent(...)) but does not clearly await them before returning; this can lead to unhandled promise behavior and incomplete updates. Ensure the function returns Promise.all(result) to resolve all update operations.

attendees: attendeesList,
uid: booking.uid,
recurringEvent: parseRecurringEvent(booking.eventType?.recurringEvent),
destinationCalendar: booking.destinationCalendar || user.destinationCalendar,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔷 Medium: Changing the empty case to [] alters truthiness (an empty array is truthy), which can cause downstream checks like if (evt.destinationCalendar) to behave as if a calendar is present. To avoid subtle bugs across consumers, return null when there is no destination calendar or ensure all consumers check .length > 0.

Suggested change
destinationCalendar: booking.destinationCalendar || user.destinationCalendar,
destinationCalendar: selectedDestinationCalendar ? [selectedDestinationCalendar] : null,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants