Skip to content

[feature] Add support for multiple local calendars - #1243

Draft
torbenvanassche wants to merge 7 commits into
ulsklyc:mainfrom
torbenvanassche:feature/local-calendars
Draft

torbenvanassche wants to merge 7 commits into
ulsklyc:mainfrom
torbenvanassche:feature/local-calendars

Conversation

@torbenvanassche

@torbenvanassche torbenvanassche commented Sep 16, 2026

Copy link
Copy Markdown

Summary

This adds a local_calendars table and links local calendar_events through local_calendar_id. External synced events keep local_calendar_id = NULL, so local calendars remain separate from Google/CalDAV/Outlook sources while local events and imported ICS events can be grouped, colored, and exported per calendar.

This PR is a result of the discussion in #1231

Changes

  • Add local_calendars storage, default-calendar migration, and calendar_events.local_calendar_id
  • Add local calendar CRUD endpoints and OpenAPI coverage
  • Let calendar create/update/import flows target a specific local calendar
  • Scope local calendar ICS export feeds to one local calendar
  • Show local calendar selection and management in the calendar UI and subscription import flow
  • Add localized strings for local calendar labels/actions
  • Add tests for local calendars, ICS import targeting, and calendar API structure

Checklist

  • npm test passes
  • Follows CONTRIBUTING.md conventions
  • No new frontend dependencies (vanilla JS, no frameworks)
  • UI strings use t('key') (no hardcoded text)
  • CHANGELOG.md updated (if user-facing change)

@ulsklyc ulsklyc left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this, and sorry for the wait. You opened the draft 46 minutes after I asked for it and then heard nothing for four days, which is on me.

I read the whole branch at 69ec7cd49 in a separate worktree and ran the affected suites. Before the findings, the part that matters most: the model is close, and the shape you chose is the right starting point. A separate table plus a nullable link on the event is what I would have sketched too, the delete path reassigns events instead of dropping them, the filter key local:<id> slots in next to sub: and cal: without inventing a second convention, and all 25 new strings are in all 24 locales with real translations rather than placeholders. That last one is the thing contributors almost always get wrong.

Most of what follows is either a decision that belongs to me, not a defect in your work, or a consequence of the branch having been written against a main that has moved. I have separated those out.

Blocking

1. The three reserved migrations have to go. server/db.js:8697-8712

{ version: 218, description: 'Reserved for documents expiry migration in PR #1255', up: `SELECT 1;` },
{ version: 219, description: 'Reserved for health prevention migration in PR #1256', up: `SELECT 1;` },
{ version: 220, description: 'Reserved for inventory service log migration in PR #1257', up: `SELECT 1;` },

I understand why you did this, and it was a thoughtful instinct, but the runner decides purely by number (server/db.js:9307):

const pending = MIGRATIONS.filter((m) => !applied.has(m.version));

and records the version on success (server/db.js:9329). Any installation that runs the placeholder 218 writes 218 into schema_migrations and can then never run the real 218. The documents-expiry columns would simply never be created there, silently, forever. A number cannot be reserved by a no-op in an append-only scheme, only consumed.

npm run test:migrations-append-only is green on your branch, so the guard did not warn you. That is a gap in my guard, not something you missed.

Please drop all three entries. Local calendars take one number, the next free one.

2. The migration number. server/db.js:8715

My note in #1231 was already out of date when I wrote it. Measured now: main ends at 220 (218 documents expiry, 219 health prevention, 220 fasting reminders from #1179). So 221 is the next free number and your choice is right, except that #1257 also claims 221 in its description. Whoever lands first pushes the other to 222. I will sequence that; nothing for you to guess at. Just expect one more bump.

3. innerHTML write. public/pages/calendar.js:3544

body.innerHTML = renderLocalCalendarsContent();

npm run test:frontend-audit is red on this line and green at your merge base. This is a hard constraint with a commit hook behind it. body.replaceChildren() followed by body.insertAdjacentHTML('beforeend', ...) is the pattern used a few lines above at calendar.js:1796-1797. Everything else in the new UI is clean, including the escaping, so this is a one-line fix.

4. npm run test:calendar-occurrence-overrides is 45 of 86 red

Error: table calendar_events has no column named local_calendar_id
    at server/services/calendar-occurrence-overrides.js:1064

