From e31a588f19a6a6ac963ad4c233b4275bac2501a1 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sun, 27 Sep 2026 19:43:04 +0200 Subject: [PATCH] fix(menus): one tick for every choice; a listbox holds its options only - The saved views, the view switcher, the year filter and the table's single-choice filter mark the chosen row with MenuCheck, as DESIGN asks, not a trailing tick, a dot or a filled row; a contract test holds every checkable menu row to it. keepSelectedPaint had no caller left and is gone. - A combobox's panel carries its listbox role on an inner PopoverList (listInside), so the failed load, Loading, No matches and the 'n more' line sit beside the options, not inside the listbox. The Plex search box's Loading line likewise. - The Plex sidebar's playlist picker offers Create only once the lists have loaded: over a failed load it pushed the error and its Retry out at the first keystroke. --- .../src/components/PlexSearchBox.dom.test.tsx | 10 +++ frontend/src/components/PlexSearchBox.tsx | 71 ++++++++-------- .../PlexSidebar.picker.dom.test.tsx | 76 +++++++++++++++++ frontend/src/components/PlexSidebar.tsx | 7 +- frontend/src/components/SavedViewsPopover.tsx | 6 +- frontend/src/components/ViewSwitcher.tsx | 5 +- frontend/src/components/YearFilter.tsx | 5 +- .../src/components/ui/AnchoredPopover.tsx | 44 +++++++--- .../src/components/ui/Combobox.dom.test.tsx | 29 +++++++ frontend/src/components/ui/Combobox.tsx | 85 ++++++++++--------- .../components/ui/menuCheck.contract.test.ts | 32 +++++-- frontend/src/components/ui/tableHeader.tsx | 12 +-- 12 files changed, 266 insertions(+), 116 deletions(-) create mode 100644 frontend/src/components/PlexSidebar.picker.dom.test.tsx diff --git a/frontend/src/components/PlexSearchBox.dom.test.tsx b/frontend/src/components/PlexSearchBox.dom.test.tsx index cefaaa98..872fba57 100644 --- a/frontend/src/components/PlexSearchBox.dom.test.tsx +++ b/frontend/src/components/PlexSearchBox.dom.test.tsx @@ -219,4 +219,14 @@ describe("PlexSearchBox — typeahead", () => { await waitFor(() => expect(input.getAttribute("aria-expanded")).toBe("false")); expect(input.getAttribute("aria-controls")).toBeNull(); }); + + // Round 2 (F4 sibling): a listbox may own options only — "Loading…" sat inside it. + it("shows its loading line beside the listbox, not in it", async () => { + plexState.q = "matrix"; + plexLibrary.mockReturnValue(new Promise(() => {})); + renderBox(); + fireEvent.focus(screen.getByRole("combobox")); + const loading = await screen.findByText("plex.loading"); + expect(screen.getByRole("listbox").contains(loading)).toBe(false); + }); }); diff --git a/frontend/src/components/PlexSearchBox.tsx b/frontend/src/components/PlexSearchBox.tsx index 7499cc60..17c447bc 100644 --- a/frontend/src/components/PlexSearchBox.tsx +++ b/frontend/src/components/PlexSearchBox.tsx @@ -137,47 +137,46 @@ export default function PlexSearchBox() { below={ open ? (
+ {/* Beside the listbox, not in it: a listbox may own options only (round 2, F4). */} {suggestions.length === 0 && suggestQ.isFetching && (
{t("plex.loading")}
)} - {suggestions.map((c, i) => ( - e.preventDefault()} // keep input focus through the click - onMouseEnter={() => setHi(i)} - onClick={() => pick(i)} - // The pointer moves `hi` too, so the highlight IS the hover: the menus' roving - // recipe (AnchoredPopover's ROVING_HIGHLIGHT), one look for the mouse and the keys. - data-active={hi === i ? "true" : undefined} - className={`flex w-full items-center gap-3 rounded-lg p-1.5 transition ${ROVING_HIGHLIGHT}`} - > - - {c.title} - {c.year != null && ( - {c.year} - )} - {c.type === "show" ? ( - - ))} +
+ {suggestions.map((c, i) => ( + e.preventDefault()} // keep input focus through the click + onMouseEnter={() => setHi(i)} + onClick={() => pick(i)} + // The pointer moves `hi` too, so the highlight IS the hover: the menus' roving + // recipe (AnchoredPopover's ROVING_HIGHLIGHT), one look for the mouse and the keys. + data-active={hi === i ? "true" : undefined} + className={`flex w-full items-center gap-3 rounded-lg p-1.5 transition ${ROVING_HIGHLIGHT}`} + > + + {c.title} + {c.year != null && ( + {c.year} + )} + {c.type === "show" ? ( + + ))} +
) : null } diff --git a/frontend/src/components/PlexSidebar.picker.dom.test.tsx b/frontend/src/components/PlexSidebar.picker.dom.test.tsx new file mode 100644 index 00000000..1be3c43f --- /dev/null +++ b/frontend/src/components/PlexSidebar.picker.dom.test.tsx @@ -0,0 +1,76 @@ +// @vitest-environment jsdom +// +// The sidebar's playlist picker (E28 review round 2, P7): typing a name offers "Create …" — but only +// once the lists have loaded. Over a failed load the create row pushed the error and its Retry out +// of the list at the first keystroke, and whether the name already existed was unknown anyway. +import { describe, it, expect, vi, afterEach, beforeAll } from "vitest"; +import { render, cleanup, screen, fireEvent, waitFor } from "@testing-library/react"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import { FloatingTree } from "@floating-ui/react"; + +vi.mock("react-i18next", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + useTranslation: () => ({ + t: (key: string, o?: { name?: string }) => (o?.name != null ? `${key}:${o.name}` : key), + i18n: { language: "en" }, + }), + }; +}); +vi.mock("./plexContext", () => ({ + usePlex: () => ({}), + usePlexActions: () => ({ setPlaylistOpen: vi.fn() }), +})); + +import { PlaylistPicker } from "./PlexSidebar"; +import { api } from "../lib/api"; + +beforeAll(() => { + class Stub { + observe() {} + unobserve() {} + disconnect() {} + takeRecords() { + return []; + } + } + globalThis.ResizeObserver ??= Stub as unknown as typeof ResizeObserver; +}); + +afterEach(() => { + cleanup(); + vi.restoreAllMocks(); +}); + +function mount() { + const qc = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + render( + + + + + , + ); + return screen.getByRole("combobox"); +} + +describe("the sidebar playlist picker", () => { + it("keeps the load error and its Retry while a name is typed, with no Create row", async () => { + vi.spyOn(api, "plexPlaylists").mockRejectedValue(new TypeError("Failed to fetch")); + const input = mount(); + fireEvent.focus(input); + fireEvent.change(input, { target: { value: "Road trip" } }); + await waitFor(() => expect(screen.getByRole("alert", { hidden: true })).toBeTruthy()); + expect(screen.getByText("common.retry")).toBeTruthy(); + expect(screen.queryByText("plex.playlist.createNamed:Road trip")).toBeNull(); + }); + + it("offers Create for a new name once the lists are in", async () => { + vi.spyOn(api, "plexPlaylists").mockResolvedValue({ playlists: [] } as never); + const input = mount(); + fireEvent.focus(input); + fireEvent.change(input, { target: { value: "Road trip" } }); + expect(await screen.findByText("plex.playlist.createNamed:Road trip")).toBeTruthy(); + }); +}); diff --git a/frontend/src/components/PlexSidebar.tsx b/frontend/src/components/PlexSidebar.tsx index 28c83c76..0a1630a2 100644 --- a/frontend/src/components/PlexSidebar.tsx +++ b/frontend/src/components/PlexSidebar.tsx @@ -332,7 +332,7 @@ export default function PlexSidebar({ layout, setLayout, collapsed, onToggleColl // you can still add one — the same reuse-the-Combobox idea as the feed's "Your tags" widget. `selected` // stays empty: this picks an action (open / create), it doesn't hold a persistent selection. const CREATE_PLAYLIST = "__create__"; -function PlaylistPicker() { +export function PlaylistPicker() { const { t } = useTranslation(); const { setPlaylistOpen: onOpenPlaylist } = usePlexActions(); const qc = useQueryClient(); @@ -346,8 +346,9 @@ function PlaylistPicker() { const exact = playlists.some((p) => p.title.toLowerCase() === ql); const options: ComboboxOption[] = [ ...matches.map((p) => ({ value: `pl:${p.id}`, label: p.title, count: p.item_count })), - // Offer to create only when the typed name isn't already a playlist. - ...(q && !exact ? [{ value: CREATE_PLAYLIST, label: q }] : []), + // Offer to create only when the typed name isn't already a playlist — which is unknown until the + // lists load: over a failed load the row pushed out the error and its Retry (round 2, P7). + ...(q && !exact && playlistsQ.data ? [{ value: CREATE_PLAYLIST, label: q }] : []), ]; async function createPlaylist(title: string) { diff --git a/frontend/src/components/SavedViewsPopover.tsx b/frontend/src/components/SavedViewsPopover.tsx index c372fd20..46ab9a72 100644 --- a/frontend/src/components/SavedViewsPopover.tsx +++ b/frontend/src/components/SavedViewsPopover.tsx @@ -1,10 +1,11 @@ import { useState, type ReactNode } from "react"; import { useTranslation } from "react-i18next"; -import { Bookmark, Check, Settings2 } from "lucide-react"; +import { Settings2 } from "lucide-react"; import { PopoverItem, PopoverSurface } from "./ui/AnchoredPopover"; import { closeThenAct, useAnchoredPopover } from "./ui/anchoredPopoverHooks"; import { useSavedViews } from "./savedViewsContext"; import { IconButton } from "./ui/IconButton"; +import { MenuCheck } from "./ui/MenuCheck"; import { SURFACE } from "./ui/surfaces"; import { SelectTrigger } from "./ui/SelectTrigger"; @@ -80,9 +81,8 @@ export default function SavedViewsPopover({ v.id === activeViewId ? "text-fg" : "text-muted" }`} > -