✨(subscription) import external ICS calendar feeds#45
Open
PanchoutNathan wants to merge 12 commits into
Open
Conversation
📝 WalkthroughWalkthroughAdds external ICS subscription support: backend URL validation/fetching, diff-based sync service with scheduler/tasks, CalDAV read-only enforcement, API serializers/endpoints, frontend subscription management UI/hooks/components, tests, translations, and Docker/Makefile changes to run a subscription scheduler. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/Client
participant API as API Server
participant DB as PostgreSQL
participant Redis as Redis
participant Sched as Scheduler Process
participant ICS as External ICS Server
participant DAV as SabreDAV Calendar
User->>API: POST /channels (ical-subscription)
API->>ICS: fetch_ics(url)
ICS-->>API: 200 + ICS data
API->>DAV: create_calendar()
DAV-->>API: calendar created
API->>DB: save Channel (settings including source_url, sync_interval)
API->>API: SubscriptionSyncService.sync_events (immediate)
API-->>User: 201 Created
Sched->>DB: query active ical-subscription channels
Sched->>Redis: acquire per-channel lock
Redis-->>Sched: lock acquired
Sched->>ICS: fetch_ics(url, etag, last_modified)
ICS-->>Sched: 304 or 200 + ICS data
Sched->>DAV: list & compare events
Sched->>DAV: PUT new/updated events
Sched->>DAV: DELETE removed events
Sched->>DB: update sync metadata (status/etag/last_sync_at)
Redis->>Redis: release lock
sequenceDiagram
participant UI as Frontend
participant API as API Server
participant DAV as SabreDAV Calendar
UI->>UI: User clicks event
UI->>UI: Check subscriptionCalendarUrls contains calendarUrl
alt Read-only subscription calendar
UI->>UI: Open ReadOnlyEventModal (view-only)
UI->>UI: Block drag/resize (revert)
else Writable calendar
UI->>UI: Open EventModal (edit)
UI->>API: PATCH event -> API->>DAV: update event
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 docstrings
🧪 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 |
PanchoutNathan
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (4)
src/frontend/apps/calendars/src/features/calendar/components/calendar-list/ColorPicker.tsx-25-34 (1)
25-34:⚠️ Potential issue | 🟡 MinorExpose the selected color to assistive tech.
These buttons behave like a single-choice control, but the current selection is only communicated through CSS. Add
aria-pressed/radio semantics and a more descriptive label than just the hex code so screen-reader users can tell which color is selected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/apps/calendars/src/features/calendar/components/calendar-list/ColorPicker.tsx` around lines 25 - 34, The color buttons currently only communicate selection via CSS; update the button element in ColorPicker.tsx (the element using variables c, value, onChange and className "calendar-modal__color-btn") to expose selection to assistive tech by adding ARIA semantics: set aria-pressed={value === c} (or role="radio" + aria-checked={value === c}) and replace the terse aria-label={c} with a descriptive label like aria-label={`Select color ${c}${value === c ? ', selected' : ''}`}; if there is a container for these buttons, ensure it uses role="radiogroup" to complete the single-choice semantics.src/frontend/apps/calendars/src/features/calendar/components/calendar-list/SubscriptionStatusBadge.tsx-41-45 (1)
41-45:⚠️ Potential issue | 🟡 MinorAdd an explicit accessible label to the icon-only toggle button.
titleis not a reliable accessible name. Please addaria-label={t("calendar.subscription.status.viewError")}on this button as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/apps/calendars/src/features/calendar/components/calendar-list/SubscriptionStatusBadge.tsx` around lines 41 - 45, The icon-only toggle button in SubscriptionStatusBadge uses title for accessibility but needs an explicit accessible name; update the button element (className "subscription-status__toggle" in the SubscriptionStatusBadge component) to include aria-label={t("calendar.subscription.status.viewError")} alongside the existing title and onClick handler (which toggles showError via setShowError) so screen readers receive a reliable label.src/frontend/apps/calendars/src/features/calendar/components/calendar-list/SubscriptionStatusBadge.tsx-70-73 (1)
70-73:⚠️ Potential issue | 🟡 MinorProvide a fallback message for empty error payloads.
If
last_sync_status === "error"butlast_sync_erroris empty, the panel renders blank content. Add the same translated fallback used in the stopped state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/apps/calendars/src/features/calendar/components/calendar-list/SubscriptionStatusBadge.tsx` around lines 70 - 73, When rendering the error panel in SubscriptionStatusBadge, handle empty error payloads by showing the same translated fallback used for the stopped state instead of blank content: inside the showError block (where channel.last_sync_error is currently rendered) render channel.last_sync_error if truthy, otherwise call the same translation used by the stopped state fallback (use the same translation key/function already used elsewhere in SubscriptionStatusBadge) so a user-visible message appears whenever last_sync_status === "error" but last_sync_error is empty.src/frontend/apps/calendars/src/features/calendar/components/calendar-list/AddSubscriptionModal.tsx-95-103 (1)
95-103:⚠️ Potential issue | 🟡 MinorReset mutation error when user edits the URL field.
After a failed submit, the error state persists while typing, which makes the field look permanently invalid. Resetting mutation state on input change gives immediate feedback recovery.
Proposed fix
<Input label={t("calendar.subscription.add.urlLabel")} value={url} - onChange={(e) => setUrl(e.target.value)} + onChange={(e) => { + if (createMutation.isError) createMutation.reset(); + setUrl(e.target.value); + }} placeholder="https://example.com/calendar.ics" fullWidth🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/apps/calendars/src/features/calendar/components/calendar-list/AddSubscriptionModal.tsx` around lines 95 - 103, The URL input's change handler should clear the mutation error so the field doesn't stay stuck in error state; update the onChange for the URL input in AddSubscriptionModal to call setUrl(e.target.value) and then reset the createMutation state (e.g., call createMutation.reset()) when createMutation.isError is true so typing clears the previous error and the input returns to default state.
🧹 Nitpick comments (6)
Makefile (1)
273-281: Avoid hard-coding the database name/user inreset-db-full.This helper ignores local
DB_NAME/DB_USERoverrides and also sidesteps the Makefile’s own “define variables in the VARIABLES section” rule. Lift them into variables with defaults before reusing them here.♻️ Suggested variable extraction
+# -- Database +DB_NAME ?= calendars +DB_USER ?= pgroot- @$(COMPOSE) exec postgresql psql -U pgroot -d postgres -c "SELECT pg_terminate_backend(pid) FROM pg_stat_activity WHERE datname = 'calendars' AND pid <> pg_backend_pid();" > /dev/null 2>&1 || true - @$(COMPOSE) exec postgresql dropdb -U pgroot --if-exists calendars - @$(COMPOSE) exec postgresql createdb -U pgroot calendars + @$(COMPOSE) exec postgresql psql -U $(DB_USER) -d postgres -c "SELECT pg_terminate_backend(pid) FROM pg_stat_activity WHERE datname = '$(DB_NAME)' AND pid <> pg_backend_pid();" > /dev/null 2>&1 || true + @$(COMPOSE) exec postgresql dropdb -U $(DB_USER) --if-exists $(DB_NAME) + @$(COMPOSE) exec postgresql createdb -U $(DB_USER) $(DB_NAME)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 273 - 281, The reset-db-full target hard-codes the database name and user; introduce variables (e.g., DB_NAME ?= calendars and DB_USER ?= pgroot) in the VARIABLES section and then replace literal 'calendars' and 'pgroot' in the reset-db-full recipe (the SELECT pg_terminate_backend... WHERE datname = 'calendars'..., dropdb -U pgroot --if-exists calendars, createdb -U pgroot calendars) with $(DB_NAME) and $(DB_USER) respectively so local overrides are honored; keep using $(COMPOSE) and existing commands but reference these new variables throughout the reset-db-full target.src/backend/core/tests/test_subscription_tasks.py (1)
47-50: Strengthen dispatch assertions beyond call count.This verifies fan-out volume, but not payload correctness. Consider also asserting
send_with_optionsreceivesargs=(str(channel.pk),)and a numericdelayto catch silent regressions in task scheduling arguments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/tests/test_subscription_tasks.py` around lines 47 - 50, The test currently only checks send_with_options.call_count; extend it to assert each call's payload and scheduling by examining mock_sync_one.send_with_options.call_args_list from sync_all_subscriptions() and verifying that for each call the positional args equal (str(channel.pk),) (or the expected channel PK string values) and that the keyword arg "delay" exists and is a numeric type (int/float) to ensure correct task arguments and scheduling; reference the sync_all_subscriptions invocation and mock_sync_one.send_with_options to locate where to add these assertions.src/backend/core/tests/test_calendar_subscription_api.py (1)
421-433: Avoid hard-coded subscription limit in tests.These cases should reference
settings.MAX_SUBSCRIPTIONS_PER_USERinstead of literal20, so tests stay valid when configuration changes.Proposed fix
+from django.conf import settings ... - def test_create_subscription_limit_reached(self, mock_fetch, mock_caldav): - """Cannot create a 21st subscription when limit is 20.""" + def test_create_subscription_limit_reached(self, mock_fetch, mock_caldav): + """Cannot create one more subscription when limit is reached.""" ... - # Create 20 subscription channels - for i in range(20): + # Create channels up to limit + for _ in range(settings.MAX_SUBSCRIPTIONS_PER_USER): factories.ICalSubscriptionChannelFactory(user=user) ... - ).count() == 20 + ).count() == settings.MAX_SUBSCRIPTIONS_PER_USER ... - def test_create_subscription_under_limit(self, mock_fetch, mock_caldav): - """Can create the 20th subscription (limit not exceeded).""" + def test_create_subscription_under_limit(self, mock_fetch, mock_caldav): + """Can create a subscription while still below limit.""" ... - # Create 19 subscription channels - for _ in range(19): + # Create channels just below limit + for _ in range(settings.MAX_SUBSCRIPTIONS_PER_USER - 1): factories.ICalSubscriptionChannelFactory(user=user) ... - ).count() == 20 + ).count() == settings.MAX_SUBSCRIPTIONS_PER_USERAlso applies to: 452-482
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/tests/test_calendar_subscription_api.py` around lines 421 - 433, The test hard-codes the max subscription value as 20; replace literal 20 with settings.MAX_SUBSCRIPTIONS_PER_USER in the loop that creates subscriptions (for i in range(...)) and in the assertion that checks Channel.objects.filter(...).count(), and update any other similar occurrences in test_calendar_subscription_api.py (e.g., the related block around the later test) to reference settings.MAX_SUBSCRIPTIONS_PER_USER so the test adapts to configuration changes; ensure you import django.conf.settings at the top of the test module if not already present.src/frontend/apps/calendars/src/features/calendar/components/calendar-list/SubscriptionCalendarSection.tsx (1)
111-123: Minor: Redundant condition in onClick handler.The button already has
disabled={isLimitReached}, so the!isLimitReached &&check in the onClick handler is redundant. The disabled attribute prevents clicks.♻️ Suggested simplification
<button className="calendar-list__add-btn" - onClick={() => !isLimitReached && setIsAddModalOpen(true)} + onClick={() => setIsAddModalOpen(true)} title={ isLimitReached ? t("calendar.subscription.add.limitReached") : t("calendar.subscription.add.title") } disabled={isLimitReached} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/apps/calendars/src/features/calendar/components/calendar-list/SubscriptionCalendarSection.tsx` around lines 111 - 123, The onClick handler on the button in SubscriptionCalendarSection.tsx redundantly guards with !isLimitReached even though the button already uses disabled={isLimitReached}; simplify the handler by removing the condition and directly calling setIsAddModalOpen(true) in the onClick (keep the disabled prop as-is) so clicks open the add modal when enabled and are naturally ignored when disabled.src/frontend/apps/calendars/src/features/calendar/components/calendar-list/CalendarList.tsx (1)
170-185: Consider adding error handling for delete failures.The
handleDeleteSubscriptionfunction correctly cleans updeletingPathsin thefinallyblock, but ifdeleteSubMutation.mutateAsyncfails, the error is silently swallowed. Users won't know if deletion failed.♻️ Suggested improvement
const handleDeleteSubscription = useCallback( async (channel: SubscriptionChannel) => { setDeletingPaths((prev) => new Set(prev).add(channel.caldav_path)); try { await deleteSubMutation.mutateAsync(channel.id); await refreshCalendars(); - } finally { + } catch (error) { + // Consider showing a toast/notification here + console.error("Failed to delete subscription:", error); + } finally { setDeletingPaths((prev) => { const next = new Set(prev); next.delete(channel.caldav_path); return next; }); } }, [deleteSubMutation, refreshCalendars], );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/apps/calendars/src/features/calendar/components/calendar-list/CalendarList.tsx` around lines 170 - 185, The delete handler handleDeleteSubscription currently swallows errors from deleteSubMutation.mutateAsync; update it to catch errors, notify the user (e.g., show a toast or set an error state) with the mutation error message, and optionally log the error before allowing the finally block to clear deletingPaths; ensure you still call refreshCalendars only on success (or explicitly after handling failures if desired) and keep references to deleteSubMutation.mutateAsync, refreshCalendars, setDeletingPaths unchanged so the change is localized to adding a try/catch around the mutation and invoking the user-facing notification in the catch.src/backend/core/api/serializers.py (1)
248-285: Consider extracting duplicatevalidate_source_urllogic.Both
ChannelSubscriptionCreateSerializerandChannelSubscriptionUpdateSerializerhave identicalvalidate_source_urlmethods. While the duplication is minor, extracting to a mixin would improve maintainability.♻️ Suggested refactor using a mixin
class SourceUrlValidationMixin: """Mixin for validating subscription source URLs.""" def validate_source_url(self, value): """Validate and normalize the source URL.""" from core.services.url_validation import ( # noqa: PLC0415 URLValidationError, validate_subscription_url, ) try: return validate_subscription_url(value) except URLValidationError as exc: raise serializers.ValidationError(str(exc)) from exc class ChannelSubscriptionCreateSerializer(SourceUrlValidationMixin, serializers.Serializer): # ... fields ... pass class ChannelSubscriptionUpdateSerializer(SourceUrlValidationMixin, serializers.Serializer): # ... fields ... pass🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/api/serializers.py` around lines 248 - 285, Both ChannelSubscriptionCreateSerializer and ChannelSubscriptionUpdateSerializer duplicate the validate_source_url implementation; extract that logic into a shared mixin (e.g., SourceUrlValidationMixin) that defines validate_source_url and import URLValidationError/validate_subscription_url there, then have ChannelSubscriptionCreateSerializer and ChannelSubscriptionUpdateSerializer inherit from SourceUrlValidationMixin in addition to serializers.Serializer so they can remove their local validate_source_url methods and reuse the single implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Around line 113-114: The Makefile target start-back currently brings up only
worker-dev and subscription-scheduler, so add the missing Django service
backend-dev to the docker-compose up command; update the start-back recipe (the
line invoking $(COMPOSE) up --force-recreate -d ...) to include backend-dev
along with worker-dev and subscription-scheduler so start-back and any
dependents like start-back-e2e will actually start the Django API.
In `@src/backend/calendars/settings.py`:
- Around line 87-95: The settings currently allow non-positive values for
SUBSCRIPTION_SYNC_INTERVAL and MAX_SUBSCRIPTIONS_PER_USER which cause scheduler
distortion or immediate subscription failures; add startup validation in the
calendars settings module to enforce semantics: for SUBSCRIPTION_SYNC_INTERVAL
treat 0 as an explicit "disabled" sentinel (if desired) and reject negative
values, or require >=1 if disable-by-zero is not supported, and require
MAX_SUBSCRIPTIONS_PER_USER >= 1 (or treat 0 as "no subscriptions allowed" only
if intended) by raising a clear ConfigurationError/ValueError during app
initialization; update validation logic near the definitions of
SUBSCRIPTION_SYNC_INTERVAL and MAX_SUBSCRIPTIONS_PER_USER to check values and
raise with an explanatory message so invalid environment configs fail fast.
In `@src/backend/core/api/viewsets_caldav.py`:
- Around line 122-128: Looping over
Channel.objects.filter(...).values_list("caldav_path", flat=True) may yield None
or empty strings; before using each sub_path in the prefix checks with
normalized.startswith(sub_path) or full_path.startswith(sub_path) skip falsy
values and normalize to a string (e.g., continue if not sub_path) or ensure the
queryset excludes null/empty paths; update the logic around subscription_paths
iteration (the variable sub_path and the surrounding for loop that compares
against normalized and full_path) to skip None/"" entries and normalize paths
before calling startswith.
In `@src/backend/core/api/viewsets_channels.py`:
- Around line 127-140: The current per-user quota check uses
models.Channel.objects.filter(...).count() which is racy—concurrent POSTs can
bypass the cap; make the check atomic by wrapping the count-and-create sequence
in a transaction (transaction.atomic()) and acquiring a row lock on the owning
user (e.g., reload the user with select_for_update()) or otherwise perform the
insertion under a database-enforced constraint; then call create_calendar()
inside that transaction so the check against settings.MAX_SUBSCRIPTIONS_PER_USER
and the subsequent creation are serialized for this user (referencing
models.Channel.objects.filter, settings.MAX_SUBSCRIPTIONS_PER_USER, and
create_calendar()).
- Around line 241-255: The code calls SubscriptionSyncService().sync_events(...)
but ignores its return (SyncResult) and always sets
channel.settings["last_sync_status"]="ok" on success or leaves "pending" on
exception; change both places where sync_events is called (the block around
SubscriptionSyncService and the similar block handling initial sync later) to
capture the returned SyncResult, inspect SyncResult.errors/ok status, and
persist channel.settings["last_sync_status"] as "ok" or "failed" (and include
SyncResult.errors or a concise error summary in
channel.settings["last_sync_error"]) along with updating "last_sync_at", "etag",
"last_modified"; still log exceptions but ensure partial failures update status
based on the SyncResult rather than unconditionally writing "ok" or leaving
"pending".
In `@src/backend/core/management/commands/run_subscription_scheduler.py`:
- Around line 17-18: SCHEDULER_INTERVAL is being parsed directly from env and
used in the loop; validate and clamp it before entering the scheduler loop by
parsing with error handling (catch ValueError), defaulting to 60 on parse
failure, and enforcing a minimum of 1 (e.g., max(parsed_value, 1)). Update the
top-level SCHEDULER_INTERVAL initialization and any code that consumes it
(reference SCHEDULER_INTERVAL and the scheduler loop where it's used) so
non-integer, zero, or negative env values do not crash or overload dispatching.
In `@src/backend/core/services/subscription_sync_service.py`:
- Around line 202-204: sync_events currently calls _sync_events directly and
thus bypasses the per-channel subscription lock, allowing concurrent
scheduler-driven syncs (sync_channel) to interleave PUT/DELETE operations;
change sync_events to acquire the same per-channel subscription lock used by
sync_channel (or delegate to sync_channel) before calling _sync_events so all
public request-driven syncs serialize on the channel lock and avoid concurrent
writes to the same SabreDAV calendar.
- Around line 383-416: _event_props currently flattens VEVENTs to {k: str(v)}
which drops property parameters and subcomponents (so changes to
ATTENDEE;PARTSTAT or VALARM are missed); update
SubscriptionSyncService._event_props (and thus _events_differ/_keyed) to produce
a stable serialized representation that includes for each property its name,
full serialized value (e.g., to_ical()/raw value) and its parameter set, and
also include any subcomponents (walk() children like VALARM) in the comparison;
produce comparable dicts/tuples (property -> (params, serialized_value) and
subcomponent representations) so _keyed(existing_events) != _keyed(new_events)
will detect parameter and subcomponent changes.
- Around line 115-118: The 304 branch in subscription_sync_service (the if
status_code == 304 block) treats the response as successful but doesn't reset
prior error state; modify that branch to also clear
channel.settings["error_count"] (set to 0) and
channel.settings["last_sync_error"] (set to None or empty string) in addition to
updating channel.settings["last_sync_at"] and
channel.settings["last_sync_status"], then call self._save_channel(channel) so a
304 won't contribute to auto-stop logic.
In `@src/backend/core/services/url_validation.py`:
- Around line 128-159: The redirect loop in fetch_ics (inside url_validation.py)
leaks Response objects and lets raw read errors escape; before replacing
response in the while loop (and before any early returns like on 304/non-200 in
fetch_ics), call response.close() to ensure the previous streamed response is
closed, and ensure the final response is closed in a finally block when
fetch_ics exits; also catch exceptions from _read_response_body() and wrap them
in URLValidationError (preserving the original exception as __cause__) so
callers like SubscriptionSyncService._do_sync() see a URLValidationError instead
of a raw exception; update references: fetch_ics(), _read_response_body(),
URLValidationError, and the redirect loop where response is reassigned.
- Around line 117-118: The exception message and warning currently interpolate
the raw URL (e.g., in the requests.exceptions.RequestException handler that
raises URLValidationError and the later warning at the same module), which can
leak tokenized subscription URLs; update those sites (the except block that
raises URLValidationError, the earlier raise around lines 158-159, and the
warning around line 171) to redact sensitive query parameters before including
the URL in any log or exception text—extract the URL variable used there and
pass it through a sanitizer function (e.g., redact_subscription_url(url) or a
small utility that masks token/query params) and use that redacted string in
processLogger.warning / raise URLValidationError messages while still chaining
the original exception (from exc) so debugging details remain without exposing
tokens.
In `@src/backend/core/tasks.py`:
- Line 40: channel.settings is untyped JSON so reading sync_interval as shown
can raise if it's a string or null; change the logic around the sync_interval
assignment in the function where sync_interval is computed (the line using
channel.settings.get("sync_interval", 300)) to coerce the value to an int with a
safe fallback per-channel (e.g., try/except or conditional parse) and then apply
max(parsed_value, 1) so a bad value doesn't raise and abort the loop — update
the code that assigns sync_interval to parse int(channel.settings.get(...)) with
a fallback to 300 on failure.
In
`@src/frontend/apps/calendars/src/features/calendar/components/scheduler/hooks/useSchedulerHandlers.ts`:
- Around line 111-115: The current drop guard in useSchedulerHandlers.ts only
checks info.event.editable (source) and fails to reject drops onto read-only
calendars; update UseSchedulerHandlersProps to accept the readOnlyCalendarUrls
set (thread it from Scheduler.tsx), and in the drop/receive handler (the block
using info.event.editable) also inspect info.newResource or info.newResource?.id
/ info.resource?.id (or info.draggedEl/resourceId as applicable) to determine
the target calendar URL/id and if it is present in readOnlyCalendarUrls call
info.revert() and return; ensure you reference these symbols
(useSchedulerHandlers, UseSchedulerHandlersProps, readOnlyCalendarUrls,
info.newResource/resourceId) so the handler blocks drops to subscription
(read-only) calendars.
In
`@src/frontend/apps/calendars/src/features/calendar/components/scheduler/Scheduler.tsx`:
- Around line 92-93: readOnlyCalendarUrlsRef.current is updated but existing
rendered events never get their `editable` flags recomputed, so when
subscriptionCalendarUrls arrives late drag/resize/editability remains stale; fix
by reacting to changes in subscriptionCalendarUrls: when
subscriptionCalendarUrls changes, remap the current fetched data into calendar
events (the same logic used where events are originally created) and set the
events state again so `editable` is recalculated (or trigger the same effect
that maps fetched data into events). Use readOnlyCalendarUrlsRef and
subscriptionCalendarUrls to determine read-only status during that remap so all
existing events update their `editable` flags immediately.
In `@src/frontend/apps/calendars/src/features/calendar/config.ts`:
- Around line 7-12: SYNC_POLL_INTERVAL and MAX_SUBSCRIPTIONS_PER_USER currently
accept negative or non-finite values because they use Number(...) || default;
change their exports to validate parsed values explicitly: parse the env into a
number, check Number.isFinite(value) and value > 0 (for
MAX_SUBSCRIPTIONS_PER_USER also ensure an integer via Math.floor or similar),
and if the check fails fall back to the existing defaults (60_000 and 20
respectively); update the exports to use these validated values so negative,
zero, NaN, or Infinity from NEXT_PUBLIC_* are rejected.
In
`@src/frontend/apps/calendars/src/features/calendar/contexts/CalendarContext.tsx`:
- Around line 126-146: The current URL-path comparison in
subscriptionCalendarUrls only strips a leading "/caldav" but must mirror the
backend normalizer that removes any prefix up to "/calendars/"; create a small
helper (e.g., normalizeCalPath) and use it both when building subscriptionPaths
from subscriptionChannelsData (caldav_path) and when extracting calPath from new
URL(cal.url) so both sides apply the same normalization: trim trailing slashes,
then if the path contains "/calendars/" take the substring starting at
"/calendars/" (otherwise fall back to removing a leading "/caldav" or leaving
as-is), and then compare those normalized strings in subscriptionPaths.has(...)
inside the useMemo that defines subscriptionCalendarUrls.
---
Minor comments:
In
`@src/frontend/apps/calendars/src/features/calendar/components/calendar-list/AddSubscriptionModal.tsx`:
- Around line 95-103: The URL input's change handler should clear the mutation
error so the field doesn't stay stuck in error state; update the onChange for
the URL input in AddSubscriptionModal to call setUrl(e.target.value) and then
reset the createMutation state (e.g., call createMutation.reset()) when
createMutation.isError is true so typing clears the previous error and the input
returns to default state.
In
`@src/frontend/apps/calendars/src/features/calendar/components/calendar-list/ColorPicker.tsx`:
- Around line 25-34: The color buttons currently only communicate selection via
CSS; update the button element in ColorPicker.tsx (the element using variables
c, value, onChange and className "calendar-modal__color-btn") to expose
selection to assistive tech by adding ARIA semantics: set aria-pressed={value
=== c} (or role="radio" + aria-checked={value === c}) and replace the terse
aria-label={c} with a descriptive label like aria-label={`Select color
${c}${value === c ? ', selected' : ''}`}; if there is a container for these
buttons, ensure it uses role="radiogroup" to complete the single-choice
semantics.
In
`@src/frontend/apps/calendars/src/features/calendar/components/calendar-list/SubscriptionStatusBadge.tsx`:
- Around line 41-45: The icon-only toggle button in SubscriptionStatusBadge uses
title for accessibility but needs an explicit accessible name; update the button
element (className "subscription-status__toggle" in the SubscriptionStatusBadge
component) to include aria-label={t("calendar.subscription.status.viewError")}
alongside the existing title and onClick handler (which toggles showError via
setShowError) so screen readers receive a reliable label.
- Around line 70-73: When rendering the error panel in SubscriptionStatusBadge,
handle empty error payloads by showing the same translated fallback used for the
stopped state instead of blank content: inside the showError block (where
channel.last_sync_error is currently rendered) render channel.last_sync_error if
truthy, otherwise call the same translation used by the stopped state fallback
(use the same translation key/function already used elsewhere in
SubscriptionStatusBadge) so a user-visible message appears whenever
last_sync_status === "error" but last_sync_error is empty.
---
Nitpick comments:
In `@Makefile`:
- Around line 273-281: The reset-db-full target hard-codes the database name and
user; introduce variables (e.g., DB_NAME ?= calendars and DB_USER ?= pgroot) in
the VARIABLES section and then replace literal 'calendars' and 'pgroot' in the
reset-db-full recipe (the SELECT pg_terminate_backend... WHERE datname =
'calendars'..., dropdb -U pgroot --if-exists calendars, createdb -U pgroot
calendars) with $(DB_NAME) and $(DB_USER) respectively so local overrides are
honored; keep using $(COMPOSE) and existing commands but reference these new
variables throughout the reset-db-full target.
In `@src/backend/core/api/serializers.py`:
- Around line 248-285: Both ChannelSubscriptionCreateSerializer and
ChannelSubscriptionUpdateSerializer duplicate the validate_source_url
implementation; extract that logic into a shared mixin (e.g.,
SourceUrlValidationMixin) that defines validate_source_url and import
URLValidationError/validate_subscription_url there, then have
ChannelSubscriptionCreateSerializer and ChannelSubscriptionUpdateSerializer
inherit from SourceUrlValidationMixin in addition to serializers.Serializer so
they can remove their local validate_source_url methods and reuse the single
implementation.
In `@src/backend/core/tests/test_calendar_subscription_api.py`:
- Around line 421-433: The test hard-codes the max subscription value as 20;
replace literal 20 with settings.MAX_SUBSCRIPTIONS_PER_USER in the loop that
creates subscriptions (for i in range(...)) and in the assertion that checks
Channel.objects.filter(...).count(), and update any other similar occurrences in
test_calendar_subscription_api.py (e.g., the related block around the later
test) to reference settings.MAX_SUBSCRIPTIONS_PER_USER so the test adapts to
configuration changes; ensure you import django.conf.settings at the top of the
test module if not already present.
In `@src/backend/core/tests/test_subscription_tasks.py`:
- Around line 47-50: The test currently only checks
send_with_options.call_count; extend it to assert each call's payload and
scheduling by examining mock_sync_one.send_with_options.call_args_list from
sync_all_subscriptions() and verifying that for each call the positional args
equal (str(channel.pk),) (or the expected channel PK string values) and that the
keyword arg "delay" exists and is a numeric type (int/float) to ensure correct
task arguments and scheduling; reference the sync_all_subscriptions invocation
and mock_sync_one.send_with_options to locate where to add these assertions.
In
`@src/frontend/apps/calendars/src/features/calendar/components/calendar-list/CalendarList.tsx`:
- Around line 170-185: The delete handler handleDeleteSubscription currently
swallows errors from deleteSubMutation.mutateAsync; update it to catch errors,
notify the user (e.g., show a toast or set an error state) with the mutation
error message, and optionally log the error before allowing the finally block to
clear deletingPaths; ensure you still call refreshCalendars only on success (or
explicitly after handling failures if desired) and keep references to
deleteSubMutation.mutateAsync, refreshCalendars, setDeletingPaths unchanged so
the change is localized to adding a try/catch around the mutation and invoking
the user-facing notification in the catch.
In
`@src/frontend/apps/calendars/src/features/calendar/components/calendar-list/SubscriptionCalendarSection.tsx`:
- Around line 111-123: The onClick handler on the button in
SubscriptionCalendarSection.tsx redundantly guards with !isLimitReached even
though the button already uses disabled={isLimitReached}; simplify the handler
by removing the condition and directly calling setIsAddModalOpen(true) in the
onClick (keep the disabled prop as-is) so clicks open the add modal when enabled
and are naturally ignored when disabled.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 63d9a04e-892f-4f03-bf77-cb159b1866fe
📒 Files selected for processing (38)
Makefilecompose.yamldocs/external-subscriptions-test-plan.mdsrc/backend/calendars/settings.pysrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets_caldav.pysrc/backend/core/api/viewsets_channels.pysrc/backend/core/factories.pysrc/backend/core/management/__init__.pysrc/backend/core/management/commands/run_subscription_scheduler.pysrc/backend/core/services/subscription_sync_service.pysrc/backend/core/services/url_validation.pysrc/backend/core/tasks.pysrc/backend/core/tests/test_calendar_subscription_api.pysrc/backend/core/tests/test_subscription_proxy.pysrc/backend/core/tests/test_subscription_sync.pysrc/backend/core/tests/test_subscription_tasks.pysrc/backend/core/tests/test_url_validation.pysrc/frontend/apps/calendars/src/features/calendar/api.tssrc/frontend/apps/calendars/src/features/calendar/components/calendar-list/AddSubscriptionModal.tsxsrc/frontend/apps/calendars/src/features/calendar/components/calendar-list/CalendarList.tsxsrc/frontend/apps/calendars/src/features/calendar/components/calendar-list/CalendarModal.tsxsrc/frontend/apps/calendars/src/features/calendar/components/calendar-list/ColorPicker.tsxsrc/frontend/apps/calendars/src/features/calendar/components/calendar-list/EditSubscriptionModal.tsxsrc/frontend/apps/calendars/src/features/calendar/components/calendar-list/SubscriptionCalendarSection.tsxsrc/frontend/apps/calendars/src/features/calendar/components/calendar-list/SubscriptionStatusBadge.tsxsrc/frontend/apps/calendars/src/features/calendar/components/scheduler/EventModal.scsssrc/frontend/apps/calendars/src/features/calendar/components/scheduler/ReadOnlyEventModal.tsxsrc/frontend/apps/calendars/src/features/calendar/components/scheduler/Scheduler.tsxsrc/frontend/apps/calendars/src/features/calendar/components/scheduler/hooks/useSchedulerHandlers.tssrc/frontend/apps/calendars/src/features/calendar/components/scheduler/hooks/useSchedulerInit.tssrc/frontend/apps/calendars/src/features/calendar/components/scheduler/types.tssrc/frontend/apps/calendars/src/features/calendar/config.tssrc/frontend/apps/calendars/src/features/calendar/contexts/CalendarContext.tsxsrc/frontend/apps/calendars/src/features/calendar/hooks/useCalendars.tssrc/frontend/apps/calendars/src/features/calendar/services/dav/EventCalendarAdapter.tssrc/frontend/apps/calendars/src/features/calendar/services/dav/types/event-calendar.tssrc/frontend/apps/calendars/src/features/i18n/translations.json
sylvinus
force-pushed
the
feat/import-external-feed
branch
from
0c228f7 to
290a8df
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (11)
src/backend/core/tasks.py (1)
42-42:⚠️ Potential issue | 🟠 MajorCoerce
sync_intervalout of JSON before using it in arithmetic.
channel.settingsis untyped JSON. A persisted"300"ornullhere raises and stops dispatching every remaining subscription instead of just falling back for the bad row.Suggested hardening
- sync_interval = max(channel.settings.get("sync_interval", 300), 1) + raw_sync_interval = channel.settings.get("sync_interval", 300) + try: + sync_interval = max(int(raw_sync_interval), 1) + except (TypeError, ValueError): + logger.warning( + "Invalid sync_interval for channel %s: %r", + channel.pk, + raw_sync_interval, + ) + sync_interval = 300🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/tasks.py` at line 42, The code reads sync_interval directly from channel.settings and may get a string or null; change the assignment that sets sync_interval to first retrieve the raw value (e.g., raw = channel.settings.get("sync_interval")), then coerce it to an int inside a try/except (or handle TypeError/ValueError) falling back to the default 300 on failure, and finally take max(coerced_value, 1). Update the line that currently assigns sync_interval so it uses this robust conversion (referencing sync_interval and channel.settings.get).src/frontend/apps/calendars/src/features/calendar/contexts/CalendarContext.tsx (1)
126-146:⚠️ Potential issue | 🟠 MajorNormalize subscription paths with the same rule used for DAV calendar URLs.
This matcher only strips a leading
/caldav. Ifcal.urlcontains a prefix like/api/v1.0/caldav/.../calendars/..., the normalized path never matchesch.caldav_path, so that subscription calendar is omitted fromsubscriptionCalendarUrlsand its events stay editable client-side.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/apps/calendars/src/features/calendar/contexts/CalendarContext.tsx` around lines 126 - 146, subscriptionCalendarUrls normalization only strips a leading "/caldav" from cal.url paths but does not normalize subscriptionChannelsData.caldav_path the same way, causing mismatches for URLs like "/api/v1.0/caldav/...". Update the useMemo for subscriptionCalendarUrls so both sides use the same normalization: for each cal compute calPath = new URL(cal.url).pathname.replace(/\/$/, ""); then derive normalizedPath by removing everything up to and including the first "/caldav" (e.g. normalizedPath = calPath.replace(/^.*\/caldav/, "") or by finding "/caldav" and taking substring), and apply the same trim of trailing slashes to subscriptionChannelsData.map(ch => ch.caldav_path.replace(/\/$/, "")) but also normalize ch.caldav_path by removing any leading prefixes before "/caldav" so the comparison subscriptionPaths.has(normalizedPath) succeeds; update variable names (subscriptionPaths, normalizedPath) in the subscriptionCalendarUrls computation accordingly.src/frontend/apps/calendars/src/features/calendar/components/scheduler/hooks/useSchedulerHandlers.ts (1)
111-115:⚠️ Potential issue | 🟠 MajorAlso reject drops whose destination calendar is read-only.
This guard still only checks the dragged event. A writable event moved onto a subscription resource will pass here because the target calendar/resource is never inspected, so cross-calendar drops can still reach a forbidden write path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/apps/calendars/src/features/calendar/components/scheduler/hooks/useSchedulerHandlers.ts` around lines 111 - 115, The current guard in useSchedulerHandlers.ts only checks the dragged event (info.event.editable) and misses read-only destination calendars/resources; update the drop handler to also inspect the drop target's calendar/resource editability (e.g., check info.resource, info.newResource, or the destination calendar object provided by the FullCalendar drop event) and if the destination is read-only reject the drop by calling info.revert() and returning. Locate the block with info.event.editable === false and add a second conditional that looks up the destination resource/calendar's editable/subscription flag (falling back to any destination resource id on info.event) and reverts the operation when that destination is not writable.src/backend/core/services/subscription_sync_service.py (4)
115-118:⚠️ Potential issue | 🟠 MajorTreat a 304 as a clean sync.
This branch still updates
last_sync_at, but it leaveserror_countandlast_sync_erroruntouched. After transient failures, repeated 304 responses still keep the channel on the path to auto-stop.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/services/subscription_sync_service.py` around lines 115 - 118, The 304 branch treats the sync as partially successful but doesn't clear failure state; update the channel's failure tracking when status_code == 304 by resetting channel.settings["error_count"] to 0 and clearing channel.settings["last_sync_error"] (e.g., set to None or empty string) before calling self._save_channel(channel) so that SubscriptionSyncService's handling (in the same method where status_code is checked and self._save_channel is called) treats a 304 as a clean sync.
202-204:⚠️ Potential issue | 🔴 Critical
sync_events()still bypasses the channel lock.This public entry point still writes directly to SabreDAV without acquiring the per-channel lock, and
src/backend/core/api/viewsets_channels.pycalls it from request threads during create/update. That can overlap with scheduler-drivensync_channel()runs and interleave PUT/DELETE operations against the same calendar.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/services/subscription_sync_service.py` around lines 202 - 204, sync_events currently bypasses the per-channel lock and can race with scheduler-driven sync_channel runs; change sync_events to acquire the same per-channel lock used by sync_channel before calling _sync_events (or else delegate to the locked sync_channel helper) so all mutations to the SabreDAV calendar for the same channel serialize. Locate sync_events and modify it to obtain the channel lock (using the existing lock mechanism used by sync_channel or the per-channel lock helper) around the call to _sync_events(user, caldav_path, ics_data) and release the lock after completion to prevent concurrent PUT/DELETE interleaving.
79-87:⚠️ Potential issue | 🔴 CriticalUse an ownership token for the Redis lock.
The lock still stores a constant value and always deletes in
finally. If a sync runs longer thanSYNC_LOCK_TIMEOUT, another worker can acquire the key and the first worker will then delete the second worker's lease on exit, reopening the calendar to concurrent syncs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/services/subscription_sync_service.py` around lines 79 - 87, The lock currently stores a constant value and unconditionally deletes the key in finally, which can remove another worker's lease; change to use an ownership token: generate a unique token (e.g. uuid4) and call cache.add(lock_key, token, timeout=SYNC_LOCK_TIMEOUT) to acquire, then in finally only delete the lock if cache.get(lock_key) == token so you don't remove someone else's lock; keep the rest of the flow (lock_key, SYNC_LOCK_TIMEOUT, cache.add, cache.get, cache.delete, _do_sync, channel_id) the same.
383-416:⚠️ Potential issue | 🟠 MajorThe event comparison is still dropping real changes.
_event_props()still flattens each property tostr(value), which discards parameters and subcomponents. Changes likeATTENDEE;PARTSTAT=...,CN=..., orVALARMedits will compare equal and never trigger an update.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/services/subscription_sync_service.py` around lines 383 - 416, _event_props currently flattens each property to str(value) which drops parameters and subcomponents (so ATTENDEE params, CN, VALARM, etc. are lost); change SubscriptionSyncService._event_props to produce a stable serialized representation that includes both the property's serialized value and its parameters/subcomponents (e.g. use the icalendar value's to_ical() output (decoded) and include sorted params/items or serialize the full subcomponent via component.to_ical() per property name) instead of str(v), so SubscriptionSyncService._events_differ comparison will catch parameter/subcomponent changes.src/backend/core/services/url_validation.py (2)
117-118:⚠️ Potential issue | 🟠 MajorRedact subscription URLs before logging or surfacing fetch errors.
These messages can still carry the full feed URL via
requestsexception text, and Line 171 logs the rawurldirectly.SubscriptionSyncService._handle_error()persistsstr(exc)intolast_sync_error, so signed feed URLs can leak to logs, the database, and the frontend.Also applies to: 158-159, 171-171
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/services/url_validation.py` around lines 117 - 118, The exception text and raw URL are leaking signed feed URLs; update URL fetch error handling and SubscriptionSyncService._handle_error to redact sensitive query params before logging or persisting errors: create/use a helper (e.g., redact_url or sanitize_url) that strips or replaces query strings/token parameters from the `url` and any exception messages, call it when constructing the URLValidationError (raised in the fetch exception handler) and before assigning or logging `last_sync_error` in SubscriptionSyncService._handle_error, and ensure processLogger/error messages use the redacted URL or redacted exception string rather than the raw `url` or str(exc).
128-159:⚠️ Potential issue | 🟠 MajorClose every streamed response and normalize body-read failures.
The redirect loop still replaces
responsewithout closing the previous object, andfetch_ics()still returns/raises on 304/non-200 without closing the final streamed response. Also, an exception fromiter_content()escapes as a rawrequestserror, so callers bypass theURLValidationErrorhandling path.Also applies to: 218-226
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/services/url_validation.py` around lines 128 - 159, The redirect loop and body-reading paths leak streamed responses and allow raw requests exceptions to escape; before replacing the `response` object in the redirect loop in url_validation.py (the block using redirect_count, urljoin, and requests.get) call `response.close()` on the previous response, and do the same in the other redirect loop (lines ~218-226) so the prior streamed response is closed before reassigning; in `fetch_ics()` ensure the final streamed response is closed on all non-200/304 exits and wrap the body-read (`iter_content()`/any streaming reads) in a try/except that catches requests exceptions and raises `URLValidationError` (including the original exception as cause), and use a finally block to always call `response.close()` so no streamed response is leaked.src/backend/core/api/viewsets_channels.py (2)
241-253:⚠️ Potential issue | 🟠 MajorDon't overwrite partial sync failures with
"ok".Both immediate-sync paths still ignore the returned
SyncResultand unconditionally persistlast_sync_status = "ok". If some event PUT/DELETE operations fail, the subscription will look healthy even though the imported calendar is incomplete. Persist status/error fromresult.errorsin both blocks.Also applies to: 343-355
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/api/viewsets_channels.py` around lines 241 - 253, The immediate-sync blocks call SubscriptionSyncService().sync_events(...) but ignore its SyncResult and unconditionally set channel.settings["last_sync_status"]="ok"; change both occurrences (the block around service.sync_events and the duplicate later) to capture the return value (e.g., result = service.sync_events(request.user, caldav_path, ics_data)), examine result.errors or result.success flag, set channel.settings["last_sync_status"] to "ok" only when there are no errors and otherwise set it to "error" (or a more specific status) and persist result.errors (e.g., channel.settings["last_sync_error"] = serialized error details) along with last_sync_at/etag/last_modified, then call channel.save(update_fields=[...]) so the persisted status reflects failed PUT/DELETE operations reported by SyncResult.
128-141:⚠️ Potential issue | 🟠 MajorMake the subscription quota check atomic.
Line 129 still does a standalone
count()before any write, so concurrent POSTs can both pass and create more thanMAX_SUBSCRIPTIONS_PER_USER. Serialize the check-and-create path per user before creating the SabreDAV calendar and saving the channel.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/api/viewsets_channels.py` around lines 128 - 141, The quota check is non-atomic causing race conditions; wrap the check-and-create in a database transaction and acquire a per-user row lock (e.g., inside transaction.atomic() and using models.User.objects.select_for_update().get(pk=request.user.pk)) before re-counting models.Channel.objects.filter(user=request.user, type="ical-subscription").count(), then proceed to create the SabreDAV calendar and save the Channel only while the lock/transaction is held; this serializes concurrent POSTs for the same request.user and ensures the MAX_SUBSCRIPTIONS_PER_USER check (settings.MAX_SUBSCRIPTIONS_PER_USER) is enforced.
🧹 Nitpick comments (1)
src/frontend/apps/calendars/src/features/calendar/components/calendar-list/CalendarList.tsx (1)
151-188: Extract the subscription path matching into a shared helper.
CalendarListandCalendarContextnow implement the same calendar/subscription path matching separately. Centralizing that normalization would keep list grouping and read-only enforcement from drifting again.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/apps/calendars/src/features/calendar/components/calendar-list/CalendarList.tsx` around lines 151 - 188, Both CalendarList and CalendarContext duplicate caldav path normalization and matching logic; extract that into a single helper (e.g., normalizeCaldavPath or caldavPathKey) that takes a calendar URL or ch.caldav_path and returns the canonical path used for comparisons. Replace the inline calls to extractCaldavPath + Set building (subscriptionPaths) and all checks like subscriptionPaths.has(calPath) and the path lookups in the subscription sort (where you compare pathA/pathB to ch.caldav_path) to use this new helper so both CalendarList and CalendarContext use the exact same normalization and matching logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/external-subscriptions-test-plan.md`:
- Around line 19-23: Extend the "1.2 Validation du formulaire" test list to
include SSRF-specific cases: add checks for URLs pointing to localhost (e.g.,
http(s)://localhost, http://127.0.0.1, http://[::1]) and private RFC1918
addresses (e.g., 10.x.x.x, 192.168.x.x, 172.16.x.x-172.31.x.x) that must be
rejected; also add tests for hostnames that resolve to private IPs and for URLs
that redirect (3xx) to private/local addresses — verify server rejects them and
surfaces an appropriate error. Ensure the new cases mention both direct target
and redirect-chain scenarios in the "1.2 Validation du formulaire" section so
SSRF protections are exercised.
In `@src/frontend/apps/calendars/src/features/calendar/api.ts`:
- Around line 128-137: The SubscriptionChannel interface's last_sync_status
union currently omits the "stopped" state; update the SubscriptionChannel type
to include "stopped" (i.e., change last_sync_status: "pending" | "ok" | "error"
to last_sync_status: "pending" | "ok" | "error" | "stopped") so frontend
consumers can type-check API responses without unsafe casts; ensure any code
using last_sync_status (e.g., components or selectors referencing
SubscriptionChannel.last_sync_status) handles the new "stopped" value
appropriately.
In
`@src/frontend/apps/calendars/src/features/calendar/components/calendar-list/EditSubscriptionModal.tsx`:
- Around line 128-139: The URL input currently shows a generic error when
updateMutation.isError is true; instead extract the server-side validation
message from the mutation error and surface it in the Input text so users see
the specific backend reason (e.g., non-HTTPS or SSRF). Update the Input usage
(the component rendering sourceUrl) to compute a clearer message from
updateMutation.error (for example checking
updateMutation.error?.response?.data?.errors?.source_url or
updateMutation.error?.message) and pass that string to the text prop, falling
back to t("calendar.subscription.edit.error") when no server message exists;
keep state toggled by updateMutation.isError as before. Ensure you reference the
updateMutation object and the Input component when changing the code.
---
Duplicate comments:
In `@src/backend/core/api/viewsets_channels.py`:
- Around line 241-253: The immediate-sync blocks call
SubscriptionSyncService().sync_events(...) but ignore its SyncResult and
unconditionally set channel.settings["last_sync_status"]="ok"; change both
occurrences (the block around service.sync_events and the duplicate later) to
capture the return value (e.g., result = service.sync_events(request.user,
caldav_path, ics_data)), examine result.errors or result.success flag, set
channel.settings["last_sync_status"] to "ok" only when there are no errors and
otherwise set it to "error" (or a more specific status) and persist
result.errors (e.g., channel.settings["last_sync_error"] = serialized error
details) along with last_sync_at/etag/last_modified, then call
channel.save(update_fields=[...]) so the persisted status reflects failed
PUT/DELETE operations reported by SyncResult.
- Around line 128-141: The quota check is non-atomic causing race conditions;
wrap the check-and-create in a database transaction and acquire a per-user row
lock (e.g., inside transaction.atomic() and using
models.User.objects.select_for_update().get(pk=request.user.pk)) before
re-counting models.Channel.objects.filter(user=request.user,
type="ical-subscription").count(), then proceed to create the SabreDAV calendar
and save the Channel only while the lock/transaction is held; this serializes
concurrent POSTs for the same request.user and ensures the
MAX_SUBSCRIPTIONS_PER_USER check (settings.MAX_SUBSCRIPTIONS_PER_USER) is
enforced.
In `@src/backend/core/services/subscription_sync_service.py`:
- Around line 115-118: The 304 branch treats the sync as partially successful
but doesn't clear failure state; update the channel's failure tracking when
status_code == 304 by resetting channel.settings["error_count"] to 0 and
clearing channel.settings["last_sync_error"] (e.g., set to None or empty string)
before calling self._save_channel(channel) so that SubscriptionSyncService's
handling (in the same method where status_code is checked and self._save_channel
is called) treats a 304 as a clean sync.
- Around line 202-204: sync_events currently bypasses the per-channel lock and
can race with scheduler-driven sync_channel runs; change sync_events to acquire
the same per-channel lock used by sync_channel before calling _sync_events (or
else delegate to the locked sync_channel helper) so all mutations to the
SabreDAV calendar for the same channel serialize. Locate sync_events and modify
it to obtain the channel lock (using the existing lock mechanism used by
sync_channel or the per-channel lock helper) around the call to
_sync_events(user, caldav_path, ics_data) and release the lock after completion
to prevent concurrent PUT/DELETE interleaving.
- Around line 79-87: The lock currently stores a constant value and
unconditionally deletes the key in finally, which can remove another worker's
lease; change to use an ownership token: generate a unique token (e.g. uuid4)
and call cache.add(lock_key, token, timeout=SYNC_LOCK_TIMEOUT) to acquire, then
in finally only delete the lock if cache.get(lock_key) == token so you don't
remove someone else's lock; keep the rest of the flow (lock_key,
SYNC_LOCK_TIMEOUT, cache.add, cache.get, cache.delete, _do_sync, channel_id) the
same.
- Around line 383-416: _event_props currently flattens each property to
str(value) which drops parameters and subcomponents (so ATTENDEE params, CN,
VALARM, etc. are lost); change SubscriptionSyncService._event_props to produce a
stable serialized representation that includes both the property's serialized
value and its parameters/subcomponents (e.g. use the icalendar value's to_ical()
output (decoded) and include sorted params/items or serialize the full
subcomponent via component.to_ical() per property name) instead of str(v), so
SubscriptionSyncService._events_differ comparison will catch
parameter/subcomponent changes.
In `@src/backend/core/services/url_validation.py`:
- Around line 117-118: The exception text and raw URL are leaking signed feed
URLs; update URL fetch error handling and SubscriptionSyncService._handle_error
to redact sensitive query params before logging or persisting errors: create/use
a helper (e.g., redact_url or sanitize_url) that strips or replaces query
strings/token parameters from the `url` and any exception messages, call it when
constructing the URLValidationError (raised in the fetch exception handler) and
before assigning or logging `last_sync_error` in
SubscriptionSyncService._handle_error, and ensure processLogger/error messages
use the redacted URL or redacted exception string rather than the raw `url` or
str(exc).
- Around line 128-159: The redirect loop and body-reading paths leak streamed
responses and allow raw requests exceptions to escape; before replacing the
`response` object in the redirect loop in url_validation.py (the block using
redirect_count, urljoin, and requests.get) call `response.close()` on the
previous response, and do the same in the other redirect loop (lines ~218-226)
so the prior streamed response is closed before reassigning; in `fetch_ics()`
ensure the final streamed response is closed on all non-200/304 exits and wrap
the body-read (`iter_content()`/any streaming reads) in a try/except that
catches requests exceptions and raises `URLValidationError` (including the
original exception as cause), and use a finally block to always call
`response.close()` so no streamed response is leaked.
In `@src/backend/core/tasks.py`:
- Line 42: The code reads sync_interval directly from channel.settings and may
get a string or null; change the assignment that sets sync_interval to first
retrieve the raw value (e.g., raw = channel.settings.get("sync_interval")), then
coerce it to an int inside a try/except (or handle TypeError/ValueError) falling
back to the default 300 on failure, and finally take max(coerced_value, 1).
Update the line that currently assigns sync_interval so it uses this robust
conversion (referencing sync_interval and channel.settings.get).
In
`@src/frontend/apps/calendars/src/features/calendar/components/scheduler/hooks/useSchedulerHandlers.ts`:
- Around line 111-115: The current guard in useSchedulerHandlers.ts only checks
the dragged event (info.event.editable) and misses read-only destination
calendars/resources; update the drop handler to also inspect the drop target's
calendar/resource editability (e.g., check info.resource, info.newResource, or
the destination calendar object provided by the FullCalendar drop event) and if
the destination is read-only reject the drop by calling info.revert() and
returning. Locate the block with info.event.editable === false and add a second
conditional that looks up the destination resource/calendar's
editable/subscription flag (falling back to any destination resource id on
info.event) and reverts the operation when that destination is not writable.
In
`@src/frontend/apps/calendars/src/features/calendar/contexts/CalendarContext.tsx`:
- Around line 126-146: subscriptionCalendarUrls normalization only strips a
leading "/caldav" from cal.url paths but does not normalize
subscriptionChannelsData.caldav_path the same way, causing mismatches for URLs
like "/api/v1.0/caldav/...". Update the useMemo for subscriptionCalendarUrls so
both sides use the same normalization: for each cal compute calPath = new
URL(cal.url).pathname.replace(/\/$/, ""); then derive normalizedPath by removing
everything up to and including the first "/caldav" (e.g. normalizedPath =
calPath.replace(/^.*\/caldav/, "") or by finding "/caldav" and taking
substring), and apply the same trim of trailing slashes to
subscriptionChannelsData.map(ch => ch.caldav_path.replace(/\/$/, "")) but also
normalize ch.caldav_path by removing any leading prefixes before "/caldav" so
the comparison subscriptionPaths.has(normalizedPath) succeeds; update variable
names (subscriptionPaths, normalizedPath) in the subscriptionCalendarUrls
computation accordingly.
---
Nitpick comments:
In
`@src/frontend/apps/calendars/src/features/calendar/components/calendar-list/CalendarList.tsx`:
- Around line 151-188: Both CalendarList and CalendarContext duplicate caldav
path normalization and matching logic; extract that into a single helper (e.g.,
normalizeCaldavPath or caldavPathKey) that takes a calendar URL or
ch.caldav_path and returns the canonical path used for comparisons. Replace the
inline calls to extractCaldavPath + Set building (subscriptionPaths) and all
checks like subscriptionPaths.has(calPath) and the path lookups in the
subscription sort (where you compare pathA/pathB to ch.caldav_path) to use this
new helper so both CalendarList and CalendarContext use the exact same
normalization and matching logic.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b0668f72-3a38-4430-b9df-5d6a177cd1ec
📒 Files selected for processing (38)
Makefilecompose.yamldocs/external-subscriptions-test-plan.mdsrc/backend/calendars/settings.pysrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets_caldav.pysrc/backend/core/api/viewsets_channels.pysrc/backend/core/factories.pysrc/backend/core/management/__init__.pysrc/backend/core/management/commands/run_subscription_scheduler.pysrc/backend/core/services/subscription_sync_service.pysrc/backend/core/services/url_validation.pysrc/backend/core/tasks.pysrc/backend/core/tests/test_calendar_subscription_api.pysrc/backend/core/tests/test_subscription_proxy.pysrc/backend/core/tests/test_subscription_sync.pysrc/backend/core/tests/test_subscription_tasks.pysrc/backend/core/tests/test_url_validation.pysrc/frontend/apps/calendars/src/features/calendar/api.tssrc/frontend/apps/calendars/src/features/calendar/components/calendar-list/AddSubscriptionModal.tsxsrc/frontend/apps/calendars/src/features/calendar/components/calendar-list/CalendarList.tsxsrc/frontend/apps/calendars/src/features/calendar/components/calendar-list/CalendarModal.tsxsrc/frontend/apps/calendars/src/features/calendar/components/calendar-list/ColorPicker.tsxsrc/frontend/apps/calendars/src/features/calendar/components/calendar-list/EditSubscriptionModal.tsxsrc/frontend/apps/calendars/src/features/calendar/components/calendar-list/SubscriptionCalendarSection.tsxsrc/frontend/apps/calendars/src/features/calendar/components/calendar-list/SubscriptionStatusBadge.tsxsrc/frontend/apps/calendars/src/features/calendar/components/scheduler/EventModal.scsssrc/frontend/apps/calendars/src/features/calendar/components/scheduler/ReadOnlyEventModal.tsxsrc/frontend/apps/calendars/src/features/calendar/components/scheduler/Scheduler.tsxsrc/frontend/apps/calendars/src/features/calendar/components/scheduler/hooks/useSchedulerHandlers.tssrc/frontend/apps/calendars/src/features/calendar/components/scheduler/hooks/useSchedulerInit.tssrc/frontend/apps/calendars/src/features/calendar/components/scheduler/types.tssrc/frontend/apps/calendars/src/features/calendar/config.tssrc/frontend/apps/calendars/src/features/calendar/contexts/CalendarContext.tsxsrc/frontend/apps/calendars/src/features/calendar/hooks/useCalendars.tssrc/frontend/apps/calendars/src/features/calendar/services/dav/EventCalendarAdapter.tssrc/frontend/apps/calendars/src/features/calendar/services/dav/types/event-calendar.tssrc/frontend/apps/calendars/src/features/i18n/translations.json
✅ Files skipped from review due to trivial changes (7)
- src/frontend/apps/calendars/src/features/calendar/components/scheduler/EventModal.scss
- src/frontend/apps/calendars/src/features/calendar/config.ts
- src/backend/core/tests/test_subscription_proxy.py
- src/backend/core/management/commands/run_subscription_scheduler.py
- src/frontend/apps/calendars/src/features/i18n/translations.json
- src/backend/core/tests/test_url_validation.py
- src/frontend/apps/calendars/src/features/calendar/components/calendar-list/ColorPicker.tsx
🚧 Files skipped from review as they are similar to previous changes (17)
- src/frontend/apps/calendars/src/features/calendar/services/dav/types/event-calendar.ts
- src/frontend/apps/calendars/src/features/calendar/components/scheduler/hooks/useSchedulerInit.ts
- src/backend/calendars/settings.py
- Makefile
- src/frontend/apps/calendars/src/features/calendar/components/scheduler/types.ts
- src/frontend/apps/calendars/src/features/calendar/components/calendar-list/SubscriptionStatusBadge.tsx
- src/backend/core/tests/test_subscription_tasks.py
- src/frontend/apps/calendars/src/features/calendar/components/calendar-list/AddSubscriptionModal.tsx
- compose.yaml
- src/backend/core/tests/test_calendar_subscription_api.py
- src/backend/core/factories.py
- src/frontend/apps/calendars/src/features/calendar/components/scheduler/Scheduler.tsx
- src/backend/core/tests/test_subscription_sync.py
- src/frontend/apps/calendars/src/features/calendar/components/calendar-list/SubscriptionCalendarSection.tsx
- src/backend/core/api/viewsets_caldav.py
- src/frontend/apps/calendars/src/features/calendar/services/dav/EventCalendarAdapter.ts
- src/frontend/apps/calendars/src/features/calendar/components/scheduler/ReadOnlyEventModal.tsx
Add URL validation with SSRF protection for external calendar subscription URLs. Includes hostname resolution checks, private IP blocking, safe redirect handling, conditional request support (ETag/If-Modified-Since), and response size limits.
Add diff-based sync service that compares UIDs from an external ICS feed with events in a SabreDAV calendar, then creates, updates, or deletes events as needed. Includes Redis-based locking, auto-stop after consecutive errors, infinite RRULE capping, and volatile property filtering.
Add Dramatiq tasks for syncing one or all active subscriptions, with staggered dispatch based on channel ID. Add a management command that runs the scheduler loop, dispatching sync_all_subscriptions at a configurable interval.
Add serializers and viewset actions for creating, updating, deleting, and reactivating subscription channels. Includes per-user limit enforcement, immediate sync on creation, event purge on URL change, and SabreDAV calendar cleanup on deletion. Add ICalSubscriptionChannelFactory for tests.
Add read-only enforcement in the CalDAV proxy: write methods (PUT, DELETE, MOVE, PROPPATCH) are rejected with 403 for calendars owned by an ical-subscription channel, even if the subscription is stopped.
Add a subscription-scheduler service in compose.yaml that runs the management command for periodic subscription syncs. Update Makefile start targets to include the new service. Add reset-db-full target for full database recreation.
Add API functions and React Query hooks for subscription channel CRUD: create, update, delete, reactivate, and list. Add config.ts with SYNC_POLL_INTERVAL constant for periodic refresh.
Add components for managing external calendar subscriptions: add/edit modals, status badge, calendar section in the sidebar. Extract ColorPicker from CalendarModal for reuse in subscription modals.
Mark subscription calendar events as non-editable in FullCalendar. Open a read-only modal on click instead of the edit form. Block drag-and-drop and resize on subscription events. Add periodic calendar refresh for sync updates. Filter subscription calendars from the event creation calendar picker.
Add translation keys for subscription management UI: add/edit modals, status messages, section headers, and read-only event modal. Add external subscriptions test plan document.
sylvinus
force-pushed
the
feat/import-external-feed
branch
from
2d69931 to
c52dde3
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/backend/core/services/url_validation.py (1)
27-40:⚠️ Potential issue | 🟠 MajorRedact URL credentials too.
_redact_url()drops the query string, but it still reusesparsed.netloc, so a feed URL likescheme://user:pass@host/pathwill leak credentials intolast_sync_errorand logs. Rebuild the authority fromhostname/portonly before re-serializing.Suggested fix
def _redact_url(url: str) -> str: """Strip query string and fragment to avoid leaking signed tokens. @@ """ try: parsed = urlparse(url) - return urlunparse((parsed.scheme, parsed.netloc, parsed.path, "", "", "")) + hostname = parsed.hostname or "" + if ":" in hostname and not hostname.startswith("["): + hostname = f"[{hostname}]" + netloc = f"{hostname}:{parsed.port}" if parsed.port else hostname + return urlunparse((parsed.scheme, netloc, parsed.path, "", "", "")) except Exception: # noqa: BLE001 # pylint: disable=broad-exception-caught return "[unparseable url]"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/services/url_validation.py` around lines 27 - 40, _redact_url currently reuses parsed.netloc which can leak user:pass credentials; modify _redact_url to reconstruct the authority from parsed.hostname and parsed.port only (e.g., host or host:port) before calling urlunparse so credentials from parsed.netloc are not included, falling back to an empty authority (or parsed.netloc only if hostname is None) to avoid breaking unparseable inputs; keep the rest of the behavior (strip params/query/fragment) and preserve the existing exception handling.
🧹 Nitpick comments (2)
Makefile (1)
278-280: Replace fixed sleep with DB readiness check forreset-db-full.Using a fixed delay can flake on slower environments. Prefer waiting until PostgreSQL is actually ready before drop/create steps.
Proposed patch
@$(COMPOSE) up -d postgresql - `@sleep` 2 + `@until` $(COMPOSE) exec -T postgresql pg_isready -U pgroot >/dev/null 2>&1; do sleep 1; done `@echo` "$(BOLD)Dropping and recreating database...$(RESET)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 278 - 280, Replace the fixed "@sleep 2" wait with an active readiness loop that polls PostgreSQL until it is ready before proceeding with the drop/create step: use the existing "$(COMPOSE)" invocation to target the "postgresql" service (e.g., call "pg_isready" or a minimal "psql -c" check against the DB in a short retry loop) and only continue when the command succeeds; update the Makefile snippet that currently contains "@sleep 2" to run that readiness check and fail with a clear error if the DB never becomes ready after N attempts.src/backend/core/tests/test_calendar_subscription_api.py (1)
415-478: Avoid baking the default quota into these tests.Both cases hard-code
20, so a future change toMAX_SUBSCRIPTIONS_PER_USERwill break the suite for the wrong reason. Prefer overriding the setting to a small value or reading it from settings when seeding/asserting.Example refactor
+from django.test import override_settings + @@ -@pytest.mark.django_db +@pytest.mark.django_db +@override_settings(MAX_SUBSCRIPTIONS_PER_USER=2) class TestSubscriptionChannelLimit: @@ - # Create 20 subscription channels - for _ in range(20): + # Create the maximum allowed subscription channels + for _ in range(2): factories.ICalSubscriptionChannelFactory(user=user) - assert Channel.objects.filter(user=user, type="ical-subscription").count() == 20 + assert Channel.objects.filter(user=user, type="ical-subscription").count() == 2 @@ - # Create 19 subscription channels - for _ in range(19): + # Leave one slot available + for _ in range(1): factories.ICalSubscriptionChannelFactory(user=user) @@ - assert Channel.objects.filter(user=user, type="ical-subscription").count() == 20 + assert Channel.objects.filter(user=user, type="ical-subscription").count() == 2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/tests/test_calendar_subscription_api.py` around lines 415 - 478, The tests in TestSubscriptionChannelLimit hard-code “20” for the per-user subscription cap; update both test_create_subscription_limit_reached and test_create_subscription_under_limit to avoid baking in the default by either reading the limit from settings (e.g. import and use MAX_SUBSCRIPTIONS_PER_USER) or by overriding the setting (e.g. use override_settings to set MAX_SUBSCRIPTIONS_PER_USER to a small value) and then create that many factories.ICalSubscriptionChannelFactory(user=user) accordingly; ensure assertions and setup (mock_fetch behavior, CHANNELS_URL posts, and final counts using Channel.objects.filter(...).count()) use the dynamic limit variable instead of the literal 20.
🤖 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/services/url_validation.py`:
- Around line 241-248: The DNS-rebinding gap is that
_resolve_and_check(parsed.hostname) only checks one DNS answer but
requests.get() will re-resolve when opening the socket; fix by pinning the
validated IP at transport time: after _resolve_and_check resolves and returns
the safe IP(s), create and use a custom requests transport/HTTPAdapter that
opens the TCP socket to the pre-resolved IP (not the hostname) while setting the
Host header to parsed.hostname so TLS and virtual-hosting work; update the call
site that uses requests.get() to use a Session with this adapter (or, if
transport-level IP pinning cannot be implemented in this codepath, document and
enforce private-range egress blocking as a required deployment control).
In `@src/backend/core/tests/test_subscription_sync.py`:
- Around line 225-255: The test currently deletes the Channel before calling
SubscriptionSyncService.sync_channel so it only hits the "not found" branch;
change the test to delete the row mid-sync by using a side-effect on the mocked
fetch/cache call (e.g., set mock_fetch.side_effect or
mock_cache.add.side_effect) that deletes the Channel instance after the service
has loaded it but before save, then call service.sync_channel(channel_id) and
assert it returns False; reference the test function
test_sync_survives_channel_deletion, the mock_fetch/mock_cache mocks, and
SubscriptionSyncService.sync_channel so you delete the Channel inside the mock
side-effect rather than before calling sync_channel.
In
`@src/frontend/apps/calendars/src/features/calendar/components/scheduler/Scheduler.tsx`:
- Around line 406-413: The modal can be given a calendarUrl that was removed by
filtering subscription feeds; before passing calendarUrl to EventModal (from
Scheduler.tsx where davCalendars, subscriptionCalendarUrls and modalState are
used), compute the filtered writable list (e.g., filteredCalendars =
davCalendars.filter(cal => !subscriptionCalendarUrls.has(cal.url))) and derive a
sanitizedCalendarUrl that checks if modalState.calendarUrl is still present in
filteredCalendars, otherwise fall back to the first available writable calendar
URL or undefined; pass sanitizedCalendarUrl to EventModal instead of
modalState.calendarUrl so the modal never receives an invalid/non-writable
selection.
In
`@src/frontend/apps/calendars/src/features/calendar/contexts/CalendarContext.tsx`:
- Around line 356-363: The polling useEffect is causing duplicate event reloads
because refreshCalendars() already triggers Scheduler.tsx to refetch when
davCalendars changes; remove the direct calendarRef.current.refetchEvents() call
inside the interval in CalendarContext's useEffect so only refreshCalendars()
runs on each poll (or alternatively gate the direct refetch behind a flag to
ensure it does not run when davCalendars will change). Update the useEffect that
references refreshCalendars and calendarRef to eliminate the redundant
calendarRef.current.refetchEvents() invocation.
---
Duplicate comments:
In `@src/backend/core/services/url_validation.py`:
- Around line 27-40: _redact_url currently reuses parsed.netloc which can leak
user:pass credentials; modify _redact_url to reconstruct the authority from
parsed.hostname and parsed.port only (e.g., host or host:port) before calling
urlunparse so credentials from parsed.netloc are not included, falling back to
an empty authority (or parsed.netloc only if hostname is None) to avoid breaking
unparseable inputs; keep the rest of the behavior (strip params/query/fragment)
and preserve the existing exception handling.
---
Nitpick comments:
In `@Makefile`:
- Around line 278-280: Replace the fixed "@sleep 2" wait with an active
readiness loop that polls PostgreSQL until it is ready before proceeding with
the drop/create step: use the existing "$(COMPOSE)" invocation to target the
"postgresql" service (e.g., call "pg_isready" or a minimal "psql -c" check
against the DB in a short retry loop) and only continue when the command
succeeds; update the Makefile snippet that currently contains "@sleep 2" to run
that readiness check and fail with a clear error if the DB never becomes ready
after N attempts.
In `@src/backend/core/tests/test_calendar_subscription_api.py`:
- Around line 415-478: The tests in TestSubscriptionChannelLimit hard-code “20”
for the per-user subscription cap; update both
test_create_subscription_limit_reached and test_create_subscription_under_limit
to avoid baking in the default by either reading the limit from settings (e.g.
import and use MAX_SUBSCRIPTIONS_PER_USER) or by overriding the setting (e.g.
use override_settings to set MAX_SUBSCRIPTIONS_PER_USER to a small value) and
then create that many factories.ICalSubscriptionChannelFactory(user=user)
accordingly; ensure assertions and setup (mock_fetch behavior, CHANNELS_URL
posts, and final counts using Channel.objects.filter(...).count()) use the
dynamic limit variable instead of the literal 20.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9b37bf71-4f30-4a46-b387-36c668a42f4b
📒 Files selected for processing (19)
Makefilesrc/backend/calendars/settings.pysrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets_caldav.pysrc/backend/core/api/viewsets_channels.pysrc/backend/core/management/commands/run_subscription_scheduler.pysrc/backend/core/management/commands/sync_mailbox_acls.pysrc/backend/core/services/subscription_sync_service.pysrc/backend/core/services/url_validation.pysrc/backend/core/tasks.pysrc/backend/core/tests/test_calendar_subscription_api.pysrc/backend/core/tests/test_subscription_proxy.pysrc/backend/core/tests/test_subscription_sync.pysrc/backend/core/tests/test_subscription_tasks.pysrc/backend/core/tests/test_url_validation.pysrc/frontend/apps/calendars/src/features/calendar/components/scheduler/Scheduler.tsxsrc/frontend/apps/calendars/src/features/calendar/components/scheduler/hooks/useSchedulerHandlers.tssrc/frontend/apps/calendars/src/features/calendar/config.tssrc/frontend/apps/calendars/src/features/calendar/contexts/CalendarContext.tsx
✅ Files skipped from review due to trivial changes (2)
- src/backend/core/management/commands/sync_mailbox_acls.py
- src/backend/core/tests/test_subscription_tasks.py
🚧 Files skipped from review as they are similar to previous changes (9)
- src/backend/core/api/viewsets_caldav.py
- src/backend/core/management/commands/run_subscription_scheduler.py
- src/frontend/apps/calendars/src/features/calendar/config.ts
- src/frontend/apps/calendars/src/features/calendar/components/scheduler/hooks/useSchedulerHandlers.ts
- src/backend/core/api/serializers.py
- src/backend/core/tests/test_subscription_proxy.py
- src/backend/core/tasks.py
- src/backend/core/api/viewsets_channels.py
- src/backend/core/services/subscription_sync_service.py
There was a problem hiding this comment.
🧹 Nitpick comments (7)
src/frontend/apps/calendars/src/features/calendar/components/calendar-list/AddSubscriptionModal.tsx (1)
98-103: Show specific backend validation messages for consistency with EditSubscriptionModal.
EditSubscriptionModaluseserrorToString(updateMutation.error)to display specific backend errors (e.g., SSRF rejection, non-HTTPS URL), but this modal shows a generic error. Users won't know why their subscription URL was rejected.♻️ Proposed fix
+import { errorToString } from "@/features/api/APIError"; ... state={createMutation.isError ? "error" : "default"} text={ createMutation.isError - ? t("calendar.subscription.add.error") + ? errorToString(createMutation.error) || + t("calendar.subscription.add.error") : undefined }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/apps/calendars/src/features/calendar/components/calendar-list/AddSubscriptionModal.tsx` around lines 98 - 103, The AddSubscriptionModal currently shows a generic error message; update it to mirror EditSubscriptionModal by passing the backend validation details: use errorToString(createMutation.error) instead of the generic t("calendar.subscription.add.error") when createMutation.isError is true, and ensure errorToString is imported into AddSubscriptionModal; keep the state logic using createMutation.isError and only swap the text value to the errorToString output so users see specific backend rejection reasons (e.g., SSRF/non-HTTPS).src/backend/core/tasks.py (1)
66-73: Simplify UUID conversion and move import outside loop.The
uuidimport inside the loop is inefficient, anduuid.UUID(str(channel.pk))is redundant sincechannel.pkis already a UUID object.♻️ Proposed fix
`@register_task`(queue="sync") def sync_all_subscriptions(): """Fan out sync tasks for all active subscriptions due for sync.""" from django.utils import timezone as tz # noqa: PLC0415 from core.models import Channel # noqa: PLC0415 now = tz.now() channels = Channel.objects.filter( type="ical-subscription", is_active=True, ).iterator(chunk_size=500) dispatched = 0 for channel in channels: ... # Stagger delay based on channel ID (deterministic across restarts) - import uuid # noqa: PLC0415 - - delay_ms = (int(uuid.UUID(str(channel.pk))) % sync_interval) * 1000 + delay_ms = (channel.pk.int % sync_interval) * 1000 sync_one_subscription.send_with_options(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/tasks.py` around lines 66 - 73, The loop currently imports uuid and reconverts channel.pk unnecessarily; move the import uuid out of the loop (top-level module import) and compute the stagger using the UUID object's integer directly instead of uuid.UUID(str(channel.pk)); e.g., calculate delay_ms = (channel.pk.int % sync_interval) * 1000 before calling sync_one_subscription.send_with_options(args=(str(channel.pk),), delay=delay_ms) so the import is not repeated and the redundant conversion is removed.src/frontend/apps/calendars/src/features/calendar/components/scheduler/Scheduler.tsx (1)
406-434: Consider extracting writable calendar computation touseMemo.The IIFE pattern works but is uncommon in JSX. Extracting this to a memoized value would improve readability and make the dependencies explicit.
♻️ Suggested refactor
+ const writableCalendars = useMemo( + () => davCalendars.filter((cal) => !subscriptionCalendarUrls.has(cal.url)), + [davCalendars, subscriptionCalendarUrls], + ); + + const sanitizedCalendarUrl = useMemo(() => { + if (subscriptionCalendarUrls.has(modalState.calendarUrl)) { + return writableCalendars[0]?.url ?? ""; + } + return modalState.calendarUrl; + }, [modalState.calendarUrl, subscriptionCalendarUrls, writableCalendars]); {modalState.mode === "view" ? ( <ReadOnlyEventModal ... /> ) : ( - (() => { - const writableCalendars = davCalendars.filter( - (cal) => !subscriptionCalendarUrls.has(cal.url), - ); - const sanitizedCalendarUrl = subscriptionCalendarUrls.has( - modalState.calendarUrl, - ) - ? (writableCalendars[0]?.url ?? "") - : modalState.calendarUrl; - return ( - <EventModal - ... - calendarUrl={sanitizedCalendarUrl} - calendars={writableCalendars} - ... - /> - ); - })() + <EventModal + ... + calendarUrl={sanitizedCalendarUrl} + calendars={writableCalendars} + ... + /> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/apps/calendars/src/features/calendar/components/scheduler/Scheduler.tsx` around lines 406 - 434, The inline IIFE that computes writableCalendars and sanitizedCalendarUrl inside the JSX makes dependencies implicit and reduces readability; extract the computation into a memoized value using useMemo so it only recalculates when inputs change. Specifically, create a useMemo that derives writableCalendars = davCalendars.filter(cal => !subscriptionCalendarUrls.has(cal.url)) and sanitizedCalendarUrl = subscriptionCalendarUrls.has(modalState.calendarUrl) ? (writableCalendars[0]?.url ?? "") : modalState.calendarUrl, and then pass those memoized values into the EventModal props (EventModal, davCalendars, subscriptionCalendarUrls, modalState, writableCalendars, sanitizedCalendarUrl) so dependencies are explicit and the JSX no longer contains the IIFE.src/backend/core/api/serializers.py (1)
262-272: Consider extracting duplicatevalidate_source_urlto reduce duplication.The
validate_source_urlmethod is identical in bothChannelSubscriptionCreateSerializerandChannelSubscriptionUpdateSerializer. This could be extracted to a mixin class for better maintainability.♻️ Optional: Extract to a mixin
class SourceUrlValidationMixin: """Mixin providing SSRF-safe source URL validation.""" def validate_source_url(self, value): """Validate and normalize the source URL.""" from core.services.url_validation import ( URLValidationError, validate_subscription_url, ) try: return validate_subscription_url(value) except URLValidationError as exc: raise serializers.ValidationError(str(exc)) from exc class ChannelSubscriptionCreateSerializer(SourceUrlValidationMixin, serializers.Serializer): # ... class ChannelSubscriptionUpdateSerializer(SourceUrlValidationMixin, serializers.Serializer): # ...Also applies to: 283-293
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/api/serializers.py` around lines 262 - 272, The validate_source_url method is duplicated in ChannelSubscriptionCreateSerializer and ChannelSubscriptionUpdateSerializer; extract it into a new SourceUrlValidationMixin that defines validate_source_url (keeping the internal import of URLValidationError and validate_subscription_url and identical try/except that raises serializers.ValidationError), then have both ChannelSubscriptionCreateSerializer and ChannelSubscriptionUpdateSerializer inherit from SourceUrlValidationMixin and remove their local validate_source_url implementations so the shared mixin provides the behavior.src/frontend/apps/calendars/src/features/calendar/api.ts (1)
206-212: Consider reusingdeleteChannelfor subscription deletion.
deleteSubscriptionChannelis identical todeleteChannel(lines 115-119). Since both call the same endpoint, you could either reusedeleteChanneldirectly or export a single genericdeleteChannelfunction.♻️ Optional: Reuse existing deleteChannel
-/** - * Delete a subscription channel by ID. - */ -export const deleteSubscriptionChannel = async ( - channelId: string, -): Promise<void> => { - await fetchAPI(`channels/${channelId}/`, { - method: "DELETE", - }); -}; +// Reuse the existing deleteChannel function +export const deleteSubscriptionChannel = deleteChannel;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/apps/calendars/src/features/calendar/api.ts` around lines 206 - 212, The deleteSubscriptionChannel function duplicates deleteChannel; remove the duplication by having callers use the existing deleteChannel or consolidate both into a single exported function (e.g., keep/deleteChannel and remove deleteSubscriptionChannel, or export a generic deleteChannel that accepts channelId and is used for subscriptions too). Update any imports/call sites that reference deleteSubscriptionChannel to call deleteChannel and delete the duplicate function definition (check functions named deleteSubscriptionChannel and deleteChannel to locate and update usages).src/backend/core/api/viewsets_channels.py (2)
379-400: MultipleCalDAVClient()instantiations could be consolidated.
partial_updatecreates newCalDAVClient()._httpinstances at lines 380 and 452. IfCalDAVClientinitialization has overhead, consider caching the instance.♻️ Optional: Cache CalDAVClient instance
+ caldav_http = CalDAVClient()._http # noqa: SLF001 + if new_source_url and new_source_url != channel.settings.get("source_url"): # ... # Update WebDAV property try: - http = CalDAVClient()._http # noqa: SLF001 proppatch_body = (...) - http.request( + caldav_http.request( "PROPPATCH", # ... ) # ... if (new_color or new_name) and channel.caldav_path: # ... try: - http = CalDAVClient()._http # noqa: SLF001 proppatch = (...) - http.request( + caldav_http.request( "PROPPATCH", # ... )Also applies to: 451-467
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/api/viewsets_channels.py` around lines 379 - 400, partial_update currently constructs CalDAVClient() inline twice (accessing ._http) which may duplicate expensive initialization; create a single CalDAVClient() instance (e.g., caldav_client = CalDAVClient()) at the start of the block where PROPPATCH/other CalDAV operations occur and reuse caldav_client._http for both request calls instead of calling CalDAVClient() twice (refer to the CalDAVClient() usages and the http.request calls) to avoid repeated setup and improve performance.
526-535: Task enqueue failure is logged but user receives success response.If
sync_one_subscription.send()fails, the exception is logged but the response still indicates success. Consider whether the response should reflect this partial failure, or document that sync failures are handled asynchronously.For consistency, the response could include a warning field when task enqueue fails:
response_data = serializer.data try: sync_one_subscription.send(str(channel.pk)) except Exception: logger.exception(...) response_data["_warning"] = "Sync task could not be enqueued" return Response(response_data)Alternatively, this behavior may be acceptable if the periodic scheduler will pick up the sync anyway.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/api/viewsets_channels.py` around lines 526 - 535, The current reactivation flow enqueues a background task with sync_one_subscription.send(str(channel.pk)) but logs exceptions while still returning a success response; update the code that builds the response (use serializer.data as response_data) to catch enqueue failures and attach a warning key to the response (e.g., response_data["_warning"] = "Sync task could not be enqueued") when the except block runs, and ensure logger.exception(...) remains for diagnostics so callers receive the success payload plus an explicit warning if send(...) failed; reference sync_one_subscription, send, logger.exception, channel.pk, serializer.data and the Response return to locate and modify the logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/backend/core/api/serializers.py`:
- Around line 262-272: The validate_source_url method is duplicated in
ChannelSubscriptionCreateSerializer and ChannelSubscriptionUpdateSerializer;
extract it into a new SourceUrlValidationMixin that defines validate_source_url
(keeping the internal import of URLValidationError and validate_subscription_url
and identical try/except that raises serializers.ValidationError), then have
both ChannelSubscriptionCreateSerializer and ChannelSubscriptionUpdateSerializer
inherit from SourceUrlValidationMixin and remove their local validate_source_url
implementations so the shared mixin provides the behavior.
In `@src/backend/core/api/viewsets_channels.py`:
- Around line 379-400: partial_update currently constructs CalDAVClient() inline
twice (accessing ._http) which may duplicate expensive initialization; create a
single CalDAVClient() instance (e.g., caldav_client = CalDAVClient()) at the
start of the block where PROPPATCH/other CalDAV operations occur and reuse
caldav_client._http for both request calls instead of calling CalDAVClient()
twice (refer to the CalDAVClient() usages and the http.request calls) to avoid
repeated setup and improve performance.
- Around line 526-535: The current reactivation flow enqueues a background task
with sync_one_subscription.send(str(channel.pk)) but logs exceptions while still
returning a success response; update the code that builds the response (use
serializer.data as response_data) to catch enqueue failures and attach a warning
key to the response (e.g., response_data["_warning"] = "Sync task could not be
enqueued") when the except block runs, and ensure logger.exception(...) remains
for diagnostics so callers receive the success payload plus an explicit warning
if send(...) failed; reference sync_one_subscription, send, logger.exception,
channel.pk, serializer.data and the Response return to locate and modify the
logic.
In `@src/backend/core/tasks.py`:
- Around line 66-73: The loop currently imports uuid and reconverts channel.pk
unnecessarily; move the import uuid out of the loop (top-level module import)
and compute the stagger using the UUID object's integer directly instead of
uuid.UUID(str(channel.pk)); e.g., calculate delay_ms = (channel.pk.int %
sync_interval) * 1000 before calling
sync_one_subscription.send_with_options(args=(str(channel.pk),), delay=delay_ms)
so the import is not repeated and the redundant conversion is removed.
In `@src/frontend/apps/calendars/src/features/calendar/api.ts`:
- Around line 206-212: The deleteSubscriptionChannel function duplicates
deleteChannel; remove the duplication by having callers use the existing
deleteChannel or consolidate both into a single exported function (e.g.,
keep/deleteChannel and remove deleteSubscriptionChannel, or export a generic
deleteChannel that accepts channelId and is used for subscriptions too). Update
any imports/call sites that reference deleteSubscriptionChannel to call
deleteChannel and delete the duplicate function definition (check functions
named deleteSubscriptionChannel and deleteChannel to locate and update usages).
In
`@src/frontend/apps/calendars/src/features/calendar/components/calendar-list/AddSubscriptionModal.tsx`:
- Around line 98-103: The AddSubscriptionModal currently shows a generic error
message; update it to mirror EditSubscriptionModal by passing the backend
validation details: use errorToString(createMutation.error) instead of the
generic t("calendar.subscription.add.error") when createMutation.isError is
true, and ensure errorToString is imported into AddSubscriptionModal; keep the
state logic using createMutation.isError and only swap the text value to the
errorToString output so users see specific backend rejection reasons (e.g.,
SSRF/non-HTTPS).
In
`@src/frontend/apps/calendars/src/features/calendar/components/scheduler/Scheduler.tsx`:
- Around line 406-434: The inline IIFE that computes writableCalendars and
sanitizedCalendarUrl inside the JSX makes dependencies implicit and reduces
readability; extract the computation into a memoized value using useMemo so it
only recalculates when inputs change. Specifically, create a useMemo that
derives writableCalendars = davCalendars.filter(cal =>
!subscriptionCalendarUrls.has(cal.url)) and sanitizedCalendarUrl =
subscriptionCalendarUrls.has(modalState.calendarUrl) ?
(writableCalendars[0]?.url ?? "") : modalState.calendarUrl, and then pass those
memoized values into the EventModal props (EventModal, davCalendars,
subscriptionCalendarUrls, modalState, writableCalendars, sanitizedCalendarUrl)
so dependencies are explicit and the JSX no longer contains the IIFE.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e8dd8f2e-83ff-4c46-88b1-6c8bf218eff9
📒 Files selected for processing (39)
Makefilecompose.yamldocs/external-subscriptions-test-plan.mdsrc/backend/calendars/settings.pysrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets_caldav.pysrc/backend/core/api/viewsets_channels.pysrc/backend/core/factories.pysrc/backend/core/management/__init__.pysrc/backend/core/management/commands/run_subscription_scheduler.pysrc/backend/core/management/commands/sync_mailbox_acls.pysrc/backend/core/services/subscription_sync_service.pysrc/backend/core/services/url_validation.pysrc/backend/core/tasks.pysrc/backend/core/tests/test_calendar_subscription_api.pysrc/backend/core/tests/test_subscription_proxy.pysrc/backend/core/tests/test_subscription_sync.pysrc/backend/core/tests/test_subscription_tasks.pysrc/backend/core/tests/test_url_validation.pysrc/frontend/apps/calendars/src/features/calendar/api.tssrc/frontend/apps/calendars/src/features/calendar/components/calendar-list/AddSubscriptionModal.tsxsrc/frontend/apps/calendars/src/features/calendar/components/calendar-list/CalendarList.tsxsrc/frontend/apps/calendars/src/features/calendar/components/calendar-list/CalendarModal.tsxsrc/frontend/apps/calendars/src/features/calendar/components/calendar-list/ColorPicker.tsxsrc/frontend/apps/calendars/src/features/calendar/components/calendar-list/EditSubscriptionModal.tsxsrc/frontend/apps/calendars/src/features/calendar/components/calendar-list/SubscriptionCalendarSection.tsxsrc/frontend/apps/calendars/src/features/calendar/components/calendar-list/SubscriptionStatusBadge.tsxsrc/frontend/apps/calendars/src/features/calendar/components/scheduler/EventModal.scsssrc/frontend/apps/calendars/src/features/calendar/components/scheduler/ReadOnlyEventModal.tsxsrc/frontend/apps/calendars/src/features/calendar/components/scheduler/Scheduler.tsxsrc/frontend/apps/calendars/src/features/calendar/components/scheduler/hooks/useSchedulerHandlers.tssrc/frontend/apps/calendars/src/features/calendar/components/scheduler/hooks/useSchedulerInit.tssrc/frontend/apps/calendars/src/features/calendar/components/scheduler/types.tssrc/frontend/apps/calendars/src/features/calendar/config.tssrc/frontend/apps/calendars/src/features/calendar/contexts/CalendarContext.tsxsrc/frontend/apps/calendars/src/features/calendar/hooks/useCalendars.tssrc/frontend/apps/calendars/src/features/calendar/services/dav/EventCalendarAdapter.tssrc/frontend/apps/calendars/src/features/calendar/services/dav/types/event-calendar.tssrc/frontend/apps/calendars/src/features/i18n/translations.json
✅ Files skipped from review due to trivial changes (7)
- src/backend/core/management/commands/sync_mailbox_acls.py
- src/frontend/apps/calendars/src/features/calendar/services/dav/EventCalendarAdapter.ts
- src/frontend/apps/calendars/src/features/calendar/components/calendar-list/ColorPicker.tsx
- src/frontend/apps/calendars/src/features/calendar/components/scheduler/EventModal.scss
- src/backend/core/tests/test_subscription_tasks.py
- src/backend/core/tests/test_subscription_proxy.py
- src/frontend/apps/calendars/src/features/calendar/config.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- src/frontend/apps/calendars/src/features/calendar/components/scheduler/hooks/useSchedulerInit.ts
- Makefile
- src/backend/core/api/viewsets_caldav.py
- src/frontend/apps/calendars/src/features/calendar/components/calendar-list/SubscriptionStatusBadge.tsx
- src/backend/calendars/settings.py
- src/backend/core/factories.py
- src/frontend/apps/calendars/src/features/calendar/components/scheduler/hooks/useSchedulerHandlers.ts
- src/backend/core/management/commands/run_subscription_scheduler.py
- compose.yaml
- src/frontend/apps/calendars/src/features/calendar/components/scheduler/types.ts
- src/backend/core/tests/test_calendar_subscription_api.py
- src/frontend/apps/calendars/src/features/i18n/translations.json
- src/frontend/apps/calendars/src/features/calendar/components/calendar-list/SubscriptionCalendarSection.tsx
- src/backend/core/services/subscription_sync_service.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Allow users to subscribe to external ICS calendar feeds
(e.g. public holidays, shared team calendars) and have
events automatically synced into their La Suite calendar.
Subscribed calendars appear alongside owned calendars in
the sidebar but are read-only: events cannot be edited,
moved, or deleted by the user.
What was done
Backend
hostname resolution checks, private IP blocking,
safe redirect handling, conditional requests
(ETag / If-Modified-Since), and response size limits.
the external ICS with existing SabreDAV events, then
creates, updates, or deletes as needed. Includes
Redis-based locking, auto-stop after 3 consecutive
errors, infinite RRULE capping, and volatile property
filtering to avoid unnecessary updates.
periodic sync with staggered dispatch per channel.
color), delete, reactivate. Per-user limit (20),
immediate sync on creation, event purge on URL change,
SabreDAV calendar cleanup on deletion.
(PUT, DELETE, MOVE, PROPPATCH) return 403 for
subscription calendars, even when the subscription
is stopped.
Infrastructure
subscription-schedulerDocker service runningthe management command for periodic syncs.
start,start-back).reset-db-fulltarget for full DB recreation.Frontend
channel CRUD (create, update, delete, reactivate,
list) with periodic polling.
URL validation feedback, status badge (syncing /
error / stopped), dedicated sidebar section,
reusable ColorPicker component.
non-editable in FullCalendar, a read-only modal
opens on click, drag-and-drop and resize are blocked,
subscription calendars are filtered from the event
creation calendar picker.
Tests
content validation, conditional requests.
RRULE capping, error handling, locking.
Test plan
Summary by CodeRabbit
New Features
Security / Limits
Documentation
Tests
Chores