familysync/.planning/phases/05-web-push-notifications/05-REVIEW.md

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

phase	reviewed	depth	files_reviewed	files_reviewed_list	findings	status
05-web-push-notifications	2026-06-09T12:00:00Z	standard	21	apps/api/src/lib/pushDispatcher.ts apps/api/src/lib/pushCoalescer.ts apps/api/src/lib/listChangeDispatcher.ts apps/api/src/lib/eventChangeDispatcher.ts apps/api/src/broker/reminderScheduler.ts apps/api/src/broker/sync.ts apps/api/src/broker/poller.ts apps/api/src/broker/outboxWorker.ts apps/api/src/routes/push.ts apps/api/src/routes/lists.ts apps/api/src/index.ts apps/api/src/db/schema.ts apps/api/src/db/migrations/0003_same_xavin.sql apps/api/src/db/migrations/0004_mature_maximus.sql apps/pwa/src/sw.ts apps/pwa/src/hooks/usePushSubscription.ts apps/pwa/src/components/PushPermissionPrompt.tsx apps/pwa/src/components/SettingsSheet.tsx apps/pwa/src/components/PermissionDeniedBanner.tsx apps/pwa/src/App.tsx apps/pwa/src/components/AppNav.tsx apps/pwa/src/components/CalendarShell.tsx apps/pwa/src/components/InstallPrompt.tsx apps/pwa/vite.config.ts docker-compose.yml	critical warning info total 0 0 0 0	clean

Phase 5: Code Review Report (Re-review)

Reviewed: 2026-06-09 Depth: standard Files Reviewed: 21 (includes new 0004 migration) Status: clean (after iteration-3 fixes)

Resolution (iteration 3)

All 12 original findings plus the 2 fix-induced findings are resolved:

• NEW-CR-01 (iOS gesture gate re-broken by await navigator.serviceWorker.ready in the tap path) — FIXED in c7ef581. PushPermissionPrompt.tsx and SettingsSheet.tsx now pre-resolve both the SW registration and VAPID key into state via useEffect, disable the Enable control until both are ready, and call subscribe(registration, vapidKey) synchronously — zero await between the user gesture and pushManager.subscribe(). Manually verified.
• NEW-WR-01 (whole-cache-clear delete branch emitted no delete change events) — FIXED in 17756fc, with a new regression test in sync.test.ts.

Final state: api + pwa typecheck clean; PWA builds; API suite 214 tests (one occasional flaky real-DB timeout in lists.test.ts under full-suite parallel load — passes 59/59 in isolation; test-infra timing, not a code defect).

Summary (historical — pre-fix)

This is an --auto re-review after a fix pass claiming to resolve all 12 prior findings. Ten of the twelve are genuinely fixed. Two issues remain: one new critical introduced by the fix for CR-04, and one prior warning that is partially fixed but not fully resolved.

Prior findings status:

ID	Status	Notes
CR-01	CONFIRMED-FIXED	Stale-bucket prune loop added after dispatch at lines 176-182 of reminderScheduler.ts
CR-02	CONFIRMED-FIXED	0003 SQL is untouched; 0004_mature_maximus.sql adds the two MODIFY COLUMN statements; schema.ts uses varchar(2048)/varchar(512)
CR-03	CONFIRMED-FIXED	sw.ts now uses clients.matchAll + focus + navigate(url) + openWindow fallback inside event.waitUntil
CR-04	NOT-FIXED (new critical introduced)	See NEW-CR-01 below
WR-01	CONFIRMED-FIXED	sentReminders.add(key) is now after the fan-out loop (line 162)
WR-02	CONFIRMED-FIXED	and import removed; only eq, inArray remain
WR-03	CONFIRMED-FIXED	Both POST and DELETE catch blocks log err.message only
WR-04	CONFIRMED-FIXED	Delete changes now collected after db.delete() via pendingDeleteRows pattern
WR-05	CONFIRMED-FIXED	Health-check POSTs existingSub.toJSON() to re-confirm server record before setIsSubscribed(true)
IN-01	CONFIRMED-FIXED	dispatchEventChange runs Promise.all to fetch actorRows in parallel with subs query
IN-02	CONFIRMED-FIXED	VAPID vars have :- empty-string fallbacks in docker-compose.yml
IN-03	CONFIRMED-FIXED	useId() replaces Math.random() in PushPermissionPrompt

---

Critical Issues

NEW-CR-01: iOS Gesture Gate Still Broken in PushPermissionPrompt — await navigator.serviceWorker.ready Before subscribe()

File: apps/pwa/src/components/PushPermissionPrompt.tsx:128-131

