fix: sync YouTube live panel mute state with native player controls

PR_285_REVIEW.md

@@ -0,0 +1,87 @@
+# PR #285 Review — "fix: sync YouTube live panel mute state with native player controls"
+
+## Summary
+
+Clean, focused bug fix across 2 files (+53 / -1). Synchronizes the LiveNewsPanel's mute button with the YouTube player's native mute controls, which previously could drift out of sync.
+
+**Verdict: Approve with minor suggestions.**
+
+---
+
+## How It Works
+
+Two parallel sync paths cover both player modes:
+
+### Path 1: Desktop embed proxy (`api/youtube/embed.js`)
+- New `readMuted()` helper checks `player.isMuted()` with `getVolume() === 0` fallback
+- `startMuteSync()` polls at 500ms, posts `yt-mute-state` message to parent **only on state change** — avoids unnecessary message traffic
+- Called from `onReady` callback so polling starts as soon as the player is initialized
+- Panel-side handler (`msg.type === 'yt-mute-state'`) updates `this.isMuted` and calls `updateMuteIcon()`
+
+### Path 2: Native YT IFrame API (`src/components/LiveNewsPanel.ts`)
+- `startMuteSyncPolling()` polls `syncMuteStateFromPlayer()` at 500ms
+- `syncMuteStateFromPlayer()` short-circuits immediately if `this.useDesktopEmbedProxy` — no double-sync when using the embed proxy path
+- Reads mute state via `isMuted()` or `getVolume() === 0` fallback, same as embed side
+- Updates `this.isMuted` + `updateMuteIcon()` on change
+
+### Cleanup
+- `stopMuteSyncPolling()` called in both `destroyPlayer()` and `destroy()` — no leaked intervals
+- `getVolume?(): number` correctly added to the `YouTubePlayer` type definition
+
+---
+
+## What's Good
+
+- **No double-sync**: The `useDesktopEmbedProxy` guard in `syncMuteStateFromPlayer()` prevents both paths from running simultaneously
+- **Change-only messaging**: The embed only posts `yt-mute-state` when the value actually changes (`m !== lastMuted`), not every 500ms
+- **Proper lifecycle management**: Intervals are cleaned up on player destruction and panel teardown
+- **Defensive null checks**: `readMuted()` returns `null` if player isn't ready; callers handle it gracefully
+
+---
+
+## Suggestions
+
+### 1. No cleanup of `muteSyncIntervalId` in the embed on player destruction
+
+In `embed.js`, `muteSyncIntervalId` is set but never cleared. If the player errors out or the iframe is reloaded, the interval keeps firing against a dead player. Consider clearing it in `onError` or adding a `stopMuteSync()` function:
+
+```js
+function stopMuteSync(){
+  if(muteSyncIntervalId){clearInterval(muteSyncIntervalId);muteSyncIntervalId=null}
+}
+```
+
+This is low-risk since the iframe gets destroyed entirely on channel switch, but it's good hygiene.
+
+### 2. `postMessage` uses `'*'` as target origin
+
+```js
+window.parent.postMessage({type:'yt-mute-state',muted:lastMuted},'*');
+```
+
+This is acceptable here since the embed is served from a Vercel edge function and the parent origin varies, but worth noting. The panel-side handler should ideally validate `event.origin` against the expected embed URL to prevent spoofed `yt-mute-state` messages from other iframes. Currently the handler at line ~211 doesn't check origin:
+
+```ts
+} else if (msg.type === 'yt-mute-state') {
+  const muted = msg.muted === true;
+```
+
+Low severity since the worst case is toggling a mute icon, but worth hardening.
+
+### 3. Consider using `onVolumeChange` YouTube event instead of polling
+
+The YouTube IFrame API fires a `volumechange` event (via `onApiChange`) that could replace the 500ms polling approach. This would be more efficient and responsive than polling, though the IFrame API's event support can be inconsistent across browsers, so polling is the safer choice. Just noting the alternative.
+
+### 4. Minor: `MUTE_SYNC_POLL_MS` as instance property
+
+```ts
+private readonly MUTE_SYNC_POLL_MS = 500;
+```
+
+This is an instance property but behaves as a constant. A `static readonly` or a module-level `const` would be more idiomatic, but this is purely stylistic.
+
+---
+
+## Overall
+
+Solid, well-scoped fix. The dual-path approach correctly handles both the native API and desktop embed proxy cases. Cleanup is thorough. The suggestions above are minor hardening — nothing blocking.

