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

- DataTable: keep `pagerNode` NULLABLE. Extracting Pager made it an always-truthy
  element, so the `controls` / `bottomControls` guards always fired and a table with
  nothing to page rendered empty `my-3` bands (plus an empty in-band PageToolbar).
- ChannelsProvider: `setView` bails when the tab is unchanged (re-selecting the
  active tab, or goToFullHistory forcing "subscribed", stacked dead Back entries),
  and pushes a FRESH state like setPage does instead of spreading `_chan`/`_yt`
  markers into the tab entry.
- Lift the layout into its own narrow context (`useChannelLayout`): App read the
  whole ChannelsState for the fade, so every keystroke in the manager's search box
  re-rendered the app root.
- Reset the card page when the row set or sort changes (both tabs) — the clamp only
  hid a stale page, which came back when the filter was cleared.
- Sort + slice the card rows only in cards mode; table mode was copy-sorting ~300
  rows per render for a result nothing read.
- Validate persisted `cardSize` / `sort` (`parseCardSize`, `parseSortState`), matching
  the clamp-on-read convention the module's other persisted values already follow;
  a corrupt size made the page count NaN and emptied the grid.
- Pager: page-size changes scroll to the top too (they re-slice from a new first item).
- ColumnSortControl: a stored sort naming a column this table no longer offers is
  treated as no sort, instead of showing a direction arrow for a sort that isn't applied.
