fix(caldav): default calendar not written on booking#28943
fix(caldav): default calendar not written on booking#28943Harshithk951 wants to merge 2 commits intocalcom:mainfrom
Conversation
- Add isDefault flag to IntegrationCalendar type - Resolve CALDAV:schedule-default-calendar-URL via RFC 6638 §9.2 - Fall back to calendar-user-address-set with debug logging - Replace new Error() with ErrorWithCode per project conventions - Cache DAVClient instance to avoid re-initialization per request - Fix pre-existing bug: this.credentials.type → this.credential.type - Fix null-safety on getUserTimezoneFromDB return value Fixes #<issue-number>
|
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! |
📝 WalkthroughWalkthroughThe changes introduce CalDAV client support to the CalendarService, including RFC 6638–driven default calendar resolution via principal URL and 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/lib/CalendarService.ts (1)
457-458:⚠️ Potential issue | 🟡 MinorLeftover
new Error(...)contradicts the PR description.The PR summary states "Replace generic
new Error()withErrorWithCodeto follow project conventions," but these two call sites were missed. They're in this file and will throw plainErrorrather than a coded one, so downstreamcode-based branching won't see them.🛠 Suggested fix
- if (error || !iCalString) - throw new Error(`Error creating iCalString:=> ${error?.message} : ${error?.name} `); + if (error || !iCalString) + throw new ErrorWithCode( + ErrorCode.InternalServerError, + `Error creating iCalString: ${error?.message} : ${error?.name}` + );- if (responses.some((r) => !r.ok)) { - throw new Error( - `Error creating event: ${(await Promise.all(responses.map((r) => r.text()))).join(", ")}` - ); - } + if (responses.some((r) => !r.ok)) { + throw new ErrorWithCode( + ErrorCode.InternalServerError, + `Error creating event: ${(await Promise.all(responses.map((r) => r.text()))).join(", ")}` + ); + }As per coding guidelines: "Use
ErrorWithCodefor errors in non-tRPC files (services, repositories, utilities)".Also applies to: 514-518
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lib/CalendarService.ts` around lines 457 - 458, Replace the plain Error instantiations in CalendarService (e.g., the throw in the code path that currently does `throw new Error(\`Error creating iCalString:=> ${error?.message} : ${error?.name} \`)`) with the project's ErrorWithCode so downstream code can branch on error.code; update the throw to use ErrorWithCode with a meaningful code (e.g., "ICAL_CREATION_FAILED") and preserve the original message text, and ensure ErrorWithCode is imported/available in the CalendarService module; apply the same replacement for the other matching call site(s) in this file (the block around the comment referencing lines 514-518) so all non-trpc service errors use ErrorWithCode.
🧹 Nitpick comments (2)
packages/lib/CalendarService.ts (1)
847-862:resolveDefaultCalendarUrlruns on everylistCalendars()call — consider memoizing.
listCalendars()is called on the hot path (availability, create/update/delete flows,getEventsByUID, etc.), andresolveDefaultCalendarUrl()issues up to two PROPFIND requests against the principal each time. For a single booking you can easily fan out to 3–5 extra round-trips that return the same answer.Since you already cache
this.client, cache the resolved URL alongside it (e.g.,private defaultCalendarUrl?: string | nullinitialized lazily), and invalidate together if you ever reset the client.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lib/CalendarService.ts` around lines 847 - 862, listCalendars currently calls resolveDefaultCalendarUrl on every invocation which issues extra PROPFINDs; add a memoized property (e.g., private defaultCalendarUrl?: string | null) on the class, have resolveDefaultCalendarUrl check and return the cached value when present, set the cached value the first time you successfully resolve the URL, and ensure you clear/invalidate defaultCalendarUrl wherever the client is reset (same places you reset this.client or in getClient) so the cache stays consistent with the cached client.packages/lib/__tests__/CalendarService.caldav-defaults.test.ts (1)
36-40: Replaceanywith typed helpers.Per the repo guideline "Never use
as any- use proper type-safe solutions instead," thecredential: any,as anyoncreateDAVClient, andevent: anyusages should be typed — e.g.,credential: CredentialPayload, andvi.mocked(createDAVClient).mockResolvedValue(mockClient as unknown as DAVClient)(or a narrowly-typed partial). Keeps the test honest about the contract it exercises.As per coding guidelines: "Never use
as any- use proper type-safe solutions instead".Also applies to: 67-67, 93-93, 106-106
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/lib/__tests__/CalendarService.caldav-defaults.test.ts` around lines 36 - 40, The test uses untyped "any" for the constructor credential and other mocked values; replace these with proper types to follow the guideline (e.g., change constructor(credential: any) to constructor(credential: CredentialPayload) and type the fake client/event). Update mocks to use vi.mocked(createDAVClient).mockResolvedValue(mockClient as unknown as DAVClient) or mockResolvedValue(mockClient as Partial<DAVClient>) rather than using "as any", and type the test event variable (e.g., event: SomeEventType or Partial<EventType>) so createDAVClient, vi.mocked(...), and event usages are all strongly typed.
🤖 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/__tests__/CalendarService.caldav-defaults.test.ts`:
- Around line 108-118: The test is hitting ics.createEvent before the "no target
calendars" guard because the test event lacks required fields; update the test's
event object used in CalendarService.caldav-defaults.test.ts to include minimal
valid fields (e.g., title and calendarDescription) so ics.createEvent succeeds
and execution reaches CalendarService.createEvent's targetCalendars check, and
replace the string-based throws assertion with stronger checks such as
expect(...).rejects.toBeInstanceOf(ErrorWithCode) and
expect(...).rejects.toMatchObject({ code: ErrorCode.InternalServerError }) to
ensure the guard's ErrorWithCode is asserted.
- Around line 15-22: The test's module mock is targeting the wrong relative path
so the logger mock never replaces the real logger; change the vi.mock call to
target the same module id imported by CalendarService (i.e., mock the package's
logger module that CalendarService imports rather than "./logger" from the
test), and update the mock to export the same default API (getSubLogger, debug,
warn, error). Also fix the event fixture passed into CalendarService.createEvent
(and subsequently ics.createEvent) by adding a non-empty title property (e.g.,
title: "Test Event") so the ICS creation won't error and the test can reach the
intended ErrorWithCode.InternalServerError "No target calendars found..."
assertion.
In `@packages/lib/CalendarService.ts`:
- Around line 867-870: Create a small, type-safe local interface (e.g.,
PropfindPropsResponse with optional props: Record<string, { href?: string } |
undefined>) and replace all `as any` casts by casting to that interface where
the code reads `props` on tsdav responses: replace `(calendar as any).props` in
the isDefault computation, `(principalProps?.[0] as any)` when reading
`schedule-default-calendar-URL` and `(addressSetProps?.[0] as any)` when reading
`calendar-user-address-set` with the new PropfindPropsResponse type; ensure
nullable chaining (?.) is preserved so the href accesses and boolean checks
remain safe and type-checked.
- Around line 1105-1125: Remove the Priority 2 fallback that queries
CALDAV:calendar-user-address-set in resolve default calendar logic: delete the
propfind block that sets addressSetProps, the cast using "as any", and the
fallbackUrl handling (the code that logs and returns fallbackUrl), since
calendar-user-address-set contains mailto: URIs not calendar collection URLs and
will never match the comparison used elsewhere (calendar.url ===
defaultCalendarUrl); rely on the existing schedule-default-calendar-URL path and
the secondary fallback instead.
In `@packages/types/Calendar.d.ts`:
- Around line 7-66: The current diff overly narrows exported calendar types and
removes fields used elsewhere; revert those narrowing changes and instead only
add isDefault?: boolean to IntegrationCalendar. Specifically, restore any
previously exported types (e.g., CalendarFetchMode, SelectedCalendar) and ensure
CalendarEvent/CalendarServiceEvent include calendarDescription (and any other
existing fields consumers use), and IntegrationCalendar retains credentialId and
other original properties while adding the new optional isDefault flag — do not
restructure or remove public type surface in this file.
---
Outside diff comments:
In `@packages/lib/CalendarService.ts`:
- Around line 457-458: Replace the plain Error instantiations in CalendarService
(e.g., the throw in the code path that currently does `throw new Error(\`Error
creating iCalString:=> ${error?.message} : ${error?.name} \`)`) with the
project's ErrorWithCode so downstream code can branch on error.code; update the
throw to use ErrorWithCode with a meaningful code (e.g., "ICAL_CREATION_FAILED")
and preserve the original message text, and ensure ErrorWithCode is
imported/available in the CalendarService module; apply the same replacement for
the other matching call site(s) in this file (the block around the comment
referencing lines 514-518) so all non-trpc service errors use ErrorWithCode.
---
Nitpick comments:
In `@packages/lib/__tests__/CalendarService.caldav-defaults.test.ts`:
- Around line 36-40: The test uses untyped "any" for the constructor credential
and other mocked values; replace these with proper types to follow the guideline
(e.g., change constructor(credential: any) to constructor(credential:
CredentialPayload) and type the fake client/event). Update mocks to use
vi.mocked(createDAVClient).mockResolvedValue(mockClient as unknown as DAVClient)
or mockResolvedValue(mockClient as Partial<DAVClient>) rather than using "as
any", and type the test event variable (e.g., event: SomeEventType or
Partial<EventType>) so createDAVClient, vi.mocked(...), and event usages are all
strongly typed.
In `@packages/lib/CalendarService.ts`:
- Around line 847-862: listCalendars currently calls resolveDefaultCalendarUrl
on every invocation which issues extra PROPFINDs; add a memoized property (e.g.,
private defaultCalendarUrl?: string | null) on the class, have
resolveDefaultCalendarUrl check and return the cached value when present, set
the cached value the first time you successfully resolve the URL, and ensure you
clear/invalidate defaultCalendarUrl wherever the client is reset (same places
you reset this.client or in getClient) so the cache stays consistent with the
cached client.
🪄 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: 569f580f-e3c3-45be-8203-14ab9f3fc790
📒 Files selected for processing (3)
packages/lib/CalendarService.tspackages/lib/__tests__/CalendarService.caldav-defaults.test.tspackages/types/Calendar.d.ts
| vi.mock("./logger", () => ({ | ||
| default: { | ||
| getSubLogger: vi.fn().mockReturnThis(), | ||
| debug: vi.fn(), | ||
| warn: vi.fn(), | ||
| error: vi.fn(), | ||
| }, | ||
| })); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Vitest, is a relative path passed to vi.mock() resolved relative to the test file or the file under test?
💡 Result:
In Vitest, a relative path passed to vi.mock is resolved relative to the test file (the file containing the vi.mock call), not relative to the file under test.
Citations:
- 1: Mock with a relative path does not work vitest-dev/vitest#4927
- 2: mocking a module with relative path alias is not working vitest-dev/vitest#3081
🏁 Script executed:
head -120 packages/lib/__tests__/CalendarService.caldav-defaults.test.ts | tail -25Repository: calcom/cal.diy
Length of output: 988
🏁 Script executed:
# Find and examine the CalendarService.ts file to see how it handles the event object
find . -name "CalendarService.ts" -type f | head -5Repository: calcom/cal.diy
Length of output: 358
🏁 Script executed:
# Check the test file to see which CalendarService it imports
grep -n "import.*CalendarService\|from.*CalendarService" packages/lib/__tests__/CalendarService.caldav-defaults.test.tsRepository: calcom/cal.diy
Length of output: 113
🏁 Script executed:
# Examine the CalendarService.ts file in packages/lib/ to see how createEvent handles the event object
cat -n packages/lib/CalendarService.ts | head -500Repository: calcom/cal.diy
Length of output: 21542
🏁 Script executed:
# Check other tests or usages of ics.createEvent to see if title is required
rg "createEvent" packages/lib/__tests__/ -A 5 -B 2Repository: calcom/cal.diy
Length of output: 704
🏁 Script executed:
# Search for other usages of ics.createEvent to see the typical structure
rg "createEvent\(" packages/lib/CalendarService.ts -A 15 | head -40Repository: calcom/cal.diy
Length of output: 1780
vi.mock("./logger") resolves to the wrong file — the mock is inert.
vi.mock(path) resolves path relative to the importing file (the test). From packages/lib/__tests__/CalendarService.caldav-defaults.test.ts, ./logger resolves to packages/lib/__tests__/logger, which doesn't exist. CalendarService.ts imports ./logger (resolving to packages/lib/logger.ts), so the mock never attaches to that module and the real logger is used at runtime.
🛠 Fix
-vi.mock("./logger", () => ({
+vi.mock("../logger", () => ({
default: {
getSubLogger: vi.fn().mockReturnThis(),
debug: vi.fn(),
warn: vi.fn(),
error: vi.fn(),
},
}));Event object missing title field — test will throw wrong error.
The event object (lines 108-117) lacks a title property. When CalendarService.createEvent calls the ics.createEvent function at line 438 with title: undefined, it returns an error. The code throws a generic Error at line 458 instead of reaching the "No target calendars" check at line 493. The test expects ErrorWithCode.InternalServerError with message "No target calendars found..." but will receive a different error first.
Add title: "Test Event" (or similar) to the event object:
const event: any = {
startTime: "2024-01-01T10:00:00Z",
endTime: "2024-01-01T11:00:00Z",
organizer: { timeZone: "UTC", email: "m@e.com" },
attendees: [],
+ title: "Test Event",
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/lib/__tests__/CalendarService.caldav-defaults.test.ts` around lines
15 - 22, The test's module mock is targeting the wrong relative path so the
logger mock never replaces the real logger; change the vi.mock call to target
the same module id imported by CalendarService (i.e., mock the package's logger
module that CalendarService imports rather than "./logger" from the test), and
update the mock to export the same default API (getSubLogger, debug, warn,
error). Also fix the event fixture passed into CalendarService.createEvent (and
subsequently ics.createEvent) by adding a non-empty title property (e.g., title:
"Test Event") so the ICS creation won't error and the test can reach the
intended ErrorWithCode.InternalServerError "No target calendars found..."
assertion.
| const event: any = { | ||
| startTime: "2024-01-01T10:00:00Z", | ||
| endTime: "2024-01-01T11:00:00Z", | ||
| organizer: { timeZone: "UTC", email: "m@e.com" }, | ||
| attendees: [], | ||
| }; | ||
|
|
||
| await expect(service.createEvent(event, 1)).rejects.toThrow( | ||
| new ErrorWithCode(ErrorCode.InternalServerError, "No target calendars found to create CalDAV calendar entry") | ||
| ); | ||
| }); |
There was a problem hiding this comment.
This test will not exercise the "no target calendars" path — it will throw earlier.
The event object lacks a title (and calendarDescription). In createEvent() the ics.createEvent({...}) call runs before the targetCalendars.length === 0 guard, and the ics library rejects missing/empty titles with an error, causing throw new Error("Error creating iCalString:=> ...") at line 458 of CalendarService.ts. That's a plain Error, not ErrorWithCode(InternalServerError, "No target calendars found..."), so the rejects.toThrow(...) assertion will match on the message substring and falsely suggest the intended guard fired.
🛠 Suggested fix: give the event the minimum fields required to pass ics validation, so we actually reach the guard
const event: any = {
+ type: "test-caldav",
+ title: "Test Event",
startTime: "2024-01-01T10:00:00Z",
endTime: "2024-01-01T11:00:00Z",
organizer: { timeZone: "UTC", email: "m@e.com" },
attendees: [],
};Also consider asserting on the error instance directly to avoid accidental message-substring matches:
await expect(service.createEvent(event, 1)).rejects.toBeInstanceOf(ErrorWithCode);
await expect(service.createEvent(event, 1)).rejects.toMatchObject({
code: ErrorCode.InternalServerError,
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const event: any = { | |
| startTime: "2024-01-01T10:00:00Z", | |
| endTime: "2024-01-01T11:00:00Z", | |
| organizer: { timeZone: "UTC", email: "m@e.com" }, | |
| attendees: [], | |
| }; | |
| await expect(service.createEvent(event, 1)).rejects.toThrow( | |
| new ErrorWithCode(ErrorCode.InternalServerError, "No target calendars found to create CalDAV calendar entry") | |
| ); | |
| }); | |
| const event: any = { | |
| type: "test-caldav", | |
| title: "Test Event", | |
| startTime: "2024-01-01T10:00:00Z", | |
| endTime: "2024-01-01T11:00:00Z", | |
| organizer: { timeZone: "UTC", email: "m@e.com" }, | |
| attendees: [], | |
| }; | |
| await expect(service.createEvent(event, 1)).rejects.toThrow( | |
| new ErrorWithCode(ErrorCode.InternalServerError, "No target calendars found to create CalDAV calendar entry") | |
| ); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/lib/__tests__/CalendarService.caldav-defaults.test.ts` around lines
108 - 118, The test is hitting ics.createEvent before the "no target calendars"
guard because the test event lacks required fields; update the test's event
object used in CalendarService.caldav-defaults.test.ts to include minimal valid
fields (e.g., title and calendarDescription) so ics.createEvent succeeds and
execution reaches CalendarService.createEvent's targetCalendars check, and
replace the string-based throws assertion with stronger checks such as
expect(...).rejects.toBeInstanceOf(ErrorWithCode) and
expect(...).rejects.toMatchObject({ code: ErrorCode.InternalServerError }) to
ensure the guard's ErrorWithCode is asserted.
| const isDefault = defaultCalendarUrl | ||
| ? calendar.url === defaultCalendarUrl | ||
| : !!(calendar as any).props?.["schedule-default-calendar-URL"]; // fallback: check on the calendar object itself | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm tsdav exposes a typed propfind response we can lean on
rg -nP 'propfind' node_modules/tsdav/dist 2>/dev/null | head -20 || \
rg -nP 'propfind' --type=ts -C2 | head -40Repository: calcom/cal.diy
Length of output: 40
🏁 Script executed:
# First, let's look at the CalendarService.ts file at the flagged lines
head -900 packages/lib/CalendarService.ts | tail -50Repository: calcom/cal.diy
Length of output: 2569
🏁 Script executed:
# Also check the broader context around line 1099 and 1115
sed -n '1085,1130p' packages/lib/CalendarService.tsRepository: calcom/cal.diy
Length of output: 1538
🏁 Script executed:
# Check what tsdav exports related to propfind or types
find node_modules/tsdav -name "*.d.ts" 2>/dev/null | head -5 || \
find node_modules/tsdav -name "index.ts" -o -name "types.ts" 2>/dev/nullRepository: calcom/cal.diy
Length of output: 40
🏁 Script executed:
# Search for type definitions in tsdav
cat node_modules/tsdav/package.json | grep -E '"types"|"main"|"exports"' 2>/dev/null || echo "Check tsdav structure"Repository: calcom/cal.diy
Length of output: 160
🏁 Script executed:
# Look for propfind related types in tsdav dist
find node_modules/tsdav/dist -type f -name "*.d.ts" 2>/dev/null | head -10Repository: calcom/cal.diy
Length of output: 40
🏁 Script executed:
# Check imports and type definitions at top of CalendarService.ts
head -100 packages/lib/CalendarService.tsRepository: calcom/cal.diy
Length of output: 3442
🏁 Script executed:
# Search for DAVCalendar type usage and related types in the file
rg -n "type |interface " packages/lib/CalendarService.ts | head -20Repository: calcom/cal.diy
Length of output: 414
🏁 Script executed:
# Check if tsdav is listed in package.json dependencies
rg -A 5 -B 5 "tsdav" package.jsonRepository: calcom/cal.diy
Length of output: 40
Remove as any casts — define a type-safe shape for propfind responses.
The code uses as any at lines 869, 1099, and 1115 to access props on tsdav responses, directly violating the guideline "Never use as any - use proper type-safe solutions instead."
Define a local type to capture the expected shape of the propfind response instead:
type PropfindPropsResponse = {
props?: Record<string, { href?: string } | undefined>;
};
// At line 1099
const defaultUrl = (principalProps?.[0] as PropfindPropsResponse)?.props?.["schedule-default-calendar-URL"]?.href;
// At line 1115
const fallbackUrl = (addressSetProps?.[0] as PropfindPropsResponse)?.props?.["calendar-user-address-set"]?.href;
// At line 869
const isDefault = defaultCalendarUrl
? calendar.url === defaultCalendarUrl
: !!(calendar as PropfindPropsResponse).props?.["schedule-default-calendar-URL"];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/lib/CalendarService.ts` around lines 867 - 870, Create a small,
type-safe local interface (e.g., PropfindPropsResponse with optional props:
Record<string, { href?: string } | undefined>) and replace all `as any` casts by
casting to that interface where the code reads `props` on tsdav responses:
replace `(calendar as any).props` in the isDefault computation,
`(principalProps?.[0] as any)` when reading `schedule-default-calendar-URL` and
`(addressSetProps?.[0] as any)` when reading `calendar-user-address-set` with
the new PropfindPropsResponse type; ensure nullable chaining (?.) is preserved
so the href accesses and boolean checks remain safe and type-checked.
| // Priority 2: CALDAV:calendar-user-address-set (Fallback) | ||
| const addressSetProps = await client.propfind({ | ||
| url: principalUrl, | ||
| props: [{ | ||
| name: "calendar-user-address-set", | ||
| namespace: DAVNamespace.CALDAV, | ||
| }], | ||
| depth: "0", | ||
| }); | ||
|
|
||
| const fallbackUrl = (addressSetProps?.[0] as any)?.props?.["calendar-user-address-set"]?.href; | ||
| if (fallbackUrl) { | ||
| this.log.debug(`CalDAV: Found default calendar via calendar-user-address-set: ${fallbackUrl}`); | ||
| return fallbackUrl; | ||
| } | ||
|
|
||
| return undefined; | ||
| } catch (e) { | ||
| this.log.warn("CalDAV: Could not resolve default calendar URL from inbox/principal", e); | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Per RFC 6638, what is the semantic of the CALDAV:calendar-user-address-set property and what value types does it hold (mailto URI vs calendar collection URL)?
💡 Result:
According to RFC 6638, Section 2.4.1, the CALDAV:calendar-user-address-set property identifies the calendar addresses of the associated principal resource. It is a multivalued property defined as , containing one or more DAV:href elements. These href values are URIs that represent the calendar addresses, such as mailto: URIs (e.g., mailto:bernard@example.com). The RFC example uses mailto: URIs exclusively. While DAV:href allows any URI (including http/https URLs for calendar collections or principal URIs), the semantic and normative example specify mailto: URIs as the primary type for calendar user addresses, consistent with iCalendar CAL-ADDRESS (RFC 5545), which requires mailto: for email transports. The description notes that if no well-defined identifier exists, the principal resource URI can be used as a fallback, but mailto: URIs are the standard value type shown.
Citations:
- 1: https://datatracker.ietf.org/doc/html/rfc6638
- 2: https://icalendar.org/CalDAV-Scheduling-RFC-6638/2-4-1-caldav-calendar-user-address-set-property.html
- 3: https://www.rfc-editor.org/rfc/rfc6638
- 4: https://rfc-editor.org/rfc/rfc6638.html
- 5: https://tools.ietf.org/html/rfc6638.html
🏁 Script executed:
# First, locate and examine the CalendarService.ts file
fd -t f -e ts CalendarService
# Get the file size to determine best approach
wc -l packages/lib/CalendarService.ts 2>/dev/null || trueRepository: calcom/cal.diy
Length of output: 1180
🏁 Script executed:
# Read the specific lines mentioned in the review
sed -n '1105,1125p' packages/lib/CalendarService.tsRepository: calcom/cal.diy
Length of output: 785
🏁 Script executed:
# Check for `as any` casts in the file
rg 'as any' packages/lib/CalendarService.ts -n
# Also check the broader context of resolveDefaultCalendarUrl to understand impact
rg -A 5 -B 5 'resolveDefaultCalendarUrl' packages/lib/CalendarService.ts | head -100Repository: calcom/cal.diy
Length of output: 1553
🏁 Script executed:
# Check the context around line 869 to see how defaultCalendarUrl is used
sed -n '860,875p' packages/lib/CalendarService.tsRepository: calcom/cal.diy
Length of output: 896
Remove the calendar-user-address-set fallback — it returns the wrong type and will never match.
Per RFC 6638 §2.4.1, CALDAV:calendar-user-address-set is a set of URIs that identify the calendar user (typically mailto:user@example.com), not the URL of a calendar collection. The code at line 1115 casts this to defaultCalendarUrl and compares it to calendar.url at line 869 using calendar.url === defaultCalendarUrl. Since calendar.url is always an https:// URL and the fallback returns mailto: URIs, this comparison will never match — the fallback is dead code that silently fails.
Additionally: The as any cast at line 1115 violates the coding guideline "Never use as any — use proper type-safe solutions instead."
Suggested fix: Remove the entire Priority 2 fallback block (lines 1105–1119) and let line 869's secondary fallback handle the case when no schedule-default-calendar-URL is found.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/lib/CalendarService.ts` around lines 1105 - 1125, Remove the
Priority 2 fallback that queries CALDAV:calendar-user-address-set in resolve
default calendar logic: delete the propfind block that sets addressSetProps, the
cast using "as any", and the fallbackUrl handling (the code that logs and
returns fallbackUrl), since calendar-user-address-set contains mailto: URIs not
calendar collection URLs and will never match the comparison used elsewhere
(calendar.url === defaultCalendarUrl); rely on the existing
schedule-default-calendar-URL path and the secondary fallback instead.
| export type Calendar = { | ||
| listCalendars(event?: CalendarEvent): Promise<IntegrationCalendar[]>; | ||
| createEvent(event: CalendarServiceEvent, credentialId: number): Promise<NewCalendarEventType>; | ||
| updateEvent(uid: string, event: CalendarEvent): Promise<NewCalendarEventType | NewCalendarEventType[]>; | ||
| deleteEvent(uid: string, event: CalendarEvent): Promise<void>; | ||
| getAvailability(params: GetAvailabilityParams): Promise<EventBusyDate[]>; | ||
| getAccountEmail(): string; | ||
| }; | ||
|
|
||
| export type EventBusyDate = { | ||
| start: Date | string; | ||
| end: Date | string; | ||
| source?: string | null; | ||
| timeZone?: string; | ||
| export type IntegrationCalendar = { | ||
| id: string; | ||
| externalId: string; | ||
| name: string; | ||
| primary?: boolean; | ||
| readOnly?: boolean; | ||
| email?: string; | ||
| integration: string; | ||
| userId?: number | null; | ||
| isDefault?: boolean; | ||
| }; | ||
|
|
||
| export type EventBusyDetails = EventBusyDate & { | ||
| title?: string; | ||
| source: string; | ||
| userId?: number | null; | ||
| export type CalendarEvent = { | ||
| type: string; | ||
| title: string; | ||
| description?: string; | ||
| startTime: string; | ||
| endTime: string; | ||
| organizer: Person; | ||
| attendees: Person[]; | ||
| location?: string; | ||
| uid?: string; | ||
| destinationCalendar?: IntegrationCalendar[]; | ||
| team?: { | ||
| members: Person[]; | ||
| }; | ||
| hideCalendarEventDetails?: boolean; | ||
| additionalInformation?: string; | ||
| }; | ||
|
|
||
| export type AdditionalInfo = Record<string, unknown> & { calWarnings?: string[] }; | ||
| export type CalendarServiceEvent = CalendarEvent; | ||
|
|
||
| export type NewCalendarEventType = { | ||
| uid: string; | ||
| id: string; | ||
| thirdPartyRecurringEventId?: string | null; | ||
| type: string; | ||
| password: string; | ||
| url: string; | ||
| additionalInfo: AdditionalInfo; | ||
| iCalUID?: string | null; | ||
| location?: string | null; | ||
| hangoutLink?: string | null; | ||
| conferenceData?: ConferenceData; | ||
| delegatedToId?: string | null; | ||
| password?: string; | ||
| url?: string; | ||
| additionalInfo?: Record<string, unknown>; | ||
| }; | ||
|
|
||
| export type EventBusyDate = { | ||
| start: string; | ||
| end: string; | ||
| }; | ||
|
|
||
| export type GetAvailabilityParams = { | ||
| dateFrom: string; | ||
| dateTo: string; | ||
| selectedCalendars: IntegrationCalendar[]; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the extent of the breakage across the codebase.
echo "=== CalendarFetchMode usages ==="
rg -nP '\bCalendarFetchMode\b' --type=ts -C1
echo "=== SelectedCalendar type usages (excluding prisma schema) ==="
rg -nP "from ['\"]\@calcom/types/Calendar['\"]" --type=ts -A3 | rg -n 'SelectedCalendar\b' | head -50
echo "=== credentialId on IntegrationCalendar ==="
rg -nP '\bcredentialId\b' --type=ts -C1 packages/lib/CalendarService.ts
echo "=== calendarDescription on CalendarEvent ==="
rg -nP '\bcalendarDescription\b' --type=ts -C1Repository: calcom/cal.diy
Length of output: 12472
Type narrowing removes fields and exports used throughout the codebase, breaking this PR's own code.
The PR objective is to add isDefault to IntegrationCalendar, but this file aggressively strips the public type surface. Concrete breakages confirmed:
-
Breaks
CalendarService.ts(this PR):- Line 444 reads
event.calendarDescriptionon aCalendarServiceEvent/CalendarEvent, but this field is not declared in the narrowed types. - Line 466-467 reads
cal.credentialIdonIntegrationCalendarobjects withinevent.destinationCalendar, but the new shape has nocredentialIdfield.
- Line 444 reads
-
Breaks imports across the codebase:
CalendarFetchModeis imported in 8+ files:packages/app-store/_utils/getCalendar.ts,packages/trpc/server/routers/viewer/slots/util.ts,packages/features/calendars/lib/getCalendarsEvents.ts,packages/features/calendars/lib/CalendarManager.ts,packages/features/bookings/lib/handleNewBooking/ensureAvailableUsers.ts,packages/features/calendar-subscription/lib/telemetry/CalendarTelemetryWrapper.ts,packages/features/busyTimes/services/getBusyTimes.ts, andpackages/features/availability/lib/getUserAvailability.ts.SelectedCalendaris imported inpackages/features/calendars/lib/getCalendarsEvents.ts.calendarDescriptionis actively used by calendar service implementations across all integrations (Google, Zoho, Office365, Lark, Feishu).
Restrict changes on this file to just adding isDefault?: boolean to IntegrationCalendar. Type surface cleanup should be a separate PR to isolate scope and review impact independently.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/types/Calendar.d.ts` around lines 7 - 66, The current diff overly
narrows exported calendar types and removes fields used elsewhere; revert those
narrowing changes and instead only add isDefault?: boolean to
IntegrationCalendar. Specifically, restore any previously exported types (e.g.,
CalendarFetchMode, SelectedCalendar) and ensure
CalendarEvent/CalendarServiceEvent include calendarDescription (and any other
existing fields consumers use), and IntegrationCalendar retains credentialId and
other original properties while adding the new optional isDefault flag — do not
restructure or remove public type surface in this file.
1 participant
Fixes #28934
What does this PR do?
This PR refactors the CalDAV calendar service to ensure that bookings are written to the correct user-designated "default" calendar when no destination is explicitly provided.
Key improvements:
CALDAV:schedule-default-calendar-URLproperty on the user's principal/inbox.CALDAV:calendar-user-address-setand includes debug logging to track exactly which calendar URL was resolved.new Error()with the codebase-standardErrorWithCodefor better reporting in non-tRPC library files.DAVClientinstance within the service lifecycle to prevent redundant account re-initializations during a single request.credentialsvscredential) in the event update flow and ensuresnullsafety for timezone lookups.Visual Demo (For contributors especially)
Image Demo:
Automated tests have been implemented and verified.
✓ BaseCalendarService - CalDAV Default Calendar Selection (3 tests passed)✓ CalendarService Logic Tests (25 tests passed)Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
externalIdfor the destination calendar.CalDAV: Using schedule-default-calendar-URL calendar: [URL].Checklist