From 02c8c71c53d1b724f23ef65c4baa8fa4609c9488 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Fri, 14 Aug 2026 00:00:31 +0200 Subject: [PATCH] fix(sort): address R13 S1 review findings - PlexSidebar activeCount now compares the HEALED sort, so an out-of-scope key (movie "duration" after switching to shows) no longer inflates the active-filter badge while the control + grid show the default. - PlexProvider strips the legacy `sortDir` from the filters blob, so it stops leaking into the facets query key and being re-persisted on every change. - PlexProvider sort migration guards the fall-through: valid JSON that isn't a SortState now falls back instead of using the raw JSON string as the key. - New shared healPlexSort(sort, scope) is the single heal both PlexBrowse and PlexSidebar use (removes the noop-label plexSortOptions call + the duplication). - Tests: useSortState.dom.test.ts (hydrate/persist/heal round-trip via renderHook) and SortControl.dom.test.tsx (options, pick, direction toggle, directionless, trailing). vitest 94 -> 105. Gate green: tsc/eslint(0 err)/prettier/knip clean, vitest 105. --- frontend/src/components/PlexBrowse.tsx | 9 +-- frontend/src/components/PlexProvider.tsx | 12 ++- frontend/src/components/PlexSidebar.tsx | 10 ++- .../src/components/SortControl.dom.test.tsx | 77 +++++++++++++++++++ frontend/src/lib/plexSort.ts | 15 +++- frontend/src/lib/useSortState.dom.test.ts | 56 ++++++++++++++ 6 files changed, 164 insertions(+), 15 deletions(-) create mode 100644 frontend/src/components/SortControl.dom.test.tsx create mode 100644 frontend/src/lib/useSortState.dom.test.ts diff --git a/frontend/src/components/PlexBrowse.tsx b/frontend/src/components/PlexBrowse.tsx index 6b295fa5..1b6b094a 100644 --- a/frontend/src/components/PlexBrowse.tsx +++ b/frontend/src/components/PlexBrowse.tsx @@ -9,8 +9,7 @@ import { type ReactNode, } from "react"; import { qk } from "../lib/queryKeys"; -import { healSort } from "../lib/columnSort"; -import { plexSortOptions, PLEX_SORT_FALLBACK } from "../lib/plexSort"; +import { healPlexSort } from "../lib/plexSort"; import { createPortal } from "react-dom"; import { useTranslation } from "react-i18next"; import { @@ -94,11 +93,7 @@ export default function PlexBrowse() { const dq = useDebounced(q.trim(), 350); // The provider holds sort scope-agnostically; heal it against THIS scope's options so a movie-only // key (e.g. duration) left over from before a switch to shows doesn't ask for an unavailable sort. - const sortState = healSort( - sort, - plexSortOptions(scope, () => ""), - PLEX_SORT_FALLBACK, - ); + const sortState = healPlexSort(sort, scope); const sub = useHistorySubview({ kind: "grid" }); // Sidebar asked to open a playlist → push it as a subview (browser Back returns to the grid), then diff --git a/frontend/src/components/PlexProvider.tsx b/frontend/src/components/PlexProvider.tsx index 150d54af..83b8b53f 100644 --- a/frontend/src/components/PlexProvider.tsx +++ b/frontend/src/components/PlexProvider.tsx @@ -72,12 +72,17 @@ export function PlexProvider({ children }: { children: ReactNode }) { const [sort, setSortState] = useState(() => { const raw = getAccountRaw(LS.plexSort); if (!raw) return PLEX_SORT_FALLBACK; + let wasJson = false; try { const parsed = parseSortState(JSON.parse(raw)); + wasJson = true; if (parsed) return parsed; } catch { - /* legacy bare-key string handled below */ + /* not JSON → a legacy bare-key string, handled below */ } + // Valid JSON that wasn't a SortState is corrupt — fall back rather than treat the JSON text as a + // key. Only a non-JSON raw is a genuine legacy bare key. + if (wasJson) return PLEX_SORT_FALLBACK; let dir: "asc" | "desc" = "asc"; try { if ( @@ -94,7 +99,10 @@ export function PlexProvider({ children }: { children: ReactNode }) { const [filtersRaw, setFiltersRaw] = useState(() => getAccountRaw(LS.plexFilters) ?? "{}"); const filters = useMemo(() => { try { - return { ...EMPTY_PLEX_FILTERS, ...JSON.parse(filtersRaw) }; + // Drop the legacy `sortDir` — direction now lives in the sort SortState. Left here it would leak + // into the facets query key and be re-persisted on every filter change. + const { sortDir: _drop, ...rest } = JSON.parse(filtersRaw); + return { ...EMPTY_PLEX_FILTERS, ...rest }; } catch { return EMPTY_PLEX_FILTERS; } diff --git a/frontend/src/components/PlexSidebar.tsx b/frontend/src/components/PlexSidebar.tsx index 241cf970..8cd41b77 100644 --- a/frontend/src/components/PlexSidebar.tsx +++ b/frontend/src/components/PlexSidebar.tsx @@ -4,8 +4,7 @@ import { useTranslation } from "react-i18next"; import { useQuery, useQueryClient } from "@tanstack/react-query"; import { Layers, ListMusic, Plus, SlidersHorizontal, X } from "lucide-react"; import { api, EMPTY_PLEX_FILTERS, plexFilterCount, type PlexFilters } from "../lib/api"; -import { healSort } from "../lib/columnSort"; -import { plexSortOptions, PLEX_SORT_FALLBACK } from "../lib/plexSort"; +import { healPlexSort, plexSortOptions, PLEX_SORT_FALLBACK } from "../lib/plexSort"; import { defaultLayout, type PanelLayout } from "../lib/panelLayout"; import { useDebounced } from "../lib/useDebounced"; import { useScrollFade } from "../lib/useScrollFade"; @@ -115,7 +114,10 @@ export default function PlexSidebar({ layout, setLayout, collapsed, onToggleColl const collections = collectionsQ.data?.collections ?? []; const fCount = plexFilterCount(filters); - const activeCount = fCount + (show !== "all" ? 1 : 0) + (sort && sort.key !== "title" ? 1 : 0); + // Heal against the current scope so a stale out-of-scope key (e.g. movie "duration" after switching + // to shows) doesn't inflate the active-filter count while the control + grid show the default. + const healedSort = healPlexSort(sort, scope); + const activeCount = fCount + (show !== "all" ? 1 : 0) + (healedSort.key !== "title" ? 1 : 0); const anyActive = activeCount > 0; const patch = (p: Partial) => setFilters({ ...filters, ...p }); @@ -219,7 +221,7 @@ export default function PlexSidebar({ layout, setLayout, collapsed, onToggleColl body: ( setSort(next ?? PLEX_SORT_FALLBACK)} label={t("plex.filter.sort")} ascLabel={t("plex.filter.dir.asc")} diff --git a/frontend/src/components/SortControl.dom.test.tsx b/frontend/src/components/SortControl.dom.test.tsx new file mode 100644 index 00000000..2459bb1f --- /dev/null +++ b/frontend/src/components/SortControl.dom.test.tsx @@ -0,0 +1,77 @@ +// @vitest-environment jsdom +// +// The rendering integration of SortControl (its pickSort/heal decision logic is covered in +// columnSort.test.ts). Labels are plain props here — no i18n needed. +import { describe, it, expect, vi, afterEach } from "vitest"; +import { render, screen, fireEvent, cleanup } from "@testing-library/react"; +import SortControl from "./SortControl"; +import type { SortOption, SortState } from "../lib/columnSort"; + +const OPTS: SortOption[] = [ + { key: "date", label: "Date", defaultDir: "desc" }, + { key: "title", label: "Title" }, + { key: "shuffle", label: "Shuffle", directionless: true }, +]; + +function renderControl(sort: SortState, extra: Record = {}) { + const onChange = vi.fn(); + render( + , + ); + return { onChange }; +} + +afterEach(cleanup); + +describe("SortControl", () => { + it("renders one option per SortOption, no empty option without defaultLabel", () => { + renderControl({ key: "title", dir: "asc" }); + expect(screen.getAllByRole("option").map((o) => o.textContent)).toEqual([ + "Date", + "Title", + "Shuffle", + ]); + }); + + it("adds a leading no-sort option when defaultLabel is set (tables)", () => { + renderControl(null, { defaultLabel: "None" }); + const none = screen.getByRole("option", { name: "None" }) as HTMLOptionElement; + expect(none.value).toBe(""); + }); + + it("picking a new option fires onChange at that option's defaultDir", () => { + const { onChange } = renderControl({ key: "title", dir: "asc" }); + fireEvent.change(screen.getByRole("combobox"), { target: { value: "date" } }); + expect(onChange).toHaveBeenCalledWith({ key: "date", dir: "desc" }); + }); + + it("shows the direction toggle for a directional sort and flips it on click", () => { + const { onChange } = renderControl({ key: "date", dir: "desc" }); + const toggle = screen.getByRole("button", { name: "Descending" }); + fireEvent.click(toggle); + expect(onChange).toHaveBeenCalledWith({ key: "date", dir: "asc" }); + }); + + it("hides the direction toggle for a directionless sort", () => { + renderControl({ key: "shuffle", dir: "desc" }); + expect(screen.queryByRole("button")).toBeNull(); + }); + + it("renders the trailing adornment", () => { + renderControl( + { key: "shuffle", dir: "desc" }, + { + trailing: , + }, + ); + expect(screen.getByRole("button", { name: "Reshuffle" })).toBeTruthy(); + }); +}); diff --git a/frontend/src/lib/plexSort.ts b/frontend/src/lib/plexSort.ts index 7b7df241..4c92ee45 100644 --- a/frontend/src/lib/plexSort.ts +++ b/frontend/src/lib/plexSort.ts @@ -1,4 +1,4 @@ -import { type SortOption, type SortState } from "./columnSort"; +import { healSort, type SortOption, type SortState } from "./columnSort"; // Plex's sort vocabulary, source-agnostic like the feed's — a `SortOption[]` fed to the shared // SortControl. Direction lives in the SortState (no more sibling `sortDir` on PlexFilters); the wire @@ -16,9 +16,20 @@ function plexSortKeys(scope: string): string[] { export const PLEX_SORT_FALLBACK: SortState = { key: "title", dir: "asc" }; /** Labelled, alpha-ordered options for the SortControl. `label` resolves each key (i18n - * `plex.sort.`); pass a no-op label when only the key set matters (healing). */ + * `plex.sort.`). */ export function plexSortOptions(scope: string, label: (key: string) => string): SortOption[] { return plexSortKeys(scope) .map((key) => ({ key, label: label(key) })) .sort((a, b) => a.label.localeCompare(b.label)); } + +/** Heal a stored sort against a scope's available keys, collapsing an out-of-scope key to the default. + * The single heal both the sidebar control and the browse query use, so they never diverge. Always + * non-null (the fallback is), so callers can read `.key` without a guard. */ +export function healPlexSort(sort: SortState, scope: string): NonNullable { + return healSort( + sort, + plexSortKeys(scope).map((key) => ({ key, label: key })), + PLEX_SORT_FALLBACK, + )!; +} diff --git a/frontend/src/lib/useSortState.dom.test.ts b/frontend/src/lib/useSortState.dom.test.ts new file mode 100644 index 00000000..98989e9f --- /dev/null +++ b/frontend/src/lib/useSortState.dom.test.ts @@ -0,0 +1,56 @@ +// @vitest-environment jsdom +// +// The persistence/hydration wiring of useSortState (the pure hydrateSort/healSort logic it leans on is +// covered in columnSort.test.ts). storage is mocked with an in-memory map so the hook can be exercised +// without a logged-in account (getAccountRaw/setAccountRaw are account-keyed in the real module). +import { describe, it, expect, beforeEach, vi } from "vitest"; +import { renderHook, act } from "@testing-library/react"; + +const { store } = vi.hoisted(() => ({ store: new Map() })); +vi.mock("./storage", () => ({ + getAccountRaw: (k: string) => store.get(k) ?? null, + setAccountRaw: (k: string, v: string) => { + store.set(k, v); + }, +})); + +import { useSortState } from "./useSortState"; + +describe("useSortState", () => { + beforeEach(() => store.clear()); + + it("hydrates to the fallback when storage is empty", () => { + const { result } = renderHook(() => useSortState("k")); + expect(result.current[0]).toBeNull(); + const withFallback = renderHook(() => useSortState("k2", [], { key: "date", dir: "desc" })); + expect(withFallback.result.current[0]).toEqual({ key: "date", dir: "desc" }); + }); + + it("hydrates a stored SortState on mount", () => { + store.set("k", JSON.stringify({ key: "views", dir: "desc" })); + const { result } = renderHook(() => useSortState("k")); + expect(result.current[0]).toEqual({ key: "views", dir: "desc" }); + }); + + it("persists on set, and a fresh mount reads it back (write → reload → hydrate)", () => { + const first = renderHook(() => useSortState("k")); + act(() => first.result.current[1]({ key: "name", dir: "asc" })); + expect(first.result.current[0]).toEqual({ key: "name", dir: "asc" }); + expect(store.get("k")).toBe(JSON.stringify({ key: "name", dir: "asc" })); + const second = renderHook(() => useSortState("k")); + expect(second.result.current[0]).toEqual({ key: "name", dir: "asc" }); + }); + + it("heals a stored key that the supplied options no longer offer", () => { + store.set("k", JSON.stringify({ key: "gone", dir: "asc" })); + const opts = [{ key: "name", label: "Name" }]; + const { result } = renderHook(() => useSortState("k", opts, { key: "name", dir: "asc" })); + expect(result.current[0]).toEqual({ key: "name", dir: "asc" }); + }); + + it("keeps a stored key un-healed when no options are supplied (caller heals for display)", () => { + store.set("k", JSON.stringify({ key: "anything", dir: "desc" })); + const { result } = renderHook(() => useSortState("k")); + expect(result.current[0]).toEqual({ key: "anything", dir: "desc" }); + }); +});