86 of 86 pass at your merge base, so this is new. That suite builds its schema from MIGRATIONS_SQL in server/db-schema-test.js, which is a hand-maintained mirror the branch does not touch. Its own header comment describes exactly this failure mode (server/db-schema-test.js:15-19).

One wrinkle that will bite you: your migration 221 uses up(db) as a function, not a SQL string, so it cannot simply be mirrored into MIGRATIONS_SQL. Either add a hand-written SQL equivalent under key 221, or add the column to the entry the suite actually runs.

5. The new routes have no permission check. server/routes/calendar/local-calendars.js:28,41,62,108,130,144

Right now any authenticated user can create, rename and delete local calendars and mint or revoke ICS feed tokens. Every sibling in that directory is gated: google.js:31, apple.js:43, caldav.js:21, outlook.js:24 all use requireAdmin, and the precedent for a member-level calendar write is mayWriteModule(req, 'calendar') at sync-targets.js:119.

main standardized this after you branched: #1303 added explicit per-route write checks to meals, recipes and shopping, plus a test:cross-module-write-rights suite. The rule deliberately does not live in a middleware, so each route states it.

The UI side needs the same: calendar.js:1803 renders the manage button unconditionally, and there is no role check anywhere in that file.

Your own answer in #1231 was "administrator, potentially an owner of the calendar", so I think you already had the right instinct here. See the model questions for which one I want.

Should fix

6. ensureDefaultCalendar claims external events. server/services/local-calendars.js:43-47

UPDATE calendar_events
SET local_calendar_id = ?
WHERE local_calendar_id IS NULL

Your migration gets this right by pairing the two columns (server/db.js:8777: WHERE external_source = 'local' AND local_calendar_id IS NULL). This copy of the same rule omits the external_source half, so it sweeps up every Google, CalDAV and ICS event, which the model says must stay NULL. I reproduced it:

CalDAV meeting   src=caldav  local_calendar_id=2

In fairness: I could only trigger this by deleting the local_calendars rows directly. No route flips is_default, and DELETE refuses the default (local-calendars.js:114), so after migration 221 a default always exists and this branch is not reachable through the running app today. It is latent rather than live. It matters anyway because calendar_events has been rebuilt by migrations before (v114/v117/v166/v194), and local_calendars is not in CRITICAL_COLUMNS (server/db.js:8884-8891), so the self-healing path does not cover it.

7. Generated local events never get a calendar

server/services/birthdays.js:290, server/routes/housekeeping.js:292 and server/mcp/tools.js:236 all insert external_source = 'local' without local_calendar_id. Measured with the full migration chain:

after migration 221:    Birthday Anna  src=local  local_calendar_id=1
created after upgrade:  Birthday Bob   src=local  local_calendar_id=null

So the same kind of event lands in the default calendar or nowhere, decided only by whether it predates the upgrade, and nothing repairs it later. The pre-existing birthday also ends up in the birthdays layer and a local calendar at once, which is the "one assignment, two truths" shape I have been chasing in #1270.

An event with external_source = 'local' and local_calendar_id IS NULL also gets null from eventSourceKey() (calendar.js:1133), so it cannot be hidden as a group, which is the original request.

This is mostly a model decision, so it is in the questions below too.

8. The new suite never runs. package.json

test:calendar-local-calendars is defined, but the test chain does not include it. I checked: calendar-structure, ics-import, calendar-occurrence-overrides and frontend-audit are all in the chain, only the new one is missing. A full npm test would not have run it either, so leaving the checkbox unticked was the honest call.

9. One assertion cannot go red for its own stated reason. test/test-calendar-local-calendars.js:109-120

"local calendar feeds exclude externally synchronized events" seeds a Google event without local_calendar_id (:112-116). The feed filter has two clauses, and the local_calendar_id = ? one already excludes a NULL row on its own. I removed only AND e.external_source = 'local' from ics-export.js:266-267 and reran:

exit=0   tests 5 / pass 5 / fail 0

The test named after that distinction stays green when you delete it. To make it measure, the fixture needs an external event that carries a calendar id, which is exactly the state finding 6 produces. The two fixes go well together.

Related: :14,18 set const ADMIN and let actor = ADMIN, and actor is never reassigned, so the suite only ever runs as admin and cannot see finding 5.

10. The per-calendar feed borrows someone's identity. server/services/ics-export.js:402-414

const userId = calendar.created_by ?? /* first admin */ ?? 0;

