diff --git a/.gitea/workflows/check.yml b/.gitea/workflows/check.yml index 21684623..017d6393 100644 --- a/.gitea/workflows/check.yml +++ b/.gitea/workflows/check.yml @@ -1,8 +1,14 @@ # Server-side mirror of the local `siftlode check` gate (E1 S3 / DX-1), so `main` is always green -# independent of any one machine. Two parallel jobs match the two local lanes: +# independent of any one machine. Two jobs match the two local lanes: # frontend — tsc (3 projects) + eslint + prettier --check + knip + vitest # backend — ruff + the full pytest suite (pure-logic + the DB-backed lane) against a real Postgres # +# They are declared side by side but they DO NOT run in parallel: the lab has a single act_runner +# with one slot, so they queue. Measured across runs #86-#91 — the backend job starts at the exact +# second the frontend one ends, every time, and the pair costs ~4 + ~6 = **~10 minutes from the +# push**. Anything that waits on this gate has to budget for the sum, not the max; the ops +# `release` read-back timed out on its first real run for believing otherwise. +# # Runner: the lab act_runner (LXC 115) mounts the Docker socket, so service containers work. The backend # runs pytest NATIVELY against a `postgres` service (not the local docker-compose.test.yml, whose # bind-mount is unreliable under a mounted-socket runner); fidelity is kept by installing the SAME pinned @@ -87,6 +93,22 @@ jobs: run: python -m ruff check --config backend/ruff.toml backend scripts - name: ruff format --check run: python -m ruff format --check --config backend/ruff.toml backend scripts + # The ninth lane (E27 S3). Run from backend/ so it resolves exactly as the local lane does — + # pyrightconfig.json's include/extraPaths are relative to the config, but the baseline is + # found relative to the working directory. New errors fail; the ~200 pre-existing ones are in + # .basedpyright/baseline.json and burn down epic by epic. Matching is per (file, rule, column + # span) plus a count, so a new error of the SAME shape in a file where a sibling was just + # fixed is absorbed — burn the hot files down first. + # + # The second step is not redundant. basedpyright TIGHTENS the baseline in place when the count + # drops, with no --writebaseline, and CI throws its workspace away — so without this a + # burn-down is green here and red on the developer's `siftlode check`, which has the same + # guard. Two gates over one tree must not answer differently. + - name: basedpyright + run: python -m basedpyright --project pyrightconfig.json + working-directory: backend + - name: basedpyright baseline is committed + run: git diff --exit-code -- backend/.basedpyright/baseline.json - name: pytest run: python -m pytest working-directory: backend diff --git a/.gitignore b/.gitignore index 4d92360b..d5349a1d 100644 --- a/.gitignore +++ b/.gitignore @@ -37,3 +37,7 @@ backups/ Thumbs.db .idea/ .vscode/ + +# TypeScript incremental build artifacts — machine-local, absolute-path-bearing, and no gate lane +# reads or regenerates them, so a committed one is stale from the commit that adds it. +*.tsbuildinfo diff --git a/.python-version b/.python-version new file mode 100644 index 00000000..e4fba218 --- /dev/null +++ b/.python-version @@ -0,0 +1 @@ +3.12 diff --git a/CHANGELOG.md b/CHANGELOG.md index 9383445c..005bd4d7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,23 @@ edit the release notes and re-run the generator. Upgrading, pinning a version and rolling back: [docs/self-hosting.md](docs/self-hosting.md#upgrading). +## 0.94.0 — 2026-09-22 + +No new features, on purpose: a release spent entirely on the machinery underneath, so that whole classes of fault stop being possible rather than being fixed one at a time. + +### Fixes + +- Typing an impossible number into a schedule - 0 minutes, 5000 minutes - used to close the editor and quietly keep the old value, as if it had been saved. The editor now says what the allowed range is, refuses to save until the number is inside it, and hands the keyboard back to the button you opened it from when you leave with Escape. + +### Internal + +- Every request is now measured against a size limit before anything else reads it - before a password is checked, before an address is even resolved. Until now a stranger could make the app swallow a 32 MB upload and only then be told the password was wrong. Each address carries its own ceiling, including the ones that legitimately receive a film. +- The server's Python is type-checked by the release gate for the first time: about 36 000 lines carried type annotations that nothing had ever verified. The mistakes already there are written down so they can be worked off release by release, and a new one now fails the build. +- The app moved to React 19 and its compiler. The compiler now does, automatically and correctly, the bookkeeping that was written by hand in about 150 places - and hand-written bookkeeping is where most of this year's stale-value and stale-click bugs came from. +- The 45 places where the old code told that same checker to stop looking at a piece of work are gone. Each was either a real bug, work that should never have been in that part of the code, or a genuine exception that now states the rule it depends on in code rather than in a comment. +- The engine versions the app is built and shipped with are now pinned and checked on both sides, the way the JavaScript side was fixed in the previous release. +- Four adversarial review rounds closed the work: 22 findings, then 13, then 6, then 2 - and the last two were in the review's own tests, not in the app. Everything found was fixed here, and every regression test was proven to fail with its own fix taken out. + ## 0.93.2 — 2026-09-20 An amount on a Plex film reads the same in every browser. diff --git a/CLAUDE.md b/CLAUDE.md index 84c8184f..c256bd4f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -7,17 +7,18 @@ worse than no rule — if you change how the code works, change this file in the Siftlode is a self-hosted YouTube reader (+ a local Plex browser/player and a download center). It is a **modular monolith**: one Postgres, one FastAPI process (uvicorn + APScheduler), one worker process -(yt-dlp/ffmpeg), one PO-token sidecar. React 18 + react-query frontend with a router-free navigation -provider. +(yt-dlp/ffmpeg), one PO-token sidecar. React 19 (with the React Compiler) + react-query frontend, with a +router-free navigation provider. ## The gate — never merge or ship red -The local gate is **ruff (`check` + `format --check`) + typecheck (3 tsc projects) + eslint + prettier +The local gate is **ruff (`check` + `format --check`) + basedpyright (backend) + typecheck (3 tsc projects) + eslint + prettier --check + knip + vitest + pytest**. It runs on every `publish`, which **refuses to build on failure — there is deliberately no `--skip` flag.** Run it and get it green **before** any merge, release, or handing work to review. - Full gate (agents with the private ops dispatcher): `siftlode check`. - Directly, without the dispatcher: `cd frontend && npm run typecheck && npm run lint && npm run knip && npm run test`, plus backend `ruff check backend` + `ruff format --check backend` and `pytest` (the DB-lane needs Postgres). +- **The backend type lane runs in the test image, not on the host** — `requirements.lock` cannot be installed on Windows (uvloop), and a type checker without the real packages reports nonsense. It fails on NEW errors only; the pre-existing ones sit in `backend/.basedpyright/baseline.json` and burn down epic by epic. Fixed some? Regenerate with `--writebaseline`. - New DB-backed test? It must actually run, not silently skip — assume `SIFTLODE_TEST_REQUIRE_DB=1`. ## Workflow @@ -57,7 +58,7 @@ how it gets built. Concretely: `routes/*` (HTTP only: validation, auth-dependency, serialization) → domain packages `sync/`, `plex/`, `downloads/`, `youtube/`, `issues/` (shared by routes + scheduler + worker) → flat core (`auth`, `quota`, -`sysconfig`, `state`, `audit`, `notifications`, `userscope`, `realtime`, `ratelimit`, `progress`, +`sysconfig`, `state`, `audit`, `notifications`, `userscope`, `realtime`, `ratelimit`, `bodylimit`, `progress`, `security`, `logs`, `logsink`, `diagnostics`, `debugsession`) → `models/db/config`. **Dependency direction is one-way: core never imports routes.** Frontend: `main.tsx` provider tree → `App.tsx` shell → `moduleRegistry` route-chunks → module components → the **platform layer `components/ui/`** (domain-free primitives: the popover engine and @@ -74,29 +75,34 @@ CQRS, a repository/DI framework, a router library, a generic CRUD-router factory ## Canonical patterns — when you have a choice, pick THIS -| Area | Use | Not | -| ----------------------- | ------------------------------------------------------------ | ------------------------------------------------------ | -| ORM | SQLAlchemy 2.0 `select()` | `db.query(...)` | -| Request body | a Pydantic model | `payload: dict` | -| Ownership check | `owned_or_404` / `get_or_404` | ad-hoc `_own_playlist`/`_owned`/`_own_job` | -| Admin gate import | `app.auth` | the `app.routes.admin` re-export | -| Upstream failure status | `422` with JSON detail | `502` (reserve 502 for raw byte-proxies) | -| User-facing error | structured `detail` (`{code, reason}`) | a bare English string under a bilingual UI | -| Mutation response | `{"ok": true, …}` (`{"deleted": id}`), test-held | a bare `{"unsubscribed"}`/`{"cleared": n}`, no `ok` | -| Transaction commit | the handler commits; helpers `flush` (test-held list) | helpers that commit (only `quota` owns its session) | -| Admin knob storage | `sysconfig` (values) | env / `app_state` / `scheduler_settings` scatter | -| Job status word | the `downloads/status.py` StrEnum | loose `"canceled"`/`"cancelled"` literals | -| Current time | inline `datetime.now(timezone.utc)` | new `now_utc`/`_now` helpers | -| FE per-account state | `usePref` | raw `readAccount`/`writeAccount` or bespoke hooks | -| FE action button | `ui/Button` — pick a `SIZES` rung | a hand-rolled ` + ) : ( + + + {score.toFixed(1)} + + ); +} + // Multi-source ratings cluster (E5 S5): IMDb (links out) · TMDb (with vote count) · Plex audience — // whichever are present. The "primary" score (IMDb, else audience) mirrors the backend rating column // (coalesce(imdb, audience)), so only it carries the ratingMin filter; the others are informational. @@ -65,29 +97,17 @@ function RatingsRow({ const primary = imdbRating ?? audience ?? null; // the filterable score const chipCls = "inline-flex items-center gap-1 rounded-md bg-amber-400/15 px-1.5 py-0.5 font-semibold text-amber-500"; - const Score = ({ score, filterable }: { score: number; filterable: boolean }) => - filterable && onFilterRating ? ( - - ) : ( - - - {score.toFixed(1)} - - ); const has = (x?: number | null): x is number => x != null; if (!has(imdbRating) && !has(audience) && !has(tmdbRating)) return null; return (
{has(imdbRating) && ( - + IMDb {imdbUrl && ( - + {t("plex.info.audience")} )} diff --git a/frontend/src/components/PlexPlayer.tsx b/frontend/src/components/PlexPlayer.tsx index 7063d7cd..2d513091 100644 --- a/frontend/src/components/PlexPlayer.tsx +++ b/frontend/src/components/PlexPlayer.tsx @@ -208,6 +208,13 @@ export default function PlexPlayer({ itemId, onClose, queue }: Props) { // Refs for values read inside stable callbacks / the keyboard handler (avoid stale closures). const barAutoHideRef = useRef(prefs.barAutoHide); barAutoHideRef.current = prefs.barAutoHide; + // The two language prefs are read only at ITEM LOAD (the detail effect below), so they go through + // a ref for the same reason `tRef` above does: naming them in that effect's deps would re-run it + // when the user picks a track from the player's own menu, double-loading the session the picker + // has already reloaded. The ref says "latest value, but not a trigger" in code, which an excused + // dep list only said in prose. Same shape as barAutoHideRef/seekStepRef either side of this. + const langPrefsRef = useRef({ audioLang: prefs.audioLang, subLang: prefs.subLang }); + langPrefsRef.current = { audioLang: prefs.audioLang, subLang: prefs.subLang }; const seekStepRef = useRef(prefs.seekStep); seekStepRef.current = prefs.seekStep; const fineSeekStepRef = useRef(prefs.fineSeekStep); @@ -474,7 +481,11 @@ export default function PlexPlayer({ itemId, onClose, queue }: Props) { restartsRef.current.at = now; if (restartsRef.current.n < MAX_SESSION_RECLAIMS) { restartsRef.current.n += 1; - loadSession(playhead(absRef, loadStartRef), !video.paused); + // Through the ref, not by name: this is loadSession calling ITSELF from inside its own + // useCallback, which the checker rightly reads as "accessed before it is declared". + // The ref (assigned right below the callback) is the file's existing answer to that — + // detachSessionRef and cancelAutoSkipRef do the same for the same reason. + void loadSessionRef.current(playhead(absRef, loadStartRef), !video.paused); return; } } @@ -497,6 +508,11 @@ export default function PlexPlayer({ itemId, onClose, queue }: Props) { }, [id], // NOT `t` — see tRef above (a language switch must not rebuild this / reload the session). ); + // The self-restart path above reaches the CURRENT loadSession through this, so the recursion is + // not a forward reference to a binding that is still being defined. + const loadSessionRef = useRef(loadSession); + // eslint-disable-next-line react-hooks/immutability -- the latest-value ref the self-restart path above reads; see the comment there for why the recursion cannot name loadSession directly. + loadSessionRef.current = loadSession; // Audio/subtitle track selection + subtitle rendering. Owns the selected-ordinal state; the shared // media refs stay component-owned (loadSession above + the detail effect below write audioRef too). @@ -552,16 +568,14 @@ export default function PlexPlayer({ itemId, onClose, queue }: Props) { restartsRef.current = { n: 0, at: 0 }; // Restore the per-account audio/subtitle language choice, resolved to THIS item's tracks. Set the // audio ref synchronously so the loadSession below picks the right track on the first request. - const aOrd = audioOrdForLang(detail.audio_streams ?? [], prefs.audioLang); + const { audioLang, subLang } = langPrefsRef.current; + const aOrd = audioOrdForLang(detail.audio_streams ?? [], audioLang); audioRef.current = aOrd; setAudioOrd(aOrd); - setSubOrd(subOrdForLang(detail.subtitle_streams ?? [], prefs.subLang)); + setSubOrd(subOrdForLang(detail.subtitle_streams ?? [], subLang)); const resume = detail.status !== "watched" ? detail.position_seconds || 0 : 0; - loadSession(resume); - // Prefs are applied at item load only; changing one via the picker must NOT re-run this (it would - // double-load the session — the picker handlers already reload/toggle). - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [detail, id, loadSession]); + void loadSession(resume); + }, [detail, id, loadSession, audioRef, setAudioOrd, setSubOrd]); // Leaving the item: a session still in flight for it must not land after it, and its stream goes. useEffect( () => () => { @@ -593,7 +607,11 @@ export default function PlexPlayer({ itemId, onClose, queue }: Props) { const v = videoRef.current; return () => { aliveRef.current = false; - // eslint-disable-next-line react-hooks/exhaustive-deps -- the live counter, as above + // The rule's advice — copy the ref into a variable and use the copy — is precisely wrong here. + // sessGenRef is a LIVE counter: bumping it is how the unmount tells every in-flight session + // that its response no longer belongs to anything, and a value captured when the effect ran + // would invalidate a generation that has since been superseded instead of the current one. + // eslint-disable-next-line react-hooks/exhaustive-deps sessGenRef.current++; if (hlsRef.current) { hlsRef.current.destroy(); @@ -839,7 +857,14 @@ export default function PlexPlayer({ itemId, onClose, queue }: Props) { setUpNextOpen(false); if (nextId) go(nextId); }, [nextId, go]); - useEffect(() => setUpNextOpen(false), [id]); + // The "up next" card belongs to the item that was playing, so a queue advance closes it. Adjusted + // during the render that changes `id` rather than in an effect: as an effect it closed one commit + // after the new episode had already drawn behind it. + const [upNextForId, setUpNextForId] = useState(id); + if (upNextForId !== id) { + setUpNextForId(id); + setUpNextOpen(false); + } // "Back" cascade shared by the on-screen Back arrow AND the keyboard/remote Backspace·Escape: peel // off whatever is layered on top (a tuning/tracks menu, then the info overlay) before leaving the diff --git a/frontend/src/components/PlexPlayerSettings.tsx b/frontend/src/components/PlexPlayerSettings.tsx index 0a8e9211..994f1d86 100644 --- a/frontend/src/components/PlexPlayerSettings.tsx +++ b/frontend/src/components/PlexPlayerSettings.tsx @@ -12,7 +12,10 @@ type SettingsTab = "sync" | "playback" | "subtitle" | "clock"; // panel owns no state of its own. `now` is the live wall clock (passed in, not a second ticker) used // only for the clock-date-style preview buttons. export function PlexPlayerSettings(props: { - menuRef: RefObject; + // `| null` is not optional under React 19's types: `useRef(null)` is now `RefObject`, + // which is the honest shape — the element is null until the div mounts. The old `RefObject` lied + // about that and 18's types let it. + menuRef: RefObject; settingsTabs: { key: SettingsTab; label: string }[]; activeTab: SettingsTab; setSettingsTab: (tab: SettingsTab) => void; diff --git a/frontend/src/components/PlexSearchBox.tsx b/frontend/src/components/PlexSearchBox.tsx index 3c8834fc..1195527c 100644 --- a/frontend/src/components/PlexSearchBox.tsx +++ b/frontend/src/components/PlexSearchBox.tsx @@ -48,8 +48,14 @@ export default function PlexSearchBox() { const suggestions = suggestEnabled ? (suggestQ.data?.items ?? []) : []; const open = suggestEnabled && (suggestQ.isFetching || suggestions.length > 0); - // A fresh query resets the highlight. - useEffect(() => setHi(-1), [dq, activeField, scope]); + // A fresh query resets the highlight — during THAT render, so no frame ever draws the new + // suggestion list with the old row highlighted. + const suggestKey = `${scope}|${activeField}|${dq}`; + const [hiFor, setHiFor] = useState(suggestKey); + if (hiFor !== suggestKey) { + setHiFor(suggestKey); + setHi(-1); + } // Keep the highlighted option scrolled into view during arrow-key navigation. useEffect(() => { if (hi < 0) return; 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/Scheduler.dom.test.tsx b/frontend/src/components/Scheduler.dom.test.tsx new file mode 100644 index 00000000..512d1980 --- /dev/null +++ b/frontend/src/components/Scheduler.dom.test.tsx @@ -0,0 +1,232 @@ +// @vitest-environment jsdom +// +// The scheduler's two number editors — a job's interval and the maintenance batch size. Both used to +// hold a MIRROR of the server value that an effect re-synced; both now show the server value and keep +// a draft only while the field is being edited (E27 S2). That matters here more than elsewhere +// because this page polls: every few seconds a fresh status arrives, and a mirror is exactly the +// thing a poll can overwrite at the wrong moment. +import { describe, it, expect, vi, afterEach, beforeEach } from "vitest"; +import { render, screen, fireEvent, cleanup, act, waitFor } from "@testing-library/react"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; + +vi.mock("react-i18next", async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, useTranslation: () => ({ t: (k: string) => k }) }; +}); + +import Scheduler from "./Scheduler"; +import { api, type SchedulerStatus } from "../lib/api"; +import { qk } from "../lib/queryKeys"; + +const status = (over: Partial = {}): SchedulerStatus => ({ + running_here: true, + enabled: true, + paused: false, + jobs: [ + { + id: "sync", + interval_minutes: 30, + next_run: null, + running: false, + status: "ok", + last_started: null, + last_finished: null, + last_result: null, + last_error: null, + progress: null, + }, + ], + queue: { + channels_recent_pending: 0, + channels_deep_pending: 0, + deep_eta_seconds: 0, + videos_pending_enrich: 0, + videos_pending_shorts: 0, + videos_live_refresh: 0, + }, + quota: { used_today: 0, remaining_today: 100, daily_budget: 100, backfill_reserve: 0 }, + maintenance: { revalidate_batch: 5000, revalidate_batch_default: 5000, min: 1000, max: 50000 }, + ...over, +}); + +let qc: QueryClient; +let current: SchedulerStatus; + +async function mount(initial = status()) { + current = initial; + qc = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + render( + + + , + ); + await screen.findByText("scheduler.maintenance.batchLabel"); +} + +/** A poll landing, driven through the query the page actually makes. */ +async function poll(next: SchedulerStatus) { + current = next; + await act(() => qc.refetchQueries({ queryKey: qk.scheduler() })); +} + +beforeEach(() => { + vi.spyOn(api, "schedulerStatus").mockImplementation(() => Promise.resolve(current)); +}); +afterEach(() => { + cleanup(); + vi.restoreAllMocks(); +}); + +const intervalBox = () => screen.queryByRole("spinbutton", { name: "" }); +const batchBox = () => screen.getByLabelText("scheduler.maintenance.batchLabel"); + +// The FIRST FOUR tests below are CHARACTERIZATION, not regression guards for E27 S2: the old +// JobRow's re-sync effect was already guarded by `if (!editing)` and the mirror only rendered while +// editing, so each of those four passes against the pre-sweep code too. They are here because the +// editor had no tests at all. The two after them — the range refusal and the focus return — ARE +// regression guards, for the silent-discard fix, and they fail against the code this file shipped +// with. The editor that genuinely lost the user's typing to a poll is the maintenance batch box +// below: its old effect had no guard, and that one has its own test. +describe("Scheduler · the job interval editor", () => { + it("opens on the current interval and commits a new one", async () => { + const update = vi.spyOn(api, "updateSchedulerJob").mockResolvedValue(undefined as never); + await mount(); + + fireEvent.click(screen.getByRole("button", { name: /scheduler.everyMin/ })); + const box = intervalBox()!; + expect(box).toHaveProperty("value", "30"); + + fireEvent.change(box, { target: { value: "45" } }); + fireEvent.keyDown(box, { key: "Enter" }); + expect(intervalBox()).toBeNull(); // committing closes the editor at once + await waitFor(() => expect(update).toHaveBeenCalledWith("sync", 45)); + }); + + it("drops what you typed on Escape and shows the server's value again", async () => { + const update = vi.spyOn(api, "updateSchedulerJob").mockResolvedValue(undefined as never); + await mount(); + + fireEvent.click(screen.getByRole("button", { name: /scheduler.everyMin/ })); + fireEvent.change(intervalBox()!, { target: { value: "999" } }); + fireEvent.keyDown(intervalBox()!, { key: "Escape" }); + + expect(update).not.toHaveBeenCalled(); + expect(intervalBox()).toBeNull(); + fireEvent.click(screen.getByRole("button", { name: /scheduler.everyMin/ })); + expect(intervalBox()).toHaveProperty("value", "30"); + }); + + it("keeps what you are typing when a poll lands underneath", async () => { + await mount(); + fireEvent.click(screen.getByRole("button", { name: /scheduler.everyMin/ })); + fireEvent.change(intervalBox()!, { target: { value: "45" } }); + + await poll(status({ jobs: [{ ...status().jobs[0]!, interval_minutes: 30, running: true }] })); + + expect(intervalBox()).toHaveProperty("value", "45"); + }); + + it("follows the server while the editor is closed", async () => { + await mount(); + await poll(status({ jobs: [{ ...status().jobs[0]!, interval_minutes: 90 }] })); + await waitFor(() => + expect(screen.getByRole("button", { name: /scheduler.everyMin/ })).toBeTruthy(), + ); + + fireEvent.click(screen.getByRole("button", { name: /scheduler.everyMin/ })); + expect(intervalBox()).toHaveProperty("value", "90"); + }); + + it("refuses a value outside the range instead of silently dropping it", async () => { + const update = vi.spyOn(api, "updateSchedulerJob").mockResolvedValue(undefined as never); + await mount(); + + fireEvent.click(screen.getByRole("button", { name: /scheduler.everyMin/ })); + fireEvent.change(intervalBox()!, { target: { value: "99999" } }); + fireEvent.keyDown(intervalBox()!, { key: "Enter" }); + + expect(update).not.toHaveBeenCalled(); + // Still open, still showing what was typed, and saying why. + expect(intervalBox()).toHaveProperty("value", "99999"); + expect(intervalBox()!.getAttribute("aria-invalid")).toBe("true"); + expect(screen.getByRole("alert").textContent).toBe("scheduler.range"); + }); + + it("hands focus back to the trigger when the editor closes", async () => { + await mount(); + const trigger = screen.getByRole("button", { name: /scheduler.everyMin/ }); + fireEvent.click(trigger); + expect(document.activeElement).toBe(intervalBox()); + + fireEvent.keyDown(intervalBox()!, { key: "Escape" }); + await waitFor(() => + expect(document.activeElement).toBe( + screen.getByRole("button", { name: /scheduler.everyMin/ }), + ), + ); + }); +}); + +describe("Scheduler · the maintenance batch editor", () => { + it("keeps what you are typing when a poll changes the batch underneath", async () => { + // THE regression this editor needed: its pre-E27 effect re-synced on every + // `revalidate_batch` change with no editing guard, so a poll landing mid-keystroke replaced + // what the admin had typed. + await mount(); + fireEvent.change(batchBox(), { target: { value: "9000" } }); + + // The poll carries a new interval TOO, and the interval label follows the server unconditionally + // — so waiting for it is how this test knows the new status has actually been rendered. Without + // that anchor the assertion below passes on a page that simply has not polled yet. + await poll( + status({ + jobs: [{ ...status().jobs[0]!, interval_minutes: 90 }], + maintenance: { ...status().maintenance, revalidate_batch: 12000 }, + }), + ); + await waitFor(() => + expect(screen.getByRole("button", { name: /scheduler.everyMin/ })).toBeTruthy(), + ); + + expect(batchBox()).toHaveProperty("value", "9000"); + }); + + it("refuses a value outside min..max instead of silently dropping it", async () => { + const save = vi.spyOn(api, "updateMaintenanceBatch").mockResolvedValue({ + revalidate_batch: 5000, + }); + await mount(); + + fireEvent.change(batchBox(), { target: { value: "10" } }); // below m.min = 1000 + fireEvent.keyDown(batchBox(), { key: "Enter" }); + + expect(save).not.toHaveBeenCalled(); + expect(batchBox()).toHaveProperty("value", "10"); + expect(batchBox().getAttribute("aria-invalid")).toBe("true"); + expect(screen.getByRole("alert").textContent).toBe("scheduler.range"); + expect(screen.getByRole("button", { name: "common.save" })).toHaveProperty("disabled", true); + }); + + it("is not dirty until edited, and stops being dirty once saved", async () => { + const save = vi + .spyOn(api, "updateMaintenanceBatch") + .mockResolvedValue({ revalidate_batch: 9000 }); + await mount(); + const saveBtn = () => screen.getByRole("button", { name: "common.save" }); + + expect(saveBtn()).toHaveProperty("disabled", true); + fireEvent.change(batchBox(), { target: { value: "9000" } }); + expect(saveBtn()).toHaveProperty("disabled", false); + + fireEvent.keyDown(batchBox(), { key: "Enter" }); + expect(saveBtn()).toHaveProperty("disabled", true); // the draft is dropped at once + await waitFor(() => expect(save).toHaveBeenCalledWith(9000)); + }); + + it("follows a poll that changes the batch while nothing is being typed", async () => { + await mount(); + expect(batchBox()).toHaveProperty("value", "5000"); + await poll(status({ maintenance: { ...status().maintenance, revalidate_batch: 12000 } })); + await waitFor(() => expect(batchBox()).toHaveProperty("value", "12000")); + }); +}); diff --git a/frontend/src/components/Scheduler.tsx b/frontend/src/components/Scheduler.tsx index 270d7cb4..bb3659f7 100644 --- a/frontend/src/components/Scheduler.tsx +++ b/frontend/src/components/Scheduler.tsx @@ -1,4 +1,4 @@ -import { useEffect, useState } from "react"; +import { useEffect, useId, useRef, useState } from "react"; import { qk } from "../lib/queryKeys"; import { useTranslation } from "react-i18next"; import i18n from "../i18n"; @@ -139,17 +139,33 @@ function JobRow({ runDisabled: boolean; }) { const { t } = useTranslation(); - const [editing, setEditing] = useState(false); - const [val, setVal] = useState(String(job.interval_minutes)); + // The box shows the server's value and holds a draft only while it is being edited, so there is + // nothing for an effect to re-sync: `null` means "follow the job". The poll behind this page + // refreshes every few seconds, which is exactly when a mirror-plus-effect would have shown the + // previous value for a frame. + const rangeId = useId(); + const [draft, setDraft] = useState(null); + const editing = draft !== null; + const val = draft ?? String(job.interval_minutes); const next = secsUntil(job.next_run); - useEffect(() => { - if (!editing) setVal(String(job.interval_minutes)); - }, [job.interval_minutes, editing]); + const m = parseInt(val, 10); + const valid = Number.isFinite(m) && m >= 1 && m <= 1440; + // Every way out of the editor unmounts the focused control, so hand focus back to the trigger that + // opened it — otherwise Escape drops a keyboard user onto and the next Tab restarts at the + // top of the page. Same pattern as IssueComposer's review buttons. + const triggerRef = useRef(null); + function close() { + setDraft(null); + queueMicrotask(() => triggerRef.current?.focus()); + } function save() { - const m = parseInt(val, 10); - if (Number.isFinite(m) && m >= 1 && m <= 1440 && m !== job.interval_minutes) onSave(m); - setEditing(false); + // A value outside the range is REFUSED, not swallowed: the draft stays so the user can see and + // correct what they typed, and `aria-invalid` + the hint say why. Dropping it silently left the + // field showing the server's number with no clue whether the save had happened. + if (!valid) return; + if (m !== job.interval_minutes) onSave(m); + close(); } return ( @@ -170,34 +186,46 @@ function JobRow({ max={1440} value={val} autoFocus - onChange={(e) => setVal(e.target.value)} + aria-invalid={!valid} + aria-errormessage={valid ? undefined : rangeId} + onChange={(e) => setDraft(e.target.value)} onKeyDown={(e) => { if (e.key === "Enter") save(); - if (e.key === "Escape") setEditing(false); + if (e.key === "Escape") close(); }} - className="w-16 bg-surface border border-border rounded-sm px-1.5 py-0.5 text-xs focus:outline-hidden focus:ring-2 focus:ring-accent/40" + className={`w-16 bg-surface border rounded-sm px-1.5 py-0.5 text-xs focus:outline-hidden focus:ring-2 ${ + valid + ? "border-border focus:ring-accent/40" + : "border-danger focus:ring-danger/40" + }`} /> {t("scheduler.minutes")} + {!valid && ( + + {t("scheduler.range", { min: 1, max: 1440 })} + + )} ) : (
+ {!valid && ( + + )}
{t("scheduler.maintenance.batchNote", { default: formatNumber(m.revalidate_batch_default), diff --git a/frontend/src/components/SetupWizard.tsx b/frontend/src/components/SetupWizard.tsx index e7377292..a02bcfee 100644 --- a/frontend/src/components/SetupWizard.tsx +++ b/frontend/src/components/SetupWizard.tsx @@ -20,17 +20,17 @@ export default function SetupWizard() { const { t, i18n } = useTranslation(); const [token] = useState(() => new URLSearchParams(window.location.search).get("token") ?? ""); const [info, setInfo] = useState<{ secrets_manageable: boolean } | null>(null); - const [tokenError, setTokenError] = useState(false); + // "No token at all" is not a fetch outcome, it is a fact about the URL — so it is derived rather + // than pushed into state by an effect, which had to run and commit before the error could show. + const [infoFailed, setInfoFailed] = useState(false); + const tokenError = !token || infoFailed; useEffect(() => { - if (!token) { - setTokenError(true); - return; - } + if (!token) return; api .setupInfo(token) .then(setInfo) - .catch(() => setTokenError(true)); + .catch(() => setInfoFailed(true)); }, [token]); // Step state. diff --git a/frontend/src/components/ShareDialog.tsx b/frontend/src/components/ShareDialog.tsx index 3fa4fe5a..0ea941bd 100644 --- a/frontend/src/components/ShareDialog.tsx +++ b/frontend/src/components/ShareDialog.tsx @@ -299,15 +299,16 @@ function LinkShare({ job }: { job: DownloadJob }) { // user had pressed "New link" → "Create". All further controls (password, download toggle, expiry, // extra links, revoke) stay available on the created link and via the "New link" button. const autoTried = useRef(false); + // The mutation OBJECT is rebuilt every render; `mutate` is not (react-query memoises it), so the + // two halves the effect actually uses can be named while `links.data` stays the only real trigger. + // The autoTried ref is what makes it fire exactly once. + const { mutate: createLink, isPending: createPending } = create; useEffect(() => { - if (links.data && links.data.length === 0 && !autoTried.current && !create.isPending) { + if (links.data && links.data.length === 0 && !autoTried.current && !createPending) { autoTried.current = true; - create.mutate(); + createLink(); } - // Only links.data (empty → auto-create once) should drive this; `create` is a fresh object every - // render and would re-run the effect needlessly. The autoTried ref makes it fire exactly once. - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [links.data]); + }, [links.data, createLink, createPending]); return (
diff --git a/frontend/src/components/TagFilterChips.dom.test.tsx b/frontend/src/components/TagFilterChips.dom.test.tsx new file mode 100644 index 00000000..cd2513fe --- /dev/null +++ b/frontend/src/components/TagFilterChips.dom.test.tsx @@ -0,0 +1,102 @@ +// @vitest-environment jsdom +// +// The chip-overflow popovers are open/closed by a DERIVED flag (`openRaw && hidden > 0`), which is +// what keeps a menu from outliving the "+N" trigger it is anchored to for a frame. The trap the +// derivation sets is that it hides the surface without closing it: floating-ui only reports a close +// for a real event on a mounted surface, so when the lane stops overflowing, the trigger and the +// menu disappear while the raw flag stays true — and the next re-fit that overflows again brings +// back a menu nobody asked for. `useChipOverflowFit` re-decides from a ResizeObserver, so that +// second re-fit needs no user action at all. +// +// The fit hook is mocked so `visible` is drivable: in jsdom every box measures 0, so the real +// measurement can only ever produce one answer. +import { describe, it, expect, vi, afterEach } from "vitest"; +import { render, screen, fireEvent, cleanup } from "@testing-library/react"; + +vi.mock("react-i18next", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + useTranslation: () => ({ + t: (k: string, o?: Record) => (o?.count != null ? `${k}:${o.count}` : k), + }), + }; +}); + +const fit = { visible: 1 }; +vi.mock("../lib/useChipOverflowFit", async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, useChipOverflowFit: () => fit.visible }; +}); + +import TagFilterChips, { type FilterChip } from "./TagFilterChips"; + +const chip = (key: string): FilterChip => ({ key, label: key, active: false, onToggle: () => {} }); +const chips = [chip("alpha"), chip("beta"), chip("gamma")]; + +// The surface mounts with `visibility: hidden` until floating-ui has positioned it, which never +// happens in jsdom (every box measures 0) — so it is out of the accessibility tree and only a +// `hidden: true` query can see it. Presence is the whole question here, not visibility. +const menu = () => screen.queryByRole("menu", { hidden: true }); +const moreBtn = () => screen.queryByRole("button", { name: /channels.tags.moreCount/ }); + +afterEach(() => { + cleanup(); + fit.visible = 1; +}); + +describe("TagFilterChips — the overflow menu", () => { + it("does not come back on its own once the lane stops overflowing", () => { + const { rerender } = render(); + fireEvent.click(moreBtn()!); + expect(menu()).toBeTruthy(); + + // The lane widens (a resize, the text-size slider, the trailing Clear button unmounting) and + // nothing is collapsed any more: both trigger and menu go. + fit.visible = 3; + rerender(); + expect(menu()).toBeNull(); + expect(moreBtn()).toBeNull(); + + // …and it narrows again, with no user interaction in between. The menu must stay shut, and the + // returning trigger must not claim to be expanded. + fit.visible = 1; + rerender(); + expect(menu()).toBeNull(); + expect(moreBtn()).toHaveProperty("ariaExpanded", "false"); + }); + + it("does not come back when the chip set SHRINKS under the open menu", () => { + // The nastier half of the same bug, and it needs the hook's real timing to reproduce. + // `useChipOverflowFit` keeps `visible` in state and only rewrites it from a LAYOUT EFFECT, so + // the render in which the set shrinks still carries the measurement taken for the PREVIOUS, + // longer set. `length - visible` is negative in that render, a `hidden === 0` guard reading it + // unclamped never fires, and the flag survives the one window it exists to close. + // + // So `fit.visible` is left ALONE across the shrink — that staleness is the precondition, not a + // convenience of the mock. Five chips of which three fit, then three of them are deleted in the + // tag manager — so the shrink render computes `chips.length - visible` = 2 - 3 = -1. + const five = ["alpha", "beta", "gamma", "delta", "epsilon"].map(chip); + fit.visible = 3; + const { rerender } = render(); + fireEvent.click(moreBtn()!); + expect(menu()).toBeTruthy(); + + rerender(); // `visible` is still the stale 3 + expect(menu()).toBeNull(); + + // …and now the layout effect lands: two long names, only one of them fits. + fit.visible = 1; + rerender(); + expect(menu()).toBeNull(); + expect(moreBtn()).toHaveProperty("ariaExpanded", "false"); + }); + + it("still opens and closes on the trigger", () => { + render(); + fireEvent.click(moreBtn()!); + expect(menu()).toBeTruthy(); + fireEvent.click(moreBtn()!); + expect(menu()).toBeNull(); + }); +}); diff --git a/frontend/src/components/TagFilterChips.tsx b/frontend/src/components/TagFilterChips.tsx index 2d6bb4b2..c9ba083e 100644 --- a/frontend/src/components/TagFilterChips.tsx +++ b/frontend/src/components/TagFilterChips.tsx @@ -1,4 +1,4 @@ -import { useEffect, useRef, useState } from "react"; +import { useRef, useState } from "react"; import { useTranslation } from "react-i18next"; import { fitChipsWithPill } from "../lib/chipOverflow"; import { chipLaneClass, useChipOverflowFit } from "../lib/useChipOverflowFit"; @@ -58,20 +58,33 @@ export default function TagFilterChips({ fitChipsWithPill({ chipRights: g.chipRights, pillW: g.pillW, avail: g.avail, gap: g.gap }), }); - const hidden = chips.length - visible; - const [open, setOpen] = useState(false); + // Clamped, and not defensively: `useChipOverflowFit` keeps `visible` in state and re-decides in a + // layout effect, so the render in which the chip set SHRINKS still holds the previous measurement + // and a raw subtraction goes negative. An unclamped `hidden` slips past the `=== 0` reset below — + // the flag survives exactly the window it exists to close, and the popover springs back on the + // next re-fit. + const hidden = Math.max(0, chips.length - visible); + // The popover can only be open while there is a "+N" trigger to anchor it. Derived rather than + // closed by an effect, so the frame in which the last overflowing chip fits again cannot also + // contain a menu hanging off a reference that is already gone. + // + // Deriving alone is NOT enough, and the missing half is the flag itself: floating-ui only reports + // a close for a real event on a mounted surface, so when the lane stops overflowing the trigger + // and the menu just disappear and `openRaw` stays true. The next re-fit that overflows again — + // a window resize, the text-size slider, or the trailing Clear button mounting as a tag is + // toggled; `useChipOverflowFit` re-decides from a ResizeObserver, with no user near this menu — + // would then re-open a menu nobody asked for, with its trigger already `aria-expanded`. So the + // flag is reset in RENDER against the same condition that hides it: convergent (the write + // falsifies it) and, unlike an effect, gone before the frame is painted. + const [openRaw, setOpen] = useState(false); + if (openRaw && hidden === 0) setOpen(false); + const open = openRaw && hidden > 0; const pop = useAnchoredPopover({ open, onOpenChange: setOpen, mode: "menu", placement: "bottom-end", }); - // If nothing overflows anymore, the "+N" trigger unmounts — force the popover closed so it can't - // linger against a gone reference. - useEffect(() => { - if (hidden === 0 && open) setOpen(false); - }, [hidden, open]); - const moreLabel = t("channels.tags.moreCount", { count: hidden }); return ( diff --git a/frontend/src/components/WatchPage.tsx b/frontend/src/components/WatchPage.tsx index f3ae6f37..b6b26639 100644 --- a/frontend/src/components/WatchPage.tsx +++ b/frontend/src/components/WatchPage.tsx @@ -138,8 +138,10 @@ const T = STR[lang]; export default function WatchPage() { const token = window.location.pathname.split("/watch/")[1]?.split(/[/?#]/)[0] ?? ""; + // Seeded from the URL rather than corrected by the effect below: with no token there is nothing + // to load, and "loading" was committed and painted before the effect said so. const [status, setStatus] = useState<"loading" | "ready" | "password" | "gone" | "error">( - "loading", + token ? "loading" : "gone", ); const [meta, setMeta] = useState(null); const [password, setPassword] = useState(""); @@ -166,10 +168,7 @@ export default function WatchPage() { // Bumped by Retry, which asks again. const [attempt, setAttempt] = useState(0); useEffect(() => { - if (!token) { - setStatus("gone"); - return; - } + if (!token) return; // the seed above already says "gone" send(`/api/public/watch/${encodeURIComponent(token)}`) .then((r) => { // Only a 404 says the link is gone (unknown, expired, revoked, or its file removed). Anything diff --git a/frontend/src/components/Welcome.tsx b/frontend/src/components/Welcome.tsx index fb273019..611a14e7 100644 --- a/frontend/src/components/Welcome.tsx +++ b/frontend/src/components/Welcome.tsx @@ -432,12 +432,17 @@ function AuthCard({ verify }: { verify: VerifyEntry }) { // `done` and `error` belong to the view that produced them, and the render is `done ? msg : form` // — without this, Back out of a submitted form would show the old success message under the // previous view's title. Clearing on view change covers both Back/Forward and the mode links. - useEffect(() => { + // Cleared during the render that changes the view, not a commit later: the render is + // `done ? msg : form`, so an effect meant Back out of a submitted form drew the old success + // message under the new view's title for one frame — which is the exact symptom this guards. + const [outcomeFor, setOutcomeFor] = useState(mode); + if (outcomeFor !== mode) { + setOutcomeFor(mode); setDone(""); setError(""); setBlocked(null); setDeadReset(false); - }, [mode]); + } // Explicit demo entry (replaces the old hidden probe): a labelled email field, still gated // by the demo whitelist server-side. 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]); diff --git a/frontend/src/components/channelsContext.ts b/frontend/src/components/channelsContext.ts index 5aacab1f..99f49cd0 100644 --- a/frontend/src/components/channelsContext.ts +++ b/frontend/src/components/channelsContext.ts @@ -33,6 +33,10 @@ export interface ChannelsActions { // "Focus this channel in the manager": jump to Channels (subscriptions tab) and seed its name // filter so the channel is isolated. Called from the feed sidebar and the notifications inbox. focusChannel: (name: string) => void; + // Mark the current focus intent as applied. The manager calls this the moment it seeds its name + // filter, so the intent cannot be re-applied when the page chunk remounts on a later visit and + // overwrite a search the user has typed since. + consumeFocus: () => void; // Jump to the manager filtered to channels still missing full history (the header sync chip). goToFullHistory: () => void; } @@ -57,6 +61,7 @@ export const ChannelsActionsContext = createContext({ setLayout: () => {}, setSort: () => {}, focusChannel: () => {}, + consumeFocus: () => {}, goToFullHistory: () => {}, }); diff --git a/frontend/src/components/comfortPin.tsx b/frontend/src/components/comfortPin.tsx index d7ff1bc7..8abe1494 100644 --- a/frontend/src/components/comfortPin.tsx +++ b/frontend/src/components/comfortPin.tsx @@ -1,4 +1,4 @@ -import { useCallback, useEffect, useState } from "react"; +import { useCallback, useState } from "react"; import { useTranslation } from "react-i18next"; import { useQuery, useQueryClient } from "@tanstack/react-query"; import { Loader2, Pin, PinOff } from "lucide-react"; @@ -100,9 +100,9 @@ export function useComfortPin(open: boolean): ComfortPin { // Latch: react-query keeps the data cached when `enabled` goes false again, so this only decides // WHEN the first fetch happens, not whether the answer survives the menu closing. const [everOpen, setEverOpen] = useState(false); - useEffect(() => { - if (open) setEverOpen(true); - }, [open]); + // Latched during render: the query below is gated on it, so an effect delayed the first fetch by + // a commit for no reason. Convergent — the write falsifies its own condition. + if (open && !everOpen) setEverOpen(true); const shelf = useQuery({ queryKey: qk.comfortPins(), diff --git a/frontend/src/components/downloadRow.tsx b/frontend/src/components/downloadRow.tsx index f5e0861b..9de3413d 100644 --- a/frontend/src/components/downloadRow.tsx +++ b/frontend/src/components/downloadRow.tsx @@ -1,4 +1,4 @@ -import { useEffect, useState, type ReactNode } from "react"; +import { useState, type ReactNode } from "react"; import { useTranslation } from "react-i18next"; import { CalendarClock, Copy, File, Link2, Scissors } from "lucide-react"; import clsx from "clsx"; @@ -77,8 +77,15 @@ function Thumb({ job }: { job: DownloadJob }) { const poster = job.poster_url ? api.posterImageUrl(job.id) : null; const primary = poster || remote; // On load failure (a genuinely missing poster file) fall back to the remote source thumbnail. - const [src, setSrc] = useState(primary); - useEffect(() => setSrc(primary), [primary]); + // Remembered BY the URL that failed, not as a value an effect re-syncs: these rows are recycled + // down a long virtualized list, so a boolean-style mirror painted the previous job's fallback + // over the next one's poster for a frame. See ui/Avatar, which had the same defect. + // …and forgotten once `primary` has moved off it, for the same reason ui/Avatar does: a poster + // that 404'd because its file was still being written should be re-requested when the row comes + // back, not written off for the life of the recycled slot. Convergent, so it belongs in render. + const [failedPrimary, setFailedPrimary] = useState(null); + if (failedPrimary !== null && failedPrimary !== primary) setFailedPrimary(null); + const src = failedPrimary && failedPrimary === primary ? remote : primary; // A generic uploaded file (media_kind='file') has no video frame to poster — it's a document/ // archive, not playable media. Show a neutral file glyph instead of an (always-empty) thumbnail. if (job.asset?.media_kind === "file") { @@ -97,7 +104,7 @@ function Thumb({ job }: { job: DownloadJob }) { loading="lazy" className="w-full h-full object-cover" onError={() => { - if (remote && src !== remote) setSrc(remote); + if (remote && src !== remote) setFailedPrimary(primary); }} /> ) : null} diff --git a/frontend/src/components/ui/Avatar.dom.test.tsx b/frontend/src/components/ui/Avatar.dom.test.tsx new file mode 100644 index 00000000..74722030 --- /dev/null +++ b/frontend/src/components/ui/Avatar.dom.test.tsx @@ -0,0 +1,84 @@ +// @vitest-environment jsdom +import { useLayoutEffect } from "react"; +import { describe, it, expect, afterEach } from "vitest"; +import { render, screen, fireEvent, cleanup } from "@testing-library/react"; + +import Avatar from "./Avatar"; + +afterEach(cleanup); + +// The failure state is keyed by URL rather than held as a boolean an effect resets, and the reason +// is a frame the user could see: Avatar is recycled across items in the virtualized feed, so after +// ONE image 429s (Google's avatar CDNs throttle bursts — see the component's own note), the next +// item to land in that slot rendered with its new `src` and the previous item's `failed`. The +// fallback initial was therefore committed and painted over a perfectly good image, and only the +// post-paint effect put it right. +// +// `useLayoutEffect` is what makes that observable instead of a race: layout effects run against the +// committed DOM BEFORE any passive effect, so the snapshot it takes is exactly the frame the browser +// would paint — the one the old reset effect had not repaired yet. +function Probe({ src, painted }: { src: string; painted: string[] }) { + useLayoutEffect(() => { + painted.push(document.body.innerHTML); + }); + return ; +} + +describe("Avatar · a failed image must not follow the component to the next item", () => { + it("shows the new src in the SAME commit that changes it, with no fallback frame between", () => { + const painted: string[] = []; + const { rerender } = render(); + + fireEvent.error(screen.getByRole("img", { name: "avatar" })); + expect(screen.queryByRole("img")).toBeNull(); + expect(screen.getByText("Z")).toBeTruthy(); + + painted.length = 0; + rerender(); + + // The FIRST frame after the src change already carries the image. Reading the last snapshot + // instead would let a passive-effect repair pass, which is precisely the bug. + expect(painted.length).toBeGreaterThan(0); + expect(painted[0]).toContain("https://cdn.test/b.jpg"); + expect(painted[0]).not.toContain(">Z<"); + }); + + // CHARACTERIZATION: the old boolean-plus-effect passes this too (the rerender is wrapped in act(), + // so its reset effect has run by the time the src is read). The frame-level guard is the test + // above, which the old shape fails; the retry guard is the test below, which it also fails. + it("keeps the fallback for the URL that actually failed, not for whatever is current", () => { + const { rerender } = render( + , + ); + fireEvent.error(screen.getByRole("img", { name: "avatar" })); + expect(screen.getByText("Z")).toBeTruthy(); + + rerender(); + expect(screen.getByRole("img", { name: "avatar" }).getAttribute("src")).toBe( + "https://cdn.test/b.jpg", + ); + }); + + it("retries a URL it comes back to — a 429 is transient, and so is the memory of it", () => { + // The instance is not remounted here on purpose: the player's channel avatar and the Messages + // thread header both swap `src` on a live instance. A memory that outlived the reason for it + // would pin a throttled channel to its initial for the rest of the session. + const { rerender } = render( + , + ); + fireEvent.error(screen.getByRole("img", { name: "avatar" })); + expect(screen.getByText("Z")).toBeTruthy(); + + rerender(); + rerender(); + expect(screen.getByRole("img", { name: "avatar" }).getAttribute("src")).toBe( + "https://cdn.test/a.jpg", + ); + }); + + it("falls back when there is no src at all", () => { + render(); + expect(screen.queryByRole("img")).toBeNull(); + expect(screen.getByText("Z")).toBeTruthy(); + }); +}); diff --git a/frontend/src/components/ui/Avatar.tsx b/frontend/src/components/ui/Avatar.tsx index c7477b5c..7172df31 100644 --- a/frontend/src/components/ui/Avatar.tsx +++ b/frontend/src/components/ui/Avatar.tsx @@ -1,4 +1,4 @@ -import { useEffect, useState } from "react"; +import { useState } from "react"; // Avatar image with a graceful fallback. // @@ -20,8 +20,22 @@ export default function Avatar({ fallback?: string; // a name/title; its first letter is shown when there's no image className?: string; }) { - const [failed, setFailed] = useState(false); - useEffect(() => setFailed(false), [src]); + // The failure is remembered BY URL, not as a boolean that an effect resets when `src` changes. + // That distinction is visible: this component is recycled across items in the virtualized feed, so + // one avatar's 429 used to paint the fallback initial over the NEXT item's perfectly good image + // for a frame — React committed the render with the new `src` and the stale `failed`, and only the + // post-paint effect cleared it. Derived state cannot be stale, so there is nothing to reset. + // + // Forgetting the URL once `src` has moved off it is the other half, and it is what keeps the 429 + // TRANSIENT: without it the memory outlives the reason for it, and a mounted instance whose `src` + // swaps — the player's channel avatar stepping through a queue, the Messages thread header — would + // show the initial for a throttled channel forever, never re-requesting even after the CDN + // relented. Written in render because it is convergent (the write falsifies its own condition) and + // because an effect would leave one committed frame in which the memory is stale, which is the + // frame this whole component is about. + const [failedSrc, setFailedSrc] = useState(null); + if (failedSrc !== null && failedSrc !== src) setFailedSrc(null); + const failed = !!src && failedSrc === src; if (src && !failed) { return ( @@ -30,7 +44,7 @@ export default function Avatar({ alt={alt} loading="lazy" referrerPolicy="no-referrer" - onError={() => setFailed(true)} + onError={() => setFailedSrc(src)} className={className} /> ); diff --git a/frontend/src/components/ui/BackToTop.tsx b/frontend/src/components/ui/BackToTop.tsx index ec9ff758..51399a3a 100644 --- a/frontend/src/components/ui/BackToTop.tsx +++ b/frontend/src/components/ui/BackToTop.tsx @@ -36,6 +36,7 @@ export default function BackToTop({ dep, threshold = 600 }: { dep?: unknown; thr useEffect(() => { const el = document.querySelector("main"); scrollerRef.current = el; + // eslint-disable-next-line react-hooks/set-state-in-effect -- reading the DOM is the point —
is swapped by the page switch above us, and only after that commit can its scrollTop be read at all. Nothing derivable in render knows where the new page is scrolled to. setShow(!!el && el.scrollTop > threshold); }, [dep, threshold]); diff --git a/frontend/src/components/ui/Combobox.dom.test.tsx b/frontend/src/components/ui/Combobox.dom.test.tsx index 1feb7335..ec5e62ff 100644 --- a/frontend/src/components/ui/Combobox.dom.test.tsx +++ b/frontend/src/components/ui/Combobox.dom.test.tsx @@ -246,6 +246,56 @@ describe("Combobox", () => { expect(onChange).toHaveBeenCalledWith(["Action"]); // replaced, not appended expect(screen.getByRole("combobox").getAttribute("aria-expanded")).toBe("false"); // closed }); + + // The typed query is discarded when the list closes. CHARACTERIZATION, not the regression guard: + // the old effect-based clear satisfies these assertions too, because RTL wraps fireEvent in act() + // and the passive effect has flushed by the time the value is read. What the move into the close + // funnel actually fixed is measured by the next test, which the old shape fails. This one is here + // because the behaviour is worth pinning: the list is for "find and pick", and a leftover filter + // would hide everything worth seeing the next time it opens. + it("discards the typed query when the list closes, so the next open shows everything", () => { + wrap(); + const input = screen.getByRole("combobox"); + fireEvent.focus(input); + fireEvent.change(input, { target: { value: "com" } }); + expect(input).toHaveProperty("value", "com"); + expect(screen.queryByText("Drama")).toBeNull(); // filtered away + + fireEvent.keyDown(input, { key: "Escape" }); + expect(input.getAttribute("aria-expanded")).toBe("false"); + expect(input).toHaveProperty("value", ""); + + fireEvent.focus(input); + expect(screen.getByText("Drama")).toBeTruthy(); // the whole list is back + }); + + it("does not report a query change until something actually changes it", () => { + // A clear driven by an effect on `open` fires at MOUNT too — `open` starts false — so a + // controlled caller was told its own initial query had been cleared before the user touched + // anything. Driving it from the close event instead means no event, no callback. + const onQueryChange = vi.fn(); + const onOpenChange = vi.fn(); + wrap( + , + ); + expect(onQueryChange).not.toHaveBeenCalled(); + + fireEvent.focus(screen.getByRole("combobox")); + expect(onQueryChange).not.toHaveBeenCalled(); + fireEvent.click(screen.getByText("Action")); + expect(onOpenChange).toHaveBeenLastCalledWith(false); + expect(onQueryChange.mock.calls).toEqual([[""]]); + }); }); // PageUp/PageDown (E11, owner request): the sidebar's channel filter and the Plex pickers run to diff --git a/frontend/src/components/ui/Combobox.tsx b/frontend/src/components/ui/Combobox.tsx index 7fb775a2..6d314796 100644 --- a/frontend/src/components/ui/Combobox.tsx +++ b/frontend/src/components/ui/Combobox.tsx @@ -106,6 +106,13 @@ export default function Combobox({ const setOpenAnd = (next: boolean) => { setOpen(next); onOpenChange?.(next); + // Closing discards the typed query: the list is for "find and pick", and a leftover filter would + // hide everything worth seeing on the next open. It belongs HERE, not in an effect on `open`, + // because this is the single funnel every close goes through — floating-ui's dismiss, Escape and + // outside-click all arrive as onOpenChange, and the single-select commit calls it directly + // (verified: `setOpen` is called nowhere else). So the clear lands in the same commit as the + // close, and the controlled caller's onQueryChange fires once, from an event rather than a render. + if (!next) setQuery(""); }; const pop = useAnchoredPopover({ open, onOpenChange: setOpenAnd, mode: "combobox" }); // The input is the combobox reference for floating-ui AND our own focus target (the field's @@ -127,12 +134,6 @@ export default function Combobox({ // would make Enter a no-op over a highlighted row. const { activeIndex, highlightIndex } = pop; - // Clear the typed query when the list closes. - useEffect(() => { - if (!open) setQuery(""); - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [open]); - const filtered = useMemo(() => { if (!clientFilter) return options; const q = query.trim().toLowerCase(); diff --git a/frontend/src/components/ui/CommandPalette.tsx b/frontend/src/components/ui/CommandPalette.tsx index d21fddbc..b661d342 100644 --- a/frontend/src/components/ui/CommandPalette.tsx +++ b/frontend/src/components/ui/CommandPalette.tsx @@ -81,6 +81,15 @@ export default function CommandPalette({ onClose: () => void; groups: CommandGroup[]; }) { + // Closed means UNMOUNTED, not merely hidden, and that one line replaces the effect that used to + // blank the query and the picked row on every open: fresh state is what mounting already means. + // It also stops the ranking memo below — which folds and scores every channel, playlist and saved + // view — from re-running on every parent render while the palette is shut. + if (!open) return null; + return ; +} + +function CommandPaletteBody({ onClose, groups }: { onClose: () => void; groups: CommandGroup[] }) { const { t } = useTranslation(); const listId = useId(); const groupId = useId(); @@ -94,15 +103,6 @@ export default function CommandPalette({ const activeRef = useRef(null); const listRef = useRef(null); - // A fresh open starts from a blank field: the palette is for "take me somewhere", and a leftover - // query from last time hides everything you might want now. - useEffect(() => { - if (open) { - setQuery(""); - setPicked(null); - } - }, [open]); - // The GROUP order stays fixed — modules, channels, playlists, views, actions read the same every // time — and the rows are ranked inside each group. The best row overall starts highlighted, so // Enter takes the best match even when it sits in a later group. @@ -144,6 +144,7 @@ export default function CommandPalette({ const [announced, setAnnounced] = useState(""); useEffect(() => { if (!summary) { + // eslint-disable-next-line react-hooks/set-state-in-effect -- the announcement is DEBOUNCED on purpose (ANNOUNCE_DELAY_MS above): a polite region rewritten per keystroke queues one utterance per letter. A timer's result cannot be derived in render, and the empty branch is the same timer's cancel. setAnnounced(""); return; } @@ -151,8 +152,6 @@ export default function CommandPalette({ return () => window.clearTimeout(timer); }, [summary]); - if (!open) return null; - // Close, wait for this dialog's own Back-to-close entry to be reclaimed, THEN run: most commands // navigate, and a pushState issued before that reclaim is undone by it. Only THIS dialog closes — // a command that navigates closes whatever is underneath itself, one that does not (a theme) must diff --git a/frontend/src/components/ui/DataTable.tsx b/frontend/src/components/ui/DataTable.tsx index d13e2ba3..c84b246e 100644 --- a/frontend/src/components/ui/DataTable.tsx +++ b/frontend/src/components/ui/DataTable.tsx @@ -1,4 +1,4 @@ -import { useEffect, useRef, useState, type ReactNode } from "react"; +import { useCallback, useEffect, useRef, useState, type ReactNode } from "react"; import { useTranslation } from "react-i18next"; import { sortRows, cycleSort, type SortState } from "../../lib/columnSort"; import Pager from "./Pager"; @@ -149,10 +149,19 @@ export default function DataTable({ // `filters` as a dep (which would re-run them on every filter change). const filtersRef = useRef(filters); filtersRef.current = filters; - const commitFilters = (next: FilterMap) => { - if (controlledFilters) onFiltersChange!(next); - else setInternalFilters(next); - }; + // …and the same trick for the CALLBACK, one line further: a controlled caller almost always + // passes an inline arrow, so `onFiltersChange` is a fresh function on every render. Reading it + // through a ref is what lets `commitFilters` be stable, and a stable commitFilters is what lets + // the two effects below NAME it in their dep lists instead of switching the check off. + const onFiltersChangeRef = useRef(onFiltersChange); + onFiltersChangeRef.current = onFiltersChange; + const commitFilters = useCallback( + (next: FilterMap) => { + if (controlledFilters) onFiltersChangeRef.current!(next); + else setInternalFilters(next); + }, + [controlledFilters], + ); // Persist page/size always, but only the sort/filter slices this table actually OWNS — a controlled // caller persists those itself, so writing them here would just leave a stale, never-read duplicate. @@ -169,12 +178,16 @@ export default function DataTable({ ); }, [persistKey, controlledSort, controlledFilters, sort, filters, page, size]); + // Destructured so the deps are the values the body actually reads. `externalFilter` itself is an + // object literal at every call site, so depending on it would re-apply the filter — and reset the + // page — on every parent render. + const externalKey = externalFilter?.key; + const externalValue = externalFilter?.value; useEffect(() => { - if (!externalFilter) return; - commitFilters({ ...filtersRef.current, [externalFilter.key]: externalFilter.value }); + if (externalKey === undefined || externalValue === undefined) return; + commitFilters({ ...filtersRef.current, [externalKey]: externalValue }); setPage(0); - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [externalFilter?.key, externalFilter?.value]); + }, [externalKey, externalValue, commitFilters]); // Clear column filters when the reset token advances. The ref starts at 0 (not at the prop) // so a navigation that bumped the token still clears even when it remounts this table fresh @@ -186,8 +199,7 @@ export default function DataTable({ resetRef.current = resetFiltersToken; commitFilters({}); setPage(0); - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [resetFiltersToken]); + }, [resetFiltersToken, commitFilters]); const sorted = sortRows(filterRows(rows, columns, filters), columns, sort); diff --git a/frontend/src/components/ui/Pager.dom.test.tsx b/frontend/src/components/ui/Pager.dom.test.tsx index 2f187cbb..92f9d550 100644 --- a/frontend/src/components/ui/Pager.dom.test.tsx +++ b/frontend/src/components/ui/Pager.dom.test.tsx @@ -1,4 +1,5 @@ // @vitest-environment jsdom +import { useLayoutEffect } from "react"; import { describe, it, expect, vi, afterEach } from "vitest"; import { render, screen, fireEvent, cleanup } from "@testing-library/react"; @@ -32,3 +33,80 @@ describe("Pager · rows per page", () => { expect(onSizeChange.mock.calls).toEqual([[50], [0]]); }); }); + +// The page box DERIVES the current page (E27 S2): it holds a draft only while being edited, so the +// parent moving the page needs no re-sync effect. The layout-phase probe is what proves "derived" +// rather than "re-synced after paint" — a passive effect could not have run yet when it reads. +describe("Pager · the page box follows the page without an effect", () => { + function Probe({ page, painted }: { page: number; painted: string[] }) { + useLayoutEffect(() => { + painted.push( + (document.querySelector('input[type="number"]') as HTMLInputElement | null)?.value ?? "", + ); + }); + return ( + {}} + onSizeChange={() => {}} + sizeLabel="Rows per page" + /> + ); + } + + it("shows the new page in the first frame after the parent moves it", () => { + const painted: string[] = []; + const { rerender } = render(); + expect(screen.getByLabelText("datatable.pager.pageLabel")).toHaveProperty("value", "1"); + + painted.length = 0; + rerender(); + expect(painted[0]).toBe("5"); + }); + + it("keeps what you type until you commit it, then follows the page again", () => { + const onPageChange = vi.fn(); + render( + {}} + sizeLabel="Rows per page" + />, + ); + const box = screen.getByLabelText("datatable.pager.pageLabel"); + fireEvent.change(box, { target: { value: "7" } }); + expect(box).toHaveProperty("value", "7"); + fireEvent.keyDown(box, { key: "Enter" }); + expect(onPageChange).toHaveBeenCalledWith(6); + // The parent in this test never moves, so dropping the draft must put page 1 back — proof the + // box stopped holding its own copy rather than merely being overwritten. + expect(box).toHaveProperty("value", "1"); + }); + + it("discards an unparseable entry instead of committing it", () => { + const onPageChange = vi.fn(); + render( + {}} + sizeLabel="Rows per page" + />, + ); + const box = screen.getByLabelText("datatable.pager.pageLabel"); + fireEvent.change(box, { target: { value: "abc" } }); + fireEvent.blur(box); + expect(onPageChange).not.toHaveBeenCalled(); + expect(box).toHaveProperty("value", "3"); + }); +}); diff --git a/frontend/src/components/ui/Pager.tsx b/frontend/src/components/ui/Pager.tsx index c7bbaa47..2623ad1c 100644 --- a/frontend/src/components/ui/Pager.tsx +++ b/frontend/src/components/ui/Pager.tsx @@ -1,4 +1,4 @@ -import { useEffect, useState } from "react"; +import { useState } from "react"; import { useTranslation } from "react-i18next"; import { ChevronLeft, ChevronRight } from "lucide-react"; import { hasPagerContent, pageCount } from "./pagerContent"; @@ -34,9 +34,13 @@ export default function Pager({ const totalPages = pageCount(total, size); const safePage = Math.min(page, totalPages - 1); - // Local text for the editable "jump to page" box (committed on Enter/blur). - const [pageInput, setPageInput] = useState(String(safePage + 1)); - useEffect(() => setPageInput(String(safePage + 1)), [safePage]); + // Local text for the editable "jump to page" box (committed on Enter/blur). `null` means "not + // being edited", so the box simply DERIVES the current page instead of holding a copy that an + // effect has to re-sync after every page change — one render instead of two, and no window in + // which the box shows the page you just left. The draft is dropped on commit, which is the only + // place the two can disagree: clicking any other pager control blurs the input first. + const [draft, setDraft] = useState(null); + const pageInput = draft ?? String(safePage + 1); // Turning the page — or resizing it, which re-slices from a different first item — swaps the whole // list, so put the reader at its top; landing mid-list in a page you haven't seen is disorienting. @@ -51,8 +55,10 @@ export default function Pager({ } function commitPageInput() { const n = parseInt(pageInput, 10); + // Dropping the draft either way is what makes the box follow the real page again — on a valid + // jump because the page moves to it, on garbage because there is nothing to commit. + setDraft(null); if (!isNaN(n)) goToPage(Math.min(totalPages - 1, Math.max(0, n - 1))); - else setPageInput(String(safePage + 1)); } if (!hasPagerContent(total, size)) return null; @@ -75,7 +81,7 @@ export default function Pager({ min={1} max={totalPages} value={pageInput} - onChange={(e) => setPageInput(e.target.value)} + onChange={(e) => setDraft(e.target.value)} onBlur={commitPageInput} onKeyDown={(e) => e.key === "Enter" && commitPageInput()} aria-label={t("datatable.pager.pageLabel")} diff --git a/frontend/src/components/ui/Toaster.tsx b/frontend/src/components/ui/Toaster.tsx index 5f52dedf..263cbbf3 100644 --- a/frontend/src/components/ui/Toaster.tsx +++ b/frontend/src/components/ui/Toaster.tsx @@ -89,10 +89,12 @@ export default function Toaster({ // AFTER the snapshot above has read the outgoing host's screen — this host's own first render as // top would otherwise overwrite the handover with the toasts it is about to classify. Writing the // same value twice is harmless; that is the point of the ref. + // eslint-disable-next-line react-hooks/globals -- the handover between toast hosts is module-level BY DESIGN and must be written during render — see the 12 lines above, which record why a useMemo produced the wrong answer under StrictMode. The write is idempotent, which is what makes a repeated render invocation safe. if (isTop) lastTopIds = toasts.map((n) => n.id); const [armed, setArmed] = useState(false); useEffect(() => { if (!isTop) { + // eslint-disable-next-line react-hooks/set-state-in-effect -- arming is a TIMER, not derived state: the live region must be mounted and empty for a tick before content lands in it or a screen reader never announces the first toast. Deriving `isTop && armed` would let a host that regains top skip that tick. setArmed(false); return; } @@ -196,6 +198,10 @@ export default function Toaster({
{isTop && Docked && (
+ {/* lib/toastDock is a one-slot registry: this identity changes only when a surface docks + or undocks, and remounting is then the correct behaviour, not the hazard the rule + warns about. */} + {/* eslint-disable-next-line react-hooks/static-components */}
)} diff --git a/frontend/src/components/ui/VirtualGrid.tsx b/frontend/src/components/ui/VirtualGrid.tsx index 9180df48..98ff4a8e 100644 --- a/frontend/src/components/ui/VirtualGrid.tsx +++ b/frontend/src/components/ui/VirtualGrid.tsx @@ -127,6 +127,15 @@ export default function VirtualGrid({ const rows = useMemo(() => chunk(items, colCount), [items, colCount]); + // The React Compiler (E27 S1) SKIPS memoising this whole component, and says so: TanStack + // Virtual's `useVirtualizer()` hands back functions that cannot be memoised without going stale, + // so the compiler bails rather than risk a frozen viewport. That is the correct trade here — this + // component's cost is the row window it mounts, not its render closures — and it is the ONE such + // site in the app, which is why this is a targeted disable rather than the rule being turned off: + // a second incompatible library anywhere else must still stop the gate. The caution in the rule's + // own text applies, though: values taken from this API and handed to a memoised child can go + // stale, so keep the virtualizer's output inside this component. + // eslint-disable-next-line react-hooks/incompatible-library -- see above; @tanstack/react-virtual const virtualizer = useVirtualizer({ count: rows.length, getScrollElement: () => scrollEl, diff --git a/frontend/src/components/ui/anchoredPopoverHooks.ts b/frontend/src/components/ui/anchoredPopoverHooks.ts index f8dc7692..0b81da66 100644 --- a/frontend/src/components/ui/anchoredPopoverHooks.ts +++ b/frontend/src/components/ui/anchoredPopoverHooks.ts @@ -143,6 +143,7 @@ export function useAnchoredPopover(opts: AnchoredPopoverOptions) { // menu opens; clear it whenever the surface closes. useEffect(() => { if (!opts.open) { + // eslint-disable-next-line react-hooks/set-state-in-effect -- both indices are also floating-ui's list-navigation state (passed to useListNavigation below), so they cannot be derived from `open` without lying to the library about where the cursor is. Clearing on close is what stops a stale row stealing the next open's initial focus. setActiveIndex(null); setHoverIndex(null); } diff --git a/frontend/src/components/ui/sortableA11y.ts b/frontend/src/components/ui/sortableA11y.ts index cf8b817b..3c4dd5bc 100644 --- a/frontend/src/components/ui/sortableA11y.ts +++ b/frontend/src/components/ui/sortableA11y.ts @@ -61,6 +61,11 @@ export function useSortableA11y(describe: (id: UniqueIdentifier) => SortableSubj }, onDragOver: ({ active, over }) => { if (!over || (!moved && over.id === active.id)) return undefined; + // `moved` is per-DRAG state, and the announcement closures capturing it is the point: + // dnd-kit reports the item over ITSELF in the pick-up tick (see above), and this flag is + // the only thing separating that from a real move. A ref would be the wrong lifetime — + // one drag, not the component. + // eslint-disable-next-line react-hooks/immutability moved = true; return t("common.dnd.movedOver", landing(active.id, over.id)); }, diff --git a/frontend/src/components/usePlexPlaylistEdits.ts b/frontend/src/components/usePlexPlaylistEdits.ts index fe270cd4..809a29b8 100644 --- a/frontend/src/components/usePlexPlaylistEdits.ts +++ b/frontend/src/components/usePlexPlaylistEdits.ts @@ -20,6 +20,7 @@ export function usePlexPlaylistEdits(playlistId: number) { // Local optimistic copy so drag/remove are instant; re-synced whenever the query returns. const [items, setItems] = useState([]); useEffect(() => { + // eslint-disable-next-line react-hooks/set-state-in-effect -- the local copy is OPTIMISTIC — drag and remove write it before the server answers — so it cannot be derived from the query. This re-syncs it to the server's answer, and the rollback path below deliberately writes it too. if (q.data) setItems(q.data.items); }, [q.data]); diff --git a/frontend/src/i18n/locales/en/scheduler.json b/frontend/src/i18n/locales/en/scheduler.json index bb17b854..61f5e184 100644 --- a/frontend/src/i18n/locales/en/scheduler.json +++ b/frontend/src/i18n/locales/en/scheduler.json @@ -23,6 +23,7 @@ "quotaRemaining": "{{formatted}} left", "quotaNote": "Shared YouTube Data API budget; resets at midnight US Pacific. Backfill yields to leave headroom for interactive use.", "minutes": "min", + "range": "Must be between {{min}} and {{max}}.", "editInterval": "Click to change how often this runs", "intervalFailed": "Couldn't change the interval", "runNow": "Run now", diff --git a/frontend/src/i18n/locales/hu/scheduler.json b/frontend/src/i18n/locales/hu/scheduler.json index 9de4ee15..9b66c95e 100644 --- a/frontend/src/i18n/locales/hu/scheduler.json +++ b/frontend/src/i18n/locales/hu/scheduler.json @@ -23,6 +23,7 @@ "quotaRemaining": "{{formatted}} maradt", "quotaNote": "Közös YouTube Data API-keret; éjfélkor (US Pacific) nullázódik. A backfill hátrébb sorol, hogy maradjon hely az interaktív használatnak.", "minutes": "perc", + "range": "{{min}} és {{max}} között lehet.", "editInterval": "Kattints a gyakoriság módosításához", "intervalFailed": "Nem sikerült módosítani a gyakoriságot", "runNow": "Futtatás most", 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) ------------------------------- diff --git a/frontend/src/lib/playerModalHooks.ts b/frontend/src/lib/playerModalHooks.ts index b81e656c..164b7ccf 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,21 @@ 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. + // + // This body DOES capture render values — `resumeAt`, `setWatched`, `advanceOnEnd`, + // `reapplyStored`, `focusModal`, `qc` — and it must, because each of them is a PER-ITEM + // SNAPSHOT: they describe the video this player was built for, and re-reading them mid-session + // would be the bug, not the fix. `resumeAt` is the clearest case, since persist() writes + // sessionPosRef and so changes it while `activeId` has not moved; the player is already playing + // from it and must not be told again. What genuinely has to stay live mid-session is behind refs + // (currentIdRef, modeRef, navRef, sessionPosRef, autoMarkedRef, errorStreakRef, playerRef) — and + // THAT split is the enforcement: a value that is not a snapshot does not belong in this closure. + // + // 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 (