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.
This commit is contained in:
@@ -550,17 +550,18 @@ export async function deleteJailLocalOverride(name: string): Promise<void> {
|
||||
export async function fetchFail2BanLog(
|
||||
lines?: number,
|
||||
filter?: string,
|
||||
signal?: AbortSignal,
|
||||
): Promise<Fail2BanLogResponse> {
|
||||
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<Fail2BanLogResponse>(`${ENDPOINTS.configFail2BanLog}${query}`);
|
||||
return get<Fail2BanLogResponse>(`${ENDPOINTS.configFail2BanLog}${query}`, signal);
|
||||
}
|
||||
|
||||
/** Fetch fail2ban service health status with current log configuration. */
|
||||
export async function fetchServiceStatus(): Promise<ServiceStatusResponse> {
|
||||
return get<ServiceStatusResponse>(ENDPOINTS.configServiceStatus);
|
||||
export async function fetchServiceStatus(signal?: AbortSignal): Promise<ServiceStatusResponse> {
|
||||
return get<ServiceStatusResponse>(ENDPOINTS.configServiceStatus, signal);
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
@@ -18,8 +18,8 @@ import type {
|
||||
*
|
||||
* @returns Setup status response with a `completed` boolean.
|
||||
*/
|
||||
export async function getSetupStatus(): Promise<SetupStatusResponse> {
|
||||
return api.get<SetupStatusResponse>(ENDPOINTS.setup);
|
||||
export async function getSetupStatus(signal?: AbortSignal): Promise<SetupStatusResponse> {
|
||||
return api.get<SetupStatusResponse>(ENDPOINTS.setup, signal);
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
31
frontend/src/hooks/README.md
Normal file
31
frontend/src/hooks/README.md
Normal file
@@ -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<AbortController | null>(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.
|
||||
120
frontend/src/hooks/__tests__/useSetupAndServerHealth.test.ts
Normal file
120
frontend/src/hooks/__tests__/useSetupAndServerHealth.test.ts
Normal file
@@ -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);
|
||||
});
|
||||
});
|
||||
@@ -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<ServiceStatusResponse | null>(null);
|
||||
const [logData, setLogData] = useState<Fail2BanLogResponse | null>(null);
|
||||
const [error, setError] = useState<string | null>(null);
|
||||
const abortRef = useRef<AbortController | null>(null);
|
||||
|
||||
const refresh = useCallback(async (): Promise<void> => {
|
||||
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 };
|
||||
}
|
||||
|
||||
@@ -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<string | null>(null);
|
||||
const [submitting, setSubmitting] = useState(false);
|
||||
const [submitError, setSubmitError] = useState<string | null>(null);
|
||||
const abortRef = useRef<AbortController | null>(null);
|
||||
|
||||
const refresh = useCallback(async (): Promise<void> => {
|
||||
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<void> => {
|
||||
|
||||
Reference in New Issue
Block a user