Issue: The fix for CR-04 correctly moves fetchVapidKey out of subscribe() and pre-fetches the VAPID key into state (vapidKey). However, the tap handler (handleEnableClick) wraps the call in an immediately-invoked async IIFE:

void (async () => {
  try {
    const registration = await navigator.serviceWorker.ready   // ← AWAIT before subscribe()
    await subscribe(registration, resolvedVapidKey)             // ← pushManager.subscribe inside
  ...
})()

navigator.serviceWorker.ready is a Promise<ServiceWorkerRegistration>. On iOS, the gesture gate requires pushManager.subscribe() to be called synchronously within the user-gesture call stack. The await navigator.serviceWorker.ready that precedes subscribe() yields the microtask queue before pushManager.subscribe() is ever called — on a cache miss or slow SW activation this is an async network/IPC round-trip, which breaks the gesture gate and produces NotAllowedError on iOS exactly as the original await fetchVapidKey() did.

navigator.serviceWorker.ready resolves immediately only when the SW is already active and controlling the page. In that common steady-state case iOS may not enforce the synchrony requirement strictly. But on first install (SW just activated, ready may take >1 frame to resolve) or after a SW update cycle, the await is observable and iOS will reject with NotAllowedError.

The same pattern is present in SettingsSheet.tsx:115:

const registration = await navigator.serviceWorker?.ready   // ← same problem
if (registration) {
  await subscribe(registration, resolvedVapidKey)
}

Fix: Pre-fetch navigator.serviceWorker.ready into state alongside vapidKey, using a parallel useEffect. Then the tap handler has synchronous access to both:

// In PushPermissionPrompt (and SettingsSheet equivalently):
const [swRegistration, setSwRegistration] = useState<ServiceWorkerRegistration | null>(null)

useEffect(() => {
  if (!installed || permission !== 'default' || dismissed) return
  void navigator.serviceWorker?.ready.then(setSwRegistration).catch(() => {})
}, [installed, permission, dismissed])

// Disable button until BOTH are ready
<button disabled={loading || !vapidKey || !swRegistration} ...>

// Tap handler — no await before subscribe():
function handleEnableClick() {
  if (loading || !vapidKey || !swRegistration) return
  setLoading(true)
  void subscribe(swRegistration, vapidKey).then(() => {
    setLoading(false)
    onClose?.()
  }).catch((err) => {
    setLoading(false)
    // ... error handling
  })
}

This satisfies the iOS requirement: subscribe() is called synchronously in the onClick handler, with pushManager.subscribe() as the first async operation inside subscribe().

---

Warnings

NEW-WR-01: sync.ts Delete-Change Pre-Capture Misses the All-Calendars-Empty Case

File: apps/api/src/broker/sync.ts:248-258

Issue: The pendingDeleteRows pre-capture query is guarded by if (onChanges && seenUids.length > 0):

let pendingDeleteRows: Array<{ uid: string; title: string | null }> = []
if (onChanges && seenUids.length > 0) {
  pendingDeleteRows = await db
    .select(...)
    .where(and(eq(...calendarId...), notInArray(...uid...seenUids)))
}

However, the subsequent delete runs two branches:

1. seenUids.length > 0: deletes cache rows NOT in seenUids (the standard prune)
2. seenUids.length === 0: deletes ALL rows for the calendar (db.delete(...).where(eq(calendarId, cal.id)))

When seenUids.length === 0 (server returned zero events — entire calendar deleted or temporarily empty), branch 2 wipes all cached rows. But because the pre-capture is guarded by seenUids.length > 0, pendingDeleteRows remains empty and no delete changes are emitted to onChanges. Users whose push subscriptions would be notified of the deleted events receive no notification.

This is a partial regression of WR-04: the delete-before-collect ordering is fixed for the common case but the empty-server-response case is silently missed.

Fix: Add the pre-capture for the empty-seenUids branch:

let pendingDeleteRows: Array<{ uid: string; title: string | null }> = []
if (onChanges) {
  if (seenUids.length > 0) {
    pendingDeleteRows = await db
      .select({ uid: calendarEvents.uid, title: calendarEvents.title })
      .from(calendarEvents)
      .where(and(eq(calendarEvents.calendarId, cal.id), notInArray(calendarEvents.uid, seenUids)))
  } else {
    // Server returned zero events — all cached rows will be deleted
    pendingDeleteRows = await db
      .select({ uid: calendarEvents.uid, title: calendarEvents.title })
      .from(calendarEvents)
      .where(eq(calendarEvents.calendarId, cal.id))
  }
}

---

Reviewed: 2026-06-09 Reviewer: Claude (gsd-code-reviewer) Depth: standard