refactor(frontend): a stable useHistorySubview, and PlexBrowse can stop disabling the check (E27 S2)

useHistorySubview returned a fresh object with a fresh `open` on every render. Correct, but it
made "re-run when the sub-view moves" unwriteable: PlexBrowse had three effects that wanted
exactly that and had to disable the dependency check to get it, and a fourth that listed `sub`
honestly and therefore re-ran on EVERY parent render, saved only by a ref guard.

Fixed at the source, which is where the house rule says a principle belongs: `open` and `back`
are useCallbacks and the returned object is memoised. `url` is a module constant at every call
site (PLEX_SUB_URL, MODE_URL) and the body otherwise touches only module functions, so there is
nothing left to churn on. All three PlexBrowse dep lists are now the honest ones, and the
comment that claimed `open` was "behaviourally stable" is replaced by it actually being stable.

PlexBrowse's remaining four are escape hatches, and two of them are now ONE:
- `infoScrollRef` is written from four places, and the compiler objects to a ref being modified
  outside the effect that reads it. That objection is a conservative false positive — a scroll
  memory needs exactly that arrangement — but four bare mutations were worth concentrating
  anyway. One `rememberInfoScroll` writer now holds the "stash on the way out, reset on a fresh
  page" rule, and answers the checker once instead of three times.
- The language-commit effect cannot move into render: it resets PlexProvider's filters, and a
  render-phase setState may only touch the component's own state. Said so at the site.

Suppressions 78 -> 75; exhaustive-deps 23 -> 20.
This commit is contained in:
2026-09-21 00:20:05 +02:00
parent ea6d829c29
commit ce08fc2aec
3 changed files with 40 additions and 25 deletions
-8
View File
@@ -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
+25 -11
View File
@@ -112,14 +112,14 @@ export default function PlexBrowse() {
const sub = useHistorySubview<Sub>({ 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<PlexCard>();
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" });
}
+15 -6
View File
@@ -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<T>(
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<T>(
);
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) -------------------------------