From c630d29ff69916554c411bc8963308ae8890511a Mon Sep 17 00:00:00 2001 From: npeter83 Date: Mon, 20 Jul 2026 00:56:47 +0200 Subject: [PATCH] fix(managers): address the E4 S3 code-review round 2 findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - The portalled tag picker now FOLLOWS its trigger on scroll (rAF-coalesced) instead of closing. Closing on every wheel tick was a regression for the table, where the menu used to travel with its row; it still closes once the trigger leaves the viewport (nothing left to anchor to) or on resize. - Place the menu by MEASUREMENT, not a px estimate: it opens below, then the layout effect flips it above only if the real box overflows. The old `rows * 30px` guess read short at font-scale 1.3 (the rows are rem), which could leave a fixed menu hanging off the bottom with no way to reach it. - `toggle` only opens once a position exists, so the `open && coords` gate can never show a stale anchor. - One owner for "is there anything to page": `hasPagerContent()` lives in Pager and DataTable calls it, instead of both re-deriving the rule — that drift is what left empty control bands behind in the first place. - Reset the card page on the row COUNT too, not just the filter inputs, so an unsubscribe or a shrinking refetch can't leave a stale page to spring back. - `parseCardSize` clamps to the sizes the pager actually offers (like clampView / clampStatus), so the select can't display one value while the grid pages by another. - VirtualGrid's docblock now carries the stacking-context constraint: its rows are transformed, so anything floating an item renders must portal out. --- frontend/src/components/ChannelDiscovery.tsx | 11 +-- frontend/src/components/Channels.tsx | 76 ++++++++++++-------- frontend/src/components/DataTable.tsx | 7 +- frontend/src/components/Pager.tsx | 10 ++- frontend/src/components/VirtualGrid.tsx | 5 ++ frontend/src/lib/channelLayout.ts | 12 ++-- 6 files changed, 78 insertions(+), 43 deletions(-) diff --git a/frontend/src/components/ChannelDiscovery.tsx b/frontend/src/components/ChannelDiscovery.tsx index 7bcfbf2f..5e6a3145 100644 --- a/frontend/src/components/ChannelDiscovery.tsx +++ b/frontend/src/components/ChannelDiscovery.tsx @@ -54,11 +54,6 @@ export default function ChannelDiscovery({ parseCardSize(readAccount(LS.channelDiscoveryCardSize, null)), ); const [cardPage, setCardPage] = useState(0); - // A narrowed or re-sorted list invalidates the page you were on — reset to the first, as the table - // does on its own filter/sort changes. - useEffect(() => { - setCardPage(0); - }, [search, sort]); const query = useQuery({ queryKey: ["discovered-channels"], @@ -103,6 +98,12 @@ export default function ChannelDiscovery({ const rows = (query.data ?? []).filter( (c) => !q || `${c.title ?? ""} ${c.handle ?? ""}`.toLowerCase().includes(q) ); + // A re-sorted or RESIZED list invalidates the page you were on — reset to the first, as the table + // does on its own filter/sort changes. The row count is in the deps too, so a subscribe (which + // drops a channel from this tab) counts, not just an explicit filter change. + useEffect(() => { + setCardPage(0); + }, [search, sort, rows.length]); const columns: Column[] = [ // Actions first, mirroring the Subscriptions table (where it sits right after Prio) so the two diff --git a/frontend/src/components/Channels.tsx b/frontend/src/components/Channels.tsx index 073b6d5c..1177e769 100644 --- a/frontend/src/components/Channels.tsx +++ b/frontend/src/components/Channels.tsx @@ -221,12 +221,6 @@ export default function Channels() { // the page is transient. const [cardSize, setCardSize] = useState(() => parseCardSize(readAccount(LS.channelCardSize, null))); const [cardPage, setCardPage] = useState(0); - // A narrowed or re-sorted list invalidates the page you were on, so go back to the first — the way - // DataTable resets its own page on a filter/sort change. Without this the clamp only hides a stale - // page: clearing the filter again would silently return you to page 10 of the old list. - useEffect(() => { - setCardPage(0); - }, [statusFilter, search, tagFilter, sort]); // A focus-channel intent (header "without full history" / tag manager) seeds the search box. useEffect(() => { if (focusChannelName) setSearch(focusChannelName); @@ -259,6 +253,13 @@ export default function Channels() { if (tagFilter.length && !tagFilter.some((id) => c.tag_ids.includes(id))) return false; return true; }); + // A re-sorted or RESIZED list invalidates the page you were on, so go back to the first — the way + // DataTable resets its own page on a filter/sort change. Keyed on the resulting row count, not just + // the filter inputs, so an unsubscribe or a refetch that shrinks the list counts too: otherwise the + // clamp merely HIDES a stale page, which returns the moment the list grows again. + useEffect(() => { + setCardPage(0); + }, [statusFilter, search, tagFilter, sort, visibleChannels.length]); const s = statusQuery.data; const onView = (c: ManagedChannel) => @@ -851,47 +852,64 @@ function TagsCell({ [fade.ref], ); - // Open upward when there isn't room below, anchoring the menu's BOTTOM to just above the button so - // the flip is exact whatever the real height turns out to be. - const place = () => { + // Anchor below the trigger; the layout effect below corrects it against the menu's REAL box. Only + // opens once a position exists, so the render gate can never show a stale anchor. + const place = useCallback(() => { const r = btnRef.current?.getBoundingClientRect(); - if (!r) return; - const est = Math.min(userTags.length * 30 + 12, 264); // matches the max-height cap below - const up = r.bottom + est > window.innerHeight - 8 && r.top - est > 8; - setCoords({ - left: r.left, - ...(up ? { bottom: window.innerHeight - r.top + 4 } : { top: r.bottom + 4 }), - }); - }; + if (!r) return false; + setCoords({ left: r.left, top: r.bottom + 4 }); + return true; + }, []); const toggle = () => { - if (!open) place(); - setOpen((o) => !o); + if (open) setOpen(false); + else if (place()) setOpen(true); }; - // Nudge back inside the viewport once the real (rem-scaled) width is known — measuring beats a px - // constant here, since the text-size slider drives the root font and `w-44` grows with it. + // Correct the placement against the MEASURED box: flip above when it would run off the bottom, + // nudge back inside the right edge. Measured, not estimated — the menu's rows and its `w-44` are + // rem, so both grow with the text-size slider and any px guess reads short at 1.3. useLayoutEffect(() => { const pop = popRef.current; - if (!open || !pop || !coords) return; - const w = pop.offsetWidth; - if (coords.left + w > window.innerWidth - 8) { - const left = Math.max(8, window.innerWidth - 8 - w); - if (Math.abs(left - coords.left) > 0.5) setCoords((cur) => (cur ? { ...cur, left } : cur)); + const r = btnRef.current?.getBoundingClientRect(); + if (!open || !pop || !coords || !r) return; + const { offsetHeight: h, offsetWidth: w } = pop; + const next = { ...coords }; + // Flip up only when it actually overflows AND there is room above (else stay put and clip). + if (coords.top !== undefined && coords.top + h > window.innerHeight - 8 && r.top - h - 4 > 8) { + delete next.top; + next.bottom = window.innerHeight - r.top + 4; + } + if (coords.left + w > window.innerWidth - 8) next.left = Math.max(8, window.innerWidth - 8 - w); + if (next.left !== coords.left || next.top !== coords.top || next.bottom !== coords.bottom) { + setCoords(next); } }, [open, coords]); - // A fixed menu would strand at a stale anchor once the page scrolls (or the window resizes) — - // close it, exactly as AboutCell's overlay does. Scrolling INSIDE the menu must not close it. + // A fixed menu doesn't travel with its row, so FOLLOW the trigger on scroll rather than closing: + // in the table the menu used to scroll with the row, and closing on every wheel tick would make + // attaching several tags a reopen-per-tag chore. Closes only once the trigger leaves the viewport + // (nothing left to anchor to) or on resize. Scrolling INSIDE the menu must not move it. useEffect(() => { if (!open) return; + let raf = 0; + const reposition = () => { + raf = 0; + const r = btnRef.current?.getBoundingClientRect(); + if (!r || r.bottom < 0 || r.top > window.innerHeight) { + setOpen(false); + return; + } + setCoords({ left: r.left, top: r.bottom + 4 }); + }; const onScroll = (e: Event) => { if (popRef.current && e.target instanceof Node && popRef.current.contains(e.target)) return; - setOpen(false); + if (!raf) raf = requestAnimationFrame(reposition); // coalesce a scroll burst into one move }; const onResize = () => setOpen(false); window.addEventListener("scroll", onScroll, true); window.addEventListener("resize", onResize); return () => { + if (raf) cancelAnimationFrame(raf); window.removeEventListener("scroll", onScroll, true); window.removeEventListener("resize", onResize); }; diff --git a/frontend/src/components/DataTable.tsx b/frontend/src/components/DataTable.tsx index ef6025dd..fa480f9c 100644 --- a/frontend/src/components/DataTable.tsx +++ b/frontend/src/components/DataTable.tsx @@ -4,7 +4,7 @@ import { ArrowDown, ArrowUp, ListFilter, X } from "lucide-react"; import { useDismiss } from "../lib/useDismiss"; import { useScrollFade } from "../lib/useScrollFade"; import { sortRows, cycleSort, type SortState } from "../lib/columnSort"; -import Pager from "./Pager"; +import Pager, { hasPagerContent } from "./Pager"; import { PageToolbar } from "./PageShell"; // A reusable, client-side data table: per-column sort + filter (in the header) + pagination, @@ -285,9 +285,8 @@ export default function DataTable({ // are header controls and belong only at the top. // Stays NULLABLE: the wrappers below decide whether to render a controls row at all, and an always // -truthy element would leave an empty `my-3` band (and an empty in-band PageToolbar) on a table - // with nothing to page. - const pagerNode = - totalPages > 1 || sorted.length > 0 ? ( + // with nothing to page. The emptiness rule itself lives in Pager, so the two can't drift apart. + const pagerNode = hasPagerContent(sorted.length, size) ? ( 1 || total > 0; +} export default function Pager({ page, size, @@ -52,7 +60,7 @@ export default function Pager({ else setPageInput(String(safePage + 1)); } - if (!(totalPages > 1 || total > 0)) return null; + if (!hasPagerContent(total, size)) return null; return (
{totalPages > 1 && ( diff --git a/frontend/src/components/VirtualGrid.tsx b/frontend/src/components/VirtualGrid.tsx index 8a875bc0..9a1ec501 100644 --- a/frontend/src/components/VirtualGrid.tsx +++ b/frontend/src/components/VirtualGrid.tsx @@ -6,6 +6,11 @@ import { useVirtualizer } from "@tanstack/react-virtual"; // mechanics — measure the container, derive the column count, chunk into rows, virtualize, and place // each row absolutely — and knows nothing about what an item is. The caller supplies `renderItem`. // +// ⚠️ Each row is placed with `position:absolute` + `transform` (below), which makes EVERY ROW ITS OWN +// STACKING CONTEXT. A z-indexed child therefore cannot escape its row: a menu or popover rendered +// inside an item draws UNDER the next row, whatever z-index it carries — the channel manager's tag +// picker hit exactly this. Anything floating that an item renders must portal out (see Overlay). +// // Column sizing mirrors the CSS the feed used before virtualization // (grid-cols-[repeat(auto-fill,minmax(260px,1fr))] with a gap-4 / 1rem gutter). const GRID_GAP = 16; diff --git a/frontend/src/lib/channelLayout.ts b/frontend/src/lib/channelLayout.ts index 14ed6dd1..b4a1d7e4 100644 --- a/frontend/src/lib/channelLayout.ts +++ b/frontend/src/lib/channelLayout.ts @@ -35,9 +35,13 @@ export function parseChannelLayout(raw: unknown): ChannelLayout { export const CARD_PAGE_SIZES = [10, 25, 50, 100]; const CARD_SIZE_DEFAULT = 25; -/** Coerce a stored cards-per-page value. Clamped like every other persisted value in this module: - * a corrupt entry would otherwise make the page count NaN and slice the grid down to nothing, with - * no in-app way back. */ +/** Coerce a stored cards-per-page value to one the pager actually OFFERS (the sizes above, or 0 = + * "All"). Clamped against the allowed set — like `clampView`/`clampStatus` — not merely type-checked: + * a corrupt entry would make the page count NaN and slice the grid down to nothing, and an + * off-menu number (a hand-edited 7, or an option a future build dropped) would page by a value the + * `