175 lines
8.5 KiB
Markdown
175 lines
8.5 KiB
Markdown
---
|
||
phase: 05-web-push-notifications
|
||
plan: 05
|
||
subsystem: api/list-change-dispatcher
|
||
tags: [web-push, notif-02, list-change, coalescer, tdd, red-green, D-01, D-02, D-03]
|
||
dependency_graph:
|
||
requires: [05-02, 05-03, 05-04]
|
||
provides: [notifyListChange — access-scoped, self-suppressed, coalesced list-change push]
|
||
affects:
|
||
- apps/api/src/lib/listChangeDispatcher.ts
|
||
- apps/api/src/routes/lists.ts
|
||
- apps/api/tests/lib/listChangeDispatcher.test.ts
|
||
- apps/api/tests/routes/lists.test.ts
|
||
tech_stack:
|
||
added: []
|
||
patterns:
|
||
- real-timer + pollUntil polling for async DB assertions (avoids fake-timer + real-I/O mismatch)
|
||
- vi.doMock + vi.resetModules per-test pattern (fresh mock instances for each test)
|
||
- windowMs optional param for testability (coalescer window override in tests)
|
||
key_files:
|
||
created:
|
||
- apps/api/src/lib/listChangeDispatcher.ts
|
||
- apps/api/tests/lib/listChangeDispatcher.test.ts
|
||
modified:
|
||
- apps/api/src/routes/lists.ts
|
||
- apps/api/tests/routes/lists.test.ts
|
||
decisions:
|
||
- "windowMs exposed as optional 3rd arg on notifyListChange for test-time override (avoids fake-timer/real-I/O race)"
|
||
- "pollUntil() helper (inline, no test-library deps) replaces @testing-library/waitFor for async DB assertion polling"
|
||
- "vi.doMock + vi.resetModules in beforeEach — required so each test gets a fresh vi.fn() mock instance for dispatchPush"
|
||
- "DELETE /:id notifyListChange fires after DB delete — sendListChangePush handles missing list gracefully (early return)"
|
||
- "List create (POST /) does NOT notify — empty list is not a change worth pinging (D-01 spirit)"
|
||
metrics:
|
||
duration: 8
|
||
completed_date: "2026-06-10"
|
||
tasks_completed: 2
|
||
files_changed: 4
|
||
---
|
||
|
||
# Phase 05 Plan 05: listChangeDispatcher — NOTIF-02 List-Change Push — Summary
|
||
|
||
TDD RED→GREEN: `listChangeDispatcher.ts` (notifyListChange) implemented; hooked into all meaningful list/item mutation points in `routes/lists.ts`; reorder (position) changes excluded; 64 tests GREEN.
|
||
|
||
## Tasks Executed
|
||
|
||
### Task 1: listChangeDispatcher — access-scoped, self-suppressed fan-out
|
||
|
||
**Status:** Completed.
|
||
|
||
**Commits:**
|
||
- RED: `test(05-05): add failing tests for listChangeDispatcher — RED gate` — 97f7026
|
||
- GREEN: `feat(05-05): implement listChangeDispatcher — access-scoped, self-suppressed, coalesced push (NOTIF-02)` — 6923104
|
||
|
||
Created `apps/api/src/lib/listChangeDispatcher.ts` exporting `notifyListChange(listId, actorId, windowMs?)`:
|
||
|
||
**`notifyListChange`** — wraps `coalesceListPush` with a dispatch closure that:
|
||
1. Resolves actor `displayName` and list `name` from DB in parallel
|
||
2. Builds audience: `{list owner} ∪ {list_shares.userId} MINUS actorId` (D-03)
|
||
3. Loads `push_subscriptions` for all audience members
|
||
4. Calls `dispatchPush(sub, notification)` per subscription — one failure never aborts the loop
|
||
5. D-02 generic copy: `"{Actor} made {N} changes to {ListName}"` — no item text
|
||
|
||
**Threat mitigations:**
|
||
- T-05-14: audience derived from list access (owner + list_shares only) — never all users
|
||
- T-05-15: notification body carries actor name + count, no item text (D-02)
|
||
- T-05-16: actorId filtered before audience union → actor's own subscriptions never dispatched (D-03)
|
||
|
||
**Tests (5/5 GREEN):**
|
||
- Burst coalescing: 3 rapid calls → 1 `dispatchPush` to non-actor with `count=3`, body contains actor name + "3"
|
||
- D-03 self-suppression: actor-only list → 0 dispatches
|
||
- T-05-14 access scoping: unrelated 3rd user (no owner/share) → never dispatched
|
||
- Empty audience (no other members) → no dispatch, no crash
|
||
- Empty audience (other has no subscription) → no dispatch, no crash
|
||
|
||
### Task 2: Hook notifyListChange into list/item mutations (reorder excluded)
|
||
|
||
**Status:** Completed.
|
||
|
||
**Commit:** `feat(05-05): hook notifyListChange into list/item mutations (reorder excluded)` — d2ce4e0
|
||
|
||
`apps/api/src/routes/lists.ts` updated — `notifyListChange` called (fire-and-forget) after each meaningful mutation:
|
||
|
||
| Route | Mutation | Push? |
|
||
|-------|----------|-------|
|
||
| `POST /api/lists/:id/items` | Item added | YES |
|
||
| `PATCH /api/list-items/:itemId` | checked/text change | YES |
|
||
| `PATCH /api/list-items/:itemId` | position change (reorder) | **NO** (D-01) |
|
||
| `DELETE /api/list-items/:itemId` | Item deleted | YES |
|
||
| `PATCH /api/lists/:id` | List rename/sharing toggle | YES |
|
||
| `DELETE /api/lists/:id` | List deleted | YES |
|
||
| `POST /api/lists` | List created | **NO** (empty list, D-01 spirit) |
|
||
|
||
Critical D-01 guard in `PATCH /list-items/:itemId`:
|
||
```typescript
|
||
if (patch.position === undefined) {
|
||
notifyListChange(item.listId, currentUserId)
|
||
}
|
||
```
|
||
|
||
**New tests in lists.test.ts (2 tests):**
|
||
- `PATCH { position }` (reorder) does NOT call `notifyListChange` — spy confirms 0 calls
|
||
- `PATCH { checked: true }` DOES call `notifyListChange(listId, ownerId)` — spy confirms 1 call with correct args
|
||
|
||
**Final test count:** 59/59 lists.test.ts + 5/5 listChangeDispatcher.test.ts = **64/64 GREEN**
|
||
|
||
## Verification
|
||
|
||
```
|
||
pnpm --filter @familysync/api exec vitest run tests/lib/listChangeDispatcher.test.ts tests/routes/lists.test.ts
|
||
|
||
Test Files 2 passed (2)
|
||
Tests 64 passed (64)
|
||
```
|
||
|
||
`pnpm --filter @familysync/api typecheck` — passes (no errors).
|
||
|
||
## Deviations from Plan
|
||
|
||
### Auto-fixed Issues
|
||
|
||
**1. [Rule 1 - Bug] Fake timer + real DB I/O race condition in listChangeDispatcher tests**
|
||
|
||
- **Found during:** Task 1 (GREEN phase, first test run)
|
||
- **Issue:** `vi.useFakeTimers()` + `vi.runAllTimersAsync()` fires the coalescer timer but returns before the subsequent real DB queries (`sendListChangePush`) complete. This caused the "burst coalesces" and "access scoping" tests to fail (0 `dispatchPush` calls observed even though the logic was correct).
|
||
- **Fix:**
|
||
1. Switched test approach to real timers (no `vi.useFakeTimers`) with a tiny `windowMs=10ms` passed to `notifyListChange`.
|
||
2. Added optional `windowMs` parameter to `notifyListChange` (defaults to `undefined`, which passes through to `coalesceListPush`'s 45s default) — test-only override.
|
||
3. Added inline `pollUntil()` helper (no `@testing-library/waitFor` dependency) that polls a predicate until it passes or a 3s timeout.
|
||
- **Files modified:** `apps/api/src/lib/listChangeDispatcher.ts`, `apps/api/tests/lib/listChangeDispatcher.test.ts`
|
||
- **Commit:** 6923104
|
||
|
||
**2. [Rule 1 - Bug] `vi.mock()` top-level hoisted mock lost after `vi.resetModules()`**
|
||
|
||
- **Found during:** Task 1 (first test run attempt with top-level `vi.mock`)
|
||
- **Issue:** Top-level `vi.mock('../../src/lib/pushDispatcher.js', ...)` is hoisted before each test file execution, but `vi.resetModules()` in `beforeEach` clears the module registry. When tests dynamically imported `listChangeDispatcher.js`, the fresh load of `pushDispatcher.js` bypassed the mock factory.
|
||
- **Fix:** Removed top-level `vi.mock`; used `vi.doMock` inside `beforeEach` (after `vi.resetModules`) so each test's dynamic import of `listChangeDispatcher.js` gets a fresh mocked `pushDispatcher.js`.
|
||
- **Files modified:** `apps/api/tests/lib/listChangeDispatcher.test.ts`
|
||
- **Commit:** 6923104
|
||
|
||
## Known Stubs
|
||
|
||
None. `notifyListChange` is fully wired end-to-end. Push dispatch will fail with a logged error if VAPID keys are malformed (pre-existing infra issue from Plan 05-04, not a stub).
|
||
|
||
## Threat Flags
|
||
|
||
No new threat surface beyond what the plan's threat model covers. All three threats mitigated:
|
||
|
||
| Threat | Status |
|
||
|--------|--------|
|
||
| T-05-14: Info disclosure — push to non-member | Mitigated — audience = owner ∪ list_shares only |
|
||
| T-05-15: Info disclosure — item text in payload | Mitigated — D-02 generic copy only |
|
||
| T-05-16: Spoofing — actor notified of own change | Mitigated — D-03 excludeUserId = actorId |
|
||
|
||
## Self-Check
|
||
|
||
**Files created/verified:**
|
||
- [x] apps/api/src/lib/listChangeDispatcher.ts — exists (min_lines: 25 ✓, ~110 lines)
|
||
- [x] apps/api/tests/lib/listChangeDispatcher.test.ts — exists
|
||
|
||
**Key links verified:**
|
||
- [x] apps/api/src/routes/lists.ts imports and calls `notifyListChange` at 5 mutation sites
|
||
- [x] apps/api/src/lib/listChangeDispatcher.ts calls `coalesceListPush` from `pushCoalescer.ts`
|
||
|
||
**Commits verified:**
|
||
- 97f7026: test(05-05): add failing tests for listChangeDispatcher — RED gate
|
||
- 6923104: feat(05-05): implement listChangeDispatcher — VAPID send + access-scoped, self-suppressed, coalesced push (NOTIF-02)
|
||
- d2ce4e0: feat(05-05): hook notifyListChange into list/item mutations (reorder excluded)
|
||
|
||
## TDD Gate Compliance
|
||
|
||
- RED: `test(05-05): add failing tests for listChangeDispatcher — RED gate` — 97f7026
|
||
- GREEN: `feat(05-05): implement listChangeDispatcher...` — 6923104
|
||
|
||
## Self-Check: PASSED
|