From 7d2bee59a3a7cb552260fa1524c78839ea1bff4e Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sun, 23 Aug 2026 07:22:53 +0200 Subject: [PATCH] fix(plex): round-2 deep-review follow-ups on the review fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - collections: the resync-failed-after-successful-write path now returns 422 (honest "reconcile on next sync" detail), NOT 502. The frontend treats 502/503/504 as gateway-unreachable and raises a sticky offline banner (api/core.ts RETRIABLE_GATEWAY); the app answered, so a 422 action-error toast is correct. Removes the false-offline regression the 502 introduced, and — since 422 isn't retried — the create double-write vector with it. - tests: make the mutate-then-resync rollback assertion non-vacuous — the resync stub now flushes a partial local change before raising, so the test proves db.rollback() actually undoes it; split into a write-failed (422) case and a resync-failed (422 + reconcile detail + rollback) case. - New-collection dup hint is now muted-tone with its own nameMaybeTaken copy (per-library uniqueness means the name may be free here) instead of the red authoritative nameTaken, which the still-enabled create flow contradicts. - history: cover the navCounter post-reload collision guard (ensureNavId advances the counter past a higher persisted sfScroll id). --- backend/app/routes/plex/collections.py | 26 ++++++++++--------- backend/tests/test_plex_collections_filter.py | 24 ++++++++++++----- frontend/src/components/plexBrowseLists.tsx | 2 +- frontend/src/i18n/locales/en/plex.json | 1 + frontend/src/i18n/locales/hu/plex.json | 1 + frontend/src/lib/history.nav.dom.test.ts | 10 +++++++ 6 files changed, 44 insertions(+), 20 deletions(-) diff --git a/backend/app/routes/plex/collections.py b/backend/app/routes/plex/collections.py index 36fe86cb..84c4aff0 100644 --- a/backend/app/routes/plex/collections.py +++ b/backend/app/routes/plex/collections.py @@ -209,20 +209,22 @@ def _reject_duplicate_name( 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).""" + """Map a PlexError raised inside a mutate-then-resync block to the honest message. If the mutating + call itself failed (``wrote`` False) the write never landed → "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; say the local copy will reconcile on the next sync. + + Both use 422, NOT a 5xx: the frontend treats 502/503/504 as "gateway unreachable" and raises a + sticky offline banner (see api/core.ts RETRIABLE_GATEWAY). A 422 surfaces the detail as an ordinary + action-error toast, which is what both of these are — the app answered, it didn't go unreachable.""" 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})" - ), + 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}") + else: + detail = f"Plex write failed: {e}" + return HTTPException(status_code=422, detail=detail) class CreateCollectionIn(BaseModel): diff --git a/backend/tests/test_plex_collections_filter.py b/backend/tests/test_plex_collections_filter.py index dc668890..11a0891d 100644 --- a/backend/tests/test_plex_collections_filter.py +++ b/backend/tests/test_plex_collections_filter.py @@ -238,35 +238,45 @@ def test_remove_item_writes_through_to_plex(db, monkeypatch): app.dependency_overrides.clear() -def test_mutation_failure_returns_422_and_keeps_local_row(db, monkeypatch): +def test_mutation_failure_returns_422_write_failed(db, monkeypatch): + # The Plex mutation itself raised → the write never landed → 422 "Plex write failed", and resync + # is never reached (stub.calls holds only the mutating call). _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 + assert "write failed" in r.json()["detail"].lower() 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. +def test_resync_failure_after_successful_write_is_422_and_rolls_back(db, monkeypatch): + # The mutation landed on Plex; only the follow-up refresh READ failed. We must (a) NOT claim a + # write failure and NOT use a 5xx (which the frontend treats as offline) → 422 with a "reconcile + # on the next sync" detail, and (b) roll back any partial local upsert the resync had flushed. _lib, user = _seed_editable(db) stub = _CollStub() def _resync_boom(db_, plex, lib_, rk): + # Simulate resync having flushed a partial local change before its Plex read blew up — the + # route's rollback must undo it (this is what makes the rollback assertion non-vacuous). + row = db_.execute(select(PlexCollection).where(PlexCollection.rating_key == rk)).scalars().first() + row.title = "PARTIAL_SHOULD_ROLL_BACK" + db_.flush() 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 r.status_code == 422, r.text + assert "reconcile" in r.json()["detail"].lower() assert "rename_collection" in stub.calls # the write DID happen finally: app.dependency_overrides.clear() + row = db.execute(select(PlexCollection).where(PlexCollection.rating_key == "A")).scalars().first() + assert row.title == "My Coll" # the partial resync upsert was rolled back def test_show_collection_gets_montage_art_from_shows(db, monkeypatch): diff --git a/frontend/src/components/plexBrowseLists.tsx b/frontend/src/components/plexBrowseLists.tsx index 37dade26..e8f54a3a 100644 --- a/frontend/src/components/plexBrowseLists.tsx +++ b/frontend/src/components/plexBrowseLists.tsx @@ -140,7 +140,7 @@ function NewCollectionPanel({ className="glass-card w-full rounded-lg px-3 py-1.5 text-sm focus:outline-hidden focus:ring-2 focus:ring-accent/40" /> {dupHint ? ( -

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

+

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

) : (

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

)} diff --git a/frontend/src/i18n/locales/en/plex.json b/frontend/src/i18n/locales/en/plex.json index de6683d5..835ad1a2 100644 --- a/frontend/src/i18n/locales/en/plex.json +++ b/frontend/src/i18n/locales/en/plex.json @@ -11,6 +11,7 @@ "enableEdit": "Enable editing", "removeItem": "Remove from collection", "removeConfirm": "Remove \"{{title}}\" from this collection? This affects all users on Plex.", + "nameMaybeTaken": "A collection with this name may already exist in another library — you can still create it here.", "readOnly": "Read-only", "empty": "This collection has no items.", "more": "More", diff --git a/frontend/src/i18n/locales/hu/plex.json b/frontend/src/i18n/locales/hu/plex.json index 29ff3af9..43bb5684 100644 --- a/frontend/src/i18n/locales/hu/plex.json +++ b/frontend/src/i18n/locales/hu/plex.json @@ -11,6 +11,7 @@ "enableEdit": "Szerkesztés engedélyezése", "removeItem": "Eltávolítás a kollekcióból", "removeConfirm": "Eltávolítod a(z) „{{title}}” elemet ebből a kollekcióból? Ez a Plexen minden felhasználót érint.", + "nameMaybeTaken": "Lehet, hogy már létezik ilyen nevű kollekció egy másik könyvtárban — itt akkor is létrehozhatod.", "readOnly": "Csak olvasható", "empty": "Ennek a kollekciónak nincsenek elemei.", "more": "Több", diff --git a/frontend/src/lib/history.nav.dom.test.ts b/frontend/src/lib/history.nav.dom.test.ts index 0cfc9c13..57cc863c 100644 --- a/frontend/src/lib/history.nav.dom.test.ts +++ b/frontend/src/lib/history.nav.dom.test.ts @@ -42,6 +42,16 @@ describe("nav scroll ids", () => { expect(second).toBeGreaterThan(first); }); + it("ensureNavId advances the counter past a higher persisted id (post-reload collision guard)", () => { + // After a full reload the module counter resets to 0 while sfScroll ids persist in history.state. + // Landing on an entry whose id is above the counter must push the counter past it, so the next + // minted id can't duplicate a live back-stack entry and mis-restore its scroll. + const high = 999999; + window.history.replaceState({ sfScroll: high }, ""); + expect(ensureNavId()).toBe(high); // returns the persisted id, doesn't reassign + expect(nextNavId()).toBeGreaterThan(high); // and the next mint clears it + }); + it("notifyNav fires subscribers until they unsubscribe", () => { const cb = vi.fn(); const unsub = subscribeNav(cb);