packages/app-store/googlecalendar/lib/__tests__/CalendarService.test.ts (1)

783-785: Improve error handling with proper Error objects

Use proper Error objects instead of throwing strings, and remove console.log.

     } catch (error) {
-      console.log({ error });
-      throw "Looks like cache was not used";
+      throw new Error(`Cache was not used: ${error instanceof Error ? error.message : String(error)}`);
     }

packages/app-store/googlecalendar/lib/__tests__/google-calendar-sync-tokens.integration-test.ts (3)

10-13: Simplify date handling for clarity

The current approach creates a UTC date and then converts it to ISO string, which could be confusing. Consider using a fixed date string for better test predictability.

-const TEST_DATE = new Date(
-  Date.UTC(new Date().getUTCFullYear(), new Date().getUTCMonth(), new Date().getUTCDate() + 1)
-);
-const TEST_DATE_ISO = TEST_DATE.toISOString();
+// Use a fixed date for consistent test behavior
+const TEST_DATE_ISO = "2025-01-15T00:00:00.000Z";
+const TEST_DATE = new Date(TEST_DATE_ISO);

---

140-143: Avoid resetting test variables that could affect parallel test execution

Resetting test variables to specific values in cleanup could cause issues if tests run in parallel. The variables are already scoped to each test through beforeEach, so this reset is unnecessary.

-      // Reset test variables
-      testUserId = 0;
-      testCredentialId = 0;
-      testUniqueId = "";

---

203-205: Use direct mocking instead of spy with mockImplementation

Using vi.spyOn with mockImplementation is redundant. Since you're replacing the implementation entirely, use vi.mocked or mock the method directly.

-      vi.spyOn(calendarService, "fetchAvailabilityAndSetCacheIncremental").mockImplementation(
-        mockImplementation
-      );
+      calendarService.fetchAvailabilityAndSetCacheIncremental = mockImplementation;

packages/app-store/googlecalendar/lib/__tests__/CalendarService.test.ts (3)

764-764: Remove console.log statement from test

Debug console.log statements should be removed from test code.

-    console.log({ calendarCachesAfter });

---

1800-1816: Simplify test date range generation

The current date manipulation is complex and depends on the current date, which could make tests non-deterministic. Consider using fixed dates for more predictable tests.

   const getTestDateRange = () => {
-    const now = new Date();
-    // Use current month boundaries for consistency
-    const timeMin = getTimeMin(now.toISOString());
-    const timeMax = getTimeMax(now.toISOString());
-
-    // Use specific dates within the current month for event testing
-    const testStart = new Date(now.getFullYear(), now.getMonth(), 15, 10, 0, 0);
-    const testEnd = new Date(now.getFullYear(), now.getMonth(), 15, 23, 59, 59);
+    // Use fixed dates for deterministic tests
+    const timeMin = "2025-01-01T00:00:00.000Z";
+    const timeMax = "2025-01-31T23:59:59.999Z";
+    const testStart = new Date("2025-01-15T10:00:00.000Z");
+    const testEnd = new Date("2025-01-15T23:59:59.000Z");

     return {
       timeMin,
       timeMax,
       testDateFrom: testStart.toISOString(),
       testDateTo: testEnd.toISOString(),
     };
   };

---

2527-2693: Consider splitting this large test into smaller, focused tests

This test is quite large and tests multiple behaviors (sibling refresh, optimization, multi-calendar query). Consider splitting it into separate tests for better maintainability and easier debugging.

Consider splitting into:

1. test("refreshes sibling calendars when processing webhook")
2. test("skips sibling calendars with fresh cache on subsequent webhooks")
3. test("multi-calendar queries merge individual calendar caches")

This would make each test more focused and easier to understand when failures occur.

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

525-544: Add documentation and consider performance optimization.

The method correctly filters events but lacks documentation about intentionally skipping all-day events, which could be confusing for future maintainers.

+ /**
+  * Converts Google Calendar events to busy time intervals.
+  * Note: All-day events are intentionally excluded as they don't block specific time slots.
+  */
  private convertEventsToBusyTimes(events: calendar_v3.Schema$Event[]): EventBusyDate[] {
-   const busyTimes: EventBusyDate[] = [];
-
-   for (const event of events) {
-     if (event.status === "cancelled" || !event.start || !event.end) {
-       continue;
-     }
-
-     if (!event.start.dateTime || !event.end.dateTime) {
-       continue;
-     }
-
-     busyTimes.push({
-       start: event.start.dateTime,
-       end: event.end.dateTime,
-     });
-   }
-
-   return busyTimes;
+   return events
+     .filter((event) => 
+       event.status !== "cancelled" && 
+       event.start?.dateTime && 
+       event.end?.dateTime
+     )
+     .map((event) => ({
+       start: event.start.dateTime!,
+       end: event.end.dateTime!,
+     }));
  }

---

728-761: Consider extracting individual cache merging logic for better maintainability.

