From ea6d829c29b8fd44e143319c093fb912f84cca63 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Mon, 21 Sep 2026 00:12:44 +0200 Subject: [PATCH] refactor(frontend): the context providers adjust state in render, not after it (E27 S2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Eleven suppressions across the five providers. The recurring shape here was "an effect that undoes something when you navigate away", and every one of them ran a commit LATE — the new page painted once while the old module's state was still live. Adjusted during render instead (React discards that pass rather than committing it, so there is no in-between frame): - SavedViewsProvider reset the sticky view and closed its modal from an effect on `source`, so the first Plex render carried the feed's sticky id — long enough for a stale view chip. It also followed the active view into stickyId from a second effect; that one converges by construction, since preferSticky returns the sticky match when there is one. - NavigationProvider ended a live YouTube search from an effect on `page`. Keyed on the page CHANGE rather than derived from it, because "ended" has to be permanent — deriving would have quietly brought the search back on return to the feed. - ChannelsProvider's manager focus is now simply DERIVED from the page: it means nothing off the Channels page, and focusChannel sets it and navigates in one handler. Honest deps by freezing what is genuinely mount-time: - FeedFiltersProvider persisted a share link's filters with an empty dep list; listing `filters` honestly would have re-persisted on every edit and spun. The mount value is frozen into its own state, so all three deps are stable and the effect says what it does. - ChannelsProvider's URL-seed layout effect and NavigationProvider's initial history stamp get the same treatment. The three slice setters they call were already stable useCallbacks (useFilterState / useSortState / useAccountPersistedState all return one). - SavedViewsProvider reads `cfg` through a ref: it re-memoizes on every filter edit, and the per-source seed guard already enforces once. That also made the set-state finding go away — with the render value gone, the checker can see the seed for what it is. WizardProvider keeps its one escape hatch, now inline with the invariant: auto-opening is a one-way decision taken from server data as it arrives, and deriving it would reopen a wizard the user had just closed. Suppressions 89 -> 78; exhaustive-deps 27 -> 23. --- frontend/eslint-suppressions.json | 20 ------------ frontend/src/components/ChannelsProvider.tsx | 21 ++++++++----- .../src/components/FeedFiltersProvider.tsx | 11 +++++-- .../src/components/NavigationProvider.tsx | 25 ++++++++++----- .../src/components/SavedViewsProvider.tsx | 31 ++++++++++++------- frontend/src/components/WizardProvider.tsx | 1 + 6 files changed, 59 insertions(+), 50 deletions(-) diff --git a/frontend/eslint-suppressions.json b/frontend/eslint-suppressions.json index 594be87a..3785a63f 100644 --- a/frontend/eslint-suppressions.json +++ b/frontend/eslint-suppressions.json @@ -14,11 +14,6 @@ "count": 2 } }, - "src/components/ChannelsProvider.tsx": { - "react-hooks/set-state-in-effect": { - "count": 1 - } - }, "src/components/ConfigPanel.tsx": { "react-hooks/set-state-in-effect": { "count": 1 @@ -85,11 +80,6 @@ "count": 1 } }, - "src/components/NavigationProvider.tsx": { - "react-hooks/set-state-in-effect": { - "count": 1 - } - }, "src/components/PersonalViewsModal.tsx": { "react-hooks/set-state-in-effect": { "count": 1 @@ -134,11 +124,6 @@ "count": 1 } }, - "src/components/SavedViewsProvider.tsx": { - "react-hooks/set-state-in-effect": { - "count": 3 - } - }, "src/components/Scheduler.tsx": { "react-hooks/set-state-in-effect": { "count": 2 @@ -164,11 +149,6 @@ "count": 1 } }, - "src/components/WizardProvider.tsx": { - "react-hooks/set-state-in-effect": { - "count": 1 - } - }, "src/components/comfortPin.tsx": { "react-hooks/set-state-in-effect": { "count": 1 diff --git a/frontend/src/components/ChannelsProvider.tsx b/frontend/src/components/ChannelsProvider.tsx index 6a2aa672..a412093b 100644 --- a/frontend/src/components/ChannelsProvider.tsx +++ b/frontend/src/components/ChannelsProvider.tsx @@ -110,7 +110,7 @@ export function ChannelsProvider({ children }: { children: ReactNode }) { // it against the live columns for display, so the provider stores it un-healed (no options passed). const [sort, setSort] = useSortState(LS.channelSort); const [filtersResetToken, setFiltersResetToken] = useState(0); - const [focusName, setFocusName] = useState(null); + const [focusNameRaw, setFocusName] = useState(null); const [focusToken, setFocusToken] = useState(0); // S8 — shareable manager view. Captured once at mount (or null when no manager URL keys are present). @@ -120,14 +120,18 @@ export function ChannelsProvider({ children }: { children: ReactNode }) { // Apply the shared-link seed once, BEFORE paint, through the slice setters — which persist it, so the // shared view also survives a later PLAIN reload (no URL). setViewRaw (not setView) restores the tab // without pushing a phantom history entry. A normal load has urlSeed === null and this is a no-op. + // `view` is frozen at mount for the same reason `urlSeed` is: the comparison is against the tab + // this load started on, and the three setters are stable by construction (useFilterState, + // useSortState and useAccountPersistedState all return a useCallback). So the dep list can name + // every value the body reads without the seed re-arming on a later tab switch. + const [mountView] = useState(view); useLayoutEffect(() => { if (!urlSeed || seededRef.current) return; seededRef.current = true; - if (urlSeed.view !== view) setViewRaw(urlSeed.view); + if (urlSeed.view !== mountView) setViewRaw(urlSeed.view); setFilters(urlSeed.filters); setSort(urlSeed.sort); - // eslint-disable-next-line react-hooks/exhaustive-deps - }, []); + }, [urlSeed, mountView, setViewRaw, setFilters, setSort]); // Latest-value ref so the STABLE setView can compare against the current tab without depending on // it (a dep would recreate the action on every switch and re-render every actions consumer). @@ -175,10 +179,11 @@ export function ChannelsProvider({ children }: { children: ReactNode }) { setPage("channels"); }, [setFilters, setView, setPage]); - // Clear the manager focus when leaving the Channels page so it doesn't re-apply on a later visit. - useEffect(() => { - if (page !== "channels") setFocusName(null); - }, [page]); + // The manager focus only means anything ON the Channels page, so it is DERIVED from the page + // rather than cleared by an effect when you leave. focusChannel sets the name and navigates in the + // same handler, so the value is live by the time channels renders; everywhere else it reads null, + // one render earlier than the effect used to make it. + const focusName = page === "channels" ? focusNameRaw : null; // Label the entry the manager was REACHED on (setPage pushes a bare { sfPage }), so stepping back // onto it later restores the tab it belonged to rather than leaving whatever tab is current. diff --git a/frontend/src/components/FeedFiltersProvider.tsx b/frontend/src/components/FeedFiltersProvider.tsx index e26fc3c7..f55c4d3a 100644 --- a/frontend/src/components/FeedFiltersProvider.tsx +++ b/frontend/src/components/FeedFiltersProvider.tsx @@ -67,10 +67,15 @@ export function FeedFiltersProvider({ children }: { children: ReactNode }) { // A share link's filters (captured at mount) win over the account's stored view and are persisted // to it once, so the view the link opened also survives a later plain reload. + // + // The mount-time filters are FROZEN into their own state rather than left to an empty dep list: + // `filters` changes on every edit, so listing it honestly would re-persist — and setFilters + // setting the state it depends on would spin. Frozen, all three deps are stable and the effect + // means exactly what it says: write what the URL brought, once. + const [urlSeededFilters] = useState(filters); useEffect(() => { - if (initialHadUrlFilters) setFilters(filters); - // eslint-disable-next-line react-hooks/exhaustive-deps - }, []); + if (initialHadUrlFilters) setFilters(urlSeededFilters); + }, [initialHadUrlFilters, urlSeededFilters, setFilters]); const actions = useMemo(() => ({ setFilters }), [setFilters]); diff --git a/frontend/src/components/NavigationProvider.tsx b/frontend/src/components/NavigationProvider.tsx index 7a37a724..9aea5a63 100644 --- a/frontend/src/components/NavigationProvider.tsx +++ b/frontend/src/components/NavigationProvider.tsx @@ -153,10 +153,16 @@ export function NavigationProvider({ children }: { children: ReactNode }) { // Expose setPage to decoupled callers (download toast "View", etc.) via the lib/nav singleton. useEffect(() => setNavigator(setPage), [setPage]); - // Leaving the feed ends any live YouTube search, so coming back shows the normal feed. - useEffect(() => { + // Leaving the feed ends any live YouTube search, so coming back shows the normal feed. Adjusted + // DURING the render that changes the page, against the page we were last on: as an effect it ran + // after the new page had already committed, so for one frame a module rendered with a live search + // still in the nav state. Keyed on the CHANGE rather than derived from `page`, because "ended" has + // to be permanent — a derived null would quietly come back when you returned to the feed. + const [pageSearchWasOn, setPageSearchWasOn] = useState(page); + if (pageSearchWasOn !== page) { + setPageSearchWasOn(page); if (page !== "feed") setYtSearch(null); - }, [page]); + } // Reflect the current module in the browser tab title. Feed = brand only; an open channel page // shows the channel name. Signed out, the page on screen is the public landing whatever `page` @@ -174,6 +180,10 @@ export function NavigationProvider({ children }: { children: ReactNode }) { document.title = label ? `${label} · ${brand}` : brand; }, [signedIn, page, channelView, i18n.language, t]); + // The page this tab LOADED on. Frozen, because the stamp below belongs to the initial history + // entry: every later page change writes its own entry through setPage. + const [mountPage] = useState(page); + // In-app Back/Forward: stamp the initial entry with the current page (mirrored to the address bar), // then sync state from history.state on popstate — the browser restores each entry's URL for us, so // the popstate handler only needs to read history.state. @@ -185,12 +195,12 @@ export function NavigationProvider({ children }: { children: ReactNode }) { // Mirror the resolved initial page to the address bar (feed implicit) so a page reached from the // stored landing pref — not just from the URL — is itself bookmarkable. urlWithParams preserves any // ?chan / ?v / ?pl / filter params the loaders already read. - const url = urlWithParams({ page: page === "feed" ? null : page }); + const url = urlWithParams({ page: mountPage === "feed" ? null : mountPage }); if (st._yt && st._ytScrape) { - window.history.replaceState({ ...st, sfPage: page }, "", url); + window.history.replaceState({ ...st, sfPage: mountPage }, "", url); } else { const { _yt: _staleYt, _ytScrape: _staleScrape, ...rest } = st; - window.history.replaceState({ ...rest, sfPage: page }, "", url); + window.history.replaceState({ ...rest, sfPage: mountPage }, "", url); } function onPop(e: PopStateEvent) { const p = (e.state?.sfPage as Page) ?? "feed"; @@ -207,8 +217,7 @@ export function NavigationProvider({ children }: { children: ReactNode }) { } window.addEventListener("popstate", onPop); return () => window.removeEventListener("popstate", onPop); - // eslint-disable-next-line react-hooks/exhaustive-deps - }, []); + }, [mountPage]); const state = useMemo( () => ({ page, channelView, ytSearch }), diff --git a/frontend/src/components/SavedViewsProvider.tsx b/frontend/src/components/SavedViewsProvider.tsx index 2155b5a9..b7a21c80 100644 --- a/frontend/src/components/SavedViewsProvider.tsx +++ b/frontend/src/components/SavedViewsProvider.tsx @@ -129,11 +129,15 @@ export function SavedViewsProvider({ children }: { children: ReactNode }) { const [modalOpen, setModalOpen] = useState(false); // Switching modules (feed ⟷ plex) starts fresh: a feed view can't be the sticky/active one on plex, - // and a feed modal shouldn't linger open on plex. - useEffect(() => { + // and a feed modal shouldn't linger open on plex. Adjusted DURING RENDER against the previous + // source rather than from an effect, so the first plex render never commits carrying the feed's + // sticky id — which is exactly long enough for a stale view chip to paint. + const [prevSource, setPrevSource] = useState(source); + if (prevSource !== source) { + setPrevSource(source); setStickyId(null); setModalOpen(false); - }, [source]); + } // The ONE default-application path for both modules. On the views query's FIRST resolve for a // source, seed the starred default view's filters — but only into a genuinely fresh module (no @@ -146,17 +150,21 @@ export function SavedViewsProvider({ children }: { children: ReactNode }) { // per-account stored-filters key, which is always readable here: BootGate pins the tab's account // before this provider mounts. const seededRef = useRef>(new Set()); + // `cfg` re-memoizes on every filter edit, so it is read through a ref: the seed must fire on the + // views query's first resolve for a source and never again, which `seededRef` enforces. Listing + // cfg would re-arm nothing (the guard still holds) but would re-run the effect on every keystroke + // in the filter bar. The ref keeps the dep list honest without that. + const cfgRef = useRef(cfg); + cfgRef.current = cfg; useEffect(() => { const data = viewsQuery.data; if (!data || seededRef.current.has(source)) return; seededRef.current.add(source); - const def = pickDefaultToSeed(data, cfg.canSeedDefault()); + const c = cfgRef.current; + const def = pickDefaultToSeed(data, c.canSeedDefault()); if (!def) return; - cfg.setFilters(cfg.asFilters(def.filters)); + c.setFilters(c.asFilters(def.filters)); setStickyId(def.id); - // cfg is read fresh on this resolve and the per-source guard makes the body run once, so cfg - // (which re-memoizes on every filter edit) is deliberately kept out of the deps. - // eslint-disable-next-line react-hooks/exhaustive-deps }, [viewsQuery.data, source]); const currentKey = cfg.keyOf(cfg.filters); @@ -168,9 +176,10 @@ export function SavedViewsProvider({ children }: { children: ReactNode }) { ), [views, currentKey, stickyId, cfg], ); - useEffect(() => { - if (activeView) setStickyId(activeView.id); - }, [activeView]); + // The sticky id follows whichever view is active — also adjusted during render. It converges by + // construction: preferSticky returns the sticky match when there is one, so the very write below + // makes its own condition false on the next pass. + if (activeView && activeView.id !== stickyId) setStickyId(activeView.id); const stickyView = useMemo( () => (stickyId == null ? null : (views.find((v) => v.id === stickyId) ?? null)), diff --git a/frontend/src/components/WizardProvider.tsx b/frontend/src/components/WizardProvider.tsx index 36737c29..e51aee82 100644 --- a/frontend/src/components/WizardProvider.tsx +++ b/frontend/src/components/WizardProvider.tsx @@ -39,6 +39,7 @@ export function WizardProvider({ children }: { children: ReactNode }) { useEffect(() => { if (meId == null || canRead == null) return; const account = { can_read: canRead, is_demo: isDemo, google_enabled: googleEnabled }; + // eslint-disable-next-line react-hooks/set-state-in-effect -- opening is a one-way decision taken from SERVER data as it arrives, not a value derivable in render — deriving `open` from the grants would re-open a wizard the user had just closed, which is exactly what the narrowed dep list above exists to prevent. if (shouldAutoOpenOnboarding(account)) setOpen(true); }, [meId, canRead, canWrite, isDemo, googleEnabled]);