peter/siftlode
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fix(e10): review round 2 — revert what round 1 got wrong, narrow what it got too broad

Browse Source 
Round 2 reviewed round 1's own fix diff, which is where a fix round does
its damage. It found 15 issues in 792 lines — a much higher density than
round 1 found in the whole epic — and most were regressions round 1
introduced. So this follows the rule rather than fighting it: revert the
fixes whose mechanism was wrong, narrow the ones whose guard was too
broad, and stop reaching for a third variant.

REVERTED, because the mechanism does not work:

- The `inert` menu item. Round 1 swapped a native `disabled` for
  `aria-disabled` so a row's "why is this off" tooltip could be reached.
  floating-ui's list navigation skips `aria-disabled="true"` exactly as it
  skips `disabled` (isListIndexDisabled) — and AnchoredPopover's own
  comment, written before this branch, already said so. The swap fixed
  nothing and made the mouse worse: without native `disabled` the row took
  pointer events, the full active highlight and DOM focus, then no-opped
  on click. The underlying finding is logged where the prop was, unfixed
  and honest: the reason has to go somewhere a disabled row is not.

- `useContextMenu` owning the focus restore. Turning floating-ui's
  returnFocus off platform-wide traded one bug for two: Playlists spreads
  triggerProps on a bare <tr>, so restoring to it is a spec no-op and
  Escape stranded focus on <body>; and with dismissOnScroll now on, the
  restore's plain focus() scrolled the row back into view, undoing the
  scroll that closed the menu. Patching that needs three more guards on
  one fix. The original bug — the rail's rename field dying on arrival —
  is fixed at the field instead: it focuses from an effect, which is
  scheduled after the microtask that was blurring it.

NARROWED, because the fix was right and the guard was not:

- PlexPlayer's Backspace deferred to `anyLayerAbove`, but the player's own
  gear and tracks menus register layers via useDismiss — so Backspace
  stopped closing them and did nothing at all. Only the cheat sheet and
  the palette own that key, so it names them.
- `historyDepth = 0` also desynced the counter from overlays the
  navigation does NOT unmount (About and ReleaseNotes live outside the
  page column), leaving one Back to both navigate and close them. The
  entries are marked buried and consumed silently by reconcile instead.
- The move's error toast fired for every status, so a transport fault
  surfaced twice on top of req's own dialog. Scoped to 403/404.
- The palette's moved-pointer guard could not judge the FIRST move of a
  session and went stale across a close. `e.movementX/Y` asks the browser
  directly and needs no state.
- `useKeyScope("contextMenu")` runs once per hook, and the Playlists page
  mounts two — so the cheat sheet rendered the chords twice under a
  duplicated React key. The scope stack dedupes.
- The `enabled:hover:` sweep missed the half that lives in CSS, so a
  disabled glass button still lifted. `.glass-hover:not(:disabled):hover`.
- ShareDialog's migrated buttons passed a colour className that the
  variant's own colour class outranks under clsx, so Revoke hovered
  neutral and a password-protected link looked unprotected. The colour
  goes on the icon, as the Copy button beside them already did.
- `matchesShortcut` took an untyped id and ids are only unique within a
  scope, so `back` would have answered for the wrong player. Keyed on
  scope + id, both literal.

And the systemic one: round 2 reverted eight of round 1's behaviour fixes
as a single mutation and all 763 tests still passed. Round 1 tested the
three fixes that were easy to test and none of the rest. So the guards
with the widest blast radius are pinned now, each mutation-checked to
fail for its own reason: the global keymap's repeat/allowInInput/disabled
handling (a new dom test), the cheat sheet bound to the real
useContextMenu rather than a scope stub, the scope dedupe, the history
burial, and the Button accessible-name union — whose @ts-expect-error
fixtures turn a widened type into a red typecheck lane.

