refactoring-backend #3

Merged
lukas.pupkalipinski merged 403 commits from refactoring-backend into main 2026-05-20 20:23:46 +02:00
7 changed files with 299 additions and 279 deletions
Showing only changes of commit 8bd5713d38 - Show all commits

View File

@@ -1,27 +1,3 @@
### T-12 · Apply `useListData` consistently across all data-fetching hooks
**Where found:** `frontend/src/hooks/useJailList.ts`, `useJailDetail.ts`, `useServerStatus.ts`, `useBanTrend.ts`, `useDashboardCountryData.ts` — all re-implement abort-controller / loading / error state manually. `useListData.ts` exists and is used by `useBlocklists`, `useJailConfigs`, `useActionList`, `useFilterList`.
**Why this is needed:** At least 5 hooks implement the same 40-line pattern. Any fix to the pattern (e.g. abort-guard in `.finally()`) must be applied to every copy independently. `useHistory` has a real bug because of this (see T-18).
**Goal:** All hooks that load a list and need refresh semantics use `useListData` or a shared base.
**What to do:**
1. Audit all hooks for the manual abort-controller pattern.
2. Refactor `useJailList` first (cleanest candidate — no mutations).
3. For hooks with side-effects beyond listing (e.g. `useJailDetail`), split into data hook + command hook (see T-13) and use `useListData` for the data half.
4. Extend `useListData` if needed to support `onSuccess` callbacks returning non-array data (e.g. `total`).
**Possible traps and issues:**
- `useListData` currently requires `selector: (response) → TItem[]`. Hooks that expose `total` alongside items need `onSuccess` to capture it — the `onSuccess` callback already exists in `UseListDataOptions`.
- `useServerStatus` has a polling interval and window-focus refetch that `useListData` does not support — may need a `usePolledData` variant or extension.
**Docs changes needed:** None.
**Doc references:** `frontend/src/hooks/useListData.ts`
---
### T-13 · Split `useJailDetail` — SRP violation (read state + write commands in one hook)
**Where found:** `frontend/src/hooks/useJailDetail.ts`

View File

