fix: avoid duplicate emails from the organizer's calendar by setting schedule agent to client#21095
fix: avoid duplicate emails from the organizer's calendar by setting schedule agent to client#21095mitchellderijcke wants to merge 9 commits intocalcom:mainfrom
Conversation
|
|
|
@rijx is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
github-actions
bot
added
$500
caldav
dosubot
bot
added
the
emails
mitchellderijcke
force-pushed
the
fix-caldav-emails
branch
from
df7f6d8 to
1335e95
Compare
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (05/03/25)1 reviewer was added to this PR based on Keith Williams's automation. "Add community label" took an action on this PR • (05/03/25)1 label was added to this PR based on Keith Williams's automation. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
mrge found 1 issue across 3 files. View it in mrge.io
mitchellderijcke
changed the title
Unfortunately I had to fork A) get schedule agent support (Open PR, no replies yet: adamgibbons/ics#289) I also opened a PR for Considering how simple the files actually are (you can check the test, it contains the expected file contents) it wouldn't actually be crazy to integrate the code considering that it's a core functionality and we would benefit from having complete control. Happy to implement any of the alternatives if there's a preference for one. |
I implemented this template and I'm actually rather pleased by its simplicity now that there's no abstraction object between So that might actually be the way to go, at least until we decide to whether to migrate other instances of |
|
This PR is being marked as stale due to inactivity. |
|
This PR is being marked as stale due to inactivity. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe PR switches ICS generation from the ics package to ics2 in CalendarService and updates attendee mapping to include rsvp: false and scheduleAgent: "CLIENT". It adds ics2 as a dependency. A new test suite (CalendarService.test.ts) mocks crypto, tsdav, uuid, and parsing utilities to validate ICS input creation and CalDAV create/update flows. Tests check that createEvent is called with expected inputs and that createCalendarObject/updateCalendarObject receive correct payloads, including deterministic UID handling in tests and verification of returned event object structures. Assessment against linked issues
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/lib/CalendarService.ts (2)
273-279: Bug: wrong event type returned in updateEvent.
this.credentialsdoes not have atypefield; should mirror createEvent and usethis.integrationName.Fix:
- type: this.credentials.type, + type: this.integrationName,
703-709: Incorrect object URL when fetching by UID (missing slash).
${cal.externalId}${uid}.icswill produce …/calendar.ics for bases without trailing slash.Patch:
- const calEvents = await this.getEvents(cal.externalId, null, null, [`${cal.externalId}${uid}.ics`]); + const base = cal.externalId.replace(/\/$/, ""); + const calEvents = await this.getEvents(cal.externalId, null, null, [`${base}/${uid}.ics`]);Optionally update tests/mocks to reflect the object URL shape (…/calendar/.ics).
🧹 Nitpick comments (4)
packages/lib/CalendarService.ts (1)
136-147: De-duplicate attendees by email to avoid repeated ATTENDEE lines.If an attendee is both in event.attendees and team.members, duplicates will be emitted.
Apply:
private getAttendees(event: CalendarEvent) { - const attendees = mapAttendees(event.attendees); - - if (event.team?.members) { - const teamAttendeesWithoutCurrentUser = event.team.members.filter( - (member) => member.email !== this.credential.user?.email - ); - attendees.push(...mapAttendees(teamAttendeesWithoutCurrentUser)); - } - - return attendees; + const combined = + [ + ...event.attendees, + ...(event.team?.members?.filter((m) => m.email !== this.credential.user?.email) ?? []), + ] as (AttendeeInCalendarEvent | TeamMember)[]; + + const seen = new Set<string>(); + const deduped = combined.filter((a) => { + const key = (a.email ?? "").toLowerCase(); + if (!key || seen.has(key)) return false; + seen.add(key); + return true; + }); + + return mapAttendees(deduped); }packages/lib/CalendarService.test.ts (3)
65-87: Brittle full-ICS snapshot; assert essentials instead.Full string snapshots (PRODID, folding, header order) are upgrade-fragile. Prefer asserting critical fields (UID, DTSTART, DURATION, ORGANIZER, and SCHEDULE-AGENT on ATTENDEE).
Example:
- iCalString: getExpectedICS({ title: "Test Event" }), + iCalString: expect.stringContaining("SCHEDULE-AGENT=CLIENT"),And separately assert UID/DTSTART/DURATION with targeted matchers.
177-186: Also assert METHOD is stripped before upload (RFC 4791).You already strip METHOD in code; add an explicit test.
Apply:
expect(createCalendarObject).toHaveBeenCalledWith({ calendar: { url: "https://caldevtest/calendar", }, headers: { authorization: "MOCK", }, filename: "00000000-0000-0000-0000-000000000001.ics", iCalString: getExpectedICS({ title: "Test Event" }), }); + + const uploaded = vi.mocked(createCalendarObject).mock.calls[0][0].iCalString as string; + expect(uploaded).not.toMatch(/METHOD:/);
225-234: Mirror METHOD stripping assertion on update.Ensure updates also upload without METHOD.
Apply:
expect(updateCalendarObject).toHaveBeenCalledWith({ calendarObject: { data: getExpectedICS({ title: "NEW TITLE" }), etag: "testetag", url: "https://caldevtest/calendar", }, headers: { authorization: "MOCK", }, }); + + const updated = vi.mocked(updateCalendarObject).mock.calls[0][0].calendarObject.data as string; + expect(updated).not.toMatch(/METHOD:/);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
packages/lib/CalendarService.test.ts(1 hunks)packages/lib/CalendarService.ts(2 hunks)packages/lib/package.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
**/*.ts: For Prisma queries, only select data you need; never useinclude, always useselect
Ensure thecredential.keyfield is never returned from tRPC endpoints or APIs
Files:
packages/lib/CalendarService.test.tspackages/lib/CalendarService.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Flag excessive Day.js use in performance-critical code; prefer native Date or Day.js
.utc()in hot paths like loops
Files:
packages/lib/CalendarService.test.tspackages/lib/CalendarService.ts
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
Flag default exports and encourage named exports. Named exports provide better tree-shaking, easier refactoring, and clearer imports. Exempt main components like pages, layouts, and components that serve as the primary export of a module.
Files:
packages/lib/CalendarService.test.tspackages/lib/CalendarService.ts
**/*Service.ts
📄 CodeRabbit inference engine (.cursor/rules/review.mdc)
Service files must include
Servicesuffix, use PascalCase matching exported class, and avoid generic names (e.g.,MembershipService.ts)
Files:
packages/lib/CalendarService.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: anglerfishlyy
PR: calcom/cal.com#0
File: :0-0
Timestamp: 2025-08-27T16:39:38.156Z
Learning: anglerfishlyy successfully implemented CAL-3076 email invitation feature for Cal.com team event-types in PR #23312. The feature allows inviting people via email directly from assignment flow, with automatic team invitation if email doesn't belong to existing team member. Implementation includes Host type modifications (userId?: number, email?: string, isPending?: boolean), CheckedTeamSelect component updates with CreatableSelect, TRPC schema validation with zod email validation, and integration with existing teamInvite system.
Learnt from: din-prajapati
PR: calcom/cal.com#21854
File: packages/app-store/office365calendar/__tests__/unit_tests/SubscriptionManager.test.ts:0-0
Timestamp: 2025-08-05T12:04:29.037Z
Learning: In packages/app-store/office365calendar/lib/CalendarService.ts, the fetcher method in Office365CalendarService class is public, not private. It was specifically changed from private to public in this PR to support proper testing and external access patterns.
📚 Learning: 2025-08-05T12:04:29.037Z
Learnt from: din-prajapati
PR: calcom/cal.com#21854
File: packages/app-store/office365calendar/__tests__/unit_tests/SubscriptionManager.test.ts:0-0
Timestamp: 2025-08-05T12:04:29.037Z
Learning: In packages/app-store/office365calendar/lib/CalendarService.ts, the fetcher method in Office365CalendarService class is public, not private. It was specifically changed from private to public in this PR to support proper testing and external access patterns.
Applied to files:
packages/lib/CalendarService.test.tspackages/lib/CalendarService.ts
📚 Learning: 2025-07-18T17:57:16.395Z
Learnt from: vijayraghav-io
PR: calcom/cal.com#21072
File: packages/app-store/office365calendar/api/webhook.ts:120-123
Timestamp: 2025-07-18T17:57:16.395Z
Learning: The office365calendar webhook handler in packages/app-store/office365calendar/api/webhook.ts is specifically designed for Office365 calendar integration, not as a generic webhook handler. Therefore, it's safe to assume that fetchAvailabilityAndSetCache method will be implemented in the Office365CalendarService, making explicit validation checks unnecessary.
Applied to files:
packages/lib/CalendarService.test.ts
🧬 Code graph analysis (1)
packages/lib/CalendarService.test.ts (1)
packages/types/Calendar.d.ts (1)
Person(33-45)
🔇 Additional comments (4)
packages/lib/CalendarService.ts (2)
102-108: SCHEDULE-AGENT=CLIENT set on attendees — LGTM.This should stop provider-initiated attendee mail. RSVP=FALSE + PARTSTAT default also looks correct.
157-160: Timezone still serialized as UTC; confirm if local TZ requirements are satisfied.Issue #9485 calls out including timezone info. Here we force UTC (startInputType: "utc"). If providers/users expect TZID/VTIMEZONE for local rendering, consider emitting local time with TZID or including VTIMEZONE.
Would you like a follow-up patch to add TZID/VTIMEZONE while preserving server-side METHOD stripping?
packages/lib/CalendarService.test.ts (2)
54-61: Stable DTSTAMP extraction — LGTM.Good approach to avoid flakiness by deriving the library’s DTSTAMP at runtime.
199-205: Mocked object URL shape may hide a bug.Real CalDAV object URLs are typically …/calendar/.ics. The mock uses the collection URL only, which would mask the missing “/” bug in getEventsByUID. Align the mock with the expected object URL.
Apply (after fixing code):
- url: "https://caldevtest/calendar", + url: "https://caldevtest/calendar/00000000-0000-0000-0000-000000000001.ics",
| import type { Attendee, DateArray, DurationObject } from "ics2"; | ||
| import { createEvent } from "ics2"; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Switch to ics2 acknowledged; verify no lingering ics imports.
Good move to ics2 for SCHEDULE-AGENT support. Please verify nothing else still imports "ics".
Run:
🏁 Script executed:
#!/bin/bash
# Find any remaining imports/usages of the old 'ics' package
rg -nP --glob '!**/node_modules/**' '\bfrom\s+[\"\']ics[\"\']|\brequire\(\s*[\"\']ics[\"\']\s*\)'Length of output: 116
🏁 Script executed:
#!/bin/bash
# Re-run search for any remaining imports or requires of 'ics' with correct quoting
rg -nP --glob '!**/node_modules/**' "from\s+['\"]ics['\"]|require\(\s*['\"]ics['\"]\s*\)"Length of output: 861
Replace all remaining ics imports with ics2
The following files still import from "ics" and need their import paths updated to "ics2":
- packages/lib/bookings/getCalendarLinks.ts (line 3)
- packages/lib/bookings/getCalendarLinks.test.ts (line 2)
- packages/features/ee/workflows/lib/getiCalEventAsString.ts (lines 1–2)
- packages/features/ee/workflows/lib/reminders/emailReminderManager.ts (line 1)
- packages/emails/lib/generateIcsString.ts (lines 2–3)
- packages/emails/lib/generateIcsFile.ts (line 2)
Ensure all imports and type references switch fromicstoics2.
🤖 Prompt for AI Agents
In packages/lib/CalendarService.ts around lines 5–6 and in the other files
listed (packages/lib/bookings/getCalendarLinks.ts line 3,
packages/lib/bookings/getCalendarLinks.test.ts line 2,
packages/features/ee/workflows/lib/getiCalEventAsString.ts lines 1–2,
packages/features/ee/workflows/lib/reminders/emailReminderManager.ts line 1,
packages/emails/lib/generateIcsString.ts lines 2–3,
packages/emails/lib/generateIcsFile.ts line 2), replace any import statements
and type references that import from "ics" to import from "ics2" (e.g.,
import/createEvent/type imports), ensuring both value imports and "import type"
usages are updated consistently so all modules reference "ics2".
| "i18next": "^23.2.3", | ||
| "ical.js": "^1.4.0", | ||
| "ics": "^2.37.0", | ||
| "ics2": "^2.37.2", |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add ics2 — pin exact version and consider removing ics if unused.
To avoid unintentional upgrades on a forked package and reduce duplicate deps:
- Pin exactly:
"ics2": "2.37.2". - Remove
"ics"if not referenced elsewhere.
Script to check usage:
🏁 Script executed:
#!/bin/bash
# Verify 'ics' is unused; list any imports
rg -nP --glob '!**/node_modules/**' '\bfrom\s+[\"\']ics[\"\']|\brequire\(\s*[\"\']ics[\"\']\s*\)'Length of output: 116
🏁 Script executed:
#!/bin/bash
# Check if 'ics' is listed in dependencies
grep -n '"ics"' packages/lib/package.json || echo "'ics' not found in package.json"
# Search code for imports/requires of 'ics'
rg -nP --glob '!**/node_modules/**' "\\bfrom\\s+['\\\"]ics['\\\"]|\\brequire\\(\\s*['\\\"]ics['\\\"]\\s*\\)" || echo "No imports of 'ics' found in code"Length of output: 938
Pin ics2 to an exact version
In packages/lib/package.json, change the dependency from
"ics2": "^2.37.2",to
"ics2": "2.37.2"The suggestion to remove ics can be dropped—ics is imported in multiple modules (e.g. getiCalEventAsString.ts, emailReminderManager.ts, getCalendarLinks.ts, generateIcsString.ts).
🤖 Prompt for AI Agents
In packages/lib/package.json at line 24, the dependency "ics2" is using a caret
range "^2.37.2" and should be pinned to the exact version; update the
package.json dependency entry to use "2.37.2" (remove the caret) and run
npm/yarn install or update lockfile to persist the change.
Devanshusharma2005
left a comment
Devanshusharma2005
left a comment
There was a problem hiding this comment.
Could you please address the failing tests ?
|
tests are still failing |
|
Closing this PR due to inactivity. If you’d like to revisit this in the future, please feel free to open a new one, we’d be happy to take another look. |
5 participants
What does this PR do?
When an event is scheduled it is inserted into the organizer's calendar. By default most calendars using calDAV will then send their own e-mail notifying the attendees. This conflicts with Cal.com which does this already. By setting
SCHEDULE-AGENT=CLIENTin the .ics calDAV object we tell the server to not send these emails.To make minimal changes, especially because the
icspackage is used in multiple parts of the application, I forkedics(namedics2) and created a patch for the version Cal.com is using right now (v2.37.0=>v2.37.2). I'm contacting the maintainer oficsto publish this fix on the officialicspackage.In the near future it would be a good idea to add additional tests to the codebase where
icsis used so we can also confidently move tov3which more closely aligns with the calDAV spec such as quoting email addresses./claim #9485
Visual Demo
Video Demo:
Full demonstration before (Cal.com) and after (local) using Fastmail calDAV:
https://www.youtube.com/watch?v=EE6wjrhxwUU
Image Demo:
Before fix (on official Cal.com):
After fix (on local instance using RIJX Sendgrid):
Mandatory Tasks
How should this be tested?
Checklist
Summary by mrge
Updated calendar event invites to set SCHEDULE-AGENT=CLIENT in .ics files, preventing duplicate attendee emails from the organizer’s calendar.
Dependencies
Bug Fixes