luckberg/familysync
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

docs(05): add code review report

Browse Source
This commit is contained in:
Lucas Berger
2026-06-09 22:33:09 -04:00
parent 44fbb2bb3a
commit dc8516beb8
1 changed files with 345 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.planning/phases/05-web-push-notifications/05-REVIEW.md
+345
View File
@@ -0,0 +1,345 @@
---
phase: 05-web-push-notifications
reviewed: 2026-06-09T00:00:00Z
depth: standard
files_reviewed: 20
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/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: 4
warning: 5
info: 3
total: 12
status: issues_found
---
# Phase 5: Code Review Report
**Reviewed:** 2026-06-09
**Depth:** standard
**Files Reviewed:** 20+
**Status:** issues_found
## Summary
Phase 5 delivers Web Push across three trigger paths (NOTIF-01/02/03) plus the iOS reliability infrastructure. The architecture is sound: VAPID signing is delegated to `web-push`, the D-03 actor-suppression filter is applied both at the DB layer and in application code, `event.waitUntil()` is used correctly in the SW, and the D-13 boundary (no tsdav in notification code) is respected.
Four blockers were found. Two are correctness bugs: (1) the `sentReminders` set in `reminderScheduler.ts` grows without bound and is never pruned — it will leak memory and eventually begin skipping due reminders after a process restart populates a new set without stale guard entries; (2) the `uniq_push_endpoint` unique constraint is defined on a `text` column in the migration SQL, which MariaDB/InnoDB silently truncates to 767 bytes when creating the index — a subscription with a very long endpoint can collide with or fail to match a different subscription, causing phantom prune-without-delete or missed 410 cleanup. Two are security: (3) `VAPID_PRIVATE_KEY` is passed via `docker-compose.yml` environment block from the host `.env` but with no fallback (`${VAPID_PRIVATE_KEY}` with no `:-` default) — Docker Compose will error at startup if the variable is unset rather than leaving push non-functional, which is correct, but the VAPID private key has no runtime check that it is well-formed before `setVapidDetails` is called, and invalid keys are only warned, not blocked; and (4) the `notificationclick` handler in `sw.ts` uses `client.url === url` for exact-URL matching, which means a focused-window match will never fire for notifications with query-string deep-links whose current window URL does not exactly match (e.g. `/calendar?date=2026-06-09&event=uid` vs `/calendar`) — this is a functional regression, not a security issue, but is reclassified as a correctness blocker because the D-14 deep-link requirement is unmet for the most common case.
---
## Critical Issues
### CR-01: `sentReminders` Set Grows Without Bound (Memory Leak + Correctness)
**File:** `apps/api/src/broker/reminderScheduler.ts:38`
**Issue:** `sentReminders` is a module-level `Set<string>` that accumulates one entry per `${uid}:${minuteBucket}` every minute for every shared timed event. Entries are never removed. For an app running continuously with N shared events, the set grows by N entries/minute indefinitely. After a long uptime the set consumes significant memory. More critically: the deduplication key includes `minuteBucket = floor(ms / 60000)`, so after a process restart the set is empty and an event that fired in the previous minute (same wall-clock minute bucket) will fire again — the dedup logic is only effective within a single process lifetime. This is documented as "acceptable data loss on restart" for the double-fire case, but the unbounded growth is not documented and constitutes a bug.
**Fix:** Prune entries for past minute buckets after each scan. After the dispatch loop completes, remove any set entry whose minuteBucket is strictly less than `currentBucket - 1`:
```typescript
// After the dispatch loop in runReminderCheck():
const staleBucket = minuteBucket - 1
for (const key of sentReminders) {
const [, bucketStr] = key.split(':')
if (Number(bucketStr) < staleBucket) {
sentReminders.delete(key)
}
}
```
---
### CR-02: `uniq_push_endpoint` Unique Constraint on `text` Column Will Silently Truncate in MariaDB/InnoDB
**File:** `apps/api/src/db/migrations/0003_same_xavin.sql:10` and `apps/api/src/db/schema.ts:243-244`
**Issue:** `endpoint` is declared `text NOT NULL` in both the schema and migration, and `CONSTRAINT uniq_push_endpoint UNIQUE(endpoint)` is applied to it. MariaDB/InnoDB cannot index an unbounded `text` column directly — it requires a prefix length. The behavior differs by server version and `innodb_large_prefix` setting: some versions silently create a prefix index (767 bytes by default, 3072 bytes with `ROW_FORMAT=DYNAMIC`), others fail. In either case a silent prefix match means two endpoints that share the same first 767 characters — which is possible for push endpoints from the same push service domain — are treated as duplicates. This makes the upsert in `push.ts`'s `onDuplicateKeyUpdate` unreliable: a second distinct subscription may silently collide, or the migration may fail entirely on strict-mode MariaDB.
The correct fix is to define `endpoint` as `varchar(2048)` (matching the Zod validation bound of `z.string().url().max(2048)` in `routes/push.ts:61`) in both the schema and migration.
**Fix:**
```sql
-- Migration: change endpoint column type
ALTER TABLE `push_subscriptions`
MODIFY COLUMN `endpoint` varchar(2048) NOT NULL;
-- Schema: change text() to varchar
endpoint: varchar('endpoint', { length: 2048 }).notNull(),
```
The Zod schema in `push.ts` already enforces `max(2048)`, so server-validated inputs fit within the column. Update `p256dh` similarly — it is also `text` but a `varchar(512)` would match the Zod bound.
---
### CR-03: `notificationclick` Deep-Link Window Focus Uses Exact URL Match — D-14 Never Fires for Query-String Deep-Links
**File:** `apps/pwa/src/sw.ts:154`
**Issue:** The `notificationclick` handler matches `client.url === url` to find an existing window. Push deep-link URLs for event notifications are of the form `/calendar?event=<uid>` or `/calendar?date=<date>&event=<uid>`. When the user has the PWA open on `/calendar` (the root path, no query string), `client.url` will be `https://host/calendar` but `url` will be `https://host/calendar?event=uid` — the exact comparison fails, so the handler falls through to `openWindow` and opens a second PWA window instead of focusing the existing one. On iOS standalone mode, `openWindow` does NOT open a second window; it navigates the existing window, so the net behavior is correct on iOS. On Android Chrome it opens a second window. The deeper problem: the handler never navigates the focused window to the deep-link URL; it only calls `focus()` without `navigate()` — so even when the match succeeds (notification for the currently-open URL), the window is focused at the current route, not the notification target.
**Fix:** Use `clients.matchAll` to find any window on the same origin, focus it, and call `client.navigate(url)`:
```typescript
self.addEventListener('notificationclick', (event: NotificationEvent) => {
event.notification.close()
const url: string =
typeof event.notification.data?.url === 'string'
? event.notification.data.url
: '/'
event.waitUntil(
self.clients
.matchAll({ type: 'window', includeUncontrolled: true })
.then((clientList) => {
// Focus any existing window on this origin and navigate it
for (const client of clientList) {
if ('focus' in client) {
return (client as WindowClient).focus().then(() =>
(client as WindowClient).navigate(url)
)
}
}
if (self.clients.openWindow) {
return self.clients.openWindow(url)
}
}),
)
})
```
---
### CR-04: `subscribe()` in `usePushSubscription` Calls `fetchVapidKey()` (an async `await`) Before `pushManager.subscribe()` — iOS Gesture Gate May Be Broken on Cache Miss
**File:** `apps/pwa/src/hooks/usePushSubscription.ts:197-206`
**Issue:** The `subscribe()` function unconditionally `await`s `fetchVapidKey()` before calling `registration.pushManager.subscribe()`. The comment says "Fetch VAPID key (from cache if already prefetched)" and the VAPID key is pre-fetched in a `useEffect` by `PushPermissionPrompt`. However:
1. The `useEffect` pre-fetch is fire-and-forget (`void prefetchVapidKey()`). There is no guarantee it completes before the user taps "Enable Notifications" — especially on a slow connection on first install.
2. If `sessionStorage` is unavailable (private mode, quota exceeded) or the pre-fetch failed silently, `fetchVapidKey()` will make a network round-trip inside `subscribe()`.
3. iOS requires `pushManager.subscribe()` to be called synchronously within the user gesture's call stack. An `await fetch(...)` before `pushManager.subscribe()` breaks the gesture gate — the iOS browser will throw `NotAllowedError` with the message "The operation was not permitted." The existing code has this `await` on the very first line of `subscribe()`.
The `setEnabled(true)` path in the same hook has the same structure (`await fetchVapidKey()` before `pushManager.subscribe()`), but `setEnabled(true)` is only called when `Notification.permission === 'granted'` (OS already approved), so no OS dialog is shown — the gesture gate for permission is not relevant there. For the `subscribe()` path used from `PushPermissionPrompt` and `SettingsSheet`, the gate IS relevant when `Notification.permission === 'default'`.
**Fix:** Pre-fetch the VAPID key synchronously into a variable before the tap, and pass it as an argument to `subscribe()`. Remove the `fetchVapidKey()` call from inside `subscribe()`:
```typescript
// In PushPermissionPrompt: pre-fetch before the button renders
const [vapidKey, setVapidKey] = useState<string | null>(null)
useEffect(() => {
fetchVapidKey().then(setVapidKey).catch(() => {})
}, [])
// subscribe() signature change:
const subscribe = async (
registration: ServiceWorkerRegistration,
vapidKey: string,
): Promise<void> => {
// No await before this line — iOS gesture gate preserved
const sub = await registration.pushManager.subscribe({
userVisibleOnly: true,
applicationServerKey: urlBase64ToUint8Array(vapidKey),
})
// ... POST to server
}
```
If a pre-pass API approach is not desirable, the minimum safe fix is to ensure `fetchVapidKey()` always resolves from `sessionStorage` (never from the network) when called from within a tap handler, by making the pre-fetch mandatory and blocking the button render until the key is ready:
```typescript
// Disable the "Enable Notifications" button until vapidKey is loaded
<button disabled={!vapidKey} onClick={handleEnableClick}>
```
---
## Warnings
### WR-01: `sentReminders` Dedup Key Survives Process Restart — Same-Minute Double-Fire Is Possible
**File:** `apps/api/src/broker/reminderScheduler.ts:133-134`
**Issue:** The dedup comment documents "Acceptable data loss on process restart for a two-person household." This is a reasonable MVP trade-off, but the dedup fires AFTER marking the key in the set (`sentReminders.add(key)` before `dispatchPush`). If `dispatchPush` throws after the key is added (a non-410/404 failure), the event is marked as sent but no notification was delivered, and the set prevents retry in the same minute bucket. Combined with a process restart before the next bucket, no notification is ever delivered for that event.
**Fix:** Move `sentReminders.add(key)` to after the fan-out loop succeeds (or accept that at-least-once delivery requires not pre-marking):
```typescript
// Only mark sent after all dispatches have been attempted
for (const sub of event.subs) {
try { await dispatchPush(sub, notification) } catch { /* log */ }
}
sentReminders.add(key) // moved here
```
---
### WR-02: `listChangeDispatcher` Unused Import `and`
**File:** `apps/api/src/lib/listChangeDispatcher.ts:20`
**Issue:** `and` is imported from `drizzle-orm` but never used in the file. `inArray` and `eq` are used; `and` is dead.
**Fix:**
```typescript
import { eq, inArray } from 'drizzle-orm'
```
---
### WR-03: `push.ts` POST `/subscription` Catches and Logs the Full Error Object (Potential Key Leakage)
**File:** `apps/api/src/routes/push.ts:116-118`
**Issue:** The catch block logs `err` directly via `console.error('[push/POST /subscription] DB operation failed:', err)`. If the DB error includes the failing query parameters in its message (e.g. MariaDB's `ER_DUP_ENTRY` sometimes includes the duplicate value), the p256dh or auth values from the request body could appear in server logs. The threat model (T-05-03) explicitly says "never logs subscription keys."
**Fix:** Log only `err instanceof Error ? err.message : String(err)` — do not pass the raw error object:
```typescript
} catch (err) {
console.error(
'[push/POST /subscription] DB operation failed:',
err instanceof Error ? err.message : String(err),
)
return c.json({ error: 'Service unavailable' }, 503)
}
```
Apply the same fix to the DELETE handler's catch at line 136.
---
### WR-04: `sync.ts` Emits `delete` Change Events Before the DB DELETE — Race Condition in Fan-Out
**File:** `apps/api/src/broker/sync.ts:242-259`
**Issue:** Deleted UIDs are added to `changes` (lines 252-258) before the `db.delete()` call that removes them from the cache (lines 262-268). The `onChanges` callback is fired at line 272-274 after the delete, but within `onChanges` (in `poller.ts` and `outboxWorker.ts`) `dispatchEventChange` is called as fire-and-forget with `.catch()`. There is no await on the notification dispatch before `syncCalendar` returns. The sequence is:
1. `changes.push({ operation: 'delete', uid })` — collected
2. `db.delete(calendarEvents)` — DB row removed
3. `onChanges(changes)` — fires `dispatchEventChange` fire-and-forget
4. `syncCalendar` returns
If a rapid second poll cycle begins at step 4 (before the fire-and-forget dispatch at step 3 completes), the deleted uid may be re-fetched from Fastmail (if it was not actually deleted on the server — false delete detection from a transient REPORT response) and re-inserted, then `dispatchEventChange` fires with `operation: 'delete'` for an event that now exists again in the cache. This is an edge case in the two-person household but worth documenting: the delete change detection and DB delete should be atomic, or the `changes` collection for deletes should happen after the DB delete.
**Fix:** Collect delete changes AFTER the `db.delete()`:
```typescript
// 5. Delete pruned rows from DB first
if (seenUids.length > 0) {
await db.delete(calendarEvents)
.where(and(eq(calendarEvents.calendarId, cal.id), notInArray(calendarEvents.uid, seenUids)))
} else {
await db.delete(calendarEvents).where(eq(calendarEvents.calendarId, cal.id))
}
// THEN collect delete changes for onChanges
if (onChanges && cachedRows) {
for (const row of cachedRows) {
changes.push({ uid: row.uid, title: row.title ?? null, operation: 'delete' })
}
}
```
---
### WR-05: Health-Check Re-Subscribe in `usePushSubscription` Silently Re-Subscribes Without Checking Server-Side Record
**File:** `apps/pwa/src/hooks/usePushSubscription.ts:166-178`
**Issue:** The health-check `useEffect` (lines 152-183) detects a missing browser-side subscription and silently re-subscribes. The POST to `/api/push/subscription` will upsert (due to `onDuplicateKeyUpdate` on endpoint). However, if `pushManager.getSubscription()` returns a subscription object (line 157: `setIsSubscribed(true); return`) but the server-side record was deleted (e.g. pruned by a 410), the `isSubscribed` state shows `true` but notifications silently fail because the server has no record to dispatch to. The health-check does not verify the server-side record exists.
This is a silent failure mode: the user sees "notifications on" (isSubscribed=true), the app sends no notification, and the user never knows. Reminders and alerts are missed with no indication.
**Fix:** After `existingSub` is found in the browser, POST it to the server as a health confirmation (the upsert is idempotent):
```typescript
if (existingSub) {
// Re-confirm server-side record (handles 410-prune recovery)
await fetch('/api/push/subscription', {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
credentials: 'include',
body: JSON.stringify(existingSub.toJSON()),
}).catch(() => {}) // non-blocking
setIsSubscribed(true)
return
}
```
---
## Info
### IN-01: `eventChangeDispatcher.ts` Does Not Name the Actor in Notification Copy (D-02/D-03 Partial Implementation)
**File:** `apps/api/src/lib/eventChangeDispatcher.ts:83`
**Issue:** The `buildCopy` function uses generic copy (`'New calendar event'`, `'Calendar event updated'`) rather than naming the actor per D-02/D-03 spec ("Lucas added…", "Wife moved Dentist → Wed 3pm"). The comment at line 81-82 acknowledges this: "actorName resolution (querying users table) is a future D-02 enhancement." The acceptance criteria spec requires actor attribution. For the two-person household this means the notification body only shows the event title, not who made the change, which is less useful and technically incomplete against D-03's "name the actor" requirement.
**Fix:** Query `users.displayName` for `actorUserId` at the top of `dispatchEventChange` and pass it to `buildCopy`:
```typescript
const actorRow = await db.select({ displayName: users.displayName })
.from(users).where(eq(users.id, actorUserId)).limit(1)
const actorName = actorRow[0]?.displayName ?? 'A family member'
// Pass actorName into buildCopy
```
---
### IN-02: `docker-compose.yml` VAPID Variables Have No Fallback — Compose Startup Fails if Unset
**File:** `docker-compose.yml:29-31`
**Issue:** `VAPID_PUBLIC_KEY: ${VAPID_PUBLIC_KEY}`, `VAPID_PRIVATE_KEY: ${VAPID_PRIVATE_KEY}`, and `VAPID_SUBJECT: ${VAPID_SUBJECT}` have no `:-` fallbacks. If any of these are unset in the operator's `.env`, Docker Compose fails to start the API service entirely (`variable is not set` error) rather than starting with push disabled. The `index.ts` startup guard (lines 118-128) already handles missing VAPID vars gracefully with a `console.warn`, but that logic is never reached because Compose errors first.
This is an operator experience issue, not a security issue (keeping VAPID_PRIVATE_KEY out of source control is correct). Adding empty-string fallbacks would allow the container to start and push to fail gracefully:
```yaml
VAPID_PUBLIC_KEY: ${VAPID_PUBLIC_KEY:-}
VAPID_PRIVATE_KEY: ${VAPID_PRIVATE_KEY:-}
VAPID_SUBJECT: ${VAPID_SUBJECT:-}
```
---
### IN-03: `PushPermissionPrompt` Uses `Math.random()` for `headingId` — Stable Between Renders But Unstable Across Mounts
**File:** `apps/pwa/src/components/PushPermissionPrompt.tsx:63`
**Issue:** `const headingId = useRef(`push-prompt-heading-${Math.random().toString(36).slice(2)}`)` generates a random ID at component mount. This is stable within a mount lifecycle but changes each time the component unmounts and remounts (e.g. `App.tsx` renders `<PushPermissionPrompt />` at the root, and the component may remount on navigation). The aria-labelledby/id pairing is still valid within a mount, so there is no accessibility regression per se, but React's Strict Mode double-invocation in dev will produce two different IDs for what is logically the same element. Use `useId()` (React 18+) instead.
**Fix:**
```typescript
import { useId } from 'react'
// inside the component:
const headingId = useId()
```
---
_Reviewed: 2026-06-09_
_Reviewer: Claude (gsd-code-reviewer)_
_Depth: standard_