From ca9dd549233b9a1b0806421022f2998f5b8bae29 Mon Sep 17 00:00:00 2001
From: npeter83 <npeter83+claude@gmail.com>
Date: Fri, 14 Aug 2026 02:19:44 +0200
Subject: [PATCH] fix(channels,datatable): address R13 S2a review + unify tag
 chips with the Tags column filter
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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.
---
 frontend/src/components/Channels.tsx         | 66 ++++++++++----------
 frontend/src/components/ChannelsProvider.tsx | 12 +++-
 frontend/src/components/DataTable.tsx        | 22 ++++++-
 frontend/src/lib/storage.ts                  |  3 +
 frontend/src/lib/useFilterState.dom.test.ts  | 15 +++++
 frontend/src/lib/useFilterState.ts           | 14 ++++-
 6 files changed, 93 insertions(+), 39 deletions(-)

diff --git a/frontend/src/components/Channels.tsx b/frontend/src/components/Channels.tsx
index 82718126..0f1f7fe9 100644
--- a/frontend/src/components/Channels.tsx
+++ b/frontend/src/components/Channels.tsx
@@ -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 (
-                    <button
-                      key={tg.id}
-                      onClick={() =>
-                        setTagFilter((f) => (active ? f.filter((x) => x !== tg.id) : [...f, tg.id]))
-                      }
-                      aria-pressed={active}
-                      className="pill text-xs px-2 py-1 rounded-full border"
-                    >
-                      {tg.name}
-                    </button>
-                  );
-                })}
+                {userTags.map((tg) => (
+                  <button
+                    key={tg.id}
+                    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")}
diff --git a/frontend/src/components/ChannelsProvider.tsx b/frontend/src/components/ChannelsProvider.tsx
index df04e989..6e9a1425 100644
--- a/frontend/src/components/ChannelsProvider.tsx
+++ b/frontend/src/components/ChannelsProvider.tsx
@@ -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)),
   );
diff --git a/frontend/src/components/DataTable.tsx b/frontend/src/components/DataTable.tsx
index c15334b1..d6da209e 100644
--- a/frontend/src/components/DataTable.tsx
+++ b/frontend/src/components/DataTable.tsx
@@ -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;
diff --git a/frontend/src/lib/storage.ts b/frontend/src/lib/storage.ts
index b783c93a..2e12bc97 100644
--- a/frontend/src/lib/storage.ts
+++ b/frontend/src/lib/storage.ts
@@ -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",
diff --git a/frontend/src/lib/useFilterState.dom.test.ts b/frontend/src/lib/useFilterState.dom.test.ts
index dc444dfd..ea692198 100644
--- a/frontend/src/lib/useFilterState.dom.test.ts
+++ b/frontend/src/lib/useFilterState.dom.test.ts
@@ -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({});
+  });
 });
diff --git a/frontend/src/lib/useFilterState.ts b/frontend/src/lib/useFilterState.ts
index facc3be4..469a7fcf 100644
--- a/frontend/src/lib/useFilterState.ts
+++ b/frontend/src/lib/useFilterState.ts
@@ -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);