The individual cache merging logic is a good optimization but makes the method complex. Consider extracting it to a separate method for better readability and testability.

      // If multi-calendar cache miss and we have multiple calendars, try individual cache entries
      if (calendarIds.length > 1) {
-       const individualCacheEntries: EventBusyDate[] = [];
-       let allIndividualCacheHits = true;
-
-       for (const calendarId of calendarIds) {
-         const individualCached = await calendarCache.getCachedAvailability({
-           credentialId: this.credential.id,
-           userId: this.credential.userId,
-           args: {
-             timeMin: getTimeMin(timeMin),
-             timeMax: getTimeMax(timeMax),
-             items: [{ id: calendarId }],
-           },
-         });
-
-         if (individualCached) {
-           const freeBusyResult = individualCached.value as unknown as calendar_v3.Schema$FreeBusyResponse;
-           const busyTimes = this.convertFreeBusyToEventBusyDates(freeBusyResult);
-           individualCacheEntries.push(...busyTimes);
-         } else {
-           allIndividualCacheHits = false;
-           break;
-         }
-       }
-
-       if (allIndividualCacheHits) {
-         this.log.debug(
-           "[Cache Hit] Merged individual calendar cache entries for multi-calendar query",
-           safeStringify({ timeMin, timeMax, calendarIds })
-         );
-         return individualCacheEntries;
-       }
+       return await this.tryMergeIndividualCacheEntries(calendarCache, timeMin, timeMax, calendarIds);
      }

      return null;

Add the extracted method:

private async tryMergeIndividualCacheEntries(
  calendarCache: CalendarCache,
  timeMin: string,
  timeMax: string,
  calendarIds: string[]
): Promise<EventBusyDate[] | null> {
  const individualCacheEntries: EventBusyDate[] = [];
  
  for (const calendarId of calendarIds) {
    try {
      const individualCached = await calendarCache.getCachedAvailability({
        credentialId: this.credential.id,
        userId: this.credential.userId,
        args: {
          timeMin: getTimeMin(timeMin),
          timeMax: getTimeMax(timeMax),
          items: [{ id: calendarId }],
        },
      });

      if (individualCached) {
        const freeBusyResult = individualCached.value as unknown as calendar_v3.Schema$FreeBusyResponse;
        const busyTimes = this.convertFreeBusyToEventBusyDates(freeBusyResult);
        individualCacheEntries.push(...busyTimes);
      } else {
        return null; // Cache miss for this calendar
      }
    } catch (error) {
      this.log.debug(`Error checking individual cache for calendar ${calendarId}`, { error });
      return null; // Error accessing cache
    }
  }

  this.log.debug(
    "[Cache Hit] Merged individual calendar cache entries for multi-calendar query",
    safeStringify({ timeMin, timeMax, calendarIds })
  );
  return individualCacheEntries;
}

---

1182-1222: Consider adding rate limiting and improving error specificity.

The method processes multiple calendars sequentially without rate limiting, which could hit API limits. Consider adding more specific error handling and rate limiting.

  async fetchAvailabilityAndSetCacheIncremental(selectedCalendars: IntegrationCalendar[]) {
    const calendarCache = await CalendarCache.init(this);
+   const MAX_CONCURRENT_CALENDARS = 3; // Prevent API rate limiting
+   
+   // Process calendars in batches to avoid rate limiting
+   for (let i = 0; i < selectedCalendars.length; i += MAX_CONCURRENT_CALENDARS) {
+     const batch = selectedCalendars.slice(i, i + MAX_CONCURRENT_CALENDARS);
+     await Promise.allSettled(batch.map(calendar => this.processSingleCalendarIncremental(calendarCache, calendar)));
+   }
+ }
+
+ private async processSingleCalendarIncremental(calendarCache: any, selectedCalendar: IntegrationCalendar) {
-   for (const selectedCalendar of selectedCalendars) {
      try {
        const cached = await calendarCache.getCachedAvailability({
          credentialId: this.credential.id,
          userId: this.credential.userId,
          args: {
            timeMin: getTimeMin(),
            timeMax: getTimeMax(),
            items: [{ id: selectedCalendar.externalId }],
          },
        });

        const existingSyncToken = (cached as any)?.nextSyncToken || undefined;

        const { events, nextSyncToken } = await this.fetchEventsIncremental(
          selectedCalendar.externalId,
          existingSyncToken
        );

        const busyTimes = this.convertEventsToBusyTimes(events);

        await this.setAvailabilityInCacheWithSyncToken(
          [{ id: selectedCalendar.externalId }],
          busyTimes,
          nextSyncToken
        );

        // 🚀 PROACTIVE SIBLING CACHE REFRESH
        await this.refreshSiblingCalendars(selectedCalendar);
      } catch (error) {
+       const err = error as GoogleCalError;
+       if (err.code === 403) {
+         log.warn("Rate limit hit, skipping incremental sync for calendar", {
+           calendarId: selectedCalendar.externalId,
+         });
+         return; // Don't fall back to full sync on rate limits
+       }
        log.error("Error in incremental sync, falling back to full sync", {
          error,
          calendarId: selectedCalendar.externalId,
        });
        await this.fetchAvailabilityAndSetCache([selectedCalendar]);
      }
-   }
  }

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