peter/siftlode
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fix(api): a write that hit a connection failure is still reported, and every failure is reported once

Browse Source 
Deep-review round 4, the "one report per failure" contract round 3 introduced.

- `req` marked gateway (502/503/504) and network failures "already reported" because they raise
  the connection notice. But that notice is about connectivity and removes itself on the very next
  answer — which every caller's own resync refetch fetches at once — so a delete that hit a
  keepalive reset flashed the notice and was then reported nowhere, the list still there. Only the
  durable reports (the error dialog, the 401 sign-out) count now; the caller says the action failed.
- A connection notice the user closed swallowed every later one for the rest of the visit; it is
  raised again.
- `req` and `uploadReq` share one failure policy (`reportFailure`). The upload copy had drifted: a
  502 opened the blocking "Server error (502)" dialog and cleared a notice that was still true.
- A taken Plex collection name is a structured 409, localized, and the create/rename calls are
  quiet: it was an English dialog plus a Hungarian toast. `notifyCollectionEditError` is gone.
- The mutation onErrors that toasted unconditionally after a non-quiet request (invite approve /
  deny / add, demo whitelist, audit clear, scheduler, share, full history, edit, config tests, tag
  attach) go through `notifyActionError`; the playlist delete's hand-rolled copy of it too (it takes
  a level now). The user role / suspend / delete actions had no onError at all: a user deleted in
  another tab failed silently and stayed listed — now said, and the list refetched.
