From 9c5757eeb050f1c58a3aceff3a2396d96ddae214 Mon Sep 17 00:00:00 2001 From: Lukas Date: Thu, 23 Apr 2026 09:51:16 +0200 Subject: [PATCH] Refactor useHistory hook: replace HistoryQuery with explicit parameters and add documentation - Split useHistory interface to accept explicit parameters (page, pageSize, range, origin, jail, ip, source) instead of HistoryQuery object - Add comprehensive JSDoc for useHistory function - Update HistoryPage and tests to use new parameter structure - Move TaskList documentation from Tasks.md to Web-Development.md - Improve type safety with explicit TimeRange and BanOriginFilter types Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- Docs/Tasks.md | 28 --------- Docs/Web-Development.md | 31 ++++++++++ frontend/src/hooks/useHistory.ts | 57 +++++++++++++++---- frontend/src/pages/HistoryPage.tsx | 57 +++++++------------ .../src/pages/__tests__/HistoryPage.test.tsx | 39 ++++++------- 5 files changed, 119 insertions(+), 93 deletions(-) diff --git a/Docs/Tasks.md b/Docs/Tasks.md index a6d6990..dc20845 100644 --- a/Docs/Tasks.md +++ b/Docs/Tasks.md @@ -1,31 +1,3 @@ -### TASK-QUALITY-02 — `useConfigItem.save()` Briefly Shows Session-Expiry as Save Error - -**Where found** -`frontend/src/hooks/useConfigItem.ts` lines 70–80. The `save()` function's catch block calls `setSaveError(err.message)` for all errors including `ApiError(401)` and `ApiError(403)`. The HTTP client layer dispatches `SESSION_EXPIRED_EVENT` on those status codes, which triggers auth handling, but `setSaveError` still runs first and may briefly display an "Unauthorized" or similar message before the navigation occurs. - -**Goal** -Check for auth errors before setting save error state: -```ts -} catch (err: unknown) { - if (isAuthError(err)) throw err; // let auth handler deal with it - const message = err instanceof Error ? err.message : "Failed to save data"; - setSaveError(message); - throw err; -} -``` - -**Possible traps and issues** -- Rethrowing auth errors is correct here since the caller might also have error handling. Confirm that all callers of `save()` handle the re-thrown auth error gracefully (typically by not doing anything — the session expiry flow handles navigation). -- Import `isAuthError` from `../api/client`. - -**Docs changes needed** -None required. - -**Why this is needed** -Briefly flashing "Unauthorized" in a form's save-error field is confusing UX when the correct outcome is a redirect to the login page. - ---- - ### TASK-QUALITY-03 — `useHistory` Object Identity Dependency Footgun **Where found** diff --git a/Docs/Web-Development.md b/Docs/Web-Development.md index 2573ca6..cced1a4 100644 --- a/Docs/Web-Development.md +++ b/Docs/Web-Development.md @@ -328,6 +328,37 @@ export default BanTable; - Always include the correct dependency arrays in `useEffect`, `useMemo`, and `useCallback`. Disable the ESLint exhaustive-deps rule **only** with a comment explaining why. - Clean up side effects (subscriptions, timers, abort controllers) in the `useEffect` cleanup function. +### Object Parameters in Hooks (Reference Stability) + +**When a hook accepts an object parameter, include it in dependency arrays only if it is guaranteed to be a stable reference.** If callers pass inline object literals, the object reference changes on every render, causing unnecessary re-fetches and potential infinite loops. + +**Preferred solution:** Design hook signatures to accept individual **primitive parameters** instead of objects. This makes incorrect usage a compile-time error: + +```ts +// ❌ Footgun: query object causes infinite fetches if caller uses inline literals +export function useHistory(query: HistoryQuery = {}): UseHistoryResult { + const load = useCallback(() => { /* ... */ }, [query]); +} + +// Called like this (creates new object every render): +const result = useHistory({ page: 1, jail: selectedJail }); + +// ✅ Safe: individual primitives can't be accidentally unstable +export function useHistory( + page: number = 1, + pageSize: number = 50, + jail?: string, +): UseHistoryResult { + const load = useCallback(() => { /* ... */ }, [page, pageSize, jail]); +} + +// Called like this (all primitives are stable): +const result = useHistory(page, PAGE_SIZE, jailFilter); +``` + +If refactoring to individual parameters is not feasible, document the constraint clearly in JSDoc and require callers to stabilize the reference using `useMemo`. + + ```tsx // hooks/useBans.ts import { useState, useEffect } from "react"; diff --git a/frontend/src/hooks/useHistory.ts b/frontend/src/hooks/useHistory.ts index 33089d6..75b8a7a 100644 --- a/frontend/src/hooks/useHistory.ts +++ b/frontend/src/hooks/useHistory.ts @@ -5,11 +5,8 @@ import { useCallback, useEffect, useRef, useState } from "react"; import { fetchHistory, fetchIpHistory } from "../api/history"; import { handleFetchError } from "../utils/fetchError"; -import type { - HistoryBanItem, - HistoryQuery, - IpDetailResponse, -} from "../types/history"; +import type { HistoryBanItem, IpDetailResponse } from "../types/history"; +import type { BanOriginFilter, TimeRange } from "../types/ban"; // --------------------------------------------------------------------------- // useHistory — paginated list @@ -25,10 +22,30 @@ export interface UseHistoryResult { refresh: () => void; } -export function useHistory(query: HistoryQuery = {}): UseHistoryResult { +/** + * Fetch and manage paginated ban history with optional filters. + * + * @param page - Current page number (1-indexed) + * @param pageSize - Items per page + * @param range - Time range filter (e.g., "7d", "30d") + * @param origin - Ban origin filter (e.g., "fail2ban", "blocklist") + * @param jail - Jail name filter (optional) + * @param ip - IP address filter (optional) + * @param source - Data source ("fail2ban" | "archive") + * @returns History data with pagination and error state + */ +export function useHistory( + page: number = 1, + pageSize: number = 50, + range?: TimeRange, + origin?: BanOriginFilter, + jail?: string, + ip?: string, + source: "fail2ban" | "archive" = "archive", +): UseHistoryResult { const [items, setItems] = useState([]); const [total, setTotal] = useState(0); - const [page, setPage] = useState(query.page ?? 1); + const [currentPage, setCurrentPage] = useState(page); const [loading, setLoading] = useState(true); const [error, setError] = useState(null); const abortRef = useRef(null); @@ -39,7 +56,18 @@ export function useHistory(query: HistoryQuery = {}): UseHistoryResult { setLoading(true); setError(null); - fetchHistory({ ...query, page }, abortRef.current.signal) + fetchHistory( + { + page: currentPage, + page_size: pageSize, + range, + origin, + jail, + ip, + source, + }, + abortRef.current.signal, + ) .then((resp) => { setItems(resp.items); setTotal(resp.total); @@ -50,7 +78,8 @@ export function useHistory(query: HistoryQuery = {}): UseHistoryResult { .finally((): void => { setLoading(false); }); - }, [query, page]); + }, [currentPage, pageSize, range, origin, jail, ip, source]); + useEffect((): (() => void) => { load(); @@ -59,7 +88,15 @@ export function useHistory(query: HistoryQuery = {}): UseHistoryResult { }; }, [load]); - return { items, total, page, loading, error, setPage, refresh: load }; + return { + items, + total, + page: currentPage, + loading, + error, + setPage: setCurrentPage, + refresh: load, + }; } // --------------------------------------------------------------------------- diff --git a/frontend/src/pages/HistoryPage.tsx b/frontend/src/pages/HistoryPage.tsx index f20f2f0..5511d8b 100644 --- a/frontend/src/pages/HistoryPage.tsx +++ b/frontend/src/pages/HistoryPage.tsx @@ -6,7 +6,7 @@ * Rows with repeatedly-banned IPs are highlighted in amber. */ -import { useCallback, useEffect, useMemo, useRef, useState } from "react"; +import { useCallback, useEffect, useMemo, useState } from "react"; import { Badge, Button, @@ -32,8 +32,7 @@ import { import { DashboardFilterBar } from "../components/DashboardFilterBar"; import { useHistory } from "../hooks/useHistory"; import { IpDetailView } from "./history/IpDetailView"; -import { areHistoryQueriesEqual } from "../utils/queryUtils"; -import type { HistoryBanItem, HistoryQuery, TimeRange } from "../types/history"; +import type { HistoryBanItem, TimeRange } from "../types/history"; import type { BanOriginFilter } from "../types/ban"; // --------------------------------------------------------------------------- @@ -199,20 +198,21 @@ export function HistoryPage(): React.JSX.Element { const [originFilter, setOriginFilter] = useState("all"); const [jailFilter, setJailFilter] = useState(""); const [ipFilter, setIpFilter] = useState(""); - const defaultQuery: HistoryQuery = { - range: "7d", - source: "archive", - page_size: PAGE_SIZE, - page: 1, - }; - const [appliedQuery, setAppliedQuery] = useState(defaultQuery); - const appliedQueryRef = useRef(defaultQuery); + const [page, setPage] = useState(1); // Per-IP detail navigation const [selectedIp, setSelectedIp] = useState(null); - const { items, total, page, loading, error, setPage, refresh } = - useHistory(appliedQuery); + const { items, total, page: currentPage, loading, error, setPage: setCurrentPage, refresh } = + useHistory( + page, + PAGE_SIZE, + range, + originFilter !== "all" ? originFilter : undefined, + jailFilter.trim() || undefined, + ipFilter.trim() || undefined, + "archive", + ); const handleIpClick = useCallback((ip: string): void => { setSelectedIp(ip); @@ -223,25 +223,10 @@ export function HistoryPage(): React.JSX.Element { [handleIpClick, styles], ); + // Reset to page 1 when filters change useEffect((): void => { - const nextQuery: HistoryQuery = { - range, - origin: originFilter !== "all" ? originFilter : undefined, - jail: jailFilter.trim() || undefined, - ip: ipFilter.trim() || undefined, - source: "archive", - page: 1, - page_size: PAGE_SIZE, - }; - - if (areHistoryQueriesEqual(nextQuery, appliedQueryRef.current)) { - return; - } - setPage(1); - setAppliedQuery(nextQuery); - appliedQueryRef.current = nextQuery; - }, [range, originFilter, jailFilter, ipFilter, setPage]); + }, [range, originFilter, jailFilter, ipFilter]); const totalPages = Math.max(1, Math.ceil(total / PAGE_SIZE)); @@ -310,7 +295,7 @@ export function HistoryPage(): React.JSX.Element { {!loading && !error && ( {String(total)} record{total !== 1 ? "s" : ""} found · - Page {String(page)} of {String(totalPages)} · + Page {String(currentPage)} of {String(totalPages)} · Rows highlighted in yellow have {String(HIGH_BAN_THRESHOLD)}+ repeat bans )} @@ -362,21 +347,21 @@ export function HistoryPage(): React.JSX.Element { icon={} appearance="subtle" size="small" - disabled={page <= 1} + disabled={currentPage <= 1} onClick={(): void => { - setPage(page - 1); + setCurrentPage(currentPage - 1); }} /> - Page {String(page)} / {String(totalPages)} + Page {String(currentPage)} / {String(totalPages)}