From e683108965f2a3d39ef72fad6bc7eda25abb25dc Mon Sep 17 00:00:00 2001 From: Lukas Date: Tue, 21 Apr 2026 17:38:35 +0200 Subject: [PATCH] Standardise AbortController cancellation in setup and server health hooks Add abortable API signals for setup status and server health/log fetches, document hook cancellation patterns, and cover stale refresh cancellation with tests. --- Docs/Tasks.md | 4 +- frontend/src/api/config.ts | 7 +- frontend/src/api/setup.ts | 4 +- frontend/src/hooks/README.md | 31 +++++ .../__tests__/useSetupAndServerHealth.test.ts | 120 ++++++++++++++++++ frontend/src/hooks/useServerHealth.ts | 25 +++- frontend/src/hooks/useSetup.ts | 24 +++- 7 files changed, 203 insertions(+), 12 deletions(-) create mode 100644 frontend/src/hooks/README.md create mode 100644 frontend/src/hooks/__tests__/useSetupAndServerHealth.test.ts diff --git a/Docs/Tasks.md b/Docs/Tasks.md index b41c342..10902d5 100644 --- a/Docs/Tasks.md +++ b/Docs/Tasks.md @@ -283,7 +283,9 @@ Issues are grouped by category and ordered roughly by severity. Each entry descr --- -### TASK-015 — Standardise AbortController pattern across all hooks +### TASK-015 — Standardise AbortController pattern across all hooks (done) + +**Where fixed:** `frontend/src/hooks/useSetup.ts`, `frontend/src/hooks/useServerHealth.ts`, `frontend/src/api/setup.ts`, `frontend/src/api/config.ts`, `frontend/src/hooks/README.md`, `frontend/src/hooks/__tests__/useSetupAndServerHealth.test.ts` **Where found:** Three different patterns exist in `frontend/src/hooks/`: 1. `useRef` with manual abort before each fetch (correct — used in `useActiveBans`, `useActionList`). diff --git a/frontend/src/api/config.ts b/frontend/src/api/config.ts index 58b9433..587028b 100644 --- a/frontend/src/api/config.ts +++ b/frontend/src/api/config.ts @@ -550,17 +550,18 @@ export async function deleteJailLocalOverride(name: string): Promise { export async function fetchFail2BanLog( lines?: number, filter?: string, + signal?: AbortSignal, ): Promise { const params = new URLSearchParams(); if (lines !== undefined) params.set("lines", String(lines)); if (filter !== undefined && filter !== "") params.set("filter", filter); const query = params.toString() ? `?${params.toString()}` : ""; - return get(`${ENDPOINTS.configFail2BanLog}${query}`); + return get(`${ENDPOINTS.configFail2BanLog}${query}`, signal); } /** Fetch fail2ban service health status with current log configuration. */ -export async function fetchServiceStatus(): Promise { - return get(ENDPOINTS.configServiceStatus); +export async function fetchServiceStatus(signal?: AbortSignal): Promise { + return get(ENDPOINTS.configServiceStatus, signal); } // --------------------------------------------------------------------------- diff --git a/frontend/src/api/setup.ts b/frontend/src/api/setup.ts index 1070513..bb7fb51 100644 --- a/frontend/src/api/setup.ts +++ b/frontend/src/api/setup.ts @@ -18,8 +18,8 @@ import type { * * @returns Setup status response with a `completed` boolean. */ -export async function getSetupStatus(): Promise { - return api.get(ENDPOINTS.setup); +export async function getSetupStatus(signal?: AbortSignal): Promise { + return api.get(ENDPOINTS.setup, signal); } /** diff --git a/frontend/src/hooks/README.md b/frontend/src/hooks/README.md new file mode 100644 index 0000000..41af56c --- /dev/null +++ b/frontend/src/hooks/README.md @@ -0,0 +1,31 @@ +# React Hook Fetch Cancellation + +This folder follows a shared convention for network fetch cancellation in React hooks. + +## Patterns + +### 1. Hooks with manual refresh + +Hooks that expose a `refresh()` callback must use a long-lived `AbortController` stored in a ref: + +- `const abortRef = useRef(null); +- Call `abortRef.current?.abort()` before starting a new request. +- Create a fresh controller before every `refresh()` invocation. +- Pass `controller.signal` to the API function. +- In the cleanup effect, abort the controller when the hook unmounts. +- After each `await`, check `signal.aborted` before updating state. + +This prevents stale responses from overwriting newer results and avoids React state updates after unmount. + +### 2. One-shot mount-only requests + +Hooks that only fetch once inside `useEffect` and do not expose a manual refresh may use a local controller: + +- Create `const controller = new AbortController();` inside the effect. +- Pass `controller.signal` to the request. +- Abort it in the effect cleanup. +- This is the simplest correct pattern for single-fetch hooks. + +### 3. Do not use boolean cancelled flags for network requests + +A boolean `cancelled` flag is not sufficient because it does not stop the underlying fetch. Abort signals are the correct cancellation mechanism for fetch-based hooks. diff --git a/frontend/src/hooks/__tests__/useSetupAndServerHealth.test.ts b/frontend/src/hooks/__tests__/useSetupAndServerHealth.test.ts new file mode 100644 index 0000000..f4a97f0 --- /dev/null +++ b/frontend/src/hooks/__tests__/useSetupAndServerHealth.test.ts @@ -0,0 +1,120 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { renderHook, act } from "@testing-library/react"; +import { useSetup } from "../useSetup"; +import { useServerHealth } from "../useServerHealth"; +import * as setupApi from "../../api/setup"; +import * as configApi from "../../api/config"; +import type { Fail2BanLogResponse, ServiceStatusResponse } from "../../types/config"; + +vi.mock("../../api/setup"); +vi.mock("../../api/config"); + +const mockedGetSetupStatus = vi.mocked(setupApi.getSetupStatus); +const mockedFetchServiceStatus = vi.mocked(configApi.fetchServiceStatus); +const mockedFetchFail2BanLog = vi.mocked(configApi.fetchFail2BanLog); + +describe("useSetup", () => { + beforeEach(() => { + vi.useFakeTimers(); + vi.clearAllMocks(); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + it("loads setup status on mount", async () => { + mockedGetSetupStatus.mockResolvedValue({ completed: true }); + + const { result } = renderHook(() => useSetup()); + + expect(result.current.loading).toBe(true); + await act(async () => { + await Promise.resolve(); + }); + + expect(mockedGetSetupStatus).toHaveBeenCalledTimes(1); + expect(result.current.status).toEqual({ completed: true }); + expect(result.current.loading).toBe(false); + expect(result.current.error).toBeNull(); + }); + + it("cancels stale setup fetches when refresh is called again", async () => { + const firstResult = { completed: false }; + const secondResult = { completed: true }; + let callCount = 0; + + mockedGetSetupStatus.mockImplementation(async (signal?: AbortSignal) => { + const response = callCount++ === 0 ? firstResult : secondResult; + await new Promise((resolve) => setTimeout(resolve, 10)); + if (signal?.aborted) { + throw new Error("Request aborted"); + } + return response; + }); + + const { result } = renderHook(() => useSetup()); + + await act(async () => { + void result.current.refresh(); + void result.current.refresh(); + vi.advanceTimersByTime(20); + await Promise.resolve(); + }); + + expect(mockedGetSetupStatus).toHaveBeenCalledTimes(3); + expect(result.current.status).toEqual(secondResult); + expect(result.current.loading).toBe(false); + }); +}); + +describe("useServerHealth", () => { + beforeEach(() => { + vi.useFakeTimers(); + vi.clearAllMocks(); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + it("updates state only for the latest refresh and aborts stale requests", async () => { + const firstService = { running: false } as unknown as ServiceStatusResponse; + const secondService = { running: true } as unknown as ServiceStatusResponse; + const firstLog = { lines: [] } as unknown as Fail2BanLogResponse; + const secondLog = { lines: ["ok"] } as unknown as Fail2BanLogResponse; + let callCount = 0; + + mockedFetchServiceStatus.mockImplementation(async (signal?: AbortSignal) => { + const response = callCount++ === 0 ? firstService : secondService; + await new Promise((resolve) => setTimeout(resolve, 10)); + if (signal?.aborted) { + throw new Error("Request aborted"); + } + return response; + }); + mockedFetchFail2BanLog.mockImplementation(async (_lines?: number, _filter?: string, signal?: AbortSignal) => { + const response = callCount === 1 ? firstLog : secondLog; + await new Promise((resolve) => setTimeout(resolve, 10)); + if (signal?.aborted) { + throw new Error("Request aborted"); + } + return response; + }); + + const { result } = renderHook(() => useServerHealth(10, "test")); + + await act(async () => { + void result.current.refresh(); + void result.current.refresh(); + vi.advanceTimersByTime(20); + await Promise.resolve(); + }); + + expect(result.current.status).toEqual(secondService); + expect(result.current.logData).toEqual(secondLog); + expect(result.current.error).toBeNull(); + expect(mockedFetchServiceStatus).toHaveBeenCalledTimes(2); + expect(mockedFetchFail2BanLog).toHaveBeenCalledTimes(2); + }); +}); diff --git a/frontend/src/hooks/useServerHealth.ts b/frontend/src/hooks/useServerHealth.ts index eec9b0c..5fce413 100644 --- a/frontend/src/hooks/useServerHealth.ts +++ b/frontend/src/hooks/useServerHealth.ts @@ -1,7 +1,7 @@ /** * React hook for service health and log viewer data fetching. */ -import { useCallback, useState } from "react"; +import { useCallback, useEffect, useRef, useState } from "react"; import { fetchFail2BanLog, fetchServiceStatus } from "../api/config"; import type { Fail2BanLogResponse, ServiceStatusResponse } from "../types/config"; @@ -22,14 +22,24 @@ export function useServerHealth( const [status, setStatus] = useState(null); const [logData, setLogData] = useState(null); const [error, setError] = useState(null); + const abortRef = useRef(null); const refresh = useCallback(async (): Promise => { + abortRef.current?.abort(); + const controller = new AbortController(); + abortRef.current = controller; + const { signal } = controller; + try { const [svcResult, logResult] = await Promise.allSettled([ - fetchServiceStatus(), - fetchFail2BanLog(linesCount, filterValue || undefined), + fetchServiceStatus(signal), + fetchFail2BanLog(linesCount, filterValue || undefined, signal), ]); + if (signal.aborted) { + return; + } + if (svcResult.status === "fulfilled") { setStatus(svcResult.value); } else { @@ -44,10 +54,19 @@ export function useServerHealth( setError(reason instanceof Error ? reason.message : "Failed to load log data."); } } catch (err: unknown) { + if (signal.aborted) { + return; + } const reason = err instanceof Error ? err.message : String(err); setError(reason); } }, [filterValue, linesCount]); + useEffect(() => { + return (): void => { + abortRef.current?.abort(); + }; + }, []); + return { status, logData, error, refresh }; } diff --git a/frontend/src/hooks/useSetup.ts b/frontend/src/hooks/useSetup.ts index f6c9d46..40736ff 100644 --- a/frontend/src/hooks/useSetup.ts +++ b/frontend/src/hooks/useSetup.ts @@ -4,7 +4,7 @@ * Exposes the current setup completion status and a submission handler. */ -import { useCallback, useEffect, useState } from "react"; +import { useCallback, useEffect, useRef, useState } from "react"; import { ApiError } from "../api/client"; import { handleFetchError } from "../utils/fetchError"; import { getSetupStatus, submitSetup } from "../api/setup"; @@ -36,24 +36,42 @@ export function useSetup(): UseSetupResult { const [error, setError] = useState(null); const [submitting, setSubmitting] = useState(false); const [submitError, setSubmitError] = useState(null); + const abortRef = useRef(null); const refresh = useCallback(async (): Promise => { + abortRef.current?.abort(); + const controller = new AbortController(); + abortRef.current = controller; + const { signal } = controller; + setLoading(true); setError(null); try { - const resp = await getSetupStatus(); + const resp = await getSetupStatus(signal); + if (signal.aborted) { + return; + } setStatus(resp); } catch (err: unknown) { + if (signal.aborted) { + return; + } const fallback = "Failed to fetch setup status"; handleFetchError(err, setError, fallback); } finally { - setLoading(false); + if (!signal.aborted) { + setLoading(false); + } } }, []); useEffect(() => { void refresh(); + + return (): void => { + abortRef.current?.abort(); + }; }, [refresh]); const submit = useCallback(async (payload: SetupRequest): Promise => {