packages/app-store/office365calendar/lib/CalendarService.ts (1)

348-390: Add input validation for date range and calendar IDs.

The method should validate inputs before processing to prevent unnecessary API calls and potential errors.

Add validation at the beginning of the method:

 async fetchAvailability(
   dateFrom: string,
   dateTo: string,
   calendarIds: string[],
   getCreatedDateTime = false
 ): Promise<EventBusyDate[]> {
+  if (!calendarIds || calendarIds.length === 0) {
+    return [];
+  }
+  
   const dateFromParsed = new Date(dateFrom);
   const dateToParsed = new Date(dateTo);
+  
+  if (isNaN(dateFromParsed.getTime()) || isNaN(dateToParsed.getTime())) {
+    throw new Error("Invalid date format provided");
+  }
+  
+  if (dateFromParsed >= dateToParsed) {
+    throw new Error("dateFrom must be before dateTo");
+  }

packages/app-store/office365calendar/lib/CalendarService.ts (2)

697-699: LGTM! Well-documented subscription scoping.

The implementation correctly scopes subscriptions to specific calendars using /calendars/{calendar_id}/events to reduce notification volume, as documented in the Microsoft Graph API guidelines.

---

700-700: LGTM! Using recommended TTL for calendar subscriptions.

The 3-day TTL aligns with Microsoft's recommended duration for calendar event subscriptions as per the documentation.

packages/app-store/office365calendar/lib/CalendarService.ts (3)

405-436: Consider implementing cache warming strategy.

The cache miss path fetches data but doesn't automatically populate the cache. Consider warming the cache after a miss to benefit subsequent requests.

Consider updating the cache after a miss:

 this.log.debug("[Cache Miss] Fetching availability", { dateFrom, dateTo, calendarIds });
 const data = await this.fetchAvailability(dateFrom, dateTo, calendarIds);
+
+// Optionally warm the cache for future requests
+await this.setAvailabilityInCache(cacheArgs, data).catch((err) => {
+  this.log.warn("Failed to warm cache after miss", err);
+});
+
 return data;

---

712-726: Add error handling for failed subscription deletions.

While the method logs warnings for failed deletions, consider tracking which deletions failed and potentially retrying or escalating critical failures.

Consider tracking failed deletions:

 private async stopWatchingCalendarsInMicrosoft(subscriptions: { subscriptionId: string | null }[]) {
   const uniqueSubscriptions = subscriptions.filter(
     (s, i, arr) => s.subscriptionId && arr.findIndex((x) => x.subscriptionId === s.subscriptionId) === i
   );

-  await Promise.allSettled(
+  const results = await Promise.allSettled(
     uniqueSubscriptions.map(({ subscriptionId }) =>
       subscriptionId
         ? this.fetcher(`/subscriptions/${subscriptionId}`, { method: "DELETE" }).catch((err) => {
             this.log.warn(`Failed to delete subscription ${subscriptionId}`, err);
+            throw err; // Re-throw to capture in allSettled
           })
         : Promise.resolve()
     )
   );
+  
+  const failedDeletions = results.filter(r => r.status === 'rejected');
+  if (failedDeletions.length > 0) {
+    this.log.error(`Failed to delete ${failedDeletions.length} subscriptions`);
+  }
 }

---

728-756: Consider batching cache operations for performance.

The method processes calendars per event type sequentially. For better performance with many event types, consider batching the cache operations.

Consider parallel processing with batching:

-for (const [_eventTypeId, selectedCalendars] of Array.from(selectedCalendarsPerEventType.entries())) {
-  const parsedArgs = {
-    timeMin: getTimeMin(),
-    timeMax: getTimeMax(),
-    items: selectedCalendars.map((sc) => ({ id: sc.externalId })),
-  };
-  const data = await this.fetchAvailability(
-    parsedArgs.timeMin,
-    parsedArgs.timeMax,
-    parsedArgs.items.map((i) => i.id)
-  );
-  await this.setAvailabilityInCache(parsedArgs, data);
-}
+const cacheOperations = Array.from(selectedCalendarsPerEventType.entries()).map(
+  async ([_eventTypeId, selectedCalendars]) => {
+    const parsedArgs = {
+      timeMin: getTimeMin(),
+      timeMax: getTimeMax(),
+      items: selectedCalendars.map((sc) => ({ id: sc.externalId })),
+    };
+    const data = await this.fetchAvailability(
+      parsedArgs.timeMin,
+      parsedArgs.timeMax,
+      parsedArgs.items.map((i) => i.id)
+    );
+    return this.setAvailabilityInCache(parsedArgs, data);
+  }
+);
+
+await Promise.all(cacheOperations);

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/app-store/office365calendar/api/webhook.ts:120-123
Timestamp: 2025-07-18T17:57:16.395Z
Learning: The office365calendar webhook handler in packages/app-store/office365calendar/api/webhook.ts is specifically designed for Office365 calendar integration, not as a generic webhook handler. Therefore, it's safe to assume that fetchAvailabilityAndSetCache method will be implemented in the Office365CalendarService, making explicit validation checks unnecessary.

Learnt from: din-prajapati
PR: calcom/cal.com#21854
File: packages/app-store/office365calendar/__tests__/unit_tests/SubscriptionManager.test.ts:0-0
Timestamp: 2025-08-05T12:04:29.037Z
Learning: In packages/app-store/office365calendar/lib/CalendarService.ts, the fetcher method in Office365CalendarService class is public, not private. It was specifically changed from private to public in this PR to support proper testing and external access patterns.

Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/app-store/office365calendar/api/webhook.ts:120-123
Timestamp: 2025-07-18T17:57:16.395Z
Learning: The office365calendar webhook handler in packages/app-store/office365calendar/api/webhook.ts is specifically designed for Office365 calendar integration, not as a generic webhook handler. Therefore, it's safe to assume that fetchAvailabilityAndSetCache method will be implemented in the Office365CalendarService, making explicit validation checks unnecessary.

Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/prisma/schema.prisma:891-891
Timestamp: 2025-07-18T08:47:01.264Z
Learning: The Outlook Calendar integration in Cal.com intentionally reuses subscription IDs across multiple event types for efficiency. The `upsertSelectedCalendarsForEventTypeIds` method creates separate SelectedCalendar records for each eventTypeId, all sharing the same outlookSubscriptionId. This subscription sharing pattern means that unique constraints like `@unique([outlookSubscriptionId, eventTypeId])` should not be applied as they would prevent this intended functionality.

Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/prisma/schema.prisma:891-891
Timestamp: 2025-07-18T08:47:01.264Z
Learning: In Cal.com's calendar integration, both Google Calendar and Outlook Calendar are designed to allow multiple eventTypeIds to share the same subscription ID (googleChannelId or outlookSubscriptionId). This is an intentional design pattern to reuse existing subscriptions for efficiency rather than creating separate subscriptions for each event type. Therefore, unique constraints like `@unique([outlookSubscriptionId, eventTypeId])` should not be added as they would break this subscription sharing functionality.

packages/app-store/office365calendar/lib/CalendarService.ts (4)

177-185: LGTM! Non-delegated user handling is correct.

The implementation properly extracts userPrincipalName from the /me endpoint for non-delegated scenarios and includes appropriate error handling for missing values. This aligns with the past review comment addressing non-delegatedTo scenarios.

---

208-236: LGTM! Delegated user credential handling is well-implemented.

The implementation correctly:

• Uses UPN instead of GUID as noted in past comments
• Handles OAuth client alias regex pattern removal from email
• Provides proper error handling for missing users and UserPrincipalName
• Uses appropriate Microsoft Graph API filtering

The approach follows Microsoft's recommended practices for delegated scenarios.

---

566-569: Consider returning a structured error response for consistency.

When MICROSOFT_WEBHOOK_TOKEN is not set, the method returns undefined. For better error handling consistency, consider returning a structured error response.

Apply this diff to improve error handling consistency:

 if (!process.env.MICROSOFT_WEBHOOK_TOKEN) {
   this.log.warn("MICROSOFT_WEBHOOK_TOKEN is not set, skipping watching calendar");
-  return;
+  return { error: "MICROSOFT_WEBHOOK_TOKEN is not configured" };
 }

---

692-710: LGTM! Microsoft Graph subscription creation is properly implemented.

The private method correctly:

• Uses appropriate change types (created, updated, deleted)
• Sets proper webhook URL and client state for validation
• Respects the recommended TTL for calendar subscriptions (3 days)
• Returns structured subscription data

The implementation follows Microsoft Graph API best practices as noted in past comments.

packages/app-store/office365calendar/lib/CalendarService.ts (1)

2-2: Consider the necessity of the Prisma type import.

The Prisma import is only used for the Prisma.SelectedCalendarUncheckedCreateInput type in the upsertSelectedCalendarsForEventTypeIds method. If this is the sole usage, consider whether a more specific import or inline type definition would be more appropriate.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/app-store/office365calendar/api/webhook.ts:120-123
Timestamp: 2025-07-18T17:57:16.395Z
Learning: The office365calendar webhook handler in packages/app-store/office365calendar/api/webhook.ts is specifically designed for Office365 calendar integration, not as a generic webhook handler. Therefore, it's safe to assume that fetchAvailabilityAndSetCache method will be implemented in the Office365CalendarService, making explicit validation checks unnecessary.

Learnt from: din-prajapati
PR: calcom/cal.com#21854
File: packages/app-store/office365calendar/__tests__/unit_tests/SubscriptionManager.test.ts:0-0
Timestamp: 2025-08-05T12:04:29.037Z
Learning: In packages/app-store/office365calendar/lib/CalendarService.ts, the fetcher method in Office365CalendarService class is public, not private. It was specifically changed from private to public in this PR to support proper testing and external access patterns.

Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/app-store/office365calendar/api/webhook.ts:120-123
Timestamp: 2025-07-18T17:57:16.395Z
Learning: The office365calendar webhook handler in packages/app-store/office365calendar/api/webhook.ts is specifically designed for Office365 calendar integration, not as a generic webhook handler. Therefore, it's safe to assume that fetchAvailabilityAndSetCache method will be implemented in the Office365CalendarService, making explicit validation checks unnecessary.

Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/prisma/schema.prisma:891-891
Timestamp: 2025-07-18T08:47:01.264Z
Learning: The Outlook Calendar integration in Cal.com intentionally reuses subscription IDs across multiple event types for efficiency. The `upsertSelectedCalendarsForEventTypeIds` method creates separate SelectedCalendar records for each eventTypeId, all sharing the same outlookSubscriptionId. This subscription sharing pattern means that unique constraints like `@unique([outlookSubscriptionId, eventTypeId])` should not be applied as they would prevent this intended functionality.

Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/prisma/schema.prisma:891-891
Timestamp: 2025-07-18T08:47:01.264Z
Learning: In Cal.com's calendar integration, both Google Calendar and Outlook Calendar are designed to allow multiple eventTypeIds to share the same subscription ID (googleChannelId or outlookSubscriptionId). This is an intentional design pattern to reuse existing subscriptions for efficiency rather than creating separate subscriptions for each event type. Therefore, unique constraints like `@unique([outlookSubscriptionId, eventTypeId])` should not be added as they would break this subscription sharing functionality.