110 lines
9.1 KiB
Markdown
110 lines
9.1 KiB
Markdown
---
|
|
phase: 03-event-write-back-pwa-install
|
|
reviewed: 2026-06-09T00:00:00Z
|
|
depth: standard
|
|
files_reviewed: 29
|
|
files_reviewed_list:
|
|
- apps/api/src/broker/outboxWorker.ts
|
|
- apps/api/src/broker/sync.ts
|
|
- apps/api/src/broker/vevent.ts
|
|
- apps/api/src/broker/write.ts
|
|
- apps/api/src/db/schema.ts
|
|
- apps/api/src/index.ts
|
|
- apps/api/src/routes/events.ts
|
|
- apps/api/tests/broker/outboxWorker.test.ts
|
|
- apps/api/tests/broker/vevent.test.ts
|
|
- apps/api/tests/broker/write.test.ts
|
|
- apps/api/tests/routes/events.test.ts
|
|
- apps/pwa/index.html
|
|
- apps/pwa/package.json
|
|
- apps/pwa/src/api/client.test.ts
|
|
- apps/pwa/src/api/client.ts
|
|
- apps/pwa/src/components/CalendarShell.tsx
|
|
- apps/pwa/src/components/DeleteConfirmationDialog.test.tsx
|
|
- apps/pwa/src/components/DeleteConfirmationDialog.tsx
|
|
- apps/pwa/src/components/EventDetailPopover.test.tsx
|
|
- apps/pwa/src/components/EventDetailPopover.tsx
|
|
- apps/pwa/src/components/EventForm.test.tsx
|
|
- apps/pwa/src/components/EventForm.tsx
|
|
- apps/pwa/src/components/InstallPrompt.test.tsx
|
|
- apps/pwa/src/components/InstallPrompt.tsx
|
|
- apps/pwa/src/components/SyncStateToast.test.tsx
|
|
- apps/pwa/src/components/SyncStateToast.tsx
|
|
- apps/pwa/src/store/calendarStore.ts
|
|
- apps/pwa/vite.config.ts
|
|
- apps/pwa/vitest.config.ts
|
|
findings:
|
|
critical: 0
|
|
warning: 2
|
|
info: 2
|
|
total: 4
|
|
status: issues_found
|
|
---
|
|
|
|
# Phase 3: Code Review Report (Re-Review, Iteration 3 — final --auto pass)
|
|
|
|
**Reviewed:** 2026-06-09
|
|
**Depth:** standard
|
|
**Files Reviewed:** 29
|
|
**Status:** issues_found (no blockers — remaining items are accepted v1 limitations)
|
|
|
|
## Summary
|
|
|
|
Final re-review of the event write-back + PWA-install phase after the iteration-2 fix pass. I traced each iteration-2 fix end-to-end against its implementation and tests. All iteration-2 fixes are correct and introduce no regressions. The prior BLOCKER (CR-01: edit-as-move strips the RRULE) is now **resolved and correct**.
|
|
|
|
### Iteration-2 fixes — verified
|
|
|
|
- **Move-path RRULE forwarding (CR-01) — VERIFIED FIXED.** `events.ts` now selects `rawVevent` in the edit lookup (events.ts:338) and, in the move branch, extracts the source RRULE and stashes it as `_preservedRrule` on the create payload **only when the edit carried no explicit recurrence** (events.ts:388-395). The worker create branch reads it back: `hasExplicitRecurrence` is computed via `hasOwnProperty(fields,'recurrence')` (outboxWorker.ts:336), and `rruleString` resolves to `preservedRrule ?? rruleFromPayload` only when there is no explicit recurrence (outboxWorker.ts:341-353). The two sides agree: an EDIT omits `recurrence`, so `hasExplicitRecurrence=false` and the stashed RRULE is applied; an explicit `recurrence` (including `'none'`) still wins. `JSON.stringify` on the move payload drops the absent `recurrence` key, so `hasOwnProperty` is correctly `false` after the round-trip. Covered by events.test.ts:422-469 (route stashes RRULE) and outboxWorker.test.ts:311-358 (worker re-applies; explicit `'none'` still emits no RRULE). No regression to the same-calendar `update` preserve path (outboxWorker.ts:281-285).
|
|
|
|
- **Outbox payload re-validation (IN-03) — VERIFIED FIXED.** Both the `update` and `create` branches parse the stored JSON, then `outboxPayloadSchema.safeParse` it (outboxWorker.ts:231-235, 323-327). A schema-invalid row is hard-failed (no retry, no CalDAV dispatch). The schema mirrors `eventFieldsSchema` and uses `.passthrough()` so `_preservedRrule` survives validation (outboxWorker.ts:70-82). Covered by outboxWorker.test.ts:288-306 (missing title → hard-fail, never dispatched).
|
|
|
|
- **Sync-status failed-row ranking (WR-04) — VERIFIED FIXED.** `sync-status` orders by a status-priority CASE (`failed`/`dead`=0, `pending`=1, else=2) then `createdAt DESC` (events.ts:549-552), so an earlier failed/dead row for a uid outranks a later `done` row. Covered by events.test.ts:556-589, which also asserts the CASE expression is present in the ORDER BY chunks.
|
|
|
|
- **Helper-text / all-day toggle clamp (WR-01/WR-02 UI) — VERIFIED FIXED.** The recurrence `<select>` is disabled in edit mode with explanatory helper text (EventForm.tsx:789-800), and `handleAllDayToggle` clamps `endDate` to `max(startDate,endDate)` on toggle-on and clears stale time errors (EventForm.tsx:296-305).
|
|
|
|
- **All-day inclusive/exclusive DTEND symmetry (CR-03) — STILL CORRECT.** `vevent.ts:116-123` rolls the inclusive end forward one UTC day on write; `EventForm.tsx:86-96` rolls it back on pre-fill. Symmetric; covered by vevent.test.ts:140-160.
|
|
|
|
The two findings below are **carried-forward, deliberately-accepted v1 limitations** (documented in code), not regressions; they are recorded for completeness. There are no blockers in this phase.
|
|
|
|
## Warnings
|
|
|
|
### WR-01: Missing cached etag still produces an unconditional PUT/DELETE (D-08 gap)
|
|
|
|
**File:** `apps/api/src/broker/write.ts:74-78, 103-107`; `apps/api/src/routes/events.ts:406, 432, 507`
|
|
|
|
**Issue:** When `eventRow.etag` is null (event cached before an etag was captured, or Fastmail omitted it), the outbox row's `etag` is `undefined`, and `write.ts` maps null/`''` to "no If-Match header" — an **unconditional** PUT/DELETE. That defeats D-08 conflict detection for exactly the rows most likely to be stale: a concurrent external edit is silently overwritten with no 412. The iteration-1 fix added a `console.warn` so the path is observable (write.ts:75-77, 104-106), but the unconditional write itself is unchanged — observability is not prevention. Only triggers when the cached etag is missing, so Warning, not Blocker.
|
|
|
|
**Fix:** When no etag is available, fetch the current etag (REPORT/GET) before writing, or skip the write and force a targeted re-sync so the next attempt carries a real etag. At minimum, document that the no-etag path is an accepted unconditional-write window for v1.
|
|
|
|
### WR-02: Edit cannot change or remove an RRULE; "no recurrence field" is overloaded as "keep existing"
|
|
|
|
**File:** `apps/pwa/src/components/EventForm.tsx:361-371, 768-800`; `apps/api/src/broker/outboxWorker.ts:281-285, 336-353`
|
|
|
|
**Issue:** The recurrence `<select>` is hard-disabled on edit and the payload always omits `recurrence` on edit (EventForm.tsx:367). The server treats an absent `recurrence` as "preserve the stored RRULE" (both the same-calendar update and the move path). The consequence is that a user can never (a) change a recurring event's frequency, nor (b) intentionally make a recurring event non-recurring — there is no way to express "remove the RRULE" through the edit form, because "omit recurrence" is reserved to mean "unchanged." Helper text now surfaces the constraint (EventForm.tsx:789-800), which is the iteration-2 mitigation, so this is a documented v1 scope cut rather than a silent trap. Recorded because the overloaded semantics will need disentangling when recurrence editing ships (a sentinel distinct from "omitted" will be required to express "remove").
|
|
|
|
**Fix:** When recurrence editing lands, introduce an explicit "remove recurrence" signal distinct from an omitted field (e.g. `recurrence: 'none'` already overrides — wire the edit form to send it when the user clears the schedule), and parse-and-modify the stored RECUR in place rather than replacing it with a bare preset (see IN-01).
|
|
|
|
## Info
|
|
|
|
### IN-01: `RRULE_PRESETS` round-trip is lossy for any parameterized RRULE
|
|
|
|
**File:** `apps/api/src/broker/vevent.ts:49-54`; `apps/api/src/broker/outboxWorker.ts:245-248, 337-340`
|
|
|
|
**Issue:** `RRULE_PRESETS` maps only to bare `FREQ=DAILY|WEEKLY|MONTHLY|YEARLY`. `extractRruleString` correctly preserves the full stored RECUR (which may carry `BYDAY`/`INTERVAL`/`COUNT`/`UNTIL`), and both preserve paths keep that rich rule. But if a `recurrence` preset value is ever applied to a previously-rich rule, it collapses the rule to the bare preset — silently dropping qualifiers. This cannot happen in v1 (the picker offers only the four bare presets and is disabled on edit), so it is latent, not active. The limitation is now documented at the `RRULE_PRESETS` definition (vevent.ts:39-48).
|
|
|
|
**Fix:** When recurrence editing ships, parse the existing RECUR and modify it in place instead of replacing it with a preset.
|
|
|
|
### IN-02: Move-path RRULE preservation depends silently on `rawVevent` being non-empty
|
|
|
|
**File:** `apps/api/src/routes/events.ts:388-391`
|
|
|
|
**Issue:** In the move branch, `preservedRrule = payload.recurrence === undefined ? extractRruleString(eventRow.rawVevent ?? '') : undefined`. If `eventRow.rawVevent` is ever null/empty (it is selected at events.ts:338 and `calendar_events.rawVevent` is `notNull` per schema.ts:106, so this is not currently reachable), `extractRruleString('')` returns `undefined` and the move silently drops the RRULE with no diagnostic. The schema NOT NULL constraint makes this safe today; the fragility is that the preserve path has no observability if that invariant ever changes (unlike write.ts:75-77 which logs the analogous no-etag gap).
|
|
|
|
**Fix:** Optional — log a warning when a move with no explicit recurrence finds no extractable RRULE on a recurring-looking source, so a future schema/contract change that empties `rawVevent` is diagnosable rather than silent.
|
|
|
|
---
|
|
|
|
_Reviewed: 2026-06-09_
|
|
_Reviewer: Claude (gsd-code-reviewer)_
|
|
_Depth: standard_
|