Files

T

Lucas Berger 7c687ea413 docs(03): add code review fix report (--auto, 2 fix passes)

Auto-fix loop converged after 2 fix iterations + a final verifying re-review:
- Pass 1: 13/14 findings fixed (3 Critical, 6 Warning, 4 Info).
- Re-review surfaced 1 new Critical (move-path RRULE data loss) + 4 lower.
- Pass 2: 8/8 fixed, including the move-path RRULE forwarding.
- Final re-review: 0 Critical. Remaining 2 Warning / 2 Info are documented
  v1 scope cuts (recurrence-editing deferred), not defects.

Test suites green throughout: api 108, pwa 145; both tsc --noEmit clean.
Per-iteration REVIEW/REVIEW-FIX snapshots retained as audit trail.

2026-06-09 11:12:09 -04:00

9.1 KiB

Raw Blame History

phase, reviewed, depth, files_reviewed, files_reviewed_list, findings, status

phase

reviewed

depth

files_reviewed

files_reviewed_list

findings

status

03-event-write-back-pwa-install

2026-06-09T00:00:00Z

standard

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

critical	warning	info	total
0	2	2	4

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

9.1 KiB Raw Blame History