fix(caldav): default calendar not written on booking#28934
fix(caldav): default calendar not written on booking#28934Harshithk951 wants to merge 2 commits intocalcom:mainfrom
Conversation
|
Welcome to Cal.diy, @Harshithk951! Thanks for opening this pull request. A few things to keep in mind:
A maintainer will review your PR soon. Thanks for contributing! |
|
Hey @sahitya-chandra — this is the rebased version of #28445, now with a clean single commit on top of latest main. CI is green. Would appreciate a review when you get a chance! |
📝 WalkthroughWalkthroughThe event creation logic in CalendarService has been refactored to compute a target subset of calendars upfront rather than filtering all calendars during processing. The target calendars are determined based on 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/lib/CalendarService.ts`:
- Around line 466-472: Replace the thrown plain Error with an ErrorWithCode
(e.g., new ErrorWithCode("No target calendars found to create CalDAV calendar
entry", "CALDAV:no-target-calendar")) where the current check on targetCalendars
fails; when selecting the primary calendar prefer the
CALDAV:schedule-default-calendar-URL property if exposed by tsdav (inspect the
collection metadata returned for a schedule-default-calendar-URL and match that
URL against calendars to build targetCalendars) and only if that property is not
available fall back to picking the first calendar from calendars but emit a
debug/info log that includes the chosen calendar URL (log the selected
calendar's href/externalId) so misrouted writes are traceable; update the logic
around mainHostDestinationCalendar, calendars and targetCalendars to implement
these checks and the ErrorWithCode replacement.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5300df62-f419-4d6d-8cc5-a5d87be1f320
📒 Files selected for processing (1)
packages/lib/CalendarService.ts
| const targetCalendars = mainHostDestinationCalendar?.externalId | ||
| ? calendars.filter((c) => c.externalId === mainHostDestinationCalendar.externalId) | ||
| : calendars.slice(0, 1); | ||
|
|
||
| if (targetCalendars.length === 0) { | ||
| throw new Error("No target calendars found to create CalDAV calendar entry"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🌐 Web query:
Does the CalDAV/WebDAV PROPFIND response for calendar-home-set guarantee the primary/default calendar is listed first? Is there a standard property (e.g., CALDAV:schedule-default-calendar-URL) to identify the default calendar?
💡 Result:
No, the CalDAV/WebDAV PROPFIND response for calendar-home-set does not guarantee that the primary/default calendar is listed first. RFC 4791 defines CALDAV:calendar-home-set as containing zero or more DAV:href elements identifying collections with the principal's calendars, with no specification on order (RFC 4791 Section 6.2.1; sources 1,11). Implementations may vary in ordering, and clients cannot rely on position (e.g., GitHub issue on Stalwart notes clients like Apple do not consistently pick first; source 5). Yes, there is a standard property: CALDAV:schedule-default-calendar-URL (urn:ietf:params:xml:ns:caldav), defined in RFC 6638 Section 9.2. It MAY be defined on the scheduling Inbox collection (discovered via CALDAV:schedule-inbox-URL on the principal) and, if present with a DAV:href, points to the default calendar where new scheduling objects are created (sources 6,7,9,10). Servers SHOULD use this for new scheduling objects (RFC 6638 Section 4.3). Some implementations expose it on the principal too, but standard location is the inbox (source 9).
Citations:
- 1: https://icalendar.org/CalDAV-Access-RFC-4791/6-2-1-caldav-calendar-home-set-property.html
- 2: 🐛: Return all shared resources via WebDAV ACL in
calendar-home-setandaddressbook-home-setstalwartlabs/stalwart#1796 - 3: https://icalendar.org/CalDAV-Scheduling-RFC-6638/9-2-caldav-schedule-default-calendar-url-property.html
- 4: https://icalendar.org/CalDAV-Scheduling-RFC-6638/4-3-default-calendar-collection.html
- 5: [Bug]: [CalDAV]
CALDAV:schedule-default-calendar-URLis set on the principal instead of the scheduling inbox nextcloud/server#57883 - 6: https://datatracker.ietf.org/doc/html/rfc6638
- 7: https://datatracker.ietf.org/doc/html/rfc4791
🏁 Script executed:
# Check lines around 471 in CalendarService.ts to verify the Error handling
head -n 480 packages/lib/CalendarService.ts | tail -n 30Repository: calcom/cal.diy
Length of output: 1332
🏁 Script executed:
# Check imports and error handling patterns in CalendarService.ts
head -n 50 packages/lib/CalendarService.ts | grep -E "^import|ErrorWithCode|TRPCError"Repository: calcom/cal.diy
Length of output: 664
🏁 Script executed:
# Search for ErrorWithCode usage in the codebase to confirm the pattern
rg "ErrorWithCode" --type ts --type tsx -A 2 -B 2 | head -n 30Repository: calcom/cal.diy
Length of output: 85
🏁 Script executed:
# Search for ErrorWithCode import pattern and usage
rg "ErrorWithCode" --type ts -A 2 -B 2 | head -n 50Repository: calcom/cal.diy
Length of output: 3255
🏁 Script executed:
# Check if ErrorWithCode is used in lib files (non-tRPC context)
rg "ErrorWithCode" packages/lib --type tsRepository: calcom/cal.diy
Length of output: 2295
🏁 Script executed:
# Verify if CalendarService.ts uses ErrorWithCode anywhere
grep -n "ErrorWithCode\|import.*Error" packages/lib/CalendarService.tsRepository: calcom/cal.diy
Length of output: 40
🏁 Script executed:
# Check how other errors are thrown in CalendarService.ts
rg "throw new Error|throw.*Error" packages/lib/CalendarService.ts | head -n 20Repository: calcom/cal.diy
Length of output: 261
Use ErrorWithCode instead of new Error, and prefer the standard CALDAV:schedule-default-calendar-URL property for identifying the primary calendar.
The fix correctly avoids writing to all discovered collections, but has two issues:
-
Error type: Line 471 throws
new Error(...), violating the guideline to useErrorWithCodein non-tRPC lib files. Refactor toErrorWithCodewith an appropriate error code. -
Calendar selection fallback:
calendars.slice(0, 1)assumes the server returns the primary calendar first, but RFC 4791 (CalDAV) does not guarantee PROPFIND response order. Per RFC 6638 Section 9.2, servers expose the default calendar via theCALDAV:schedule-default-calendar-URLproperty (typically on the scheduling inbox collection discovered viaCALDAV:schedule-inbox-URL). Iftsdavsurfaces this property, prefer it over positional fallback. If not available, at least log which calendar URL was chosen when falling back to the first one, so misrouted writes are debuggable.
Note: The behavior change is intentional — previously an unmatched externalId produced an empty list and Promise.all([]) resolved silently; now it throws and triggers the compensating-update pattern (per RegularBookingService), potentially flipping a booking to PENDING. More correct, but user-visible.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/lib/CalendarService.ts` around lines 466 - 472, Replace the thrown
plain Error with an ErrorWithCode (e.g., new ErrorWithCode("No target calendars
found to create CalDAV calendar entry", "CALDAV:no-target-calendar")) where the
current check on targetCalendars fails; when selecting the primary calendar
prefer the CALDAV:schedule-default-calendar-URL property if exposed by tsdav
(inspect the collection metadata returned for a schedule-default-calendar-URL
and match that URL against calendars to build targetCalendars) and only if that
property is not available fall back to picking the first calendar from calendars
but emit a debug/info log that includes the chosen calendar URL (log the
selected calendar's href/externalId) so misrouted writes are traceable; update
the logic around mainHostDestinationCalendar, calendars and targetCalendars to
implement these checks and the ErrorWithCode replacement.
|
Superseded by #28943 which includes the CodeRabbit fixes. |
1 participant
What does this PR do?
When "Add to calendar" is set to "Default" in the CalDAV app, no calendar entry was created on booking even though confirmation emails were sent correctly.
Root cause: the filter in
BaseCalendarService.createEventfell through with: truewhen nodestinationCalendarexternalId was set, causingcreateCalendarObjectto fire against every discovered calendar collection. Servers like GMX silently drop these writes so no entry is ever created.Fix: replace
: truewithcalendars.slice(0, 1)— use only the first (primary) calendar when no explicit destination is configured. The explicit-calendar path is unchanged.Mandatory Tasks
How should this be tested?
Checklist
Screen.Recording.2026-03-25.at.8.18.49.PM.mov
Supersedes: #28934