From abe5bd8b39cfb5349c4b00e5c5fd354933fc4985 Mon Sep 17 00:00:00 2001 From: Robert Hamilton Date: Sun, 12 Apr 2026 18:11:02 -0500 Subject: [PATCH 1/5] refactor(display): collapse publisher + subscriber into display-realtime Merge display-publisher.ts (429 LOC) and display-subscriber.ts (232 LOC) into a single display-realtime.ts (231 LOC). Drops three failure state machines (silent-drop warning, terminal failure escalation, removeChannel failure counter), the PublisherMode enum, and the duplicated removeChannelSafe helper. Replace three test files that unit-tested Supabase channel internals with a single display-realtime.test.ts testing observable behavior: channel.send receives correct event and payload, handlers fire on incoming broadcasts, hello handshake round-trip. Net: -1,010 LOC source, -610 LOC tests, 2440 tests passing. --- .../active-workout-gym-label.test.tsx | 2 +- .../workout/active-workout-gym-label.tsx | 2 +- .../__tests__/use-display-broadcast.test.tsx | 2 +- src/hooks/use-display-broadcast.ts | 2 +- .../__tests__/display-publisher-hello.test.ts | 174 ------- src/lib/__tests__/display-publisher.test.ts | 456 ------------------ src/lib/__tests__/display-realtime.test.ts | 391 +++++++++++++++ src/lib/__tests__/display-subscriber.test.ts | 371 -------------- src/lib/display-publisher.ts | 429 ---------------- src/lib/display-realtime.ts | 231 +++++++++ src/lib/display-subscriber.ts | 232 --------- .../__tests__/-start-workout-flow.test.tsx | 2 +- src/routes/_authenticated/index.tsx | 2 +- src/routes/_authenticated/log.$workoutId.tsx | 2 +- .../display/__tests__/-gym-route.test.tsx | 2 +- src/routes/display/gym/$gymId.tsx | 2 +- src/stores/active-workout-store.ts | 2 +- 17 files changed, 632 insertions(+), 1672 deletions(-) delete mode 100644 src/lib/__tests__/display-publisher-hello.test.ts delete mode 100644 src/lib/__tests__/display-publisher.test.ts create mode 100644 src/lib/__tests__/display-realtime.test.ts delete mode 100644 src/lib/__tests__/display-subscriber.test.ts delete mode 100644 src/lib/display-publisher.ts create mode 100644 src/lib/display-realtime.ts delete mode 100644 src/lib/display-subscriber.ts diff --git a/src/components/workout/__tests__/active-workout-gym-label.test.tsx b/src/components/workout/__tests__/active-workout-gym-label.test.tsx index 1159a7f4..e2091f5c 100644 --- a/src/components/workout/__tests__/active-workout-gym-label.test.tsx +++ b/src/components/workout/__tests__/active-workout-gym-label.test.tsx @@ -18,7 +18,7 @@ vi.mock('@/hooks/use-gyms', () => ({ })) const mockGetActiveGymId = vi.fn() -vi.mock('@/lib/display-publisher', () => ({ +vi.mock('@/lib/display-realtime', () => ({ getActiveGymId: () => mockGetActiveGymId(), })) diff --git a/src/components/workout/active-workout-gym-label.tsx b/src/components/workout/active-workout-gym-label.tsx index 2f4cb391..6bb783d0 100644 --- a/src/components/workout/active-workout-gym-label.tsx +++ b/src/components/workout/active-workout-gym-label.tsx @@ -1,6 +1,6 @@ import { useEffect, type ReactElement } from 'react' import { useGyms } from '@/hooks/use-gyms' -import { getActiveGymId } from '@/lib/display-publisher' +import { getActiveGymId } from '@/lib/display-realtime' // --------------------------------------------------------------------------- // ActiveWorkoutGymLabel -- workout header gym label (F018, Tech.md D12) diff --git a/src/hooks/__tests__/use-display-broadcast.test.tsx b/src/hooks/__tests__/use-display-broadcast.test.tsx index 01bf2251..63e25082 100644 --- a/src/hooks/__tests__/use-display-broadcast.test.tsx +++ b/src/hooks/__tests__/use-display-broadcast.test.tsx @@ -53,7 +53,7 @@ vi.mock('@/lib/supabase', () => ({ getSupabaseClient: mockGetSupabaseClient, })) -vi.mock('@/lib/display-publisher', () => ({ +vi.mock('@/lib/display-realtime', () => ({ initDisplayPublisher: mockInitDisplayPublisher, configureDisplayPublisher: mockConfigureDisplayPublisher, isPublisherReady: mockIsPublisherReady, diff --git a/src/hooks/use-display-broadcast.ts b/src/hooks/use-display-broadcast.ts index 52c7529d..7bfe6a5d 100644 --- a/src/hooks/use-display-broadcast.ts +++ b/src/hooks/use-display-broadcast.ts @@ -8,7 +8,7 @@ import { publishUnfocusEvent, destroyDisplayPublisher, setHelloResponder, -} from '@/lib/display-publisher' +} from '@/lib/display-realtime' import { setSnapshotContext, useActiveWorkoutStore, diff --git a/src/lib/__tests__/display-publisher-hello.test.ts b/src/lib/__tests__/display-publisher-hello.test.ts deleted file mode 100644 index 9ace69cf..00000000 --- a/src/lib/__tests__/display-publisher-hello.test.ts +++ /dev/null @@ -1,174 +0,0 @@ -import { describe, it, expect, vi, beforeEach } from 'vitest' -import type { SupabaseClient } from '@supabase/supabase-js' -import { - initDisplayPublisher, - configureDisplayPublisher, - setHelloResponder, - destroyDisplayPublisher, - publishDisplaySnapshot, -} from '@/lib/display-publisher' -import type { DisplaySnapshot } from '@/domain/types/display-snapshot' - -// --------------------------------------------------------------------------- -// Mock helpers -// --------------------------------------------------------------------------- - -function createMockChannel() { - let helloListener: (() => void) | null = null - - const channel = { - send: vi.fn().mockResolvedValue('ok'), - subscribe: vi.fn().mockImplementation((cb) => { - cb?.('SUBSCRIBED') - return channel - }), - on: vi.fn((_type: string, opts: { event: string }, handler: () => void) => { - if (opts.event === 'display_hello') { - helloListener = handler - } - return channel - }), - unsubscribe: vi.fn(), - } - - return { - channel, - fireHello: () => helloListener?.(), - } -} - -function createMockClient(channel: ReturnType['channel']) { - return { - channel: vi.fn(() => channel), - removeChannel: vi.fn(), - } as unknown as SupabaseClient -} - -// Test gym ID (F018 -- publisher is now gym-scoped) -const GYM_A = 'gym-a-0000-0000-000000000000' - -// Minimal valid snapshot to trigger channel creation -const SNAPSHOT: DisplaySnapshot = { - user_id: 'user-1', - display_name: 'Test', - session_name: 'Session', - workout_started_at: '2026-04-03T10:00:00Z', - current_exercise: 'Bench Press', - exercise_index: 0, - total_exercises: 1, - sets: [], - rest_timer: { state: 'idle' }, - session_type: 'STRENGTH', - is_visible: true, -} - -// --------------------------------------------------------------------------- -// Reset -// --------------------------------------------------------------------------- - -beforeEach(() => { - destroyDisplayPublisher() -}) - -// --------------------------------------------------------------------------- -// IMPLICIT COUPLING NOTICE (P15-049) -// -// Every test below that asserts on `fireHello()` calls `publishDisplaySnapshot` -// first for the sole purpose of driving the publisher down its LAZY -// `ensureChannel()` path -- this is what attaches the `display_hello` listener -// on the mock channel and thereby populates `helloListener` in -// `createMockChannel()`. If `display-publisher.ts` ever subscribes to the -// channel during `configureDisplayPublisher` (eager init) instead of on first -// publish, replace the `publishDisplaySnapshot(SNAPSHOT)` calls below with -// the new init path -- otherwise these tests will pass for the wrong reason -// (the `fireHello()` call would silently no-op against a null listener, and -// the `responder not invoked` branch of each test would go undetected). -// --------------------------------------------------------------------------- - -// =========================================================================== -// setHelloResponder -// =========================================================================== - -describe('setHelloResponder', () => { - it('stores the callback', () => { - const responder = vi.fn() - setHelloResponder(responder) - - // Verify it was stored by triggering it through the channel - const { channel, fireHello } = createMockChannel() - const client = createMockClient(channel) - initDisplayPublisher(client) - configureDisplayPublisher({ gymId: GYM_A, intent: 'broadcasting' }) - - // Force channel creation by publishing - publishDisplaySnapshot(SNAPSHOT) - - // Simulate the display_hello broadcast event - fireHello() - - expect(responder).toHaveBeenCalledOnce() - }) -}) - -// =========================================================================== -// display_hello event -// =========================================================================== - -describe('display_hello event on channel', () => { - it('triggers the stored callback', () => { - const { channel, fireHello } = createMockChannel() - const client = createMockClient(channel) - initDisplayPublisher(client) - configureDisplayPublisher({ gymId: GYM_A, intent: 'broadcasting' }) - - const responder = vi.fn() - setHelloResponder(responder) - - // Force channel creation - publishDisplaySnapshot(SNAPSHOT) - - fireHello() - - expect(responder).toHaveBeenCalledOnce() - }) - - it('null responder does not crash when hello event fires', () => { - const { channel, fireHello } = createMockChannel() - const client = createMockClient(channel) - initDisplayPublisher(client) - configureDisplayPublisher({ gymId: GYM_A, intent: 'broadcasting' }) - - setHelloResponder(null) - - // Force channel creation - publishDisplaySnapshot(SNAPSHOT) - - // Should not throw - expect(() => fireHello()).not.toThrow() - }) -}) - -// =========================================================================== -// destroyDisplayPublisher clears responder -// =========================================================================== - -describe('destroyDisplayPublisher', () => { - it('clears the responder', () => { - const responder = vi.fn() - setHelloResponder(responder) - - destroyDisplayPublisher() - - // After destroy, set up a fresh publisher and trigger hello - const { channel, fireHello } = createMockChannel() - const client = createMockClient(channel) - initDisplayPublisher(client) - configureDisplayPublisher({ gymId: GYM_A, intent: 'broadcasting' }) - - publishDisplaySnapshot(SNAPSHOT) - fireHello() - - // The old responder should not have been called since it was cleared - expect(responder).not.toHaveBeenCalled() - }) -}) diff --git a/src/lib/__tests__/display-publisher.test.ts b/src/lib/__tests__/display-publisher.test.ts deleted file mode 100644 index 9a992681..00000000 --- a/src/lib/__tests__/display-publisher.test.ts +++ /dev/null @@ -1,456 +0,0 @@ -import { describe, it, expect, vi, beforeEach } from 'vitest' -import type { SupabaseClient } from '@supabase/supabase-js' -import type { DisplaySnapshot } from '@/domain/types/display-snapshot' -import { - initDisplayPublisher, - configureDisplayPublisher, - publishDisplaySnapshot, - publishSessionEnded, - publishFocusEvent, - publishUnfocusEvent, - destroyDisplayPublisher, - isPublisherReady, - getActiveGymId, -} from '@/lib/display-publisher' -import { getGymChannelName } from '@/lib/gym-channel' - -// --------------------------------------------------------------------------- -// Mock helpers -// --------------------------------------------------------------------------- - -function createMockClient() { - const mockSend = vi.fn().mockResolvedValue('ok') - const mockSubscribe = vi.fn().mockImplementation((cb) => { - cb?.('SUBSCRIBED') - return mockChannel - }) - const mockChannel: Record = { - send: mockSend, - subscribe: mockSubscribe, - unsubscribe: vi.fn(), - on: vi.fn().mockReturnThis(), - } - const mockClient = { - channel: vi.fn().mockReturnValue(mockChannel), - removeChannel: vi.fn(), - } as unknown as SupabaseClient - return { mockClient, mockChannel, mockSend } -} - -// Test gym IDs (not required to be UUIDs; the publisher is agnostic) -const GYM_A = 'gym-a-0000-0000-000000000000' -const GYM_B = 'gym-b-0000-0000-000000000000' - -// Minimal valid DisplaySnapshot fixture -const SNAPSHOT_FIXTURE: DisplaySnapshot = { - user_id: 'user-123', - display_name: 'Test User', - session_name: 'Morning Workout', - workout_started_at: '2026-04-03T10:00:00Z', - current_exercise: 'Bench Press', - exercise_index: 0, - total_exercises: 3, - sets: [ - { - set_number: 1, - prescribed: { reps: 5, weight: { value: 135, unit: 'lb' } }, - completed: false, - }, - ], - rest_timer: { state: 'idle' }, - session_type: 'STRENGTH', - is_visible: true, -} - -// --------------------------------------------------------------------------- -// Tests -// --------------------------------------------------------------------------- - -beforeEach(() => { - destroyDisplayPublisher() -}) - -// =========================================================================== -// publishDisplaySnapshot -// =========================================================================== - -describe('publishDisplaySnapshot', () => { - it('is a no-op when publisher is not initialized', () => { - const { mockSend } = createMockClient() - // Do NOT call initDisplayPublisher - publishDisplaySnapshot(SNAPSHOT_FIXTURE) - expect(mockSend).not.toHaveBeenCalled() - }) - - it('is a no-op when gymId is null (Private workout)', () => { - const { mockClient, mockSend } = createMockClient() - initDisplayPublisher(mockClient) - configureDisplayPublisher({ gymId: null, intent: 'private' }) - - publishDisplaySnapshot(SNAPSHOT_FIXTURE) - expect(mockSend).not.toHaveBeenCalled() - expect(mockClient.channel).not.toHaveBeenCalled() - }) - - it('sends workout_snapshot to the per-gym channel when configured', () => { - const { mockClient, mockSend } = createMockClient() - initDisplayPublisher(mockClient) - configureDisplayPublisher({ gymId: GYM_A, intent: 'broadcasting' }) - - publishDisplaySnapshot(SNAPSHOT_FIXTURE) - - expect(mockClient.channel).toHaveBeenCalledWith(getGymChannelName(GYM_A), { - config: { broadcast: { ack: false, self: false } }, - }) - expect(mockSend).toHaveBeenCalledWith({ - type: 'broadcast', - event: 'workout_snapshot', - payload: SNAPSHOT_FIXTURE, - }) - }) -}) - -// =========================================================================== -// publishSessionEnded -// =========================================================================== - -describe('publishSessionEnded', () => { - it('is a no-op when gymId is null', () => { - const { mockClient, mockSend } = createMockClient() - initDisplayPublisher(mockClient) - configureDisplayPublisher({ gymId: null, intent: 'private' }) - - publishSessionEnded('user-abc') - expect(mockSend).not.toHaveBeenCalled() - }) - - it('sends session_ended event with user_id to the per-gym channel', () => { - const { mockClient, mockSend } = createMockClient() - initDisplayPublisher(mockClient) - configureDisplayPublisher({ gymId: GYM_A, intent: 'broadcasting' }) - - publishSessionEnded('user-abc') - expect(mockClient.channel).toHaveBeenCalledWith(getGymChannelName(GYM_A), expect.any(Object)) - expect(mockSend).toHaveBeenCalledWith({ - type: 'broadcast', - event: 'session_ended', - payload: { user_id: 'user-abc' }, - }) - }) -}) - -// =========================================================================== -// publishFocusEvent -// =========================================================================== - -describe('publishFocusEvent', () => { - it('is a no-op when gymId is null', () => { - const { mockClient, mockSend } = createMockClient() - initDisplayPublisher(mockClient) - configureDisplayPublisher({ gymId: null, intent: 'private' }) - - publishFocusEvent('user-xyz') - expect(mockSend).not.toHaveBeenCalled() - }) - - it('sends focus event with user_id to the per-gym channel', () => { - const { mockClient, mockSend } = createMockClient() - initDisplayPublisher(mockClient) - configureDisplayPublisher({ gymId: GYM_A, intent: 'broadcasting' }) - - publishFocusEvent('user-xyz') - expect(mockClient.channel).toHaveBeenCalledWith(getGymChannelName(GYM_A), expect.any(Object)) - expect(mockSend).toHaveBeenCalledWith({ - type: 'broadcast', - event: 'focus', - payload: { user_id: 'user-xyz' }, - }) - }) -}) - -// =========================================================================== -// publishUnfocusEvent -// =========================================================================== - -describe('publishUnfocusEvent', () => { - it('is a no-op when gymId is null', () => { - const { mockClient, mockSend } = createMockClient() - initDisplayPublisher(mockClient) - configureDisplayPublisher({ gymId: null, intent: 'private' }) - - publishUnfocusEvent() - expect(mockSend).not.toHaveBeenCalled() - }) - - it('sends unfocus event with empty payload to the per-gym channel', () => { - const { mockClient, mockSend } = createMockClient() - initDisplayPublisher(mockClient) - configureDisplayPublisher({ gymId: GYM_A, intent: 'broadcasting' }) - - publishUnfocusEvent() - expect(mockClient.channel).toHaveBeenCalledWith(getGymChannelName(GYM_A), expect.any(Object)) - expect(mockSend).toHaveBeenCalledWith({ - type: 'broadcast', - event: 'unfocus', - payload: {}, - }) - }) -}) - -// =========================================================================== -// Switching gyms between publishes -// =========================================================================== - -describe('switching gyms between publishes', () => { - it('recreates the channel when gymId changes and sends to the new channel', () => { - const { mockClient, mockSend } = createMockClient() - initDisplayPublisher(mockClient) - - // First publish to gym A - configureDisplayPublisher({ gymId: GYM_A, intent: 'broadcasting' }) - publishDisplaySnapshot(SNAPSHOT_FIXTURE) - - expect(mockClient.channel).toHaveBeenCalledTimes(1) - expect(mockClient.channel).toHaveBeenLastCalledWith( - getGymChannelName(GYM_A), - expect.any(Object), - ) - - // Switch to gym B -- the stale channel should be torn down on reconfigure - configureDisplayPublisher({ gymId: GYM_B, intent: 'broadcasting' }) - - expect(mockClient.removeChannel).toHaveBeenCalledTimes(1) - - // Publish again; a new channel for gym B should be created - publishDisplaySnapshot(SNAPSHOT_FIXTURE) - - expect(mockClient.channel).toHaveBeenCalledTimes(2) - expect(mockClient.channel).toHaveBeenLastCalledWith( - getGymChannelName(GYM_B), - expect.any(Object), - ) - - // Both sends happened - expect(mockSend).toHaveBeenCalledTimes(2) - }) -}) - -// =========================================================================== -// isPublisherReady / getActiveGymId -// =========================================================================== - -describe('isPublisherReady', () => { - it('returns false when client is not initialized', () => { - expect(isPublisherReady()).toBe(false) - }) - - it('returns false when client is initialized but gymId is null', () => { - const { mockClient } = createMockClient() - initDisplayPublisher(mockClient) - configureDisplayPublisher({ gymId: null, intent: 'private' }) - expect(isPublisherReady()).toBe(false) - }) - - it('returns true when client is initialized and gymId is set', () => { - const { mockClient } = createMockClient() - initDisplayPublisher(mockClient) - configureDisplayPublisher({ gymId: GYM_A, intent: 'broadcasting' }) - expect(isPublisherReady()).toBe(true) - }) -}) - -describe('getActiveGymId', () => { - it('returns null when not configured', () => { - expect(getActiveGymId()).toBeNull() - }) - - it('returns the configured gym ID', () => { - const { mockClient } = createMockClient() - initDisplayPublisher(mockClient) - configureDisplayPublisher({ gymId: GYM_A, intent: 'broadcasting' }) - expect(getActiveGymId()).toBe(GYM_A) - }) - - it('returns null after configuring with null', () => { - const { mockClient } = createMockClient() - initDisplayPublisher(mockClient) - configureDisplayPublisher({ gymId: GYM_A, intent: 'broadcasting' }) - configureDisplayPublisher({ gymId: null, intent: 'private' }) - expect(getActiveGymId()).toBeNull() - }) -}) - -// =========================================================================== -// destroyDisplayPublisher -// =========================================================================== - -describe('destroyDisplayPublisher', () => { - it('calls removeChannel and resets state so subsequent publish is a no-op', () => { - const { mockClient, mockSend } = createMockClient() - initDisplayPublisher(mockClient) - configureDisplayPublisher({ gymId: GYM_A, intent: 'broadcasting' }) - - // First publish to create the channel - publishDisplaySnapshot(SNAPSHOT_FIXTURE) - expect(mockSend).toHaveBeenCalledTimes(1) - - // Destroy - destroyDisplayPublisher() - expect(mockClient.removeChannel).toHaveBeenCalled() - - // getActiveGymId should reset - expect(getActiveGymId()).toBeNull() - - // Subsequent publish should be a no-op (no client, no gymId) - mockSend.mockClear() - publishDisplaySnapshot(SNAPSHOT_FIXTURE) - expect(mockSend).not.toHaveBeenCalled() - }) -}) - -// =========================================================================== -// Fire-and-forget error handling -// =========================================================================== - -describe('fire-and-forget error handling', () => { - it('does not propagate when send rejects', async () => { - const { mockClient, mockSend } = createMockClient() - initDisplayPublisher(mockClient) - configureDisplayPublisher({ gymId: GYM_A, intent: 'broadcasting' }) - - mockSend.mockRejectedValueOnce(new Error('network failure')) - - // Should not throw - expect(() => publishDisplaySnapshot(SNAPSHOT_FIXTURE)).not.toThrow() - - // Let the microtask queue flush so the .catch handler runs - await vi.waitFor(() => { - expect(mockSend).toHaveBeenCalledTimes(1) - }) - }) -}) - -// =========================================================================== -// Lazy channel creation -// =========================================================================== - -describe('lazy channel creation', () => { - it('creates channel on first publish and reuses it on second within the same gym', () => { - const { mockClient, mockSend } = createMockClient() - initDisplayPublisher(mockClient) - configureDisplayPublisher({ gymId: GYM_A, intent: 'broadcasting' }) - - publishDisplaySnapshot(SNAPSHOT_FIXTURE) - publishDisplaySnapshot(SNAPSHOT_FIXTURE) - - // client.channel should only be called once (lazy init, then reuse) - expect(mockClient.channel).toHaveBeenCalledTimes(1) - // But send should be called twice - expect(mockSend).toHaveBeenCalledTimes(2) - }) -}) - -// =========================================================================== -// Silent drop warning (P14-001 / P14-002) -// =========================================================================== - -describe('silent drop warning', () => { - it('logs a one-shot warning when publishing in unconfigured mode (refresh case)', () => { - const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) - const { mockClient, mockSend } = createMockClient() - // Init but never configure -- simulates the page-refresh case where - // module state is wiped but the workout log still tries to publish. - initDisplayPublisher(mockClient) - - publishDisplaySnapshot(SNAPSHOT_FIXTURE) - publishDisplaySnapshot(SNAPSHOT_FIXTURE) - publishSessionEnded('user-x') - - expect(mockSend).not.toHaveBeenCalled() - // Exactly one warning fired (one per session, not per dropped event) - const matchingWarns = warnSpy.mock.calls.filter((call) => - String(call[0]).includes('publisher unconfigured'), - ) - expect(matchingWarns).toHaveLength(1) - expect(matchingWarns[0][0]).toContain('refreshed mid-workout') - - warnSpy.mockRestore() - }) - - it('does NOT warn when publishing in private mode (silent drops are intentional)', () => { - const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) - const { mockClient, mockSend } = createMockClient() - initDisplayPublisher(mockClient) - configureDisplayPublisher({ gymId: null, intent: 'private' }) - - publishDisplaySnapshot(SNAPSHOT_FIXTURE) - publishSessionEnded('user-x') - publishFocusEvent('user-x') - publishUnfocusEvent() - - expect(mockSend).not.toHaveBeenCalled() - const matchingWarns = warnSpy.mock.calls.filter((call) => - String(call[0]).includes('publisher unconfigured'), - ) - expect(matchingWarns).toHaveLength(0) - - warnSpy.mockRestore() - }) - - it('rejects configureDisplayPublisher with intent=broadcasting and gymId=null', () => { - const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}) - const { mockClient } = createMockClient() - initDisplayPublisher(mockClient) - - // Runtime guard: types allow gymId=null+intent=broadcasting but the - // function rejects it because the combination is incoherent. - configureDisplayPublisher({ gymId: null, intent: 'broadcasting' }) - - expect(errSpy).toHaveBeenCalledWith( - expect.stringContaining('intent=broadcasting but gymId=null'), - ) - // Publisher state should still be 'unconfigured' since the call was rejected - expect(getActiveGymId()).toBeNull() - - errSpy.mockRestore() - }) - - it('rejects configureDisplayPublisher with intent=private and non-null gymId', () => { - const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}) - const { mockClient } = createMockClient() - initDisplayPublisher(mockClient) - - // Runtime guard: types allow gymId=string+intent=private but the - // function rejects it because the combination is incoherent. - configureDisplayPublisher({ gymId: GYM_A, intent: 'private' }) - - expect(errSpy).toHaveBeenCalledWith(expect.stringContaining('intent=private but gymId=')) - - errSpy.mockRestore() - }) - - it('resets the silent-drop warning flag on reconfigure (next workout warns fresh)', () => { - const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) - const { mockClient } = createMockClient() - initDisplayPublisher(mockClient) - - // First workout: drop in unconfigured mode -- warns once - publishDisplaySnapshot(SNAPSHOT_FIXTURE) - let unconfigured = warnSpy.mock.calls.filter((c) => - String(c[0]).includes('publisher unconfigured'), - ) - expect(unconfigured).toHaveLength(1) - - // Configure to a real gym, then reconfigure to private -- subsequent - // unconfigured drop should warn again (flag was reset by reconfigure) - configureDisplayPublisher({ gymId: GYM_A, intent: 'broadcasting' }) - destroyDisplayPublisher() - initDisplayPublisher(mockClient) - - publishDisplaySnapshot(SNAPSHOT_FIXTURE) - unconfigured = warnSpy.mock.calls.filter((c) => String(c[0]).includes('publisher unconfigured')) - expect(unconfigured).toHaveLength(2) - - warnSpy.mockRestore() - }) -}) diff --git a/src/lib/__tests__/display-realtime.test.ts b/src/lib/__tests__/display-realtime.test.ts new file mode 100644 index 00000000..7c2905fe --- /dev/null +++ b/src/lib/__tests__/display-realtime.test.ts @@ -0,0 +1,391 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' +import type { SupabaseClient } from '@supabase/supabase-js' +import type { DisplaySnapshot } from '@/domain/types/display-snapshot' +import { + initDisplayPublisher, + configureDisplayPublisher, + publishDisplaySnapshot, + publishSessionEnded, + publishFocusEvent, + publishUnfocusEvent, + setHelloResponder, + destroyDisplayPublisher, + isPublisherReady, + initDisplaySubscriber, + subscribeToDisplay, + publishHello, + destroyDisplaySubscriber, + type DisplayEventHandlers, +} from '@/lib/display-realtime' +import { getGymChannelName } from '@/lib/gym-channel' + +// --------------------------------------------------------------------------- +// Mock helpers +// --------------------------------------------------------------------------- + +type BroadcastCallback = (msg: { payload: unknown }) => void +type SubscribeCallback = (status: string, err?: unknown) => void + +function createMockClient() { + const mockSend = vi.fn().mockResolvedValue('ok') + const broadcastHandlers = new Map() + let subscribeCallback: SubscribeCallback | null = null + + const mockChannel: Record = { + send: mockSend, + subscribe: vi.fn().mockImplementation((cb: SubscribeCallback) => { + subscribeCallback = cb + cb('SUBSCRIBED') + return mockChannel + }), + unsubscribe: vi.fn(), + on: vi + .fn() + .mockImplementation((_type: string, opts: { event: string }, cb: BroadcastCallback) => { + broadcastHandlers.set(opts.event, cb) + return mockChannel + }), + } + + const mockClient = { + channel: vi.fn().mockReturnValue(mockChannel), + removeChannel: vi.fn(), + } as unknown as SupabaseClient + + return { + mockClient, + mockChannel, + mockSend, + broadcastHandlers, + getSubscribeCallback: () => subscribeCallback, + } +} + +function createMockHandlers(): DisplayEventHandlers { + return { + onSnapshot: vi.fn(), + onSessionEnded: vi.fn(), + onFocus: vi.fn(), + onUnfocus: vi.fn(), + onIdleSnapshot: vi.fn(), + onStatusChange: vi.fn(), + } +} + +// Test gym IDs +const GYM_A = 'gym-a-0000-0000-000000000000' + +// Minimal valid DisplaySnapshot fixture +const SNAPSHOT_FIXTURE: DisplaySnapshot = { + user_id: 'user-123', + display_name: 'Test User', + session_name: 'Morning Workout', + workout_started_at: '2026-04-03T10:00:00Z', + current_exercise: 'Bench Press', + exercise_index: 0, + total_exercises: 3, + sets: [ + { + set_number: 1, + prescribed: { reps: 5, weight: { value: 135, unit: 'lb' } }, + completed: false, + }, + ], + rest_timer: { state: 'idle' }, + session_type: 'STRENGTH', + is_visible: true, +} + +const BC_OPTS = { config: { broadcast: { ack: false, self: false } } } + +// --------------------------------------------------------------------------- +// Lifecycle +// --------------------------------------------------------------------------- + +beforeEach(() => { + vi.useFakeTimers() +}) + +afterEach(() => { + destroyDisplayPublisher() + destroyDisplaySubscriber() + vi.useRealTimers() + vi.restoreAllMocks() +}) + +// =========================================================================== +// Publisher +// =========================================================================== + +describe('Publisher', () => { + describe('initDisplayPublisher + configureDisplayPublisher', () => { + it('sets up state so subsequent publishes reach the channel', () => { + const { mockClient, mockSend } = createMockClient() + initDisplayPublisher(mockClient) + configureDisplayPublisher({ gymId: GYM_A, intent: 'broadcasting' }) + + publishDisplaySnapshot(SNAPSHOT_FIXTURE) + + expect(mockClient.channel).toHaveBeenCalledWith(getGymChannelName(GYM_A), BC_OPTS) + expect(mockSend).toHaveBeenCalledTimes(1) + }) + }) + + describe('publishDisplaySnapshot', () => { + it('calls channel.send with workout_snapshot event and the snapshot as payload', () => { + const { mockClient, mockSend } = createMockClient() + initDisplayPublisher(mockClient) + configureDisplayPublisher({ gymId: GYM_A, intent: 'broadcasting' }) + + publishDisplaySnapshot(SNAPSHOT_FIXTURE) + + expect(mockSend).toHaveBeenCalledWith({ + type: 'broadcast', + event: 'workout_snapshot', + payload: SNAPSHOT_FIXTURE, + }) + }) + }) + + describe('publishSessionEnded', () => { + it('sends session_ended event with user_id payload', () => { + const { mockClient, mockSend } = createMockClient() + initDisplayPublisher(mockClient) + configureDisplayPublisher({ gymId: GYM_A, intent: 'broadcasting' }) + + publishSessionEnded('user-abc') + + expect(mockSend).toHaveBeenCalledWith({ + type: 'broadcast', + event: 'session_ended', + payload: { user_id: 'user-abc' }, + }) + }) + }) + + describe('publishFocusEvent', () => { + it('sends focus event with user_id payload', () => { + const { mockClient, mockSend } = createMockClient() + initDisplayPublisher(mockClient) + configureDisplayPublisher({ gymId: GYM_A, intent: 'broadcasting' }) + + publishFocusEvent('user-xyz') + + expect(mockSend).toHaveBeenCalledWith({ + type: 'broadcast', + event: 'focus', + payload: { user_id: 'user-xyz' }, + }) + }) + }) + + describe('publishUnfocusEvent', () => { + it('sends unfocus event with empty payload', () => { + const { mockClient, mockSend } = createMockClient() + initDisplayPublisher(mockClient) + configureDisplayPublisher({ gymId: GYM_A, intent: 'broadcasting' }) + + publishUnfocusEvent() + + expect(mockSend).toHaveBeenCalledWith({ + type: 'broadcast', + event: 'unfocus', + payload: {}, + }) + }) + }) + + describe('configureDisplayPublisher boundary validation', () => { + it('logs error and returns when intent=broadcasting but gymId=null', () => { + const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}) + const { mockClient } = createMockClient() + initDisplayPublisher(mockClient) + + configureDisplayPublisher({ gymId: null, intent: 'broadcasting' }) + + expect(errSpy).toHaveBeenCalledWith(expect.stringContaining('broadcasting requires gymId')) + // State should not have been mutated + expect(isPublisherReady()).toBe(false) + }) + + it('logs error and returns when intent=private but gymId is non-null', () => { + const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}) + const { mockClient } = createMockClient() + initDisplayPublisher(mockClient) + + configureDisplayPublisher({ gymId: GYM_A, intent: 'private' }) + + expect(errSpy).toHaveBeenCalledWith(expect.stringContaining('private requires gymId=null')) + }) + }) + + describe('destroyDisplayPublisher', () => { + it('calls removeChannel and resets state so subsequent publish is a no-op', () => { + const { mockClient, mockSend } = createMockClient() + initDisplayPublisher(mockClient) + configureDisplayPublisher({ gymId: GYM_A, intent: 'broadcasting' }) + + // Create the channel via a publish + publishDisplaySnapshot(SNAPSHOT_FIXTURE) + expect(mockSend).toHaveBeenCalledTimes(1) + + destroyDisplayPublisher() + + expect(mockClient.removeChannel).toHaveBeenCalled() + + // Subsequent publish should be a no-op + mockSend.mockClear() + publishDisplaySnapshot(SNAPSHOT_FIXTURE) + expect(mockSend).not.toHaveBeenCalled() + }) + }) + + describe('isPublisherReady', () => { + it('returns false when no client is initialized', () => { + expect(isPublisherReady()).toBe(false) + }) + + it('returns false when client is set but gymId is null', () => { + const { mockClient } = createMockClient() + initDisplayPublisher(mockClient) + configureDisplayPublisher({ gymId: null, intent: 'private' }) + + expect(isPublisherReady()).toBe(false) + }) + + it('returns true when client and gymId are both set', () => { + const { mockClient } = createMockClient() + initDisplayPublisher(mockClient) + configureDisplayPublisher({ gymId: GYM_A, intent: 'broadcasting' }) + + expect(isPublisherReady()).toBe(true) + }) + }) +}) + +// =========================================================================== +// Subscriber +// =========================================================================== + +describe('Subscriber', () => { + describe('subscribeToDisplay', () => { + it('subscribes to the correct gym channel name', () => { + const { mockClient } = createMockClient() + initDisplaySubscriber(mockClient) + const handlers = createMockHandlers() + + subscribeToDisplay({ gymId: GYM_A, handlers }) + + expect(mockClient.channel).toHaveBeenCalledWith(getGymChannelName(GYM_A), BC_OPTS) + }) + }) + + describe('workout_snapshot broadcast', () => { + it('fires onSnapshot handler with parsed data when subscriber receives a workout_snapshot', () => { + const { mockClient, broadcastHandlers } = createMockClient() + initDisplaySubscriber(mockClient) + const handlers = createMockHandlers() + + subscribeToDisplay({ gymId: GYM_A, handlers }) + + // Simulate a broadcast arriving on the channel + const snapshotCb = broadcastHandlers.get('workout_snapshot') + expect(snapshotCb).toBeDefined() + snapshotCb!({ payload: SNAPSHOT_FIXTURE }) + + expect(handlers.onSnapshot).toHaveBeenCalledWith(SNAPSHOT_FIXTURE) + }) + }) + + describe('session_ended broadcast', () => { + it('fires onSessionEnded handler when subscriber receives a session_ended event', () => { + const { mockClient, broadcastHandlers } = createMockClient() + initDisplaySubscriber(mockClient) + const handlers = createMockHandlers() + + subscribeToDisplay({ gymId: GYM_A, handlers }) + + const sessionEndedCb = broadcastHandlers.get('session_ended') + expect(sessionEndedCb).toBeDefined() + sessionEndedCb!({ payload: { user_id: 'user-abc' } }) + + expect(handlers.onSessionEnded).toHaveBeenCalledWith({ user_id: 'user-abc' }) + }) + }) + + describe('publishHello', () => { + it('sends display_hello event on the subscriber channel', () => { + const { mockClient, mockSend } = createMockClient() + initDisplaySubscriber(mockClient) + const handlers = createMockHandlers() + + subscribeToDisplay({ gymId: GYM_A, handlers }) + publishHello() + + expect(mockSend).toHaveBeenCalledWith({ + type: 'broadcast', + event: 'display_hello', + payload: {}, + }) + }) + }) + + describe('destroyDisplaySubscriber', () => { + it('calls removeChannel and clears retry timers', () => { + const { mockClient, getSubscribeCallback } = createMockClient() + initDisplaySubscriber(mockClient) + const handlers = createMockHandlers() + + subscribeToDisplay({ gymId: GYM_A, handlers }) + + // Initial subscribe fires 'connected' -- clear that so we can assert + // no NEW connected call arrives after destroy. + ;(handlers.onStatusChange as ReturnType).mockClear() + const channelCallsBefore = (mockClient.channel as ReturnType).mock.calls.length + + // Simulate a channel error to trigger a retry timer + const subCb = getSubscribeCallback() + subCb?.('CHANNEL_ERROR', new Error('test')) + + // Now destroy -- should clear the pending retry timer and remove channel + destroyDisplaySubscriber() + + expect(mockClient.removeChannel).toHaveBeenCalled() + + // Advance timers past the retry delay -- no new subscription should occur + vi.advanceTimersByTime(60_000) + + // No new channel was created (the retry timer was cleared) + const channelCallsAfter = (mockClient.channel as ReturnType).mock.calls.length + expect(channelCallsAfter).toBe(channelCallsBefore) + + // onStatusChange should NOT have been called with 'connected' again + expect(handlers.onStatusChange).not.toHaveBeenCalledWith('connected') + }) + }) +}) + +// =========================================================================== +// Hello handshake +// =========================================================================== + +describe('Hello handshake', () => { + it('publisher fires the registered helloResponder when it receives a display_hello event', () => { + const { mockClient, broadcastHandlers } = createMockClient() + initDisplayPublisher(mockClient) + configureDisplayPublisher({ gymId: GYM_A, intent: 'broadcasting' }) + + const responder = vi.fn() + setHelloResponder(responder) + + // Force channel creation by publishing something + publishDisplaySnapshot(SNAPSHOT_FIXTURE) + + // Simulate the display_hello broadcast arriving on the publisher channel + const helloCb = broadcastHandlers.get('display_hello') + expect(helloCb).toBeDefined() + helloCb!({ payload: {} }) + + expect(responder).toHaveBeenCalledTimes(1) + }) +}) diff --git a/src/lib/__tests__/display-subscriber.test.ts b/src/lib/__tests__/display-subscriber.test.ts deleted file mode 100644 index 7382e098..00000000 --- a/src/lib/__tests__/display-subscriber.test.ts +++ /dev/null @@ -1,371 +0,0 @@ -import { vi, describe, it, expect, afterEach, beforeEach } from 'vitest' -import type { SupabaseClient } from '@supabase/supabase-js' -import type { DisplaySnapshot } from '@/domain/types/display-snapshot' -import { - initDisplaySubscriber, - subscribeToDisplay, - publishHello, - destroyDisplaySubscriber, - type DisplayEventHandlers, -} from '@/lib/display-subscriber' -import { getGymChannelName } from '@/lib/gym-channel' - -// --------------------------------------------------------------------------- -// Mock helpers -// --------------------------------------------------------------------------- - -function createMockChannel() { - const listeners = new Map void>() - let subscribeCallback: ((status: string, err?: Error) => void) | null = null - - const channel = { - on: vi.fn( - ( - _type: string, - opts: { event: string }, - handler: (incoming: { payload: unknown }) => void, - ) => { - listeners.set(opts.event, handler) - return channel // chainable - }, - ), - subscribe: vi.fn((cb: (status: string, err?: Error) => void) => { - subscribeCallback = cb - return channel - }), - send: vi.fn(() => Promise.resolve()), - } - - return { channel, listeners, getSubscribeCallback: () => subscribeCallback } -} - -function createMockClient(channel: ReturnType['channel']) { - return { - channel: vi.fn(() => channel), - removeChannel: vi.fn(), - } as unknown as SupabaseClient -} - -// Test gym IDs -const GYM_A = 'gym-a-0000-0000-000000000000' -const GYM_B = 'gym-b-0000-0000-000000000000' - -// Minimal valid snapshot -const SNAPSHOT_FIXTURE: DisplaySnapshot = { - user_id: 'user-123', - display_name: 'Test User', - session_name: 'Morning Workout', - workout_started_at: '2026-04-03T10:00:00Z', - current_exercise: 'Bench Press', - exercise_index: 0, - total_exercises: 3, - sets: [], - rest_timer: { state: 'idle' }, - session_type: 'STRENGTH', - is_visible: true, -} - -function createMockHandlers(): DisplayEventHandlers { - return { - onSnapshot: vi.fn(), - onSessionEnded: vi.fn(), - onFocus: vi.fn(), - onUnfocus: vi.fn(), - onIdleSnapshot: vi.fn(), - onStatusChange: vi.fn(), - } -} - -// --------------------------------------------------------------------------- -// Reset module state between tests -// --------------------------------------------------------------------------- - -beforeEach(() => { - vi.useFakeTimers() -}) - -afterEach(() => { - destroyDisplaySubscriber() - vi.useRealTimers() -}) - -// =========================================================================== -// initDisplaySubscriber -// =========================================================================== - -describe('initDisplaySubscriber', () => { - it('stores client so subscribeToDisplay can create a channel', () => { - const { channel } = createMockChannel() - const client = createMockClient(channel) - - initDisplaySubscriber(client) - - const handlers = createMockHandlers() - subscribeToDisplay({ gymId: GYM_A, handlers }) - - // The client.channel method should have been called with the per-gym name - expect(client.channel).toHaveBeenCalledWith(getGymChannelName(GYM_A), { - config: { broadcast: { ack: false, self: false } }, - }) - }) -}) - -// =========================================================================== -// subscribeToDisplay -// =========================================================================== - -describe('subscribeToDisplay', () => { - it('creates channel named display:gym: with correct config', () => { - const { channel } = createMockChannel() - const client = createMockClient(channel) - initDisplaySubscriber(client) - - subscribeToDisplay({ gymId: GYM_A, handlers: createMockHandlers() }) - - expect(client.channel).toHaveBeenCalledWith(getGymChannelName(GYM_A), { - config: { broadcast: { ack: false, self: false } }, - }) - }) - - it('opens a different channel name for a different gym', () => { - const { channel } = createMockChannel() - const client = createMockClient(channel) - initDisplaySubscriber(client) - - subscribeToDisplay({ gymId: GYM_B, handlers: createMockHandlers() }) - - expect(client.channel).toHaveBeenCalledWith(getGymChannelName(GYM_B), { - config: { broadcast: { ack: false, self: false } }, - }) - }) - - it('throws if client is not initialized (P14-004)', () => { - // Per F018 P14-004: subscribeToDisplay must throw rather than warn-and- - // return when the client is not initialized, so the route's outer - // try/catch can map this into a `subscribe-failed` BootError with a - // visible Retry button. - expect(() => subscribeToDisplay({ gymId: GYM_A, handlers: createMockHandlers() })).toThrow( - /client not initialized/i, - ) - }) -}) - -// =========================================================================== -// Event dispatching -// =========================================================================== - -describe('event dispatching', () => { - it('valid workout_snapshot calls onSnapshot with parsed data', () => { - const { channel, listeners } = createMockChannel() - const client = createMockClient(channel) - initDisplaySubscriber(client) - - const handlers = createMockHandlers() - subscribeToDisplay({ gymId: GYM_A, handlers }) - - // Simulate a valid snapshot broadcast - const listener = listeners.get('workout_snapshot')! - listener({ payload: SNAPSHOT_FIXTURE }) - - expect(handlers.onSnapshot).toHaveBeenCalledWith(SNAPSHOT_FIXTURE) - }) - - it('invalid workout_snapshot is dropped with console.warn', () => { - const { channel, listeners } = createMockChannel() - const client = createMockClient(channel) - initDisplaySubscriber(client) - - const handlers = createMockHandlers() - const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) - subscribeToDisplay({ gymId: GYM_A, handlers }) - - const listener = listeners.get('workout_snapshot')! - listener({ payload: { bad: 'data' } }) - - expect(handlers.onSnapshot).not.toHaveBeenCalled() - expect(warnSpy).toHaveBeenCalledWith( - expect.stringContaining('Invalid workout_snapshot'), - expect.anything(), - ) - warnSpy.mockRestore() - }) - - it('valid session_ended calls onSessionEnded', () => { - const { channel, listeners } = createMockChannel() - const client = createMockClient(channel) - initDisplaySubscriber(client) - - const handlers = createMockHandlers() - subscribeToDisplay({ gymId: GYM_A, handlers }) - - const listener = listeners.get('session_ended')! - listener({ payload: { user_id: 'u1' } }) - - expect(handlers.onSessionEnded).toHaveBeenCalledWith({ user_id: 'u1' }) - }) - - it('valid focus calls onFocus', () => { - const { channel, listeners } = createMockChannel() - const client = createMockClient(channel) - initDisplaySubscriber(client) - - const handlers = createMockHandlers() - subscribeToDisplay({ gymId: GYM_A, handlers }) - - const listener = listeners.get('focus')! - listener({ payload: { user_id: 'u2' } }) - - expect(handlers.onFocus).toHaveBeenCalledWith({ user_id: 'u2' }) - }) - - it('unfocus calls onUnfocus', () => { - const { channel, listeners } = createMockChannel() - const client = createMockClient(channel) - initDisplaySubscriber(client) - - const handlers = createMockHandlers() - subscribeToDisplay({ gymId: GYM_A, handlers }) - - // unfocus handler has a different signature -- no payload arg - const listener = listeners.get('unfocus')! - listener({ payload: {} }) - - expect(handlers.onUnfocus).toHaveBeenCalled() - }) -}) - -// =========================================================================== -// Status changes -// =========================================================================== - -describe('status changes', () => { - it('SUBSCRIBED calls onStatusChange("connected")', () => { - const { channel, getSubscribeCallback } = createMockChannel() - const client = createMockClient(channel) - initDisplaySubscriber(client) - - const handlers = createMockHandlers() - subscribeToDisplay({ gymId: GYM_A, handlers }) - - const cb = getSubscribeCallback()! - cb('SUBSCRIBED') - - expect(handlers.onStatusChange).toHaveBeenCalledWith('connected') - }) - - it.each(['TIMED_OUT', 'CHANNEL_ERROR', 'CLOSED'])( - '%s calls onStatusChange("reconnecting")', - (status) => { - const { channel, getSubscribeCallback } = createMockChannel() - const client = createMockClient(channel) - initDisplaySubscriber(client) - - const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) - const handlers = createMockHandlers() - subscribeToDisplay({ gymId: GYM_A, handlers }) - - const cb = getSubscribeCallback()! - cb(status) - - expect(handlers.onStatusChange).toHaveBeenCalledWith('reconnecting') - warnSpy.mockRestore() - }, - ) -}) - -// =========================================================================== -// Reconnect retains the original gymId -// =========================================================================== - -describe('reconnect retains gym ID', () => { - it('reuses the original gymId on retry after a CHANNEL_ERROR', () => { - const { channel, getSubscribeCallback } = createMockChannel() - const client = createMockClient(channel) - initDisplaySubscriber(client) - - const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}) - const infoSpy = vi.spyOn(console, 'info').mockImplementation(() => {}) - - const handlers = createMockHandlers() - subscribeToDisplay({ gymId: GYM_A, handlers }) - - expect(client.channel).toHaveBeenCalledTimes(1) - expect(client.channel).toHaveBeenLastCalledWith(getGymChannelName(GYM_A), expect.any(Object)) - - // Trigger a terminal status to force a retry - const cb = getSubscribeCallback()! - cb('CHANNEL_ERROR') - - // Fast-forward through the retry delay using an explicit time budget - // (P14-046). `vi.runOnlyPendingTimers` was sensitive to timer-ordering - // changes (e.g., a defensive setTimeout(0) added in the subscribe - // path). Using a generous budget keeps the test deterministic against - // future production-code timer additions. - vi.advanceTimersByTime(5_000) - - // The reconnect must have opened a channel for the same gym, not a - // different one. - expect(client.channel).toHaveBeenCalledTimes(2) - expect(client.channel).toHaveBeenNthCalledWith(2, getGymChannelName(GYM_A), expect.any(Object)) - - // Guard: the other gym's channel name was never used - const channelNames = (client.channel as unknown as ReturnType).mock.calls.map( - (call: unknown[]) => call[0] as string, - ) - expect(channelNames.every((name) => name === getGymChannelName(GYM_A))).toBe(true) - expect(channelNames.includes(getGymChannelName(GYM_B))).toBe(false) - - warnSpy.mockRestore() - infoSpy.mockRestore() - }) -}) - -// =========================================================================== -// publishHello -// =========================================================================== - -describe('publishHello', () => { - it('sends correct event through the channel', () => { - const { channel } = createMockChannel() - const client = createMockClient(channel) - initDisplaySubscriber(client) - subscribeToDisplay({ gymId: GYM_A, handlers: createMockHandlers() }) - - publishHello() - - expect(channel.send).toHaveBeenCalledWith({ - type: 'broadcast', - event: 'display_hello', - payload: {}, - }) - }) - - it('is a no-op when no channel exists', () => { - // No init, no subscribe -- publishHello should not throw - expect(() => publishHello()).not.toThrow() - }) -}) - -// =========================================================================== -// destroyDisplaySubscriber -// =========================================================================== - -describe('destroyDisplaySubscriber', () => { - it('removes channel and resets state', () => { - const { channel } = createMockChannel() - const client = createMockClient(channel) - initDisplaySubscriber(client) - subscribeToDisplay({ gymId: GYM_A, handlers: createMockHandlers() }) - - destroyDisplaySubscriber() - - expect(client.removeChannel).toHaveBeenCalled() - - // Subsequent subscribe should throw (no client). Per F018 P14-004, - // subscribeToDisplay throws on uninit so the route's outer try/catch - // can map this into a `subscribe-failed` BootError. - expect(() => subscribeToDisplay({ gymId: GYM_A, handlers: createMockHandlers() })).toThrow( - /client not initialized/i, - ) - }) -}) diff --git a/src/lib/display-publisher.ts b/src/lib/display-publisher.ts deleted file mode 100644 index be7b503b..00000000 --- a/src/lib/display-publisher.ts +++ /dev/null @@ -1,429 +0,0 @@ -import type { SupabaseClient } from '@supabase/supabase-js' -import type { RealtimeChannel } from '@supabase/realtime-js' -import type { DisplaySnapshot } from '@/domain/types/display-snapshot' -import { getGymChannelName } from '@/lib/gym-channel' - -// --------------------------------------------------------------------------- -// Module-scope state -// --------------------------------------------------------------------------- - -/** - * Publisher mode -- distinguishes the three operationally distinct states - * the broadcast pipeline can be in. Without this distinction, callers cannot - * tell apart "user explicitly chose Private" (silent drops are correct) from - * "publisher state was lost on tab refresh" (silent drops are a bug, P14-001) - * from "publisher initialized but caller has not configured intent yet." - * - * - 'unconfigured': initial state after init OR after a tab refresh wiped - * module memory. Drops in this mode trigger a one-shot warning so operators - * notice broadcasts vanishing silently. - * - 'private': user explicitly picked "Private (don't publish)" at workout - * start. Drops in this mode are expected and silent. - * - 'broadcasting': user picked a gym at workout start. Drops in this mode - * indicate something is wrong (channel died, client disappeared) and - * trigger a one-shot warning. - * - * This will be subsumed by the discriminated `PublisherState` ADR-013, but - * the distinction is needed today to fix the silent-downgrade bug class. - */ -export type PublisherMode = 'unconfigured' | 'private' | 'broadcasting' - -let _client: SupabaseClient | null = null -let _channel: RealtimeChannel | null = null -let _activeGymId: string | null = null -let _channelGymId: string | null = null -let _helloResponder: (() => void) | null = null -let _publisherMode: PublisherMode = 'unconfigured' -let _silentDropWarned = false -// P14-015: track consecutive terminal-status events on the same channel. -// After three failures on the same gym, escalate the log level so a -// flapping channel is loud in production logs. -let _terminalFailureCount = 0 -const TERMINAL_FAILURE_ESCALATE_AFTER = 3 -// P14-013: track consecutive removeChannel failures across all teardown -// paths. After 5 in a row, escalate from warn to error so a broken -// supabase client surface area is loud in production logs. -let _removeChannelFailureCount = 0 -const REMOVE_CHANNEL_ESCALATE_AFTER = 5 - -/** - * Wrap `_client.removeChannel(channel)` with a counted failure log so we - * have one place to track removeChannel failures across the three teardown - * paths (gym switch in ensureChannel, gym switch in configureDisplayPublisher, - * channel terminal status, destroy). P14-013 escalation lives here. - */ -function removeChannelSafe(channel: RealtimeChannel, context: string): void { - if (!_client) return - try { - _client.removeChannel(channel) - _removeChannelFailureCount = 0 - } catch (err) { - _removeChannelFailureCount++ - const escalate = _removeChannelFailureCount >= REMOVE_CHANNEL_ESCALATE_AFTER - const logFn = escalate ? console.error : console.warn - logFn( - `[display-publisher] removeChannel failed in ${context} ` + - `(consecutive=${_removeChannelFailureCount}${escalate ? ', escalated' : ''}):`, - err, - ) - } -} - -// --------------------------------------------------------------------------- -// Helpers -// --------------------------------------------------------------------------- - -/** - * Lazily creates and subscribes the broadcast channel on first use for the - * currently-active gym ID. Returns the channel, or null if the client is not - * initialized or there is no active gym (Private workout OR publisher was - * never configured -- in the latter case `maybeLogSilentDrop` in the public - * entrypoints will log the drop before the caller sees the null). - * - * If the active gym ID has changed since the last call (i.e., the cached - * channel was bound to a different gym), this helper tears down the stale - * channel and creates a fresh one for the new gym. - */ -function ensureChannel(): RealtimeChannel | null { - if (!_client) { - // P15-015: In broadcasting mode the client should never be null (it is - // cleared on teardown, and broadcasting mode should have been cleared - // first). Log an error if this invariant is ever violated -- this is a - // logic-bug signal, not a routine silent-drop. - if (_publisherMode === 'broadcasting') { - console.error('[display-publisher] Invariant violation: broadcasting mode with null client') - } - return null - } - if (_activeGymId === null) return null - - // If the cached channel is bound to a different gym, tear it down so we can - // create a new channel for the new gym on this call. - if (_channel && _channelGymId !== _activeGymId) { - removeChannelSafe(_channel, 'ensureChannel/gym-switch') - _channel = null - _channelGymId = null - } - - if (_channel) return _channel - - const channelName = getGymChannelName(_activeGymId) - _channel = _client.channel(channelName, { - config: { broadcast: { ack: false, self: false } }, - }) - _channelGymId = _activeGymId - - _channel.on('broadcast', { event: 'display_hello' }, () => { - if (!_helloResponder) { - // P14-042: hello arrived during teardown (responder was cleared but - // the channel hasn't been removed yet). Log at info so test logs and - // prod traces are honest about the race. - console.info( - '[display-publisher] Hello received with no responder; ignored (likely teardown race)', - ) - return - } - // P15-005: Wrap responder invocation in try/catch -- the responder may - // call republishCurrentState() which can throw, and Supabase Realtime - // swallows exceptions inside `on` callbacks in some versions. Logging - // here ensures the failure is traceable instead of silently lost. - try { - _helloResponder() - } catch (err) { - console.error('[display-publisher] Hello responder threw:', err) - } - }) - - _channel.subscribe((status, err) => { - if (status === 'CLOSED') { - // CLOSED is a normal teardown path -- log at info level so we have a - // breadcrumb but do not paint it as an error. Still call removeChannel - // so the supabase client releases its reference (P14-008). - console.info('[display-publisher] Channel closed (normal teardown)') - if (_channel) removeChannelSafe(_channel, 'subscribe/CLOSED') - _channel = null - _channelGymId = null - } else if (status === 'TIMED_OUT' || status === 'CHANNEL_ERROR') { - // TIMED_OUT and CHANNEL_ERROR are unexpected terminal states. Log at - // warn for the first few failures, then escalate to error so a - // flapping channel is loud in production logs (P14-015). - // P14-008: explicitly call removeChannel so the supabase client does - // not retain a reference to the dead channel. Without this, a workout - // that bounces network connectivity a few times leaks several dead - // channels for the lifetime of the active workout. - _terminalFailureCount++ - const escalate = _terminalFailureCount >= TERMINAL_FAILURE_ESCALATE_AFTER - const logFn = escalate ? console.error : console.warn - logFn( - `[display-publisher] Channel terminal status: ${status} ` + - `(consecutive=${_terminalFailureCount}${escalate ? ', escalated' : ''})`, - err, - ) - if (_channel) removeChannelSafe(_channel, `subscribe/${status}`) - _channel = null - _channelGymId = null - } else if (status === 'SUBSCRIBED') { - // Reset the terminal-failure counter on a successful subscribe so a - // recovered channel does not carry stale escalation state. - _terminalFailureCount = 0 - } - }) - - return _channel -} - -// --------------------------------------------------------------------------- -// Silent-drop logging (P14-001 / P14-002) -// --------------------------------------------------------------------------- - -/** - * Logs the first silent drop per session whenever broadcast was expected - * (publisher mode is 'unconfigured' or 'broadcasting' but the gym/client - * check still failed). Stays silent in 'private' mode where drops are - * intentional. Logs once per session to avoid spamming the console with one - * warning per published set. - * - * Reset by `configureDisplayPublisher` and `destroyDisplayPublisher` so a - * fresh workout gets a fresh warning budget. - */ -function maybeLogSilentDrop(eventName: string): void { - if (_silentDropWarned) return - if (_publisherMode === 'private') return - _silentDropWarned = true - if (_publisherMode === 'unconfigured') { - console.warn( - `[display-publisher] Dropped ${eventName}: publisher unconfigured. ` + - 'This commonly indicates the page was refreshed mid-workout, which wipes the in-memory ' + - 'gym selection. Subsequent broadcasts will also be dropped silently. ' + - 'Restart the workout to re-establish broadcasting. Known v1 limitation; ' + - 'see ADR-013 for the type-safe fix.', - ) - } else { - console.warn( - `[display-publisher] Dropped ${eventName}: client missing or no active gym ` + - `while in mode=${_publisherMode}. This is unexpected; further drops will be silent.`, - ) - } -} - -// --------------------------------------------------------------------------- -// Public API -// --------------------------------------------------------------------------- - -/** - * Store the Supabase client reference for later channel creation. - * Does NOT create the channel yet -- that happens lazily on first publish. - */ -export function initDisplayPublisher(client: SupabaseClient): void { - _client = client -} - -/** - * Configure the publisher with the gym ID the current workout is being - * broadcast to and the explicit caller intent. - * - * `intent` is required so the publisher can distinguish "explicit Private" - * (silent drops are correct) from "publisher unconfigured / refresh case" - * (silent drops are a bug worth warning about). See `PublisherMode` docs. - * - * - `intent: 'broadcasting'` requires a non-null `gymId`. Sets the active gym - * and switches the publisher into broadcasting mode. - * - `intent: 'private'` requires `gymId === null`. Switches the publisher - * into private mode -- subsequent publish calls drop silently. - * - * If the gym ID changes between calls and a channel already exists, the - * stale channel is torn down so the next publish creates a fresh channel - * bound to the new gym. - */ -export function configureDisplayPublisher({ - gymId, - intent, -}: { - gymId: string | null - intent: 'broadcasting' | 'private' -}): void { - // Validate the gymId/intent combination at the boundary so future callers - // cannot bypass the state machine. Per .claude/rules/state-management.md, - // module-state setters should validate at their own boundary. - if (intent === 'broadcasting' && gymId === null) { - console.error( - '[display-publisher] configureDisplayPublisher called with intent=broadcasting but gymId=null; ignoring', - ) - return - } - if (intent === 'private' && gymId !== null) { - console.error( - `[display-publisher] configureDisplayPublisher called with intent=private but gymId=${gymId}; ignoring`, - ) - return - } - - // No-op when nothing actually changes (avoids redundant log spam and - // unnecessary channel teardowns when an effect re-runs with stable values). - if (_publisherMode === intent && _activeGymId === gymId) return - - // Breadcrumb so production debugging "why did this workout publish to the - // wrong gym?" has a traceable entry point. The actual broadcast path is - // fire-and-forget so this is the best place to record intent. - console.info( - `[display-publisher] Mode change: ${_publisherMode} -> ${intent} ` + - `(gym ${_activeGymId ?? 'none'} -> ${gymId ?? 'none'})`, - ) - - _activeGymId = gymId - _publisherMode = intent - // Reset the silent-drop warning budget so the next workout can warn fresh - // if drops happen. - _silentDropWarned = false - - // If the cached channel belongs to the old gym, tear it down eagerly so the - // next publish creates a fresh channel for the new gym. We also handle this - // defensively in ensureChannel, but eager teardown here avoids a lingering - // subscription in the rare W5 path. - if (_channel && _channelGymId !== _activeGymId) { - removeChannelSafe(_channel, 'configureDisplayPublisher/gym-switch') - _channel = null - _channelGymId = null - } -} - -/** - * Broadcast a full workout snapshot to the active gym's display channel. - * Fire-and-forget: errors are logged, not thrown. - */ -export function publishDisplaySnapshot(snapshot: DisplaySnapshot): void { - if (!_client || _activeGymId === null) { - maybeLogSilentDrop('workout_snapshot') - return - } - - const channel = ensureChannel() - if (!channel) { - maybeLogSilentDrop('workout_snapshot') - return - } - - channel - .send({ type: 'broadcast', event: 'workout_snapshot', payload: snapshot }) - .catch((err: unknown) => { - console.error('[display-publisher] Failed to send workout_snapshot', err) - }) -} - -/** - * Broadcast that the user's workout session has ended. - * Fire-and-forget: errors are logged, not thrown. - */ -export function publishSessionEnded(userId: string): void { - if (!_client || _activeGymId === null) { - maybeLogSilentDrop('session_ended') - return - } - - const channel = ensureChannel() - if (!channel) { - maybeLogSilentDrop('session_ended') - return - } - - channel - .send({ type: 'broadcast', event: 'session_ended', payload: { user_id: userId } }) - .catch((err: unknown) => { - console.error('[display-publisher] Failed to send session_ended', err) - }) -} - -/** - * Broadcast a focus event -- tells remote displays to highlight this user. - * Fire-and-forget: errors are logged, not thrown. - */ -export function publishFocusEvent(userId: string): void { - if (!_client || _activeGymId === null) { - maybeLogSilentDrop('focus') - return - } - - const channel = ensureChannel() - if (!channel) { - maybeLogSilentDrop('focus') - return - } - - channel - .send({ type: 'broadcast', event: 'focus', payload: { user_id: userId } }) - .catch((err: unknown) => { - console.error('[display-publisher] Failed to send focus', err) - }) -} - -/** - * Broadcast an unfocus event -- tells remote displays to clear the highlight. - * Fire-and-forget: errors are logged, not thrown. - */ -export function publishUnfocusEvent(): void { - if (!_client || _activeGymId === null) { - maybeLogSilentDrop('unfocus') - return - } - - const channel = ensureChannel() - if (!channel) { - maybeLogSilentDrop('unfocus') - return - } - - channel.send({ type: 'broadcast', event: 'unfocus', payload: {} }).catch((err: unknown) => { - console.error('[display-publisher] Failed to send unfocus', err) - }) -} - -/** - * Register a callback to invoke when a display_hello event is received. - * Pass null to deregister the current responder. - */ -export function setHelloResponder(fn: (() => void) | null): void { - _helloResponder = fn -} - -/** - * Check whether the publisher has a working client and an active gym. - * Used by the hook to reflect true broadcast readiness in the UI. - */ -export function isPublisherReady(): boolean { - return _client !== null && _activeGymId !== null -} - -/** - * Return the gym ID the publisher is currently configured to broadcast to, or - * null when the publisher is in Private mode (or has not been configured yet). - * Consumed by the active-workout header label component (F018 D12). - */ -export function getActiveGymId(): string | null { - return _activeGymId -} - -/** - * Return the publisher's current operational mode. Useful for callers that - * need to distinguish "explicit Private" from "publisher unconfigured / - * refresh case" without inspecting `_activeGymId` directly. - */ -export function getPublisherMode(): PublisherMode { - return _publisherMode -} - -/** - * Tear down the broadcast channel and reset all module-scope state. - */ -export function destroyDisplayPublisher(): void { - if (_channel) removeChannelSafe(_channel, 'destroyDisplayPublisher') - - _channel = null - _channelGymId = null - _client = null - _activeGymId = null - _helloResponder = null - _publisherMode = 'unconfigured' - _silentDropWarned = false - _terminalFailureCount = 0 - _removeChannelFailureCount = 0 -} diff --git a/src/lib/display-realtime.ts b/src/lib/display-realtime.ts new file mode 100644 index 00000000..a2b3ce63 --- /dev/null +++ b/src/lib/display-realtime.ts @@ -0,0 +1,231 @@ +import type { SupabaseClient } from '@supabase/supabase-js' +import type { RealtimeChannel } from '@supabase/realtime-js' +import { + displaySnapshotSchema, + idleSnapshotSchema, + type DisplaySnapshot, + type DisplayConnectionStatus, + type IdleSnapshot, +} from '@/domain/types/display-snapshot' +import { z } from 'zod' +import { getGymChannelName } from '@/lib/gym-channel' + +export interface DisplayEventHandlers { + onSnapshot: (snapshot: DisplaySnapshot) => void + onSessionEnded: (payload: { user_id: string }) => void + onFocus: (payload: { user_id: string }) => void + onUnfocus: () => void + onIdleSnapshot: (snapshot: IdleSnapshot) => void + onStatusChange: (status: DisplayConnectionStatus) => void +} +export interface SubscribeToDisplayArgs { + gymId: string + handlers: DisplayEventHandlers +} + +const L = '[display-realtime]' +const userIdPayload = z.object({ user_id: z.string() }) +const BC = { config: { broadcast: { ack: false, self: false } } } as const + +function removeSafe(client: SupabaseClient | null, ch: RealtimeChannel, ctx: string): void { + if (!client) return + try { + client.removeChannel(ch) + } catch (err) { + console.error(`${L} removeChannel failed in ${ctx}:`, err) + } +} + +function validated(schema: z.ZodType, p: unknown, ev: string, cb: (d: T) => void) { + const r = schema.safeParse(p) + if (r.success) cb(r.data) + else console.warn(`${L} Invalid ${ev} payload, dropping`, r.error) +} + +let _client: SupabaseClient | null = null +let _pubChannel: RealtimeChannel | null = null +let _pubGymId: string | null = null +let _pubHelloResponder: (() => void) | null = null +let _pubConfigured = false + +function teardownPubChannel(): void { + if (_pubChannel) { + removeSafe(_client, _pubChannel, 'pubTeardown') + _pubChannel = null + } +} + +function ensurePubChannel(): RealtimeChannel | null { + if (!_client || _pubGymId === null) return null + if (_pubChannel) return _pubChannel + _pubChannel = _client.channel(getGymChannelName(_pubGymId), BC) + _pubChannel.on('broadcast', { event: 'display_hello' }, () => { + if (!_pubHelloResponder) return + try { + _pubHelloResponder() + } catch (err) { + console.error(`${L} responder threw:`, err) + } + }) + _pubChannel.subscribe((status, err) => { + if (status === 'CLOSED' || status === 'TIMED_OUT' || status === 'CHANNEL_ERROR') { + ;(status === 'CLOSED' ? console.info : console.error)(`${L} Pub channel ${status}`, err ?? '') + teardownPubChannel() + } + }) + return _pubChannel +} + +function pubSend(event: string, payload: Record): void { + const ch = ensurePubChannel() + if (!ch) { + if (_pubConfigured && _pubGymId !== null) + console.warn(`${L} Dropped ${event}: channel unavailable while broadcasting`) + return + } + ch.send({ type: 'broadcast', event, payload }).catch((err: unknown) => { + console.error(`${L} Failed to send ${event}:`, err) + }) +} + +export function initDisplayPublisher(client: SupabaseClient): void { + _client = client +} + +type PubConfig = { gymId: string | null; intent: 'broadcasting' | 'private' } + +export function configureDisplayPublisher({ gymId, intent }: PubConfig): void { + if (intent === 'broadcasting' && gymId === null) { + console.error(`${L} configure: broadcasting requires gymId`) + return + } + if (intent === 'private' && gymId !== null) { + console.error(`${L} configure: private requires gymId=null, got ${gymId}`) + return + } + const next = intent === 'broadcasting' + if (_pubConfigured === next && _pubGymId === gymId) return + console.info(`${L} ${_pubConfigured ? 'broadcasting' : 'off'} -> ${intent}`) + if (gymId !== _pubGymId) teardownPubChannel() + _pubGymId = gymId + _pubConfigured = next +} + +export function publishDisplaySnapshot(snapshot: DisplaySnapshot): void { + pubSend('workout_snapshot', snapshot as unknown as Record) +} +export function publishSessionEnded(userId: string): void { + pubSend('session_ended', { user_id: userId }) +} +export function publishFocusEvent(userId: string): void { + pubSend('focus', { user_id: userId }) +} +export function publishUnfocusEvent(): void { + pubSend('unfocus', {}) +} +export function setHelloResponder(fn: (() => void) | null): void { + _pubHelloResponder = fn +} +export function isPublisherReady(): boolean { + return _client !== null && _pubGymId !== null +} +export function getActiveGymId(): string | null { + return _pubGymId +} + +export function destroyDisplayPublisher(): void { + teardownPubChannel() + _pubGymId = null + _pubHelloResponder = null + _pubConfigured = false + if (!_subChannel) _client = null +} + +let _subChannel: RealtimeChannel | null = null +let _subStatus: DisplayConnectionStatus = 'disconnected' +let _subRetryTimer: ReturnType | null = null +let _subRetryAttempt = 0 +let _subConnectedBefore = false + +function scheduleRetry(gymId: string, handlers: DisplayEventHandlers): void { + const delay = Math.min(2_000 * 2 ** _subRetryAttempt, 30_000) + _subRetryAttempt++ + console.info(`${L} Reconnecting in ${delay}ms (attempt ${_subRetryAttempt})`) + _subRetryTimer = setTimeout(() => { + _subRetryTimer = null + if (_client) subscribeToDisplay({ gymId, handlers }) + }, delay) +} + +export function initDisplaySubscriber(client: SupabaseClient): void { + _client = client +} + +export function subscribeToDisplay({ gymId, handlers }: SubscribeToDisplayArgs): void { + if (!_client) + throw new Error(`${L} Cannot subscribe: no client. Call initDisplaySubscriber first.`) + if (_subChannel) { + removeSafe(_client, _subChannel, 'sub/existing') + _subChannel = null + } + _subChannel = _client.channel(getGymChannelName(gymId), BC) + _subChannel + .on('broadcast', { event: 'workout_snapshot' }, ({ payload }) => + validated(displaySnapshotSchema, payload, 'workout_snapshot', handlers.onSnapshot), + ) + .on('broadcast', { event: 'session_ended' }, ({ payload }) => + validated(userIdPayload, payload, 'session_ended', handlers.onSessionEnded), + ) + .on('broadcast', { event: 'focus' }, ({ payload }) => + validated(userIdPayload, payload, 'focus', handlers.onFocus), + ) + .on('broadcast', { event: 'idle_snapshot' }, ({ payload }) => + validated(idleSnapshotSchema, payload, 'idle_snapshot', handlers.onIdleSnapshot), + ) + .on('broadcast', { event: 'unfocus' }, () => handlers.onUnfocus()) + .subscribe((status, err) => { + if (status === 'SUBSCRIBED') { + _subStatus = 'connected' + _subRetryAttempt = 0 + if (_subConnectedBefore) publishHello() + _subConnectedBefore = true + handlers.onStatusChange('connected') + } else if (status === 'TIMED_OUT' || status === 'CHANNEL_ERROR' || status === 'CLOSED') { + console.warn(`${L} Subscriber channel ${status}`, err) + _subStatus = 'reconnecting' + handlers.onStatusChange('reconnecting') + if (_subChannel) removeSafe(_client, _subChannel, `subSub/${status}`) + _subChannel = null + scheduleRetry(gymId, handlers) + } + }) +} + +export function publishHello(): void { + if (!_subChannel) { + console.warn(`${L} publishHello: no subscriber channel`) + return + } + _subChannel + .send({ type: 'broadcast', event: 'display_hello', payload: {} }) + .catch((err: unknown) => { + console.error(`${L} Failed to send display_hello:`, err) + }) +} + +export function getSubscriberStatus(): DisplayConnectionStatus { + return _subStatus +} + +export function destroyDisplaySubscriber(): void { + if (_subRetryTimer !== null) { + clearTimeout(_subRetryTimer) + _subRetryTimer = null + } + if (_subChannel) removeSafe(_client, _subChannel, 'destroySub') + _subChannel = null + _subStatus = 'disconnected' + _subRetryAttempt = 0 + _subConnectedBefore = false + if (!_pubChannel && _pubGymId === null) _client = null +} diff --git a/src/lib/display-subscriber.ts b/src/lib/display-subscriber.ts deleted file mode 100644 index 9c59acbf..00000000 --- a/src/lib/display-subscriber.ts +++ /dev/null @@ -1,232 +0,0 @@ -import type { SupabaseClient } from '@supabase/supabase-js' -import type { RealtimeChannel } from '@supabase/realtime-js' -import { - displaySnapshotSchema, - idleSnapshotSchema, - type DisplaySnapshot, - type DisplayConnectionStatus, - type IdleSnapshot, -} from '@/domain/types/display-snapshot' -import { z } from 'zod' -import { getGymChannelName } from '@/lib/gym-channel' - -// --------------------------------------------------------------------------- -// Types -// --------------------------------------------------------------------------- - -export interface DisplayEventHandlers { - onSnapshot: (snapshot: DisplaySnapshot) => void - onSessionEnded: (payload: { user_id: string }) => void - onFocus: (payload: { user_id: string }) => void - onUnfocus: () => void - onIdleSnapshot: (snapshot: IdleSnapshot) => void - onStatusChange: (status: DisplayConnectionStatus) => void -} - -export interface SubscribeToDisplayArgs { - gymId: string - handlers: DisplayEventHandlers -} - -// --------------------------------------------------------------------------- -// Module-scope state -// --------------------------------------------------------------------------- - -let _client: SupabaseClient | null = null -let _channel: RealtimeChannel | null = null -let _status: DisplayConnectionStatus = 'disconnected' -let _retryTimer: ReturnType | null = null -let _retryAttempt = 0 -let _hasConnectedBefore = false - -const RETRY_BASE_MS = 2_000 -const RETRY_MAX_MS = 30_000 - -// P15-012: Mirror `display-publisher.ts::removeChannelSafe` so subscriber -// teardown paths cannot propagate a supabase throw. Each module keeps its -// own failure counter so flapping in one surface doesn't silently reset the -// other. Escalate warn -> error after 5 consecutive failures. -let _removeChannelFailureCount = 0 -const REMOVE_CHANNEL_ESCALATE_AFTER = 5 - -function removeChannelSafe(channel: RealtimeChannel, context: string): void { - if (!_client) return - try { - _client.removeChannel(channel) - _removeChannelFailureCount = 0 - } catch (err) { - _removeChannelFailureCount++ - const escalate = _removeChannelFailureCount >= REMOVE_CHANNEL_ESCALATE_AFTER - const logFn = escalate ? console.error : console.warn - logFn( - `[display-subscriber] removeChannel failed in ${context} ` + - `(consecutive=${_removeChannelFailureCount}${escalate ? ', escalated' : ''}):`, - err, - ) - } -} - -// --------------------------------------------------------------------------- -// Inline schemas for lightweight payload validation -// --------------------------------------------------------------------------- - -const userIdPayloadSchema = z.object({ user_id: z.string() }) - -// --------------------------------------------------------------------------- -// Public API -// --------------------------------------------------------------------------- - -/** - * Store the Supabase client reference for later channel creation. - * Does NOT create the channel yet -- that happens in subscribeToDisplay. - */ -export function initDisplaySubscriber(client: SupabaseClient): void { - _client = client -} - -/** - * Create the broadcast channel for the given gym, register listeners for each - * event type, and subscribe. Validates all incoming payloads with Zod before - * dispatching. - * - * On reconnect (after a terminal channel status), the subscriber automatically - * re-subscribes to the same gym. The `gymId` is captured in the closure so - * retries do not leak to a different gym. - */ -export function subscribeToDisplay({ gymId, handlers }: SubscribeToDisplayArgs): void { - if (!_client) { - // P14-004: throw instead of warn-and-return so the route's outer - // try/catch can map this into a `subscribe-failed` BootError with a - // visible Retry button. Returning silently here puts the route into a - // "stuck reconnecting" state with no recovery affordance for the user. - throw new Error( - '[display-subscriber] Cannot subscribe: client not initialized. ' + - 'Call initDisplaySubscriber(client) before subscribeToDisplay().', - ) - } - - // Tear down any existing channel before creating a new one - if (_channel) { - removeChannelSafe(_channel, 'subscribeToDisplay/existing') - _channel = null - } - - const channelName = getGymChannelName(gymId) - _channel = _client.channel(channelName, { - config: { broadcast: { ack: false, self: false } }, - }) - - _channel - .on('broadcast', { event: 'workout_snapshot' }, ({ payload }) => { - const result = displaySnapshotSchema.safeParse(payload) - if (result.success) { - handlers.onSnapshot(result.data) - } else { - console.warn( - '[display-subscriber] Invalid workout_snapshot payload, dropping', - result.error, - ) - } - }) - .on('broadcast', { event: 'session_ended' }, ({ payload }) => { - const result = userIdPayloadSchema.safeParse(payload) - if (result.success) { - handlers.onSessionEnded(result.data) - } else { - console.warn('[display-subscriber] Invalid session_ended payload, dropping', result.error) - } - }) - .on('broadcast', { event: 'focus' }, ({ payload }) => { - const result = userIdPayloadSchema.safeParse(payload) - if (result.success) { - handlers.onFocus(result.data) - } else { - console.warn('[display-subscriber] Invalid focus payload, dropping', result.error) - } - }) - .on('broadcast', { event: 'idle_snapshot' }, ({ payload }) => { - const result = idleSnapshotSchema.safeParse(payload) - if (result.success) { - handlers.onIdleSnapshot(result.data) - } else { - console.error('[display-subscriber] Invalid idle_snapshot payload, dropping', result.error) - } - }) - .on('broadcast', { event: 'unfocus' }, () => { - handlers.onUnfocus() - }) - .subscribe((status, err) => { - if (status === 'SUBSCRIBED') { - _status = 'connected' - _retryAttempt = 0 - if (_hasConnectedBefore) { - publishHello() - } - _hasConnectedBefore = true - handlers.onStatusChange('connected') - } else if (status === 'TIMED_OUT' || status === 'CHANNEL_ERROR' || status === 'CLOSED') { - console.warn(`[display-subscriber] Channel terminal status: ${status}`, err) - _status = 'reconnecting' - handlers.onStatusChange('reconnecting') - - // Remove the dead channel before clearing the reference (P6-002) - if (_channel && _client) { - removeChannelSafe(_channel, `subscribe/${status}`) - } - _channel = null - - const delay = Math.min(RETRY_BASE_MS * 2 ** _retryAttempt, RETRY_MAX_MS) - _retryAttempt++ - console.info(`[display-subscriber] Reconnecting in ${delay}ms (attempt ${_retryAttempt})`) - _retryTimer = setTimeout(() => { - _retryTimer = null - // Re-subscribe using the same gymId captured in this closure so the - // retry cannot leak to a different gym channel. - if (_client) subscribeToDisplay({ gymId, handlers }) - }, delay) - } - }) -} - -/** - * Send a display_hello broadcast to signal presence on the channel. - * Fire-and-forget: errors are logged, not thrown. - */ -export function publishHello(): void { - if (!_channel) return - - _channel - .send({ type: 'broadcast', event: 'display_hello', payload: {} }) - .catch((err: unknown) => { - console.error('[display-subscriber] Failed to send display_hello', err) - }) -} - -/** - * Return the current subscriber connection status. - */ -export function getSubscriberStatus(): DisplayConnectionStatus { - return _status -} - -/** - * Tear down the broadcast channel, cancel any pending reconnect, and reset - * all module-scope state. - */ -export function destroyDisplaySubscriber(): void { - if (_retryTimer !== null) { - clearTimeout(_retryTimer) - _retryTimer = null - } - - if (_channel && _client) { - removeChannelSafe(_channel, 'destroyDisplaySubscriber') - } - - _channel = null - _client = null - _status = 'disconnected' - _retryAttempt = 0 - _hasConnectedBefore = false - _removeChannelFailureCount = 0 -} diff --git a/src/routes/_authenticated/__tests__/-start-workout-flow.test.tsx b/src/routes/_authenticated/__tests__/-start-workout-flow.test.tsx index 1a9a5445..16d455a6 100644 --- a/src/routes/_authenticated/__tests__/-start-workout-flow.test.tsx +++ b/src/routes/_authenticated/__tests__/-start-workout-flow.test.tsx @@ -127,7 +127,7 @@ vi.mock('@/hooks/use-gym-picker', () => ({ }), })) -vi.mock('@/lib/display-publisher', () => ({ +vi.mock('@/lib/display-realtime', () => ({ configureDisplayPublisher: mockConfigureDisplayPublisher, })) diff --git a/src/routes/_authenticated/index.tsx b/src/routes/_authenticated/index.tsx index aecffbf8..06f68130 100644 --- a/src/routes/_authenticated/index.tsx +++ b/src/routes/_authenticated/index.tsx @@ -21,7 +21,7 @@ import { Button } from '@/components/ui/button' import { Skeleton } from '@/components/ui/skeleton' import { useNextUpcomingEvent } from '@/hooks/use-event-items' import { formatDuration } from '@/lib/format-duration' -import { configureDisplayPublisher } from '@/lib/display-publisher' +import { configureDisplayPublisher } from '@/lib/display-realtime' import { writeLastGymChoice, readLastGymChoice } from '@/lib/gym-picker-storage' import type { WorkoutLog } from '@/domain/types' import type { ProgramFull } from '@/lib/data-adapter' diff --git a/src/routes/_authenticated/log.$workoutId.tsx b/src/routes/_authenticated/log.$workoutId.tsx index fa258154..7b6061d8 100644 --- a/src/routes/_authenticated/log.$workoutId.tsx +++ b/src/routes/_authenticated/log.$workoutId.tsx @@ -12,7 +12,7 @@ import { OnboardingHint } from '@/components/onboarding/onboarding-hint' import { useProgramFull } from '@/hooks/use-programs' import { detectPersonalRecords } from '@/lib/pr-detection' import { useDisplayBroadcast } from '@/hooks/use-display-broadcast' -import { getActiveGymId } from '@/lib/display-publisher' +import { getActiveGymId } from '@/lib/display-realtime' import { WorkoutHeader } from '@/components/workout/workout-header' import { WorkoutPausedBar } from '@/components/workout/workout-paused-bar' import { ErrorBanner } from '@/components/workout/error-banner' diff --git a/src/routes/display/__tests__/-gym-route.test.tsx b/src/routes/display/__tests__/-gym-route.test.tsx index 89f07956..51cc6bb6 100644 --- a/src/routes/display/__tests__/-gym-route.test.tsx +++ b/src/routes/display/__tests__/-gym-route.test.tsx @@ -98,7 +98,7 @@ vi.mock('@/lib/config-store', () => ({ resolveConfig: mockResolveConfig, })) -vi.mock('@/lib/display-subscriber', () => ({ +vi.mock('@/lib/display-realtime', () => ({ initDisplaySubscriber: mockInitDisplaySubscriber, subscribeToDisplay: mockSubscribeToDisplay, destroyDisplaySubscriber: mockDestroyDisplaySubscriber, diff --git a/src/routes/display/gym/$gymId.tsx b/src/routes/display/gym/$gymId.tsx index 2de7a68a..7ae4981f 100644 --- a/src/routes/display/gym/$gymId.tsx +++ b/src/routes/display/gym/$gymId.tsx @@ -9,7 +9,7 @@ import { subscribeToDisplay, destroyDisplaySubscriber, type DisplayEventHandlers, -} from '@/lib/display-subscriber' +} from '@/lib/display-realtime' import { useDisplayStore, getDisplayMode } from '@/stores/display-store' import { IdleView } from '@/components/display/idle-view' import { BoardView } from '@/components/display/board-view' diff --git a/src/stores/active-workout-store.ts b/src/stores/active-workout-store.ts index 428385f2..0211e5e2 100644 --- a/src/stores/active-workout-store.ts +++ b/src/stores/active-workout-store.ts @@ -15,7 +15,7 @@ import { noteContentSchema } from '@/domain/types' import { UNDO_WINDOW_MS } from '@/lib/workout-utils' import type { SnapshotContext } from '@/lib/display-snapshot' import { buildDisplaySnapshot } from '@/lib/display-snapshot' -import { publishDisplaySnapshot, publishSessionEnded } from '@/lib/display-publisher' +import { publishDisplaySnapshot, publishSessionEnded } from '@/lib/display-realtime' // --------------------------------------------------------------------------- // Nested state: groups contain activities, activities contain sets From 79c76db1f876ae01bb8ff82ac96ff7141397ec7d Mon Sep 17 00:00:00 2001 From: Robert Hamilton Date: Sun, 12 Apr 2026 18:41:04 -0500 Subject: [PATCH 2/5] fix(display): resolve PR review findings P16 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - P16-001: null-client branch in scheduleRetry stops reconnect loop, calls onStatusChange('disconnected') instead of silently abandoning - P16-002: clearTimeout pending retry before re-subscribing prevents stale-gymId ghost subscription on useEffect re-run - P16-004: removeSafe logs warn when client is null instead of silent return - P16-005: unfocus broadcast handler wrapped in try-catch matching the display_hello responder pattern — prevents throw propagating into Supabase - P16-006: subscriber CHANNEL_ERROR/TIMED_OUT logged at error, CLOSED at info, mirroring the publisher's existing conditional - P16-007: empty-string gymId guard added to configureDisplayPublisher and subscribeToDisplay — prevents display:gym: channel name - P16-011: validated() logs r.error.toString() + raw payload instead of raw ZodError instance — serializes correctly in Sentry/Datadog - P16-014: pubSend widened to unknown payload, removes double-cast in publishDisplaySnapshot Review: Context/Reviews/0016-pr106-simplify-display-subsystem-2026-04-12.md --- src/lib/display-realtime.ts | 50 +++++++++++++++++++++++++++++-------- 1 file changed, 40 insertions(+), 10 deletions(-) diff --git a/src/lib/display-realtime.ts b/src/lib/display-realtime.ts index a2b3ce63..3e49a286 100644 --- a/src/lib/display-realtime.ts +++ b/src/lib/display-realtime.ts @@ -28,7 +28,10 @@ const userIdPayload = z.object({ user_id: z.string() }) const BC = { config: { broadcast: { ack: false, self: false } } } as const function removeSafe(client: SupabaseClient | null, ch: RealtimeChannel, ctx: string): void { - if (!client) return + if (!client) { + console.warn(`${L} removeSafe(${ctx}): client is null, channel may be leaked`) + return + } try { client.removeChannel(ch) } catch (err) { @@ -39,7 +42,7 @@ function removeSafe(client: SupabaseClient | null, ch: RealtimeChannel, ctx: str function validated(schema: z.ZodType, p: unknown, ev: string, cb: (d: T) => void) { const r = schema.safeParse(p) if (r.success) cb(r.data) - else console.warn(`${L} Invalid ${ev} payload, dropping`, r.error) + else console.warn(`${L} Invalid ${ev} payload (dropping):`, r.error.toString(), p) } let _client: SupabaseClient | null = null @@ -76,16 +79,18 @@ function ensurePubChannel(): RealtimeChannel | null { return _pubChannel } -function pubSend(event: string, payload: Record): void { +function pubSend(event: string, payload: unknown): void { const ch = ensurePubChannel() if (!ch) { if (_pubConfigured && _pubGymId !== null) console.warn(`${L} Dropped ${event}: channel unavailable while broadcasting`) return } - ch.send({ type: 'broadcast', event, payload }).catch((err: unknown) => { - console.error(`${L} Failed to send ${event}:`, err) - }) + ch.send({ type: 'broadcast', event, payload: payload as Record }).catch( + (err: unknown) => { + console.error(`${L} Failed to send ${event}:`, err) + }, + ) } export function initDisplayPublisher(client: SupabaseClient): void { @@ -103,6 +108,10 @@ export function configureDisplayPublisher({ gymId, intent }: PubConfig): void { console.error(`${L} configure: private requires gymId=null, got ${gymId}`) return } + if (gymId !== null && gymId.trim() === '') { + console.error(`${L} configure: gymId must not be empty`) + return + } const next = intent === 'broadcasting' if (_pubConfigured === next && _pubGymId === gymId) return console.info(`${L} ${_pubConfigured ? 'broadcasting' : 'off'} -> ${intent}`) @@ -112,7 +121,7 @@ export function configureDisplayPublisher({ gymId, intent }: PubConfig): void { } export function publishDisplaySnapshot(snapshot: DisplaySnapshot): void { - pubSend('workout_snapshot', snapshot as unknown as Record) + pubSend('workout_snapshot', snapshot) } export function publishSessionEnded(userId: string): void { pubSend('session_ended', { user_id: userId }) @@ -153,7 +162,13 @@ function scheduleRetry(gymId: string, handlers: DisplayEventHandlers): void { console.info(`${L} Reconnecting in ${delay}ms (attempt ${_subRetryAttempt})`) _subRetryTimer = setTimeout(() => { _subRetryTimer = null - if (_client) subscribeToDisplay({ gymId, handlers }) + if (_client) { + subscribeToDisplay({ gymId, handlers }) + } else { + console.error(`${L} Retry fired but client is null — stopping reconnect loop`) + _subStatus = 'disconnected' + handlers.onStatusChange('disconnected') + } }, delay) } @@ -164,6 +179,12 @@ export function initDisplaySubscriber(client: SupabaseClient): void { export function subscribeToDisplay({ gymId, handlers }: SubscribeToDisplayArgs): void { if (!_client) throw new Error(`${L} Cannot subscribe: no client. Call initDisplaySubscriber first.`) + if (!gymId || gymId.trim() === '') + throw new Error(`${L} Cannot subscribe: gymId must be a non-empty string`) + if (_subRetryTimer !== null) { + clearTimeout(_subRetryTimer) + _subRetryTimer = null + } if (_subChannel) { removeSafe(_client, _subChannel, 'sub/existing') _subChannel = null @@ -182,7 +203,13 @@ export function subscribeToDisplay({ gymId, handlers }: SubscribeToDisplayArgs): .on('broadcast', { event: 'idle_snapshot' }, ({ payload }) => validated(idleSnapshotSchema, payload, 'idle_snapshot', handlers.onIdleSnapshot), ) - .on('broadcast', { event: 'unfocus' }, () => handlers.onUnfocus()) + .on('broadcast', { event: 'unfocus' }, () => { + try { + handlers.onUnfocus() + } catch (err) { + console.error(`${L} unfocus handler threw:`, err) + } + }) .subscribe((status, err) => { if (status === 'SUBSCRIBED') { _subStatus = 'connected' @@ -191,7 +218,10 @@ export function subscribeToDisplay({ gymId, handlers }: SubscribeToDisplayArgs): _subConnectedBefore = true handlers.onStatusChange('connected') } else if (status === 'TIMED_OUT' || status === 'CHANNEL_ERROR' || status === 'CLOSED') { - console.warn(`${L} Subscriber channel ${status}`, err) + ;(status === 'CLOSED' ? console.info : console.error)( + `${L} Subscriber channel ${status}`, + err ?? '', + ) _subStatus = 'reconnecting' handlers.onStatusChange('reconnecting') if (_subChannel) removeSafe(_client, _subChannel, `subSub/${status}`) From 83d59f68a0f6ad037c4115b8e67b35b64990135d Mon Sep 17 00:00:00 2001 From: Robert Hamilton Date: Sun, 12 Apr 2026 18:41:11 -0500 Subject: [PATCH 3/5] =?UTF-8?q?docs(display):=20P16=20review=20resolution?= =?UTF-8?q?=20=E2=80=94=20tasks,=20ADR-015,=20review=20file?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tasks: S015-T through S021-T added to Steps.md for missing display-realtime test coverage (subscribeToDisplay throw, onStatusChange, focus/unfocus/idle handlers, gym-switch teardown, backoff sequencing, auto-publishHello, getters) ADR-015: documents shared _client teardown contract — publisher and subscriber are mutually exclusive by route structure; P16-001/P16-002 fixes mitigate the retry-window race without requiring a new co-existence mechanism Review: Context/Reviews/0016-pr106-simplify-display-subsystem-2026-04-12.md --- ...display-realtime-shared-client-teardown.md | 134 +++++++++++++ .../Steps.md | 130 +++++++++++-- ...6-simplify-display-subsystem-2026-04-12.md | 180 ++++++++++++++++++ 3 files changed, 433 insertions(+), 11 deletions(-) create mode 100644 Context/Decisions/ADR-015-display-realtime-shared-client-teardown.md create mode 100644 Context/Reviews/0016-pr106-simplify-display-subsystem-2026-04-12.md diff --git a/Context/Decisions/ADR-015-display-realtime-shared-client-teardown.md b/Context/Decisions/ADR-015-display-realtime-shared-client-teardown.md new file mode 100644 index 00000000..84c30491 --- /dev/null +++ b/Context/Decisions/ADR-015-display-realtime-shared-client-teardown.md @@ -0,0 +1,134 @@ +# ADR-015: Shared `_client` teardown contract in `display-realtime.ts` + +## Status + +Accepted + +## Context + +`display-realtime.ts` houses both the publisher (phone → gym TV) and the subscriber +(gym TV) in a single module, sharing a `_client: SupabaseClient | null` reference. +Both halves must null `_client` on destroy to avoid retaining the Supabase client +across route unmounts, but neither can unconditionally null it because the other half +might still be active. + +The original guards are asymmetric: + +```typescript +// destroyDisplayPublisher (line 150) +if (!_subChannel) _client = null + +// destroyDisplaySubscriber (line 260) +if (!_pubChannel && _pubGymId === null) _client = null +``` + +Both use channel handles (`_subChannel`, `_pubChannel`) as proxies for "the other half +is idle." This is unreliable during the retry window: `_subChannel` is nulled **before** +`scheduleRetry` is called (it is torn down and set to null at line 228), so from +`destroyDisplayPublisher`'s perspective, the subscriber appears idle even while a retry +timer is pending and the subscriber is still logically active. A caller that destroys +the publisher while a subscriber retry is in-flight will null `_client`, and when the +timer fires it will find `_client === null` and log an error instead of reconnecting — +a confusing failure mode with no obvious cause. + +Review finding P16-015 raised the deeper question: are publisher and subscriber +**designed to co-exist** in the same browser session, or are they **mutually +exclusive**? + +Inspecting the call sites: + +- `src/hooks/use-display-broadcast.ts` mounts the publisher in the authenticated + workout route (phone acting as athlete). +- `src/routes/display/gym/$gymId.tsx` mounts the subscriber in the display route + (gym TV). + +These routes are never active in the same browser session. The display route is +a fullscreen kiosk URL opened on a dedicated device; it never mounts alongside the +authenticated athlete UI. Publisher and subscriber are therefore **mutually +exclusive** in practice. + +## Decision + +Document and assert the mutual exclusion constraint rather than implement a +general-purpose co-existence mechanism. + +Concretely: + +1. **Each `destroy` function unconditionally nulls `_client` for its own role's + state**, and uses a simple `_role` sentinel (or equivalent boolean flags) to + determine whether to also null the shared reference. Because publisher and + subscriber are mutually exclusive, in practice only one will ever be active, so + this is equivalent to "always null on destroy." + +2. **The shared `_client` is considered owned by whichever role initialized it.** + `initDisplayPublisher` and `initDisplaySubscriber` both write to `_client`; the + last writer wins. Each `destroy` nulls `_client` unconditionally, because the + mutual exclusion guarantee means the other role is already idle. + +3. **A `console.error` guard in each `destroy` makes the assumption explicit:** + if both `_pubChannel` and `_subChannel` are non-null when either destroy is called, + log an error — this should never happen in production and indicates a mount/unmount + ordering bug. + +Regarding the retry-window gap identified in P16-001 and P16-002: after those fixes +land, `subscribeToDisplay` clears any pending retry timer at entry (P16-002), and +the retry callback's null-client branch explicitly stops the loop and calls +`handlers.onStatusChange('disconnected')` (P16-001). Combined, these ensure a +destroyed subscriber does not silently continue reconnecting. + +## Consequences + +### Positive + +- Teardown logic becomes explicit and auditable: the mutual exclusion assumption is + documented in code comments, not implicit in channel-null checks. +- The retry-window race (P16-015) is mitigated by the P16-001/P16-002 fixes, which + are more targeted than reworking the teardown contract. +- No new runtime state is required beyond what the P16-001/P16-002 fixes already + introduce (`_subRetryTimer` clear at subscribe entry). + +### Negative + +- If a future feature does require publisher and subscriber to co-exist in the same + session (e.g., a coach's tablet that both publishes its own workout and subscribes + to a second athlete's display), the teardown contract will need revisiting. This + ADR should be reviewed at that point. + +### Neutral + +- The asymmetric guards in the current code (`if (!_subChannel)` vs + `if (!_pubChannel && _pubGymId === null)`) remain as-is for this PR. The + authoritative fix is the P16-001/P16-002 pair. The teardown guards are + opportunistic cleanup that fire only in the steady-state case (both halves + are fully idle when either is destroyed), which is the common case. + +## Alternatives Considered + +### Add `_subActive` boolean sentinel + +Introduce `_subActive: boolean` set by `initDisplaySubscriber`/`destroyDisplaySubscriber` +and checked in `destroyDisplayPublisher` instead of testing `_subChannel`. This would +be reliable during the retry window. Rejected because the mutual exclusion guarantee +makes it unnecessary complexity — the co-existence case we'd be guarding against +cannot occur with the current route structure. + +### Always null `_client` unconditionally on each destroy + +Simple and correct given mutual exclusion. Slightly less defensive (if someone does +accidentally mount both halves, destroying one kills the other's client). Rejected +in favor of the error-log guard approach, which makes the violation observable. + +### Rearchitect with a shared `DisplayRealtimeClient` class + +Replace module-scope state with an instantiated class that both sides hold a reference +to. The class manages the shared client lifecycle and tracks which sides are active. +Rejected as over-engineered for a module with exactly two callers that are never +concurrent. + +## References + +- Review finding P16-015: `Context/Reviews/0016-pr106-simplify-display-subsystem-2026-04-12.md` +- Review findings P16-001, P16-002: same file (retry-window fixes that mitigate the race) +- `src/lib/display-realtime.ts` — lines 150 and 260 (teardown guards) +- `src/hooks/use-display-broadcast.ts` — publisher mount site +- `src/routes/display/gym/$gymId.tsx` — subscriber mount site diff --git a/Context/Features/008-display-broadcast-infrastructure/Steps.md b/Context/Features/008-display-broadcast-infrastructure/Steps.md index 3c29b4c9..c9e67eb5 100644 --- a/Context/Features/008-display-broadcast-infrastructure/Steps.md +++ b/Context/Features/008-display-broadcast-infrastructure/Steps.md @@ -8,11 +8,11 @@ ## Team Composition -| Role | Agent | Scope | -|------|-------|-------| -| `backend-specialist` | backend-specialist | Migrations (Postgres + SQLite), Rust model/command changes, data mapper | +| Role | Agent | Scope | +| --------------------- | ------------------- | ----------------------------------------------------------------------------------------- | +| `backend-specialist` | backend-specialist | Migrations (Postgres + SQLite), Rust model/command changes, data mapper | | `frontend-specialist` | frontend-specialist | Zod schema, snapshot builder, publisher module, hook, Settings UI, Push to Display button | -| `quality-engineer` | quality-engineer | Unit tests for schema, builder, and publisher; integration validation | +| `quality-engineer` | quality-engineer | Unit tests for schema, builder, and publisher; integration validation | --- @@ -296,14 +296,122 @@ Add "Remote Display" section to the profile/settings page: --- +## Wave 5: display-realtime.ts Coverage Gaps (quality-engineer, post-refactor) + +### S015-T: Missing test — `subscribeToDisplay` throws when uninitialized + +**Agent:** quality-engineer +**Files:** `src/lib/__tests__/display-realtime.test.ts` +**Depends on:** None (existing module) +**Parallel:** Yes (all Wave 5 tasks parallel) + +- [ ] Add test: `'throws when subscribeToDisplay is called before initDisplaySubscriber'` +- [ ] Verify the thrown `Error` message includes `'Call initDisplaySubscriber first'` +- [ ] This contract is relied on by `src/routes/display/gym/$gymId.tsx` to render the Retry button via its outer `try/catch` → `BootError` mapping + +**Acceptance:** Test confirms the throw and message. (Ported from deleted `display-subscriber.test.ts`) + +--- + +### S016-T: Missing test — auto-`publishHello` on reconnect + +**Agent:** quality-engineer +**Files:** `src/lib/__tests__/display-realtime.test.ts` +**Depends on:** None +**Parallel:** Yes + +- [ ] Add test: `'auto-fires publishHello on reconnect when previously connected'` +- [ ] Sequence: `SUBSCRIBED` → `CHANNEL_ERROR` → timer fires → `SUBSCRIBED` again +- [ ] Assert `channel.send` called with `{ type: 'broadcast', event: 'display_hello', payload: {} }` on the second `SUBSCRIBED` +- [ ] Assert `channel.send` is NOT called with `display_hello` on the first `SUBSCRIBED` (first connect, not reconnect) + +**Acceptance:** Full reconnect cycle exercises the `_subConnectedBefore` flag correctly. + +--- + +### S017-T: Missing tests — `onStatusChange` callback assertions + +**Agent:** quality-engineer +**Files:** `src/lib/__tests__/display-realtime.test.ts` +**Depends on:** None +**Parallel:** Yes + +- [ ] Add test: `'calls onStatusChange("connected") on SUBSCRIBED'` +- [ ] Add parameterized tests: `onStatusChange("reconnecting")` fires for `TIMED_OUT`, `CHANNEL_ERROR`, and `CLOSED` + +**Acceptance:** `onStatusChange` is asserted in subscriber tests (it was mocked but never asserted). + +--- + +### S018-T: Missing tests — `focus`, `unfocus`, `idle_snapshot` subscriber handler tests + +**Agent:** quality-engineer +**Files:** `src/lib/__tests__/display-realtime.test.ts` +**Depends on:** None +**Parallel:** Yes + +- [ ] Add test: `'fires onFocus when subscriber receives a focus broadcast'` +- [ ] Add test: `'fires onUnfocus when subscriber receives an unfocus broadcast'` +- [ ] Add test: `'fires onIdleSnapshot when subscriber receives a valid idle_snapshot broadcast'` +- [ ] The `idle_snapshot` test must use a valid `idleSnapshotSchema` payload (distinct from `displaySnapshotSchema`) + +**Acceptance:** All five subscriber event handlers are covered (currently only `workout_snapshot` and `session_ended` are tested). + +--- + +### S019-T: Missing test — gym-switch channel teardown/recreation + +**Agent:** quality-engineer +**Files:** `src/lib/__tests__/display-realtime.test.ts` +**Depends on:** None +**Parallel:** Yes + +- [ ] Add test: `'tears down the old channel and creates a new one when gymId changes via configureDisplayPublisher'` +- [ ] Configure publisher with `GYM_A`, publish to force channel creation, then configure with `GYM_B` +- [ ] Assert `removeChannel` was called for the `GYM_A` channel +- [ ] Assert a new channel is created with `getGymChannelName(GYM_B)` on next publish + +**Acceptance:** Gym-switch path exercises `teardownPubChannel()` correctly. (Ported from deleted `display-publisher.test.ts`) + +--- + +### S020-T: Missing tests — exponential backoff sequencing + +**Agent:** quality-engineer +**Files:** `src/lib/__tests__/display-realtime.test.ts` +**Depends on:** None +**Parallel:** Yes + +- [ ] Add test: `'reconnects after CHANNEL_ERROR with exponential backoff delay'` — first failure uses 2000ms delay +- [ ] Add test: `'resets retry attempt counter after successful reconnect'` +- [ ] Add test: `'caps retry delay at 30 seconds'` — advance attempt counter high enough to hit the cap + +**Acceptance:** `scheduleRetry` delay math and `_subRetryAttempt` reset-on-reconnect are verified. + +--- + +### S021-T: Missing tests — `getActiveGymId` and `getSubscriberStatus` state transitions + +**Agent:** quality-engineer +**Files:** `src/lib/__tests__/display-realtime.test.ts` +**Depends on:** None +**Parallel:** Yes + +- [ ] `getActiveGymId`: add tests — returns `null` when unconfigured, returns gym ID when broadcasting, returns `null` after `destroyDisplayPublisher` +- [ ] `getSubscriberStatus`: add tests — returns `'disconnected'` when unsubscribed, `'connected'` after `SUBSCRIBED`, `'reconnecting'` after `CHANNEL_ERROR` + +**Acceptance:** Both exported getters have state-transition coverage. (Ported from deleted `display-publisher.test.ts`; `getSubscriberStatus` was never tested.) + +--- + ## Milestone Summary -| Wave | Tasks | Parallel | Description | -|------|-------|----------|-------------| -| Wave 1 | S001-S005 | S001-S004 parallel; S005 sequential after S002+S004 | Domain types, migrations, data mapper | -| Wave 2 | S006-S007 | Both parallel | Snapshot builder + publisher module | -| Wave 3 | S008-S012 | S010+S011 parallel with S008+S009; S012 sequential last | Store wiring, hook, Settings UI, button | -| Wave 4 | S013-T, S014-T | Both parallel | Unit tests for schema, builder, publisher | +| Wave | Tasks | Parallel | Description | +| ------ | -------------- | ------------------------------------------------------- | ----------------------------------------- | +| Wave 1 | S001-S005 | S001-S004 parallel; S005 sequential after S002+S004 | Domain types, migrations, data mapper | +| Wave 2 | S006-S007 | Both parallel | Snapshot builder + publisher module | +| Wave 3 | S008-S012 | S010+S011 parallel with S008+S009; S012 sequential last | Store wiring, hook, Settings UI, button | +| Wave 4 | S013-T, S014-T | Both parallel | Unit tests for schema, builder, publisher | **Totals:** 12 implementation tasks + 2 validation tasks = 14 tasks **Agents:** 1 frontend specialist + 1 backend specialist + 1 quality engineer = 3 agents @@ -334,7 +442,7 @@ interface DisplaySnapshot { user_id: string display_name: string session_name: string - workout_started_at: string // ISO 8601 + workout_started_at: string // ISO 8601 current_exercise: string exercise_index: number total_exercises: number diff --git a/Context/Reviews/0016-pr106-simplify-display-subsystem-2026-04-12.md b/Context/Reviews/0016-pr106-simplify-display-subsystem-2026-04-12.md new file mode 100644 index 00000000..96e48789 --- /dev/null +++ b/Context/Reviews/0016-pr106-simplify-display-subsystem-2026-04-12.md @@ -0,0 +1,180 @@ +# PR Review: chore/simplify-display-subsystem → main + +**Date:** 2026-04-12 +**Feature:** N/A (chore refactor — display subsystem, see `Context/Features/008-display-broadcast-infrastructure/`) +**Branch:** worktree-chore+simplify-display-subsystem +**PR:** #106 — `refactor(display): collapse publisher + subscriber into display-realtime` +**Reviewers:** pr-review-toolkit:code-reviewer, pr-review-toolkit:silent-failure-hunter, pr-review-toolkit:pr-test-analyzer +**Status:** ✅ Resolved + +## Summary + +16 findings across `src/lib/display-realtime.ts` (231 LOC) and `src/lib/__tests__/display-realtime.test.ts` +(391 LOC). The 15 import-path-only caller files are clean. Two critical runtime bugs (silent retry abandonment, +stale-closure ghost subscription), five high-severity error-handling gaps, and nine test coverage gaps. +Import path changes in the 15 caller files are correct. + +## Findings + +### Fix-Now + +#### [FIX] P16-001: Silent retry abandonment leaves status stuck at `reconnecting` + +- **File:** `src/lib/display-realtime.ts:156` +- **Severity:** Critical +- **Detail:** In `scheduleRetry`, the timeout callback checks `if (_client) subscribeToDisplay(...)` — when `_client` is null at fire time, the retry is silently skipped with no log and no status update. `_subStatus` stays `'reconnecting'` forever. This is reachable: `destroyDisplayPublisher` nulls `_client` when `!_subChannel` (line 141), and `_subChannel` is already null before the retry timer fires (nulled on line 198 before `scheduleRetry` is called). Fix: add `console.error` + `handlers.onStatusChange('disconnected')` in the null-client branch. +- **Status:** ✅ Fixed +- **Resolution:** Added `else` branch in `scheduleRetry` timeout: logs error, sets `_subStatus = 'disconnected'`, calls `handlers.onStatusChange('disconnected')` + +#### [FIX] P16-002: `subscribeToDisplay` missing `clearTimeout` before re-subscribing + +- **File:** `src/lib/display-realtime.ts:164` +- **Severity:** Critical +- **Detail:** If `subscribeToDisplay` is called again while a retry timer is pending (e.g., `useEffect` re-runs with a new `gymId`), the old timer is not cancelled. It fires, finds `_client` non-null, and calls `subscribeToDisplay` with the stale `gymId` captured in the closure — ghost-subscribing to the wrong gym's channel. Fix: add `if (_subRetryTimer !== null) { clearTimeout(_subRetryTimer); _subRetryTimer = null }` at the top of `subscribeToDisplay`, before the existing `_subChannel` cleanup. +- **Status:** ✅ Fixed +- **Resolution:** Added `clearTimeout(_subRetryTimer)` guard at the top of `subscribeToDisplay` before channel cleanup + +#### [FIX] P16-004: `removeSafe` silently leaks channels when `_client` is null + +- **File:** `src/lib/display-realtime.ts:30-37` +- **Severity:** High +- **Detail:** The `if (!client) return` early return swallows the channel without logging. The channel handle stays open from the SDK's perspective. This path is reachable during the `destroyDisplaySubscriber` → `destroyDisplayPublisher` interleave described in P16-001. Fix: `console.warn(`${L} removeSafe(${ctx}): client is null, channel may be leaked`)` before return. +- **Status:** ✅ Fixed +- **Resolution:** Added `console.warn` before the early return in `removeSafe` when client is null + +#### [FIX] P16-005: `unfocus` handler bypasses `validated` with no try-catch + +- **File:** `src/lib/display-realtime.ts:185` +- **Severity:** High +- **Detail:** All other broadcast event handlers go through `validated`. The `unfocus` handler calls `handlers.onUnfocus()` directly with no protection. A throwing handler propagates into Supabase's broadcast dispatch code. Fix: wrap in try-catch matching the publisher `display_hello` responder pattern at lines 63-68, or route through a no-schema `validated` variant. +- **Status:** ✅ Fixed +- **Resolution:** Wrapped `handlers.onUnfocus()` in try-catch with `console.error` on throw, matching the responder pattern + +#### [FIX] P16-006: `CHANNEL_ERROR`/`TIMED_OUT` logged at `warn` not `error` + +- **File:** `src/lib/display-realtime.ts:194` +- **Severity:** High +- **Detail:** The subscriber's status callback uses `console.warn` for all three error states. `CHANNEL_ERROR` and `TIMED_OUT` are network-level failures that take the display offline — they should log at `console.error`. `CLOSED` is an intentional teardown and is reasonably `console.info`. The publisher at line 72 already applies this distinction correctly. Fix: mirror the publisher's conditional: `(status === 'CLOSED' ? console.info : console.error)(...)`. +- **Status:** ✅ Fixed +- **Resolution:** Replaced `console.warn` with `(status === 'CLOSED' ? console.info : console.error)(...)` mirroring the publisher pattern + +#### [FIX] P16-007: Empty `gymId` not validated — produces `display:gym:` channel name + +- **File:** `src/lib/display-realtime.ts:97, 164` +- **Severity:** High +- **Detail:** `configureDisplayPublisher` and `subscribeToDisplay` validate `null` but not empty string. An empty string passes both guards and produces channel name `display:gym:` via `getGymChannelName("")`. `.claude/rules/state-management.md` explicitly flags this exact empty-string risk for this module. Fix: add `if (!gymId || gymId.trim() === '')` guards at both entry points. +- **Status:** ✅ Fixed +- **Resolution:** Added `gymId.trim() === ''` guard in `configureDisplayPublisher` (logs error, returns) and `subscribeToDisplay` (throws) + +#### [FIX] P16-011: `validated` logs raw `ZodError` object — serializes poorly in aggregators + +- **File:** `src/lib/display-realtime.ts:42` +- **Severity:** Medium +- **Detail:** `console.warn(`${L} Invalid ${ev} payload, dropping`, r.error)` passes the raw `ZodError` instance. In Sentry/Datadog this serializes as `[object Object]` or a truncated structure. Fix: use `r.error.toString()` and include the raw payload: `console.warn(`${L} Invalid ${ev} payload (dropping):`, r.error.toString(), p)`. +- **Status:** ✅ Fixed +- **Resolution:** Changed to `console.warn(..., r.error.toString(), p)` — includes stringified error and raw payload for aggregator context + +#### [FIX] P16-014: Double-cast `snapshot as unknown as Record` + +- **File:** `src/lib/display-realtime.ts:115` +- **Severity:** Low +- **Detail:** `publishDisplaySnapshot` casts the typed `DisplaySnapshot` to `Record` because `pubSend` requires that type. A cleaner approach: widen `pubSend`'s payload parameter to `unknown` since `channel.send()` accepts `unknown` internally. +- **Status:** ✅ Fixed +- **Resolution:** Widened `pubSend` parameter to `unknown` with a single internal cast at the `ch.send` call site; `publishDisplaySnapshot` now passes `snapshot` directly + +--- + +### Missing Tasks + +#### [TASK] P16-003: Missing test — `subscribeToDisplay` throw when uninitialized + +- **File:** `src/lib/__tests__/display-realtime.test.ts` +- **Severity:** Critical +- **Detail:** `subscribeToDisplay` throws `Error` when `_client` is null (line 165-167). The calling route at `src/routes/display/gym/$gymId.tsx` relies on this contract to render the Retry button via its outer `try/catch` `BootError` mapping. This test existed in the deleted `display-subscriber.test.ts` and must be ported. Missing test: `'throws when subscribeToDisplay is called before initDisplaySubscriber'`. +- **Relates to:** `Context/Features/008-display-broadcast-infrastructure/` +- **Status:** ✅ Task created +- **Resolution:** Added as S015-T in Steps.md + +#### [TASK] P16-008: Missing test — `onStatusChange` never asserted in subscriber tests + +- **File:** `src/lib/__tests__/display-realtime.test.ts` +- **Severity:** Medium +- **Detail:** `createMockHandlers()` includes `onStatusChange: vi.fn()` but no test asserts it fires. The deleted subscriber test had parameterized cases for all three error statuses. Missing tests: `'calls onStatusChange("connected") on SUBSCRIBED'` and `'calls onStatusChange("reconnecting") for TIMED_OUT, CHANNEL_ERROR, and CLOSED'`. +- **Status:** ✅ Task created +- **Resolution:** Added as S017-T in Steps.md + +#### [TASK] P16-009: Missing tests — `focus`, `unfocus`, `idle_snapshot` subscriber handlers + +- **File:** `src/lib/__tests__/display-realtime.test.ts` +- **Severity:** Medium +- **Detail:** `subscribeToDisplay` registers five event handlers. The new test file covers `workout_snapshot` and `session_ended` only. Missing tests: `'fires onFocus when subscriber receives a focus broadcast'`, `'fires onUnfocus when subscriber receives an unfocus broadcast'`, `'fires onIdleSnapshot when subscriber receives a valid idle_snapshot broadcast'`. `idle_snapshot` is especially important — it uses `idleSnapshotSchema`, distinct from the others. +- **Status:** ✅ Task created +- **Resolution:** Added as S018-T in Steps.md + +#### [TASK] P16-010: Missing test — gym-switch channel teardown/recreation + +- **File:** `src/lib/__tests__/display-realtime.test.ts` +- **Severity:** Medium +- **Detail:** `configureDisplayPublisher` calls `teardownPubChannel()` when `gymId !== _pubGymId` (line 109). The deleted publisher test had a dedicated `'switching gyms between publishes'` block verifying `removeChannel` fires and the new gym gets its own channel. An athlete switching gyms mid-session could silently broadcast to the wrong display. Missing test: `'tears down the old channel and creates a new one when gymId changes via configureDisplayPublisher'`. +- **Status:** ✅ Task created +- **Resolution:** Added as S019-T in Steps.md + +#### [TASK] P16-012: Missing test — exponential backoff sequencing + +- **File:** `src/lib/__tests__/display-realtime.test.ts` +- **Severity:** Medium +- **Detail:** The `scheduleRetry` delay math (`Math.min(2000 * 2^attempt, 30_000)`) and `_subRetryAttempt` reset-on-reconnect are untested. Given gym-floor reliability requirements, silent backoff math regressions are high-cost. Missing tests: `'reconnects after CHANNEL_ERROR with backoff delay'`, `'resets retry attempt counter after successful reconnect'`, `'caps retry delay at 30 seconds'`. +- **Status:** ✅ Task created +- **Resolution:** Added as S020-T in Steps.md + +#### [TASK] P16-013: Missing test — auto-publishHello on reconnect + +- **File:** `src/lib/__tests__/display-realtime.test.ts` +- **Severity:** High +- **Detail:** The `_subConnectedBefore` flag (line 190) drives the guarantee that a reconnecting display automatically fires `publishHello()` so the gym board sees it come back online. No test exercises the full reconnect cycle: `SUBSCRIBED` → `CHANNEL_ERROR` → timer fires → `SUBSCRIBED` again → assert `display_hello` sent. Missing test: `'auto-fires publishHello on reconnect when previously connected'`. +- **Status:** ✅ Task created +- **Resolution:** Added as S016-T in Steps.md + +#### [TASK] P16-016: Missing tests — `getActiveGymId` and `getSubscriberStatus` exported but untested + +- **File:** `src/lib/__tests__/display-realtime.test.ts` +- **Severity:** Medium +- **Detail:** Both functions are exported and consumed by callers. The deleted `display-publisher.test.ts` had a full `getActiveGymId` describe block (null when unconfigured, returns ID when broadcasting, null after destroy). `getSubscriberStatus` was never tested. Missing tests: three `getActiveGymId` cases and three `getSubscriberStatus` state transition cases. +- **Status:** ✅ Task created +- **Resolution:** Added as S021-T in Steps.md + +--- + +### Architectural Concerns + +#### [ADR] P16-015: Shared `_client` teardown contract is implicit and asymmetric + +- **File:** `src/lib/display-realtime.ts:141, 230` +- **Severity:** Medium +- **Detail:** `destroyDisplayPublisher` uses `!_subChannel` as proxy for "subscriber is idle" (line 141). `destroyDisplaySubscriber` uses `!_pubChannel && _pubGymId === null` (line 230). These are asymmetric and both use `_subChannel`/`_pubChannel` as proxies that are unreliable during the retry window (both are null while a retry timer is pending). The fundamental question: are publisher and subscriber designed to co-exist in the same browser context, or are they mutually exclusive? If mutually exclusive, a comment and assertion should document this. If they can co-exist, the guards need a more reliable sentinel (`_subActive` boolean set/cleared by `subscribeToDisplay`/`destroyDisplaySubscriber`). +- **Relates to:** `Context/Features/008-display-broadcast-infrastructure/Tech.md` +- **Status:** ✅ ADR created +- **Resolution:** ADR-015 — `Context/Decisions/ADR-015-display-realtime-shared-client-teardown.md` + +--- + +## Resolution Checklist + +- [x] All [FIX] findings resolved (P16-001, P16-002, P16-004, P16-005, P16-006, P16-007, P16-011, P16-014) +- [x] All [TASK] findings added to Steps.md or test file (P16-003, P16-008, P16-009, P16-010, P16-012, P16-013, P16-016) +- [x] All [ADR] findings have ADRs created or dismissed (P16-015) +- [x] All [RULE] findings applied or dismissed (none) +- [ ] Review verified by review-verify agent + +## Resolution Summary + +**Resolved at:** 2026-04-12 +**Session:** resolve-review on worktree chore+simplify-display-subsystem + +| Category | Total | Resolved | +| --------- | ------ | -------- | +| [FIX] | 8 | 8 | +| [TASK] | 7 | 7 | +| [ADR] | 1 | 1 | +| [RULE] | 0 | 0 | +| **Total** | **16** | **16** | From 60f6f17e70030ca6af3ef26c7ee7ac78adb2f317 Mon Sep 17 00:00:00 2001 From: Robert Hamilton Date: Sun, 12 Apr 2026 18:55:08 -0500 Subject: [PATCH 4/5] =?UTF-8?q?test(display):=20add=20Wave=205=20coverage?= =?UTF-8?q?=20for=20display-realtime.ts=20(S015-T=E2=80=93S021-T)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds 21 new tests across 7 task items from the P16 review: - S015-T: subscribeToDisplay throws when uninitialized or gymId is empty - S016-T: publishHello fires on reconnect but not on first connect - S017-T: onStatusChange asserted for SUBSCRIBED/TIMED_OUT/CHANNEL_ERROR/CLOSED - S018-T: focus, unfocus, idle_snapshot broadcast handler coverage - S019-T: gym-switch tears down old channel and creates new one - S020-T: backoff delay math (2000ms first, resets on reconnect, cap at 30s) - S021-T: getActiveGymId and getSubscriberStatus state transitions Introduces autoSubscribe option on createMockClient to accumulate _subRetryAttempt across consecutive errors without SUBSCRIBED resetting it. Tasks: S015-T, S016-T, S017-T, S018-T, S019-T, S020-T, S021-T Feature: 008-display-broadcast-infrastructure --- .../Steps.md | 44 +-- src/lib/__tests__/display-realtime.test.ts | 339 +++++++++++++++++- 2 files changed, 358 insertions(+), 25 deletions(-) diff --git a/Context/Features/008-display-broadcast-infrastructure/Steps.md b/Context/Features/008-display-broadcast-infrastructure/Steps.md index c9e67eb5..bdfbe962 100644 --- a/Context/Features/008-display-broadcast-infrastructure/Steps.md +++ b/Context/Features/008-display-broadcast-infrastructure/Steps.md @@ -305,9 +305,9 @@ Add "Remote Display" section to the profile/settings page: **Depends on:** None (existing module) **Parallel:** Yes (all Wave 5 tasks parallel) -- [ ] Add test: `'throws when subscribeToDisplay is called before initDisplaySubscriber'` -- [ ] Verify the thrown `Error` message includes `'Call initDisplaySubscriber first'` -- [ ] This contract is relied on by `src/routes/display/gym/$gymId.tsx` to render the Retry button via its outer `try/catch` → `BootError` mapping +- [x] Add test: `'throws when subscribeToDisplay is called before initDisplaySubscriber'` +- [x] Verify the thrown `Error` message includes `'Call initDisplaySubscriber first'` +- [x] This contract is relied on by `src/routes/display/gym/$gymId.tsx` to render the Retry button via its outer `try/catch` → `BootError` mapping **Acceptance:** Test confirms the throw and message. (Ported from deleted `display-subscriber.test.ts`) @@ -320,10 +320,10 @@ Add "Remote Display" section to the profile/settings page: **Depends on:** None **Parallel:** Yes -- [ ] Add test: `'auto-fires publishHello on reconnect when previously connected'` -- [ ] Sequence: `SUBSCRIBED` → `CHANNEL_ERROR` → timer fires → `SUBSCRIBED` again -- [ ] Assert `channel.send` called with `{ type: 'broadcast', event: 'display_hello', payload: {} }` on the second `SUBSCRIBED` -- [ ] Assert `channel.send` is NOT called with `display_hello` on the first `SUBSCRIBED` (first connect, not reconnect) +- [x] Add test: `'auto-fires publishHello on reconnect when previously connected'` +- [x] Sequence: `SUBSCRIBED` → `CHANNEL_ERROR` → timer fires → `SUBSCRIBED` again +- [x] Assert `channel.send` called with `{ type: 'broadcast', event: 'display_hello', payload: {} }` on the second `SUBSCRIBED` +- [x] Assert `channel.send` is NOT called with `display_hello` on the first `SUBSCRIBED` (first connect, not reconnect) **Acceptance:** Full reconnect cycle exercises the `_subConnectedBefore` flag correctly. @@ -336,8 +336,8 @@ Add "Remote Display" section to the profile/settings page: **Depends on:** None **Parallel:** Yes -- [ ] Add test: `'calls onStatusChange("connected") on SUBSCRIBED'` -- [ ] Add parameterized tests: `onStatusChange("reconnecting")` fires for `TIMED_OUT`, `CHANNEL_ERROR`, and `CLOSED` +- [x] Add test: `'calls onStatusChange("connected") on SUBSCRIBED'` +- [x] Add parameterized tests: `onStatusChange("reconnecting")` fires for `TIMED_OUT`, `CHANNEL_ERROR`, and `CLOSED` **Acceptance:** `onStatusChange` is asserted in subscriber tests (it was mocked but never asserted). @@ -350,10 +350,10 @@ Add "Remote Display" section to the profile/settings page: **Depends on:** None **Parallel:** Yes -- [ ] Add test: `'fires onFocus when subscriber receives a focus broadcast'` -- [ ] Add test: `'fires onUnfocus when subscriber receives an unfocus broadcast'` -- [ ] Add test: `'fires onIdleSnapshot when subscriber receives a valid idle_snapshot broadcast'` -- [ ] The `idle_snapshot` test must use a valid `idleSnapshotSchema` payload (distinct from `displaySnapshotSchema`) +- [x] Add test: `'fires onFocus when subscriber receives a focus broadcast'` +- [x] Add test: `'fires onUnfocus when subscriber receives an unfocus broadcast'` +- [x] Add test: `'fires onIdleSnapshot when subscriber receives a valid idle_snapshot broadcast'` +- [x] The `idle_snapshot` test must use a valid `idleSnapshotSchema` payload (distinct from `displaySnapshotSchema`) **Acceptance:** All five subscriber event handlers are covered (currently only `workout_snapshot` and `session_ended` are tested). @@ -366,10 +366,10 @@ Add "Remote Display" section to the profile/settings page: **Depends on:** None **Parallel:** Yes -- [ ] Add test: `'tears down the old channel and creates a new one when gymId changes via configureDisplayPublisher'` -- [ ] Configure publisher with `GYM_A`, publish to force channel creation, then configure with `GYM_B` -- [ ] Assert `removeChannel` was called for the `GYM_A` channel -- [ ] Assert a new channel is created with `getGymChannelName(GYM_B)` on next publish +- [x] Add test: `'tears down the old channel and creates a new one when gymId changes via configureDisplayPublisher'` +- [x] Configure publisher with `GYM_A`, publish to force channel creation, then configure with `GYM_B` +- [x] Assert `removeChannel` was called for the `GYM_A` channel +- [x] Assert a new channel is created with `getGymChannelName(GYM_B)` on next publish **Acceptance:** Gym-switch path exercises `teardownPubChannel()` correctly. (Ported from deleted `display-publisher.test.ts`) @@ -382,9 +382,9 @@ Add "Remote Display" section to the profile/settings page: **Depends on:** None **Parallel:** Yes -- [ ] Add test: `'reconnects after CHANNEL_ERROR with exponential backoff delay'` — first failure uses 2000ms delay -- [ ] Add test: `'resets retry attempt counter after successful reconnect'` -- [ ] Add test: `'caps retry delay at 30 seconds'` — advance attempt counter high enough to hit the cap +- [x] Add test: `'reconnects after CHANNEL_ERROR with exponential backoff delay'` — first failure uses 2000ms delay +- [x] Add test: `'resets retry attempt counter after successful reconnect'` +- [x] Add test: `'caps retry delay at 30 seconds'` — advance attempt counter high enough to hit the cap **Acceptance:** `scheduleRetry` delay math and `_subRetryAttempt` reset-on-reconnect are verified. @@ -397,8 +397,8 @@ Add "Remote Display" section to the profile/settings page: **Depends on:** None **Parallel:** Yes -- [ ] `getActiveGymId`: add tests — returns `null` when unconfigured, returns gym ID when broadcasting, returns `null` after `destroyDisplayPublisher` -- [ ] `getSubscriberStatus`: add tests — returns `'disconnected'` when unsubscribed, `'connected'` after `SUBSCRIBED`, `'reconnecting'` after `CHANNEL_ERROR` +- [x] `getActiveGymId`: add tests — returns `null` when unconfigured, returns gym ID when broadcasting, returns `null` after `destroyDisplayPublisher` +- [x] `getSubscriberStatus`: add tests — returns `'disconnected'` when unsubscribed, `'connected'` after `SUBSCRIBED`, `'reconnecting'` after `CHANNEL_ERROR` **Acceptance:** Both exported getters have state-transition coverage. (Ported from deleted `display-publisher.test.ts`; `getSubscriberStatus` was never tested.) diff --git a/src/lib/__tests__/display-realtime.test.ts b/src/lib/__tests__/display-realtime.test.ts index 7c2905fe..9a9ab22f 100644 --- a/src/lib/__tests__/display-realtime.test.ts +++ b/src/lib/__tests__/display-realtime.test.ts @@ -1,6 +1,6 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' import type { SupabaseClient } from '@supabase/supabase-js' -import type { DisplaySnapshot } from '@/domain/types/display-snapshot' +import type { DisplaySnapshot, IdleSnapshot } from '@/domain/types/display-snapshot' import { initDisplayPublisher, configureDisplayPublisher, @@ -11,10 +11,12 @@ import { setHelloResponder, destroyDisplayPublisher, isPublisherReady, + getActiveGymId, initDisplaySubscriber, subscribeToDisplay, publishHello, destroyDisplaySubscriber, + getSubscriberStatus, type DisplayEventHandlers, } from '@/lib/display-realtime' import { getGymChannelName } from '@/lib/gym-channel' @@ -26,7 +28,7 @@ import { getGymChannelName } from '@/lib/gym-channel' type BroadcastCallback = (msg: { payload: unknown }) => void type SubscribeCallback = (status: string, err?: unknown) => void -function createMockClient() { +function createMockClient({ autoSubscribe = true }: { autoSubscribe?: boolean } = {}) { const mockSend = vi.fn().mockResolvedValue('ok') const broadcastHandlers = new Map() let subscribeCallback: SubscribeCallback | null = null @@ -35,7 +37,7 @@ function createMockClient() { send: mockSend, subscribe: vi.fn().mockImplementation((cb: SubscribeCallback) => { subscribeCallback = cb - cb('SUBSCRIBED') + if (autoSubscribe) cb('SUBSCRIBED') return mockChannel }), unsubscribe: vi.fn(), @@ -58,6 +60,7 @@ function createMockClient() { mockSend, broadcastHandlers, getSubscribeCallback: () => subscribeCallback, + fireSubscribeCallback: (status: string, err?: unknown) => subscribeCallback?.(status, err), } } @@ -74,6 +77,7 @@ function createMockHandlers(): DisplayEventHandlers { // Test gym IDs const GYM_A = 'gym-a-0000-0000-000000000000' +const GYM_B = 'gym-b-0000-0000-000000000000' // Minimal valid DisplaySnapshot fixture const SNAPSHOT_FIXTURE: DisplaySnapshot = { @@ -96,6 +100,23 @@ const SNAPSHOT_FIXTURE: DisplaySnapshot = { is_visible: true, } +// Minimal valid IdleSnapshot fixture +const IDLE_SNAPSHOT_FIXTURE: IdleSnapshot = { + server_time: '2026-04-12T10:00:00Z', + scheduled_sessions: [ + { + display_name: 'Test User', + session_name: 'Morning Strength', + session_type: 'STRENGTH', + day_label: 'Sunday', + }, + ], + next_session: { + display_name: 'Test User', + session_name: 'Morning Strength', + }, +} + const BC_OPTS = { config: { broadcast: { ack: false, self: false } } } // --------------------------------------------------------------------------- @@ -389,3 +410,315 @@ describe('Hello handshake', () => { expect(responder).toHaveBeenCalledTimes(1) }) }) + +// =========================================================================== +// S015-T: subscribeToDisplay throws when uninitialized +// =========================================================================== + +describe('Subscriber (uninitialized guards)', () => { + it('throws when called before initDisplaySubscriber', () => { + // No initDisplaySubscriber — _client is null from afterEach cleanup + expect(() => subscribeToDisplay({ gymId: GYM_A, handlers: createMockHandlers() })).toThrow( + /Cannot subscribe: no client/, + ) + }) + + it('throws when gymId is an empty string', () => { + const { mockClient } = createMockClient() + initDisplaySubscriber(mockClient) + + expect(() => subscribeToDisplay({ gymId: '', handlers: createMockHandlers() })).toThrow( + /gymId must be a non-empty string/, + ) + }) +}) + +// =========================================================================== +// S016-T: auto-publishHello on reconnect +// =========================================================================== + +describe('Reconnect', () => { + it('does not fire publishHello on the first connection', () => { + const { mockClient, mockSend } = createMockClient() + initDisplaySubscriber(mockClient) + + subscribeToDisplay({ gymId: GYM_A, handlers: createMockHandlers() }) + + expect(mockSend).not.toHaveBeenCalledWith(expect.objectContaining({ event: 'display_hello' })) + }) + + it('auto-fires publishHello on reconnect when previously connected', () => { + const { mockClient, mockSend, getSubscribeCallback } = createMockClient() + initDisplaySubscriber(mockClient) + + subscribeToDisplay({ gymId: GYM_A, handlers: createMockHandlers() }) + + // Clear sends from the initial connect + mockSend.mockClear() + + // Trigger channel error — schedules retry + getSubscribeCallback()?.('CHANNEL_ERROR', new Error('network')) + + // Advance past the first retry delay (2000ms for attempt 1) + vi.advanceTimersByTime(2_000) + + // Retry fires subscribeToDisplay, mock auto-fires SUBSCRIBED, + // _subConnectedBefore=true → publishHello is called + expect(mockSend).toHaveBeenCalledWith({ + type: 'broadcast', + event: 'display_hello', + payload: {}, + }) + }) +}) + +// =========================================================================== +// S017-T: onStatusChange assertions +// =========================================================================== + +describe('Subscriber (onStatusChange)', () => { + it('calls onStatusChange("connected") on SUBSCRIBED', () => { + const { mockClient } = createMockClient() + initDisplaySubscriber(mockClient) + const handlers = createMockHandlers() + + subscribeToDisplay({ gymId: GYM_A, handlers }) + + expect(handlers.onStatusChange).toHaveBeenCalledWith('connected') + }) + + it.each(['TIMED_OUT', 'CHANNEL_ERROR', 'CLOSED'])( + 'calls onStatusChange("reconnecting") for %s', + (status) => { + const { mockClient, getSubscribeCallback } = createMockClient() + initDisplaySubscriber(mockClient) + const handlers = createMockHandlers() + + subscribeToDisplay({ gymId: GYM_A, handlers }) + ;(handlers.onStatusChange as ReturnType).mockClear() + + getSubscribeCallback()?.(status, new Error('test')) + + expect(handlers.onStatusChange).toHaveBeenCalledWith('reconnecting') + }, + ) +}) + +// =========================================================================== +// S018-T: focus, unfocus, idle_snapshot broadcast handlers +// =========================================================================== + +describe('Subscriber (extended event handlers)', () => { + describe('focus broadcast', () => { + it('fires onFocus when subscriber receives a focus broadcast', () => { + const { mockClient, broadcastHandlers } = createMockClient() + initDisplaySubscriber(mockClient) + const handlers = createMockHandlers() + + subscribeToDisplay({ gymId: GYM_A, handlers }) + + const focusCb = broadcastHandlers.get('focus') + expect(focusCb).toBeDefined() + focusCb!({ payload: { user_id: 'user-123' } }) + + expect(handlers.onFocus).toHaveBeenCalledWith({ user_id: 'user-123' }) + }) + }) + + describe('unfocus broadcast', () => { + it('fires onUnfocus when subscriber receives an unfocus broadcast', () => { + const { mockClient, broadcastHandlers } = createMockClient() + initDisplaySubscriber(mockClient) + const handlers = createMockHandlers() + + subscribeToDisplay({ gymId: GYM_A, handlers }) + + const unfocusCb = broadcastHandlers.get('unfocus') + expect(unfocusCb).toBeDefined() + unfocusCb!({ payload: {} }) + + expect(handlers.onUnfocus).toHaveBeenCalledTimes(1) + }) + }) + + describe('idle_snapshot broadcast', () => { + it('fires onIdleSnapshot when subscriber receives a valid idle_snapshot broadcast', () => { + const { mockClient, broadcastHandlers } = createMockClient() + initDisplaySubscriber(mockClient) + const handlers = createMockHandlers() + + subscribeToDisplay({ gymId: GYM_A, handlers }) + + const idleCb = broadcastHandlers.get('idle_snapshot') + expect(idleCb).toBeDefined() + idleCb!({ payload: IDLE_SNAPSHOT_FIXTURE }) + + expect(handlers.onIdleSnapshot).toHaveBeenCalledWith(IDLE_SNAPSHOT_FIXTURE) + }) + }) +}) + +// =========================================================================== +// S019-T: gym-switch channel teardown +// =========================================================================== + +describe('Publisher (gym switch)', () => { + it('tears down the old channel and creates a new one when gymId changes', () => { + const { mockClient } = createMockClient() + initDisplayPublisher(mockClient) + configureDisplayPublisher({ gymId: GYM_A, intent: 'broadcasting' }) + + // Force GYM_A channel creation + publishDisplaySnapshot(SNAPSHOT_FIXTURE) + expect(mockClient.channel).toHaveBeenCalledWith(getGymChannelName(GYM_A), BC_OPTS) + + const removeChannelSpy = mockClient.removeChannel as ReturnType + const removeCallsBefore = removeChannelSpy.mock.calls.length + + // Switch to GYM_B — should tear down GYM_A channel + configureDisplayPublisher({ gymId: GYM_B, intent: 'broadcasting' }) + + expect(mockClient.removeChannel).toHaveBeenCalledTimes(removeCallsBefore + 1) + + // Next publish should create a GYM_B channel + publishDisplaySnapshot(SNAPSHOT_FIXTURE) + expect(mockClient.channel).toHaveBeenLastCalledWith(getGymChannelName(GYM_B), BC_OPTS) + }) +}) + +// =========================================================================== +// S020-T: exponential backoff sequencing +// =========================================================================== + +describe('Backoff', () => { + it('schedules first retry after 2000ms on CHANNEL_ERROR', () => { + const { mockClient, getSubscribeCallback } = createMockClient() + initDisplaySubscriber(mockClient) + + subscribeToDisplay({ gymId: GYM_A, handlers: createMockHandlers() }) + const channelCallsBefore = (mockClient.channel as ReturnType).mock.calls.length + + getSubscribeCallback()?.('CHANNEL_ERROR', new Error('test')) + + vi.advanceTimersByTime(1_999) + expect((mockClient.channel as ReturnType).mock.calls.length).toBe( + channelCallsBefore, + ) + + vi.advanceTimersByTime(1) + expect((mockClient.channel as ReturnType).mock.calls.length).toBe( + channelCallsBefore + 1, + ) + }) + + it('resets retry attempt counter after successful reconnect', () => { + const { mockClient, getSubscribeCallback } = createMockClient() + initDisplaySubscriber(mockClient) + + subscribeToDisplay({ gymId: GYM_A, handlers: createMockHandlers() }) + + // First error: attempt 1, delay=2000ms; retry fires SUBSCRIBED → counter resets to 0 + getSubscribeCallback()?.('CHANNEL_ERROR', new Error('test')) + vi.advanceTimersByTime(2_000) + + // Second error after reset should also use 2000ms, not 4000ms + const channelCallsBefore = (mockClient.channel as ReturnType).mock.calls.length + getSubscribeCallback()?.('CHANNEL_ERROR', new Error('test')) + + vi.advanceTimersByTime(1_999) + expect((mockClient.channel as ReturnType).mock.calls.length).toBe( + channelCallsBefore, + ) + + vi.advanceTimersByTime(1) + expect((mockClient.channel as ReturnType).mock.calls.length).toBe( + channelCallsBefore + 1, + ) + }) + + it('caps retry delay at 30 seconds', () => { + // autoSubscribe=false prevents SUBSCRIBED from resetting _subRetryAttempt, + // allowing the counter to accumulate across consecutive errors. + const { mockClient, getSubscribeCallback } = createMockClient({ autoSubscribe: false }) + initDisplaySubscriber(mockClient) + + subscribeToDisplay({ gymId: GYM_A, handlers: createMockHandlers() }) + + // Drive 4 errors + advance their delays so _subRetryAttempt reaches 4 + // without a successful SUBSCRIBED resetting it. + const delays = [2_000, 4_000, 8_000, 16_000] + for (const delay of delays) { + getSubscribeCallback()?.('CHANNEL_ERROR', new Error('test')) + vi.advanceTimersByTime(delay) + } + + // 5th error: attempt=4 → delay = min(2000 * 2^4, 30000) = min(32000, 30000) = 30000ms + const channelCallsBefore = (mockClient.channel as ReturnType).mock.calls.length + getSubscribeCallback()?.('CHANNEL_ERROR', new Error('test')) + + vi.advanceTimersByTime(29_999) + expect((mockClient.channel as ReturnType).mock.calls.length).toBe( + channelCallsBefore, + ) + + vi.advanceTimersByTime(1) + expect((mockClient.channel as ReturnType).mock.calls.length).toBe( + channelCallsBefore + 1, + ) + }) +}) + +// =========================================================================== +// S021-T: getActiveGymId and getSubscriberStatus state transitions +// =========================================================================== + +describe('Getter state transitions', () => { + describe('getActiveGymId', () => { + it('returns null when publisher is not configured', () => { + expect(getActiveGymId()).toBeNull() + }) + + it('returns the configured gymId while broadcasting', () => { + const { mockClient } = createMockClient() + initDisplayPublisher(mockClient) + configureDisplayPublisher({ gymId: GYM_A, intent: 'broadcasting' }) + + expect(getActiveGymId()).toBe(GYM_A) + }) + + it('returns null after destroyDisplayPublisher', () => { + const { mockClient } = createMockClient() + initDisplayPublisher(mockClient) + configureDisplayPublisher({ gymId: GYM_A, intent: 'broadcasting' }) + + destroyDisplayPublisher() + + expect(getActiveGymId()).toBeNull() + }) + }) + + describe('getSubscriberStatus', () => { + it('returns "disconnected" before subscribing', () => { + expect(getSubscriberStatus()).toBe('disconnected') + }) + + it('returns "connected" after SUBSCRIBED', () => { + const { mockClient } = createMockClient() + initDisplaySubscriber(mockClient) + + subscribeToDisplay({ gymId: GYM_A, handlers: createMockHandlers() }) + + expect(getSubscriberStatus()).toBe('connected') + }) + + it('returns "reconnecting" after CHANNEL_ERROR', () => { + const { mockClient, getSubscribeCallback } = createMockClient() + initDisplaySubscriber(mockClient) + + subscribeToDisplay({ gymId: GYM_A, handlers: createMockHandlers() }) + getSubscribeCallback()?.('CHANNEL_ERROR', new Error('test')) + + expect(getSubscriberStatus()).toBe('reconnecting') + }) + }) +}) From f68501b02bf0ff5c13d3c54d37a4d6fc434a8f0d Mon Sep 17 00:00:00 2001 From: Robert Hamilton Date: Sun, 12 Apr 2026 18:55:16 -0500 Subject: [PATCH 5/5] docs(display): mark P16 review fully verified --- .../0016-pr106-simplify-display-subsystem-2026-04-12.md | 2 +- src/routes/_authenticated/library.tsx | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/Context/Reviews/0016-pr106-simplify-display-subsystem-2026-04-12.md b/Context/Reviews/0016-pr106-simplify-display-subsystem-2026-04-12.md index 96e48789..ac42a8de 100644 --- a/Context/Reviews/0016-pr106-simplify-display-subsystem-2026-04-12.md +++ b/Context/Reviews/0016-pr106-simplify-display-subsystem-2026-04-12.md @@ -164,7 +164,7 @@ Import path changes in the 15 caller files are correct. - [x] All [TASK] findings added to Steps.md or test file (P16-003, P16-008, P16-009, P16-010, P16-012, P16-013, P16-016) - [x] All [ADR] findings have ADRs created or dismissed (P16-015) - [x] All [RULE] findings applied or dismissed (none) -- [ ] Review verified by review-verify agent +- [x] Review verified by review-verify agent ## Resolution Summary diff --git a/src/routes/_authenticated/library.tsx b/src/routes/_authenticated/library.tsx index 4d87eeda..1a515864 100644 --- a/src/routes/_authenticated/library.tsx +++ b/src/routes/_authenticated/library.tsx @@ -49,7 +49,7 @@ import { useDebouncedValue } from '@/hooks/use-debounced-value' import { useAuth } from '@/lib/auth' import { useActiveWorkout } from '@/hooks/use-active-workout' import { useGymPicker } from '@/hooks/use-gym-picker' -import { configureDisplayPublisher } from '@/lib/display-publisher' +import { configureDisplayPublisher } from '@/lib/display-realtime' import { writeLastGymChoice, readLastGymChoice } from '@/lib/gym-picker-storage' import { toast } from 'sonner' import { OnboardingHint } from '@/components/onboarding/onboarding-hint' @@ -1100,4 +1100,3 @@ function ProgramCard({ ) } -