From 6154aaebb28cc4034fd729ac40ef4329a4e6eda8 Mon Sep 17 00:00:00 2001
From: Timothy <timothy.look@gmail.com>
Date: Tue, 21 Jul 2026 22:57:37 +0200
Subject: [PATCH] fix(60): onPlaying reflects real playback, panel is styled,
 availability is type-safe

- HlsPlayer: drive onPlaying from the <video> element's own `playing` event on
  BOTH the hls.js and Safari-native paths instead of MANIFEST_PARSED, which
  fires before any media has decoded (an HttpLiveStreamingDirect manifest
  always parses, even over a black video). MANIFEST_PARSED now only kicks
  play(). Restore `void video.play().catch(...)` at both call sites and stub
  HTMLMediaElement.prototype.play in setupTests.ts instead, so the `?.` that
  existed only to survive jsdom is gone from production code.
- HlsPlayer.test.tsx: assert the auto-recovery guard against hls.js's own
  startLoad()/recoverMediaError(), not just loadSource's call count.
- ChannelPreviewPanel: reuse existing ctv-* classes (ctv-channels-error,
  ctv-settings-warn-callout, ctv-detail-actions, ctv-detail-infogrid) instead
  of five undefined ctv-preview-* classes; add the two genuinely new rules
  (ctv-preview-video max-width, spacing tweaks) to shell.css.
- Add an exported ChannelPreviewAvailability union (web/src/api/channels.ts)
  so a typo like 'ForcedHLSOnly' fails to compile instead of silently
  disabling a branch forever; use it in ChannelPreviewPanel's prop type and
  at the ChannelsScreen comparison sites.
---
 docs/spa-conventions.md                       | 13 +++++---
 web/src/api/channels.ts                       |  6 ++++
 web/src/media/HlsPlayer.test.tsx              | 33 ++++++++++++++++---
 web/src/media/HlsPlayer.tsx                   | 16 ++++++---
 web/src/screens/ChannelsScreen.test.tsx       |  8 ++---
 web/src/screens/ChannelsScreen.tsx            | 20 +++++++++--
 .../channels/ChannelPreviewPanel.test.tsx     | 23 +++++++++++--
 .../screens/channels/ChannelPreviewPanel.tsx  | 19 +++++++----
 web/src/setupTests.ts                         |  6 ++++
 web/src/shell.css                             | 17 ++++++++++
 10 files changed, 131 insertions(+), 30 deletions(-)

diff --git a/docs/spa-conventions.md b/docs/spa-conventions.md
index d953e4c4b..32f537a44 100644
--- a/docs/spa-conventions.md
+++ b/docs/spa-conventions.md
@@ -216,11 +216,14 @@ never auto-recovered — a diagnostic surface must show the fault rather than re
 identity restarts the stream every render. When mocking `hls.js` in a test that exercises errors,
 include `ERROR` in the mock's `static Events`.
 
-It also takes an optional `onPlaying()`, called once playback has actually started — wired to
-`Hls.Events.MANIFEST_PARSED` on the hls.js path, and to the `<video>` element's own `playing` event
-on the Safari native-HLS path. It is purely additive (omitting it is safe) and mirrors `onError`'s
-requirements exactly: pass a **stable** `onPlaying` (`useCallback`), since it too sits in the attach
-effect's dependency array and an unstable identity would restart the stream every render.
+It also takes an optional `onPlaying()`, called once playback has actually started — wired to the
+`<video>` element's own `playing` event on **both** the hls.js/MSE path and the Safari native-HLS
+path. It is deliberately **not** wired to `Hls.Events.MANIFEST_PARSED`: that fires before any media
+has decoded (the manifest for an `HttpLiveStreamingDirect` channel always parses, even over a black
+video), so it only issues the `video.play()` kick. `onPlaying` is purely additive (omitting it is
+safe) and mirrors `onError`'s requirements exactly: pass a **stable** `onPlaying` (`useCallback`),
+since it too sits in the attach effect's dependency array and an unstable identity would restart the
+stream every render.
 
 ## 5c. Media "Add to…" affordances
 
diff --git a/web/src/api/channels.ts b/web/src/api/channels.ts
index 3e7323d0f..38baf1606 100644
--- a/web/src/api/channels.ts
+++ b/web/src/api/channels.ts
@@ -2,6 +2,12 @@ import { useCallback, useEffect, useRef, useState } from 'react';
 import { ApiError, request } from './client';
 import type { components } from './generated/v1';
 
