From 4b33036e7c99bf5538044a13da60b18d41449aa5 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sun, 6 Sep 2026 01:57:32 +0200 Subject: [PATCH] =?UTF-8?q?fix(e10):=20review=20round=202=20=E2=80=94=20re?= =?UTF-8?q?vert=20what=20round=201=20got=20wrong,=20narrow=20what=20it=20g?= =?UTF-8?q?ot=20too=20broad?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 , so restoring to it is a spec no-op and Escape stranded focus on ; 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. --- CLAUDE.md | 47 ++++--- frontend/src/components/Playlists.tsx | 19 +-- frontend/src/components/PlaylistsRail.tsx | 20 ++- frontend/src/components/PlexPlayer.tsx | 19 ++- frontend/src/components/ShareDialog.tsx | 9 +- frontend/src/components/ui/ActionMenu.tsx | 24 ++-- .../src/components/ui/AnchoredPopover.tsx | 11 +- .../src/components/ui/Button.dom.test.tsx | 29 ++++ frontend/src/components/ui/CommandPalette.tsx | 11 +- .../components/ui/KeyboardHelp.dom.test.tsx | 38 +++++- frontend/src/components/ui/README.md | 9 +- frontend/src/components/ui/useContextMenu.ts | 49 +------ frontend/src/index.css | 5 +- frontend/src/lib/chipLane.contract.test.ts | 14 +- frontend/src/lib/globalKeymap.dom.test.tsx | 127 ++++++++++++++++++ frontend/src/lib/history.ts | 28 ++-- frontend/src/lib/keymap.ts | 20 ++- 17 files changed, 331 insertions(+), 148 deletions(-) create mode 100644 frontend/src/lib/globalKeymap.dom.test.tsx 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 ───────────────────────────────────────────────────────────────────