fix(plex): address S4 review — cap filmography, dedupe toggle, scroll guard, tests
- Efficiency: cap each person section at _PERSON_SECTION_MAX (200) so a prolific person can't return an unbounded list the client mounts a card per row for. - Reuse: extract the optimistic watch-toggle (flip → persist → resync) into a shared toggleWatchState helper; the grid and the person page each supply their own cache updater. - Correctness (scroll): onOpenPerson now stashes infoScrollRef ONLY when leaving an info page (not from a show/grid context), and a jumped-to info page resets it — so a stale offset can't wrongly restore a fresh info page. - Test-coverage: add PlexPersonView.dom.test.tsx (export CastStrip + PlexPersonView) covering the cast-row rewiring (open-person vs filter fallback) and the person page's sections + filter + empty state. The name-as-identity finding is inherent to Plex cast carrying no per-person id (PlexPerson is name-keyed too); documented in the endpoint docstring, not structurally fixable in S4.
This commit is contained in:
@@ -14,6 +14,7 @@ import { healPlexSort, plexSortOptions, PLEX_SORT_FALLBACK } from "../lib/plexSo
|
||||
import SortControl from "./SortControl";
|
||||
import { createPortal } from "react-dom";
|
||||
import { useTranslation } from "react-i18next";
|
||||
import type { TFunction } from "i18next";
|
||||
import {
|
||||
useInfiniteQuery,
|
||||
useQuery,
|
||||
@@ -88,6 +89,25 @@ type Sub =
|
||||
|
||||
const PAGE = 40;
|
||||
|
||||
// Persist a watch-state toggle from any Plex surface: flip via the caller's `optimistic` cache
|
||||
// updater, call the API, surface a failure (the click used to look dead on 403/404/429), then
|
||||
// `resync` (invalidate) — which corrects the optimistic flip to truth either way. Shared by the
|
||||
// browse grid and the person page, each of which owns a different cache shape.
|
||||
async function toggleWatchState(
|
||||
card: PlexCard,
|
||||
t: TFunction,
|
||||
handlers: { optimistic: (next: string) => void; resync: () => void },
|
||||
) {
|
||||
const next = card.status === "watched" ? "new" : "watched";
|
||||
handlers.optimistic(next);
|
||||
try {
|
||||
await api.plexSetState(card.id, next);
|
||||
} catch (e) {
|
||||
notifyActionError(e, t("plex.watchedFailed"));
|
||||
}
|
||||
handlers.resync();
|
||||
}
|
||||
|
||||
export default function PlexBrowse() {
|
||||
const { t } = useTranslation();
|
||||
const qc = useQueryClient();
|
||||
@@ -123,6 +143,7 @@ export default function PlexBrowse() {
|
||||
// info page like the card's ⓘ, a show's drill-down), then clear the hand-off so it doesn't re-open.
|
||||
useEffect(() => {
|
||||
if (jumpTo != null) {
|
||||
if (jumpTo.type !== "show") infoScrollRef.current = 0; // a jumped-to info page starts at the top
|
||||
sub.open(
|
||||
jumpTo.type === "show" ? { kind: "show", id: jumpTo.id } : { kind: "info", id: jumpTo.id },
|
||||
);
|
||||
@@ -235,10 +256,12 @@ export default function PlexBrowse() {
|
||||
infoScrollRef.current = 0; // fresh info page starts at the top
|
||||
sub.open({ kind: "info", id: card.id });
|
||||
}
|
||||
// Open a cast/crew member's filmography page (from an info/show cast row). Remember the info-page
|
||||
// scroll so browser Back to a cast row lands where it was, mirroring applyFilter's restore.
|
||||
// Open a cast/crew member's filmography page (from an info/show cast row, or an actor filter-chip).
|
||||
// Only when leaving an INFO page do we stash its scroll (so browser Back to that cast row lands where
|
||||
// it was, like applyFilter) — from a show/grid context infoScrollRef must stay untouched, else a
|
||||
// later info page would wrongly restore this stale offset.
|
||||
function onOpenPerson(name: string) {
|
||||
infoScrollRef.current = scroller()?.scrollTop ?? 0;
|
||||
if (sub.view.kind === "info") infoScrollRef.current = scroller()?.scrollTop ?? 0;
|
||||
sub.open({ kind: "person", name });
|
||||
}
|
||||
// Optimistically flip a card's status in every cached library page — both the title grid (`items`)
|
||||
@@ -258,18 +281,11 @@ export default function PlexBrowse() {
|
||||
: old,
|
||||
);
|
||||
}
|
||||
async function toggleWatched(card: PlexCard) {
|
||||
const next = card.status === "watched" ? "new" : "watched";
|
||||
optimisticStatus(card.id, next);
|
||||
// On failure surface the server's reason (403/404/429) — the toggle used to fail silently, so the
|
||||
// click looked dead. The invalidate below runs either way, resyncing the optimistic flip to truth.
|
||||
try {
|
||||
await api.plexSetState(card.id, next);
|
||||
} catch (e) {
|
||||
notifyActionError(e, t("plex.watchedFailed"));
|
||||
}
|
||||
qc.invalidateQueries({ queryKey: qk.plexLibrary() });
|
||||
}
|
||||
const toggleWatched = (card: PlexCard) =>
|
||||
toggleWatchState(card, t, {
|
||||
optimistic: (next) => optimisticStatus(card.id, next),
|
||||
resync: () => qc.invalidateQueries({ queryKey: qk.plexLibrary() }),
|
||||
});
|
||||
// Apply a metadata filter (clicked on an info page / show hero / cast) then show the unified grid.
|
||||
// Array filters (genres/people/studios) UNION with what's set; scalars replace. Clicking a person
|
||||
// (cast/crew) widens to the 'both' scope so their movies AND shows show up together (mixed feed).
|
||||
@@ -715,7 +731,7 @@ function PlexInfoView({
|
||||
// The inverse of the cast-name → library-filter link: opened from a cast row (info/show page), it
|
||||
// shows the person's headshot + name, a "filter the library by them" action, and their titles as two
|
||||
// poster-card sections (Movies / Shows). Mirrors PlexInfoView's fetch-wrapper + glass-hero shape.
|
||||
function PlexPersonView({
|
||||
export function PlexPersonView({
|
||||
name,
|
||||
onBack,
|
||||
onOpenCard,
|
||||
@@ -733,27 +749,25 @@ function PlexPersonView({
|
||||
const q = useQuery({ queryKey: qk.plexPerson(name), queryFn: () => api.plexPerson(name) });
|
||||
|
||||
// Quick watch-toggle from a filmography card: optimistically flip in THIS person's cached payload
|
||||
// (both the movies and shows arrays — an id matches only one), then persist + resync. Mirrors the
|
||||
// grid's optimisticStatus on the person-detail shape; also invalidates the library so its grid agrees.
|
||||
async function toggle(card: PlexCard) {
|
||||
const next = card.status === "watched" ? "new" : "watched";
|
||||
qc.setQueryData<PlexPersonDetail>(qk.plexPerson(name), (old) =>
|
||||
old
|
||||
? {
|
||||
...old,
|
||||
movies: old.movies.map((m) => (m.id === card.id ? { ...m, status: next } : m)),
|
||||
shows: old.shows.map((s) => (s.id === card.id ? { ...s, status: next } : s)),
|
||||
}
|
||||
: old,
|
||||
);
|
||||
try {
|
||||
await api.plexSetState(card.id, next);
|
||||
} catch (e) {
|
||||
notifyActionError(e, t("plex.watchedFailed"));
|
||||
}
|
||||
qc.invalidateQueries({ queryKey: qk.plexPerson(name) });
|
||||
qc.invalidateQueries({ queryKey: qk.plexLibrary() });
|
||||
}
|
||||
// (both the movies and shows arrays — an id matches only one), then persist + resync via the shared
|
||||
// helper; also invalidate the library so its grid agrees.
|
||||
const toggle = (card: PlexCard) =>
|
||||
toggleWatchState(card, t, {
|
||||
optimistic: (next) =>
|
||||
qc.setQueryData<PlexPersonDetail>(qk.plexPerson(name), (old) =>
|
||||
old
|
||||
? {
|
||||
...old,
|
||||
movies: old.movies.map((m) => (m.id === card.id ? { ...m, status: next } : m)),
|
||||
shows: old.shows.map((s) => (s.id === card.id ? { ...s, status: next } : s)),
|
||||
}
|
||||
: old,
|
||||
),
|
||||
resync: () => {
|
||||
qc.invalidateQueries({ queryKey: qk.plexPerson(name) });
|
||||
qc.invalidateQueries({ queryKey: qk.plexLibrary() });
|
||||
},
|
||||
});
|
||||
|
||||
const section = (titleKey: string, cards: PlexCard[]) =>
|
||||
cards.length > 0 ? (
|
||||
@@ -985,7 +999,7 @@ function SeasonCard({
|
||||
);
|
||||
}
|
||||
|
||||
function CastStrip({
|
||||
export function CastStrip({
|
||||
cast,
|
||||
onFilter,
|
||||
onOpenPerson,
|
||||
|
||||
@@ -0,0 +1,111 @@
|
||||
// @vitest-environment jsdom
|
||||
//
|
||||
// E5 S4: the actor detail page. Covers the two pieces the /code-review flagged as untested — the
|
||||
// cast-row rewiring (a cast click now OPENS the person page via onOpenPerson, falling back to the old
|
||||
// filter gesture only when no onOpenPerson is wired) and PlexPersonView's render (headshot/name hero,
|
||||
// the Movies/Shows sections, and the "filter library by this person" action). i18n is key-passthrough
|
||||
// and the api is mocked so the query resolves synchronously in the test.
|
||||
import { describe, it, expect, afterEach, vi } from "vitest";
|
||||
import { render, screen, fireEvent, cleanup } from "@testing-library/react";
|
||||
import { QueryClient, QueryClientProvider } from "@tanstack/react-query";
|
||||
|
||||
vi.mock("react-i18next", async (importOriginal) => {
|
||||
const actual = await importOriginal<typeof import("react-i18next")>();
|
||||
return { ...actual, useTranslation: () => ({ t: (key: string) => key }) };
|
||||
});
|
||||
|
||||
const plexPerson = vi.fn();
|
||||
const plexSetState = vi.fn(() => Promise.resolve());
|
||||
vi.mock("../lib/api", () => ({
|
||||
api: {
|
||||
plexPerson: (...a: unknown[]) => plexPerson(...a),
|
||||
plexSetState: (...a: unknown[]) => plexSetState(...a),
|
||||
},
|
||||
EMPTY_PLEX_FILTERS: {},
|
||||
plexFilterCount: () => 0,
|
||||
}));
|
||||
|
||||
import { CastStrip, PlexPersonView } from "./PlexBrowse";
|
||||
|
||||
afterEach(cleanup);
|
||||
|
||||
const card = (id: string, title: string, type: "movie" | "show" = "movie") => ({
|
||||
id,
|
||||
type,
|
||||
title,
|
||||
year: 2000,
|
||||
duration_seconds: 3600,
|
||||
thumb: `/api/plex/image/${id}`,
|
||||
playable: "direct",
|
||||
status: "new",
|
||||
position_seconds: 0,
|
||||
});
|
||||
|
||||
function wrap(ui: React.ReactElement) {
|
||||
const qc = new QueryClient({ defaultOptions: { queries: { retry: false } } });
|
||||
return render(<QueryClientProvider client={qc}>{ui}</QueryClientProvider>);
|
||||
}
|
||||
|
||||
describe("CastStrip cast click", () => {
|
||||
it("opens the person page (onOpenPerson) rather than filtering, when both are wired", () => {
|
||||
const onOpenPerson = vi.fn();
|
||||
const onFilter = vi.fn();
|
||||
render(
|
||||
<CastStrip cast={[{ name: "Jane Doe" }]} onOpenPerson={onOpenPerson} onFilter={onFilter} />,
|
||||
);
|
||||
fireEvent.click(screen.getByTitle("plex.info.viewPerson"));
|
||||
expect(onOpenPerson).toHaveBeenCalledWith("Jane Doe");
|
||||
expect(onFilter).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("falls back to the filter gesture when only onFilter is wired (e.g. the overlay)", () => {
|
||||
const onFilter = vi.fn();
|
||||
render(<CastStrip cast={[{ name: "Jane Doe" }]} onFilter={onFilter} />);
|
||||
fireEvent.click(screen.getByTitle("plex.info.filterActor"));
|
||||
expect(onFilter).toHaveBeenCalledWith({ actors: ["Jane Doe"] });
|
||||
});
|
||||
});
|
||||
|
||||
describe("PlexPersonView", () => {
|
||||
it("renders the Movies + Shows sections and the filter-by-person action", async () => {
|
||||
plexPerson.mockResolvedValue({
|
||||
name: "Jane Doe",
|
||||
photo: null,
|
||||
movies: [card("m1", "Movie One")],
|
||||
shows: [card("s1", "Show One", "show")],
|
||||
});
|
||||
const onFilter = vi.fn();
|
||||
wrap(
|
||||
<PlexPersonView
|
||||
name="Jane Doe"
|
||||
onBack={() => {}}
|
||||
onOpenCard={() => {}}
|
||||
onInfoCard={() => {}}
|
||||
onFilter={onFilter}
|
||||
/>,
|
||||
);
|
||||
// Both section grids render their cards once the mocked query resolves.
|
||||
await screen.findByText("Movie One");
|
||||
expect(screen.getByText("Show One")).toBeTruthy();
|
||||
expect(screen.getByText("plex.person.movies")).toBeTruthy(); // section headings (key passthrough)
|
||||
expect(screen.getByText("plex.person.shows")).toBeTruthy();
|
||||
|
||||
fireEvent.click(screen.getByText("plex.person.filterBy"));
|
||||
expect(onFilter).toHaveBeenCalledWith({ actors: ["Jane Doe"] });
|
||||
expect(plexPerson).toHaveBeenCalledWith("Jane Doe");
|
||||
});
|
||||
|
||||
it("shows the empty message when the person has no titles", async () => {
|
||||
plexPerson.mockResolvedValue({ name: "Nobody", photo: null, movies: [], shows: [] });
|
||||
wrap(
|
||||
<PlexPersonView
|
||||
name="Nobody"
|
||||
onBack={() => {}}
|
||||
onOpenCard={() => {}}
|
||||
onInfoCard={() => {}}
|
||||
onFilter={() => {}}
|
||||
/>,
|
||||
);
|
||||
await screen.findByText("plex.person.empty");
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user