From 69bc57221bef1c827adf5faf58d8e8ed639305d9 Mon Sep 17 00:00:00 2001 From: Lucas Berger Date: Sun, 7 Jun 2026 15:39:43 -0400 Subject: [PATCH] docs(quick-260607-l6l): summary, state, handoff update + reminder-options todo - 260607-l6l SUMMARY (executor + orchestrator review follow-ups) - STATE.md: quick-task row + session continuity (edit/delete blocker resolved) - HANDOFF.json: 3 write-path code bugs marked done; remaining = operator rebuild + Gate 2 human/device checkpoints - new backlog todo: event-creation reminder/VALARM options --- .planning/HANDOFF.json | 18 +- .planning/STATE.md | 7 +- .../260607-l6l-SUMMARY.md | 173 ++++++++++++++++++ .../event-creation-reminder-options.md | 34 ++++ 4 files changed, 221 insertions(+), 11 deletions(-) create mode 100644 .planning/quick/260607-l6l-fix-phase-03-write-path-correctness-bugs/260607-l6l-SUMMARY.md create mode 100644 .planning/todos/pending/event-creation-reminder-options.md diff --git a/.planning/HANDOFF.json b/.planning/HANDOFF.json index ece5ee7..9fc7073 100644 --- a/.planning/HANDOFF.json +++ b/.planning/HANDOFF.json @@ -5,9 +5,10 @@ "phase_name": "event-write-back-pwa-install", "phase_dir": ".planning/phases/03-event-write-back-pwa-install", "plan": "Gate 2 (Part D live verification)", - "task": "delete/edit join fix (diagnosed, not started)", + "task": "delete/edit join fix RESOLVED (quick 260607-l6l); Gate 2 human/device checkpoints remain", "total_tasks": null, "status": "paused", + "update_2026_06_07": "Quick task 260607-l6l cleared the three write-path CODE bugs (edit/delete join 503, displayName, GET events filter) on branch gsd/v1.0-milestone — build clean, 100/100 api tests pass, handler-coupled regression test added, deriveDisplayName extracted as shared helper. Remaining items are operator rebuild + human/device Gate 2 checkpoints, not code work.", "completed_tasks": [ {"id": "tunnel", "name": "PWA loads through Pangolin/newt — newt MTU 1280→1200 (operator) fixed large-asset blackhole; API serves full ./public tree (431ab31)", "status": "done"}, {"id": "auth", "name": "Real Authelia OIDC login working — client_id=familysync-dev, scopes incl offline_access, /api/login route + redirect (quick 260606-tv8), fetchMe redirect:manual (1adb460)", "status": "done", "commit": "874f23d"}, @@ -17,21 +18,22 @@ {"id": "spike-cleanup", "name": "Deleted obsolete spike user (id=1 Dev User) + calendar id=1 + cached events (DB op, operator-approved)", "status": "done"} ], "remaining_tasks": [ - {"id": "edit-delete-join", "name": "BLOCKING: edit (PATCH /:uid/edit ~line295) + delete (DELETE /:uid ~line392) handlers in apps/api/src/routes/events.ts select calendars.url/userId WITHOUT joining calendars → Drizzle 503 → delete dialog never closes. Add .innerJoin(calendars, eq(calendarEvents.calendarId, calendars.id)) to both + regression test that runs the REAL query builder (existing tests mock db.select() and miss it).", "status": "not_started"}, - {"id": "displayName", "name": "me.ts passes (missing) email claim as displayName and doesn't read name/preferred_username; oidc_iss also blank → legend name blank (user id=2 display_name=''). May also need Authelia to put name/email in ID token or call userinfo.", "status": "not_started"}, - {"id": "events-filter", "name": "GET /api/events has no userId/isShared filter (returns all users' events). Latent now (only 1 real user) but a real bug for 2nd member.", "status": "not_started"}, + {"id": "edit-delete-join", "name": "RESOLVED (quick 260607-l6l, commit 2870413): added .innerJoin(calendars,...) to PATCH /:uid/edit + DELETE /:uid lookups; handler-coupled regression test (asserts 202 + innerJoin spy, verified RED when join removed).", "status": "done", "commit": "2870413"}, + {"id": "displayName", "name": "RESOLVED (quick 260607-l6l, commits 23c8bb3 + a99ef1d): shared deriveDisplayName helper (name→preferred_username→email→Member) used in me.ts + resolveUserId; user.ts now UPDATEs an existing blank display_name on re-upsert (so user id=2 self-corrects on next request). OPERATOR FOLLOW-UP: legend shows full name only if Authelia emits name/preferred_username — else it shows email.", "status": "done", "commit": "23c8bb3"}, + {"id": "events-filter", "name": "RESOLVED (quick 260607-l6l, commit 00a0454): GET /api/events now filters WHERE (calendars.userId=currentUser OR calendars.isShared) AND , correctly grouped.", "status": "done", "commit": "00a0454"}, {"id": "sync-toast", "name": "Syncing toast not animated / ~27s (outbox 15s drain + CalDAV) looks stalled — UI polish (backlog candidate).", "status": "not_started"}, {"id": "gate2-A2A3", "name": "Gate 2 Part A2 (session persists across browser restart) + A3 (2nd member distinct color) — operator verify.", "status": "not_started"}, {"id": "gate2-B", "name": "Gate 2 Part B — iOS standalone install + login (device-only; manifest/sw fixes now unblock it).", "status": "not_started"}, {"id": "gate2-C", "name": "Gate 2 Part C — 5-min SSE smoke (Phase 4 entry gate).", "status": "not_started"} ], "blockers": [ - {"description": "Delete (and latently edit) events 503 — missing calendars join in events.ts. Diagnosed; fix not yet applied. User was choosing how to land it (batch vs quick-only vs inline) and paused.", "type": "technical", "workaround": "none — delete is broken until fixed"}, + {"description": "RESOLVED — edit/delete 503 (missing calendars join) fixed in quick 260607-l6l (commit 2870413). Code on gsd/v1.0-milestone; not yet rebuilt into the running container.", "type": "technical", "workaround": "n/a — fixed; operator must rebuild to deploy"}, {"description": "playwright-cli daemon wedges/crashes in this WSL2 env (hangs on never-settling pages; even open/run-code fail after). Cannot drive browser verification here.", "type": "technical", "workaround": "Verify via curl + ask the operator to test in their real browser/incognito."} ], "human_actions_pending": [ - {"action": "Re-test in browser: create a NEW 9am event (verify time + your-user attribution); the OLD wrong-time test event (5am) should be deleted once delete is fixed.", "context": "BUG A/B fixes deployed; old test event was written pre-fix with bad time.", "blocking": false}, - {"action": "Decide how to land the edit/delete join fix (batch with displayName+events-filter, quick-only, or inline).", "context": "User paused mid-AskUserQuestion on this.", "blocking": true} + {"action": "Rebuild + deploy: docker compose up -d --build (newt target drops ~30s then self-recovers — not a bug).", "context": "The 3 write-path code fixes (260607-l6l) are committed but not in the running container.", "blocking": true}, + {"action": "Browser re-test after rebuild: delete an event (dialog should close, event disappears) and edit an event (no 503). Create a NEW 9am event; the OLD wrong-time 5am test event can now be deleted.", "context": "BUG 1 (join) + earlier BUG A/B (timezone/identity) fixes.", "blocking": false}, + {"action": "Confirm calendar legend shows a name. If it shows email instead of full name, configure Authelia to emit the name/preferred_username OIDC claim (code reads them preferentially; existing blank row self-corrects on next request).", "context": "BUG 2 — displayName helper deployed; legend content depends on Authelia claim emission.", "blocking": false} ], "decisions": [ {"decision": "newt -mtu 1200 (systemd drop-in)", "rationale": "newt default tunnel MTU 1280 == eth0 underlay; WireGuard overhead made encrypted packets exceed 1280 → large transfers (JS bundle) blackholed. THE root cause of the 'spinner'.", "phase": "03"}, @@ -39,6 +41,6 @@ {"decision": "fetchMe uses redirect:manual; serve full ./public; OIDC_SCOPES constrained", "rationale": "fetch followed cross-origin 302 and hung; static serving only did /assets/*; empty OIDC_SCOPES requested all scopes_supported (Authelia invalid_scope).", "phase": "03"} ], "uncommitted_files": [], - "next_action": "Fix the edit+delete missing-calendars-join bug in apps/api/src/routes/events.ts (add .innerJoin(calendars, eq(calendarEvents.calendarId, calendars.id)) to both the PATCH /:uid/edit (~line 295) and DELETE /:uid (~line 392) lookups), with a regression test that exercises the real query builder. Re-offer the user the landing choice they were deciding (batch with displayName + /api/events filter, quick-only, or inline). Then rebuild (docker compose up -d --build) and have the operator re-test delete in the browser.", + "next_action": "The three write-path code bugs are FIXED (quick 260607-l6l, on gsd/v1.0-milestone, build clean + 100/100 api tests). NEXT: operator rebuilds (docker compose up -d --build) and re-tests delete/edit + legend name in a real browser. Then resume the remaining Gate 2 checkpoints: Part A2 (session persists across browser restart) + A3 (2nd member distinct color); Part B iOS standalone install+login (device-only); Part C 5-min SSE smoke (Phase 4 entry gate). sync-toast polish is a backlog candidate. New backlog todo captured: event-creation reminder/VALARM options (.planning/todos/pending/event-creation-reminder-options.md).", "context_notes": "This session went from 'paused awaiting docker decisions' to a full Gate 2 live bring-up. The big unlock was the newt MTU fix — every earlier 'spinner' symptom was the JS bundle blackholing through the tunnel, not auth. Along the way fixed 6+ real bugs (auth redirect, static serving, OIDC scopes/client_id, timezone, calendar identity) and cleaned spike data. Stack is running (docker compose production target); /health 200 through tunnel; real OIDC login works. Write path (create) works end-to-end to Fastmail. Delete/edit are the current blocker (trivial join fix). Do NOT use playwright-cli (broken here). Do NOT read/write .env via tools (permission-locked; operator applies .env changes). Every docker compose recreate drops newt's target ~30s (503) then self-recovers — not a bug. DB now: 1 user (id=2, display_name blank), calendars id=2 Calendar(509ev) + id=3 USA Holidays(32)." } diff --git a/.planning/STATE.md b/.planning/STATE.md index 516ad60..306dd47 100644 --- a/.planning/STATE.md +++ b/.planning/STATE.md @@ -105,6 +105,7 @@ Recent decisions affecting current work: | # | Description | Date | Commit | Directory | |---|-------------|------|--------|-----------| | 260606-tv8 | Fix missing sign-in redirect in the PWA (Phase 03 auth-entry gap from Gate 2): guarded /api/login → / + full-page redirect on unauthenticated fetchMe | 2026-06-07 | 7c6531f | [260606-tv8-fix-missing-sign-in-redirect-in-the-pwa-](./quick/260606-tv8-fix-missing-sign-in-redirect-in-the-pwa-/) | +| 260607-l6l | Batch-fix Phase 03 write-path bugs: events.ts edit/delete missing calendars innerJoin (503, BLOCKING) + handler-coupled regression test; shared deriveDisplayName helper (me.ts + resolveUserId, corrects blank rows); GET /api/events userId/isShared ownership filter | 2026-06-07 | 2870413 | [260607-l6l-fix-phase-03-write-path-correctness-bugs](./quick/260607-l6l-fix-phase-03-write-path-correctness-bugs/) | ## Deferred Items @@ -119,6 +120,6 @@ Recent decisions affecting current work: ## Session Continuity -Last session: 2026-06-07T01:30:00.000Z -Stopped at: Gate 2 live bring-up — real OIDC login working; quick task 260606-tv8 fixed the missing sign-in redirect; rebuild + browser re-verify pending -Resume file: None +Last session: 2026-06-07 — Completed quick task 260607-l6l (write-path bug batch) +Stopped at: Edit/delete 503 blocker RESOLVED (missing calendars join), displayName + GET-events-filter fixed. Code on gsd/v1.0-milestone; build clean, 100/100 api tests pass. NEXT: operator must rebuild (docker compose up -d --build) and browser re-test delete/edit + legend name. Remaining Gate 2 items (A2/A3 session+2nd-member-color, B iOS install, C SSE smoke) are human/device checkpoints — see HANDOFF.json. +Resume file: .planning/HANDOFF.json (updated — 3 code bugs cleared, gate-2 human checkpoints remain) diff --git a/.planning/quick/260607-l6l-fix-phase-03-write-path-correctness-bugs/260607-l6l-SUMMARY.md b/.planning/quick/260607-l6l-fix-phase-03-write-path-correctness-bugs/260607-l6l-SUMMARY.md new file mode 100644 index 0000000..ce537cc --- /dev/null +++ b/.planning/quick/260607-l6l-fix-phase-03-write-path-correctness-bugs/260607-l6l-SUMMARY.md @@ -0,0 +1,173 @@ +--- +phase: quick-260607-l6l +plan: 01 +subsystem: api/routes +tags: [bug-fix, events, auth, displayName, calendar-ownership, drizzle, tdd] +dependency_graph: + requires: [] + provides: + - "PATCH /:uid/edit and DELETE /:uid event lookups work (no 503)" + - "Robust displayName derivation from OIDC claims in me.ts + resolveUserId" + - "GET /api/events scoped to current user + shared calendars" + affects: + - apps/api/src/routes/events.ts + - apps/api/src/routes/me.ts + - apps/api/src/auth/user.ts + - apps/api/tests/routes/events.test.ts +tech_stack: + added: [] + patterns: + - "Drizzle innerJoin for cross-table selects (events → calendars)" + - "toSQL() on real drizzle instance (no DB) as regression guard" + - "OIDC claim preference chain: name → preferred_username → email → sub fallback" +key_files: + created: [] + modified: + - apps/api/src/routes/events.ts + - apps/api/src/routes/me.ts + - apps/api/src/auth/user.ts + - apps/api/tests/routes/events.test.ts +decisions: + - "Updated user.ts to fix blank displayName for existing rows (update on re-upsert when displayName was null)" + - "Used and(ownership_predicate, date_window_or) structure for GET / WHERE clause" + - "Symlinked worktree node_modules to main repo for test execution (runtime-only)" +metrics: + duration: "~25 minutes" + completed: "2026-06-07" + tasks_completed: 3 + files_modified: 4 +--- + +# Phase quick-260607-l6l Plan 01: Fix Phase 03 Write-Path Correctness Bugs Summary + +**One-liner:** Fix three confirmed Phase 03 write-path bugs: missing innerJoin on PATCH/DELETE lookups (503), blank displayName from weak OIDC claim reading, and unscoped GET returning all users' events. + +## Tasks Completed + +| # | Name | Commit | Files | +|---|------|--------|-------| +| 1 | Add missing innerJoin to PATCH+DELETE event lookups | 2870413 | events.ts, events.test.ts | +| 2 | Derive displayName from OIDC claims in me.ts + resolveUserId | 23c8bb3 | events.ts, me.ts, user.ts | +| 3 | Scope GET /api/events to current user + shared calendars | 00a0454 | events.ts | + +## Bug Details + +### BUG 1 — Missing join on PATCH/:uid/edit and DELETE/:uid (BLOCKING) + +**Root cause:** Both handlers selected `calendars.url` and `calendars.userId` from `.from(calendarEvents)` with no join. Drizzle's query builder throws at `toSQL()` time when a selected column references a table not in FROM — this propagates as an unhandled exception → 503. + +**Fix:** Added `.innerJoin(calendars, eq(calendarEvents.calendarId, calendars.id))` between `.from(calendarEvents)` and `.where(eq(calendarEvents.uid, uid))` in both handlers. Mirrors the working GET / join at ~line 153. + +**Regression test:** Added `describe('regression: edit/delete lookups join calendars')` using `vi.importActual` to access the real drizzle query builder (no DB connection — `toSQL()` only). Two tests assert the generated SQL matches `/inner join[\s\S]*\`calendars\`/i`. These tests catch any future removal of the join. + +**Mock updates:** Updated PATCH, DELETE, and CR-01 PATCH `beforeEach` blocks to route through `.from().innerJoin().where()` instead of the old `.from().where()` chain. + +### BUG 2 — Blank displayName from weak OIDC claim reading + +**Root cause:** Both `me.ts` and `events.ts resolveUserId` read only the `email` claim and passed it as `displayName` to `upsertUser`. When `email` is absent (which Authelia may or may not emit depending on configuration), `displayName` becomes `undefined` → `null` in the DB → blank calendar legend name. + +Additionally, `upsertUser` returned existing rows unchanged — meaning an already-blank `displayName` would never be corrected even after the claim-derivation fix. + +**Fix (me.ts + events.ts):** Both call sites now derive `displayName` using the preference chain: +1. `name` — full name set by the IdP (most human-friendly) +2. `preferred_username` — login handle; still readable +3. `email` — reveals contact info but acceptable fallback +4. `"Member " + sub.slice(0, 8)` — always present; not human-friendly but never blank + +Each candidate is tested defensively: `typeof v === 'string' && v.trim() !== ''`. + +**Fix (user.ts):** Added an UPDATE branch: when an existing user row has `displayName = null` and the caller supplies a non-null value, issue `UPDATE users SET display_name = ? WHERE id = ?` and return the corrected row. Signature unchanged. + +**Re: existing blank rows:** Existing rows with blank `displayName` ARE corrected — on the next request that flows through `/api/me` or any write endpoint (`resolveUserId`), `upsertUser` detects the null displayName and issues the UPDATE. No manual DB intervention needed. + +**Re: operator action still required:** Whether Authelia emits `name` or `preferred_username` depends on Authelia's claim-emission configuration (userinfo scope, claim mappings). The code now reads whatever claims are present. If Authelia only emits `email`, the legend will show the email address — readable but not the full name. To get the full name in the legend, the operator must configure Authelia to emit the `name` or `preferred_username` claim. This is an operator concern, out of scope for this code fix. + +### BUG 3 — GET /api/events returning all users' events + +**Root cause:** The GET / handler had no ownership predicate — it returned events from all calendars in the DB, regardless of who owns them. The second household member would see the first member's private events. + +**Fix:** Added `resolveUserId(c)` call at the top of the GET handler (returns 401 if unauthenticated). Added ownership predicate wrapped with `and()` around the existing date-window `or()` block: +``` +and( + or(eq(calendars.userId, currentUserId), eq(calendars.isShared, true)), + or( ...existing date-window predicates... ) +) +``` + +Mirrors the authoritative `/writable-calendars` ownership idiom (D-03). + +## Deviations from Plan + +### Auto-fixed Issues + +**1. [Rule 1 - Bug] Fixed upsertUser returning stale null displayName for existing rows** +- **Found during:** Task 2 +- **Issue:** The plan said "do not alter upsertUser signature or user.ts" but the scope amendment required that existing blank rows be corrected. `upsertUser` returned existing rows without updating displayName, making the me.ts fix useless for already-created users. +- **Fix:** Modified user.ts to issue an UPDATE when existing row has `displayName = null` and caller provides a non-null value. Signature unchanged. +- **Files modified:** `apps/api/src/auth/user.ts` +- **Commit:** 23c8bb3 + +**2. [Rule 3 - Blocking] Node_modules symlink for worktree test execution** +- **Found during:** Task 1 (TDD verification) +- **Issue:** Worktree has no `node_modules` — vitest could not find dependencies. Main repo tests run against main repo source files, not worktree files. +- **Fix:** Created symlink `apps/api/node_modules -> /home/luc/Projects/familysync/apps/api/node_modules`. Symlink is not tracked by git (node_modules is gitignored) — this is a runtime-only convenience for test execution in the worktree context. +- **Files modified:** none (symlink only) + +## Operator Actions Required + +1. **Authelia claim emission (for full-name legend):** If you want the calendar legend to show members' full names rather than email addresses, configure Authelia to emit the `name` and/or `preferred_username` OIDC claims. The code now reads these claims preferentially but cannot populate what Authelia does not emit. Check your Authelia OIDC client config (`userinfo_signing_algorithm`, claim mappings, `scope`). + +2. **Browser re-test after rebuild:** Rebuild and test in a real browser: + - Delete an event → dialog should close, event disappears (BUG 1 fix) + - Edit an event → should complete without 503 (BUG 1 fix) + - Calendar legend should show a name (BUG 2 — depends on Authelia claims) + - Second member should not see first member's private events (BUG 3) + +## Known Stubs + +None — all three fixes are fully wired. + +## Threat Flags + +None — all changes are within existing security perimeter. The GET / ownership predicate tightens the security boundary (previously too permissive). No new endpoints or auth paths introduced. + +## Self-Check + +- [x] apps/api/src/routes/events.ts — modified (PATCH+DELETE join, GET ownership, resolveUserId) +- [x] apps/api/src/routes/me.ts — modified (displayName claim derivation) +- [x] apps/api/src/auth/user.ts — modified (update displayName on re-upsert when null) +- [x] apps/api/tests/routes/events.test.ts — modified (mocks + regression tests) +- [x] Commit 2870413 — fix(260607-l6l): add missing innerJoin to PATCH+DELETE event lookups +- [x] Commit 23c8bb3 — fix(260607-l6l): derive displayName from OIDC claims in me.ts + resolveUserId +- [x] Commit 00a0454 — fix(260607-l6l): scope GET /api/events to current user + shared calendars +- [x] Full test suite: 100/100 tests passing across 13 test files +- [x] Typecheck: clean (tsc --noEmit, no errors) + +## Self-Check: PASSED + +## Orchestrator Review Follow-ups (post-executor) + +The orchestrator verified all three fixes and made two follow-up commits: + +**1. `509f4b2` — test: make BUG 1 join regression test couple to the handler.** +The executor's original `toSQL()` regression test was **tautological**: it +hand-built the joined query *inside the test body* and asserted the SQL +contained a join — it never exercised the handler, so removing `.innerJoin` +from `events.ts` left it green. Replaced with two tests that issue real +PATCH/DELETE requests against the mocked select-chain (`from → innerJoin → +where`) and assert the handler returns 202 (not 503) **and** invokes the +`innerJoin` spy. Verified empirically: removing the edit+delete joins from +`events.ts` turns both tests RED (and also flips the existing 202 success-path +tests to 503); GREEN with the joins present. The real regression guard now +lives in handler-coupled assertions, not a self-fulfilling SQL string match. + +**2. `a99ef1d` — refactor: extract shared `deriveDisplayName` helper (BUG 2 DRY).** +The claim-preference logic (name → preferred_username → email → sub fallback) +was duplicated verbatim in `me.ts` and `events.ts resolveUserId`. The scope +amendment had asked for a single shared helper; the executor duplicated it +instead. Extracted to `auth/user.ts` as `deriveDisplayName(claims, sub)` and +used in both call sites. Updated the `events.test.ts` `user.js` mock to spread +`importActual` (keeping the real pure helper) while still stubbing `upsertUser`. + +Final state: build clean (tsc), 100/100 tests pass, joins present in all three +event lookups, ownership predicate correctly grouped, displayName helper shared. diff --git a/.planning/todos/pending/event-creation-reminder-options.md b/.planning/todos/pending/event-creation-reminder-options.md new file mode 100644 index 0000000..1e29cb3 --- /dev/null +++ b/.planning/todos/pending/event-creation-reminder-options.md @@ -0,0 +1,34 @@ +--- +title: Add notification/reminder (VALARM) options to event creation +date: 2026-06-07 +priority: medium +phase_hint: Phase 5 (push) or earlier in Phase 03 write-path polish +--- + +# Add notification/reminder options to event creation + +The event-create/edit form (`apps/pwa` EventForm → `POST /api/events`, `PATCH /:uid/edit`) +has **no UI for setting a reminder/alarm on an event**. A user creating an event cannot +choose "remind me 10 min / 1 hour / 1 day before". The written `.ics` therefore contains +no `VALARM` component, so neither Fastmail's native clients nor any downstream notification +path can fire an event reminder. + +This is distinct from (but feeds) the Active requirement +"Web Push notifications for event reminders and list changes" (Phase 5): even with web-push +infrastructure, there is nothing to notify *about* unless events carry reminder data. + +## Scope to decide when promoted + +- Add a reminder selector to EventForm (none / at time / 10m / 30m / 1h / 1d before; possibly + multiple). +- Serialize chosen offsets as `VALARM` (TRIGGER) blocks in the iCalendar payload written + back to Fastmail via the outbox. +- On read, parse existing `VALARM`s so edits preserve/show the current reminder. +- Decide division of labour vs Phase 5 web-push: VALARM gives native-client reminders + (Fastmail/Apple Calendar) for free; app-delivered web-push reminders are the separate + Phase 5 piece that would read these offsets. + +## Open question + +- iCalendar `VALARM` round-trips through `ical.js`; confirm tsdav PUT preserves it and that + Fastmail honours `DISPLAY`/`AUDIO` alarms set by a third-party CalDAV client.