fix(05-review): IN-01 resolve actor display name in eventChangeDispatcher for D-02/D-03

apps/api/src/lib/eventChangeDispatcher.ts

@@ -15,9 +15,9 @@
  * Fire-and-forget: sync correctness does not depend on push success.
  */
 
-import { ne } from 'drizzle-orm'
+import { eq, ne } from 'drizzle-orm'
 import { db } from '../db/client.js'
-import { pushSubscriptions } from '../db/schema.js'
+import { users, pushSubscriptions } from '../db/schema.js'
 import { dispatchPush } from './pushDispatcher.js'
 
 // ── Types ────────────────────────────────────────────────────────────────────
@@ -71,32 +71,32 @@ export function isMeaningfulChange(change: EventChange): boolean {
 }
 
 /**
- * Builds the notification title for a change.
+ * Builds the notification copy for a change (D-02/D-03).
- * Actor name is omitted here (we don't have it from userId alone in the fast path);
- * the copy follows the D-02 spec using generic "A member" as fallback. For the
- * two-person household the non-actor will always be informed by the other member's
- * action, so the body context is sufficient.
  *
- * Note: actorName resolution (querying users table) is a future D-02 enhancement.
+ * @param change    - The detected calendar event change.
- * For MVP, use generic copy that still satisfies the acceptance criteria.
+ * @param actorName - Display name of the actor (D-03: named in every notification).
+ *                    Falls back to 'A family member' when the users row is absent.
  */
 function buildCopy(
   change: EventChange,
+  actorName: string,
 ): { notifTitle: string; notifBody: string; navigate: string } {
   const eventTitle = change.title ?? change.uid
 
   let notifTitle: string
   let notifBody: string
 
+  // D-02: event notifications show specifics — actor + title.
+  // D-03: name the actor in every change notification.
   if (change.operation === 'create') {
-    notifTitle = 'New calendar event'
+    notifTitle = `${actorName} added an event`
     notifBody = eventTitle
   } else if (change.operation === 'delete') {
-    notifTitle = 'Calendar event removed'
+    notifTitle = `${actorName} removed an event`
     notifBody = eventTitle
   } else {
     // update
-    notifTitle = 'Calendar event updated'
+    notifTitle = `${actorName} updated an event`
     notifBody = eventTitle
   }
 
@@ -128,13 +128,24 @@ export async function dispatchEventChange(
     return
   }
 
+  // IN-01: resolve actor display name for D-02/D-03 notification copy.
+  // Runs in parallel with subscription query for minimal latency.
+  const [actorRows, allSubs] = await Promise.all([
+    db
+      .select({ displayName: users.displayName })
+      .from(users)
+      .where(eq(users.id, actorUserId))
+      .limit(1),
     // D-13: query push_subscriptions from MariaDB only
     // D-03: ne() filter excludes the actor at DB level; application-level filter
     // below provides defence-in-depth (also makes the mock-based tests deterministic).
-  const allSubs = await db
+    db
       .select()
       .from(pushSubscriptions)
-    .where(ne(pushSubscriptions.userId, actorUserId))
+      .where(ne(pushSubscriptions.userId, actorUserId)),
+  ])
+
+  const actorName: string = actorRows[0]?.displayName ?? 'A family member'
 
   // D-03: additional application-level actor exclusion (defence-in-depth)
   const subs = allSubs.filter((s) => s.userId !== actorUserId)
@@ -143,7 +154,7 @@ export async function dispatchEventChange(
     return
   }
 
-  const { notifTitle, notifBody, navigate } = buildCopy(change)
+  const { notifTitle, notifBody, navigate } = buildCopy(change, actorName)
 
   // Fan out to all non-actor subscriptions (D-03 already enforced by ne() filter)
   for (const sub of subs) {

apps/api/tests/lib/eventChangeDispatcher.test.ts

@@ -1,13 +1,14 @@
 /**
- * RED scaffold — eventChangeDispatcher (Plan 05-05 turns this GREEN).
+ * eventChangeDispatcher tests (D-04, D-02/D-03 actor naming — IN-01 fix).
  *
  * Asserts that dispatchEventChange:
  *   - Fires for new events (operation='create')
  *   - Fires for updated events with meaningful changes: time/date/title/location (D-04)
  *   - Does NOT fire for description-only edits (D-04)
  *   - Excludes the actor's own push subscriptions (D-03)
+ *   - Names the actor in the notification title (IN-01 / D-02/D-03)
+ *   - Falls back to 'A family member' when the actor row is missing (IN-01)
  *
- * These tests fail now because eventChangeDispatcher.ts does not yet exist.
  * Run: pnpm --filter @familysync/api exec vitest run tests/lib/eventChangeDispatcher.test.ts
  */
 
@@ -16,11 +17,7 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'
 // Mock DB
 vi.mock('../../src/db/client.js', () => ({
   db: {
-    select: vi.fn().mockReturnValue({
+    select: vi.fn(),
-      from: vi.fn().mockReturnValue({
-        where: vi.fn().mockResolvedValue([]),
-      }),
-    }),
   },
 }))
 
@@ -29,6 +26,37 @@ vi.mock('../../src/lib/pushDispatcher.js', () => ({
   dispatchPush: vi.fn().mockResolvedValue(undefined),
 }))
 
+// ---------------------------------------------------------------------------
+// Helper: build a mock db.select chain.
+// dispatchEventChange now calls Promise.all([actorQuery, subsQuery]).
+// The first select is for the actor name (users table, returns [{displayName}]),
+// the second is for push subscriptions (returns sub rows).
+// ---------------------------------------------------------------------------
+function mockDbSelect(
+  db: { select: ReturnType<typeof vi.fn> },
+  actorDisplayName: string | null,
+  subRows: Array<{ id: number; userId: number; endpoint: string; p256dh: string; auth: string }>,
+) {
+  // Two sequential .select() calls via Promise.all:
+  // First call  → actor name query  (select.from.where.limit → [{ displayName }])
+  // Second call → subscriptions query (select.from.where → subRows)
+  db.select
+    .mockReturnValueOnce({
+      from: vi.fn().mockReturnValue({
+        where: vi.fn().mockReturnValue({
+          limit: vi.fn().mockResolvedValue(
+            actorDisplayName !== null ? [{ displayName: actorDisplayName }] : [],
+          ),
+        }),
+      }),
+    })
+    .mockReturnValueOnce({
+      from: vi.fn().mockReturnValue({
+        where: vi.fn().mockResolvedValue(subRows),
+      }),
+    })
+}
+
 describe('eventChangeDispatcher — trigger conditions (D-04)', () => {
   beforeEach(() => {
     vi.clearAllMocks()
@@ -41,11 +69,7 @@ describe('eventChangeDispatcher — trigger conditions (D-04)', () => {
     const { dispatchEventChange } = await import('../../src/lib/eventChangeDispatcher.js')
 
     const otherUserSub = { id: 2, userId: 2, endpoint: 'https://push.example.com/sub/2', p256dh: 'x', auth: 'y' }
-    vi.mocked(db.select).mockReturnValue({
+    mockDbSelect(vi.mocked(db), 'Lucas', [otherUserSub])
-      from: vi.fn().mockReturnValue({
-        where: vi.fn().mockResolvedValue([otherUserSub]),
-      }),
-    } as never)
 
     await dispatchEventChange(
       { uid: 'new-event-uid', title: 'Doctor appointment', operation: 'create' },
@@ -61,11 +85,7 @@ describe('eventChangeDispatcher — trigger conditions (D-04)', () => {
     const { dispatchEventChange } = await import('../../src/lib/eventChangeDispatcher.js')
 
     const otherUserSub = { id: 2, userId: 2, endpoint: 'https://push.example.com/sub/2', p256dh: 'x', auth: 'y' }
-    vi.mocked(db.select).mockReturnValue({
+    mockDbSelect(vi.mocked(db), 'Lucas', [otherUserSub])
-      from: vi.fn().mockReturnValue({
-        where: vi.fn().mockResolvedValue([otherUserSub]),
-      }),
-    } as never)
 
     await dispatchEventChange(
       {
@@ -85,13 +105,11 @@ describe('eventChangeDispatcher — trigger conditions (D-04)', () => {
     const { dispatchPush } = await import('../../src/lib/pushDispatcher.js')
     const { dispatchEventChange } = await import('../../src/lib/eventChangeDispatcher.js')
 
-    vi.mocked(db.select).mockReturnValue({
+    // isMeaningfulChange returns false before DB is queried — no select needed.
-      from: vi.fn().mockReturnValue({
+    // But set up a mock just in case (won't be called).
-        where: vi.fn().mockResolvedValue([
+    mockDbSelect(vi.mocked(db), 'Lucas', [
       { id: 2, userId: 2, endpoint: 'https://push.example.com/sub/2', p256dh: 'x', auth: 'y' },
-        ]),
+    ])
-      }),
-    } as never)
 
     await dispatchEventChange(
       {
@@ -111,14 +129,12 @@ describe('eventChangeDispatcher — trigger conditions (D-04)', () => {
     const { dispatchPush } = await import('../../src/lib/pushDispatcher.js')
     const { dispatchEventChange } = await import('../../src/lib/eventChangeDispatcher.js')
 
-    // DB returns only the actor's own subscription (userId 1)
+    // DB returns only the actor's own subscription (userId 1).
-    vi.mocked(db.select).mockReturnValue({
+    // The ne() filter in the SQL should exclude it, but here we simulate that
-      from: vi.fn().mockReturnValue({
+    // the application-level filter also catches it.
-        where: vi.fn().mockResolvedValue([
+    mockDbSelect(vi.mocked(db), 'Lucas', [
       { id: 1, userId: 1, endpoint: 'https://push.example.com/sub/1', p256dh: 'x', auth: 'y' },
-        ]),
+    ])
-      }),
-    } as never)
 
     await dispatchEventChange(
       { uid: 'event-uid-3', title: 'Soccer practice', operation: 'create' },
@@ -128,4 +144,40 @@ describe('eventChangeDispatcher — trigger conditions (D-04)', () => {
     // No subscriptions remain after excluding the actor — nothing dispatched
     expect(vi.mocked(dispatchPush)).not.toHaveBeenCalled()
   })
+
+  it('names the actor in the notification title (IN-01 / D-02/D-03)', async () => {
+    const { db } = await import('../../src/db/client.js')
+    const { dispatchPush } = await import('../../src/lib/pushDispatcher.js')
+    const { dispatchEventChange } = await import('../../src/lib/eventChangeDispatcher.js')
+
+    const otherUserSub = { id: 2, userId: 2, endpoint: 'https://push.example.com/sub/2', p256dh: 'x', auth: 'y' }
+    mockDbSelect(vi.mocked(db), 'Lucas', [otherUserSub])
+
+    await dispatchEventChange(
+      { uid: 'event-uid-4', title: 'Dentist', operation: 'create' },
+      /* actorUserId */ 1,
+    )
+
+    expect(vi.mocked(dispatchPush)).toHaveBeenCalled()
+    const calledWith = vi.mocked(dispatchPush).mock.calls[0][1]
+    expect(calledWith.title).toContain('Lucas')
+  })
+
+  it('falls back to "A family member" when actor row is missing (IN-01)', async () => {
+    const { db } = await import('../../src/db/client.js')
+    const { dispatchPush } = await import('../../src/lib/pushDispatcher.js')
+    const { dispatchEventChange } = await import('../../src/lib/eventChangeDispatcher.js')
+
+    const otherUserSub = { id: 2, userId: 2, endpoint: 'https://push.example.com/sub/2', p256dh: 'x', auth: 'y' }
+    mockDbSelect(vi.mocked(db), null, [otherUserSub]) // actor row absent → null
+
+    await dispatchEventChange(
+      { uid: 'event-uid-5', title: 'Soccer', operation: 'create' },
+      /* actorUserId */ 99, // non-existent user
+    )
+
+    expect(vi.mocked(dispatchPush)).toHaveBeenCalled()
+    const calledWith = vi.mocked(dispatchPush).mock.calls[0][1]
+    expect(calledWith.title).toContain('A family member')
+  })
 })