fix(ui): a write that fails says what failed and why, wherever it fails
`req` opens its blocking dialog for 400/409/422/5xx only. A 403/404/429 was left to the caller and a 502-504 or lost connection got just the self-clearing connection notice, so 34 mutations without a reporting onError and 15 direct api writes with a bare or missing catch failed in silence: a hidden channel that snapped back, a share link that never revoked, a confirmed account delete that ended with nothing said. - lib/queryClient: one MutationCache reporter. react-query awaits it BEFORE the mutation's own onError, so it waits a task and skips a failure the site already reported; the toast title comes from `meta.failTitle`, `meta.errorShownInline` opts out. - notifyActionError: title names WHAT failed, the line under it says WHY (`failureReason`: a structured localized detail, else the status). It marks the error reported (`markReported`), so nothing reports one failure twice; `notifyKnownFailure` for a site's own specific sentence. - Every silent site named (45 new HU/EN titles, 4 reasons). Stale "the global dialog shows it" comments corrected on the job actions, channel block, Plex sync, cookies, download enqueue, account delete and usePref. - Channels passed the tag hook's channel id to the reporter as the error; the tag and priority hooks now report themselves, with the real error. - A contract test keeps every useMutation naming its failure; CLAUDE.md gains the pattern row. - history.dom.test: the "navigates only once the entries are back" assertion raced an async act's macrotask flush (1 in 5 runs); it now asserts from a later popstate listener.
This commit is contained in:
@@ -8,9 +8,10 @@ vi.mock("./notifications", () => ({ notify: vi.fn(() => 0) }));
|
||||
vi.mock("./errorDialog", () => ({ reportError: vi.fn() }));
|
||||
|
||||
import { HttpError, api } from "./api";
|
||||
import { alreadyReported } from "./api/core";
|
||||
import { notify } from "./notifications";
|
||||
import { reportError } from "./errorDialog";
|
||||
import { notifyActionError } from "./actionErrors";
|
||||
import { failureReason, notifyActionError, notifyKnownFailure } from "./actionErrors";
|
||||
import i18n from "../i18n";
|
||||
|
||||
const notifyMock = vi.mocked(notify);
|
||||
@@ -33,7 +34,7 @@ describe("a Plex collection name that is already taken", () => {
|
||||
notifyMock.mockClear();
|
||||
notifyActionError(err, "EDIT_FAILED");
|
||||
expect(notifyMock.mock.calls).toEqual([
|
||||
[{ level: "error", message: i18n.t("plex.collection.nameTaken") }],
|
||||
[{ level: "error", title: "EDIT_FAILED", message: i18n.t("plex.collection.nameTaken") }],
|
||||
]);
|
||||
expect(reportError).not.toHaveBeenCalled();
|
||||
});
|
||||
@@ -60,12 +61,30 @@ describe("notifyActionError", () => {
|
||||
expect(notifyMock).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("reports one failure once, however many callers try", () => {
|
||||
// What keeps the app-wide mutation reporter quiet for a mutation whose own onError already spoke.
|
||||
const err = new HttpError(404, "gone");
|
||||
notifyActionError(err, "FIRST");
|
||||
notifyActionError(err, "SECOND");
|
||||
expect(notifyMock).toHaveBeenCalledTimes(1);
|
||||
expect(alreadyReported(err)).toBe(true);
|
||||
});
|
||||
|
||||
it("says nothing about an upload the user cancelled", () => {
|
||||
notifyActionError(new HttpError(0, "aborted"), "FAILED");
|
||||
expect(notifyMock).not.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" });
|
||||
expect(notifyMock).toHaveBeenCalledWith({
|
||||
level: "error",
|
||||
title: "TRANSLATED",
|
||||
message: i18n.t("errors.notFound.generic"),
|
||||
});
|
||||
});
|
||||
|
||||
it("tells a demo user that the demo account is the reason", async () => {
|
||||
@@ -84,13 +103,14 @@ describe("notifyActionError", () => {
|
||||
notifyActionError(err, "GENERIC FALLBACK");
|
||||
expect(notifyMock).toHaveBeenCalledWith({
|
||||
level: "error",
|
||||
title: "GENERIC FALLBACK",
|
||||
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.
|
||||
// AND more specific than anything a status can say — 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",
|
||||
@@ -98,7 +118,54 @@ describe("notifyActionError", () => {
|
||||
notifyActionError(err, "TRANSLATED FALLBACK");
|
||||
expect(notifyMock).toHaveBeenCalledWith({
|
||||
level: "error",
|
||||
title: "TRANSLATED FALLBACK",
|
||||
message: "Ez a csatorna már nincs a könyvtárban.",
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe("failureReason — why a write failed, when the server did not say", () => {
|
||||
it.each([
|
||||
[403, "errors.reason.forbidden"],
|
||||
[404, "errors.notFound.generic"],
|
||||
[429, "errors.reason.rateLimited"],
|
||||
[502, "errors.reason.unreachable"],
|
||||
[503, "errors.reason.unreachable"],
|
||||
[504, "errors.reason.unreachable"],
|
||||
[0, "errors.reason.unreachable"], // uploadReq's network failure
|
||||
[500, "errors.generic"],
|
||||
])("a %s reads %s", (status, key) => {
|
||||
expect(failureReason(new HttpError(status, "hand-written English"))).toBe(i18n.t(key));
|
||||
});
|
||||
|
||||
it("a request that never reached the server reads as unreachable", () => {
|
||||
expect(failureReason(new TypeError("Failed to fetch"))).toBe(
|
||||
i18n.t("errors.reason.unreachable"),
|
||||
);
|
||||
});
|
||||
|
||||
it("anything else is the app's own fault, not the server's", () => {
|
||||
expect(failureReason(new Error("key import failed"))).toBe(i18n.t("errors.reason.unexpected"));
|
||||
});
|
||||
|
||||
it("every reason exists in both languages", () => {
|
||||
for (const lng of ["en", "hu"])
|
||||
for (const key of [
|
||||
"errors.reason.forbidden",
|
||||
"errors.reason.rateLimited",
|
||||
"errors.reason.unreachable",
|
||||
"errors.reason.unexpected",
|
||||
])
|
||||
expect(i18n.exists(key, { lng }), `${lng}:${key}`).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
describe("notifyKnownFailure", () => {
|
||||
it("shows the caller's specific sentence and marks the failure reported", () => {
|
||||
const err = new HttpError(409, "x", { code: "resend", reason: "email_off" });
|
||||
notifyKnownFailure(err, "EMAIL IS OFF");
|
||||
expect(notifyMock).toHaveBeenCalledWith({ level: "error", message: "EMAIL IS OFF" });
|
||||
notifyActionError(err, "GENERIC");
|
||||
expect(notifyMock).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1,28 +1,62 @@
|
||||
// From the core, not the `./api` barrel: these two are pure (no request), and a test that mocks the
|
||||
// From the core, not the `./api` barrel: these are pure (no request), and a test that mocks the
|
||||
// barrel wholesale to stub its requests must not take the error path's own plumbing with it.
|
||||
import { alreadyReported, HttpError } from "./api/core";
|
||||
import { alreadyReported, HttpError, isAborted, markReported } from "./api/core";
|
||||
import { notify } from "./notifications";
|
||||
import i18n from "../i18n";
|
||||
|
||||
const GATEWAY = new Set([502, 503, 504]);
|
||||
|
||||
/**
|
||||
* Surface a failed write action as an error toast instead of the click silently doing nothing.
|
||||
* 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.
|
||||
* WHY a write failed, in the reader's language — the line under the toast's title.
|
||||
*
|
||||
* A STRUCTURED server detail (`{code, reason}`) wins: `req` has already run it through
|
||||
* `localizeDetail`, and it is the most specific thing there is to say (quota numbers, which row is
|
||||
* gone). A plain-string `detail` never does — it is whatever English the route hand-wrote, and
|
||||
* `reportFailure` leaves exactly 403/404/429 to the caller, so those used to put "No user with that
|
||||
* email." into a Hungarian UI (deep-review round 5). Otherwise the status says it.
|
||||
*/
|
||||
export function failureReason(err: unknown): string {
|
||||
const t = i18n.t.bind(i18n);
|
||||
if (err instanceof HttpError) {
|
||||
if (err.detailData != null && err.detail) return err.detail;
|
||||
// 0: `uploadReq`'s network failure. 502–504: the gateway could not reach the app. Either way the
|
||||
// change did not go through, which the connection notice alone does not say.
|
||||
if (err.status === 0 || GATEWAY.has(err.status)) return t("errors.reason.unreachable");
|
||||
if (err.status === 403) return t("errors.reason.forbidden");
|
||||
if (err.status === 404) return t("errors.notFound.generic");
|
||||
if (err.status === 429) return t("errors.reason.rateLimited");
|
||||
return t("errors.generic");
|
||||
}
|
||||
// `fetch` rejects with a TypeError when the request never reached the server.
|
||||
if (err instanceof TypeError) return t("errors.reason.unreachable");
|
||||
return t("errors.reason.unexpected");
|
||||
}
|
||||
|
||||
/**
|
||||
* Surface a failed write as an error toast instead of the click silently doing nothing: the title
|
||||
* names WHAT failed (the caller's translated "Couldn't hide the channel"), the line under it says
|
||||
* WHY (`failureReason`). Domain-agnostic — the one way every action failure is reported, including
|
||||
* the app-wide mutation reporter in lib/queryClient.
|
||||
*/
|
||||
export function notifyActionError(
|
||||
err: unknown,
|
||||
fallbackMessage: string,
|
||||
title: string,
|
||||
level: "error" | "warning" = "error",
|
||||
): void {
|
||||
// Already on screen (`req` opened the error dialog, or the tab is being signed out): a toast on
|
||||
// top reported one fault twice. Playlists filtered this by status by hand; the watched toggles,
|
||||
// the saved views and a playlist delete did not.
|
||||
// Already on screen (`req` opened the error dialog, the tab is being signed out, or a caller's own
|
||||
// `onError` got there first): a second toast reported one fault twice. A cancel is not a failure.
|
||||
if (alreadyReported(err) || isAborted(err)) return;
|
||||
notify({ level, title, message: failureReason(err) });
|
||||
markReported(err);
|
||||
}
|
||||
|
||||
/**
|
||||
* A failure the caller has a SPECIFIC sentence for, because it read the error's own code ("this
|
||||
* instance cannot send email", "someone registered on that address") — shown as it is, and marked,
|
||||
* so the app-wide mutation reporter does not put a second, generic toast next to it.
|
||||
*/
|
||||
export function notifyKnownFailure(err: unknown, message: string): void {
|
||||
if (alreadyReported(err)) return;
|
||||
const localized = err instanceof HttpError && err.detailData != null ? err.detail : null;
|
||||
notify({ level, message: localized || fallbackMessage });
|
||||
notify({ level: "error", message });
|
||||
markReported(err);
|
||||
}
|
||||
|
||||
@@ -23,10 +23,18 @@ export const uiLang = (): string => i18n.language.split("-")[0] || "en";
|
||||
// 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).
|
||||
//
|
||||
// A caller that reports a failure itself marks it too (`notifyActionError` does), which is what lets
|
||||
// the ONE app-wide mutation reporter (lib/queryClient) stay quiet for a mutation whose own `onError`
|
||||
// already said something, and speak for every one that did not.
|
||||
const reported = new WeakSet<object>();
|
||||
export function alreadyReported(err: unknown): boolean {
|
||||
return typeof err === "object" && err !== null && reported.has(err);
|
||||
}
|
||||
/** Record that this failure is on screen now, so nothing reports it a second time. */
|
||||
export function markReported(err: unknown): void {
|
||||
if (typeof err === "object" && err !== null) reported.add(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
|
||||
|
||||
@@ -0,0 +1,83 @@
|
||||
// @vitest-environment jsdom
|
||||
//
|
||||
// The two shared channel write flows report their own failures, with the real error (E11 S2b).
|
||||
// Before: the channel detail page's tag toggle and both pages' priority stepper only refetched, so a
|
||||
// refused write just snapped back; and the manager reported a failed tag toggle by handing the
|
||||
// CHANNEL ID to the reporter as though it were the error.
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
|
||||
import type { ReactNode } from "react";
|
||||
import { act, renderHook, waitFor } from "@testing-library/react";
|
||||
import { QueryClient, QueryClientProvider } from "@tanstack/react-query";
|
||||
|
||||
vi.mock("./notifications", () => ({ notify: vi.fn(() => 0) }));
|
||||
|
||||
import { api, HttpError } from "./api";
|
||||
import { notify } from "./notifications";
|
||||
import { useChannelTagActions } from "./useChannelTagActions";
|
||||
import { useChannelPriority } from "./useChannelPriority";
|
||||
import i18n from "../i18n";
|
||||
|
||||
const notifyMock = vi.mocked(notify);
|
||||
beforeEach(() => notifyMock.mockClear());
|
||||
afterEach(() => vi.restoreAllMocks());
|
||||
|
||||
const wrapper = ({ children }: { children: ReactNode }) => (
|
||||
<QueryClientProvider client={new QueryClient()}>{children}</QueryClientProvider>
|
||||
);
|
||||
|
||||
describe("useChannelTagActions", () => {
|
||||
it("names a refused tag toggle with its real reason, then lets the caller revert", async () => {
|
||||
vi.spyOn(api, "attachChannelTag").mockRejectedValue(new HttpError(403, "nope"));
|
||||
const onError = vi.fn();
|
||||
const { result } = renderHook(
|
||||
() => useChannelTagActions({ optimisticToggle: () => {}, onError }),
|
||||
{ wrapper },
|
||||
);
|
||||
act(() => result.current.toggleTag("UC1", 7, false));
|
||||
await waitFor(() => expect(onError).toHaveBeenCalledWith("UC1"));
|
||||
expect(notifyMock).toHaveBeenCalledWith({
|
||||
level: "error",
|
||||
title: i18n.t("channels.tags.updateFailed"),
|
||||
message: i18n.t("errors.reason.forbidden"),
|
||||
});
|
||||
});
|
||||
|
||||
it("names a refused create-and-attach", async () => {
|
||||
vi.spyOn(api, "createTag").mockRejectedValue(new HttpError(429, "slow"));
|
||||
const { result } = renderHook(
|
||||
() => useChannelTagActions({ optimisticToggle: () => {}, onError: () => {} }),
|
||||
{ wrapper },
|
||||
);
|
||||
act(() => result.current.createAndAttachTag("UC1", "Music"));
|
||||
await waitFor(() =>
|
||||
expect(notifyMock).toHaveBeenCalledWith({
|
||||
level: "error",
|
||||
title: i18n.t("channels.tags.createFailed"),
|
||||
message: i18n.t("errors.reason.rateLimited"),
|
||||
}),
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe("useChannelPriority", () => {
|
||||
it("names a refused priority save, then lets the caller revert", async () => {
|
||||
vi.spyOn(api, "updateChannel").mockRejectedValue(new TypeError("Failed to fetch"));
|
||||
const onError = vi.fn();
|
||||
let priority = 1;
|
||||
const { result } = renderHook(() =>
|
||||
useChannelPriority({
|
||||
read: () => priority,
|
||||
optimistic: (_id, next) => void (priority = next),
|
||||
onError,
|
||||
delay: 0,
|
||||
}),
|
||||
);
|
||||
act(() => result.current.bumpPriority("UC1", 1));
|
||||
await waitFor(() => expect(onError).toHaveBeenCalledWith("UC1"));
|
||||
expect(notifyMock).toHaveBeenCalledWith({
|
||||
level: "error",
|
||||
title: i18n.t("errors.writeFailed.channelPriority"),
|
||||
message: i18n.t("errors.reason.unreachable"),
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -139,13 +139,21 @@ describe("closeOverlaysThenNavigate", () => {
|
||||
expect(closed).toEqual(["palette", "player"]);
|
||||
expect(navigate).not.toHaveBeenCalled();
|
||||
|
||||
await settle(); // the reconcile reclaims both entries in one go
|
||||
expect(back).toHaveBeenCalledTimes(2);
|
||||
// Not inside the popstate dispatch that settled it — a later listener would still apply the
|
||||
// OLD entry's page over the push.
|
||||
expect(navigate).not.toHaveBeenCalled();
|
||||
|
||||
await tick();
|
||||
// OLD entry's page over the push. Asserted FROM such a later listener (registered after the
|
||||
// helper's own), not after `settle()`: that async act can flush the unmounts on a macrotask,
|
||||
// and the navigate task sometimes ran first — a 1-in-5 false failure (E11 S2b gate).
|
||||
const navigatedByPopstate: number[] = [];
|
||||
const laterListener = () => navigatedByPopstate.push(navigate.mock.calls.length);
|
||||
window.addEventListener("popstate", laterListener);
|
||||
try {
|
||||
await settle(); // the reconcile reclaims both entries in one go
|
||||
expect(back).toHaveBeenCalledTimes(2);
|
||||
await tick();
|
||||
} finally {
|
||||
window.removeEventListener("popstate", laterListener);
|
||||
}
|
||||
expect(navigatedByPopstate).toEqual([0, 0]);
|
||||
expect(navigate).toHaveBeenCalledTimes(1);
|
||||
await tick();
|
||||
expect(back).toHaveBeenCalledTimes(2); // nothing reclaims over the entry navigate pushed
|
||||
|
||||
@@ -0,0 +1,77 @@
|
||||
import { describe, expect, it } from "vitest";
|
||||
|
||||
// Every `useMutation` NAMES what failed (E11 S2b, owner decision: the toast's title says what, the
|
||||
// line under it why). The app-wide reporter (lib/queryClient) reports a mutation that names nothing
|
||||
// too — so nothing is silent any more — but under the generic "Couldn't complete that", which is the
|
||||
// very toast that says nothing about what it is about when it lands a few seconds after the click.
|
||||
//
|
||||
// A mutation passes when it carries one of:
|
||||
// - `meta: { failTitle }` the reporter's title for it,
|
||||
// - `meta: { errorShownInline }` the page itself renders the failure,
|
||||
// - an `onError` which reports (or rolls back — the reporter then speaks for it,
|
||||
// under the title from `meta` if there is one).
|
||||
//
|
||||
// The assertion pins the SET of unnamed mutations, never a count, and checks the glob matched at
|
||||
// all: a guard whose glob stops matching is green over nothing (E10 review round 4, round 7).
|
||||
const NAMES = /\bfailTitle\b|\berrorShownInline\b|\bonError\b/;
|
||||
|
||||
/** The source of each `useMutation(...)` call, found by paren balance with comments stripped —
|
||||
* a comment with a stray "(" would otherwise run one block into the next one's `failTitle`. */
|
||||
export function mutationCalls(source: string): string[] {
|
||||
const code = source.replace(/\/\*[\s\S]*?\*\//g, "").replace(/(^|[^:"'`])\/\/.*$/gm, "$1");
|
||||
const calls: string[] = [];
|
||||
let from = 0;
|
||||
for (;;) {
|
||||
const at = code.indexOf("useMutation(", from);
|
||||
if (at < 0) return calls;
|
||||
let depth = 0;
|
||||
let i = at + "useMutation".length;
|
||||
for (; i < code.length; i++) {
|
||||
if (code[i] === "(") depth++;
|
||||
else if (code[i] === ")" && --depth === 0) break;
|
||||
}
|
||||
calls.push(code.slice(at, i + 1));
|
||||
from = i + 1;
|
||||
}
|
||||
}
|
||||
|
||||
function unnamed(): { hits: string[]; mutations: number } {
|
||||
const files = import.meta.glob("../**/*.{ts,tsx}", {
|
||||
query: "?raw",
|
||||
import: "default",
|
||||
eager: true,
|
||||
});
|
||||
const hits: string[] = [];
|
||||
let mutations = 0;
|
||||
for (const [rel, source] of Object.entries(files)) {
|
||||
const path = rel.startsWith("./") ? `lib/${rel.slice(2)}` : rel.replace(/^\.\.\//, "");
|
||||
if (/\.test\.tsx?$|\.d\.ts$/.test(path)) continue;
|
||||
mutationCalls(String(source)).forEach((call, n) => {
|
||||
mutations++;
|
||||
if (!NAMES.test(call)) hits.push(`${path}#${n + 1}`);
|
||||
});
|
||||
}
|
||||
return { hits: hits.sort(), mutations };
|
||||
}
|
||||
|
||||
describe("the failed-write contract", () => {
|
||||
it("has no mutation that leaves its failure unnamed", () => {
|
||||
const { hits, mutations } = unnamed();
|
||||
expect(mutations).toBeGreaterThan(50);
|
||||
expect(hits).toEqual([]);
|
||||
});
|
||||
|
||||
it("reads a mutation call to its end, whatever its body holds", () => {
|
||||
const src = `
|
||||
const a = useMutation({
|
||||
mutationFn: () => api.x(fn(1), (2)),
|
||||
// a stray "(" in a comment must not swallow the next call
|
||||
});
|
||||
const b = useMutation({ mutationFn: api.y, meta: { failTitle: "k" } });
|
||||
`;
|
||||
const calls = mutationCalls(src);
|
||||
expect(calls).toHaveLength(2);
|
||||
expect(NAMES.test(calls[0] ?? "")).toBe(false);
|
||||
expect(NAMES.test(calls[1] ?? "")).toBe(true);
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,104 @@
|
||||
// @vitest-environment jsdom
|
||||
//
|
||||
// The ONE app-wide reporter for a failed mutation (E11 S2b). `req` shows its blocking dialog for
|
||||
// 400/409/422/5xx only, so before this every mutation without a reporting `onError` failed in
|
||||
// silence on a 403/404/429 or a lost connection — 34 of them.
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
|
||||
|
||||
vi.mock("./notifications", () => ({ notify: vi.fn(() => 0) }));
|
||||
vi.mock("./errorDialog", () => ({ reportError: vi.fn() }));
|
||||
|
||||
import { MutationObserver, type MutationObserverOptions } from "@tanstack/react-query";
|
||||
import { createQueryClient } from "./queryClient";
|
||||
import { HttpError, req } from "./api/core";
|
||||
import { notify } from "./notifications";
|
||||
import { reportError } from "./errorDialog";
|
||||
import { notifyActionError } from "./actionErrors";
|
||||
import i18n from "../i18n";
|
||||
|
||||
const notifyMock = vi.mocked(notify);
|
||||
beforeEach(() => {
|
||||
notifyMock.mockClear();
|
||||
vi.mocked(reportError).mockClear();
|
||||
});
|
||||
afterEach(() => vi.restoreAllMocks());
|
||||
|
||||
async function fail(options: MutationObserverOptions<unknown, unknown, void>): Promise<void> {
|
||||
const observer = new MutationObserver(createQueryClient(), options);
|
||||
await observer.mutate().catch(() => {});
|
||||
// The reporter waits a task for the mutation's own onError; give it that task.
|
||||
await new Promise((r) => setTimeout(r, 5));
|
||||
}
|
||||
|
||||
const TITLE = "errors.writeFailed.hideChannel";
|
||||
|
||||
describe("a failed mutation", () => {
|
||||
it("is named, with the reason under it, when the mutation did not report it itself", async () => {
|
||||
await fail({
|
||||
mutationFn: () => Promise.reject(new HttpError(404, "Not subscribed")),
|
||||
meta: { failTitle: TITLE },
|
||||
});
|
||||
expect(notifyMock.mock.calls).toEqual([
|
||||
[{ level: "error", title: i18n.t(TITLE), message: i18n.t("errors.notFound.generic") }],
|
||||
]);
|
||||
});
|
||||
|
||||
it("is still reported under the generic title when the mutation names nothing", async () => {
|
||||
await fail({ mutationFn: () => Promise.reject(new HttpError(429, "slow down")) });
|
||||
expect(notifyMock).toHaveBeenCalledWith({
|
||||
level: "error",
|
||||
title: i18n.t("errors.title"),
|
||||
message: i18n.t("errors.reason.rateLimited"),
|
||||
});
|
||||
});
|
||||
|
||||
it("says a lost connection is why the change did not go through", async () => {
|
||||
await fail({
|
||||
mutationFn: () => Promise.reject(new TypeError("Failed to fetch")),
|
||||
meta: { failTitle: TITLE },
|
||||
});
|
||||
expect(notifyMock).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ message: i18n.t("errors.reason.unreachable") }),
|
||||
);
|
||||
});
|
||||
|
||||
it("gets ONE toast when the mutation's own onError reported it — react-query runs that one second", async () => {
|
||||
await fail({
|
||||
mutationFn: () => Promise.reject(new HttpError(404, "gone")),
|
||||
meta: { failTitle: TITLE },
|
||||
onError: (e) => notifyActionError(e, "OWN TITLE"),
|
||||
});
|
||||
expect(notifyMock.mock.calls.map(([n]) => n.title)).toEqual(["OWN TITLE"]);
|
||||
});
|
||||
|
||||
it("is still reported when the mutation's onError only rolls back", async () => {
|
||||
const rollback = vi.fn();
|
||||
await fail({
|
||||
mutationFn: () => Promise.reject(new HttpError(403, "nope")),
|
||||
meta: { failTitle: TITLE },
|
||||
onError: rollback,
|
||||
});
|
||||
expect(rollback).toHaveBeenCalled();
|
||||
expect(notifyMock.mock.calls.map(([n]) => n.title)).toEqual([i18n.t(TITLE)]);
|
||||
});
|
||||
|
||||
it("gets no toast when the page shows the failure itself", async () => {
|
||||
await fail({
|
||||
mutationFn: () => Promise.reject(new HttpError(403, "nope")),
|
||||
meta: { errorShownInline: true },
|
||||
});
|
||||
expect(notifyMock).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("gets no toast on top of the error dialog the request layer already opened", async () => {
|
||||
vi.spyOn(globalThis, "fetch").mockResolvedValue(
|
||||
new Response(JSON.stringify({ detail: "conflict" }), {
|
||||
status: 409,
|
||||
headers: { "content-type": "application/json" },
|
||||
}),
|
||||
);
|
||||
await fail({ mutationFn: () => req("/api/x", { method: "POST" }), meta: { failTitle: TITLE } });
|
||||
expect(reportError).toHaveBeenCalled();
|
||||
expect(notifyMock).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,42 @@
|
||||
import { MutationCache, QueryClient } from "@tanstack/react-query";
|
||||
import i18n from "../i18n";
|
||||
import { notifyActionError } from "./actionErrors";
|
||||
|
||||
declare module "@tanstack/react-query" {
|
||||
interface Register {
|
||||
mutationMeta: {
|
||||
/** i18n key of the toast title that names what failed ("Couldn't hide the channel"). Without
|
||||
* one the failure is still reported, under the generic "Couldn't complete that". */
|
||||
failTitle?: string;
|
||||
/** The component renders this mutation's failure on the page itself — no toast. */
|
||||
errorShownInline?: boolean;
|
||||
};
|
||||
}
|
||||
}
|
||||
|
||||
type FailMeta = { failTitle?: string; errorShownInline?: boolean } | undefined;
|
||||
|
||||
/**
|
||||
* The ONE reporter for a failed mutation. `req` shows its blocking dialog for 400/409/422/5xx only;
|
||||
* a 403/404/429 was left to the caller and a 502–504 or network failure got just the connection
|
||||
* notice — so every `useMutation` without a reporting `onError` failed in silence: a channel hide
|
||||
* that snapped back, a share link that never revoked, a scheduler run that never started (E11 S2b
|
||||
* counted 34 such mutations). Reporting centrally means a new mutation cannot be silent.
|
||||
*
|
||||
* react-query awaits this BEFORE the mutation's own `onError`, so it waits a task: a site that
|
||||
* reported the failure itself (through `notifyActionError`, which marks it) has had its turn, and
|
||||
* `notifyActionError` skips what is already on screen.
|
||||
*/
|
||||
function reportMutationFailure(err: unknown, meta: FailMeta): void {
|
||||
if (meta?.errorShownInline) return;
|
||||
setTimeout(() => notifyActionError(err, i18n.t(meta?.failTitle ?? "errors.title")), 0);
|
||||
}
|
||||
|
||||
export function createQueryClient(): QueryClient {
|
||||
return new QueryClient({
|
||||
mutationCache: new MutationCache({
|
||||
onError: (err, _vars, _onMutateResult, mutation) => reportMutationFailure(err, mutation.meta),
|
||||
}),
|
||||
defaultOptions: { queries: { retry: false, staleTime: 30_000, refetchOnWindowFocus: false } },
|
||||
});
|
||||
}
|
||||
@@ -1,5 +1,7 @@
|
||||
import { useCallback, useEffect, useRef } from "react";
|
||||
import { api } from "./api";
|
||||
import { notifyActionError } from "./actionErrors";
|
||||
import i18n from "../i18n";
|
||||
|
||||
/** The shared feed-priority flow used by both the channel manager and the channel detail page:
|
||||
* an optimistic bump/set with a debounced server flush (~400ms after the last notch) so a
|
||||
@@ -34,7 +36,12 @@ export function useChannelPriority(opts: {
|
||||
api
|
||||
.updateChannel(channelId, { priority: latest })
|
||||
.then(() => optsRef.current.onSuccess?.(channelId))
|
||||
.catch(() => optsRef.current.onError(channelId));
|
||||
.catch((err: unknown) => {
|
||||
// Said here, once, for both callers: their `onError` only refetches, so a refused priority
|
||||
// used to just snap back — often after the stepper had already been let go (E11 S2b).
|
||||
notifyActionError(err, i18n.t("errors.writeFailed.channelPriority"));
|
||||
optsRef.current.onError(channelId);
|
||||
});
|
||||
}, []);
|
||||
|
||||
// Set an absolute priority (used by both the arrows — via bumpPriority — and the inline edit),
|
||||
|
||||
@@ -2,12 +2,16 @@ import { useQueryClient } from "@tanstack/react-query";
|
||||
import { useTranslation } from "react-i18next";
|
||||
import { api } from "./api";
|
||||
import { qk } from "./queryKeys";
|
||||
import { notify } from "./notifications";
|
||||
import { notifyActionError } from "./actionErrors";
|
||||
|
||||
/** The shared channel-tag mutation flow used by both the channel manager and the channel detail
|
||||
* page: optimistic attach/detach + a create-then-attach shortcut, with the cache-shape-specific
|
||||
* optimistic update injected (the manager maps over a ManagedChannel[]; the detail patches one
|
||||
* ChannelDetail). Keeps the two call sites from drifting. */
|
||||
* ChannelDetail). Keeps the two call sites from drifting.
|
||||
*
|
||||
* A failure is REPORTED here, once, with the real error: the manager used to do it in its `onError`
|
||||
* and hand the channel id to the reporter as though it were the error, and the detail page did not
|
||||
* do it at all — a tag toggled there just snapped back (E11 S2b). */
|
||||
export function useChannelTagActions(opts: {
|
||||
/** Apply the optimistic tag change to whichever cache the caller owns. `attach` true = add. */
|
||||
optimisticToggle: (channelId: string, tagId: number, attach: boolean) => void;
|
||||
@@ -24,7 +28,12 @@ export function useChannelTagActions(opts: {
|
||||
const call = hasTag
|
||||
? api.detachChannelTag(channelId, tagId)
|
||||
: api.attachChannelTag(channelId, tagId);
|
||||
call.then(() => opts.onSuccess?.(channelId)).catch(() => opts.onError(channelId));
|
||||
call
|
||||
.then(() => opts.onSuccess?.(channelId))
|
||||
.catch((err: unknown) => {
|
||||
notifyActionError(err, t("channels.tags.updateFailed"));
|
||||
opts.onError(channelId);
|
||||
});
|
||||
};
|
||||
|
||||
// Create a brand-new personal tag and attach it in one step (so tagging doesn't require the
|
||||
@@ -36,7 +45,7 @@ export function useChannelTagActions(opts: {
|
||||
qc.invalidateQueries({ queryKey: qk.tags() });
|
||||
toggleTag(channelId, tag.id, false);
|
||||
})
|
||||
.catch(() => notify({ level: "error", message: t("channels.tags.createFailed") }));
|
||||
.catch((err: unknown) => notifyActionError(err, t("channels.tags.createFailed")));
|
||||
};
|
||||
|
||||
return { toggleTag, createAndAttachTag };
|
||||
|
||||
@@ -14,7 +14,9 @@ const { store, savePrefs } = vi.hoisted(() => ({
|
||||
savePrefs: vi.fn(() => Promise.resolve({})),
|
||||
}));
|
||||
|
||||
vi.mock("./storage", () => ({
|
||||
// Partial: i18n (which usePref now imports, to title a refused save) reads its key from `LS`.
|
||||
vi.mock("./storage", async (importOriginal) => ({
|
||||
...(await importOriginal<typeof import("./storage")>()),
|
||||
readAccount: (base: string, fallback: unknown) => {
|
||||
const raw = store.get(base);
|
||||
return raw == null ? fallback : JSON.parse(raw);
|
||||
@@ -24,6 +26,9 @@ vi.mock("./storage", () => ({
|
||||
},
|
||||
}));
|
||||
vi.mock("./api", () => ({ api: { savePrefs, me: vi.fn() } }));
|
||||
// The failure path (usePref.saveFailure.dom.test.ts) is not this file's subject; stubbed so the
|
||||
// reporter's notifications → storage chain does not meet the storage mock above.
|
||||
vi.mock("./actionErrors", () => ({ notifyActionError: vi.fn() }));
|
||||
|
||||
import { usePref, useSavePref } from "./usePref";
|
||||
import { qk } from "./queryKeys";
|
||||
|
||||
@@ -0,0 +1,51 @@
|
||||
// @vitest-environment jsdom
|
||||
//
|
||||
// A preference save the server refuses is REPORTED (E11 S2b). The local state and the `me` cache
|
||||
// already show the new value, so a swallowed failure was invisible until the next load quietly put
|
||||
// the old value back — and the comment above the swallow claimed `req` toasted it, which it does not
|
||||
// for a 403/404/429 or a lost connection.
|
||||
import { afterEach, describe, expect, it, vi } from "vitest";
|
||||
import { act, renderHook, waitFor } from "@testing-library/react";
|
||||
import { QueryClient, QueryClientProvider } from "@tanstack/react-query";
|
||||
import { createElement, type ReactNode } from "react";
|
||||
|
||||
const { savePrefs, notifyActionError } = vi.hoisted(() => ({
|
||||
savePrefs: vi.fn(),
|
||||
notifyActionError: vi.fn(),
|
||||
}));
|
||||
vi.mock("./api", () => ({ api: { savePrefs, me: vi.fn() } }));
|
||||
vi.mock("./actionErrors", () => ({ notifyActionError }));
|
||||
|
||||
import { usePref, useSavePref } from "./usePref";
|
||||
import i18n from "../i18n";
|
||||
|
||||
afterEach(() => {
|
||||
savePrefs.mockReset();
|
||||
notifyActionError.mockReset();
|
||||
});
|
||||
|
||||
const wrapper = ({ children }: { children: ReactNode }) =>
|
||||
createElement(QueryClientProvider, { client: new QueryClient() }, children);
|
||||
|
||||
describe("a refused preference save", () => {
|
||||
it("is reported from the usePref setter", async () => {
|
||||
const refusal = new TypeError("Failed to fetch");
|
||||
savePrefs.mockRejectedValue(refusal);
|
||||
const { result } = renderHook(() => usePref<boolean>("hintsOn", true), { wrapper });
|
||||
act(() => result.current[1](false));
|
||||
await waitFor(() =>
|
||||
expect(notifyActionError).toHaveBeenCalledWith(refusal, i18n.t("errors.writeFailed.prefs")),
|
||||
);
|
||||
expect(result.current[0]).toBe(false); // the local value stays; the toast says it did not save
|
||||
});
|
||||
|
||||
it("is reported from the raw useSavePref persister", async () => {
|
||||
const refusal = new Error("403");
|
||||
savePrefs.mockRejectedValue(refusal);
|
||||
const { result } = renderHook(() => useSavePref(), { wrapper });
|
||||
act(() => result.current({ stripSources: { c1: false } }));
|
||||
await waitFor(() =>
|
||||
expect(notifyActionError).toHaveBeenCalledWith(refusal, i18n.t("errors.writeFailed.prefs")),
|
||||
);
|
||||
});
|
||||
});
|
||||
@@ -4,6 +4,14 @@ import { qk } from "./queryKeys";
|
||||
import { api } from "./api";
|
||||
import type { Me } from "./api/account";
|
||||
import { readAccount, writeAccount } from "./storage";
|
||||
import { notifyActionError } from "./actionErrors";
|
||||
import i18n from "../i18n";
|
||||
|
||||
// The local state + cache already show the new value, so a refused save is otherwise invisible until
|
||||
// the next load quietly puts the old one back. `req` reports none of 403/404/429 and only the
|
||||
// connection notice for a lost connection — say it (E11 S2b).
|
||||
const reportPrefSaveFailure = (err: unknown) =>
|
||||
notifyActionError(err, i18n.t("errors.writeFailed.prefs"));
|
||||
|
||||
// The ONE per-account + server preference hook. A preference (unlike an ephemeral filter) persists to
|
||||
// the account on the server (PUT /api/me/preferences, which MERGES the partial payload) AND is adopted
|
||||
@@ -55,7 +63,7 @@ export function useSavePref(): (patch: Record<string, unknown>) => void {
|
||||
const qc = useQueryClient();
|
||||
return useCallback(
|
||||
(patch: Record<string, unknown>) => {
|
||||
api.savePrefs(patch).catch(() => {});
|
||||
api.savePrefs(patch).catch(reportPrefSaveFailure);
|
||||
mergePrefsCache(qc, patch);
|
||||
},
|
||||
[qc],
|
||||
@@ -116,9 +124,9 @@ export function usePref<T>(
|
||||
// see the new value without a refetch. Merges into preferences, mirroring the server PUT.
|
||||
const payload = { [serverKey]: next };
|
||||
mergePrefsCache(qc, payload);
|
||||
// savePrefs is fire-and-forget: api.req() already surfaces a failure toast, and the local state +
|
||||
// cache already reflect the change (idempotent PUT retries a transient blip).
|
||||
const flush = () => api.savePrefs(payload).catch(() => {});
|
||||
// savePrefs is fire-and-forget (idempotent PUT, so `req` retries a transient blip once); a
|
||||
// failure that survives that is reported, not swallowed.
|
||||
const flush = () => api.savePrefs(payload).catch(reportPrefSaveFailure);
|
||||
if (debounceMs) {
|
||||
window.clearTimeout(saveTimer.current);
|
||||
pendingSave.current = flush;
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
import { HttpError } from "./api";
|
||||
import { markReported } from "./api/core";
|
||||
import { notify } from "./notifications";
|
||||
import { notifyActionError } from "./actionErrors";
|
||||
import i18n from "../i18n";
|
||||
@@ -22,6 +23,7 @@ export function notifyYouTubeActionError(
|
||||
? { label: i18n.t("channels.notify.connect"), onClick: onConnect }
|
||||
: undefined,
|
||||
});
|
||||
markReported(err);
|
||||
return;
|
||||
}
|
||||
// Any other error: prefer the server's own reason (e.g. "Not enough quota left today") over the
|
||||
|
||||
Reference in New Issue
Block a user