Skip to content

feat: add calendar cache status and actions (#22532)#1

Open
Frawatson wants to merge 1 commit into
calendar-cache-foundationfrom
introduce-cache-key-overflow
Open

feat: add calendar cache status and actions (#22532)#1
Frawatson wants to merge 1 commit into
calendar-cache-foundationfrom
introduce-cache-key-overflow

Conversation

@Frawatson

Copy link
Copy Markdown
Owner

Benchmark test PR ai-code-review-evaluation#11 - Replicating Greptile benchmark for Grapple PR evaluation.

* feat: add calendar cache status dropdown

- Add updatedAt field to CalendarCache schema with migration
- Create tRPC cacheStatus endpoint for fetching cache timestamps
- Add action dropdown to CalendarSwitch for Google Calendar entries
- Display formatted last updated timestamp in dropdown
- Add placeholder for cache deletion functionality
- Include translation strings for dropdown content

The dropdown only appears for Google Calendar integrations that have
active cache entries and provides cache management options for future
extensibility.

Co-Authored-By: zomars@cal.com <zomars@me.com>

* fix: resolve Prisma type incompatibilities in repository files

- Remove problematic satisfies clause in selectedCalendar.ts
- Add missing cacheStatus parameter to ConnectedCalendarList component
- Fixes type errors that were preventing CI from passing

Co-Authored-By: zomars@cal.com <zomars@me.com>

* refactor: integrate cache status into connectedCalendars handler

- Remove separate cacheStatus tRPC endpoint as requested
- Return cache status as separate field in connectedCalendars response
- Update UI components to use cache data from connectedCalendars
- Fix Prisma type incompatibilities in repository files

Co-Authored-By: zomars@cal.com <zomars@me.com>

* fix: resolve Prisma type incompatibilities and fix data flow for cache status

- Fix Prisma.SortOrder usage in membership.ts orderBy clauses
- Remove problematic satisfies clause in selectedCalendar.ts
- Fix TeamSelect type reference in team.ts
- Update SelectedCalendarsSettingsWebWrapper to properly pass cacheStatus data flow

Co-Authored-By: zomars@cal.com <zomars@me.com>

* Discard changes to packages/lib/server/repository/membership.ts

* Discard changes to packages/lib/server/repository/team.ts

* fix: improve calendar cache dropdown with proper formatting and subscription logic

- Fix timestamp HTML entity encoding with interpolation escapeValue: false
- Only show dropdown for subscribed Google calendars (googleChannelId exists)
- Hide delete option when no cache data exists
- Include updatedAt and googleChannelId fields upstream in user repository
- Update data flow to pass subscription status through components

Co-Authored-By: zomars@cal.com <zomars@me.com>

* feat: update SelectedCalendar.updatedAt when Google webhooks trigger cache refresh

- Add updateManyByCredentialId method to SelectedCalendarRepository
- Update fetchAvailabilityAndSetCache to refresh SelectedCalendar timestamps
- Ensure webhook flow updates both CalendarCache and SelectedCalendar records
- Maintain proper timestamp tracking for calendar cache operations

Co-Authored-By: zomars@cal.com <zomars@me.com>

* Add script to automate Tunnelmole webhook setup

Introduces test-gcal-webhooks.sh to start Tunnelmole, extract the public URL, and update GOOGLE_WEBHOOK_URL in the .env file. Handles process management, rate limits, and ensures environment configuration for Google Calendar webhooks.

* Update dev:cron script to use npx tsx

Replaces 'ts-node' with 'npx tsx' in the dev:cron script for running cron-tester.ts, likely to improve compatibility or leverage tsx features.

* Update cache status string and improve CalendarSwitch UI

Renamed 'last_updated' to 'cache_last_updated' in locale file for clarity and updated CalendarSwitch to use the new string. Also added dark mode text color support for cache status display.

* refactor: move cache management to credential-level dropdown with Remove App

- Create CredentialActionsDropdown component consolidating cache and app removal actions
- Add deleteCache tRPC mutation for credential-level cache deletion
- Update connectedCalendars handler to include cacheUpdatedAt at credential level
- Move dropdown from individual CalendarSwitch to credential level in SelectedCalendarsSettingsWebWrapper
- Remove cache-related props from CalendarSwitch component
- Add translation strings for cache management actions
- Consolidate all credential-level actions (cache management + Remove App) in one dropdown

Co-Authored-By: zomars@cal.com <zomars@me.com>

* fix: remove duplicate translation keys in common.json

- Remove duplicate cache-related keys at lines 51-56
- Keep properly positioned keys later in file
- Addresses GitHub comment from zomars about duplicate keys

Co-Authored-By: zomars@cal.com <zomars@me.com>

* fix: rename translation key to cache_last_updated

- Address GitHub comment from zomars
- Rename 'last_updated' to 'cache_last_updated' for specificity
- Update usage in CredentialActionsDropdown component

Co-Authored-By: zomars@cal.com <zomars@me.com>

* fix: remove duplicate last_updated translation key

Co-Authored-By: zomars@cal.com <zomars@me.com>

* fix: add confirmation dialog for cache deletion and use repository pattern

- Add confirmation dialog for destructive cache deletion action
- Replace direct Prisma calls with CalendarCacheRepository pattern
- Add getCacheStatusByCredentialIds method to repository interface
- Fix import paths for UI components
- Address GitHub review comments from zomars

Co-Authored-By: zomars@cal.com <zomars@me.com>

* Update CredentialActionsDropdown.tsx

* Update common.json

* Update common.json

* fix: remove nested div wrapper to resolve HTML structure error

- Remove wrapping div around DisconnectIntegration component
- Fixes nested <p> tag validation error preventing Remove App functionality
- Maintains existing confirmation dialog patterns

Co-Authored-By: zomars@cal.com <zomars@me.com>

* Fix API handler response termination logic

Removed unnecessary return values after setting status in the integrations API handler. This clarifies response handling and prevents returning the response object when not needed. Resolves "API handler should not return a value, received object".

* fix: 400 is correct error code for computing slot for past booking (#22574)

* fix

* add test

* chore: release v5.5.1

* Refactor credential disconnect to use confirmation dialog

Replaces the DisconnectIntegration component with an inline confirmation dialog for removing app credentials. Adds disconnect mutation logic and updates UI to improve user experience and consistency.

* Set default value for CalendarCache.updatedAt

Added a default value of NOW() for the updatedAt column in the CalendarCache table to ensure existing and future rows have a valid timestamp. Updated the Prisma schema to reflect this change and provide compatibility for legacy data and raw inserts.

---------

Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-authored-by: Benny Joo <sldisek783@gmail.com>
Co-authored-by: emrysal <me@alexvanandel.com>
@Frawatson Frawatson closed this Apr 7, 2026
@Frawatson Frawatson reopened this Apr 7, 2026

@grapple-pr grapple-pr Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🤖 Grapple PR Review

Summary

This PR adds calendar cache status visibility and management actions to the connected calendars UI, including a new dropdown component that shows when Google Calendar cache was last updated and allows users to delete cached data. It also adds an updatedAt field to the CalendarCache model and a updateManyByCredentialId method to track when cache was last refreshed per credential.

Changes: 17 files (+379 / -31 lines)

  • 🟢 4 files added: packages/features/apps/components/CredentialActionsDropdown.tsx, packages/prisma/migrations/20250715160635_add_calendar_cache_updated_at/migration.sql, packages/trpc/server/routers/viewer/calendars/deleteCache.handler.ts, scripts/test-gcal-webhooks.sh
  • 🟡 13 files modified: apps/web/package.json, apps/web/public/static/locales/en/common.json, packages/app-store/googlecalendar/lib/CalendarService.ts, packages/features/calendar-cache/calendar-cache.repository.interface.ts, packages/features/calendar-cache/calendar-cache.repository.mock.ts +8 more

CI: ⏳ 0 passed, 0 failed

Impact Diagram

graph TD
  pr_0[/"🔀 PR Changes: 17 files"/]
  f1["🟡 package.json +1/-1"]
  pr_0 --> f1
  f2["🟡 common.json +7/-0"]
  pr_0 --> f2
  f3["🟡 CalendarService.ts +3/-0"]
  pr_0 --> f3
  f4["🟢 CredentialActionsDropdown.tsx +157/-0"]
  pr_0 --> f4
  f5["🟡 calendar-cache.repository.interface.ts +3/-0"]
  pr_0 --> f5
  f6["🟡 calendar-cache.repository.mock.ts +5/-0"]
  pr_0 --> f6
  f7["🟡 calendar-cache.repository.ts +17/-0"]
  pr_0 --> f7
  f8["🟡 getConnectedDestinationCalendars.ts +8/-2"]
  pr_0 --> f8
  f9["🟡 selectedCalendar.ts +8/-2"]
  pr_0 --> f9
  f10["🟡 user.ts +2/-0"]
  pr_0 --> f10
  f11["🟡 SelectedCalendarsSettingsWebWrapper.tsx +23/-25"]
  pr_0 --> f11
  f12["🟢 migration.sql +9/-0"]
  pr_0 --> f12
  more_13["... +5 more files"]
  pr_0 --> more_13
  f5 -.- f6
  f6 -.- f7
  f9 -.- f10
  style f1 fill:#7c3aed,color:#fff,stroke:#7c3aed
  style f2 fill:#7c3aed,color:#fff,stroke:#7c3aed
  style f3 fill:#7c3aed,color:#fff,stroke:#7c3aed
  style f4 fill:#7c3aed,color:#fff,stroke:#7c3aed
  style f5 fill:#7c3aed,color:#fff,stroke:#7c3aed
  style f6 fill:#7c3aed,color:#fff,stroke:#7c3aed
  style f7 fill:#7c3aed,color:#fff,stroke:#7c3aed
  style f8 fill:#7c3aed,color:#fff,stroke:#7c3aed
  style f9 fill:#7c3aed,color:#fff,stroke:#7c3aed
  style f10 fill:#7c3aed,color:#fff,stroke:#7c3aed
  style f11 fill:#7c3aed,color:#fff,stroke:#7c3aed
  style f12 fill:#7c3aed,color:#fff,stroke:#7c3aed
Loading

3 critical issues found — please address before merging.

Severity Count
🔴 Critical 3
🟠 Major 9
🟡 Minor 9
🔵 Info 5
Review details
  • Reviewed at: 2026-04-07T18:39:17.724Z
  • Agents used: security, logic, style, architecture, performance, verification
  • Context level: full
  • Execution time: 115.8s
  • Low-confidence findings suppressed: 2
  • Blast radius: 0 (contained)

Powered by Grapple PR — AI that understands your code.

const data = await this.fetchAvailability(parsedArgs);
await this.setAvailabilityInCache(parsedArgs, data);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 CRITICAL — Intent Alignment (confidence: 100%)

The intent states 'When Google Calendar fetches availability and updates the cache, it also calls SelectedCalendarRepository.updateManyByCredentialId to update updatedAt for all selected calendars under that credential.' However, the cacheUpdatedAt displayed in the UI comes from CalendarCache.updatedAt (queried via CalendarCacheRepository.getCacheStatusByCredentialIds), not from SelectedCalendar.updatedAt. Updating SelectedCalendar.updatedAt does not affect the cache status displayed in the dropdown. The CalendarCache.updatedAt field has @updatedAt in the Prisma schema, so it should auto-update when cache rows are modified via setAvailabilityInCache. This SelectedCalendarRepository.updateManyByCredentialId call appears to update the wrong table for its stated purpose.

Evidence:

  • connectedCalendars.handler.ts queries CalendarCacheRepository.getCacheStatusByCredentialIds which reads CalendarCache.updatedAt
  • CalendarService.ts calls SelectedCalendarRepository.updateManyByCredentialId which updates SelectedCalendar table
  • These are two different tables - SelectedCalendar vs CalendarCache
  • The CalendarCache model already has updatedAt DateTime @default(now()) @updatedAt which auto-updates on Prisma writes
  • The setAvailabilityInCache call on line 1020 already writes to CalendarCache, which should trigger the @updatedAt auto-update on CalendarCache

Agent: logic

});

if (!credential) {
throw new Error("Credential not found or access denied");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 CRITICAL — API contract violation (confidence: 100%)

The handler throws a plain Error instead of a TRPCError when the credential is not found or access is denied. In tRPC, a plain Error results in an INTERNAL_SERVER_ERROR (HTTP 500) response, which leaks the wrong status code to clients and obscures the actual authorization failure. This should be a TRPCError with code FORBIDDEN or NOT_FOUND.

Evidence:

  • Line 25: throw new Error("Credential not found or access denied");
  • tRPC wraps non-TRPCError exceptions as INTERNAL_SERVER_ERROR by default
  • Other handlers in the codebase (e.g., credential deletion) use TRPCError with appropriate codes for authorization failures

Agent: architecture

const data = await this.fetchAvailability(parsedArgs);
await this.setAvailabilityInCache(parsedArgs, data);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 CRITICAL — Business Logic Correctness (confidence: 98%)

Calling updateManyByCredentialId with an empty object {} as the data payload may not trigger Prisma's @updatedAt auto-update. Prisma's @updatedAt directive updates the timestamp when a record is modified via Prisma client, but updateMany with an empty data object may be treated as a no-op or may not trigger the @updatedAt behavior since no actual fields are being changed. This means SelectedCalendar.updatedAt may never get updated, silently breaking the cache timestamp tracking feature.

Evidence:

  • The intent states 'reviewers should verify that passing {} as the update data actually updates the updatedAt field via Prisma's auto-update behavior'
  • Prisma's @updatedAt is documented to update the field 'automatically stores the time when a record was last updated' - but this requires an actual update operation with field changes
  • The updateMany Prisma method with empty data {} may generate a SQL UPDATE with no SET clauses or skip the query entirely
  • This is also flagged as a risk area: 'Prisma's @updatedAt auto-update may or may not trigger on empty update payloads; if it doesn't, the cache timestamp tracking will silently fail'
  • Note: The intent says to update SelectedCalendar.updatedAt but the feature tracks CalendarCache.updatedAt - however the code is calling SelectedCalendarRepository, not CalendarCacheRepository

Agent: logic

throw new Error("Credential not found or access denied");
}

await prisma.calendarCache.deleteMany({

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 MAJOR — Authentication/Authorization (confidence: 100%)

Authorization failure throws a plain Error instead of a TRPCError with an appropriate HTTP status code. When a credential lookup fails (not found or wrong owner), the plain Error will bubble up as an unhandled exception, causing tRPC to return a 500 Internal Server Error rather than a 401/403. This leaks the fact that an authorization check occurred via the error message and uses the wrong HTTP semantics.

Evidence:

  • Line 26: throw new Error("Credential not found or access denied") — a plain Error, not a TRPCError
  • tRPC converts unrecognized errors to INTERNAL_SERVER_ERROR (HTTP 500), masking the true authorization failure to clients but potentially logging sensitive stack traces server-side
  • The PR acceptance criteria explicitly flags: 'deleteCacheHandler throws an error (not a TRPCError) when the credential does not belong to the authenticated user — reviewers should check if a proper TRPCError with UNAUTHORIZED/FORBIDDEN code should be used instead'

Agent: security


const enrichedConnectedCalendars = connectedCalendars.map((calendar) => ({
...calendar,
cacheUpdatedAt: cacheStatusMap.get(calendar.credentialId) || null,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 MAJOR — Data Exposure (confidence: 100%)

The connectedCalendarsHandler directly instantiates CalendarCacheRepository (the real implementation) instead of using the injected/environment-aware version. In environments where calendar cache is disabled, the mock repository is expected to be used, but this bypasses that mechanism. This could cause unexpected database queries or errors in environments where the CalendarCache table is absent or disabled, and leaks implementation details about cache timestamps to clients regardless of feature flags.

Evidence:

  • Line 26: import { CalendarCacheRepository } from '@calcom/features/calendar-cache/calendar-cache.repository' — imports concrete class directly
  • Line 28: const cacheRepository = new CalendarCacheRepository() — direct instantiation bypasses DI/feature flag
  • The mock CalendarCacheRepositoryMock exists specifically for environments where cache is disabled, but this handler will never use it
  • PR risk areas note: 'The connectedCalendarsHandler instantiates CalendarCacheRepository directly rather than using dependency injection, making it harder to test and potentially bypassing the mock in environments where calendar cache is disabled'

Agent: security

Warnings:

- Added the required column `updatedAt` to the `CalendarCache` table without a default value. This is not possible if the table is not empty.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 INFO — Cryptographic issues (confidence: 89%)

The migration file contains a misleading warning comment: 'Added the required column without a default value. This is not possible if the table is not empty.' The actual migration correctly adds DEFAULT NOW(), contradicting the warning. This comment is auto-generated by Prisma and is factually incorrect given the actual SQL. If developers rely on this warning text to understand the migration's safety, they may incorrectly conclude it is unsafe to run on a non-empty table.

Evidence:

  • Lines 3-5: Warning text says 'without a default value' and 'not possible if the table is not empty'
  • Line 9: ALTER TABLE "CalendarCache" ADD COLUMN "updatedAt" TIMESTAMP(3) NOT NULL DEFAULT NOW() — correctly adds a default
  • This contradiction could confuse operators during incident response or deployment reviews
  • PR edge cases note: 'The migration comment warns... but then correctly adds DEFAULT NOW() — the Prisma migration warning text may be misleading'

Agent: security

if grep -q '^GOOGLE_WEBHOOK_URL=' "$ENV_FILE"; then
sed -i '' -E "s|^GOOGLE_WEBHOOK_URL=.*|GOOGLE_WEBHOOK_URL=$TUNNEL_URL|" "$ENV_FILE"
else
echo "GOOGLE_WEBHOOK_URL=$TUNNEL_URL" >> "$ENV_FILE"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 INFO — Platform Compatibility (confidence: 89%)

The sed -i '' -E syntax is macOS-specific. On Linux, sed -i does not take an empty string argument, and this command will fail or behave unexpectedly.

Evidence:

  • macOS sed requires sed -i '' for in-place editing without backup
  • Linux GNU sed uses sed -i (no empty string argument) for the same behavior
  • The intent notes: 'The scripts/test-gcal-webhooks.sh uses sed -i '' -E which is macOS-specific syntax and will fail on Linux'

Agent: logic


if grep -q '^GOOGLE_WEBHOOK_URL=' "$ENV_FILE"; then
sed -i '' -E "s|^GOOGLE_WEBHOOK_URL=.*|GOOGLE_WEBHOOK_URL=$TUNNEL_URL|" "$ENV_FILE"
else

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 INFO — Code Organization (confidence: 89%)

The sed command uses macOS-specific syntax (sed -i '' -E) that will fail on Linux. This dev script will not work in CI/Linux environments without modification.

Evidence:

  • Line 68: sed -i '' -E is macOS syntax (empty string for backup)
  • Linux sed requires either sed -i -E (GNU) or different quoting
  • Script will fail silently or corrupt the .env file on Linux systems
  • This affects developer experience in cross-platform teams

Agent: style

value Json
expiresAt DateTime
// Provide an initial value for legacy rows and future raw inserts
updatedAt DateTime @default(now()) @updatedAt

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 INFO — Documentation (confidence: 77%)

Comment mentions 'legacy rows' but the migration adds DEFAULT NOW() which timestamps existing rows with current time, potentially misrepresenting when they were actually cached. This could be misleading to future maintainers.

Evidence:

  • Schema comment says 'Provide an initial value for legacy rows'
  • Migration uses DEFAULT NOW() which gives all existing rows the current timestamp
  • Existing stale cache entries will appear to have been recently updated
  • This timing inconsistency should be documented

Agent: style

credentialId={cal.credentialId}
eventTypeId={shouldUseEventTypeScope ? eventTypeId : null}
delegationCredentialId={connectedCalendar.delegationCredentialId}
delegationCredentialId={connectedCalendar.delegationCredentialId || null}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 INFO — Naming Conventions (confidence: 74%)

Inconsistent handling of delegationCredentialId type coercion using || null. The prop is typed as string | undefined but coerced to string | null, which could mask cases where undefined was previously expected.

Evidence:

  • Line 98: delegationCredentialId={connectedCalendar.delegationCredentialId || null}
  • This changes semantic meaning from 'not provided' (undefined) to 'explicitly null'
  • Could indicate a type mismatch that should be addressed at the source rather than coerced in the component

Agent: style

@Frawatson Frawatson closed this Apr 7, 2026
@Frawatson Frawatson reopened this Apr 7, 2026

@grapple-pr grapple-pr Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🤖 Grapple PR Review

Summary

This PR adds calendar cache status visibility and cache management actions to the connected calendars UI, allowing users to see when their Google Calendar cache was last updated and delete cached data via a new dropdown menu that replaces the previous disconnect-only button.

Changes: 17 files (+379 / -31 lines)

  • 🟢 4 files added: packages/features/apps/components/CredentialActionsDropdown.tsx, packages/prisma/migrations/20250715160635_add_calendar_cache_updated_at/migration.sql, packages/trpc/server/routers/viewer/calendars/deleteCache.handler.ts, scripts/test-gcal-webhooks.sh
  • 🟡 13 files modified: apps/web/package.json, apps/web/public/static/locales/en/common.json, packages/app-store/googlecalendar/lib/CalendarService.ts, packages/features/calendar-cache/calendar-cache.repository.interface.ts, packages/features/calendar-cache/calendar-cache.repository.mock.ts +8 more

CI: ⏳ 0 passed, 0 failed

Impact Diagram

graph TD
  pr_0[/"🔀 PR Changes: 17 files"/]
  f1["🟡 package.json +1/-1"]
  pr_0 --> f1
  f2["🟡 common.json +7/-0"]
  pr_0 --> f2
  f3["🟡 CalendarService.ts +3/-0"]
  pr_0 --> f3
  f4["🟢 CredentialActionsDropdown.tsx +157/-0"]
  pr_0 --> f4
  f5["🟡 calendar-cache.repository.interface.ts +3/-0"]
  pr_0 --> f5
  f6["🟡 calendar-cache.repository.mock.ts +5/-0"]
  pr_0 --> f6
  f7["🟡 calendar-cache.repository.ts +17/-0"]
  pr_0 --> f7
  f8["🟡 getConnectedDestinationCalendars.ts +8/-2"]
  pr_0 --> f8
  f9["🟡 selectedCalendar.ts +8/-2"]
  pr_0 --> f9
  f10["🟡 user.ts +2/-0"]
  pr_0 --> f10
  f11["🟡 SelectedCalendarsSettingsWebWrapper.tsx +23/-25"]
  pr_0 --> f11
  f12["🟢 migration.sql +9/-0"]
  pr_0 --> f12
  more_13["... +5 more files"]
  pr_0 --> more_13
  f5 -.- f6
  f6 -.- f7
  f9 -.- f10
  style f1 fill:#7c3aed,color:#fff,stroke:#7c3aed
  style f2 fill:#7c3aed,color:#fff,stroke:#7c3aed
  style f3 fill:#7c3aed,color:#fff,stroke:#7c3aed
  style f4 fill:#7c3aed,color:#fff,stroke:#7c3aed
  style f5 fill:#7c3aed,color:#fff,stroke:#7c3aed
  style f6 fill:#7c3aed,color:#fff,stroke:#7c3aed
  style f7 fill:#7c3aed,color:#fff,stroke:#7c3aed
  style f8 fill:#7c3aed,color:#fff,stroke:#7c3aed
  style f9 fill:#7c3aed,color:#fff,stroke:#7c3aed
  style f10 fill:#7c3aed,color:#fff,stroke:#7c3aed
  style f11 fill:#7c3aed,color:#fff,stroke:#7c3aed
  style f12 fill:#7c3aed,color:#fff,stroke:#7c3aed
Loading

1 critical issue found — please address before merging.

Severity Count
🔴 Critical 1
🟠 Major 7
🟡 Minor 7
🔵 Info 4
Review details
  • Reviewed at: 2026-04-08T00:59:34.658Z
  • Agents used: security, logic, style, architecture, performance, verification
  • Context level: full
  • Execution time: 84.1s
  • Low-confidence findings suppressed: 5
  • Blast radius: 0 (contained)

Powered by Grapple PR — AI that understands your code.

const data = await this.fetchAvailability(parsedArgs);
await this.setAvailabilityInCache(parsedArgs, data);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 CRITICAL — Business Logic Correctness (confidence: 100%)

Calling updateManyByCredentialId(credentialId, {}) with an empty data object will NOT trigger Prisma's @updatedAt auto-update on SelectedCalendar. Prisma's @updatedAt directive only sets the timestamp when actual data fields are being written in an update operation. Passing an empty {} as the data argument means no fields are being modified, so Prisma either treats it as a no-op or does not generate the updatedAt assignment in the SQL. This means the cache status timestamp shown to users will never actually get updated after a fetch cycle, defeating the purpose of the feature.

Evidence:

  • The intent states: 'After a cache fetch cycle completes in GoogleCalendarService.fetchAvailabilityAndSetCache, SelectedCalendar.updatedAt is updated for all calendars under that credential via SelectedCalendarRepository.updateManyByCredentialId'
  • The risk areas explicitly call this out: 'passing an empty data object may not correctly trigger Prisma's @updatedat auto-update on SelectedCalendar, making the cache status update a no-op'
  • Prisma's updateMany with data: {} generates no SET clauses, and @updatedAt is only injected when Prisma detects an actual update operation with fields to write
  • Furthermore, the feature reads cacheUpdatedAt from CalendarCache.updatedAt (via getCacheStatusByCredentialIds), NOT from SelectedCalendar.updatedAt, so this entire call may be targeting the wrong model for the displayed timestamp

Agent: logic

throw new Error("Credential not found or access denied");
}

await prisma.calendarCache.deleteMany({

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 MAJOR — Improper Error Handling / Information Exposure (confidence: 100%)

The handler throws a plain Error instead of a tRPC TRPCError with an appropriate HTTP status code. When a credential is not found or does not belong to the authenticated user, the client receives a generic 500 Internal Server Error response rather than a 401 Unauthorized or 404 Not Found. This prevents the client from properly distinguishing authentication/authorization failures from server errors, and generic 500 responses may trigger unintended retry logic or confuse error handling in the frontend.

Evidence:

  • Line 26: throw new Error("Credential not found or access denied") — plain Error, not TRPCError
  • tRPC middleware converts unrecognized errors to INTERNAL_SERVER_ERROR (HTTP 500), masking the actual cause
  • The acceptance criteria explicitly state: 'The deleteCache handler throws an error (not just returns null) when the credential does not exist or does not belong to ctx.user.id' — but the type of error matters for proper HTTP semantics
  • A TRPCError with code 'UNAUTHORIZED' or 'NOT_FOUND' would produce a 401/404 response, enabling the client to surface a meaningful error message

Agent: security

});

if (!credential) {
throw new Error("Credential not found or access denied");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 MAJOR — Error Handling (confidence: 100%)

The handler throws a plain Error instead of a tRPC TRPCError with an appropriate HTTP status code. This will result in the client receiving a generic 500 INTERNAL_SERVER_ERROR instead of a proper 403 FORBIDDEN or 404 NOT_FOUND response, making it harder for the frontend to distinguish between authorization failures and actual server errors.

Evidence:

  • The intent states: 'The deleteCache handler throws an error (not just returns null) when the credential does not exist or does not belong to ctx.user.id' — while it does throw, using plain Error is incorrect for tRPC
  • The risk areas note: 'throws a plain Error instead of a tRPC TRPCError with appropriate HTTP status'
  • Standard tRPC pattern in Cal.com uses TRPCError with code: 'NOT_FOUND' or 'UNAUTHORIZED'
  • Line 25 throws new Error() which is not a tRPC error
  • Should use TRPCError with code: 'UNAUTHORIZED' or 'NOT_FOUND' for proper HTTP status codes
  • Client-side error handling expects tRPC error structure

Agent: logic


const cacheStatusMap = new Map(cacheStatuses.map((cache) => [cache.credentialId, cache.updatedAt]));

const enrichedConnectedCalendars = connectedCalendars.map((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.

🟠 MAJOR — Edge Case Handling (confidence: 100%)

Using || null to coerce undefined from Map.get() will also coerce a legitimate Date value of epoch zero (new Date(0)) or any falsy value to null. While unlikely for timestamps in practice, the safer and more semantically correct approach is to use ?? null (nullish coalescing) which only converts undefined and null, not other falsy values.

Evidence:

  • The edge cases note: 'cacheStatusMap.get() uses || null which will also coerce a legitimate updatedAt of 0 (epoch) to null'
  • The _max.updatedAt from Prisma groupBy returns Date | null, and Map.get() returns Date | null | undefined?? null would be the correct operator

Agent: logic


if grep -q '^GOOGLE_WEBHOOK_URL=' "$ENV_FILE"; then
sed -i '' -E "s|^GOOGLE_WEBHOOK_URL=.*|GOOGLE_WEBHOOK_URL=$TUNNEL_URL|" "$ENV_FILE"
else

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 MAJOR — Platform Compatibility (confidence: 100%)

The sed -i '' -E syntax is macOS-specific. On Linux (including most CI environments), GNU sed requires sed -i -E (without the empty string argument). This will cause the script to fail on Linux with an error like 'can't read s|...|: No such file or directory'.

Evidence:

  • Line 66: sed -i '' -E "s|^GOOGLE_WEBHOOK_URL=.*|GOOGLE_WEBHOOK_URL=$TUNNEL_URL|" "$ENV_FILE"
  • GNU sed (Linux) treats '' as the backup suffix argument and interprets the -E as the filename to edit in-place
  • The review context explicitly flags: 'The scripts/test-gcal-webhooks.sh uses sed -i '' -E which is macOS-specific syntax; on Linux sed -i -E is required'
  • The edge cases explicitly flag this: 'The scripts/test-gcal-webhooks.sh uses sed -i '' -E which is macOS-specific syntax; on Linux sed -i -E is required — this could break CI if run on Linux'
  • macOS uses BSD sed where -i '' means in-place edit with no backup extension; GNU sed interprets '' as the input file name
  • Line 69: sed -i '' -E "s|^GOOGLE_WEBHOOK_URL=.*|GOOGLE_WEBHOOK_URL=$TUNNEL_URL|" "$ENV_FILE"
  • BSD sed requires sed -i '' for in-place editing without backup; GNU sed interprets '' as the backup suffix pattern

Agent: logic

externalId: true,
integration: true,
updatedAt: true,
googleChannelId: true,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 MINOR — Module Boundaries / Type Expansion (confidence: 80%)

Adding updatedAt and googleChannelId to the SelectedCalendar select in the user repository broadens the data fetched for all consumers of this query, not just the calendar cache feature. This increases the data surface area returned from the database for every call path that uses this select, which may have unintended side effects on other features consuming this user data shape.

Evidence:

  • Lines 899-900 add updatedAt: true and googleChannelId: true to the select clause
  • The UserWithCalendars type in getConnectedDestinationCalendars.ts is also updated to include these fields
  • Any downstream consumer of this user query now receives these additional fields

Agent: architecture

Warnings:

- Added the required column `updatedAt` to the `CalendarCache` table without a default value. This is not possible if the table is not empty.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 INFO — Migration Safety (confidence: 93%)

The auto-generated Prisma migration warning comment states 'Added the required column updatedAt to the CalendarCache table without a default value. This is not possible if the table is not empty.' However, the actual SQL on line 9 includes DEFAULT NOW(), which safely handles existing rows. The contradictory comment could confuse future developers reviewing migration history.

Evidence:

  • Lines 2-5: Warning says 'without a default value'
  • Line 9: SQL includes DEFAULT NOW()
  • The Prisma schema also includes @default(now()) confirming the default is intentional

Agent: architecture

@@ -23,8 +24,19 @@ export const connectedCalendarsHandler = async ({ ctx, input }: ConnectedCalenda
prisma,
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 INFO — Code organization (confidence: 86%)

Direct instantiation of CalendarCacheRepository instead of dependency injection. This bypasses mocks in test environments and couples the handler to the implementation.

Evidence:

  • Line 26: const cacheRepository = new CalendarCacheRepository();
  • Tight coupling makes unit testing harder without refactoring
  • Repository pattern with DI would allow test mocks to be injected via ctx

Agent: style

}

return (
<>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 INFO — Code patterns (confidence: 79%)

The cacheStatusMap.get() uses || null which will coerce falsy values (including timestamp 0 for epoch) to null. While unlikely for timestamps, this could mask legitimate cache data.

Evidence:

  • Line 74: cacheStatusMap.get(calendar.credentialId) || null
  • The || null pattern coerces any falsy value to null
  • Edge case: if updatedAt is somehow 0 (epoch), it would be treated as null
  • Better to explicitly check for undefined

Agent: style

touch "$ENV_FILE"
fi

if grep -q '^GOOGLE_WEBHOOK_URL=' "$ENV_FILE"; then

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 INFO — Code patterns (confidence: 71%)

Uses macOS-specific sed syntax 'sed -i '' -E' which will fail on Linux. CI systems running on Linux will break.

Evidence:

  • Line 67: sed -i '' -E uses empty string placeholder, which is macOS sed syntax
  • Linux sed requires sed -i -E (no empty string)
  • This will cause CI failures if run on Linux environments

Agent: style

@Frawatson

Copy link
Copy Markdown
Owner Author

@grapple-pr review

@grapple-pr grapple-pr Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🤖 Grapple PR Review

Summary

This PR adds calendar cache status visibility and management actions to the connected calendars UI, introducing a new CredentialActionsDropdown component that replaces the simple disconnect button with a dropdown supporting both cache deletion and credential disconnection. It also adds an updatedAt field to the CalendarCache model to track when cache was last refreshed.

Changes: 17 files (+379 / -31 lines)

  • 🟢 4 files added: packages/features/apps/components/CredentialActionsDropdown.tsx, packages/prisma/migrations/20250715160635_add_calendar_cache_updated_at/migration.sql, packages/trpc/server/routers/viewer/calendars/deleteCache.handler.ts, scripts/test-gcal-webhooks.sh
  • 🟡 13 files modified: apps/web/package.json, apps/web/public/static/locales/en/common.json, packages/app-store/googlecalendar/lib/CalendarService.ts, packages/features/calendar-cache/calendar-cache.repository.interface.ts, packages/features/calendar-cache/calendar-cache.repository.mock.ts +8 more

CI: ⏳ 0 passed, 0 failed

Impact Diagram

graph TD
  pr_0[/"🔀 PR Changes: 17 files"/]
  f1["🟡 package.json +1/-1"]
  pr_0 --> f1
  f2["🟡 common.json +7/-0"]
  pr_0 --> f2
  f3["🟡 CalendarService.ts +3/-0"]
  pr_0 --> f3
  f4["🟢 CredentialActionsDropdown.tsx +157/-0"]
  pr_0 --> f4
  f5["🟡 calendar-cache.repository.interface.ts +3/-0"]
  pr_0 --> f5
  f6["🟡 calendar-cache.repository.mock.ts +5/-0"]
  pr_0 --> f6
  f7["🟡 calendar-cache.repository.ts +17/-0"]
  pr_0 --> f7
  f8["🟡 getConnectedDestinationCalendars.ts +8/-2"]
  pr_0 --> f8
  f9["🟡 selectedCalendar.ts +8/-2"]
  pr_0 --> f9
  f10["🟡 user.ts +2/-0"]
  pr_0 --> f10
  f11["🟡 SelectedCalendarsSettingsWebWrapper.tsx +23/-25"]
  pr_0 --> f11
  f12["🟢 migration.sql +9/-0"]
  pr_0 --> f12
  more_13["... +5 more files"]
  pr_0 --> more_13
  f5 -.- f6
  f6 -.- f7
  f9 -.- f10
  style f1 fill:#7c3aed,color:#fff,stroke:#7c3aed
  style f2 fill:#7c3aed,color:#fff,stroke:#7c3aed
  style f3 fill:#7c3aed,color:#fff,stroke:#7c3aed
  style f4 fill:#7c3aed,color:#fff,stroke:#7c3aed
  style f5 fill:#7c3aed,color:#fff,stroke:#7c3aed
  style f6 fill:#7c3aed,color:#fff,stroke:#7c3aed
  style f7 fill:#7c3aed,color:#fff,stroke:#7c3aed
  style f8 fill:#7c3aed,color:#fff,stroke:#7c3aed
  style f9 fill:#7c3aed,color:#fff,stroke:#7c3aed
  style f10 fill:#7c3aed,color:#fff,stroke:#7c3aed
  style f11 fill:#7c3aed,color:#fff,stroke:#7c3aed
  style f12 fill:#7c3aed,color:#fff,stroke:#7c3aed
Loading

1 critical issue found — please address before merging.

Severity Count
🔴 Critical 1
🟠 Major 3
🟡 Minor 5
🔵 Info 2
Review details
  • Reviewed at: 2026-04-12T01:58:48.987Z
  • Agents used: security, logic, style, architecture, performance, verification
  • Context level: full
  • Execution time: 83.7s
  • Blast radius: 0 (contained)

Powered by Grapple PR — AI that understands your code.

const data = await this.fetchAvailability(parsedArgs);
await this.setAvailabilityInCache(parsedArgs, data);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 CRITICAL — Business logic correctness (confidence: 100%)

Calling updateManyByCredentialId(credentialId, {}) with an empty data object will likely NOT trigger Prisma's @updatedAt directive. Prisma's updateMany with an empty data object is effectively a no-op — no SQL SET clause is generated, so the @updatedAt auto-update mechanism never fires. This means SelectedCalendar.updatedAt will never be refreshed after cache warmup, defeating the purpose of tracking when the cache was last updated.

Evidence:

  • The intent states: 'When GoogleCalendarService.warmupCache completes, SelectedCalendar.updatedAt should be updated for all calendars under that credential'
  • The edge cases explicitly call this out: 'Prisma's updateMany with empty data may be a no-op depending on the version — the @updatedat field may not auto-update if no fields are explicitly changed'
  • The repository method signature accepts Prisma.SelectedCalendarUpdateInput and passes it directly as data: {} to updateMany
  • Prisma documentation states @updatedat is only set when a write operation modifies data

Agent: logic

disableConnectionModification={disableConnectionModification}
/>
</div>
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 MAJOR — State management (confidence: 100%)

In the error/fallback branch (when connectedCalendar.calendars is falsy), CredentialActionsDropdown is rendered without the disableConnectionModification prop. This means even when the parent has disableConnectionModification=true, the error-state card will still allow disconnection, which is inconsistent with the primary branch behavior.

Evidence:

  • Line 70-75 in the primary branch passes disableConnectionModification={disableConnectionModification}
  • Line 120-133 in the error/fallback branch does not pass disableConnectionModification
  • The old code had !connectedCalendar.delegationCredentialId but no disableConnectionModification check in the error branch either, but the new code inconsistently applies it only in one branch

Agent: logic


return cacheStatuses.map((cache) => ({
credentialId: cache.credentialId,
updatedAt: cache._max.updatedAt,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 MAJOR — Edge cases (confidence: 94%)

When credentialIds is an empty array, Prisma's groupBy with { in: [] } may generate an invalid SQL IN () clause on some database engines/versions. While recent Prisma versions may handle this gracefully by short-circuiting, this is database-dependent and not guaranteed. The mock implementation returns [] for empty input, but the real implementation does not guard against it.

Evidence:

  • The intent specification calls this out: 'getCacheStatusByCredentialIds is called even when credentialIds is empty... An empty IN () clause in some databases is invalid — verify Prisma handles empty arrays gracefully'
  • The mock returns [] unconditionally, suggesting the expected behavior for empty input is an empty result
  • connectedCalendarsHandler maps all credential IDs and passes them directly without checking for empty array

Agent: logic

throw new Error("Credential not found or access denied");
}

await prisma.calendarCache.deleteMany({

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 MAJOR — Error Handling / Information Disclosure (confidence: 89%)

The handler throws a plain Error instead of a TRPCError. When tRPC catches a non-TRPCError, it returns a generic INTERNAL_SERVER_ERROR but may also log or expose the original error message ('Credential not found or access denied') in ways that differ from tRPC's normal error sanitization pipeline. Additionally, throwing a plain Error means the HTTP status code will be 500 instead of the more appropriate 403/404, which can be fingerprinted by attackers to distinguish 'credential exists but wrong user' from 'credential not found'.

Evidence:

  • Line 26: throw new Error("Credential not found or access denied");
  • tRPC's authedProcedure expects TRPCError for controlled error responses
  • Using a plain Error may bypass tRPC's error formatting/sanitization and expose stack traces in development or staging environments
  • A 500 response for an authorization failure leaks that the endpoint exists and the input was valid format

Agent: security

const data = await this.fetchAvailability(parsedArgs);
await this.setAvailabilityInCache(parsedArgs, data);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 MINOR — Logic Bug / Silent Failure (confidence: 98%)

Calling SelectedCalendarRepository.updateManyByCredentialId(this.credential.id, {}) with an empty data object {} to trigger Prisma's @updatedAt auto-update is unreliable. Prisma's updateMany with an empty data object may be a no-op at the database level — the @updatedAt field is only auto-populated when at least one other field is updated. If Prisma optimizes away the empty update, the updatedAt timestamp will never be written, silently breaking the cache freshness UI feature.

Evidence:

  • Line 1022: await SelectedCalendarRepository.updateManyByCredentialId(this.credential.id, {});
  • Prisma documentation states @updatedAt is set automatically on update/updateMany operations, but behavior with empty data: {} is not explicitly guaranteed
  • In practice, many Prisma versions will emit a SQL UPDATE with only updatedAt = NOW() when @updatedAt is present, but this depends on version and may silently skip the query
  • The entire cacheUpdatedAt feature in the UI depends on this mechanism working correctly

Agent: security

});

if (!credential) {
throw new Error("Credential not found or access denied");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 MINOR — Error handling (confidence: 89%)

The handler throws a generic Error instead of a TRPCError with a proper error code (e.g., NOT_FOUND or FORBIDDEN). This means the error will be caught by tRPC's error handler and returned as an INTERNAL_SERVER_ERROR (500) rather than a proper 403/404, making it harder for the client to distinguish between authorization failures and actual server errors.

Evidence:

  • Other tRPC handlers in the codebase typically use throw new TRPCError({ code: 'NOT_FOUND' }) or similar
  • The acceptance criteria states: 'deleteCacheHandler validates that the credentialId belongs to ctx.user.id before deleting any rows' — the validation exists but the error response is suboptimal

Agent: logic

interface CredentialActionsDropdownProps {
credentialId: number;
integrationType: string;
cacheUpdatedAt?: Date | null;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 MINOR — Data flow (confidence: 88%)

The cacheUpdatedAt prop is typed as Date | null but the data coming from the tRPC response will be a JSON-serialized string (ISO format), not a Date object. The code at line 96 does new Date(cacheUpdatedAt) which handles this correctly at runtime, but the type annotation is misleading and could cause issues if someone relies on it being a Date object elsewhere.

Evidence:

  • tRPC serializes Date objects to ISO strings over the wire
  • Line 96: new Date(cacheUpdatedAt) — this works with both strings and Dates, showing the developer was aware of the potential string input
  • The interface declares cacheUpdatedAt?: Date | null but the actual runtime value from connectedCalendarsHandler will be a string after JSON serialization

Agent: logic

id: credentialId,
userId: user.id,
},
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 MINOR — Edge cases (confidence: 84%)

The authorization check only verifies userId === user.id but does not account for delegation credentials or shared/team credentials. A user could potentially delete cache for a delegation credential they are assigned to but don't own, if the credential's userId matches (which it would for delegated credentials assigned to them).

Evidence:

  • The intent's risk areas state: 'deleteCacheHandler authorization: only checks credential.userId === user.id but does not check if the credential is a delegation credential or shared credential — potential for unauthorized cache deletion in edge cases'
  • The CredentialActionsDropdown UI guards against delegation credentials showing cache delete, but the API endpoint itself does not

Agent: logic

</div>
}>
<div className="border-subtle border-t">
{!fromOnboarding && (

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 INFO — Empty Wrapper DOM Node (confidence: 89%)

The `` wrapper is now unconditionally rendered even when CredentialActionsDropdown returns `null`. Previously, the entire actions block was conditionally rendered. This leaves an empty 32-wide flex div in the DOM when no actions are available (delegation credentials with no cache), potentially causing layout artifacts.

Evidence:

  • Lines 70-82: — no conditional wrapper
  • Old code: !connectedCalendar.delegationCredentialId && !disableConnectionModification && (...) — fully conditional
  • CredentialActionsDropdown returns null when !canDisconnect && !hasCache, but the parent div still occupies space

Agent: security


if grep -q '^GOOGLE_WEBHOOK_URL=' "$ENV_FILE"; then
sed -i '' -E "s|^GOOGLE_WEBHOOK_URL=.*|GOOGLE_WEBHOOK_URL=$TUNNEL_URL|" "$ENV_FILE"
else

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 INFO — Portability (confidence: 89%)

The sed -i '' -E syntax is macOS-specific. On Linux, sed -i does not take an empty string argument — it would interpret '' as the filename. This script will fail on Linux/CI environments.

Evidence:

  • macOS BSD sed: sed -i '' -E 's/...' (empty string means no backup extension)
  • GNU/Linux sed: sed -i -E 's/...' (no argument after -i means no backup)
  • The intent specification notes: 'The sed -i '' -E command uses macOS-style sed syntax which will fail on Linux'

Agent: logic

@grapple-pr grapple-pr Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔧 [Grapple PR] 6 auto-fix suggestions available

Apply individually or use GitHub's "Add suggestion to batch""Commit suggestions" to apply all at once.

Commits from Grapple PR are prefixed with [Grapple PR] for easy identification in merge conflicts.

const data = await this.fetchAvailability(parsedArgs);
await this.setAvailabilityInCache(parsedArgs, data);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[Grapple PR] Auto-fix — logic agent (Small fix (2 lines, 1 file))

Calling updateManyByCredentialId(credentialId, {}) with an empty data object will likely NOT trigger Prisma's @updatedAt directive. Prisma's updateMany with an empty data object is effectively a no-op — no SQL SET clause is generated, so the @updatedAt auto-update mechanism never fires. This means SelectedCalendar.updatedAt will never be refreshed after cache warmup, defeating the purpose of tracking when the cache was last updated.

Suggested change
await SelectedCalendarRepository.updateManyByCredentialId(this.credential.id, { updatedAt: new Date() });

🤖 Grapple PR auto-fix • critical • confidence: 100%

disableConnectionModification={disableConnectionModification}
/>
</div>
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[Grapple PR] Auto-fix — logic agent (Small fix (1 lines, 1 file))

In the error/fallback branch (when connectedCalendar.calendars is falsy), CredentialActionsDropdown is rendered without the disableConnectionModification prop. This means even when the parent has disableConnectionModification=true, the error-state card will still allow disconnection, which is inconsistent with the primary branch behavior.

Suggested change
}
disableConnectionModification={disableConnectionModification}

🤖 Grapple PR auto-fix • major • confidence: 100%


return cacheStatuses.map((cache) => ({
credentialId: cache.credentialId,
updatedAt: cache._max.updatedAt,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[Grapple PR] Auto-fix — logic agent (Small fix (6 lines, 1 file))

When credentialIds is an empty array, Prisma's groupBy with { in: [] } may generate an invalid SQL IN () clause on some database engines/versions. While recent Prisma versions may handle this gracefully by short-circuiting, this is database-dependent and not guaranteed. The mock implementation returns [] for empty input, but the real implementation does not guard against it.

Suggested change
updatedAt: cache._max.updatedAt,
// Guard against empty array — some DB engines generate invalid `IN ()` SQL.
// The mock implementation also returns [] for empty input, so this is the expected contract.
if (credentialIds.length === 0) {
return [];
}

🤖 Grapple PR auto-fix • major • confidence: 94%

const data = await this.fetchAvailability(parsedArgs);
await this.setAvailabilityInCache(parsedArgs, data);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[Grapple PR] Auto-fix — security agent (Small fix (2 lines, 1 file))

Calling SelectedCalendarRepository.updateManyByCredentialId(this.credential.id, {}) with an empty data object {} to trigger Prisma's @updatedAt auto-update is unreliable. Prisma's updateMany with an empty data object may be a no-op at the database level — the @updatedAt field is only auto-populated when at least one other field is updated. If Prisma optimizes away the empty update, the updatedAt timestamp will never be written, silently breaking the cache freshness UI feature.

Suggested change
await SelectedCalendarRepository.updateManyByCredentialId(this.credential.id, { updatedAt: new Date() });

🤖 Grapple PR auto-fix • minor • confidence: 98%

interface CredentialActionsDropdownProps {
credentialId: number;
integrationType: string;
cacheUpdatedAt?: Date | null;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[Grapple PR] Auto-fix — logic agent (Small fix (2 lines, 1 file))

The cacheUpdatedAt prop is typed as Date | null but the data coming from the tRPC response will be a JSON-serialized string (ISO format), not a Date object. The code at line 96 does new Date(cacheUpdatedAt) which handles this correctly at runtime, but the type annotation is misleading and could cause issues if someone relies on it being a Date object elsewhere.

Suggested change
cacheUpdatedAt?: Date | null;
cacheUpdatedAt?: string | null;

🤖 Grapple PR auto-fix • minor • confidence: 88%

id: credentialId,
userId: user.id,
},
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[Grapple PR] Auto-fix — logic agent (Small fix (5 lines, 1 file))

The authorization check only verifies userId === user.id but does not account for delegation credentials or shared/team credentials. A user could potentially delete cache for a delegation credential they are assigned to but don't own, if the credential's userId matches (which it would for delegated credentials assigned to them).

Suggested change
});
// Do not allow cache deletion for delegation credentials or credentials
// not directly owned by this user. The UI (CredentialActionsDropdown)
// already hides this action for delegation credentials, but we enforce
// it at the API level as well.
delegatedToUserId: null,

🤖 Grapple PR auto-fix • minor • confidence: 84%

throw new Error("Credential not found or access denied");
}

await prisma.calendarCache.deleteMany({

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 [Grapple PR] Suggested fix — security agent (Small fix (7 lines, 1 file))

The handler throws a plain Error instead of a TRPCError. When tRPC catches a non-TRPCError, it returns a generic INTERNAL_SERVER_ERROR but may also log or expose the original error message ('Credential not found or access denied') in ways that differ from tRPC's normal error sanitization pipeline. Additionally, throwing a plain Error means the HTTP status code will be 500 instead of the more appropriate 403/404, which can be fingerprinted by attackers to distinguish 'credential exists but wrong user' from 'credential not found'.

--- a/packages/trpc/server/routers/viewer/calendars/deleteCache.handler.ts
+++ b/packages/trpc/server/routers/viewer/calendars/deleteCache.handler.ts
@@ -1,5 +1,6 @@
+import { TRPCError } from "@trpc/server";
+
 import { prisma } from "@calcom/prisma";
 import type { TrpcSessionUser } from "@calcom/trpc/server/types";
 
 type DeleteCacheOptions = {
@@ -22,7 +23,10 @@
   });
 
   if (!credential) {
-    throw new Error("Credential not found or access denied");
+    throw new TRPCError({
+      code: "NOT_FOUND",
+      message: "Credential not found or access denied",
+    });
   }
 
   await prisma.calendarCache.deleteMany({

🤖 Grapple PR auto-fix • major • Review this diff before applying

fi

if grep -q '^GOOGLE_WEBHOOK_URL=' "$ENV_FILE"; then
sed -i '' -E "s|^GOOGLE_WEBHOOK_URL=.*|GOOGLE_WEBHOOK_URL=$TUNNEL_URL|" "$ENV_FILE"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 [Grapple PR] Suggested fix — security agent (Larger fix (13 lines, 1 file) — review recommended)

The TUNNEL_URL variable is used unquoted inside a sed substitution expression. While the regex constrains the URL to https://.*\.tunnelmole\.net, the .* wildcard could match URLs containing | (the sed delimiter) if the tunnelmole log is tampered or contains unexpected output. Additionally, macOS-style sed -i '' -E syntax will fail on Linux systems, which could cause CI breakage if this script is ever run in a Linux CI environment.

--- a/scripts/test-gcal-webhooks.sh
+++ b/scripts/test-gcal-webhooks.sh
@@ -65,7 +65,13 @@ fi
 
 if grep -q '^GOOGLE_WEBHOOK_URL=' "$ENV_FILE"; then
-  sed -i '' -E "s|^GOOGLE_WEBHOOK_URL=.*|GOOGLE_WEBHOOK_URL=$TUNNEL_URL|" "$ENV_FILE"
+  # Use python3 for portable in-place replacement (macOS + Linux compatible).
+  # TUNNEL_URL is passed as a Python variable — not interpolated into the regex
+  # expression — so characters like `|` in the URL cannot break the substitution.
+  python3 - "$ENV_FILE" "$TUNNEL_URL" <<'EOF'
+import sys, re
+path, url = sys.argv[1], sys.argv[2]
+with open(path, 'r') as f:
+    content = f.read()
+content = re.sub(r'^GOOGLE_WEBHOOK_URL=.*', 'GOOGLE_WEBHOOK_URL=' + url, content, flags=re.MULTILINE)
+with open(path, 'w') as f:
+    f.write(content)
+EOF
 else
   echo "GOOGLE_WEBHOOK_URL=$TUNNEL_URL" >> "$ENV_FILE"
 fi

🤖 Grapple PR auto-fix • minor • Review this diff before applying

});

if (!credential) {
throw new Error("Credential not found or access denied");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 [Grapple PR] Suggested fix — logic agent (Small fix (6 lines, 1 file))

The handler throws a generic Error instead of a TRPCError with a proper error code (e.g., NOT_FOUND or FORBIDDEN). This means the error will be caught by tRPC's error handler and returned as an INTERNAL_SERVER_ERROR (500) rather than a proper 403/404, making it harder for the client to distinguish between authorization failures and actual server errors.

--- a/packages/trpc/server/routers/viewer/calendars/deleteCache.handler.ts
+++ b/packages/trpc/server/routers/viewer/calendars/deleteCache.handler.ts
@@ -1,5 +1,6 @@
 import { prisma } from "@calcom/prisma";
+import { TRPCError } from "@trpc/server";
 import type { TrpcSessionUser } from "@calcom/trpc/server/types";
 
 type DeleteCacheOptions = {
@@ -22,7 +23,9 @@ export const deleteCacheHandler = async ({ ctx, input }: DeleteCacheOptions) =>
 
   if (!credential) {
-    throw new Error("Credential not found or access denied");
+    throw new TRPCError({
+      code: "NOT_FOUND",
+      message: "Credential not found or access denied",
+    });
   }
 
   await prisma.calendarCache.deleteMany({

🤖 Grapple PR auto-fix • minor • Review this diff before applying

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