refactor(frontend): honest effect deps in the app shell and the ui layer (E27 S2)
Burns the first 14 of the 113 hook suppressions. Three verdicts, applied per site:
A useEffect that should not exist (6):
- Avatar remembered "the image failed" as a boolean an effect reset on src change. The
component is recycled across items in the virtualized feed, so after one avatar 429s the
next item's slot committed the fallback initial over a perfectly good image and only the
post-paint effect cleared it. The failure is now keyed BY URL; derived state cannot be stale.
- Combobox cleared the typed query from an effect on `open`. Moved into setOpenAnd, the single
funnel every close goes through. That also stops a spurious onQueryChange("") AT MOUNT, which
told a controlled caller its own initial query had been cleared before the user touched it.
- CommandPalette reset query + picked row from an effect. Closed now means UNMOUNTED, not
hidden, so a fresh open starts blank because mounting already means that -- and the ranking
memo over every channel, playlist and saved view stops running while the palette is shut.
- Pager's page box held a copy of the page that an effect re-synced; it now derives it and
keeps a draft only while being edited.
- App's ?link= / ?oauth= / ?admin= one-shots asserted "once per mount" with an empty dep list.
The ref guard the codebase already uses in 22 places ENFORCES it, so the honest deps can
include `t` -- which a language switch legitimately changes, and which would otherwise have
re-fired the toast.
Honest deps that also fix something (5):
- App's module preload was keyed on the account id, so a permission change that alters the
module set without moving the id never re-preloaded. `preload()` is idempotent, so depending
on `me` costs nothing.
- App's access-request nudge now re-issues on a language switch; it used to stay in the old
language until the pending count changed.
- App's diagnostics effect: applyDiagnostics already no-ops on an unchanged effective level,
so picking four fields out by hand was guarding nothing and missed any field added later.
- DataTable's two filter effects: a ref holds the latest onFiltersChange so commitFilters can
be stable, which is what lets the effects name it instead of switching the check off.
Genuine escape hatches (7) moved OUT of eslint-suppressions.json into inline directives that
name the invariant at the site -- the toast handover's render-phase global, the live region's
deliberate arming tick, the docked one-slot component, the popover engine's floating-ui list
state, dnd-kit's per-drag `moved` flag, BackToTop's DOM read and the palette's debounced
announcement. A reader of the code now sees why; a JSON file no one opens does not.
Tests: Avatar (3, new file) and Pager (3) pin the derived-state frames with a layout-phase
probe -- layout effects run before any passive effect, so they read the frame the browser would
paint. Combobox (2) pins the close-clears-query contract and the no-callback-at-mount rule.
All three sets mutation-checked against the effect-based originals.
Suppressions 113 -> 99; exhaustive-deps 45 -> 35.
This commit is contained in:
@@ -179,52 +179,6 @@
|
||||
"count": 1
|
||||
}
|
||||
},
|
||||
"src/components/ui/Avatar.tsx": {
|
||||
"react-hooks/set-state-in-effect": {
|
||||
"count": 1
|
||||
}
|
||||
},
|
||||
"src/components/ui/BackToTop.tsx": {
|
||||
"react-hooks/set-state-in-effect": {
|
||||
"count": 1
|
||||
}
|
||||
},
|
||||
"src/components/ui/Combobox.tsx": {
|
||||
"react-hooks/set-state-in-effect": {
|
||||
"count": 1
|
||||
}
|
||||
},
|
||||
"src/components/ui/CommandPalette.tsx": {
|
||||
"react-hooks/set-state-in-effect": {
|
||||
"count": 2
|
||||
}
|
||||
},
|
||||
"src/components/ui/Pager.tsx": {
|
||||
"react-hooks/set-state-in-effect": {
|
||||
"count": 1
|
||||
}
|
||||
},
|
||||
"src/components/ui/Toaster.tsx": {
|
||||
"react-hooks/globals": {
|
||||
"count": 1
|
||||
},
|
||||
"react-hooks/set-state-in-effect": {
|
||||
"count": 1
|
||||
},
|
||||
"react-hooks/static-components": {
|
||||
"count": 1
|
||||
}
|
||||
},
|
||||
"src/components/ui/anchoredPopoverHooks.ts": {
|
||||
"react-hooks/set-state-in-effect": {
|
||||
"count": 1
|
||||
}
|
||||
},
|
||||
"src/components/ui/sortableA11y.ts": {
|
||||
"react-hooks/immutability": {
|
||||
"count": 1
|
||||
}
|
||||
},
|
||||
"src/components/usePlexPlaylistEdits.ts": {
|
||||
"react-hooks/set-state-in-effect": {
|
||||
"count": 1
|
||||
|
||||
+70
-32
@@ -1,4 +1,4 @@
|
||||
import { lazy, Suspense, useEffect, useState } from "react";
|
||||
import { lazy, Suspense, useEffect, useRef, useState } from "react";
|
||||
import { qk } from "./lib/queryKeys";
|
||||
import { useTranslation } from "react-i18next";
|
||||
import { useQuery, useQueryClient } from "@tanstack/react-query";
|
||||
@@ -131,6 +131,9 @@ export default function App() {
|
||||
});
|
||||
const needsSetup = setupQuery.data?.configured === false;
|
||||
const meQuery = useQuery({ queryKey: qk.me(), queryFn: api.me, enabled: false });
|
||||
// The signed-in account, or null/undefined while signed out. Hoisted here because half a dozen
|
||||
// effects below depend on it, and a dep list is only honest if it names the value the body reads.
|
||||
const me = meQuery.data;
|
||||
|
||||
// The `global` key scope: `h` opens the cheat sheet, Mod+K the jump-to palette, Shift+W jumps to
|
||||
// Home ("mit nézzek?"). Bound only once signed in, because that is where the overlays are mounted
|
||||
@@ -141,32 +144,40 @@ export default function App() {
|
||||
// page underneath (owner decision 2026-09-09: the key always ends with the answer on screen).
|
||||
// The `page` scope rides the same binding: the `g` chords jump to a module, but only to one this
|
||||
// account can reach (the nav rail's own list, plus Settings, which is not in the rail's order).
|
||||
useGlobalKeymap(
|
||||
!!meQuery.data,
|
||||
setPage,
|
||||
meQuery.data ? [...moduleOrder(meQuery.data), "settings"] : [],
|
||||
);
|
||||
useGlobalKeymap(!!me, setPage, me ? [...moduleOrder(me), "settings"] : []);
|
||||
|
||||
// The account's consented diagnostics level (E22 S5), applied to this tab's log: raised, the log is
|
||||
// kept in more detail and streamed to the server until the level lapses. A debug session that is
|
||||
// recording (E16) raises it for its own run and tags what it keeps. A lapse (its own timer, or the
|
||||
// server saying it is off) refetches `me`, so Settings and the recorder show what is in force.
|
||||
const diagnostics = meQuery.data?.diagnostics;
|
||||
const debugSession = meQuery.data?.debug_session;
|
||||
// Re-running this on every `me` refetch is FREE and therefore the deps are the honest ones:
|
||||
// applyDiagnostics computes an effective-level key and returns early when it has not moved
|
||||
// (lib/diagnostics.ts), so an unchanged level is a no-op. It does refresh the `lapsed` callback
|
||||
// before that early return, which is the half we WANT re-run. The old dep list picked out four
|
||||
// primitive fields to dodge the fresh object identity a refetch produces; that is the checker's
|
||||
// job to verify, not a comment's to assert, and it silently missed any other field the module
|
||||
// may come to read.
|
||||
const diagnostics = me?.diagnostics;
|
||||
const debugSession = me?.debug_session;
|
||||
useEffect(() => {
|
||||
applyDiagnostics(
|
||||
diagnostics,
|
||||
debugSession,
|
||||
() => void queryClient.invalidateQueries({ queryKey: qk.me() }),
|
||||
);
|
||||
}, [diagnostics?.level, diagnostics?.until, debugSession?.id, debugSession?.ended_at]); // eslint-disable-line react-hooks/exhaustive-deps
|
||||
}, [diagnostics, debugSession, queryClient]);
|
||||
|
||||
// Fetch the code of every module this account can open — and the channel page — once the first page
|
||||
// is on screen, so a module's first visit renders at once instead of flashing the chunk fallback.
|
||||
// Idle-scheduled so it never competes with the first page's own requests (Safari has no
|
||||
// requestIdleCallback, hence the timer).
|
||||
// Depending on the whole `me` rather than its id is both honest and MORE correct: the module set
|
||||
// is derived from `me` through each module's own gate (lib/modules moduleOrder), so a permission
|
||||
// change — an admin grant, a Plex server appearing — alters it without the id moving, and the old
|
||||
// `[me.id]` dep never re-preloaded for those. Re-running costs nothing: `preload()` is idempotent
|
||||
// (lazyWithPreload memoises the pending promise), so a refetch that changes nothing fetches
|
||||
// nothing; only the idle callback is re-scheduled.
|
||||
useEffect(() => {
|
||||
const me = meQuery.data;
|
||||
if (!me) return;
|
||||
const run = () => {
|
||||
preloadPages([...moduleOrder(me), "settings"]);
|
||||
@@ -179,7 +190,7 @@ export default function App() {
|
||||
}
|
||||
const id = window.setTimeout(run, 1000);
|
||||
return () => window.clearTimeout(id);
|
||||
}, [meQuery.data?.id]); // eslint-disable-line react-hooks/exhaustive-deps
|
||||
}, [me]);
|
||||
|
||||
// Measure the scrollbar's gutter width once and publish it as --sbw, so the floating header and the
|
||||
// toolbar band can pad their right edge to line up with the scrolled cards (the app scroller reserves
|
||||
@@ -197,9 +208,13 @@ export default function App() {
|
||||
// Once signed in, drop any leftover ONE-TIME query params from the pre-login flow (?access, ?verify,
|
||||
// ?reset) — but NOT the nav/filter params, which the logged-in app now reads from and keeps in the
|
||||
// URL. Gated on auth so the logged-out Welcome page still reads its params first.
|
||||
// Keyed on "is there a session at all", which is the whole condition the body tests. Re-running
|
||||
// on an account switch was never the point — by then the params are already gone and a second
|
||||
// clearParams is a no-op.
|
||||
const signedIn = !!me;
|
||||
useEffect(() => {
|
||||
if (meQuery.data) clearParams("access", "verify", "reset");
|
||||
}, [meQuery.data?.id]); // eslint-disable-line react-hooks/exhaustive-deps
|
||||
if (signedIn) clearParams("access", "verify", "reset");
|
||||
}, [signedIn]);
|
||||
|
||||
// Any request that 401s means the session ended server-side. If we were signed in (cached
|
||||
// `me` is a user object), reload to the login page at once (also covers any click that hits
|
||||
@@ -218,12 +233,20 @@ export default function App() {
|
||||
// Returning from an in-app Google link/upgrade round-trip (auth.py redirects to ?link=…).
|
||||
// Surface the outcome, refresh `me` (can_read/can_write/has_google may have flipped), land on
|
||||
// Settings → Account so the new state is visible, then strip the param for a clean URL.
|
||||
// "Exactly once per mount" is now ENFORCED by the ref rather than asserted by an empty dep list:
|
||||
// `t` changes identity on a language switch, so the honest dep list would otherwise re-fire the
|
||||
// toast when the user changes language. (The old `[]` got away with it only because clearParams
|
||||
// had already removed the param — an invariant held by a side effect two lines further down.)
|
||||
const linkAnswered = useRef(false);
|
||||
const { refetch: refetchMe } = meQuery;
|
||||
useEffect(() => {
|
||||
if (linkAnswered.current) return;
|
||||
const result = new URLSearchParams(window.location.search).get("link");
|
||||
if (!result) return;
|
||||
linkAnswered.current = true;
|
||||
if (result === "ok") {
|
||||
notify({ level: "success", message: t("settings.account.googleLink.linked") });
|
||||
void meQuery.refetch();
|
||||
void refetchMe();
|
||||
setAccountRaw(LS.settingsTab, "account");
|
||||
setPage("settings");
|
||||
} else {
|
||||
@@ -231,42 +254,59 @@ export default function App() {
|
||||
notify({ level: "error", message: t(`settings.account.googleLink.${key}`) });
|
||||
}
|
||||
clearParams("link");
|
||||
}, []); // eslint-disable-line react-hooks/exhaustive-deps
|
||||
}, [t, setPage, refetchMe]);
|
||||
|
||||
// A Google OAuth flow started while already signed in (add-another-account, link, or upgrade) was
|
||||
// cancelled at Google's consent screen. The backend only emits this for a live session, so a
|
||||
// neutral toast is the right feedback — the pre-auth login banner would never render here.
|
||||
// Same one-shot shape as the `?link=` handler above, same reason for the ref.
|
||||
const oauthAnswered = useRef(false);
|
||||
useEffect(() => {
|
||||
if (oauthAnswered.current) return;
|
||||
if (new URLSearchParams(window.location.search).get("oauth") !== "cancelled") return;
|
||||
oauthAnswered.current = true;
|
||||
notify({ level: "info", message: t("settings.account.googleCancelled") });
|
||||
clearParams("oauth");
|
||||
}, []); // eslint-disable-line react-hooks/exhaustive-deps
|
||||
}, [t]);
|
||||
|
||||
// Deep-link from the "new access request" admin email (?admin=access-requests) → jump straight to
|
||||
// the admin Users → Access requests view. Snapshot the param up front (read once at mount); act once
|
||||
// `me` is known, and only for an admin (others just land on their default page).
|
||||
const [adminDeepLink] = useState(() => new URLSearchParams(window.location.search).get("admin"));
|
||||
// Ref-guarded like the two above, and here it closes a real hole rather than only satisfying the
|
||||
// checker: the snapshot lives for the whole session, so with honest deps a later role change —
|
||||
// or any `me` refetch, under the old id-only key — would yank the user to Users long after the
|
||||
// deep link was answered.
|
||||
const adminDeepLinkAnswered = useRef(false);
|
||||
useEffect(() => {
|
||||
if (adminDeepLink !== "access-requests" || !meQuery.data) return;
|
||||
if (meQuery.data.role === "admin") {
|
||||
if (adminDeepLinkAnswered.current) return;
|
||||
if (adminDeepLink !== "access-requests" || !me) return;
|
||||
adminDeepLinkAnswered.current = true;
|
||||
if (me.role === "admin") {
|
||||
focusAccessRequestsTab();
|
||||
setPage("users");
|
||||
}
|
||||
clearParams("admin");
|
||||
}, [adminDeepLink, meQuery.data?.id]); // eslint-disable-line react-hooks/exhaustive-deps
|
||||
}, [adminDeepLink, me, setPage]);
|
||||
|
||||
// Nudge admins when access requests are waiting (in lieu of a server-push bell). Keyed on the
|
||||
// pending COUNT so it re-resolves the moment an approval/denial changes it — not only on a
|
||||
// fresh load (which is why the notice used to linger until F5 after an approval).
|
||||
// Honest deps, and `t` among them is a fix rather than a concession: the nudge is a persisted
|
||||
// toast whose text is translated at issue time, so a language switch must re-issue it. It did not
|
||||
// before — the notice stayed in the old language until the count changed.
|
||||
const role = me?.role;
|
||||
const pendingInvites = me?.pending_invites ?? 0;
|
||||
const pendingActivations = me?.pending_activations ?? 0;
|
||||
useEffect(() => {
|
||||
if (meQuery.data?.role !== "admin") return;
|
||||
if (role !== "admin") return;
|
||||
// Keep a single, current nudge: drop any prior one (persisted copies reload as dismissed
|
||||
// history, so they'd otherwise pile up one-per-reload) before re-issuing the live notice.
|
||||
removeByMetaKind("access-requests");
|
||||
// Both kinds of "someone is waiting on you": a request needing a decision, and a sign-up whose
|
||||
// address was approved before they registered. The second used to produce NO signal at all —
|
||||
// nothing lands in the queue for it — which is how the 2026-09-03 registrant went unnoticed.
|
||||
const pending = (meQuery.data.pending_invites ?? 0) + (meQuery.data.pending_activations ?? 0);
|
||||
const pending = pendingInvites + pendingActivations;
|
||||
if (pending > 0) {
|
||||
notify({
|
||||
level: "warning",
|
||||
@@ -285,8 +325,7 @@ export default function App() {
|
||||
},
|
||||
});
|
||||
}
|
||||
// eslint-disable-next-line react-hooks/exhaustive-deps
|
||||
}, [meQuery.data?.role, meQuery.data?.pending_invites, meQuery.data?.pending_activations]);
|
||||
}, [role, pendingInvites, pendingActivations, t, setPage]);
|
||||
|
||||
// Language is edited outside the Settings page (sidebar/login), so it stays instant.
|
||||
function changeLanguage(code: LangCode) {
|
||||
@@ -302,7 +341,7 @@ export default function App() {
|
||||
if (needsSetup) return <SetupWizard />;
|
||||
// /api/me answers 200 always (null body = logged out), and BootGate has already handled a failed
|
||||
// probe — so no data here means "not signed in" → the public landing.
|
||||
if (!meQuery.data) return <Welcome verify={verifyEntry} />;
|
||||
if (!me) return <Welcome verify={verifyEntry} />;
|
||||
|
||||
// Shown while a page's chunk is still on its way: only if its visit beats the preload above (or the
|
||||
// preload failed), and like every loading state it stays invisible for a load that is quick anyway.
|
||||
@@ -323,16 +362,15 @@ export default function App() {
|
||||
// page the account can't reach (a gated module via stale nav / a deep link) falls back to the feed,
|
||||
// exactly as the old ternary's fallthrough did. A channel page overlays the content column, so no
|
||||
// side rail shows behind it.
|
||||
const me = meQuery.data!;
|
||||
const pageGate = moduleDef(page).gate;
|
||||
const activePage: Page = !pageGate || pageGate(me) ? page : "feed";
|
||||
const activePage: Page = !pageGate || pageGate(me!) ? page : "feed";
|
||||
const { Component: PageContent, maxWidth } = PAGE_CONTENT[activePage];
|
||||
const railDef = channelView ? undefined : PAGE_RAIL[activePage];
|
||||
|
||||
return (
|
||||
<div className="h-screen flex text-fg">
|
||||
<NavSidebar
|
||||
me={meQuery.data!}
|
||||
me={me!}
|
||||
onOpenAbout={() => setAboutOpen(true)}
|
||||
onChangeLanguage={changeLanguage}
|
||||
language={i18n.language as LangCode}
|
||||
@@ -368,7 +406,7 @@ export default function App() {
|
||||
key={channelView.id}
|
||||
channelId={channelView.id}
|
||||
initialName={channelView.name}
|
||||
me={meQuery.data!}
|
||||
me={me!}
|
||||
view={view}
|
||||
setView={setView}
|
||||
onShowInFeed={() => {
|
||||
@@ -393,11 +431,11 @@ export default function App() {
|
||||
<PageShell
|
||||
scrollKey={normalScrollKey}
|
||||
maxWidth={maxWidth}
|
||||
header={<Header me={meQuery.data!} onYtSearch={enterYtSearch} />}
|
||||
header={<Header me={me!} onYtSearch={enterYtSearch} />}
|
||||
banners={
|
||||
<>
|
||||
<VerifyBanner entry={verifyEntry} />
|
||||
{meQuery.data!.is_demo && <DemoBanner />}
|
||||
{me!.is_demo && <DemoBanner />}
|
||||
<VersionBanner onOpen={() => openReleaseNotes(FRONTEND_VERSION)} />
|
||||
</>
|
||||
}
|
||||
@@ -417,9 +455,9 @@ export default function App() {
|
||||
"take me to X" the same way. Their keys are bound by useGlobalKeymap above. */}
|
||||
<KeyboardHelp />
|
||||
<AppCommandPalette />
|
||||
{meQuery.data && !meQuery.data.is_demo && <ChatDock meId={meQuery.data.id} />}
|
||||
{me && !me.is_demo && <ChatDock meId={me.id} />}
|
||||
{/* The debug recorder (E16): its start dialog, and its pill docked under the toasts. */}
|
||||
{meQuery.data && !meQuery.data.is_demo && <DebugRecorder />}
|
||||
{me && !me.is_demo && <DebugRecorder />}
|
||||
<Suspense fallback={null}>
|
||||
{aboutOpen && (
|
||||
<About
|
||||
|
||||
@@ -0,0 +1,64 @@
|
||||
// @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 <Avatar src={src} alt="avatar" fallback="Zed" />;
|
||||
}
|
||||
|
||||
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(<Probe src="https://cdn.test/a.jpg" painted={painted} />);
|
||||
|
||||
fireEvent.error(screen.getByRole("img", { name: "avatar" }));
|
||||
expect(screen.queryByRole("img")).toBeNull();
|
||||
expect(screen.getByText("Z")).toBeTruthy();
|
||||
|
||||
painted.length = 0;
|
||||
rerender(<Probe src="https://cdn.test/b.jpg" painted={painted} />);
|
||||
|
||||
// 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<");
|
||||
});
|
||||
|
||||
it("keeps the fallback for the URL that actually failed, not for whatever is current", () => {
|
||||
const { rerender } = render(
|
||||
<Avatar src="https://cdn.test/a.jpg" alt="avatar" fallback="Zed" />,
|
||||
);
|
||||
fireEvent.error(screen.getByRole("img", { name: "avatar" }));
|
||||
expect(screen.getByText("Z")).toBeTruthy();
|
||||
|
||||
rerender(<Avatar src="https://cdn.test/b.jpg" alt="avatar" fallback="Zed" />);
|
||||
expect(screen.getByRole("img", { name: "avatar" }).getAttribute("src")).toBe(
|
||||
"https://cdn.test/b.jpg",
|
||||
);
|
||||
});
|
||||
|
||||
it("falls back when there is no src at all", () => {
|
||||
render(<Avatar src={null} alt="avatar" fallback="Zed" />);
|
||||
expect(screen.queryByRole("img")).toBeNull();
|
||||
expect(screen.getByText("Z")).toBeTruthy();
|
||||
});
|
||||
});
|
||||
@@ -1,4 +1,4 @@
|
||||
import { useEffect, useState } from "react";
|
||||
import { useState } from "react";
|
||||
|
||||
// Avatar image with a graceful fallback.
|
||||
//
|
||||
@@ -20,8 +20,13 @@ 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.
|
||||
const [failedSrc, setFailedSrc] = useState<string | null>(null);
|
||||
const failed = !!src && failedSrc === src;
|
||||
|
||||
if (src && !failed) {
|
||||
return (
|
||||
@@ -30,7 +35,7 @@ export default function Avatar({
|
||||
alt={alt}
|
||||
loading="lazy"
|
||||
referrerPolicy="no-referrer"
|
||||
onError={() => setFailed(true)}
|
||||
onError={() => setFailedSrc(src)}
|
||||
className={className}
|
||||
/>
|
||||
);
|
||||
|
||||
@@ -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 — <main> 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]);
|
||||
|
||||
|
||||
@@ -246,6 +246,54 @@ 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, and it happens IN the close — the clear used
|
||||
// to be an effect on `open`, which fired a render later and, in controlled mode, reported the
|
||||
// empty query outside any event (E27 S2). The list is for "find and pick": 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(<Combobox label="Genre" options={OPTIONS} selected={[]} onChange={vi.fn()} />);
|
||||
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(
|
||||
<Combobox
|
||||
label="Collection"
|
||||
multiple={false}
|
||||
options={OPTIONS}
|
||||
selected={[]}
|
||||
onChange={vi.fn()}
|
||||
query="com"
|
||||
onQueryChange={onQueryChange}
|
||||
onOpenChange={onOpenChange}
|
||||
clientFilter={false}
|
||||
/>,
|
||||
);
|
||||
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
|
||||
|
||||
@@ -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();
|
||||
|
||||
@@ -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 <CommandPaletteBody onClose={onClose} groups={groups} />;
|
||||
}
|
||||
|
||||
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<HTMLButtonElement | null>(null);
|
||||
const listRef = useRef<HTMLDivElement | null>(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
|
||||
|
||||
@@ -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<T>({
|
||||
// `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<T>({
|
||||
);
|
||||
}, [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<T>({
|
||||
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);
|
||||
|
||||
|
||||
@@ -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 (
|
||||
<Pager
|
||||
page={page}
|
||||
size={10}
|
||||
total={100}
|
||||
pageSizeOptions={[10, 25]}
|
||||
onPageChange={() => {}}
|
||||
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(<Probe page={0} painted={painted} />);
|
||||
expect(screen.getByLabelText("datatable.pager.pageLabel")).toHaveProperty("value", "1");
|
||||
|
||||
painted.length = 0;
|
||||
rerender(<Probe page={4} painted={painted} />);
|
||||
expect(painted[0]).toBe("5");
|
||||
});
|
||||
|
||||
it("keeps what you type until you commit it, then follows the page again", () => {
|
||||
const onPageChange = vi.fn();
|
||||
render(
|
||||
<Pager
|
||||
page={0}
|
||||
size={10}
|
||||
total={100}
|
||||
pageSizeOptions={[10, 25]}
|
||||
onPageChange={onPageChange}
|
||||
onSizeChange={() => {}}
|
||||
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(
|
||||
<Pager
|
||||
page={2}
|
||||
size={10}
|
||||
total={100}
|
||||
pageSizeOptions={[10, 25]}
|
||||
onPageChange={onPageChange}
|
||||
onSizeChange={() => {}}
|
||||
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");
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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<string | null>(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")}
|
||||
|
||||
@@ -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({
|
||||
</div>
|
||||
{isTop && Docked && (
|
||||
<div className="pointer-events-auto w-max max-w-full">
|
||||
{/* 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 */}
|
||||
<Docked />
|
||||
</div>
|
||||
)}
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
|
||||
@@ -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));
|
||||
},
|
||||
|
||||
Reference in New Issue
Block a user