The chipLane contract test that round 1 rewrote to count per lane counted
the import line as a lane, giving every file one free mirror — so the
exact hole it was rewritten to close was still open. Mutation-checked
both ways this time.
This commit is contained in:
peter
2026-09-06 01:57:32 +02:00
parent 80dea4f307
commit 4b33036e7c
17 changed files with 331 additions and 148 deletions
Download Patch File Download Diff File Expand all files Collapse all files
frontend/src/components/ui/ActionMenu.tsx
+11 -13
View File
@@ -22,16 +22,17 @@ 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)
/** Optional native tooltip (carries a longer hint the label alone can't).
*
* KNOWN GAP, deliberately still open: on a `disabled` row this reaches nobody — a disabled button
* fires no pointer events, so no tooltip, and floating-ui's roving focus skips it. E10 review
* round 1 tried an `aria-disabled` "inert" row to fix that; round 2 found `useListNavigation`
* skips `aria-disabled` identically (`isListIndexDisabled`), which AnchoredPopover's own comment
* already said — so the swap fixed nothing and made the mouse worse (the row took the active
* highlight, then no-opped on click). Reverted. A real fix has to put the reason somewhere a
* disabled row is not: a visible hint line in the menu, or an `aria-describedby` note. */
title?: string;
/** Draw a divider above this item. Ignored on the first item — a menu never opens with a rule. */
separatorBefore?: boolean;
}
@@ -79,16 +80,13 @@ 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.inert ? " opacity-50" : ""}${it.danger ? " text-danger" : ""}`}
className={`${ROW}${it.danger ? " text-danger" : ""}`}
>
{it.icon && (
<span className={`shrink-0 ${it.danger ? "text-danger" : "text-muted"}`}>
frontend/src/components/ui/AnchoredPopover.tsx
+1 -10
View File
@@ -118,13 +118,6 @@ export interface AnchoredPopoverOptions {
* 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.
@@ -301,7 +294,6 @@ export function useAnchoredPopover(opts: AnchoredPopoverOptions) {
roves,
open: opts.open,
selectedIndex: opts.selectedIndex ?? null,
returnFocus: opts.returnFocus ?? true,
refs,
floatingStyles,
context,
@@ -526,7 +518,6 @@ function PopoverSurfaceInner({
isNav,
isCombobox,
roves,
returnFocus,
selectedIndex,
activeIndex,
highlightIndex,
@@ -624,7 +615,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>
frontend/src/components/ui/Button.dom.test.tsx
+29
View File
@@ -115,3 +115,32 @@ describe("Button primitive (S5)", () => {
expect(screen.getByRole("button", { name: "Wide" }).className).toContain("w-full");
});
});
// --- the accessible-name contract, pinned at the TYPE level ------------------------------------
// A button that renders no text must carry a name, and that rule is a union in Button.tsx rather
// than a doc comment because the comment version let two nameless icon buttons ship (E10 review
// round 1). But a type with no test is a type someone widens to silence a stubborn call site:
// replacing `ButtonProps` with the permissive shape left tsc, eslint, knip and vitest all green
// (round 2). Each `@ts-expect-error` below FAILS THE BUILD if the error it expects stops happening,
// so weakening the union turns the whole typecheck lane red.
//
// Rendered, not just declared, so eslint sees them used and the file stays one test suite.
describe("Button accessible-name contract (type level)", () => {
it("rejects a text-less button with no name, and accepts every named spelling", () => {
render(
<>
{/* @ts-expect-error size="icon" with no label/aria-label/title has no accessible name */}
<Button size="icon" icon={<span />} data-testid="bad-a" />
{/* @ts-expect-error iconOnly with no label/aria-label/title has no accessible name */}
<Button iconOnly icon={<span />} data-testid="bad-b" />
<Button size="icon" icon={<span />} label="by label" />
<Button size="icon-sm" icon={<span />} aria-label="by aria-label" />
<Button iconOnly icon={<span />} title="by title" />
<Button>plain text needs none</Button>
</>,
);
expect(screen.getByRole("button", { name: "by label" })).toBeTruthy();
expect(screen.getByRole("button", { name: "by aria-label" })).toBeTruthy();
expect(screen.getByRole("button", { name: "by title" })).toBeTruthy();
});
});
frontend/src/components/ui/CommandPalette.tsx
+5 -6
View File
@@ -64,8 +64,6 @@ 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.
@@ -170,10 +168,11 @@ export default function CommandPalette({
// 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;
// whatever row slid under a resting cursor (E10 review round 1). The browser
// reports zero movement for exactly that synthetic event, so ask it directly:
// round 1 remembered the last position instead, which could not judge the FIRST
// move of a session and went stale across a close (round 2).
if (e.movementX === 0 && e.movementY === 0) return;
setActive(flat.indexOf(item));
}}
className={`flex w-full items-center gap-2 rounded-lg px-2 py-1.5 text-left text-sm ${
frontend/src/components/ui/KeyboardHelp.dom.test.tsx
+35 -3
View File
@@ -6,6 +6,7 @@
import { describe, it, expect, vi, afterEach } from "vitest";
import { render, screen, fireEvent, cleanup, act } from "@testing-library/react";
import KeyboardHelp from "./KeyboardHelp";
import { useContextMenu } from "./useContextMenu";
import {
closeKeyboardHelp,
useGlobalKeymap,
@@ -95,14 +96,45 @@ describe("KeyboardHelp", () => {
// 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(
// Through the REAL hook, not a scope stub: round 2 showed that commenting out
// `useKeyScope("contextMenu")` inside `useContextMenu` left the whole suite green, because the
// only coverage registered the scope by hand. This binds the sheet to the thing that owns it.
it("offers the context-menu key on a surface that has one, and not otherwise", () => {
function WithMenu() {
useContextMenu<string>();
return null;
}
const { unmount } = render(
<Host>
<Surface scope="contextMenu" />
<WithMenu />
</Host>,
);
pressH();
expect(descriptions()).toContain("keys.contextMenu");
unmount();
cleanup();
closeKeyboardHelp();
render(<Host />);
pressH();
expect(descriptions()).not.toContain("keys.contextMenu");
});
// Two live context menus (the Playlists page mounts one, its rail another) must not double the row.
it("lists the context-menu chords once however many surfaces register", () => {
function WithMenu() {
useContextMenu<string>();
return null;
}
render(
<Host>
<WithMenu />
<WithMenu />
</Host>,
);
pressH();
expect(descriptions().filter((d) => d === "keys.contextMenu")).toHaveLength(1);
expect(screen.getAllByText("⇧ F10")).toHaveLength(1);
});
// "Previous / next episode" inside a movie would be a promise the player does not keep.
frontend/src/components/ui/README.md
+6 -3
View File
@@ -48,9 +48,12 @@ somewhere:
the app actually used. Pick a rung; do not hand-roll padding on a primitive.
- **Type, padding and box geometry are rem-derived**, so the Settings text-size slider (`--font-scale`
on the root font-size) scales boxes and padding with the label. A `text-[Npx]` font size is an
eslint error for exactly this reason. The deliberate exception is `VirtualGrid`'s `minCol` /
`colMaxWidth` / `GRID_GAP`, which are px: those are layout-engine measurements compared against a
container's own pixel width, not text boxes.
eslint error for exactly this reason. The deliberate exceptions are the layout-engine measurements
— `VirtualGrid`'s `minCol` / `colMaxWidth` and `gridColumns`' `GRID_GAP` — which are compared
against a container's own pixel width rather than sized against text. Two px box values are NOT
deliberate and are worth converting when touched: `Tooltip`'s `max-w-[240px]` (it does not widen
with the slider while its text does, so the tooltip re-wraps at large text sizes) and `form.tsx`'s
`left-[18px]` toggle knob.
- **Minimum target: 2.75rem, on coarse pointers only** (`.touch-target`, index.css). A 32px icon
button is fine with a mouse and not with a thumb, so the box grows where there is no precise
pointer. It grows the real box rather than painting an invisible hit area: two expanded hit areas
frontend/src/components/ui/useContextMenu.ts
+3 -46
View File
@@ -64,8 +64,6 @@ 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({
@@ -73,10 +71,7 @@ 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);
restoreFocusRef.current();
}
if (!next) setTarget(null);
},
// A native context menu drops down-and-right from the pointer; flip()/shift() take it the other
// way near an edge.
@@ -86,44 +81,18 @@ export function useContextMenu<T>(): ContextMenuController<T> {
// 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);
@@ -197,7 +166,7 @@ export function useContextMenu<T>(): ContextMenuController<T> {
// pointer that isn't there.
// 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;
if (!matchesShortcut(e.nativeEvent, "contextMenu", "contextMenu")) return;
e.preventDefault();
const r = e.currentTarget.getBoundingClientRect();
openAt(r.left, r.bottom, payload, e.currentTarget);
@@ -206,17 +175,5 @@ export function useContextMenu<T>(): ContextMenuController<T> {
[openAt, cancelPress],
);
restoreFocusRef.current = restoreFocus;
return {
target,
open,
close: () => {
setOpen(false);
setTarget(null);
restoreFocus();
},
triggerProps,
popover: pop,
};
return { target, open, close: () => setOpen(false), triggerProps, popover: pop };
}