From 569190480d2ce73202e2947a374e105d2a1cab47 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Thu, 17 Sep 2026 02:45:19 +0200 Subject: [PATCH] refactor: hook dependencies name everything the hooks read The 12 react-hooks/exhaustive-deps warnings the gate let through: - ShareDialog and YearFilter built a fresh [] per render while their query loaded, so their memos recomputed on every render; they share one empty list now. - VideoEditor's drag effect called a seek/moveBoundary recreated each render; both are stable callbacks in its deps. - WizardProvider re-checks onboarding on exactly the fields it reads (not the me object, whose 60 s refetch would reopen a closed wizard). - Channels, Feed and PlexPlayer list the stable values they already used. - The player's keydown effect is bound once on purpose (its helpers read refs): said so, with a disable, as the codebase's other deliberate sites do. - dialogFocus and the YouTube player's cleanup read refs React may already have let go of; they use what they captured when the effect ran. --- frontend/src/components/Channels.tsx | 2 +- frontend/src/components/Feed.tsx | 8 ++--- frontend/src/components/PlayerModal.tsx | 4 +++ frontend/src/components/PlexPlayer.tsx | 5 +-- frontend/src/components/ShareDialog.tsx | 8 +++-- frontend/src/components/VideoEditor.tsx | 40 +++++++++++++--------- frontend/src/components/WizardProvider.tsx | 14 ++++++-- frontend/src/components/YearFilter.tsx | 6 ++-- frontend/src/components/ui/dialogFocus.ts | 7 +++- frontend/src/lib/playerModalHooks.ts | 4 ++- 10 files changed, 66 insertions(+), 32 deletions(-) diff --git a/frontend/src/components/Channels.tsx b/frontend/src/components/Channels.tsx index 41545499..a4a008e7 100644 --- a/frontend/src/components/Channels.tsx +++ b/frontend/src/components/Channels.tsx @@ -292,7 +292,7 @@ export default function Channels() { if (filtersResetToken === resetRef.current) return; resetRef.current = filtersResetToken; setSearch(""); - }, [filtersResetToken]); + }, [filtersResetToken, setSearch]); // Tag/priority/text filters stay controlled COLUMN filters (`filters.tags` etc.) shared by the top // chips, the column funnels and the cards. The sync-status filter (`filters.sync`) is no longer a diff --git a/frontend/src/components/Feed.tsx b/frontend/src/components/Feed.tsx index 7a113dc2..c0c1f1c4 100644 --- a/frontend/src/components/Feed.tsx +++ b/frontend/src/components/Feed.tsx @@ -230,15 +230,15 @@ export default function Feed({ // rows are unchanged, so clearing overrides there would make a just-hidden card flash back. // Only a real refetch (page count doesn't grow) is authoritative. const pagesLenRef = useRef(0); + const pagesLen = query.data?.pages?.length ?? 0; useEffect(() => { - const len = query.data?.pages?.length ?? 0; - const appended = len > pagesLenRef.current; - pagesLenRef.current = len; + const appended = pagesLen > pagesLenRef.current; + pagesLenRef.current = pagesLen; if (!appended) { setOverrides({}); setSavedOverrides({}); } - }, [query.dataUpdatedAt]); + }, [query.dataUpdatedAt, pagesLen]); // One shared query with the sidebar (same key + filters -> React Query dedupes to a single request): // the feed reads `.count`, the sidebar reads `.facets` (PERF-13, replaces feedCount + facets). diff --git a/frontend/src/components/PlayerModal.tsx b/frontend/src/components/PlayerModal.tsx index 10f30288..125449ce 100644 --- a/frontend/src/components/PlayerModal.tsx +++ b/frontend/src/components/PlayerModal.tsx @@ -489,6 +489,10 @@ 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]); // Create the player, resume from the saved position, checkpoint progress, and auto-mark watched at diff --git a/frontend/src/components/PlexPlayer.tsx b/frontend/src/components/PlexPlayer.tsx index 1b430b1f..3b628812 100644 --- a/frontend/src/components/PlexPlayer.tsx +++ b/frontend/src/components/PlexPlayer.tsx @@ -227,12 +227,13 @@ export default function PlexPlayer({ itemId, onClose, queue }: Props) { // Must be stable: an inline arrow would detach (null) and re-attach the node on every render, // and the fade's node-state ref would re-render in response — a loop. `fade.ref` is a setState. const tracksFade = useScrollFade(); + const tracksFadeRef = tracksFade.ref; const setTracksNode = useCallback( (node: HTMLElement | null) => { tracksRef.current = node as HTMLDivElement | null; - tracksFade.ref(node); + tracksFadeRef(node); }, - [tracksFade.ref], + [tracksFadeRef], ); // The keyboard cheat sheet is app-wide now (ui/KeyboardHelp, opened by the global "h"), not this // player's own overlay. Mirror its open state into a ref because usePlayerVolume still needs to diff --git a/frontend/src/components/ShareDialog.tsx b/frontend/src/components/ShareDialog.tsx index a9caf014..79ad9322 100644 --- a/frontend/src/components/ShareDialog.tsx +++ b/frontend/src/components/ShareDialog.tsx @@ -10,19 +10,23 @@ import { notify } from "../lib/notifications"; import { notifyActionError } from "../lib/actionErrors"; import { copyText } from "../lib/clipboard"; import { formatDate } from "../lib/format"; -import { api, type DownloadJob, type ShareLink } from "../lib/api"; +import { api, type DownloadJob, type ShareLink, type ShareRecipient } from "../lib/api"; import { inputCls } from "./ui/form"; import { Button } from "./ui/Button"; const EXPIRY_DAYS = [null, 1, 7, 30] as const; // --- share with a registered user (ACL) ----------------------------------------------------- +// One shared empty list while the recipients load: a fresh `[]` per render made `filtered` below +// recompute on every render. +const NO_RECIPIENTS: ShareRecipient[] = []; + function UserShare({ job }: { job: DownloadJob }) { const { t } = useTranslation(); const [q, setQ] = useState(""); const [open, setOpen] = useState(false); const recipients = useQuery({ queryKey: qk.shareRecipients(), queryFn: api.shareRecipients }); - const list = recipients.data ?? []; + const list = recipients.data ?? NO_RECIPIENTS; const filtered = useMemo(() => { const s = q.trim().toLowerCase(); if (!s) return list.slice(0, 8); diff --git a/frontend/src/components/VideoEditor.tsx b/frontend/src/components/VideoEditor.tsx index 696d4636..77b64591 100644 --- a/frontend/src/components/VideoEditor.tsx +++ b/frontend/src/components/VideoEditor.tsx @@ -130,10 +130,13 @@ export default function VideoEditor({ job, onClose }: { job: DownloadJob; onClos const v = videoRef.current; if (v) setCurrent(v.currentTime); }; - const seek = (s: number) => { - const v = videoRef.current; - if (v) v.currentTime = Math.max(0, Math.min(duration, s)); - }; + const seek = useCallback( + (s: number) => { + const v = videoRef.current; + if (v) v.currentTime = Math.max(0, Math.min(duration, s)); + }, + [duration], + ); const playPause = () => { const v = videoRef.current; if (!v) return; @@ -154,18 +157,21 @@ export default function VideoEditor({ job, onClose }: { job: DownloadJob; onClos return next; }); }; - const moveBoundary = (bi: number, time: number) => - setSegments((segs) => { - if (creating.current || bi < 0 || bi >= segs.length - 1) return segs; - // bi is in [0, length-2] here, so bi and bi+1 both index existing segments. - const lo = segs[bi]!.start + MIN_SEG; - const hi = segs[bi + 1]!.end - MIN_SEG; - const tt = Math.max(lo, Math.min(hi, time)); - const copy = [...segs]; - copy[bi] = { ...copy[bi]!, end: tt }; - copy[bi + 1] = { ...copy[bi + 1]!, start: tt }; - return copy; - }); + const moveBoundary = useCallback( + (bi: number, time: number) => + setSegments((segs) => { + if (creating.current || bi < 0 || bi >= segs.length - 1) return segs; + // bi is in [0, length-2] here, so bi and bi+1 both index existing segments. + const lo = segs[bi]!.start + MIN_SEG; + const hi = segs[bi + 1]!.end - MIN_SEG; + const tt = Math.max(lo, Math.min(hi, time)); + const copy = [...segs]; + copy[bi] = { ...copy[bi]!, end: tt }; + copy[bi + 1] = { ...copy[bi + 1]!, start: tt }; + return copy; + }), + [], + ); const deleteBoundary = (bi: number) => setSegments((segs) => { if (creating.current || segs.length < 2) return segs; @@ -205,7 +211,7 @@ export default function VideoEditor({ job, onClose }: { job: DownloadJob; onClos window.removeEventListener("pointermove", move); window.removeEventListener("pointerup", up); }; - }, [drag, timeAt]); + }, [drag, timeAt, seek, moveBoundary]); // --- crop drag --- const overlayRef = useRef(null); diff --git a/frontend/src/components/WizardProvider.tsx b/frontend/src/components/WizardProvider.tsx index 078f22b1..d55b3cfc 100644 --- a/frontend/src/components/WizardProvider.tsx +++ b/frontend/src/components/WizardProvider.tsx @@ -51,9 +51,19 @@ export function WizardProvider({ children }: { children: ReactNode }) { // First-login onboarding: prompt the user to connect YouTube (and resume the flow after each // consent redirect). Re-checked when the account or its grants change. + // Keyed on the fields that matter, NOT the `me` object: the 60 s refetch hands out a new object, + // and re-running on that would reopen a wizard the user just closed. `can_write` is read by nothing + // here but is a trigger: a completed write grant resumes the flow. + const meId = me?.id; + const canRead = me?.can_read; + const canWrite = me?.can_write; + const isDemo = me?.is_demo; + const googleEnabled = me?.google_enabled; useEffect(() => { - if (me && shouldAutoOpenOnboarding(me)) setOpen(true); - }, [me?.id, me?.can_read, me?.can_write]); + if (meId == null || canRead == null) return; + const account = { can_read: canRead, is_demo: isDemo, google_enabled: googleEnabled }; + if (shouldAutoOpenOnboarding(account)) setOpen(true); + }, [meId, canRead, canWrite, isDemo, googleEnabled]); const actions = useMemo( () => ({ openWizard, closeWizard }), diff --git a/frontend/src/components/YearFilter.tsx b/frontend/src/components/YearFilter.tsx index 12b8124e..e686f1df 100644 --- a/frontend/src/components/YearFilter.tsx +++ b/frontend/src/components/YearFilter.tsx @@ -3,7 +3,7 @@ import { useTranslation } from "react-i18next"; import { keepPreviousData, useQuery } from "@tanstack/react-query"; import { ChevronDown } from "lucide-react"; import { qk } from "../lib/queryKeys"; -import { api, type FeedFilters } from "../lib/api"; +import { api, type FeedFilters, type YearCount } from "../lib/api"; import { PopoverItem, PopoverSurface, useAnchoredPopover } from "./ui/AnchoredPopover"; import { filterChipClass } from "./ui/FilterChipGroup"; import CountBadge from "./ui/CountBadge"; @@ -21,6 +21,8 @@ import { useDebounced } from "../lib/useDebounced"; // own category. const QUICK = 6; // recent-year chips shown before the "More…" overflow +// One shared empty list while the counts load: a fresh `[]` per render made the memos below recompute. +const NO_YEARS: YearCount[] = []; export default function YearFilter({ filters, @@ -51,7 +53,7 @@ export default function YearFilter({ staleTime: 30_000, placeholderData: keepPreviousData, }); - const years = yearsQuery.data?.years ?? []; + const years = yearsQuery.data?.years ?? NO_YEARS; const selected = filters.year; // A year pick and the Upload-date control are mutually exclusive publish-date narrowings. diff --git a/frontend/src/components/ui/dialogFocus.ts b/frontend/src/components/ui/dialogFocus.ts index 56d32440..610c910d 100644 --- a/frontend/src/components/ui/dialogFocus.ts +++ b/frontend/src/components/ui/dialogFocus.ts @@ -108,6 +108,11 @@ export function useDialogChrome( // the scope itself, with `focusScope`), so the dialog opens focused rather than stranded on the // background. const scope = scopeRef.current; + // The dialog's element as it is while open, for the restore in the CLEANUP — where a closing + // dialog's refs are already detached (refs detach in the commit, passive cleanups run after). + // Today that reads the same either way (a detached owner and a null one both fail every + // `contains` below), but the cleanup should not depend on a ref React has already let go of. + const ownerAtOpen = ownerRef?.current ?? scope; if (scope && !scope.contains(document.activeElement)) { const back = focusedRef.current; const target = @@ -151,7 +156,7 @@ export function useDialogChrome( // this, a dialog closing on its own (an in-flight mutation's `onSuccess`) under a modal raised // above it pulled the focus out of that standing dialog onto our own trigger, behind its // scrim — where Enter still activated it (deep-review round 7). - const owner = ownerRef?.current ?? scopeRef.current; + const owner = ownerAtOpen; const active = document.activeElement; const above = active instanceof Element ? active.closest('[aria-modal="true"]') : null; const standingAbove = diff --git a/frontend/src/lib/playerModalHooks.ts b/frontend/src/lib/playerModalHooks.ts index 65572a35..b81e656c 100644 --- a/frontend/src/lib/playerModalHooks.ts +++ b/frontend/src/lib/playerModalHooks.ts @@ -441,6 +441,8 @@ export function useYtProgress({ }); }); + // The session's positions map (mutated in place, never replaced) — held here for the cleanup. + const sessionPos = sessionPosRef.current; // Periodic checkpoint so progress (and auto-watch) survive a crash/refresh. const timer = window.setInterval(persist, 5000); // Save on page unload too (F5/close/navigate): effect cleanup doesn't run on a full reload. A @@ -469,7 +471,7 @@ export function useYtProgress({ // not enough on its own: nothing refetches the feed until it remounts (staleTime 30s, // refetchOnWindowFocus off), so a reopen used to resume from the position the cache was // fetched with — and the next checkpoint then persisted that older value over this one. - patchCachedProgress(qc, sessionPosRef.current); + patchCachedProgress(qc, sessionPos); qc.invalidateQueries({ queryKey: qk.feed(), refetchType: "none" }); qc.invalidateQueries({ queryKey: qk.playlist(), refetchType: "none" }); });