From 1cfe6a7f5953a21ca3717c09db73dd8a8fb30814 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Fri, 11 Sep 2026 18:05:19 +0200 Subject: [PATCH] fix(api): a gone row and a refused playlist action are said in the reader's language, and a failure is reported once MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deep-review round 3 of bug hunt 2026-09. - get_or_404 / owned_or_404 answer a structured {code: "not_found", reason: } (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. --- backend/app/routes/_common.py | 18 +++-- backend/app/routes/admin.py | 26 +++---- backend/app/routes/channels.py | 2 +- backend/app/routes/discover.py | 2 +- backend/app/routes/playlists.py | 61 ++++++++-------- backend/app/routes/plex/playlists.py | 2 +- backend/app/routes/saved_views.py | 2 +- backend/app/routes/tags.py | 2 +- backend/tests/test_not_found_detail.py | 20 ++++++ backend/tests/test_playlist_move.py | 40 ++++++++++- frontend/src/components/Playlists.tsx | 20 +++--- .../usePlaylistActions.dom.test.tsx | 69 +++++++++++++++++++ frontend/src/components/usePlaylistActions.ts | 18 ++--- frontend/src/i18n/locales/en/errors.json | 11 +++ frontend/src/i18n/locales/en/playlists.json | 9 ++- frontend/src/i18n/locales/hu/errors.json | 11 +++ frontend/src/i18n/locales/hu/playlists.json | 9 ++- frontend/src/lib/actionErrors.test.ts | 26 ++++++- frontend/src/lib/actionErrors.ts | 8 ++- frontend/src/lib/api.ts | 2 + frontend/src/lib/api/core.test.ts | 59 +++++++++++++++- frontend/src/lib/api/core.ts | 35 ++++++++-- 22 files changed, 358 insertions(+), 94 deletions(-) create mode 100644 backend/tests/test_not_found_detail.py create mode 100644 frontend/src/components/usePlaylistActions.dom.test.tsx diff --git a/backend/app/routes/_common.py b/backend/app/routes/_common.py index 32590e7b..7223ce93 100644 --- a/backend/app/routes/_common.py +++ b/backend/app/routes/_common.py @@ -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.`, 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 diff --git a/backend/app/routes/admin.py b/backend/app/routes/admin.py index b9b3a04f..86ad5ade 100644 --- a/backend/app/routes/admin.py +++ b/backend/app/routes/admin.py @@ -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, diff --git a/backend/app/routes/channels.py b/backend/app/routes/channels.py index d848d6cd..f2c28603 100644 --- a/backend/app/routes/channels.py +++ b/backend/app/routes/channels.py @@ -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, diff --git a/backend/app/routes/discover.py b/backend/app/routes/discover.py index f029f0e2..e7447c09 100644 --- a/backend/app/routes/discover.py +++ b/backend/app/routes/discover.py @@ -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} diff --git a/backend/app/routes/playlists.py b/backend/app/routes/playlists.py index 1051ddcb..a7083c95 100644 --- a/backend/app/routes/playlists.py +++ b/backend/app/routes/playlists.py @@ -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.`) — 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"} diff --git a/backend/app/routes/plex/playlists.py b/backend/app/routes/plex/playlists.py index 81739a34..022af3ae 100644 --- a/backend/app/routes/plex/playlists.py +++ b/backend/app/routes/plex/playlists.py @@ -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: diff --git a/backend/app/routes/saved_views.py b/backend/app/routes/saved_views.py index 4ac5b835..e2d6f68e 100644 --- a/backend/app/routes/saved_views.py +++ b/backend/app/routes/saved_views.py @@ -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): diff --git a/backend/app/routes/tags.py b/backend/app/routes/tags.py index 0000cb7c..d5b8c8d8 100644 --- a/backend/app/routes/tags.py +++ b/backend/app/routes/tags.py @@ -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): diff --git a/backend/tests/test_not_found_detail.py b/backend/tests/test_not_found_detail.py new file mode 100644 index 00000000..c5eb78a2 --- /dev/null +++ b/backend/tests/test_not_found_detail.py @@ -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 diff --git a/backend/tests/test_playlist_move.py b/backend/tests/test_playlist_move.py index 027f0606..9a936d32 100644 --- a/backend/tests/test_playlist_move.py +++ b/backend/tests/test_playlist_move.py @@ -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.""" diff --git a/frontend/src/components/Playlists.tsx b/frontend/src/components/Playlists.tsx index 0c6641ab..b006635a 100644 --- a/frontend/src/components/Playlists.tsx +++ b/frontend/src/components/Playlists.tsx @@ -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 diff --git a/frontend/src/components/usePlaylistActions.dom.test.tsx b/frontend/src/components/usePlaylistActions.dom.test.tsx new file mode 100644 index 00000000..32f43c3c --- /dev/null +++ b/frontend/src/components/usePlaylistActions.dom.test.tsx @@ -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(); + return { ...actual, useTranslation: () => ({ t: (key: string) => key }) }; +}); +vi.mock("../lib/notifications", async (importOriginal) => ({ + ...(await importOriginal()), + 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 }) => ( + {children} + ); + 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" })); + }); +}); diff --git a/frontend/src/components/usePlaylistActions.ts b/frontend/src/components/usePlaylistActions.ts index 4b4a64d4..541f376e 100644 --- a/frontend/src/components/usePlaylistActions.ts +++ b/frontend/src/components/usePlaylistActions.ts @@ -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() }); diff --git a/frontend/src/i18n/locales/en/errors.json b/frontend/src/i18n/locales/en/errors.json index d595a028..2d1b8388 100644 --- a/frontend/src/i18n/locales/en/errors.json +++ b/frontend/src/i18n/locales/en/errors.json @@ -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", diff --git a/frontend/src/i18n/locales/en/playlists.json b/frontend/src/i18n/locales/en/playlists.json index 06792711..2b2d6ddb 100644 --- a/frontend/src/i18n/locales/en/playlists.json +++ b/frontend/src/i18n/locales/en/playlists.json @@ -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.", diff --git a/frontend/src/i18n/locales/hu/errors.json b/frontend/src/i18n/locales/hu/errors.json index 3fcd3f93..7fb3e5c8 100644 --- a/frontend/src/i18n/locales/hu/errors.json +++ b/frontend/src/i18n/locales/hu/errors.json @@ -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", diff --git a/frontend/src/i18n/locales/hu/playlists.json b/frontend/src/i18n/locales/hu/playlists.json index 50d68821..44f0b30a 100644 --- a/frontend/src/i18n/locales/hu/playlists.json +++ b/frontend/src/i18n/locales/hu/playlists.json @@ -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.", diff --git a/frontend/src/lib/actionErrors.test.ts b/frontend/src/lib/actionErrors.test.ts index 7142f0f4..721f9ab6 100644 --- a/frontend/src/lib/actionErrors.test.ts +++ b/frontend/src/lib/actionErrors.test.ts @@ -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" }); + }); +}); diff --git a/frontend/src/lib/actionErrors.ts b/frontend/src/lib/actionErrors.ts index f59b099c..93fd8a6b 100644 --- a/frontend/src/lib/actionErrors.ts +++ b/frontend/src/lib/actionErrors.ts @@ -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 }); } diff --git a/frontend/src/lib/api.ts b/frontend/src/lib/api.ts index 151b6d07..372a9c9b 100644 --- a/frontend/src/lib/api.ts +++ b/frontend/src/lib/api.ts @@ -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, diff --git a/frontend/src/lib/api/core.test.ts b/frontend/src/lib/api/core.test.ts index fee88f07..ca5c2b79 100644 --- a/frontend/src/lib/api/core.test.ts +++ b/frontend/src/lib/api/core.test.ts @@ -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); + }); +}); diff --git a/frontend/src/lib/api/core.ts b/frontend/src/lib/api/core.ts index 2847df40..7d598e92 100644 --- a/frontend/src/lib/api/core.ts +++ b/frontend/src/lib/api/core.ts @@ -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(); +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( continue; } markConnectivityLost(); + if (typeof e === "object" && e !== null) reported.add(e); throw e; } @@ -257,20 +271,26 @@ export async function req( // 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( } 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);