From 6fda49369f8a74d3cfc7c898ac3689c1846f768a Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sat, 19 Sep 2026 07:51:33 +0200 Subject: [PATCH] fix(ui): a sticky table header turns the scroller's top fade off itself; DataTable renders one layout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - "A page with a sticky ColumnHead must declare fadeTop: false" lived in prose: a registry flag on three pages plus an App special case for the Channels card layout, and the next table page would have re-acquired the fade-on-the-header defect with every lane green. ColumnHead now registers itself with the enclosing PageScroller (ui/stickyTop) while mounted; the registry flag, the App branch and the prop plumbing are gone. Measured on localdev: the top mask is 0px under Channels' and Audit's headers, 20px on Home and the Feed. - ui/DataTable mounted its table AND its card list at every width, one hidden by CSS: double DOM and render cost, duplicate ids, two keyboard copies of every row. It now mounts the one the `md` media query calls for (lib/useMediaQuery); the keyboard test asserts one layout at each width. - The Plex module's content width went px → rem with the rest of the page. --- frontend/src/App.tsx | 11 +- frontend/src/components/moduleRegistry.tsx | 11 +- .../ui/DataTable.keyboard.dom.test.tsx | 47 ++++-- frontend/src/components/ui/DataTable.tsx | 141 +++++++++--------- frontend/src/components/ui/PageScroller.tsx | 17 ++- frontend/src/components/ui/PageShell.tsx | 7 +- .../src/components/ui/stickyTop.dom.test.tsx | 62 ++++++++ frontend/src/components/ui/stickyTop.ts | 17 +++ frontend/src/components/ui/tableHeader.tsx | 4 +- frontend/src/lib/useMediaQuery.ts | 18 +++ 10 files changed, 225 insertions(+), 110 deletions(-) create mode 100644 frontend/src/components/ui/stickyTop.dom.test.tsx create mode 100644 frontend/src/components/ui/stickyTop.ts create mode 100644 frontend/src/lib/useMediaQuery.ts diff --git a/frontend/src/App.tsx b/frontend/src/App.tsx index 8dec8954..49853ba7 100644 --- a/frontend/src/App.tsx +++ b/frontend/src/App.tsx @@ -20,7 +20,6 @@ import { usePref } from "./lib/usePref"; import { useVerifyEntry } from "./lib/useVerifyEntry"; import { useNavigation, useNavigationActions } from "./components/navigationContext"; import { useFeedFilters, useFeedFiltersActions } from "./components/feedFiltersContext"; -import { useChannelLayout } from "./components/channelsContext"; import { useFeedView, useFeedViewActions } from "./components/feedViewContext"; import { moduleDef, moduleOrder } from "./lib/modules"; import { PAGE_CONTENT, PAGE_RAIL, preloadPages } from "./components/moduleRegistry"; @@ -71,11 +70,6 @@ export default function App() { // handlers that mutate the feed view (onShowInFeed, tag-click, the feed scrollKey). const filters = useFeedFilters(); const { setFilters } = useFeedFiltersActions(); - // The channel manager turns its top fade OFF for the table (its sticky header sits at the scroller - // top) but wants it ON for the card layout (no sticky header) — so the fade depends on the layout. - // Deliberately the NARROW layout context: subscribing to the manager's whole state here would - // re-render the app root on every keystroke in its search box. - const channelLayout = useChannelLayout(); // The feed's view mode (card/row/tile density) lives in FeedViewProvider — server-synced, shared // by the feed page and the channel page. App reads it only to hand to /. const view = useFeedView(); @@ -325,7 +319,7 @@ export default function App() { const me = meQuery.data!; const pageGate = moduleDef(page).gate; const activePage: Page = !pageGate || pageGate(me) ? page : "feed"; - const { Component: PageContent, fadeTop, maxWidth } = PAGE_CONTENT[activePage]; + const { Component: PageContent, maxWidth } = PAGE_CONTENT[activePage]; const railDef = channelView ? undefined : PAGE_RAIL[activePage]; return ( @@ -392,9 +386,6 @@ export default function App() { } banners={ <> diff --git a/frontend/src/components/moduleRegistry.tsx b/frontend/src/components/moduleRegistry.tsx index 464dbba3..4b424090 100644 --- a/frontend/src/components/moduleRegistry.tsx +++ b/frontend/src/components/moduleRegistry.tsx @@ -39,9 +39,6 @@ export type PageDef = { // Prop-free by construction: reads `me` + module state from hooks/providers. Preloadable by type, so // a new page cannot silently opt out of preloadPages. Component: PreloadableComponent; - // Channels + Audit render a table with a sticky header at the scroller top; the shell's top fade - // would sit on it and flip on with the first scroll, so they turn it off. Defaults to on. - fadeTop?: boolean; // The content column's max width. The header + toolbar centre on this same width (via --content-mw) // so the floating chrome lines up with the centred content instead of spanning the whole column. // MUST match the max-w the page's own content wrapper uses. Omit for a full-width page (feed). @@ -53,9 +50,9 @@ export const PAGE_CONTENT: Record = { home: { Component: Home, maxWidth: "80rem" }, // The two wrappers preload the lazy chunk they render. feed: { Component: Object.assign(FeedPage, { preload: Feed.preload }) }, - channels: { Component: Channels, fadeTop: false, maxWidth: "96rem" }, - playlists: { Component: Playlists, fadeTop: false, maxWidth: "64rem" }, - plex: { Component: PlexBrowse, maxWidth: "1600px" }, + channels: { Component: Channels, maxWidth: "96rem" }, + playlists: { Component: Playlists, maxWidth: "64rem" }, + plex: { Component: PlexBrowse, maxWidth: "100rem" }, notifications: { Component: NotificationsPanel, maxWidth: "48rem" }, messages: { Component: Object.assign(MessagesPage, { preload: Messages.preload }) }, downloads: { Component: DownloadCenter, maxWidth: "56rem" }, @@ -63,7 +60,7 @@ export const PAGE_CONTENT: Record = { scheduler: { Component: Scheduler }, config: { Component: ConfigPanel }, users: { Component: AdminUsers, maxWidth: "48rem" }, - audit: { Component: AuditLog, fadeTop: false }, + audit: { Component: AuditLog }, logs: { Component: AdminLogs }, settings: { Component: SettingsPanel }, }; diff --git a/frontend/src/components/ui/DataTable.keyboard.dom.test.tsx b/frontend/src/components/ui/DataTable.keyboard.dom.test.tsx index 9e540c45..643f3a11 100644 --- a/frontend/src/components/ui/DataTable.keyboard.dom.test.tsx +++ b/frontend/src/components/ui/DataTable.keyboard.dom.test.tsx @@ -1,9 +1,9 @@ // @vitest-environment jsdom // // DataTable's `keyboardNav` (E11 S4c): the rows are a keyboard list — `j`/`k` walk them and Enter -// clicks the element a column marks `data-nav-open`. The table renders its rows twice (a table and, -// below `md`, a card list); only the copy on screen may be walked, or `j` past the last row would carry -// on into the hidden one. +// clicks the element a column marks `data-nav-open`. It used to render its rows twice (a table and, +// below `md`, a card list, one hidden by CSS), so `j` past the last row could carry on into the hidden +// copy. Now it mounts exactly one layout, chosen by the `md` media query. import { afterEach, describe, expect, it, vi } from "vitest"; import { act, cleanup, render } from "@testing-library/react"; import DataTable, { type Column } from "./DataTable"; @@ -13,7 +13,10 @@ vi.mock("react-i18next", async (importOriginal) => { return { ...actual, useTranslation: () => ({ t: (key: string) => key }) }; }); -afterEach(cleanup); +afterEach(() => { + cleanup(); + vi.unstubAllGlobals(); +}); type Row = { id: string }; const rows: Row[] = [{ id: "a" }, { id: "b" }]; @@ -45,9 +48,15 @@ function press(key: string) { }); } -/** What a display:none copy answers in a browser; jsdom lays nothing out, so every copy is "visible". */ -const offScreen = (el: Element | null) => - Object.defineProperty(el!, "checkVisibility", { value: () => false, configurable: true }); +/** The viewport `md` question, answered as asked (jsdom has no matchMedia of its own). */ +function viewport(wide: boolean) { + vi.stubGlobal("matchMedia", (query: string) => ({ + matches: query === "(min-width: 48rem)" ? wide : false, + media: query, + addEventListener: () => {}, + removeEventListener: () => {}, + })); +} /** The focused row: which copy (TR / DIV) and which row. */ const focused = () => { @@ -58,26 +67,38 @@ const focused = () => { describe("DataTable · keyboardNav", () => { it("walks the table's rows, stops at the last one, and opens a row on Enter", () => { const opened: string[] = []; - const { container } = table((id) => opened.push(id)); - offScreen(container.querySelector(".md\\:hidden")); + viewport(true); + table((id) => opened.push(id)); press("j"); expect(focused()).toBe("TR a"); press("j"); expect(focused()).toBe("TR b"); - // The hidden card copy follows the table in the DOM — it must not be the next stop. + // The last row is the last stop — there is no second copy after it to walk into. press("j"); expect(focused()).toBe("TR b"); press("Enter"); expect(opened).toEqual(["b"]); }); - it("walks the card copy when that is the one on screen", () => { - const { container } = table(() => {}); - offScreen(container.querySelector("tbody")); + it("walks the card list when the viewport is narrow", () => { + viewport(false); + table(() => {}); press("j"); expect(focused()).toBe("DIV a"); }); + it("mounts one layout, never both", () => { + viewport(true); + const wide = table(() => {}); + expect(wide.container.querySelectorAll("[data-nav-index]")).toHaveLength(2); + expect(wide.container.querySelector("table")).not.toBeNull(); + cleanup(); + viewport(false); + const narrow = table(() => {}); + expect(narrow.container.querySelectorAll("[data-nav-index]")).toHaveLength(2); + expect(narrow.container.querySelector("table")).toBeNull(); + }); + it("leaves the rows out of the keyboard without keyboardNav", () => { const { container } = table(() => {}, false); expect(container.querySelector("[data-nav-index]")).toBeNull(); diff --git a/frontend/src/components/ui/DataTable.tsx b/frontend/src/components/ui/DataTable.tsx index 98273168..910b02e8 100644 --- a/frontend/src/components/ui/DataTable.tsx +++ b/frontend/src/components/ui/DataTable.tsx @@ -8,6 +8,7 @@ import { ColumnHead } from "./tableHeader"; import { alignClass } from "./tableAlign"; import { filterRows, type Column, type FilterMap } from "../../lib/tableColumns"; import { navItemAttrs, useListNav } from "../../lib/listNav"; +import { useMediaQuery } from "../../lib/useMediaQuery"; // A reusable, client-side data table: per-column sort + filter (in the header) + pagination, // with its sort/filter/page state optionally persisted to localStorage so a reload (F5) keeps @@ -134,6 +135,7 @@ export default function DataTable({ const filters = controlledFilters ? (filtersProp ?? EMPTY_FILTERS) : internalFilters; const [page, setPage] = useState(initial.page); const [size, setSize] = useState(initial.size ?? pageSize); + const wide = useMediaQuery("(min-width: 48rem)"); const bodyRef = useRef(null); const cardsRef = useRef(null); useListNav(bodyRef, { enabled: keyboardNav }); @@ -249,83 +251,88 @@ export default function DataTable({ return (
{(controlsPosition === "top" || controlsPosition === "both") && topControls} - {/* Wide screens: table. The wrapper isn't clipped so header filter popovers can overflow. */} -
- - {/* In fixed-layout mode a colgroup drives the column widths: those with an explicit + {/* ONE layout, chosen by width — not both mounted with CSS hiding one: that doubled every + row's DOM and render cost, duplicated ids and test hooks, and handed keyboard nav and any + DOM query two copies of each row. `md` (48rem) is the breakpoint the CSS used. + Wide screens: table. The wrapper isn't clipped so header filter popovers can overflow. */} + {wide ? ( +
+
+ {/* In fixed-layout mode a colgroup drives the column widths: those with an explicit `width` keep it, and the unspecified (fluid) columns split the remainder — so the table never exceeds its container and the fluid cells truncate instead. */} - {fixedLayout && ( - - {columns.map((col) => ( - - ))} - - )} - - - {paged.map((row, i) => ( - + {fixedLayout && ( + {columns.map((col) => ( - + ))} - - ))} - -
- {col.render(row)} -
-
- - {/* Narrow screens: a compact card per row — the primary column as the heading, the - rest flowing in a single wrapping meta line (no full-width label/value gaps). */} -
- {paged.map((row, i) => ( -
- {columns - .filter((c) => c.cardPrimary) - .map((col) => ( -
- {col.render(row)} -
+ + )} + + + {paged.map((row, i) => ( + + {columns.map((col) => ( + + {col.render(row)} + + ))} + ))} -
+ + +
+ ) : ( + /* Narrow screens: a compact card per row — the primary column as the heading, the + rest flowing in a single wrapping meta line (no full-width label/value gaps). */ +
+ {paged.map((row, i) => ( +
{columns - .filter((c) => !c.hideInCard && !c.cardPrimary) + .filter((c) => c.cardPrimary) .map((col) => ( - - {col.cardLabel !== false && ( - {col.header} - )} +
{col.render(row)} - +
))} +
+ {columns + .filter((c) => !c.hideInCard && !c.cardPrimary) + .map((col) => ( + + {col.cardLabel !== false && ( + {col.header} + )} + {col.render(row)} + + ))} +
-
- ))} -
+ ))} +
+ )} {paged.length === 0 && (
diff --git a/frontend/src/components/ui/PageScroller.tsx b/frontend/src/components/ui/PageScroller.tsx index 93d3749a..1a49126e 100644 --- a/frontend/src/components/ui/PageScroller.tsx +++ b/frontend/src/components/ui/PageScroller.tsx @@ -1,5 +1,6 @@ -import { useCallback, useLayoutEffect, useRef, type ReactNode } from "react"; +import { useCallback, useLayoutEffect, useRef, useState, type ReactNode } from "react"; import { useScrollFade } from "../../lib/useScrollFade"; +import { StickyTopProvider } from "./stickyTop"; import { ensureNavId, peekNavId, subscribeNav } from "../../lib/history"; // Saved scroll offsets, keyed by `${historyEntryId}:${scrollKey}`. The entry id ties an offset to the @@ -78,14 +79,18 @@ function restoreScroll(el: HTMLElement, top: number): () => void { export default function PageScroller({ children, scrollKey, - fadeTop = true, }: { children: ReactNode; scrollKey?: string; - // Off when the scroller's top is a sticky element (a table's sticky header) — see useScrollFade. - fadeTop?: boolean; }) { - const fade = useScrollFade(20, fadeTop); + // How many sticky elements sit at the scroller's top right now (see ./stickyTop): the top fade is + // off while any is mounted — a table layout — and back on the moment none is (a card layout). + const [stickyTops, setStickyTops] = useState(0); + const registerStickyTop = useCallback(() => { + setStickyTops((n) => n + 1); + return () => setStickyTops((n) => n - 1); + }, []); + const fade = useScrollFade(20, stickyTops === 0); const fadeRef = fade.ref; const elRef = useRef(null); // Stable identity: an inline ref callback changes every render, which makes React detach and @@ -151,7 +156,7 @@ export default function PageScroller({ style={fade.style} className="flex-1 min-w-0 overflow-y-auto scrollbar-gutter-stable" > - {children} + {children} ); } diff --git a/frontend/src/components/ui/PageShell.tsx b/frontend/src/components/ui/PageShell.tsx index 7ed48449..7ec51cec 100644 --- a/frontend/src/components/ui/PageShell.tsx +++ b/frontend/src/components/ui/PageShell.tsx @@ -45,7 +45,6 @@ export default function PageShell({ banners, children, scrollKey, - fadeTop = true, maxWidth, }: { header?: ReactNode; @@ -53,8 +52,6 @@ export default function PageShell({ children: ReactNode; // Identifies the logical view for the scroll restore/reset policy — see PageScroller. scrollKey?: string; - // Off when this page's scroll region starts with a sticky header — see PageScroller. - fadeTop?: boolean; // The content column's max width (e.g. "1600px"); the header + toolbar centre on the same width via // --content-mw. Omit for a full-width page (feed). See index.css --content-mw. maxWidth?: string; @@ -145,9 +142,7 @@ export default function PageShell({ {/* pr-[--sbw]: the toolbar band isn't the scroller, so without this it would run the scrollbar-gutter's width past the scrolled cards on the right. Pad it back to card width. */}
- - {children} - + {children}
); diff --git a/frontend/src/components/ui/stickyTop.dom.test.tsx b/frontend/src/components/ui/stickyTop.dom.test.tsx new file mode 100644 index 00000000..427984a5 --- /dev/null +++ b/frontend/src/components/ui/stickyTop.dom.test.tsx @@ -0,0 +1,62 @@ +// @vitest-environment jsdom +// +// The page scroller's top fade sits ON a sticky table header, so a page with one used to opt out by +// hand (`fadeTop: false` in the module registry, plus an App special case for the Channels card +// layout) — and the next table page would have re-acquired the defect with every lane green. The +// header now says so itself: while a `ColumnHead` is mounted the scroller asks for no top fade. +import { afterEach, describe, expect, it, vi } from "vitest"; +import { cleanup, render } from "@testing-library/react"; + +const fadeTopArgs: boolean[] = []; +vi.mock("../../lib/useScrollFade", () => ({ + useScrollFade: (_px: number, fadeTop: boolean) => { + fadeTopArgs.push(fadeTop); + return { ref: () => {}, style: {} }; + }, +})); +vi.mock("react-i18next", async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, useTranslation: () => ({ t: (key: string) => key }) }; +}); + +import PageScroller from "./PageScroller"; +import { ColumnHead } from "./tableHeader"; + +afterEach(() => { + cleanup(); + fadeTopArgs.length = 0; +}); + +function page(withTable: boolean) { + return ( + + {withTable ? ( + + {}} + filters={{}} + onFilter={() => {}} + /> +
+ ) : ( +
cards
+ )} + + ); +} + +describe("the page scroller's top fade and a sticky table header", () => { + it("is off while the header is mounted, and back on the moment it is gone", () => { + const view = render(page(true)); + expect(fadeTopArgs.at(-1)).toBe(false); + view.rerender(page(false)); // e.g. the Channels card layout, or a narrow DataTable + expect(fadeTopArgs.at(-1)).toBe(true); + }); + + it("is on for a page with no sticky header at all", () => { + render(page(false)); + expect(fadeTopArgs.at(-1)).toBe(true); + }); +}); diff --git a/frontend/src/components/ui/stickyTop.ts b/frontend/src/components/ui/stickyTop.ts new file mode 100644 index 00000000..519e7e48 --- /dev/null +++ b/frontend/src/components/ui/stickyTop.ts @@ -0,0 +1,17 @@ +import { createContext, useContext, useLayoutEffect } from "react"; + +// A sticky element at the very top of the page scroller (a table's sticky header) and the +// scroller's top fade cannot coexist: the fade sits ON the header and flips on with the first +// scroll. Pages used to opt out by hand — a `fadeTop: false` in the module registry, plus an App +// special case for the Channels card layout, which has no sticky header — and the next table page +// would have re-acquired the defect with every lane green. Now the sticky element says so itself: +// while it is mounted the enclosing `PageScroller` keeps its top edge crisp. +const StickyTopContext = createContext<(() => () => void) | null>(null); + +export const StickyTopProvider = StickyTopContext.Provider; + +/** Call from a component that renders a sticky element at the top of the page scroller. */ +export function useStickyTop(): void { + const register = useContext(StickyTopContext); + useLayoutEffect(() => register?.(), [register]); +} diff --git a/frontend/src/components/ui/tableHeader.tsx b/frontend/src/components/ui/tableHeader.tsx index b08eeffc..f0542f97 100644 --- a/frontend/src/components/ui/tableHeader.tsx +++ b/frontend/src/components/ui/tableHeader.tsx @@ -7,6 +7,7 @@ import { useScrollFade } from "../../lib/useScrollFade"; import { type SortState } from "../../lib/columnSort"; import { isActive, type Column, type FilterMap } from "../../lib/tableColumns"; import { alignClass } from "./tableAlign"; +import { useStickyTop } from "./stickyTop"; // The column-header machinery — the sortable header row with its filter funnel + popover — lifted // OUT of DataTable so a non-DataTable body can render under the same headers. The column contract @@ -185,7 +186,7 @@ function FilterFunnel({ * through; the bottom border rides a `box-shadow` because a sticky ``'s own border scrolls away * with the collapsed table box in some browsers. The `::after` paints a short fade just BELOW the * header so rows dissolve as they slide under it while the header itself stays crisp — which is why - * pages using this pass `PageShell fadeTop={false}`. */ + * it switches the page scroller's own top fade off while it is mounted (`useStickyTop`). */ export function ColumnHead({ columns, sort, @@ -199,6 +200,7 @@ export function ColumnHead({ filters: FilterMap; onFilter: (key: string, value: string | string[]) => void; }) { + useStickyTop(); // Only one column's filter panel is open at a time; each funnel owns its own anchoring. const [openFilter, setOpenFilter] = useState(null); diff --git a/frontend/src/lib/useMediaQuery.ts b/frontend/src/lib/useMediaQuery.ts new file mode 100644 index 00000000..0b8929ed --- /dev/null +++ b/frontend/src/lib/useMediaQuery.ts @@ -0,0 +1,18 @@ +import { useSyncExternalStore } from "react"; + +/** Whether a CSS media query matches, live. For a component that must RENDER differently at a + * breakpoint rather than merely hide one of two renders with CSS (`ui/DataTable` mounted its table + * AND its card list at every width: double the DOM, duplicate ids and keyboard targets). Where + * `matchMedia` is missing (jsdom) it answers `fallback`. */ +export function useMediaQuery(query: string, fallback = true): boolean { + return useSyncExternalStore( + (onChange) => { + if (typeof window.matchMedia !== "function") return () => {}; + const mql = window.matchMedia(query); + mql.addEventListener("change", onChange); + return () => mql.removeEventListener("change", onChange); + }, + () => (typeof window.matchMedia === "function" ? window.matchMedia(query).matches : fallback), + () => fallback, + ); +}