fix(channels,datatable): address R13 S2a review + unify tag chips with the Tags column filter
Review fixes (user /code-review high):
- DataTable: hoist a stable EMPTY_FILTERS fallback so a controlled caller that omits
`filters` doesn't churn a fresh {} each render.
- DataTable: the persistKey blob now stores only the slices this table OWNS — a
controlled sort/filter is written as null/{} instead of a stale never-read duplicate.
- useFilterState: optional `migrate` seed (used only when storage is empty);
ChannelsProvider carries the retired LS.channelFilter sync chip forward via migrateSyncChip.
- Channels: guard the sync-chip read with typeof instead of an `as string` cast.
Also unify the tag filter, same as the sync one: the top "YOUR TAGS" chip row is now a
controlled view over the Tags COLUMN filter (`filters.tags`), replacing its own tagFilter
state — so the chips and the Tags funnel share one state bidirectionally, the card layout
applies it via filterRows, and it persists with the rest of the map.
This commit is contained in:
@@ -209,36 +209,41 @@ export default function Channels() {
|
||||
notifyYouTubeActionError(e, t("channels.notify.fullHistoryFailed"), onOpenWizard),
|
||||
});
|
||||
|
||||
// Channel-name search (from the shared top SearchBar, via props) + tag-chip filtering, applied
|
||||
// client-side over the status-filtered list.
|
||||
const [tagFilter, setTagFilter] = useState<number[]>([]);
|
||||
// The tag-chip row is a controlled view over the Tags COLUMN filter (`filters.tags`, a string[] of
|
||||
// tag ids), the same way the sync chips drive `filters.sync` — so the top chips and the Tags column
|
||||
// funnel share ONE state and stay in sync (bidirectional). Toggling a chip rewrites `filters.tags`;
|
||||
// filterRows / DataTable then applies it. Channel-name search stays separate (the shared top box).
|
||||
const tagFilter = Array.isArray(filters.tags) ? filters.tags : [];
|
||||
const toggleTagFilter = (tagId: number) => {
|
||||
const id = String(tagId);
|
||||
const next = tagFilter.includes(id) ? tagFilter.filter((x) => x !== id) : [...tagFilter, id];
|
||||
setFilters({ ...filters, tags: next });
|
||||
};
|
||||
// A focus-channel intent (header "without full history" / tag manager) seeds the search box.
|
||||
useEffect(() => {
|
||||
if (focusChannelName) setSearch(focusChannelName);
|
||||
// eslint-disable-next-line react-hooks/exhaustive-deps
|
||||
}, [focusChannelToken]);
|
||||
// The header's reset intent clears the search + tag chips (skip the initial mount).
|
||||
// The header's reset intent clears the search box (skip the initial mount); the tag chips ride in
|
||||
// `filters.tags`, which goToFullHistory already clears by setting a fresh map.
|
||||
const resetRef = useRef(filtersResetToken);
|
||||
useEffect(() => {
|
||||
if (filtersResetToken === resetRef.current) return;
|
||||
resetRef.current = filtersResetToken;
|
||||
setSearch("");
|
||||
setTagFilter([]);
|
||||
}, [filtersResetToken]);
|
||||
|
||||
// The sync-status filter is now ONE controlled column filter (`filters.sync`) shared by the top
|
||||
// chips, the Sync column funnel, and the card layout — no separate pre-filter. Hidden channels have
|
||||
// their own tab, so they're excluded from the subscribed table (no more dimmed rows). The name
|
||||
// search + tag chips still filter here; the column filters (sync + priority/tags/text funnels) are
|
||||
// applied by DataTable in table mode and by filterRows below in card mode, over the same `columns`.
|
||||
// The sync-status AND tag filters are now controlled column filters (`filters.sync` / `filters.tags`)
|
||||
// shared by the top chips, the column funnels, and the card layout — no separate pre-filter. Hidden
|
||||
// channels have their own tab, so they're excluded from the subscribed table (no more dimmed rows).
|
||||
// Only the name search filters here; the column filters (sync/tags/priority/text funnels) are applied
|
||||
// by DataTable in table mode and by filterRows below in card mode, over the same `columns`.
|
||||
const hiddenChannels = (channelsQuery.data ?? []).filter((c) => c.hidden);
|
||||
const channels = (channelsQuery.data ?? []).filter((c) => !c.hidden);
|
||||
const q = search.trim().toLowerCase();
|
||||
const visibleChannels = channels.filter((c) => {
|
||||
if (q && !`${c.title ?? ""} ${c.handle ?? ""}`.toLowerCase().includes(q)) return false;
|
||||
if (tagFilter.length && !tagFilter.some((id) => c.tag_ids.includes(id))) return false;
|
||||
return true;
|
||||
});
|
||||
const visibleChannels = channels.filter(
|
||||
(c) => !q || `${c.title ?? ""} ${c.handle ?? ""}`.toLowerCase().includes(q),
|
||||
);
|
||||
const s = statusQuery.data;
|
||||
|
||||
const onView = (c: ManagedChannel) =>
|
||||
@@ -286,7 +291,7 @@ export default function Channels() {
|
||||
|
||||
// Status chips sit in the table's controls row (next to paging) — compact and close to
|
||||
// where they act.
|
||||
const syncFilter = (filters.sync as string) ?? "";
|
||||
const syncFilter = typeof filters.sync === "string" ? filters.sync : "";
|
||||
const statusChips = (
|
||||
<div className="flex flex-wrap items-center gap-1.5">
|
||||
{SYNC_CHIPS.map((c) => (
|
||||
@@ -423,7 +428,7 @@ export default function Channels() {
|
||||
// handled by the hook's clamp instead.
|
||||
useEffect(() => {
|
||||
resetPage();
|
||||
}, [filters, search, tagFilter, sort, resetPage]);
|
||||
}, [filters, search, sort, resetPage]);
|
||||
const cardPager = <Pager {...pagerProps} />;
|
||||
|
||||
const tabs = (
|
||||
@@ -466,24 +471,19 @@ export default function Channels() {
|
||||
{t("channels.tags.yourTags")}
|
||||
</span>
|
||||
</Tooltip>
|
||||
{userTags.map((tg) => {
|
||||
const active = tagFilter.includes(tg.id);
|
||||
return (
|
||||
{userTags.map((tg) => (
|
||||
<button
|
||||
key={tg.id}
|
||||
onClick={() =>
|
||||
setTagFilter((f) => (active ? f.filter((x) => x !== tg.id) : [...f, tg.id]))
|
||||
}
|
||||
aria-pressed={active}
|
||||
onClick={() => toggleTagFilter(tg.id)}
|
||||
aria-pressed={tagFilter.includes(String(tg.id))}
|
||||
className="pill text-xs px-2 py-1 rounded-full border"
|
||||
>
|
||||
{tg.name}
|
||||
</button>
|
||||
);
|
||||
})}
|
||||
))}
|
||||
{tagFilter.length > 0 && (
|
||||
<button
|
||||
onClick={() => setTagFilter([])}
|
||||
onClick={() => setFilters({ ...filters, tags: [] })}
|
||||
className="text-xs px-2 py-1 text-muted hover:text-accent transition"
|
||||
>
|
||||
{t("datatable.clear")}
|
||||
|
||||
@@ -33,6 +33,16 @@ function clampView(v: string | null): ChannelsView {
|
||||
return v === "discovery" || v === "blocked" || v === "hidden" ? v : "subscribed";
|
||||
}
|
||||
|
||||
// One-time carry-forward of the retired single-value sync chip (LS.legacyChannelFilter, vocab
|
||||
// all/needs_full/fully_synced) into the new filter map's `sync` key (vocab ""/needsFull/fully), so a
|
||||
// user who had a chip pinned keeps it. Only consulted when channelsFilterMap is still empty.
|
||||
function migrateSyncChip(): FilterMap | null {
|
||||
const old = getAccountRaw(LS.legacyChannelFilter);
|
||||
if (old === "needs_full") return { sync: "needsFull" };
|
||||
if (old === "fully_synced") return { sync: "fully" };
|
||||
return null;
|
||||
}
|
||||
|
||||
type ChannelsState = {
|
||||
search: string;
|
||||
// The subscribed table's column filters, lifted out of DataTable so the top status chips, the Sync
|
||||
@@ -105,7 +115,7 @@ export function ChannelsProvider({ children }: { children: ReactNode }) {
|
||||
const [search, setSearchState] = useState("");
|
||||
// The subscribed table's column-filter map, owned here (lifted out of DataTable) so the top status
|
||||
// chips, the Sync column funnel, and the card layout share one state; useFilterState persists it.
|
||||
const [filters, setFilters] = useFilterState(LS.channelsFilterMap);
|
||||
const [filters, setFilters] = useFilterState(LS.channelsFilterMap, migrateSyncChip);
|
||||
const [view, setViewState] = useState<ChannelsView>(() =>
|
||||
clampView(getAccountRaw(LS.channelsView)),
|
||||
);
|
||||
|
||||
@@ -25,6 +25,11 @@ interface Persisted {
|
||||
size?: number;
|
||||
}
|
||||
|
||||
// Stable empty-map identity for the controlled-but-value-omitted fallback, so `filters` keeps a
|
||||
// constant reference across renders (a fresh `{}` would churn the persist effect + re-filter every
|
||||
// render). Mirrors controlled sort's stable `null` fallback.
|
||||
const EMPTY_FILTERS: FilterMap = {};
|
||||
|
||||
function loadPersist(key?: string): Persisted {
|
||||
const empty: Persisted = { sort: null, filters: {}, page: 0 };
|
||||
if (!key) return empty;
|
||||
@@ -103,7 +108,7 @@ export default function DataTable<T>({
|
||||
// otherwise the table keeps its own, seeded from and saved to persistKey — symmetric to sort.
|
||||
const controlledFilters = onFiltersChange !== undefined;
|
||||
const [internalFilters, setInternalFilters] = useState<FilterMap>(initial.filters);
|
||||
const filters = controlledFilters ? (filtersProp ?? {}) : internalFilters;
|
||||
const filters = controlledFilters ? (filtersProp ?? EMPTY_FILTERS) : internalFilters;
|
||||
const [page, setPage] = useState(initial.page);
|
||||
const [size, setSize] = useState(initial.size ?? pageSize);
|
||||
|
||||
@@ -117,9 +122,20 @@ export default function DataTable<T>({
|
||||
else setInternalFilters(next);
|
||||
};
|
||||
|
||||
// Persist page/size always, but only the sort/filter slices this table actually OWNS — a controlled
|
||||
// caller persists those itself, so writing them here would just leave a stale, never-read duplicate.
|
||||
useEffect(() => {
|
||||
if (persistKey) localStorage.setItem(persistKey, JSON.stringify({ sort, filters, page, size }));
|
||||
}, [persistKey, sort, filters, page, size]);
|
||||
if (!persistKey) return;
|
||||
localStorage.setItem(
|
||||
persistKey,
|
||||
JSON.stringify({
|
||||
sort: controlledSort ? null : sort,
|
||||
filters: controlledFilters ? {} : filters,
|
||||
page,
|
||||
size,
|
||||
}),
|
||||
);
|
||||
}, [persistKey, controlledSort, controlledFilters, sort, filters, page, size]);
|
||||
|
||||
useEffect(() => {
|
||||
if (!externalFilter) return;
|
||||
|
||||
@@ -26,6 +26,9 @@ export const LS = {
|
||||
// chips, the Sync column funnel, and the card layout all share ONE state (per-account). Supersedes
|
||||
// the old single-string `channelFilter` sync chip.
|
||||
channelsFilterMap: "siftlode.channelsFilterMap",
|
||||
// DEPRECATED — the retired single-value sync chip; read once to migrate a pinned value into
|
||||
// `channelsFilterMap`, then inert. Do not write.
|
||||
legacyChannelFilter: "siftlode.channelFilter",
|
||||
channelsView: "siftlode.channelsView",
|
||||
// The channel manager's layout (table / cards — see lib/channelLayout.ts), per-account.
|
||||
channelLayout: "siftlode.channelLayout",
|
||||
|
||||
@@ -53,4 +53,19 @@ describe("useFilterState", () => {
|
||||
const { result } = renderHook(() => useFilterState("k"));
|
||||
expect(result.current[0]).toEqual({ sync: "needsFull" });
|
||||
});
|
||||
|
||||
it("uses the migrate seed only when storage is empty", () => {
|
||||
const migrate = () => ({ sync: "needsFull" });
|
||||
// Empty storage → seed applies.
|
||||
expect(renderHook(() => useFilterState("k", migrate)).result.current[0]).toEqual({
|
||||
sync: "needsFull",
|
||||
});
|
||||
// Stored value wins over the seed.
|
||||
store.set("k2", JSON.stringify({ sync: "fully" }));
|
||||
expect(renderHook(() => useFilterState("k2", migrate)).result.current[0]).toEqual({
|
||||
sync: "fully",
|
||||
});
|
||||
// A seed that returns null falls back to an empty map.
|
||||
expect(renderHook(() => useFilterState("k3", () => null)).result.current[0]).toEqual({});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -6,8 +6,18 @@ import type { FilterMap } from "../components/tableHeader";
|
||||
// caller (so a second view of the same rows — the channel manager's card grid, the top status chips
|
||||
// — shares one filter state). The table analogue of useSortState: one home so a controlled-filter
|
||||
// surface gets persistence for free, seeded from and saved to `storageBase`.
|
||||
export function useFilterState(storageBase: string): [FilterMap, (next: FilterMap) => void] {
|
||||
const [filters, setFilters] = useState<FilterMap>(() => hydrate(getAccountRaw(storageBase)));
|
||||
export function useFilterState(
|
||||
storageBase: string,
|
||||
// Optional one-time seed used ONLY when `storageBase` has nothing stored yet — e.g. to carry a
|
||||
// retired single-value key forward into the new map. Runs synchronously at init (no first-render
|
||||
// flash); it stays idempotent because the seed persists as soon as the user touches the filters.
|
||||
migrate?: () => FilterMap | null,
|
||||
): [FilterMap, (next: FilterMap) => void] {
|
||||
const [filters, setFilters] = useState<FilterMap>(() => {
|
||||
const raw = getAccountRaw(storageBase);
|
||||
if (raw) return hydrate(raw);
|
||||
return migrate?.() ?? {};
|
||||
});
|
||||
const set = useCallback(
|
||||
(next: FilterMap) => {
|
||||
setFilters(next);
|
||||
|
||||
Reference in New Issue
Block a user