From ab5f3c9fbb6d65d78b6c050bd9f11f7045c0adf6 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Mon, 20 Jul 2026 01:10:13 +0200 Subject: [PATCH] fix(managers): address the E4 S3 code-review round 3 findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - The tag menu's "trigger is gone" test now measures against the SCROLLER, not the window. The page scroller starts ~230px down (measured), so a row that slid under the fixed toolbar was still "on screen" by window coords and the menu floated over the chrome, anchored to something invisible. - Scroll-repositioning keeps whichever side the menu is anchored to instead of resetting it to "below" every frame — that reset made the layout effect flip it back, costing two renders and two forced layouts per frame near the bottom edge. - One placement rule (`coordsFrom`) now serves the initial open, the layout correction and every reposition, so those can no longer drift apart. - Card page: reset on the filter/sort INPUTS only, and separately COMMIT the clamp when the page falls out of range. Keying the reset on the row count (round 2) was too blunt — a background refetch that merely added a channel threw the reader off page 7 for something they didn't do. - Pager derives `pageCount()` once and `hasPagerContent` reuses it, instead of the same expression twice in one file. - Re-indent the pagerNode JSX left dangling by round 2's ternary edit. --- frontend/src/components/ChannelDiscovery.tsx | 13 ++-- frontend/src/components/Channels.tsx | 75 +++++++++++++------- frontend/src/components/DataTable.tsx | 26 +++---- frontend/src/components/Pager.tsx | 11 +-- 4 files changed, 78 insertions(+), 47 deletions(-) diff --git a/frontend/src/components/ChannelDiscovery.tsx b/frontend/src/components/ChannelDiscovery.tsx index 5e6a3145..c3ce3d5c 100644 --- a/frontend/src/components/ChannelDiscovery.tsx +++ b/frontend/src/components/ChannelDiscovery.tsx @@ -98,12 +98,13 @@ 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. + // Changing what you're looking at starts over at the first page, as the table does on its own + // filter/sort change. NOT keyed on the row count: subscribing drops a row from this tab, and being + // thrown to page 1 for it would punish the very action you just took. A list that shrank past your + // page is handled by committing the clamp instead (below). useEffect(() => { setCardPage(0); - }, [search, sort, rows.length]); + }, [search, sort]); const columns: Column[] = [ // Actions first, mirroring the Subscriptions table (where it sits right after Prio) so the two @@ -202,6 +203,10 @@ export default function ChannelDiscovery({ const sortedRows = isCards ? sortRows(rows, columns, sort) : rows; const cardTotalPages = cardSize <= 0 ? 1 : Math.max(1, Math.ceil(sortedRows.length / cardSize)); const cardSafePage = Math.min(cardPage, cardTotalPages - 1); + // Commit the clamp when the list shrinks past your page, so it can't spring back later. + useEffect(() => { + if (cardPage > cardTotalPages - 1) setCardPage(cardTotalPages - 1); + }, [cardPage, cardTotalPages]); const pagedRows = !isCards || cardSize <= 0 ? sortedRows diff --git a/frontend/src/components/Channels.tsx b/frontend/src/components/Channels.tsx index 1177e769..4b778540 100644 --- a/frontend/src/components/Channels.tsx +++ b/frontend/src/components/Channels.tsx @@ -253,13 +253,14 @@ 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. + // Changing what you're looking at starts over at the first page, the way DataTable resets its own + // page on a filter/sort change. Deliberately NOT keyed on the row count: the ["channels"] query + // refetches on window focus and after every sync, so a catalogue that merely gained a channel would + // otherwise yank you off page 7 for something you didn't do. A list that shrank under you is handled + // by committing the clamp instead (see below). useEffect(() => { setCardPage(0); - }, [statusFilter, search, tagFilter, sort, visibleChannels.length]); + }, [statusFilter, search, tagFilter, sort]); const s = statusQuery.data; const onView = (c: ManagedChannel) => @@ -539,6 +540,12 @@ export default function Channels() { const sortedChannels = isCards ? sortRows(visibleChannels, columns, sort) : visibleChannels; const cardTotalPages = cardSize <= 0 ? 1 : Math.max(1, Math.ceil(sortedChannels.length / cardSize)); const cardSafePage = Math.min(cardPage, cardTotalPages - 1); + // When the list shrinks under you the clamp above only HIDES the out-of-range page — commit it, or + // it springs back the moment the list grows again. Keeps your place otherwise (unlike a blanket + // reset), because it only fires when the page you're on no longer exists. + useEffect(() => { + if (cardPage > cardTotalPages - 1) setCardPage(cardTotalPages - 1); + }, [cardPage, cardTotalPages]); const pagedCards = !isCards || cardSize <= 0 ? sortedChannels @@ -852,54 +859,70 @@ function TagsCell({ [fade.ref], ); - // 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. + // ONE placement rule, so the position the menu opens at and the one it keeps while scrolling can't + // drift apart. `flip` keeps it anchored above the trigger once it has been flipped; the horizontal + // clamp uses the MEASURED width (`w-44` is rem — it grows with the text-size slider, so a px guess + // reads short at 1.3). Before first paint the menu isn't measurable yet; the layout effect below + // re-runs this once it is. + const coordsFrom = useCallback((r: DOMRect, flip: boolean) => { + const w = popRef.current?.offsetWidth ?? 0; + const left = + w && r.left + w > window.innerWidth - 8 ? Math.max(8, window.innerWidth - 8 - w) : r.left; + return flip + ? { left, bottom: window.innerHeight - r.top + 4 } + : { left, top: r.bottom + 4 }; + }, []); + const place = useCallback(() => { const r = btnRef.current?.getBoundingClientRect(); if (!r) return false; - setCoords({ left: r.left, top: r.bottom + 4 }); + setCoords(coordsFrom(r, false)); return true; - }, []); + }, [coordsFrom]); const toggle = () => { if (open) setOpen(false); else if (place()) setOpen(true); }; - // 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. + // Correct the placement against the MEASURED box: flip above when it would run off the bottom (and + // there is room above), and apply the width clamp now that the menu can actually be measured. useLayoutEffect(() => { const pop = popRef.current; 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); + const h = pop.offsetHeight; // the width is read inside coordsFrom, off the same node + const flip = + coords.top !== undefined + ? coords.top + h > window.innerHeight - 8 && r.top - h - 4 > 8 // overflows below, fits above + : true; // already flipped — keep it there + const next = coordsFrom(r, flip); if (next.left !== coords.left || next.top !== coords.top || next.bottom !== coords.bottom) { setCoords(next); } - }, [open, coords]); + }, [open, coords, coordsFrom]); // 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. + // attaching several tags a reopen-per-tag chore. Closes only once the trigger leaves the SCROLLER + // (not the window — the page scroller starts below ~230px of fixed chrome, so a row hidden behind + // the toolbar is still "on screen" by window coords and the menu would float over the chrome). + // 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) { + const scroller = document.querySelector("main")?.getBoundingClientRect(); + const topEdge = scroller ? scroller.top : 0; + const bottomEdge = scroller ? scroller.bottom : window.innerHeight; + if (!r || r.bottom < topEdge || r.top > bottomEdge) { setOpen(false); return; } - setCoords({ left: r.left, top: r.bottom + 4 }); + // Keep whichever side it is anchored to, so the layout effect has nothing left to correct — + // resetting to "below" every frame would make it flip back and forth, two renders per frame. + setCoords((cur) => (cur ? coordsFrom(r, cur.bottom !== undefined) : cur)); }; const onScroll = (e: Event) => { if (popRef.current && e.target instanceof Node && popRef.current.contains(e.target)) return; @@ -913,7 +936,7 @@ function TagsCell({ window.removeEventListener("scroll", onScroll, true); window.removeEventListener("resize", onResize); }; - }, [open]); + }, [open, coordsFrom]); if (userTags.length === 0) return null; // Show only the tags actually attached to this channel; the “+” opens a per-channel // picker to attach/detach (kept the convenient "kirakás" without listing every tag diff --git a/frontend/src/components/DataTable.tsx b/frontend/src/components/DataTable.tsx index fa480f9c..e6cf2ee7 100644 --- a/frontend/src/components/DataTable.tsx +++ b/frontend/src/components/DataTable.tsx @@ -287,19 +287,19 @@ export default function DataTable({ // -truthy element would leave an empty `my-3` band (and an empty in-band PageToolbar) on a table // with nothing to page. The emptiness rule itself lives in Pager, so the two can't drift apart. const pagerNode = hasPagerContent(sorted.length, size) ? ( - { - setSize(s); - setPage(0); - }} - sizeLabel={t("datatable.rowsPerPage")} - /> - ) : null; + { + setSize(s); + setPage(0); + }} + sizeLabel={t("datatable.rowsPerPage")} + /> + ) : null; const controls = controlsLeading != null || pagerNode ? ( diff --git a/frontend/src/components/Pager.tsx b/frontend/src/components/Pager.tsx index 5e3a651a..00505137 100644 --- a/frontend/src/components/Pager.tsx +++ b/frontend/src/components/Pager.tsx @@ -7,12 +7,16 @@ import { ChevronLeft, ChevronRight } from "lucide-react"; // size and slices its own rows; this only renders the controls and reports changes. `sizeLabel` lets // the caller say "Rows per page" or "Cards per page". Returns null when there's nothing to page. +/** How many pages `total` rows make at `size` (`size <= 0` = "All" = one page). */ +function pageCount(total: number, size: number): number { + return size <= 0 ? 1 : Math.max(1, Math.ceil(total / size)); +} + /** Whether a pager would render anything — the ONE owner of that rule. Callers that wrap the pager * in their own chrome gate on this instead of re-deriving it: a drifted second copy is exactly what * left empty control bands behind on tables with nothing to page. */ export function hasPagerContent(total: number, size: number): boolean { - const totalPages = size <= 0 ? 1 : Math.max(1, Math.ceil(total / size)); - return totalPages > 1 || total > 0; + return pageCount(total, size) > 1 || total > 0; } export default function Pager({ page, @@ -35,8 +39,7 @@ export default function Pager({ sizeLabel: string; }) { const { t } = useTranslation(); - const allRows = size <= 0; - const totalPages = allRows ? 1 : Math.max(1, Math.ceil(total / size)); + 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).