fix(e10): review round 1 — the boundary that did not bind, and 14 more
The epic-end review found 32 verified issues. This is the first fix round: the three highs, the user-facing mediums, and the tests that were passing for the wrong reason. Docs and the remaining low-severity tail are next. The boundary rule was the worst of them. Both `no-restricted-imports` regexes were written in plain "..." literals, where `\.` is an unknown escape that collapses to the any-char `.` — so the rule read like a boundary and matched far less than one. `../../lib/api` (the barrel every feature component imports) and `../../components/Feed` (the same file `../Feed` names) both sailed through a green gate. String.raw, a third pattern for the second spelling, and a vitest over ESLint.lintText so a rule that silently matches nothing can never look like a clean codebase again. closeThenNavigate waited for exactly one reclaim popstate. With a second Back-closable layer still mounted — a palette opened over the player — the navigation unmounted that layer too, and its reconcile backed over the entry the navigation had just pushed: "go to Channels" closed the player and landed on the feed. It now waits for the LAST programmatic back, then adopts the entries the navigation buried, and falls back after 250ms rather than hanging if no popstate is ever coming. The rail's inline rename died on arrival. With no domReference, floating- ui's returnFocus falls through to a module-global stack and restores some unrelated element from an earlier popover — blurring the field into its own commit-and-close, from the second popover of a session onward. The context menu now owns the restore: back to the row, but only if the row is still there and nothing else has claimed focus. Also: - Backspace under the cheat sheet stopped Plex playback and dropped to the grid; `back` now defers to any layer above, like `escape` beside it. - A move invalidated only the source list, so the target kept serving pre-move contents for 30s while the rail already counted N+1. - Every move error a user can reach is a 404, which `req` leaves to the caller — so the row snapped back in silence and the shipped moveErrors strings were unreachable. - swallowClick armed on long-press but was disarmed only by the click it was armed for; any press that produced no click ate the next tap on the whole list. - The context menu stayed parked over whatever row scrolled under it. - overflow-clip on the chip lanes ate the focus ring's top and bottom. - "Why is this off" rode on disabled rows, which show no tooltip and take no focus: a new `inert` item keeps the reason readable. - Shift+F10 was advertised app-wide but wired only on Playlists; it has its own scope now, and useContextMenu matches through the shared parser instead of its own literals. - Mod+K could not close the palette, `h` machine-gunned on auto-repeat, and both were bound above the branch that mounts their overlays. - Palette options were in the tab order under aria-activedescendant, and a scroll-synthesised mousemove stole the keyboard selection. - The OSD live region was created together with its own text. - Ending a rename stranded focus on <body>. - The Button variants' hover halves are `enabled:` — the one migrated site that had guarded its own lost the guard on the way in. - HU builtinLocked named the list "Watch later" in a Hungarian sentence. Tests: the chip-lane contract counted per FILE, so a second hand-rolled lane in an already-compliant file was invisible; the closeThenNavigate ordering test only passed on a popstate left behind by its neighbours and failed alone, so it moved to its own file.
This commit is contained in:
@@ -78,16 +78,29 @@ export default tseslint.config(
|
||||
"no-restricted-imports": [
|
||||
"error",
|
||||
{
|
||||
// NOTE the String.raw: these are REGEX sources, and in a plain "..." literal `\.` is an
|
||||
// unknown escape that collapses to a bare `.` — the any-char wildcard. The first cut of
|
||||
// this rule shipped that way and matched far less than it read like (E10 review round 1).
|
||||
patterns: [
|
||||
{
|
||||
// `../X` (exactly one level up = a feature component). A `regex` rather than a glob:
|
||||
// eslint matches `patterns` gitignore-style, where `../*` also swallows `../../lib/x`.
|
||||
regex: "^\.\./(?!\.\./)",
|
||||
regex: String.raw`^\.\./(?!\.\./)`,
|
||||
message:
|
||||
"components/ui/ is domain-free: a platform component may not import a feature component. Invert the dependency (take a prop/render-prop) or keep the component outside ui/.",
|
||||
},
|
||||
{
|
||||
regex: "^\.\./\.\./lib/api/",
|
||||
// The same file by its OTHER spelling. `../../components/Feed` resolves exactly where
|
||||
// `../Feed` does, so the rule above is only half the boundary; `ui/` is carved out
|
||||
// because a sibling primitive is the one thing this path may legitimately name.
|
||||
regex: String.raw`^\.\./\.\./components/(?!ui/)`,
|
||||
message:
|
||||
"components/ui/ is domain-free: a platform component may not import a feature component. Invert the dependency (take a prop/render-prop) or keep the component outside ui/.",
|
||||
},
|
||||
{
|
||||
// `(/|$)` so the BARREL counts too: `../../lib/api` is the spelling every feature
|
||||
// component uses, and a `/`-anchored pattern let exactly that one through.
|
||||
regex: String.raw`^\.\./\.\./lib/api(/|$)`,
|
||||
message:
|
||||
"components/ui/ is domain-free: a platform component may not call a domain API slice. Take the data as props.",
|
||||
},
|
||||
|
||||
@@ -59,9 +59,6 @@ export default function App() {
|
||||
const { t, i18n } = useTranslation();
|
||||
const { page, channelView, ytSearch } = useNavigation();
|
||||
const { setPage, enterYtSearch } = useNavigationActions();
|
||||
// The `global` key scope, bound once for the whole app: `h` opens the cheat sheet, Mod+K the
|
||||
// jump-to palette (both rendered at the bottom of this tree).
|
||||
useGlobalKeymap();
|
||||
// An emailed verification link is confirmed HERE, above the signed-in/signed-out branch below, so
|
||||
// it works in a browser that already holds a session. It used to be handled inside Welcome, which
|
||||
// that branch never renders when signed in — the token was silently dropped. Signed out, the
|
||||
@@ -140,6 +137,11 @@ export default function App() {
|
||||
refetchInterval: (query) => (query.state.data ? 60_000 : false),
|
||||
});
|
||||
|
||||
// The `global` key scope: `h` opens the cheat sheet, Mod+K the jump-to palette. Bound only once
|
||||
// signed in, because that is where both overlays are mounted (bottom of this tree) — see
|
||||
// useGlobalKeymap on why binding it above the signed-out branch is worse than not binding it.
|
||||
useGlobalKeymap(!!meQuery.data);
|
||||
|
||||
// Pin this tab to whatever account it first loaded as. Without this, a tab that never picked
|
||||
// an account (it was riding the session's default) would silently swap identity when ANOTHER
|
||||
// tab changes the default — e.g. that other tab adds/switches an account. Once pinned, this
|
||||
|
||||
@@ -214,6 +214,9 @@ function Directory({ onPick, onBack }: { onPick: (u: MessageUser) => void; onBac
|
||||
<Button
|
||||
variant="ghost"
|
||||
size="icon-sm"
|
||||
// An icon-only button needs a name: both of these announced themselves as a bare "button",
|
||||
// and they are the only way out of the sub-view (E10 review round 1).
|
||||
label={t("channel.back")}
|
||||
onClick={onBack}
|
||||
icon={<ArrowLeft className="w-4 h-4" />}
|
||||
/>
|
||||
@@ -281,6 +284,9 @@ function PageThread({
|
||||
<Button
|
||||
variant="ghost"
|
||||
size="icon-sm"
|
||||
// An icon-only button needs a name: both of these announced themselves as a bare "button",
|
||||
// and they are the only way out of the sub-view (E10 review round 1).
|
||||
label={t("channel.back")}
|
||||
onClick={onBack}
|
||||
icon={<ArrowLeft className="w-4 h-4" />}
|
||||
/>
|
||||
|
||||
@@ -39,7 +39,7 @@ import {
|
||||
X,
|
||||
Youtube,
|
||||
} from "lucide-react";
|
||||
import { api, type Playlist, type Video } from "../lib/api";
|
||||
import { api, HttpError, type Playlist, type Video } from "../lib/api";
|
||||
import { notify } from "../lib/notifications";
|
||||
import { notifyYouTubeActionError } from "../lib/youtubeErrors";
|
||||
import { playlistName } from "../lib/playlistName";
|
||||
@@ -516,10 +516,24 @@ export default function Playlists() {
|
||||
level: "success",
|
||||
message: t("playlists.movedToast", { title: video.title, name: plName(target) }),
|
||||
});
|
||||
} catch {
|
||||
// api.req already surfaced the reason; the refetch below puts the row back.
|
||||
} catch (e) {
|
||||
// NOT a silent revert. `req` deliberately leaves 401/403/404 to the caller as control flow, and
|
||||
// every move failure a user can actually reach is a 404 (the 400 `same_playlist` case is already
|
||||
// impossible — the submenu never offers the current list). So the row used to snap back with no
|
||||
// explanation and the shipped moveErrors strings were unreachable (E10 review round 1).
|
||||
// `req` already localized the structured `{code, reason}` into HttpError.detail — this is the
|
||||
// consumer it was always missing.
|
||||
notify({
|
||||
level: "error",
|
||||
message: e instanceof HttpError && e.detail ? e.detail : t("playlists.moveFailed"),
|
||||
});
|
||||
}
|
||||
refreshAll();
|
||||
// BOTH sides changed, and so did this video's membership. `qk.playlist(selectedId)` is an exact
|
||||
// key, so it cannot cover the target — invalidate the whole `["playlist"]` prefix, exactly as
|
||||
// AddToPlaylist does for the same reason.
|
||||
qc.invalidateQueries({ queryKey: qk.playlists() });
|
||||
qc.invalidateQueries({ queryKey: qk.playlist() });
|
||||
qc.invalidateQueries({ queryKey: qk.playlistMembership(video.id) });
|
||||
}
|
||||
|
||||
// Keyboard/menu reordering, for the same stored order the drag handle writes — so it goes through
|
||||
@@ -627,7 +641,11 @@ export default function Playlists() {
|
||||
label: t("playlists.moveTo"),
|
||||
icon: <FolderInput className="w-4 h-4" />,
|
||||
separatorBefore: true,
|
||||
disabled: true,
|
||||
// `inert`, not `disabled`: a natively disabled <button> fires no pointer events
|
||||
// (so the title never shows) and roving focus skips it (so the keyboard never
|
||||
// reaches it) — the reason reached nobody, which is the post-mortem this file
|
||||
// already carries for the drag handle (E10 review round 1).
|
||||
inert: true,
|
||||
title: t("playlists.moveNoTargets"),
|
||||
onSelect: () => {},
|
||||
},
|
||||
@@ -645,7 +663,7 @@ export default function Playlists() {
|
||||
separatorBefore: i === 0,
|
||||
// Same gate as the drag handle, and it explains itself the same way instead of
|
||||
// silently doing nothing.
|
||||
disabled: !storedOrderEditable,
|
||||
inert: !storedOrderEditable,
|
||||
title: storedOrderEditable ? undefined : dragHint,
|
||||
onSelect: () => moveInOrder(ctxRow, to),
|
||||
})),
|
||||
|
||||
@@ -163,9 +163,26 @@ export default function PlaylistsRail({
|
||||
},
|
||||
});
|
||||
|
||||
async function commitRename(pl: Playlist) {
|
||||
const name = renameValue.trim();
|
||||
// Which row to hand focus back to once the inline editor closes. Enter and Escape both unmount the
|
||||
// focused <input> and re-render the row as a <button> that nothing focused, so a keyboard user was
|
||||
// dropped on <body> and lost their place in the rail (E10 review round 1).
|
||||
const refocusIdRef = useRef<number | null>(null);
|
||||
const rowRefs = useRef(new Map<number, HTMLButtonElement | null>());
|
||||
useEffect(() => {
|
||||
const id = refocusIdRef.current;
|
||||
if (renamingId !== null || id === null) return;
|
||||
refocusIdRef.current = null;
|
||||
rowRefs.current.get(id)?.focus();
|
||||
}, [renamingId]);
|
||||
|
||||
function endRename(pl: Playlist, refocus: boolean) {
|
||||
if (refocus) refocusIdRef.current = pl.id;
|
||||
setRenamingId(null);
|
||||
}
|
||||
|
||||
async function commitRename(pl: Playlist, refocus = false) {
|
||||
const name = renameValue.trim();
|
||||
endRename(pl, refocus);
|
||||
if (name && name !== pl.name) await plActions.rename(pl.id, name);
|
||||
}
|
||||
|
||||
@@ -173,7 +190,12 @@ export default function PlaylistsRail({
|
||||
// The built-in Watch later list is renameable/deletable nowhere else in the UI either; the rows
|
||||
// stay visible and disabled so the menu doesn't change shape from list to list.
|
||||
const ctxBuiltin = ctxPl?.kind === "watch_later";
|
||||
const lockedTitle = ctxBuiltin ? t("playlists.builtinLocked") : undefined;
|
||||
// Interpolated, not spelled out: the HU copy said "Watch later", a name the Hungarian UI never
|
||||
// shows for this list ("Megnézem később") — so the message named a list the reader had never seen
|
||||
// (E10 review round 1). Passing the localized name makes the two impossible to drift.
|
||||
const lockedTitle = ctxBuiltin
|
||||
? t("playlists.builtinLocked", { name: t("playlists.watchLater") })
|
||||
: undefined;
|
||||
const railItems: ActionMenuItem[] = !ctxPl
|
||||
? []
|
||||
: [
|
||||
@@ -181,7 +203,9 @@ export default function PlaylistsRail({
|
||||
id: "rename",
|
||||
label: t("playlists.rename"),
|
||||
icon: <Pencil className="w-4 h-4" />,
|
||||
disabled: ctxBuiltin,
|
||||
// `inert`, not `disabled`: the whole point of the row is to carry `lockedTitle`, and a
|
||||
// native disabled button shows no tooltip and takes no focus (E10 review round 1).
|
||||
inert: ctxBuiltin,
|
||||
title: lockedTitle,
|
||||
onSelect: () => {
|
||||
setRenameValue(ctxPl.name);
|
||||
@@ -194,7 +218,7 @@ export default function PlaylistsRail({
|
||||
icon: <Trash2 className="w-4 h-4" />,
|
||||
separatorBefore: true,
|
||||
danger: true,
|
||||
disabled: ctxBuiltin,
|
||||
inert: ctxBuiltin,
|
||||
title: lockedTitle,
|
||||
// The same dialog (and the same "what gets selected next" rule) as the detail header's
|
||||
// trash button — one implementation, two entry points.
|
||||
@@ -293,11 +317,13 @@ export default function PlaylistsRail({
|
||||
autoFocus
|
||||
value={renameValue}
|
||||
onChange={(e) => setRenameValue(e.target.value)}
|
||||
// A blur has already moved focus somewhere the user chose — only the keys need
|
||||
// the row handed back to them.
|
||||
onBlur={() => void commitRename(pl)}
|
||||
onKeyDown={(e) => {
|
||||
if (e.key === "Enter") void commitRename(pl);
|
||||
if (e.key === "Enter") void commitRename(pl, true);
|
||||
// Unmounting the input fires no blur in React, so Escape really does cancel.
|
||||
if (e.key === "Escape") setRenamingId(null);
|
||||
if (e.key === "Escape") endRename(pl, true);
|
||||
}}
|
||||
aria-label={t("playlists.rename")}
|
||||
className="flex-1 min-w-0 bg-surface border border-border rounded-md px-2 py-1 text-xs focus:outline-hidden focus:ring-2 focus:ring-accent/40"
|
||||
@@ -306,7 +332,10 @@ export default function PlaylistsRail({
|
||||
) : (
|
||||
<button
|
||||
key={pl.id}
|
||||
ref={pl.id === selectedId ? selectedRef : null}
|
||||
ref={(el) => {
|
||||
rowRefs.current.set(pl.id, el);
|
||||
if (pl.id === selectedId) selectedRef.current = el;
|
||||
}}
|
||||
onClick={() => setSelectedId(pl.id)}
|
||||
{...railMenu.triggerProps(pl)}
|
||||
className={`${ROW_BASE} text-left ${
|
||||
|
||||
@@ -706,6 +706,13 @@ export default function PlexPlayer({ itemId, onClose, queue }: Props) {
|
||||
stats: () => setStatsOpen((v) => !v),
|
||||
back: (ev) => {
|
||||
// HTPC-style "stop & back to the feed" — mirrors the mouse Back button (shared cascade).
|
||||
// Defer to any layer above us for the SAME reason Escape does, one handler down. Backspace
|
||||
// is the sheet's own "back" too, and without this guard opening the keyboard cheat sheet
|
||||
// over the player and pressing ⌫ stopped playback and dropped to the Plex grid while the
|
||||
// sheet stayed up — Modal stopPropagation()s Escape only, and its focused close button is
|
||||
// not a typing target, so the key sailed through to us (E10 review round 1). The old
|
||||
// `handleBack` had a help-specific branch; a layer check covers every overlay instead.
|
||||
if (layerIdRef.current != null && anyLayerAbove(layerIdRef.current)) return;
|
||||
ev.preventDefault();
|
||||
if (skipProgressRef.current != null)
|
||||
cancelAutoSkipRef.current(); // cancel auto-skip, don't go back
|
||||
@@ -876,17 +883,22 @@ export default function PlexPlayer({ itemId, onClose, queue }: Props) {
|
||||
|
||||
{/* Keyboard track-switch OSD. Sits ABOVE the control bar's own space so it reads even while the
|
||||
bar is showing, and inside the player element so it survives fullscreen. `aria-live` because
|
||||
the whole point is confirming something you cannot otherwise see. */}
|
||||
{osd && (
|
||||
the whole point is confirming something you cannot otherwise see.
|
||||
The REGION is always mounted and only the bubble is conditional: a polite region has to
|
||||
pre-exist in the accessibility tree for a later insertion to be announced, so one created
|
||||
together with its own first message is unreliably read — and since `osd` clears after 2.2s,
|
||||
conditioning the wrapper re-created it for every switch, not just the first (E10 review
|
||||
round 1). Toaster.tsx carries the same rule and the same shape. */}
|
||||
<div
|
||||
className="pointer-events-none absolute inset-x-0 bottom-24 z-menu flex justify-center px-4"
|
||||
aria-live="polite"
|
||||
>
|
||||
{osd && (
|
||||
<div className="max-w-full truncate rounded-full border border-white/15 bg-black/75 px-4 py-2 text-sm text-white shadow-2xl">
|
||||
{osd}
|
||||
</div>
|
||||
</div>
|
||||
)}
|
||||
</div>
|
||||
|
||||
{/* While a panel/menu is open or an auto-skip countdown is running, take over the browser/mouse
|
||||
Back button (history popstate) so it closes/cancels that instead of dropping to the grid. */}
|
||||
|
||||
@@ -22,7 +22,15 @@ interface MenuItemBase {
|
||||
id: string;
|
||||
label: string;
|
||||
icon?: ReactNode; // an element (so a caller can swap in a spinning icon while pending)
|
||||
/** Off because it is BUSY (a mutation in flight): a native `disabled` button, skipped by roving
|
||||
* focus. Use it when there is nothing to explain — the row will be back in a moment. */
|
||||
disabled?: boolean;
|
||||
/** Off because it does not APPLY here (no other list to move to, a built-in list you cannot
|
||||
* rename): reachable but inert — `aria-disabled`, still focusable, still tooltip-capable, and it
|
||||
* no-ops on activation. `disabled` would hide the very explanation the row exists to give, since
|
||||
* a disabled button fires no pointer events (no `title`) and roving focus skips it, so neither a
|
||||
* mouse nor a keyboard could ever read the `title` (E10 review round 1). */
|
||||
inert?: boolean;
|
||||
title?: string; // optional native tooltip (carries a longer hint the label alone can't)
|
||||
/** Draw a divider above this item. Ignored on the first item — a menu never opens with a rule. */
|
||||
separatorBefore?: boolean;
|
||||
@@ -71,13 +79,16 @@ export function MenuItemList({
|
||||
<PopoverItem
|
||||
role="menuitem"
|
||||
disabled={it.disabled}
|
||||
aria-disabled={it.inert || undefined}
|
||||
label={it.label}
|
||||
title={it.title}
|
||||
onSelect={() => {
|
||||
// An inert row is reachable on purpose (so its `title` can be read) but must not act.
|
||||
if (it.inert) return;
|
||||
it.onSelect();
|
||||
onClose();
|
||||
}}
|
||||
className={`${ROW}${it.danger ? " text-danger" : ""}`}
|
||||
className={`${ROW}${it.inert ? " opacity-50" : ""}${it.danger ? " text-danger" : ""}`}
|
||||
>
|
||||
{it.icon && (
|
||||
<span className={`shrink-0 ${it.danger ? "text-danger" : "text-muted"}`}>
|
||||
|
||||
@@ -112,6 +112,19 @@ export interface AnchoredPopoverOptions {
|
||||
* lands the roving focus on it instead of the first item — the behaviour a "pick one of these
|
||||
* modes" menu needs. Leave unset for action menus and multi-select toggle lists. */
|
||||
selectedIndex?: number | null;
|
||||
/** Close when a scroll container under the panel scrolls. Off by default, because a panel anchored
|
||||
* to a real trigger element simply MOVES with it. Turn it on when the anchor is a fixed point
|
||||
* rather than an element — a right-click menu, which otherwise stays parked over whatever row
|
||||
* scrolls under it while its actions still target the row you opened it on (E10 review round 1).
|
||||
* This is also what a native context menu does. */
|
||||
dismissOnScroll?: boolean;
|
||||
/** Hand the focus back to the trigger on close. On by default, and right whenever there IS a
|
||||
* trigger element. Turn it off when the anchor is a virtual point: floating-ui then has no
|
||||
* `domReference` to return to and falls back to a module-global "previously focused" stack, so it
|
||||
* restores some unrelated element from an earlier popover — which, in the rail's inline rename,
|
||||
* stole focus back from the field that had just opened and tore it down again (E10 review round
|
||||
* 1). A caller that turns this off owns the restore. */
|
||||
returnFocus?: boolean;
|
||||
}
|
||||
|
||||
// Internal: call sites use the inferred hook return, they don't name the type.
|
||||
@@ -215,7 +228,10 @@ export function useAnchoredPopover(opts: AnchoredPopoverOptions) {
|
||||
// off entirely. (Leaving it on for hover made an open tooltip's document-level handler either
|
||||
// stopPropagation the Escape away from a Modal/player underneath, or double-close both.) Outside-
|
||||
// press stays with floating-ui (tree-aware).
|
||||
const dismiss = useDismiss(context, { escapeKey: false });
|
||||
const dismiss = useDismiss(context, {
|
||||
escapeKey: false,
|
||||
ancestorScroll: opts.dismissOnScroll ?? false,
|
||||
});
|
||||
// `role:"combobox"` makes Floating UI wire the whole typeable-combobox ARIA tree: the reference input
|
||||
// → role=combobox + aria-expanded/controls/autocomplete + aria-activedescendant (from the virtual nav),
|
||||
// the surface → role=listbox, the items → role=option + their option ids. (`combobox` maps to the
|
||||
@@ -285,6 +301,7 @@ export function useAnchoredPopover(opts: AnchoredPopoverOptions) {
|
||||
roves,
|
||||
open: opts.open,
|
||||
selectedIndex: opts.selectedIndex ?? null,
|
||||
returnFocus: opts.returnFocus ?? true,
|
||||
refs,
|
||||
floatingStyles,
|
||||
context,
|
||||
@@ -509,6 +526,7 @@ function PopoverSurfaceInner({
|
||||
isNav,
|
||||
isCombobox,
|
||||
roves,
|
||||
returnFocus,
|
||||
selectedIndex,
|
||||
activeIndex,
|
||||
highlightIndex,
|
||||
@@ -606,7 +624,7 @@ function PopoverSurfaceInner({
|
||||
// — in an item-less menu (e.g. a first-run tag editor with only a create input) — the
|
||||
// input, so such a menu still has a keyboard entry point instead of stranding focus.
|
||||
initialFocus={initialFocus ?? (isNav && selectedIndex != null ? -1 : undefined)}
|
||||
returnFocus
|
||||
returnFocus={returnFocus}
|
||||
>
|
||||
{positioned}
|
||||
</FloatingFocusManager>
|
||||
|
||||
@@ -43,23 +43,30 @@ const VARIANTS: Record<ButtonVariant, string> = {
|
||||
// (ghost), while this primitive hovered to `fg` and filled with `surface` — so the 37 migrated sites
|
||||
// and the ~100 hand-rolled ones had drifted into two different looks. That split IS the
|
||||
// inconsistency this epic exists to remove; the primitive follows the majority, not the reverse.
|
||||
outline: "font-medium border border-border text-muted hover:text-accent hover:border-accent",
|
||||
"outline-accent": "font-medium border border-accent/40 text-accent hover:bg-accent/10",
|
||||
outline:
|
||||
"font-medium border border-border text-muted enabled:hover:text-accent enabled:hover:border-accent",
|
||||
"outline-accent": "font-medium border border-accent/40 text-accent enabled:hover:bg-accent/10",
|
||||
// Neutral until you reach for it. NOT `danger-soft`, which is already red at rest — the difference
|
||||
// is whether the button should advertise the destruction before the pointer arrives.
|
||||
"outline-danger":
|
||||
"font-medium border border-border text-muted hover:text-danger hover:border-danger/40",
|
||||
"outline-warn": "font-medium border border-warn/40 text-warn hover:bg-warn/10",
|
||||
ghost: "font-medium text-muted hover:text-fg hover:bg-card",
|
||||
glass: "font-medium glass-card glass-hover hover:text-accent",
|
||||
"font-medium border border-border text-muted enabled:hover:text-danger enabled:hover:border-danger/40",
|
||||
"outline-warn": "font-medium border border-warn/40 text-warn enabled:hover:bg-warn/10",
|
||||
ghost: "font-medium text-muted enabled:hover:text-fg enabled:hover:bg-card",
|
||||
glass: "font-medium glass-card glass-hover enabled:hover:text-accent",
|
||||
"danger-soft":
|
||||
"font-medium border border-danger/60 text-danger hover:bg-danger/10 hover:border-danger",
|
||||
"font-medium border border-danger/60 text-danger enabled:hover:bg-danger/10 enabled:hover:border-danger",
|
||||
// No font-weight here on purpose: BASE already sets font-medium, and a second font-* utility would
|
||||
// be a coin-flip (both are single-class utilities of equal specificity, and without tailwind-merge
|
||||
// the winner is decided by the generated stylesheet order, not by the class list order).
|
||||
link: "font-semibold text-accent hover:underline",
|
||||
link: "font-semibold text-accent enabled:hover:underline",
|
||||
};
|
||||
|
||||
// Why every hover above is `enabled:hover:` — Tailwind's `:not(:disabled)`. A disabled button that
|
||||
// still lights up on hover advertises a click that does nothing, and the ONE migrated call site that
|
||||
// had guarded its own hover (TagManager's Add) silently lost the guard when it moved onto this
|
||||
// primitive (E10 review round 1). The contract belongs here, not in ~160 call sites: BASE dims a
|
||||
// disabled button, and the variant must not paint over that dimming.
|
||||
|
||||
// The geometry ladder, and the whole of it — a call site picks a rung, it does not invent one. The
|
||||
// rungs were derived from what the app actually used (counted during E10 S1b): `px-3 py-2 text-sm`
|
||||
// (22 sites), `px-2 py-1 text-xs` (12), `px-3 py-1.5 text-sm` (11). Every value is rem-derived, so
|
||||
|
||||
@@ -5,7 +5,7 @@
|
||||
// (no i18n, no domain), so this asserts the shell only. i18n is key-passthrough.
|
||||
import { useState } from "react";
|
||||
import { describe, it, expect, vi, afterEach, beforeAll } from "vitest";
|
||||
import { render, screen, fireEvent, cleanup, waitFor } from "@testing-library/react";
|
||||
import { render, screen, fireEvent, cleanup } from "@testing-library/react";
|
||||
import CommandPalette, { type CommandGroup } from "./CommandPalette";
|
||||
|
||||
vi.mock("react-i18next", async (importOriginal) => {
|
||||
@@ -149,19 +149,10 @@ describe("CommandPalette", () => {
|
||||
expect(selected()).toBe("ch:1");
|
||||
});
|
||||
|
||||
// The ORDER is the point (closeThenNavigate): the dialog closes and gives its Back-to-close
|
||||
// history entry back BEFORE the command runs, because a command that navigates would otherwise
|
||||
// have its pushState undone by that reclaim.
|
||||
it("closes, and runs the command only once the close has landed", async () => {
|
||||
open();
|
||||
fireEvent.click(screen.getByText("TheVR Podcast"));
|
||||
expect(ran).toEqual(["closed"]);
|
||||
await waitFor(() => expect(ran).toEqual(["closed", "ch:1"]));
|
||||
});
|
||||
|
||||
// Only ONE test here can go through the full close→popstate→run cycle: jsdom gives a file a single
|
||||
// session-history entry, so the first back() consumes it and later ones fire no popstate. Which
|
||||
// command Enter picks is covered by the selection assertions above; this checks it chooses at all.
|
||||
// No test in THIS file goes through the full close→reclaim→run cycle: jsdom gives a file one
|
||||
// session history and history.ts's overlay bookkeeping is module-global, so the cycle only holds
|
||||
// in isolation — it lives in CommandPalette.ordering.dom.test.tsx. Which command Enter picks is
|
||||
// covered by the selection assertions above; this checks it chooses at all.
|
||||
it("chooses the selected row on Enter", () => {
|
||||
open();
|
||||
press("ArrowDown");
|
||||
|
||||
@@ -0,0 +1,67 @@
|
||||
// @vitest-environment jsdom
|
||||
//
|
||||
// ONE test, ONE file, on purpose. This is the close→reclaim→run ordering contract of the palette
|
||||
// (see closeThenNavigate), and it is the only assertion in the suite that drives a real
|
||||
// useBackToClose entry through a real programmatic history.back().
|
||||
//
|
||||
// It cannot live in CommandPalette.dom.test.tsx. jsdom gives a test FILE a single session history,
|
||||
// and history.ts's overlay bookkeeping is module-global, so the sibling tests there leave an
|
||||
// in-flight programmatic back() behind — under which this one either hangs or, worse, goes green on
|
||||
// somebody else's popstate. That is exactly how it used to pass: it asserted nothing on its own, and
|
||||
// running it with `-t` failed (E10 review round 1). Its own file gives it clean module state.
|
||||
import { useState } from "react";
|
||||
import { describe, it, expect, vi, beforeAll } from "vitest";
|
||||
import { render, screen, fireEvent, waitFor, act } from "@testing-library/react";
|
||||
|
||||
import CommandPalette, { type CommandGroup } from "./CommandPalette";
|
||||
|
||||
vi.mock("react-i18next", async (importOriginal) => {
|
||||
const actual = await importOriginal<typeof import("react-i18next")>();
|
||||
return { ...actual, useTranslation: () => ({ t: (k: string) => k }) };
|
||||
});
|
||||
|
||||
beforeAll(() => {
|
||||
// jsdom has no layout, so scrollIntoView is missing on the elements the active-row effect touches.
|
||||
Element.prototype.scrollIntoView ??= () => {};
|
||||
});
|
||||
|
||||
const ran: string[] = [];
|
||||
|
||||
function Harness() {
|
||||
const [isOpen, setIsOpen] = useState(true);
|
||||
const groups: CommandGroup[] = [
|
||||
{
|
||||
id: "channels",
|
||||
label: "Channels",
|
||||
items: [{ id: "ch:1", label: "TheVR Podcast", run: () => ran.push("ch:1") }],
|
||||
},
|
||||
];
|
||||
return (
|
||||
<CommandPalette
|
||||
open={isOpen}
|
||||
onClose={() => {
|
||||
ran.push("closed");
|
||||
setIsOpen(false);
|
||||
}}
|
||||
groups={groups}
|
||||
/>
|
||||
);
|
||||
}
|
||||
|
||||
describe("CommandPalette · closeThenNavigate ordering", () => {
|
||||
it("closes, and runs the command only once the close has landed", async () => {
|
||||
render(<Harness />);
|
||||
// Load-bearing: useBackToClose pushes its entry from a coalesced microtask, so a click in the
|
||||
// same synchronous tick as the render finds no entry to reclaim — no back(), no popstate.
|
||||
await act(async () => {
|
||||
await Promise.resolve();
|
||||
});
|
||||
|
||||
fireEvent.click(screen.getByText("TheVR Podcast"));
|
||||
expect(ran).toEqual(["closed"]);
|
||||
|
||||
// Inside closeThenNavigate's 250ms no-popstate fallback, so a pass means the RECLAIM ran the
|
||||
// command — not the safety net that exists for overlays which never got an entry at all.
|
||||
await waitFor(() => expect(ran).toEqual(["closed", "ch:1"]), { timeout: 200 });
|
||||
});
|
||||
});
|
||||
@@ -64,6 +64,8 @@ export default function CommandPalette({
|
||||
// Ref on the highlighted row rather than a selector lookup: item ids come from domain data
|
||||
// (playlist names, channel handles) and would need escaping to be safe in a query string.
|
||||
const activeRef = useRef<HTMLButtonElement | null>(null);
|
||||
// Last pointer position, so a mousemove the BROWSER synthesised can be told from one the user made.
|
||||
const lastPointer = useRef<{ x: number; y: number } | null>(null);
|
||||
|
||||
// A fresh open starts from a blank field: the palette is for "take me somewhere", and a leftover
|
||||
// query from last time hides everything you might want now.
|
||||
@@ -158,8 +160,22 @@ export default function CommandPalette({
|
||||
role="option"
|
||||
aria-selected={item.id === activeId}
|
||||
type="button"
|
||||
// The combobox input owns the tab stop; the arrows move a VIRTUAL selection over
|
||||
// these rows. Leaving them tabbable put DOM focus on a row while
|
||||
// aria-activedescendant named a different one — two competing "current" options —
|
||||
// and made Tab walk every result (E10 review round 1).
|
||||
tabIndex={-1}
|
||||
onClick={() => choose(item)}
|
||||
onMouseMove={() => setActive(flat.indexOf(item))}
|
||||
onMouseMove={(e) => {
|
||||
// Only a pointer that actually MOVED may take the selection. After the arrow keys
|
||||
// scroll a row past the fold, Blink re-dispatches a mousemove at the unchanged
|
||||
// cursor position to refresh hover — which handed the highlight straight back to
|
||||
// whatever row slid under a resting cursor (E10 review round 1).
|
||||
const prev = lastPointer.current;
|
||||
lastPointer.current = { x: e.clientX, y: e.clientY };
|
||||
if (prev && prev.x === e.clientX && prev.y === e.clientY) return;
|
||||
setActive(flat.indexOf(item));
|
||||
}}
|
||||
className={`flex w-full items-center gap-2 rounded-lg px-2 py-1.5 text-left text-sm ${
|
||||
item.id === activeId ? "bg-card text-fg" : "text-muted hover:text-fg"
|
||||
}`}
|
||||
|
||||
@@ -11,6 +11,7 @@ import {
|
||||
useGlobalKeymap,
|
||||
useHelpConditions,
|
||||
useKeyScope,
|
||||
type KeyScope,
|
||||
} from "../../lib/keymap";
|
||||
|
||||
vi.mock("react-i18next", async (importOriginal) => {
|
||||
@@ -37,8 +38,8 @@ function Host({ children }: { children?: React.ReactNode }) {
|
||||
}
|
||||
|
||||
/** A stand-in for a surface that registers a scope while mounted, the way the players do. */
|
||||
function Surface({ episode }: { episode?: boolean }) {
|
||||
useKeyScope("plexPlayer");
|
||||
function Surface({ episode, scope = "plexPlayer" }: { episode?: boolean; scope?: KeyScope }) {
|
||||
useKeyScope(scope);
|
||||
useHelpConditions({ episode: !!episode });
|
||||
return null;
|
||||
}
|
||||
@@ -81,13 +82,27 @@ describe("KeyboardHelp", () => {
|
||||
const rows = descriptions();
|
||||
// A plexPlayer row first, the app-wide rows last — the reading order the reader wants.
|
||||
expect(rows[0]).toBe("keys.playPause");
|
||||
expect(rows.slice(-3)).toEqual(["keys.palette", "keys.help", "keys.contextMenu"]);
|
||||
expect(rows.slice(-2)).toEqual(["keys.palette", "keys.help"]);
|
||||
});
|
||||
|
||||
it("lists only the app-wide keys when no surface is registered", () => {
|
||||
render(<Host />);
|
||||
pressH();
|
||||
expect(descriptions()).toEqual(["keys.palette", "keys.help", "keys.contextMenu"]);
|
||||
expect(descriptions()).toEqual(["keys.palette", "keys.help"]);
|
||||
});
|
||||
|
||||
// The context menu is NOT app-wide: it has its own scope, registered by `useContextMenu`, so the
|
||||
// sheet only offers Shift+F10 where something actually answers it. Advertising it globally was a
|
||||
// promise the feed, Plex, Downloads and Settings did not keep (E10 review round 1) — the test above
|
||||
// pins the other half, that a bare app lists the two global keys and nothing else.
|
||||
it("offers the context-menu key on a surface that has one", () => {
|
||||
render(
|
||||
<Host>
|
||||
<Surface scope="contextMenu" />
|
||||
</Host>,
|
||||
);
|
||||
pressH();
|
||||
expect(descriptions()).toContain("keys.contextMenu");
|
||||
});
|
||||
|
||||
// "Previous / next episode" inside a movie would be a promise the player does not keep.
|
||||
|
||||
@@ -19,7 +19,10 @@ export default function UndoToolbar({
|
||||
onRedo: () => void;
|
||||
}) {
|
||||
const { t } = useTranslation();
|
||||
useKeyScope("undo");
|
||||
// Only while a key actually DOES something: the sheet must not advertise Ctrl+Z on a page where
|
||||
// nothing has been done yet and the handler returns immediately (E10 review round 1) — the same
|
||||
// honest-states gate PlexBrowse uses for its Backspace.
|
||||
useKeyScope("undo", canUndo || canRedo);
|
||||
|
||||
useEffect(() => {
|
||||
function onKey(e: KeyboardEvent) {
|
||||
|
||||
@@ -1,4 +1,6 @@
|
||||
import { useCallback, useRef, useState, type PointerEvent as ReactPointerEvent } from "react";
|
||||
|
||||
import { matchesShortcut, useKeyScope } from "../../lib/keymap";
|
||||
import { useAnchoredPopover } from "./AnchoredPopover";
|
||||
|
||||
// E10 S2 — the right-click context menu's INPUT half: the gestures that open it and the row they
|
||||
@@ -60,6 +62,10 @@ export interface ContextMenuController<T> {
|
||||
}
|
||||
|
||||
export function useContextMenu<T>(): ContextMenuController<T> {
|
||||
// Register the scope HERE, so the cheat sheet lists Shift+F10 exactly on the surfaces that answer it.
|
||||
useKeyScope("contextMenu");
|
||||
// `onOpenChange` is built before `restoreFocus` exists; a ref keeps the order from mattering.
|
||||
const restoreFocusRef = useRef<() => void>(() => {});
|
||||
const [target, setTarget] = useState<T | null>(null);
|
||||
const [open, setOpen] = useState(false);
|
||||
const pop = useAnchoredPopover({
|
||||
@@ -67,31 +73,70 @@ export function useContextMenu<T>(): ContextMenuController<T> {
|
||||
onOpenChange: (next) => {
|
||||
setOpen(next);
|
||||
// Drop the target on close so a stale row can't drive a re-open's first paint.
|
||||
if (!next) setTarget(null);
|
||||
if (!next) {
|
||||
setTarget(null);
|
||||
restoreFocusRef.current();
|
||||
}
|
||||
},
|
||||
// A native context menu drops down-and-right from the pointer; flip()/shift() take it the other
|
||||
// way near an edge.
|
||||
placement: "bottom-start",
|
||||
offset: 0,
|
||||
fitHeight: true,
|
||||
// The anchor is a fixed POINT, not an element, so scrolling cannot move the panel with its row —
|
||||
// it would sit over a different row than the one its actions target. Native menus close; so do we.
|
||||
dismissOnScroll: true,
|
||||
// Same reason: with no `domReference`, floating-ui's own returnFocus restores an unrelated
|
||||
// element off its module-global stack. We know the actual row, so we hand focus back ourselves.
|
||||
returnFocus: false,
|
||||
});
|
||||
|
||||
// The row the open menu belongs to, so focus can go back where it came from on close.
|
||||
const triggerEl = useRef<Element | null>(null);
|
||||
|
||||
const { setPositionReference } = pop.refs;
|
||||
const openAt = useCallback(
|
||||
(x: number, y: number, payload: T, contextElement?: Element) => {
|
||||
setPositionReference({ getBoundingClientRect: () => pointRect(x, y), contextElement });
|
||||
triggerEl.current = contextElement ?? null;
|
||||
setTarget(payload);
|
||||
setOpen(true);
|
||||
},
|
||||
[setPositionReference],
|
||||
);
|
||||
|
||||
/** Hand focus back to the row — but only if that is still the right thing to do.
|
||||
*
|
||||
* After the commit, because both conditions are about the world the close produced. A row that is
|
||||
* simply still there gets its focus back (the keyboard user is where they were). A row that has
|
||||
* been REPLACED keeps whatever replaced it: the rail swaps in an inline rename field on this very
|
||||
* path, and blindly restoring is what used to blur that field into its own commit-and-close (E10
|
||||
* review round 1). Likewise, an action that deliberately moved focus somewhere else keeps it. */
|
||||
const restoreFocus = useCallback(() => {
|
||||
const el = triggerEl.current;
|
||||
triggerEl.current = null;
|
||||
if (!el) return;
|
||||
queueMicrotask(() => {
|
||||
if (!el.isConnected) return;
|
||||
const active = document.activeElement;
|
||||
if (active && active !== document.body && !el.contains(active)) return;
|
||||
if (el instanceof HTMLElement) el.focus();
|
||||
});
|
||||
}, []);
|
||||
|
||||
// Long-press bookkeeping. Refs, not state: none of it paints, and a re-render per pointermove
|
||||
// during a scroll would be a real cost on the device this path exists for.
|
||||
const timer = useRef<number | null>(null);
|
||||
const startedAt = useRef<{ x: number; y: number } | null>(null);
|
||||
// A long-press is followed by a click on release. Without this the menu would open and the row's
|
||||
// own default action (play / select) would fire underneath it.
|
||||
//
|
||||
// It is armed by the timer and disarmed by the click it was armed FOR — but plenty of long presses
|
||||
// never produce that click (a pan ends in pointercancel, the finger lifts over the portalled menu,
|
||||
// Android Chrome suppresses the trailing click after its own contextmenu). The flag then stayed
|
||||
// armed, and since it is one ref per LIST, the next genuine tap on ANY row of that list was eaten
|
||||
// and the row looked dead until tapped twice (E10 review round 1). So a fresh press also disarms
|
||||
// it: whatever the last gesture left behind cannot outlive the start of the next one.
|
||||
const swallowClick = useRef(false);
|
||||
const cancelPress = useCallback(() => {
|
||||
if (timer.current !== null) window.clearTimeout(timer.current);
|
||||
@@ -111,6 +156,9 @@ export function useContextMenu<T>(): ContextMenuController<T> {
|
||||
openAt(e.clientX, e.clientY, payload, e.currentTarget);
|
||||
},
|
||||
onPointerDown: (e: ReactPointerEvent<HTMLElement>) => {
|
||||
// Before the pointerType guard: a stale arm left by an earlier touch press must not survive
|
||||
// into an unrelated interaction, mouse click included.
|
||||
swallowClick.current = false;
|
||||
// Mouse has a right button; only touch/pen need the timer.
|
||||
if (e.pointerType === "mouse") return;
|
||||
const { clientX: x, clientY: y } = e;
|
||||
@@ -132,7 +180,12 @@ export function useContextMenu<T>(): ContextMenuController<T> {
|
||||
cancelPress(); // they are scrolling, not pressing
|
||||
},
|
||||
onPointerUp: cancelPress,
|
||||
onPointerCancel: cancelPress,
|
||||
onPointerCancel: () => {
|
||||
// The gesture was taken over (a pan, the browser's own menu): no click is coming, so the
|
||||
// arm has nothing left to swallow and must not wait around for someone else's.
|
||||
swallowClick.current = false;
|
||||
cancelPress();
|
||||
},
|
||||
onClickCapture: (e: React.MouseEvent<HTMLElement>) => {
|
||||
if (!swallowClick.current) return;
|
||||
swallowClick.current = false;
|
||||
@@ -142,7 +195,9 @@ export function useContextMenu<T>(): ContextMenuController<T> {
|
||||
onKeyDown: (e: React.KeyboardEvent<HTMLElement>) => {
|
||||
// The keyboard route every desktop menu has. Anchored to the row's bottom-left rather than a
|
||||
// pointer that isn't there.
|
||||
if (e.key !== "ContextMenu" && !(e.key === "F10" && e.shiftKey)) return;
|
||||
// Through the shared parser, against the table's own row: the chords live in ONE place, and
|
||||
// `matches` is exact on modifiers (the hand-rolled version answered Ctrl+Shift+F10 too).
|
||||
if (!matchesShortcut(e.nativeEvent, "contextMenu")) return;
|
||||
e.preventDefault();
|
||||
const r = e.currentTarget.getBoundingClientRect();
|
||||
openAt(r.left, r.bottom, payload, e.currentTarget);
|
||||
@@ -151,5 +206,17 @@ export function useContextMenu<T>(): ContextMenuController<T> {
|
||||
[openAt, cancelPress],
|
||||
);
|
||||
|
||||
return { target, open, close: () => setOpen(false), triggerProps, popover: pop };
|
||||
restoreFocusRef.current = restoreFocus;
|
||||
|
||||
return {
|
||||
target,
|
||||
open,
|
||||
close: () => {
|
||||
setOpen(false);
|
||||
setTarget(null);
|
||||
restoreFocus();
|
||||
},
|
||||
triggerProps,
|
||||
popover: pop,
|
||||
};
|
||||
}
|
||||
|
||||
@@ -84,7 +84,7 @@
|
||||
"orderDown": "Move down",
|
||||
"orderBottom": "Move to bottom",
|
||||
"railMenu": "Playlist actions",
|
||||
"builtinLocked": "The built-in Watch later list can't be renamed or deleted.",
|
||||
"builtinLocked": "The built-in {{name}} list can’t be renamed or deleted.",
|
||||
"removeItem": "Remove",
|
||||
"removeItemOf": "Remove “{{title}}” from the playlist",
|
||||
"confirmRemove": "Remove “{{title}}” from this playlist?",
|
||||
|
||||
@@ -84,7 +84,7 @@
|
||||
"orderDown": "Eggyel lejjebb",
|
||||
"orderBottom": "Legalulra",
|
||||
"railMenu": "Lejátszási lista műveletei",
|
||||
"builtinLocked": "A beépített Watch later lista nem nevezhető át és nem törölhető.",
|
||||
"builtinLocked": "A beépített „{{name}}” lista nem nevezhető át és nem törölhető.",
|
||||
"removeItem": "Eltávolítás",
|
||||
"removeItemOf": "„{{title}}” eltávolítása a listáról",
|
||||
"confirmRemove": "Eltávolítod a(z) „{{title}}” videót erről a listáról?",
|
||||
|
||||
@@ -11,38 +11,60 @@ import { chipLaneClass } from "./useChipOverflowFit";
|
||||
// contains it — so the invariant worth locking is not "this lane looks right" but "every mirror lives
|
||||
// inside a lane that clips". A fifth lane hand-rolling `relative` is exactly the drift this catches,
|
||||
// and no DOM test can: jsdom has no layout, so the overflow it guards against is unobservable there.
|
||||
//
|
||||
// COUNTED per occurrence, not tested per file. The first cut asked `src.includes(chipLaneClass)`, which
|
||||
// a file could satisfy with ONE compliant lane while hand-rolling a second — and ChannelTags already
|
||||
// holds two lanes and two mirrors, so the likeliest place for a fifth lane was the one file the check
|
||||
// was blind inside (E10 review round 1).
|
||||
const MIRROR = "invisible absolute left-0 top-0";
|
||||
// A mirror only exists to be measured, so the hook is the other half of the signature. Discovering by
|
||||
// BOTH means a mirror written with the utilities in another order is still caught by its import.
|
||||
const HOOK = "useChipOverflowFit";
|
||||
|
||||
function componentSources(): Array<[string, string]> {
|
||||
const dir = new URL("../components/", import.meta.url);
|
||||
function count(haystack: string, needle: string): number {
|
||||
return haystack.split(needle).length - 1;
|
||||
}
|
||||
|
||||
/** Every source file under src/, not just src/components/ — App.tsx and main.tsx sit above it. */
|
||||
function sources(): Array<[string, string]> {
|
||||
const walk = (u: URL, prefix: string): Array<[string, string]> =>
|
||||
readdirSync(u, { withFileTypes: true }).flatMap((e) => {
|
||||
if (e.isDirectory()) return walk(new URL(`${e.name}/`, u), `${prefix}${e.name}/`);
|
||||
if (!e.name.endsWith(".tsx") || e.name.includes(".test.")) return [];
|
||||
if (!/\.tsx?$/.test(e.name) || e.name.includes(".test.")) return [];
|
||||
return [
|
||||
[`${prefix}${e.name}`, readFileSync(new URL(e.name, u), "utf-8")] as [string, string],
|
||||
];
|
||||
});
|
||||
return walk(dir, "");
|
||||
return walk(new URL("../", import.meta.url), "");
|
||||
}
|
||||
|
||||
/** Files that render a measurer, by either half of its signature. */
|
||||
function withMirror(): Array<[string, string]> {
|
||||
return sources().filter(
|
||||
([name, src]) =>
|
||||
!name.startsWith("lib/useChipOverflowFit") && (src.includes(MIRROR) || src.includes(HOOK)),
|
||||
);
|
||||
}
|
||||
|
||||
describe("chip lane containment contract", () => {
|
||||
it("keeps both halves of the lane class — the offsetParent AND the clip", () => {
|
||||
it("keeps all three halves of the lane class — offsetParent, clip, and room for the focus ring", () => {
|
||||
expect(chipLaneClass.split(/\s+/)).toEqual(
|
||||
expect.arrayContaining(["relative", "overflow-clip"]),
|
||||
expect.arrayContaining(["relative", "overflow-clip", "[overflow-clip-margin:0.25rem]"]),
|
||||
);
|
||||
});
|
||||
|
||||
it("every component rendering a hidden chip measurer uses chipLaneClass for its lane", () => {
|
||||
const offenders = componentSources()
|
||||
.filter(([, src]) => src.includes(MIRROR))
|
||||
.filter(([, src]) => !src.includes("chipLaneClass"))
|
||||
.map(([name]) => name);
|
||||
it("gives every hidden chip measurer its own chipLaneClass lane", () => {
|
||||
const offenders = withMirror()
|
||||
.map(([name, src]) => ({
|
||||
name,
|
||||
mirrors: count(src, MIRROR),
|
||||
lanes: count(src, "chipLaneClass"),
|
||||
}))
|
||||
.filter((f) => f.lanes < f.mirrors);
|
||||
expect(offenders).toEqual([]);
|
||||
});
|
||||
|
||||
it("finds the measurers it claims to guard (the marker has not been renamed away)", () => {
|
||||
const withMirror = componentSources().filter(([, src]) => src.includes(MIRROR));
|
||||
expect(withMirror.length).toBeGreaterThanOrEqual(3);
|
||||
expect(withMirror().length).toBeGreaterThanOrEqual(3);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -111,4 +111,55 @@ describe("closeThenNavigate", () => {
|
||||
window.dispatchEvent(new PopStateEvent("popstate"));
|
||||
expect(navigate).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
// E10 review round 1. Waiting for ONE reclaim is not enough when a SECOND Back-closable layer is
|
||||
// still mounted and the navigation is what unmounts it: that layer's own reconcile fired another
|
||||
// history.back(), which popped the entry the navigation had just pushed and bounced the user back
|
||||
// where they started (palette opened over the player → "go to Channels" landed on the feed).
|
||||
//
|
||||
// NOTE this one may NOT use the raw-popstate shortcut the rest of the file uses: a synthetic
|
||||
// popstate is read as a GENUINE Back, which pops the lower layer off the stack and decrements
|
||||
// historyDepth by itself — so the test would pass with or without the fix. The bug only appears
|
||||
// on the programmatic path, so back() is stubbed to land its own popstate instead.
|
||||
it("adopts the entries of layers the navigation itself unmounts", async () => {
|
||||
const back = vi.spyOn(window.history, "back").mockImplementation(() => {
|
||||
window.dispatchEvent(new PopStateEvent("popstate", { state: window.history.state }));
|
||||
});
|
||||
try {
|
||||
const player = renderHook(() => useBackToClose(() => {}));
|
||||
await settle();
|
||||
const palette = renderHook(() => useBackToClose(() => {}));
|
||||
await settle();
|
||||
expect(back).not.toHaveBeenCalled(); // both layers hold an entry; nothing reclaimed yet
|
||||
|
||||
const navigate = vi.fn(() => window.history.pushState({ sfPage: "channels" }, ""));
|
||||
await act(async () => {
|
||||
closeThenNavigate(() => palette.unmount(), navigate);
|
||||
await Promise.resolve(); // let the palette's own reclaim reconcile run
|
||||
});
|
||||
expect(navigate).toHaveBeenCalledTimes(1);
|
||||
expect(back).toHaveBeenCalledTimes(1); // the palette's entry, and only that
|
||||
|
||||
player.unmount(); // …which is what the navigation does to the layer below
|
||||
await settle();
|
||||
expect(back).toHaveBeenCalledTimes(1); // must NOT back over the entry navigate just pushed
|
||||
} finally {
|
||||
back.mockRestore();
|
||||
}
|
||||
});
|
||||
|
||||
// Without this the command is not late, it never happens: an overlay closed in the same tick it
|
||||
// opened never pushed an entry, so no reclaim popstate is ever coming.
|
||||
it("navigates anyway when no reclaim popstate ever arrives", () => {
|
||||
vi.useFakeTimers();
|
||||
try {
|
||||
const navigate = vi.fn();
|
||||
closeThenNavigate(() => {}, navigate);
|
||||
expect(navigate).not.toHaveBeenCalled();
|
||||
vi.advanceTimersByTime(300);
|
||||
expect(navigate).toHaveBeenCalledTimes(1);
|
||||
} finally {
|
||||
vi.useRealTimers();
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
@@ -200,11 +200,34 @@ function ensureOverlayListener(): void {
|
||||
* Only call this from INSIDE an overlay that uses `useBackToClose`: the popstate we wait for is
|
||||
* that overlay's own entry being reclaimed, and without one it never arrives. */
|
||||
export function closeThenNavigate(close: () => void, navigate: () => void): void {
|
||||
const once = () => {
|
||||
window.removeEventListener("popstate", once);
|
||||
let done = false;
|
||||
// Armed BEFORE the close, so it is a plain const and there is no declaration order to reason about.
|
||||
// Degrades honestly if no popstate is coming at all — an overlay closed in the same tick it opened
|
||||
// never pushed an entry, so there is nothing to reclaim and the command would hang forever.
|
||||
// Navigating a beat late beats a control that silently does nothing.
|
||||
const fallback = setTimeout(() => run(), 250);
|
||||
const run = () => {
|
||||
if (done) return;
|
||||
done = true;
|
||||
window.removeEventListener("popstate", onPop);
|
||||
clearTimeout(fallback);
|
||||
navigate();
|
||||
// Any overlay entry still counted in `historyDepth` is now BURIED under the entry the navigation
|
||||
// just pushed, so a later unmount must NOT try to reclaim it with history.back() — that pops the
|
||||
// navigation instead and silently bounces the user back where they started. Found with a palette
|
||||
// opened over the player (E10 review round 1): closing the palette landed its own popstate, the
|
||||
// navigation then unmounted the player too, and the player's reconcile ate the new page entry.
|
||||
// Adopting the entries (rather than backing over them) leaves Back meaning "return to where I
|
||||
// was", which is what a navigation should mean anyway.
|
||||
historyDepth = 0;
|
||||
};
|
||||
window.addEventListener("popstate", once);
|
||||
const onPop = () => {
|
||||
// The same reconcile can issue MORE than one back(); a pushState made between them would be
|
||||
// popped by the next. Wait for the last one to land.
|
||||
if (pendingProgrammatic > 0) return;
|
||||
run();
|
||||
};
|
||||
window.addEventListener("popstate", onPop);
|
||||
close();
|
||||
}
|
||||
|
||||
|
||||
@@ -19,7 +19,7 @@ import { createStore } from "./store";
|
||||
|
||||
/** Which surface a shortcut belongs to. `global` is live app-wide; the rest only while that surface
|
||||
* is mounted. The help sheet shows `global` plus the topmost registered scope. */
|
||||
export type KeyScope = "global" | "undo" | "ytPlayer" | "plexPlayer" | "plexBrowse";
|
||||
export type KeyScope = "global" | "undo" | "contextMenu" | "ytPlayer" | "plexPlayer" | "plexBrowse";
|
||||
|
||||
/** Extra render condition for the help sheet (the shortcut itself is always live). */
|
||||
type ShortcutWhen = "episode";
|
||||
@@ -51,13 +51,18 @@ export const SHORTCUTS = [
|
||||
// --- global ---------------------------------------------------------------------------------
|
||||
// `h`, not `?`: on a Hungarian layout `?` is a Shift chord, and the one help key people already
|
||||
// have in their fingers here is the Plex player's `h`. Making it app-wide costs no relearning.
|
||||
{ id: "palette", scope: "global", keys: ["Mod+k"], labelKey: "keys.palette" },
|
||||
// `allowInInput` because the palette AUTOFOCUSES its search field: without it the typing guard ate
|
||||
// the second Mod+K, so the toggle only ever fired one way and Chrome took the chord instead
|
||||
// (E10 review round 1). Its handler preventDefaults, so nothing leaks to the browser either way.
|
||||
{ id: "palette", scope: "global", keys: ["Mod+k"], labelKey: "keys.palette", allowInInput: true },
|
||||
{ id: "help", scope: "global", keys: ["h"], labelKey: "keys.help" },
|
||||
// Matched by `useContextMenu` on the focused row, not by a window-level dispatch: which row the
|
||||
// menu belongs to is a property of focus, not of the app.
|
||||
// --- the row context menu (wherever useContextMenu is wired) ---------------------------------
|
||||
// Its OWN scope, not `global`: only the surfaces that actually call `useContextMenu` register it,
|
||||
// so the sheet stops advertising "Open the context menu" on the feed, Plex, Downloads and Settings
|
||||
// — where nothing answers the key (E10 review round 1). Same honest-states rule `plexBrowse` obeys.
|
||||
{
|
||||
id: "contextMenu",
|
||||
scope: "global",
|
||||
scope: "contextMenu",
|
||||
keys: ["Shift+F10", "ContextMenu"],
|
||||
labelKey: "keys.contextMenu",
|
||||
handledLocally: true,
|
||||
@@ -183,6 +188,16 @@ export function matches(e: KeyboardEvent, chord: string): boolean {
|
||||
* whether to `preventDefault`, because only it knows if it actually acted). `handlers` is a mapped
|
||||
* type over the scope's ids, so the table and the implementation cannot drift without tsc saying
|
||||
* so. Skipped while typing unless the row opts in with `allowInInput` (Escape, and only Escape). */
|
||||
/** Test an event against ONE row's chords, by id. The escape hatch for a `handledLocally` row: it is
|
||||
* matched at the element level (Shift+F10 belongs to whichever row has focus, not to the window),
|
||||
* but it must still go through `matches` or the table's `keys` become decoration. They had: the
|
||||
* context menu hand-rolled `e.key === "F10" && e.shiftKey`, so Ctrl+Shift+F10 opened it too and
|
||||
* editing the row changed the help sheet and nothing else (E10 review round 1). */
|
||||
export function matchesShortcut(e: KeyboardEvent, id: string): boolean {
|
||||
const row = ROWS.find((s) => s.id === id);
|
||||
return !!row?.keys.some((k) => matches(e, k));
|
||||
}
|
||||
|
||||
export function dispatch<S extends KeyScope>(
|
||||
e: KeyboardEvent,
|
||||
scope: S,
|
||||
@@ -252,10 +267,26 @@ export const usePaletteOpen = (): boolean => paletteOpen.use();
|
||||
/** Bind the `global` scope, once, for the whole app. It lives here rather than inside either
|
||||
* overlay because both are opened by it: a listener that sat in one of them would have to route a
|
||||
* key belonging to the other, and `dispatch`'s exhaustiveness would stop meaning anything. App
|
||||
* calls this; the overlays are pure renderers of their store. */
|
||||
export function useGlobalKeymap(): void {
|
||||
* calls this; the overlays are pure renderers of their store.
|
||||
*
|
||||
* `enabled` must track where those overlays are actually MOUNTED. Bound unconditionally it swallowed
|
||||
* `h` and Mod+K on the loading/Welcome/setup screens — the browser shortcut was preventDefault'ed,
|
||||
* nothing appeared, and the flipped module-level store then popped the overlay open by itself the
|
||||
* moment the signed-in shell mounted (E10 review round 1). */
|
||||
export function useGlobalKeymap(enabled = true): void {
|
||||
useEffect(() => {
|
||||
if (!enabled) {
|
||||
// Leaving a stale `true` behind would spring the overlay open on the next mount.
|
||||
helpOpen.set(false);
|
||||
paletteOpen.set(false);
|
||||
return;
|
||||
}
|
||||
const onKey = (e: KeyboardEvent) => {
|
||||
// Both global keys TOGGLE an overlay, and each overlay mount/unmount costs a real history
|
||||
// entry (useBackToClose). On auto-repeat that is ~30 push/back pairs a second, which Chrome
|
||||
// starts dropping while our own depth bookkeeping keeps counting them — a desynced stack whose
|
||||
// next browser Back walks out of the app. Holding a key is not a second press.
|
||||
if (e.repeat) return;
|
||||
dispatch(e, "global", {
|
||||
help: (ev) => {
|
||||
ev.preventDefault();
|
||||
@@ -270,7 +301,7 @@ export function useGlobalKeymap(): void {
|
||||
};
|
||||
window.addEventListener("keydown", onKey);
|
||||
return () => window.removeEventListener("keydown", onKey);
|
||||
}, []);
|
||||
}, [enabled]);
|
||||
}
|
||||
|
||||
/** Answers for the `when` conditions, published by the surface that knows them. */
|
||||
|
||||
@@ -0,0 +1,63 @@
|
||||
import { fileURLToPath } from "node:url";
|
||||
|
||||
import { ESLint } from "eslint";
|
||||
import { describe, expect, it } from "vitest";
|
||||
|
||||
// E10 S1 declared `components/ui/` a platform layer and put the rule that makes it one — no feature
|
||||
// components, no domain API slices — into eslint.config.js rather than prose. This asserts the rule
|
||||
// actually FIRES, because a `no-restricted-imports` pattern that matches nothing is indistinguishable
|
||||
// from a clean codebase: the first cut wrote its regexes in plain "..." literals, where `\.` collapses
|
||||
// to the any-char `.`, and it silently let through both `../../lib/api` (the barrel every feature
|
||||
// component imports) and `../../components/Feed` (the same file `../Feed` names). Eight green lanes
|
||||
// said nothing (E10 review round 1). A rule stated in a comment must also be enforced in code — and a
|
||||
// rule meant to enforce something must itself be tested.
|
||||
const BOUNDARY = "no-restricted-imports";
|
||||
|
||||
// Linted as if it were this file, so eslint.config.js's `files: ["src/components/ui/**"]` block applies.
|
||||
const AS_UI = "src/components/ui/__boundary_probe.tsx";
|
||||
|
||||
// fileURLToPath, not URL.pathname: on Windows the latter yields "/D:/…", which eslint cannot resolve
|
||||
// a config from. Anchored to this file rather than process.cwd() so the test holds wherever it is run.
|
||||
const FRONTEND = fileURLToPath(new URL("../../", import.meta.url));
|
||||
|
||||
async function restrictedIn(source: string, filePath: string): Promise<string[]> {
|
||||
const eslint = new ESLint({ cwd: FRONTEND });
|
||||
const [result] = await eslint.lintText(source, { filePath });
|
||||
return (result?.messages ?? []).filter((m) => m.ruleId === BOUNDARY).map((m) => m.message);
|
||||
}
|
||||
|
||||
describe("components/ui/ boundary rule", () => {
|
||||
it("refuses a feature component by BOTH of its spellings", async () => {
|
||||
const found = await restrictedIn(
|
||||
`import A from "../Sidebar";\nimport B from "../../components/Feed";\nexport default [A, B];\n`,
|
||||
AS_UI,
|
||||
);
|
||||
expect(found).toHaveLength(2);
|
||||
expect(found.every((m) => m.includes("may not import a feature component"))).toBe(true);
|
||||
});
|
||||
|
||||
it("refuses a domain API slice AND the barrel that re-exports every slice", async () => {
|
||||
const found = await restrictedIn(
|
||||
`import { api } from "../../lib/api";\nimport * as p from "../../lib/api/playlists";\nexport default [api, p];\n`,
|
||||
AS_UI,
|
||||
);
|
||||
expect(found).toHaveLength(2);
|
||||
expect(found.every((m) => m.includes("may not call a domain API slice"))).toBe(true);
|
||||
});
|
||||
|
||||
it("allows what the layer is FOR — sibling primitives and generic lib helpers", async () => {
|
||||
const found = await restrictedIn(
|
||||
`import B from "./Button";\nimport M from "../../components/ui/Modal";\nimport { qk } from "../../lib/queryKeys";\nexport default [B, M, qk];\n`,
|
||||
AS_UI,
|
||||
);
|
||||
expect(found).toEqual([]);
|
||||
});
|
||||
|
||||
it("binds only inside ui/ — a feature component may still import its neighbours", async () => {
|
||||
const found = await restrictedIn(
|
||||
`import { api } from "../lib/api";\nimport S from "./Sidebar";\nexport default [api, S];\n`,
|
||||
"src/components/__boundary_probe.tsx",
|
||||
);
|
||||
expect(found).toEqual([]);
|
||||
});
|
||||
});
|
||||
@@ -31,8 +31,16 @@ import { useLayoutEffect, useRef, useState, type RefObject } from "react";
|
||||
* fixed-layout column had assigned it. `clip`, not `hidden`, because clip creates no scrollport, so
|
||||
* nothing can ever scroll the mirror into view. The clip cannot disturb the measurement: offsetLeft and
|
||||
* offsetWidth are layout values, and clipping does not change layout.
|
||||
*
|
||||
* `overflow-clip-margin` is what keeps the clip from eating the KEYBOARD FOCUS RING. A bare
|
||||
* `overflow: clip` cuts at the padding box, while the app-wide base ring is `outline: 2px` at
|
||||
* `outline-offset: 2px` — i.e. it paints 4px OUTSIDE the border box. These lanes have no padding and
|
||||
* their height is set by the chip itself, so the first commit of this fix silently cut the ring's top
|
||||
* and bottom bands off every applied-filter chip and the tag lane's `+` trigger, leaving two floating
|
||||
* side bars (E10 review round 1). 0.25rem is exactly the ring's reach and nowhere near the ~600px the
|
||||
* mirror needs, so containment is unaffected.
|
||||
*/
|
||||
export const chipLaneClass = "relative overflow-clip";
|
||||
export const chipLaneClass = "relative overflow-clip [overflow-clip-margin:0.25rem]";
|
||||
|
||||
export interface ChipRowGeometry {
|
||||
/** Available lane width — the visible row's clientWidth. */
|
||||
|
||||
Reference in New Issue
Block a user