fix(api): a gone row and a refused playlist action are said in the reader's language, and a failure is reported once

Deep-review round 3 of bug hunt 2026-09.

- get_or_404 / owned_or_404 answer a structured {code: "not_found", reason: <what>}
  (errors.notFound.* in HU+EN, generic fallback). The structured-404 fix of round 2 had gone into the
  ad-hoc `_own_playlist` (CLAUDE.md's "Not" pattern) while the shared helpers stayed string-only, so
  views, tags, pins, Plex playlists, invites and users still 404'd in English. `_own_playlist` now
  goes through owned_or_404; admin's hand-rolled user lookups use get_or_404.
- A move into a list that is gone names the TARGET (reason "move_target") — "this playlist doesn't
  exist any more" was read as the list on screen, which still exists.
- Every refusal in the playlists router is a {code: "playlist", reason} (playlists.errors.* HU+EN)
  instead of a bare English string shown verbatim in the Hungarian UI.
- `req` / `uploadReq` mark the errors they already put on screen (the error dialog, the connection
  notice, the 401 sign-out), and notifyActionError skips those. The watched toggles, the saved views
  and a playlist delete toasted a fault the dialog was already showing; Playlists' two hand-written
  status filters now go through the same check.
This commit is contained in:
2026-09-11 18:05:19 +02:00
parent 33fe895539
commit 1cfe6a7f59
22 changed files with 358 additions and 94 deletions
+13 -5
View File
@@ -17,12 +17,20 @@ from app.models import User
M = TypeVar("M", bound=Base)
def get_or_404(db: Session, model: type[M], id_: Any, *, detail: str = "Not found") -> M:
def not_found(what: str = "generic") -> HTTPException:
"""The 404 for a row that is gone — structured, so the bilingual UI says it in the reader's
language (`errors.notFound.<what>`, generic fallback). This is the 404 a user actually meets: the
thing was deleted in another tab or on another device. It used to be a bare English string
("Unknown view", "Not found"), which the Hungarian UI toasted verbatim."""
return HTTPException(status_code=404, detail={"code": "not_found", "reason": what})
def get_or_404(db: Session, model: type[M], id_: Any, *, what: str = "generic") -> M:
"""Fetch a row by primary key or raise 404. Replaces the `db.get(Model, id); if None: 404`
idiom repeated across routes."""
obj = db.get(model, id_)
if obj is None:
raise HTTPException(status_code=404, detail=detail)
raise not_found(what)
return obj
@@ -33,13 +41,13 @@ def owned_or_404(
user: User,
*,
owner_attr: str = "user_id",
detail: str = "Not found",
what: str = "generic",
) -> M:
"""Fetch a row by primary key that `user` owns, or raise 404. A row owned by someone
else 404s (NOT 403) on purpose — a 403 would confirm the id exists to a stranger.
Generalizes the near-identical per-route ownership helpers (e.g. plex's
`_own_playlist_or_404`, the saved-views owner check)."""
obj = get_or_404(db, model, id_, detail=detail)
obj = get_or_404(db, model, id_, what=what)
if getattr(obj, owner_attr) != user.id:
raise HTTPException(status_code=404, detail=detail)
raise not_found(what)
return obj
+10 -16
View File
@@ -173,7 +173,7 @@ def _activate_user_by_email(db: Session, email: str) -> None:
def _decide(db: Session, invite_id: int, admin: User, approved: bool) -> Invite:
inv = get_or_404(db, Invite, invite_id, detail="No such invite")
inv = get_or_404(db, Invite, invite_id, what="invite")
# Idempotency guard: only a still-pending invite can be decided. Without this a repeat/stale
# approve or deny re-runs its side effects — re-activating a user, or (now that deny emails)
# sending a duplicate decision mail, even flipping an already-approved invite to "denied" and
@@ -291,7 +291,7 @@ def send_invitation(
Only an APPROVED row can be invited: there is nothing to invite someone to while their request is
undecided, and inviting a denied address would contradict the decision."""
inv = get_or_404(db, Invite, invite_id, detail="No such invite")
inv = get_or_404(db, Invite, invite_id, what="invite")
if inv.status != "approved":
raise HTTPException(
status_code=409,
@@ -364,7 +364,7 @@ def delete_invite(
falls back to the env ALLOWED_EMAILS / ADMIN_EMAILS bootstrap lists once no invite exists, so for
an address on those the deletion leaves access intact (and an ADMIN_EMAILS address is still
promoted to admin on its next Google sign-in). The admin has to hear that, not guess it."""
inv = get_or_404(db, Invite, invite_id, detail="No such invite")
inv = get_or_404(db, Invite, invite_id, what="invite")
if _accounts_for(db, [inv.email]):
raise HTTPException(
status_code=409,
@@ -434,9 +434,7 @@ def set_user_role(
role = body.role
if role not in ("user", "admin"):
raise HTTPException(status_code=400, detail="role must be 'user' or 'admin'")
target = db.get(User, user_id)
if target is None:
raise HTTPException(status_code=404, detail="No such user")
target = get_or_404(db, User, user_id, what="user")
if target.is_demo:
raise HTTPException(status_code=400, detail="The demo account's role can't be changed.")
if target.id == admin.id:
@@ -491,9 +489,7 @@ def set_user_suspended(
un-suspend the user is emailed they can return (suspend is announced reactively, on a blocked
sign-in)."""
suspended = body.suspended
target = db.get(User, user_id)
if target is None:
raise HTTPException(status_code=404, detail="No such user")
target = get_or_404(db, User, user_id, what="user")
if target.is_demo:
raise HTTPException(status_code=400, detail="The demo account can't be suspended.")
if target.id == admin.id:
@@ -537,9 +533,7 @@ def admin_delete_user(
"""Admin-triggered account deletion — the same full erasure a user can perform on themselves
(cascade delete of all personal data + access-request cleanup + Google-grant revoke). Guards:
not the demo account, not yourself (use Settings → Account), and not the last admin."""
target = db.get(User, user_id)
if target is None:
raise HTTPException(status_code=404, detail="No such user")
target = get_or_404(db, User, user_id, what="user")
if target.is_demo:
raise HTTPException(status_code=400, detail="The demo account can't be deleted.")
if target.id == admin.id:
@@ -632,7 +626,7 @@ def user_access(
Its own endpoint rather than extra columns on /users: `is_allowed` and `can_activate` each run a
query, and the panel opens for ONE user at a time — folding them into the list would pay that cost
per row on every load to answer a question nobody asked yet."""
target = get_or_404(db, User, user_id, detail="No such user")
target = get_or_404(db, User, user_id, what="user")
return _serialize_user(target) | {
"is_allowed": is_allowed(db, target.email),
"can_activate": can_activate(db, target),
@@ -657,7 +651,7 @@ def create_set_password_link(
an instance with no mail — narrowing it to one of those would only make the admin's real case the
one it refuses. The demo account is the single exception: it is shared, so handing anyone a way to
change its password is handing them everyone else's session."""
target = get_or_404(db, User, user_id, detail="No such user")
target = get_or_404(db, User, user_id, what="user")
if target.is_demo:
raise HTTPException(
status_code=400, detail={"code": "setpw", "reason": "demo", "email": target.email}
@@ -693,7 +687,7 @@ def activate_user(
This endpoint changes nothing about what an unauthenticated caller can cause — the admin could
already do exactly this by re-entering the address in the whitelist box (`add_invite` →
`_activate_user_by_email`). What was missing was any way to know they had to."""
target = get_or_404(db, User, user_id, detail="No such user")
target = get_or_404(db, User, user_id, what="user")
if target.is_demo:
raise HTTPException(status_code=400, detail="The demo account can't be activated.")
if target.is_active:
@@ -732,7 +726,7 @@ def resend_user_verification(
Not rate-limited: an admin is not a threat model to enumerate against, and the deliberate silence
of the public endpoint is exactly what this exists to escape."""
target = get_or_404(db, User, user_id, detail="No such user")
target = get_or_404(db, User, user_id, what="user")
if target.is_demo or target.email_verified:
raise HTTPException(
status_code=409,
+1 -1
View File
@@ -706,7 +706,7 @@ def attach_tag(
_user_subscription(db, user, channel_id)
# Only the user's own tags may be attached as personal links (owned_or_404 → 404 if the tag
# is missing OR belongs to someone else, without leaking which).
owned_or_404(db, Tag, body.tag_id, user, detail="Unknown tag")
owned_or_404(db, Tag, body.tag_id, user, what="tag")
exists = db.execute(
select(ChannelTag).where(
ChannelTag.channel_id == channel_id,
+1 -1
View File
@@ -168,7 +168,7 @@ def remove_pin(
user: User = Depends(require_human),
db: Session = Depends(get_db),
) -> dict:
pin = owned_or_404(db, ComfortPin, pin_id, user)
pin = owned_or_404(db, ComfortPin, pin_id, user, what="pin")
db.delete(pin)
db.commit()
return {"deleted": pin_id}
+27 -34
View File
@@ -23,6 +23,7 @@ from app.sync.playlists import (
sync_user_playlists,
)
from app.ratelimit import RateLimiter
from app.routes._common import not_found, owned_or_404
from app.youtube.client import YouTubeClient, YouTubeError
router = APIRouter(prefix="/api/playlists", tags=["playlists"])
@@ -48,13 +49,13 @@ def _check_pull_cooldown(user: User) -> None:
def _own_playlist(db: Session, user: User, playlist_id: int) -> Playlist:
pl = db.get(Playlist, playlist_id)
if pl is None or pl.user_id != user.id:
# Structured, so the UI can say it in the reader's language: this 404 is exactly the one a
# user meets in practice (the list was deleted in another tab), and a bare "Unknown
# playlist" reached the Hungarian UI in English. One reason for "gone" and "not yours" alike.
raise HTTPException(status_code=404, detail={"code": "playlist", "reason": "not_found"})
return pl
return owned_or_404(db, Playlist, playlist_id, user, what="playlist")
def _refused(status: int, reason: str) -> HTTPException:
"""A playlist action the server refuses, as a reason the UI translates
(`playlists.errors.<reason>`) — a bare English detail reached the Hungarian UI verbatim."""
return HTTPException(status_code=status, detail={"code": "playlist", "reason": reason})
def _touch(pl: Playlist) -> None:
@@ -259,7 +260,7 @@ def create_playlist(
) -> dict:
name = body.name.strip()
if not name:
raise HTTPException(status_code=400, detail="name is required")
raise _refused(400, "name_required")
pl = Playlist(user_id=user.id, name=name, position=_next_playlist_position(db, user.id))
db.add(pl)
db.commit()
@@ -292,7 +293,7 @@ def add_watch_later(
"""Bookmark shortcut: add a video to the built-in Watch later playlist."""
video_id = body.video_id
if db.get(Video, video_id) is None:
raise HTTPException(status_code=404, detail="Unknown video")
raise not_found("video")
pl = _watch_later(db, user)
_add_item(db, pl, video_id, mark_dirty=False)
return {"saved": True, "playlist_id": pl.id}
@@ -320,10 +321,7 @@ def sync_youtube(
"""Pull the user's YouTube playlists into local mirrors now (read-only direction).
Also runs automatically on the scheduler; this is the manual 'sync now'."""
if not has_read_scope(user):
raise HTTPException(
status_code=403,
detail="Connect YouTube (read access) to sync your playlists.",
)
raise _refused(403, "youtube_read_scope")
_check_pull_cooldown(user)
with quota.attribute(user.id, quota.QuotaAction.PLAYLISTS_PULL):
return sync_user_playlists(db, user)
@@ -340,18 +338,16 @@ def revert_youtube(
once the in-session undo history is gone."""
pl = _own_playlist(db, user, playlist_id)
if not pl.yt_playlist_id:
raise HTTPException(status_code=400, detail="This playlist isn't linked to YouTube.")
raise _refused(400, "not_on_youtube")
if not has_read_scope(user):
raise HTTPException(
status_code=403, detail="Connect YouTube (read access) to sync your playlists."
)
raise _refused(403, "youtube_read_scope")
_check_pull_cooldown(user)
try:
with quota.attribute(user.id, quota.QuotaAction.PLAYLISTS_PULL):
return repull_playlist(db, user, pl)
except YouTubeError:
db.rollback()
raise HTTPException(status_code=422, detail="Couldn't refresh from YouTube.")
raise _refused(422, "youtube_failed")
def _pushable(db: Session, user: User, playlist_id: int) -> Playlist:
@@ -359,9 +355,7 @@ def _pushable(db: Session, user: User, playlist_id: int) -> Playlist:
Watch later and read-only YouTube mirrors are never pushable."""
pl = _own_playlist(db, user, playlist_id)
if pl.kind == "watch_later":
raise HTTPException(
status_code=400, detail="The Watch later list can't be sent to YouTube."
)
raise _refused(400, "watch_later_not_pushable")
return pl
@@ -379,7 +373,7 @@ def push_plan(
with quota.attribute(user.id, quota.QuotaAction.PLAYLISTS_PUSH):
plan = plan_merge(db, user, pl)
except YouTubeError:
raise HTTPException(status_code=422, detail="Couldn't read the playlist on YouTube.")
raise _refused(422, "youtube_failed")
plan["remaining_today"] = quota.remaining_today(db)
plan["affordable"] = plan["units_estimate"] <= plan["remaining_today"]
return plan
@@ -409,14 +403,11 @@ def push(
)
plan = plan_merge(db, user, pl, live=live)
if plan["units_estimate"] > quota.remaining_today(db):
raise HTTPException(
status_code=429,
detail="Not enough YouTube quota left today for this sync.",
)
raise _refused(429, "quota_exceeded")
return merge_playlist(db, user, pl, live=live)
except YouTubeError:
db.rollback()
raise HTTPException(status_code=422, detail="YouTube playlist sync failed.")
raise _refused(422, "youtube_failed")
@router.get("/{playlist_id}")
@@ -461,7 +452,7 @@ def rename_playlist(
if "name" in body.model_fields_set:
name = (body.name or "").strip()
if not name:
raise HTTPException(status_code=400, detail="name cannot be empty")
raise _refused(400, "name_required")
if name != pl.name:
pl.name = name
recompute_dirty(db, pl)
@@ -496,18 +487,18 @@ def delete_playlist(
) -> dict:
pl = _own_playlist(db, user, playlist_id)
if pl.kind == "watch_later":
raise HTTPException(status_code=400, detail="The Watch later list can't be deleted.")
raise _refused(400, "watch_later_undeletable")
if on_youtube:
if not pl.yt_playlist_id:
raise HTTPException(status_code=400, detail="This playlist isn't on YouTube.")
raise _refused(400, "not_on_youtube")
if not has_write_scope(user):
raise HTTPException(status_code=403, detail="Enable YouTube editing in Settings first.")
raise _refused(403, "youtube_editing_off")
try:
with quota.attribute(user.id, quota.QuotaAction.PLAYLISTS_DELETE):
with YouTubeClient(db, user) as yt:
yt.delete_playlist(pl.yt_playlist_id)
except YouTubeError:
raise HTTPException(status_code=422, detail="YouTube delete failed.")
raise _refused(422, "youtube_failed")
db.delete(pl) # items cascade
db.commit()
return {"deleted": playlist_id}
@@ -523,7 +514,7 @@ def add_item(
pl = _own_playlist(db, user, playlist_id)
video_id = body.video_id
if db.get(Video, video_id) is None:
raise HTTPException(status_code=404, detail="Unknown video")
raise not_found("video")
_add_item(db, pl, video_id, mark_dirty=True)
return {"playlist_id": pl.id, "video_id": video_id}
@@ -560,7 +551,9 @@ def move_item(
case. Idempotent: if the video is already in the target, the source row is simply dropped
(moving into a list that already has it is a removal, not a duplicate)."""
src = _own_playlist(db, user, playlist_id)
dst = _own_playlist(db, user, body.target_playlist_id)
# Its own reason: "this playlist doesn't exist any more" about a TARGET that is gone was read as
# the list on screen — which is still there.
dst = owned_or_404(db, Playlist, body.target_playlist_id, user, what="move_target")
if src.id == dst.id:
raise HTTPException(
status_code=400, detail={"code": "playlist_move", "reason": "same_playlist"}
+1 -1
View File
@@ -25,7 +25,7 @@ router = APIRouter(prefix="/api/plex", tags=["plex"])
def _own_playlist_or_404(db: Session, pid: int, user: User) -> PlexPlaylist:
return owned_or_404(db, PlexPlaylist, pid, user, detail="Playlist not found")
return owned_or_404(db, PlexPlaylist, pid, user, what="playlist")
def _playlist_card(pl: PlexPlaylist, count: int, thumb_rk: str | None) -> dict:
+1 -1
View File
@@ -91,7 +91,7 @@ def create_view(
def _own_view(db: Session, user: User, view_id: int) -> SavedView:
return owned_or_404(db, SavedView, view_id, user, detail="Unknown view")
return owned_or_404(db, SavedView, view_id, user, what="view")
class UpdateViewIn(BaseModel):
+1 -1
View File
@@ -80,7 +80,7 @@ def create_tag(
def _own_tag(db: Session, user: User, tag_id: int) -> Tag:
# System tags (user_id NULL) are not user-editable → owned_or_404 404s them too.
return owned_or_404(db, Tag, tag_id, user, detail="Unknown tag")
return owned_or_404(db, Tag, tag_id, user, what="tag")
class TagUpdateIn(BaseModel):
+20
View File
@@ -0,0 +1,20 @@
"""A row that is gone answers with a reason the bilingual UI can translate (deep-review round 3).
The 404 a user actually meets is the one for something deleted in another tab or on another device.
`owned_or_404` / `get_or_404` sent it as a bare English string ("Unknown view", "Not found"), and the
Hungarian UI toasted it verbatim. The structured-404 fix had gone into one ad-hoc helper instead of
the shared one, so every other owner check kept the English. Needs the DB.
"""
def test_a_gone_view_tag_or_pin_says_what_is_gone(db, make_user, as_user):
client = as_user(make_user())
for method, url, what in [
("patch", "/api/saved-views/999999", "view"),
("delete", "/api/saved-views/999999", "view"),
("delete", "/api/tags/999999", "tag"),
("delete", "/api/discover/pins/999999", "pin"),
]:
r = getattr(client, method)(url, **({"json": {"name": "x"}} if method == "patch" else {}))
assert r.status_code == 404, (url, r.text)
assert r.json()["detail"] == {"code": "not_found", "reason": what}, url
+39 -1
View File
@@ -139,11 +139,49 @@ def test_a_list_that_is_not_yours_answers_with_a_reason_the_ui_can_translate(
r = as_user(user).delete(f"/api/playlists/{theirs.id}/items/v1")
assert r.status_code == 404
assert r.json()["detail"] == {"code": "playlist", "reason": "not_found"}
assert r.json()["detail"] == {"code": "not_found", "reason": "playlist"}
db.expire_all()
assert _ids(db, theirs.id) == ["v1"]
def test_a_move_into_a_list_that_is_gone_names_the_target_not_the_list_on_screen(
db, make_user, as_user
):
# Both lists 404'd with the same reason, which the UI reads as "THIS playlist doesn't exist any
# more" — said about the source the reader is looking at, which is still there.
user = make_user()
_seed_videos(db, ["v1"])
src = _playlist(db, user, "Mine", ["v1"])
gone = _playlist(db, user, "Gone", [])
gone_id = gone.id
db.delete(gone)
db.commit()
r = _move(as_user(user), src.id, "v1", gone_id)
assert r.status_code == 404
assert r.json()["detail"] == {"code": "not_found", "reason": "move_target"}
db.expire_all()
assert _ids(db, src.id) == ["v1"]
def test_a_refused_playlist_action_answers_with_a_reason_the_ui_can_translate(
db, make_user, as_user
):
# The rest of the router's refusals were bare English too ("The Watch later list can't be
# deleted.", "YouTube delete failed.", ...), shown verbatim under the Hungarian UI.
user = make_user()
client = as_user(user)
wl = client.post("/api/playlists/watch-later", json={"video_id": "nope"})
assert wl.status_code == 404
assert wl.json()["detail"] == {"code": "not_found", "reason": "video"}
_seed_videos(db, ["v1"])
saved = client.post("/api/playlists/watch-later", json={"video_id": "v1"})
wl_id = saved.json()["playlist_id"]
r = client.delete(f"/api/playlists/{wl_id}")
assert r.status_code == 400
assert r.json()["detail"] == {"code": "playlist", "reason": "watch_later_undeletable"}
def test_move_marks_both_linked_sides_dirty(db, make_user, as_user):
"""A move edits two YouTube mirrors at once — the source lost an item, the target gained one.
Marking only the source is the asymmetric fix this repo keeps re-learning."""
+8 -12
View File
@@ -40,7 +40,7 @@ import {
X,
Youtube,
} from "lucide-react";
import { api, HttpError, type Playlist, type PlaylistDetail, type Video } from "../lib/api";
import { api, type Playlist, type PlaylistDetail, type Video } from "../lib/api";
import { notify } from "../lib/notifications";
import { notifyActionError } from "../lib/actionErrors";
import { notifyYouTubeActionError } from "../lib/youtubeErrors";
@@ -576,12 +576,10 @@ export default function Playlists() {
refreshAll(listId);
return true;
} catch (e) {
// `req` raises the global dialog for 4xx-validation and 5xx, but leaves 403/404 to the caller
// (the list was deleted or de-owned meanwhile) — so say it here, as moveItemTo does, or a
// confirmed removal fails in silence (inside the full-screen player there is not even a row
// snapping back to show it).
if (e instanceof HttpError && (e.status === 404 || e.status === 403))
notifyActionError(e, t("playlists.removeFailed"));
// Said here, as moveItemTo does, or a confirmed removal fails in silence (inside the
// full-screen player there is not even a row snapping back to show it) — unless `req` already
// said it (the error dialog, the connection notice), which `notifyActionError` checks.
notifyActionError(e, t("playlists.removeFailed"));
refreshAll(listId); // …and resync, which restores any row the optimistic removal took out
return false;
}
@@ -613,11 +611,9 @@ export default function Playlists() {
// every move failure a user can actually reach is a 404 (the 400 `same_playlist` case is already
// impossible — the submenu never offers the current list). So the row used to snap back with no
// explanation and the shipped moveErrors strings were unreachable (E10 review round 1).
// ONLY the statuses `req` deliberately leaves to the caller. It already opens the blocking
// error dialog for 400/409/422 and 5xx and raises the connectivity notice for a bad gateway,
// so toasting unconditionally reported a transport fault twice (E10 review round 2).
if (e instanceof HttpError && (e.status === 404 || e.status === 403))
notifyActionError(e, t("playlists.moveFailed"));
// A fault `req` already reported (the error dialog, the connection notice) is not toasted
// again — `notifyActionError` checks (E10 review round 2 found it reported twice).
notifyActionError(e, t("playlists.moveFailed"));
}
// BOTH sides changed, and so did this video's membership. `qk.playlist(selectedId)` is an exact
// key, so it cannot cover the target — invalidate the whole `["playlist"]` prefix, exactly as
@@ -0,0 +1,69 @@
// @vitest-environment jsdom
//
// A failed playlist delete is reported once (deep-review round 3 of bug hunt 2026-09). `req` already
// opens the blocking error dialog for a 422 (a YouTube-side delete failure) and a 5xx; the delete
// then toasted the same failure as well — in English, before the router's errors were structured.
import { describe, it, expect, vi, afterEach } from "vitest";
import { renderHook, act, cleanup } from "@testing-library/react";
import { QueryClient, QueryClientProvider } from "@tanstack/react-query";
import type { ReactNode } from "react";
const h = vi.hoisted(() => ({ notify: vi.fn(), reportError: vi.fn() }));
vi.mock("react-i18next", async (importOriginal) => {
const actual = await importOriginal<typeof import("react-i18next")>();
return { ...actual, useTranslation: () => ({ t: (key: string) => key }) };
});
vi.mock("../lib/notifications", async (importOriginal) => ({
...(await importOriginal<typeof import("../lib/notifications")>()),
notify: (...a: unknown[]) => h.notify(...a),
}));
vi.mock("../lib/errorDialog", () => ({ reportError: (...a: unknown[]) => h.reportError(...a) }));
vi.mock("./ConfirmProvider", () => ({ useChoice: () => async () => "both" }));
vi.mock("./PlaylistsProvider", () => ({
usePlaylists: () => ({ selectedId: null }),
usePlaylistsActions: () => ({ setSelectedId: () => {} }),
}));
vi.mock("../lib/useMe", () => ({ useMe: () => ({ can_write: true }) }));
import { usePlaylistActions } from "./usePlaylistActions";
import { HttpError, api } from "../lib/api";
afterEach(() => {
cleanup();
vi.restoreAllMocks();
h.notify.mockReset();
h.reportError.mockReset();
});
function mount() {
const qc = new QueryClient();
const wrapper = ({ children }: { children: ReactNode }) => (
<QueryClientProvider client={qc}>{children}</QueryClientProvider>
);
return renderHook(() => usePlaylistActions(), { wrapper });
}
const linked = { id: 4, name: "Mix", kind: "user", yt_playlist_id: "PL1" } as const;
describe("usePlaylistActions — deleting a list", () => {
it("does not toast a failure the request layer already put on screen", async () => {
vi.spyOn(globalThis, "fetch").mockResolvedValue(
new Response(JSON.stringify({ detail: { code: "playlist", reason: "youtube_failed" } }), {
status: 422,
headers: { "content-type": "application/json" },
}),
);
const { result } = mount();
await act(() => result.current.remove(linked));
expect(h.reportError).toHaveBeenCalledTimes(1);
expect(h.notify).not.toHaveBeenCalled();
});
it("does say one it was left to handle", async () => {
vi.spyOn(api, "deletePlaylist").mockRejectedValue(new HttpError(404, "gone"));
const { result } = mount();
await act(() => result.current.remove(linked));
expect(h.notify).toHaveBeenCalledWith(expect.objectContaining({ message: "gone" }));
});
});
+10 -8
View File
@@ -1,7 +1,7 @@
import { useQueryClient } from "@tanstack/react-query";
import { useTranslation } from "react-i18next";
import { qk } from "../lib/queryKeys";
import { api, HttpError, type Playlist } from "../lib/api";
import { alreadyReported, api, HttpError, type Playlist } from "../lib/api";
import { notify } from "../lib/notifications";
import { notifyYouTubeActionError } from "../lib/youtubeErrors";
import { playlistName } from "../lib/playlistName";
@@ -70,13 +70,15 @@ export function usePlaylistActions() {
await api.deletePlaylist(pl.id, onYoutube);
} catch (err) {
// Prefer the server's specific reason (e.g. a local vs YouTube-side failure) over a blanket
// "couldn't delete on YouTube" that misleads when the failure wasn't the YouTube side.
notify({
level: "warning",
message:
(err instanceof HttpError && err.detail) ||
(onYoutube ? t("playlists.deleteYtFailed") : t("playlists.deleteFailed")),
});
// "couldn't delete on YouTube" that misleads when the failure wasn't the YouTube side — unless
// `req` already put it on screen (the error dialog, the connection notice): not twice.
if (!alreadyReported(err))
notify({
level: "warning",
message:
(err instanceof HttpError && err.detail) ||
(onYoutube ? t("playlists.deleteYtFailed") : t("playlists.deleteFailed")),
});
}
qc.removeQueries({ queryKey: qk.playlist(pl.id) });
qc.invalidateQueries({ queryKey: qk.playlists() });
+11
View File
@@ -4,6 +4,17 @@
"validation": "Some of the values you entered weren't valid. Please check them and try again.",
"server": "Server error",
"syncCooldown": "You've synced a few times just now — give it a few minutes. The YouTube budget is shared with everyone on this instance, and background syncing keeps running either way.",
"notFound": {
"generic": "This item doesn't exist any more.",
"view": "This view doesn't exist any more.",
"tag": "This tag doesn't exist any more.",
"pin": "This isn't on your comfort shelf any more.",
"playlist": "This playlist doesn't exist any more.",
"move_target": "The playlist you're moving it to doesn't exist any more.",
"video": "This video isn't in the library any more.",
"invite": "This invite doesn't exist any more.",
"user": "This user doesn't exist any more."
},
"ok": "OK",
"offline": {
"title": "Connection lost",
+8 -1
View File
@@ -78,7 +78,14 @@
"moveFailed": "Couldn't move the video.",
"removeFailed": "Couldn't remove the video.",
"errors": {
"not_found": "This playlist doesn't exist any more."
"name_required": "Give the playlist a name.",
"youtube_read_scope": "Connect YouTube (read access) to sync your playlists.",
"not_on_youtube": "This playlist isn't on YouTube.",
"youtube_failed": "YouTube didn't accept that just now. Please try again.",
"watch_later_not_pushable": "The Watch later list can't be sent to YouTube.",
"watch_later_undeletable": "The Watch later list can't be deleted.",
"youtube_editing_off": "Enable YouTube editing in Settings first.",
"quota_exceeded": "Not enough YouTube quota left today for this sync."
},
"moveErrors": {
"not_in_playlist": "That video isn't in this playlist any more — it changed somewhere else.",
+11
View File
@@ -4,6 +4,17 @@
"validation": "Néhány megadott érték érvénytelen volt. Ellenőrizd, majd próbáld újra.",
"server": "Szerverhiba",
"syncCooldown": "Az imént többször is szinkronizáltál — várj pár percet. A YouTube-keret közös az instance összes felhasználójával, a háttérszinkron pedig így is fut tovább.",
"notFound": {
"generic": "Ez az elem már nem létezik.",
"view": "Ez a nézet már nem létezik.",
"tag": "Ez a címke már nem létezik.",
"pin": "Ez már nincs a komfort polcodon.",
"playlist": "Ez a lejátszási lista már nem létezik.",
"move_target": "A lejátszási lista, ahová áthelyeznéd, már nem létezik.",
"video": "Ez a videó már nincs a könyvtárban.",
"invite": "Ez a meghívó már nem létezik.",
"user": "Ez a felhasználó már nem létezik."
},
"ok": "OK",
"offline": {
"title": "Megszakadt a kapcsolat",
+8 -1
View File
@@ -78,7 +78,14 @@
"moveFailed": "Nem sikerült áthelyezni a videót.",
"removeFailed": "Nem sikerült eltávolítani a videót.",
"errors": {
"not_found": "Ez a lejátszási lista már nem létezik."
"name_required": "Adj nevet a lejátszási listának.",
"youtube_read_scope": "A listák szinkronizálásához kapcsold össze a YouTube-ot (olvasási hozzáférés).",
"not_on_youtube": "Ez a lejátszási lista nincs fent a YouTube-on.",
"youtube_failed": "A YouTube most nem fogadta el. Próbáld újra.",
"watch_later_not_pushable": "A „Megnézem később” listát nem lehet a YouTube-ra küldeni.",
"watch_later_undeletable": "A „Megnézem később” lista nem törölhető.",
"youtube_editing_off": "Előbb engedélyezd a YouTube-szerkesztést a Beállításokban.",
"quota_exceeded": "Mára nem maradt elég YouTube-keret ehhez a szinkronhoz."
},
"moveErrors": {
"not_in_playlist": "Ez a videó már nincs ebben a listában — időközben máshol megváltozott.",
+24 -2
View File
@@ -6,9 +6,9 @@ import { describe, it, expect, vi, beforeEach } from "vitest";
// Spy on the toast sink so we can assert exactly what each error maps to.
vi.mock("./notifications", () => ({ notify: vi.fn(() => 0) }));
import { HttpError } from "./api";
import { HttpError, api } from "./api";
import { notify } from "./notifications";
import { notifyCollectionEditError } from "./actionErrors";
import { notifyActionError, notifyCollectionEditError } from "./actionErrors";
const notifyMock = vi.mocked(notify);
beforeEach(() => notifyMock.mockClear());
@@ -29,3 +29,25 @@ describe("notifyCollectionEditError", () => {
expect(notifyMock).toHaveBeenCalledWith({ level: "error", message: "EDIT_FAILED" });
});
});
describe("notifyActionError", () => {
it("does not toast a fault the request layer already put on screen", async () => {
// A 500 opens the blocking error dialog inside `req`; the watched toggles, the saved views and a
// playlist delete then toasted the same failure as well.
vi.spyOn(globalThis, "fetch").mockResolvedValue(
new Response(JSON.stringify({ detail: "boom" }), {
status: 500,
headers: { "content-type": "application/json" },
}),
);
const err = await api.plexSetState("1", "watched").catch((e: unknown) => e);
notifyMock.mockClear();
notifyActionError(err, "FAILED");
expect(notifyMock).not.toHaveBeenCalled();
});
it("does toast one the caller was left to handle", () => {
notifyActionError(new HttpError(404, "gone"), "FAILED");
expect(notifyMock).toHaveBeenCalledWith({ level: "error", message: "gone" });
});
});
+7 -1
View File
@@ -1,4 +1,6 @@
import { HttpError } from "./api";
// From the core, not the `./api` barrel: these two are pure (no request), and a test that mocks the
// barrel wholesale to stub its requests must not take the error path's own plumbing with it.
import { alreadyReported, HttpError } from "./api/core";
import { notify } from "./notifications";
/**
@@ -9,6 +11,10 @@ import { notify } from "./notifications";
* for its non-403 case.
*/
export function notifyActionError(err: unknown, fallbackMessage: string): void {
// Already on screen (`req` opened the error dialog, or raised the connection notice): a toast on
// top reported one fault twice. Playlists filtered this by status by hand; the watched toggles,
// the saved views and a playlist delete did not.
if (alreadyReported(err)) return;
const detail = err instanceof HttpError ? err.detail : null;
notify({ level: "error", message: detail || fallbackMessage });
}
+2
View File
@@ -7,6 +7,7 @@
// A new endpoint therefore has nowhere to land except a domain module — the interleave that
// motivated R7.5 cannot re-grow here. (An automated guard for that invariant lands in S3.)
import {
alreadyReported,
getActiveAccount,
HttpError,
setActiveAccount,
@@ -16,6 +17,7 @@ import {
// Re-export the shared HTTP machinery so consumers keep importing these from "./api" unchanged.
export {
alreadyReported,
HttpError,
getActiveAccount,
setActiveAccount,
+56 -3
View File
@@ -21,7 +21,7 @@ vi.mock("../activeAccount", () => ({
notifyActiveAccountChange: vi.fn(),
}));
import { req, uploadReq, setUnauthorizedHandler, HttpError } from "./core";
import { req, uploadReq, setUnauthorizedHandler, HttpError, alreadyReported } from "./core";
// A fake XHR whose lifecycle the test drives: capture the instance, set status/responseText, then
// invoke onload/onerror/abort by hand.
@@ -123,15 +123,68 @@ describe("req — a structured refusal reads in the user's language", () => {
// The 404 every playlist route answers when the list was deleted in another tab. It used to be a
// bare "Unknown playlist" string, shown verbatim under the Hungarian UI.
vi.spyOn(globalThis, "fetch").mockResolvedValue(
new Response(JSON.stringify({ detail: { code: "playlist", reason: "not_found" } }), {
new Response(JSON.stringify({ detail: { code: "not_found", reason: "playlist" } }), {
status: 404,
headers: { "content-type": "application/json" },
}),
);
await expect(req("/api/playlists/7/items/v1", { method: "DELETE" })).rejects.toMatchObject({
status: 404,
detail: "playlists.errors.not_found",
detail: "errors.notFound.playlist",
});
expect(reportError).not.toHaveBeenCalled(); // a 404 is the caller's to report
});
});
describe("req — what it has already put in front of the user", () => {
// A caller that toasts a failed action on top of the dialog `req` already opened reported one fault
// twice. `alreadyReported` is how `notifyActionError` tells the two apart.
const answer = (status: number) =>
vi.spyOn(globalThis, "fetch").mockResolvedValue(
new Response(JSON.stringify({ detail: "x" }), {
status,
headers: { "content-type": "application/json" },
}),
);
it.each([500, 422, 409, 400])("marks a %s it raised the error dialog for", async (status) => {
answer(status);
const err = await req("/api/x", { method: "POST" }).catch((e: unknown) => e);
expect(reportError).toHaveBeenCalled();
expect(alreadyReported(err)).toBe(true);
});
it.each([404, 403, 429])("leaves a %s to the caller", async (status) => {
answer(status);
const err = await req("/api/x", { method: "POST" }).catch((e: unknown) => e);
expect(err).toBeInstanceOf(HttpError);
expect(alreadyReported(err)).toBe(false);
});
it("marks a 401 the app shell handles (it signs the tab out)", async () => {
answer(401);
const err = await req("/api/x", { method: "POST" }).catch((e: unknown) => e);
expect(alreadyReported(err)).toBe(true);
});
it("marks a request that never reached the server (the connection notice says it)", async () => {
vi.spyOn(globalThis, "fetch").mockRejectedValue(new TypeError("Failed to fetch"));
const err = await req("/api/x", { method: "POST" }).catch((e: unknown) => e);
expect(notify).toHaveBeenCalled();
expect(alreadyReported(err)).toBe(true);
});
it("marks what the upload path reported the same way", async () => {
const dead = uploadReq("/u", form());
FakeXHR.last.onerror!();
expect(alreadyReported(await dead.catch((e: unknown) => e))).toBe(true);
const refused = uploadReq("/u", form());
FakeXHR.last.status = 500;
FakeXHR.last.onload!();
expect(alreadyReported(await refused.catch((e: unknown) => e))).toBe(true);
const gone = uploadReq("/u", form());
FakeXHR.last.status = 404;
FakeXHR.last.onload!();
expect(alreadyReported(await gone.catch((e: unknown) => e))).toBe(false);
});
});
+30 -5
View File
@@ -14,6 +14,14 @@ import { activeAccountId, ACTIVE_ACCOUNT_KEY, notifyActiveAccountChange } from "
* field nobody ever filled (E9.5 S6 → S7). */
export const uiLang = (): string => i18n.language.split("-")[0] || "en";
// Errors `req` has already put in front of the user — the blocking error dialog, or the connection
// notice. A caller that toasts a failed action as well reported the same fault twice (a modal AND a
// toast); `notifyActionError` asks this instead of each caller filtering statuses by hand.
const reported = new WeakSet<object>();
export function alreadyReported(err: unknown): boolean {
return typeof err === "object" && err !== null && reported.has(err);
}
export class HttpError extends Error {
status: number;
detail?: string; // server-provided reason (FastAPI's `detail`), when present
@@ -82,8 +90,13 @@ function localizeDetail(d: unknown): string {
// unknown reason falls back to the plain failure rather than printing a key.
if (o?.code === "playlist_move")
return t(`playlists.moveErrors.${o.reason}`, t("playlists.moveFailed"));
// Any playlist route whose list is gone (deleted in another tab) — or was never the caller's.
// A playlist action the server refused (Watch later can't be deleted, YouTube said no, …).
if (o?.code === "playlist") return t(`playlists.errors.${o.reason}`, t("errors.generic"));
// Any row that is gone — deleted in another tab or device, or never the caller's (a stranger's
// row 404s the same way). `reason` names WHAT is gone; one this build doesn't know says it
// generically rather than printing a key.
if (o?.code === "not_found")
return t(`errors.notFound.${o.reason}`, t("errors.notFound.generic"));
// SEC-14: a manual sync trigger on cooldown. Not an error the user caused so much as a pace
// limit, and the copy says so.
if (o?.code === "sync_cooldown") return t("errors.syncCooldown");
@@ -223,6 +236,7 @@ export async function req<T = unknown>(
continue;
}
markConnectivityLost();
if (typeof e === "object" && e !== null) reported.add(e);
throw e;
}
@@ -257,20 +271,26 @@ export async function req<T = unknown>(
// self-clearing toast rather than a blocking modal the user must acknowledge.
// A genuine 500 (real fault, carries a JSON detail) still gets the modal, as do
// 400/409/422 (validation/conflict). 401/403/404 are caller-handled control flow.
const err = new HttpError(r.status, detail, detailData);
if (RETRIABLE_GATEWAY.has(r.status)) {
markConnectivityLost();
reported.add(err);
} else if (r.status === 401) {
// Session ended server-side (e.g. the account was suspended or deleted mid-visit). Let the
// app shell decide what to do (drop to the login page if we were signed in). Not a modal.
// app shell decide what to do (drop to the login page if we were signed in). Not a modal —
// but handled all the same, so not the caller's to toast either.
onUnauthorized?.();
reported.add(err);
} else if (cfg.quiet) {
/* caller handles the error inline — no global modal */
} else if (r.status >= 500) {
reportError(detail || `${i18n.t("errors.server")} (${r.status})`);
reported.add(err);
} else if (r.status === 400 || r.status === 409 || r.status === 422) {
reportError(detail);
reported.add(err);
}
throw new HttpError(r.status, detail, detailData);
throw err;
}
// The single audited cast in this layer: the server contract is asserted here, once, so
// every typed api.* method (T comes from its declared return type) is checked from here up.
@@ -326,16 +346,21 @@ export function uploadReq<T = unknown>(
} catch {
/* no JSON body */
}
const err = new HttpError(xhr.status, detail, detailData);
if (xhr.status === 401) onUnauthorized?.();
else if (xhr.status >= 500)
reportError(detail || `${i18n.t("errors.server")} (${xhr.status})`);
else if (xhr.status === 400 || xhr.status === 409 || xhr.status === 422) reportError(detail);
reject(new HttpError(xhr.status, detail, detailData));
if (xhr.status === 401 || xhr.status >= 500 || [400, 409, 422].includes(xhr.status))
reported.add(err); // as in `req`: already on screen, not the caller's to toast again
reject(err);
};
// A network-level failure (server unreachable) — same soft "connection lost" status as `req`.
xhr.onerror = () => {
markConnectivityLost();
reject(new HttpError(0, i18n.t("errors.offline.body")));
const err = new HttpError(0, i18n.t("errors.offline.body"));
reported.add(err);
reject(err);
};
xhr.onabort = () => reject(new HttpError(0, "aborted"));
xhr.send(form);