Cal.com / PR #10

feat: add calendar cache status and actions

calcom/cal.com#22532
Golden recall: 50% Extra findings: 2

Golden Comments (1/2 found)

# Comment Severity Status FriendlyReviewer Detail
#1 updateManyByCredentialId with empty data {} — Prisma's @updatedAt does not trigger on updateMany (unlike singular update) → timestamp never updated HIGH ✓ Found FriendlyReviewer found this bug with precise diagnosis: "Prisma's updateMany does NOT automatically set @updatedAt. Passing an empty data object {} results in a no-op. Explicitly pass { updatedAt: new Date() }" (CalendarService.ts L1022). Classified as HIGH severity.
#2 macOS-specific sed -i '' syntax — empty string after -i flag fails on Linux systems LOW ✗ Missed FriendlyReviewer supports `.sh` files but did not catch this particular bug in the shell script.

Supplementary Findings (8 findings)

Additional issues found by FriendlyReviewer that are not part of the golden (reference) set.

Finding File Severity Legitimate?
deleteCache handler uses prisma.calendarCache.deleteMany directly instead of repository → bypasses feature flag mock layer deleteCache.handler.ts:33 HIGH ✓ Architectural concern
getCacheStatusByCredentialIds does not filter by expiresAt → expired entries reported as valid calendar-cache.repository.ts:172 MEDIUM ✓ Functional bug
No guard for credentialIds.length > 0 → unnecessary DB query when array is empty connectedCalendars.handler.ts:24 LOW ✓ Minor performance
|| null instead of ?? null (nullish coalescing) connectedCalendars.handler.ts:35 LOW ✓ Style consistency
No distinction between 403 (Forbidden) and 404 (Not Found) error responses; no server-side invalidation after cache deletion deleteCache.handler.ts:17 LOW ✓ Error handling gap
CalendarsRouterHandlerCache does not type deleteCache → TypeScript won't catch handler signature mismatches _router.tsx:7 LOW ✓ Type safety gap
SessionUser has any[] for calendar arrays → no type safety on calendar data user.ts:915 LOW ✓ Type safety
googleChannelId in session → unnecessarily increases payload for all users user.ts:896 LOW ✓ Payload concern

Honest Analysis

FriendlyReviewer found 1 of 2 golden bugs (50% recall). Golden #1 (Prisma updateMany with empty data {}) was precisely diagnosed with an excellent explanation of why @updatedAt does not fire with updateMany. This is the kind of Prisma-specific gotcha that most developers would discover only after deploying to production. Golden #2 (macOS sed syntax) was missed — FR supports .sh files, but this particular bug in scripts/test-gcal-webhooks.sh went undetected.

The 9 supplementary findings are heavily skewed toward LOW severity (6 of 9). The architectural finding about bypassing CalendarCacheRepository (deleteCache.handler.ts L33) is genuinely important: the handler uses prisma.calendarCache.deleteMany directly instead of the repository, breaking the feature flag mock layer. The getCacheStatus missing expiresAt filter is a functional bug. But the remaining 6 LOW findings (|| vs ??, missing 403 vs 404 distinction, missing type in router cache, SessionUser any[], googleChannelId in session) are code quality observations that belong in a style guide.

FriendlyReviewer's full review

As posted on the PR — the complete output from FriendlyReviewer.

friendly-reviewer bot commented now

Hi there 👋

Cloud Tech
2 high, 1 medium, 6 low

This merge request adds calendar cache status and actions. While the feature is architecturally sound, the implementation introduces several issues that should be addressed before merging. (1) The deleteCache handler calls prisma.calendarCache.deleteMany directly instead of using CalendarCacheRepository, bypassing the feature flag mock layer and making the endpoint untestable in isolation. (2) getCacheStatusByCredentialIds lacks an expiresAt filter, so expired cache entries are reported as valid. (3) updateManyByCredentialId passes an empty data object to Prisma, and unlike singular update, @updatedAt does not fire with updateMany, leaving the timestamp perpetually null. (4) Missing nullish coalescing consistency with || null instead of ?? null. (5) No guard on credentialIds.length causes unnecessary database queries on empty arrays. (6) CalendarsRouterHandlerCache does not type deleteCache, losing compile-time safety. (7) SessionUser uses any[] for calendar arrays. (8) googleChannelId in the session payload increases cookie size unnecessarily for all users.

These might need a close look

  • 🔴 packages/trpc/server/routers/viewer/deleteCache.handler.ts (L33)
    CRITICAL: deleteCache uses prisma.calendarCache.deleteMany directly instead of the CalendarCacheRepository. This bypasses the feature flag mock layer, making it impossible to test or feature-gate this endpoint. Inject the repository and call its method instead.
  • 🔴 packages/core/CalendarService.ts (L1022)
    BUG: updateManyByCredentialId passes an empty data object {} to Prisma’s updateMany. Unlike the singular update, @updatedAt does NOT automatically trigger with updateMany — the updatedAt timestamp will never be set. Add { updatedAt: new Date() } to the data object.

Worth checking

  • 🟡 packages/trpc/server/routers/viewer/calendar-cache.repository.ts (L172)
    getCacheStatusByCredentialIds does not filter by expiresAt, so expired cache entries are reported as valid. Add a where clause for expiresAt > new Date() to exclude stale entries from the status report.

Small things (take or leave)

  • 🔵 packages/trpc/server/routers/viewer/connectedCalendars.handler.ts (L24)
    No guard for credentialIds.length > 0 — if the array is empty, the database query executes unnecessarily. Add an early return.
  • 🔵 packages/trpc/server/routers/viewer/connectedCalendars.handler.ts (L35)
    Uses || null instead of ?? null (nullish coalescing). Functionally the same here but ?? is more idiomatic and avoids treating empty strings or 0 as falsy.
  • 🔵 packages/trpc/server/routers/viewer/deleteCache.handler.ts (L17)
    No distinction between 403 (Forbidden) and 404 (Not Found) error responses. Also missing server-side cache invalidation — deleting from the database but not clearing any in-memory or CDN caches.
  • 🔵 packages/trpc/server/routers/viewer/_router.tsx (L7)
    CalendarsRouterHandlerCache does not include a type entry for deleteCache. TypeScript will not catch signature mismatches in the handler.
  • 🔵 packages/trpc/server/routers/viewer/user.ts (L915)
    SessionUser uses any[] for calendar array fields, losing all type safety. Define proper types for selectedCalendars and calendarCredentials.
  • 🔵 packages/trpc/server/routers/viewer/user.ts (L896)
    googleChannelId is included in the session payload, unnecessarily increasing session cookie size for every request, even for non-Google Calendar users.