@@ -1,7 +1,8 @@
import { describe, it, expect, vi, beforeEach } from "vitest";
import { renderHook, act } from "@testing-library/react";
import * as jailsApi from "../../api/jails";
import { useJailDetail } from "../useJailDetail";
import { useJailData } from "../useJailData";
import { useJailCommands } from "../useJailCommands";
import type { Jail } from "../../types/jail";
// Mock the API module
@@ -25,7 +26,7 @@ const mockJail: Jail = {
bantime_escalation: null,
};
describe("useJailDetail control methods", () => {
describe("useJailData — fetch state", () => {
beforeEach(() => {
vi.clearAllMocks();
vi.mocked(jailsApi.fetchJail).mockResolvedValue({
@@ -35,173 +36,153 @@ describe("useJailDetail control methods", () => {
});
});
it("calls start() and refetches jail data", async () => {
vi.mocked(jailsApi.startJail).mockResolvedValue({ message: "jail started", jail: "sshd" });
it("fetches jail data on mount", async () => {
const { result } = renderHook(() => useJailData("sshd"));
const { result } = renderHook(() => useJailDetail("sshd"));
// Wait for initial fetch
await act(async () => {
await new Promise((r) => setTimeout(r, 0));
});
expect(result.current.jail?.name).toBe("sshd");
expect(jailsApi.startJail).not.toHaveBeenCalled();
expect(result.current.ignoreList).toEqual([]);
expect(result.current.ignoreSelf).toBe(false);
expect(result.current.loading).toBe(false);
expect(result.current.error).toBe(null);
});
it("exposes refresh function to refetch data", async () => {
const { result } = renderHook(() => useJailData("sshd"));
await act(async () => {
await new Promise((r) => setTimeout(r, 0));
});
expect(jailsApi.fetchJail).toHaveBeenCalledTimes(1);
await act(async () => {
result.current.refresh();
await new Promise((r) => setTimeout(r, 0));
});
expect(jailsApi.fetchJail).toHaveBeenCalledTimes(2);
});
it("handles fetch errors", async () => {
const error = new Error("Network error");
vi.mocked(jailsApi.fetchJail).mockRejectedValue(error);
const { result } = renderHook(() => useJailData("sshd"));
await act(async () => {
await new Promise((r) => setTimeout(r, 0));
});
expect(result.current.error).toBeTruthy();
expect(result.current.loading).toBe(false);
});
});
describe("useJailCommands — write operations", () => {
const mockOnSuccess = vi.fn();
beforeEach(() => {
vi.clearAllMocks();
vi.mocked(jailsApi.startJail).mockResolvedValue({ message: "ok", jail: "sshd" });
vi.mocked(jailsApi.stopJail).mockResolvedValue({ message: "ok", jail: "sshd" });
vi.mocked(jailsApi.reloadJail).mockResolvedValue({ message: "ok", jail: "sshd" });
vi.mocked(jailsApi.setJailIdle).mockResolvedValue({ message: "ok", jail: "sshd" });
vi.mocked(jailsApi.addIgnoreIp).mockResolvedValue({ message: "ok", jail: "sshd" });
vi.mocked(jailsApi.delIgnoreIp).mockResolvedValue({ message: "ok", jail: "sshd" });
vi.mocked(jailsApi.toggleIgnoreSelf).mockResolvedValue({ message: "ok", jail: "sshd" });
});
it("calls start() API and invokes onSuccess", async () => {
const { result } = renderHook(() => useJailCommands("sshd", mockOnSuccess));
// Call start()
await act(async () => {
await result.current.start();
});
expect(jailsApi.startJail).toHaveBeenCalledWith("sshd");
expect(jailsApi.fetchJail).toHaveBeenCalledTimes(2); // Initial fetch + refetch after start
expect(mockOnSuccess).toHaveBeenCalledOnce();
});
it("calls stop() and refetches jail data", async () => {
vi.mocked(jailsApi.stopJail).mockResolvedValue({ message: "jail stopped", jail: "sshd" });
it("calls stop() API and invokes onSuccess", async () => {
const { result } = renderHook(() => useJailCommands("sshd", mockOnSuccess));
const { result } = renderHook(() => useJailDetail("sshd"));
// Wait for initial fetch
await act(async () => {
await new Promise((r) => setTimeout(r, 0));
});
// Call stop()
await act(async () => {
await result.current.stop();
});
expect(jailsApi.stopJail).toHaveBeenCalledWith("sshd");
expect(jailsApi.fetchJail).toHaveBeenCalledTimes(2); // Initial fetch + refetch after stop
expect(mockOnSuccess).toHaveBeenCalledOnce();
});
it("calls reload() and refetches jail data", async () => {
vi.mocked(jailsApi.reloadJail).mockResolvedValue({ message: "jail reloaded", jail: "sshd" });
it("calls reload() API and invokes onSuccess", async () => {
const { result } = renderHook(() => useJailCommands("sshd", mockOnSuccess));
const { result } = renderHook(() => useJailDetail("sshd"));
// Wait for initial fetch
await act(async () => {
await new Promise((r) => setTimeout(r, 0));
});
// Call reload()
await act(async () => {
await result.current.reload();
});
expect(jailsApi.reloadJail).toHaveBeenCalledWith("sshd");
expect(jailsApi.fetchJail).toHaveBeenCalledTimes(2); // Initial fetch + refetch after reload
expect(mockOnSuccess).toHaveBeenCalledOnce();
});
it("calls setIdle() with correct parameter and refetches jail data", async () => {
vi.mocked(jailsApi.setJailIdle).mockResolvedValue({ message: "jail idle toggled", jail: "sshd" });
it("calls setIdle() API with parameter and invokes onSuccess", async () => {
const { result } = renderHook(() => useJailCommands("sshd", mockOnSuccess));
const { result } = renderHook(() => useJailDetail("sshd"));
// Wait for initial fetch
await act(async () => {
await new Promise((r) => setTimeout(r, 0));
});
// Call setIdle(true)
await act(async () => {
await result.current.setIdle(true);
});
expect(jailsApi.setJailIdle).toHaveBeenCalledWith("sshd", true);
expect(jailsApi.fetchJail).toHaveBeenCalledTimes(2);
expect(mockOnSuccess).toHaveBeenCalledOnce();
});
// Reset mock to verify second call
vi.mocked(jailsApi.setJailIdle).mockClear();
vi.mocked(jailsApi.fetchJail).mockResolvedValue({
jail: { ...mockJail, idle: true },
ignore_list: [],
ignore_self: false,
});
it("calls addIp() API and invokes onSuccess", async () => {
const { result } = renderHook(() => useJailCommands("sshd", mockOnSuccess));
// Call setIdle(false)
await act(async () => {
await result.current.setIdle(false);
await result.current.addIp("192.168.1.1");
});
expect(jailsApi.setJailIdle).toHaveBeenCalledWith("sshd", false);
expect(jailsApi.addIgnoreIp).toHaveBeenCalledWith("sshd", "192.168.1.1");
expect(mockOnSuccess).toHaveBeenCalledOnce();
});
it("calls removeIp() API and invokes onSuccess", async () => {
const { result } = renderHook(() => useJailCommands("sshd", mockOnSuccess));
await act(async () => {
await result.current.removeIp("192.168.1.1");
});
expect(jailsApi.delIgnoreIp).toHaveBeenCalledWith("sshd", "192.168.1.1");
expect(mockOnSuccess).toHaveBeenCalledOnce();
});
it("calls toggleIgnoreSelf() API and invokes onSuccess", async () => {
const { result } = renderHook(() => useJailCommands("sshd", mockOnSuccess));
await act(async () => {
await result.current.toggleIgnoreSelf(true);
});
expect(jailsApi.toggleIgnoreSelf).toHaveBeenCalledWith("sshd", true);
expect(mockOnSuccess).toHaveBeenCalledOnce();
});
it("propagates errors from start()", async () => {
const error = new Error("Failed to start jail");
vi.mocked(jailsApi.startJail).mockRejectedValue(error);
const { result } = renderHook(() => useJailDetail("sshd"));
const { result } = renderHook(() => useJailCommands("sshd", mockOnSuccess));
// Wait for initial fetch
await act(async () => {
await new Promise((r) => setTimeout(r, 0));
});
// Call start() and expect it to throw
await expect(
act(async () => {
await result.current.start();
}),
).rejects.toThrow("Failed to start jail");
});
it("propagates errors from stop()", async () => {
const error = new Error("Failed to stop jail");
vi.mocked(jailsApi.stopJail).mockRejectedValue(error);
const { result } = renderHook(() => useJailDetail("sshd"));
// Wait for initial fetch
await act(async () => {
await new Promise((r) => setTimeout(r, 0));
});
// Call stop() and expect it to throw
await expect(
act(async () => {
await result.current.stop();
}),
).rejects.toThrow("Failed to stop jail");
});
it("propagates errors from reload()", async () => {
const error = new Error("Failed to reload jail");
vi.mocked(jailsApi.reloadJail).mockRejectedValue(error);
const { result } = renderHook(() => useJailDetail("sshd"));
// Wait for initial fetch
await act(async () => {
await new Promise((r) => setTimeout(r, 0));
});
// Call reload() and expect it to throw
await expect(
act(async () => {
await result.current.reload();
}),
).rejects.toThrow("Failed to reload jail");
});
it("propagates errors from setIdle()", async () => {
const error = new Error("Failed to set idle mode");
vi.mocked(jailsApi.setJailIdle).mockRejectedValue(error);
const { result } = renderHook(() => useJailDetail("sshd"));
// Wait for initial fetch
await act(async () => {
await new Promise((r) => setTimeout(r, 0));
});
// Call setIdle() and expect it to throw
await expect(
act(async () => {
await result.current.setIdle(true);
}),
).rejects.toThrow("Failed to set idle mode");
});
});

View File

@@ -0,0 +1,94 @@
/**
* React hook for jail state mutations.
*
* Handles write operations (start, stop, reload, setIdle, addIp, removeIp, toggleIgnoreSelf).
* Each operation calls the API and then invokes `onSuccess` to trigger a data refresh.
*/
import { useCallback } from "react";
import {
addIgnoreIp,
delIgnoreIp,
reloadJail,
setJailIdle,
startJail,
stopJail,
toggleIgnoreSelf as toggleIgnoreSelfApi,
} from "../api/jails";
export interface UseJailCommandsResult {
addIp: (ip: string) => Promise<void>;
removeIp: (ip: string) => Promise<void>;
toggleIgnoreSelf: (on: boolean) => Promise<void>;
start: () => Promise<void>;
stop: () => Promise<void>;
reload: () => Promise<void>;
setIdle: (on: boolean) => Promise<void>;
}
/**
* Manage mutations for a jail (start, stop, reload, etc.).
* Calls `onSuccess` after each operation to trigger data refresh.
*
* @param name - The name of the jail.
* @param onSuccess - Callback to invoke after each successful operation (typically to refresh data).
* @returns Mutation functions.
*/
export function useJailCommands(
name: string,
onSuccess: () => void,
): UseJailCommandsResult {
const addIp = useCallback(
async (ip: string): Promise<void> => {
await addIgnoreIp(name, ip);
onSuccess();
},
[name, onSuccess],
);
const removeIp = useCallback(
async (ip: string): Promise<void> => {
await delIgnoreIp(name, ip);
onSuccess();
},
[name, onSuccess],
);
const toggleIgnoreSelf = useCallback(
async (on: boolean): Promise<void> => {
await toggleIgnoreSelfApi(name, on);
onSuccess();
},
[name, onSuccess],
);
const start = useCallback(async (): Promise<void> => {
await startJail(name);
onSuccess();
}, [name, onSuccess]);
const stop = useCallback(async (): Promise<void> => {
await stopJail(name);
onSuccess();
}, [name, onSuccess]);
const reload = useCallback(async (): Promise<void> => {
await reloadJail(name);
onSuccess();
}, [name, onSuccess]);
const setIdle = useCallback(async (on: boolean): Promise<void> => {
await setJailIdle(name, on);
onSuccess();
}, [name, onSuccess]);
return {
addIp,
removeIp,
toggleIgnoreSelf,
start,
stop,
reload,
setIdle,
};
}

View File

@@ -0,0 +1,78 @@
/**
* React hook for fetching a single jail's detailed metadata.
*
* Reads jail data: configuration, ignore list, ignore self flag, and related state.
* Does not handle mutations — use `useJailCommands` for write operations.
*/
import { useCallback, useEffect, useRef, useState } from "react";
import { fetchJail } from "../api/jails";
import { handleFetchError } from "../utils/fetchError";
import type { Jail } from "../types/jail";
export interface UseJailDataResult {
jail: Jail | null;
ignoreList: string[];
ignoreSelf: boolean;
loading: boolean;
error: string | null;
refresh: () => void;
}
/**
* Fetch and manage the detail view for a single jail.
*
* @param name - The name of the jail to fetch.
* @returns Jail data and refresh function.
*/
export function useJailData(name: string): UseJailDataResult {
const [jail, setJail] = useState<Jail | null>(null);
const [ignoreList, setIgnoreList] = useState<string[]>([]);
const [ignoreSelf, setIgnoreSelf] = useState(false);
const [loading, setLoading] = useState(false);
const [error, setError] = useState<string | null>(null);
const abortRef = useRef<AbortController | null>(null);
const refresh = useCallback(() => {
abortRef.current?.abort();
const ctrl = new AbortController();
abortRef.current = ctrl;
setLoading(true);
setError(null);
fetchJail(name)
.then((res) => {
if (!ctrl.signal.aborted) {
setJail(res.jail);
setIgnoreList(res.ignore_list);
setIgnoreSelf(res.ignore_self);
}
})
.catch((err: unknown) => {
if (!ctrl.signal.aborted) {
handleFetchError(err, setError, "Failed to fetch jail detail");
}
})
.finally(() => {
if (!ctrl.signal.aborted) {
setLoading(false);
}
});
}, [name]);
useEffect(() => {
refresh();
return (): void => {
abortRef.current?.abort();
};
}, [refresh]);
return {
jail,
ignoreList,
ignoreSelf,
loading,
error,
refresh,
};
}

View File

@@ -1,121 +0,0 @@
/**
* React hook for fetching a single jail's detailed metadata.
*/
import { useCallback, useEffect, useRef, useState } from "react";
import { addIgnoreIp, delIgnoreIp, fetchJail, reloadJail, setJailIdle, startJail, stopJail, toggleIgnoreSelf as toggleIgnoreSelfApi } from "../api/jails";
import { handleFetchError } from "../utils/fetchError";
import type { Jail } from "../types/jail";
export interface UseJailDetailResult {
jail: Jail | null;
ignoreList: string[];
ignoreSelf: boolean;
loading: boolean;
error: string | null;
refresh: () => void;
addIp: (ip: string) => Promise<void>;
removeIp: (ip: string) => Promise<void>;
toggleIgnoreSelf: (on: boolean) => Promise<void>;
start: () => Promise<void>;
stop: () => Promise<void>;
reload: () => Promise<void>;
setIdle: (on: boolean) => Promise<void>;
}
/**
* Fetch and manage the detail view for a single jail.
*/
export function useJailDetail(name: string): UseJailDetailResult {
const [jail, setJail] = useState<Jail | null>(null);
const [ignoreList, setIgnoreList] = useState<string[]>([]);
const [ignoreSelf, setIgnoreSelf] = useState(false);
const [loading, setLoading] = useState(false);
const [error, setError] = useState<string | null>(null);
const abortRef = useRef<AbortController | null>(null);
const load = useCallback(() => {
abortRef.current?.abort();
const ctrl = new AbortController();
abortRef.current = ctrl;
setLoading(true);
setError(null);
fetchJail(name)
.then((res) => {
if (!ctrl.signal.aborted) {
setJail(res.jail);
setIgnoreList(res.ignore_list);
setIgnoreSelf(res.ignore_self);
}
})
.catch((err: unknown) => {
if (!ctrl.signal.aborted) {
handleFetchError(err, setError, "Failed to fetch jail detail");
}
})
.finally(() => {
if (!ctrl.signal.aborted) {
setLoading(false);
}
});
}, [name]);
useEffect(() => {
load();
return (): void => {
abortRef.current?.abort();
};
}, [load]);
const addIp = useCallback(async (ip: string): Promise<void> => {
await addIgnoreIp(name, ip);
load();
}, [name, load]);
const removeIp = useCallback(async (ip: string): Promise<void> => {
await delIgnoreIp(name, ip);
load();
}, [name, load]);
const toggleIgnoreSelf = useCallback(async (on: boolean): Promise<void> => {
await toggleIgnoreSelfApi(name, on);
load();
}, [name, load]);
const start = useCallback(async (): Promise<void> => {
await startJail(name);
load();
}, [name, load]);
const stop = useCallback(async (): Promise<void> => {
await stopJail(name);
load();
}, [name, load]);
const reload = useCallback(async (): Promise<void> => {
await reloadJail(name);
load();
}, [name, load]);
const setIdle = useCallback(async (on: boolean): Promise<void> => {
await setJailIdle(name, on);
load();
}, [name, load]);
return {
jail,
ignoreList,
ignoreSelf,
loading,
error,
refresh: load,
addIp,
removeIp,
toggleIgnoreSelf,
start,
stop,
reload,
setIdle,
};
}

View File

@@ -8,7 +8,8 @@
import { Button, MessageBar, MessageBarBody, Spinner, Text } from "@fluentui/react-components";
import { ArrowLeftRegular } from "@fluentui/react-icons";
import { Link, useParams } from "react-router-dom";
import { useJailDetail } from "../hooks/useJailDetail";
import { useJailData } from "../hooks/useJailData";
import { useJailCommands } from "../hooks/useJailCommands";
import { useJailBannedIps } from "../hooks/useJailBannedIps";
import { BannedIpsSection } from "../components/jail/BannedIpsSection";
import { JailInfoSection } from "./jail/JailInfoSection";
@@ -20,8 +21,8 @@ import { useJailDetailPageStyles } from "./jail/jailDetailPageStyles";
export function JailDetailPage(): React.JSX.Element {
const styles = useJailDetailPageStyles();
const { name = "" } = useParams<{ name: string }>();
const { jail, ignoreList, ignoreSelf, loading, error, refresh, addIp, removeIp, toggleIgnoreSelf, start, stop, reload, setIdle } =
useJailDetail(name);
const { jail, ignoreList, ignoreSelf, loading, error, refresh } = useJailData(name);
const { addIp, removeIp, toggleIgnoreSelf, start, stop, reload, setIdle } = useJailCommands(name, refresh);
const {
items,
total,

View File

@@ -15,7 +15,8 @@ import { FluentProvider, webLightTheme } from "@fluentui/react-components";
import { MemoryRouter, Route, Routes } from "react-router-dom";
import { JailDetailPage } from "../JailDetailPage";
import type { Jail } from "../../types/jail";
import type { UseJailDetailResult } from "../../hooks/useJailDetail";
import type { UseJailDataResult } from "../../hooks/useJailData";
import type { UseJailCommandsResult } from "../../hooks/useJailCommands";
// ---------------------------------------------------------------------------
// Module mocks
@@ -38,9 +39,14 @@ const {
mockRefresh: vi.fn(),
}));
// Mock the jail detail hook — tests control the returned state directly.
vi.mock("../../hooks/useJailDetail", () => ({
useJailDetail: vi.fn(),
// Mock the jail data hook — tests control the returned state directly.
vi.mock("../../hooks/useJailData", () => ({
useJailData: vi.fn(),
}));
// Mock the jail commands hook — tests control the returned functions directly.
vi.mock("../../hooks/useJailCommands", () => ({
useJailCommands: vi.fn(),
}));
vi.mock("../../hooks/useJailBannedIps", () => ({
@@ -79,7 +85,8 @@ vi.mock("../../components/jail/BannedIpsSection", () => ({
// Helpers
// ---------------------------------------------------------------------------
import { useJailDetail } from "../../hooks/useJailDetail";
import { useJailData } from "../../hooks/useJailData";
import { useJailCommands } from "../../hooks/useJailCommands";
/** Minimal `Jail` fixture. */
function makeJail(): Jail {
@@ -107,15 +114,19 @@ function makeJail(): Jail {
};
}
/** Wire `useJailDetail` to return the given `ignoreSelf` value. */
function mockHook(ignoreSelf: boolean): void {
const result: UseJailDetailResult = {
/** Wire `useJailData` and `useJailCommands` to return the given `ignoreSelf` value. */
function mockHooks(ignoreSelf: boolean): void {
const dataResult: UseJailDataResult = {
jail: makeJail(),
ignoreList: ["10.0.0.0/8"],
ignoreSelf,
loading: false,
error: null,
refresh: mockRefresh,
};
vi.mocked(useJailData).mockReturnValue(dataResult);
const commandsResult: UseJailCommandsResult = {
addIp: mockAddIp,
removeIp: mockRemoveIp,
toggleIgnoreSelf: mockToggleIgnoreSelf,
@@ -124,7 +135,7 @@ function mockHook(ignoreSelf: boolean): void {
reload: vi.fn().mockResolvedValue(undefined),
setIdle: vi.fn().mockResolvedValue(undefined),
};
vi.mocked(useJailDetail).mockReturnValue(result);
vi.mocked(useJailCommands).mockReturnValue(commandsResult);
}
/** Render the JailDetailPage with a fake `/jails/sshd` route. */
@@ -151,7 +162,7 @@ describe("JailDetailPage — ignore self toggle", () => {
});
it("renders the switch checked when ignoreSelf is true", async () => {
mockHook(true);
mockHooks(true);
renderPage();
const switchEl = await screen.findByRole("switch", { name: /ignore self/i });
@@ -159,7 +170,7 @@ describe("JailDetailPage — ignore self toggle", () => {
});
it("renders the switch unchecked when ignoreSelf is false", async () => {
mockHook(false);
mockHooks(false);
renderPage();
const switchEl = await screen.findByRole("switch", { name: /ignore self/i });
@@ -167,7 +178,7 @@ describe("JailDetailPage — ignore self toggle", () => {
});
it("calls toggleIgnoreSelf(false) when switch is toggled off", async () => {
mockHook(true);
mockHooks(true);
renderPage();
const user = userEvent.setup();
@@ -181,7 +192,7 @@ describe("JailDetailPage — ignore self toggle", () => {
});
it("calls toggleIgnoreSelf(true) when switch is toggled on", async () => {
mockHook(false);
mockHooks(false);
renderPage();
const user = userEvent.setup();
@@ -195,7 +206,7 @@ describe("JailDetailPage — ignore self toggle", () => {
});
it("shows an error message bar when toggleIgnoreSelf rejects", async () => {
mockHook(false);
mockHooks(false);
mockToggleIgnoreSelf.mockRejectedValue(new Error("Connection refused"));
renderPage();
const user = userEvent.setup();