From 4a7b65d187fdd53ee9e6c13272406e39b8d2a1ef Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sat, 12 Sep 2026 04:07:25 +0200 Subject: [PATCH] fix(api): a refused write is named, in the reader's language MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deep-review round 5, over round 4's own fixes. - `notifyActionError` preferred the server's `detail` over the caller's translated fallback, and `reportFailure` deliberately leaves 403/404/429 to the caller — so for exactly those statuses the dozen call sites round 4 converted started printing whatever English a route hand-wrote into an otherwise Hungarian UI ("No user with that email.", "Not subscribed to this channel", "Unknown job"). A STRUCTURED detail is still preferred: `req` has already run it through `localizeDetail`, so it is both localized and more specific. A plain string is not. - The other half of the same defect, on the backend: round 4's `get_or_404(what="channel")` sweep reached exactly ONE of the five identical sites in channels.py and shipped the HU/EN string with it, so the Block button said it in Hungarian and the four controls beside it in English. All five are converted, and the subscription / share-recipient / scheduler-job 404s with them (two new reasons in both locales). - `PlexCollectionEditor`'s add/remove, take-over and delete were try/finally with no catch — in the very file round 4 edited to give `createNew` one. 403 and 404 are the caller's to report, so the tick simply never appeared and a CONFIRMED "delete collection" did nothing, silently. - `UploadDialog` is the only `uploadReq` caller and kept an empty catch whose comment still claimed the global error modal had it. It had not since `reportFailure` became shared: a proxy 502 raised only the self-clearing connection notice and a 413/403/404 nothing at all, so the dialog snapped back to its pre-upload state with no error named. The user's own Cancel stays silent — and since a cancel and a network drop both carry status 0, that difference is now a named `isAborted` rather than a string literal at each end. Regression tests for each (5 vitest, 2 pytest, plus two existing assertions corrected), all mutation-checked: 6 mutations, each red on the assertion it is named after. vitest 1007, pytest 1127. --- backend/app/routes/channels.py | 20 +-- backend/app/routes/downloads.py | 4 +- backend/app/routes/scheduler.py | 5 +- backend/tests/test_not_found_detail.py | 33 ++++ .../src/components/PlexCollectionEditor.tsx | 11 ++ frontend/src/components/UploadDialog.tsx | 14 +- .../WriteFailureReporting.dom.test.tsx | 146 ++++++++++++++++++ .../usePlaylistActions.dom.test.tsx | 21 ++- frontend/src/i18n/locales/en/downloads.json | 3 +- frontend/src/i18n/locales/en/errors.json | 5 +- frontend/src/i18n/locales/en/plex.json | 3 +- frontend/src/i18n/locales/hu/downloads.json | 3 +- frontend/src/i18n/locales/hu/errors.json | 5 +- frontend/src/i18n/locales/hu/plex.json | 3 +- frontend/src/lib/actionErrors.test.ts | 24 ++- frontend/src/lib/actionErrors.ts | 16 +- frontend/src/lib/api/core.ts | 14 +- 17 files changed, 291 insertions(+), 39 deletions(-) create mode 100644 frontend/src/components/WriteFailureReporting.dom.test.tsx diff --git a/backend/app/routes/channels.py b/backend/app/routes/channels.py index 85ef7a11..4c868976 100644 --- a/backend/app/routes/channels.py +++ b/backend/app/routes/channels.py @@ -26,7 +26,7 @@ from app.models import ( User, Video, ) -from app.routes._common import get_or_404, owned_or_404 +from app.routes._common import get_or_404, not_found, owned_or_404 from app.sync.explore import explore_ingest_page from app.sync.runner import backfill_channel_now, run_recent_backfill from app.sync.subscriptions import apply_channel_details @@ -287,9 +287,7 @@ def channel_detail( needed — a stub, or a channel last enriched before those fields existed (published_at is a reliable sentinel: every real channel has a creation date). The enrich uses the API key (1 unit, cached after), so it's skipped for the quota-less demo account.""" - channel = db.get(Channel, channel_id) - if channel is None: - raise HTTPException(status_code=404, detail="Unknown channel") + channel = get_or_404(db, Channel, channel_id, what="channel") if channel.published_at is None and not user.is_demo: try: with ( @@ -366,9 +364,7 @@ def explore_channel( it, then ingests one page of its uploads (newest-first) marked via_explore + enriched. Returns the next page token so the page can "load more". Guarded against the shared backfill quota reserve; blocked for the demo account (it spends shared quota).""" - channel = db.get(Channel, channel_id) - if channel is None: - raise HTTPException(status_code=404, detail="Unknown channel") + channel = get_or_404(db, Channel, channel_id, what="channel") if _is_blocked(db, user, channel_id): raise HTTPException(status_code=403, detail="You've blocked this channel.") if quota.backfill_reserve_reached(): @@ -426,7 +422,7 @@ def _user_subscription(db: Session, user: User, channel_id: str) -> Subscription ) ).scalar_one_or_none() if sub is None: - raise HTTPException(status_code=404, detail="Not subscribed to this channel") + raise not_found("subscription") return sub @@ -512,9 +508,7 @@ def reset_backfill( state (a "reset" trigger). Clears the channel's backfill markers, opts it back into deep backfill, and re-runs its recent pull immediately; the deep scheduler re-pages the full back-catalog on its next run. Idempotent — videos upsert by id, so nothing duplicates.""" - channel = db.get(Channel, channel_id) - if channel is None: - raise HTTPException(status_code=404, detail="Unknown channel") + channel = get_or_404(db, Channel, channel_id, what="channel") channel.backfill_done = False channel.backfill_cursor = None channel.recent_synced_at = None @@ -544,9 +538,7 @@ def subscribe( local subscription with YouTube's returned resource id and enriches the channel's metadata if it's still a stub, so the new row renders properly in the manager. It does NOT pull the channel's videos — the scheduler picks those up on its next run, like any subscribed channel.""" - channel = db.get(Channel, channel_id) - if channel is None: - raise HTTPException(status_code=404, detail="Unknown channel") + channel = get_or_404(db, Channel, channel_id, what="channel") existing = db.execute( select(Subscription).where( Subscription.user_id == user.id, Subscription.channel_id == channel_id diff --git a/backend/app/routes/downloads.py b/backend/app/routes/downloads.py index 417d7c72..01a56f56 100644 --- a/backend/app/routes/downloads.py +++ b/backend/app/routes/downloads.py @@ -38,7 +38,7 @@ from app.models import ( MediaAsset, User, ) -from app.routes._common import get_or_404 +from app.routes._common import get_or_404, not_found from app.security import hash_password from app.userscope import is_messageable_user, messageable_clauses from app.utils import iso @@ -802,7 +802,7 @@ def share_download( # Uniform "no user" for a missing OR non-messageable (demo/suspended/deactivated) target, so # sharing can't be used to probe which addresses exist or to reach a suspended account. if not is_messageable_user(recipient): - raise HTTPException(status_code=404, detail="No user with that email.") + raise not_found("recipient") if recipient.id == user.id: raise HTTPException(status_code=400, detail="That's already your download.") exists = db.execute( diff --git a/backend/app/routes/scheduler.py b/backend/app/routes/scheduler.py index b605fea6..c5a39258 100644 --- a/backend/app/routes/scheduler.py +++ b/backend/app/routes/scheduler.py @@ -23,6 +23,7 @@ from app.scheduler import ( trigger_job, ) from app.sync.runner import estimate_deep_backfill +from app.routes._common import not_found router = APIRouter(prefix="/api/admin/scheduler", tags=["admin"]) @@ -41,7 +42,7 @@ def set_job_interval( """Change how often a scheduler job runs (minutes). Persisted as an override and applied to the live scheduler immediately.""" if job_id not in JOB_INTERVALS: - raise HTTPException(status_code=404, detail="Unknown job") + raise not_found("job") minutes = body.interval_minutes if not (MIN_INTERVAL <= minutes <= MAX_INTERVAL): raise HTTPException( @@ -73,7 +74,7 @@ def run_job_now( try: result = trigger_job(job_id, actor_id=user.id) except KeyError: - raise HTTPException(status_code=404, detail="Unknown job") + raise not_found("job") if result == "already_running": raise HTTPException(status_code=409, detail="Job is already running") return {"id": job_id, "started": True} diff --git a/backend/tests/test_not_found_detail.py b/backend/tests/test_not_found_detail.py index 1510fff9..6f177d98 100644 --- a/backend/tests/test_not_found_detail.py +++ b/backend/tests/test_not_found_detail.py @@ -41,6 +41,39 @@ def test_a_gone_channel_video_or_user_says_what_is_gone(db, make_user, as_user): assert r.json()["detail"] == {"code": "not_found", "reason": what}, url +def test_every_gone_channel_route_says_it_the_same_way(db, make_user, as_user): + """The round-4 sweep converted ONE of the five `db.get(Channel, …) is None -> 404 "Unknown + channel"` sites (block), and shipped the HU/EN string with it — so the Hungarian UI said it in + Hungarian on the Block button and in English on the four beside it (deep-review round 5). The + asymmetric fix is the smell this repo bans, so all five are pinned together.""" + admin = make_user(role="admin") + client = as_user(admin) + gone = "UCgone000000000000000000" + # Subscribe is behind the write-scope gate, so it is covered by the tags route below instead. + for method, url in [ + ("get", f"/api/channels/{gone}"), + ("post", f"/api/channels/{gone}/reset-backfill"), + ("post", f"/api/channels/{gone}/block"), + ]: + r = getattr(client, method)(url) + assert r.status_code == 404, (url, r.text) + assert r.json()["detail"] == {"code": "not_found", "reason": "channel"}, url + + +def test_a_gone_subscription_recipient_or_job_says_what_is_gone(db, make_user, as_user): + """The three bare-English 404s the same sweep left behind, which `notifyActionError` printed + verbatim into an otherwise Hungarian UI: "Not subscribed to this channel", "No user with that + email.", "Unknown job".""" + admin = make_user(role="admin") + client = as_user(admin) + r = client.post("/api/channels/UCgone000000000000000000/tags", json={"tag_id": 1}) + assert r.status_code == 404, r.text + assert r.json()["detail"] == {"code": "not_found", "reason": "subscription"} + r = client.patch("/api/admin/scheduler/jobs/no_such_job", json={"interval_minutes": 30}) + assert r.status_code == 404, r.text + assert r.json()["detail"] == {"code": "not_found", "reason": "job"} + + def test_a_nul_in_a_request_string_is_a_422_not_a_500(db, make_user, as_user): """Postgres cannot hold NUL in text, so the driver refuses the query outright: a DataError, i.e. a 500 with a traceback, wherever a `%00` or an escaped NUL reached a comparison (deep-review diff --git a/frontend/src/components/PlexCollectionEditor.tsx b/frontend/src/components/PlexCollectionEditor.tsx index ab5dac28..f6091804 100644 --- a/frontend/src/components/PlexCollectionEditor.tsx +++ b/frontend/src/components/PlexCollectionEditor.tsx @@ -77,6 +77,11 @@ export default function PlexCollectionEditor({ return next; }); refresh(); + } catch (e) { + // 403/404 are left to the caller by `req` (see reportFailure), so without this the tick + // simply never appeared and the user clicked again — a collection deleted on the Plex server + // or flipped read-only failed in silence (deep-review round 5). + notifyActionError(e, t("plex.collection.editFailed")); } finally { setBusy(null); } @@ -88,6 +93,8 @@ export default function PlexCollectionEditor({ try { await api.plexSetCollectionEditable(id, true); refresh(); + } catch (e) { + notifyActionError(e, t("plex.collection.editFailed")); } finally { setBusy(null); } @@ -123,6 +130,10 @@ export default function PlexCollectionEditor({ return next; }); refresh(); + } catch (e) { + // The worst of the three to lose: the user has just confirmed a destructive dialog, and the + // row stayed where it was with nothing said. + notifyActionError(e, t("plex.collection.deleteFailed")); } finally { setBusy(null); } diff --git a/frontend/src/components/UploadDialog.tsx b/frontend/src/components/UploadDialog.tsx index 7b586b07..c46951aa 100644 --- a/frontend/src/components/UploadDialog.tsx +++ b/frontend/src/components/UploadDialog.tsx @@ -5,6 +5,8 @@ import { UploadCloud, File as FileIcon } from "lucide-react"; import Modal from "./ui/Modal"; import { Button } from "./ui/Button"; import { api } from "../lib/api"; +import { isAborted } from "../lib/api/core"; +import { notifyActionError } from "../lib/actionErrors"; import { invalidateDownloads } from "../lib/downloads"; import { notify } from "../lib/notifications"; import { navigateTo } from "../lib/nav"; @@ -54,9 +56,15 @@ export default function UploadDialog({ onClose }: { onClose: () => void }) { action: { label: t("downloads.dialog.view"), onClick: () => navigateTo("downloads") }, }); onClose(); - } catch { - // An aborted upload (Cancel) or a server rejection (cap/quota — already surfaced by the global - // error modal, like every other req call). Re-arm the form so the user can retry or pick again. + } catch (e) { + // Everything except the user's own Cancel is said out loud. `uploadReq` shares `reportFailure` + // with `req`, which deliberately leaves 403/404/429 to the caller and answers a 502/503/504 + // with only the transient "connection lost" notice — so this empty catch meant a proxy 502 on + // a long upload, or an nginx 413, snapped the dialog back to its pre-upload state with nothing + // named and no job in the list. The comment here claimed the global error modal had it; it + // had not since the shared `reportFailure` landed (deep-review round 5). + if (!isAborted(e)) notifyActionError(e, t("downloads.upload.failed")); + // Re-arm the form so the user can retry or pick again. setPct(null); abortRef.current = null; } diff --git a/frontend/src/components/WriteFailureReporting.dom.test.tsx b/frontend/src/components/WriteFailureReporting.dom.test.tsx new file mode 100644 index 00000000..b76e1555 --- /dev/null +++ b/frontend/src/components/WriteFailureReporting.dom.test.tsx @@ -0,0 +1,146 @@ +// @vitest-environment jsdom +// +// A write that fails is named, wherever it fails. `req`/`uploadReq` deliberately leave 403/404/429 +// to the caller (see reportFailure) and answer a 502/503/504 with only the transient "connection +// lost" notice — so a handler with no `catch` reports nothing at all. Round 4 swept a dozen such +// call sites and missed these two: the collection editor's three siblings, in the very file it +// edited, and the one `uploadReq` caller, whose empty catch still claimed the global error modal had +// it (deep-review round 5). +import { describe, it, expect, vi, beforeAll, beforeEach, afterEach } from "vitest"; +import { render, cleanup, screen, fireEvent, waitFor } from "@testing-library/react"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; + +vi.mock("react-i18next", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + useTranslation: () => ({ t: (key: string) => key, i18n: { language: "en" } }), + }; +}); +vi.mock("./ConfirmProvider", () => ({ useConfirm: () => async () => true })); +const { notify } = vi.hoisted(() => ({ + notify: vi.fn((_n: { level: string; message: string }) => 0), +})); +vi.mock("../lib/notifications", async (importOriginal) => ({ + ...(await importOriginal()), + notify, +})); + +import PlexCollectionEditor from "./PlexCollectionEditor"; +import UploadDialog from "./UploadDialog"; +import { api, HttpError } from "../lib/api"; + +beforeAll(() => { + class Stub { + observe() {} + unobserve() {} + disconnect() {} + takeRecords() { + return []; + } + } + globalThis.ResizeObserver ??= Stub as unknown as typeof ResizeObserver; + globalThis.IntersectionObserver ??= Stub as unknown as typeof IntersectionObserver; +}); +beforeEach(() => notify.mockClear()); +afterEach(() => { + cleanup(); + vi.restoreAllMocks(); +}); + +const errors = () => notify.mock.calls.filter(([n]) => n.level === "error").map(([n]) => n.message); +const client = () => new QueryClient({ defaultOptions: { queries: { retry: false } } }); + +describe("the Plex collection editor", () => { + const collections = [ + { id: "c1", title: "Marathon", can_edit: true, child_count: 3, smart: false }, + // Not marked "mine to edit" yet, so it offers the take-over button. + { + id: "c2", + title: "Theirs", + can_edit: false, + child_count: 9, + smart: false, + source: "collection", + }, + ]; + const mount = () => + render( + + {}} + onChanged={() => {}} + /> + , + ); + + it("says so when adding to a collection that is gone, instead of nothing happening", async () => { + vi.spyOn(api, "plexCollections").mockResolvedValue({ collections } as never); + vi.spyOn(api, "plexCollectionAddItem").mockRejectedValue( + new HttpError(404, "Unknown collection"), + ); + mount(); + fireEvent.click(await screen.findByText("Marathon")); + await waitFor(() => expect(errors()).toEqual(["plex.collection.editFailed"])); + }); + + it("says so when enabling editing is refused", async () => { + vi.spyOn(api, "plexCollections").mockResolvedValue({ collections } as never); + vi.spyOn(api, "plexSetCollectionEditable").mockRejectedValue( + new HttpError(403, "This collection is read-only"), + ); + mount(); + fireEvent.click(await screen.findByText("plex.collEditor.makeEditable")); + await waitFor(() => expect(errors()).toEqual(["plex.collection.editFailed"])); + }); + + it("says so when a CONFIRMED delete does not happen", async () => { + // The worst of the three to lose: the user has just confirmed a destructive dialog. + vi.spyOn(api, "plexCollections").mockResolvedValue({ collections } as never); + vi.spyOn(api, "plexDeleteCollection").mockRejectedValue( + new HttpError(404, "Unknown collection"), + ); + mount(); + fireEvent.click(await screen.findByText("plex.collEditor.delete")); + await waitFor(() => expect(errors()).toEqual(["plex.collection.deleteFailed"])); + }); +}); + +describe("the upload dialog", () => { + const pick = () => { + const input = document.querySelector('input[type="file"]') as HTMLInputElement; + const file = new File(["x"], "clip.mp4", { type: "video/mp4" }); + Object.defineProperty(input, "files", { value: [file], configurable: true }); + fireEvent.change(input); + }; + const mount = () => + render( + + {}} /> + , + ); + + it("names an upload the server refused", async () => { + // A 502 from the proxy raises only the self-clearing connection notice; the next successful poll + // wipes it, and the dialog snapped back with no error and no job in the list. + vi.spyOn(api, "uploadDownload").mockRejectedValue(new HttpError(502, "Bad Gateway")); + mount(); + pick(); + fireEvent.click(screen.getByText("downloads.upload.submit")); + await waitFor(() => expect(errors()).toEqual(["downloads.upload.failed"])); + }); + + it("says nothing when the user cancelled it themselves", async () => { + // A cancel is not a failure — and it rejects with the same status 0 a genuine network drop does, + // which is why the difference is a named sentinel (`isAborted`) and not a status check. + const upload = vi.spyOn(api, "uploadDownload").mockRejectedValue(new HttpError(0, "aborted")); + mount(); + pick(); + fireEvent.click(screen.getByText("downloads.upload.submit")); + await waitFor(() => expect(upload).toHaveBeenCalled()); + expect(errors()).toEqual([]); + }); +}); diff --git a/frontend/src/components/usePlaylistActions.dom.test.tsx b/frontend/src/components/usePlaylistActions.dom.test.tsx index 32f43c3c..5ab31b77 100644 --- a/frontend/src/components/usePlaylistActions.dom.test.tsx +++ b/frontend/src/components/usePlaylistActions.dom.test.tsx @@ -60,10 +60,25 @@ describe("usePlaylistActions — deleting a list", () => { expect(h.notify).not.toHaveBeenCalled(); }); - it("does say one it was left to handle", async () => { - vi.spyOn(api, "deletePlaylist").mockRejectedValue(new HttpError(404, "gone")); + it("does say one it was left to handle — in the reader's language", async () => { + // A plain-string `detail` is whatever English the route hand-wrote; the caller's own translated + // fallback is the one a Hungarian reader can read (deep-review round 5). + vi.spyOn(api, "deletePlaylist").mockRejectedValue(new HttpError(404, "Unknown playlist")); const { result } = mount(); await act(() => result.current.remove(linked)); - expect(h.notify).toHaveBeenCalledWith(expect.objectContaining({ message: "gone" })); + expect(h.notify).toHaveBeenCalledWith( + expect.objectContaining({ message: "playlists.deleteYtFailed" }), + ); + }); + + it("prefers a STRUCTURED reason, which the request layer has already localized", async () => { + vi.spyOn(api, "deletePlaylist").mockRejectedValue( + new HttpError(404, "Ez a lista már nem létezik.", { code: "not_found", reason: "playlist" }), + ); + const { result } = mount(); + await act(() => result.current.remove(linked)); + expect(h.notify).toHaveBeenCalledWith( + expect.objectContaining({ message: "Ez a lista már nem létezik." }), + ); }); }); diff --git a/frontend/src/i18n/locales/en/downloads.json b/frontend/src/i18n/locales/en/downloads.json index 693adfb3..e0cc6b04 100644 --- a/frontend/src/i18n/locales/en/downloads.json +++ b/frontend/src/i18n/locales/en/downloads.json @@ -34,7 +34,8 @@ "submit": "Upload", "uploading": "Uploading… {{pct}}%", "uploadProgress": "Upload progress", - "added": "File uploaded" + "added": "File uploaded", + "failed": "The upload failed." }, "tabs": { "queue": "Queue", diff --git a/frontend/src/i18n/locales/en/errors.json b/frontend/src/i18n/locales/en/errors.json index 64c6d201..05c6e9c2 100644 --- a/frontend/src/i18n/locales/en/errors.json +++ b/frontend/src/i18n/locales/en/errors.json @@ -14,7 +14,10 @@ "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.", - "channel": "This channel isn't in the library any more." + "channel": "This channel isn't in the library any more.", + "subscription": "You're not subscribed to this channel any more.", + "recipient": "There's no user with that email address.", + "job": "This job doesn't exist any more." }, "ok": "OK", "offline": { diff --git a/frontend/src/i18n/locales/en/plex.json b/frontend/src/i18n/locales/en/plex.json index 7970e9de..f4d4c768 100644 --- a/frontend/src/i18n/locales/en/plex.json +++ b/frontend/src/i18n/locales/en/plex.json @@ -29,7 +29,8 @@ "namePlaceholder": "Collection name", "nameTaken": "A collection with this name already exists.", "newHint": "Pick the first title to add — a collection needs at least one.", - "createWith": "Create" + "createWith": "Create", + "deleteFailed": "Couldn't delete the collection." }, "searchPlaceholder": "Search Plex…", "searchField": { diff --git a/frontend/src/i18n/locales/hu/downloads.json b/frontend/src/i18n/locales/hu/downloads.json index f845cc2c..a396d465 100644 --- a/frontend/src/i18n/locales/hu/downloads.json +++ b/frontend/src/i18n/locales/hu/downloads.json @@ -34,7 +34,8 @@ "submit": "Feltöltés", "uploading": "Feltöltés… {{pct}}%", "uploadProgress": "Feltöltés folyamata", - "added": "Fájl feltöltve" + "added": "Fájl feltöltve", + "failed": "A feltöltés nem sikerült." }, "tabs": { "queue": "Sor", diff --git a/frontend/src/i18n/locales/hu/errors.json b/frontend/src/i18n/locales/hu/errors.json index 14b0d867..0971ba85 100644 --- a/frontend/src/i18n/locales/hu/errors.json +++ b/frontend/src/i18n/locales/hu/errors.json @@ -14,7 +14,10 @@ "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.", - "channel": "Ez a csatorna már nincs a könyvtárban." + "channel": "Ez a csatorna már nincs a könyvtárban.", + "subscription": "Már nem vagy feliratkozva erre a csatornára.", + "recipient": "Nincs felhasználó ezzel az e-mail címmel.", + "job": "Ez a feladat már nem létezik." }, "ok": "OK", "offline": { diff --git a/frontend/src/i18n/locales/hu/plex.json b/frontend/src/i18n/locales/hu/plex.json index e85ab902..61da094c 100644 --- a/frontend/src/i18n/locales/hu/plex.json +++ b/frontend/src/i18n/locales/hu/plex.json @@ -29,7 +29,8 @@ "namePlaceholder": "Kollekció neve", "nameTaken": "Már létezik ilyen nevű kollekció.", "newHint": "Válaszd ki az első címet — egy kollekcióhoz legalább egy elem kell.", - "createWith": "Létrehozás" + "createWith": "Létrehozás", + "deleteFailed": "Nem sikerült törölni a gyűjteményt." }, "searchPlaceholder": "Keresés a Plexben…", "searchField": { diff --git a/frontend/src/lib/actionErrors.test.ts b/frontend/src/lib/actionErrors.test.ts index 1f64c5b7..f46cb78a 100644 --- a/frontend/src/lib/actionErrors.test.ts +++ b/frontend/src/lib/actionErrors.test.ts @@ -57,6 +57,28 @@ describe("notifyActionError", () => { it("does toast one the caller was left to handle", () => { notifyActionError(new HttpError(404, "gone"), "FAILED"); - expect(notifyMock).toHaveBeenCalledWith({ level: "error", message: "gone" }); + expect(notifyMock).toHaveBeenCalled(); + }); + + it("says a bare-English server reason in the reader's language, not the server's", () => { + // `reportFailure` leaves 403/404/429 to the caller, so for exactly those statuses whatever + // English a route hand-wrote went straight into the toast — "No user with that email." in an + // otherwise Hungarian UI, while the caller's translated fallback sat unused (round 5). + notifyActionError(new HttpError(404, "No user with that email."), "TRANSLATED"); + expect(notifyMock).toHaveBeenCalledWith({ level: "error", message: "TRANSLATED" }); + }); + + it("still prefers a STRUCTURED reason, which the request layer has already localized", () => { + // `req` runs `{code, reason}` through localizeDetail, so `detail` is in the reader's language + // AND more specific than the caller's generic fallback — quota numbers, which row is gone. + const err = new HttpError(404, "Ez a csatorna már nincs a könyvtárban.", { + code: "not_found", + reason: "channel", + }); + notifyActionError(err, "TRANSLATED FALLBACK"); + expect(notifyMock).toHaveBeenCalledWith({ + level: "error", + message: "Ez a csatorna már nincs a könyvtárban.", + }); }); }); diff --git a/frontend/src/lib/actionErrors.ts b/frontend/src/lib/actionErrors.ts index e2ad1d26..5a8949f2 100644 --- a/frontend/src/lib/actionErrors.ts +++ b/frontend/src/lib/actionErrors.ts @@ -5,10 +5,14 @@ import { notify } from "./notifications"; /** * Surface a failed write action as an error toast instead of the click silently doing nothing. - * Prefers the server's own reason (`HttpError.detail` — e.g. "Not enough quota left today", a - * 404/429 message) over the caller's already-translated generic fallback. Domain-agnostic: used by - * the Plex watched-toggles and the saved-view ops, and delegated to by `notifyYouTubeActionError` - * for its non-403 case. + * Prefers the server's own reason over the caller's already-translated generic fallback — but only + * when the server sent a STRUCTURED detail (`{code, reason}`), which `req` has already run through + * `localizeDetail`. A plain-string `detail` is whatever English the route hand-wrote, and + * `reportFailure` deliberately leaves 403/404/429 to the caller, so exactly those statuses used to + * put the route's English sentence into an otherwise Hungarian UI — "No user with that email.", + * "Not subscribed to this channel" — where the caller's own translated fallback was right there + * (deep-review round 5). Domain-agnostic: used by the Plex watched-toggles and the saved-view ops, + * and delegated to by `notifyYouTubeActionError` for its non-403 case. */ export function notifyActionError( err: unknown, @@ -19,6 +23,6 @@ export function notifyActionError( // 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, message: detail || fallbackMessage }); + const localized = err instanceof HttpError && err.detailData != null ? err.detail : null; + notify({ level, message: localized || fallbackMessage }); } diff --git a/frontend/src/lib/api/core.ts b/frontend/src/lib/api/core.ts index 2afc3e66..a8cce25f 100644 --- a/frontend/src/lib/api/core.ts +++ b/frontend/src/lib/api/core.ts @@ -28,6 +28,16 @@ export function alreadyReported(err: unknown): boolean { return typeof err === "object" && err !== null && reported.has(err); } +/** What an upload the USER cancelled rejects with. A cancel is not a failure to report, and the + * only other rejection carrying status 0 is a genuine network drop, which IS worth reporting — so + * the difference has to be a named thing both sides agree on, not a string literal in each. */ +const ABORTED = "aborted"; + +/** True for the rejection of a request the user cancelled (see `ABORTED`). */ +export function isAborted(err: unknown): boolean { + return err instanceof HttpError && err.status === 0 && err.detail === ABORTED; +} + export class HttpError extends Error { status: number; detail?: string; // server-provided reason (FastAPI's `detail`), when present @@ -342,7 +352,7 @@ export function uploadReq( }; } if (cfg.signal) { - if (cfg.signal.aborted) return reject(new HttpError(0, "aborted")); + if (cfg.signal.aborted) return reject(new HttpError(0, ABORTED)); cfg.signal.addEventListener("abort", () => xhr.abort()); } xhr.onload = () => { @@ -376,7 +386,7 @@ export function uploadReq( markConnectivityLost(); reject(new HttpError(0, i18n.t("errors.offline.body"))); }; - xhr.onabort = () => reject(new HttpError(0, "aborted")); + xhr.onabort = () => reject(new HttpError(0, ABORTED)); xhr.send(form); }); }