diff --git a/backend/app/routes/plex/collections.py b/backend/app/routes/plex/collections.py index 58091e07..36fe86cb 100644 --- a/backend/app/routes/plex/collections.py +++ b/backend/app/routes/plex/collections.py @@ -208,6 +208,23 @@ def _reject_duplicate_name( raise HTTPException(status_code=409, detail="A collection with this name already exists") +def _write_or_pending_error(wrote: bool, e: Exception) -> HTTPException: + """Map a PlexError raised inside a mutate-then-resync block to the honest status. If the mutating + call itself failed (``wrote`` False) the write never landed → 422 "Plex write failed". If the + write succeeded but the follow-up resync READ (Plex metadata/children) failed, the change IS on + the Plex server → don't claim a write failure; report 502 "sync pending" (the next full sync + reconciles the local mirror).""" + if wrote: + return HTTPException( + status_code=502, + detail=( + "Collection updated in Plex, but refreshing the local copy failed; " + f"it will reconcile on the next sync ({e})" + ), + ) + return HTTPException(status_code=422, detail=f"Plex write failed: {e}") + + class CreateCollectionIn(BaseModel): library: str = "" title: str = "" @@ -239,15 +256,17 @@ def create_collection( if lib is None: raise HTTPException(status_code=404, detail="Unknown library") _reject_duplicate_name(db, lib.id, title) + wrote = False try: with PlexClient(db) as plex: new = plex.create_collection(lib.plex_key, title, seed) + wrote = True col = plex_sync.resync_collection(db, plex, lib, str(new.get("ratingKey"))) col.editable = True db.commit() except (PlexError, PlexNotConfigured) as e: db.rollback() - raise HTTPException(status_code=422, detail=f"Plex write failed: {e}") + raise _write_or_pending_error(wrote, e) return _collection_card(col) @@ -260,14 +279,16 @@ def collection_add_item( ) -> dict: col = _editable_collection_or_403(db, rating_key) lib = db.get(PlexLibrary, col.library_id) + wrote = False try: with PlexClient(db) as plex: plex.add_collection_item(rating_key, str(item_rating_key)) + wrote = True col = plex_sync.resync_collection(db, plex, lib, rating_key) db.commit() except (PlexError, PlexNotConfigured) as e: db.rollback() - raise HTTPException(status_code=422, detail=f"Plex write failed: {e}") + raise _write_or_pending_error(wrote, e) return _collection_card(col) @@ -280,14 +301,16 @@ def collection_remove_item( ) -> dict: col = _editable_collection_or_403(db, rating_key) lib = db.get(PlexLibrary, col.library_id) + wrote = False try: with PlexClient(db) as plex: plex.remove_collection_item(rating_key, str(item_rating_key)) + wrote = True col = plex_sync.resync_collection(db, plex, lib, rating_key) db.commit() except (PlexError, PlexNotConfigured) as e: db.rollback() - raise HTTPException(status_code=422, detail=f"Plex write failed: {e}") + raise _write_or_pending_error(wrote, e) return _collection_card(col) @@ -308,14 +331,16 @@ def collection_rename( raise HTTPException(status_code=400, detail="title is required") _reject_duplicate_name(db, col.library_id, title, exclude_rk=rating_key) lib = db.get(PlexLibrary, col.library_id) + wrote = False try: with PlexClient(db) as plex: plex.rename_collection(lib.plex_key, rating_key, title) + wrote = True col = plex_sync.resync_collection(db, plex, lib, rating_key) db.commit() except (PlexError, PlexNotConfigured) as e: db.rollback() - raise HTTPException(status_code=422, detail=f"Plex write failed: {e}") + raise _write_or_pending_error(wrote, e) return _collection_card(col) diff --git a/backend/tests/test_plex_collections_filter.py b/backend/tests/test_plex_collections_filter.py index 926655f1..dc668890 100644 --- a/backend/tests/test_plex_collections_filter.py +++ b/backend/tests/test_plex_collections_filter.py @@ -5,12 +5,15 @@ DB lane (`db` fixture) — skipped without Postgres. from datetime import datetime, timezone from fastapi.testclient import TestClient +from sqlalchemy import select import app.state as state from app.auth import admin_user, current_user from app.db import get_db from app.main import app from app.models import PlexCollection, PlexItem, PlexLibrary, PlexShow, User +from app.plex.client import PlexError +from app.routes.plex import collections as coll_routes def _seed(db): @@ -142,6 +145,130 @@ def test_collection_detail_hero_and_ordered_members(db, monkeypatch): assert [i["id"] for i in d["items"]] == ["early", "later"] # release-year order +# --- Write-through collection mutations (create/add/remove/rename write to Plex, then resync the local +# mirror). We stub PlexClient so the DB lane can drive the happy path, the 422-on-mutation-failure +# rollback, and the 502-on-resync-failure split (a successful write whose follow-up refresh read fails +# must NOT be reported as a write failure). ------------------------------------------------------------ +class _CollStub: + """A PlexClient stand-in recording calls; `fail_on` makes one method raise PlexError.""" + + def __init__(self, fail_on: str | None = None): + self.fail_on = fail_on + self.calls: list[str] = [] + + def __enter__(self): + return self + + def __exit__(self, *a): + return False + + def _maybe(self, name: str): + self.calls.append(name) + if self.fail_on == name: + raise PlexError("boom") + + def create_collection(self, section_key, title, item_rating_key): + self._maybe("create_collection") + return {"ratingKey": "NEW"} + + def add_collection_item(self, rk, item): + self._maybe("add_collection_item") + + def remove_collection_item(self, rk, item): + self._maybe("remove_collection_item") + + def rename_collection(self, section_key, rk, title): + self._maybe("rename_collection") + + def delete_collection(self, rk): + self._maybe("delete_collection") + + +def _resync_ok(db, plex, lib, rk): + """Stand in for the post-write refresh: return the existing local row (no live Plex read).""" + return db.execute(select(PlexCollection).where(PlexCollection.rating_key == rk)).scalars().first() + + +def _seed_editable(db): + lib = PlexLibrary(plex_key="1", title="Movies", kind="movie", enabled=True) + db.add(lib) + db.flush() + # Plain, admin-editable collection (title isn't a provider word, not smart → source "collection"). + db.add(PlexCollection(rating_key="A", library_id=lib.id, title="My Coll", editable=True, + child_count=1)) + db.add(PlexItem(rating_key="m1", library_id=lib.id, kind="movie", title="M1", + collection_keys=["A"])) + user = User(email="mut@example.com") + db.add(user) + db.commit() + return lib, user + + +def _admin_client(db, user, monkeypatch, stub, resync=_resync_ok): + monkeypatch.setattr(state, "_configured_cache", True) + monkeypatch.setattr(coll_routes, "PlexClient", lambda db: stub) + monkeypatch.setattr(coll_routes.plex_sync, "resync_collection", resync) + app.dependency_overrides[get_db] = lambda: db + app.dependency_overrides[current_user] = lambda: user + app.dependency_overrides[admin_user] = lambda: user + return TestClient(app) + + +def test_rename_writes_through_to_plex(db, monkeypatch): + _lib, user = _seed_editable(db) + stub = _CollStub() + client = _admin_client(db, user, monkeypatch, stub) + try: + r = client.patch("/api/plex/collections/A", json={"title": "Renamed"}) + assert r.status_code == 200, r.text + assert "rename_collection" in stub.calls + finally: + app.dependency_overrides.clear() + + +def test_remove_item_writes_through_to_plex(db, monkeypatch): + _lib, user = _seed_editable(db) + stub = _CollStub() + client = _admin_client(db, user, monkeypatch, stub) + try: + r = client.delete("/api/plex/collections/A/items/m1") + assert r.status_code == 200, r.text + assert "remove_collection_item" in stub.calls + finally: + app.dependency_overrides.clear() + + +def test_mutation_failure_returns_422_and_keeps_local_row(db, monkeypatch): + _lib, user = _seed_editable(db) + stub = _CollStub(fail_on="rename_collection") + client = _admin_client(db, user, monkeypatch, stub) + try: + r = client.patch("/api/plex/collections/A", json={"title": "Renamed"}) + assert r.status_code == 422, r.text + finally: + app.dependency_overrides.clear() + row = db.execute(select(PlexCollection).where(PlexCollection.rating_key == "A")).scalars().first() + assert row.title == "My Coll" # rolled back — the write never landed + + +def test_resync_failure_after_successful_write_returns_502(db, monkeypatch): + # The mutation landed on Plex; only the follow-up refresh READ failed → 502 "sync pending", NOT a + # false 422 "write failed". Regression guard for the mutate-then-resync error split. + _lib, user = _seed_editable(db) + stub = _CollStub() + + def _resync_boom(db_, plex, lib_, rk): + raise PlexError("read failed") + + client = _admin_client(db, user, monkeypatch, stub, resync=_resync_boom) + try: + r = client.patch("/api/plex/collections/A", json={"title": "Renamed"}) + assert r.status_code == 502, r.text + assert "rename_collection" in stub.calls # the write DID happen + finally: + app.dependency_overrides.clear() + + def test_show_collection_gets_montage_art_from_shows(db, monkeypatch): # A show-library collection must get its montage from PlexShow members (else it renders blank) — the # A3 fix that unions movie + show art. Runtime stays 0 (shows have no single duration). diff --git a/frontend/src/components/PageScroller.tsx b/frontend/src/components/PageScroller.tsx index 2789c0a7..7733f3da 100644 --- a/frontend/src/components/PageScroller.tsx +++ b/frontend/src/components/PageScroller.tsx @@ -8,6 +8,18 @@ import { ensureNavId, peekNavId, subscribeNav } from "../lib/history"; // top WITHIN the same entry. Module-level so a view's position survives its scroller unmounting // (opening a channel page swaps in a different shell/scroller) and is restored when it returns. const savedScroll = new Map(); +// The key embeds an ever-increasing history-entry id, so without a bound this map would grow one +// entry per scrolled navigation for the life of the session. Cap it with a small LRU (Map preserves +// insertion order): re-insert on write to mark most-recently-used, evict the oldest past the cap. +const SCROLL_MEMORY_CAP = 200; +function rememberScroll(key: string, top: number): void { + savedScroll.delete(key); + savedScroll.set(key, top); + if (savedScroll.size > SCROLL_MEMORY_CAP) { + const oldest = savedScroll.keys().next().value; + if (oldest !== undefined) savedScroll.delete(oldest); + } +} // Put the scroller back to `top`. The feed is virtualized, so on a remount the list needs a frame // or two to render enough rows to reach its full height; an immediate `scrollTop` would otherwise @@ -120,7 +132,7 @@ export default function PageScroller({ if (!el) return; const onScroll = () => { if (scrollKeyRef.current == null) return; - savedScroll.set(`${peekNavId()}:${scrollKeyRef.current}`, el.scrollTop); + rememberScroll(`${peekNavId()}:${scrollKeyRef.current}`, el.scrollTop); }; el.addEventListener("scroll", onScroll, { passive: true }); window.addEventListener("popstate", resolve); diff --git a/frontend/src/components/PlexCollectionView.tsx b/frontend/src/components/PlexCollectionView.tsx index c1c53eb6..e7496b67 100644 --- a/frontend/src/components/PlexCollectionView.tsx +++ b/frontend/src/components/PlexCollectionView.tsx @@ -3,7 +3,7 @@ import { useTranslation } from "react-i18next"; import { useQuery, useQueryClient } from "@tanstack/react-query"; import { Play, Shuffle, Pencil, Trash2, Plus, X, Lock, Check, Search } from "lucide-react"; import { qk } from "../lib/queryKeys"; -import { api, type PlexCard, type PlexCollectionDetail } from "../lib/api"; +import { api, isIntrinsicReadOnly, type PlexCard, type PlexCollectionDetail } from "../lib/api"; import { formatRuntime } from "../lib/format"; import { notifyActionError } from "../lib/actionErrors"; import { useDebounced } from "../lib/useDebounced"; @@ -67,8 +67,9 @@ export default function PlexCollectionView({ // The Play queue is the playable-leaf members (movies/episodes) in order — a show can't queue directly. const playable = items.filter((c) => c.type !== "show").map((c) => c.id); const canEdit = isAdmin && !!d?.can_edit; - const canEnableEdit = - isAdmin && !d?.can_edit && !d?.editable && !d?.smart && d?.source === "collection"; + // A plain (non-smart, non-external) collection can be admin-enabled for editing. Shares + // isIntrinsicReadOnly with the browse tile so the lock/edit affordance can't drift between them. + const canEnableEdit = isAdmin && !d?.can_edit && !d?.editable && !!d && !isIntrinsicReadOnly(d); const readOnly = !d?.can_edit && !canEnableEdit; // external / smart auto-list const toggleWatched = (card: PlexCard) => @@ -120,6 +121,9 @@ export default function PlexCollectionView({ } async function doRemove(card: PlexCard) { + // Write-through to the shared Plex library — confirm first, matching the delete-collection guard. + if (!(await confirm({ message: t("plex.collection.removeConfirm", { title: card.title }) }))) + return; setBusy(true); try { await api.plexCollectionRemoveItem(collectionId, card.id); @@ -381,6 +385,8 @@ function AddItemPanel({

{t("plex.collection.addHint")}

) : q.isLoading ? ( + ) : q.isError ? ( +

{t("common.loadError")}

) : results.length === 0 ? (

{t("plex.collection.addNoResults")}

) : ( diff --git a/frontend/src/components/plexBrowseLists.tsx b/frontend/src/components/plexBrowseLists.tsx index 9255a4bd..37dade26 100644 --- a/frontend/src/components/plexBrowseLists.tsx +++ b/frontend/src/components/plexBrowseLists.tsx @@ -4,9 +4,16 @@ import type { TFunction } from "i18next"; import { useQuery, useQueryClient } from "@tanstack/react-query"; import { Check, ListMusic, Lock, Plus, Search, X } from "lucide-react"; import { qk } from "../lib/queryKeys"; -import { api, type PlexCard, type PlexCollection } from "../lib/api"; +import { + api, + HttpError, + isIntrinsicReadOnly, + type PlexCard, + type PlexCollection, +} from "../lib/api"; import { formatRuntime } from "../lib/format"; import { notifyActionError } from "../lib/actionErrors"; +import { notify } from "../lib/notifications"; import { useDebounced } from "../lib/useDebounced"; import { LoadingState, StateMessage } from "./QueryState"; import PosterTile from "./PosterTile"; @@ -21,12 +28,6 @@ function collectionMeta(c: PlexCollection, t: TFunction): string { return parts.join(" · "); } -// A read-only collection (a smart or external auto-list — imdb/tmdb/…) can never be edited; mark it up -// front on the tile, mirroring the badge on the info page. -function isReadOnlyCollection(c: PlexCollection): boolean { - return !!c.smart || (!!c.source && c.source !== "collection"); -} - // The Playlist / Collection browse surfaces reached from the header pill (Piece 2): a grid of tiles, // each a way INTO that area — a playlist tile opens the playlist view, a collection tile filters the // title grid by that collection. Both reuse the existing list endpoints (plexPlaylists / plexCollections @@ -87,8 +88,12 @@ function NewCollectionPanel({ const [busy, setBusy] = useState(false); const debounced = useDebounced(term.trim(), 300); const trimmed = name.trim(); - const dup = trimmed.length > 0 && existingNames.has(trimmed.toLowerCase()); - const nameOk = trimmed.length > 0 && !dup; + // Advisory only: `existingNames` is the search-scoped, all-library collection union, whereas the + // backend enforces uniqueness PER library (Plex only conflates same-named collections within one + // section). So a match here is a hint, not a hard block — it must not gate creation, or a valid + // same-name collection in another library would be un-creatable. The backend's 409 is authoritative. + const dupHint = trimmed.length > 0 && existingNames.has(trimmed.toLowerCase()); + const nameOk = trimmed.length > 0; const q = useQuery({ queryKey: qk.plexLibrary("collection-new", debounced), queryFn: () => api.plexLibrary({ scope: "both", q: debounced, limit: 12 }), @@ -103,7 +108,13 @@ function NewCollectionPanel({ const col = await api.plexCreateCollection("", trimmed, card.id); // backend derives the library onCreated(col.id); } catch (e) { - notifyActionError(e, t("plex.collection.editFailed")); + // The backend is authoritative on per-library name uniqueness — localize its 409 to nameTaken + // (notifyActionError would otherwise surface the server's raw English detail). + if (e instanceof HttpError && e.status === 409) { + notify({ level: "error", message: t("plex.collection.nameTaken") }); + } else { + notifyActionError(e, t("plex.collection.editFailed")); + } setBusy(false); } } @@ -128,7 +139,7 @@ function NewCollectionPanel({ aria-label={t("plex.collection.newCollection")} className="glass-card w-full rounded-lg px-3 py-1.5 text-sm focus:outline-hidden focus:ring-2 focus:ring-accent/40" /> - {dup ? ( + {dupHint ? (

{t("plex.collection.nameTaken")}

) : (

{t("plex.collection.newHint")}

@@ -146,6 +157,8 @@ function NewCollectionPanel({ {!nameOk || debounced.length === 0 ? null : q.isLoading ? ( + ) : q.isError ? ( +

{t("common.loadError")}

) : results.length === 0 ? (

{t("plex.collection.addNoResults")}

) : ( @@ -244,7 +257,7 @@ export function PlexCollectionsBrowse({ onPick }: { onPick: (id: string, title: art={c.art} subtitle={collectionMeta(c, t)} topRight={ - isReadOnlyCollection(c) ? ( + isIntrinsicReadOnly(c) ? ( ): boolean { + return !!c.smart || (!!c.source && c.source !== "collection"); +} // The Collection info page: the card fields + hero aggregates + the ordered members. export interface PlexCollectionDetail extends PlexCollection { content_rating?: string | null; diff --git a/frontend/src/lib/history.ts b/frontend/src/lib/history.ts index fc82face..1134c58d 100644 --- a/frontend/src/lib/history.ts +++ b/frontend/src/lib/history.ts @@ -21,6 +21,15 @@ if (typeof window !== "undefined" && "scrollRestoration" in window.history) { window.history.scrollRestoration = "manual"; } let navCounter = 0; +// sfScroll ids are persisted into window.history.state and survive a full page reload, but this +// module-level counter resets to 0 on every load. Seed it past the current entry's id so freshly +// minted ids can't collide with ids already carried by pre-reload back-stack entries (ids are +// assigned in forward order, so the current entry's id is ≥ every ancestor's) — a collision would +// otherwise cross-contaminate PageScroller's per-entry scroll offsets after a reload + navigation. +if (typeof window !== "undefined") { + const seed = (window.history.state as { sfScroll?: number } | null)?.sfScroll; + if (typeof seed === "number") navCounter = seed; +} /** A fresh entry id — stamp it into the state of every pushState that opens a new main-content view. */ export function nextNavId(): number { return ++navCounter; @@ -29,7 +38,12 @@ export function nextNavId(): number { * initial load, or an entry pushed without one — so the id survives Back/Forward. */ export function ensureNavId(): number { const st = (window.history.state ?? {}) as { sfScroll?: number }; - if (typeof st.sfScroll === "number") return st.sfScroll; + if (typeof st.sfScroll === "number") { + // Keep the counter ahead of any id we observe (e.g. a Back landed on a higher-id entry after a + // reload seeded us low) so the next mint never duplicates a live entry's id. + if (st.sfScroll > navCounter) navCounter = st.sfScroll; + return st.sfScroll; + } const id = nextNavId(); window.history.replaceState({ ...st, sfScroll: id }, ""); return id;