packages/features/calendar-cache-sql/CalendarSubscriptionService.ts (2)

62-66: Avoid class-name shadowing; alias the dynamically imported class.

This destructuring shadows the local class name, hurting readability and IDE navigation. Alias the imported class.

-      const { CalendarSubscriptionService } = await import(
+      const { CalendarSubscriptionService: GoogleCalendarSubscriptionService } = await import(
         "@calcom/app-store/googlecalendar/lib/CalendarSubscriptionService"
       );
-      const subscriptionService = new CalendarSubscriptionService(credentialWithEmail);
+      const subscriptionService = new GoogleCalendarSubscriptionService(credentialWithEmail);

---

85-89: Repeat of shadowing issue in unwatchCalendar; alias here as well.

Same problem as above; please alias the imported class.

-      const { CalendarSubscriptionService } = await import(
+      const { CalendarSubscriptionService: GoogleCalendarSubscriptionService } = await import(
         "@calcom/app-store/googlecalendar/lib/CalendarSubscriptionService"
       );
-      const subscriptionService = new CalendarSubscriptionService(credentialWithEmail);
+      const subscriptionService = new GoogleCalendarSubscriptionService(credentialWithEmail);

packages/features/calendar-cache-sql/CalendarEventRepository.ts (1)

125-132: Consider logging per-event failure inside the transaction for easier debugging

If one upsert fails, Promise.all aborts the transaction without context. Wrapping the per-item call with try/catch and logging the failing externalEventId will help triage.

-    return await this.prismaClient.$transaction(async (tx) => {
-      const operations = events.map((event) => this.upsertEvent(event, subscriptionId, tx));
-      await Promise.all(operations);
-    });
+    return await this.prismaClient.$transaction(async (tx) => {
+      const operations = events.map(async (event) => {
+        try {
+          await this.upsertEvent(event, subscriptionId, tx);
+        } catch (error) {
+          // Best-effort identifier (avoid accessing nested relation)
+          const id = (event as any)?.externalEventId;
+          console.error(`Failed to upsert event ${id ?? "<unknown>"} for subscription ${subscriptionId}`, error);
+          throw error;
+        }
+      });
+      await Promise.all(operations);
+    });

packages/features/calendar-cache-sql/CalendarCacheService.ts (1)

127-142: Timezone-aware availability path addresses prior feedback

The with-timezones variant returns {start, end, timeZone, title, source} as requested earlier in review threads.

packages/features/calendar-cache-sql/CalendarSubscriptionService.ts (6)

15-21: De-duplicate GoogleChannelProps; import the type from the app-store module.

The type already exists in the Google app-store service. Importing it avoids drift and keeps a single source of truth.

+import type { GoogleChannelProps } from "@calcom/app-store/googlecalendar/lib/CalendarSubscriptionService";
@@
-export type GoogleChannelProps = {
-  kind?: string | null;
-  id?: string | null;
-  resourceId?: string | null;
-  resourceUri?: string | null;
-  expiration?: string | null;
-};

Also applies to: 1-1

---

23-31: Rename the orchestrator service to avoid name collisions with the app-store class.

There are two CalendarSubscriptionService classes in the codebase (this orchestrator and the Google app-store one). Renaming improves clarity and avoids confusion/import collisions. Also update the logger prefix.

-const log = logger.getSubLogger({ prefix: ["CalendarSubscriptionService"] });
+const log = logger.getSubLogger({ prefix: ["SqlCalendarSubscriptionService"] });
 
-export class CalendarSubscriptionService {
+export class SqlCalendarSubscriptionService {

Follow-up: rename file and update imports/usages in cron route/tests if applicable. I can generate a codemod or PR-wide diff on request.

---

53-56: Tighten the return type: this method should not return undefined on success.

Errors are thrown; callers shouldn’t handle undefined.

-  ): Promise<GoogleChannelProps | undefined> {
+  ): Promise<GoogleChannelProps> {

---

98-105: Improve missing-dependency error message to pinpoint what’s absent.

This makes troubleshooting DI issues faster.

-    if (!subscriptionRepo || !selectedCalendarRepo) {
-      throw new Error("Missing repositories in CalendarSubscriptionService.deps");
-    }
+    if (!subscriptionRepo || !selectedCalendarRepo) {
+      const missing = [
+        !subscriptionRepo ? "subscriptionRepo" : null,
+        !selectedCalendarRepo ? "selectedCalendarRepo" : null,
+      ]
+        .filter(Boolean)
+        .join(", ");
+      throw new Error(`Missing repositories in CalendarSubscriptionService.deps: ${missing}`);
+    }

---

112-123: createdCount may be misleading; consider renaming or deriving from repo result.

upsertManyBySelectedCalendarId can noop existing rows. Counting selectedCalendars length as createdCount suggests new rows were created. Consider renaming to upsertedCount or use a return value that tells how many were actually inserted.

---

53-72: Longer-term: decouple from the app-store service to make this orchestrator standalone.

Per the earlier discussion, this class still depends on the app-store Google subscription service. Consider inlining the minimal googleapis flow (events.watch/channels.stop) here to:

• remove cross-package coupling,
• localize webhook URL decisions,
• simplify testing and ownership.

I can draft a minimal, typed googleapis client wrapper for watch/unwatch aligned with your credential shapes.

Also applies to: 74-96

packages/prisma/schema.prisma (1)

1597-1627: Avoid redundant index on selectedCalendarId (unique already creates one)

You declare selectedCalendarId as unique and also create an explicit index on the same field. The extra index is redundant and wastes storage/maintenance.

Apply this diff:

 model CalendarSubscription {
@@
   @@index([channelExpiration, syncErrors, backoffUntil])
   @@index([syncCursor])
-  @@index([selectedCalendarId])
 }

packages/features/calendar-cache-sql/CalendarEventRepository.ts (1)

20-27: Remove any and validate externalEventId before upsert

The key is read via (data as any).externalEventId. This loses type-safety and risks undefined lookups. Extract and validate the id from scalarFields instead.

-    const { calendarSubscription: _ignored, ...scalarFields } = data;
+    const { calendarSubscription: _ignored, ...scalarFields } = data;
+    const { externalEventId } = scalarFields as { externalEventId?: string };
+    if (!externalEventId) {
+      throw new Error("externalEventId is required for calendar event upsert");
+    }
@@
-          externalEventId: (data as any).externalEventId,
+          externalEventId,

packages/features/calendar-cache-sql/CalendarCacheSqlService.ts (3)

39-46: Avoid logging entire subscription objects

console.info("Got subscription", subscription) may log user and credential metadata. Prefer structured logging with key identifiers only.

Example:

• subscriptionId
• selectedCalendarId
• selectedCalendar.externalId
• userId

---

202-208: Minimize payload when fetching a fresh sync token

You can avoid returning any items by adding maxResults: 1; this still returns nextSyncToken.

-    const resp = await calendar.events.list({ calendarId, singleEvents: true });
+    const resp = await calendar.events.list({ calendarId, singleEvents: true, maxResults: 1 });

---

313-341: Deduplicate calendar lookups before querying to reduce OR explosion

If calendars contain duplicates, the OR clause can grow unnecessarily. Build a distinct set of pairs.

-    const calendarLookups = calendars.flatMap((cal) =>
-      (cal.calendars || []).map((calendar) => ({
-        externalId: calendar.externalId,
-        credentialId: cal.credentialId,
-      }))
-    );
+    const pairSet = new Set<string>();
+    const calendarLookups = calendars.flatMap((cal) =>
+      (cal.calendars || []).reduce((acc, calendar) => {
+        const key = `${calendar.externalId}_${cal.credentialId}`;
+        if (!pairSet.has(key)) {
+          pairSet.add(key);
+          acc.push({ externalId: calendar.externalId, credentialId: cal.credentialId });
+        }
+        return acc;
+      }, [] as Array<{ externalId: string; credentialId: number }>)
+    );

packages/features/calendar-cache-sql/CalendarSubscriptionRepository.ts (2)

61-69: Use select to avoid returning all fields

This read returns the full entity; project only the fields you need (e.g., id) to keep payloads small and consistent with the “always use select” guideline.

-  async findByCredentialId(credentialId: number) {
-    return await this.prismaClient.calendarSubscription.findFirst({
+  async findByCredentialId(credentialId: number) {
+    return await this.prismaClient.calendarSubscription.findFirst({
       where: {
         selectedCalendar: {
           credentialId,
         },
       },
-    });
+      select: {
+        id: true,
+        selectedCalendarId: true,
+        updatedAt: true,
+      },
+    });
   }

---

211-235: Progressive backoff logic is good; consider capturing the error message for observability

You already cap exponential backoff and respect maxSyncErrors. Optionally store the latest error message if you add an error field later; for now, app-level logging should capture it.

packages/features/calendar-cache-sql/CalendarCacheSqlService.test.ts (7)

12-26: Reset mocks between tests to avoid cross-test leakage

Add a reset call in beforeEach so module/function mocks don’t bleed across tests.

 beforeEach(() => {
+    vi.resetAllMocks();
+    vi.clearAllMocks();
     mockSubscriptionRepo = {
       findBySelectedCalendar: vi.fn(),
       findByChannelId: vi.fn(),
       findByCredentialId: vi.fn(),
       findBySelectedCalendarIds: vi.fn(),
       upsert: vi.fn(),
       upsertMany: vi.fn(),
       updateSyncToken: vi.fn(),
       updateWatchDetails: vi.fn(),
       getSubscriptionsToWatch: vi.fn(),
       setWatchError: vi.fn(),
       clearWatchError: vi.fn(),
     };

---

170-189: Consider asserting exact payload structure to ensure no PII sneaks in via extra fields

The current assertions use objectContaining, which won’t fail if PII fields are accidentally included. Strengthen the check to ensure only the intended fields are persisted.

Example follow-up assertions to add after your current expect:

const [eventsArg] = vi.mocked(mockEventRepo.bulkUpsertEvents).mock.calls[0];
const cancelled = eventsArg.find((e: any) => e.externalEventId === "cancelled-event-id");
const active = eventsArg.find((e: any) => e.externalEventId === "active-event-id");

// Ensure no participants or other PII-ish structures slip in
expect(cancelled).not.toHaveProperty("attendees");
expect(cancelled).not.toHaveProperty("creator");
expect(cancelled).not.toHaveProperty("organizer");
expect(active).not.toHaveProperty("attendees");
expect(active).not.toHaveProperty("creator");
expect(active).not.toHaveProperty("organizer");

---

264-277: PII filtering test should also assert absence of participant-related fields

You confirm summary/description/location are nulled, but if attendees/organizer/creator are accidentally persisted, this test still passes. Add explicit negative assertions.

Example additions:

const [eventsArg] = vi.mocked(mockEventRepo.bulkUpsertEvents).mock.calls[0];
const external = eventsArg.find((e: any) => e.externalEventId === "external-event-id");

expect(external).not.toHaveProperty("attendees");
expect(external).not.toHaveProperty("creator");
expect(external).not.toHaveProperty("organizer");

---

357-369: Strengthen the participant data test: ensure participants aren’t persisted at all

Using objectContaining doesn’t preclude extra fields from being present. Add negative assertions to guarantee no participants/organizer/creator fields are saved.

Example additions:

const [eventsArg] = vi.mocked(mockEventRepo.bulkUpsertEvents).mock.calls[0];
const withParticipants = eventsArg.find((e: any) => e.externalEventId === "event-with-participants");

expect(withParticipants).not.toHaveProperty("attendees");
expect(withParticipants).not.toHaveProperty("creator");
expect(withParticipants).not.toHaveProperty("organizer");

---

109-189: Add coverage for stale sync token (410) fallback path

Service logic (per PR summary) falls back to a full time-ranged sync on 410. Add a test where events.list first rejects with a 410-like error, then a subsequent full sync succeeds, and verify updateSyncToken is called with a fresh token.

Happy to draft this additional test with mock sequencing for the Google API list calls.

---

109-189: Add pagination test for incremental fetch (nextPageToken)

Cover a case where events are split across multiple pages to validate that bulkUpsertEvents receives the combined list and sync token is updated once.

I can provide a test snippet that chains multiple list responses with nextPageToken.

---

159-163: Isolate dynamic import mocks with vi.mock and resetModules
Switch from vi.doMock to a combination of vi.resetModules() and vi.mock() to ensure each dynamic import of CalendarService is intercepted cleanly and not served from cache. For example, update at lines 159–163 (and similarly at 253–257, 347–349):

- // Mock the import
- vi.doMock("@calcom/app-store/googlecalendar/lib/CalendarService", () => ({
-   default: vi.fn().mockImplementation(() => mockGoogleCalendarService),
- }));
+ // Ensure no cached modules, then mock
+ vi.resetModules();
+ vi.mock("@calcom/app-store/googlecalendar/lib/CalendarService", () => ({
+   default: vi.fn().mockImplementation(() => mockGoogleCalendarService),
+ }));

Please confirm that Vitest (v^2.1.1) in your setup supports local-scoped vi.mock with dynamic imports.
If not, consider one of:

• A top-level vi.mock() with a mutable factory you adjust per test
• vi.importActual() + vi.unmock() patterns

packages/features/calendar-cache-sql/README.md (5)

346-351: Clarify manual tmole installation guidance

Explicitly reference the tap for clarity and consistency with the install step.

-# Start tmole manually
-tmole 3000
+# Start tmole manually (installed via tunnelmole/tap)
+tmole 3000

---

439-452: Document X-Goog-Channel-Token requirement and webhook token validation

The webhook route validates GOOGLE_WEBHOOK_TOKEN per PR summary. Add the token header to the doc for completeness.

 **Headers:**
 - `X-Goog-Channel-ID`: Google webhook channel ID
 - `X-Goog-Resource-ID`: Google resource ID
+- `X-Goog-Channel-Token`: Verification token (must match GOOGLE_WEBHOOK_TOKEN)

---

241-253: Small usage snippet enhancement: show feature flag check and fallback

You already show this below, but adding one line here helps prevent copy/paste without guardrails.

-import { CalendarCacheService } from "@calcom/features/calendar-cache-sql/CalendarCacheService";
+import { CalendarCacheService } from "@calcom/features/calendar-cache-sql/CalendarCacheService";
+// Ensure "calendar-cache-sql-read" is enabled for the user's team before using this service.

---

516-531: Minor wording tweaks for bullets (optional)

Grammar flags are cosmetic; content is clear. Feel free to ignore, but tightening phrasing reduces lint noise in docs.

Examples:

• “Batch queries” → “Indexes for batch queries”
• “Transaction-based operations for data consistency” → “Transactions for data consistency”
• “Configurable cleanup intervals via cron jobs” → “Cleanup intervals configured via cron jobs”

---

283-287: Clarify that the cron only acts when write flag is enabled

You mention this elsewhere; repeating it here reduces operational mistakes.

-# The cron job at /api/cron/calendar-subscriptions will automatically
-# set up webhooks for users with the calendar-cache-sql-write feature enabled
+# The cron job at /api/cron/calendar-subscriptions will automatically
+# set up webhooks for users with the calendar-cache-sql-write feature enabled.
+# If the feature is disabled globally or for the user's team, the cron is a no-op.

Also applies to: 317-323

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: CR
PR: calcom/cal.com#0
File: .cursor/rules/review.mdc:0-0
Timestamp: 2025-07-28T11:50:23.946Z
Learning: Applies to **/*.ts : For Prisma queries, only select data you need; never use `include`, always use `select`

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: Udit-takkar
PR: calcom/cal.com#22919
File: packages/lib/server/repository/PrismaPhoneNumberRepository.ts:412-417
Timestamp: 2025-08-07T18:42:34.081Z
Learning: In Cal.com codebase, the coding guideline requiring explicit `select` clauses instead of `include` for Prisma queries applies to read operations but not to update operations. Update operations don't need explicit select clauses.

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: CR
PR: calcom/cal.com#0
File: .cursor/rules/review.mdc:0-0
Timestamp: 2025-07-28T11:50:23.946Z
Learning: Applies to **/*Service.ts : Service files must include `Service` suffix, use PascalCase matching exported class, and avoid generic names (e.g., `MembershipService.ts`)

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/prisma/migrations/20250819013055_add_calendar_subscription_system/migration.sql (4)

54-55: Consider query patterns: add standalone index on backoffUntil

The composite index (channelExpiration, syncErrors, backoffUntil) won’t help queries that filter primarily on backoffUntil (the leftmost column rule applies). If your scheduler picks “all subs where backoffUntil <= now” regardless of channelExpiration, you’ll get a seq scan.

Add:

+-- CreateIndex
+CREATE INDEX "CalendarSubscription_backoffUntil_idx" ON "CalendarSubscription"("backoffUntil");

---

13-15: Add CHECK constraints to keep error counters sane

Defensive constraints prevent accidental negatives and make invariants explicit.

You can add:

+ALTER TABLE "CalendarSubscription"
+  ADD CONSTRAINT "CalendarSubscription_syncErrors_nonneg_chk" CHECK ("syncErrors" >= 0),
+  ADD CONSTRAINT "CalendarSubscription_maxSyncErrors_nonneg_chk" CHECK ("maxSyncErrors" >= 0);

---

69-70: Reassess global start/end/status index; consider a partial index

A global index on (start, end, status) may be low-selectivity and large. If your common scans target confirmed events, a partial index can reduce bloat and improve performance.

Keep existing indexes for now, but consider adding:

+-- CreateIndex (Postgres partial index example)
+CREATE INDEX "CalendarEvent_start_end_confirmed_idx"
+  ON "CalendarEvent"("start", "end")
+  WHERE "status" = 'confirmed';

You can then measure usage and decide whether to drop "CalendarEvent_start_end_status_idx".

---

40-41: Optional: add recurrence-friendly index

If you query instances by series, an index helps. You already have (calendarSubscriptionId, iCalUID). If queries use recurringEventId or pair iCalUID+originalStartTime, add one accordingly.

For series lookups by recurringEventId:

+-- CreateIndex
+CREATE INDEX "CalendarEvent_calendarSubscriptionId_recurringEventId_idx"
+  ON "CalendarEvent"("calendarSubscriptionId", "recurringEventId");

Also applies to: 62-67

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/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/features/calendar-cache-sql/CalendarCacheService.ts (5)

92-100: Early return when no valid selected calendar IDs

If selectedCalendars is non-empty but none have an id, selectedCalendarIds becomes empty and the repo call might inadvertently query broadly. Short-circuit to avoid unnecessary/expensive queries.

     const selectedCalendarIds = CalendarCachePresenter.presentCalendarIds(selectedCalendars);
 
+    if (selectedCalendarIds.length === 0) {
+      log.debug("No valid selectedCalendar IDs, returning empty results");
+      return [];
+    }
+
     const subscriptions = await this.subscriptionRepo.findBySelectedCalendarIds(selectedCalendarIds);

---

83-109: Validate and reuse parsed dates to avoid invalid ranges and double parsing

Currently new Date() is called twice and invalid inputs aren’t handled. Validate once, bail early on invalid ranges, and reuse the parsed dates.

   private async loadEventsForSelectedCalendars(
     dateFrom: string,
     dateTo: string,
     selectedCalendars: IntegrationCalendar[]
   ): Promise<CalendarEventForAvailability[]> {
     if (selectedCalendars.length === 0) {
       return [];
     }
 
+    const from = new Date(dateFrom);
+    const to = new Date(dateTo);
+    if (isNaN(from.getTime()) || isNaN(to.getTime())) {
+      log.warn(
+        "Invalid date range provided to loadEventsForSelectedCalendars",
+        safeStringify({ dateFrom, dateTo })
+      );
+      return [];
+    }
+
     const selectedCalendarIds = CalendarCachePresenter.presentCalendarIds(selectedCalendars);
 
     const subscriptions = await this.subscriptionRepo.findBySelectedCalendarIds(selectedCalendarIds);
     const subscriptionIds = CalendarCachePresenter.presentSubscriptionIds(subscriptions);
 
     if (subscriptionIds.length === 0) {
       log.debug("No subscriptions found, returning empty results");
       return [];
     }
 
     const allEvents = await this.eventRepo.getEventsForAvailabilityBatch(
       subscriptionIds,
-      new Date(dateFrom),
-      new Date(dateTo)
+      from,
+      to
     );
 
     return allEvents;
   }

---

118-119: Trim debug logs to avoid logging potentially large objects

Logging full selectedCalendars can be verbose. Log counts and identifiers only.

-    log.debug("Getting availability from cache", safeStringify({ dateFrom, dateTo, selectedCalendars }));
+    log.debug(
+      "Getting availability from cache",
+      safeStringify({ dateFrom, dateTo, selectedCount: selectedCalendars.length })
+    );

-    log.debug(
-      "Getting availability with timezones from cache",
-      safeStringify({ dateFrom, dateTo, selectedCalendars })
-    );
+    log.debug(
+      "Getting availability with timezones from cache",
+      safeStringify({ dateFrom, dateTo, selectedCount: selectedCalendars.length })
+    );

Also applies to: 136-139

---

43-50: Type guard for calendar IDs

If IntegrationCalendar["id"] can be string | undefined | null in some call sites, prefer a type-guard to ensure string[] at compile-time.

-  static presentCalendarIds(selectedCalendars: IntegrationCalendar[]): string[] {
-    return selectedCalendars.reduce<string[]>((acc, sc) => {
-      if (sc.id !== undefined) {
-        acc.push(sc.id);
-      }
-      return acc;
-    }, []);
-  }
+  static presentCalendarIds(selectedCalendars: IntegrationCalendar[]): string[] {
+    return selectedCalendars
+      .map((sc) => sc.id)
+      .filter((id): id is string => typeof id === "string" && id.length > 0);
+  }

Please confirm IntegrationCalendar.id is always string. If it can be number, we should adjust the return type and consumers accordingly.

---

20-20: Minor: extract source string into a constant

Avoid string duplication and typos by centralizing the source value.

-const log = logger.getSubLogger({ prefix: ["CalendarCacheService"] });
+const log = logger.getSubLogger({ prefix: ["CalendarCacheService"] });
+const SOURCE = "calendar-cache-sql";

Then use SOURCE in presenters:

-      source: "calendar-cache-sql",
+      source: SOURCE,

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: 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.

packages/features/calendar-cache-sql/CalendarSubscriptionRepository.ts (1)

162-171: Type mismatch: channelExpiration should be Date (matches schema/interface)

The interface and Prisma model use DateTime. Using string here risks invalid values and breaks comparisons.

Apply:

   async updateWatchDetails(
     id: string,
     details: {
       channelId: string;
       channelKind?: string;
       channelResourceId?: string;
       channelResourceUri?: string;
-      channelExpiration: string;
+      channelExpiration: Date;
     }
   ) {

packages/features/calendar-cache-sql/CalendarCacheService.ts (2)

31-38: Mask event titles to avoid PII leakage in availability responses

Do not expose event.summary in public/free-busy outputs. Always return “Busy”.

Apply:

   static presentAvailabilityData(events: CalendarEventForAvailability[]): EventBusyDate[] {
     return events.map((event) => ({
       start: event.start.toISOString(),
       end: event.end.toISOString(),
       source: "calendar-cache-sql",
-      title: event.summary || "Busy",
+      title: "Busy",
     }));
   }

---

59-69: Also mask titles in timezone variant

Same rationale as above; avoid leaking summaries.

Apply:

   static presentAvailabilityDataWithTimezones(
     events: CalendarEventForAvailability[]
   ): Ensure<EventBusyDate, "timeZone">[] {
     return events.map((event) => ({
       start: event.start.toISOString(),
       end: event.end.toISOString(),
       timeZone: event.timeZone || "",
       source: "calendar-cache-sql",
-      title: event.summary || "Busy",
+      title: "Busy",
     }));
   }

packages/features/calendar-cache-sql/CalendarSubscriptionRepository.ts (3)

61-69: Use explicit select to comply with Prisma “select-only” guideline

This read currently returns all columns. Add a select projection consistent with other methods to keep the contract tight.

Apply:

   async findByCredentialId(credentialId: number) {
     return await this.prismaClient.calendarSubscription.findFirst({
       where: {
         selectedCalendar: {
           credentialId,
         },
       },
+      select: {
+        id: true,
+        selectedCalendarId: true,
+        channelId: true,
+        channelKind: true,
+        channelResourceId: true,
+        channelResourceUri: true,
+        channelResource: true,
+        clientState: true,
+        channelExpiration: true,
+        syncCursor: true,
+        syncErrors: true,
+        maxSyncErrors: true,
+        backoffUntil: true,
+        createdAt: true,
+        updatedAt: true,
+      },
     });
   }

---

136-150: Batch upsert: consider chunking to avoid large concurrent bursts

Using Promise.all is fine for small/medium sets. If this ever handles hundreds/thousands of IDs, chunk requests to keep DB and connection pool healthy.

I can provide a chunking helper if you expect large batches.

---

241-265: Optional: Use the error parameter for observability and progressive backoff context

The _error arg is ignored. Consider logging it at debug/warn level to aid troubleshooting, without storing sensitive payloads.

-  async setWatchError(id: string, _error: string) {
+  async setWatchError(id: string, error: string) {
     await this.prismaClient.$transaction(async (tx) => {
+      // Optionally log error context (avoid logging tokens/PII)
+      // log.warn({ id, error }, "Set watch error");

packages/features/calendar-cache-sql/CalendarCacheService.ts (1)

118-121: Reduce PII in logs: avoid logging full selectedCalendars payload

Logging the entire selectedCalendars can include emails and other PII. Log counts and IDs instead.

Apply:

-    log.debug("Getting availability from cache", safeStringify({ dateFrom, dateTo, selectedCalendars }));
+    const selectedCalendarIds = CalendarCachePresenter.presentCalendarIds(selectedCalendars);
+    log.debug(
+      "Getting availability from cache",
+      safeStringify({ dateFrom, dateTo, selectedCalendarCount: selectedCalendars.length, selectedCalendarIds })
+    );

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: 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: Udit-takkar
PR: calcom/cal.com#22919
File: packages/features/calAIPhone/interfaces/AIPhoneService.interface.ts:118-143
Timestamp: 2025-08-08T09:29:11.681Z
Learning: In calcom/cal.com PR #22919, packages/features/calAIPhone/interfaces/AIPhoneService.interface.ts (TypeScript), the AIPhoneServiceAgentListItem is required to include user.email in listAgents responses (per maintainer Udit-takkar). Future reviews should not flag this as unnecessary PII unless requirements change.

Learnt from: Udit-takkar
PR: calcom/cal.com#22919
File: packages/features/calAIPhone/providers/retellAI/services/PhoneNumberService.ts:212-220
Timestamp: 2025-08-08T10:26:13.362Z
Learning: In calcom/cal.com PR #22919, packages/features/calAIPhone/providers/retellAI/services/PhoneNumberService.ts should include the phone number in client-facing HttpError messages (e.g., in updatePhoneNumber/getPhoneNumber catch blocks). Do not suggest redacting the phone number from these errors unless requirements change (per maintainer: Udit-takkar).

Learnt from: CR
PR: calcom/cal.com#0
File: .cursor/rules/review.mdc:0-0
Timestamp: 2025-07-28T11:50:23.946Z
Learning: Applies to **/*.ts : For Prisma queries, only select data you need; never use `include`, always use `select`

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: Udit-takkar
PR: calcom/cal.com#22919
File: packages/lib/server/repository/PrismaPhoneNumberRepository.ts:412-417
Timestamp: 2025-08-07T18:42:34.081Z
Learning: In Cal.com codebase, the coding guideline requiring explicit `select` clauses instead of `include` for Prisma queries applies to read operations but not to update operations. Update operations don't need explicit select clauses.

packages/features/calendar-cache-sql/CalendarSubscriptionRepository.ts (1)

181-239: Fix invalid field-to-field comparison in Prisma filter; filter in application layer and add deterministic ordering.

this.prismaClient.calendarSubscription.fields.maxSyncErrors is not a valid Prisma construct. Prisma does not support column-to-column comparisons in where. Fetch a superset, filter syncErrors < maxSyncErrors in memory, then slice to the limit. Also add an orderBy for determinism.

Apply:

   async getSubscriptionsToWatch(limit = 100) {
     const oneDayInMS = 24 * 60 * 60 * 1000;
     const tomorrow = new Date(Date.now() + oneDayInMS);

-    return await this.prismaClient.calendarSubscription.findMany({
-      take: limit,
+    const rows = await this.prismaClient.calendarSubscription.findMany({
+      // Over-fetch to compensate for post-filtering (syncErrors < maxSyncErrors)
+      take: limit * 3,
       where: {
         selectedCalendar: {
           user: {
             teams: {
               some: {
                 team: {
                   features: {
                     some: {
                       featureId: CALENDAR_CACHE_SQL_WRITE_FEATURE,
                     },
                   },
                 },
               },
             },
           },
           integration: "google_calendar",
         },
-        syncErrors: {
-          lt: this.prismaClient.calendarSubscription.fields.maxSyncErrors,
-        },
         OR: [{ channelExpiration: null }, { channelExpiration: { lt: tomorrow } }],
         AND: [{ OR: [{ backoffUntil: null }, { backoffUntil: { lte: new Date() } }] }],
       },
+      orderBy: [{ channelExpiration: "asc" }, { updatedAt: "asc" }],
       select: {
         id: true,
         selectedCalendarId: true,
         channelId: true,
         channelKind: true,
         channelResourceId: true,
         channelResourceUri: true,
         channelResource: true,
         clientState: true,
         channelExpiration: true,
         syncCursor: true,
         syncErrors: true,
         maxSyncErrors: true,
         backoffUntil: true,
         createdAt: true,
         updatedAt: true,
         selectedCalendar: {
           select: {
             id: true,
             externalId: true,
             integration: true,
             userId: true,
             credential: {
               select: safeCredentialSelectForCalendarCache,
             },
           },
         },
       },
     });
+    return rows.filter((s) => s.syncErrors < s.maxSyncErrors).slice(0, limit);
   }

packages/features/calendar-cache-sql/CalendarSubscriptionRepository.ts (5)

71-84: Early-return on empty array to skip unnecessary DB call.

Minor optimization: if selectedCalendarIds is empty, return [] immediately.

Apply:

 async findBySelectedCalendarIds(selectedCalendarIds: string[]) {
-  return await this.prismaClient.calendarSubscription.findMany({
+  if (selectedCalendarIds.length === 0) return [];
+  return await this.prismaClient.calendarSubscription.findMany({
     where: {
       selectedCalendarId: {
         in: selectedCalendarIds,
       },
     },
     select: {
       id: true,
       selectedCalendarId: true,
       updatedAt: true,
     },
   });
 }

---

136-150: Avoid unbounded Promise.all inside a transaction; chunk for scalability.

Large arrays can create many concurrent operations within a single transaction. Chunking reduces DB pressure and memory spikes.

Apply:

 async upsertManyBySelectedCalendarId(selectedCalendarIds: string[]) {
   // Use transactional callback for better compatibility with Prismock tests
   return await this.prismaClient.$transaction(async (tx) => {
-    const results = await Promise.all(
-      selectedCalendarIds.map((selectedCalendarId) =>
-        tx.calendarSubscription.upsert({
-          where: { selectedCalendarId },
-          create: { selectedCalendar: { connect: { id: selectedCalendarId } } },
-          update: { updatedAt: new Date() },
-        })
-      )
-    );
-    return results;
+    const results: Prisma.CalendarSubscription[] = [];
+    const BATCH_SIZE = 50;
+    for (let i = 0; i < selectedCalendarIds.length; i += BATCH_SIZE) {
+      const slice = selectedCalendarIds.slice(i, i + BATCH_SIZE);
+      const batch = await Promise.all(
+        slice.map((selectedCalendarId) =>
+          tx.calendarSubscription.upsert({
+            where: { selectedCalendarId },
+            create: { selectedCalendar: { connect: { id: selectedCalendarId } } },
+            update: { updatedAt: new Date() },
+          })
+        )
+      );
+      results.push(...batch);
+    }
+    return results;
   });
 }

---

152-160: Nit: Method name vs field name mismatch (token vs cursor).

updateSyncToken writes syncCursor. Consider renaming to updateSyncCursor (or documenting the mapping) to avoid confusion.

---

241-265: setWatchError: Progressive backoff is good; consider using the error param or documenting why it’s ignored.

If the schema has a place to store the last error (message/timestamp), consider persisting it. Otherwise, add a short comment indicating the error string is handled upstream to avoid confusion.

Where is the error parameter consumed or logged today (service layer? monitoring)? If not used, should we remove it from the interface or store it for troubleshooting?

---

61-69: Consider narrowing selected fields since only existence is checked

In the only call site (getCalendar.ts), the returned subscription is used solely to test for existence (truthiness), not to access any of its properties. You can reduce query payload and improve clarity by:

• Adding a select: { id: true } (or minimal required fields) to findByCredentialId
• Alternatively, renaming it to something like existsByCredentialId and returning a boolean

If you ever need full subscription data downstream, update this method’s select accordingly.

/// packages/features/calendar-cache-sql/CalendarSubscriptionRepository.ts
 async findByCredentialId(credentialId: number) {
-  return await this.prismaClient.calendarSubscription.findFirst({
-    where: { selectedCalendar: { credentialId } },
-  });
+  return await this.prismaClient.calendarSubscription.findFirst({
+    where: { selectedCalendar: { credentialId } },
+    select: { id: true }, // only fetch the primary key since we only check existence
+  });
 }

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: CR
PR: calcom/cal.com#0
File: .cursor/rules/review.mdc:0-0
Timestamp: 2025-07-28T11:50:23.946Z
Learning: Applies to **/*.ts : For Prisma queries, only select data you need; never use `include`, always use `select`

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: Udit-takkar
PR: calcom/cal.com#22919
File: packages/lib/server/repository/PrismaPhoneNumberRepository.ts:412-417
Timestamp: 2025-08-07T18:42:34.081Z
Learning: In Cal.com codebase, the coding guideline requiring explicit `select` clauses instead of `include` for Prisma queries applies to read operations but not to update operations. Update operations don't need explicit select clauses.