From 80dea4f307e36d2b83b8f57e4caec77c87728111 Mon Sep 17 00:00:00 2001
From: npeter83 <npeter83+claude@gmail.com>
Date: Sun, 6 Sep 2026 01:30:40 +0200
Subject: [PATCH] =?UTF-8?q?fix(e10):=20review=20round=201,=20part=202=20?=
 =?UTF-8?q?=E2=80=94=20the=20docs=20the=20epic=20invalidated,=20and=20thre?=
 =?UTF-8?q?e=20contracts=20made=20real?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Finishes round 1. Part 1 took the runtime bugs; this takes the prose that
had gone false and the three "the comment says so" gaps behind it.

CLAUDE.md is auto-loaded into every agent session and ui/README.md is
not, so an architecture paragraph that still described a flat components
bag was the most expensive stale sentence in the repo: the next agent
adding a popover reads it, sees no platform layer, and writes the generic
component into components/ — the invisible-layer failure this epic exists
to end. It now names components/ui/ and says the boundary is lint-enforced.
The canonical-patterns table gained the three rows the epic established:
ui/Button over a hand-rolled <button>, a keymap row over a private keydown
listener, and disabled-when-busy vs inert-when-inapplicable.

ui/README.md's inventory was written in this branch's first commit and
never learned about CommandPalette, ContextMenu/useContextMenu,
KeyboardHelp or gridColumns — in a section whose own closing rule is that
a second component painting the same thing is how the last inconsistency
happened. It also claimed "everything is rem-derived" while VirtualGrid's
minCol/GRID_GAP are deliberately px; that is now stated as the exception
it is, and a new section records which promises here are tests rather
than sentences.

Three contracts stopped being prose:

- A text-less Button must carry a name. "Required for iconOnly" was a doc
  comment, which is why the migration shipped two nameless icon buttons
  through eight green lanes. It is a union now, covering both spellings
  (iconOnly, and size="icon"/"icon-sm"), satisfied by label, aria-label or
  title. It caught one more on the way in: Banner's dismiss took an
  optional dismissTitle, so it could render unnamed — it falls back to
  common.close now.
- gridColumns' gutter arithmetic. Every existing assertion survived
  replacing floor((w + GAP) / (minCol + GAP)) with the naive
  floor(w / (minCol + GAP)): the fixed widths agree, the capped cases are
  cap-bound, and the loops compare columnsFor against itself. Pinned at
  the exact-fit boundaries where the two first disagree (536 and 828).
- ShareDialog's link row. Only Copy was migrated in S1, leaving one 2rem
  button between two 1.75rem ones — and on a coarse pointer .touch-target
  grew only the migrated one to 2.75rem while the destructive Revoke
  stayed small.

Also: the maxCols docstring said 8 columns of 384px where the test it
shipped with asserts 369.
---
 CLAUDE.md                                     | 14 +++++--
 frontend/src/components/ShareDialog.tsx       | 41 +++++++++++--------
 frontend/src/components/ui/Banner.tsx         |  7 +++-
 frontend/src/components/ui/Button.tsx         | 26 ++++++++++--
 frontend/src/components/ui/README.md          | 36 +++++++++++-----
 frontend/src/components/ui/VirtualGrid.tsx    |  2 +-
 .../src/components/ui/gridColumns.test.ts     | 15 +++++++
 7 files changed, 107 insertions(+), 34 deletions(-)

diff --git a/CLAUDE.md b/CLAUDE.md
index 8db5c549..edd8f63e 100644
--- a/CLAUDE.md
+++ b/CLAUDE.md
@@ -59,9 +59,14 @@ how it gets built. Concretely:
 `downloads/`, `youtube/` (shared by routes + scheduler + worker) → flat core (`auth`, `quota`,
 `sysconfig`, `state`, `audit`, `notifications`, `userscope`, `realtime`, `ratelimit`, `progress`,
 `security`) → `models/db/config`. **Dependency direction is one-way: core never imports routes.**
-Frontend: `main.tsx` provider tree → `App.tsx` shell → `moduleRegistry` route-chunks → components; data
-via react-query (`qk` factory), per-account `localStorage`, and server prefs; API through `lib/api/*`
-slices over `core.req<T>`.
+Frontend: `main.tsx` provider tree → `App.tsx` shell → `moduleRegistry` route-chunks → module
+components → the **platform layer `components/ui/`** (domain-free primitives: the popover engine and
+its menus, `Modal`/`Overlay`, `Button`/`form`, `DataTable`/`VirtualGrid`, `QueryState`, …). That
+boundary is one-way and **lint-enforced**, not just described: a platform component may not import a
+feature component or a domain API slice (`no-restricted-imports` in `eslint.config.js`, with its own
+test). A new generic component goes in `ui/`; see [`frontend/src/components/ui/README.md`](frontend/src/components/ui/README.md).
+Data via react-query (`qk` factory), per-account `localStorage`, and server prefs; API through
+`lib/api/*` slices over `core.req<T>`.
 
 **Not wanted** (reviewed and rejected as over-engineering for this size): microservices, event bus,
 CQRS, a repository/DI framework, a router library, a generic CRUD-router factory or base serializer.
