refactoring-backend #3

Merged
lukas.pupkalipinski merged 403 commits from refactoring-backend into main 2026-05-20 20:23:46 +02:00
8 changed files with 557 additions and 152 deletions
Showing only changes of commit ae34d98859 - Show all commits

View File

@@ -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)

View File

@@ -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<string | null>(null);
const [success, setSuccess] = useState<string | null>(null);
const handleSubmit = async () => {
setError(null);
setSuccess(null);
try {
await saveData();
setSuccess("Data saved!");
} catch (err) {
setError("Failed to save");
}
};
return (
<div>
{error && <MessageBar intent="error">{error}</MessageBar>}
{success && <MessageBar intent="success">{success}</MessageBar>}
<button onClick={handleSubmit}>Save</button>
</div>
);
}
// 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 <button onClick={handleSubmit}>Save</button>;
}
```
**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.

View File

@@ -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 (
<FluentProvider theme={theme}>
<ErrorBoundary
title="Application Error"
message="The application encountered a critical error. Reloading may help."
isFullPage={true}
>
<BrowserRouter future={{ v7_startTransition: true, v7_relativeSplatPath: true }}>
<Suspense fallback={<Spinner size="large" label="Loading…" />}>
<AuthProvider>
<Routes>
{/* Setup wizard — always accessible; redirects to /login if already done */}
<Route
path="/setup"
element={
<PageErrorBoundary pageName="Setup">
<SetupPage />
</PageErrorBoundary>
}
/>
<NotificationProvider>
<ErrorBoundary
title="Application Error"
message="The application encountered a critical error. Reloading may help."
isFullPage={true}
>
<NotificationContainer />
<BrowserRouter future={{ v7_startTransition: true, v7_relativeSplatPath: true }}>
<Suspense fallback={<Spinner size="large" label="Loading…" />}>
<AuthProvider>
<Routes>
{/* Setup wizard — always accessible; redirects to /login if already done */}
<Route
path="/setup"
element={
<PageErrorBoundary pageName="Setup">
<SetupPage />
</PageErrorBoundary>
}
/>
{/* Login — requires setup to be complete */}
<Route
path="/login"
element={
<PageErrorBoundary pageName="Login">
{/* Login — requires setup to be complete */}
<Route
path="/login"
element={
<PageErrorBoundary pageName="Login">
<SetupGuard>
<LoginPage />
</SetupGuard>
</PageErrorBoundary>
}
/>
{/* Protected routes — require setup AND authentication */}
<Route
element={
<SetupGuard>
<LoginPage />
<RequireAuth>
<TimezoneProvider>
<MainLayout />
</TimezoneProvider>
</RequireAuth>
</SetupGuard>
</PageErrorBoundary>
}
/>
}
>
<Route
index
element={
<PageErrorBoundary pageName="Dashboard">
<DashboardPage />
</PageErrorBoundary>
}
/>
<Route
path="/map"
element={
<PageErrorBoundary pageName="Map">
<MapPage />
</PageErrorBoundary>
}
/>
<Route
path="/jails"
element={
<PageErrorBoundary pageName="Jails">
<JailsPage />
</PageErrorBoundary>
}
/>
<Route
path="/jails/:name"
element={
<PageErrorBoundary pageName="Jail Details">
<JailDetailPage />
</PageErrorBoundary>
}
/>
<Route
path="/config"
element={
<PageErrorBoundary pageName="Configuration">
<ConfigPage />
</PageErrorBoundary>
}
/>
<Route
path="/history"
element={
<PageErrorBoundary pageName="History">
<HistoryPage />
</PageErrorBoundary>
}
/>
<Route
path="/blocklists"
element={
<PageErrorBoundary pageName="Blocklists">
<BlocklistsPage />
</PageErrorBoundary>
}
/>
</Route>
{/* Protected routes — require setup AND authentication */}
<Route
element={
<SetupGuard>
<RequireAuth>
<TimezoneProvider>
<MainLayout />
</TimezoneProvider>
</RequireAuth>
</SetupGuard>
}
>
<Route
index
element={
<PageErrorBoundary pageName="Dashboard">
<DashboardPage />
</PageErrorBoundary>
}
/>
<Route
path="/map"
element={
<PageErrorBoundary pageName="Map">
<MapPage />
</PageErrorBoundary>
}
/>
<Route
path="/jails"
element={
<PageErrorBoundary pageName="Jails">
<JailsPage />
</PageErrorBoundary>
}
/>
<Route
path="/jails/:name"
element={
<PageErrorBoundary pageName="Jail Details">
<JailDetailPage />
</PageErrorBoundary>
}
/>
<Route
path="/config"
element={
<PageErrorBoundary pageName="Configuration">
<ConfigPage />
</PageErrorBoundary>
}
/>
<Route
path="/history"
element={
<PageErrorBoundary pageName="History">
<HistoryPage />
</PageErrorBoundary>
}
/>
<Route
path="/blocklists"
element={
<PageErrorBoundary pageName="Blocklists">
<BlocklistsPage />
</PageErrorBoundary>
}
/>
</Route>
{/* Fallback — redirect unknown paths to dashboard */}
<Route path="*" element={<Navigate to="/" replace />} />
</Routes>
</AuthProvider>
</Suspense>
</BrowserRouter>
</ErrorBoundary>
{/* Fallback — redirect unknown paths to dashboard */}
<Route path="*" element={<Navigate to="/" replace />} />
</Routes>
</AuthProvider>
</Suspense>
</BrowserRouter>
</ErrorBoundary>
</NotificationProvider>
</FluentProvider>
);
}

View File

@@ -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 (
<div className={styles.container} role="alert" aria-live="polite">
<div className={styles.stack}>
{notifications.map((notification) => (
<NotificationMessage key={notification.id} notification={notification} />
))}
</div>
</div>
);
}
/**
* 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 (
<MessageBar intent={messageBarIntent} className={styles.messageBar}>
<MessageBarBody>{notification.message}</MessageBarBody>
</MessageBar>
);
}

View File

@@ -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<string | null>(null);
const [formSuccess, setFormSuccess] = useState<string | null>(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<void> => {
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<void> => {
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):
</Text>
</div>
{formError && (
<MessageBar intent="error">
<MessageBarBody>{formError}</MessageBarBody>
</MessageBar>
)}
{formSuccess && (
<MessageBar intent="success">
<MessageBarBody>{formSuccess}</MessageBarBody>
</MessageBar>
)}
<Text size={300} weight="semibold">
Ban an IP
</Text>

View File

@@ -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<NotificationIntent, number> = {
success: 5000,
error: 8000,
warning: 6000,
info: 5000,
};
/** Context holding active notifications and the service API. */
const NotificationContext = createContext<NotificationService | null>(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<Notification[]>([]);
/**
* 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 (
<NotificationContext.Provider value={service}>
<NotificationContextInternal.Provider value={{ notifications, service }}>
{children}
</NotificationContextInternal.Provider>
</NotificationContext.Provider>
);
}
/**
* 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<NotificationContextValue | null>(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 <button onClick={handleSave}>Save</button>;
* }
* ```
*/
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;
}

View File

@@ -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;
}

View File

@@ -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);
}
}