diff --git a/Docs/Tasks.md b/Docs/Tasks.md index 0b1b00d..f68fd66 100644 --- a/Docs/Tasks.md +++ b/Docs/Tasks.md @@ -1,26 +1,3 @@ -### T-17 · `useHistory` is missing abort-signal guards — stale state update bug - -**Where found:** `frontend/src/hooks/useHistory.ts` — `.then()`, `.catch()`, `.finally()` callbacks update state without checking `abortRef.current.signal.aborted` - -**Why this is needed:** Every other data-fetching hook in the codebase guards all state-update callbacks against aborted signals. `useHistory` does not. If the component unmounts mid-request, `setItems`, `setTotal`, `setLoading` will all fire on an unmounted component. In React 18 this is a no-op but it still indicates a broken invariant and `handleFetchError` could misclassify the abort as a real error (depends on whether `fetch` threw `AbortError` or the API module swallowed it). - -**Goal:** All callbacks in `useHistory` check the abort signal before mutating state. - -**What to do:** -1. Capture the controller in a local variable inside `load()` (already done: `abortRef.current = new AbortController()`). -2. In `.then()`: add `if (abortRef.current.signal.aborted) return;` before `setItems(...)`. -3. In `.catch()`: add the same guard before `handleFetchError(...)`. -4. In `.finally()`: add `if (!abortRef.current.signal.aborted)` before `setLoading(false)`. - -**Possible traps and issues:** -- `abortRef.current` may have been replaced by a new controller before the callback fires. Capture the controller in a closure variable at the top of `load()`: `const controller = abortRef.current`. - -**Docs changes needed:** None. - -**Doc references:** `frontend/src/hooks/useHistory.ts` - ---- - ### T-18 · Merge `useDashboardCountryData` and `useMapData` — near-identical hooks **Where found:** `frontend/src/hooks/useDashboardCountryData.ts` and `frontend/src/hooks/useMapData.ts` diff --git a/frontend/src/hooks/__tests__/useBansByCountry.test.ts b/frontend/src/hooks/__tests__/useBansByCountry.test.ts new file mode 100644 index 0000000..c80562c --- /dev/null +++ b/frontend/src/hooks/__tests__/useBansByCountry.test.ts @@ -0,0 +1,83 @@ +import { describe, expect, it, vi, beforeEach } from "vitest"; +import { renderHook, act, waitFor } from "@testing-library/react"; +import type { BansByCountryResponse } from "../../types/map"; +import { useBansByCountry } from "../useBansByCountry"; +import * as api from "../../api/map"; + +vi.mock("../../api/map"); + +describe("useBansByCountry", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it("fetches data automatically on mount", async () => { + const fetchMock = vi.mocked(api.fetchBansByCountry); + const response: BansByCountryResponse = { + countries: { US: 1, GB: 2 }, + country_names: { US: "United States", GB: "United Kingdom" }, + bans: [], + total: 3, + }; + + fetchMock.mockResolvedValueOnce(response); + + const { result } = renderHook(() => useBansByCountry("24h", "all", "fail2ban")); + + expect(result.current.loading).toBe(true); + + await waitFor(() => { + expect(result.current.loading).toBe(false); + }); + + expect(result.current.countries).toEqual({ US: 1, GB: 2 }); + expect(result.current.countryNames).toEqual({ US: "United States", GB: "United Kingdom" }); + expect(result.current.total).toBe(3); + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + + it("does not fetch automatically when autoFetch is false", async () => { + const fetchMock = vi.mocked(api.fetchBansByCountry); + const response: BansByCountryResponse = { + countries: { US: 1 }, + country_names: { US: "United States" }, + bans: [], + total: 1, + }; + + fetchMock.mockResolvedValueOnce(response); + + const { result } = renderHook(() => useBansByCountry("24h", "all", "fail2ban", undefined, false)); + + expect(result.current.loading).toBe(true); + expect(fetchMock).not.toHaveBeenCalled(); + + act(() => { + result.current.refresh(); + }); + + await waitFor(() => { + expect(result.current.loading).toBe(false); + }); + + expect(result.current.countries).toEqual({ US: 1 }); + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + + it("handles fetch errors correctly", async () => { + const fetchMock = vi.mocked(api.fetchBansByCountry); + const error = new Error("Network error"); + + fetchMock.mockRejectedValueOnce(error); + + const { result } = renderHook(() => useBansByCountry("24h", "all", "fail2ban")); + + await waitFor(() => { + expect(result.current.loading).toBe(false); + }); + + expect(result.current.error).not.toBeNull(); + expect(result.current.countries).toEqual({}); + expect(result.current.total).toBe(0); + }); +}); diff --git a/frontend/src/hooks/__tests__/useDashboardCountryData.test.ts b/frontend/src/hooks/__tests__/useDashboardCountryData.test.ts new file mode 100644 index 0000000..e1e5919 --- /dev/null +++ b/frontend/src/hooks/__tests__/useDashboardCountryData.test.ts @@ -0,0 +1,83 @@ +import { describe, expect, it, vi, beforeEach } from "vitest"; +import { renderHook, act, waitFor } from "@testing-library/react"; +import type { BansByCountryResponse } from "../../types/map"; +import { useDashboardCountryData } from "../useDashboardCountryData"; +import * as api from "../../api/map"; + +vi.mock("../../api/map"); + +describe("useDashboardCountryData", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it("fetches data and exposes country data with reload function", async () => { + const fetchMock = vi.mocked(api.fetchBansByCountry); + const response: BansByCountryResponse = { + countries: { US: 1, GB: 2 }, + country_names: { US: "United States", GB: "United Kingdom" }, + bans: [ + { + ip: "1.2.3.4", + jail: "sshd", + banned_at: "2024-01-01T00:00:00Z", + service: null, + country_code: "US", + country_name: "United States", + asn: "AS123", + org: "Org", + ban_count: 1, + origin: "selfblock", + }, + ], + total: 3, + }; + + fetchMock.mockResolvedValueOnce(response); + + const { result } = renderHook(() => useDashboardCountryData("24h", "all", "fail2ban")); + + expect(result.current.loading).toBe(true); + + await waitFor(() => { + expect(result.current.loading).toBe(false); + }); + + expect(result.current.countries).toEqual({ US: 1, GB: 2 }); + expect(result.current.countryNames).toEqual({ US: "United States", GB: "United Kingdom" }); + expect(result.current.bans).toHaveLength(1); + expect(result.current.total).toBe(3); + expect(fetchMock).toHaveBeenCalledTimes(1); + }); + + it("supports the reload function for manual refresh", async () => { + const fetchMock = vi.mocked(api.fetchBansByCountry); + const response: BansByCountryResponse = { + countries: { US: 1 }, + country_names: { US: "United States" }, + bans: [], + total: 1, + }; + + fetchMock.mockResolvedValueOnce(response); + fetchMock.mockResolvedValueOnce({ ...response, total: 2 }); + + const { result } = renderHook(() => useDashboardCountryData("24h", "all", "fail2ban")); + + await waitFor(() => { + expect(result.current.loading).toBe(false); + }); + + expect(result.current.total).toBe(1); + + act(() => { + result.current.reload(); + }); + + await waitFor(() => { + expect(result.current.total).toBe(2); + }); + + expect(fetchMock).toHaveBeenCalledTimes(2); + }); +}); diff --git a/frontend/src/hooks/useBansByCountry.ts b/frontend/src/hooks/useBansByCountry.ts new file mode 100644 index 0000000..831f87b --- /dev/null +++ b/frontend/src/hooks/useBansByCountry.ts @@ -0,0 +1,112 @@ +/** + * Base hook for fetching ban-by-country aggregates. + * + * This hook encapsulates the core logic for fetching country-level ban statistics, + * maintaining abort-controller semantics, and exposing both the full response and + * its individual fields for convenient destructuring. + * + * For a debounced version, use {@link useMapData}. + * For a dashboard-specific wrapper, use {@link useDashboardCountryData}. + */ + +import { useCallback, useEffect, useRef, useState } from "react"; +import { fetchBansByCountry } from "../api/map"; +import { handleFetchError } from "../utils/fetchError"; +import type { BanOriginFilter, TimeRange } from "../types/ban"; +import type { BansByCountryResponse, MapBanItem } from "../types/map"; + +/** + * Return value shape for {@link useBansByCountry}. + */ +export interface UseBansByCountryResult { + /** Per-country ban counts (ISO alpha-2 → count). */ + countries: Record; + /** ISO alpha-2 → country name mapping. */ + countryNames: Record; + /** All ban records in the selected window. */ + bans: MapBanItem[]; + /** Total ban count. */ + total: number; + /** True while a fetch is in flight. */ + loading: boolean; + /** Error message or null. */ + error: string | null; + /** Trigger a manual re-fetch. */ + refresh: () => void; + /** The full response object (for callers that need raw data). */ + data: BansByCountryResponse | null; +} + +/** + * Fetch and manage ban-by-country data. + * + * This is the base hook shared by both `useDashboardCountryData` and `useMapData`. + * It handles the fetching, abort-controller pattern, and state management. + * Callers can add debouncing or other behaviour on top. + * + * @param timeRange - Time-range preset: `"24h"`, `"7d"`, `"30d"`, or `"365d"`. + * @param origin - Origin filter: `"all"`, `"blocklist"`, or `"selfblock"`. + * @param source - Data source: `"fail2ban"` or `"archive"`. + * @param countryCode - Optional ISO alpha-2 country code to filter by. + * @param autoFetch - If false, caller must invoke refresh() manually (default: true). + * @returns Country data, loading state, error, and refresh function. + */ +export function useBansByCountry( + timeRange: TimeRange, + origin: BanOriginFilter, + source: "fail2ban" | "archive" = "fail2ban", + countryCode?: string, + autoFetch: boolean = true, +): UseBansByCountryResult { + const [data, setData] = useState(null); + const [loading, setLoading] = useState(true); + const [error, setError] = useState(null); + const abortRef = useRef(null); + + const refresh = useCallback((): void => { + // Abort any in-flight request from a previous filter selection. + abortRef.current?.abort(); + const controller = new AbortController(); + abortRef.current = controller; + + setLoading(true); + setError(null); + + fetchBansByCountry(timeRange, origin, source, countryCode, controller.signal) + .then((resp) => { + if (!controller.signal.aborted) { + setData(resp); + } + }) + .catch((err: unknown) => { + if (!controller.signal.aborted) { + handleFetchError(err, setError, "Failed to fetch ban-by-country data"); + } + }) + .finally((): void => { + if (!controller.signal.aborted) { + setLoading(false); + } + }); + }, [timeRange, origin, source, countryCode]); + + useEffect((): (() => void) => { + if (autoFetch) { + refresh(); + } + return (): void => { + abortRef.current?.abort(); + }; + }, [refresh, autoFetch]); + + return { + countries: data?.countries ?? {}, + countryNames: data?.country_names ?? {}, + bans: data?.bans ?? [], + total: data?.total ?? 0, + loading, + error, + refresh, + data, + }; +} diff --git a/frontend/src/hooks/useDashboardCountryData.ts b/frontend/src/hooks/useDashboardCountryData.ts index aab6b32..61da5c0 100644 --- a/frontend/src/hooks/useDashboardCountryData.ts +++ b/frontend/src/hooks/useDashboardCountryData.ts @@ -1,17 +1,17 @@ /** * `useDashboardCountryData` hook. * - * Fetches ban-by-country aggregates for dashboard chart components. Unlike - * `useMapData`, this hook has no debouncing or map-specific state. + * Fetches ban-by-country aggregates for dashboard chart components. This is a thin + * wrapper around {@link useBansByCountry} that preserves the original API and naming. + * + * Unlike `useMapData`, this hook has no debouncing. * * Re-fetches automatically when `timeRange` or `origin` changes. */ -import { useCallback, useState } from "react"; -import { fetchBansByCountry } from "../api/map"; -import { useListData } from "./useListData"; import type { BanOriginFilter, TimeRange } from "../types/ban"; -import type { BansByCountryResponse, MapBanItem } from "../types/map"; +import type { MapBanItem } from "../types/map"; +import { useBansByCountry } from "./useBansByCountry"; /** Return value shape for {@link useDashboardCountryData}. */ export interface UseDashboardCountryDataResult { @@ -36,6 +36,7 @@ export interface UseDashboardCountryDataResult { * * @param timeRange - Time-range preset: `"24h"`, `"7d"`, `"30d"`, or `"365d"`. * @param origin - Origin filter: `"all"`, `"blocklist"`, or `"selfblock"`. + * @param source - Data source: `"fail2ban"` or `"archive"`. * @returns Aggregated country data, ban list, loading state, and error. */ export function useDashboardCountryData( @@ -43,30 +44,8 @@ export function useDashboardCountryData( origin: BanOriginFilter, source: "fail2ban" | "archive" = "fail2ban", ): UseDashboardCountryDataResult { - const [countries, setCountries] = useState>({}); - const [countryNames, setCountryNames] = useState>({}); - const [total, setTotal] = useState(0); - - const fetcher = useCallback( - (signal: AbortSignal) => - fetchBansByCountry(timeRange, origin, source, undefined, signal), - [timeRange, origin, source], - ); - - const selector = useCallback((response: BansByCountryResponse) => response.bans, []); - - const onSuccess = useCallback((response: BansByCountryResponse) => { - setCountries(response.countries); - setCountryNames(response.country_names); - setTotal(response.total); - }, []); - - const { items: bans, loading, error, refresh } = useListData({ - fetcher, - selector, - errorMessage: "Failed to fetch dashboard country data", - onSuccess, - }); + const { countries, countryNames, bans, total, loading, error, refresh } = + useBansByCountry(timeRange, origin, source, undefined); return { countries, countryNames, bans, total, loading, error, reload: refresh }; } diff --git a/frontend/src/hooks/useMapData.ts b/frontend/src/hooks/useMapData.ts index 0702cc9..42f1288 100644 --- a/frontend/src/hooks/useMapData.ts +++ b/frontend/src/hooks/useMapData.ts @@ -1,12 +1,15 @@ /** - * `useMapData` hook — fetches and manages ban-by-country data. + * `useMapData` hook — fetches and manages ban-by-country data with debouncing. + * + * This hook wraps {@link useBansByCountry} and adds a 300ms debounce to reduce + * unnecessary fetches when filters change rapidly (e.g., during user interaction + * with UI controls). */ -import { useCallback, useEffect, useRef, useState } from "react"; -import { fetchBansByCountry } from "../api/map"; -import { handleFetchError } from "../utils/fetchError"; -import type { BansByCountryResponse, MapBanItem, TimeRange } from "../types/map"; -import type { BanOriginFilter } from "../types/ban"; +import { useCallback, useEffect, useRef } from "react"; +import { useBansByCountry } from "./useBansByCountry"; +import type { BanOriginFilter, TimeRange } from "../types/ban"; +import type { MapBanItem } from "../types/map"; // --------------------------------------------------------------------------- // Constants @@ -46,38 +49,23 @@ export function useMapData( source: "fail2ban" | "archive" = "fail2ban", countryCode?: string, ): UseMapDataResult { - const [data, setData] = useState(null); - const [loading, setLoading] = useState(true); - const [error, setError] = useState(null); - const abortRef = useRef(null); const debounceRef = useRef | null>(null); + const base = useBansByCountry(range, origin, source, countryCode, false); + const load = useCallback((): void => { // Cancel any pending debounce timer. if (debounceRef.current != null) { clearTimeout(debounceRef.current); } - setError(null); debounceRef.current = setTimeout((): void => { - // Show loading only when the fetch is about to start. - setLoading(true); - // Abort any in-flight request from a previous filter selection. - abortRef.current?.abort(); - abortRef.current = new AbortController(); - - fetchBansByCountry(range, origin, source, countryCode, abortRef.current.signal) - .then((resp) => { - setData(resp); - }) - .catch((err: unknown) => { - handleFetchError(err, setError, "Failed to fetch map data"); - }) - .finally((): void => { - setLoading(false); - }); + base.refresh(); }, DEBOUNCE_MS); - }, [range, origin, source, countryCode]); + // base.refresh is memoized with all relevant deps; including base would cause + // unnecessary recreations on every render due to object identity. + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [base.refresh]); useEffect((): (() => void) => { load(); @@ -85,17 +73,16 @@ export function useMapData( if (debounceRef.current != null) { clearTimeout(debounceRef.current); } - abortRef.current?.abort(); }; }, [load]); return { - countries: data?.countries ?? {}, - countryNames: data?.country_names ?? {}, - bans: data?.bans ?? [], - total: data?.total ?? 0, - loading, - error, + countries: base.countries, + countryNames: base.countryNames, + bans: base.bans, + total: base.total, + loading: base.loading, + error: base.error, refresh: load, }; }