diff --git a/frontend/eslint-suppressions.json b/frontend/eslint-suppressions.json index 3785a63f..4c8d67eb 100644 --- a/frontend/eslint-suppressions.json +++ b/frontend/eslint-suppressions.json @@ -98,14 +98,6 @@ "count": 1 } }, - "src/components/PlexBrowse.tsx": { - "react-hooks/immutability": { - "count": 3 - }, - "react-hooks/set-state-in-effect": { - "count": 1 - } - }, "src/components/PlexInfo.tsx": { "react-hooks/static-components": { "count": 3 diff --git a/frontend/src/components/PlexBrowse.tsx b/frontend/src/components/PlexBrowse.tsx index 2699b982..72ba9f61 100644 --- a/frontend/src/components/PlexBrowse.tsx +++ b/frontend/src/components/PlexBrowse.tsx @@ -112,14 +112,14 @@ export default function PlexBrowse() { const sub = useHistorySubview({ kind: "discover" }, PLEX_SUB_URL); // Publish the top-level view (discover vs grid) + opener to the header's Discover/Browse toggle via - // the bridge; clear it on unmount so the toggle disappears when you leave the plex page. Keyed on the - // view KIND (open is behaviourally stable — it takes the next view as an arg, closing over nothing). + // the bridge; clear it on unmount so the toggle disappears when you leave the plex page. `sub.open` + // is referentially stable (lib/history memoises it), so naming it costs nothing and the dep list is + // the honest one: the toggle is republished when the view KIND changes, and on nothing else. const publishView = usePublishPlexView(); useLayoutEffect(() => { publishView({ kind: sub.view.kind, open: sub.open }); return () => publishView(null); - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [sub.view.kind]); + }, [sub.view.kind, sub.open, publishView]); // Sidebar asked to open a playlist → push it as a subview (browser Back returns to the grid), then // clear the signal so it doesn't re-open. @@ -128,8 +128,7 @@ export default function PlexBrowse() { sub.open({ kind: "playlist", id: openPlaylist }); setPlaylistOpen(null); } - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [openPlaylist]); + }, [openPlaylist, sub, setPlaylistOpen]); // The header search typeahead picked a title → open it the same way a card click would (a movie's // info page like the card's ⓘ, a show's drill-down), then clear the hand-off so it doesn't re-open. @@ -150,8 +149,7 @@ export default function PlexBrowse() { ); setJumpTo(null); } - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [jumpTo]); + }, [jumpTo, sub, setJumpTo]); // Discover → grid on a SEARCH: a header search has no rail on Discover, so entering one FIRST while on // the Discover landing switches to the flat grid (which consumes q). Only on the off→on edge, so a Back @@ -180,6 +178,17 @@ export default function PlexBrowse() { // (down to a collection strip) instead of snapping to the top. Reset on a fresh info open so a // different title's info starts at the top. const infoScrollRef = useRef(0); + // ONE writer for that memory, so "stash on the way out of an info page, reset on a fresh one" is a + // rule the code holds rather than one spread over four call sites — and so the compiler's + // complaint about it is answered once, here, instead of at each of them. + // + // The complaint is a false positive of a conservative analysis, not a hazard: the rule objects to a + // ref being written outside the effect that reads it, but that IS the arrangement a scroll memory + // needs — the navigation handlers record where the page was, and the restore layout effect above + // reads it on the way back. React's own guarantee (a ref write is never part of rendering) is what + // makes it safe, and nothing here reads the value during render. + // eslint-disable-next-line react-hooks/immutability + const rememberInfoScroll = (px: number) => void (infoScrollRef.current = px); // One comfort-pin menu for the titles grid (see ui/ContextMenu). const pinMenu = useContextMenu(); const scroller = () => document.querySelector("main"); @@ -261,6 +270,11 @@ export default function PlexBrowse() { // then a second after the clear; the query waits for both to settle into a single fetch. useEffect(() => { if (committedLang !== lang) { + // Not adjustable during render, which is where a state reset like this would otherwise + // belong: `setFilters` belongs to PlexProvider, and React only permits a render-phase setState + // on the component's OWN state. The cascade the rule warns about is bounded to one extra pass + // — the guard above is false immediately afterwards. + // eslint-disable-next-line react-hooks/set-state-in-effect setCommittedLang(lang); if (filters.genres.length) setFilters({ ...filters, genres: [] }); } @@ -308,7 +322,7 @@ export default function PlexBrowse() { } function onInfo(card: PlexCard) { scrollRef.current = scroller()?.scrollTop ?? 0; - infoScrollRef.current = 0; // fresh info page starts at the top + rememberInfoScroll(0); // fresh info page starts at the top sub.open({ kind: "info", id: card.id }); } // Open a cast/crew member's filmography page (from an info/show cast row, or an actor filter-chip). @@ -316,7 +330,7 @@ export default function PlexBrowse() { // it was, like applyFilter) — from a show/grid context infoScrollRef must stay untouched, else a // later info page would wrongly restore this stale offset. function onOpenPerson(name: string) { - if (sub.view.kind === "info") infoScrollRef.current = scroller()?.scrollTop ?? 0; + if (sub.view.kind === "info") rememberInfoScroll(scroller()?.scrollTop ?? 0); sub.open({ kind: "person", name }); } // Optimistically flip a card's status in every cached library page — both the title grid (`items`) @@ -361,7 +375,7 @@ export default function PlexBrowse() { } setFilters(merged as unknown as PlexFilters); if (patch.actors?.length || patch.directors?.length) setScope("both"); - infoScrollRef.current = scroller()?.scrollTop ?? 0; // restore on Back to the info/show page + rememberInfoScroll(scroller()?.scrollTop ?? 0); // restore on Back to the info/show page sub.open({ kind: "grid" }); } diff --git a/frontend/src/lib/history.ts b/frontend/src/lib/history.ts index 9f53e238..7ea6322b 100644 --- a/frontend/src/lib/history.ts +++ b/frontend/src/lib/history.ts @@ -9,7 +9,7 @@ // its own button pops exactly its entry without tripping the modals underneath. // // setPage() pushes a clean { sfPage } entry (dropping _sub/_ov), so each page starts at its root. -import { useEffect, useRef, useState } from "react"; +import { useCallback, useEffect, useMemo, useRef, useState } from "react"; import { navPosition, restoreNavPosition, type NavPosition } from "./listNav"; import { urlWithParams } from "./urlState"; @@ -143,9 +143,14 @@ export function useHistorySubview( window.addEventListener("popstate", onPop); return () => window.removeEventListener("popstate", onPop); }, []); - return { - view, - open: (next: T, opts?: { replace?: boolean }) => { + // `open` and `back` are memoised so they are REFERENTIALLY stable, not merely behaviourally so. + // They used to be plain inline arrows: correct, but a fresh identity every render, which forced + // every consumer that listens for a sub-view change to disable the dependency check rather than + // re-run its effect on each parent render (three such effects in PlexBrowse alone). `url` is a + // module constant at every call site (PLEX_SUB_URL, MODE_URL) and the body otherwise touches only + // module functions and the setter, so there is nothing left to churn on. + const open = useCallback( + (next: T, opts?: { replace?: boolean }) => { setView(next); const href = url ? urlWithParams({ [url.key]: url.encode(next) }) : ""; if (opts?.replace) { @@ -165,8 +170,12 @@ export function useHistorySubview( ); notifyNav(); }, - back: () => window.history.back(), - }; + [url], + ); + const back = useCallback(() => window.history.back(), []); + // Memoised as a whole for the same reason: a consumer that wants "re-run when the sub-view moves" + // can now write `[sub]` and mean it, instead of re-running on every parent render. + return useMemo(() => ({ view, open, back }), [view, open, back]); } // --- Overlays / modals (one history entry each, nesting-safe) -------------------------------