From a0114deff194f9053fcf9036bd899fa60eb4f712 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sat, 12 Sep 2026 05:50:31 +0200 Subject: [PATCH] fix(api,ui): a refused write says why, wherever the refusal happens MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 5 taught `notifyActionError` to discard a plain-string `detail` in favour of the caller's translated fallback, because `req` leaves 403/404/429 to the caller and those routes hand-write English. Three places did not get the message. **The demo account lost its explanation.** `require_human` refused with a plain string, so from that commit on the shared demo account got its caller's generic "couldn't do that" and no hint that the demo account was the reason — and the gate is not backed by a UI gate everywhere: the comfort shelf has no `is_demo` check, so the pin button is live and simply fails. A structured `{code: "demo"}` and an `errors.demo` string in both locales. The two auth routes that hand-roll the same check were swept with it. **`ChatThread` carried a second copy of the contract.** It filtered 404/429 itself and then did `e.detail || t(…)`, the pre-fix order, so the central fix could not reach it and "That member hasn't set up messaging yet." stayed English under a Hungarian UI. It goes through the shared helper now, and a contract test pins the remaining raw-`detail` consumers as an explicit SET — never a count — so a new copy fails the gate instead of shipping. The five on that list are inline surfaces (a form field's error line, a setup banner) and are deferred with the ~60 backend routes whose details they print. **The Plex "Add to playlist" dialog was still silent.** Same shape as the collection editor round 5 fixed and the YouTube twin that already had the fix: no catch, so a playlist deleted on another device answered a structured 404 that nothing reported — the tick never flipped, the gone row stayed, and the floating promise became an unhandled rejection. Also: the `ABORTED` sentinel's VALUE was asserted nowhere. The consumer test hand-built the error it expected, so replacing `xhr.onabort`'s rejection with a friendlier message left the lane green — and every upload the user cancels would then raise a red "the upload failed" toast, which is what the sentinel prevents. Both producer tests assert through `isAborted` now. And two backend tests were pinning fewer sites than their own docstrings claimed: the channel-404 loop said "all five" and issued three requests (explore and subscribe unexercised, the excuse for subscribe naming a different call site with a different reason slug), and the recipient/job test named three refusals and requested two — `share_download`'s `not_found("recipient")` had no test in the suite at all, nor did the locale keys added beside it. Every site named now has a request behind it. 6 tests, mutation-checked against 7 mutations: the demo detail at both the gate and an auth route, a re-hand-rolled ChatThread, the silent Plex dialog, the abort sentinel, and each unpinned 404 — all red on the test that names them. --- backend/app/auth.py | 6 +- backend/app/routes/auth.py | 4 +- backend/tests/test_demo_refusal_detail.py | 47 ++++++++++++++ backend/tests/test_not_found_detail.py | 53 +++++++++++++-- frontend/src/components/ChatThread.tsx | 15 +++-- .../PlaylistEditFailures.dom.test.tsx | 25 +++++++ frontend/src/components/PlexPlaylistAdd.tsx | 19 +++++- frontend/src/i18n/locales/en/errors.json | 1 + frontend/src/i18n/locales/en/plex.json | 5 +- frontend/src/i18n/locales/hu/errors.json | 1 + frontend/src/i18n/locales/hu/plex.json | 5 +- .../src/lib/actionErrors.contract.test.ts | 65 +++++++++++++++++++ frontend/src/lib/actionErrors.test.ts | 20 ++++++ frontend/src/lib/api/core.test.ts | 17 ++++- frontend/src/lib/api/core.ts | 4 ++ 15 files changed, 266 insertions(+), 21 deletions(-) create mode 100644 backend/tests/test_demo_refusal_detail.py create mode 100644 frontend/src/lib/actionErrors.contract.test.ts diff --git a/backend/app/auth.py b/backend/app/auth.py index 8a6859bb..95d027ec 100644 --- a/backend/app/auth.py +++ b/backend/app/auth.py @@ -341,7 +341,11 @@ def require_human(user: User = Depends(current_user)) -> User: no token, so has_read_scope/has_write_scope are False); this makes the boundary explicit where there's no scope check to lean on.""" if user.is_demo: - raise HTTPException(status_code=403, detail="Not available in the demo account.") + # Structured, because `req` leaves 403 to the caller and `notifyActionError` only shows a + # STRUCTURED detail — a plain string here reached the demo user as their caller's generic + # "couldn't do that", with no hint that the demo account is the reason (round 6). Affordances + # this gate sits behind are not all UI-gated on `is_demo` (the comfort shelf is not). + raise HTTPException(status_code=403, detail={"code": "demo"}) return user diff --git a/backend/app/routes/auth.py b/backend/app/routes/auth.py index c76bd49f..b96d4304 100644 --- a/backend/app/routes/auth.py +++ b/backend/app/routes/auth.py @@ -1246,7 +1246,7 @@ def logout_others( are rejected, then re-stamp THIS cookie so the caller stays signed in. For "I left myself logged in somewhere" / a suspected session compromise. The demo account (shared) is excluded.""" if user.is_demo: - raise HTTPException(status_code=403, detail="Not available in the demo account.") + raise HTTPException(status_code=403, detail={"code": "demo"}) # Commit the bump BEFORE re-stamping this cookie, so a failed commit can't strand the current # session at a newer epoch than the DB (see set_password). bump_session_epoch(user) @@ -1293,7 +1293,7 @@ def set_password( account) needs no current password — the session already proves identity. Changing an existing one requires the current password, so a hijacked session can't silently rotate it.""" if user.is_demo: - raise HTTPException(status_code=403, detail="Not available in the demo account.") + raise HTTPException(status_code=403, detail={"code": "demo"}) new_password = body.password or "" if len(new_password) < PASSWORD_MIN_LEN: raise HTTPException( diff --git a/backend/tests/test_demo_refusal_detail.py b/backend/tests/test_demo_refusal_detail.py new file mode 100644 index 00000000..6596c862 --- /dev/null +++ b/backend/tests/test_demo_refusal_detail.py @@ -0,0 +1,47 @@ +"""The shared demo account's refusal says WHY, in the reader's language (deep-review round 6). + +`require_human` answered `403 "Not available in the demo account."` — a plain string. `req` leaves +403 to the caller, and round 5 taught `notifyActionError` to discard a plain-string detail in favour +of the caller's already-translated fallback, so from that commit on the demo user got a generic +"couldn't do that" and no hint that the demo account was the reason. Not every affordance behind +this gate is UI-gated on `is_demo` either — the comfort shelf is not, so the pin button is live. + +A structured `{code: "demo"}` is what `localizeDetail` can turn into `errors.demo` in both locales. +Pinned at all three sites: the shared dependency and the two auth routes that hand-roll the same +check, because an asymmetric fix is the smell this repo bans. Needs the DB. +""" + +from app.models import User + + +def test_every_demo_refusal_carries_the_same_structured_reason(db, make_user, as_user): + demo = make_user(email="demo@example.com", is_demo=True) + client = as_user(demo) + for method, url, body in [ + # `require_human` itself, through an affordance the UI does NOT gate on is_demo. + ("post", "/api/discover/pins", {"kind": "video", "ref": "v1"}), + # The two routes that repeat the check inline rather than depending on the gate. + ("post", "/auth/logout-others", None), + ("post", "/auth/set-password", {"password": "a long enough password"}), + ]: + r = getattr(client, method)(url, **({"json": body} if body is not None else {})) + assert r.status_code == 403, (url, r.text) + assert r.json()["detail"] == {"code": "demo"}, url + + +def test_a_real_account_is_not_refused_by_the_gate(db, make_user, as_user): + """The other half: the structured detail must not come from having broken the gate open the + other way. A non-demo account gets past `require_human` and fails (or succeeds) on its own + merits — anything but a 403 demo refusal.""" + client = as_user(make_user(email="real@example.com")) + r = client.post("/auth/logout-others") + assert r.status_code == 200, r.text + assert not isinstance(r.json().get("detail"), dict) + + +def test_the_gate_reads_the_flag_and_not_the_email(db): + """`is_demo` is a column, not a convention about the address — a real account that happens to + be called demo@ must not be refused.""" + from app.auth import require_human + + assert require_human(User(email="demo@example.com", is_demo=False)) is not None diff --git a/backend/tests/test_not_found_detail.py b/backend/tests/test_not_found_detail.py index 6f177d98..b7e9645e 100644 --- a/backend/tests/test_not_found_detail.py +++ b/backend/tests/test_not_found_detail.py @@ -6,6 +6,9 @@ Hungarian UI toasted it verbatim. The structured-404 fix had gone into one ad-ho the shared one, so every other owner check kept the English. Needs the DB. """ +from app.auth import require_write_scope +from app.models import DownloadJob, MediaAsset + def test_a_gone_view_tag_or_pin_says_what_is_gone(db, make_user, as_user): client = as_user(make_user()) @@ -41,18 +44,27 @@ 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): +def test_every_gone_channel_route_says_it_the_same_way(db, make_user, as_user, override_dep): """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.""" + asymmetric fix is the smell this repo bans, so all five are pinned together. + + Literally all five: round 6 found this loop issuing three requests under a docstring claiming + five, with explore silently absent and subscribe excused as "covered by the tags route" — which + reaches `_user_subscription`, a different site answering `reason: subscription`. Reverting + either of those two to its bare English string left the whole lane green.""" admin = make_user(role="admin") client = as_user(admin) + # Subscribe's own gate (the optional YouTube write scope) sits in FRONT of the lookup, so a + # plain account 403s before reaching it. The gate is not what is under test here. + override_dep(require_write_scope, lambda: 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}/explore"), ("post", f"/api/channels/{gone}/reset-backfill"), + ("post", f"/api/channels/{gone}/subscribe"), ("post", f"/api/channels/{gone}/block"), ]: r = getattr(client, method)(url) @@ -63,15 +75,44 @@ def test_every_gone_channel_route_says_it_the_same_way(db, make_user, as_user): 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".""" + email.", "Unknown job". + + All three, with a request each. Round 6 found this test naming three and issuing two: the + recipient site had no test in the whole suite (nor did the `errors.notFound.recipient` keys + added beside it), and `run_job_now` rode on its PATCH sibling's coverage.""" 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}) + for method, url in [ + ("patch", "/api/admin/scheduler/jobs/no_such_job"), + ("post", "/api/admin/scheduler/jobs/no_such_job/run"), + ]: + kw = {"json": {"interval_minutes": 30}} if method == "patch" else {} + r = getattr(client, method)(url, **kw) + assert r.status_code == 404, (url, r.text) + assert r.json()["detail"] == {"code": "not_found", "reason": "job"}, url + # Sharing needs a finished download of one's own to share; the recipient lookup is the next check. + asset = MediaAsset( + source_kind="url", source_ref="share-404", format_sig="sig", status="ready", ref_count=1 + ) + db.add(asset) + db.flush() + job = DownloadJob( + user_id=admin.id, + asset_id=asset.id, + source_kind="url", + source_ref="share-404", + status="done", + queue_pos=1, + profile_snapshot={}, + ) + db.add(job) + db.commit() + r = client.post(f"/api/downloads/{job.id}/share", json={"email": "nobody@example.com"}) assert r.status_code == 404, r.text - assert r.json()["detail"] == {"code": "not_found", "reason": "job"} + assert r.json()["detail"] == {"code": "not_found", "reason": "recipient"} def test_a_nul_in_a_request_string_is_a_422_not_a_500(db, make_user, as_user): diff --git a/frontend/src/components/ChatThread.tsx b/frontend/src/components/ChatThread.tsx index cfcd63b8..4f9e8f2e 100644 --- a/frontend/src/components/ChatThread.tsx +++ b/frontend/src/components/ChatThread.tsx @@ -3,13 +3,13 @@ import { qk } from "../lib/queryKeys"; import { useTranslation } from "react-i18next"; import { useMutation, useQueryClient } from "@tanstack/react-query"; import { Send } from "lucide-react"; -import { api, HttpError, type Message, type MessageUser } from "../lib/api"; +import { api, type Message, type MessageUser } from "../lib/api"; import { useLiveQuery } from "../lib/useLiveQuery"; import { useScrollFade } from "../lib/useScrollFade"; import { Button } from "./ui/Button"; import { LoadingState, StateMessage } from "./ui/QueryState"; import { relativeTime } from "../lib/format"; -import { notify } from "../lib/notifications"; +import { notifyActionError } from "../lib/actionErrors"; import { partnerPub, POLL_MS, refreshPartnerPub, useKeyState } from "../lib/messaging"; import * as e2ee from "../lib/e2ee"; import KeyGate from "./KeyGate"; @@ -132,9 +132,14 @@ export default function ChatThread({ // The draft is kept (only cleared on success) so the user can retry. 400/409/422/500 already // raised the global error dialog; 404 (recipient gone) and 429 (rate-limited) are "caller- // handled" in api.req (no modal), so surface those here or the send fails silently. - if (e instanceof HttpError && (e.status === 404 || e.status === 429)) { - notify({ level: "error", message: e.detail || t("messages.sendFailed") }); - } + // + // Through the shared helper, which knows both of those rules: it no-ops on anything already + // reported, and it prefers the server's reason only when that reason is a STRUCTURED detail. + // The hand-written copy that used to live here did `e.detail || …`, so a Hungarian reader got + // the route's English sentence — "That member hasn't set up messaging yet." — which is + // exactly what round 5 changed `notifyActionError` to stop doing. A second copy of a contract + // is how the fix misses a caller (round 6). + notifyActionError(e, t("messages.sendFailed")); }, }); diff --git a/frontend/src/components/PlaylistEditFailures.dom.test.tsx b/frontend/src/components/PlaylistEditFailures.dom.test.tsx index 304982b5..625af9a0 100644 --- a/frontend/src/components/PlaylistEditFailures.dom.test.tsx +++ b/frontend/src/components/PlaylistEditFailures.dom.test.tsx @@ -27,6 +27,7 @@ vi.mock("../lib/notifications", async (importOriginal) => ({ import PlexPlaylistView from "./PlexPlaylistView"; import AddToPlaylist from "./AddToPlaylist"; +import PlexPlaylistAdd from "./PlexPlaylistAdd"; import TagManager from "./TagManager"; import { api, HttpError, type PlexCard } from "../lib/api"; @@ -123,6 +124,30 @@ describe("the add-to-playlist popover", () => { await waitFor(() => expect(errors()).toEqual(["gone playlist"])); await waitFor(() => expect(lists.mock.calls.length).toBeGreaterThan(1)); }); + + it("says so on the PLEX side too, where the same sweep had missed the dialog", async () => { + // Same shape, same 404, same silence — `req` leaves 403/404/429 to the caller, and this one had + // no catch: the tick did not flip, nothing was said, the gone row stayed, and the floating + // promise became an unhandled rejection. Round 5 fixed the collection editor and the YouTube + // twin above; this dialog was left behind (round 6). + const lists = vi + .spyOn(api, "plexPlaylists") + .mockResolvedValue({ playlists: [{ id: 9, title: "Gone", has_item: false }] } as never); + vi.spyOn(api, "plexPlaylistAddItem").mockRejectedValue(gone("playlist")); + render( + + {}} + /> + , + ); + fireEvent.click(await screen.findByText("Gone")); + // The server's own localized reason wins over the caller's fallback when the detail is + // STRUCTURED — same as the twin above. What is under test is that anything is said at all. + await waitFor(() => expect(errors()).toEqual(["gone playlist"])); + await waitFor(() => expect(lists.mock.calls.length).toBeGreaterThan(1)); + }); }); describe("the tag manager", () => { diff --git a/frontend/src/components/PlexPlaylistAdd.tsx b/frontend/src/components/PlexPlaylistAdd.tsx index 76d37f58..bdd63f76 100644 --- a/frontend/src/components/PlexPlaylistAdd.tsx +++ b/frontend/src/components/PlexPlaylistAdd.tsx @@ -4,6 +4,7 @@ import { useTranslation } from "react-i18next"; import { useQuery, useQueryClient } from "@tanstack/react-query"; import { Check, Minus, Plus } from "lucide-react"; import { api, type PlexPlaylist } from "../lib/api"; +import { notifyActionError } from "../lib/actionErrors"; import { useScrollFade } from "../lib/useScrollFade"; import { Button } from "./ui/Button"; import Modal from "./ui/Modal"; @@ -54,6 +55,11 @@ export default function PlexPlaylistAdd({ return isGroup ? (p.group_in ?? 0) : p.has_item ? 1 : 0; } + // A failure is said, and the list refreshed either way. With no catch, a playlist deleted on + // another device (a 404 `not_found/playlist` — still offered here, from a cache up to 30 s old) + // was an unhandled rejection: `req` leaves 403/404/429 to the caller, so nothing was shown, the + // tick never flipped and the gone row stayed. Round 5 swept the collection editor and its own + // YouTube twin (`AddToPlaylist`) and missed this one (round 6). async function toggle(p: PlexPlaylist) { if (busy) return; setBusy(true); @@ -67,8 +73,13 @@ export default function PlexPlaylistAdd({ else await api.plexPlaylistAddItem(p.id, target.ratingKey); } setOverride((m) => new Map(m).set(p.id, allIn ? 0 : size)); - refresh(); + } catch (e) { + notifyActionError( + e, + t(allIn ? "plex.playlistAdd.removeFailed" : "plex.playlistAdd.addFailed"), + ); } finally { + refresh(); setBusy(false); } } @@ -87,8 +98,12 @@ export default function PlexPlaylistAdd({ setOverride((m) => new Map(m).set(pl.id, 1)); } setNewName(""); - refresh(); + } catch (e) { + // The create may already have landed when the add fails — `refresh()` in `finally` is what + // makes that half-done state visible instead of leaving a playlist the dialog never lists. + notifyActionError(e, t("plex.playlistAdd.createFailed")); } finally { + refresh(); setBusy(false); } } diff --git a/frontend/src/i18n/locales/en/errors.json b/frontend/src/i18n/locales/en/errors.json index 05c6e9c2..f7420be4 100644 --- a/frontend/src/i18n/locales/en/errors.json +++ b/frontend/src/i18n/locales/en/errors.json @@ -1,6 +1,7 @@ { "title": "Couldn't complete that", "generic": "The server couldn't carry out that action. Please try again.", + "demo": "This isn't available in the demo account.", "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.", diff --git a/frontend/src/i18n/locales/en/plex.json b/frontend/src/i18n/locales/en/plex.json index f4d4c768..a1fc8a67 100644 --- a/frontend/src/i18n/locales/en/plex.json +++ b/frontend/src/i18n/locales/en/plex.json @@ -297,7 +297,10 @@ "create": "Create", "none": "No playlists yet. Create one above.", "count": "{{count}} items", - "groupProgress": "{{in}} / {{size}}" + "groupProgress": "{{in}} / {{size}}", + "addFailed": "Couldn't add it to the playlist.", + "removeFailed": "Couldn't remove it from the playlist.", + "createFailed": "Couldn't create the playlist." }, "playlist": { "section": "Playlists", diff --git a/frontend/src/i18n/locales/hu/errors.json b/frontend/src/i18n/locales/hu/errors.json index 0971ba85..f6e73b46 100644 --- a/frontend/src/i18n/locales/hu/errors.json +++ b/frontend/src/i18n/locales/hu/errors.json @@ -1,6 +1,7 @@ { "title": "Nem sikerült", "generic": "A szerver nem tudta végrehajtani a műveletet. Próbáld újra.", + "demo": "Ez a bemutató fiókban nem érhető el.", "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.", diff --git a/frontend/src/i18n/locales/hu/plex.json b/frontend/src/i18n/locales/hu/plex.json index 61da094c..f3fd3afc 100644 --- a/frontend/src/i18n/locales/hu/plex.json +++ b/frontend/src/i18n/locales/hu/plex.json @@ -297,7 +297,10 @@ "create": "Létrehozás", "none": "Még nincs lista. Hozz létre egyet fent.", "count": "{{count}} elem", - "groupProgress": "{{in}} / {{size}}" + "groupProgress": "{{in}} / {{size}}", + "addFailed": "Nem sikerült hozzáadni a lejátszási listához.", + "removeFailed": "Nem sikerült eltávolítani a lejátszási listáról.", + "createFailed": "Nem sikerült létrehozni a lejátszási listát." }, "playlist": { "section": "Lejátszási listák", diff --git a/frontend/src/lib/actionErrors.contract.test.ts b/frontend/src/lib/actionErrors.contract.test.ts new file mode 100644 index 00000000..f15de4fe --- /dev/null +++ b/frontend/src/lib/actionErrors.contract.test.ts @@ -0,0 +1,65 @@ +import { readFileSync } from "node:fs"; +import { describe, expect, it } from "vitest"; + +// One contract, one implementation — `notifyActionError` (see actionErrors.ts). +// +// `req` deliberately leaves 403/404/429 to the caller, and the routes behind those statuses +// hand-write English. Round 5 taught the shared helper to discard a plain-string `detail` in favour +// of the caller's already-translated fallback. Round 6 then found `ChatThread` carrying a SECOND, +// hand-written copy of the same contract (`e.detail || t("messages.sendFailed")`) — so the central +// fix could not reach it and the English sentence stayed in a Hungarian UI. A fix that lands in one +// place while a copy of the rule lives in another is this repo's recurring smell; this is the check +// that names the copies. +// +// The assertion pins the expected SET, never a count: a guard that counts is green the moment its +// glob stops matching (E10 review round 4 learned that the hard way). + +// Reads a raw `HttpError.detail` and uses it as the message a user sees. Matches `e.detail ||`, +// `err.detail ? err.detail :` and `(e instanceof HttpError ? e.detail : undefined) ??`. +const RAW_DETAIL = /\b[A-Za-z_$][\w$]*\.detail\b(?!\s*(?:=[^=]|\)))/; + +// Every place that still prefers the server's raw string over a translated fallback. These are +// INLINE surfaces — a form field's error line, a setup step's banner — not toasts, and each shows a +// reason the user is mid-flow on, so converting them is a sweep of its own (with the ~60 backend +// routes that hand-write those strings). Logged on ROADMAP.md under the bug-hunt deferrals. What +// must not happen is a NEW one, or a toast path growing one again. +const KNOWN_RAW_DETAIL_CONSUMERS = [ + "components/Feed.tsx", // the live-YouTube search error line + "components/KeyGate.tsx", // the passphrase field's own refusal + "components/SettingsPanel.tsx", // the change-password field + "components/SetupWizard.tsx", // the first-run wizard's banner + "components/Welcome.tsx", // the sign-in card's refusal panel +]; + +// Where the rule itself lives, plus the request layer that produces and localizes the value. +const IMPLEMENTATION = ["lib/actionErrors.ts", "lib/api/core.ts"]; + +function consumers(): string[] { + const files = import.meta.glob("../**/*.{ts,tsx}", { + query: "?raw", + import: "default", + eager: true, + }); + const hits: string[] = []; + for (const [rel, source] of Object.entries(files)) { + // Vite resolves the glob against THIS file, so same-directory hits come back as "./x". + const path = rel.startsWith("./") ? `lib/${rel.slice(2)}` : rel.replace(/^\.\.\//, ""); + if (/\.test\.tsx?$|\.d\.ts$/.test(path)) continue; + if (IMPLEMENTATION.includes(path)) continue; + const lines = String(source) + .split("\n") + .filter((l) => !/^\s*(\/\/|\*|\/\*)/.test(l)); // a comment about the rule is not a copy of it + if (lines.some((l) => RAW_DETAIL.test(l))) hits.push(path); + } + return hits.sort(); +} + +describe("the refused-write contract", () => { + it("has no copies beyond the ones on the deferral list", () => { + // The glob must actually match something, or this whole file is a green no-op. + expect(readFileSync(new URL("./actionErrors.ts", import.meta.url), "utf8")).toContain( + "detailData", + ); + expect(consumers()).toEqual(KNOWN_RAW_DETAIL_CONSUMERS); + }); +}); diff --git a/frontend/src/lib/actionErrors.test.ts b/frontend/src/lib/actionErrors.test.ts index f46cb78a..1e5f5b22 100644 --- a/frontend/src/lib/actionErrors.test.ts +++ b/frontend/src/lib/actionErrors.test.ts @@ -68,6 +68,26 @@ describe("notifyActionError", () => { expect(notifyMock).toHaveBeenCalledWith({ level: "error", message: "TRANSLATED" }); }); + it("tells a demo user that the demo account is the reason", async () => { + // `require_human`'s refusal was a plain English string, so round 5's fix — discard a + // plain-string detail, show the caller's translated fallback — discarded the only explanation + // the demo user got: a generic "couldn't pin it" with no hint why (round 6). The gate answers + // a structured code now, and `localizeDetail` has a branch for it in both locales. + vi.spyOn(globalThis, "fetch").mockResolvedValue( + new Response(JSON.stringify({ detail: { code: "demo" } }), { + status: 403, + headers: { "content-type": "application/json" }, + }), + ); + const err = await api.addComfortPin("video", "v1").catch((e: unknown) => e); + notifyMock.mockClear(); + notifyActionError(err, "GENERIC FALLBACK"); + expect(notifyMock).toHaveBeenCalledWith({ + level: "error", + message: i18n.t("errors.demo"), + }); + }); + 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. diff --git a/frontend/src/lib/api/core.test.ts b/frontend/src/lib/api/core.test.ts index dc614ed8..fa038bf6 100644 --- a/frontend/src/lib/api/core.test.ts +++ b/frontend/src/lib/api/core.test.ts @@ -22,7 +22,14 @@ vi.mock("../activeAccount", () => ({ notifyActiveAccountChange: vi.fn(), })); -import { req, uploadReq, setUnauthorizedHandler, HttpError, alreadyReported } from "./core"; +import { + req, + uploadReq, + setUnauthorizedHandler, + HttpError, + alreadyReported, + isAborted, +} from "./core"; // A fake XHR whose lifecycle the test drives: capture the instance, set status/responseText, then // invoke onload/onerror/abort by hand. @@ -74,11 +81,15 @@ describe("uploadReq", () => { await expect(p).resolves.toEqual({ id: 7 }); }); + // Both abort producers are asserted through `isAborted`, not through `{status: 0}`: what the + // upload dialog reads is the SENTINEL, and a cancelled upload that stops matching it becomes a + // red "the upload failed" toast. Changing either `reject` to a friendlier message left every + // test green, because the only consumer test hand-builds the error it expects (round 6). it("rejects immediately if the signal is already aborted (never sends)", async () => { const ac = new AbortController(); ac.abort(); const p = uploadReq("/u", form(), { signal: ac.signal }); - await expect(p).rejects.toBeInstanceOf(HttpError); + await expect(p).rejects.toSatisfy(isAborted); expect(FakeXHR.last?.sent).not.toBe(true); }); @@ -86,7 +97,7 @@ describe("uploadReq", () => { const ac = new AbortController(); const p = uploadReq("/u", form(), { signal: ac.signal }); ac.abort(); // → our abort listener calls xhr.abort() → onabort → reject - await expect(p).rejects.toMatchObject({ status: 0 }); + await expect(p).rejects.toSatisfy(isAborted); }); it("calls the unauthorized handler on 401", async () => { diff --git a/frontend/src/lib/api/core.ts b/frontend/src/lib/api/core.ts index a8cce25f..45858f3f 100644 --- a/frontend/src/lib/api/core.ts +++ b/frontend/src/lib/api/core.ts @@ -85,6 +85,10 @@ function localizeDetail(d: unknown): string { // a raw key, which is what an i18n miss would otherwise put on screen. if (o?.code === "login") return t(`welcome.auth.blocked.${o.reason}.short`, t("welcome.auth.genericError")); + // The shared demo account refusing a personal-state write (`require_human`). Not every affordance + // behind that gate is UI-gated on `is_demo` — the comfort shelf is not — so without this the demo + // user got their caller's generic "couldn't do that" and no reason at all (round 6). + if (o?.code === "demo") return t("errors.demo"); // The two email-link confirmations. Their failure is one a user meets by simply clicking an old // link, so it is the last place that may answer in hardcoded English under a bilingual UI. if (o?.code === "verify") return t("welcome.auth.verifyInvalid");