api/youtube/embed.js

@@ -79,8 +79,26 @@ export default async function handler(request) {
     var tag=document.createElement('script');
     tag.src='https://www.youtube.com/iframe_api';
     document.head.appendChild(tag);
-    var player,overlay=document.getElementById('play-overlay'),started=false;
+    var player,overlay=document.getElementById('play-overlay'),started=false,muteSyncIntervalId;
     function hideOverlay(){overlay.classList.add('hidden')}
+    function readMuted(){
+      if(!player)return null;
+      if(typeof player.isMuted==='function')return player.isMuted();
+      if(typeof player.getVolume==='function')return player.getVolume()===0;
+      return null;
+    }
+    function stopMuteSync(){
+      if(muteSyncIntervalId){clearInterval(muteSyncIntervalId);muteSyncIntervalId=null}
+    }
+    function startMuteSync(){
+      if(muteSyncIntervalId)return;
+      var lastMuted=readMuted();
+      if(lastMuted!==null)window.parent.postMessage({type:'yt-mute-state',muted:lastMuted},'*');
+      muteSyncIntervalId=setInterval(function(){
+        var m=readMuted();
+        if(m!==null&&m!==lastMuted){lastMuted=m;window.parent.postMessage({type:'yt-mute-state',muted:m},'*')}
+      },500);
+    }
     function onYouTubeIframeAPIReady(){
       player=new YT.Player('player',{
         videoId:'${videoId}',
@@ -90,8 +108,9 @@ export default async function handler(request) {
           onReady:function(){
             window.parent.postMessage({type:'yt-ready'},'*');
             if(${autoplay}===1){player.playVideo()}
+            startMuteSync();
           },
-          onError:function(e){window.parent.postMessage({type:'yt-error',code:e.data},'*')},
+          onError:function(e){stopMuteSync();window.parent.postMessage({type:'yt-error',code:e.data},'*')},
           onStateChange:function(e){
             window.parent.postMessage({type:'yt-state',state:e.data},'*');
             if(e.data===1||e.data===3){hideOverlay();started=true}

src/components/LiveNewsPanel.ts

@@ -15,6 +15,7 @@ type YouTubePlayer = {
   loadVideoById(videoId: string): void;
   cueVideoById(videoId: string): void;
   getIframe?(): HTMLIFrameElement;
+  getVolume?(): number;
   destroy(): void;
 };
 
@@ -74,58 +75,6 @@ const TECH_LIVE_CHANNELS: LiveChannel[] = [
   { id: 'nasa', name: 'NASA TV', handle: '@NASA', fallbackVideoId: 'fO9e9jnhYK8', useFallbackOnly: true },
 ];
 
-// Optional channels users can add from the "Available Channels" tab UI
-export const OPTIONAL_LIVE_CHANNELS: LiveChannel[] = [
-  // North America
-  { id: 'livenow-fox', name: 'LiveNOW from FOX', handle: '@LiveNOWfromFOX' },
-  { id: 'fox-news', name: 'Fox News', handle: '@FoxNews', fallbackVideoId: 'QaftgYkG-ek', useFallbackOnly: true },
-  { id: 'newsmax', name: 'Newsmax', handle: '@NEWSMAX', fallbackVideoId: 'cZikyozILOY', useFallbackOnly: true },
-  { id: 'abc-news', name: 'ABC News', handle: '@ABCNews' },
-  { id: 'cbs-news', name: 'CBS News', handle: '@CBSNews' },
-  { id: 'nbc-news', name: 'NBC News', handle: '@NBCNews' },
-  // Europe
-  { id: 'welt', name: 'WELT', handle: '@WELTNachrichtensender' },
-  { id: 'rtve', name: 'RTVE 24H', handle: '@RTVENoticias', fallbackVideoId: '7_srED6k0bE' },
-  { id: 'trt-haber', name: 'TRT Haber', handle: '@trthaber' },
-  { id: 'ntv-turkey', name: 'NTV', handle: '@NTV' },
-  { id: 'cnn-turk', name: 'CNN TURK', handle: '@cnnturk' },
-  { id: 'tv-rain', name: 'TV Rain', handle: '@tvrain' },
-  // Latin America & Portuguese
-  { id: 'cnn-brasil', name: 'CNN Brasil', handle: '@CNNbrasil', fallbackVideoId: '1kWRw-DA6Ns' },
-  { id: 'jovem-pan', name: 'Jovem Pan News', handle: '@jovempannews' },
-  { id: 'record-news', name: 'Record News', handle: '@recordnewsoficial' },
-  { id: 'band-jornalismo', name: 'Band Jornalismo', handle: '@BandJornalismo' },
-  { id: 'tn-argentina', name: 'TN (Todo Noticias)', handle: '@todonoticias', fallbackVideoId: 'cb12KmMMDJA' },
-  { id: 'c5n', name: 'C5N', handle: '@c5n', fallbackVideoId: 'NdQSOItOQ5Y' },
-  { id: 'milenio', name: 'MILENIO', handle: '@MILENIO' },
-  { id: 'noticias-caracol', name: 'Noticias Caracol', handle: '@NoticiasCaracol' },
-  { id: 'ntn24', name: 'NTN24', handle: '@NTN24' },
-  { id: 't13', name: 'T13', handle: '@T13' },
-  // Asia
-  { id: 'tbs-news', name: 'TBS NEWS DIG', handle: '@tbsnewsdig', fallbackVideoId: 'ohI356mwBp8' },
-  { id: 'ann-news', name: 'ANN News', handle: '@ANNnewsCH' },
-  { id: 'ntv-news', name: 'NTV News (Japan)', handle: '@ntv_news' },
-  { id: 'cti-news', name: 'CTI News (Taiwan)', handle: '@CtiTv', fallbackVideoId: 'wUPPkSANpyo', useFallbackOnly: true },
-  { id: 'wion', name: 'WION', handle: '@WIONews' },
-  { id: 'vtc-now', name: 'VTC NOW', handle: '@VTCNOW' },
-  { id: 'cna-asia', name: 'CNA (NewsAsia)', handle: '@channelnewsasia' },
-  { id: 'nhk-world', name: 'NHK World Japan', handle: '@NHKWORLDJAPAN' },
-  // Africa
-  { id: 'africanews', name: 'Africanews', handle: '@africanews' },
-  { id: 'channels-tv', name: 'Channels TV', handle: '@channelstv' },
-  { id: 'ktn-news', name: 'KTN News', handle: '@KTNNewsKE' },
-  { id: 'enca', name: 'eNCA', handle: '@enewschannel' },
-  { id: 'sabc-news', name: 'SABC News', handle: '@SABCNews' },
-];
-
-export const OPTIONAL_CHANNEL_REGIONS: { key: string; labelKey: string; channelIds: string[] }[] = [
-  { key: 'na', labelKey: 'components.liveNews.regionNorthAmerica', channelIds: ['livenow-fox', 'fox-news', 'newsmax', 'abc-news', 'cbs-news', 'nbc-news'] },
-  { key: 'eu', labelKey: 'components.liveNews.regionEurope', channelIds: ['welt', 'rtve', 'trt-haber', 'ntv-turkey', 'cnn-turk', 'tv-rain'] },
-  { key: 'latam', labelKey: 'components.liveNews.regionLatinAmerica', channelIds: ['cnn-brasil', 'jovem-pan', 'record-news', 'band-jornalismo', 'tn-argentina', 'c5n', 'milenio', 'noticias-caracol', 'ntn24', 't13'] },
-  { key: 'asia', labelKey: 'components.liveNews.regionAsia', channelIds: ['tbs-news', 'ann-news', 'ntv-news', 'cti-news', 'wion', 'vtc-now', 'cna-asia', 'nhk-world'] },
-  { key: 'africa', labelKey: 'components.liveNews.regionAfrica', channelIds: ['africanews', 'channels-tv', 'ktn-news', 'enca', 'sabc-news'] },
-];
-
 const DEFAULT_LIVE_CHANNELS = SITE_VARIANT === 'tech' ? TECH_LIVE_CHANNELS : FULL_LIVE_CHANNELS;
 
 /** Default channel list for the current variant (for restore in channel management). */
@@ -147,7 +96,6 @@ const DEFAULT_STORED: StoredLiveChannels = {
 export const BUILTIN_IDS = new Set([
   ...FULL_LIVE_CHANNELS.map((c) => c.id),
   ...TECH_LIVE_CHANNELS.map((c) => c.id),
-  ...OPTIONAL_LIVE_CHANNELS.map((c) => c.id),
 ]);
 
 export function loadChannelsFromStorage(): LiveChannel[] {
@@ -156,7 +104,6 @@ export function loadChannelsFromStorage(): LiveChannel[] {
   const channelMap = new Map<string, LiveChannel>();
   for (const c of FULL_LIVE_CHANNELS) channelMap.set(c.id, { ...c });
   for (const c of TECH_LIVE_CHANNELS) channelMap.set(c.id, { ...c });
-  for (const c of OPTIONAL_LIVE_CHANNELS) channelMap.set(c.id, { ...c });
   for (const c of stored.custom ?? []) {
     if (c.id && c.handle) channelMap.set(c.id, { ...c });
   }
@@ -177,7 +124,7 @@ export function saveChannelsToStorage(channels: LiveChannel[]): void {
   const order = channels.map((c) => c.id);
   const custom = channels.filter((c) => !BUILTIN_IDS.has(c.id));
   const builtinNames = new Map<string, string>();
-  for (const c of [...FULL_LIVE_CHANNELS, ...TECH_LIVE_CHANNELS, ...OPTIONAL_LIVE_CHANNELS]) builtinNames.set(c.id, c.name);
+  for (const c of [...FULL_LIVE_CHANNELS, ...TECH_LIVE_CHANNELS]) builtinNames.set(c.id, c.name);
   const displayNameOverrides: Record<string, string> = {};
   for (const c of channels) {
     if (builtinNames.has(c.id) && c.name !== builtinNames.get(c.id)) {
@@ -218,6 +165,8 @@ export class LiveNewsPanel extends Panel {
   private desktopEmbedIframe: HTMLIFrameElement | null = null;
   private desktopEmbedRenderToken = 0;
   private boundMessageHandler!: (e: MessageEvent) => void;
+  private muteSyncInterval: ReturnType<typeof setInterval> | null = null;
+  private static readonly MUTE_SYNC_POLL_MS = 500;
 
   constructor() {
     super({ id: 'live-news', title: t('panels.liveNews') });
@@ -262,6 +211,12 @@ export class LiveNewsPanel extends Panel {
         } else {
           this.showEmbedError(this.activeChannel, code);
         }
+      } else if (msg.type === 'yt-mute-state') {
+        const muted = msg.muted === true;
+        if (this.isMuted !== muted) {
+          this.isMuted = muted;
+          this.updateMuteIcon();
+        }
       }
     };
     window.addEventListener('message', this.boundMessageHandler);
@@ -328,7 +283,32 @@ export class LiveNewsPanel extends Panel {
     this.destroyPlayer();
   }
 
+  private stopMuteSyncPolling(): void {
+    if (this.muteSyncInterval !== null) {
+      clearInterval(this.muteSyncInterval);
+      this.muteSyncInterval = null;
+    }
+  }
+
+  private startMuteSyncPolling(): void {
+    this.stopMuteSyncPolling();
+    this.muteSyncInterval = setInterval(() => this.syncMuteStateFromPlayer(), LiveNewsPanel.MUTE_SYNC_POLL_MS);
+  }
+
+  private syncMuteStateFromPlayer(): void {
+    if (this.useDesktopEmbedProxy || !this.player || !this.isPlayerReady) return;
+    const p = this.player as { getVolume?(): number; isMuted?(): boolean };
+    const muted = typeof p.isMuted === 'function'
+      ? p.isMuted()
+      : (p.getVolume?.() === 0);
+    if (typeof muted === 'boolean' && muted !== this.isMuted) {
+      this.isMuted = muted;
+      this.updateMuteIcon();
+    }
+  }
+
   private destroyPlayer(): void {
+    this.stopMuteSyncPolling();
     if (this.player) {
       this.player.destroy();
       this.player = null;
@@ -786,6 +766,7 @@ export class LiveNewsPanel extends Panel {
           const iframe = this.player?.getIframe?.();
           if (iframe) iframe.referrerPolicy = 'strict-origin-when-cross-origin';
           this.syncPlayerState();
+          this.startMuteSyncPolling();
         },
         onError: (event) => {
           const errorCode = Number(event?.data ?? 0);
@@ -896,6 +877,7 @@ export class LiveNewsPanel extends Panel {
   }
 
   public destroy(): void {
+    this.stopMuteSyncPolling();
     if (this.idleTimeout) {
       clearTimeout(this.idleTimeout);
       this.idleTimeout = null;