docs(05-05): complete list-change push plan
This commit is contained in:
Lucas Berger
@@ -0,0 +1,174 @@
|
||||
---
|
||||
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
|
||||
Reference in New Issue
Block a user