fix(plex): round-2 deep-review follow-ups on the review fixes
- 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).
This commit is contained in:
@@ -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):
|
||||
|
||||
@@ -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):
|
||||
|
||||
@@ -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 ? (
|
||||
<p className="mt-1 mb-2 text-xs text-danger">{t("plex.collection.nameTaken")}</p>
|
||||
<p className="mt-1 mb-2 text-xs text-muted">{t("plex.collection.nameMaybeTaken")}</p>
|
||||
) : (
|
||||
<p className="mt-1 mb-2 text-xs text-muted">{t("plex.collection.newHint")}</p>
|
||||
)}
|
||||
|
||||
@@ -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",
|
||||
|
||||
@@ -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",
|
||||
|
||||
@@ -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);
|
||||
|
||||
Reference in New Issue
Block a user