From ae34d98859d80635e4bd02d84f0c58b01ac92a82 Mon Sep 17 00:00:00 2001 From: Lukas Date: Tue, 28 Apr 2026 08:41:33 +0200 Subject: [PATCH] feat: centralized error notification service (issue #15) - Create NotificationService with context provider for centralized error/success messaging - Add NotificationContainer component to render notification stack - Integrate NotificationProvider into App root - Refactor BanUnbanForm to use notification service instead of local error state - Update fetchError utility to optionally use notification callbacks - Add comprehensive error handling guidelines to Web-Development.md - Prevent duplicate notifications with deduplication logic - Support auto-dismiss with configurable TTL per notification type Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- Docs/Tasks.md | 21 -- Docs/Web-Development.md | 115 +++++++++- frontend/src/App.tsx | 213 +++++++++--------- .../src/components/NotificationContainer.tsx | 86 +++++++ frontend/src/pages/jails/BanUnbanForm.tsx | 33 +-- frontend/src/services/notificationService.tsx | 165 ++++++++++++++ frontend/src/types/notification.ts | 53 +++++ frontend/src/utils/fetchError.ts | 23 +- 8 files changed, 557 insertions(+), 152 deletions(-) create mode 100644 frontend/src/components/NotificationContainer.tsx create mode 100644 frontend/src/services/notificationService.tsx create mode 100644 frontend/src/types/notification.ts diff --git a/Docs/Tasks.md b/Docs/Tasks.md index 0dadcef..8602692 100644 --- a/Docs/Tasks.md +++ b/Docs/Tasks.md @@ -1,24 +1,3 @@ -## 14) Error boundary granularity is too coarse -- Where found: - - [frontend/src/App.tsx](frontend/src/App.tsx) - - [frontend/src/components/ErrorBoundary.tsx](frontend/src/components/ErrorBoundary.tsx) -- Why this is needed: - - Single top boundary causes full app fallback on local component failures. -- Goal: - - Add page-level and section-level boundaries. -- What to do: - - Wrap risky pages/widgets with local boundaries. - - Preserve shell/navigation when a section fails. -- Possible traps and issues: - - Too many boundaries can complicate fallback UX consistency. -- Docs changes needed: - - Add frontend resilience/fallback strategy. -- Doc references: - - [Docs/Web-Development.md](Docs/Web-Development.md) - - https://react.dev/reference/react/Component#catching-rendering-errors-with-an-error-boundary - ---- - ## 15) Fragmented async error UX handling in components - Where found: - [frontend/src/pages/jails/BanUnbanForm.tsx](frontend/src/pages/jails/BanUnbanForm.tsx) diff --git a/Docs/Web-Development.md b/Docs/Web-Development.md index 24b5950..8574324 100644 --- a/Docs/Web-Development.md +++ b/Docs/Web-Development.md @@ -810,10 +810,123 @@ When an API request returns 401 or 403: ## 12. Error Handling & Resilience +### Centralized Error Notification Service + +The application provides a centralized `NotificationService` for displaying consistent, user-facing error and success messages across the entire UI. This prevents fragmented error handling and duplicate messaging. + +**When to use:** +- **Form operations** (ban, unban, save settings) → use global notifications for success/error feedback +- **Async actions** that affect application state → use notifications to inform the user +- **API errors** in components → convert to user-friendly messages and show via notifications + +**When NOT to use (keep local state):** +- Input validation errors that should not trigger API calls (display inline field errors instead) +- Component-internal loading states (no need for notifications) +- Errors in error boundaries (error boundaries have their own fallback UI) + +**API:** + +```ts +// In any component or hook within NotificationProvider +const notification = useNotification(); + +notification.success("Data saved successfully!"); +notification.error("Failed to save: invalid format"); +notification.warning("This action cannot be undone"); +notification.info("Processing in background…"); + +// Control auto-dismiss duration (milliseconds) +notification.error("Connection lost", 10000); // Auto-dismiss after 10 seconds +notification.error("Critical error", null); // Never auto-dismiss +``` + +**Auto-dismiss defaults:** +- Success: 5000ms +- Error: 8000ms +- Warning: 6000ms +- Info: 5000ms + +**Duplicate prevention:** The service prevents identical notifications from appearing multiple times. If you try to show the same message with the same intent level twice, the second is ignored. + +**Example refactor — from local state to notifications:** + +```ts +// Before: Local error/success state in component +function MyForm() { + const [error, setError] = useState(null); + const [success, setSuccess] = useState(null); + + const handleSubmit = async () => { + setError(null); + setSuccess(null); + try { + await saveData(); + setSuccess("Data saved!"); + } catch (err) { + setError("Failed to save"); + } + }; + + return ( +
+ {error && {error}} + {success && {success}} + +
+ ); +} + +// After: Using notification service +function MyForm() { + const notification = useNotification(); + + const handleSubmit = async () => { + try { + await saveData(); + notification.success("Data saved!"); + } catch (err) { + notification.error("Failed to save"); + } + }; + + return ; +} +``` + +**Using notifications with hooks:** + +If a hook manages data fetching and needs to notify of errors, accept a notification callback or use `useNotification` directly within the hook: + +```ts +// hooks/useSaveData.ts +export function useSaveData() { + const notification = useNotification(); + const [loading, setLoading] = useState(false); + + const save = useCallback(async (data: unknown) => { + setLoading(true); + try { + await api.post("/data", data); + notification.success("Data saved"); + return true; + } catch (err) { + notification.error("Failed to save data"); + return false; + } finally { + setLoading(false); + } + }, [notification]); + + return { save, loading }; +} +``` + ### API Error Handling - Wrap API calls in `try-catch` inside hooks — components should never see raw exceptions. -- **All hook catch blocks must use `handleFetchError` rather than directly calling `setError`.** This ensures auth errors (401/403) are routed to the global session-expiry flow instead of displaying confusing error text in the UI. Use the pattern: `handleFetchError(err, setError, "User-friendly fallback message")`. +- **All hook catch blocks should use `handleFetchError` or notifications.** This ensures auth errors (401/403) are routed to the global session-expiry flow instead of displaying confusing error text in the UI. + - **With local state:** `handleFetchError(err, setError, "User-friendly fallback message")` + - **With notifications:** `handleFetchError(err, setError, "Default message", notification.error)` - Display user-friendly error messages — never expose stack traces or raw server responses in the UI. - Log errors to the console (or a future logging service) with sufficient context for debugging. - Always handle the **loading**, **error**, and **empty** states for every data-driven component. diff --git a/frontend/src/App.tsx b/frontend/src/App.tsx index 5930440..7391723 100644 --- a/frontend/src/App.tsx +++ b/frontend/src/App.tsx @@ -31,10 +31,12 @@ import { darkTheme, lightTheme } from "./theme/customTheme"; import { AuthProvider } from "./providers/AuthProvider"; import { ThemeProvider, useThemeMode } from "./providers/ThemeProvider"; import { TimezoneProvider } from "./providers/TimezoneProvider"; +import { NotificationProvider } from "./services/notificationService"; import { RequireAuth } from "./components/RequireAuth"; import { SetupGuard } from "./components/SetupGuard"; import { ErrorBoundary } from "./components/ErrorBoundary"; import { PageErrorBoundary } from "./components/PageErrorBoundary"; +import { NotificationContainer } from "./components/NotificationContainer"; import { MainLayout } from "./layouts/MainLayout"; const SetupPage = lazy(() => import("./pages/SetupPage").then((m) => ({ default: m.SetupPage }))); @@ -56,114 +58,117 @@ function AppContents(): React.JSX.Element { return ( - - - }> - - - {/* Setup wizard — always accessible; redirects to /login if already done */} - - - - } - /> + + + + + }> + + + {/* Setup wizard — always accessible; redirects to /login if already done */} + + + + } + /> - {/* Login — requires setup to be complete */} - + {/* Login — requires setup to be complete */} + + + + + + } + /> + + {/* Protected routes — require setup AND authentication */} + - + + + + + - - } - /> + } + > + + + + } + /> + + + + } + /> + + + + } + /> + + + + } + /> + + + + } + /> + + + + } + /> + + + + } + /> + - {/* Protected routes — require setup AND authentication */} - - - - - - - - } - > - - - - } - /> - - - - } - /> - - - - } - /> - - - - } - /> - - - - } - /> - - - - } - /> - - - - } - /> - - - {/* Fallback — redirect unknown paths to dashboard */} - } /> - - - - - + {/* Fallback — redirect unknown paths to dashboard */} + } /> + + + + + + ); } diff --git a/frontend/src/components/NotificationContainer.tsx b/frontend/src/components/NotificationContainer.tsx new file mode 100644 index 0000000..139a61c --- /dev/null +++ b/frontend/src/components/NotificationContainer.tsx @@ -0,0 +1,86 @@ +/** + * Notification display container. + * + * Renders all active notifications from the NotificationService as a vertical stack + * at the top of the application. Each notification displays with appropriate styling + * based on its intent level and auto-dismisses after a configured duration. + */ + +import { MessageBar, MessageBarBody, makeStyles, tokens } from "@fluentui/react-components"; +import { useNotificationQueue } from "../services/notificationService"; +import type { Notification } from "../types/notification"; + +/** Styles for the notification container and messages. */ +const useStyles = makeStyles({ + container: { + position: "fixed", + top: 0, + left: 0, + right: 0, + zIndex: 10000, // Above all other content + pointerEvents: "none", // Allow clicks to pass through to content below + padding: tokens.spacingVerticalL, + maxHeight: "60vh", + overflowY: "auto", + }, + stack: { + display: "flex", + flexDirection: "column", + gap: tokens.spacingVerticalM, + maxWidth: "600px", + margin: "0 auto", + }, + messageBar: { + pointerEvents: "auto", // But restore pointer events on the messages + }, +}); + +/** + * NotificationContainer renders the notification queue. + * + * Place this component once in your app root (e.g., in App.tsx) to display + * all notifications managed by the NotificationService. + * + * @returns JSX element rendering the notification stack or empty fragment if no notifications. + */ +export function NotificationContainer(): React.JSX.Element { + const styles = useStyles(); + const notifications = useNotificationQueue(); + + if (notifications.length === 0) { + return <>; + } + + return ( +
+
+ {notifications.map((notification) => ( + + ))} +
+
+ ); +} + +/** + * Single notification message component. + * Renders a Fluent UI MessageBar with appropriate styling based on notification intent. + */ +interface NotificationMessageProps { + notification: Notification; +} + +function NotificationMessage({ notification }: NotificationMessageProps): React.JSX.Element { + const styles = useStyles(); + + // Map notification intent to MessageBar intent + const messageBarIntent: "success" | "error" | "warning" | "info" = + notification.intent === "info" ? "info" : notification.intent; + + return ( + + {notification.message} + + ); +} + diff --git a/frontend/src/pages/jails/BanUnbanForm.tsx b/frontend/src/pages/jails/BanUnbanForm.tsx index 20ecb55..7b99603 100644 --- a/frontend/src/pages/jails/BanUnbanForm.tsx +++ b/frontend/src/pages/jails/BanUnbanForm.tsx @@ -3,14 +3,13 @@ import { Button, Field, Input, - MessageBar, - MessageBarBody, Select, Text, } from "@fluentui/react-components"; import { LockClosedRegular, LockOpenRegular } from "@fluentui/react-icons"; import { useCommonSectionStyles } from "../../components/commonStyles"; import { useJailsPageStyles } from "./jailsPageStyles"; +import { useNotification } from "../../services/notificationService"; import { ApiError } from "../../api/client"; interface BanUnbanFormProps { @@ -22,12 +21,11 @@ interface BanUnbanFormProps { export function BanUnbanForm({ jailNames, onBan, onUnban }: BanUnbanFormProps): React.JSX.Element { const styles = useJailsPageStyles(); const sectionStyles = useCommonSectionStyles(); + const notification = useNotification(); const [banIpVal, setBanIpVal] = useState(""); const [banJail, setBanJail] = useState(""); const [unbanIpVal, setUnbanIpVal] = useState(""); const [unbanJail, setUnbanJail] = useState(""); - const [formError, setFormError] = useState(null); - const [formSuccess, setFormSuccess] = useState(null); const [isBanning, setIsBanning] = useState(false); const [isUnbanning, setIsUnbanning] = useState(false); @@ -39,10 +37,8 @@ export function BanUnbanForm({ jailNames, onBan, onUnban }: BanUnbanFormProps): : String(err); const handleBan = async (): Promise => { - setFormError(null); - setFormSuccess(null); if (!banIpVal.trim() || !banJail) { - setFormError("Both IP address and jail are required."); + notification.error("Both IP address and jail are required."); return; } @@ -50,20 +46,18 @@ export function BanUnbanForm({ jailNames, onBan, onUnban }: BanUnbanFormProps): try { const ip = banIpVal.trim(); await onBan(banJail, ip); - setFormSuccess(`${ip} banned in ${banJail}.`); + notification.success(`${ip} banned in ${banJail}.`); setBanIpVal(""); } catch (err: unknown) { - setFormError(formatErrorMessage(err)); + notification.error(formatErrorMessage(err)); } finally { setIsBanning(false); } }; const handleUnban = async (fromAllJails: boolean): Promise => { - setFormError(null); - setFormSuccess(null); if (!unbanIpVal.trim()) { - setFormError("IP address is required."); + notification.error("IP address is required."); return; } @@ -73,11 +67,11 @@ export function BanUnbanForm({ jailNames, onBan, onUnban }: BanUnbanFormProps): const jail = fromAllJails ? undefined : unbanJail || undefined; await onUnban(ip, jail); const scope = jail ?? "all jails"; - setFormSuccess(`${ip} unbanned from ${scope}.`); + notification.success(`${ip} unbanned from ${scope}.`); setUnbanIpVal(""); setUnbanJail(""); } catch (err: unknown) { - setFormError(formatErrorMessage(err)); + notification.error(formatErrorMessage(err)); } finally { setIsUnbanning(false); } @@ -91,17 +85,6 @@ export function BanUnbanForm({ jailNames, onBan, onUnban }: BanUnbanFormProps): - {formError && ( - - {formError} - - )} - {formSuccess && ( - - {formSuccess} - - )} - Ban an IP diff --git a/frontend/src/services/notificationService.tsx b/frontend/src/services/notificationService.tsx new file mode 100644 index 0000000..b55403a --- /dev/null +++ b/frontend/src/services/notificationService.tsx @@ -0,0 +1,165 @@ +/** + * Centralized notification service and provider. + * + * Provides a single channel for displaying user-facing notifications (errors, success, warnings, info). + * Prevents duplicate messaging, handles auto-dismiss, and maintains a queue of active notifications. + * Consumers use the `useNotification` hook to access the service. + */ + +import { createContext, useCallback, useContext, useMemo, useState } from "react"; +import type { Notification, NotificationIntent, NotificationService } from "../types/notification"; + +/** Default auto-dismiss durations (milliseconds) for each notification type. */ +const DEFAULT_DURATIONS: Record = { + success: 5000, + error: 8000, + warning: 6000, + info: 5000, +}; + +/** Context holding active notifications and the service API. */ +const NotificationContext = createContext(null); + +/** + * NotificationProvider component. + * Must wrap the entire application to make notifications available to all children. + */ +export function NotificationProvider({ children }: { children: React.ReactNode }): React.JSX.Element { + const [notifications, setNotifications] = useState([]); + + /** + * Remove a notification by ID. + */ + const dismiss = useCallback((id: string): void => { + setNotifications((prev) => prev.filter((n) => n.id !== id)); + }, []); + + /** + * Check if a notification with this message and intent already exists to prevent duplicates. + */ + const isDuplicate = useCallback( + (message: string, intent: NotificationIntent): boolean => + notifications.some((n) => n.message === message && n.intent === intent), + [notifications], + ); + + /** + * Generate a unique ID for a notification. + */ + const generateId = useCallback((): string => { + const timestamp = Date.now().toString(36); + const random = Math.random().toString(36).substring(2, 9); + return `notif-${timestamp}-${random}`; + }, []); + + /** + * Add a notification to the queue. + */ + const add = useCallback( + (intent: NotificationIntent, message: string, autoCloseMsec?: number): void => { + // Prevent duplicate identical notifications + if (isDuplicate(message, intent)) { + return; + } + + const id = generateId(); + const duration = autoCloseMsec !== undefined ? autoCloseMsec : DEFAULT_DURATIONS[intent]; + + setNotifications((prev) => [...prev, { id, intent, message, autoCloseMsec: duration }]); + + // Schedule auto-dismiss if duration is set + if (duration) { + setTimeout(() => { + dismiss(id); + }, duration); + } + }, + [isDuplicate, generateId, dismiss], + ); + + /** + * Public service API. + */ + const service: NotificationService = useMemo((): NotificationService => { + return { + success: (message: string, autoCloseMsec?: number): void => { + add("success", message, autoCloseMsec); + }, + error: (message: string, autoCloseMsec?: number): void => { + add("error", message, autoCloseMsec); + }, + warning: (message: string, autoCloseMsec?: number): void => { + add("warning", message, autoCloseMsec); + }, + info: (message: string, autoCloseMsec?: number): void => { + add("info", message, autoCloseMsec); + }, + dismiss, + }; + }, [add, dismiss]); + + return ( + + + {children} + + + ); +} + +/** + * Internal context for accessing the notification queue (for rendering). + * This is separate from the public context to keep the public API clean. + */ +interface NotificationContextValue { + notifications: Notification[]; + service: NotificationService; +} + +const NotificationContextInternal = createContext(null); + +/** + * Hook to access the notification service. + * Use this in any component to show success, error, warning, or info messages. + * + * @returns NotificationService API for showing messages. + * @throws Error if used outside NotificationProvider. + * + * @example + * ```tsx + * function MyComponent() { + * const notification = useNotification(); + * + * const handleSave = async () => { + * try { + * await saveData(); + * notification.success("Data saved successfully"); + * } catch (err) { + * notification.error("Failed to save data"); + * } + * }; + * + * return ; + * } + * ``` + */ +export function useNotification(): NotificationService { + const context = useContext(NotificationContext); + if (!context) { + throw new Error("useNotification must be used within NotificationProvider"); + } + return context; +} + +/** + * Internal hook to access the notifications queue for rendering. + * Only used by NotificationContainer. + */ +export function useNotificationQueue(): Notification[] { + const context = useContext(NotificationContextInternal); + if (!context) { + throw new Error("NotificationContextInternal must be used within NotificationProvider"); + } + return context.notifications; +} + diff --git a/frontend/src/types/notification.ts b/frontend/src/types/notification.ts new file mode 100644 index 0000000..28ed670 --- /dev/null +++ b/frontend/src/types/notification.ts @@ -0,0 +1,53 @@ +/** + * Notification type definitions. + * Defines the contract for error, success, warning, and info messages + * rendered through the centralized notification service. + */ + +/** Severity level of a notification. */ +export type NotificationIntent = "error" | "success" | "warning" | "info"; + +/** Unique notification message. */ +export interface Notification { + /** Unique identifier for this notification (auto-generated). */ + id: string; + /** Severity level. */ + intent: NotificationIntent; + /** User-facing message text. */ + message: string; + /** Auto-dismiss after this many milliseconds. Null means manual dismiss only. */ + autoCloseMsec?: number | null; +} + +/** Notification service public API. */ +export interface NotificationService { + /** + * Show a success notification. + * @param message User-facing message text. + * @param autoCloseMsec Auto-dismiss after this many ms. Defaults to 5000. + */ + success(message: string, autoCloseMsec?: number): void; + /** + * Show an error notification. + * @param message User-facing message text. + * @param autoCloseMsec Auto-dismiss after this many ms. Defaults to 8000. + */ + error(message: string, autoCloseMsec?: number): void; + /** + * Show a warning notification. + * @param message User-facing message text. + * @param autoCloseMsec Auto-dismiss after this many ms. Defaults to 6000. + */ + warning(message: string, autoCloseMsec?: number): void; + /** + * Show an info notification. + * @param message User-facing message text. + * @param autoCloseMsec Auto-dismiss after this many ms. Defaults to 5000. + */ + info(message: string, autoCloseMsec?: number): void; + /** + * Remove a notification by ID. + * Called automatically on auto-dismiss; can also be called manually. + */ + dismiss(id: string): void; +} diff --git a/frontend/src/utils/fetchError.ts b/frontend/src/utils/fetchError.ts index 093b9b0..77408fb 100644 --- a/frontend/src/utils/fetchError.ts +++ b/frontend/src/utils/fetchError.ts @@ -2,19 +2,40 @@ import { isAuthError } from "../api/client"; /** * Normalize fetch error handling across hooks. + * + * Handles three error cases: + * 1. Request was aborted — silently ignored (expected cleanup) + * 2. Auth error (401/403) — silently handled by AuthProvider (do not display) + * 3. Other error — stored in component state or notified via callback + * + * @param err - The caught error + * @param setError - State setter to store error message (used when no notification callback) + * @param fallback - Default error message if err is not an Error instance + * @param onError - Optional callback to notify of errors instead of using setError */ export function handleFetchError( err: unknown, setError: (value: string | null) => void, fallback: string = "Unknown error", + onError?: (message: string) => void, ): void { + // Abort errors are expected during cleanup — ignore silently if (err instanceof DOMException && err.name === "AbortError") { return; } + // Auth errors are handled globally by AuthProvider — do not display locally if (isAuthError(err)) { return; } - setError(err instanceof Error ? err.message : fallback); + const message = err instanceof Error ? err.message : fallback; + + // Use notification callback if provided; otherwise use local state setter + if (onError) { + onError(message); + } else { + setError(message); + } } +