peter/siftlode
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

refactor(frontend): the two players — one real ordering bug avoided, five suppressions gone (E27 S2)

Browse Source 
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.
This commit is contained in:
peter
2026-09-21 00:27:15 +02:00
parent ce08fc2aec
commit 4065ac7e6a
3 changed files with 120 additions and 81 deletions
Download Patch File Download Diff File Expand all files Collapse all files
frontend/src/components/PlayerModal.tsx
+86 -63
View File
@@ -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<HTMLDivElement | null>(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