- Drop the stale "rows" layout comments (that variant was retired this sprint).
This commit is contained in:
2026-07-20 00:34:38 +02:00
parent a50608d026
commit be9d17ebb1
9 changed files with 141 additions and 62 deletions
+4 -2
View File
@@ -27,7 +27,7 @@ import {
} from "./lib/storage";
import { useNavigation, useNavigationActions } from "./components/NavigationProvider";
import { useFeedFilters, useFeedFiltersActions } from "./components/FeedFiltersProvider";
import { useChannels } from "./components/ChannelsProvider";
import { useChannelLayout } from "./components/ChannelsProvider";
import { useFeedView, useFeedViewActions } from "./components/FeedViewProvider";
import { moduleDef } from "./lib/modules";
import { PAGE_CONTENT, PAGE_RAIL } from "./components/moduleRegistry";
@@ -65,7 +65,9 @@ export default function App() {
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.
const { layout: channelLayout } = useChannels();
// 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 <Feed>/<ChannelPage>.
const view = useFeedView();
+21 -10
View File
@@ -1,11 +1,12 @@
import { useState } from "react";
import { useEffect, useState } from "react";
import { useTranslation } from "react-i18next";
import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query";
import { UserPlus } from "lucide-react";
import { api, type DiscoveredChannel } from "../lib/api";
import { accountKey, LS, readAccount, writeAccount } from "../lib/storage";
import { formatCountOrDash } from "../lib/format";
import { sortRows, type SortState } from "../lib/columnSort";
import { parseSortState, sortRows, type SortState } from "../lib/columnSort";
import { CARD_PAGE_SIZES, parseCardSize } from "../lib/channelLayout";
import { subsColumn } from "./channelColumns";
import { notify } from "../lib/notifications";
import { notifyYouTubeActionError } from "../lib/youtubeErrors";
@@ -18,7 +19,7 @@ import ColumnSortControl from "./ColumnSortControl";
import Pager from "./Pager";
import { PageToolbar } from "./PageShell";
import { useConfirm } from "./ConfirmProvider";
import { useChannels } from "./ChannelsProvider";
import { useChannelLayout } from "./ChannelsProvider";
// The Channel manager's "Discovery" tab: channels that turn up in the user's playlists but
// that they don't subscribe to. Subscribing here only creates the subscription + enriches
@@ -40,17 +41,24 @@ export default function ChannelDiscovery({
const confirm = useConfirm();
// The table/cards density is shared with the Subscriptions tab (ChannelsProvider). The sort is
// this tab's own (its columns differ), lifted out of the table so the card layout shares it.
const { layout } = useChannels();
const layout = useChannelLayout();
const [sort, setSortState] = useState<SortState>(() =>
readAccount<SortState>(LS.channelDiscoverySort, null),
parseSortState(readAccount(LS.channelDiscoverySort, null)),
);
const setSort = (s: SortState) => {
setSortState(s);
writeAccount(LS.channelDiscoverySort, s);
};
// Card layout pagination (its own; the table keeps its rows-per-page inside DataTable).
const [cardSize, setCardSize] = useState(() => readAccount<number>(LS.channelDiscoveryCardSize, 25));
const [cardSize, setCardSize] = useState(() =>
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"],
@@ -186,12 +194,15 @@ export default function ChannelDiscovery({
},
];
// Card layout paginates like the table (slice the sorted rows; the Pager reports changes).
const sortedRows = sortRows(rows, columns, sort);
// Card layout paginates like the table (slice the sorted rows; the Pager reports changes). Only
// computed for cards — DataTable sorts and slices internally, so doing it in table mode too would
// copy+sort the whole list every render for a result nothing reads.
const isCards = layout === "cards";
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);
const pagedRows =
cardSize <= 0
!isCards || cardSize <= 0
? sortedRows
: sortedRows.slice(cardSafePage * cardSize, cardSafePage * cardSize + cardSize);
@@ -208,7 +219,7 @@ export default function ChannelDiscovery({
page={cardSafePage}
size={cardSize}
total={sortedRows.length}
pageSizeOptions={[10, 25, 50, 100]}
pageSizeOptions={CARD_PAGE_SIZES}
onPageChange={setCardPage}
onSizeChange={(s) => {
setCardSize(s);
+24 -13
View File
@@ -31,7 +31,12 @@ import ChannelLayoutGrid from "./ChannelLayoutGrid";
import ViewSwitcher, { type ViewOption } from "./ViewSwitcher";
import ColumnSortControl from "./ColumnSortControl";
import Pager from "./Pager";
import { CHANNEL_LAYOUTS, type ChannelLayout } from "../lib/channelLayout";
import {
CARD_PAGE_SIZES,
CHANNEL_LAYOUTS,
parseCardSize,
type ChannelLayout,
} from "../lib/channelLayout";
import { sortRows } from "../lib/columnSort";
import { PageToolbar } from "./PageShell";
import ChannelLink from "./ChannelLink";
@@ -39,7 +44,7 @@ import AboutCell from "./AboutCell";
import ChannelDiscovery from "./ChannelDiscovery";
import TagManager from "./TagManager";
import { useConfirm } from "./ConfirmProvider";
import { useChannels, useChannelsActions } from "./ChannelsProvider";
import { useChannelLayout, useChannels, useChannelsActions } from "./ChannelsProvider";
import { useNavigationActions } from "./NavigationProvider";
import { useFeedFilters, useFeedFiltersActions } from "./FeedFiltersProvider";
import { useWizard } from "./WizardProvider";
@@ -61,10 +66,6 @@ const LAYOUT_ICON: Record<ChannelLayout, typeof Table> = {
cards: LayoutGrid,
};
// Cards-per-page options (mirrors the DataTable default); the card grid gets a pager like the table.
const CARD_PAGE_SIZES = [10, 25, 50, 100];
const CARD_SIZE_DEFAULT = 25;
export default function Channels() {
const me = useMe();
const canWrite = me.can_write;
@@ -78,12 +79,12 @@ export default function Channels() {
search,
statusFilter,
view,
layout,
sort,
filtersResetToken,
focusName: focusChannelName,
focusToken: focusChannelToken,
} = useChannels();
const layout = useChannelLayout();
const { setSearch, setStatusFilter, setView, setLayout, setSort, focusChannel: onFocusChannel } =
useChannelsActions();
const { openChannel: onViewChannel, setPage } = useNavigationActions();
@@ -217,8 +218,14 @@ export default function Channels() {
const [tagFilter, setTagFilter] = useState<number[]>([]);
// Card layout pagination (the table keeps its own inside DataTable). Size persists per account;
// the page is transient.
const [cardSize, setCardSize] = useState(() => readAccount<number>(LS.channelCardSize, CARD_SIZE_DEFAULT));
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);
@@ -492,8 +499,8 @@ export default function Channels() {
</div>
);
// Table / cards / rows density — shared by the subscribed + discovery tables (blocked is a chip
// list, no layout to switch). Rides the same generic ViewSwitcher the feed uses.
// Table / cards density — shared by the subscribed + discovery tables (blocked is a chip list, no
// layout to switch). Rides the same generic ViewSwitcher the feed uses.
const layoutOptions: ViewOption<ChannelLayout>[] = CHANNEL_LAYOUTS.map((id) => ({
id,
label: t("channels.layout." + id),
@@ -524,11 +531,14 @@ export default function Channels() {
);
// The card layout paginates like the table (a page of cards + a pager), rather than one long
// infinite scroll. Slice the sorted rows; the Pager reports page/size changes.
const sortedChannels = sortRows(visibleChannels, columns, sort);
// Only computed for cards: in table mode DataTable sorts and slices internally, so doing it here
// too would copy+sort the whole ~300-row array on every render for a result nothing reads.
const isCards = layout === "cards";
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);
const pagedCards =
cardSize <= 0
!isCards || cardSize <= 0
? sortedChannels
: sortedChannels.slice(cardSafePage * cardSize, cardSafePage * cardSize + cardSize);
const cardPager = (
@@ -691,7 +701,8 @@ export default function Channels() {
<div className="px-4 pb-4 max-w-[96rem] mx-auto">
{/* Channel table — only the rows scroll; the header row is sticky, and the controls row
(status chips + pager) rides in the fixed band right above it (controlsInBand). The
card/row layouts drop the table + pager for a virtualized grid over the same rows. */}
card layout swaps the table for a virtualized grid over the same rows (its pager and
status chips ride in the fixed band above instead). */}
{channelsQuery.isLoading ? (
<div className="text-muted py-8">{t("channels.loading")}</div>
) : layout === "table" ? (
+32 -11
View File
@@ -4,12 +4,13 @@ import {
useContext,
useEffect,
useMemo,
useRef,
useState,
type ReactNode,
} from "react";
import { getAccountRaw, LS, readAccount, setAccountRaw, writeAccount } from "../lib/storage";
import { parseChannelLayout, type ChannelLayout } from "../lib/channelLayout";
import { type SortState } from "../lib/columnSort";
import { parseSortState, type SortState } from "../lib/columnSort";
import { useNavigation, useNavigationActions } from "./NavigationProvider";
import type { ChannelStatusFilter, ChannelsView } from "./Channels";
@@ -43,8 +44,6 @@ type ChannelsState = {
search: string;
statusFilter: ChannelStatusFilter;
view: ChannelsView;
// The table/cards density (shared by the subscribed + discovery tabs).
layout: ChannelLayout;
// The subscribed table's sort, lifted out of DataTable so the card layout shares it.
sort: SortState;
// Bumped to tell the manager to drop a stale column filter when an intent sends the user to a
@@ -73,12 +72,15 @@ const StateContext = createContext<ChannelsState>({
search: "",
statusFilter: "all",
view: "subscribed",
layout: "table",
sort: null,
filtersResetToken: 0,
focusName: null,
focusToken: 0,
});
// The table/cards density gets its OWN context, deliberately narrow: App reads it (to decide the
// shell's top fade) and would otherwise re-render the whole app on every keystroke in the manager's
// search box, since the state context above changes with it.
const LayoutContext = createContext<ChannelLayout>("table");
const ActionsContext = createContext<ChannelsActions>({
setSearch: () => {},
setStatusFilter: () => {},
@@ -92,6 +94,11 @@ const ActionsContext = createContext<ChannelsActions>({
export function useChannels(): ChannelsState {
return useContext(StateContext);
}
/** The manager's table/cards density on its own — subscribe to this instead of the whole state when
* the layout is all you need (App does, for the shell's top fade). */
export function useChannelLayout(): ChannelLayout {
return useContext(LayoutContext);
}
export function useChannelsActions(): ChannelsActions {
return useContext(ActionsContext);
}
@@ -110,11 +117,19 @@ export function ChannelsProvider({ children }: { children: ReactNode }) {
const [layout, setLayoutState] = useState<ChannelLayout>(() =>
parseChannelLayout(getAccountRaw(LS.channelLayout)),
);
const [sort, setSortState] = useState<SortState>(() => readAccount<SortState>(LS.channelSort, null));
const [sort, setSortState] = useState<SortState>(() =>
parseSortState(readAccount(LS.channelSort, null)),
);
const [filtersResetToken, setFiltersResetToken] = useState(0);
const [focusName, setFocusName] = useState<string | null>(null);
const [focusToken, setFocusToken] = useState(0);
// Latest-value ref so the STABLE setView can compare against the current tab without depending on
// it (a dep would recreate the action on every switch and re-render every actions consumer).
// Written during render — the sanctioned pattern NavigationProvider uses for `pageRef`.
const viewRef = useRef(view);
viewRef.current = view;
const setSearch = useCallback((q: string) => setSearchState(q), []);
const setStatusFilter = useCallback((f: ChannelStatusFilter) => {
setStatusFilterState(f);
@@ -124,14 +139,18 @@ export function ChannelsProvider({ children }: { children: ReactNode }) {
// live YouTube search (`_yt`) do — so Back steps back through the tabs you actually visited instead
// of jumping straight out to the previous module.
const setView = useCallback((v: ChannelsView) => {
// Re-selecting the tab you are already on is not a navigation — bail before pushing, or clicking
// the active tab (and goToFullHistory forcing "subscribed") stacks Back entries that do nothing.
if (v === viewRef.current) return;
setViewState(v);
setAccountRaw(LS.channelsView, v);
const st = window.history.state || {};
// Only own an entry while the manager is already on screen. Arriving from elsewhere
// (focusChannel / goToFullHistory) lets setPage push the entry — the stamp effect labels it — so
// one navigation never costs two Back presses.
if (st.sfPage === "channels") {
window.history.pushState({ ...st, sfPage: "channels", _chTab: v }, "");
if (window.history.state?.sfPage === "channels") {
// A FRESH state, like setPage's: carrying the old entry's `_chan`/`_yt` markers over would make
// Back re-open the channel overlay or a spent search on top of the tab.
window.history.pushState({ sfPage: "channels", _chTab: v }, "");
}
}, []);
const setLayout = useCallback((l: ChannelLayout) => {
@@ -186,8 +205,8 @@ export function ChannelsProvider({ children }: { children: ReactNode }) {
}, []);
const state = useMemo<ChannelsState>(
() => ({ search, statusFilter, view, layout, sort, filtersResetToken, focusName, focusToken }),
[search, statusFilter, view, layout, sort, filtersResetToken, focusName, focusToken],
() => ({ search, statusFilter, view, sort, filtersResetToken, focusName, focusToken }),
[search, statusFilter, view, sort, filtersResetToken, focusName, focusToken],
);
const actions = useMemo<ChannelsActions>(
() => ({ setSearch, setStatusFilter, setView, setLayout, setSort, focusChannel, goToFullHistory }),
@@ -196,7 +215,9 @@ export function ChannelsProvider({ children }: { children: ReactNode }) {
return (
<ActionsContext.Provider value={actions}>
<StateContext.Provider value={state}>{children}</StateContext.Provider>
<LayoutContext.Provider value={layout}>
<StateContext.Provider value={state}>{children}</StateContext.Provider>
</LayoutContext.Provider>
</ActionsContext.Provider>
);
}
+11 -7
View File
@@ -26,14 +26,18 @@ export default function ColumnSortControl<T>({
descLabel: string;
}) {
const sortable = columns.filter((c) => c.sortable && c.sortValue);
// A stored sort can name a column this table no longer offers (renamed, or `sortable` dropped).
// `sortRows` already leaves the rows unsorted then, so treat it as no sort here too — otherwise the
// select falls back to showing "default" while a direction arrow claims a sort that isn't applied.
const active = sortable.some((c) => c.key === sort?.key) ? sort : null;
return (
<div className="flex items-center gap-1">
<select
value={sort?.key ?? ""}
value={active?.key ?? ""}
onChange={(e) => {
const key = e.target.value;
// Keep the direction when re-picking the same column; a new column starts ascending.
onChange(key ? { key, dir: sort?.key === key ? sort.dir : "asc" } : null);
onChange(key ? { key, dir: active?.key === key ? active.dir : "asc" } : null);
}}
aria-label={label}
className="bg-card border border-border rounded-lg px-2 py-1.5 text-sm outline-none focus:border-accent"
@@ -45,14 +49,14 @@ export default function ColumnSortControl<T>({
</option>
))}
</select>
{sort && (
{active && (
<button
onClick={() => onChange({ key: sort.key, dir: sort.dir === "asc" ? "desc" : "asc" })}
title={sort.dir === "asc" ? ascLabel : descLabel}
aria-label={sort.dir === "asc" ? ascLabel : descLabel}
onClick={() => onChange({ key: active.key, dir: active.dir === "asc" ? "desc" : "asc" })}
title={active.dir === "asc" ? ascLabel : descLabel}
aria-label={active.dir === "asc" ? ascLabel : descLabel}
className="shrink-0 p-1.5 rounded-lg border border-border bg-card text-fg hover:border-accent hover:text-accent active:translate-y-px transition"
>
{sort.dir === "asc" ? <ArrowUp className="w-4 h-4" /> : <ArrowDown className="w-4 h-4" />}
{active.dir === "asc" ? <ArrowUp className="w-4 h-4" /> : <ArrowDown className="w-4 h-4" />}
</button>
)}
</div>
+18 -14
View File
@@ -283,20 +283,24 @@ export default function DataTable<T>({
// The pager + rows-per-page cluster on its own, so it can be rendered a second time at the bottom
// (controlsPosition "both") WITHOUT repeating the leading controls (e.g. the status chips) — those
// are header controls and belong only at the top.
const pagerNode = (
<Pager
page={safePage}
size={size}
total={sorted.length}
pageSizeOptions={pageSizeOptions}
onPageChange={setPage}
onSizeChange={(s) => {
setSize(s);
setPage(0);
}}
sizeLabel={t("datatable.rowsPerPage")}
/>
);
// 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 ? (
<Pager
page={safePage}
size={size}
total={sorted.length}
pageSizeOptions={pageSizeOptions}
onPageChange={setPage}
onSizeChange={(s) => {
setSize(s);
setPage(0);
}}
sizeLabel={t("datatable.rowsPerPage")}
/>
) : null;
const controls =
controlsLeading != null || pagerNode ? (
+12 -5
View File
@@ -35,12 +35,16 @@ export default function Pager({
const [pageInput, setPageInput] = useState(String(safePage + 1));
useEffect(() => setPageInput(String(safePage + 1)), [safePage]);
// Turning the page swaps the whole list, so put the reader at its top — landing mid-list in a page
// you haven't seen is disorienting. The page scroller is the app's <main> (the same implicit
// contract BackToTop and VirtualGrid's getScrollParent rely on).
// Turning the page — or resizing it, which re-slices from a different first item — swaps the whole
// list, so put the reader at its top; landing mid-list in a page you haven't seen is disorienting.
// The page scroller is the app's <main> (PageScroller documents itself as the ONE scroller; the
// same implicit contract BackToTop and VirtualGrid's getScrollParent already rely on).
function scrollListToTop() {
document.querySelector("main")?.scrollTo({ top: 0 });
}
function goToPage(p: number) {
onPageChange(p);
document.querySelector("main")?.scrollTo({ top: 0 });
scrollListToTop();
}
function commitPageInput() {
const n = parseInt(pageInput, 10);
@@ -91,7 +95,10 @@ export default function Pager({
{sizeLabel}
<select
value={size}
onChange={(e) => onSizeChange(Number(e.target.value))}
onChange={(e) => {
onSizeChange(Number(e.target.value));
scrollListToTop();
}}
className="bg-card border border-border rounded-md px-1.5 py-1 text-xs outline-none focus:border-accent"
>
{pageSizeOptions.map((n) => (
+11
View File
@@ -30,3 +30,14 @@ export function parseChannelLayout(raw: unknown): ChannelLayout {
? (raw as ChannelLayout)
: CHANNEL_LAYOUT_DEFAULT;
}
/** Cards-per-page choices for the card layout's pager (0 = "All"), shared by both manager tabs. */
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. */
export function parseCardSize(raw: unknown): number {
return typeof raw === "number" && Number.isFinite(raw) && raw >= 0 ? raw : CARD_SIZE_DEFAULT;
}
+8
View File
@@ -26,6 +26,14 @@ export function sortRows<T>(rows: T[], columns: Sortable<T>[], sort: SortState):
});
}
/** Coerce a stored value to a SortState. Anything malformed (corrupt storage, a shape from an older
* build) becomes "no sort" rather than flowing a junk `dir` into the comparator. */
export function parseSortState(raw: unknown): SortState {
if (!raw || typeof raw !== "object") return null;
const { key, dir } = raw as { key?: unknown; dir?: unknown };
return typeof key === "string" && (dir === "asc" || dir === "desc") ? { key, dir } : null;
}
/** The header-click cycle: unsorted → ascending → descending → unsorted. */
export function cycleSort(current: SortState, key: string): SortState {
if (!current || current.key !== key) return { key, dir: "asc" };