+// ChannelResponseModel.preview.availability is typed as a bare `string` in the generated types
+// (ChannelPreviewResponseModel.cs uses a plain C# string, not an enum, to keep the OpenAPI schema
+// simple — see the comment there). Hand-maintain the real set of values here so a typo like
+// 'ForcedHLSOnly' fails to compile instead of silently disabling a branch forever.
+export type ChannelPreviewAvailability = 'Available' | 'ForcedHlsOnly' | 'Unavailable';
+
 export type ChannelSummary = components['schemas']['ChannelResponseModel'];
 export type ChannelState = components['schemas']['ChannelStateResponseModel'];
 export type Channel = components['schemas']['ChannelDetailResponseModel'];
diff --git a/web/src/media/HlsPlayer.test.tsx b/web/src/media/HlsPlayer.test.tsx
index c4e0c9099..1337d4841 100644
--- a/web/src/media/HlsPlayer.test.tsx
+++ b/web/src/media/HlsPlayer.test.tsx
@@ -7,7 +7,9 @@ const hlsMock = {
   loadSource: vi.fn(),
   attachMedia: vi.fn(),
   on: vi.fn(),
-  destroy: vi.fn()
+  destroy: vi.fn(),
+  startLoad: vi.fn(),
+  recoverMediaError: vi.fn()
 };
 
 vi.mock('hls.js', () => {
@@ -20,6 +22,8 @@ vi.mock('hls.js', () => {
     attachMedia = hlsMock.attachMedia;
     on = hlsMock.on;
     destroy = hlsMock.destroy;
+    startLoad = hlsMock.startLoad;
+    recoverMediaError = hlsMock.recoverMediaError;
   }
   return { default: MockHls };
 });
@@ -58,11 +62,17 @@ describe('HlsPlayer', () => {
     const errorHandler = hlsMock.on.mock.calls.find((call) => call[0] === 'hlsError')?.[1];
     errorHandler({}, { fatal: true, type: 'networkError', details: 'manifestLoadError' });
 
-    // A diagnostic player must surface the fault, not silently retry past it.
+    // A diagnostic player must surface the fault, not silently retry past it — auto-recovery is
+    // hls.js's own startLoad()/recoverMediaError(), so assert those directly rather than only
+    // inferring it from loadSource's call count.
     expect(hlsMock.loadSource).toHaveBeenCalledTimes(1);
+    expect(hlsMock.startLoad).not.toHaveBeenCalled();
+    expect(hlsMock.recoverMediaError).not.toHaveBeenCalled();
   });
 
-  it('reports onPlaying when hls.js parses the manifest', () => {
+  it('does not report onPlaying when hls.js merely parses the manifest', () => {
+    // MANIFEST_PARSED fires before any media has decoded — an HttpLiveStreamingDirect manifest
+    // always parses even over a black video, so onPlaying must not be driven by it.
     const onPlaying = vi.fn();
     render(<HlsPlayer onPlaying={onPlaying} src="/iptv/channel/12.1.m3u8" />);
 
@@ -71,13 +81,28 @@ describe('HlsPlayer', () => {
 
     manifestHandler();
 
+    expect(onPlaying).not.toHaveBeenCalled();
+  });
+
+  it('reports onPlaying when the video element fires its real playing event', () => {
+    const onPlaying = vi.fn();
+    const { container } = render(<HlsPlayer onPlaying={onPlaying} src="/iptv/channel/12.1.m3u8" />);
+
+    const video = container.querySelector('video');
+    expect(video).not.toBeNull();
+
+    video?.dispatchEvent(new Event('playing'));
+
     expect(onPlaying).toHaveBeenCalledTimes(1);
   });
 
   it('is safe with no onPlaying provided', () => {
-    render(<HlsPlayer src="/iptv/channel/12.1.m3u8" />);
+    const { container } = render(<HlsPlayer src="/iptv/channel/12.1.m3u8" />);
 
     const manifestHandler = hlsMock.on.mock.calls.find((call) => call[0] === 'hlsManifestParsed')?.[1];
     expect(() => manifestHandler()).not.toThrow();
+
+    const video = container.querySelector('video');
+    expect(() => video?.dispatchEvent(new Event('playing'))).not.toThrow();
   });
 });
diff --git a/web/src/media/HlsPlayer.tsx b/web/src/media/HlsPlayer.tsx
index f04507f0b..96c3cdce4 100644
--- a/web/src/media/HlsPlayer.tsx
+++ b/web/src/media/HlsPlayer.tsx
@@ -15,8 +15,10 @@ export interface HlsPlayerProps {
   // Called with a human-readable reason when playback fails. Fatal errors are reported, never
   // auto-recovered: a diagnostic player that silently retries hides the fault it exists to reveal.
   onError?: (message: string) => void;
-  // Called once playback has actually started (hls.js MANIFEST_PARSED, or the native-HLS `<video>`
-  // element's `playing` event on Safari). Optional and purely additive — omitting it is safe.
+  // Called once playback has actually started — wired to the `<video>` element's own `playing`
+  // event on both the hls.js/MSE path and the native-HLS (Safari) path, since MANIFEST_PARSED fires
+  // before any media has decoded (an HttpLiveStreamingDirect manifest always parses even over a
+  // black video). Optional and purely additive — omitting it is safe.
   onPlaying?: () => void;
 }
 
@@ -61,8 +63,7 @@ export function HlsPlayer({ src, playToken = 0, className, style, onError, onPla
       hls.loadSource(src);
       hls.attachMedia(video);
       hls.on(Hls.Events.MANIFEST_PARSED, () => {
-        onPlaying?.();
-        video.play()?.catch(() => {
+        void video.play().catch(() => {
           // Autoplay may be blocked; the user can press play manually.
         });
       });
@@ -71,8 +72,13 @@ export function HlsPlayer({ src, playToken = 0, className, style, onError, onPla
           onError?.(`${data.type}: ${data.details}`);
         }
       });
+      const onVideoPlaying = () => {
+        onPlaying?.();
+      };
+      video.addEventListener('playing', onVideoPlaying);
 
       return () => {
+        video.removeEventListener('playing', onVideoPlaying);
         destroyHls();
       };
     }
@@ -80,7 +86,7 @@ export function HlsPlayer({ src, playToken = 0, className, style, onError, onPla
     if (video.canPlayType('application/vnd.apple.mpegurl')) {
       video.src = src;
       const onCanPlay = () => {
-        video.play()?.catch(() => {
+        void video.play().catch(() => {
           // Autoplay may be blocked; the user can press play manually.
         });
       };
diff --git a/web/src/screens/ChannelsScreen.test.tsx b/web/src/screens/ChannelsScreen.test.tsx
index 426b8e032..ab6b23c2e 100644
--- a/web/src/screens/ChannelsScreen.test.tsx
+++ b/web/src/screens/ChannelsScreen.test.tsx
@@ -44,9 +44,9 @@ function channelRow(overrides: Record<string, unknown> = {}): Record<string, unk
     // The API always sends playoutCount; default to a healthy channel so only tests that opt in
     // render the "No playout" fault state.
     playoutCount: 1,
-    // Server-derived preview eligibility (Task 2). Defaults to the conservative "no legacy preview
-    // support data" case most existing fixtures assume; tests that need a different availability
-    // override this explicitly.
+    // Server-derived preview eligibility (Task 2). Defaults to Unavailable so most existing
+    // fixtures keep asserting the disabled preview button; tests that need a different
+    // availability override this explicitly.
     preview: { availability: 'Unavailable', manifestUrl: null, unavailableReason: null },
     showInEpg: true,
     sortNumber: 5.1,
@@ -284,7 +284,7 @@ describe('ChannelsScreen — load + render', () => {
     expect(fetchCount(handle, '/api/v1/channels')).toBe(channelFetchesBeforePoll);
   });
 
-  it('keeps preview disabled because the list DTO lacks legacy preview support data', async () => {
+  it('keeps preview disabled when the fixture defaults to Unavailable', async () => {
     mockApi({
       channels: [channelRow({ streamingMode: 'HLS Segmenter' })],
       channelStates: [stateRow({ onAir: true })]
diff --git a/web/src/screens/ChannelsScreen.tsx b/web/src/screens/ChannelsScreen.tsx
index c110a8323..73607afcd 100644
--- a/web/src/screens/ChannelsScreen.tsx
+++ b/web/src/screens/ChannelsScreen.tsx
@@ -22,6 +22,7 @@ import {
   getWatermarks,
   messageFromError,
   useChannelsScreenQuery,
+  type ChannelPreviewAvailability,
   type ChannelState,
   type ChannelSummary,
   type CreateChannelRequest
@@ -499,7 +500,16 @@ export function ChannelsScreen() {
           }
           return (
             <ChannelPreviewPanel
-              channel={previewChannel}
+              channel={{
+                ...previewChannel,
+                preview: {
+                  ...previewChannel.preview,
+                  // The generated ChannelSummary types `availability` as a bare `string` (see
+                  // web/src/api/channels.ts); narrow it once here so ChannelPreviewPanel's own
+                  // comparisons are checked against the real ChannelPreviewAvailability union.
+                  availability: previewChannel.preview.availability as ChannelPreviewAvailability
+                }
+              }}
               nowPlaying={statesById.get(previewChannelId)?.nowPlaying ?? null}
               onClose={() => setPreviewChannelId(null)}
               open
@@ -577,6 +587,10 @@ function ChannelTableRow({
   const dim = !channel.isEnabled;
   const progress = state ? progressFromChannelState(state) : null;
   const streamingTone = streamingModeBadgeTones[channel.streamingMode] ?? 'neutral';
+  // Narrow once: ChannelSummary types `preview.availability` as a bare `string` (see
+  // web/src/api/channels.ts), so comparing the union-typed local below against a mistyped literal
+  // (e.g. 'UnavailableX') is a compile error, where comparing the raw string field would not be.
+  const previewAvailability = channel.preview.availability as ChannelPreviewAvailability;
 
   return (
     <tr className={`${live ? 'ctv-channel-row-live ' : ''}${selected ? 'ctv-channel-row-selected ' : ''}${dim ? 'ctv-channel-row-dim' : ''}`}>
@@ -620,11 +634,11 @@ function ChannelTableRow({
       <td>
         <div className="ctv-channel-actions">
           <IconButton
-            disabled={channel.preview.availability === 'Unavailable'}
+            disabled={previewAvailability === 'Unavailable'}
             onClick={() => onPreview(channel.id)}
             size="sm"
             title={
-              channel.preview.availability === 'Unavailable'
+              previewAvailability === 'Unavailable'
                 ? channel.preview.unavailableReason
                   ? `Preview unavailable for ${channel.name} — ${channel.preview.unavailableReason}`
                   : `Preview unavailable for ${channel.name}`
diff --git a/web/src/screens/channels/ChannelPreviewPanel.test.tsx b/web/src/screens/channels/ChannelPreviewPanel.test.tsx
index ce97196a3..c50ff281a 100644
--- a/web/src/screens/channels/ChannelPreviewPanel.test.tsx
+++ b/web/src/screens/channels/ChannelPreviewPanel.test.tsx
@@ -1,6 +1,7 @@
 import { act } from 'react';
 import { cleanup, fireEvent, render, screen } from '@testing-library/react';
 import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
+import type { ChannelPreviewAvailability } from '../../api/channels';
 import { ChannelPreviewPanel } from './ChannelPreviewPanel';
 
 const hlsMock = {
@@ -30,7 +31,7 @@ function channel(preview: ChannelPreview, streamingMode = 'HLS Segmenter', id =
 }
 
 interface ChannelPreview {
-  availability: string;
+  availability: ChannelPreviewAvailability;
   manifestUrl: null | string;
   unavailableReason: null | string;
 }
@@ -195,7 +196,8 @@ describe('ChannelPreviewPanel', () => {
     expect(screen.queryByText(/networkError: manifestLoadError/)).not.toBeInTheDocument();
   });
 
-  it('reaches the playing state once hls.js reports MANIFEST_PARSED', () => {
+  it('stays in starting once hls.js merely reports MANIFEST_PARSED', () => {
+    // MANIFEST_PARSED fires before any media has decoded, so it must not be enough to reach playing.
     render(
       <ChannelPreviewPanel channel={channel(available)} nowPlaying={null} onClose={vi.fn()} open />
     );
@@ -207,6 +209,23 @@ describe('ChannelPreviewPanel', () => {
       manifestHandler();
     });
 
+    expect(screen.getByText('starting')).toBeInTheDocument();
+  });
+
+  it('reaches the playing state once the video element fires its real playing event', () => {
+    render(
+      <ChannelPreviewPanel channel={channel(available)} nowPlaying={null} onClose={vi.fn()} open />
+    );
+
+    expect(screen.getByText('starting')).toBeInTheDocument();
+
+    // SlideOver renders into a document.body portal, not the render() container.
+    const video = document.body.querySelector('video');
+    expect(video).not.toBeNull();
+    act(() => {
+      video?.dispatchEvent(new Event('playing'));
+    });
+
     expect(screen.getByText('playing')).toBeInTheDocument();
   });
 
diff --git a/web/src/screens/channels/ChannelPreviewPanel.tsx b/web/src/screens/channels/ChannelPreviewPanel.tsx
index 6439dbfe8..b36bda6a5 100644
--- a/web/src/screens/channels/ChannelPreviewPanel.tsx
+++ b/web/src/screens/channels/ChannelPreviewPanel.tsx
@@ -1,4 +1,5 @@
 import { useCallback, useState } from 'react';
+import type { ChannelPreviewAvailability } from '../../api/channels';
 import { Button } from '../../components/forms';
 import { SlideOver } from '../../components/overlay';
 import { HlsPlayer } from '../../media/HlsPlayer';
@@ -20,7 +21,11 @@ export interface ChannelPreviewPanelProps {
     name: string;
     number: string;
     streamingMode: string;
-    preview: { availability: string; manifestUrl: null | string; unavailableReason: null | string };
+    preview: {
+      availability: ChannelPreviewAvailability;
+      manifestUrl: null | string;
+      unavailableReason: null | string;
+    };
   };
   nowPlaying: ChannelPreviewNowPlaying | null;
 }
@@ -89,13 +94,13 @@ export function ChannelPreviewPanel({ channel, nowPlaying, onClose, open }: Chan
   return (
     <SlideOver onClose={onClose} open={open} subtitle={`Channel ${channel.number}`} title={`Preview: ${channel.name}`}>
       {availability === 'Unavailable' ? (
-        <p className="ctv-preview-unavailable" role="alert">
+        <div className="ctv-channels-error" role="alert">
           {unavailableReason}
-        </p>
+        </div>
       ) : (
         <>
           {forced && (
-            <p className="ctv-preview-caveat">{FORCED_HLS_CAVEAT}</p>
+            <p className="ctv-settings-warn-callout ctv-preview-caveat">{FORCED_HLS_CAVEAT}</p>
           )}
           {forced && !started && (
             <Button onClick={onOptIn} variant="secondary">
@@ -110,9 +115,9 @@ export function ChannelPreviewPanel({ channel, nowPlaying, onClose, open }: Chan
                 onPlaying={onPlaying}
                 playToken={playToken}
                 src={src}
-                style={{ aspectRatio: '16 / 9', width: '100%', background: '#000' }}
+                style={{ aspectRatio: '16 / 9', background: '#000' }}
               />
-              <div className="ctv-preview-actions">
+              <div className="ctv-detail-actions">
                 <Button onClick={onRetry} size="sm" variant="secondary">
                   Retry
                 </Button>
@@ -122,7 +127,7 @@ export function ChannelPreviewPanel({ channel, nowPlaying, onClose, open }: Chan
         </>
       )}
 
-      <dl className="ctv-preview-facts">
+      <dl className="ctv-detail-infogrid ctv-preview-facts">
         <dt>Mode</dt>
         <dd>{channel.streamingMode}</dd>
         <dt>URL</dt>
diff --git a/web/src/setupTests.ts b/web/src/setupTests.ts
index 97fc320b7..e56309512 100644
--- a/web/src/setupTests.ts
+++ b/web/src/setupTests.ts
@@ -1,5 +1,11 @@
 import '@testing-library/jest-dom/vitest';
 
+// jsdom has no real media pipeline: HTMLMediaElement.prototype.play is unimplemented and returns
+// undefined rather than a Promise, which would otherwise force production code (HlsPlayer) to guard
+// every call site with `?.` purely to survive tests. Stub it to behave like a real browser instead —
+// tests that need an autoplay-blocked rejection can override it with vi.spyOn(...).mockRejectedValue.
+HTMLMediaElement.prototype.play = () => Promise.resolve();
+
 if (!window.localStorage) {
   const storage = new Map<string, string>();
 
diff --git a/web/src/shell.css b/web/src/shell.css
index f89d611b2..8a68361d9 100644
--- a/web/src/shell.css
+++ b/web/src/shell.css
@@ -1117,6 +1117,23 @@ body {
   gap: 2px;
 }
 
+/* ChannelPreviewPanel (#60) — the video needs a max-width so it never overflows the slide-over
+   body, and the forced-HLS caveat needs a little breathing room around the shared warn-callout. */
+.ctv-preview-video {
+  display: block;
+  width: 100%;
+  max-width: 100%;
+  border-radius: var(--radius-sm);
+}
+
+.ctv-preview-caveat {
+  margin: 0 0 var(--space-6, 12px);
+}
+
+.ctv-preview-facts {
+  margin-top: var(--space-6, 12px);
+}
+
 .ctv-channels-footer {
   display: flex;
   align-items: center;