---
phase: 05-web-push-notifications
reviewed: 2026-06-09T12:00:00Z
depth: standard
files_reviewed: 21
files_reviewed_list:
  - 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
findings:
  critical: 0
  warning: 0
  info: 0
  total: 0
status: 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:

```typescript
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`:

```typescript
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:

```typescript
// 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)`:

```typescript
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:

```typescript
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_