From d17af1c52aee7a15447ed0bfbcfcdd5b0d6d47c3 Mon Sep 17 00:00:00 2001 From: NuklearRabbit <145918611+NuklearRabbit@users.noreply.github.com> Date: Tue, 4 Aug 2026 03:08:07 +0200 Subject: [PATCH] feat: centralize API error localization Replace the err instanceof ApiError ? err.message : t(fallback) anti- pattern -- which showed raw English backend text for the common case and only used the localized fallback for the rare network-failure case -- at all 13 call sites across 7 files. New frontend/src/api/errorMessages.ts (describeApiError) resolves a caught error to a localized {title, explanation, nextStep?, technical} by checking the 32 known AppError codes first, then known HTTP statuses (401/403/404/409/422/500), then a fully generic fallback. New ApiErrorNotice (PageChrome.tsx) renders title/explanation/nextStep with the raw text demoted to a "Technical details"/"Details techniques" disclosure -- never shown as the primary message. ApiError itself is split out of client.ts into a standalone api/apiError.ts with no import.meta.env dependency, so errorMessages.ts (and its tests) can be loaded outside a Vite/browser context. --- frontend/e2e/error-messages.spec.ts | 205 ++++++++++++++++++ frontend/src/api/apiError.ts | 12 + frontend/src/api/client.ts | 17 +- frontend/src/api/errorMessages.ts | 100 +++++++++ frontend/src/components/DemoGuide.tsx | 10 +- frontend/src/components/Layout.tsx | 10 +- frontend/src/components/PageChrome.tsx | 20 ++ frontend/src/components/ReturnForm.tsx | 12 +- frontend/src/i18n/locales/en-GB/errors.json | 184 +++++++++++++++- frontend/src/i18n/locales/fr-BE/errors.json | 184 +++++++++++++++- frontend/src/i18n/locales/nl-BE/errors.json | 184 +++++++++++++++- frontend/src/pages/Automation.tsx | 11 +- frontend/src/pages/DataQuality.tsx | 11 +- frontend/src/pages/DataQualityIssueDetail.tsx | 45 ++-- frontend/src/pages/Knowledge.tsx | 11 +- frontend/src/styles.css | 6 + 16 files changed, 940 insertions(+), 82 deletions(-) create mode 100644 frontend/e2e/error-messages.spec.ts create mode 100644 frontend/src/api/apiError.ts create mode 100644 frontend/src/api/errorMessages.ts diff --git a/frontend/e2e/error-messages.spec.ts b/frontend/e2e/error-messages.spec.ts new file mode 100644 index 0000000..4de6df1 --- /dev/null +++ b/frontend/e2e/error-messages.spec.ts @@ -0,0 +1,205 @@ +import { expect, test } from "@playwright/test"; +import fs from "node:fs"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; +import { ApiError } from "../src/api/apiError"; +import { describeApiError, KNOWN_CODES } from "../src/api/errorMessages"; + +// Pure Node-context checks for the central API-error-localization function (section 7 / +// 11 of the Fleet Ops final localization brief). No browser needed: describeApiError() +// only depends on a `t` function and a caught error, so it's tested here against the +// real locale JSON with a minimal i18next-shaped `t` stub -- proving the known-code and +// known-HTTP-status paths never leak raw backend English as the primary message, and +// that the raw text is always still available via `.technical` for "Technical details". + +const __dirname = path.dirname(fileURLToPath(import.meta.url)); +const LOCALES_DIR = path.resolve(__dirname, "../src/i18n/locales"); +const LANGUAGES = ["nl-BE", "en-GB", "fr-BE"] as const; + +function loadNamespace(language: string, namespace: string): Record { + const filePath = path.join(LOCALES_DIR, language, `${namespace}.json`); + return JSON.parse(fs.readFileSync(filePath, "utf-8")); +} + +// Mirrors the (namespace, options.defaultValue) contract react-i18next's `t` exposes, +// resolving "namespace:dotted.path" against the real locale files for the given language. +function makeT(language: string): (key: string, options?: Record) => string { + return (key: string, options?: Record) => { + const [ns, ...rest] = key.includes(":") ? key.split(":") : ["errors", key]; + const dottedPath = key.includes(":") ? rest.join(":") : rest.join(""); + const data = loadNamespace(language, ns); + const value = dottedPath.split(".").reduce((acc, part) => { + if (acc && typeof acc === "object") return (acc as Record)[part]; + return undefined; + }, data); + if (typeof value === "string") return value; + if (options && "defaultValue" in options) return String(options.defaultValue); + return key; + }; +} + +const KNOWN_HTTP_STATUSES = ["401", "403", "404", "409", "422", "500"]; + +test("every known AppError code has a non-empty title+explanation in all 3 locales", () => { + for (const language of LANGUAGES) { + const codes = loadNamespace(language, "errors").codes as Record; + for (const code of KNOWN_CODES) { + expect(codes[code], `${language}/errors.json is missing codes.${code}`).toBeTruthy(); + expect(codes[code]?.title?.trim().length ?? 0, `${language}/errors.json:codes.${code}.title is empty`).toBeGreaterThan(0); + expect( + codes[code]?.explanation?.trim().length ?? 0, + `${language}/errors.json:codes.${code}.explanation is empty`, + ).toBeGreaterThan(0); + } + } +}); + +test("every known HTTP status fallback has a non-empty title+explanation in all 3 locales", () => { + for (const language of LANGUAGES) { + const http = loadNamespace(language, "errors").http as Record; + for (const status of KNOWN_HTTP_STATUSES) { + expect(http[status], `${language}/errors.json is missing http.${status}`).toBeTruthy(); + expect(http[status]?.title?.trim().length ?? 0, `${language}/errors.json:http.${status}.title is empty`).toBeGreaterThan(0); + expect( + http[status]?.explanation?.trim().length ?? 0, + `${language}/errors.json:http.${status}.explanation is empty`, + ).toBeGreaterThan(0); + } + } +}); + +test("a known AppError code resolves to its localized codes.* entry, never the raw backend message", () => { + for (const language of LANGUAGES) { + const t = makeT(language); + const raw = "IntegrityError: duplicate key value violates unique constraint"; + const err = new ApiError(409, "VEHICLE_NOT_FOUND", raw, "corr-1"); + const info = describeApiError(t, err); + const expected = loadNamespace(language, "errors").codes as Record; + expect(info.title).toBe(expected.VEHICLE_NOT_FOUND.title); + expect(info.explanation).toBe(expected.VEHICLE_NOT_FOUND.explanation); + expect(info.title).not.toBe(raw); + expect(info.explanation).not.toBe(raw); + // The raw backend text must still be reachable, just demoted to `.technical`. + expect(info.technical).toBe(raw); + } +}); + +test("a code with nextStep populates it; a code without nextStep leaves it undefined", () => { + const t = makeT("nl-BE"); + const withNextStep = describeApiError(t, new ApiError(422, "EMPTY_VALUE", "raw", "c1")); + expect(withNextStep.nextStep).toBeTruthy(); + + const withoutNextStep = describeApiError(t, new ApiError(404, "CUSTOMER_NOT_FOUND", "raw", "c2")); + expect(withoutNextStep.nextStep).toBeUndefined(); +}); + +test("an unrecognized AppError code falls back to the matching known HTTP status, not raw text", () => { + for (const language of LANGUAGES) { + const t = makeT(language); + const raw = "Some brand-new backend code nobody localized yet"; + const err = new ApiError(404, "SOME_FUTURE_CODE_NOT_YET_LOCALIZED", raw, "corr-2"); + const info = describeApiError(t, err); + const expected404 = (loadNamespace(language, "errors").http as Record)["404"]; + expect(info.title).toBe(expected404.title); + expect(info.explanation).toBe(expected404.explanation); + expect(info.title).not.toBe(raw); + expect(info.technical).toBe(raw); + } +}); + +test("an unrecognized code and an unrecognized HTTP status fall back to the fully generic message", () => { + for (const language of LANGUAGES) { + const t = makeT(language); + const raw = "418 I'm a teapot (never mapped)"; + const err = new ApiError(418, "418", raw, "corr-3"); + const info = describeApiError(t, err); + const generic = loadNamespace(language, "errors").generic as { title: string; explanation: string }; + expect(info.title).toBe(generic.title); + expect(info.explanation).toBe(generic.explanation); + expect(info.technical).toBe(raw); + } +}); + +test("a stringified HTTP status used as the AppError code (plain HTTPException path) resolves via the http map", () => { + // Mirrors app/main.py's plain-HTTPException handler, which sets code = str(status_code) + // (e.g. "401") rather than a semantic AppError code -- see backend/app/main.py. + const t = makeT("fr-BE"); + const err = new ApiError(401, "401", "Not authenticated", "corr-4"); + const info = describeApiError(t, err); + const expected401 = (loadNamespace("fr-BE", "errors").http as Record)["401"]; + expect(info.title).toBe(expected401.title); + expect(info.title).not.toBe("Not authenticated"); +}); + +test("a non-ApiError (e.g. network failure before any response) uses the fallback key, never a raw JS error message as the primary text", () => { + const t = makeT("nl-BE"); + const networkFailure = new TypeError("Failed to fetch"); + // Real call sites (e.g. Automation.tsx) invoke t() with their own default namespace + // already scoped via useTranslation("integrations"); this stub's default namespace is + // "errors", so the fallback key is qualified explicitly here to match. + const info = describeApiError(t, networkFailure, "integrations:ledger.retryFailed"); + const expectedFallback = loadNamespace("nl-BE", "integrations").ledger as Record; + expect(info.explanation).toBe(expectedFallback.retryFailed); + expect(info.explanation).not.toBe("Failed to fetch"); + expect(info.technical).toBe("Failed to fetch"); +}); + +// --- Backend/frontend AppError code drift guard --- +// KNOWN_CODES is a hand-maintained mirror of every `raise AppError("CODE", ...)` in the +// backend (see app/core/errors.py::AppError and every raise site). If the backend adds a +// new code and nobody updates KNOWN_CODES, it silently falls back to the generic-but- +// still-localized HTTP/generic message rather than raw English -- not a broken build, but +// a missed opportunity for a more specific message. This test surfaces that drift instead +// of letting it go unnoticed indefinitely. +const BACKEND_APP_DIR = path.resolve(__dirname, "../../backend/app"); + +function collectPyFiles(dir: string): string[] { + const entries = fs.readdirSync(dir, { withFileTypes: true }); + return entries.flatMap((entry) => { + const full = path.join(dir, entry.name); + if (entry.isDirectory()) return collectPyFiles(full); + return entry.name.endsWith(".py") ? [full] : []; + }); +} + +function collectBackendAppErrorCodes(): Set { + const codes = new Set(); + for (const file of collectPyFiles(BACKEND_APP_DIR)) { + const source = fs.readFileSync(file, "utf-8"); + const pattern = /AppError\(\s*"([A-Z_]+)"/g; + let match: RegExpExecArray | null; + while ((match = pattern.exec(source)) !== null) { + codes.add(match[1]); + } + } + return codes; +} + +test("frontend KNOWN_CODES exactly matches every AppError code actually raised by the backend", () => { + const backendCodes = collectBackendAppErrorCodes(); + const frontendCodes = KNOWN_CODES; + + const missingFromFrontend = [...backendCodes].filter((c) => !frontendCodes.has(c)).sort(); + const staleInFrontend = [...frontendCodes].filter((c) => !backendCodes.has(c)).sort(); + + expect( + missingFromFrontend, + `Backend raises AppError code(s) with no localized entry in errorMessages.ts KNOWN_CODES ` + + `(they'll fall back to a generic/HTTP-status message): ${missingFromFrontend.join(", ")}`, + ).toEqual([]); + expect( + staleInFrontend, + `errorMessages.ts KNOWN_CODES lists code(s) the backend never raises -- likely renamed or ` + + `removed on the backend side: ${staleInFrontend.join(", ")}`, + ).toEqual([]); +}); + +test("a non-ApiError with no fallbackKey uses the fully generic explanation", () => { + for (const language of LANGUAGES) { + const t = makeT(language); + const info = describeApiError(t, new TypeError("Failed to fetch")); + const generic = loadNamespace(language, "errors").generic as { title: string; explanation: string }; + expect(info.title).toBe(generic.title); + expect(info.explanation).toBe(generic.explanation); + } +}); diff --git a/frontend/src/api/apiError.ts b/frontend/src/api/apiError.ts new file mode 100644 index 0000000..3f30deb --- /dev/null +++ b/frontend/src/api/apiError.ts @@ -0,0 +1,12 @@ +export class ApiError extends Error { + status: number; + code: string; + correlationId: string; + + constructor(status: number, code: string, message: string, correlationId: string) { + super(message); + this.status = status; + this.code = code; + this.correlationId = correlationId; + } +} diff --git a/frontend/src/api/client.ts b/frontend/src/api/client.ts index 21176b2..4543a34 100644 --- a/frontend/src/api/client.ts +++ b/frontend/src/api/client.ts @@ -1,3 +1,7 @@ +import { ApiError } from "./apiError"; + +export { ApiError } from "./apiError"; + const API_BASE = import.meta.env.VITE_API_BASE_URL ?? ""; type UnauthorizedListener = () => void; @@ -8,19 +12,6 @@ export function onUnauthorized(listener: UnauthorizedListener): () => void { return () => unauthorizedListeners.delete(listener); } -export class ApiError extends Error { - status: number; - code: string; - correlationId: string; - - constructor(status: number, code: string, message: string, correlationId: string) { - super(message); - this.status = status; - this.code = code; - this.correlationId = correlationId; - } -} - async function request(path: string, init?: RequestInit): Promise { const response = await fetch(`${API_BASE}${path}`, { ...init, diff --git a/frontend/src/api/errorMessages.ts b/frontend/src/api/errorMessages.ts new file mode 100644 index 0000000..0cf2fe3 --- /dev/null +++ b/frontend/src/api/errorMessages.ts @@ -0,0 +1,100 @@ +import { ApiError } from "./apiError"; + +export interface ApiErrorInfo { + title: string; + explanation: string; + nextStep?: string; + technical: string; +} + +type TFn = (key: string, options?: Record) => string; + +// Backend AppError codes this frontend knows how to present with a localized title, +// explanation and (where useful) a next step -- see +// backend/app/core/errors.py::AppError and every `raise AppError("CODE", ...)` site. +// Anything not in this list still gets a sensible HTTP-status-based fallback below, so +// a newly-introduced backend code never regresses to raw English -- it just falls back +// to a generic-but-localized message until this list is extended. +export const KNOWN_CODES = new Set([ + "VEHICLE_NOT_FOUND", + "BOOKING_NOT_FOUND", + "CUSTOMER_NOT_FOUND", + "ENTITY_NOT_FOUND", + "EVENT_NOT_FOUND", + "ISSUE_NOT_FOUND", + "ISSUE_NOT_OPEN", + "BOOKING_NOT_ACTIVE", + "INVALID_BOOKING_STATE", + "NOT_RETRYABLE", + "CONFLICT_STILL_PRESENT", + "OVERLAP_STILL_PRESENT", + "EMPTY_VALUE", + "NO_FIELDS_PROVIDED", + "INVALID_FIELD", + "INVALID_FIELD_OVERRIDE", + "INVALID_SURVIVOR", + "INVALID_BOOKING_REFERENCE", + "INVALID_EVENT_ID", + "INVALID_IDEMPOTENCY_KEY", + "IDEMPOTENCY_KEY_REUSED", + "CORRECTED_VALUE_REQUIRED", + "CORRECTION_BELOW_CANONICAL", + "NOT_A_DUPLICATE_ISSUE", + "NOT_A_MISSING_FIELD_ISSUE", + "NOT_AN_ODOMETER_ISSUE", + "NOT_AN_OVERLAP_ISSUE", + "NOT_A_STATUS_CONFLICT_ISSUE", + "UNSUPPORTED_ENTITY", + "MANUAL_REVIEW_REQUIRED", + "NO_CONFLICT_DETECTED", + "RECOMMENDATION_STALE", + "UNAUTHORIZED_SERVICE", +]); + +const KNOWN_HTTP_STATUSES = new Set(["401", "403", "404", "409", "422", "500"]); + +/** + * Turns a caught error into a localized {title, explanation, nextStep?, technical} + * for display. The raw backend/network text is only ever exposed as `technical` + * (shown under "Technical details" by ApiErrorNotice) -- never as the primary message. + * + * `fallbackKey` is an existing, already-localized `t()` key used as the explanation + * when the error isn't an ApiError at all (e.g. the fetch failed before a response + * existed) and errors:generic doesn't fit the specific action being attempted. + */ +export function describeApiError(t: TFn, err: unknown, fallbackKey?: string): ApiErrorInfo { + if (!(err instanceof ApiError)) { + return { + title: t("errors:generic.title"), + explanation: fallbackKey ? t(fallbackKey) : t("errors:generic.explanation"), + technical: err instanceof Error ? err.message : String(err), + }; + } + + if (KNOWN_CODES.has(err.code)) { + const nextStep = t(`errors:codes.${err.code}.nextStep`, { defaultValue: "" }); + return { + title: t(`errors:codes.${err.code}.title`), + explanation: t(`errors:codes.${err.code}.explanation`), + nextStep: nextStep || undefined, + technical: err.message, + }; + } + + const httpKey = KNOWN_HTTP_STATUSES.has(err.code) ? err.code : String(err.status); + if (KNOWN_HTTP_STATUSES.has(httpKey)) { + const nextStep = t(`errors:http.${httpKey}.nextStep`, { defaultValue: "" }); + return { + title: t(`errors:http.${httpKey}.title`), + explanation: t(`errors:http.${httpKey}.explanation`), + nextStep: nextStep || undefined, + technical: err.message, + }; + } + + return { + title: t("errors:generic.title"), + explanation: fallbackKey ? t(fallbackKey) : t("errors:generic.explanation"), + technical: err.message, + }; +} diff --git a/frontend/src/components/DemoGuide.tsx b/frontend/src/components/DemoGuide.tsx index 8be19c1..5b671a9 100644 --- a/frontend/src/components/DemoGuide.tsx +++ b/frontend/src/components/DemoGuide.tsx @@ -1,13 +1,15 @@ import { useNavigate, useLocation } from "react-router-dom"; import { useEffect, useRef, useState } from "react"; import { useTranslation } from "react-i18next"; -import { api, ApiError } from "../api/client"; +import { api } from "../api/client"; +import { describeApiError, type ApiErrorInfo } from "../api/errorMessages"; import { useAuth } from "../context/AuthContext"; import { useDemoGuide } from "../context/DemoGuideContext"; import { useDemoManifest } from "../context/DemoManifestContext"; import { useViewportTier } from "../hooks/useViewportTier"; import { DEMO_GUIDE_STEPS } from "../data/demoGuideSteps"; import { Icon } from "./Icons"; +import { ApiErrorNotice } from "./PageChrome"; import { PRODUCT_NAME } from "../product"; export function DemoGuideTrigger() { @@ -68,7 +70,7 @@ export function DemoGuide() { setCollapsedToChip, } = useDemoGuide(); const [resetting, setResetting] = useState(false); - const [resetError, setResetError] = useState(null); + const [resetError, setResetError] = useState(null); const [mobileSheetState, setMobileSheetState] = useState<"collapsed" | "half" | "full">("half"); const pendingTarget = useRef(null); @@ -119,7 +121,7 @@ export function DemoGuide() { await logout(); navigate("/login"); } catch (err) { - setResetError(err instanceof ApiError ? err.message : t("guide.restartFailed")); + setResetError(describeApiError(t, err, "guide.restartFailed")); } finally { setResetting(false); } @@ -227,7 +229,7 @@ export function DemoGuide() { )} - {resetError &&

{resetError}

} +