fix(api,ui): a refused write says why, wherever the refusal happens
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.
This commit is contained in:
+5
-1
@@ -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
|
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."""
|
where there's no scope check to lean on."""
|
||||||
if user.is_demo:
|
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
|
return user
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@@ -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
|
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."""
|
in somewhere" / a suspected session compromise. The demo account (shared) is excluded."""
|
||||||
if user.is_demo:
|
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
|
# 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).
|
# session at a newer epoch than the DB (see set_password).
|
||||||
bump_session_epoch(user)
|
bump_session_epoch(user)
|
||||||
@@ -1293,7 +1293,7 @@ def set_password(
|
|||||||
account) needs no current password — the session already proves identity. Changing an
|
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."""
|
existing one requires the current password, so a hijacked session can't silently rotate it."""
|
||||||
if user.is_demo:
|
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 ""
|
new_password = body.password or ""
|
||||||
if len(new_password) < PASSWORD_MIN_LEN:
|
if len(new_password) < PASSWORD_MIN_LEN:
|
||||||
raise HTTPException(
|
raise HTTPException(
|
||||||
|
|||||||
@@ -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
|
||||||
@@ -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.
|
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):
|
def test_a_gone_view_tag_or_pin_says_what_is_gone(db, make_user, as_user):
|
||||||
client = as_user(make_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
|
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
|
"""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
|
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
|
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")
|
admin = make_user(role="admin")
|
||||||
client = as_user(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"
|
gone = "UCgone000000000000000000"
|
||||||
# Subscribe is behind the write-scope gate, so it is covered by the tags route below instead.
|
|
||||||
for method, url in [
|
for method, url in [
|
||||||
("get", f"/api/channels/{gone}"),
|
("get", f"/api/channels/{gone}"),
|
||||||
|
("post", f"/api/channels/{gone}/explore"),
|
||||||
("post", f"/api/channels/{gone}/reset-backfill"),
|
("post", f"/api/channels/{gone}/reset-backfill"),
|
||||||
|
("post", f"/api/channels/{gone}/subscribe"),
|
||||||
("post", f"/api/channels/{gone}/block"),
|
("post", f"/api/channels/{gone}/block"),
|
||||||
]:
|
]:
|
||||||
r = getattr(client, method)(url)
|
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):
|
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
|
"""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
|
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")
|
admin = make_user(role="admin")
|
||||||
client = as_user(admin)
|
client = as_user(admin)
|
||||||
r = client.post("/api/channels/UCgone000000000000000000/tags", json={"tag_id": 1})
|
r = client.post("/api/channels/UCgone000000000000000000/tags", json={"tag_id": 1})
|
||||||
assert r.status_code == 404, r.text
|
assert r.status_code == 404, r.text
|
||||||
assert r.json()["detail"] == {"code": "not_found", "reason": "subscription"}
|
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.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):
|
def test_a_nul_in_a_request_string_is_a_422_not_a_500(db, make_user, as_user):
|
||||||
|
|||||||
@@ -3,13 +3,13 @@ import { qk } from "../lib/queryKeys";
|
|||||||
import { useTranslation } from "react-i18next";
|
import { useTranslation } from "react-i18next";
|
||||||
import { useMutation, useQueryClient } from "@tanstack/react-query";
|
import { useMutation, useQueryClient } from "@tanstack/react-query";
|
||||||
import { Send } from "lucide-react";
|
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 { useLiveQuery } from "../lib/useLiveQuery";
|
||||||
import { useScrollFade } from "../lib/useScrollFade";
|
import { useScrollFade } from "../lib/useScrollFade";
|
||||||
import { Button } from "./ui/Button";
|
import { Button } from "./ui/Button";
|
||||||
import { LoadingState, StateMessage } from "./ui/QueryState";
|
import { LoadingState, StateMessage } from "./ui/QueryState";
|
||||||
import { relativeTime } from "../lib/format";
|
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 { partnerPub, POLL_MS, refreshPartnerPub, useKeyState } from "../lib/messaging";
|
||||||
import * as e2ee from "../lib/e2ee";
|
import * as e2ee from "../lib/e2ee";
|
||||||
import KeyGate from "./KeyGate";
|
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
|
// 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-
|
// 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.
|
// 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"));
|
||||||
},
|
},
|
||||||
});
|
});
|
||||||
|
|
||||||
|
|||||||
@@ -27,6 +27,7 @@ vi.mock("../lib/notifications", async (importOriginal) => ({
|
|||||||
|
|
||||||
import PlexPlaylistView from "./PlexPlaylistView";
|
import PlexPlaylistView from "./PlexPlaylistView";
|
||||||
import AddToPlaylist from "./AddToPlaylist";
|
import AddToPlaylist from "./AddToPlaylist";
|
||||||
|
import PlexPlaylistAdd from "./PlexPlaylistAdd";
|
||||||
import TagManager from "./TagManager";
|
import TagManager from "./TagManager";
|
||||||
import { api, HttpError, type PlexCard } from "../lib/api";
|
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(errors()).toEqual(["gone playlist"]));
|
||||||
await waitFor(() => expect(lists.mock.calls.length).toBeGreaterThan(1));
|
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(
|
||||||
|
<QueryClientProvider client={client()}>
|
||||||
|
<PlexPlaylistAdd
|
||||||
|
target={{ kind: "single", ratingKey: "m1", title: "A Movie" }}
|
||||||
|
onClose={() => {}}
|
||||||
|
/>
|
||||||
|
</QueryClientProvider>,
|
||||||
|
);
|
||||||
|
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", () => {
|
describe("the tag manager", () => {
|
||||||
|
|||||||
@@ -4,6 +4,7 @@ import { useTranslation } from "react-i18next";
|
|||||||
import { useQuery, useQueryClient } from "@tanstack/react-query";
|
import { useQuery, useQueryClient } from "@tanstack/react-query";
|
||||||
import { Check, Minus, Plus } from "lucide-react";
|
import { Check, Minus, Plus } from "lucide-react";
|
||||||
import { api, type PlexPlaylist } from "../lib/api";
|
import { api, type PlexPlaylist } from "../lib/api";
|
||||||
|
import { notifyActionError } from "../lib/actionErrors";
|
||||||
import { useScrollFade } from "../lib/useScrollFade";
|
import { useScrollFade } from "../lib/useScrollFade";
|
||||||
import { Button } from "./ui/Button";
|
import { Button } from "./ui/Button";
|
||||||
import Modal from "./ui/Modal";
|
import Modal from "./ui/Modal";
|
||||||
@@ -54,6 +55,11 @@ export default function PlexPlaylistAdd({
|
|||||||
return isGroup ? (p.group_in ?? 0) : p.has_item ? 1 : 0;
|
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) {
|
async function toggle(p: PlexPlaylist) {
|
||||||
if (busy) return;
|
if (busy) return;
|
||||||
setBusy(true);
|
setBusy(true);
|
||||||
@@ -67,8 +73,13 @@ export default function PlexPlaylistAdd({
|
|||||||
else await api.plexPlaylistAddItem(p.id, target.ratingKey);
|
else await api.plexPlaylistAddItem(p.id, target.ratingKey);
|
||||||
}
|
}
|
||||||
setOverride((m) => new Map(m).set(p.id, allIn ? 0 : size));
|
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 {
|
} finally {
|
||||||
|
refresh();
|
||||||
setBusy(false);
|
setBusy(false);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -87,8 +98,12 @@ export default function PlexPlaylistAdd({
|
|||||||
setOverride((m) => new Map(m).set(pl.id, 1));
|
setOverride((m) => new Map(m).set(pl.id, 1));
|
||||||
}
|
}
|
||||||
setNewName("");
|
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 {
|
} finally {
|
||||||
|
refresh();
|
||||||
setBusy(false);
|
setBusy(false);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -1,6 +1,7 @@
|
|||||||
{
|
{
|
||||||
"title": "Couldn't complete that",
|
"title": "Couldn't complete that",
|
||||||
"generic": "The server couldn't carry out that action. Please try again.",
|
"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.",
|
"validation": "Some of the values you entered weren't valid. Please check them and try again.",
|
||||||
"server": "Server error",
|
"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.",
|
"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.",
|
||||||
|
|||||||
@@ -297,7 +297,10 @@
|
|||||||
"create": "Create",
|
"create": "Create",
|
||||||
"none": "No playlists yet. Create one above.",
|
"none": "No playlists yet. Create one above.",
|
||||||
"count": "{{count}} items",
|
"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": {
|
"playlist": {
|
||||||
"section": "Playlists",
|
"section": "Playlists",
|
||||||
|
|||||||
@@ -1,6 +1,7 @@
|
|||||||
{
|
{
|
||||||
"title": "Nem sikerült",
|
"title": "Nem sikerült",
|
||||||
"generic": "A szerver nem tudta végrehajtani a műveletet. Próbáld újra.",
|
"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.",
|
"validation": "Néhány megadott érték érvénytelen volt. Ellenőrizd, majd próbáld újra.",
|
||||||
"server": "Szerverhiba",
|
"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.",
|
"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.",
|
||||||
|
|||||||
@@ -297,7 +297,10 @@
|
|||||||
"create": "Létrehozás",
|
"create": "Létrehozás",
|
||||||
"none": "Még nincs lista. Hozz létre egyet fent.",
|
"none": "Még nincs lista. Hozz létre egyet fent.",
|
||||||
"count": "{{count}} elem",
|
"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": {
|
"playlist": {
|
||||||
"section": "Lejátszási listák",
|
"section": "Lejátszási listák",
|
||||||
|
|||||||
@@ -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);
|
||||||
|
});
|
||||||
|
});
|
||||||
@@ -68,6 +68,26 @@ describe("notifyActionError", () => {
|
|||||||
expect(notifyMock).toHaveBeenCalledWith({ level: "error", message: "TRANSLATED" });
|
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", () => {
|
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
|
// `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.
|
// AND more specific than the caller's generic fallback — quota numbers, which row is gone.
|
||||||
|
|||||||
@@ -22,7 +22,14 @@ vi.mock("../activeAccount", () => ({
|
|||||||
notifyActiveAccountChange: vi.fn(),
|
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
|
// A fake XHR whose lifecycle the test drives: capture the instance, set status/responseText, then
|
||||||
// invoke onload/onerror/abort by hand.
|
// invoke onload/onerror/abort by hand.
|
||||||
@@ -74,11 +81,15 @@ describe("uploadReq", () => {
|
|||||||
await expect(p).resolves.toEqual({ id: 7 });
|
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 () => {
|
it("rejects immediately if the signal is already aborted (never sends)", async () => {
|
||||||
const ac = new AbortController();
|
const ac = new AbortController();
|
||||||
ac.abort();
|
ac.abort();
|
||||||
const p = uploadReq("/u", form(), { signal: ac.signal });
|
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);
|
expect(FakeXHR.last?.sent).not.toBe(true);
|
||||||
});
|
});
|
||||||
|
|
||||||
@@ -86,7 +97,7 @@ describe("uploadReq", () => {
|
|||||||
const ac = new AbortController();
|
const ac = new AbortController();
|
||||||
const p = uploadReq("/u", form(), { signal: ac.signal });
|
const p = uploadReq("/u", form(), { signal: ac.signal });
|
||||||
ac.abort(); // → our abort listener calls xhr.abort() → onabort → reject
|
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 () => {
|
it("calls the unauthorized handler on 401", async () => {
|
||||||
|
|||||||
@@ -85,6 +85,10 @@ function localizeDetail(d: unknown): string {
|
|||||||
// a raw key, which is what an i18n miss would otherwise put on screen.
|
// a raw key, which is what an i18n miss would otherwise put on screen.
|
||||||
if (o?.code === "login")
|
if (o?.code === "login")
|
||||||
return t(`welcome.auth.blocked.${o.reason}.short`, t("welcome.auth.genericError"));
|
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
|
// 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.
|
// 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");
|
if (o?.code === "verify") return t("welcome.auth.verifyInvalid");
|
||||||
|
|||||||
Reference in New Issue
Block a user