That userId decides calendar_feed_show_assignees (:275). So a household-wide token feed shows or hides member names according to one person's personal preference, and switches to the first admin's preference as soon as the creator is deleted, because created_by is ON DELETE SET NULL. The feed content changes without anyone touching the feed.

11. Event visibility does not reach the feed

ics-export.js never calls visibilityWhere(), while the read path does (read.js:95, :180, calendar-event-reader.js:191). The comment at ics-export.js:271 claims identical visibility logic to GET /api/v1/calendar, and that claim is already a bit generous on main today, so this is not your regression. But it changes character here: the personal feed belongs to exactly one user, whereas your per-calendar feed is an unauthenticated shareable link with no user, and it now carries private events from every member who filed one into that calendar. I would rather decide this before it ships than after.

12. The create guard fires when it should not. server/routes/calendar/crud.js:405-407

if (vLocalCalendar.value !== undefined && req.body.external_source && req.body.external_source !== 'local') {

With fallbackDefault: true, value is never undefined, so the left side is always true and the condition collapses to "the caller named a non-local source". A create that names a foreign source and never mentions local_calendar_id is rejected with 400 anyway. The PUT version at :665-666 is bound to localCalendarProvided and is correct; the POST one wants the same treatment.

13. Plural categories for the count string

localCalendarEventCount ships _one and _other in all 24 locales. Polish, Russian, Ukrainian, Czech and Arabic use _few elsewhere in the project, 24 times in pl.json alone (for example shopping.itemsRemovedToast_few). For count=2 Polish selects few, falls back to the base key "{{count}} wydarzeń", and renders the genitive plural where "2 wydarzenia" is correct.

test:i18n-plural is green because it checks a hand-listed set of keys plus base-key presence (test/test-i18n-plural.js:192), not the categories each language needs. Another blind spot of mine, not something you were told about.

14. Two comments now contradict the code

  • calendar.js:1148-1151 justifies the old design with "Keine eigene Route: ... die Verwaltungsrouten der Quellen sind requireAdmin - ein Mitglied kaeme dort nicht durch." There is now a route, everyone calls it, and it is not admin-gated. Once finding 5 is fixed this comment needs rewording either way.
  • public/utils/event-color.js:3-5 and :40 say three sources in fixed order; :83 now has four.

15. No CHANGELOG, docs/test-suites.md or docs/SPEC.md entry

The branch touches no documentation at all, though the CHANGELOG checkbox is ticked. A new table and a new data model want a docs/SPEC.md note, and CONTRIBUTING.md:94 asks for the suite catalogue. #1303 is a good template for the shape.

Nice to have

  • calendar.js:1493-1499: loadLocalCalendars catches everything into []. Once a permission gate exists, a member silently loses every calendar switch in the filter sheet instead of seeing an error.
  • calendar.js:4940: the picker fallback guesses id: currentEvent?.local_calendar_id || 1. 1 is not necessarily the default calendar.
  • server/db.js:8727 declares feed_token TEXT UNIQUE and :8735 adds idx_local_calendars_feed_token on top. Two indexes for one guarantee.
  • server/openapi/paths/calendar.js:96-98: POST /calendars and PUT /calendars/{id} use jsonBody(null) while /import got a full schema at :72-87. name, color and sort_order are undocumented.
  • public/styles/calendar.css: the new rule breaks at max-width: 640px, the same file uses max-width: 639px twice. At exactly 640px the new rule applies and its neighbours do not.
  • The default calendar is created with the hard-coded English name 'Calendar' (server/db.js:8765). calendar.defaultLocalCalendar exists but is only used as a frontend fallback.
  • personal-calendar-subscriptions.js creates the new calendar before running the import, so a failed import leaves an empty calendar behind.
  • test/test-calendar-structure.js:146,149 pins the literal 69 next to EXPECTED.length. That is the file's existing convention, not yours, but with #1257 in flight it is a live merge hazard.

Model questions

These are mine to answer, not defects in the PR. I am writing them out so you can see what "until the model is settled" actually means and decide how much you want to carry.

A. Is NULL the right marker? The strongest evidence that it is overloaded is in your own code: the feed pairs local_calendar_id with external_source because the id alone is not enough (ics-export.js:266-267), and ensureDefaultCalendar forgets the pairing and breaks (finding 6). Every correct read has to remember to do this. Options I see: keep NULL but put the pairing in one shared helper that nothing bypasses; or make membership explicit so "local without a calendar" becomes unrepresentable. I lean towards the shared helper plus a guard, because it is the smaller migration, but I want to think it through.

B. What belongs in a local calendar at all? Birthdays, housekeeping visits, waste pickups and name days are all external_source = 'local' and all have their own layer and their own feed already. Your migration currently claims the pre-existing ones (finding 7). I think the answer is that a local calendar holds only events a person actually authored, and generated events stay out, which means the migration needs a narrower WHERE and the generated writers stay untouched. If so, the filter needs to keep showing them as their own layers, which it already does.

C. One calendar table or two? local_calendars and external_calendars now both answer "which calendar is this event in", and five queries join ec and lc side by side. They do not currently collide, because the columns are mutually exclusive in practice. I would rather be explicit about that being the design than have it be true by accident. This is the question I most want to settle before merging.

D. Can a single occurrence leave its series' calendar? You put local_calendar_id in OVERRIDE_FIELDS (calendar-occurrence-overrides.js:34) and SCALAR_OVERRIDE_FIELDS (:620), so it can. In the per-calendar feed the master is filtered out, mastersById.get() misses (ics-export.js:394), and the replacement is emitted with its own UID and no RECURRENCE-ID instead of the master's UID (:201). The same occurrence therefore has one UID in the personal feed and a different one in the calendar feed, and the UID flips back if it is moved back. Nothing crashes. I am inclined to make membership a series-level property only, which would also simplify your override code. Note I derived this from reading rather than running a two-calendar series, so check me on it.

E. Who owns a feed and what is in it? Currently: token-only capability like the personal feed (which is fine and follows the existing pattern), household-wide, no visibility filter, and anyone signed in can mint one. Your #1231 answer offered administrator or calendar owner. My current preference is mayWriteModule(req, 'calendar') for create, rename and reorder, requireAdmin for minting and revoking feed tokens because that is the part that leaves the house, and the visibility filter applied to the feed. But E depends on F.

F. Does a local calendar get an owner now? #739 is adjacent, not the same model: it gives the connection an owner and a default visibility, while this PR groups local events with no owner at all (created_by here is provenance, and it goes to NULL when the user is deleted). D#1297 is a third thing again, one event pushed to several accounts by assignee. All three push in the same direction though, which is that "which calendar" acquires a person dimension. If local calendars stay household-wide, #739 has to reconcile that later. I would rather decide it once, here.

G. Confirm the sync answer. local_calendar_id is not in MIRRORED_FIELDS (calendar-outbound.js:39-42), so moving an event between local calendars pushes nothing outward. I believe that is right and intended: a local calendar is a Yuvomi-side grouping with no counterpart on the CalDAV or Google side, while target_caldav_* remains the outbound address. If you agree, that deserves a comment on the column, because a row now carries two unrelated notions of "calendar" and the next reader will conflate them.

Where this leaves us

Suites I ran on 69ec7cd49, one at a time: 22 green, 2 red (calendar-occurrence-overrides 45/86, frontend-audit), both green at your merge base. I deliberately did not run the full chain, so that is not the complete picture of what is red. The branch is also CONFLICTING against current main now, largely because of the reserved migrations in finding 1.

Suggested order, so you are not doing work I might invalidate:

  1. Findings 1, 3, 4 and 8 are unambiguous and independent of any decision. Worth doing whenever you like.
  2. Finding 5 waits on my answer to E and F. Please do not guess at a gate; I will give you one.
  3. Findings 6, 7 and 9 change shape depending on A and B, so they are better done after.

I will answer A through G, and I will keep #1231 as the place where the model lives so this thread stays about the code. Please do not rebase or renumber until #1257 is sequenced, or you will do it twice.

To be clear about the status, since it has not changed since #1231: this stays a draft until the model is settled, and that is a decision about Yuvomi's data model rather than a verdict on your work. The branch is careful and it is doing the arguing for me, which is exactly what I asked a draft for.

Recommendation: COMMENT. Approving a draft would be wrong here on two counts. The chain is red and there is a migration pattern that would silently break existing installations, so there is nothing to approve yet. And the substantive blockers are decisions I owe you rather than corrections you owe me, which is a conversation, not a change request. Requesting changes would put the ball in your court for items where it is actually in mine.

@torbenvanassche

Copy link
Copy Markdown
Author

That's very in-depth, thank you very much. I will review this PR in the coming days!

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants