diff --git a/CHANGELOG.md b/CHANGELOG.md index 5a533e01..93de23a2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,31 @@ edit the release notes and re-run the generator. Upgrading, pinning a version and rolling back: [docs/self-hosting.md](docs/self-hosting.md#upgrading). +## 0.85.0 — 2026-09-13 + +Siftlode can now be driven from the keyboard, and Back, dialogs and menus stop losing your place. j/k and the arrow keys walk every list and grid, Enter opens what you are on, and Back puts you on the item you opened a view from instead of at the top of the page. + +### Features + +- j/k and the arrow keys walk the feed, the library, a channel page, the Home rails, every Plex grid and rail, the Channels, Playlists and Downloads tables and the conversation list; Enter opens the item you are on. After you hide or delete an item, the next j goes on from where it was. +- Back returns you to the item a view was opened from — the Plex title you opened from a Home rail, the channel you opened from its row — with the keyboard still on it. +- Shortcuts to get around: s jumps to the search field, g-chords jump to a module, and the cheat sheet (h) names every key in the language you are reading. PageUp/PageDown move a page at a time in the command palette and in every dropdown list. +- The command palette ranks what you typed first: “theme” now puts the Theme rows above a channel whose handle happens to contain it. +- A card reached with Tab or j shows one ring around the whole card, and a screen reader calls it by its title. + +### Fixes + +- Back and overlays keep their books straight: Back after navigating over About also closes About, a late clean-up no longer undoes the navigation you just made, and Shift+W on Home closes the player. +- Dialogs and menus hand the keyboard back where it came from. Escape from a card’s context menu returns to the card, not its thumbnail link; closing the player under another dialog no longer leaves the page unscrollable; a button that is busy with a request keeps the focus instead of dropping it to the page. +- A write that fails says so, with the reason — several saves, share links and admin actions used to fail silently or in English under the Hungarian interface. +- A split video export that fails part-way (the job limit) tells you how many clips were added and leaves only the rest selected, so trying again does not queue the first ones twice. +- “Feed” is called Feed everywhere, in both languages. + +### Internal + +- Five sprints: history integrity, one dialog/focus contract, palette ranking and accessibility, the keyboard layer, and a Hungarian/English keyboard-only pass. Focus and event-order questions were measured in a real browser before each fix, not argued. +- Three epic-end review rounds — 19 → 14, 18 → 12 and a targeted 14 → 14 — each regression test mutation-checked. The last round's findings all sat inside the previous round's own fixes, which is where the loop was closed. + ## 0.84.0 — 2026-09-12 A bug-fix release, from a list you wrote while using Siftlode. Two of these were destroying your own data: a YouTube video rewound itself when you reopened it, and clearing a resume point on a half-watched Plex item told Plex nothing. Subtitles now appear in seconds instead of minutes, the Plex player's bitrate is a real measurement rather than a guess, and a channel you turn full history on for fills in front of you instead of looking frozen. diff --git a/CLAUDE.md b/CLAUDE.md index caca1a13..c273a7e8 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -63,8 +63,9 @@ Frontend: `main.tsx` provider tree → `App.tsx` shell → `moduleRegistry` rout 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). +feature component or a domain API slice, and one layer down `lib/` may not import from `components/` at +all, not even a type (`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`. @@ -89,8 +90,11 @@ CQRS, a repository/DI framework, a router library, a generic CRUD-router factory | FE per-account state | `usePref` | raw `readAccount`/`writeAccount` or bespoke hooks | | FE action button | `ui/Button` — pick a `SIZES` rung | a hand-rolled ` )} - - {col.items.map((m) => ( + + {col.items.map((m, i) => ( - diff --git a/frontend/src/components/plexBrowseLists.tsx b/frontend/src/components/plexBrowseLists.tsx index 036d78c8..a6704e6a 100644 --- a/frontend/src/components/plexBrowseLists.tsx +++ b/frontend/src/components/plexBrowseLists.tsx @@ -11,6 +11,7 @@ import { useDebounced } from "../lib/useDebounced"; import { LoadingState, StateMessage } from "./ui/QueryState"; import PosterTile from "./PosterTile"; import PlexLibraryPicker from "./PlexLibraryPicker"; +import NavGrid from "./ui/NavGrid"; // A collection tile's sub-line: item count · total runtime (each dropped when unknown, e.g. show-only // collections have no movie runtime). @@ -49,10 +50,11 @@ export function PlexPlaylistsBrowse({ onOpen }: { onOpen: (id: number) => void } return

{t("plex.playlist.none")}

; return ( -
- {playlists.map((p) => ( + + {playlists.map((p, i) => ( } @@ -60,7 +62,7 @@ export function PlexPlaylistsBrowse({ onOpen }: { onOpen: (id: number) => void } className="h-28" /> ))} -
+ ); } @@ -215,10 +217,11 @@ export function PlexCollectionsBrowse({ onPick }: { onPick: (id: string, title: ) : collections.length === 0 ? (

{t("plex.filter.collectionEmpty")}

) : ( -
- {collections.map((c) => ( + + {collections.map((c, i) => ( ))} -
+ )} ); diff --git a/frontend/src/components/plexCards.dom.test.tsx b/frontend/src/components/plexCards.dom.test.tsx index f631aba8..90c039a0 100644 --- a/frontend/src/components/plexCards.dom.test.tsx +++ b/frontend/src/components/plexCards.dom.test.tsx @@ -5,7 +5,7 @@ // aria-label — a dropped label silently reintroduces an unnamed button for screen-reader users. i18n is // key-passthrough, so the asserted names are the i18n keys. import { describe, it, expect, afterEach, vi } from "vitest"; -import { render, screen, cleanup } from "@testing-library/react"; +import { render, screen, cleanup, fireEvent } from "@testing-library/react"; vi.mock("react-i18next", async (importOriginal) => { const actual = await importOriginal(); @@ -100,3 +100,50 @@ describe("EpisodeCard a11y", () => { expect(screen.getByRole("button", { name: "plex.playlist.addEpisode" })).toBeTruthy(); }); }); + +// E11 S4b: in a keyboard-walkable grid or rail the WHOLE card is the item (the ring goes round the +// poster and its title, D2) and the poster — the card's own role=button — is what Enter opens. +describe("Plex cards in a keyboard-walkable list", () => { + const cases = [ + [ + "PlexPosterCard", + (onOpen: () => void, navIndex?: number) => ( + + ), + ], + [ + "SeasonCard", + (onOpen: () => void, navIndex?: number) => ( + + ), + ], + [ + "EpisodeCard", + (onOpen: () => void, navIndex?: number) => ( + + ), + ], + ] as const; + + it.each(cases)("%s: the root is the item and the poster is its open element", (_, card) => { + const onOpen = vi.fn(); + const { container } = render(card(onOpen, 3)); + const root = container.firstElementChild as HTMLElement; + expect(root.getAttribute("data-nav-index")).toBe("3"); + const open = root.querySelector("[data-nav-open]")!; + expect(open.getAttribute("role")).toBe("button"); + fireEvent.click(open); + expect(onOpen).toHaveBeenCalledTimes(1); + }); + + it.each(cases)("%s: outside a list it is no item", (_, card) => { + const { container } = render(card(vi.fn())); + expect(container.querySelector("[data-nav-index]")).toBeNull(); + }); +}); diff --git a/frontend/src/components/plexCards.tsx b/frontend/src/components/plexCards.tsx index 4a6adc80..7f021a4d 100644 --- a/frontend/src/components/plexCards.tsx +++ b/frontend/src/components/plexCards.tsx @@ -4,6 +4,7 @@ import { type CSSProperties } from "react"; import { formatRuntime } from "../lib/format"; import { Carousel } from "./ui/Carousel"; import type { PlexCard, PlexCastMember, PlexFilters, PlexSeasonDetail } from "../lib/api"; +import { navItemAttrs } from "../lib/listNav"; // Presentational Plex cards + strips, split out of PlexBrowse (all purely prop-driven, no shared // state). Rendered by the browse grid, the person page, and the series show/season views. @@ -52,6 +53,7 @@ export function PlexPosterCard({ onInfo, onToggleWatched, menuProps, + navIndex, }: { card: PlexCard; onOpen: () => void; @@ -61,6 +63,8 @@ export function PlexPosterCard({ * A function of the card rather than a ready-made object, so every card surface in the app takes * `menuProps={menu.triggerProps}` and reads the same at every call site. */ menuProps?: (c: PlexCard) => Record; + /** Position in a keyboard-walkable grid or rail (lib/listNav); the poster is what Enter opens. */ + navIndex?: number; }) { const { t } = useTranslation(); const inProgress = (card.position_seconds ?? 0) > 0 && card.status !== "watched"; @@ -75,6 +79,7 @@ export function PlexPosterCard({
@@ -82,6 +87,7 @@ export function PlexPosterCard({ onClick={onOpen} role="button" tabIndex={0} + data-nav-open="" onKeyDown={(e) => { if (e.key === "Enter" || e.key === " ") { e.preventDefault(); @@ -242,11 +248,13 @@ export function SeasonCard({ onOpen, onToggleWatched, onAdd, + navIndex, }: { se: PlexSeasonDetail; onOpen: () => void; onToggleWatched?: () => void; onAdd?: () => void; + navIndex?: number; }) { const { t } = useTranslation(); const watched = se.status === "watched"; @@ -254,11 +262,15 @@ export function SeasonCard({ ? t("plex.series.markSeasonUnwatched") : t("plex.series.markSeasonWatched"); return ( -
+
{ if (e.key === "Enter" || e.key === " ") { e.preventDefault(); @@ -357,7 +369,7 @@ export function CastStrip({ style={{ background: "color-mix(in srgb, var(--card) 55%, transparent)" }} >

{t("plex.info.cast")}

- + {cast.map((c, i) => { const inner = ( <> @@ -390,6 +402,7 @@ export function CastStrip({ return clickable ? ( , ); const btn = screen.getByRole("button", { name: "Save" }) as HTMLButtonElement; - expect(btn.disabled).toBe(true); expect(btn.getAttribute("aria-busy")).toBe("true"); + expect(btn.getAttribute("aria-disabled")).toBe("true"); // the leading icon is swapped for a spinner expect(btn.querySelector("[data-testid=ic]")).toBeNull(); fireEvent.click(btn); expect(onClick).not.toHaveBeenCalled(); }); - it("loading disables even when an explicit disabled={false} is passed", () => { + // E11 S2, measured in Chrome: a focused button that becomes natively disabled drops the focus to + // and never gets it back, so every "Something else" press threw a keyboard user out of Home. + // jsdom has no focus fixup, so what is pinned here is the cause — native `disabled` stays off. + it("loading keeps the button natively enabled, so it keeps the focus", () => { + render(); + const btn = screen.getByRole("button", { name: "Reroll" }) as HTMLButtonElement; + expect(btn.disabled).toBe(false); + }); + + it("loading blocks clicks even when an explicit disabled={false} is passed", () => { // Regression: `disabled ?? loading` let an explicit boolean disabled shadow loading, so a // call-site with `disabled={!valid} loading={pending}` (valid → disabled=false) stayed - // clickable mid-mutation and double-submitted. The contract is: loading always disables. + // clickable mid-mutation and double-submitted. The contract is: loading always blocks. const onClick = vi.fn(); render( , ); - const btn = screen.getByRole("button", { name: "Create" }) as HTMLButtonElement; - expect(btn.disabled).toBe(true); - fireEvent.click(btn); + fireEvent.click(screen.getByRole("button", { name: "Create" })); expect(onClick).not.toHaveBeenCalled(); }); + it("a busy submit button cancels the form's submission", () => { + // Enter in a field submits through a click on the default button; the guard cancels that click. + const onSubmit = vi.fn((e: { preventDefault: () => void }) => e.preventDefault()); + render( +
+ +
, + ); + fireEvent.click(screen.getByRole("button", { name: "Send" })); + expect(onSubmit).not.toHaveBeenCalled(); + }); + + it("a non-busy button still calls onClick", () => { + const onClick = vi.fn(); + render(); + fireEvent.click(screen.getByRole("button", { name: "Go" })); + expect(onClick).toHaveBeenCalledTimes(1); + }); + it("a link gets no button padding, and an explicit size still wins", () => { // The size default is variant-dependent (link → "none"). Locking it because the failure is // silent: a link that quietly inherits px-3 py-2 reads as a button, and nothing errors. diff --git a/frontend/src/components/ui/Button.tsx b/frontend/src/components/ui/Button.tsx index 4db42540..98630d25 100644 --- a/frontend/src/components/ui/Button.tsx +++ b/frontend/src/components/ui/Button.tsx @@ -44,28 +44,32 @@ const VARIANTS: Record = { // and the ~100 hand-rolled ones had drifted into two different looks. That split IS the // inconsistency this epic exists to remove; the primitive follows the majority, not the reverse. outline: - "font-medium border border-border text-muted enabled:hover:text-accent enabled:hover:border-accent", - "outline-accent": "font-medium border border-accent/40 text-accent enabled:hover:bg-accent/10", + "font-medium border border-border text-muted enabled:not-aria-disabled:hover:text-accent enabled:not-aria-disabled:hover:border-accent", + "outline-accent": + "font-medium border border-accent/40 text-accent enabled:not-aria-disabled:hover:bg-accent/10", // Neutral until you reach for it. NOT `danger-soft`, which is already red at rest — the difference // is whether the button should advertise the destruction before the pointer arrives. "outline-danger": - "font-medium border border-border text-muted enabled:hover:text-danger enabled:hover:border-danger/40", - "outline-warn": "font-medium border border-warn/40 text-warn enabled:hover:bg-warn/10", - ghost: "font-medium text-muted enabled:hover:text-fg enabled:hover:bg-card", - glass: "font-medium glass-card glass-hover enabled:hover:text-accent", + "font-medium border border-border text-muted enabled:not-aria-disabled:hover:text-danger enabled:not-aria-disabled:hover:border-danger/40", + "outline-warn": + "font-medium border border-warn/40 text-warn enabled:not-aria-disabled:hover:bg-warn/10", + ghost: + "font-medium text-muted enabled:not-aria-disabled:hover:text-fg enabled:not-aria-disabled:hover:bg-card", + glass: "font-medium glass-card glass-hover enabled:not-aria-disabled:hover:text-accent", "danger-soft": - "font-medium border border-danger/60 text-danger enabled:hover:bg-danger/10 enabled:hover:border-danger", + "font-medium border border-danger/60 text-danger enabled:not-aria-disabled:hover:bg-danger/10 enabled:not-aria-disabled:hover:border-danger", // No font-weight here on purpose: BASE already sets font-medium, and a second font-* utility would // be a coin-flip (both are single-class utilities of equal specificity, and without tailwind-merge // the winner is decided by the generated stylesheet order, not by the class list order). - link: "font-semibold text-accent enabled:hover:underline", + link: "font-semibold text-accent enabled:not-aria-disabled:hover:underline", }; -// Why every hover above is `enabled:hover:` — Tailwind's `:not(:disabled)`. A disabled button that -// still lights up on hover advertises a click that does nothing, and the ONE migrated call site that -// had guarded its own hover (TagManager's Add) silently lost the guard when it moved onto this -// primitive (E10 review round 1). The contract belongs here, not in ~160 call sites: BASE dims a -// disabled button, and the variant must not paint over that dimming. +// Why every hover above is `enabled:not-aria-disabled:hover:` — neither natively disabled nor busy. A +// disabled button that still lights up on hover advertises a click that does nothing, and the ONE +// migrated call site that had guarded its own hover (TagManager's Add) silently lost the guard when it +// moved onto this primitive (E10 review round 1). `not-aria-disabled` is the `loading` half: a busy +// button stays natively enabled (see `loading` below) and must not light up either. The contract +// belongs here, not in ~160 call sites: BASE dims both, and the variant must not paint over it. // The geometry ladder, and the whole of it — a call site picks a rung, it does not invent one. The // rungs were derived from what the app actually used (counted during E10 S1b): `px-3 py-2 text-sm` @@ -86,7 +90,7 @@ const SIZES: Record = { // link looks like everywhere in this app) and the action buttons want medium. Two font-* utilities on // one element is a coin flip without tailwind-merge, so exactly one has to emit it — see the test. const BASE = - "inline-flex items-center justify-center transition disabled:opacity-50 disabled:cursor-not-allowed"; + "inline-flex items-center justify-center transition disabled:opacity-50 disabled:cursor-not-allowed aria-disabled:opacity-50 aria-disabled:cursor-not-allowed"; interface ButtonOwnProps extends ButtonHTMLAttributes { variant?: ButtonVariant; @@ -98,7 +102,8 @@ interface ButtonOwnProps extends ButtonHTMLAttributes { iconOnly?: boolean; /** Accessible name + tooltip — required for `iconOnly`, optional otherwise. */ label?: string; - /** Disable + swap the icon for a spinner + mark aria-busy. */ + /** Busy: swap the icon for a spinner, mark aria-busy + aria-disabled, and swallow activation — but + * NOT native `disabled`, which would throw the focus out of the page (see the render below). */ loading?: boolean; fullWidth?: boolean; } @@ -131,6 +136,7 @@ const ButtonImpl = forwardRef(function Button children, disabled, type, + onClick, ...rest }, ref, @@ -149,11 +155,23 @@ const ButtonImpl = forwardRef(function Button + )); + const press = (key: string) => + act(() => { + (document.activeElement ?? document.body).dispatchEvent( + new KeyboardEvent("keydown", { key, bubbles: true, cancelable: true }), + ); + }); + + it("is walked by j when keyboardNav is on", () => { + render( + + {cards()} + , + ); + press("j"); + expect(document.activeElement?.textContent).toBe("card 0"); + press("j"); + expect(document.activeElement?.textContent).toBe("card 1"); + }); + + it("leaves j alone without it", () => { + render({cards()}); + press("j"); + expect(document.activeElement).toBe(document.body); + }); +}); diff --git a/frontend/src/components/ui/Carousel.tsx b/frontend/src/components/ui/Carousel.tsx index 3ef705b4..ea858cb3 100644 --- a/frontend/src/components/ui/Carousel.tsx +++ b/frontend/src/components/ui/Carousel.tsx @@ -1,8 +1,9 @@ -import { useCallback, type ReactNode } from "react"; +import { useCallback, useRef, type ReactNode } from "react"; import { ChevronLeft, ChevronRight } from "lucide-react"; import { useTranslation } from "react-i18next"; import clsx from "clsx"; import { useScrollEdges } from "../../lib/useScrollEdges"; +import { useListNav } from "../../lib/listNav"; // Shared horizontal rail — the one place the app's "row of cards you scroll sideways" lives, replacing // the hand-rolled `flex gap overflow-x-auto` strips (Cast / Related / collections) and the future @@ -43,6 +44,7 @@ export function Carousel({ ariaLabel, gap = "gap-3", className, + keyboardNav = false, children, }: { /** Optional built-in header (rails). Omit for a bare scroller inside a card that owns its own heading. */ @@ -55,10 +57,24 @@ export function Carousel({ /** Tailwind gap class for the scroller (e.g. "gap-4"). */ gap?: string; className?: string; + /** The cards spread `navItemAttrs(i)` on their roots: `j`/`k`, ←→ and Enter walk the rail, and ↑↓ + * leave it for the rail above or below (lib/listNav). */ + keyboardNav?: boolean; children: ReactNode; }) { const { t } = useTranslation(); const { ref, el, left, right } = useHScroll(); + // The scroller is the list the keyboard walks. useHScroll holds the node through a callback ref, + // listNav reads a ref object — one node, both handed it. + const navRef = useRef(null); + const setScroller = useCallback( + (node: HTMLDivElement | null) => { + ref(node); + navRef.current = node; + }, + [ref], + ); + useListNav(navRef, { enabled: keyboardNav }); const page = useCallback( (dir: -1 | 1) => { @@ -112,9 +128,11 @@ export function Carousel({ )} + {/* The px-1/pt-1 against -mx-1/-mt-1 keep the layout and give the keyboard ring room: a + scroller clips its overflow, and the ring sits 4px outside the card it surrounds. */}
{ expect(screen.getByRole("combobox").getAttribute("aria-expanded")).toBe("false"); // closed }); }); + +// PageUp/PageDown (E11, owner request): the sidebar's channel filter and the Plex pickers run to +// hundreds of rows, and floating-ui's list navigation has only the arrows. jsdom has no layout, so a +// page is the fixed fallback of 10 rows; the measured page is pinned in lib/listPaging. +describe("Combobox · paging", () => { + const rows = (disabled: number[] = []) => + Array.from({ length: 25 }, (_, i) => ({ value: `Row ${i}`, disabled: disabled.includes(i) })); + const activeValue = (input: HTMLElement) => + document.getElementById(input.getAttribute("aria-activedescendant") ?? "")?.textContent; + + it("moves the active option a page at a time, stopping at both ends", () => { + wrap(); + const input = screen.getByRole("combobox"); + input.focus(); + fireEvent.focus(input); + expect(fireEvent.keyDown(input, { key: "PageDown" })).toBe(false); // the page does not scroll + expect(activeValue(input)).toBe("Row 10"); + expect(document.activeElement).toBe(input); + fireEvent.keyDown(input, { key: "PageDown" }); + fireEvent.keyDown(input, { key: "PageDown" }); + expect(activeValue(input)).toBe("Row 24"); // no wrap to the top + fireEvent.keyDown(input, { key: "PageUp" }); + expect(activeValue(input)).toBe("Row 14"); + }); + + it("never lands on a disabled option, and Enter commits where it landed", () => { + const onChange = vi.fn(); + wrap(); + const input = screen.getByRole("combobox"); + input.focus(); + fireEvent.focus(input); + fireEvent.keyDown(input, { key: "PageDown" }); + fireEvent.keyDown(input, { key: "Enter" }); + expect(onChange).toHaveBeenCalledWith(["Row 11"]); + }); + + it("leaves the key alone while the list is closed", () => { + wrap(); + expect(fireEvent.keyDown(screen.getByRole("combobox"), { key: "PageDown" })).toBe(true); + }); +}); diff --git a/frontend/src/components/ui/CommandPalette.dom.test.tsx b/frontend/src/components/ui/CommandPalette.dom.test.tsx index c73afbcb..f27e85aa 100644 --- a/frontend/src/components/ui/CommandPalette.dom.test.tsx +++ b/frontend/src/components/ui/CommandPalette.dom.test.tsx @@ -5,7 +5,7 @@ // (no i18n, no domain), so this asserts the shell only. i18n is key-passthrough. import { useState } from "react"; import { describe, it, expect, vi, afterEach, beforeAll } from "vitest"; -import { render, screen, fireEvent, cleanup } from "@testing-library/react"; +import { render, screen, fireEvent, cleanup, waitFor } from "@testing-library/react"; import CommandPalette, { type CommandGroup } from "./CommandPalette"; vi.mock("react-i18next", async (importOriginal) => { @@ -21,7 +21,7 @@ beforeAll(() => { afterEach(cleanup); // `choose` closes and waits for the dialog's own Back-to-close history entry to be reclaimed before -// running the command (see closeThenNavigate), so the command lands a tick later — hence the waits. +// running the command (see closeTopOverlayThenRun), so the command lands a tick later — hence the waits. const ran: string[] = []; const groups = (): CommandGroup[] => [ { @@ -160,3 +160,176 @@ describe("CommandPalette", () => { expect(ran).toEqual(["closed"]); }); }); + +// The owner's case (E11 S3): typing "theme" put a channel matched only through its handle above every +// "Theme: …" row, and Enter opened the channel. +describe("CommandPalette · ranking", () => { + const noop = () => {}; + const ranked = (): CommandGroup[] => [ + { + id: "channels", + label: "Channels", + items: [ + { id: "ch:melt", label: "The Meltzone Podcast", hint: "@themeltzone", run: noop }, + { id: "ch:talk", label: "Talk about themes", run: noop }, + ], + }, + { + id: "actions", + label: "Actions", + items: [ + { id: "theme:midnight", label: "Theme: Midnight", run: noop }, + { id: "theme:forest", label: "Theme: Forest", run: noop }, + ], + }, + ]; + + it("ranks rows inside each group, keeps the group order, and starts on the best match", () => { + render(); + type("theme"); + // label elsewhere (talk) > handle only (melt); label start (midnight = forest, group order kept) + expect(rows()).toEqual(["ch:talk", "ch:melt", "theme:midnight", "theme:forest"]); + // The highlight is what Enter takes (same `activeIndex`); the full close→run cycle cannot run in + // this file — see the note on "chooses the selected row on Enter". + expect(selected()).toBe("theme:midnight"); + }); + + it("ranks by the worst word first, so every word in the label beats a label start plus a keyword", () => { + const groups: CommandGroup[] = [ + { + id: "channels", + label: "Channels", + items: [ + { id: "weekly", label: "Podcast Weekly", keywords: "melt", run: noop }, + { id: "melt", label: "The Meltzone Podcast", run: noop }, + ], + }, + ]; + render(); + type("podcast melt"); + expect(rows()).toEqual(["melt", "weekly"]); + }); + + // The channel and playlist lists load after the palette opens — often while you are already typing. + it("follows the best match as a list lands, but keeps a row the arrows picked", () => { + const early: CommandGroup[] = [ranked()[0]!]; + const r = render(); + type("theme"); + expect(selected()).toBe("ch:talk"); + r.rerender(); + expect(selected()).toBe("theme:midnight"); + + press("ArrowDown"); // theme:forest, picked + r.rerender(); + expect(selected()).toBe("theme:forest"); + type("them"); // a new query goes back to following the best match + expect(selected()).toBe("theme:midnight"); + }); +}); + +describe("CommandPalette · accessibility", () => { + it("names each group by its heading", () => { + open(); + const channels = screen.getByRole("group", { name: "Channels" }); + expect( + [...channels.querySelectorAll("[role='option']")].map((o) => o.getAttribute("data-cmd-id")), + ).toEqual(["ch:1", "ch:2"]); + expect(screen.getByRole("group", { name: "Modules" })).toBeTruthy(); + }); + + it("is expanded only while there is something to choose, and says 'nothing' outside the listbox", () => { + open(); + expect(field().getAttribute("aria-expanded")).toBe("true"); + type("zzzz"); + expect(field().getAttribute("aria-expanded")).toBe("false"); + const listbox = screen.getByRole("listbox", { hidden: true }); + expect(listbox.hidden).toBe(true); + // aria-controls still names a real element + expect(document.getElementById(field().getAttribute("aria-controls")!)).toBe(listbox); + expect(listbox.contains(screen.getByText("palette.empty"))).toBe(false); + }); + + it("announces the result count politely, once the typing pauses", async () => { + open(); + const status = screen.getByRole("status"); + expect(status.textContent).toBe(""); // nothing to announce on a blank field + type("the"); + expect(status.textContent).toBe(""); // not per keystroke + await waitFor(() => expect(status.textContent).toBe("palette.results")); + type("zzzz"); + await waitFor(() => expect(status.textContent).toBe("palette.empty")); + type(""); + expect(status.textContent).toBe(""); + }); +}); + +// Owner request (E11): with 461 rows and ~11 in view, the arrows alone were a slog. PageUp/PageDown +// move by what fits in the list, and — unlike the arrows — stop at the ends instead of wrapping. +describe("CommandPalette · paging", () => { + const many = (): CommandGroup[] => [ + { + id: "rows", + label: "Rows", + items: Array.from({ length: 12 }, (_, i) => ({ + id: `r${i}`, + label: `Row ${i}`, + run: () => {}, + })), + }, + ]; + const openMany = () => render( {}} groups={many()} />); + const rect = (top: number, height: number) => + ({ + top, + bottom: top + height, + height, + left: 0, + right: 100, + width: 100, + x: 0, + y: top, + }) as DOMRect; + + it("pages by the rows in view minus one, and stops at both ends", () => { + // The list shows 100px and the rows are 30px tall: 3 fit, so a page is 2 rows. + const spy = vi.spyOn(Element.prototype, "getBoundingClientRect").mockImplementation(function ( + this: Element, + ) { + if (this.getAttribute("role") === "listbox") return rect(0, 100); + const all = [...document.querySelectorAll('[role="option"]')]; + return rect(all.indexOf(this) * 30, 30); + }); + // Restored in `finally`: a spy left behind by a failing assertion fails every later test too. + try { + openMany(); + press("PageDown"); + expect(selected()).toBe("r2"); + press("PageDown"); + expect(selected()).toBe("r4"); + for (let i = 0; i < 10; i++) press("PageDown"); + expect(selected()).toBe("r11"); // no wrap to the top + press("PageUp"); + expect(selected()).toBe("r9"); + for (let i = 0; i < 10; i++) press("PageUp"); + expect(selected()).toBe("r0"); + } finally { + spy.mockRestore(); + } + }); + + it("steps a fixed page when there is no layout to measure", () => { + openMany(); + press("PageDown"); + expect(selected()).toBe("r10"); + press("PageDown"); + expect(selected()).toBe("r11"); + }); + + it("leaves Home and End to the text field", () => { + openMany(); + press("PageDown"); + expect(fireEvent.keyDown(field(), { key: "Home" })).toBe(true); // not preventDefault-ed + expect(fireEvent.keyDown(field(), { key: "End" })).toBe(true); + expect(selected()).toBe("r10"); + }); +}); diff --git a/frontend/src/components/ui/CommandPalette.ordering.dom.test.tsx b/frontend/src/components/ui/CommandPalette.ordering.dom.test.tsx index 81cf801e..3191b6d1 100644 --- a/frontend/src/components/ui/CommandPalette.ordering.dom.test.tsx +++ b/frontend/src/components/ui/CommandPalette.ordering.dom.test.tsx @@ -1,10 +1,9 @@ // @vitest-environment jsdom // // ONE test, ONE file, on purpose. This is the close→reclaim→run ordering contract of the palette -// (see closeThenNavigate), and it ASSERTS on a real useBackToClose entry driven through a real -// programmatic history.back() — which is why it cannot share module state with anything. (It is not -// the only place in the suite where an unstubbed back() fires; history.burial.dom.test.ts is the -// other file that asserts on one, and any test rendering a real Modal can let one go.) +// (see closeTopOverlayThenRun), and it ASSERTS on a real useBackToClose entry driven through a real +// programmatic history.back() — which is why it cannot share module state with anything. (Any test +// rendering a real Modal can let an unstubbed back() go, too.) // // It cannot live in CommandPalette.dom.test.tsx. jsdom gives a test FILE a single session history, // and history.ts's overlay bookkeeping is module-global, so the sibling tests there leave an @@ -50,7 +49,7 @@ function Harness() { ); } -describe("CommandPalette · closeThenNavigate ordering", () => { +describe("CommandPalette · close-then-run ordering", () => { it("closes, and runs the command only once the close has landed", async () => { render(); // Load-bearing: useBackToClose pushes its entry from a coalesced microtask, so a click in the @@ -62,8 +61,9 @@ describe("CommandPalette · closeThenNavigate ordering", () => { fireEvent.click(screen.getByText("TheVR Podcast")); expect(ran).toEqual(["closed"]); - // Inside closeThenNavigate's 250ms no-popstate fallback, so a pass means the RECLAIM ran the - // command — not the safety net that exists for overlays which never got an entry at all. - await waitFor(() => expect(ran).toEqual(["closed", "ch:1"]), { timeout: 200 }); + // Well inside closeTopOverlayThenRun's 1500 ms give-up timer, so a pass means the RECLAIM ran the + // command — not the late path for a back() that never reports. (The margin used to be 200 ms + // under a 250 ms fallback, which made this load-sensitive by construction.) + await waitFor(() => expect(ran).toEqual(["closed", "ch:1"]), { timeout: 1000 }); }); }); diff --git a/frontend/src/components/ui/CommandPalette.tsx b/frontend/src/components/ui/CommandPalette.tsx index d0f00fca..d21fddbc 100644 --- a/frontend/src/components/ui/CommandPalette.tsx +++ b/frontend/src/components/ui/CommandPalette.tsx @@ -2,7 +2,8 @@ import { useEffect, useId, useMemo, useRef, useState, type ReactNode } from "rea import { useTranslation } from "react-i18next"; import { Search } from "lucide-react"; import Modal from "./Modal"; -import { closeThenNavigate } from "../../lib/history"; +import { closeTopOverlayThenRun } from "../../lib/history"; +import { pageStep, pageTarget } from "../../lib/listPaging"; // The app's jump-to bar: one text field over every place you can go and every app-wide thing you can // do, opened with Ctrl/Cmd+K. It is the "one-layer operation" half of E10 — reaching a channel three @@ -40,14 +41,37 @@ function fold(s: string): string { .replace(/\p{Diacritic}/gu, ""); } -/** Every query word must appear somewhere in the item's text. Word-wise rather than substring so - * "thevr pod" finds "TheVR Podcast" — the way you actually half-remember a name. */ -function matchesQuery(item: CommandItem, groupLabel: string, words: string[]): boolean { - if (words.length === 0) return true; - const hay = fold(`${item.label} ${item.hint ?? ""} ${item.keywords ?? ""} ${groupLabel}`); - return words.every((w) => hay.includes(w)); +/** How well an item answers the query: `[worst word, sum of words]`, lower is better. */ +type Rank = readonly [number, number]; + +const byRank = (a: Rank, b: Rank) => a[0] - b[0] || a[1] - b[1]; + +/** Every query word must appear somewhere in the item's text — word-wise rather than substring, so + * "thevr pod" finds "TheVR Podcast", the way you actually half-remember a name. `null` = no match. + * + * Each word scores 0 when the label STARTS with it, 1 when it is elsewhere in the label, 2 when only + * the hint, the hidden keywords or the group name carry it. An item ranks by its worst word first, so + * a row whose label holds every word beats one that needed its handle for any of them. Before this + * the list was pure group order: "theme" put the channel "The Meltzone Podcast" — matched through its + * handle `@themeltzone` alone — above every "Theme: …" row, and Enter opened a channel (E11 S3). */ +function rankItem(item: CommandItem, groupLabel: string, words: string[]): Rank | null { + const label = fold(item.label); + const rest = fold(`${item.hint ?? ""} ${item.keywords ?? ""} ${groupLabel}`); + let worst = 0; + let sum = 0; + for (const w of words) { + const score = label.startsWith(w) ? 0 : label.includes(w) ? 1 : rest.includes(w) ? 2 : -1; + if (score < 0) return null; + worst = Math.max(worst, score); + sum += score; + } + return [worst, sum]; } +/** The result count reaches a screen reader only once the typing pauses: a polite region rewritten on + * every keystroke queues one announcement per letter. */ +const ANNOUNCE_DELAY_MS = 400; + export default function CommandPalette({ open, onClose, @@ -59,42 +83,81 @@ export default function CommandPalette({ }) { const { t } = useTranslation(); const listId = useId(); + const groupId = useId(); const [query, setQuery] = useState(""); - const [active, setActive] = useState(0); + // The row the arrows or the pointer picked, by id; `null` follows the best match. An id rather than + // an index because the lists arrive asynchronously (channels, playlists load on open), and an index + // would quietly point at a different row once they land. + const [picked, setPicked] = useState(null); // 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); + const listRef = useRef(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. useEffect(() => { if (open) { setQuery(""); - setActive(0); + setPicked(null); } }, [open]); - const visible = useMemo(() => { + // The GROUP order stays fixed — modules, channels, playlists, views, actions read the same every + // time — and the rows are ranked inside each group. The best row overall starts highlighted, so + // Enter takes the best match even when it sits in a later group. + const { visible, bestId } = useMemo(() => { const words = fold(query.trim()).split(/\s+/).filter(Boolean); - return groups - .map((g) => ({ ...g, items: g.items.filter((i) => matchesQuery(i, g.label, words)) })) - .filter((g) => g.items.length > 0); + const out: CommandGroup[] = []; + let best: { id: string; rank: Rank } | null = null; + for (const g of groups) { + const ranked: { item: CommandItem; rank: Rank }[] = []; + for (const item of g.items) { + const rank = rankItem(item, g.label, words); + if (rank) ranked.push({ item, rank }); + } + if (ranked.length === 0) continue; + // `sort` is stable, so equal ranks keep the group's own order (modules in nav-rail order). + ranked.sort((a, b) => byRank(a.rank, b.rank)); + const top = ranked[0]!; + if (!best || byRank(top.rank, best.rank) < 0) best = { id: top.item.id, rank: top.rank }; + out.push({ ...g, items: ranked.map((r) => r.item) }); + } + return { visible: out, bestId: best?.id }; }, [groups, query]); // One flat list behind the grouped rendering — arrow keys walk results, not sections. const flat = useMemo(() => visible.flatMap((g) => g.items), [visible]); - const activeId = flat[Math.min(active, flat.length - 1)]?.id; + const activeId = picked !== null && flat.some((i) => i.id === picked) ? picked : bestId; + const activeIndex = flat.findIndex((i) => i.id === activeId); // Keep the highlighted row in view when the arrows walk past the fold. useEffect(() => { activeRef.current?.scrollIntoView({ block: "nearest" }); }, [activeId]); + const summary = !query.trim() + ? "" + : flat.length === 0 + ? t("palette.empty") + : t("palette.results", { count: flat.length }); + const [announced, setAnnounced] = useState(""); + useEffect(() => { + if (!summary) { + setAnnounced(""); + return; + } + const timer = window.setTimeout(() => setAnnounced(summary), ANNOUNCE_DELAY_MS); + return () => window.clearTimeout(timer); + }, [summary]); + if (!open) return null; - // Close, wait for this dialog's own Back-to-close entry to be reclaimed, THEN run. Most commands - // navigate, and a pushState issued before that reclaim is undone by it — see closeThenNavigate. - const choose = (item: CommandItem) => closeThenNavigate(onClose, item.run); + // Close, wait for this dialog's own Back-to-close entry to be reclaimed, THEN run: most commands + // navigate, and a pushState issued before that reclaim is undone by it. Only THIS dialog closes — + // a command that navigates closes whatever is underneath itself, one that does not (a theme) must + // leave it standing. See closeTopOverlayThenRun. + const choose = (item: CommandItem) => closeTopOverlayThenRun(onClose, item.run); // These live on the input rather than in the shared keymap: they are the field's own editing keys // (like a ); + landOn(stamped); + getByRole("textbox").focus(); + render(); + await sleep(150); + expect(document.activeElement).toBe(getByRole("textbox")); + }); +}); + +describe("rememberNavPosition · the list it comes back to (deep-review E11 round 2)", () => { + // Discover: a title opened from "Recently added" and watched — the page's cache expires and + // "Continue watching" fills with that very title, a rail ABOVE. Found by its place, focus landed in + // that rail, on a title never picked. + it("finds a named list by its name when a list appeared above it", async () => { + const Rails = () => ( + <> + + + + ); + const { unmount } = render(); + const stamped = leaveFrom("a1", 2); + unmount(); + landOn(stamped); + render( + <> + + + , + ); + await waitFor(() => expect(focused()).toBe("a1")); + }); + + it("gives up on an unnamed list when the page came back with another number of lists", async () => { + const { unmount } = render(); + const stamped = leaveFrom("a1", 2); + unmount(); + landOn(stamped); + render( + <> + + + , + ); + await sleep(150); + expect(focused()).toBeUndefined(); + }); +}); diff --git a/frontend/src/lib/history.navRestore.reload.dom.test.tsx b/frontend/src/lib/history.navRestore.reload.dom.test.tsx new file mode 100644 index 00000000..78f888d6 --- /dev/null +++ b/frontend/src/lib/history.navRestore.reload.dom.test.tsx @@ -0,0 +1,50 @@ +// @vitest-environment jsdom +// +// The stamp `rememberNavPosition` leaves in history.state survives a reload, but module state does not: +// the listener that reads it was installed by the first push, so after F5 the first Back onto a +// stamped entry restored nothing (deep-review E11 round 1). Its own file, because the point is a +// module that has made NO push yet — any other test in history.navPosition would have made one. +import { useRef } from "react"; +import { afterEach, expect, it } from "vitest"; +import { act, cleanup, render, waitFor } from "@testing-library/react"; +import "./history"; +import { navItemAttrs, useListNav } from "./listNav"; + +function List({ id, count }: { id: string; count: number }) { + const ref = useRef(null); + useListNav(ref, { enabled: true }); + return ( +
+ {Array.from({ length: count }, (_, i) => ( +
+ {id} + {i} +
+ ))} +
+ ); +} + +afterEach(() => { + cleanup(); + window.history.replaceState(null, ""); +}); + +it("restores on a landing before this load has pushed anything", async () => { + // An entry stamped before the reload: list 1 of 2, item 1. + const state = { sfPage: "home", sfNav: { list: 1, lists: 2, index: 1 } }; + window.history.replaceState(state, ""); + render( + <> + + + , + ); + act(() => { + window.dispatchEvent(new PopStateEvent("popstate", { state })); + }); + await waitFor(() => + expect((document.activeElement as HTMLElement | null)?.dataset.testid).toBe("b1"), + ); + expect(window.history.state.sfNav).toBeUndefined(); +}); diff --git a/frontend/src/lib/history.ts b/frontend/src/lib/history.ts index f9b03510..9f53e238 100644 --- a/frontend/src/lib/history.ts +++ b/frontend/src/lib/history.ts @@ -10,6 +10,7 @@ // // setPage() pushes a clean { sfPage } entry (dropping _sub/_ov), so each page starts at its root. import { useEffect, useRef, useState } from "react"; +import { navPosition, restoreNavPosition, type NavPosition } from "./listNav"; import { urlWithParams } from "./urlState"; // --- Per-history-entry scroll id + a nav bus (for scroll restoration) ------------------------ @@ -64,6 +65,37 @@ export function subscribeNav(cb: () => void): () => void { return () => navListeners.delete(cb); } +// --- The list item a view was left from (history.state.sfNav) ------------------------------------- +// See lib/listNav "The item a view was left from". Stamped onto the entry being LEFT, right before a +// push opens a new view over it; read when Back/Forward lands on that entry, and dropped from it there, +// so a later landing on the same entry — an overlay closing over it — cannot pull focus to an old item. +// Installed at load, not by the first push: a stamp survives a reload in history.state, and the first +// Back after one must still find a listener (deep-review E11 round 1). +let cancelNavRestore = () => {}; +if (typeof window !== "undefined") { + window.addEventListener("popstate", (e) => { + cancelNavRestore(); + const pos = (e.state as { sfNav?: NavPosition } | null)?.sfNav; + if (!pos) return; + const { sfNav: _spent, ...st } = (window.history.state ?? {}) as Record; + window.history.replaceState(st, ""); + cancelNavRestore = restoreNavPosition(pos); + }); +} + +/** Call right before a pushState that opens a new view over the current entry (a page, a sub-view, a + * channel, a tab). Returns the current state WITHOUT the stamp: a push that spreads the current state + * must spread this, so the new entry does not inherit a position it was never left from. */ +export function rememberNavPosition(): Record { + // A restore still waiting for the last landing's lists belongs to the page being left: left + // running, it would focus "list N, item M" of whatever page this push opens. + cancelNavRestore(); + const { sfNav: _stale, ...st } = (window.history.state ?? {}) as Record; + const pos = navPosition(); + window.history.replaceState(pos ? { ...st, sfNav: pos } : st, ""); + return st; +} + // --- Sub-views (value carried in history.state._sub) --------------------------------------- /** Optional address-bar mirroring for a sub-view, so a deep sub-view (e.g. a Plex show page) is * bookmarkable. history.state._sub stays the source of truth for Back/Forward; this only adds the URL. */ @@ -127,7 +159,7 @@ export function useHistorySubview( // A fresh sfScroll so the opened sub-view is its own scroll entry (starts at the top, and Back // restores the view we came from) — then notify PageScroller, which this push doesn't re-render. window.history.pushState( - { ...window.history.state, _sub: next, sfScroll: nextNavId() }, + { ...rememberNavPosition(), _sub: next, sfScroll: nextNavId() }, "", href, ); @@ -150,6 +182,15 @@ export function useHistorySubview( // listener mistook the leaving modal's back() for a genuine user Back and closed itself, and // (b) a mangled history where the pointer ended up BEHIND the surviving entry, so a later browser // Back walked off the app entirely instead of closing the modal. Coalescing removes both. +// +// A navigation never lands UNDER an open overlay: `setPage` and `openChannel` close every overlay +// first and push only once their entries are back (`closeOverlaysThenNavigate`). The design before +// E11 let the push land above the overlays and "buried" their entries with a counter, and that +// counter was a whole family of bugs — a burial spent by a different overlay closing first, a +// navigation that could not report whether it pushed and was assumed to have, one browser Back that +// both undid the navigation AND closed the dialog above it, and the dead Back press after that (the +// last two measured in Chrome, E11 S1). With nothing ever buried, the stack and the history agree by +// construction. type Overlay = { token: number; cb: () => void }; const overlayStack: Overlay[] = []; let overlayCounter = 0; @@ -157,64 +198,25 @@ 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; - -/** Run a navigation and mark the live overlay entries buried under it — but ONLY if it really - * pushed an entry. A navigation that reports `false` pushed nothing, so its overlays are still on - * top and must stay reclaimable: `setPage` early-returns when the requested page is already - * current, and Home is the default landing page, so "Shift+W while already on Home with the player - * open" navigated nowhere yet still buried the player's entry. The player's unmount then consumed - * the mark instead of issuing history.back(), orphaning its browser entry — and the next Back - * popped onto an identical `sfPage` with an empty overlay stack, i.e. a press that did nothing - * (round 2). One rule for both entry points below — but read the next paragraph before trusting it - * at the `closeThenNavigate` one, where most callers cannot report at all. - * - * It fails OPEN: only a literal `false` skips the mark, so a `navigate` that reports nothing is - * assumed to have pushed. That is what the code did unconditionally before, so nothing regressed by - * adding the rule — it just does not yet reach everywhere. Today only `navigateOverOverlays` - * (App's Shift+W → `setPage`) is fully covered. Of `closeThenNavigate`'s callers, only a palette - * row written as a bare `() => setPage(page)` forwards the boolean: `CommandItem.run` is typed - * `() => void`, so the block-bodied and non-navigating rows report nothing — and neither does - * `openChannel`, which has its OWN no-push branch (it `replaceState`s when the entry already - * carries `_chan`) and is reached both from a palette row and from PlayerModal's channel-name jump. - * Each of those still marks a live sibling overlay's entry buried under a push that never happened, - * costing one dead browser-Back press when that overlay is closed by its own control. - * - * Closing it is three changes, not one — widen `CommandItem.run` to `() => boolean | void`, return - * the verdict from the block-bodied rows, and give `openChannel` the boolean `setPage` now has — so - * it is deferred to the project backlog rather than bolted on here. - * - * Marking AFTER the call is safe: `navigate()` is synchronous and only `reconcileHistory` (a - * microtask) and the popstate listener ever move `historyDepth`. */ -function buryUnder(navigate: () => boolean | void): void { - if (navigate() !== false) buriedEntries = historyDepth; -} +// Re-checked after every reconcile — see `afterClosing`. +const syncWaiters = new Set<() => void>(); // Push/pop history entries until their count matches the live overlay count. Runs once per tick. 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++; window.history.back(); } + syncWaiters.forEach((cb) => cb()); } function scheduleReconcile(): void { @@ -240,74 +242,113 @@ function ensureOverlayListener(): void { }); } -/** Close an overlay that holds a `useBackToClose` entry and THEN navigate — in that order, waiting - * for the browser to confirm it. - * - * The trap: closing unmounts the overlay, whose cleanup reconciles the history by issuing a - * programmatic `history.back()`. A `pushState` made before that lands (i.e. anything `setPage` / - * `openChannel` does synchronously) is simply undone by it, so the navigation silently does - * nothing. PlayerModal hit this first — clicking a video's channel name only closed the player — - * and the command palette hit it again the day it was written, which is why the fix lives here - * instead of being re-derived at each call site. - * - * Only call this from INSIDE an overlay that uses `useBackToClose`: the popstate we wait for is - * that overlay's own entry being reclaimed, and without one it never arrives. */ -export function closeThenNavigate(close: () => void, navigate: () => boolean | void): void { - let done = false; - // Armed BEFORE the close, so it is a plain const and there is no declaration order to reason about. - // Degrades honestly if no popstate is coming at all — an overlay closed in the same tick it opened - // never pushed an entry, so there is nothing to reclaim and the command would hang forever. - // Navigating a beat late beats a control that silently does nothing. - const fallback = setTimeout(() => run(), 250); - const run = () => { - if (done) return; - done = true; - window.removeEventListener("popstate", onPop); - clearTimeout(fallback); - // 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). `buryUnder` skips the mark when the command reports it navigated - // nowhere — which here is only the palette's bare `() => setPage(page)` module rows; see the - // fail-open limit documented on `buryUnder` for the rows that cannot report. - buryUnder(navigate); - }; - const onPop = () => { - // The same reconcile can issue MORE than one back(); a pushState made between them would be - // popped by the next. Wait for the last one to land. - if (pendingProgrammatic > 0) return; - run(); - }; - window.addEventListener("popstate", onPop); - close(); +/** The history holds exactly one entry per live overlay and no programmatic back() is still in + * flight — so a pushState made now cannot be undone by one. */ +function historyInSync(): boolean { + return historyDepth === overlayStack.length && pendingProgrammatic === 0 && !reconcileScheduled; } -/** Navigate from OUTSIDE any overlay — a global keyboard shortcut — when one may be open. +// How long an overlay gets to actually close before its command is dropped. Closing is one React +// render (measured back()→popstate: 1–13 ms in Chrome); nothing in the app refuses a close today, +// but a future "discard your changes?" guard must not be navigated out from under. +const CLOSE_TIMEOUT_MS = 1500; +// An overlay's Back callback is a Back STEP, and one entry can hold several steps: the Plex player's +// panels share a single entry and Back peels one per press. Navigating over it asks again, a render +// apart (the callback reads what the last one closed), until the entry is gone or the timeout drops +// the command. +const RECLOSE_MS = 50; + +/** Close `targets` (via `close`), then `run` once they have unmounted AND the history has caught up. * - * `closeThenNavigate` is for a control that lives INSIDE the overlay and can close it, and it waits - * for that overlay's own entry to come back. A global key has neither: it does not know which - * overlay is open, or whether one is at all. It just navigates, and the page change unmounts - * whatever the module column was holding — whose reconcile then reclaims the entry the navigation - * had just pushed. Shift+W over an open player closed the player and snapped back to the page it - * started on, which reads as a dead shortcut. + * The trap: closing unmounts an overlay, whose cleanup reconciles by issuing a programmatic + * `history.back()`. A pushState made before that lands is undone by it, so a command that navigates + * silently does nothing but close. PlayerModal hit it first; the command palette the day it was + * written. * - * Same fix as `closeThenNavigate`, one step shorter: bury the live overlay entries FIRST, so a - * later unmount consumes the marks instead of issuing `history.back()`. There is no popstate to - * wait for, because nothing is being closed on purpose. An overlay the navigation does NOT unmount - * (the palette and the cheat sheet live above the page column) keeps its entry buried too — the - * same trade `closeThenNavigate` already makes, and the reason closing the palette afterwards no - * longer pops the page out from under the user. + * It waits for a STATE, not an event. The helper before E11 waited for "a popstate" plus a 250 ms + * fallback — but one reconcile can issue several back()s, an overlay closed in the tick it opened + * issues none, and the fallback then ran with back()s still in flight. Here the reconcile and every + * popstate re-check one predicate. * - * `navigate` reports whether it actually pushed — see `buryUnder`, which is why `setPage` returns a - * boolean rather than this guessing from `history.length` (a `back()` followed by a `pushState` - * leaves it unchanged) or from the entry's `sfScroll` id (`openChannel` pushes without minting - * one). */ -export function navigateOverOverlays(navigate: () => boolean | void): void { - buryUnder(navigate); + * `run` goes out on a fresh task, never inside the popstate dispatch that settled it: listeners + * registered before ours (NavigationProvider's) have read that event already, but ones after it + * have not, and a page applied from the OLD entry after our push would undo the navigation. */ +function afterClosing( + targets: readonly Overlay[], + close: () => void, + run: () => void, + reclose = false, +): void { + let done = false; + let queued = false; + const gone = () => targets.every((o) => !overlayStack.includes(o)); + const ready = () => !done && gone() && historyInSync(); + const stop = () => { + done = true; + syncWaiters.delete(check); + window.removeEventListener("popstate", check); + clearTimeout(timeout); + clearInterval(again); + }; + const again = reclose + ? setInterval(() => { + if (!done && !gone()) close(); + }, RECLOSE_MS) + : undefined; + function check(): void { + if (queued || !ready()) return; + queued = true; + setTimeout(() => { + queued = false; + if (!ready()) return; + stop(); + run(); + }, 0); + } + const timeout = setTimeout(() => { + if (done) return; + stop(); + // Still mounted: the overlay refused to close, so drop the command rather than navigate under + // it. Gone but out of sync: a back() never reported back — run late rather than never. + if (gone()) run(); + }, CLOSE_TIMEOUT_MS); + syncWaiters.add(check); + window.addEventListener("popstate", check); + close(); + check(); +} + +/** Navigate over whatever overlays are open: close them all (topmost first, the order Back peels + * them in), then navigate once their entries are back. Synchronous when nothing is open — the + * ordinary rail click. + * + * `setPage` and `openChannel` call it themselves, so no call site can forget it: a global shortcut, + * a toast's "View" clicked inside the player, a palette row, the player's channel link. Closing is + * what a page change already did to the module-owned overlays (it unmounts them); for the ones above + * the page column (About, the cheat sheet, the palette) it means the destination is what the user + * sees, not a dialog still standing over it. A navigation that turns out to go nowhere — Shift+W on + * Home with the player open — still closes, so the key always ends with the answer on screen (owner + * decision 2026-09-09). */ +export function closeOverlaysThenNavigate(navigate: () => void): void { + if (overlayStack.length === 0 && historyInSync()) { + navigate(); + return; + } + const targets = [...overlayStack]; + const close = () => + [...targets] + .reverse() + .filter((o) => overlayStack.includes(o)) + .forEach((o) => o.cb()); + afterClosing(targets, close, navigate, true); +} + +/** Close the overlay a control lives in, then run its command once that overlay's entry is back — + * the command palette's rows. Overlays underneath are left alone: a theme picked over the player + * must not close the player, and a row that navigates closes it through `setPage` anyway. */ +export function closeTopOverlayThenRun(close: () => void, run: () => void): void { + const top = overlayStack[overlayStack.length - 1]; + afterClosing(top ? [top] : [], close, run); } /** While the calling component is mounted, Back (or the browser/mouse back button) closes it via diff --git a/frontend/src/lib/keymap.sequence.test.ts b/frontend/src/lib/keymap.sequence.test.ts new file mode 100644 index 00000000..cf32f0d5 --- /dev/null +++ b/frontend/src/lib/keymap.sequence.test.ts @@ -0,0 +1,183 @@ +// @vitest-environment jsdom +// +// Two-step sequences (`g h`) and the page-level quiet rule, in their own file: the pending first step +// is module state, and a test that left one pending would turn the NEXT test's first key into its +// second step. Every test here starts a fake clock far past the previous test's, so nothing carries. +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { SHORTCUTS, dispatch, formatChord, helpLines, type ShortcutId } from "./keymap"; +import { pushLayer, removeLayer } from "./layerStack"; + +let clock = Date.UTC(2030, 0, 1); +beforeEach(() => { + vi.useFakeTimers({ toFake: ["Date"] }); + clock += 3_600_000; + vi.setSystemTime(clock); +}); +afterEach(() => vi.useRealTimers()); + +function press(init: KeyboardEventInit, on?: HTMLElement): KeyboardEvent { + const e = new KeyboardEvent("keydown", { bubbles: true, cancelable: true, ...init }); + (on ?? document.body).dispatchEvent(e); + return e; +} + +/** Every page row's handler, recording which one ran. */ +function pageHandlers(calls: string[]) { + return Object.fromEntries( + SHORTCUTS.filter((s) => s.scope === "page").map((s) => [s.id, () => void calls.push(s.id)]), + ) as { readonly [K in ShortcutId<"page">]: () => void }; +} +const listHandlers = (calls: string[]) => + ({ + next: () => calls.push("next"), + prev: () => calls.push("prev"), + left: () => {}, + right: () => {}, + up: () => {}, + down: () => {}, + open: () => {}, + }) as const; + +describe("two-step sequences", () => { + it("runs the row once both steps arrive, and not on the first alone", () => { + const calls: string[] = []; + expect(dispatch(press({ key: "g" }), "page", pageHandlers(calls))).toBe(true); + expect(calls).toEqual([]); + expect(dispatch(press({ key: "f" }), "page", pageHandlers(calls))).toBe(true); + expect(calls).toEqual(["goFeed"]); + }); + + // Every scope has its own window listener, so the `h` completing `g h` reaches the global one too. + it("keeps the completing key from every other scope, whichever listener runs first", () => { + const calls: string[] = []; + const global = { + help: () => calls.push("help"), + palette: () => {}, + home: () => {}, + }; + // another scope BEFORE the owner: the pending step belongs to `page`, so it stands aside + dispatch(press({ key: "g" }), "page", pageHandlers(calls)); + const h = press({ key: "h" }); + expect(dispatch(h, "global", global)).toBe(false); + expect(dispatch(h, "page", pageHandlers(calls))).toBe(true); + expect(calls).toEqual(["goHome"]); + + // another scope AFTER the owner: the owner stamped the event + dispatch(press({ key: "g" }), "page", pageHandlers(calls)); + const j = press({ key: "j" }); // breaks the sequence: no `g j` + expect(dispatch(j, "page", pageHandlers(calls))).toBe(false); + expect(dispatch(j, "list", listHandlers(calls))).toBe(false); + expect(calls).toEqual(["goHome"]); + }); + + it("gives up after the window, so a late key is an ordinary key again", () => { + const calls: string[] = []; + dispatch(press({ key: "g" }), "page", pageHandlers(calls)); + vi.setSystemTime(clock + 1_600); + expect(dispatch(press({ key: "j" }), "list", listHandlers(calls))).toBe(true); + expect(dispatch(press({ key: "f" }), "page", pageHandlers(calls))).toBe(false); + expect(calls).toEqual(["next"]); + }); + + it("is not broken by reaching for a modifier between the steps", () => { + const calls: string[] = []; + dispatch(press({ key: "g" }), "page", pageHandlers(calls)); + dispatch(press({ key: "Shift", shiftKey: true }), "page", pageHandlers(calls)); + dispatch(press({ key: "d" }), "page", pageHandlers(calls)); + expect(calls).toEqual(["goDownloads"]); + }); + + // No second step needs a modifier (D1), so a chord with one is the reader changing their mind: it + // must reach its own row. Swallowed, `g` then Ctrl+K opened the browser's search, not the palette. + it("lets a modifier chord abandon the sequence and match as itself", () => { + const calls: string[] = []; + const global = { + help: () => calls.push("help"), + palette: () => calls.push("palette"), + home: () => calls.push("home"), + }; + dispatch(press({ key: "g" }), "page", pageHandlers(calls)); + const k = press({ key: "k", ctrlKey: true, metaKey: true }); + expect(dispatch(k, "global", global)).toBe(true); + expect(dispatch(press({ key: "f" }), "page", pageHandlers(calls))).toBe(false); // not `g f` + expect(calls).toEqual(["palette"]); + }); + + it("does not start or finish inside a text field", () => { + const calls: string[] = []; + const input = document.createElement("input"); + document.body.append(input); + expect(dispatch(press({ key: "g" }, input), "page", pageHandlers(calls))).toBe(false); + dispatch(press({ key: "g" }), "page", pageHandlers(calls)); + expect(dispatch(press({ key: "f" }, input), "page", pageHandlers(calls))).toBe(false); + expect(calls).toEqual([]); + input.remove(); + }); +}); + +describe("page-level scopes under a layer", () => { + it("are quiet while a keyboard-owning layer is open", () => { + const calls: string[] = []; + const id = pushLayer(); + expect(dispatch(press({ key: "j" }), "list", listHandlers(calls))).toBe(false); + expect(dispatch(press({ key: "s" }), "search", { focus: () => calls.push("s") })).toBe(false); + dispatch(press({ key: "g" }), "page", pageHandlers(calls)); + dispatch(press({ key: "f" }), "page", pageHandlers(calls)); + removeLayer(id); + expect(calls).toEqual([]); + }); + + // The layer's OWN keys must keep working — that is the player the page keys are standing aside for. + it("leave the layer's own scope alone", () => { + const calls: string[] = []; + const id = pushLayer(); + expect( + dispatch(press({ key: "k" }), "plexPlayer", { playPause: () => calls.push("pp") } as never), + ).toBe(true); + removeLayer(id); + expect(calls).toEqual(["pp"]); + }); + + it("keep working under a passive layer (an open tooltip)", () => { + const calls: string[] = []; + const id = pushLayer({ passive: true }); + expect(dispatch(press({ key: "j" }), "list", listHandlers(calls))).toBe(true); + removeLayer(id); + expect(calls).toEqual(["next"]); + }); +}); + +describe("the page-level rows", () => { + // Owner rule D1, enforced rather than commented: on the Hungarian layout the owner types on, a + // chord with a modifier (or `/`, `?`, which ARE Shift chords there) is not a quick key. + it("need no modifier and no Shift-only character", () => { + for (const s of SHORTCUTS) { + if (!["page", "search", "list"].includes(s.scope)) continue; + for (const chord of s.keys) { + for (const step of chord.split(" ")) { + expect(step.includes("+"), `${s.scope}:${s.id} ${chord}`).toBe(false); + expect(/^[a-z0-9,.-]$|^(Enter|Escape|Arrow(Up|Down|Left|Right))$/.test(step), chord).toBe( + true, + ); + } + } + } + }); + + it("render as the keys to press, step by step", () => { + const words = { Space: "Space", ContextMenu: "Menu" }; + expect(formatChord("g h", words)).toBe("G H"); + expect(formatChord("g ,", words)).toBe("G ,"); + }); + + // `g h` shares Home's line with Shift+W; leaving out an unreachable chord must not take that with it. + it("drop an unreachable module's chord but keep the line it shares", () => { + const lines = helpLines(["page", "global"], ["home", "feed"]); + expect(lines.find((l) => l.labelKey === "keys.home")?.chords).toEqual(["g h", "Shift+w"]); + expect(lines.some((l) => l.labelKey === "keys.goFeed")).toBe(true); + expect(lines.some((l) => l.labelKey === "keys.goPlex")).toBe(false); + expect( + helpLines(["page", "global"], []).find((l) => l.labelKey === "keys.home")?.chords, + ).toEqual(["Shift+w"]); + }); +}); diff --git a/frontend/src/lib/keymap.test.ts b/frontend/src/lib/keymap.test.ts index 681a3c25..7377bf68 100644 --- a/frontend/src/lib/keymap.test.ts +++ b/frontend/src/lib/keymap.test.ts @@ -213,11 +213,19 @@ describe("helpLines", () => { }); describe("formatChord", () => { + const words = { Space: "Szóköz", ContextMenu: "Menü" }; + it("renders glyphs and upper-cases the letter", () => { - expect(formatChord("ArrowLeft")).toBe("←"); - expect(formatChord("Backspace")).toBe("⌫"); - expect(formatChord("Shift+ArrowRight")).toBe("⇧ →"); - expect(formatChord("k")).toBe("K"); - expect(formatChord("Space")).toBe("Space"); + expect(formatChord("ArrowLeft", words)).toBe("←"); + expect(formatChord("Backspace", words)).toBe("⌫"); + expect(formatChord("Shift+ArrowRight", words)).toBe("⇧ →"); + expect(formatChord("k", words)).toBe("K"); + }); + + // A key whose cap carries a word is named in the reader's language: the Hungarian sheet said "Space". + it("names a word key in the caller's language", () => { + expect(formatChord("Space", words)).toBe("Szóköz"); + expect(formatChord("ContextMenu", words)).toBe("Menü"); + expect(formatChord("Shift+F10", words)).toBe("⇧ F10"); }); }); diff --git a/frontend/src/lib/keymap.ts b/frontend/src/lib/keymap.ts index ee47c000..19501eb7 100644 --- a/frontend/src/lib/keymap.ts +++ b/frontend/src/lib/keymap.ts @@ -1,5 +1,7 @@ -import { useEffect, useRef } from "react"; +import { useEffect, useRef, type RefObject } from "react"; +import { anyBlockingLayer } from "./layerStack"; import { createStore } from "./store"; +import type { Page } from "./urlState"; // ── The app's ONE keyboard map ──────────────────────────────────────────────────────────────── // Before E10 S3 six surfaces each owned a private `keydown` listener with its own key literals and @@ -18,8 +20,26 @@ import { createStore } from "./store"; // one place, and the sheet renders this table rather than a hand-copied array beside it. /** Which surface a shortcut belongs to. `global` is live app-wide; the rest only while that surface - * is mounted. The help sheet shows `global` plus the topmost registered scope. */ -export type KeyScope = "global" | "undo" | "contextMenu" | "ytPlayer" | "plexPlayer" | "plexBrowse"; + * is mounted. The help sheet shows `global` plus the registered scopes. + * + * `page`, `search` and `list` are the PAGE-LEVEL scopes (E11 S4): they act on the page underneath + * every overlay, so `dispatch` keeps them quiet while a layer that owns the keyboard is open. That is + * also what keeps them clear of the players' bare letters — the Plex player's `s` and `k` only ever + * run with the player's own layer up. */ +export type KeyScope = + | "global" + | "page" + | "search" + | "list" + | "undo" + | "contextMenu" + | "ytPlayer" + | "plexPlayer" + | "plexBrowse"; + +const PAGE_LEVEL: ReadonlySet = new Set(["page", "search", "list"]); +/** True for a scope that acts on the page beneath the overlays (see `KeyScope`). */ +export const isPageLevelScope = (scope: KeyScope): boolean => PAGE_LEVEL.has(scope); /** Extra render condition for the help sheet (the shortcut itself is always live). */ type ShortcutWhen = "episode"; @@ -42,11 +62,19 @@ export interface Shortcut { * and a player whose Escape died inside its own subtitle-offset input is a trap. Every other * shortcut stays out of the way of typing. */ readonly allowInInput?: boolean; + /** The module a `g` chord jumps to. The sheet lists the row only while that module is available + * to this account, and the handler refuses it otherwise — a gated module has no page to land on. */ + readonly page?: Page; } // Chord syntax: `Mod` = Ctrl on Windows/Linux, ⌘ on macOS. `Shift`/`Alt` are literal. The final // segment is a `KeyboardEvent.key` name, except `Space` (which also accepts `code`, because a -// focused button reports `key === " "` inconsistently across engines). +// focused button reports `key === " "` inconsistently across engines). A SPACE separates the two +// steps of a sequence: `g h` is `g`, then `h` within SEQUENCE_MS. +// +// Owner rule (E11 D1): no new binding may need a modifier on the Hungarian QWERTZ layout — so no `/` +// or `?` (both Shift chords there). Letters, digits, `-`, `,`, `.`, Esc/Enter/arrows only. `keyMatches` +// compares `e.key`, so a letter follows the printed legend (the HU Z/Y swap included). export const SHORTCUTS = [ // --- global --------------------------------------------------------------------------------- // `h`, not `?`: on a Hungarian layout `?` is a Shift chord, and the one help key people already @@ -60,6 +88,50 @@ export const SHORTCUTS = [ // sits directly above `s`, so a blind reach for the subtitle key would yank you out of the film // and onto the landing page. A chord makes it the deliberate "I'm stuck — answer the question". { id: "home", scope: "global", keys: ["Shift+w"], labelKey: "keys.home" }, + + // --- page-level: jump to a module (quiet under an overlay) ----------------------------------- + // English initials, because the Hungarian ones collide (Letöltések / Lejátszási listák). `g h` is + // Shift+W's modifier-free twin and shares its help line. Admin pages stay on the palette. + { id: "goHome", scope: "page", keys: ["g h"], labelKey: "keys.home", page: "home" }, + { id: "goFeed", scope: "page", keys: ["g f"], labelKey: "keys.goFeed", page: "feed" }, + { id: "goChannels", scope: "page", keys: ["g c"], labelKey: "keys.goChannels", page: "channels" }, + { + id: "goPlaylists", + scope: "page", + keys: ["g p"], + labelKey: "keys.goPlaylists", + page: "playlists", + }, + { id: "goPlex", scope: "page", keys: ["g x"], labelKey: "keys.goPlex", page: "plex" }, + { + id: "goNotifications", + scope: "page", + keys: ["g n"], + labelKey: "keys.goNotifications", + page: "notifications", + }, + { id: "goMessages", scope: "page", keys: ["g m"], labelKey: "keys.goMessages", page: "messages" }, + { + id: "goDownloads", + scope: "page", + keys: ["g d"], + labelKey: "keys.goDownloads", + page: "downloads", + }, + { id: "goStats", scope: "page", keys: ["g s"], labelKey: "keys.goStats", page: "stats" }, + { id: "goSettings", scope: "page", keys: ["g ,"], labelKey: "keys.goSettings", page: "settings" }, + + // --- the module's search box (wherever a SearchBar is mounted) ------------------------------- + { id: "focus", scope: "search", keys: ["s"], labelKey: "keys.search" }, + + // --- list / grid navigation (wherever a list registers with `useListNav`) -------------------- + { id: "next", scope: "list", keys: ["j"], labelKey: "keys.listStep" }, + { id: "prev", scope: "list", keys: ["k"], labelKey: "keys.listStep" }, + { id: "left", scope: "list", keys: ["ArrowLeft"], labelKey: "keys.gridMove" }, + { id: "right", scope: "list", keys: ["ArrowRight"], labelKey: "keys.gridMove" }, + { id: "up", scope: "list", keys: ["ArrowUp"], labelKey: "keys.gridMove" }, + { id: "down", scope: "list", keys: ["ArrowDown"], labelKey: "keys.gridMove" }, + { id: "open", scope: "list", keys: ["Enter"], labelKey: "keys.open" }, // --- the row context menu (wherever useContextMenu is wired) --------------------------------- // Its OWN scope, not `global`: only the surfaces that actually call `useContextMenu` register it, // so the sheet stops advertising "Open the context menu" on the feed, Plex, Downloads and Settings @@ -123,7 +195,7 @@ export const SHORTCUTS = [ { id: "stats", scope: "plexPlayer", keys: ["d"], labelKey: "keys.stats" }, // Two rows, one help line: ⌫ is the plain "stop & out", Escape is the layer-aware cascade (defer to // anything above → cancel auto-skip → close the topmost player overlay → close). Sharing a labelKey - // keeps them a single "⌫ Esc Stop & back to feed" row in the sheet. + // keeps them a single "⌫ Esc Stop & go back" row in the sheet. { id: "back", scope: "plexPlayer", keys: ["Backspace"], labelKey: "keys.back" }, { id: "escape", @@ -188,10 +260,6 @@ export function matches(e: KeyboardEvent, chord: string): boolean { return keyMatches(e, key); } -/** Run `scope`'s map against an event. Returns true when a shortcut matched (the handler decides - * whether to `preventDefault`, because only it knows if it actually acted). `handlers` is a mapped - * type over the scope's ids, so the table and the implementation cannot drift without tsc saying - * so. Skipped while typing unless the row opts in with `allowInInput` (Escape, and only Escape). */ /** Test an event against ONE row's chords, by id. The escape hatch for a `handledLocally` row: it is * 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 @@ -210,19 +278,94 @@ export function matchesShortcut( return !!row?.keys.some((k) => matches(e, k)); } +// ── Two-step sequences (`g h`) ───────────────────────────────────────────────────────────────── +// Each scope keeps its own window listener, so a sequence cannot live in one listener's closure: the +// `h` that completes `g h` also reaches the global listener (where `h` opens the sheet) and every +// other live scope. So the pending first step is module state, and it belongs to the scope that +// started it — every OTHER scope ignores the next key while it is pending, and the owner stamps the +// event once it has resolved it, so a scope whose listener runs after the owner's ignores it too. +// The stamp is the layerStack Escape trick: every listener in one dispatch gets the same object. +const SEQUENCE_MS = 1500; +let pending: { scope: KeyScope; key: string; at: number } | null = null; +const sequenceEvents = new WeakSet(); +// A modifier's own keydown (reaching for Shift) neither completes nor breaks a sequence. +const MODIFIER_KEYS = new Set(["Shift", "Control", "Alt", "AltGraph", "Meta", "CapsLock"]); + +function liveSequence(e: KeyboardEvent): typeof pending { + if (!pending) return null; + if (Date.now() - pending.at >= SEQUENCE_MS) { + pending = null; + return null; + } + if (MODIFIER_KEYS.has(e.key)) return null; + // A chord with a modifier cannot be a second step (no binding may need one, D1): it abandons the + // sequence and is matched as itself, or `g` then Mod+K would open the browser's search, not ours. + if (e.ctrlKey || e.metaKey || e.altKey || e.shiftKey) { + pending = null; + return null; + } + return pending; +} + +const stepsOf = (chord: string): string[] => chord.split(" "); + +/** Run `scope`'s map against an event. Returns true when a shortcut matched (the handler decides + * whether to `preventDefault`, because only it knows if it actually acted) or a sequence's first + * step was taken. `handlers` is a mapped type over the scope's ids, so the table and the + * implementation cannot drift without tsc saying so. Skipped while typing unless the row opts in + * with `allowInInput` (Escape, and only Escape), and — for a page-level scope — while a layer that + * owns the keyboard is open. The rule lives here rather than in each listener so a new page-level + * surface cannot forget it. */ export function dispatch( e: KeyboardEvent, scope: S, handlers: { readonly [K in ShortcutId]: (e: KeyboardEvent) => void }, ): boolean { + if (sequenceEvents.has(e)) return false; + const live = liveSequence(e); + if (live && live.scope !== scope) return false; + const quiet = isPageLevelScope(scope) && anyBlockingLayer(); const typing = isTypingTarget(e); + const run = (id: string) => (handlers as Record void>)[id]?.(e); + + if (live) { + // This key ends the sequence either way: it completes a row, or it breaks the attempt. + pending = null; + sequenceEvents.add(e); + if (quiet || typing) return false; + for (const s of ROWS) { + if (s.scope !== scope || s.handledLocally) continue; + const hit = s.keys.some((k) => { + const [first, second] = stepsOf(k); + return second !== undefined && first === live.key && matches(e, second); + }); + if (hit) { + run(s.id); + return true; + } + } + return false; + } + + if (quiet) return false; for (const s of ROWS) { if (s.scope !== scope || s.handledLocally) continue; if (typing && !s.allowInInput) continue; - if (!s.keys.some((k) => matches(e, k))) continue; - (handlers as Record void>)[s.id]?.(e); + if (!s.keys.some((k) => stepsOf(k).length === 1 && matches(e, k))) continue; + run(s.id); return true; } + if (typing) return false; + // Nothing matched outright — does this key open one of the scope's sequences? + for (const s of ROWS) { + if (s.scope !== scope || s.handledLocally) continue; + const first = s.keys.map(stepsOf).find((st) => st.length === 2 && matches(e, st[0]!))?.[0]; + if (first !== undefined) { + pending = { scope, key: first, at: Date.now() }; + sequenceEvents.add(e); + return true; + } + } return false; } @@ -261,18 +404,40 @@ export function useActiveScopes(): KeyScope[] { // 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"); + const live = [...new Set([...stack].reverse())]; + // The page-level scopes go in a FIXED order after the surfaces, not in registration order: App + // registers `page` in its own effect, which React runs after every child's — so by mount order + // the module jumps would head the sheet, above the list keys of the page you are looking at. + const surfaces = live.filter((s) => !isPageLevelScope(s)); + const pageLevel = (["list", "search", "page"] as const).filter((s) => live.includes(s)); + return [...surfaces, ...pageLevel, "global"]; } // ── The keyboard help sheet ─────────────────────────────────────────────────────────────────── // One sheet for the whole app, opened by the global `h` (or a surface's own keyboard button), so // there is exactly one answer to "what can I press here" instead of one per player. const helpOpen = createStore(false); +// Whether the sheet was opened OVER a layer that owns the keyboard (a player, a dialog). The page-level +// keys are quiet there, so the sheet must not list them — and it cannot ask the layer stack itself +// once it is showing, because its own Modal is a layer too. So the answer is taken at the moment of +// opening, before that Modal mounts. (The palette's "Help" row runs only after the palette has closed.) +const helpOverLayer = createStore(false); export const closeKeyboardHelp = (): void => helpOpen.set(false); -export const toggleKeyboardHelp = (): void => helpOpen.set((v) => !v); +export const toggleKeyboardHelp = (): void => { + const next = !helpOpen.get(); + if (next) helpOverLayer.set(anyBlockingLayer()); + helpOpen.set(next); +}; /** Subscribe to whether the sheet is showing (the Plex player suppresses wheel-volume under it). */ export const useKeyboardHelpOpen = (): boolean => helpOpen.use(); +/** Whether the sheet was opened over a keyboard-owning layer (see `helpOverLayer`). */ +export const useKeyboardHelpOverLayer = (): boolean => helpOverLayer.use(); + +// The modules this account can reach — published by `useGlobalKeymap`, read by the `g` chords and +// by the sheet (which drops the chord of a gated module rather than promise a page that isn't there). +const navigablePages = createStore([]); +export const useNavigablePages = (): readonly Page[] => navigablePages.use(); // ── The command palette ─────────────────────────────────────────────────────────────────────── const paletteOpen = createStore(false); @@ -280,21 +445,32 @@ const paletteOpen = createStore(false); export const closePalette = (): void => paletteOpen.set(false); export const usePaletteOpen = (): boolean => paletteOpen.use(); -/** Bind the `global` scope, once, for the whole app. It lives here rather than inside either - * overlay because both are opened by it: a listener that sat in one of them would have to route a - * key belonging to the other, and `dispatch`'s exhaustiveness would stop meaning anything. App - * calls this; the overlays are pure renderers of their store. +/** Bind the `global` and `page` scopes, once, for the whole app. It lives here rather than inside + * either overlay because both are opened by it: a listener that sat in one of them would have to + * route a key belonging to the other, and `dispatch`'s exhaustiveness would stop meaning anything. + * App calls this; the overlays are pure renderers of their store. + * + * `pages` is the set of modules this account can reach (a `g` chord to anything else is a no-op). * * `enabled` must track where those overlays (and the pages Shift+W jumps to) are actually MOUNTED. * Bound unconditionally it swallowed `h` and Mod+K on the loading/Welcome/setup screens — the * browser shortcut was preventDefault'ed, nothing appeared, and the flipped module-level store then * popped the overlay open by itself the moment the signed-in shell mounted (E10 review round 1). */ -export function useGlobalKeymap(enabled: boolean, onHome: () => void): void { - // The Home jump is the one global key whose action lives OUTSIDE this file — navigation is a +export function useGlobalKeymap( + enabled: boolean, + onNavigate: (page: Page) => void, + pages: readonly Page[], +): void { + // The page jumps are the global keys whose action lives OUTSIDE this file — navigation is a // provider and `lib/` may not import a component — so App hands it in. Kept in a ref so a fresh // closure each render doesn't rebind the window listener (and can't leave a stale `setPage`). - const homeRef = useRef(onHome); - homeRef.current = onHome; + const navRef = useRef(onNavigate); + navRef.current = onNavigate; + useKeyScope("page", enabled); + const pagesKey = pages.join(","); + useEffect(() => { + navigablePages.set(pagesKey ? (pagesKey.split(",") as Page[]) : []); + }, [pagesKey]); useEffect(() => { if (!enabled) { // Leaving a stale `true` behind would spring the overlay open on the next mount. @@ -302,6 +478,17 @@ export function useGlobalKeymap(enabled: boolean, onHome: () => void): void { paletteOpen.set(false); return; } + // Built from the rows, so a new `g` row needs no second edit here — the row's `page` IS the action. + const pageHandlers = Object.fromEntries( + ROWS.filter((s) => s.scope === "page").map((s) => [ + s.id, + (ev: KeyboardEvent) => { + if (!s.page || !navigablePages.get().includes(s.page)) return; + ev.preventDefault(); + navRef.current(s.page); + }, + ]), + ) as { readonly [K in ShortcutId<"page">]: (e: KeyboardEvent) => void }; const onKey = (e: KeyboardEvent) => { // Two of the three global keys TOGGLE an overlay, and each overlay mount/unmount costs a real // history entry (useBackToClose); the third (Shift+W) pushes a page entry of its own. Either @@ -309,10 +496,10 @@ export function useGlobalKeymap(enabled: boolean, onHome: () => void): void { // our own depth bookkeeping keeps counting them — a desynced stack whose next browser Back // walks out of the app. Holding a key is not a second press. if (e.repeat) return; - dispatch(e, "global", { + const handled = dispatch(e, "global", { help: (ev) => { ev.preventDefault(); - helpOpen.set((v) => !v); + toggleKeyboardHelp(); }, palette: (ev) => { // Mod+K is the browser's "focus the search bar" in a few builds; we want it. @@ -321,9 +508,10 @@ export function useGlobalKeymap(enabled: boolean, onHome: () => void): void { }, home: (ev) => { ev.preventDefault(); - homeRef.current(); + navRef.current("home"); }, }); + if (!handled) dispatch(e, "page", pageHandlers); }; window.addEventListener("keydown", onKey); return () => window.removeEventListener("keydown", onKey); @@ -344,6 +532,30 @@ export function useHelpConditions(conditions: HelpConditions): void { }, [episode]); } +// ── The module's search box ─────────────────────────────────────────────────────────────────── +/** Bind `s` to "put the caret in this search box" while the box is mounted. The one SearchBar in the + * header calls this; the text is selected so the next keystrokes replace the old query (an arrow key + * keeps it). `preventDefault` keeps the `s` itself out of the field it just focused. */ +export function useSearchFocusKey(input: RefObject): void { + useKeyScope("search"); + useEffect(() => { + const onKey = (e: KeyboardEvent) => { + if (e.repeat) return; + dispatch(e, "search", { + focus: (ev) => { + const el = input.current; + if (!el) return; + ev.preventDefault(); + el.focus(); + el.select(); + }, + }); + }; + window.addEventListener("keydown", onKey); + return () => window.removeEventListener("keydown", onKey); + }, [input]); +} + export const useHelpConditionValues = (): HelpConditions => helpConditions.use(); // ── Display ─────────────────────────────────────────────────────────────────────────────────── @@ -353,23 +565,33 @@ const GLYPHS: Record = { ArrowUp: "↑", ArrowDown: "↓", Backspace: "⌫", - ContextMenu: "Menu", // the dedicated menu key; "ContextMenu" is a DOM name, not a keycap Escape: "Esc", Shift: "⇧", }; +/** The keys a keycap names with a WORD rather than a legend every layout prints alike (Esc, Ctrl, + * Enter): the caller supplies them in the reader's language — a Hungarian sheet said "Space". */ +export type KeycapWords = Readonly>; + const isMac = (): boolean => typeof navigator !== "undefined" && /mac|iphone|ipad/i.test(navigator.userAgent); -/** A chord as the help sheet shows it: `Mod+Shift+z` → `Ctrl ⇧ Z` (`⌘ ⇧ Z` on a Mac). */ -export function formatChord(chord: string): string { - return chord - .split("+") - .map((part, i, all) => { - if (part.toLowerCase() === "mod") return isMac() ? "⌘" : "Ctrl"; - if (i < all.length - 1) return GLYPHS[part] ?? part; - return GLYPHS[part] ?? (part.length === 1 ? part.toUpperCase() : part); - }) +/** A chord as the help sheet shows it: `Mod+Shift+z` → `Ctrl ⇧ Z` (`⌘ ⇧ Z` on a Mac), and a + * sequence step by step: `g h` → `G H`. */ +export function formatChord(chord: string, words: KeycapWords): string { + const name = (part: string): string | undefined => + GLYPHS[part] ?? words[part as keyof KeycapWords]; + return stepsOf(chord) + .map((step) => + step + .split("+") + .map((part, i, all) => { + if (part.toLowerCase() === "mod") return isMac() ? "⌘" : "Ctrl"; + if (i < all.length - 1) return name(part) ?? part; + return name(part) ?? (part.length === 1 ? part.toUpperCase() : part); + }) + .join(" "), + ) .join(" "); } @@ -380,15 +602,19 @@ export interface HelpLine { when?: ShortcutWhen; } -/** The shortcuts of `scopes`, grouped into help lines by description. Scopes come out in the order +/** The shortcuts of `scopes`, grouped into help lines by description (with `pages`, the `g` chords + * are limited to those modules). Scopes come out in the order * ASKED FOR, rows within a scope in table order — so a surface passing its own scope first heads its * sheet with its own keys and leaves the app-wide ones underneath, where they read as context. */ -export function helpLines(scopes: readonly KeyScope[]): HelpLine[] { +export function helpLines(scopes: readonly KeyScope[], pages?: readonly Page[]): HelpLine[] { const out: HelpLine[] = []; const byLabel = new Map(); for (const scope of scopes) { for (const s of ROWS) { if (s.scope !== scope) continue; + // A `g` chord to a module this account cannot reach is left out ROW by row, not line by line: + // `g h` shares its line with Shift+W, which must stay. + if (s.page && pages && !pages.includes(s.page)) continue; const existing = byLabel.get(s.labelKey); if (existing) { existing.chords.push(...s.keys); diff --git a/frontend/src/lib/layerStack.ts b/frontend/src/lib/layerStack.ts index 2ac0057b..573a61fa 100644 --- a/frontend/src/lib/layerStack.ts +++ b/frontend/src/lib/layerStack.ts @@ -16,15 +16,28 @@ import { useEffect, useRef } from "react"; let stack: number[] = []; let nextId = 1; +// Layers that take Escape but do not own the keyboard: an open tooltip or hover card. They sit on the +// stack for Escape ordering like any other layer, but `anyBlockingLayer` looks past them — otherwise +// a tooltip that happens to be open under a resting mouse pointer would silence the page's `j`/`k`. +const passive = new Set(); -export function pushLayer(): number { +export function pushLayer(opts?: { passive?: boolean }): number { const id = nextId++; stack.push(id); + if (opts?.passive) passive.add(id); return id; } export function removeLayer(id: number): void { stack = stack.filter((x) => x !== id); + passive.delete(id); +} + +/** True while a layer that owns the keyboard is open — a modal, a player, a click-opened menu. The + * page-level keys (`s`, the `g` chords, list navigation) stay quiet then: they act on the page + * underneath, which the user cannot see or reach while such a layer is up. */ +export function anyBlockingLayer(): boolean { + return stack.some((id) => !passive.has(id)); } export function isTopmost(id: number): boolean { @@ -38,6 +51,14 @@ export function anyLayerAbove(id: number): boolean { return i !== -1 && i < stack.length - 1; } +/** True when a layer that owns the keyboard sits above `id` — the predicate for a dialog's Tab trap. + * A tooltip or hover card open under a resting pointer takes Escape first, but not the Tab: deferring + * the trap to it let Shift+Tab walk out of the dialog into the page behind its scrim. */ +export function anyBlockingLayerAbove(id: number): boolean { + const i = stack.indexOf(id); + return i !== -1 && stack.slice(i + 1).some((x) => !passive.has(x)); +} + // UX-8 — a layer that ACTS on an Escape stamps the event here; a lower layer with its own key handler // (the players, listening on `window`) reads the stamp instead of re-reading the stack. This closes // the "one Escape closes BOTH the popover and the player" bug: the popover's Escape listener is on @@ -62,14 +83,17 @@ export function escapeHandled(e: KeyboardEvent): boolean { export function useLayer( active: boolean, onEscape?: () => void, -): { isTopmost: () => boolean; anyAbove: () => boolean } { + opts?: { passive?: boolean }, +): { isTopmost: () => boolean; anyAbove: () => boolean; blockingAbove: () => boolean } { const idRef = useRef(null); const escRef = useRef(onEscape); escRef.current = onEscape; + const passiveRef = useRef(!!opts?.passive); + passiveRef.current = !!opts?.passive; useEffect(() => { if (!active) return; - const id = pushLayer(); + const id = pushLayer({ passive: passiveRef.current }); idRef.current = id; let onKey: ((e: KeyboardEvent) => void) | undefined; if (escRef.current) { @@ -94,5 +118,6 @@ export function useLayer( return { isTopmost: () => idRef.current != null && isTopmost(idRef.current), anyAbove: () => idRef.current != null && anyLayerAbove(idRef.current), + blockingAbove: () => idRef.current != null && anyBlockingLayerAbove(idRef.current), }; } diff --git a/frontend/src/lib/listNav.dom.test.tsx b/frontend/src/lib/listNav.dom.test.tsx new file mode 100644 index 00000000..ab8100d6 --- /dev/null +++ b/frontend/src/lib/listNav.dom.test.tsx @@ -0,0 +1,564 @@ +// @vitest-environment jsdom +// +// `lib/listNav` against real DOM: `j`/`k` from item to item and across lists in reading order, the +// arrows around a grid, Enter as a click on the item's own "open" element — and the places where the +// keys must stay out of the way (a field, an overlay, a control inside the card). +import { useRef, useState } from "react"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { act, cleanup, fireEvent, render, screen, waitFor } from "@testing-library/react"; +import { navItemAttrs, useListNav } from "./listNav"; +import { pushLayer, removeLayer } from "./layerStack"; + +afterEach(cleanup); + +function List({ + id, + count, + cols = 1, + mounted, + reveal, + measure, + onOpen = () => {}, +}: { + id: string; + count: number; + cols?: number; + /** Mount only the first N items, the way a virtualized grid leaves the rest out of the DOM. */ + mounted?: number; + reveal?: (index: number) => void; + /** Register without a count or columns — a rail or plain grid, measured from the DOM. */ + measure?: boolean; + onOpen?: (id: string) => void; +}) { + const ref = useRef(null); + useListNav(ref, measure ? { enabled: true } : { enabled: true, count, cols, reveal }); + return ( +
+ {Array.from({ length: mounted ?? count }, (_, i) => ( + + ))} +
+ ); +} + +/** A keydown where a real one starts: on whatever holds focus, bubbling to the window listener. */ +function press(key: string): KeyboardEvent { + const e = new KeyboardEvent("keydown", { key, bubbles: true, cancelable: true }); + act(() => { + (document.activeElement ?? document.body).dispatchEvent(e); + }); + return e; +} + +const focused = () => (document.activeElement as HTMLElement | null)?.dataset.testid; + +describe("useListNav", () => { + it("lands the first j on an item, then steps one item at a time", () => { + render(); + press("j"); + expect(focused()).toBe("a0"); + press("j"); + expect(focused()).toBe("a1"); + press("k"); + expect(focused()).toBe("a0"); + }); + + // The Tab order is the reader's, not ours: only the item the keyboard is on is focusable, and the + // previous one gives its tabindex back. + it("roves a single tabindex=-1 and never adds a Tab stop", () => { + render(); + expect(screen.getByTestId("a0").hasAttribute("tabindex")).toBe(false); + press("j"); + expect(screen.getByTestId("a0").getAttribute("tabindex")).toBe("-1"); + press("j"); + expect(screen.getByTestId("a0").hasAttribute("tabindex")).toBe(false); + expect(screen.getByTestId("a1").getAttribute("tabindex")).toBe("-1"); + }); + + // The player (E11 S2) restores focus to whatever held it when it opened. If the item gave its + // tabindex back on blur, that restore would silently fail and focus would fall to . + it("keeps the item focusable after focus leaves it, so a dialog can restore onto it", () => { + render( + <> + + + , + ); + press("j"); + act(() => screen.getByTestId("outside").focus()); + act(() => screen.getByTestId("a0").focus()); + expect(focused()).toBe("a0"); + }); + + it("stops at both ends of the page instead of wrapping", () => { + render(); + press("j"); + press("k"); + expect(focused()).toBe("a0"); + press("j"); + press("j"); + expect(focused()).toBe("a1"); + }); + + // Owner decision D4: j/k walk every collection on the page in reading order. + it("continues into the next list, and back, in document order", () => { + render( + <> + + + , + ); + press("j"); + press("j"); + press("j"); + expect(focused()).toBe("b0"); + press("k"); + expect(focused()).toBe("a1"); + }); + + it("moves around a grid with the arrows, row by row", () => { + // 3 columns, 7 items: rows [0 1 2] [3 4 5] [6] + render(); + press("j"); + press("ArrowRight"); + expect(focused()).toBe("a1"); + press("ArrowDown"); + expect(focused()).toBe("a4"); + press("ArrowDown"); // the row below is short: land on its last item + expect(focused()).toBe("a6"); + press("ArrowDown"); // already on the last row + expect(focused()).toBe("a6"); + press("ArrowUp"); + expect(focused()).toBe("a3"); + press("ArrowLeft"); // left wraps to the end of the row above — reading order + expect(focused()).toBe("a2"); + }); + + it("works the arrows from a control inside the card too", () => { + render(); + act(() => screen.getByTestId("a0-action").focus()); + press("ArrowDown"); + expect(focused()).toBe("a2"); + }); + + // Outside a list an arrow scrolls the page; that native meaning must survive. + it("leaves the arrows alone when focus is not in a list", () => { + render(); + expect(press("ArrowDown").defaultPrevented).toBe(false); + expect(focused()).toBeUndefined(); + }); + + it("opens the item on Enter by clicking its own open element", () => { + const onOpen = vi.fn(); + render(); + press("j"); + press("j"); + expect(press("Enter").defaultPrevented).toBe(true); + expect(onOpen).toHaveBeenCalledWith("a1"); + }); + + it("leaves Enter to a control inside the card", () => { + const onOpen = vi.fn(); + render(); + act(() => screen.getByTestId("a0-action").focus()); + expect(press("Enter").defaultPrevented).toBe(false); + expect(onOpen).not.toHaveBeenCalled(); + }); + + it("stays out of a text field", () => { + render( + <> + + + , + ); + act(() => screen.getByTestId("field").focus()); + press("j"); + expect(focused()).toBe("field"); + }); + + it("is quiet under a layer that owns the keyboard, but not under a tooltip", () => { + render(); + const modal = pushLayer(); + press("j"); + expect(focused()).toBeUndefined(); + removeLayer(modal); + + const tooltip = pushLayer({ passive: true }); + press("j"); + expect(focused()).toBe("a0"); + removeLayer(tooltip); + }); + + // A virtualized grid only mounts a screen of rows: the next item may not exist yet. + it("asks the list to reveal an unmounted item, then focuses it once it renders", async () => { + const reveal = vi.fn(); + const { rerender } = render(); + press("j"); + press("j"); + press("j"); + expect(reveal).toHaveBeenCalledWith(2); + expect(focused()).toBe("a1"); + rerender(); + await waitFor(() => expect(focused()).toBe("a2")); + }); + + it("unbinds when the last list unmounts", () => { + const { unmount } = render(); + unmount(); + expect(press("j").defaultPrevented).toBe(false); + }); +}); + +// jsdom lays nothing out, so the tests below give each item a box: `boxes` maps an item's test id +// (`a0`, `b3`) to where it sits. Everything else — the list containers — is a zero box at the origin. +function withLayout( + boxes: (id: string, n: number) => { left: number; top: number } | null, + run: () => void, +) { + const spy = vi.spyOn(Element.prototype, "getBoundingClientRect").mockImplementation(function ( + this: Element, + ) { + const m = /^([a-z])(\d+)$/.exec((this as HTMLElement).dataset?.testid ?? ""); + const at = m ? boxes(m[1]!, Number(m[2])) : null; + const { left, top } = at ?? { left: 0, top: 0 }; + const size = at ? 90 : 0; + return { + left, + top, + right: left + size, + bottom: top + size, + width: size, + height: size, + x: left, + y: top, + } as DOMRect; + }); + // Restored in `finally`: a spy left behind by a failing assertion would fail every later test. + try { + run(); + } finally { + spy.mockRestore(); + } +} + +describe("useListNav · measured lists (rails and plain grids)", () => { + it("reads the item count and the columns off the DOM when the list does not give them", () => { + // Three to a row: [0 1 2] [3 4 5] [6] + withLayout( + (_, n) => ({ left: (n % 3) * 100, top: Math.floor(n / 3) * 100 }), + () => { + render(); + press("j"); + press("ArrowDown"); + expect(focused()).toBe("a3"); + press("ArrowDown"); + expect(focused()).toBe("a6"); + press("ArrowRight"); // the last item: nothing past it + expect(focused()).toBe("a6"); + }, + ); + }); + + // Owner decision D4: ↑↓ from a rail go to the rail above or below, not nowhere. + it("goes on from a list's bottom row to the next list's nearest item, and back up", () => { + withLayout( + // Two rails of four; the lower one sits 30px further right. + (list, n) => (list === "a" ? { left: n * 100, top: 0 } : { left: n * 100 + 30, top: 200 }), + () => { + render( + <> + + + , + ); + press("j"); + press("ArrowRight"); + press("ArrowRight"); + expect(focused()).toBe("a2"); + press("ArrowDown"); + expect(focused()).toBe("b2"); // centre 275 is nearest a2's 45+200 + press("ArrowDown"); // the page's last list: stays + expect(focused()).toBe("b2"); + press("ArrowUp"); + expect(focused()).toBe("a2"); + press("ArrowRight"); + press("ArrowRight"); // ←→ never leave the rail + expect(focused()).toBe("a3"); + }, + ); + }); + + it("lands the first j on the first card a rail shows, not one scrolled out to the left", () => { + withLayout( + (_, n) => ({ left: (n - 2) * 100, top: 0 }), // a0 and a1 sit left of the rail's edge + () => { + render(); + press("j"); + expect(focused()).toBe("a2"); + }, + ); + }); +}); + +describe("useListNav · keys and lists it must leave alone", () => { + // DataTable renders its rows twice — a table and a card list, one of them display:none at any width. + it("skips a list that is not on screen", () => { + render( + <> + + + , + ); + Object.defineProperty(screen.getByTestId("a0").parentElement!, "checkVisibility", { + value: () => false, + }); + press("j"); + expect(focused()).toBe("b0"); + press("k"); + expect(focused()).toBe("b0"); + }); + + // A playlist row's drag handle moves the row with the arrows during a keyboard drag; dnd-kit handles + // the key on the document first and prevents its default. Moving focus as well would drop the drag. + it("leaves a key that something before it already handled", () => { + render(); + press("j"); + const handled = (e: KeyboardEvent) => e.preventDefault(); + document.addEventListener("keydown", handled); + try { + press("ArrowDown"); + press("j"); + expect(focused()).toBe("a0"); + } finally { + document.removeEventListener("keydown", handled); + } + press("ArrowDown"); + expect(focused()).toBe("a1"); + }); +}); + +describe("useListNav · the roving tabindex and a removed item (deep-review E11 round 1)", () => { + // A tile that is a native + ))} +
+ ); + } + render(); + press("j"); + expect(focused()).toBe("t0"); + expect(screen.getByTestId("t0").hasAttribute("tabindex")).toBe(false); + press("j"); + expect(focused()).toBe("t1"); + expect(screen.getByTestId("t1").hasAttribute("tabindex")).toBe(false); + }); + + // Landing again on the item that already holds the roved tabindex (focus went to in between) + // forgot it, so it kept tabindex="-1" for good — and a click on its blank area focused the card. + it("gives the tabindex back after landing twice on the same item", () => { + render(); + press("j"); + expect(focused()).toBe("a0"); + act(() => (document.activeElement as HTMLElement).blur()); + press("j"); + expect(focused()).toBe("a0"); + press("j"); + expect(focused()).toBe("a1"); + expect(screen.getByTestId("a0").hasAttribute("tabindex")).toBe(false); + }); + + // Hiding the focused feed card unmounts it and drops focus to ; the next `j` started over at + // the first visible card instead of going on from the hidden card's slot. + it("goes on from the slot of an item that was removed under the focus", () => { + function Removable() { + const [ids, setIds] = useState([0, 1, 2, 3]); + const ref = useRef(null); + useListNav(ref, { enabled: true }); + return ( +
+ {ids.map((id, i) => ( +
+ +
+ ))} +
+ ); + } + withLayout( + (_, n) => ({ left: 0, top: n * 100 }), + () => { + render(); + press("j"); + press("j"); + press("j"); + expect(focused()).toBe("r2"); + act(() => screen.getByTestId("r2-hide").focus()); + fireEvent.click(screen.getByTestId("r2-hide")); + expect(document.activeElement).toBe(document.body); + press("j"); + expect(focused()).toBe("r3"); + }, + ); + }); +}); + +describe("useListNav · leaving the list, and naming the item (deep-review E11 round 2)", () => { + function Swappable({ ids }: { ids: number[] }) { + const ref = useRef(null); + useListNav(ref, { enabled: true }); + return ( +
+ {ids.map((id, i) => ( +
+ +
+ ))} +
+ ); + } + + // `s`, a query, Escape: the feed stays mounted with new results and the old card is gone. The slot + // kept for a REMOVED card sent the next `j` to the third new result instead of the first. + it("starts from the first visible item after focus went on to a field and the items changed", () => { + withLayout( + (_, n) => ({ left: 0, top: n * 100 }), + () => { + const page = (ids: number[]) => ( + <> + + + + ); + const { rerender } = render(page([0, 1, 2, 3])); + press("j"); + press("j"); + press("j"); + expect(focused()).toBe("r2"); + act(() => screen.getByTestId("field").focus()); + rerender(page([4, 5, 6, 7])); + act(() => screen.getByTestId("field").blur()); + press("j"); + expect(focused()).toBe("r4"); + }, + ); + }); + + // A confirm (or the item's own menu) takes the focus before the delete it confirms: back from it, the + // next `j` still goes on from the deleted row's slot. + it("keeps the slot when the removal was confirmed in a dialog", () => { + withLayout( + (_, n) => ({ left: 0, top: n * 100 }), + () => { + const page = (ids: number[]) => ( + <> +
+ +
+ + + ); + const { rerender } = render(page([0, 1, 2, 3])); + press("j"); + press("j"); + press("j"); + act(() => screen.getByTestId("ok").focus()); + rerender(page([0, 1, 3])); + act(() => screen.getByTestId("ok").blur()); + press("j"); + expect(focused()).toBe("r3"); + }, + ); + }); + + // A card root is a plain
: focused by `j` it had no role and no name, so a screen reader read + // an unlabelled run of text. + it("names a focused card after what it opens while it holds the focus, then gives the name back", () => { + render(); + press("j"); + const a0 = screen.getByTestId("a0"); + expect(a0.getAttribute("role")).toBe("group"); + expect(a0.getAttribute("aria-label")).toBe("title"); + press("j"); + expect(a0.hasAttribute("role")).toBe(false); + expect(a0.hasAttribute("aria-label")).toBe(false); + expect(screen.getByTestId("a1").getAttribute("aria-label")).toBe("title"); + }); + + // A Plex poster's open element is the whole card — badges, title, year, rating and summary — and a + // season card's holds only the hover caption "Open show": named from that text, every season was + // "Open show" (deep-review E11 round 3). The item's own name wins. + it("names a card by the name its item gives, over the text of what it opens", () => { + function Posters() { + const ref = useRef(null); + useListNav(ref, { enabled: true }); + return ( +
+
+
+ Open show +
+
+
+ ); + } + render(); + press("j"); + expect(screen.getByTestId("p0").getAttribute("aria-label")).toBe("Season 1"); + }); + + // Hide a card, then Undo in the toast that put up: focus on the toast's button is not the reader + // leaving the list, and `j` goes on from the card put back (deep-review E11 round 3). + it("keeps the slot when focus went to the Undo of a toast", () => { + withLayout( + (_, n) => ({ left: 0, top: n * 100 }), + () => { + const page = (ids: number[]) => ( + <> +
+ +
+ + + ); + const { rerender } = render(page([0, 1, 2, 3])); + press("j"); + press("j"); + press("j"); + rerender(page([0, 1, 3])); + act(() => screen.getByTestId("undo").focus()); + rerender(page([0, 1, 2, 3])); + act(() => screen.getByTestId("undo").blur()); + press("j"); + expect(focused()).toBe("r2"); + }, + ); + }); +}); diff --git a/frontend/src/lib/listNav.scope.dom.test.tsx b/frontend/src/lib/listNav.scope.dom.test.tsx new file mode 100644 index 00000000..49af4087 --- /dev/null +++ b/frontend/src/lib/listNav.scope.dom.test.tsx @@ -0,0 +1,54 @@ +// @vitest-environment jsdom +// +// The `list` key scope — what puts j/k, the arrows and Enter on the cheat sheet — is live only while a +// registered list has an item to land on. An empty download queue, a list still loading or one that +// failed mounts its container all the same, and the sheet listed the keys over nothing (deep-review +// E11 round 2). Its own file: listNav keeps module state. +import { useRef } from "react"; +import { afterEach, describe, expect, it } from "vitest"; +import { cleanup, render, screen, waitFor } from "@testing-library/react"; +import { navItemAttrs, useListNav } from "./listNav"; +import { useActiveScopes } from "./keymap"; + +afterEach(cleanup); + +function Scopes() { + return {useActiveScopes().join(" ")}; +} + +function Queue({ count }: { count: number }) { + const ref = useRef(null); + useListNav(ref, { enabled: true }); + return ( +
+ {count === 0 ? ( +

Nothing queued

+ ) : ( + Array.from({ length: count }, (_, i) => ( +
+ item +
+ )) + )} +
+ ); +} + +const page = (count: number) => ( + <> + + + +); +const scopes = () => screen.getByTestId("scopes").textContent!.split(" "); + +describe("useListNav · the list key scope", () => { + it("is not live over a list with nothing in it, and is while it has items", async () => { + const { rerender } = render(page(0)); + expect(scopes()).not.toContain("list"); + rerender(page(2)); + await waitFor(() => expect(scopes()).toContain("list")); + rerender(page(0)); + await waitFor(() => expect(scopes()).not.toContain("list")); + }); +}); diff --git a/frontend/src/lib/listNav.ts b/frontend/src/lib/listNav.ts new file mode 100644 index 00000000..330fe48e --- /dev/null +++ b/frontend/src/lib/listNav.ts @@ -0,0 +1,422 @@ +import { useEffect, useRef, useState, type RefObject } from "react"; +import { dispatch, useKeyScope } from "./keymap"; +import { scrollParent } from "./listPaging"; + +// ── Keyboard navigation over lists and grids (E11 S4, UX-6) ────────────────────────────────────── +// Before this, a feed card cost ~10 Tab stops (thumb, play, info, title, channel, five actions), so +// the tenth card sat ~135 Tabs from the top of the page. `j`/`k` step from ITEM to item instead, the +// arrows move around a grid, and Enter opens — the same thing a click on the item does. +// +// How an item takes part, without this file knowing what an item is: +// - its root element carries `data-nav-index` (its index in the list — `navItemAttrs`), +// - the one element whose click means "open" carries `data-nav-open` (the title link on a video card), +// - the list's container registers with `useListNav`, saying how many items there are, how many +// columns a row has, and how to bring an unmounted item into the DOM (a virtualized grid). +// +// The Tab order is untouched: an item root is NOT a Tab stop. It gets `tabindex="-1"` at the moment +// the keyboard lands on it — one item at a time, the previous one gives it back — so Tab still walks +// the controls inside the cards, a mouse click on a card's blank area still focuses nothing, and the +// player's focus restore (E11 S2) can land back on the card it was opened from. +// +// Every page-level list shares ONE window listener, and the lists are walked in DOCUMENT order: `j` +// at the end of one list continues at the start of the next (owner decision D4, "reading order"). +// ←→ stay inside the list they are in; ↑ on a list's top row and ↓ on its bottom row go on to the +// neighbouring list — the rail above or below on Home and Discover — landing on the item nearest the +// one you left, so a column of rails reads like one grid. +// +// A list that mounts everything (a rail, a plain grid) need not say how many items or columns it has: +// both are read off the DOM when the key is pressed, so an auto-fill grid's columns follow its width. + +interface NavList { + readonly el: HTMLElement; + readonly count: () => number; + readonly cols: () => number; + readonly reveal: (index: number) => void; +} + +const lists = new Map(); +// The item we gave a tabindex to, so the next move can take it back — and whether we named it too. +let roved: HTMLElement | null = null; +let named = false; + +const INDEX = "data-nav-index"; +const LIST = "data-nav-list"; +// How long to wait for a virtualized item to mount after asking the list to reveal it. +const REVEAL_FRAMES = 10; + +/** Spread onto an item's ROOT element. `name` is what a screen reader calls the item while the keyboard + * is on it (its title) — pass it where the element Enter opens wraps the whole card (a Plex poster: + * badges, year, rating and summary run together), shows a caption that is not the item's name, or + * holds no text at all (deep-review E11 round 3). */ +export const navItemAttrs = ( + index: number, + name?: string | null, +): { "data-nav-index": number; "data-nav-name"?: string } => ({ + [INDEX]: index, + ...(name ? { "data-nav-name": name } : null), +}); + +const indexOf = (item: Element): number => Number(item.getAttribute(INDEX)); + +function itemAt(list: NavList, index: number): HTMLElement | null { + return list.el.querySelector(`[${INDEX}="${index}"]`); +} + +function ordered(): NavList[] { + return ( + [...lists.values()] + // Only lists on screen: a responsive table renders its rows twice — a table and a card list, one + // of them display:none at any width (ui/DataTable) — and `j` must not walk into the hidden copy. + .filter((l) => l.el.isConnected && l.el.checkVisibility?.() !== false) + .sort((a, b) => + a.el.compareDocumentPosition(b.el) & Node.DOCUMENT_POSITION_FOLLOWING ? -1 : 1, + ) + ); +} + +/** The item holding focus (on its root or on a control inside it), and the list it belongs to. */ +function current(): { list: NavList; index: number; item: HTMLElement } | null { + const item = (document.activeElement as HTMLElement | null)?.closest(`[${INDEX}]`); + const list = + item && lists.get(item.parentElement?.closest(`[${LIST}]`) as HTMLElement); + return item && list ? { list, index: indexOf(item), item } : null; +} + +/** The mounted items that belong to `el` itself (not to a list nested inside it), in index order. */ +function ownItems(el: HTMLElement): HTMLElement[] { + return [...el.querySelectorAll(`[${INDEX}]`)] + .filter((item) => item.parentElement?.closest(`[${LIST}]`) === el) + .sort((a, b) => indexOf(a) - indexOf(b)); +} + +// Items whose tops are this close share a row (sub-pixel layout, a card a hair taller than the next). +const ROW_SLACK = 4; +const topOf = (el: Element) => el.getBoundingClientRect().top; +const centerX = (el: Element) => { + const r = el.getBoundingClientRect(); + return r.left + r.width / 2; +}; + +/** Items per row, measured: the leading items that share the first item's top. A rail is one row. */ +function measuredCols(el: HTMLElement): number { + const items = ownItems(el); + if (!items.length) return 1; + const first = topOf(items[0]!); + const inRow = items.findIndex((item) => Math.abs(topOf(item) - first) > ROW_SLACK); + return inRow < 0 ? items.length : inRow; +} + +/** Where `j` lands when nothing in a list has focus yet: the first item the reader can see, not the + * top of the list — after scrolling down a feed, the first press must not throw them back up. "Can + * see" = its bottom edge is below the top of the scroller's visible band, which starts under the + * floating header (the item's own `scroll-margin-top` is exactly that header clearance), and — in a + * rail scrolled sideways — its right edge is past the rail's left edge. */ +function firstVisible(): { list: NavList; index: number } | null { + let fallback: { list: NavList; index: number } | null = null; + for (const list of ordered()) { + const items = ownItems(list.el); + if (!items.length) continue; + fallback ??= { list, index: indexOf(items[0]!) }; + const top = scrollParent(list.el)?.getBoundingClientRect().top ?? 0; + const left = list.el.getBoundingClientRect().left; + for (const el of items) { + const band = top + (parseFloat(getComputedStyle(el).scrollMarginTop) || 0); + const r = el.getBoundingClientRect(); + if (r.bottom > band && r.right > left) return { list, index: indexOf(el) }; + } + } + return fallback; +} + +/** The item focus was last inside, so a `j` after that item was REMOVED (a feed card hidden, a + * download deleted — focus falls to ) goes on from its slot instead of starting over. */ +let last: { list: NavList; index: number; el: HTMLElement } | null = null; + +function onFocusIn(e: FocusEvent): void { + const target = e.target as HTMLElement | null; + const item = target?.closest?.(`[${INDEX}]`); + const list = + item && lists.get(item.parentElement?.closest(`[${LIST}]`) as HTMLElement); + if (item && list) last = { list, index: indexOf(item), el: item }; + // Focus went on to page content that is not an item — the search box, a filter chip: the reader + // left the list, and whatever it shows next (new results) is not "the slot they were in" (deep-review + // E11 round 2). A layer opened FROM the item — its menu, a confirm, the Undo toast its removal put + // up — does not count: focus comes back from it, and what it did (a hidden card, a deleted row, the + // card put back) is what the slot is kept for. Removing the focused node itself fires no focusin. + else if (!target?.closest?.('[data-floating-ui-portal], [role="dialog"], [data-toast-region]')) + last = null; +} + +/** Where `j` (`k`: the item before) lands after the last-focused item left the DOM: the item now in + * its slot, if that slot is on screen. An item that only scrolled out of a virtualized list has left + * the DOM too, but its slot is off screen then, so the reader's viewport (`firstVisible`) wins. */ +function slotOfRemoved(delta: 1 | -1): { list: NavList; index: number } | null { + if (!last || last.el.isConnected || lists.get(last.list.el) !== last.list) return null; + const n = last.list.count(); + const index = Math.min(Math.max(0, delta > 0 ? last.index : last.index - 1), n - 1); + const el = index >= 0 ? itemAt(last.list, index) : null; + if (!el) return null; + const box = scrollParent(last.list.el)?.getBoundingClientRect(); + const r = el.getBoundingClientRect(); + const band = (box?.top ?? 0) + (parseFloat(getComputedStyle(el).scrollMarginTop) || 0); + return r.bottom > band && r.top < (box?.bottom ?? window.innerHeight) + ? { list: last.list, index } + : null; +} + +function focusItem(el: HTMLElement, reveal = true): void { + if (roved !== el) { + if (named) roved?.removeAttribute("role"); + if (named) roved?.removeAttribute("aria-label"); + roved?.removeAttribute("tabindex"); + roved = null; + named = false; + // An item that is a Tab stop in its own right (a native + {peek.active && expanded} +
+
+ +
+ + + ); +} + +const moveFocus = (from: HTMLElement, to: HTMLElement) => + act(() => { + fireEvent.blur(from, { relatedTarget: to }); + to.focus(); + }); + +describe("useHoverFocusWithin · focus", () => { + it("stays open while the focus is in a dialog opened over the page", () => { + render(); + const row = screen.getByText("row"); + act(() => row.focus()); + expect(screen.queryByText("expanded")).not.toBeNull(); + + moveFocus(row, screen.getByText("cancel")); + expect(screen.queryByText("expanded")).not.toBeNull(); + }); + + it("still closes when the focus leaves for the page itself", () => { + render(); + const row = screen.getByText("row"); + act(() => row.focus()); + moveFocus(row, screen.getByText("elsewhere")); + expect(screen.queryByText("expanded")).toBeNull(); + }); +}); diff --git a/frontend/src/lib/useHoverFocusWithin.ts b/frontend/src/lib/useHoverFocusWithin.ts index 3f27423d..6ecd49f0 100644 --- a/frontend/src/lib/useHoverFocusWithin.ts +++ b/frontend/src/lib/useHoverFocusWithin.ts @@ -58,7 +58,16 @@ export function useHoverFocusWithin() { const focusHandlers = { onFocus: () => setFocused(true), onBlur: (e: FocusEvent) => { - if (!e.currentTarget.contains(e.relatedTarget as Node)) setFocused(false); + const to = e.relatedTarget as Element | null; + if (e.currentTarget.contains(to)) return; + // A dialog opened FROM inside the panel (the rail row's "Delete?" confirm) renders at the app + // root, so neither the DOM nor the React tree sees it as inside — and collapsing the panel under + // it unmounted the very row the dialog hands the focus back to, leaving a keyboard user on + // after Cancel (measured in Chrome, E11 S2). Hold while the focus is in a dialog: when it + // closes, the focus either comes back in here (onFocus) or lands nowhere, which the + // document-level net below clears. + if (to?.closest('[aria-modal="true"]')) return; + setFocused(false); }, }; diff --git a/frontend/src/lib/usePref.dom.test.ts b/frontend/src/lib/usePref.dom.test.ts index a9fb61aa..e382b524 100644 --- a/frontend/src/lib/usePref.dom.test.ts +++ b/frontend/src/lib/usePref.dom.test.ts @@ -14,7 +14,9 @@ const { store, savePrefs } = vi.hoisted(() => ({ savePrefs: vi.fn(() => Promise.resolve({})), })); -vi.mock("./storage", () => ({ +// Partial: i18n (which usePref now imports, to title a refused save) reads its key from `LS`. +vi.mock("./storage", async (importOriginal) => ({ + ...(await importOriginal()), readAccount: (base: string, fallback: unknown) => { const raw = store.get(base); return raw == null ? fallback : JSON.parse(raw); @@ -24,6 +26,9 @@ vi.mock("./storage", () => ({ }, })); vi.mock("./api", () => ({ api: { savePrefs, me: vi.fn() } })); +// The failure path (usePref.saveFailure.dom.test.ts) is not this file's subject; stubbed so the +// reporter's notifications → storage chain does not meet the storage mock above. +vi.mock("./actionErrors", () => ({ notifyActionError: vi.fn() })); import { usePref, useSavePref } from "./usePref"; import { qk } from "./queryKeys"; diff --git a/frontend/src/lib/usePref.saveFailure.dom.test.ts b/frontend/src/lib/usePref.saveFailure.dom.test.ts new file mode 100644 index 00000000..5c8820b6 --- /dev/null +++ b/frontend/src/lib/usePref.saveFailure.dom.test.ts @@ -0,0 +1,51 @@ +// @vitest-environment jsdom +// +// A preference save the server refuses is REPORTED (E11 S2b). The local state and the `me` cache +// already show the new value, so a swallowed failure was invisible until the next load quietly put +// the old value back — and the comment above the swallow claimed `req` toasted it, which it does not +// for a 403/404/429 or a lost connection. +import { afterEach, describe, expect, it, vi } from "vitest"; +import { act, renderHook, waitFor } from "@testing-library/react"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import { createElement, type ReactNode } from "react"; + +const { savePrefs, notifyActionError } = vi.hoisted(() => ({ + savePrefs: vi.fn(), + notifyActionError: vi.fn(), +})); +vi.mock("./api", () => ({ api: { savePrefs, me: vi.fn() } })); +vi.mock("./actionErrors", () => ({ notifyActionError })); + +import { usePref, useSavePref } from "./usePref"; +import i18n from "../i18n"; + +afterEach(() => { + savePrefs.mockReset(); + notifyActionError.mockReset(); +}); + +const wrapper = ({ children }: { children: ReactNode }) => + createElement(QueryClientProvider, { client: new QueryClient() }, children); + +describe("a refused preference save", () => { + it("is reported from the usePref setter", async () => { + const refusal = new TypeError("Failed to fetch"); + savePrefs.mockRejectedValue(refusal); + const { result } = renderHook(() => usePref("hintsOn", true), { wrapper }); + act(() => result.current[1](false)); + await waitFor(() => + expect(notifyActionError).toHaveBeenCalledWith(refusal, i18n.t("errors.writeFailed.prefs")), + ); + expect(result.current[0]).toBe(false); // the local value stays; the toast says it did not save + }); + + it("is reported from the raw useSavePref persister", async () => { + const refusal = new Error("403"); + savePrefs.mockRejectedValue(refusal); + const { result } = renderHook(() => useSavePref(), { wrapper }); + act(() => result.current({ stripSources: { c1: false } })); + await waitFor(() => + expect(notifyActionError).toHaveBeenCalledWith(refusal, i18n.t("errors.writeFailed.prefs")), + ); + }); +}); diff --git a/frontend/src/lib/usePref.ts b/frontend/src/lib/usePref.ts index 3a8b8a71..66b6ced2 100644 --- a/frontend/src/lib/usePref.ts +++ b/frontend/src/lib/usePref.ts @@ -4,6 +4,14 @@ import { qk } from "./queryKeys"; import { api } from "./api"; import type { Me } from "./api/account"; import { readAccount, writeAccount } from "./storage"; +import { notifyActionError } from "./actionErrors"; +import i18n from "../i18n"; + +// The local state + cache already show the new value, so a refused save is otherwise invisible until +// the next load quietly puts the old one back. `req` reports none of 403/404/429 and only the +// connection notice for a lost connection — say it (E11 S2b). +const reportPrefSaveFailure = (err: unknown) => + notifyActionError(err, i18n.t("errors.writeFailed.prefs")); // The ONE per-account + server preference hook. A preference (unlike an ephemeral filter) persists to // the account on the server (PUT /api/me/preferences, which MERGES the partial payload) AND is adopted @@ -55,7 +63,7 @@ export function useSavePref(): (patch: Record) => void { const qc = useQueryClient(); return useCallback( (patch: Record) => { - api.savePrefs(patch).catch(() => {}); + api.savePrefs(patch).catch(reportPrefSaveFailure); mergePrefsCache(qc, patch); }, [qc], @@ -116,9 +124,9 @@ export function usePref( // see the new value without a refetch. Merges into preferences, mirroring the server PUT. const payload = { [serverKey]: next }; mergePrefsCache(qc, payload); - // savePrefs is fire-and-forget: api.req() already surfaces a failure toast, and the local state + - // cache already reflect the change (idempotent PUT retries a transient blip). - const flush = () => api.savePrefs(payload).catch(() => {}); + // savePrefs is fire-and-forget (idempotent PUT, so `req` retries a transient blip once); a + // failure that survives that is reported, not swallowed. + const flush = () => api.savePrefs(payload).catch(reportPrefSaveFailure); if (debounceMs) { window.clearTimeout(saveTimer.current); pendingSave.current = flush; diff --git a/frontend/src/lib/youtubeErrors.ts b/frontend/src/lib/youtubeErrors.ts index 25c3773c..df369f98 100644 --- a/frontend/src/lib/youtubeErrors.ts +++ b/frontend/src/lib/youtubeErrors.ts @@ -1,4 +1,5 @@ import { HttpError } from "./api"; +import { markReported } from "./api/core"; import { notify } from "./notifications"; import { notifyActionError } from "./actionErrors"; import i18n from "../i18n"; @@ -22,6 +23,7 @@ export function notifyYouTubeActionError( ? { label: i18n.t("channels.notify.connect"), onClick: onConnect } : undefined, }); + markReported(err); return; } // Any other error: prefer the server's own reason (e.g. "Not enough quota left today") over the diff --git a/frontend/src/main.tsx b/frontend/src/main.tsx index f0717f1c..925ec233 100644 --- a/frontend/src/main.tsx +++ b/frontend/src/main.tsx @@ -1,6 +1,7 @@ import React, { lazy, Suspense } from "react"; import { createRoot } from "react-dom/client"; -import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import { QueryClientProvider } from "@tanstack/react-query"; +import { createQueryClient } from "./lib/queryClient"; import { FloatingTree } from "@floating-ui/react"; import ErrorBoundary from "./components/ErrorBoundary"; import { ConfirmProvider } from "./components/ConfirmProvider"; @@ -28,9 +29,7 @@ const PrivacyPolicy = lazy(() => import("./components/legal/PrivacyPolicy")); const Terms = lazy(() => import("./components/legal/Terms")); const WatchPage = lazy(() => import("./components/WatchPage")); -const queryClient = new QueryClient({ - defaultOptions: { queries: { retry: false, staleTime: 30_000, refetchOnWindowFocus: false } }, -}); +const queryClient = createQueryClient(); // Baseline theme so the token CSS vars (--bg/--surface/--accent/…) resolve on first paint — including // the standalone public pages (/watch, /privacy, /terms) that render outside and never call