This commit is contained in:
peter
2026-09-11 23:27:48 +02:00
parent 6a92585098
commit 6d49da17fe
22 changed files with 328 additions and 137 deletions
Download Patch File Download Diff File Expand all files Collapse all files
frontend/src/lib/api/core.ts
+54 -40
View File
@@ -2,7 +2,7 @@
// error, structured-error localization, the connectivity toast, the per-tab active-account plumbing,
// and the keepalive beacon. Domain modules import from here; nothing here imports a domain module,
// so the runtime graph is a clean barrel → domain → core with no cycles.
import { notify, remove } from "../notifications";
import { getActiveToasts, notify, remove } from "../notifications";
import i18n from "../../i18n";
import { formatBytes } from "../format";
import { reportError } from "../errorDialog";
@@ -14,9 +14,15 @@ import { activeAccountId, ACTIVE_ACCOUNT_KEY, notifyActiveAccountChange } from "
* field nobody ever filled (E9.5 S6 → S7). */
export const uiLang = (): string => i18n.language.split("-")[0] || "en";
// Errors `req` has already put in front of the user — the blocking error dialog, or the connection
// notice. A caller that toasts a failed action as well reported the same fault twice (a modal AND a
// toast); `notifyActionError` asks this instead of each caller filtering statuses by hand.
// Errors `req` has already put in front of the user for good — the blocking error dialog, or the
// sign-out a 401 triggers. A caller that toasts a failed action as well reported the same fault twice
// (a modal AND a toast); `notifyActionError` asks this instead of each caller filtering statuses by
// hand.
//
// NOT the connection notice. It says the server is unreachable, not that this action failed, and it
// removes itself on the next answer — which every caller's own resync refetch fetches at once. Marked
// as reported, a delete that hit a keepalive reset (a 502) flashed the notice for a moment and was
// then reported nowhere at all, the list still there (deep-review round 4).
const reported = new WeakSet<object>();
export function alreadyReported(err: unknown): boolean {
return typeof err === "object" && err !== null && reported.has(err);
@@ -90,6 +96,11 @@ function localizeDetail(d: unknown): string {
// unknown reason falls back to the plain failure rather than printing a key.
if (o?.code === "playlist_move")
return t(`playlists.moveErrors.${o.reason}`, t("playlists.moveFailed"));
// A Plex collection name already taken in that library (the one collection-edit refusal).
if (o?.code === "collection")
return o.reason === "name_taken"
? t("plex.collection.nameTaken")
: t("plex.collection.editFailed");
// A playlist action the server refused (Watch later can't be deleted, YouTube said no, …).
if (o?.code === "playlist") return t(`playlists.errors.${o.reason}`, t("errors.generic"));
// Any row that is gone — deleted in another tab or device, or never the caller's (a stranger's
@@ -109,7 +120,11 @@ function localizeDetail(d: unknown): string {
// bursts of concurrent failures from stacking up — there's only ever one live handle.
let connectivityNotifId: number | null = null;
function markConnectivityLost(): void {
if (connectivityNotifId !== null) return;
// One live notice — but a notice the user closed is not live: it used to swallow every later
// "connection lost" for the rest of the visit.
const id = connectivityNotifId;
if (id !== null && getActiveToasts().some((n) => n.id === id)) return;
if (id !== null) remove(id);
connectivityNotifId = notify({
level: "error",
title: i18n.t("errors.offline.title"),
@@ -129,6 +144,35 @@ function markConnectivityRestored(): void {
const RETRIABLE_GATEWAY = new Set([502, 503, 504]);
const delay = (ms: number) => new Promise((res) => window.setTimeout(res, ms));
// What a failed answer puts on screen — ONE policy for `req` and `uploadReq` (the upload path kept
// its own copy, and it had drifted: a 502 there opened the blocking "Server error" dialog and
// cleared a connection notice that was still true).
//
// 502/503/504 mean the gateway couldn't reach the app (restarting / connection reset), not a
// request the app refused — the soft, self-clearing connection notice rather than a blocking modal,
// and still the caller's to report as a failed action (see `reported`). A genuine 500 (real fault,
// carries a JSON detail) gets the modal, as do 400/409/422 (validation/conflict). 401 is the app
// shell's; 403/404/429 are caller-handled control flow.
function reportFailure(err: HttpError, quiet: boolean): void {
if (RETRIABLE_GATEWAY.has(err.status)) {
markConnectivityLost();
} else if (err.status === 401) {
// Session ended server-side (e.g. the account was suspended or deleted mid-visit). Let the app
// shell decide what to do (drop to the login page if we were signed in). Not a modal — but
// handled all the same, so not the caller's to toast either.
onUnauthorized?.();
reported.add(err);
} else if (quiet) {
/* caller handles the error inline — no global modal */
} else if (err.status >= 500) {
reportError(err.detail || `${i18n.t("errors.server")} (${err.status})`);
reported.add(err);
} else if (err.status === 400 || err.status === 409 || err.status === 422) {
reportError(err.detail);
reported.add(err);
}
}
// `idempotent`: may this request be replayed after a transient gateway/network failure?
// GETs always can; non-GET callers opt in (e.g. the player's progress/state writes, which
// are safe to repeat). Used to recover from the keepalive race without surfacing a 502.
@@ -235,8 +279,7 @@ export async function req<T = unknown>(
await delay(250);
continue;
}
markConnectivityLost();
if (typeof e === "object" && e !== null) reported.add(e);
markConnectivityLost(); // the connection notice; the failed action is still the caller's
throw e;
}
@@ -266,30 +309,8 @@ export async function req<T = unknown>(
} catch {
/* no JSON body */
}
// 502/503/504 mean the gateway couldn't reach the app (restarting / connection reset),
// not a request the app refused — treat them like a network blip with the soft,
// self-clearing toast rather than a blocking modal the user must acknowledge.
// A genuine 500 (real fault, carries a JSON detail) still gets the modal, as do
// 400/409/422 (validation/conflict). 401/403/404 are caller-handled control flow.
const err = new HttpError(r.status, detail, detailData);
if (RETRIABLE_GATEWAY.has(r.status)) {
markConnectivityLost();
reported.add(err);
} else if (r.status === 401) {
// Session ended server-side (e.g. the account was suspended or deleted mid-visit). Let the
// app shell decide what to do (drop to the login page if we were signed in). Not a modal —
// but handled all the same, so not the caller's to toast either.
onUnauthorized?.();
reported.add(err);
} else if (cfg.quiet) {
/* caller handles the error inline — no global modal */
} else if (r.status >= 500) {
reportError(detail || `${i18n.t("errors.server")} (${r.status})`);
reported.add(err);
} else if (r.status === 400 || r.status === 409 || r.status === 422) {
reportError(detail);
reported.add(err);
}
reportFailure(err, !!cfg.quiet);
throw err;
}
// The single audited cast in this layer: the server contract is asserted here, once, so
@@ -325,7 +346,7 @@ export function uploadReq<T = unknown>(
cfg.signal.addEventListener("abort", () => xhr.abort());
}
xhr.onload = () => {
markConnectivityRestored();
if (!RETRIABLE_GATEWAY.has(xhr.status)) markConnectivityRestored(); // as in `req`
if (xhr.status >= 200 && xhr.status < 300) {
try {
resolve((xhr.responseText ? JSON.parse(xhr.responseText) : null) as T);
@@ -347,20 +368,13 @@ export function uploadReq<T = unknown>(
/* no JSON body */
}
const err = new HttpError(xhr.status, detail, detailData);
if (xhr.status === 401) onUnauthorized?.();
else if (xhr.status >= 500)
reportError(detail || `${i18n.t("errors.server")} (${xhr.status})`);
else if (xhr.status === 400 || xhr.status === 409 || xhr.status === 422) reportError(detail);
if (xhr.status === 401 || xhr.status >= 500 || [400, 409, 422].includes(xhr.status))
reported.add(err); // as in `req`: already on screen, not the caller's to toast again
reportFailure(err, false);
reject(err);
};
// A network-level failure (server unreachable) — same soft "connection lost" status as `req`.
xhr.onerror = () => {
markConnectivityLost();
const err = new HttpError(0, i18n.t("errors.offline.body"));
reported.add(err);
reject(err);
reject(new HttpError(0, i18n.t("errors.offline.body")));
};
xhr.onabort = () => reject(new HttpError(0, "aborted"));
xhr.send(form);