@@ -82,6 +87,9 @@ CQRS, a repository/DI framework, a router library, a generic CRUD-router factory
 | Job status word | the `downloads/status.py` StrEnum | loose `"canceled"`/`"cancelled"` literals |
 | Current time | inline `datetime.now(timezone.utc)` | new `now_utc`/`_now` helpers |
 | FE per-account state | `usePref` | raw `readAccount`/`writeAccount` or bespoke hooks |
+| FE action button | `ui/Button` — pick a `SIZES` rung | a hand-rolled `<button className="px-3 py-2 …">` |
+| FE keyboard shortcut | a row in `lib/keymap.ts` + `dispatch()` | a private `keydown` listener with its own key literals |
+| FE menu row that is off | `disabled` only when BUSY; `inert` when it does not apply | `disabled` + a `title` nobody can reach |
 | FE control | the chip/segmented primitives (`Select` only for >6 options) | a native `<select>` |
 | FE loading/empty/error | `QueryState` / `StateMessage` | ad-hoc conditionals |
 
diff --git a/frontend/src/components/ShareDialog.tsx b/frontend/src/components/ShareDialog.tsx
index e95a0b29..74154fff 100644
--- a/frontend/src/components/ShareDialog.tsx
+++ b/frontend/src/components/ShareDialog.tsx
@@ -138,27 +138,37 @@ function LinkRow({ link, onChanged }: { link: ShareLink; onChanged: () => void }
           value={fullUrl}
           className="flex-1 min-w-0 bg-transparent text-xs text-muted truncate outline-hidden"
         />
-        <Button variant="ghost" size="icon-sm" onClick={copy} title={t("downloads.share.copy")}>
+        <Button
+          variant="ghost"
+          size="icon-sm"
+          onClick={copy}
+          aria-label={t("downloads.share.copy")}
+          title={t("downloads.share.copy")}
+        >
           {copied ? <Check className="w-4 h-4 text-success" /> : <Copy className="w-4 h-4" />}
         </Button>
-        <button
+        {/* All three on the same rung. Only Copy was migrated in S1, which left one 2rem button
+            between two 1.75rem ones with a different radius and hover fill — and, on a coarse
+            pointer, `.touch-target` grew ONLY the migrated one to 2.75rem while the destructive
+            Revoke stayed small (E10 review round 1). */}
+        <Button
+          variant="ghost"
+          size="icon-sm"
           onClick={() => {
             setNewPass("");
             setEditPass((v) => !v);
           }}
-          title={
+          className={link.has_password ? "text-accent" : undefined}
+          label={
             link.has_password
               ? t("downloads.share.changePassword")
               : t("downloads.share.setPassword")
           }
-          className={clsx(
-            "p-1.5 rounded-md hover:bg-surface transition",
-            link.has_password ? "text-accent" : "text-muted hover:text-fg",
-          )}
-        >
-          <Lock className="w-4 h-4" />
-        </button>
-        <button
+          icon={<Lock className="w-4 h-4" />}
+        />
+        <Button
+          variant="ghost"
+          size="icon-sm"
           onClick={async () => {
             if (
               await confirm({
@@ -170,11 +180,10 @@ function LinkRow({ link, onChanged }: { link: ShareLink; onChanged: () => void }
             )
               revoke.mutate();
           }}
-          title={t("downloads.share.revoke")}
-          className="p-1.5 rounded-md hover:bg-surface text-muted hover:text-danger transition"
-        >
-          <Trash2 className="w-4 h-4" />
-        </button>
+          className="enabled:hover:text-danger"
+          label={t("downloads.share.revoke")}
+          icon={<Trash2 className="w-4 h-4" />}
+        />
       </div>
       <div className="flex items-center gap-2 text-xs text-muted flex-wrap">
         <span className="inline-flex items-center gap-1">
diff --git a/frontend/src/components/ui/Banner.tsx b/frontend/src/components/ui/Banner.tsx
index afcba0a1..4aa46974 100644
--- a/frontend/src/components/ui/Banner.tsx
+++ b/frontend/src/components/ui/Banner.tsx
@@ -1,4 +1,5 @@
 import type { ReactNode } from "react";
+import { useTranslation } from "react-i18next";
 import type { LucideIcon } from "lucide-react";
 import { X } from "lucide-react";
 import { Button } from "./Button";
@@ -34,12 +35,16 @@ export default function Banner({
   children: ReactNode;
   action?: { label: string; onClick: () => void };
   onDismiss?: () => void;
+  /** Accessible name (and tooltip) for the dismiss button. Optional at the call site; the component
+   *  falls back to the generic "Close" rather than shipping an unnamed icon button — a nameless
+   *  control is not an acceptable default, and `Button`'s type now says so. */
   dismissTitle?: string;
   /** `"status"` for a bar that APPEARS in response to something the user just did, so a screen
    * reader announces it. Left unset for the permanent bars (demo, version): those are part of the
    * page when it loads, and announcing them on every visit is noise, not information. */
   role?: "status";
 }) {
+  const { t } = useTranslation();
   const c = TONES[tone];
   return (
     <div
@@ -62,7 +67,7 @@ export default function Banner({
           size="icon-sm"
           className="ml-auto"
           onClick={onDismiss}
-          title={dismissTitle}
+          title={dismissTitle ?? t("common.close")}
           icon={<X className="w-4 h-4" />}
         />
       )}
diff --git a/frontend/src/components/ui/Button.tsx b/frontend/src/components/ui/Button.tsx
index 13e44cf5..4db42540 100644
--- a/frontend/src/components/ui/Button.tsx
+++ b/frontend/src/components/ui/Button.tsx
@@ -1,5 +1,5 @@
 import { forwardRef } from "react";
-import type { ButtonHTMLAttributes, ReactNode } from "react";
+import type { ButtonHTMLAttributes, ReactNode, RefAttributes } from "react";
 import clsx from "clsx";
 import { Loader2 } from "lucide-react";
 
@@ -88,7 +88,7 @@ const SIZES: Record<ButtonSize, string> = {
 const BASE =
   "inline-flex items-center justify-center transition disabled:opacity-50 disabled:cursor-not-allowed";
 
-export interface ButtonProps extends ButtonHTMLAttributes<HTMLButtonElement> {
+interface ButtonOwnProps extends ButtonHTMLAttributes<HTMLButtonElement> {
   variant?: ButtonVariant;
   /** Defaults to "md", except for `variant="link"`, whose default is "none" (a link owns no padding). */
   size?: ButtonSize;
@@ -103,8 +103,22 @@ export interface ButtonProps extends ButtonHTMLAttributes<HTMLButtonElement> {
   fullWidth?: boolean;
 }
 
+// A button that renders NO TEXT has to get its accessible name from somewhere, and "required for
+// `iconOnly`" was a sentence in a doc comment — so the S1 migration shipped two icon buttons that
+// announce themselves as a bare "button" and every lane stayed green (E10 review round 1). The rule
+// is a type now, in the file that owns the contract.
+//
+// Both spellings of a text-less button are covered: `iconOnly`, and the far commoner `size="icon"` /
+// `"icon-sm"` (a square box that fits one glyph). Any of the three naming props satisfies it —
+// `title` included, because the accname spec does fall back to it and ~16 call sites legitimately
+// use it for the tooltip that IS the name.
+type NamedSomehow = { label: string } | { "aria-label": string } | { title: string };
+type TextlessButton = ({ iconOnly: true } | { size: "icon" | "icon-sm" }) & NamedSomehow;
+type TextfulButton = { iconOnly?: false; size?: Exclude<ButtonSize, "icon" | "icon-sm"> };
+export type ButtonProps = ButtonOwnProps & (TextlessButton | TextfulButton);
+
 /** The shared action button. `type` defaults to "button" so it never accidentally submits a form. */
-export const Button = forwardRef<HTMLButtonElement, ButtonProps>(function Button(
+const ButtonImpl = forwardRef<HTMLButtonElement, ButtonOwnProps>(function Button(
   {
     variant = "primary",
     size,
@@ -157,3 +171,9 @@ export const Button = forwardRef<HTMLButtonElement, ButtonProps>(function Button
     </button>
   );
 });
+
+// The implementation takes the permissive shape (it has to destructure every prop); the EXPORT is
+// what call sites see, and that is where the "a text-less button must be named" union binds.
+export const Button = ButtonImpl as (
+  props: ButtonProps & RefAttributes<HTMLButtonElement>,
+) => ReturnType<typeof ButtonImpl>;
diff --git a/frontend/src/components/ui/README.md b/frontend/src/components/ui/README.md
index e9d04615..c1d1a9ea 100644
--- a/frontend/src/components/ui/README.md
+++ b/frontend/src/components/ui/README.md
@@ -29,13 +29,13 @@ domain-free.
 
 ## What lives here
 
-| area                   | components                                                                                                                                            |
-| ---------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------- |
-| overlays & anchoring   | `AnchoredPopover` (the popover engine: arrows, typeahead, Escape, anchor-flip), `ActionMenu`, `Modal`, `Overlay`, `SidePanel`, `Tooltip`, `Toaster`   |
-| actions & inputs       | `Button`, `form`, `DraftSaveBar`, `Combobox`, `FilterSelect`, `SegmentedToggle`, `SortControl`, `ColumnSortControl`, `PriorityStepper`, `UndoToolbar` |
-| data display           | `DataTable`, `tableHeader`, `Pager`, `VirtualGrid`, `readAhead`, `Carousel`, `Avatar`, `CountBadge`, `ItemCount`, `FilterChipGroup`                   |
-| page & panel structure | `PageShell`, `PageScroller`, `PanelGroup`, `PanelGroups`, `Tabs`, `BackToTop`                                                                         |
-| state & messaging      | `QueryState`, `Banner`, `InlineNotice`                                                                                                                |
+| 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)
 
@@ -46,9 +46,11 @@ somewhere:
 
 - **Geometry is a ladder, not a free choice.** `Button`'s `SIZES` are the rungs, derived from what
   the app actually used. Pick a rung; do not hand-roll padding on a primitive.
-- **Everything is 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.
+- **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.
 - **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
@@ -58,6 +60,20 @@ somewhere:
 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.ts` runs 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.ts` counts 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
diff --git a/frontend/src/components/ui/VirtualGrid.tsx b/frontend/src/components/ui/VirtualGrid.tsx
index 6b952657..24fe8d92 100644
--- a/frontend/src/components/ui/VirtualGrid.tsx
+++ b/frontend/src/components/ui/VirtualGrid.tsx
@@ -56,7 +56,7 @@ export interface VirtualGridProps<T> {
    * as fit. This is the UPPER end of the viewport contract (E10 S4): an auto-fill grid always settles
    * just above its minimum column, so on an ultrawide it answers "more room" with more, smaller
    * items forever — 3440px gave the feed 11 columns of 264px. With a cap the extra width goes into
-   * the items instead (8 columns of 384px), which is what a screen watched from a distance wants.
+   * the items instead (8 columns of ~370px), which is what a screen watched from a distance wants.
    * It only ever binds ABOVE the width where that many columns already fit, so narrower layouts —
    * including the 1024px floor and 1920/2560 — are untouched. */
   maxCols?: number | null;
diff --git a/frontend/src/components/ui/gridColumns.test.ts b/frontend/src/components/ui/gridColumns.test.ts
index 7e4c9e9e..0e939080 100644
--- a/frontend/src/components/ui/gridColumns.test.ts
+++ b/frontend/src/components/ui/gridColumns.test.ts
@@ -14,6 +14,21 @@ describe("columnsFor", () => {
     expect(columnsFor(260, 700, null)).toBe(2);
   });
 
+  // The GUTTERS have to be in the count, not just in itemWidthFor. Every other assertion in this file
+  // survives replacing `floor((w + GAP) / (minCol + GAP))` with the naive `floor(w / (minCol + GAP))`
+  // — the fixed widths agree, the capped cases are cap-bound, and the two loops compare columnsFor
+  // against itself, so they cancel any shared change (E10 review round 1). These are the exact-fit
+  // boundaries where the two formulas first disagree: an n-column row needs n*minCol + (n-1)*GAP, so
+  // at exactly that width the naive form loses the last column and wastes a third of the row.
+  it("counts the gutters, not one gap per column", () => {
+    const exactly3 = 3 * 260 + 2 * GRID_GAP; // 828
+    expect(columnsFor(260, exactly3, null)).toBe(3);
+    expect(columnsFor(260, exactly3 - 1, null)).toBe(2);
+    const exactly2 = 2 * 260 + GRID_GAP; // 536 — the FIRST width where the naive form is wrong
+    expect(columnsFor(260, exactly2, null)).toBe(2);
+    expect(columnsFor(260, exactly2 - 1, null)).toBe(1);
+  });
+
   it("never returns 0 columns, however narrow", () => {
     expect(columnsFor(260, 10, null)).toBe(1);
     expect(columnsFor(260, 0, null)).toBe(1);