From 69231043e4327ca13cf04624b1d8202dda84d466 Mon Sep 17 00:00:00 2001 From: Lucas Berger Date: Tue, 9 Jun 2026 21:24:48 -0400 Subject: [PATCH] =?UTF-8?q?feat(05-05):=20implement=20listChangeDispatcher?= =?UTF-8?q?=20=E2=80=94=20access-scoped,=20self-suppressed,=20coalesced=20?= =?UTF-8?q?push=20(NOTIF-02)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - notifyListChange(listId, actorId, windowMs?) wraps coalesceListPush with a dispatch closure that resolves actor name + list name from DB, builds audience as owner ∪ list_shares MINUS actorId (D-03), and calls dispatchPush per accessible subscriber subscription - D-02 generic copy: '{Actor} made {N} changes to {ListName}' — no item text - D-03 self-suppression: actorId filtered from audience before subscription load - T-05-14: audience strictly scoped to list access (owner + list_shares only) - T-05-15: no item text in notification body - Empty audience and missing subscriptions are silent no-ops - Tests: 5/5 GREEN (burst→1 push, self-suppress, access scope, empty audience) --- apps/api/src/lib/listChangeDispatcher.ts | 126 ++++++++++++++++++ .../tests/lib/listChangeDispatcher.test.ts | 115 ++++++++-------- 2 files changed, 189 insertions(+), 52 deletions(-) create mode 100644 apps/api/src/lib/listChangeDispatcher.ts diff --git a/apps/api/src/lib/listChangeDispatcher.ts b/apps/api/src/lib/listChangeDispatcher.ts new file mode 100644 index 0000000..ce091a9 --- /dev/null +++ b/apps/api/src/lib/listChangeDispatcher.ts @@ -0,0 +1,126 @@ +/** + * listChangeDispatcher — access-scoped, self-suppressed, coalesced list-change + * push notifications (NOTIF-02, D-01/D-02/D-03). + * + * Plugs into the same publish points as the SSE fan-out (publishListEvent). + * Called after each meaningful list/item mutation; excluded for reorder (position) + * changes (D-01). + * + * Threat mitigations: + * T-05-14: audience = list owner ∪ list_shares only — never all users. + * T-05-15: D-02 generic copy — no item text in notification body. + * T-05-16: D-03 excludeUserId = actorId — actor never receives their own push. + * + * Design notes: + * - notifyListChange is fire-and-forget (never awaited at call site). + * - dispatchPush is called per subscription; one failed send never aborts the loop. + * - coalesceListPush handles burst collapsing; this module owns audience + copy. + */ + +import { and, eq, inArray } from 'drizzle-orm' +import { db } from '../db/client.js' +import { users, lists, listShares, pushSubscriptions } from '../db/schema.js' +import { coalesceListPush } from './pushCoalescer.js' +import { dispatchPush } from './pushDispatcher.js' + +/** + * Notify all accessible, non-actor subscribers that a list changed. + * + * Coalesces bursts per (list, actor) — calls within the window are batched + * into a single push carrying the change count (D-01). + * + * @param listId - The list that changed. + * @param actorId - The user who made the change. Their own subscriptions are + * never dispatched (D-03 self-suppression). + * @param windowMs - Coalesce window in ms (default 45 s). Override in tests + * for fast fake-timer or real-timer test execution. + */ +export function notifyListChange(listId: number, actorId: number, windowMs?: number): void { + coalesceListPush(listId, actorId, async (coalListId, coalActorId, count) => { + try { + await sendListChangePush(coalListId, coalActorId, count) + } catch (err: unknown) { + console.error( + `[listChangeDispatcher] unhandled error for list ${coalListId}:`, + err instanceof Error ? err.message : String(err), + ) + } + }, windowMs) +} + +/** + * Inner dispatch: resolves actor name + list name, builds the audience, + * and sends a push to every accessible non-actor subscriber. + */ +async function sendListChangePush( + listId: number, + actorId: number, + count: number, +): Promise { + // Resolve actor display name and list name in parallel + const [actorRow, listRow] = await Promise.all([ + db.select({ displayName: users.displayName }).from(users).where(eq(users.id, actorId)).limit(1), + db.select({ name: lists.name }).from(lists).where(eq(lists.id, listId)).limit(1), + ]) + + if (!listRow[0]) { + // List deleted between mutation and coalesce fire — no-op + return + } + + const actorName: string = actorRow[0]?.displayName ?? 'Someone' + const listName: string = listRow[0].name + + // Build audience: list owner ∪ list_shares members, MINUS the actor (D-03) + const [ownerRows, shareRows] = await Promise.all([ + db.select({ ownerId: lists.ownerId }).from(lists).where(eq(lists.id, listId)).limit(1), + db.select({ userId: listShares.userId }).from(listShares).where(eq(listShares.listId, listId)), + ]) + + if (!ownerRows[0]) { + // List gone — no-op + return + } + + const ownerId = ownerRows[0].ownerId + const shareUserIds = shareRows.map((r) => r.userId) + + // Union of owner + sharees; deduplicate; exclude actor (D-03) + const audienceIds = [ + ...new Set([ownerId, ...shareUserIds]), + ].filter((uid) => uid !== actorId) + + if (audienceIds.length === 0) { + return + } + + // Load push subscriptions for all audience members + const subs = await db + .select() + .from(pushSubscriptions) + .where(inArray(pushSubscriptions.userId, audienceIds)) + + if (subs.length === 0) { + return + } + + // D-02 generic copy: "{Actor} made {N} changes to {ListName}" + // No item text — keeps the lock screen clean. + const body = `${actorName} made ${count} ${count === 1 ? 'change' : 'changes'} to ${listName}` + const notification = { + title: listName, + body, + tag: `list-change:${listId}`, + navigate: `/lists/${listId}`, + } + + // Fan out to each subscription; one failure must not abort the rest (T-05-04) + for (const sub of subs) { + await dispatchPush(sub, notification).catch((err: unknown) => { + console.error( + `[listChangeDispatcher] dispatchPush failed for sub ${sub.id}:`, + err instanceof Error ? err.message : String(err), + ) + }) + } +} diff --git a/apps/api/tests/lib/listChangeDispatcher.test.ts b/apps/api/tests/lib/listChangeDispatcher.test.ts index 45db0c8..9206c39 100644 --- a/apps/api/tests/lib/listChangeDispatcher.test.ts +++ b/apps/api/tests/lib/listChangeDispatcher.test.ts @@ -1,5 +1,5 @@ /** - * RED scaffold — listChangeDispatcher (Plan 05-05 turns this GREEN). + * Integration tests — listChangeDispatcher (Plan 05-05). * * Asserts that notifyListChange: * - Calls dispatchPush exactly ONCE per subscriber who is NOT the actor, @@ -10,27 +10,20 @@ * no crash. * * Uses a real dev MariaDB (same pattern as lists.test.ts) for DB seeding. - * Mocks dispatchPush via vi.mock to assert recipients without network. - * Uses vi.useFakeTimers to drive the coalescer window synchronously. + * Mocks dispatchPush via vi.doMock to assert recipients without network. + * Uses a tiny windowMs (10 ms) so the coalescer fires quickly with real timers. * * Run: - * set -a; . ./.env 2>/dev/null; set +a + * set -a; . .env 2>/dev/null; set +a * export DB_HOST=127.0.0.1 * pnpm --filter @familysync/api exec vitest run tests/lib/listChangeDispatcher.test.ts */ -import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' +import { describe, it, expect, vi, beforeEach } from 'vitest' import { randomUUID } from 'node:crypto' import { db } from '../../src/db/client.js' import { users, lists, listShares, pushSubscriptions } from '../../src/db/schema.js' -// --------------------------------------------------------------------------- -// Mock dispatchPush so tests assert recipients without making real VAPID calls. -// --------------------------------------------------------------------------- -vi.mock('../../src/lib/pushDispatcher.js', () => ({ - dispatchPush: vi.fn().mockResolvedValue(undefined), -})) - // --------------------------------------------------------------------------- // Seed helpers // --------------------------------------------------------------------------- @@ -64,28 +57,56 @@ async function seedSubscription(userId: number, suffix = ''): Promise { return result.id } -// --------------------------------------------------------------------------- -// Import helpers (lazy, after mocks registered) -// --------------------------------------------------------------------------- +/** Short wait for real-timer coalescer window + async DB queries to settle. */ +function sleep(ms: number): Promise { + return new Promise((resolve) => setTimeout(resolve, ms)) +} -async function getDispatchPushMock() { - const { dispatchPush } = await import('../../src/lib/pushDispatcher.js') - return vi.mocked(dispatchPush) +/** + * Poll a predicate until it passes or the timeout is exceeded. + * Replaces @testing-library/waitFor — keeps the test deps minimal. + */ +async function pollUntil( + predicate: () => void, + timeoutMs = 3000, + intervalMs = 30, +): Promise { + const deadline = Date.now() + timeoutMs + let lastErr: unknown + while (Date.now() < deadline) { + try { + predicate() + return // predicate passed + } catch (err) { + lastErr = err + } + await sleep(intervalMs) + } + throw lastErr } // --------------------------------------------------------------------------- // Tests +// Use a tiny windowMs (10 ms) so the coalescer fires quickly in real time. +// Use vi.doMock + vi.resetModules before each test so the mock is fresh. // --------------------------------------------------------------------------- +const WINDOW_MS = 10 + describe('notifyListChange — access-scoped, self-suppressed, coalesced (NOTIF-02)', () => { beforeEach(() => { - vi.useFakeTimers() vi.resetModules() + // Re-register the dispatchPush mock after each resetModules so fresh + // dynamic imports of listChangeDispatcher.js get the mocked pushDispatcher. + vi.doMock('../../src/lib/pushDispatcher.js', () => ({ + dispatchPush: vi.fn().mockResolvedValue(undefined), + })) }) - afterEach(() => { - vi.useRealTimers() - }) + async function getDispatchPushMock() { + const { dispatchPush } = await import('../../src/lib/pushDispatcher.js') + return vi.mocked(dispatchPush) + } it('burst of N calls coalesces into exactly one dispatchPush to the non-actor subscriber', async () => { const actorId = await seedUser('actor-burst', 'Alice') @@ -98,20 +119,15 @@ describe('notifyListChange — access-scoped, self-suppressed, coalesced (NOTIF- const otherSubId = await seedSubscription(otherId) const mockDispatch = await getDispatchPushMock() - mockDispatch.mockClear() - const { notifyListChange } = await import('../../src/lib/listChangeDispatcher.js') // Three rapid calls within the coalesce window - notifyListChange(listId, actorId) - notifyListChange(listId, actorId) - notifyListChange(listId, actorId) + notifyListChange(listId, actorId, WINDOW_MS) + notifyListChange(listId, actorId, WINDOW_MS) + notifyListChange(listId, actorId, WINDOW_MS) - // Advance past the coalesce window - await vi.runAllTimersAsync() - - // Exactly one call to dispatchPush (other's subscription only) - expect(mockDispatch).toHaveBeenCalledTimes(1) + // Wait for the coalescer window to expire + DB queries to settle + await pollUntil(() => expect(mockDispatch).toHaveBeenCalledTimes(1)) // The called subscription must be the other user's subscription const [calledSub, notification] = mockDispatch.mock.calls[0] @@ -121,25 +137,25 @@ describe('notifyListChange — access-scoped, self-suppressed, coalesced (NOTIF- // Notification body must contain actor name and change count expect(notification.body).toMatch(/Alice/) expect(notification.body).toMatch(/3/) - // Notification body must NOT contain item text (D-02 generic only) + // Notification must have a title (D-02 — no item text, just generic copy) expect(notification.title).toBeDefined() + expect(typeof notification.title).toBe('string') }) it('actor is never dispatched to their own subscription (D-03 self-suppression)', async () => { const actorId = await seedUser('actor-self-suppress', 'Charlie') const listId = await seedList(actorId, 'Private List') - // Actor subscribes, but there are no other accessible members → audience is empty + // Actor subscribes, but there are no other accessible members → audience is empty after D-03 await seedSubscription(actorId) const mockDispatch = await getDispatchPushMock() - mockDispatch.mockClear() - const { notifyListChange } = await import('../../src/lib/listChangeDispatcher.js') - notifyListChange(listId, actorId) - await vi.runAllTimersAsync() + notifyListChange(listId, actorId, WINDOW_MS) + + // Wait past the window; actor's subscription must never be dispatched + await sleep(WINDOW_MS + 200) - // Actor is the only accessible member; after excluding actorId, audience is empty expect(mockDispatch).not.toHaveBeenCalled() }) @@ -157,15 +173,14 @@ describe('notifyListChange — access-scoped, self-suppressed, coalesced (NOTIF- await seedSubscription(unrelatedId) const mockDispatch = await getDispatchPushMock() - mockDispatch.mockClear() - const { notifyListChange } = await import('../../src/lib/listChangeDispatcher.js') - notifyListChange(listId, actorId) - await vi.runAllTimersAsync() + notifyListChange(listId, actorId, WINDOW_MS) + + // Wait for coalescer + DB queries to settle + await pollUntil(() => expect(mockDispatch).toHaveBeenCalledTimes(1)) // Only member (Eve) should be dispatched; unrelated (Frank) must NOT receive push - expect(mockDispatch).toHaveBeenCalledTimes(1) const [calledSub] = mockDispatch.mock.calls[0] expect(calledSub.userId).toBe(memberId) expect(calledSub.userId).not.toBe(unrelatedId) @@ -178,12 +193,10 @@ describe('notifyListChange — access-scoped, self-suppressed, coalesced (NOTIF- await seedSubscription(actorId) const mockDispatch = await getDispatchPushMock() - mockDispatch.mockClear() - const { notifyListChange } = await import('../../src/lib/listChangeDispatcher.js') - notifyListChange(listId, actorId) - await vi.runAllTimersAsync() + notifyListChange(listId, actorId, WINDOW_MS) + await sleep(WINDOW_MS + 200) expect(mockDispatch).not.toHaveBeenCalled() }) @@ -200,12 +213,10 @@ describe('notifyListChange — access-scoped, self-suppressed, coalesced (NOTIF- // Deliberately no subscription for otherId const mockDispatch = await getDispatchPushMock() - mockDispatch.mockClear() - const { notifyListChange } = await import('../../src/lib/listChangeDispatcher.js') - notifyListChange(listId, actorId) - await vi.runAllTimersAsync() + notifyListChange(listId, actorId, WINDOW_MS) + await sleep(WINDOW_MS + 200) // Other has no subscription → no push dispatched expect(mockDispatch).not.toHaveBeenCalled()