diff --git a/CLAUDE.md b/CLAUDE.md index edd8f63e..caca1a13 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -26,8 +26,8 @@ The local gate is **ruff (`check` + `format --check`) + typecheck (3 tsc project keep commits logical and Conventional-Commit-styled. - **No AI attribution in commits.** No `Co-Authored-By` trailer, no "generated by" lines. (This file being AI-written is the intended, visible exception; commits are not.) -- **Release: user-owned in timing, agent-run in execution.** The user decides *when* to ship; on their - go-ahead *you* run `siftlode release` from the feature branch (merge to `main` → publish → deploy → +- **Release: user-owned in timing, agent-run in execution.** The user decides _when_ to ship; on their + go-ahead _you_ run `siftlode release` from the feature branch (merge to `main` → publish → deploy → verify `/api/version`). Never self-ship — bring work to a green, committed state and stop for the go-ahead first, then run it yourself. - **Public repo.** Keep ops/topology/secrets out — those live in a separate private repo. Nothing here @@ -41,7 +41,7 @@ how it gets built. Concretely: - **Plan before you build.** For anything non-trivial, present a short plan (sprint-level for larger work) and get a go-ahead before writing code. - **Offer choices, mark the optimal — and explain the trade-off.** When several solutions are viable, - list them and mark the recommended one. This is the *one* place to spend extra words: give the *why* + list them and mark the recommended one. This is the _one_ place to spend extra words: give the _why_ behind each option and the weight of the decision. These decision points are where the product owner engages with the build and learns from it, so make them understandable — a reasoned menu, not a bare one. - **Mockups for design/UX decisions.** For any visual or interaction choice, show mockup options to @@ -73,25 +73,24 @@ CQRS, a repository/DI framework, a router library, a generic CRUD-router factory ## Canonical patterns — when you have a choice, pick THIS -| Area | Use | Not | -|---|---|---| -| ORM | SQLAlchemy 2.0 `select()` | `db.query(...)` | -| Request body | a Pydantic model | `payload: dict` | -| Ownership check | `owned_or_404` / `get_or_404` | ad-hoc `_own_playlist`/`_owned`/`_own_job` | -| Admin gate import | `app.auth` | the `app.routes.admin` re-export | -| Upstream failure status | `422` with JSON detail | `502` (reserve 502 for raw byte-proxies) | -| User-facing error | structured `detail` (`{code, reason}`) | a bare English string under a bilingual UI | -| Mutation response | `{"ok": true, …}` (`{"deleted": id}`) | ad-hoc `{"unsubscribed"}`/`{"cleared": n}`/echo | -| Transaction commit | the handler/job-wrapper commits; helpers `flush` | helpers that commit (only `quota` owns its session) | -| Admin knob storage | `sysconfig` (values) | env / `app_state` / `scheduler_settings` scatter | -| 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 ` + , + ); + 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(); + }); +}); diff --git a/frontend/src/components/ui/CommandPalette.tsx b/frontend/src/components/ui/CommandPalette.tsx index 62e81a39..d0f00fca 100644 --- a/frontend/src/components/ui/CommandPalette.tsx +++ b/frontend/src/components/ui/CommandPalette.tsx @@ -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(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 ${ diff --git a/frontend/src/components/ui/KeyboardHelp.dom.test.tsx b/frontend/src/components/ui/KeyboardHelp.dom.test.tsx index 6780648f..cf99270c 100644 --- a/frontend/src/components/ui/KeyboardHelp.dom.test.tsx +++ b/frontend/src/components/ui/KeyboardHelp.dom.test.tsx @@ -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(); + return null; + } + const { unmount } = render( - + , ); pressH(); expect(descriptions()).toContain("keys.contextMenu"); + + unmount(); + cleanup(); + closeKeyboardHelp(); + render(); + 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(); + return null; + } + render( + + + + , + ); + 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. diff --git a/frontend/src/components/ui/README.md b/frontend/src/components/ui/README.md index c1d1a9ea..eae01500 100644 --- a/frontend/src/components/ui/README.md +++ b/frontend/src/components/ui/README.md @@ -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 diff --git a/frontend/src/components/ui/useContextMenu.ts b/frontend/src/components/ui/useContextMenu.ts index 698e85a4..1342176c 100644 --- a/frontend/src/components/ui/useContextMenu.ts +++ b/frontend/src/components/ui/useContextMenu.ts @@ -64,8 +64,6 @@ export interface ContextMenuController { export function useContextMenu(): ContextMenuController { // 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(null); const [open, setOpen] = useState(false); const pop = useAnchoredPopover({ @@ -73,10 +71,7 @@ export function useContextMenu(): ContextMenuController { 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(): ContextMenuController { // 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(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(null); @@ -197,7 +166,7 @@ export function useContextMenu(): ContextMenuController { // 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(): ContextMenuController { [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 }; } diff --git a/frontend/src/index.css b/frontend/src/index.css index 4d4cc533..ba7ded13 100644 --- a/frontend/src/index.css +++ b/frontend/src/index.css @@ -576,7 +576,10 @@ body { } } } -.glass-hover:hover { +/* `:not(:disabled)` for the same reason ui/Button prefixes every variant hover with `enabled:` — a + disabled control must not advertise a click that does nothing. The Tailwind half was gated in E10 + review round 1 and this half was missed, so a disabled glass button still lifted (round 2). */ +.glass-hover:not(:disabled):hover { border-color: color-mix(in srgb, var(--accent) 55%, transparent); box-shadow: inset 0 1px 0 color-mix(in srgb, #fff 12%, transparent), diff --git a/frontend/src/lib/chipLane.contract.test.ts b/frontend/src/lib/chipLane.contract.test.ts index 8f4e3ee9..4d9c4ee3 100644 --- a/frontend/src/lib/chipLane.contract.test.ts +++ b/frontend/src/lib/chipLane.contract.test.ts @@ -25,6 +25,14 @@ function count(haystack: string, needle: string): number { return haystack.split(needle).length - 1; } +/** Occurrences of `chipLaneClass` that are actually a LANE, i.e. not the import that brings it in. + * Counting raw occurrences gave every file one free mirror — the import — so a second, unprotected + * lane in a file that already had a compliant one still passed, which is the exact hole this test + * was rewritten to close (E10 review round 2, caught by mutation). */ +function laneCount(src: string): number { + return count(src.replace(/^\s*import[\s\S]*?from\s+"[^"]*";$/gm, ""), "chipLaneClass"); +} + /** 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]> => @@ -58,13 +66,15 @@ describe("chip lane containment contract", () => { .map(([name, src]) => ({ name, mirrors: count(src, MIRROR), - lanes: count(src, "chipLaneClass"), + lanes: laneCount(src), })) .filter((f) => f.lanes < f.mirrors); expect(offenders).toEqual([]); }); it("finds the measurers it claims to guard (the marker has not been renamed away)", () => { - expect(withMirror().length).toBeGreaterThanOrEqual(3); + // The MIRROR alone, not the hook: `lib/chipOverflow.ts` names the hook in a prose comment and was + // padding this canary, so it stayed satisfied with one real measurer fewer (round 2). + expect(sources().filter(([, src]) => src.includes(MIRROR)).length).toBeGreaterThanOrEqual(3); }); }); diff --git a/frontend/src/lib/globalKeymap.dom.test.tsx b/frontend/src/lib/globalKeymap.dom.test.tsx new file mode 100644 index 00000000..2df7ec95 --- /dev/null +++ b/frontend/src/lib/globalKeymap.dom.test.tsx @@ -0,0 +1,127 @@ +// @vitest-environment jsdom +// +// The `global` scope's LIVE behaviour, as opposed to `matches`/`dispatch` in isolation (keymap.test.ts +// covers those against a handler map the test supplies itself, which cannot see the real handlers). +// +// Every assertion here pins a fix that E10 review round 2 proved was un-pinned: reverting the guard +// each one covers left the whole suite green, which is how a fix round quietly becomes the next +// round's source of regressions. +import { useEffect } from "react"; +import { describe, it, expect, afterEach, vi } from "vitest"; +import { render, cleanup, act } from "@testing-library/react"; + +import { + closeKeyboardHelp, + closePalette, + useGlobalKeymap, + useKeyboardHelpOpen, + usePaletteOpen, +} from "./keymap"; + +vi.mock("react-i18next", async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, useTranslation: () => ({ t: (k: string) => k }) }; +}); + +afterEach(() => { + cleanup(); + closeKeyboardHelp(); + closePalette(); +}); + +/** Renders the live stores so an assertion reads what a real consumer would render. */ +function Host({ enabled = true, onState }: { enabled?: boolean; onState: (s: string) => void }) { + useGlobalKeymap(enabled); + const help = useKeyboardHelpOpen(); + const palette = usePaletteOpen(); + useEffect(() => { + onState(`${help ? "help" : ""}${palette ? "palette" : ""}` || "none"); + }, [help, palette, onState]); + return null; +} + +const key = (init: KeyboardEventInit) => + act(() => { + window.dispatchEvent( + new KeyboardEvent("keydown", { bubbles: true, cancelable: true, ...init }), + ); + }); + +describe("useGlobalKeymap", () => { + it("opens the cheat sheet on h and the palette on Mod+K", () => { + let state = ""; + render( (state = s)} />); + key({ key: "h" }); + expect(state).toBe("help"); + key({ key: "h" }); + expect(state).toBe("none"); + key({ key: "k", ctrlKey: true }); + expect(state).toBe("palette"); + }); + + // Mod+K is written as a toggle, and the palette autofocuses its own search field — so without + // `allowInInput` on the row the second press was eaten by the typing guard and Chrome took the + // chord instead (round 1). The FIRST press must still be routed from a field, too. + it("toggles the palette shut from inside a text field", () => { + let state = ""; + render( (state = s)} />); + const field = document.createElement("input"); + document.body.appendChild(field); + field.focus(); + + act(() => { + field.dispatchEvent( + new KeyboardEvent("keydown", { key: "k", ctrlKey: true, bubbles: true, cancelable: true }), + ); + }); + expect(state).toBe("palette"); + act(() => { + field.dispatchEvent( + new KeyboardEvent("keydown", { key: "k", ctrlKey: true, bubbles: true, cancelable: true }), + ); + }); + expect(state).toBe("none"); + field.remove(); + }); + + // `h` still belongs to whatever is being typed. + it("leaves a plain letter to the text field it was typed into", () => { + let state = ""; + render( (state = s)} />); + const field = document.createElement("input"); + document.body.appendChild(field); + field.focus(); + act(() => { + field.dispatchEvent( + new KeyboardEvent("keydown", { key: "h", bubbles: true, cancelable: true }), + ); + }); + expect(state).toBe("none"); + field.remove(); + }); + + // Each toggle mounts/unmounts an overlay that costs a real history entry, so ~30 of them a second + // desyncs the overlay bookkeeping from the browser's actual stack. Holding a key is not a press. + it("ignores auto-repeat", () => { + let state = ""; + render( (state = s)} />); + key({ key: "h", repeat: true }); + expect(state).toBe("none"); + key({ key: "k", ctrlKey: true, repeat: true }); + expect(state).toBe("none"); + }); + + // Bound above the branch that mounts the overlays, the keys were swallowed on the loading/Welcome + // screens and the flipped store then sprang the overlay open by itself once the shell mounted. + it("binds nothing, and clears a stale store, while disabled", () => { + let state = ""; + const { rerender } = render( (state = s)} />); + key({ key: "h" }); + expect(state).toBe("help"); + + rerender( (state = s)} />); + expect(state).toBe("none"); // the stale `true` is cleared, not carried into the next mount + key({ key: "h" }); + expect(state).toBe("none"); // and the listener is gone + }); +}); diff --git a/frontend/src/lib/history.ts b/frontend/src/lib/history.ts index 7989ba9a..1f33789b 100644 --- a/frontend/src/lib/history.ts +++ b/frontend/src/lib/history.ts @@ -146,6 +146,10 @@ let overlayCounter = 0; let historyDepth = 0; // Programmatic history.back() calls still awaiting their popstate — each must be ignored. let pendingProgrammatic = 0; +// Overlay entries a navigation has PUSHED PAST. They are still real history entries and their +// overlays may still be mounted, but they are no longer on top, so reclaiming one with history.back() +// would pop the navigation instead. Reconcile consumes them silently. See closeThenNavigate. +let buriedEntries = 0; let reconcileScheduled = false; let listenerInstalled = false; @@ -153,10 +157,17 @@ let listenerInstalled = false; function reconcileHistory(): void { reconcileScheduled = false; const want = overlayStack.length; + // A genuine user Back decrements historyDepth on its own, so a burial can outlive the entries it + // described; never claim more buried entries than there are entries. + if (buriedEntries > historyDepth) buriedEntries = historyDepth; while (historyDepth < want) { historyDepth++; window.history.pushState({ ...window.history.state, _ov: ++overlayCounter }, ""); } + while (historyDepth > want && buriedEntries > 0) { + historyDepth--; // buried: the browser entry stays, but it is not ours to reclaim + buriedEntries--; + } while (historyDepth > want) { historyDepth--; pendingProgrammatic++; @@ -212,14 +223,15 @@ export function closeThenNavigate(close: () => void, navigate: () => void): void 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; + // Every overlay entry still counted is now BURIED under the entry the navigation just pushed, so + // a later unmount must NOT reclaim it with history.back() — that pops the navigation instead and + // silently bounces the user back where they started (a palette opened over the player: closing + // the palette landed its popstate, the navigation unmounted the player too, and the player's + // reconcile ate the new page entry). MARK them rather than zeroing the depth: round 1 set + // `historyDepth = 0` outright, which also desynced the counter from overlays the navigation does + // NOT unmount — About/ReleaseNotes live outside the page column — so one Back both navigated and + // closed them (round 2). + buriedEntries = historyDepth; }; const onPop = () => { // The same reconcile can issue MORE than one back(); a pushState made between them would be diff --git a/frontend/src/lib/keymap.ts b/frontend/src/lib/keymap.ts index 6b85ee4f..d907e692 100644 --- a/frontend/src/lib/keymap.ts +++ b/frontend/src/lib/keymap.ts @@ -192,9 +192,17 @@ export function matches(e: KeyboardEvent, chord: string): boolean { * 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); + * editing the row changed the help sheet and nothing else (E10 review round 1). + * + * Keyed on scope AND id, both as literal types: ids are only unique WITHIN a scope (`back` is on + * both players), so a bare `ROWS.find(s => s.id === id)` would answer for whichever came first, and + * a typo'd string would silently never match (round 2). */ +export function matchesShortcut( + e: KeyboardEvent, + scope: S, + id: Extract["id"], +): boolean { + const row = ROWS.find((s) => s.scope === scope && s.id === id); return !!row?.keys.some((k) => matches(e, k)); } @@ -245,7 +253,11 @@ const tokens: { scope: KeyScope }[] = []; /** The live scopes, innermost first, always ending in `global`. Re-renders on change. */ export function useActiveScopes(): KeyScope[] { const stack = scopeStack.use(); - return [...stack].reverse().concat("global"); + // DEDUPED: a scope can be registered by more than one live component (the Playlists page and its + // rail each mount a `useContextMenu`), and `helpLines` appends a row's chords once per occurrence — + // so without this the sheet rendered "⇧ F10 Menu ⇧ F10 Menu" under a duplicated React key + // (E10 review round 2). Innermost registration wins the ordering. + return [...new Set([...stack].reverse())].concat("global"); } // ── The keyboard help sheet ───────────────────────────────────────────────────────────────────