You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This PR updates the external integrations specification to document the "calendar" type (B.19), enabling calendar providers (Google Calendar, Outlook, Nextcloud, CalDAV servers, and public feeds) to become ordinary external integrations that feed the same calendar view and scene triggers as the internal CalDAV service.
The specification covers:
Core prerequisites: batch upsert primitives for calendars/events, unique selector generation, ownership checks, explicit cleanup at uninstall, real-time WebSocket refresh, and drive-by fixes.
Per-user accounts: manifest account_schema (same format as communication integrations), per-user activation as consent gesture, non-admin visibility, and automation boundary via the shared flag.
Host API: five new endpoints for account management, calendar listing, and event upsert/delete with rate limiting and tenant isolation.
Normalization and bounds: field whitelisting, length/format validation, color validation, and a 10,000-event-per-calendar cap.
Sync semantics: field ownership split (integration-owned: name/description/color; user-owned: sync/shared/selector), window-based pruning for deletions, and idempotent upsert.
Milestone 2 sketch: per-user OAuth2 support (deferred to a future milestone).
Frontend and SDK: calendar category integration, real-time refresh listener, and new SDK methods (getCalendarAccounts, getCalendars, publishCalendars, deleteCalendar, publishCalendarEvents, onCalendarAccountUpdated).
The design follows the "integration syncs, core stores" principle: integrations own the provider protocol and sync cadence; the core owns storage, normalization, and automations. This is a push model (like CalDAV) rather than pull (like weather), keeping third-party containers off the critical path of scene scheduling.
Also updates AGENTS.md to note that B.19 is now specified but not yet implemented.
Checklist
No code changes (specification document only)
No breaking changes
Follows the "spec first, code second" rule for external integrations
Design for community request (forum topic 10432): open calendar
providers (Google Calendar, Outlook, Nextcloud/Framagenda, public ICS
feeds) as external integrations, feeding the same calendar view and
scene triggers as the internal CalDAV service.
- New section B.19: push model into t_calendar/t_calendar_event,
per-user accounts via an account_schema (the B.15 machinery),
host API contract (account listing, calendar/event upsert by
external_id, window prune), normalization bounds, sync semantics
(field ownership, sync/shared toggles, uninstall cleanup), and a
per-user OAuth2 milestone-2 sketch for Google/Microsoft.
- Core prerequisites called out: calendar upsert primitives,
buildUniqueSelector for calendar/event selectors, ownership checks
on the user calendar routes, explicit uninstall cleanup, calendar
WebSocket refresh, CALENDAR_TYPES constant, double-associate fix,
frontend color-join guard.
- Contracts updated: manifest type enum (C.1), WS protocol row (C.4),
non-admin reduced view (C.5), SDK methods and handler (C.8), test
plan (B.13), phase table and spec header.
- AGENTS.md: refresh the external-integrations spec pointer (B.15-B.18
shipped, B.19 specified but not implemented).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Jhfyd3Vz9icRe5QjuUUXVi
It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.
Use the following commands to manage reviews:
@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.
Use the checkboxes below for quick actions:
▶️ Resume reviews
🔍 Trigger review
📝 Walkthrough
Walkthrough
The pull request implements shipped calendar integrations. It adds per-user accounts, calendar and event synchronization, ownership checks, APIs, WebSocket updates, frontend management, manifest support, uninstall cleanup, and tests.
Changes
Calendar integration implementation
Layer / File(s)
Summary
Calendar contracts and platform wiring AGENTS.md, docs/specs/external-integrations.md, server/lib/external-integration/*, server/utils/constants.js, server/services/caldav/...
Calendar manifests, account schemas, constants, WebSocket events, SDK contracts, and integration wiring now support calendar providers.
Calendar ownership and synchronization storage server/lib/calendar/*, server/models/calendar.js
Calendar and event operations enforce user ownership. Transactional calendar and event upserts support conflict checks, pruning, movement, defaults, and capacity limits.
Calendar account and publication services server/lib/external-integration/*
Services manage per-user accounts, calendar publication, event publication, deletion, sync and sharing settings, notifications, rate limits, and uninstall cleanup.
Calendar API exposure server/api/controllers/*, server/api/routes.js
User-scoped and integration-authenticated routes expose calendar account, calendar, and event operations. Non-admin visibility includes calendar integrations.
Calendar frontend management and updates front/src/routes/calendar/*, front/src/routes/integration/*, front/src/config/i18n/*, front/src/actions/calendar.js
The frontend manages calendar accounts and calendar settings, routes calendar integrations to configuration, refreshes events after WebSocket updates, and adds translations.
Calendar validation and integration tests server/test/controllers/*, server/test/lib/*
sequenceDiagram
participant User
participant CalendarConfig
participant ExternalIntegration
participant Calendar
participant WebSocket
User->>CalendarConfig: Save account or change calendar settings
CalendarConfig->>ExternalIntegration: Submit account or calendar update
ExternalIntegration->>Calendar: Persist account, calendars, or events
Calendar-->>ExternalIntegration: Return updated calendar state
ExternalIntegration->>WebSocket: Send calendar.updated or account-updated
WebSocket-->>User: Refresh calendar data
Loading
Poem
A rabbit hops through calendars bright,
Accounts and events now sync just right.
Ownership guards each little track,
WebSockets bring fresh updates back.
Hop, hop—the integration is live!
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
✅ Passed
The title identifies the calendar external-integration work and B.19 milestone, but it describes the change as documentation despite substantial implementation changes.
Docstring Coverage
✅ Passed
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check
✅ Passed
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
✅ Passed
Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches📝 Generate docstrings
Create stacked PR
Commit on current branch
🧪 Generate unit tests (beta)
Create PR with unit tests
Commit unit tests in branch claude/calendar-integrations-spec-y64vwe
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.
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Spec-only PR for the calendar external-integration type (B.19). Overall direction looks sound: push model (integration syncs, core stores), per-user consent, shared as the scene-automation boundary, and reuse of B.15/B.18 patterns. Code claims I spot-checked (shared-only scene triggers, missing ownership on calendar update/destroy, double calendar.associate, color join TypeError, CalDAV as sole producer) match the current tree.
Not ready to approve yet — the living-spec contracts are incomplete or inconsistent in a few places that would mislead an implementer:
C.5 table drift — the prose now includes calendar for non-admins, but the route table still says communication-only (list + detail), and DELETE .../:selector still omits calendars from the uninstall contract that B.19 says must be explicit.
Multi-user external_id collision — t_calendar.external_id and t_calendar_event.external_id are globally UNIQUE; the example and conventions do not embed the Gladys user, so two household members enabling the same provider (or the same public ICS feed) will collide / steal rows.
Window prune vs multi-day events — start-only window membership + "start must be in window" rejects updates for events that start before the rolling window.
Enable-without-account_schema and B.2 uninstall wording need tightening so the implementation checklist is unambiguous.
Also worth aligning C.1 (account_schema next to contact_schema for {{port:<name>}} refusal and source validation) even though those lines are outside this diff's hunks.
No server/utils/constants.js device categories/types in this PR. Docs-only → not risk:high. Flagging needs:human-review for philosophy/trust-boundary alignment (unaudited container → calendars → shared → scene triggers), with atrovato requested.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AGENTS.md`:
- Line 30: Update the phase-2 workstream count in the external integrations
guidance from “three” to “four,” leaving the listed workstreams and remaining
wording unchanged.
In `@docs/specs/external-integrations.md`:
- Line 273: Update the verification section around the documented test command
to use the CI-required coverage command, `npm run coverage`, instead of `cd
server && npm test`. Keep the surrounding verification requirements unchanged.
- Line 423: Add `calendar.updated` to the C.4 frontend WebSocket message list,
including its payload and recipient rule: send it to the owning user, or to
everyone when the calendar is shared. Keep the existing real-time refresh
behavior and contract details consistent with the B.19 entry.
- Line 470: The normalization contract around external_id must support moving an
event between calendars without losing its existing row. Define a stable
provider event identity independent of calendar_id, or specify an explicit move
key that lets upsert locate and update the prior row while preserving the
required per-integration uniqueness and calendar-embedded external_id behavior.
- Line 812: Update both C.5 non-admin route rows to allow calendar integrations
in addition to communication integrations, while preserving the existing
reduced-view response and 404 behavior for unsupported selectors such as type
"device". Ensure the route table aligns with the B.19 per-user calendar-account
flow and the surrounding access rules.
- Line 433: Update the Host API external-ID convention to include a stable
per-user account namespace in both t_calendar.external_id and
t_calendar_event.external_id, or explicitly define account-scoped identity and
upsert keys that prevent cross-user collisions. Ensure the documented publish
and update behavior uses this account scope while preserving the existing
ext:<selector>: convention.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
Push a commit to this branch (recommended)
Create a new PR with the fixes
ℹ️ Review info⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: db5c8d25-0bbb-470a-ae9e-742e2fe67b17
📥 Commits
Reviewing files that changed from the base of the PR and between a1ca77a and efe7559.
The C.5 prose extended the non-admin reduced view to calendar
integrations, but the route table still said communication-only and
the uninstall row omitted calendars. Bring the table in line: list and
detail rows include calendar-type integrations for non-admins, and
DELETE .../:selector explicitly removes the service's calendars.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Jhfyd3Vz9icRe5QjuUUXVi
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Re-review at 9c39878 (after the C.5 table alignment commit).
Progress since last review: the C.5 route table and DELETE .../:selector uninstall wording now match the B.19 prose (non-admin list/detail include calendar; uninstall lists calendars). That prior blocker is cleared.
Still not ready to approve — several living-spec contracts remain incomplete or internally inconsistent, and would mislead an implementer:
Multi-user external_id uniqueness — global UNIQUE columns + example ids that omit the Gladys user → household collisions / possible row steal under the "move" rule.
Event move vs calendar-embedded external_id — point 4 contradicts itself (embed calendar in the id, then upsert-by-external_id to move).
Window prune is start-only — multi-day / straddling events under rolling windows are underspecified.
Enable without account_schema — public-feed Enable gesture still has no empty-schema request contract.
B.2 uninstall — B.19 claims the B.2 contract was updated; section B.2 still only lists devices/variables/containers (C.5 DELETE was fixed; B.2 was not).
calendar.updated missing from the C.4 frontend WS list — defined in B.19 but omitted from the Core → frontend contract paragraph.
Soft: phrase hasDeviceScreens as a required front change (today only communication/weather); extend the C.4 reconnect bullet (GET /device + GET /config) to the calendar surface.
Code claims re-checked and still match the tree (shared: true scene filter, global UNIQUE external_id, double calendar.associate, color join TypeError, update/destroy by selector alone, hasDeviceScreens excludes only communication/weather).
No server/utils/constants.js device categories/types. Docs-only → not risk:high. Keeping needs:human-review + atrovato for the trust-boundary / philosophy call (unaudited container → calendars → shared → scene triggers).
- User-scoped external ids: calendar and event external_ids must carry
the ext:<selector>:<user_selector>: prefix, enforced server-side, so
two household members syncing the same provider-side id can never
collide on the globally UNIQUE columns (example updated).
- Resolve the event-move contradiction: the event external_id is a
stable per-user identity independent of the calendar; moves update
calendar_id, and calendar-embedded ids explicitly renounce moves.
- Window prune membership is by overlap (start < to and (end ?? start)
>= from), so multi-day events straddling a rolling window stay
updatable and prunable; pushed events must overlap the window.
- Explicit empty-schema contract for the per-user Enable gesture
(config {} or omitted, any key -> 422) and the reserved
EXTERNAL_INTEGRATION_CALENDAR_ACCOUNT enablement variable.
- B.2 uninstall contract now lists calendars; Verification uses
npm run coverage (the CI command).
- calendar.updated added to the C.4 core->frontend WS contract, and
the reconnection resync contract extended to the calendar surface.
- C.1 aligned: account_schema next to contact_schema for the
{{port:<name>}} refusal and dynamic source validation; required
front changes (EXTERNAL_CATEGORIES, hasDeviceScreens) spelled out;
B.13 tests updated accordingly.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Jhfyd3Vz9icRe5QjuUUXVi
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Re-review at 76a6b98 (address-review-feedback commit).
Prior blockers are cleared. User-scoped ext:<selector>:<user_selector>: ids, calendar-independent event identity (with explicit renounce-moves path), overlap-based window prune, empty-account_schema Enable contract + EXTERNAL_INTEGRATION_CALENDAR_ACCOUNT, B.2 uninstall calendars wording, calendar.updated in the C.4 frontend WS list, C.4 reconnect resync, C.1 account_schema alignment, and required front changes (EXTERNAL_CATEGORIES / hasDeviceScreens) are all in the living spec now. Code claims re-checked and still match the tree.
Approving the specification. Soft residuals only (inline) — not merge blockers for a docs-only PR:
Frontend exclusion lists also live outside ExternalIntegrationPage (install / discover / device pages).
Exclusive-end day-boundary overlap formula can over-include full-day events at window edges.
No server/utils/constants.js device categories/types. Docs-only → not risk:high. Keeping needs:human-review + atrovato for the trust-boundary / philosophy call (unaudited container → calendars → shared → scene triggers). needs:cursor-review removed.
- Renumber the spec section B.19 -> B.20: open PR #2807 already reserves B.19
for the calendar provider type.
- Never sweep a freshly pulled image. `install` and `update` pull their images
*before* writing the `t_service` row that declares them; a sweep landing in
that window saw a brand new image as an orphan and deleted it under the
operation that had just fetched it — and Docker's 409 is no help there, the
container does not exist yet. `system.pull` now stamps every pull (before the
download, so a slow pull on a Raspberry Pi is covered end to end) and the
sweep skips anything pulled less than an hour ago.
- Pin `force: false` in `system.removeImage`: the "never force" rule is the
safety invariant of every caller, so it belongs in the helper rather than in
call-site discipline.
- Align the update comment and the spec with what `start()` actually
guarantees: it resolves on LOADING, not RUNNING, so the cleanup buys the
container being created and started, never a healthy integration. The
ordering still matters — a `start()` that throws skips the cleanup entirely.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012zFg1ZJerC7KcgxrkApuri
Keep both the B.19 calendar-type section (this branch) and the B.20
Docker image cleanup section added on master at the same insertion
point.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Jhfyd3Vz9icRe5QjuUUXVi
GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/specs/external-integrations.md`:
- Line 275: Expand the B.13 `calendar.updated` invalidation coverage to define
notifications for state transitions: notify the union of old and new viewers
when `shared` changes, include both old and new calendar selectors when events
move, and notify previous viewers for account disable and uninstall deletions.
Add these affected-calendar and affected-viewer cases to the B.13 coverage list
and reflect the same complete invalidation set in the related calendar
specification entries.
- Around line 430-431: Update the canonical manifest table in the
manifest-contract section to define optional account_schema for calendar
integrations, including the same flat-schema validation and rendering
restrictions as config_schema/contact_schema. Document milestone-1 rejection of
oauth2 fields and {{port:<name>}}, while preserving the existing config_schema
behavior and referencing the established B.19 calendar semantics.
- Line 604: Update dynamic "devices" option resolution and value validation for
per-user schemas such as account_schema and contact_schema to filter by both the
integration's t_service and the requesting user. Preserve integration-wide
device scoping for non-user-specific schemas, while ensuring per-user forms
never expose or accept another user's device identifier.
- Line 472: The normalization specification’s recurrence identity must
distinguish each materialized occurrence instead of using the provider UID
alone. Update the recurrence guidance around external_id to require a
deterministic occurrence-specific suffix or key, such as the occurrence date,
while retaining the user-scoped prefix and independence from calendar_id so
moving an occurrence between calendars still updates the existing row.
- Line 435: The Host API specification requires a calendar-specific way to
construct user-scoped external IDs, but the existing externalId contract only
covers selector-scoped IDs. Update the SDK contract and the related C.7 examples
to define a calendar-aware helper that produces
ext:<selector>:<user_selector>:... IDs, or document and validate the composed
suffix explicitly in calendar methods; ensure the same requirement is covered at
the additionally referenced section.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
Push a commit to this branch (recommended)
Create a new PR with the fixes
ℹ️ Review info⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4750a72e-d2b4-4588-a4b3-6fca494dcfc4
📥 Commits
Reviewing files that changed from the base of the PR and between bd71cb9 and eec9cbb.
📒 Files selected for processing (2)
AGENTS.md
docs/specs/external-integrations.md
🚧 Files skipped from review as they are similar to previous changes (1)
- CALENDAR_TYPES constant (CALDAV, WEBCAL, EXTERNAL) replacing the free
strings in the CalDAV service; WEBSOCKET_MESSAGE_TYPES.CALENDAR.UPDATED
and EXTERNAL_INTEGRATION.CALENDAR_ACCOUNT_UPDATED message types.
- calendar.upsertCalendars / calendar.upsertEvents: transactional batch
upserts keyed by external_id, buildUniqueSelector for collision-free
selectors, integration-owned vs user-owned field split, cross-owner
ConflictError, overlap-based window prune sparing manual events, and
the 10000-events-per-calendar cap.
- Ownership checks on the user calendar routes: update/destroy and the
event write routes now answer 404 on another user's calendar.
- Explicit calendar cleanup in externalIntegration.uninstall (no silent
FK cascade), fix of the double calendar.associate definition.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Jhfyd3Vz9icRe5QjuUUXVi
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Re-review at eec9cbb (master merge into the branch; prior approval was dismissed).
B.19 content is unchanged from the previously approved 76a6b98. The merge only brought already-shipped living-spec updates (notably B.20 image cleanup) into the same file. Prior blockers remain cleared: user-scoped ext:<selector>:<user_selector>: ids, calendar-independent event identity, overlap window prune, empty-account_schema Enable + EXTERNAL_INTEGRATION_CALENDAR_ACCOUNT, B.2/C.5 uninstall calendars, calendar.updated in C.4, reconnect resync, C.1 account_schema alignment, and required front changes.
Code claims re-checked against the tree: scene triggers/actions require shared: true; EXTERNAL_CATEGORIES / hasDeviceScreens still omit calendar; double calendar.associate; global UNIQUE on calendar/event external_id.
Approving again. Soft residuals only (not merge blockers for a docs-only PR):
Frontend exclusion surface also includes install/discover/device pages.
Exclusive-end day-boundary overlap can over-include full-day events at window edges.
calendar.updated recipient rule should cover state transitions (shared true→false, event moves across calendars, disable/uninstall) so previous viewers invalidate too — fine to nail down at implementation time if preferred.
CodeRabbit’s other new notes are non-blocking here: account_schema follows the same C.1 omission pattern as contact_schema (documented in B.19/B.15); user-scoped ids compose via externalId("${user}:${suffix}"); recurrence already cites the CalDAV UID + occurrence date precedent; per-user source: "devices" scoping is not a calendar-account concern.
No server/utils/constants.js device categories/types. Docs-only → not risk:high. Keeping needs:human-review + atrovato for the trust-boundary / philosophy call (unaudited container → calendars → shared → scene triggers). Removing needs:cursor-review.
…lock, live calendar view (B.19)
- Catalog: EXTERNAL_CATEGORIES and the card type mapping gain 'calendar',
non-admins see installed calendar integrations (reduced view), their
cards land on the Configuration screen like communication/weather.
- Generic page: hasDeviceScreens excludes the calendar type; new
per-user 'My calendars' card (account_schema fields via ConfigField,
Enable/Save, per-calendar sync/shared toggles with optimistic
rollback, inline-confirmed disable destroying the user's calendars).
- Calendar view: listens to the calendar.updated WebSocket message and
refetches the displayed range; the color join no longer crashes on a
shared calendar absent from the user's list.
- i18n: integration.externalIntegration.myCalendars block in en/fr/de.
- Spec kept truthful in the same diff: B.19 milestone 1 marked shipped
(per-user OAuth2 stays milestone 2, design-only), AGENTS.md pointer
refreshed.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Jhfyd3Vz9icRe5QjuUUXVi
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Re-review at da71036 — this is no longer a docs-only PR. Since the last approval (eec9cbb), milestone-1 B.19 has been implemented (core upserts, host/management APIs, front My calendars + live view). Prior living-spec contracts look largely followed (user-scoped ext:<selector>:<user_selector>: ids, ownership on user calendar write routes, overlap prune, empty-schema Enable, rate limits, tenant isolation, shared as scene boundary).
Not ready to approve — concrete issues plus red CI:
10k event cap bypassed on moves in calendar.upsertEvents (create path checks the cap; move path increments count without checking).
shared true→false under-notifies — Sequelize mutates calendar.shared before notify, so household clients keep seeing now-private events (updateUserCalendar).
Event moves / uninstall do not complete the calendar.updated invalidation set (destination-only notify; uninstall destroys with no push). Disable is correct.
Front exclusion incomplete — ExternalIntegrationPage.hasDeviceScreens includes calendar, but install / discover / device pages still special-case only ['communication','weather']. Empty-config_schema calendar installs land on Devices; deep links keep rendering device UIs. (Those lines are outside prettier hunks, so called out here rather than inline.)
CI red: Server lint prettier-check fails on several new calendar files; Front test prettier-check fails across a large trailing-comma sweep (~630 front files) that also left calendar-touched files unformatted. Please run npm run prettier in server/ and front/ (or drop the unrelated mass reformat and format only the intentional touch set).
Soft / non-blocking:
Exclusive-end overlap still uses (end ?? start) ≥ from (matches current B.19 text; full-day events ending exactly at from can over-include).
updateEvent now returns nested calendar: { user_id } via the ownership include — possible API shape regression for clients of PATCH /calendar/event/:selector.
PR title/description still say “docs / specification only”; worth updating now that milestone 1 has shipped in-tree.
No new DEVICE_FEATURE_CATEGORIES / TYPES. Not tagging risk:high (additive external-integration surface, no migration). Keeping needs:human-review + atrovato for the trust-boundary / philosophy call (unaudited container → calendars → shared → scene triggers). Removing needs:cursor-review.
Earlier commits were formatted with a prettier v3 fetched by npx while
node_modules was still installing: the pinned prettier 1.19 (the CI
version) flagged both workspaces. Re-run npm run prettier everywhere
with the project version — formatting only, no functional change.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Jhfyd3Vz9icRe5QjuUUXVi
…den the window semantics
Review feedback on the B.19 implementation:
- The 10000-events-per-calendar cap now also holds on the move path: a
destination calendar at the cap refuses an incoming move with a 400.
- Unsharing a calendar (shared true->false) still pushes calendar.updated
to everyone one last time, so previous household viewers drop it
(snapshot before the Sequelize update mutates the instance).
- An event move pushes the union of the source and destination calendar
selectors (upsertEvents reports movedFromCalendarIds), everyone when
either is shared.
- Uninstall notifies the previous viewers of the destroyed calendars,
grouped per owning user.
- Window overlap is exclusive on the end side (start < to, and end > from
when end is set, else start >= from): a full-day event ending exactly
at belongs to the previous window, both for the prune and for
the pushed-events check.
- Front: the calendar type joins the communication/weather exclusion
sets of install-page (post-install redirect) and the discover/device
page guards, not only the page chrome.
- Spec updated in the same diff (overlap formula, transition-push union
rule, B.13 coverage, front call sites).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Jhfyd3Vz9icRe5QjuUUXVi
Restrict the update payload to editable event fields.
The route passes req.body unchanged, and calendar_id is a writable model attribute. The ownership check runs before the update, so a user can reassign an event to another calendar, including one owned by another user. Add a regression test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/lib/calendar/calendar.updateEvent.js` at line 33, Restrict the payload
passed to existingCalendarEvent.update in the event update flow to approved
editable event fields, excluding calendar_id and other ownership-sensitive
attributes. Preserve the existing ownership validation and add a regression test
confirming a user cannot reassign an event to another calendar.
Use WEBSOCKET_MESSAGE_TYPES.CALENDAR.UPDATED instead of the string literal.
Line 58 compares the message type against 'calendar.updated'. The sibling test server/test/lib/external-integration/externalIntegration.calendar.test.js uses the constant. If the constant value changes, the filter here silently returns an empty array and the test fails with a confusing count mismatch.
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/test/lib/external-integration/externalIntegration.uninstall.test.js`
around lines 55 - 62, Import WEBSOCKET_MESSAGE_TYPES from the established
constants module, then update the calendarPushes filter to compare against
WEBSOCKET_MESSAGE_TYPES.CALENDAR.UPDATED instead of the hardcoded
'calendar.updated' string.
Assert that a matching websocket.send call exists before you read the last one.
If the filters at Line 220 and Line 221 return an empty array, Line 222 evaluates sendCalls[-1].args and throws a TypeError. The failure then hides the real cause. The sibling tests at Line 197 and Line 232 guard with a length assertion first. Apply the same guard here.
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/test/lib/external-integration/externalIntegration.calendar.test.js`
around lines 218 - 223, In the test around the sendCalls filtering, assert that
sendCalls contains a matching call before accessing sendCalls[sendCalls.length -
1].args. Mirror the existing length guard used by the sibling tests, then retain
the lastPayload and calendar_selectors assertions.
Line 197 asserts greaterThan(1) and Line 232 asserts greaterThan(0). Both tests perform a known number of updates, so the expected number of websocket.send-all pushes is deterministic. An exact count detects a regression that emits duplicate pushes. A lower bound does not.
Also applies to: 228-232
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/test/lib/external-integration/externalIntegration.calendar.test.js`
around lines 193 - 197, Update the assertions for filtered CALENDAR.UPDATED
websocket.send-all calls in the affected tests to use the deterministic expected
counts instead of greaterThan lower bounds. Adjust both assertions near the
first update scenario and the later scenario so duplicate pushes cause failure,
while preserving the existing event filtering.
Strengthen the rollback assertion with a real partial batch.
The array at Line 57 holds one calendar. The conflict throws before any write, so Line 62 cannot detect a rollback failure. To exercise the transaction, send a new calendar first and the conflicting one second, then assert that the new calendar was not persisted.
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/test/lib/calendar/calendar.upsertCalendars.test.js` around lines 55 -
64, Update the test case around calendar.upsertCalendars to submit a new
calendar before the conflicting calendar owned by USER_A, ensuring the conflict
occurs after an attempted write. Assert that the existing calendar remains
unchanged and the new calendar is absent from the database, verifying the whole
batch was rolled back.
The calendar manifest fixture is defined three times.testUtils.test.js exports TEST_CALENDAR_MANIFEST, but both controller test files declare their own near-identical CALENDAR_MANIFEST instead of importing it. The three copies will drift as the manifest contract evolves.
server/test/lib/external-integration/testUtils.test.js#L108-L123: keep this fixture as the single source, and confirm it stays importable from the controller test directory.
server/test/controllers/externalIntegration/externalIntegration.calendar.test.js#L7-L19: replace the local CALENDAR_MANIFEST with an import of TEST_CALENDAR_MANIFEST, and apply the per-test overrides through a spread.
server/test/controllers/integrationHost/integrationHost.calendar.test.js#L9-L18: replace the local CALENDAR_MANIFEST with the same import; this copy also drops the app_password secret field, which is a real divergence from the shared fixture.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/test/lib/external-integration/testUtils.test.js` around lines 108 -
123, The calendar manifest fixture is duplicated across three test files; keep
TEST_CALENDAR_MANIFEST in
server/test/lib/external-integration/testUtils.test.js:108-123 as the single
source and ensure it remains importable from the controller tests. In
server/test/controllers/externalIntegration/externalIntegration.calendar.test.js:7-19,
replace CALENDAR_MANIFEST with the shared import and apply per-test changes via
object spreading; make the same replacement in
server/test/controllers/integrationHost/integrationHost.calendar.test.js:9-18,
preserving the shared app_password field.
Clear the timers of every seeded service, not only in the happy path.
Line 242 calls clearTimers(otherService.id) at the end of the test body. The deviceService seeded at Line 143 never gets a clearTimers call. If an assertion throws before Line 242, the timers of otherService also stay armed. Track the seeded service ids and clear them in the existing afterEach.
Then push each newly seeded service id into seededServiceIds and remove the inline call at Line 242.
Also applies to: 219-243
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/test/controllers/integrationHost/integrationHost.calendar.test.js`
around lines 142 - 166, Track every service created by these tests in the
existing cleanup mechanism, including deviceService and otherService, by adding
each seeded service ID to seededServiceIds. Update the existing afterEach to
clearTimers for all tracked IDs, and remove the inline
clearTimers(otherService.id) call so cleanup also runs when assertions fail.
calendarWriteRateLimits starts a 60-second window at the first request. If the 30 sequential requests take longer than 60 seconds, the counter resets and the 31st request returns 200. Stub Date.now or use a fake clock so all requests use one window.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/test/controllers/integrationHost/integrationHost.calendar.test.js`
around lines 245 - 259, Update the rate-limit test around “should rate limit the
write endpoints to 30 per minute” to control time with a stubbed Date.now or
fake clock, keeping all 31 requests within the same 60-second window. Restore
the clock after the test so other integration tests retain normal time behavior.
Add an afterEach to clear calendar rate limits and timers.
Clear calendarWriteRateLimits and call gladys.externalIntegration.clearTimers() after each test so failed tests cannot leak shared state or pending timers into later tests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@server/test/controllers/externalIntegration/externalIntegration.calendar.test.js`
around lines 49 - 53, Add an afterEach hook in the “External integration
management API: my calendars” suite to clear calendarWriteRateLimits and call
gladys.externalIntegration.clearTimers() after every test, preventing shared
rate-limit state and pending timers from leaking between tests.
Extract the "no device screens" manifest-type list into one shared constant. Four frontend sites now repeat ['communication', 'weather', 'calendar']. A future type must be added to every copy, and a missed copy produces an inconsistent route. Export one constant (for example from front/src/routes/integration/all/external-integration/utils) and import it at each site.
front/src/routes/integration/all/external-integration/ExternalIntegrationPage.jsx#L26-L32: replace the inline array in the hasDeviceScreens computation with the shared constant.
front/src/routes/integration/all/external-integration/device-page/index.js#L16-L18: replace the inline array in the redirect condition with the shared constant.
front/src/routes/integration/all/external-integration/discover-page/index.js#L44-L46: replace the inline array in the redirect condition with the shared constant.
front/src/routes/integration/index.js#L227-L230: replace the inline array in getInstalledUrl, and reuse it for the card type mapping at lines 244 and 266.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@front/src/routes/integration/all/external-integration/ExternalIntegrationPage.jsx`
around lines 26 - 32, Extract the shared no-device-screen manifest-type list
into an exported constant, then import and reuse it at
front/src/routes/integration/all/external-integration/ExternalIntegrationPage.jsx
lines 26-32, device-page/index.js lines 16-18, discover-page/index.js lines
44-46, and front/src/routes/integration/index.js lines 227-230. Replace each
inline array, and reuse the constant for the card type mappings at lines 244 and
266 in front/src/routes/integration/index.js.
Move the event cap to the shared constants module.
MAX_EVENTS_PER_CALENDAR is declared locally, while the related calendar limits live in server/lib/external-integration/constants.js (MAX_CALENDARS_PER_USER, MAX_CALENDAR_EVENTS_PER_REQUEST). A single location keeps the limits discoverable and testable. This is optional and can be deferred.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/lib/calendar/calendar.upsertEvents.js` at line 6, Move
MAX_EVENTS_PER_CALENDAR from calendar.upsertEvents.js into the shared
external-integration constants module alongside MAX_CALENDARS_PER_USER and
MAX_CALENDAR_EVENTS_PER_REQUEST, then update the event upsert code to import and
use the shared constant.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@front/src/routes/integration/all/external-integration/config-page/index.js`:
- Around line 326-347: Update toggleUserCalendar to use functional setState
updaters for both the optimistic change and failure rollback, rather than
reading this.state or capturing a full calendarAccount snapshot. Preserve
concurrent calendar toggles by deriving each update from the latest previous
state, and on failure restore only the affected calendar’s original value while
retaining other toggles.
In `@server/api/controllers/integrationHost.controller.js`:
- Around line 245-259: Validate req.query.user in getIntegrationCalendars before
calling gladys.externalIntegration.getIntegrationCalendars: when present, accept
only a string and reject arrays or objects by raising BadParameters so the
endpoint returns HTTP 400. Preserve the existing behavior for an omitted user,
and add coverage for repeated and bracketed user query parameters.
In `@server/lib/calendar/calendar.updateEvent.js`:
- Around line 20-29: Update the existingCalendarEvent validation in the
update-event flow to guard against a missing calendar association before
accessing calendar.user_id, and verify that the CalendarEvent model defines the
included calendar alias consistently with calendar.destroyEvent.js. Preserve the
existing userId authorization behavior for events with a valid association.
In `@server/lib/calendar/calendar.upsertCalendars.js`:
- Around line 21-33: Update upsertCalendars to enforce MAX_CALENDARS_PER_USER
for the user before creating additional calendars, including calendars across
all services rather than filtering by service_id. Reuse the existing
limit/configuration and ensure the check accounts for the calendars being
upserted without rejecting updates to existing records.
In `@server/lib/calendar/calendar.upsertEvents.js`:
- Around line 9-13: Update the JSDoc description near the module header to match
the implemented overlap rule documented by the inline comment: use the
conditional end-time comparison and the start-time comparison for events without
an end. Capitalize the sentence beginning “optionally prune” to resolve the lint
warning, then run npm run eslint-fix from the server directory.
- Around line 106-135: Validate the required prunePrefix argument at the start
of the window-handling branch in the calendar upsert flow, before querying or
filtering candidates. When window is provided without a valid prunePrefix,
reject the call explicitly instead of allowing startsWith to receive undefined;
preserve existing pruning behavior when both arguments are supplied.
- Around line 45-48: Validate event.external_id at the start of
calendar.upsertEvents, before the db.CalendarEvent.findOne lookup, and reject
missing or empty values. Preserve the existing lookup and update behavior for
valid external_id values.
In
`@server/lib/external-integration/externalIntegration.disableCalendarAccount.js`:
- Around line 26-32: Update the notification logic in the calendars deletion
flow to partition the deleted calendars by their prior shared status. Notify
with broadcast enabled using only shared selectors, and notify userId directly
using only private selectors; avoid including private selectors in any broadcast
payload. Follow the relevant living specification in docs/specs/ for the
notification contract.
In `@server/lib/external-integration/externalIntegration.getCalendarAccount.js`:
- Around line 43-48: Remove the caught error argument from the logger.warn calls
handling JSON.parse failures in getCalendarAccount and the variables.forEach
callback: update
server/lib/external-integration/externalIntegration.getCalendarAccount.js lines
43-48 and
server/lib/external-integration/externalIntegration.getCalendarAccounts.js lines
30-35. Keep the existing warning messages and fallback behavior unchanged.
In
`@server/lib/external-integration/externalIntegration.publishCalendarEvents.js`:
- Around line 63-66: Update the `creator` association include in the
`db.Calendar.findOne` call to be required, ensuring calendars without an owner
are excluded before `calendar.creator.selector` is accessed. Apply the same
required-include change to the corresponding calendar query in
`externalIntegration.getIntegrationCalendars.js`.
In `@server/lib/external-integration/manifest.schema.json`:
- Around line 148-166: Update the account_schema item constraints in the
manifest schema, alongside the existing configField and oauth2 restriction, to
reject {{port:<name>}} placeholders in section labels and descriptions. Match
the validation behavior implemented by externalIntegration.validateManifest.js
and use the corresponding docs/specs contract as the source of truth, ensuring
manifests accepted by the schema cannot later be rejected during installation.
---
Outside diff comments:
In `@server/lib/calendar/calendar.updateEvent.js`:
- Line 33: Restrict the payload passed to existingCalendarEvent.update in the
event update flow to approved editable event fields, excluding calendar_id and
other ownership-sensitive attributes. Preserve the existing ownership validation
and add a regression test confirming a user cannot reassign an event to another
calendar.
---
Nitpick comments:
In
`@front/src/routes/integration/all/external-integration/ExternalIntegrationPage.jsx`:
- Around line 26-32: Extract the shared no-device-screen manifest-type list into
an exported constant, then import and reuse it at
front/src/routes/integration/all/external-integration/ExternalIntegrationPage.jsx
lines 26-32, device-page/index.js lines 16-18, discover-page/index.js lines
44-46, and front/src/routes/integration/index.js lines 227-230. Replace each
inline array, and reuse the constant for the card type mappings at lines 244 and
266 in front/src/routes/integration/index.js.
In `@server/lib/calendar/calendar.upsertEvents.js`:
- Line 6: Move MAX_EVENTS_PER_CALENDAR from calendar.upsertEvents.js into the
shared external-integration constants module alongside MAX_CALENDARS_PER_USER
and MAX_CALENDAR_EVENTS_PER_REQUEST, then update the event upsert code to import
and use the shared constant.
In
`@server/test/controllers/externalIntegration/externalIntegration.calendar.test.js`:
- Around line 49-53: Add an afterEach hook in the “External integration
management API: my calendars” suite to clear calendarWriteRateLimits and call
gladys.externalIntegration.clearTimers() after every test, preventing shared
rate-limit state and pending timers from leaking between tests.
In `@server/test/controllers/integrationHost/integrationHost.calendar.test.js`:
- Around line 142-166: Track every service created by these tests in the
existing cleanup mechanism, including deviceService and otherService, by adding
each seeded service ID to seededServiceIds. Update the existing afterEach to
clearTimers for all tracked IDs, and remove the inline
clearTimers(otherService.id) call so cleanup also runs when assertions fail.
- Around line 245-259: Update the rate-limit test around “should rate limit the
write endpoints to 30 per minute” to control time with a stubbed Date.now or
fake clock, keeping all 31 requests within the same 60-second window. Restore
the clock after the test so other integration tests retain normal time behavior.
In `@server/test/lib/calendar/calendar.upsertCalendars.test.js`:
- Around line 55-64: Update the test case around calendar.upsertCalendars to
submit a new calendar before the conflicting calendar owned by USER_A, ensuring
the conflict occurs after an attempted write. Assert that the existing calendar
remains unchanged and the new calendar is absent from the database, verifying
the whole batch was rolled back.
In `@server/test/lib/external-integration/externalIntegration.calendar.test.js`:
- Around line 218-223: In the test around the sendCalls filtering, assert that
sendCalls contains a matching call before accessing sendCalls[sendCalls.length -
1].args. Mirror the existing length guard used by the sibling tests, then retain
the lastPayload and calendar_selectors assertions.
- Around line 193-197: Update the assertions for filtered CALENDAR.UPDATED
websocket.send-all calls in the affected tests to use the deterministic expected
counts instead of greaterThan lower bounds. Adjust both assertions near the
first update scenario and the later scenario so duplicate pushes cause failure,
while preserving the existing event filtering.
In `@server/test/lib/external-integration/externalIntegration.uninstall.test.js`:
- Around line 55-62: Import WEBSOCKET_MESSAGE_TYPES from the established
constants module, then update the calendarPushes filter to compare against
WEBSOCKET_MESSAGE_TYPES.CALENDAR.UPDATED instead of the hardcoded
'calendar.updated' string.
In `@server/test/lib/external-integration/testUtils.test.js`:
- Around line 108-123: The calendar manifest fixture is duplicated across three
test files; keep TEST_CALENDAR_MANIFEST in
server/test/lib/external-integration/testUtils.test.js:108-123 as the single
source and ensure it remains importable from the controller tests. In
server/test/controllers/externalIntegration/externalIntegration.calendar.test.js:7-19,
replace CALENDAR_MANIFEST with the shared import and apply per-test changes via
object spreading; make the same replacement in
server/test/controllers/integrationHost/integrationHost.calendar.test.js:9-18,
preserving the shared app_password field.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
Push a commit to this branch (recommended)
Create a new PR with the fixes
ℹ️ Review info⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dc768973-0a79-4604-8e19-0faa464322c9
📥 Commits
Reviewing files that changed from the base of the PR and between eec9cbb and 22e4331.
…ound
The prune window overlap became exclusive on the end side, but the JSDoc
of upsertEvents still described the previous `(end ?? start) >= from`
test, contradicting the implementation just below it.
Autofix-Pass: 1
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Re-review at 8fae0d6 (after the invalidation / prettier / exclusive-end follow-ups).
Prior blockers from da71036 are cleared. Verified in-tree: 10k cap on the move path, shared true→false snapshots wasShared before notify, event moves push source∪destination selectors, uninstall notifies previous viewers, exclusive-end overlap, and calendar is included in install / discover / device exclusions. Soft docs residuals (exclusive-end formula, complete invalidation set) are addressed too. Living-spec contracts for user-scoped ids, ownership on user calendar writes, empty-schema Enable, rate limits, and tenant isolation still look solid.
Not ready to approve yet:
codecov/patch failing at 94.53% (target 100%) — ~76 uncovered lines, mostly validation/error paths in publishCalendarEvents / publishCalendars / account helpers. This will block merge once the suite is green.
updateEvent ownership is incomplete — userId is checked, but req.body is still mass-assigned, so calendar_id can move an owned event onto another user's calendar (inline).
Soft / non-blocking:
Latest Server test red is an unrelated flaky zigbee2mqttManager.publish sinon assertion (5701 passing otherwise; prior 22e4331 run was green).
PR title/body still read as docs-only while milestone 1 is implemented.
disable/uninstall SEND_ALL with a mixed private+shared selector list can expose private selectors to other connected users when any touched calendar is shared — consider partitioning shared vs private notifies.
Returning nested calendar: { user_id } from PATCH /calendar/event/:selector via the ownership include may be an accidental API shape change.
No new DEVICE_FEATURE_CATEGORIES / TYPES. Not tagging risk:high (additive external-integration surface, no migration). Keeping needs:human-review + atrovato for the trust-boundary / philosophy call (unaudited container → calendars → shared → scene triggers). Removing needs:cursor-review.
Address the review findings on the calendar external integrations:
- updateEvent: a user-initiated update writes only the editable fields, so
owning an event is no longer a way to move it onto someone else's
calendar; the calendar include becomes an INNER JOIN, an event without
its calendar is "not found" instead of a dereference of null.
- upsertEvents: reject an empty external_id (the lookup would otherwise
match the manually created events) and a window without prunePrefix
(startsWith(undefined) silently matched nothing).
- disableCalendarAccount: partition the deletion notification by prior
visibility, the selectors of private calendars no longer reach the
broadcast.
- getCalendarAccount(s): drop the parse error from the log line, its
message embeds a snippet of the value, which carries the secrets.
- getIntegrationCalendars: reject a non-string user filter before it
reaches Sequelize.
- front: derive the optimistic calendar toggle and its rollback from the
previous state, two toggles fired back to back no longer drop each other.
Cover every line the branch adds in server/lib/calendar and
server/lib/external-integration, including the reject paths of
publishCalendars and publishCalendarEvents.
Autofix-Pass: 2
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Re-review of 6f7d9a9c (after the calendar-primitive / host-API hardening).
Previous blockers are cleared.updateEvent now restricts user-initiated writes to USER_EDITABLE_FIELDS (ownership columns stay out of reach; CalDAV with no userId still writes the full row). codecov/patch is green. CI is green (server tests, front test/build, Cypress, Docker).
The rest of the B.19 surface looks solid: user-scoped ext:<selector>:<user>: ids, tenant isolation, empty-schema Enable, exclusive-end overlap prune, 10k cap on the move path, shared true→false snapshot, event-move notifying source ∪ destination, front exclusions for calendar, no new device categories.
Remaining nits (not merge blockers; left inline):
Uninstall still broadcasts mixed private+shared selectors on SEND_ALL — disableCalendarAccount was partitioned in this commit for that leak.
calendar.update still mass-assigns req.body after the new ownership check (same pattern updateEvent just closed).
Not risk:high: additive calendar host/management API, no auth/gateway/crypto/migration. Keeping needs:human-review for the product calls (push model, shared as the scene boundary, per-user consent).
Uninstall broadcast every selector of a user on SEND_ALL as soon as one of
their calendars was shared, leaking the selectors of their private ones to
every connected user. It now partitions by prior visibility like
disableCalendarAccount does: shared selectors on SEND_ALL, private ones on
SEND to their owner.
A user-initiated calendar update wrote the whole body, so an owner could
PATCH user_id / service_id / selector / external_id and hand the calendar —
and the events it carries — over to someone else. It now writes an explicit
field list, mirroring USER_EDITABLE_FIELDS on events; internal callers (no
userId) keep writing the full row.
Autofix-Pass: 3
The reason will be displayed to describe this comment to others. Learn more.
Re-review of 16c09d63 (after the uninstall-partition / calendar-update whitelist follow-up).
Prior nits from 6f7d9a9c are cleared. Uninstall now partitions shared vs private selectors the same way disableCalendarAccount does (empty arrays are a no-op in notifyCalendarUpdated). User-initiated calendar.update writes USER_EDITABLE_FIELDS only (name / description / color / sync / shared / notify), so an owner can no longer PATCH user_id / service_id / selector / external_id. Both paths have tests.
Blocker: the branch is CONFLICTING with master. A merge today fails in four files, and two of them are not mechanical:
front/src/routes/integration/index.js — master (#2860) replaced type-based EXTERNAL_CATEGORIES / card.type === category with browse categories from the store index. This branch still filters the catalog by manifest.type and adds 'calendar'. A naive take of either side either undoes catalog categories or drops calendar’s non-admin visibility and config-page URL.
docs/specs/external-integrations.md — C.4’s calendar.account-updated row collides with #2879’s account_link oauth wording; C.5’s store row is missing master’s categories / first_seen_at.
server/lib/external-integration/index.js — calendarWriteRateLimits vs master’s networkWakeTimes (#2864): keep both.
server/utils/constants.js — CALENDAR_TYPES vs INTEGRATION_CATALOG_CATEGORIES: keep both.
Please rebase/merge master and resolve those by combining both sides, then we can re-approve.
Not this PR: the red “Server test” on this SHA is Device.migrate “should move the whole history in slices” hitting the 2s timeout — unrelated flake; Mocha reported 5721 passing / 1 failing. Front, Cypress, Docker, and lint are green.
Still good, unchanged: user-scoped ext:<selector>:<user>: ids; host-API tenant isolation; empty-schema Enable; overlap prune with exclusive end; 10k cap on the move path; shared true→false snapshot notify; event-move notifies source ∪ destination; no new device feature categories (only CALENDAR_TYPES / calendar.updated, brand-agnostic). No DB migration.
Verdict: not ready to merge until the conflicts are resolved. Not risk:high. Keeping needs:human-review + atrovato: B.19 is a new integration type with per-user accounts, a shared automation boundary, and a catalog-placement question that now has to be answered against #2860’s browse categories.
The reason will be displayed to describe this comment to others. Learn more.
git merge origin/master conflicts here: master (#2860) dropped type-based EXTERNAL_CATEGORIES and filters the sidebar with card.categories from the store index (getCardCategories, firstSeenAt). This branch still keys the catalog on manifest.type (card.type === category) and adds 'calendar' to that list.
A naive resolution either undoes the catalog-categories model or drops the B.19 bits (non-admin visibility, config-page URL, type: 'calendar'). The merge needs both:
keep master’s getCardCategories / categories / firstSeenAt and the sidebar filter on integration.categories
keep this branch’s 'calendar' in the non-admin installed filter and in getInstalledUrl (no Devices/Discovery screens)
How a type: "calendar" integration lands in the existing Calendar catalog category (calendars.json) vs a browse category from the indexer is the remaining product call — B.19 currently says it maps into Calendar.
| core → integration | `external-integration.weather.get` | `{ "message_id": "uuid", "options": { "latitude": 48.85, "longitude": 2.35, "language": "fr", "units": "metric" \| "us" } }` — the core asks a "weather" integration (B.18) for the weather; respond via `command-result` with `data: { "weather": <pivot format, B.18> }`, values in the requested unit system; ack expected within **15 s** (a third-party API call, same exception as `camera.get-image`); the payload is normalized and bounded by the core before use |
| core → integration | `external-integration.weather.get-image` | `{ "message_id": "uuid", "key": "vigilance-map" }` — the core asks for a provider image declared in the pivot's `images` (B.18 point 6); respond via `command-result` with `data: { "image": "<raw base64, no data-URI prefix>" }` — decoded bytes ≤ 500 KB, PNG or JPEG only (magic numbers checked by the core); ack within **15 s**; the core caches the validated image 10 min and only ever sends a `key` declared in the integration's last normalized weather payload (undeclared keys 404 core-side) |
| integration → core | `external-integration.weather.refresh` | `{}` — freshness nudge (B.18 point 5): "re-pull me now and re-evaluate the alert scene triggers"; no `message_id`, no ack, fire-and-forget; accepted from "weather" integrations only; rate-limited to 1/min per integration, silently dropped beyond |
| core → integration | `external-integration.calendar.account-updated` | `{ "user": "<user_selector>" }` — a user enabled or disabled the integration, changed their account values, or toggled a calendar's `sync`/`shared` ("calendar" type, B.19); no `message_id`, no ack (the `config-updated` family), emitted **after** the core applied the change (on a disable, the user's calendars are already destroyed); the integration re-reads `GET /calendar/account` and `GET /calendar` and adjusts its sync loops |
The reason will be displayed to describe this comment to others. Learn more.
Conflicts with master: this row and the following oauth.get-authorize-url row occupy the same slot as #2879’s account_link wording (redirect_uri absent for account_link). Keep both — calendar.account-updated stays, and the oauth row must keep master’s oauth2 / account_link distinction.
| `GET .../:selector` | → detail (same fields, + the main container's `"started_at"`, + `"connection_status": { "connected", "message" }` (C.3), + `"containers": [ { "name", "status", "desired", "started_at", "ports": [{ "container_port", "protocol", "host_port", "label", "name", "browsable" }], "devices": [{ "class", "granted", "available" }] } ]` for multi-container ones — the same state as `GET /container` (C.3), from which the frontend derives the "Open" links (`browsable: false` → host port displayed without link) and the `{{port:<name>}}` placeholder values (C.1)); **non-admin**: the reduced view on a communication integration, `404` on any other (indistinguishable from an unknown selector) |
| `GET /api/v1/external_integration` | → `[ { "id", "name", "selector", "status", "version", "docker_image", "store_slug", "manifest", "update_available" } ]`; **non-admin**: only the installed `type: "communication"` and `type: "calendar"` integrations, in their reduced view |
| `GET .../:selector` | → detail (same fields, + the main container's `"started_at"`, + `"connection_status": { "connected", "message" }` (C.3), + `"containers": [ { "name", "status", "desired", "started_at", "ports": [{ "container_port", "protocol", "host_port", "label", "name", "browsable" }], "devices": [{ "class", "granted", "available" }] } ]` for multi-container ones — the same state as `GET /container` (C.3), from which the frontend derives the "Open" links (`browsable: false` → host port displayed without link) and the `{{port:<name>}}` placeholder values (C.1)); **non-admin**: the reduced view on a communication or calendar integration, `404` on any other (indistinguishable from an unknown selector) |
The reason will be displayed to describe this comment to others. Learn more.
This store row is also a merge conflict with master (#2860): current master adds categories and first_seen_at to GET .../store. Taking this branch as-is would drop that C.5 contract. Same table: keep this branch’s non-admin type: "calendar" reduced view and master’s catalog fields.
The reason will be displayed to describe this comment to others. Learn more.
Conflicts with master (#2860): INTEGRATION_CATALOG_CATEGORIES was inserted in this same place. Keep bothCALENDAR_TYPES and the catalog-categories vocabulary — they are unrelated.
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
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.
github-actions
Bot
added
the
type:chore
Pierre-Gilles
added
the
needs:human-review
Cursor
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Description
This PR updates the external integrations specification to document the "calendar" type (B.19), enabling calendar providers (Google Calendar, Outlook, Nextcloud, CalDAV servers, and public feeds) to become ordinary external integrations that feed the same calendar view and scene triggers as the internal CalDAV service.
The specification covers:
Core prerequisites: batch upsert primitives for calendars/events, unique selector generation, ownership checks, explicit cleanup at uninstall, real-time WebSocket refresh, and drive-by fixes.
Per-user accounts: manifest
account_schema(same format as communication integrations), per-user activation as consent gesture, non-admin visibility, and automation boundary via thesharedflag.Host API: five new endpoints for account management, calendar listing, and event upsert/delete with rate limiting and tenant isolation.
Normalization and bounds: field whitelisting, length/format validation, color validation, and a 10,000-event-per-calendar cap.
Sync semantics: field ownership split (integration-owned: name/description/color; user-owned: sync/shared/selector), window-based pruning for deletions, and idempotent upsert.
Milestone 2 sketch: per-user OAuth2 support (deferred to a future milestone).
Frontend and SDK: calendar category integration, real-time refresh listener, and new SDK methods (
getCalendarAccounts,getCalendars,publishCalendars,deleteCalendar,publishCalendarEvents,onCalendarAccountUpdated).The design follows the "integration syncs, core stores" principle: integrations own the provider protocol and sync cadence; the core owns storage, normalization, and automations. This is a push model (like CalDAV) rather than pull (like weather), keeping third-party containers off the critical path of scene scheduling.
Also updates
AGENTS.mdto note that B.19 is now specified but not yet implemented.Checklist
https://claude.ai/code/session_01Jhfyd3Vz9icRe5QjuUUXVi
Summary by CodeRabbit