Repository navigation
Fix Google Calendar: read-only, push, duplicates, and webhook issues - #488
Conversation
Remove read-only mode entirely (two-way sync is the only mode), fix push-to-Google being silently disabled, prevent duplicate events from alias mismatches and push/sync race conditions, and suppress noisy webhook errors for holiday calendars. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughRemoves Google-readonly enforcement: deletes guards and schema surface for Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as Server API
participant DB
participant Google as Google Calendar API
participant Push as PushService
Client->>API: Request to create/update/delete event
API->>DB: Validate & fetch event/connection
DB-->>API: Event & connection data
API->>Push: Broadcast change (webhook/push)
Push->>Google: (if applicable) send push/notify
API->>DB: Upsert/update event record (googleSyncReadOnly=false)
DB-->>API: Confirmation
API-->>Client: Success response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ter threading, and schema defaults Two-way sync is now the only mode. Remove GOOGLE_READ_ONLY_ERROR constants, read-only guard blocks, markAsReadOnly parameter threading through sync/transform functions, and flip schema defaults to false. Gate cleanupLegacyData to run once per connection. Fix pre-existing test mock (missing isNull/desc exports). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/web/src/lib/integrations/google-calendar/sync-service.ts`:
- Around line 491-526: The code sets syncedFromGoogle: true when linking a local
event to an incoming Google event, which prevents the push-service
(push-service.ts) from ever pushing local edits back; instead, update the update
block in sync-service.ts (the one updating calendarEvents for nearDuplicate.id
with googleEventId and googleCalendarId) to NOT set syncedFromGoogle to true
(leave it false or omit it) and ensure googleSyncReadOnly is false and
lastGoogleSync/updatedAt are set appropriately so the event stays pushable; keep
mapAttendeesToUsers(nearDuplicate.id, googleEvent.attendees) as-is.
🧹 Nitpick comments (3)
apps/web/src/lib/integrations/google-calendar/__tests__/sync-service.test.ts (1)
260-261: Consider adding test coverage for the new sync-service behaviors.The PR introduces significant new logic —
cleanupLegacyData(), near-duplicate detection (title + startAt within 1 min), and calendar ID alias correction — but this test file has no coverage for any of it. These are complex conditional paths that would benefit from dedicated test cases to prevent silent regressions.apps/web/src/lib/integrations/google-calendar/sync-service.ts (2)
221-224: Stale docstring: says "runs every sync" but the gate skips it after first run.Line 223 says "runs every sync but is a no-op after first run," but the call site (Line 115) gates on
connection.markAsReadOnly, so the function is never invoked again once the flag is cleared.📝 Suggested fix
/** - * Fix legacy data from when read-only mode was the default. - * Idempotent: runs every sync but is a no-op after first run. + * Fix legacy data from when read-only mode was the default. + * Gated by markAsReadOnly on the connection — runs once, then skipped on subsequent syncs. */
257-268: Raw SQL used for deduplication query — consider using Drizzle query builder.The coding guidelines require using Drizzle ORM for all database operations. While
db.execute(sql...)is still going through Drizzle, this raw query is fragile against table/column renames and skips Drizzle's type-safe ergonomics. AGROUP BY+HAVINGcan be expressed via Drizzle'sselect().from().where().groupBy().having()API.♻️ Drizzle query builder alternative
- const duplicateResult = await db.execute<{ - google_event_id: string; - cnt: string; - }>(sql` - SELECT "googleEventId" as google_event_id, COUNT(*) as cnt - FROM calendar_events - WHERE "createdById" = ${userId} - AND "googleEventId" IS NOT NULL - AND "isTrashed" = false - GROUP BY "googleEventId" - HAVING COUNT(*) > 1 - `); + const duplicateResult = await db + .select({ + googleEventId: calendarEvents.googleEventId, + cnt: sql<number>`count(*)`.as('cnt'), + }) + .from(calendarEvents) + .where(and( + eq(calendarEvents.createdById, userId), + sql`${calendarEvents.googleEventId} IS NOT NULL`, + eq(calendarEvents.isTrashed, false) + )) + .groupBy(calendarEvents.googleEventId) + .having(sql`count(*) > 1`);Then update the loop to use
row.googleEventIdinstead ofrow.google_event_id.As per coding guidelines: "Use centralized Drizzle ORM with PostgreSQL for all database operations - no direct SQL or other ORMs."
When linking a locally-created event to its Google counterpart during sync (race condition guard), keep syncedFromGoogle=false so the push service continues pushing local edits back to Google. Setting it to true would silently break two-way sync for the affected event. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
Fixes four interconnected issues with Google Calendar two-way sync introduced in PR #487:
pushNotSupportedForRequestedResourceerrors every sync cycleRoot Causes & Fixes
1. Read-only + push broken (
markAsReadOnlydefaulted totrue)The DB schema defaults
markAsReadOnly: true, so every connection starts read-only. The push service checked!connection.markAsReadOnlyto decide whether to push, silently disabling all push. The event API routes blocked PATCH/DELETE ongoogleSyncReadOnlyevents.Fix: Remove all read-only guards. Two-way sync is the only mode —
pushEnabledis alwaystrue, event PATCH/DELETE no longer checkgoogleSyncReadOnly, EventModal no longer shows read-only banner or disables fields. The OAuth callback now setsmarkAsReadOnly: falseon reconnect. Sync always setsgoogleSyncReadOnly: falseon every event update.2. Duplicate events from alias mismatch
The upsert lookup used
(createdById, googleEventId, googleCalendarId)as an exact-match key. Old events storedgoogleCalendarId = "primary"while new syncs resolve this to the actual email address, so lookups miss and create duplicates.Fix: Two-step lookup — exact match first, then fallback to
(createdById, googleEventId)alone. If the fallback finds a match with a stalegoogleCalendarId, it fixes it in place. A cleanup step at sync start also bulk-fixes all"primary"aliases to the resolved email.3. Duplicate events from push/sync race condition
When a user creates an event, push sends it to Google via
after()(fire-and-forget). If a webhook-triggered sync runs before push stores thegoogleEventIdback on the local event, sync sees nogoogleEventIdand creates a duplicate.Fix: Near-duplicate detection before insert — if a local event exists with the same title and
startAtwithin 1 minute, nogoogleEventId, andsyncedFromGoogle = false, link it to the Google event instead of creating a new one.4. Webhook noise for holiday calendars
registerWebhookChannelstried to register push notifications for ALL selected calendars, including Google's special calendars (holidays, contacts, week numbers) which don't support push.Fix: Skip calendars matching
#...@group.v.calendar.google.comwith an info-level log instead of attempting registration and logging warnings.5. Legacy data cleanup
Added an idempotent
cleanupLegacyData()that runs at the start of every sync to fix existing data:markAsReadOnly = falseon the connectiongoogleSyncReadOnly = falseon all synced eventsgoogleCalendarId = "primary"→ actual emailgoogleEventId(soft-deletes older copies)Files Changed
sync-service.tspush-service.tspushEnabled: true)events/[eventId]/route.tscallback/route.tsmarkAsReadOnly: falseto reconnect upsertsettings/route.tsmarkAsReadOnlyfrom schema, GET, and PATCHEventModal.tsxTest Plan
googleCalendarId = 'primary'— should be zeropushNotSupportedForRequestedResourceerrors for holiday calendarsmarkAsReadOnlyisfalseon the connection🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Chores