From 4065ac7e6ab614393f95a5662d42632ca5765850 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Mon, 21 Sep 2026 00:27:15 +0200 Subject: [PATCH] =?UTF-8?q?refactor(frontend):=20the=20two=20players=20?= =?UTF-8?q?=E2=80=94=20one=20real=20ordering=20bug=20avoided,=20five=20sup?= =?UTF-8?q?pressions=20gone=20(E27=20S2)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The interesting part of this batch is a change that was WRONG and the test suite said so. PlayerModal synced its local watch-status mirror and `currentVideoId` from an effect on `active.id`. Moving that into render is right — as an effect there was a frame per queue advance showing the new episode's chrome over the previous item's watch state, with `navigated` still true so the "you have navigated away" affordance flashed. But the same effect also advanced `currentIdRef`, and that line must NOT move: useYtProgress's per-item cleanup persists the OUTGOING item's position and guards it on `currentIdRef.current === id`. React runs every cleanup before any setup, so in an effect the ref still names the item being left. Advanced to render time the flip happens first, the checkpoint is skipped, and stepping next→prev back to an item resumes from the frozen launch queue's stale position. "resumes an item played earlier in the SAME session" caught it immediately (748 instead of 830); the ref write stays in its own effect with the ordering written down. The rest: - PlayerModal's keyboard handler is a useEffectEvent, so the "bound once, reads live state" contract is enforced instead of asserted. `focusModal` is a useCallback now — it reads a ref and nothing else, and three hooks take it as a parameter where a fresh identity would spread. - PlexPlayer's session reclaim called `loadSession` from inside loadSession's own useCallback, which the checker reads as a forward reference. Routed through a ref, the file's existing answer for exactly this (detachSessionRef, cancelAutoSkipRef). - PlexPlayer's item-load effect excluded `prefs` by hand so a track pick from the player's own menu would not double-load the session. The two language prefs go through a ref instead — "latest value, not a trigger", said in code rather than in a comment. - PlexPlayer closes its up-next card during the render that changes `id`, not a commit later. - The one PlexPlayer directive that stays is the live session counter, whose cleanup must bump the CURRENT value: the rule's advice to copy it into a variable is precisely wrong there, and the comment now says why rather than pointing at another comment. Suppressions 75 -> 70; exhaustive-deps 20 -> 17. --- frontend/eslint-suppressions.json | 8 -- frontend/src/components/PlayerModal.tsx | 149 ++++++++++++++---------- frontend/src/components/PlexPlayer.tsx | 44 +++++-- 3 files changed, 120 insertions(+), 81 deletions(-) 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