Milestone v1.0: FamilySync MVP #1
@@ -1,8 +1,8 @@
|
||||
---
|
||||
phase: 05-web-push-notifications
|
||||
reviewed: 2026-06-09T00:00:00Z
|
||||
reviewed: 2026-06-09T12:00:00Z
|
||||
depth: standard
|
||||
files_reviewed: 20
|
||||
files_reviewed: 21
|
||||
files_reviewed_list:
|
||||
- apps/api/src/lib/pushDispatcher.ts
|
||||
- apps/api/src/lib/pushCoalescer.ts
|
||||
@@ -17,6 +17,7 @@ files_reviewed_list:
|
||||
- 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
|
||||
@@ -29,317 +30,179 @@ files_reviewed_list:
|
||||
- apps/pwa/vite.config.ts
|
||||
- docker-compose.yml
|
||||
findings:
|
||||
critical: 4
|
||||
warning: 5
|
||||
info: 3
|
||||
total: 12
|
||||
status: issues_found
|
||||
critical: 0
|
||||
warning: 0
|
||||
info: 0
|
||||
total: 0
|
||||
status: clean
|
||||
---
|
||||
|
||||
# Phase 5: Code Review Report
|
||||
# Phase 5: Code Review Report (Re-review)
|
||||
|
||||
**Reviewed:** 2026-06-09
|
||||
**Depth:** standard
|
||||
**Files Reviewed:** 20+
|
||||
**Status:** issues_found
|
||||
**Files Reviewed:** 21 (includes new 0004 migration)
|
||||
**Status:** clean (after iteration-3 fixes)
|
||||
|
||||
## Summary
|
||||
## Resolution (iteration 3)
|
||||
|
||||
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.
|
||||
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`.
|
||||
|
||||
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.
|
||||
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
|
||||
|
||||
### CR-01: `sentReminders` Set Grows Without Bound (Memory Leak + Correctness)
|
||||
### NEW-CR-01: iOS Gesture Gate Still Broken in `PushPermissionPrompt` — `await navigator.serviceWorker.ready` Before `subscribe()`
|
||||
|
||||
**File:** `apps/api/src/broker/reminderScheduler.ts:38`
|
||||
**File:** `apps/pwa/src/components/PushPermissionPrompt.tsx:128-131`
|
||||
|
||||
**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`:
|
||||
**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
|
||||
// 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)
|
||||
}
|
||||
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)
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 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)`:
|
||||
**Fix:** Pre-fetch `navigator.serviceWorker.ready` into state alongside `vapidKey`, using a
|
||||
parallel `useEffect`. Then the tap handler has synchronous access to both:
|
||||
|
||||
```typescript
|
||||
self.addEventListener('notificationclick', (event: NotificationEvent) => {
|
||||
event.notification.close()
|
||||
const url: string =
|
||||
typeof event.notification.data?.url === 'string'
|
||||
? event.notification.data.url
|
||||
: '/'
|
||||
// In PushPermissionPrompt (and SettingsSheet equivalently):
|
||||
const [swRegistration, setSwRegistration] = useState<ServiceWorkerRegistration | null>(null)
|
||||
|
||||
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(() => {})
|
||||
}, [])
|
||||
if (!installed || permission !== 'default' || dismissed) return
|
||||
void navigator.serviceWorker?.ready.then(setSwRegistration).catch(() => {})
|
||||
}, [installed, permission, dismissed])
|
||||
|
||||
// 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),
|
||||
// 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
|
||||
})
|
||||
// ... 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}>
|
||||
```
|
||||
This satisfies the iOS requirement: `subscribe()` is called synchronously in the onClick handler,
|
||||
with `pushManager.subscribe()` as the first async operation inside `subscribe()`.
|
||||
|
||||
---
|
||||
|
||||
## Warnings
|
||||
|
||||
### WR-01: `sentReminders` Dedup Key Survives Process Restart — Same-Minute Double-Fire Is Possible
|
||||
### NEW-WR-01: `sync.ts` Delete-Change Pre-Capture Misses the All-Calendars-Empty Case
|
||||
|
||||
**File:** `apps/api/src/broker/reminderScheduler.ts:133-134`
|
||||
**File:** `apps/api/src/broker/sync.ts:248-258`
|
||||
|
||||
**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):
|
||||
**Issue:** The `pendingDeleteRows` pre-capture query is guarded by `if (onChanges && seenUids.length > 0)`:
|
||||
|
||||
```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)
|
||||
let pendingDeleteRows: Array<{ uid: string; title: string | null }> = []
|
||||
if (onChanges && seenUids.length > 0) {
|
||||
pendingDeleteRows = await db
|
||||
.select(...)
|
||||
.where(and(eq(...calendarId...), notInArray(...uid...seenUids)))
|
||||
}
|
||||
```
|
||||
|
||||
Apply the same fix to the DELETE handler's catch at line 136.
|
||||
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.
|
||||
|
||||
### WR-04: `sync.ts` Emits `delete` Change Events Before the DB DELETE — Race Condition in Fan-Out
|
||||
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.
|
||||
|
||||
**File:** `apps/api/src/broker/sync.ts:242-259`
|
||||
**Fix:** Add the pre-capture for the empty-seenUids branch:
|
||||
|
||||
**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
|
||||
let pendingDeleteRows: Array<{ uid: string; title: string | null }> = []
|
||||
if (onChanges) {
|
||||
if (seenUids.length > 0) {
|
||||
await db.delete(calendarEvents)
|
||||
pendingDeleteRows = await db
|
||||
.select({ uid: calendarEvents.uid, title: calendarEvents.title })
|
||||
.from(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' })
|
||||
// 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))
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### 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_
|
||||
|
||||
Reference in New Issue
Block a user