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

- 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.
This commit is contained in:
2026-07-20 00:56:47 +02:00
parent 260de48f41
commit c630d29ff6
6 changed files with 78 additions and 43 deletions
+6 -5
View File
@@ -54,11 +54,6 @@ export default function ChannelDiscovery({
parseCardSize(readAccount(LS.channelDiscoveryCardSize, null)), parseCardSize(readAccount(LS.channelDiscoveryCardSize, null)),
); );
const [cardPage, setCardPage] = useState(0); 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({ const query = useQuery({
queryKey: ["discovered-channels"], queryKey: ["discovered-channels"],
@@ -103,6 +98,12 @@ 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
// 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<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
+47 -29
View File
@@ -221,12 +221,6 @@ export default function Channels() {
// the page is transient. // the page is transient.
const [cardSize, setCardSize] = useState(() => parseCardSize(readAccount(LS.channelCardSize, null))); const [cardSize, setCardSize] = useState(() => parseCardSize(readAccount(LS.channelCardSize, null)));
const [cardPage, setCardPage] = useState(0); 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. // A focus-channel intent (header "without full history" / tag manager) seeds the search box.
useEffect(() => { useEffect(() => {
if (focusChannelName) setSearch(focusChannelName); 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; 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
// 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 s = statusQuery.data;
const onView = (c: ManagedChannel) => const onView = (c: ManagedChannel) =>
@@ -851,47 +852,64 @@ function TagsCell({
[fade.ref], [fade.ref],
); );
// Open upward when there isn't room below, anchoring the menu's BOTTOM to just above the button so // Anchor below the trigger; the layout effect below corrects it against the menu's REAL box. Only
// the flip is exact whatever the real height turns out to be. // opens once a position exists, so the render gate can never show a stale anchor.
const place = () => { const place = useCallback(() => {
const r = btnRef.current?.getBoundingClientRect(); const r = btnRef.current?.getBoundingClientRect();
if (!r) return; if (!r) return false;
const est = Math.min(userTags.length * 30 + 12, 264); // matches the max-height cap below setCoords({ left: r.left, top: r.bottom + 4 });
const up = r.bottom + est > window.innerHeight - 8 && r.top - est > 8; return true;
setCoords({ }, []);
left: r.left,
...(up ? { bottom: window.innerHeight - r.top + 4 } : { top: r.bottom + 4 }),
});
};
const toggle = () => { const toggle = () => {
if (!open) place(); if (open) setOpen(false);
setOpen((o) => !o); else if (place()) setOpen(true);
}; };
// Nudge back inside the viewport once the real (rem-scaled) width is known — measuring beats a px // Correct the placement against the MEASURED box: flip above when it would run off the bottom,
// constant here, since the text-size slider drives the root font and `w-44` grows with it. // 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(() => { useLayoutEffect(() => {
const pop = popRef.current; const pop = popRef.current;
if (!open || !pop || !coords) return; const r = btnRef.current?.getBoundingClientRect();
const w = pop.offsetWidth; if (!open || !pop || !coords || !r) return;
if (coords.left + w > window.innerWidth - 8) { const { offsetHeight: h, offsetWidth: w } = pop;
const left = Math.max(8, window.innerWidth - 8 - w); const next = { ...coords };
if (Math.abs(left - coords.left) > 0.5) setCoords((cur) => (cur ? { ...cur, left } : cur)); // 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]); }, [open, coords]);
// A fixed menu would strand at a stale anchor once the page scrolls (or the window resizes) — // A fixed menu doesn't travel with its row, so FOLLOW the trigger on scroll rather than closing:
// close it, exactly as AboutCell's overlay does. Scrolling INSIDE the menu must not close it. // 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(() => { useEffect(() => {
if (!open) return; 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) => { 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;
setOpen(false); if (!raf) raf = requestAnimationFrame(reposition); // coalesce a scroll burst into one move
}; };
const onResize = () => setOpen(false); const onResize = () => setOpen(false);
window.addEventListener("scroll", onScroll, true); window.addEventListener("scroll", onScroll, true);
window.addEventListener("resize", onResize); window.addEventListener("resize", onResize);
return () => { return () => {
if (raf) cancelAnimationFrame(raf);
window.removeEventListener("scroll", onScroll, true); window.removeEventListener("scroll", onScroll, true);
window.removeEventListener("resize", onResize); window.removeEventListener("resize", onResize);
}; };
+3 -4
View File
@@ -4,7 +4,7 @@ import { ArrowDown, ArrowUp, ListFilter, X } from "lucide-react";
import { useDismiss } from "../lib/useDismiss"; import { useDismiss } from "../lib/useDismiss";
import { useScrollFade } from "../lib/useScrollFade"; import { useScrollFade } from "../lib/useScrollFade";
import { sortRows, cycleSort, type SortState } from "../lib/columnSort"; import { sortRows, cycleSort, type SortState } from "../lib/columnSort";
import Pager from "./Pager"; import Pager, { hasPagerContent } from "./Pager";
import { PageToolbar } from "./PageShell"; import { PageToolbar } from "./PageShell";
// A reusable, client-side data table: per-column sort + filter (in the header) + pagination, // A reusable, client-side data table: per-column sort + filter (in the header) + pagination,
@@ -285,9 +285,8 @@ export default function DataTable<T>({
// are header controls and belong only at the top. // 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 // 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 // -truthy element would leave an empty `my-3` band (and an empty in-band PageToolbar) on a table
// with nothing to page. // with nothing to page. The emptiness rule itself lives in Pager, so the two can't drift apart.
const pagerNode = const pagerNode = hasPagerContent(sorted.length, size) ? (
totalPages > 1 || sorted.length > 0 ? (
<Pager <Pager
page={safePage} page={safePage}
size={size} size={size}
+9 -1
View File
@@ -6,6 +6,14 @@ import { ChevronLeft, ChevronRight } from "lucide-react";
// (the channel manager's card layout) can paginate the same way. Controlled: the caller owns page + // (the channel manager's card layout) can paginate the same way. Controlled: the caller owns page +
// 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.
/** 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;
}
export default function Pager({ export default function Pager({
page, page,
size, size,
@@ -52,7 +60,7 @@ export default function Pager({
else setPageInput(String(safePage + 1)); else setPageInput(String(safePage + 1));
} }
if (!(totalPages > 1 || total > 0)) return null; if (!hasPagerContent(total, size)) return null;
return ( return (
<div className="flex items-center gap-3 flex-wrap"> <div className="flex items-center gap-3 flex-wrap">
{totalPages > 1 && ( {totalPages > 1 && (
+5
View File
@@ -6,6 +6,11 @@ import { useVirtualizer } from "@tanstack/react-virtual";
// mechanics — measure the container, derive the column count, chunk into rows, virtualize, and place // 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 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 // Column sizing mirrors the CSS the feed used before virtualization
// (grid-cols-[repeat(auto-fill,minmax(260px,1fr))] with a gap-4 / 1rem gutter). // (grid-cols-[repeat(auto-fill,minmax(260px,1fr))] with a gap-4 / 1rem gutter).
const GRID_GAP = 16; const GRID_GAP = 16;
+8 -4
View File
@@ -35,9 +35,13 @@ export function parseChannelLayout(raw: unknown): ChannelLayout {
export const CARD_PAGE_SIZES = [10, 25, 50, 100]; export const CARD_PAGE_SIZES = [10, 25, 50, 100];
const CARD_SIZE_DEFAULT = 25; const CARD_SIZE_DEFAULT = 25;
/** Coerce a stored cards-per-page value. Clamped like every other persisted value in this module: /** Coerce a stored cards-per-page value to one the pager actually OFFERS (the sizes above, or 0 =
* a corrupt entry would otherwise make the page count NaN and slice the grid down to nothing, with * "All"). Clamped against the allowed set — like `clampView`/`clampStatus` — not merely type-checked:
* no in-app way back. */ * 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
* `<select>` cannot show, so the control and the behaviour would silently disagree. */
export function parseCardSize(raw: unknown): number { export function parseCardSize(raw: unknown): number {
return typeof raw === "number" && Number.isFinite(raw) && raw >= 0 ? raw : CARD_SIZE_DEFAULT; return typeof raw === "number" && (raw === 0 || CARD_PAGE_SIZES.includes(raw))
? raw
: CARD_SIZE_DEFAULT;
} }