209 lines
8.9 KiB
Markdown
209 lines
8.9 KiB
Markdown
---
|
|
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_
|