Conversation
📝 WalkthroughWalkthroughThis PR introduces CalDAV calendar integration across the application. The backend adds four new API endpoints for RSVP submissions, event additions, conflict checking, and calendar listing, with a CalDAVService class for server communication and async Celery tasks. The frontend enhances the calendar invite component with conflict detection, calendar selection, and RSVP handling. A generic task status hook replaces import-specific monitoring, and the caldav library is added as a dependency. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant API as API<br/>(CalendarRsvpView)
participant Queue as Celery<br/>Task Queue
participant Task as calendar_<br/>rsvp_task
participant Service as CalDAVService
participant Server as CalDAV<br/>Server
Client->>API: POST /calendar/rsvp/<br/>(ics_data, response, mailbox_id)
activate API
API->>API: Validate inputs
API->>API: Get CalDAV channel<br/>from mailbox
API->>Queue: calendar_rsvp_task.delay()
API-->>Client: 200 {task_id}
deactivate API
Queue->>Task: Dequeue task
activate Task
Task->>Task: Fetch channel by ID
Task->>Service: Create from channel
activate Service
Service->>Server: Connect & authenticate
Task->>Service: respond_to_event(ics_data,<br/>response, attendee_email)
Service->>Service: Update attendee PARTSTAT
Service->>Server: Save updated event
Server-->>Service: Success
Service-->>Task: Complete
deactivate Service
Task-->>Queue: {status, result}
deactivate Task
sequenceDiagram
participant Client as Client
participant Component as CalendarInvite<br/>Component
participant API as Calendar APIs
participant Service as CalDAVService
participant Server as CalDAV<br/>Server
Client->>Component: Render invite (mailboxId)
activate Component
Component->>API: GET /calendar/calendars/
activate Service
Service->>Server: List calendars
Server-->>Service: [calendars]
Service-->>API: {calendars}
deactivate Service
Component->>API: POST /calendar/conflicts/<br/>(start, end)
activate Service
Service->>Server: Search events in range
Server-->>Service: [events]
Service-->>API: {conflicts}
deactivate Service
Component->>Component: Display calendars,<br/>conflicts, RSVP buttons
Client->>Component: Submit RSVP
Component->>API: POST /calendar/rsvp/<br/>(task enqueued)
API-->>Component: {task_id}
Component->>Component: Poll task status<br/>& show toast
deactivate Component
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
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 |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
src/frontend/src/features/layouts/components/thread-view/components/calendar-invite/_index.scss (1)
267-273: Consider layout for three-item conflict rows.The conflict item uses
justify-content: space-betweenwith three children (conflict-summary,conflict-calendar,conflict-time). This distributes items with the summary on the left, calendar in the middle, and time on the right. On narrow viewports, this may cause text truncation or awkward spacing.Consider using
flex-wrap: wrapor adjusting the layout so content can flow more gracefully when space is constrained.💡 Optional: Allow wrapping for conflict items
.calendar-invite__conflict-item { display: flex; justify-content: space-between; align-items: center; gap: var(--c--globals--spacings--sm); padding: var(--c--globals--spacings--3xs) 0; + flex-wrap: wrap; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/src/features/layouts/components/thread-view/components/calendar-invite/_index.scss` around lines 267 - 273, The .calendar-invite__conflict-item currently uses justify-content: space-between which spreads .conflict-summary, .conflict-calendar, and .conflict-time awkwardly on narrow viewports; update .calendar-invite__conflict-item to allow wrapping (e.g., add flex-wrap: wrap) and adjust child flex rules so .conflict-summary can shrink/grow (e.g., give it flex: 1 1 auto and min-width: 0) while keeping .conflict-calendar and .conflict-time fixed or non-growing (e.g., flex: 0 0 auto), and tweak gaps or padding as needed so the three-item row flows gracefully on small screens.src/backend/core/services/calendar/service.py (2)
133-133: Moveimport reto module level.The
remodule is imported inside methods at lines 133 and 157. Moving it to the top of the file alongside other imports improves readability and follows Python conventions.Proposed fix at module level
"""CalDAV calendar service for RSVP and event management.""" import logging +import re import caldav from caldav.elements import davThen remove the local imports at lines 133 and 157.
Also applies to: 157-157
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/services/calendar/service.py` at line 133, Move the "import re" statement to the top of the module alongside the other imports (add a single module-level "import re") and remove the local "import re" statements that currently appear inside methods in this file; ensure those methods (which use regex functionality) continue to reference the module-level "re" symbol rather than importing it locally.
20-28: Add a timeout to DAVClient to prevent indefinite request blocking.The
caldav.DAVClientis created without a timeout parameter. If the CalDAV server is slow or unresponsive, this could cause request threads (inCalendarConflictsViewandCalendarListView) or Celery workers to block indefinitely.Proposed fix
`@property` def client(self): if self._client is None: self._client = caldav.DAVClient( url=self.url, username=self.username, password=self.password, + timeout=30, # seconds ) return self._client🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/services/calendar/service.py` around lines 20 - 28, The DAVClient instantiation in the client property lacks a timeout, risking indefinite blocking; update the client property to pass a sensible timeout value (e.g., timeout=10 or use a configured constant) into caldav.DAVClient when creating self._client so requests from CalendarConflictsView/CalendarListView and Celery workers don't hang indefinitely; ensure the timeout value is configurable via settings or an environment variable and referenced where caldav.DAVClient is constructed in the client property.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/backend/core/api/viewsets/calendar.py`:
- Around line 259-267: CalendarListView.get is making blocking I/O via
CalDAVService.from_channel and service.list_calendars (synchronous calls) which
can block the event loop; refactor this handler to offload the blocking calls to
an async-friendly pattern—either convert CalDAVService methods to async or run
the synchronous calls in a thread/process executor (e.g., using
asyncio.to_thread or a background task) before returning the Response;
specifically wrap CalDAVService.from_channel(...) and service.list_calendars()
so they execute off the main thread and handle/propagate exceptions the same way
as existing logger.exception and Response logic.
- Around line 219-229: CalendarConflictsView is performing a blocking call to
the external CalDAV server via CalDAVService.from_channel(...) and
service.check_conflicts(...); change this to avoid blocking the request thread
by either converting the view to an async Django view and awaiting an async
CalDAVService.check_conflicts (ensure CalDAVService implements an async API and
enforces a network timeout), or move the conflict check into a background Celery
task (enqueue a task from CalendarConflictsView that calls
CalDAVService.check_conflicts and return a 202 with a task id), and in either
case ensure CalDAVService has a strict configurable timeout for network calls to
prevent long waits.
- Around line 25-46: The mailbox lookup in CalDAVChannelMixin is missing an
authorization check allowing any authenticated user to access arbitrary
mailboxes; update the cached_property mailbox to resolve the Mailbox through the
same authorized/queryset pattern used in blob.py and contacts.py (i.e., restrict
the queryset to mailboxes the current request.user may access or call the shared
helper that returns an authorized mailbox) so that get_object_or_404 only
returns a mailbox the requester is permitted to use; leave get_caldav_channel
and require_caldav_channel unchanged except that they should rely on the
now-authorized mailbox property.
In
`@src/frontend/src/features/layouts/components/thread-view/components/calendar-invite/index.tsx`:
- Around line 358-361: The CalDAV query errors are being swallowed and replaced
with empty arrays, making transient failures indistinguishable from genuine "no
data" and hiding UI like the ConflictWarning; update the query hooks used around
the referenced areas (the calendar fetch and event fetch calls used to compute
`conflicts`, `calendars`, and related data) so they propagate an error state
instead of returning `[]` on failure, and adjust the rendering in this component
to check for that error state (e.g., `calendarsError`/`eventsError` or similar)
and render a local "Calendar unavailable — retry" UI with a retry action; ensure
`ConflictWarning` is still shown when `conflicts.length > 0` but that when the
queries report an error you show the unavailable/retry UI instead of collapsing
to an empty state.
- Around line 469-470: The RSVP/Add flow is committing rsvpResponse and relying
on isPending only after the POST returns a task_id, allowing multiple clicks and
lost/incorrect state; add a local "pendingAction" boolean (or enum) state (e.g.,
isSubmitting or pendingRsvp) that is set immediately in the RSVP/Add click
handlers (before the POST) to disable controls, and preserve the intended
response plus the returned task_id in a small pending map (taskId -> intended
response). Keep selectedCalendarId handling but do not call setRsvpResponse
until the background poll for that task_id reaches SUCCESS; on FAILURE/timeout
clear the pendingAction and do not commit rsvpResponse (and re-enable controls).
Update the RSVP/Add handler functions and the polling completion logic to look
up the pending map by task_id and commit or clear state accordingly so clicks
cannot enqueue multiple jobs or prematurely highlight a choice.
---
Nitpick comments:
In `@src/backend/core/services/calendar/service.py`:
- Line 133: Move the "import re" statement to the top of the module alongside
the other imports (add a single module-level "import re") and remove the local
"import re" statements that currently appear inside methods in this file; ensure
those methods (which use regex functionality) continue to reference the
module-level "re" symbol rather than importing it locally.
- Around line 20-28: The DAVClient instantiation in the client property lacks a
timeout, risking indefinite blocking; update the client property to pass a
sensible timeout value (e.g., timeout=10 or use a configured constant) into
caldav.DAVClient when creating self._client so requests from
CalendarConflictsView/CalendarListView and Celery workers don't hang
indefinitely; ensure the timeout value is configurable via settings or an
environment variable and referenced where caldav.DAVClient is constructed in the
client property.
In
`@src/frontend/src/features/layouts/components/thread-view/components/calendar-invite/_index.scss`:
- Around line 267-273: The .calendar-invite__conflict-item currently uses
justify-content: space-between which spreads .conflict-summary,
.conflict-calendar, and .conflict-time awkwardly on narrow viewports; update
.calendar-invite__conflict-item to allow wrapping (e.g., add flex-wrap: wrap)
and adjust child flex rules so .conflict-summary can shrink/grow (e.g., give it
flex: 1 1 auto and min-width: 0) while keeping .conflict-calendar and
.conflict-time fixed or non-growing (e.g., flex: 0 0 auto), and tweak gaps or
padding as needed so the three-item row flows gracefully on small screens.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a7bfb200-20b4-433f-ad17-21d8b2b39b4e
⛔ Files ignored due to path filters (1)
src/backend/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
src/backend/core/api/viewsets/calendar.pysrc/backend/core/services/calendar/__init__.pysrc/backend/core/services/calendar/service.pysrc/backend/core/services/calendar/tasks.pysrc/backend/core/tasks.pysrc/backend/core/urls.pysrc/backend/pyproject.tomlsrc/frontend/public/locales/common/en-US.jsonsrc/frontend/src/features/controlled-modals/message-importer/step-loader.tsxsrc/frontend/src/features/layouts/components/main/header/authenticated.tsxsrc/frontend/src/features/layouts/components/thread-view/components/calendar-invite/_index.scsssrc/frontend/src/features/layouts/components/thread-view/components/calendar-invite/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-message/thread-message-footer.tsxsrc/frontend/src/hooks/use-task-status.ts
| class CalDAVChannelMixin: | ||
| """Mixin to get the CalDAV channel for a mailbox.""" | ||
|
|
||
| @cached_property | ||
| def mailbox(self): | ||
| return get_object_or_404(models.Mailbox, id=self.kwargs["mailbox_id"]) | ||
|
|
||
| def get_caldav_channel(self): | ||
| """Get the CalDAV channel for the mailbox, or None.""" | ||
| return ( | ||
| models.Channel.objects.filter( | ||
| mailbox=self.mailbox, type="caldav" | ||
| ).first() | ||
| ) | ||
|
|
||
| def require_caldav_channel(self): | ||
| """Get the CalDAV channel or raise 404.""" | ||
| channel = self.get_caldav_channel() | ||
| if not channel: | ||
| from rest_framework.exceptions import NotFound | ||
| raise NotFound("No CalDAV calendar is configured for this mailbox.") | ||
| return channel |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for existing mailbox authorization patterns in the codebase
rg -n -A5 "MailboxAccess" --type py src/backend/core/api/Repository: suitenumerique/messages
Length of output: 17620
🏁 Script executed:
cat -n src/backend/core/api/viewsets/calendar.pyRepository: suitenumerique/messages
Length of output: 10817
Critical: Missing mailbox authorization check allows unauthorized access.
The CalDAVChannelMixin retrieves the mailbox by ID without verifying that the authenticated user has permission to access it. Any authenticated user can operate on any mailbox's CalDAV channel by simply knowing or guessing the mailbox_id UUID.
All four calendar views inherit this mixin with only IsAuthenticated permission checks:
CalendarRsvpView: Can RSVP on behalf of any mailboxCalendarAddEventView: Can add events to any mailbox's calendarCalendarConflictsView: Can view calendar conflicts for any mailboxCalendarListView: Can list calendars for any mailbox
Apply the mailbox access check pattern established elsewhere in the codebase (e.g., blob.py, contacts.py) to the mixin's mailbox property:
Proposed fix
class CalDAVChannelMixin:
"""Mixin to get the CalDAV channel for a mailbox."""
`@cached_property`
def mailbox(self):
- return get_object_or_404(models.Mailbox, id=self.kwargs["mailbox_id"])
+ mailbox = get_object_or_404(models.Mailbox, id=self.kwargs["mailbox_id"])
+ # Verify user has access to this mailbox
+ if not models.MailboxAccess.objects.filter(
+ mailbox=mailbox, user=self.request.user
+ ).exists():
+ from rest_framework.exceptions import PermissionDenied
+ raise PermissionDenied("You do not have access to this mailbox.")
+ return mailbox📝 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.
| class CalDAVChannelMixin: | |
| """Mixin to get the CalDAV channel for a mailbox.""" | |
| @cached_property | |
| def mailbox(self): | |
| return get_object_or_404(models.Mailbox, id=self.kwargs["mailbox_id"]) | |
| def get_caldav_channel(self): | |
| """Get the CalDAV channel for the mailbox, or None.""" | |
| return ( | |
| models.Channel.objects.filter( | |
| mailbox=self.mailbox, type="caldav" | |
| ).first() | |
| ) | |
| def require_caldav_channel(self): | |
| """Get the CalDAV channel or raise 404.""" | |
| channel = self.get_caldav_channel() | |
| if not channel: | |
| from rest_framework.exceptions import NotFound | |
| raise NotFound("No CalDAV calendar is configured for this mailbox.") | |
| return channel | |
| class CalDAVChannelMixin: | |
| """Mixin to get the CalDAV channel for a mailbox.""" | |
| `@cached_property` | |
| def mailbox(self): | |
| mailbox = get_object_or_404(models.Mailbox, id=self.kwargs["mailbox_id"]) | |
| # Verify user has access to this mailbox | |
| if not models.MailboxAccess.objects.filter( | |
| mailbox=mailbox, user=self.request.user | |
| ).exists(): | |
| from rest_framework.exceptions import PermissionDenied | |
| raise PermissionDenied("You do not have access to this mailbox.") | |
| return mailbox | |
| def get_caldav_channel(self): | |
| """Get the CalDAV channel for the mailbox, or None.""" | |
| return ( | |
| models.Channel.objects.filter( | |
| mailbox=self.mailbox, type="caldav" | |
| ).first() | |
| ) | |
| def require_caldav_channel(self): | |
| """Get the CalDAV channel or raise 404.""" | |
| channel = self.get_caldav_channel() | |
| if not channel: | |
| from rest_framework.exceptions import NotFound | |
| raise NotFound("No CalDAV calendar is configured for this mailbox.") | |
| return channel |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/backend/core/api/viewsets/calendar.py` around lines 25 - 46, The mailbox
lookup in CalDAVChannelMixin is missing an authorization check allowing any
authenticated user to access arbitrary mailboxes; update the cached_property
mailbox to resolve the Mailbox through the same authorized/queryset pattern used
in blob.py and contacts.py (i.e., restrict the queryset to mailboxes the current
request.user may access or call the shared helper that returns an authorized
mailbox) so that get_object_or_404 only returns a mailbox the requester is
permitted to use; leave get_caldav_channel and require_caldav_channel unchanged
except that they should rely on the now-authorized mailbox property.
| from core.services.calendar.service import CalDAVService | ||
|
|
||
| try: | ||
| service = CalDAVService.from_channel(channel) | ||
| conflicts = service.check_conflicts(start=start, end=end) | ||
| except Exception as e: | ||
| logger.exception("Error checking calendar conflicts: %s", e) | ||
| return Response( | ||
| {"detail": "Failed to check for conflicts."}, | ||
| status=status.HTTP_502_BAD_GATEWAY, | ||
| ) |
There was a problem hiding this comment.
Blocking I/O call to external CalDAV server on request thread.
CalendarConflictsView makes a synchronous call to the external CalDAV server within the request-response cycle. If the CalDAV server is slow or unresponsive, this will block the request thread and degrade overall API responsiveness.
Consider either:
- Making this an async view using Django's async support
- Moving the conflict check to a Celery task (like RSVP and add-event)
- Adding a strict timeout to the CalDAV client (see earlier comment on service.py)
As per coding guidelines: "Use asynchronous views and Celery tasks for I/O-bound or long-running operations"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/backend/core/api/viewsets/calendar.py` around lines 219 - 229,
CalendarConflictsView is performing a blocking call to the external CalDAV
server via CalDAVService.from_channel(...) and service.check_conflicts(...);
change this to avoid blocking the request thread by either converting the view
to an async Django view and awaiting an async CalDAVService.check_conflicts
(ensure CalDAVService implements an async API and enforces a network timeout),
or move the conflict check into a background Celery task (enqueue a task from
CalendarConflictsView that calls CalDAVService.check_conflicts and return a 202
with a task id), and in either case ensure CalDAVService has a strict
configurable timeout for network calls to prevent long waits.
| try: | ||
| service = CalDAVService.from_channel(channel) | ||
| calendars = service.list_calendars() | ||
| except Exception as e: | ||
| logger.exception("Error listing calendars: %s", e) | ||
| return Response( | ||
| {"detail": "Failed to list calendars."}, | ||
| status=status.HTTP_502_BAD_GATEWAY, | ||
| ) |
There was a problem hiding this comment.
Same blocking I/O concern as CalendarConflictsView.
CalendarListView.get also makes synchronous calls to the external CalDAV server. The same recommendation applies: consider async handling or a task-based approach for consistency with other calendar operations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/backend/core/api/viewsets/calendar.py` around lines 259 - 267,
CalendarListView.get is making blocking I/O via CalDAVService.from_channel and
service.list_calendars (synchronous calls) which can block the event loop;
refactor this handler to offload the blocking calls to an async-friendly
pattern—either convert CalDAVService methods to async or run the synchronous
calls in a thread/process executor (e.g., using asyncio.to_thread or a
background task) before returning the Response; specifically wrap
CalDAVService.from_channel(...) and service.list_calendars() so they execute off
the main thread and handle/propagate exceptions the same way as existing
logger.exception and Response logic.
| {/* Conflict Warning */} | ||
| {conflicts.length > 0 && ( | ||
| <ConflictWarning conflicts={conflicts} language={language} /> | ||
| )} |
There was a problem hiding this comment.
Don't collapse CalDAV fetch failures into an empty state.
Both CalDAV queries suppress global errors and then fall back to []. A transient failure therefore looks identical to “no calendars configured” and “no conflicts found”: the direct-action UI disappears, and conflict warnings are silently skipped. Please surface a local unavailable/retry state instead of treating query failure as empty data.
Also applies to: 491-503, 515-538, 706-742
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/frontend/src/features/layouts/components/thread-view/components/calendar-invite/index.tsx`
around lines 358 - 361, The CalDAV query errors are being swallowed and replaced
with empty arrays, making transient failures indistinguishable from genuine "no
data" and hiding UI like the ConflictWarning; update the query hooks used around
the referenced areas (the calendar fetch and event fetch calls used to compute
`conflicts`, `calendars`, and related data) so they propagate an error state
instead of returning `[]` on failure, and adjust the rendering in this component
to check for that error state (e.g., `calendarsError`/`eventsError` or similar)
and render a local "Calendar unavailable — retry" UI with a retry action; ensure
`ConflictWarning` is still shown when `conflicts.length > 0` but that when the
queries report an error you show the unavailable/retry UI instead of collapsing
to an empty state.
| const [selectedCalendarId, setSelectedCalendarId] = useState<string | null>(null); | ||
| const [rsvpResponse, setRsvpResponse] = useState<RsvpResponse | null>(null); |
There was a problem hiding this comment.
Track a local pending action before the task id exists.
isPending only becomes true after the POST resolves with a task_id, so the RSVP/Add controls stay clickable during the initial request and can enqueue multiple CalDAV jobs. Any earlier task_id is then overwritten, so only the last submission is tracked. rsvpResponse is also committed before the background task succeeds, which means a failed task leaves the chosen button highlighted as if the RSVP was stored. Please add a separate request-in-flight / pending-action state, disable the controls immediately, and only commit the selected response once polling reaches SUCCESS.
Also applies to: 541-568, 570-624, 716-740
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/frontend/src/features/layouts/components/thread-view/components/calendar-invite/index.tsx`
around lines 469 - 470, The RSVP/Add flow is committing rsvpResponse and relying
on isPending only after the POST returns a task_id, allowing multiple clicks and
lost/incorrect state; add a local "pendingAction" boolean (or enum) state (e.g.,
isSubmitting or pendingRsvp) that is set immediately in the RSVP/Add click
handlers (before the POST) to disable controls, and preserve the intended
response plus the returned task_id in a small pending map (taskId -> intended
response). Keep selectedCalendarId handling but do not call setRsvpResponse
until the background poll for that task_id reaches SUCCESS; on FAILURE/timeout
clear the pendingAction and do not commit rsvpResponse (and re-enable controls).
Update the RSVP/Add handler functions and the polling completion logic to look
up the pending map by task_id and commit or clear state accordingly so clicks
cannot enqueue multiple jobs or prematurely highlight a choice.
First step in the interop with https://github.com/suitenumerique/calendars
Summary by CodeRabbit
Release Notes