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.
This commit is contained in:
@@ -9,8 +9,7 @@ import {
|
|||||||
type ReactNode,
|
type ReactNode,
|
||||||
} from "react";
|
} from "react";
|
||||||
import { qk } from "../lib/queryKeys";
|
import { qk } from "../lib/queryKeys";
|
||||||
import { healSort } from "../lib/columnSort";
|
import { healPlexSort } from "../lib/plexSort";
|
||||||
import { plexSortOptions, PLEX_SORT_FALLBACK } from "../lib/plexSort";
|
|
||||||
import { createPortal } from "react-dom";
|
import { createPortal } from "react-dom";
|
||||||
import { useTranslation } from "react-i18next";
|
import { useTranslation } from "react-i18next";
|
||||||
import {
|
import {
|
||||||
@@ -94,11 +93,7 @@ export default function PlexBrowse() {
|
|||||||
const dq = useDebounced(q.trim(), 350);
|
const dq = useDebounced(q.trim(), 350);
|
||||||
// The provider holds sort scope-agnostically; heal it against THIS scope's options so a movie-only
|
// 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.
|
// key (e.g. duration) left over from before a switch to shows doesn't ask for an unavailable sort.
|
||||||
const sortState = healSort(
|
const sortState = healPlexSort(sort, scope);
|
||||||
sort,
|
|
||||||
plexSortOptions(scope, () => ""),
|
|
||||||
PLEX_SORT_FALLBACK,
|
|
||||||
);
|
|
||||||
const sub = useHistorySubview<Sub>({ kind: "grid" });
|
const sub = useHistorySubview<Sub>({ kind: "grid" });
|
||||||
|
|
||||||
// Sidebar asked to open a playlist → push it as a subview (browser Back returns to the grid), then
|
// Sidebar asked to open a playlist → push it as a subview (browser Back returns to the grid), then
|
||||||
|
|||||||
@@ -72,12 +72,17 @@ export function PlexProvider({ children }: { children: ReactNode }) {
|
|||||||
const [sort, setSortState] = useState<SortState>(() => {
|
const [sort, setSortState] = useState<SortState>(() => {
|
||||||
const raw = getAccountRaw(LS.plexSort);
|
const raw = getAccountRaw(LS.plexSort);
|
||||||
if (!raw) return PLEX_SORT_FALLBACK;
|
if (!raw) return PLEX_SORT_FALLBACK;
|
||||||
|
let wasJson = false;
|
||||||
try {
|
try {
|
||||||
const parsed = parseSortState(JSON.parse(raw));
|
const parsed = parseSortState(JSON.parse(raw));
|
||||||
|
wasJson = true;
|
||||||
if (parsed) return parsed;
|
if (parsed) return parsed;
|
||||||
} catch {
|
} 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";
|
let dir: "asc" | "desc" = "asc";
|
||||||
try {
|
try {
|
||||||
if (
|
if (
|
||||||
@@ -94,7 +99,10 @@ export function PlexProvider({ children }: { children: ReactNode }) {
|
|||||||
const [filtersRaw, setFiltersRaw] = useState<string>(() => getAccountRaw(LS.plexFilters) ?? "{}");
|
const [filtersRaw, setFiltersRaw] = useState<string>(() => getAccountRaw(LS.plexFilters) ?? "{}");
|
||||||
const filters = useMemo<PlexFilters>(() => {
|
const filters = useMemo<PlexFilters>(() => {
|
||||||
try {
|
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 {
|
} catch {
|
||||||
return EMPTY_PLEX_FILTERS;
|
return EMPTY_PLEX_FILTERS;
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -4,8 +4,7 @@ import { useTranslation } from "react-i18next";
|
|||||||
import { useQuery, useQueryClient } from "@tanstack/react-query";
|
import { useQuery, useQueryClient } from "@tanstack/react-query";
|
||||||
import { Layers, ListMusic, Plus, SlidersHorizontal, X } from "lucide-react";
|
import { Layers, ListMusic, Plus, SlidersHorizontal, X } from "lucide-react";
|
||||||
import { api, EMPTY_PLEX_FILTERS, plexFilterCount, type PlexFilters } from "../lib/api";
|
import { api, EMPTY_PLEX_FILTERS, plexFilterCount, type PlexFilters } from "../lib/api";
|
||||||
import { healSort } from "../lib/columnSort";
|
import { healPlexSort, plexSortOptions, PLEX_SORT_FALLBACK } from "../lib/plexSort";
|
||||||
import { plexSortOptions, PLEX_SORT_FALLBACK } from "../lib/plexSort";
|
|
||||||
import { defaultLayout, type PanelLayout } from "../lib/panelLayout";
|
import { defaultLayout, type PanelLayout } from "../lib/panelLayout";
|
||||||
import { useDebounced } from "../lib/useDebounced";
|
import { useDebounced } from "../lib/useDebounced";
|
||||||
import { useScrollFade } from "../lib/useScrollFade";
|
import { useScrollFade } from "../lib/useScrollFade";
|
||||||
@@ -115,7 +114,10 @@ export default function PlexSidebar({ layout, setLayout, collapsed, onToggleColl
|
|||||||
const collections = collectionsQ.data?.collections ?? [];
|
const collections = collectionsQ.data?.collections ?? [];
|
||||||
|
|
||||||
const fCount = plexFilterCount(filters);
|
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 anyActive = activeCount > 0;
|
||||||
|
|
||||||
const patch = (p: Partial<PlexFilters>) => setFilters({ ...filters, ...p });
|
const patch = (p: Partial<PlexFilters>) => setFilters({ ...filters, ...p });
|
||||||
@@ -219,7 +221,7 @@ export default function PlexSidebar({ layout, setLayout, collapsed, onToggleColl
|
|||||||
body: (
|
body: (
|
||||||
<SortControl
|
<SortControl
|
||||||
options={sortOptions}
|
options={sortOptions}
|
||||||
sort={healSort(sort, sortOptions, PLEX_SORT_FALLBACK)}
|
sort={healedSort}
|
||||||
onChange={(next) => setSort(next ?? PLEX_SORT_FALLBACK)}
|
onChange={(next) => setSort(next ?? PLEX_SORT_FALLBACK)}
|
||||||
label={t("plex.filter.sort")}
|
label={t("plex.filter.sort")}
|
||||||
ascLabel={t("plex.filter.dir.asc")}
|
ascLabel={t("plex.filter.dir.asc")}
|
||||||
|
|||||||
@@ -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<string, unknown> = {}) {
|
||||||
|
const onChange = vi.fn();
|
||||||
|
render(
|
||||||
|
<SortControl
|
||||||
|
options={OPTS}
|
||||||
|
sort={sort}
|
||||||
|
onChange={onChange}
|
||||||
|
label="Sort"
|
||||||
|
ascLabel="Ascending"
|
||||||
|
descLabel="Descending"
|
||||||
|
{...extra}
|
||||||
|
/>,
|
||||||
|
);
|
||||||
|
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: <button>Reshuffle</button>,
|
||||||
|
},
|
||||||
|
);
|
||||||
|
expect(screen.getByRole("button", { name: "Reshuffle" })).toBeTruthy();
|
||||||
|
});
|
||||||
|
});
|
||||||
@@ -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
|
// 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
|
// 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" };
|
export const PLEX_SORT_FALLBACK: SortState = { key: "title", dir: "asc" };
|
||||||
|
|
||||||
/** Labelled, alpha-ordered options for the SortControl. `label` resolves each key (i18n
|
/** Labelled, alpha-ordered options for the SortControl. `label` resolves each key (i18n
|
||||||
* `plex.sort.<key>`); pass a no-op label when only the key set matters (healing). */
|
* `plex.sort.<key>`). */
|
||||||
export function plexSortOptions(scope: string, label: (key: string) => string): SortOption[] {
|
export function plexSortOptions(scope: string, label: (key: string) => string): SortOption[] {
|
||||||
return plexSortKeys(scope)
|
return plexSortKeys(scope)
|
||||||
.map((key) => ({ key, label: label(key) }))
|
.map((key) => ({ key, label: label(key) }))
|
||||||
.sort((a, b) => a.label.localeCompare(b.label));
|
.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<SortState> {
|
||||||
|
return healSort(
|
||||||
|
sort,
|
||||||
|
plexSortKeys(scope).map((key) => ({ key, label: key })),
|
||||||
|
PLEX_SORT_FALLBACK,
|
||||||
|
)!;
|
||||||
|
}
|
||||||
|
|||||||
@@ -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<string, string>() }));
|
||||||
|
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" });
|
||||||
|
});
|
||||||
|
});
|
||||||
Reference in New Issue
Block a user