fix(managers): address the E4 S3 code-review round 3 findings

- 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.
This commit is contained in:
2026-07-20 01:10:13 +02:00
parent c630d29ff6
commit ab5f3c9fbb
4 changed files with 78 additions and 47 deletions
+9 -4
View File
@@ -98,12 +98,13 @@ export default function ChannelDiscovery({
const rows = (query.data ?? []).filter( const rows = (query.data ?? []).filter(
(c) => !q || `${c.title ?? ""} ${c.handle ?? ""}`.toLowerCase().includes(q) (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 // Changing what you're looking at starts over at the first page, as the table does on its own
// does on its own filter/sort changes. The row count is in the deps too, so a subscribe (which // filter/sort change. NOT keyed on the row count: subscribing drops a row from this tab, and being
// drops a channel from this tab) counts, not just an explicit filter change. // 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(() => { useEffect(() => {
setCardPage(0); setCardPage(0);
}, [search, sort, rows.length]); }, [search, sort]);
const columns: Column<DiscoveredChannel>[] = [ const columns: Column<DiscoveredChannel>[] = [
// Actions first, mirroring the Subscriptions table (where it sits right after Prio) so the two // 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 sortedRows = isCards ? sortRows(rows, columns, sort) : rows;
const cardTotalPages = cardSize <= 0 ? 1 : Math.max(1, Math.ceil(sortedRows.length / cardSize)); const cardTotalPages = cardSize <= 0 ? 1 : Math.max(1, Math.ceil(sortedRows.length / cardSize));
const cardSafePage = Math.min(cardPage, cardTotalPages - 1); 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 = const pagedRows =
!isCards || cardSize <= 0 !isCards || cardSize <= 0
? sortedRows ? sortedRows
+49 -26
View File
@@ -253,13 +253,14 @@ export default function Channels() {
if (tagFilter.length && !tagFilter.some((id) => c.tag_ids.includes(id))) return false; if (tagFilter.length && !tagFilter.some((id) => c.tag_ids.includes(id))) return false;
return true; return true;
}); });
// A re-sorted or RESIZED list invalidates the page you were on, so go back to the first — the way // Changing what you're looking at starts over at the first page, the way DataTable resets its own
// DataTable resets its own page on a filter/sort change. Keyed on the resulting row count, not just // page on a filter/sort change. Deliberately NOT keyed on the row count: the ["channels"] query
// the filter inputs, so an unsubscribe or a refetch that shrinks the list counts too: otherwise the // refetches on window focus and after every sync, so a catalogue that merely gained a channel would
// clamp merely HIDES a stale page, which returns the moment the list grows again. // 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(() => { useEffect(() => {
setCardPage(0); setCardPage(0);
}, [statusFilter, search, tagFilter, sort, visibleChannels.length]); }, [statusFilter, search, tagFilter, sort]);
const s = statusQuery.data; const s = statusQuery.data;
const onView = (c: ManagedChannel) => const onView = (c: ManagedChannel) =>
@@ -539,6 +540,12 @@ export default function Channels() {
const sortedChannels = isCards ? sortRows(visibleChannels, columns, sort) : visibleChannels; const sortedChannels = isCards ? sortRows(visibleChannels, columns, sort) : visibleChannels;
const cardTotalPages = cardSize <= 0 ? 1 : Math.max(1, Math.ceil(sortedChannels.length / cardSize)); const cardTotalPages = cardSize <= 0 ? 1 : Math.max(1, Math.ceil(sortedChannels.length / cardSize));
const cardSafePage = Math.min(cardPage, cardTotalPages - 1); 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 = const pagedCards =
!isCards || cardSize <= 0 !isCards || cardSize <= 0
? sortedChannels ? sortedChannels
@@ -852,54 +859,70 @@ function TagsCell({
[fade.ref], [fade.ref],
); );
// Anchor below the trigger; the layout effect below corrects it against the menu's REAL box. Only // ONE placement rule, so the position the menu opens at and the one it keeps while scrolling can't
// opens once a position exists, so the render gate can never show a stale anchor. // 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 place = useCallback(() => {
const r = btnRef.current?.getBoundingClientRect(); const r = btnRef.current?.getBoundingClientRect();
if (!r) return false; if (!r) return false;
setCoords({ left: r.left, top: r.bottom + 4 }); setCoords(coordsFrom(r, false));
return true; return true;
}, []); }, [coordsFrom]);
const toggle = () => { const toggle = () => {
if (open) setOpen(false); if (open) setOpen(false);
else if (place()) setOpen(true); else if (place()) setOpen(true);
}; };
// Correct the placement against the MEASURED box: flip above when it would run off the bottom, // Correct the placement against the MEASURED box: flip above when it would run off the bottom (and
// nudge back inside the right edge. Measured, not estimated — the menu's rows and its `w-44` are // there is room above), and apply the width clamp now that the menu can actually be measured.
// rem, so both grow with the text-size slider and any px guess reads short at 1.3.
useLayoutEffect(() => { useLayoutEffect(() => {
const pop = popRef.current; const pop = popRef.current;
const r = btnRef.current?.getBoundingClientRect(); const r = btnRef.current?.getBoundingClientRect();
if (!open || !pop || !coords || !r) return; if (!open || !pop || !coords || !r) return;
const { offsetHeight: h, offsetWidth: w } = pop; const h = pop.offsetHeight; // the width is read inside coordsFrom, off the same node
const next = { ...coords }; const flip =
// Flip up only when it actually overflows AND there is room above (else stay put and clip). coords.top !== undefined
if (coords.top !== undefined && coords.top + h > window.innerHeight - 8 && r.top - h - 4 > 8) { ? coords.top + h > window.innerHeight - 8 && r.top - h - 4 > 8 // overflows below, fits above
delete next.top; : true; // already flipped — keep it there
next.bottom = window.innerHeight - r.top + 4; const next = coordsFrom(r, flip);
}
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) { if (next.left !== coords.left || next.top !== coords.top || next.bottom !== coords.bottom) {
setCoords(next); 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: // 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 // 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 // attaching several tags a reopen-per-tag chore. Closes only once the trigger leaves the SCROLLER
// (nothing left to anchor to) or on resize. Scrolling INSIDE the menu must not move it. // (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(() => { useEffect(() => {
if (!open) return; if (!open) return;
let raf = 0; let raf = 0;
const reposition = () => { const reposition = () => {
raf = 0; raf = 0;
const r = btnRef.current?.getBoundingClientRect(); 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); setOpen(false);
return; 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) => { const onScroll = (e: Event) => {
if (popRef.current && e.target instanceof Node && popRef.current.contains(e.target)) return; 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("scroll", onScroll, true);
window.removeEventListener("resize", onResize); window.removeEventListener("resize", onResize);
}; };
}, [open]); }, [open, coordsFrom]);
if (userTags.length === 0) return null; if (userTags.length === 0) return null;
// Show only the tags actually attached to this channel; the “+” opens a per-channel // 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 // picker to attach/detach (kept the convenient "kirakás" without listing every tag
+13 -13
View File
@@ -287,19 +287,19 @@ export default function DataTable<T>({
// -truthy element would leave an empty `my-3` band (and an empty in-band PageToolbar) on a table // -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. // with nothing to page. The emptiness rule itself lives in Pager, so the two can't drift apart.
const pagerNode = hasPagerContent(sorted.length, size) ? ( const pagerNode = hasPagerContent(sorted.length, size) ? (
<Pager <Pager
page={safePage} page={safePage}
size={size} size={size}
total={sorted.length} total={sorted.length}
pageSizeOptions={pageSizeOptions} pageSizeOptions={pageSizeOptions}
onPageChange={setPage} onPageChange={setPage}
onSizeChange={(s) => { onSizeChange={(s) => {
setSize(s); setSize(s);
setPage(0); setPage(0);
}} }}
sizeLabel={t("datatable.rowsPerPage")} sizeLabel={t("datatable.rowsPerPage")}
/> />
) : null; ) : null;
const controls = const controls =
controlsLeading != null || pagerNode ? ( controlsLeading != null || pagerNode ? (
+7 -4
View File
@@ -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 // 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. // 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 /** 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 * 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. */ * left empty control bands behind on tables with nothing to page. */
export function hasPagerContent(total: number, size: number): boolean { export function hasPagerContent(total: number, size: number): boolean {
const totalPages = size <= 0 ? 1 : Math.max(1, Math.ceil(total / size)); return pageCount(total, size) > 1 || total > 0;
return totalPages > 1 || total > 0;
} }
export default function Pager({ export default function Pager({
page, page,
@@ -35,8 +39,7 @@ export default function Pager({
sizeLabel: string; sizeLabel: string;
}) { }) {
const { t } = useTranslation(); const { t } = useTranslation();
const allRows = size <= 0; const totalPages = pageCount(total, size);
const totalPages = allRows ? 1 : Math.max(1, Math.ceil(total / size));
const safePage = Math.min(page, totalPages - 1); const safePage = Math.min(page, totalPages - 1);
// Local text for the editable "jump to page" box (committed on Enter/blur). // Local text for the editable "jump to page" box (committed on Enter/blur).