refactor(ux): address /code-review high findings on the fixpack

- Extract the optimistic + debounced-flush feed-priority flow into a shared
  useChannelPriority hook (mirrors useChannelTagActions); the manager and the
  channel-detail page now share it instead of two drifting copies.
- NavSidebar: drop the always-true `slim` const and its dead expanded-rail
  branches; refresh the now-stale pinned/collapsible comments.
- Remove the orphaned `navCollapsed` localStorage key (no readers/writers left
  after the always-slim rail).
- Channel page: drop the wrapper native `title` that duplicated PriorityStepper's
  own Tooltip.

The keyboard-focus nav-label finding needed no change: the popover platform
already wires useFocus (visibleOnly), so a real Tab press shows the flyout label
(verified in-browser).
This commit is contained in:
2026-08-11 03:29:08 +02:00
parent 71bbf35f5a
commit 02754f479f
5 changed files with 123 additions and 123 deletions
+18 -45
View File
@@ -4,6 +4,7 @@ import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query";
import { qk } from "../lib/queryKeys";
import { useNavigationActions } from "./NavigationProvider";
import { PriorityStepper } from "./PriorityStepper";
import { useChannelPriority } from "../lib/useChannelPriority";
import {
ArrowLeft,
Ban,
@@ -169,46 +170,21 @@ export default function ChannelPage({
onError: (err) => notifyYouTubeActionError(err, t("channel.fullHistoryFailed")),
});
// Feed priority, editable here with the same stepper the manager uses. Optimistic on this channel's
// detail cache + a debounced flush (~400ms after the last notch) so a press-and-hold sends ONE
// request, not one per step; the pending flush fires on unmount so a last bump isn't lost.
const prioFlush = useRef<ReturnType<typeof setTimeout> | null>(null);
const flushPriority = useCallback(() => {
const latest = qc.getQueryData<ChannelDetail>(qk.channel(channelId))?.priority;
if (latest == null) return;
api
.updateChannel(channelId, { priority: latest })
.then(() => {
qc.invalidateQueries({ queryKey: qk.channels() });
qc.invalidateQueries({ queryKey: qk.feed() });
})
.catch(() => qc.invalidateQueries({ queryKey: qk.channel(channelId) }));
}, [qc, channelId]);
const setPriority = (next: number) => {
qc.setQueryData<ChannelDetail>(qk.channel(channelId), (old) =>
old ? { ...old, priority: next } : old,
);
if (prioFlush.current) clearTimeout(prioFlush.current);
prioFlush.current = setTimeout(() => {
prioFlush.current = null;
flushPriority();
}, 400);
};
// Read the LATEST value from the cache each step (not a render closure) so a hold-repeat accumulates
// correctly, exactly like the manager's arrows.
const bumpPriority = (delta: number) => {
const cur = qc.getQueryData<ChannelDetail>(qk.channel(channelId))?.priority ?? 0;
setPriority(cur + delta);
};
useEffect(
() => () => {
if (prioFlush.current) {
clearTimeout(prioFlush.current);
flushPriority();
}
// Feed priority, editable here with the same stepper + debounced-flush the manager uses (shared
// hook so the two can't drift). Optimistic on this channel's detail cache; a success re-sorts the
// manager list + feed, a failure reverts by refetching the detail.
const { setPriority, bumpPriority } = useChannelPriority({
read: (id) => qc.getQueryData<ChannelDetail>(qk.channel(id))?.priority,
optimistic: (id, next) =>
qc.setQueryData<ChannelDetail>(qk.channel(id), (old) =>
old ? { ...old, priority: next } : old,
),
onSuccess: () => {
qc.invalidateQueries({ queryKey: qk.channels() });
qc.invalidateQueries({ queryKey: qk.feed() });
},
[flushPriority],
);
onError: (id) => qc.invalidateQueries({ queryKey: qk.channel(id) }),
});
// Personal tags, editable right here (the same control + flow the channel manager uses). Optimistic
// on this channel's detail cache; a failed attach/detach reverts by refetching, and a success
@@ -423,15 +399,12 @@ export default function ChannelPage({
</button>
)}
{ch?.subscribed && !me.is_demo && (
<div
className="inline-flex items-center gap-1.5 h-9 px-2.5 rounded-lg border border-border"
title={t("channels.row.priorityHint")}
>
<div className="inline-flex items-center gap-1.5 h-9 px-2.5 rounded-lg border border-border">
<span className="text-xs text-muted">{t("channels.cols.priority")}</span>
<PriorityStepper
priority={ch.priority}
onPriority={bumpPriority}
onSet={setPriority}
onPriority={(d) => bumpPriority(channelId, d)}
onSet={(v) => setPriority(channelId, v)}
/>
</div>
)}
+14 -50
View File
@@ -28,6 +28,7 @@ import ChannelLayoutGrid from "./ChannelLayoutGrid";
import { LoadingState, StateMessage } from "./QueryState";
import { ChannelTagsControl } from "./ChannelTags";
import { useChannelTagActions } from "../lib/useChannelTagActions";
import { useChannelPriority } from "../lib/useChannelPriority";
import ViewSwitcher, { type ViewOption } from "./ViewSwitcher";
import ColumnSortControl from "./ColumnSortControl";
import Pager from "./Pager";
@@ -143,56 +144,19 @@ export default function Channels() {
qc.invalidateQueries({ queryKey: qk.feedCount() });
},
});
// Priority is updated optimistically in place and NOT re-sorted/refetched, so the list
// doesn't jump (the server list is priority-sorted; refetching would reorder rows and
// lose your scroll position). The new order takes effect next time the page loads.
// Press-and-hold fires many bumps, so the PATCH is DEBOUNCED per channel: each bump reads the
// accumulated value from the cache, and the server call flushes once ~400ms after the last step
// (instead of one request per notch). Pending flushes are fired on unmount so a last-moment bump
// isn't lost.
const prioFlush = useRef<Map<string, ReturnType<typeof setTimeout>>>(new Map());
const flushPriority = (id: string) => {
const latest = qc
.getQueryData<ManagedChannel[]>(qk.channels())
?.find((c) => c.id === id)?.priority;
if (latest == null) return;
api
.updateChannel(id, { priority: latest })
.catch(() => qc.invalidateQueries({ queryKey: qk.channels() }));
};
// Set an absolute priority (used by both the arrows — via bumpPriority — and the inline edit),
// optimistically + a debounced flush.
const setPriority = (id: string, next: number) => {
qc.setQueryData<ManagedChannel[]>(qk.channels(), (old) =>
(old ?? []).map((ch) => (ch.id === id ? { ...ch, priority: next } : ch)),
);
const timers = prioFlush.current;
const pending = timers.get(id);
if (pending) clearTimeout(pending);
timers.set(
id,
setTimeout(() => {
timers.delete(id);
flushPriority(id);
}, 400),
);
};
const bumpPriority = (id: string, delta: number) => {
const cur =
qc.getQueryData<ManagedChannel[]>(qk.channels())?.find((c) => c.id === id)?.priority ?? 0;
setPriority(id, cur + delta);
};
useEffect(() => {
const timers = prioFlush.current;
return () => {
timers.forEach((timer, id) => {
clearTimeout(timer);
flushPriority(id);
});
timers.clear();
};
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);
// Priority is updated optimistically in place and NOT re-sorted/refetched, so the list doesn't
// jump (the server list is priority-sorted; refetching would reorder rows and lose your scroll
// position). The new order takes effect next time the page loads. Press-and-hold is debounced +
// flushed on unmount by the shared hook (same flow the channel detail page uses).
const { setPriority, bumpPriority } = useChannelPriority({
read: (id) =>
qc.getQueryData<ManagedChannel[]>(qk.channels())?.find((c) => c.id === id)?.priority,
optimistic: (id, next) =>
qc.setQueryData<ManagedChannel[]>(qk.channels(), (old) =>
(old ?? []).map((ch) => (ch.id === id ? { ...ch, priority: next } : ch)),
),
onError: () => qc.invalidateQueries({ queryKey: qk.channels() }),
});
// Tagging is updated optimistically in place (no refetch of the whole channel list, which
// made the toggle feel ~2s slow); only revert by refetching if the server call fails. The
+12 -27
View File
@@ -17,9 +17,9 @@ import Tooltip from "./Tooltip";
import LanguageSwitcher from "./LanguageSwitcher";
import { PopoverSurface, useAnchoredPopover } from "./AnchoredPopover";
// Primary app navigation: a collapsible left rail with icon+label entries (Design C). The
// modules used to hide under the avatar dropdown; they now live here. Collapsed, it becomes
// a thin icon-only rail. Account actions sit at the bottom in a popover.
// Primary app navigation: a thin icon-only left rail. The modules used to hide under the avatar
// dropdown; they now live here, each revealing its name in a hover/focus flyout label. Account
// actions sit at the bottom in a popover.
export default function NavSidebar({
me,
onOpenAbout,
@@ -34,10 +34,10 @@ export default function NavSidebar({
const { t } = useTranslation();
const { page } = useNavigation();
const { setPage } = useNavigationActions();
// The rail is ALWAYS slim: a thin icon-only column flush against the left edge. There is no
// expanded/pinned mode anymore — instead each icon reveals a floating name label on hover (a
// rest-to-show flyout via Tooltip), so the names are one glance away without the rail ever growing.
const slim = true;
// The rail is ALWAYS a thin icon-only column flush against the left edge. There is no
// expanded/pinned mode anymore — instead each icon reveals a floating name label on hover or
// keyboard focus (a rest-to-show flyout via Tooltip), so the names are one glance away without the
// rail ever growing.
const [acctOpen, setAcctOpen] = useState(false);
// The account panel holds mixed content with its own tab order (profile, account switches, two
// actions), so it is a `dialog` popover, not a roving menu. It portals out of the nav — whose
@@ -198,8 +198,6 @@ export default function NavSidebar({
};
return (
// Layout slot: reserves only the slim footprint when unpinned, so the expanded rail (below)
// overlays content instead of pushing it. Pinned reserves the full width (in-flow).
// Slim icon rail, flush against the left viewport edge (only top/bottom margin), never grows.
// The slot is exactly the card's width so the filter tab/bar docks flush against its right edge.
<div className="relative shrink-0 w-14">
@@ -213,28 +211,15 @@ export default function NavSidebar({
style={listFade.style}
>
{userItems.map(renderItem)}
{systemItems.length > 0 &&
(slim ? (
// Collapsed rail: a short, thicker centred rule is the clearest "new section" cue
// when there's no room for a label.
<div className="my-2 mx-auto w-7 border-t-2 border-border" />
) : (
<div className="mt-4 mb-1 px-2.5 flex items-center gap-2">
<span className="text-3xs font-semibold uppercase tracking-wider text-muted/70">
{t("nav.adminSection")}
</span>
<span className="flex-1 border-t border-border" />
</div>
))}
{systemItems.length > 0 && (
// A short, thicker centred rule is the clearest "new section" cue on an icon-only rail.
<div className="my-2 mx-auto w-7 border-t-2 border-border" />
)}
{systemItems.map(renderItem)}
</div>
<div className="mt-2 pt-2 border-t border-border flex flex-col gap-1">
<div
className={`flex items-center justify-center gap-2 pb-2 mb-1 border-b border-border ${
slim ? "flex-col" : "flex-row"
}`}
>
<div className="flex flex-col items-center justify-center gap-2 pb-2 mb-1 border-b border-border">
<LanguageSwitcher variant="rail" value={language} onChange={onChangeLanguage} />
<button
onClick={onOpenAbout}
-1
View File
@@ -39,7 +39,6 @@ export const LS = {
auditTable: "siftlode.auditTable",
statsTab: "siftlode.statsTab",
adminUsersTab: "siftlode.adminUsersTab",
navCollapsed: "siftlode.navCollapsed",
filterCollapsed: "siftlode.filterCollapsed",
playlist: "siftlode.playlist",
plSort: "siftlode.plSort",
+79
View File
@@ -0,0 +1,79 @@
import { useCallback, useEffect, useRef } from "react";
import { api } from "./api";
/** The shared feed-priority flow used by both the channel manager and the channel detail page:
* an optimistic bump/set with a debounced server flush (~400ms after the last notch) so a
* press-and-hold sends ONE request, not one per step, and pending flushes fire on unmount so a
* last-moment bump isn't lost. The cache-shape-specific bits are injected (the manager maps over a
* ManagedChannel[]; the detail patches one ChannelDetail), so the two call sites can't drift — the
* same drift the sibling `useChannelTagActions` hook prevents for tags.
*
* Timers are keyed by channel id, so a pending flush survives the caller switching channels: it
* fires later and reads the right cache via `read(id)`, rather than needing the caller to flush on
* every id change. `read`/`optimistic`/`onSuccess`/`onError` are read through a ref so the returned
* callbacks stay stable — the unmount flush then runs only on unmount, never on every render. */
export function useChannelPriority(opts: {
/** Current priority for this channel from whichever cache the caller owns (undefined if absent). */
read: (channelId: string) => number | undefined;
/** Apply the optimistic priority to the caller's cache. */
optimistic: (channelId: string, next: number) => void;
/** Called after the server write lands (e.g. invalidate sibling caches). Optional. */
onSuccess?: (channelId: string) => void;
/** Called if the server write fails — revert by refetching the owning cache. */
onError: (channelId: string) => void;
/** Debounce window in ms; defaults to 400. */
delay?: number;
}) {
const timers = useRef<Map<string, ReturnType<typeof setTimeout>>>(new Map());
const optsRef = useRef(opts);
optsRef.current = opts;
const flush = useCallback((channelId: string) => {
const latest = optsRef.current.read(channelId);
if (latest == null) return;
api
.updateChannel(channelId, { priority: latest })
.then(() => optsRef.current.onSuccess?.(channelId))
.catch(() => optsRef.current.onError(channelId));
}, []);
// Set an absolute priority (used by both the arrows — via bumpPriority — and the inline edit),
// optimistically + a debounced flush.
const setPriority = useCallback(
(channelId: string, next: number) => {
optsRef.current.optimistic(channelId, next);
const pending = timers.current.get(channelId);
if (pending) clearTimeout(pending);
timers.current.set(
channelId,
setTimeout(() => {
timers.current.delete(channelId);
flush(channelId);
}, optsRef.current.delay ?? 400),
);
},
[flush],
);
// Read the LATEST value from the cache each step (not a render closure) so a hold-repeat
// accumulates correctly.
const bumpPriority = useCallback(
(channelId: string, delta: number) => {
setPriority(channelId, (optsRef.current.read(channelId) ?? 0) + delta);
},
[setPriority],
);
useEffect(() => {
const pending = timers.current;
return () => {
pending.forEach((timer, channelId) => {
clearTimeout(timer);
flush(channelId);
});
pending.clear();
};
}, [flush]);
return { setPriority, bumpPriority };
}