diff --git a/frontend/eslint-suppressions.json b/frontend/eslint-suppressions.json index 4c8d67eb..ef7880f1 100644 --- a/frontend/eslint-suppressions.json +++ b/frontend/eslint-suppressions.json @@ -85,11 +85,6 @@ "count": 1 } }, - "src/components/PlayerModal.tsx": { - "react-hooks/set-state-in-effect": { - "count": 1 - } - }, "src/components/Playlists.tsx": { "react-hooks/immutability": { "count": 2 @@ -106,9 +101,6 @@ "src/components/PlexPlayer.tsx": { "react-hooks/immutability": { "count": 1 - }, - "react-hooks/set-state-in-effect": { - "count": 1 } }, "src/components/PlexSearchBox.tsx": { diff --git a/frontend/src/components/PlayerModal.tsx b/frontend/src/components/PlayerModal.tsx index 72d9e8c0..23db65a1 100644 --- a/frontend/src/components/PlayerModal.tsx +++ b/frontend/src/components/PlayerModal.tsx @@ -1,4 +1,4 @@ -import { useEffect, useId, useRef, useState } from "react"; +import { useCallback, useEffect, useEffectEvent, useId, useRef, useState } from "react"; import { useTranslation } from "react-i18next"; import { createPortal } from "react-dom"; import { useNavigationActions } from "./navigationContext"; @@ -170,7 +170,10 @@ export default function PlayerModal({ // The whole modal (backdrop + card) — the wheel-to-volume target, so scrolling anywhere over the // modal adjusts volume (not just the small player area). const dialogRef = useRef(null); - const focusModal = () => cardRef.current?.focus(); + // Stable: it reads a ref and nothing else, and three hooks take it as a parameter (useYtVolume, + // useYtNativeControls, useYtBigView) where a fresh identity each render would spread into their + // dep lists too. + const focusModal = useCallback(() => cardRef.current?.focus(), []); // The dialog contract every other `aria-modal` shell shares (ui/dialogFocus): the focus goes back // to whatever opened the player, and the page lock is the one counted lock. It opens focused on the // CARD, not its first button, so Space/F/arrows reach the shortcuts. Until E11 this was the one @@ -241,12 +244,30 @@ export default function PlayerModal({ onState(active.id, next); }; - // On queue advance (active changes), sync local state to the new item. - useEffect(() => { + // On queue advance (active changes), sync local state to the new item — DURING that render, not + // one commit later. As an effect, the frame between showed the new episode's chrome over the + // previous one's watch state and `navigated` flag (currentVideoId still pointed at the old id, so + // the "you have navigated away" affordance flashed on every advance). React discards this pass + // instead of committing it, so there is no such frame; the guard falsifies itself immediately. + const [syncedTo, setSyncedTo] = useState(active.id); + if (syncedTo !== active.id) { + setSyncedTo(active.id); setStatus(active.status); setCurrentVideoId(active.id); + } + + // `currentIdRef` must NOT move into the block above, and the reason is EFFECT ORDERING, which the + // committed test "resumes an item played earlier in the SAME session" measures. useYtProgress's + // per-item effect cleanup persists the outgoing item's position, and its persist() is guarded on + // `currentIdRef.current === id` so a video navigated to inside the player is never checkpointed + // against the library item. React runs every cleanup BEFORE any setup, so in this effect the ref + // still names the item being left while that cleanup runs, and flips to the new one just before + // useYtProgress (declared below) sets up for it. Advanced to render time the flip happens first, + // the outgoing checkpoint is skipped, and stepping next→prev back to an item resumes from the + // frozen launch queue's stale position — which is the exact bug the session-position map exists + // to prevent (748 instead of 830, in the test's numbers). + useEffect(() => { currentIdRef.current = active.id; - // eslint-disable-next-line react-hooks/exhaustive-deps }, [active.id]); const seekTo = (seconds: number) => { @@ -412,7 +433,11 @@ export default function PlayerModal({ // scope) so the help sheet lists them; only the behaviour behind each id is here. // These fire only while focus is on our page, not inside the cross-origin player iframe — so // we focus the modal card on open and again on player-ready (autoplay can grab focus). - useEffect(() => { + // + // The handler itself is a useEffectEvent (React 19.2+): it is bound ONCE per player, but it reads + // whatever is current at the moment a key arrives, which is what the disabled dep list used to + // claim in prose and now is by construction. + const onPlayerKey = useEffectEvent((e: KeyboardEvent) => { // Volume by keyboard (YouTube/Plex-style ±5). The only volume control that survives fullscreen, // where the wheel goes to YouTube's own iframe. But the modal card scrolls (a long description) // and Up/Down is how you scroll it from the keyboard — so defer whenever the focused element can @@ -432,59 +457,61 @@ export default function PlayerModal({ e.preventDefault(); nudgeSeek(secs); }; - const onKey = (e: KeyboardEvent) => { - // The shared dialog Tab trap — only while nothing that owns the keyboard is layered above the - // player: a Modal opened over it (the download dialog) traps for itself. The title's hover card - // does not, so it must not switch the trap off. - if (e.key === "Tab") { - if (layerIdRef.current != null && anyBlockingLayerAbove(layerIdRef.current)) return; - if (dialogRef.current) trapTab(e, dialogRef.current, cardRef.current); - return; - } - dispatch(e, "ytPlayer", { - close: (ev) => { - // Defer to any layer opened above the player FIRST (a Modal like the download dialog, or an - // anchored popover): it takes the Escape and we do nothing — so we don't also step out of - // maximise or close the player underneath it. The consumed-event flag is the reliable signal - // (UX-8): the layer's `document` listener runs before ours and its close can pop it off the - // stack synchronously, so `anyLayerAbove` alone would miss it and close the player too. - if (escapeHandled(ev)) return; - if (layerIdRef.current != null && anyLayerAbove(layerIdRef.current)) return; - // In fullscreen, let the browser's Esc exit fullscreen only — don't also close the modal. - if (fullscreenElement()) return; - // Same for our own maximise: Esc steps back out of it before it closes the player. - if (maximizedRef.current) { - ev.preventDefault(); - setMaximized(false); - return; - } - onClose(); - }, - fullscreen: (ev) => { + // The shared dialog Tab trap — only while nothing that owns the keyboard is layered above the + // player: a Modal opened over it (the download dialog) traps for itself. The title's hover card + // does not, so it must not switch the trap off. + if (e.key === "Tab") { + if (layerIdRef.current != null && anyBlockingLayerAbove(layerIdRef.current)) return; + if (dialogRef.current) trapTab(e, dialogRef.current, cardRef.current); + return; + } + dispatch(e, "ytPlayer", { + close: (ev) => { + // Defer to any layer opened above the player FIRST (a Modal like the download dialog, or an + // anchored popover): it takes the Escape and we do nothing — so we don't also step out of + // maximise or close the player underneath it. The consumed-event flag is the reliable signal + // (UX-8): the layer's `document` listener runs before ours and its close can pop it off the + // stack synchronously, so `anyLayerAbove` alone would miss it and close the player too. + if (escapeHandled(ev)) return; + if (layerIdRef.current != null && anyLayerAbove(layerIdRef.current)) return; + // In fullscreen, let the browser's Esc exit fullscreen only — don't also close the modal. + if (fullscreenElement()) return; + // Same for our own maximise: Esc steps back out of it before it closes the player. + if (maximizedRef.current) { ev.preventDefault(); - toggleFullscreen(); - }, - playPause: (ev) => { - // Let Space activate a focused button/link; otherwise toggle playback (and stop the - // page from scrolling). - if (isActivationTarget(ev)) return; - ev.preventDefault(); - togglePlay(); - }, - volumeUp: (ev) => volume(ev, true), - volumeDown: (ev) => volume(ev, false), - seekBack: (ev) => seek(ev, -5), - seekFwd: (ev) => seek(ev, 5), - prevVideo: (ev) => { - ev.preventDefault(); - goPrev(); - }, - nextVideo: (ev) => { - ev.preventDefault(); - goNext(); - }, - }); - }; + setMaximized(false); + return; + } + onClose(); + }, + fullscreen: (ev) => { + ev.preventDefault(); + toggleFullscreen(); + }, + playPause: (ev) => { + // Let Space activate a focused button/link; otherwise toggle playback (and stop the + // page from scrolling). + if (isActivationTarget(ev)) return; + ev.preventDefault(); + togglePlay(); + }, + volumeUp: (ev) => volume(ev, true), + volumeDown: (ev) => volume(ev, false), + seekBack: (ev) => seek(ev, -5), + seekFwd: (ev) => seek(ev, 5), + prevVideo: (ev) => { + ev.preventDefault(); + goPrev(); + }, + nextVideo: (ev) => { + ev.preventDefault(); + goNext(); + }, + }); + }); + + useEffect(() => { + const onKey = (e: KeyboardEvent) => onPlayerKey(e); window.addEventListener("keydown", onKey); focusModal(); // (The scroll lock is the shared, counted one in useDialogChrome.) @@ -492,11 +519,7 @@ export default function PlayerModal({ window.removeEventListener("keydown", onKey); // (The debounced volume write is flushed by useYtVolume's own unmount cleanup.) }; - // Bound once per player on purpose: every helper the handler calls reads its live state through - // a ref (playerRef, navRef, maximizedRef, isFullscreenRef, the volume hook's own ref), so - // re-binding on each render would only churn the listener, never change what a key does. - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [onClose]); + }, [focusModal]); // Create the player, resume from the saved position, checkpoint progress, and auto-mark watched at // the end — recreated per active item. The once-bound event closures read fresh queue/mode/volume via diff --git a/frontend/src/components/PlexPlayer.tsx b/frontend/src/components/PlexPlayer.tsx index 7063d7cd..bc198228 100644 --- a/frontend/src/components/PlexPlayer.tsx +++ b/frontend/src/components/PlexPlayer.tsx @@ -208,6 +208,13 @@ export default function PlexPlayer({ itemId, onClose, queue }: Props) { // Refs for values read inside stable callbacks / the keyboard handler (avoid stale closures). const barAutoHideRef = useRef(prefs.barAutoHide); barAutoHideRef.current = prefs.barAutoHide; + // The two language prefs are read only at ITEM LOAD (the detail effect below), so they go through + // a ref for the same reason `tRef` above does: naming them in that effect's deps would re-run it + // when the user picks a track from the player's own menu, double-loading the session the picker + // has already reloaded. The ref says "latest value, but not a trigger" in code, which an excused + // dep list only said in prose. Same shape as barAutoHideRef/seekStepRef either side of this. + const langPrefsRef = useRef({ audioLang: prefs.audioLang, subLang: prefs.subLang }); + langPrefsRef.current = { audioLang: prefs.audioLang, subLang: prefs.subLang }; const seekStepRef = useRef(prefs.seekStep); seekStepRef.current = prefs.seekStep; const fineSeekStepRef = useRef(prefs.fineSeekStep); @@ -474,7 +481,11 @@ export default function PlexPlayer({ itemId, onClose, queue }: Props) { restartsRef.current.at = now; if (restartsRef.current.n < MAX_SESSION_RECLAIMS) { restartsRef.current.n += 1; - loadSession(playhead(absRef, loadStartRef), !video.paused); + // Through the ref, not by name: this is loadSession calling ITSELF from inside its own + // useCallback, which the checker rightly reads as "accessed before it is declared". + // The ref (assigned right below the callback) is the file's existing answer to that — + // detachSessionRef and cancelAutoSkipRef do the same for the same reason. + void loadSessionRef.current(playhead(absRef, loadStartRef), !video.paused); return; } } @@ -497,6 +508,10 @@ export default function PlexPlayer({ itemId, onClose, queue }: Props) { }, [id], // NOT `t` — see tRef above (a language switch must not rebuild this / reload the session). ); + // The self-restart path above reaches the CURRENT loadSession through this, so the recursion is + // not a forward reference to a binding that is still being defined. + const loadSessionRef = useRef(loadSession); + loadSessionRef.current = loadSession; // Audio/subtitle track selection + subtitle rendering. Owns the selected-ordinal state; the shared // media refs stay component-owned (loadSession above + the detail effect below write audioRef too). @@ -552,16 +567,14 @@ export default function PlexPlayer({ itemId, onClose, queue }: Props) { restartsRef.current = { n: 0, at: 0 }; // Restore the per-account audio/subtitle language choice, resolved to THIS item's tracks. Set the // audio ref synchronously so the loadSession below picks the right track on the first request. - const aOrd = audioOrdForLang(detail.audio_streams ?? [], prefs.audioLang); + const { audioLang, subLang } = langPrefsRef.current; + const aOrd = audioOrdForLang(detail.audio_streams ?? [], audioLang); audioRef.current = aOrd; setAudioOrd(aOrd); - setSubOrd(subOrdForLang(detail.subtitle_streams ?? [], prefs.subLang)); + setSubOrd(subOrdForLang(detail.subtitle_streams ?? [], subLang)); const resume = detail.status !== "watched" ? detail.position_seconds || 0 : 0; - loadSession(resume); - // Prefs are applied at item load only; changing one via the picker must NOT re-run this (it would - // double-load the session — the picker handlers already reload/toggle). - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [detail, id, loadSession]); + void loadSession(resume); + }, [detail, id, loadSession, audioRef, setAudioOrd, setSubOrd]); // Leaving the item: a session still in flight for it must not land after it, and its stream goes. useEffect( () => () => { @@ -593,7 +606,11 @@ export default function PlexPlayer({ itemId, onClose, queue }: Props) { const v = videoRef.current; return () => { aliveRef.current = false; - // eslint-disable-next-line react-hooks/exhaustive-deps -- the live counter, as above + // The rule's advice — copy the ref into a variable and use the copy — is precisely wrong here. + // sessGenRef is a LIVE counter: bumping it is how the unmount tells every in-flight session + // that its response no longer belongs to anything, and a value captured when the effect ran + // would invalidate a generation that has since been superseded instead of the current one. + // eslint-disable-next-line react-hooks/exhaustive-deps sessGenRef.current++; if (hlsRef.current) { hlsRef.current.destroy(); @@ -839,7 +856,14 @@ export default function PlexPlayer({ itemId, onClose, queue }: Props) { setUpNextOpen(false); if (nextId) go(nextId); }, [nextId, go]); - useEffect(() => setUpNextOpen(false), [id]); + // The "up next" card belongs to the item that was playing, so a queue advance closes it. Adjusted + // during the render that changes `id` rather than in an effect: as an effect it closed one commit + // after the new episode had already drawn behind it. + const [upNextForId, setUpNextForId] = useState(id); + if (upNextForId !== id) { + setUpNextForId(id); + setUpNextOpen(false); + } // "Back" cascade shared by the on-screen Back arrow AND the keyboard/remote Backspace·Escape: peel // off whatever is layered on top (a tuning/tracks menu, then the info overlay) before leaving the