diff --git a/Docs/Tasks.md b/Docs/Tasks.md index 8602692..5da184e 100644 --- a/Docs/Tasks.md +++ b/Docs/Tasks.md @@ -1,23 +1,3 @@ -## 15) Fragmented async error UX handling in components -- Where found: - - [frontend/src/pages/jails/BanUnbanForm.tsx](frontend/src/pages/jails/BanUnbanForm.tsx) - - [frontend/src/components](frontend/src/components) -- Why this is needed: - - Localized ad-hoc error handling leads to inconsistent user feedback. -- Goal: - - Centralized error reporting + consistent UI feedback channels. -- What to do: - - Introduce notification/error service. - - Standardize form operation error patterns. -- Possible traps and issues: - - Duplicate messaging if local and global handlers fire together. -- Docs changes needed: - - Add frontend error handling guideline. -- Doc references: - - [Docs/Web-Development.md](Docs/Web-Development.md) - ---- - ## 16) API usage pattern is inconsistent across components/hooks - Where found: - [frontend/src/pages/JailsPage.tsx](frontend/src/pages/JailsPage.tsx) diff --git a/Docs/Web-Development.md b/Docs/Web-Development.md index 8574324..39d08e0 100644 --- a/Docs/Web-Development.md +++ b/Docs/Web-Development.md @@ -187,6 +187,65 @@ export function useSharedSetupStatus(): UseSharedSetupStatusResult { - Cache TTL should be relatively short (30 seconds) unless the data is truly static - Subscribers receive notifications when the cache is invalidated, allowing them to trigger a fresh fetch if needed +### API Usage Layering + +Data fetching in BanGUI follows a strict, composable three-tier pattern to ensure consistency and testability: + +**Tier 1: API Functions** (`api/*.ts`) +- Pure, typed wrappers around HTTP calls to backend endpoints +- Accept an optional `signal?: AbortSignal` for request cancellation (GET only) +- Never manage state, handle errors, or retry logic +- Example: `fetchBans(range, page, pageSize, origin, source, signal)` returns typed data + +**Tier 2: Reusable Generic Hooks** (`hooks/useListData.ts`, `hooks/useConfigItem.ts`) +- Provide common state management patterns: fetch + abort + error handling + refresh +- Accept a `fetcher` function (Tier 1 API function) and a `selector` (to extract data from response) +- Return structured results: `{ data, loading, error, refresh }` +- Use for: lists, single-item configs, paginated data — any fetch-and-display pattern +- Automatically abort in-flight requests on unmount +- Example: `useListData({ fetcher: (signal) => fetchBans(..., signal), selector: (res) => res.items })` + +**Tier 3: Domain Hooks** (`hooks/useBans.ts`, `hooks/useActiveBans.ts`, etc.) +- Compose Tier 2 generic hooks and add domain-specific actions +- Manage domain state (e.g., `page`, `total`), expose action callbacks (`banIp`, `unbanIp`) +- Return a domain-specific result shape (e.g., `{ banItems, total, page, setPage, banIp, unbanIp, ... }`) +- Called by pages to feed state to components or context providers +- Example: + ```ts + const fetcher = useCallback((signal) => fetchBans(timeRange, page, ..., signal), [timeRange, page]); + const { items: banItems, ... } = useListData({ fetcher, selector: (res) => res.items }); + return { banItems, total, banIp: doBan, ... }; + ``` + +**Tier 4: Components** (`components/*.tsx`, `pages/*.tsx`) +- Never call API functions or Tier 1 functions directly +- Receive data and actions via **props or context** — never create hooks themselves +- Emit changes via callbacks: `onClick={() => props.onBan(jail, ip)}` +- Remain presentational and fully testable without backend mocks + +**Pattern for action callbacks with data refresh:** +When a component action needs to update the displayed list, have the domain hook refresh automatically: +```ts +const doBan = useCallback( + async (jail: string, ip: string): Promise => { + await banIp(jail, ip); // Tier 1 API call + refresh(); // Re-fetch the list from Tier 2 + }, + [refresh], +); +``` + +**When to use Tier 2 vs Tier 3:** +- Use `useListData` / `useConfigItem` for any new data-fetching scenario that matches the pattern +- Only write Tier 3 hooks when you need domain-specific logic (actions, computed values, complex state) +- Avoid duplicating Tier 3 hooks for identical patterns — refactor to a shared Tier 2 generic instead + +**Anti-patterns to avoid:** +- ❌ Components calling `fetchBans()` directly — violates Tier 4 rule +- ❌ Tier 3 hooks managing all state inline instead of using Tier 2 generics — reduces reusability +- ❌ Passing API functions directly to components — couples components to API contract +- ❌ Multiple domain hooks for the same data without deduplication — causes wasted requests and state desync + --- ## 4. Code Organization diff --git a/frontend/src/hooks/useActiveBans.ts b/frontend/src/hooks/useActiveBans.ts index 70d2897..d1fbe10 100644 --- a/frontend/src/hooks/useActiveBans.ts +++ b/frontend/src/hooks/useActiveBans.ts @@ -2,10 +2,10 @@ * React hook for live active ban list management. */ -import { useCallback, useEffect, useRef, useState } from "react"; +import { useCallback } from "react"; import { banIp, fetchActiveBans, unbanAllBans, unbanIp } from "../api/jails"; -import { handleFetchError } from "../utils/fetchError"; -import type { ActiveBan, UnbanAllResponse } from "../types/jail"; +import { useListData } from "./useListData"; +import type { ActiveBan, UnbanAllResponse, ActiveBanListResponse } from "../types/jail"; export interface UseActiveBansResult { bans: ActiveBan[]; @@ -20,69 +20,54 @@ export interface UseActiveBansResult { /** * Fetch and manage the currently-active ban list. + * + * Provides operations to ban, unban, and refresh the active ban list. + * Automatically re-fetches after state-mutating operations. + * + * @returns Active ban list, loading/error states, and action callbacks. */ export function useActiveBans(): UseActiveBansResult { - const [bans, setBans] = useState([]); - const [total, setTotal] = useState(0); - const [loading, setLoading] = useState(false); - const [error, setError] = useState(null); - const abortRef = useRef(null); + const fetcher = useCallback((signal: AbortSignal) => fetchActiveBans(signal), []); - const load = useCallback(() => { - abortRef.current?.abort(); - const ctrl = new AbortController(); - abortRef.current = ctrl; - setLoading(true); - setError(null); + const selector = useCallback((response: ActiveBanListResponse) => response.bans, []); - fetchActiveBans(ctrl.signal) - .then((res) => { - if (!ctrl.signal.aborted) { - setBans(res.bans); - setTotal(res.total); - } - }) - .catch((err: unknown) => { - if (!ctrl.signal.aborted) { - handleFetchError(err, setError, "Failed to fetch active bans"); - } - }) - .finally(() => { - if (!ctrl.signal.aborted) { - setLoading(false); - } - }); - }, []); + const { items: bans, loading, error, refresh } = useListData({ + fetcher, + selector, + errorMessage: "Failed to fetch active bans", + }); - useEffect(() => { - load(); - return (): void => { - abortRef.current?.abort(); - }; - }, [load]); + const doBan = useCallback( + async (jail: string, ip: string): Promise => { + await banIp(jail, ip); + refresh(); + }, + [refresh], + ); - const doBan = useCallback(async (jail: string, ip: string): Promise => { - await banIp(jail, ip); - load(); - }, [load]); + const doUnban = useCallback( + async (ip: string, jail?: string): Promise => { + await unbanIp(ip, jail); + refresh(); + }, + [refresh], + ); - const doUnban = useCallback(async (ip: string, jail?: string): Promise => { - await unbanIp(ip, jail); - load(); - }, [load]); - - const doUnbanAll = useCallback(async (): Promise => { - const result = await unbanAllBans(); - load(); - return result; - }, [load]); + const doUnbanAll = useCallback( + async (): Promise => { + const result = await unbanAllBans(); + refresh(); + return result; + }, + [refresh], + ); return { bans, - total, + total: bans.length, loading, error, - refresh: load, + refresh, banIp: doBan, unbanIp: doUnban, unbanAll: doUnbanAll, diff --git a/frontend/src/hooks/useBans.ts b/frontend/src/hooks/useBans.ts index 5574e87..8a685df 100644 --- a/frontend/src/hooks/useBans.ts +++ b/frontend/src/hooks/useBans.ts @@ -2,14 +2,14 @@ * `useBans` hook. * * Fetches and manages paginated ban-list data from the dashboard endpoint. - * Re-fetches automatically when `timeRange` or `page` changes. + * Re-fetches automatically when `timeRange`, `page`, `origin`, or `source` changes. */ -import { useCallback, useEffect, useRef, useState } from "react"; +import { useCallback, useState } from "react"; import { fetchBans } from "../api/dashboard"; -import { handleFetchError } from "../utils/fetchError"; +import { useListData } from "./useListData"; import { BAN_PAGE_SIZE } from "../utils/constants"; -import type { DashboardBanItem, TimeRange, BanOriginFilter } from "../types/ban"; +import type { DashboardBanItem, TimeRange, BanOriginFilter, DashboardBanListResponse } from "../types/ban"; /** Return value shape for {@link useBans}. */ export interface UseBansResult { @@ -32,68 +32,39 @@ export interface UseBansResult { /** * Fetch and manage dashboard ban-list data. * - * Automatically re-fetches when `timeRange`, `origin`, or `page` changes. + * Automatically re-fetches when `timeRange`, `origin`, `page`, or `source` changes. * * @param timeRange - Time-range preset that controls how far back to look. * @param origin - Origin filter (default `"all"`). - * @returns Current data, pagination state, loading flag, and a `refresh` - * callback. + * @param source - Data source: `"fail2ban"` (live) or `"archive"` (historical). + * @returns Current data, pagination state, loading flag, and a `refresh` callback. */ export function useBans( timeRange: TimeRange, origin: BanOriginFilter = "all", source: "fail2ban" | "archive" = "fail2ban", ): UseBansResult { - const [banItems, setBanItems] = useState([]); - const [total, setTotal] = useState(0); const [page, setPage] = useState(1); - const [loading, setLoading] = useState(true); - const [error, setError] = useState(null); - const abortRef = useRef(null); + const [total, setTotal] = useState(0); - // Reset page when time range, origin filter, or source changes. - useEffect(() => { - setPage(1); - }, [timeRange, origin, source]); + const fetcher = useCallback( + (signal: AbortSignal) => fetchBans(timeRange, page, BAN_PAGE_SIZE, origin, source, signal), + [timeRange, page, origin, source], + ); - const doFetch = useCallback(async (): Promise => { - abortRef.current?.abort(); - const controller = new AbortController(); - abortRef.current = controller; + const selector = useCallback((response: DashboardBanListResponse) => response.items, []); - setLoading(true); - setError(null); - - try { - const data = await fetchBans(timeRange, page, BAN_PAGE_SIZE, origin, source, controller.signal); - if (controller.signal.aborted) return; - setBanItems(data.items); - setTotal(data.total); - } catch (err: unknown) { - if (controller.signal.aborted) return; - handleFetchError(err, setError, "Failed to fetch bans"); - } finally { - if (!controller.signal.aborted) { - setLoading(false); - } - } - }, [timeRange, page, origin, source]); - - // Stable ref to the latest doFetch so the refresh callback is always current. - const doFetchRef = useRef(doFetch); - doFetchRef.current = doFetch; - - useEffect(() => { - void doFetch(); - return (): void => { - abortRef.current?.abort(); - }; - }, [doFetch]); - - const refresh = useCallback((): void => { - void doFetchRef.current(); + const onSuccess = useCallback((response: DashboardBanListResponse) => { + setTotal(response.total); }, []); + const { items: banItems, loading, error, refresh } = useListData({ + fetcher, + selector, + errorMessage: "Failed to fetch bans", + onSuccess, + }); + return { banItems, total,