peter
4b33036e7c
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.
6.4 KiB
components/ui/ — the platform layer
Everything in this directory is a platform component: generic, reusable, and domain-free.
Everything in components/ above it is a module component: it knows about videos, channels,
playlists, Plex or downloads.
That boundary is the whole point of this directory. Before E10 S1 these 33 modules sat flat among
~160 feature components, and the platform was invisible — several of them literally describe
themselves as "the one place", "shared", "canonical", "app-wide", and were still routinely bypassed
by a hand-rolled <button> or a re-implemented popover. A layer you cannot see is a layer nobody
uses.
The rule
A component belongs here iff its props and imports name no domain concept. It may import:
- sibling primitives in this directory,
- generic helpers from
lib/(layerStack,history,columnSort,useScrollFade, …), - i18n, React, and third-party UI libraries.
It may not import a feature component (../Feed) or a domain API slice (lib/api/*). This is
enforced by no-restricted-imports in eslint.config.js — not by this README, which is only the
explanation. If a primitive needs domain data, take it as a prop or a render-prop and let the module
supply it.
Counter-examples, deliberately NOT here: PosterTile (knows about Plex montage posters), KeyGate
(secure-messaging key setup), plexCards, downloadRow, channelColumns — all shared, none
domain-free.
What lives here
| area | components |
|---|---|
| overlays & anchoring | AnchoredPopover (the popover engine: arrows, typeahead, Escape, anchor-flip), ActionMenu, ContextMenu + useContextMenu (the SAME menu, opened by right-click / long-press / Shift+F10), Modal, Overlay, SidePanel, Tooltip, Toaster, CommandPalette, KeyboardHelp |
| actions & inputs | Button, form, DraftSaveBar, Combobox, FilterSelect, SegmentedToggle, SortControl, ColumnSortControl, PriorityStepper, UndoToolbar |
| data display | DataTable, tableHeader, Pager, VirtualGrid + gridColumns (the column/width maths, incl. the ultrawide maxCols cap), readAhead, Carousel, Avatar, CountBadge, ItemCount, FilterChipGroup |
| page & panel structure | PageShell, PageScroller, PanelGroup, PanelGroups, Tabs, BackToTop |
| state & messaging | QueryState, Banner, InlineNotice |
The size contract (E10 S1c)
The app optimises for 1024x768 (tablet landscape) as the floor; below it we owe an honest message, above it correct rendering all the way up. That contract lives in these components, not in the ~160 call sites, because a rule repeated in 160 places is a rule that is already broken somewhere:
- Geometry is a ladder, not a free choice.
Button'sSIZESare the rungs, derived from what 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-scaleon the root font-size) scales boxes and padding with the label. Atext-[Npx]font size is an eslint error for exactly this reason. The deliberate exceptions are the layout-engine measurements —VirtualGrid'sminCol/colMaxWidthandgridColumns'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'smax-w-[240px](it does not widen with the slider while its text does, so the tooltip re-wraps at large text sizes) andform.tsx'sleft-[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 on neighbouring icon buttons would overlap, and an overlap fires the wrong button. linkis excluded from the minimum: it is text inside a sentence.
Controls outside this directory (chips, list rows, the player's overlay buttons) are not covered yet -- they are the next thing to bring under the contract, not evidence that there isn't one.
Keeping it honest
Two of this directory's promises are load-bearing enough that prose was not enough, so each has a test rather than a paragraph:
- the import boundary above →
lib/uiBoundary.contract.test.tsruns eslint over fixture imports. It exists because the first cut of that rule wrote its regexes in plain"..."literals, where\.collapses to the any-char.— it read like a boundary and let the two commonest violations through, with a green gate saying nothing (E10 review round 1). - the chip lanes' clip →
lib/chipLane.contract.test.tscounts measurers against lanes per file.
If you add a rule here, ask what would fail if it stopped being true — and if the answer is "nothing", write the test instead of the sentence.
Adding one
Move a component in when a second module needs it and it is domain-free — not on speculation. Adding a variant to an existing primitive beats adding a primitive; a second component that paints the same thing differently is how the last inconsistency happened.