fix(#16): Establish consistent API usage layering patterns
- Refactor useActiveBans to use useListData generic hook instead of inline state management - Refactor useBans to use useListData generic hook for consistency - Add comprehensive 'API Usage Layering' section to Web-Development.md documenting: - Tier 1: API Functions (pure wrappers around HTTP calls) - Tier 2: Reusable Generic Hooks (useListData, useConfigItem for common patterns) - Tier 3: Domain Hooks (compose Tier 2 with domain-specific logic) - Tier 4: Components (receive data/actions via props or context) - Document pattern for action callbacks with automatic data refresh - List anti-patterns to avoid for future consistency These changes improve composability, testability, and reduce code duplication by establishing a clear convention for data-fetching patterns across the frontend. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
@@ -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
|
## 16) API usage pattern is inconsistent across components/hooks
|
||||||
- Where found:
|
- Where found:
|
||||||
- [frontend/src/pages/JailsPage.tsx](frontend/src/pages/JailsPage.tsx)
|
- [frontend/src/pages/JailsPage.tsx](frontend/src/pages/JailsPage.tsx)
|
||||||
|
|||||||
@@ -187,6 +187,65 @@ export function useSharedSetupStatus(): UseSharedSetupStatusResult {
|
|||||||
- Cache TTL should be relatively short (30 seconds) unless the data is truly static
|
- 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
|
- 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<void> => {
|
||||||
|
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
|
## 4. Code Organization
|
||||||
|
|||||||
@@ -2,10 +2,10 @@
|
|||||||
* React hook for live active ban list management.
|
* 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 { banIp, fetchActiveBans, unbanAllBans, unbanIp } from "../api/jails";
|
||||||
import { handleFetchError } from "../utils/fetchError";
|
import { useListData } from "./useListData";
|
||||||
import type { ActiveBan, UnbanAllResponse } from "../types/jail";
|
import type { ActiveBan, UnbanAllResponse, ActiveBanListResponse } from "../types/jail";
|
||||||
|
|
||||||
export interface UseActiveBansResult {
|
export interface UseActiveBansResult {
|
||||||
bans: ActiveBan[];
|
bans: ActiveBan[];
|
||||||
@@ -20,69 +20,54 @@ export interface UseActiveBansResult {
|
|||||||
|
|
||||||
/**
|
/**
|
||||||
* Fetch and manage the currently-active ban list.
|
* 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 {
|
export function useActiveBans(): UseActiveBansResult {
|
||||||
const [bans, setBans] = useState<ActiveBan[]>([]);
|
const fetcher = useCallback((signal: AbortSignal) => fetchActiveBans(signal), []);
|
||||||
const [total, setTotal] = useState(0);
|
|
||||||
const [loading, setLoading] = useState(false);
|
|
||||||
const [error, setError] = useState<string | null>(null);
|
|
||||||
const abortRef = useRef<AbortController | null>(null);
|
|
||||||
|
|
||||||
const load = useCallback(() => {
|
const selector = useCallback((response: ActiveBanListResponse) => response.bans, []);
|
||||||
abortRef.current?.abort();
|
|
||||||
const ctrl = new AbortController();
|
|
||||||
abortRef.current = ctrl;
|
|
||||||
setLoading(true);
|
|
||||||
setError(null);
|
|
||||||
|
|
||||||
fetchActiveBans(ctrl.signal)
|
const { items: bans, loading, error, refresh } = useListData<ActiveBanListResponse, ActiveBan>({
|
||||||
.then((res) => {
|
fetcher,
|
||||||
if (!ctrl.signal.aborted) {
|
selector,
|
||||||
setBans(res.bans);
|
errorMessage: "Failed to fetch active 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);
|
|
||||||
}
|
|
||||||
});
|
|
||||||
}, []);
|
|
||||||
|
|
||||||
useEffect(() => {
|
const doBan = useCallback(
|
||||||
load();
|
async (jail: string, ip: string): Promise<void> => {
|
||||||
return (): void => {
|
await banIp(jail, ip);
|
||||||
abortRef.current?.abort();
|
refresh();
|
||||||
};
|
},
|
||||||
}, [load]);
|
[refresh],
|
||||||
|
);
|
||||||
|
|
||||||
const doBan = useCallback(async (jail: string, ip: string): Promise<void> => {
|
const doUnban = useCallback(
|
||||||
await banIp(jail, ip);
|
async (ip: string, jail?: string): Promise<void> => {
|
||||||
load();
|
await unbanIp(ip, jail);
|
||||||
}, [load]);
|
refresh();
|
||||||
|
},
|
||||||
|
[refresh],
|
||||||
|
);
|
||||||
|
|
||||||
const doUnban = useCallback(async (ip: string, jail?: string): Promise<void> => {
|
const doUnbanAll = useCallback(
|
||||||
await unbanIp(ip, jail);
|
async (): Promise<UnbanAllResponse> => {
|
||||||
load();
|
const result = await unbanAllBans();
|
||||||
}, [load]);
|
refresh();
|
||||||
|
return result;
|
||||||
const doUnbanAll = useCallback(async (): Promise<UnbanAllResponse> => {
|
},
|
||||||
const result = await unbanAllBans();
|
[refresh],
|
||||||
load();
|
);
|
||||||
return result;
|
|
||||||
}, [load]);
|
|
||||||
|
|
||||||
return {
|
return {
|
||||||
bans,
|
bans,
|
||||||
total,
|
total: bans.length,
|
||||||
loading,
|
loading,
|
||||||
error,
|
error,
|
||||||
refresh: load,
|
refresh,
|
||||||
banIp: doBan,
|
banIp: doBan,
|
||||||
unbanIp: doUnban,
|
unbanIp: doUnban,
|
||||||
unbanAll: doUnbanAll,
|
unbanAll: doUnbanAll,
|
||||||
|
|||||||
@@ -2,14 +2,14 @@
|
|||||||
* `useBans` hook.
|
* `useBans` hook.
|
||||||
*
|
*
|
||||||
* Fetches and manages paginated ban-list data from the dashboard endpoint.
|
* 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 { fetchBans } from "../api/dashboard";
|
||||||
import { handleFetchError } from "../utils/fetchError";
|
import { useListData } from "./useListData";
|
||||||
import { BAN_PAGE_SIZE } from "../utils/constants";
|
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}. */
|
/** Return value shape for {@link useBans}. */
|
||||||
export interface UseBansResult {
|
export interface UseBansResult {
|
||||||
@@ -32,68 +32,39 @@ export interface UseBansResult {
|
|||||||
/**
|
/**
|
||||||
* Fetch and manage dashboard ban-list data.
|
* 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 timeRange - Time-range preset that controls how far back to look.
|
||||||
* @param origin - Origin filter (default `"all"`).
|
* @param origin - Origin filter (default `"all"`).
|
||||||
* @returns Current data, pagination state, loading flag, and a `refresh`
|
* @param source - Data source: `"fail2ban"` (live) or `"archive"` (historical).
|
||||||
* callback.
|
* @returns Current data, pagination state, loading flag, and a `refresh` callback.
|
||||||
*/
|
*/
|
||||||
export function useBans(
|
export function useBans(
|
||||||
timeRange: TimeRange,
|
timeRange: TimeRange,
|
||||||
origin: BanOriginFilter = "all",
|
origin: BanOriginFilter = "all",
|
||||||
source: "fail2ban" | "archive" = "fail2ban",
|
source: "fail2ban" | "archive" = "fail2ban",
|
||||||
): UseBansResult {
|
): UseBansResult {
|
||||||
const [banItems, setBanItems] = useState<DashboardBanItem[]>([]);
|
|
||||||
const [total, setTotal] = useState<number>(0);
|
|
||||||
const [page, setPage] = useState<number>(1);
|
const [page, setPage] = useState<number>(1);
|
||||||
const [loading, setLoading] = useState<boolean>(true);
|
const [total, setTotal] = useState<number>(0);
|
||||||
const [error, setError] = useState<string | null>(null);
|
|
||||||
const abortRef = useRef<AbortController | null>(null);
|
|
||||||
|
|
||||||
// Reset page when time range, origin filter, or source changes.
|
const fetcher = useCallback(
|
||||||
useEffect(() => {
|
(signal: AbortSignal) => fetchBans(timeRange, page, BAN_PAGE_SIZE, origin, source, signal),
|
||||||
setPage(1);
|
[timeRange, page, origin, source],
|
||||||
}, [timeRange, origin, source]);
|
);
|
||||||
|
|
||||||
const doFetch = useCallback(async (): Promise<void> => {
|
const selector = useCallback((response: DashboardBanListResponse) => response.items, []);
|
||||||
abortRef.current?.abort();
|
|
||||||
const controller = new AbortController();
|
|
||||||
abortRef.current = controller;
|
|
||||||
|
|
||||||
setLoading(true);
|
const onSuccess = useCallback((response: DashboardBanListResponse) => {
|
||||||
setError(null);
|
setTotal(response.total);
|
||||||
|
|
||||||
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 { items: banItems, loading, error, refresh } = useListData<DashboardBanListResponse, DashboardBanItem>({
|
||||||
|
fetcher,
|
||||||
|
selector,
|
||||||
|
errorMessage: "Failed to fetch bans",
|
||||||
|
onSuccess,
|
||||||
|
});
|
||||||
|
|
||||||
return {
|
return {
|
||||||
banItems,
|
banItems,
|
||||||
total,
|
total,
|
||||||
|
|||||||
Reference in New Issue
Block a user