diff --git a/Docs/Tasks.md b/Docs/Tasks.md index c20fd83..efe2af4 100644 --- a/Docs/Tasks.md +++ b/Docs/Tasks.md @@ -1,46 +1,3 @@ -### TASK-ABORT-04 — `useIpLookup` Has No AbortController or Unmount Guard - -**Where found** -`frontend/src/hooks/useIpLookup.ts`. The hook performs an async fetch but has no `AbortController`, no `useEffect` cleanup, and no unmount guard. If the component that calls this hook unmounts before the fetch completes (e.g. the user navigates away), the `.then()` callback still fires and calls `setResult` / `setLoading` on an unmounted component. - -**Goal** -Add a standard `AbortController` pattern: -```ts -const abortRef = useRef(null); -const lookup = useCallback(async (ip: string): Promise => { - abortRef.current?.abort(); - const ctrl = new AbortController(); - abortRef.current = ctrl; - setLoading(true); - try { - const result = await fetchIpInfo(ip, ctrl.signal); - if (ctrl.signal.aborted) return; - setResult(result); - } catch (err) { - if (ctrl.signal.aborted) return; - // handle error - } finally { - if (!ctrl.signal.aborted) setLoading(false); - } -}, []); -``` -Add a `useEffect` cleanup that calls `abortRef.current?.abort()` on unmount. - -**Possible traps and issues** -- `fetchIpInfo` (in `api/jails.ts` or equivalent) must accept `signal` for the abort to actually cancel the HTTP request. Check whether it already does; if not, add it as part of TASK-ABORT-01. -- This hook is likely called from a popover or panel that can close while a lookup is running; unmount cancellation is the primary concern here. - -**Docs changes needed** -None required. - -**Why this is needed** -Calling React state setters on unmounted components is a React antipattern that produces console warnings in development and can cause subtle state corruption if the component re-mounts quickly. - ---- - -## State Management - ---- ### TASK-STATE-01 — Auth Errors Silently Swallowed in `useRegexTester` and `useLogPreview` diff --git a/Docs/Web-Development.md b/Docs/Web-Development.md index b47c244..5abdcf7 100644 --- a/Docs/Web-Development.md +++ b/Docs/Web-Development.md @@ -488,6 +488,7 @@ if (data.length > MAX_VISIBLE_BANS) { ... } ## 11. Error Handling - Wrap API calls in `try-catch` inside hooks — components should never see raw exceptions. +- **All hook catch blocks must use `handleFetchError` rather than directly calling `setError`.** This ensures auth errors (401/403) are routed to the global session-expiry flow instead of displaying confusing error text in the UI. Use the pattern: `handleFetchError(err, setError, "User-friendly fallback message")`. - Display user-friendly error messages — never expose stack traces or raw server responses in the UI. - Use an **error boundary** (`ErrorBoundary` component) at the page level to catch unexpected render errors. - Log errors to the console (or a future logging service) with sufficient context for debugging. diff --git a/frontend/src/hooks/__tests__/useLogPreview.test.ts b/frontend/src/hooks/__tests__/useLogPreview.test.ts new file mode 100644 index 0000000..7471204 --- /dev/null +++ b/frontend/src/hooks/__tests__/useLogPreview.test.ts @@ -0,0 +1,126 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { renderHook, act } from "@testing-library/react"; +import { ApiError } from "../../api/client"; +import { useLogPreview } from "../useLogPreview"; + +vi.mock("../../api/config", () => ({ + previewLog: vi.fn(), +})); + +import { previewLog } from "../../api/config"; + +describe("useLogPreview", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + afterEach(() => { + vi.clearAllMocks(); + }); + + it("returns initial state with preview and loading false", () => { + const { result } = renderHook(() => useLogPreview()); + + expect(result.current.preview).toBeNull(); + expect(result.current.loading).toBe(false); + expect(result.current.error).toBeNull(); + }); + + it("sets preview on successful run", async () => { + const mockResponse = { + lines: [{ line: "test", matched: true, groups: [] }], + total_lines: 1, + matched_count: 1, + regex_error: null, + }; + vi.mocked(previewLog).mockResolvedValue(mockResponse); + + const { result } = renderHook(() => useLogPreview()); + + await act(async () => { + await result.current.run({ + log_path: "/var/log/test.log", + fail_regex: "pattern", + num_lines: 100, + }); + }); + + expect(result.current.preview).toEqual(mockResponse); + expect(result.current.error).toBeNull(); + expect(result.current.loading).toBe(false); + }); + + it("sets error for normal errors via handleFetchError", async () => { + vi.mocked(previewLog).mockRejectedValue(new Error("Network error")); + + const { result } = renderHook(() => useLogPreview()); + + await act(async () => { + await result.current.run({ + log_path: "/var/log/test.log", + fail_regex: "pattern", + num_lines: 100, + }); + }); + + expect(result.current.error).toBe("Network error"); + expect(result.current.loading).toBe(false); + }); + + it("ignores auth errors (401) via handleFetchError", async () => { + vi.mocked(previewLog).mockRejectedValue(new ApiError(401, "Unauthorized")); + + const { result } = renderHook(() => useLogPreview()); + + await act(async () => { + await result.current.run({ + log_path: "/var/log/test.log", + fail_regex: "pattern", + num_lines: 100, + }); + }); + + expect(result.current.error).toBeNull(); + expect(result.current.loading).toBe(false); + }); + + it("ignores auth errors (403) via handleFetchError", async () => { + vi.mocked(previewLog).mockRejectedValue(new ApiError(403, "Forbidden")); + + const { result } = renderHook(() => useLogPreview()); + + await act(async () => { + await result.current.run({ + log_path: "/var/log/test.log", + fail_regex: "pattern", + num_lines: 100, + }); + }); + + expect(result.current.error).toBeNull(); + expect(result.current.loading).toBe(false); + }); + + it("sets loading to false in finally block", async () => { + vi.mocked(previewLog).mockResolvedValue({ + lines: [], + total_lines: 0, + matched_count: 0, + regex_error: null, + }); + + const { result } = renderHook(() => useLogPreview()); + + expect(result.current.loading).toBe(false); + + await act(async () => { + await result.current.run({ + log_path: "/var/log/test.log", + fail_regex: "pattern", + num_lines: 100, + }); + }); + + expect(result.current.loading).toBe(false); + }); +}); diff --git a/frontend/src/hooks/__tests__/useRegexTester.test.ts b/frontend/src/hooks/__tests__/useRegexTester.test.ts new file mode 100644 index 0000000..4f96f1c --- /dev/null +++ b/frontend/src/hooks/__tests__/useRegexTester.test.ts @@ -0,0 +1,103 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { renderHook, act } from "@testing-library/react"; +import { ApiError } from "../../api/client"; +import { useRegexTester } from "../useRegexTester"; + +vi.mock("../../api/config", () => ({ + testRegex: vi.fn(), +})); + +import { testRegex } from "../../api/config"; + +describe("useRegexTester", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + afterEach(() => { + vi.clearAllMocks(); + }); + + it("returns initial state with result and testing false", () => { + const { result } = renderHook(() => useRegexTester()); + + expect(result.current.result).toBeNull(); + expect(result.current.testing).toBe(false); + expect(result.current.error).toBeNull(); + }); + + it("sets result on successful test", async () => { + const mockResponse = { matched: true, groups: ["host"], error: null }; + vi.mocked(testRegex).mockResolvedValue(mockResponse); + + const { result } = renderHook(() => useRegexTester()); + + await act(async () => { + await result.current.test({ log_line: "test", fail_regex: "pattern" }); + }); + + expect(result.current.result).toEqual(mockResponse); + expect(result.current.error).toBeNull(); + expect(result.current.testing).toBe(false); + }); + + it("sets error for normal errors via handleFetchError", async () => { + vi.mocked(testRegex).mockRejectedValue(new Error("Network error")); + + const { result } = renderHook(() => useRegexTester()); + + await act(async () => { + await result.current.test({ log_line: "test", fail_regex: "pattern" }); + }); + + expect(result.current.error).toBe("Network error"); + expect(result.current.testing).toBe(false); + }); + + it("ignores auth errors (401) via handleFetchError", async () => { + vi.mocked(testRegex).mockRejectedValue(new ApiError(401, "Unauthorized")); + + const { result } = renderHook(() => useRegexTester()); + + await act(async () => { + await result.current.test({ log_line: "test", fail_regex: "pattern" }); + }); + + expect(result.current.error).toBeNull(); + expect(result.current.testing).toBe(false); + }); + + it("ignores auth errors (403) via handleFetchError", async () => { + vi.mocked(testRegex).mockRejectedValue(new ApiError(403, "Forbidden")); + + const { result } = renderHook(() => useRegexTester()); + + await act(async () => { + await result.current.test({ log_line: "test", fail_regex: "pattern" }); + }); + + expect(result.current.error).toBeNull(); + expect(result.current.testing).toBe(false); + }); + + it("sets testing to false in finally block", async () => { + vi.mocked(testRegex).mockResolvedValue({ + matched: false, + groups: [], + error: null, + }); + + const { result } = renderHook(() => useRegexTester()); + + expect(result.current.testing).toBe(false); + + await act(async () => { + await result.current.test({ + log_line: "test", + fail_regex: "pattern", + }); + }); + + expect(result.current.testing).toBe(false); + }); +}); diff --git a/frontend/src/hooks/useLogPreview.ts b/frontend/src/hooks/useLogPreview.ts index a8b5f66..8daecd8 100644 --- a/frontend/src/hooks/useLogPreview.ts +++ b/frontend/src/hooks/useLogPreview.ts @@ -4,11 +4,13 @@ import { useCallback, useState } from "react"; import { previewLog } from "../api/config"; +import { handleFetchError } from "../utils/fetchError"; import type { LogPreviewRequest, LogPreviewResponse } from "../types/config"; export interface UseLogPreviewResult { preview: LogPreviewResponse | null; loading: boolean; + error: string | null; run: (req: LogPreviewRequest) => Promise; } @@ -18,25 +20,20 @@ export interface UseLogPreviewResult { export function useLogPreview(): UseLogPreviewResult { const [preview, setPreview] = useState(null); const [loading, setLoading] = useState(false); + const [error, setError] = useState(null); const run = useCallback(async (req: LogPreviewRequest): Promise => { setLoading(true); try { const resp = await previewLog(req); setPreview(resp); + setError(null); } catch (err: unknown) { - if (err instanceof Error) { - setPreview({ - lines: [], - total_lines: 0, - matched_count: 0, - regex_error: err.message, - }); - } + handleFetchError(err, setError, "Log preview failed"); } finally { setLoading(false); } }, []); - return { preview, loading, run }; + return { preview, loading, error, run }; } diff --git a/frontend/src/hooks/useRegexTester.ts b/frontend/src/hooks/useRegexTester.ts index 935798e..286aea4 100644 --- a/frontend/src/hooks/useRegexTester.ts +++ b/frontend/src/hooks/useRegexTester.ts @@ -4,11 +4,13 @@ import { useCallback, useState } from "react"; import { testRegex } from "../api/config"; +import { handleFetchError } from "../utils/fetchError"; import type { RegexTestRequest, RegexTestResponse } from "../types/config"; export interface UseRegexTesterResult { result: RegexTestResponse | null; testing: boolean; + error: string | null; test: (req: RegexTestRequest) => Promise; } @@ -18,20 +20,20 @@ export interface UseRegexTesterResult { export function useRegexTester(): UseRegexTesterResult { const [result, setResult] = useState(null); const [testing, setTesting] = useState(false); + const [error, setError] = useState(null); const test = useCallback(async (req: RegexTestRequest): Promise => { setTesting(true); try { const resp = await testRegex(req); setResult(resp); + setError(null); } catch (err: unknown) { - if (err instanceof Error) { - setResult({ matched: false, groups: [], error: err.message }); - } + handleFetchError(err, setError, "Regex test failed"); } finally { setTesting(false); } }, []); - return { result, testing, test }; + return { result, testing, error, test }; }