fix(api): a refused write is named, in the reader's language
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.
This commit is contained in:
@@ -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.",
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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 });
|
||||
}
|
||||
|
||||
@@ -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<T = unknown>(
|
||||
};
|
||||
}
|
||||
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<T = unknown>(
|
||||
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);
|
||||
});
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user