From 6a7ff330f6f16f08374949a100991be0c449b2b0 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Mon, 21 Sep 2026 00:07:12 +0200 Subject: [PATCH] refactor(frontend): the shared hooks in lib/ stop lying about their deps (E27 S2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ten more suppressions, and React 19 turns out to supply the missing primitive. useEffectEvent (stable since React 19.2, which S1 brought in) is the canonical answer to the "bind the listener once, but read the latest values" pattern these hooks were disabling the checker for. Three of playerModalHooks' four escape hatches become honest dep lists: the wheel-volume handler, the native-controls handover, and the fullscreen-change sync. The contract is no longer prose — the compiler enforces it. The fourth stays, deliberately, and now says why at the site: the YouTube player's lifecycle effect must run once per video (re-running destroys and rebuilds the iframe), and YT.Player takes its callbacks as an object literal, which an effect event may not be passed into. Fixed, not suppressed: - useDismiss captured onClose when the popover opened, so a caller that re-rendered with a different handler while it stayed open kept getting the OLD one called. The disabled dep list asserted the opposite ("read fresh from this run's closure"). Both the handler and the ref list are read through refs now; three tests, the two that matter mutation-checked. - usePref wrapped its seed in a useCallback whose dep list had to be excused. useState calls a lazy initializer exactly once, so the memo was saving nothing; inlined. - useCardPager clamped an out-of-range page from an effect. Adjusted during render instead — React discards that pass rather than committing it, so no out-of-range page is ever painted. - plexPlayerHooks' auto-skip countdown reads its marker from the ref that already existed, so the dep list names its real inputs. That also made the set-state-in-effect finding go away: the progress is genuinely timer-driven, and the checker can now see that. - useUrlMirror and useChipOverflowFit only needed their stable refs listed. Two test-harness suppressions (a render-phase probe ref, a module-level setState handle) moved inline with the reason they exist: both exist to observe a hook from outside without changing what is being measured. Suppressions 99 -> 89; exhaustive-deps 35 -> 27. --- frontend/eslint-suppressions.json | 20 ----- frontend/src/lib/playerModalHooks.ts | 72 +++++++++------- .../lib/plexPlayerHooks.subtitle.dom.test.tsx | 1 + frontend/src/lib/plexPlayerHooks.ts | 14 ++- frontend/src/lib/useCardPager.ts | 12 +-- frontend/src/lib/useChipOverflowFit.ts | 5 +- frontend/src/lib/useDismiss.dom.test.tsx | 85 +++++++++++++++++++ frontend/src/lib/useDismiss.ts | 22 +++-- .../lib/useFocusKeeper.restore.dom.test.tsx | 1 + frontend/src/lib/usePref.ts | 12 +-- frontend/src/lib/useUrlMirror.ts | 10 ++- 11 files changed, 175 insertions(+), 79 deletions(-) create mode 100644 frontend/src/lib/useDismiss.dom.test.tsx diff --git a/frontend/eslint-suppressions.json b/frontend/eslint-suppressions.json index c714654c..594be87a 100644 --- a/frontend/eslint-suppressions.json +++ b/frontend/eslint-suppressions.json @@ -183,25 +183,5 @@ "react-hooks/set-state-in-effect": { "count": 1 } - }, - "src/lib/plexPlayerHooks.subtitle.dom.test.tsx": { - "react-hooks/immutability": { - "count": 1 - } - }, - "src/lib/plexPlayerHooks.ts": { - "react-hooks/set-state-in-effect": { - "count": 1 - } - }, - "src/lib/useCardPager.ts": { - "react-hooks/set-state-in-effect": { - "count": 1 - } - }, - "src/lib/useFocusKeeper.restore.dom.test.tsx": { - "react-hooks/globals": { - "count": 1 - } } } diff --git a/frontend/src/lib/playerModalHooks.ts b/frontend/src/lib/playerModalHooks.ts index b81e656c..ec2f52e9 100644 --- a/frontend/src/lib/playerModalHooks.ts +++ b/frontend/src/lib/playerModalHooks.ts @@ -1,5 +1,6 @@ import { useEffect, + useEffectEvent, useRef, useState, type Dispatch, @@ -133,20 +134,20 @@ export function useYtVolume({ // Mouse-wheel volume over the WHOLE modal. Non-passive so preventDefault stops the modal/page // scrolling. The cross-origin iframe swallows wheel over its native strips, but the central // interaction overlay is our element (wheel there bubbles here), and everything outside works. + const onWheelSpin = useEffectEvent((e: WheelEvent) => { + // Ctrl/⌘+wheel is the browser's zoom gesture — never swallow it. + if (e.ctrlKey || e.metaKey) return; + e.preventDefault(); + focusModal(); // re-grab focus if it had slipped into the native controls + nudgeVolume(e.deltaY < 0 ? 5 : -5); + }); useEffect(() => { const el = dialogRef.current; if (!el) return; - const onWheel = (e: WheelEvent) => { - // Ctrl/⌘+wheel is the browser's zoom gesture — never swallow it. - if (e.ctrlKey || e.metaKey) return; - e.preventDefault(); - focusModal(); // re-grab focus if it had slipped into the native controls - nudgeVolume(e.deltaY < 0 ? 5 : -5); - }; + const onWheel = (e: WheelEvent) => onWheelSpin(e); el.addEventListener("wheel", onWheel, { passive: false }); return () => el.removeEventListener("wheel", onWheel); - // eslint-disable-next-line react-hooks/exhaustive-deps - }, []); + }, [dialogRef]); // On unmount, clear the flash timer and flush any pending debounced write — closing the player right // after a wheel spin (the normal way to leave) must not drop the level the user just set. @@ -183,18 +184,19 @@ export function useNativeControlsYield({ setNativeControls: (on: boolean) => void; } { const [nativeControls, setNativeControls] = useState(false); + const handOverToIframe = useEffectEvent(() => { + const p = playerRef.current; + const f = p && typeof p.getIframe === "function" ? p.getIframe() : null; + if (f && document.activeElement === f) setNativeControls(true); + }); + const rearmOurControls = useEffectEvent(() => { + setNativeControls(false); + focusModal(); + }); useEffect(() => { - const iframeFocused = () => { - const p = playerRef.current; - const f = p && typeof p.getIframe === "function" ? p.getIframe() : null; - return !!f && document.activeElement === f; - }; // activeElement settles on the tick after blur, so defer the check. - const onBlur = () => window.setTimeout(() => iframeFocused() && setNativeControls(true), 0); - const rearm = () => { - setNativeControls(false); - focusModal(); - }; + const onBlur = () => window.setTimeout(() => handOverToIframe(), 0); + const rearm = () => rearmOurControls(); const stage = stageRef.current; window.addEventListener("blur", onBlur); window.addEventListener("focus", rearm); @@ -204,8 +206,7 @@ export function useNativeControlsYield({ window.removeEventListener("focus", rearm); stage?.removeEventListener("mouseleave", rearm); }; - // eslint-disable-next-line react-hooks/exhaustive-deps - }, []); + }, [stageRef]); return { nativeControls, setNativeControls }; } @@ -264,19 +265,17 @@ export function useYtBigView({ // YouTube's own fullscreen — entered from its control bar, left by its button or Esc. On LEAVING, // focus is stuck in the iframe (the user had to click into the player to reach YT's button), so our // window-level shortcuts never fire — take focus back so F/Space/arrows work the moment fullscreen ends. + const syncFullscreen = useEffectEvent(() => { + const fsEl = fullscreenElement(); + const p = playerRef.current; + const frame = p && typeof p.getIframe === "function" ? p.getIframe() : null; + const ours = !!fsEl && (fsEl === stageRef.current || fsEl === frame); + isFullscreenRef.current = ours; // read by the once-bound keydown handler + setIsFullscreen(ours); // drives the overlay's pointer-events + if (!fsEl) focusModal(); + }); useEffect(() => { - const onChange = () => { - const fsEl = fullscreenElement(); - // Refs only — this listener is bound once, so it must not close over render values. - const p = playerRef.current; - const frame = p && typeof p.getIframe === "function" ? p.getIframe() : null; - const ours = !!fsEl && (fsEl === stageRef.current || fsEl === frame); - isFullscreenRef.current = ours; // read by the once-bound keydown handler - setIsFullscreen(ours); // drives the overlay's pointer-events - if (!fsEl) focusModal(); - }; - return onFullscreenChange(onChange); // both spellings — see lib/fullscreen - // eslint-disable-next-line react-hooks/exhaustive-deps + return onFullscreenChange(() => syncFullscreen()); // both spellings — see lib/fullscreen }, []); return { @@ -485,6 +484,13 @@ export function useYtProgress({ } playerRef.current = null; }; + // ONE PLAYER PER VIDEO is the invariant, and it is why this dep list is deliberately just the + // id: re-running would destroy and rebuild the YouTube iframe, losing the playing position and + // every listener attached to it. Nothing in the body reads a render value — the mutable state it + // needs mid-session (currentIdRef, modeRef, navRef, sessionPosRef, autoMarkedRef, + // errorStreakRef, playerRef) is all behind refs, which is the enforcement, not this comment. + // useEffectEvent cannot help here the way it does above: YT.Player takes its callbacks as an + // object literal, and an effect event may not be passed around as a value. // eslint-disable-next-line react-hooks/exhaustive-deps }, [activeId]); } diff --git a/frontend/src/lib/plexPlayerHooks.subtitle.dom.test.tsx b/frontend/src/lib/plexPlayerHooks.subtitle.dom.test.tsx index a56c0cdf..eb8b6017 100644 --- a/frontend/src/lib/plexPlayerHooks.subtitle.dom.test.tsx +++ b/frontend/src/lib/plexPlayerHooks.subtitle.dom.test.tsx @@ -330,6 +330,7 @@ function TrackHarness() { absRef: { current: 3600 }, loadStartRef: { current: 0 }, }); + // eslint-disable-next-line react-hooks/immutability -- a TEST probe: the hook's return has to reach the assertions, and a render-phase ref write is the only way to read it from outside without a wrapper that would change what is being measured. harness.current = tracks; return (