diff --git a/Docs/Tasks.md b/Docs/Tasks.md index efe2af4..0e043c1 100644 --- a/Docs/Tasks.md +++ b/Docs/Tasks.md @@ -1,31 +1,3 @@ - -### TASK-STATE-01 — Auth Errors Silently Swallowed in `useRegexTester` and `useLogPreview` - -**Where found** -`frontend/src/hooks/useRegexTester.ts` and `frontend/src/hooks/useLogPreview.ts`. Each hook's catch block handles errors only as `err instanceof Error`, which catches `ApiError` (a subclass of `Error`), but does not call `handleFetchError`. The `handleFetchError` utility calls `isAuthError` and returns early without setting UI error text, allowing the global `SESSION_EXPIRED_EVENT` listener in `AuthProvider` to handle the redirect. By bypassing `handleFetchError`, auth errors (401/403) are displayed as UI error text (`err.message`) instead of triggering the session expiry flow. - -**Goal** -Replace the ad-hoc catch blocks in both hooks with `handleFetchError`: -```ts -} catch (err: unknown) { - if (signal.aborted) return; - handleFetchError(err, setError, "Regex test failed"); -} -``` -`handleFetchError` already imports `isAuthError` and returns early for auth errors, allowing the global handler to take over. - -**Possible traps and issues** -- Confirm that `SESSION_EXPIRED_EVENT` is being listened to in `AuthProvider` and that it triggers navigation to `/login`. It is — this was verified in the architecture review. -- After the fix, a 401 during a regex test will show no error text (the auth handler navigates away), which is correct behaviour. - -**Docs changes needed** -Add a convention to `Docs/Web-Development.md`: "All hook catch blocks must use `handleFetchError` rather than directly calling `setError`, so auth errors are routed to the session-expiry flow." - -**Why this is needed** -When a session expires while the user is in the Regex Tester, they see a confusing "Unauthorized" error message in the tester UI instead of being cleanly redirected to the login page. - ---- - ### TASK-STATE-02 — `withRefresh` in `useJailList` Creates New Function References Every Render **Where found** diff --git a/frontend/src/hooks/__tests__/useJailList.test.ts b/frontend/src/hooks/__tests__/useJailList.test.ts new file mode 100644 index 0000000..00d46d9 --- /dev/null +++ b/frontend/src/hooks/__tests__/useJailList.test.ts @@ -0,0 +1,54 @@ +import { describe, expect, it, vi } from "vitest"; +import { renderHook } from "@testing-library/react"; +import { useJails } from "../useJailList"; + +// Mock the API calls +vi.mock("../../api/jails", () => ({ + fetchJails: vi.fn().mockResolvedValue({ + jails: [], + total: 0, + }), + startJail: vi.fn(), + stopJail: vi.fn(), + setJailIdle: vi.fn(), + reloadJail: vi.fn(), + reloadAllJails: vi.fn(), +})); + +vi.mock("../../utils/fetchError", () => ({ + handleFetchError: vi.fn(), +})); + +describe("useJails", () => { + it("returns stable function references between renders", () => { + const { result, rerender } = renderHook(() => useJails()); + + const firstRender = { + startJail: result.current.startJail, + stopJail: result.current.stopJail, + setIdle: result.current.setIdle, + reloadJail: result.current.reloadJail, + reloadAll: result.current.reloadAll, + refresh: result.current.refresh, + }; + + rerender(); + + const secondRender = { + startJail: result.current.startJail, + stopJail: result.current.stopJail, + setIdle: result.current.setIdle, + reloadJail: result.current.reloadJail, + reloadAll: result.current.reloadAll, + refresh: result.current.refresh, + }; + + // Function references should be the same between renders + expect(firstRender.startJail).toBe(secondRender.startJail); + expect(firstRender.stopJail).toBe(secondRender.stopJail); + expect(firstRender.setIdle).toBe(secondRender.setIdle); + expect(firstRender.reloadJail).toBe(secondRender.reloadJail); + expect(firstRender.reloadAll).toBe(secondRender.reloadAll); + expect(firstRender.refresh).toBe(secondRender.refresh); + }); +}); diff --git a/frontend/src/hooks/useJailList.ts b/frontend/src/hooks/useJailList.ts index c5d3e33..5dab75d 100644 --- a/frontend/src/hooks/useJailList.ts +++ b/frontend/src/hooks/useJailList.ts @@ -70,12 +70,45 @@ export function useJails(): UseJailsResult { }; }, [load]); - const withRefresh = - (fn: (name: string) => Promise) => + const startJailMemo = useCallback( async (name: string): Promise => { - await fn(name); + await startJail(name); load(); - }; + }, + [load], + ); + + const stopJailMemo = useCallback( + async (name: string): Promise => { + await stopJail(name); + load(); + }, + [load], + ); + + const reloadJailMemo = useCallback( + async (name: string): Promise => { + await reloadJail(name); + load(); + }, + [load], + ); + + const setIdleMemo = useCallback( + (name: string, on: boolean): Promise => + setJailIdle(name, on).then(() => { + load(); + }), + [load], + ); + + const reloadAllMemo = useCallback( + (): Promise => + reloadAllJails().then(() => { + load(); + }), + [load], + ); return { jails, @@ -83,14 +116,10 @@ export function useJails(): UseJailsResult { loading, error, refresh: load, - startJail: withRefresh(startJail), - stopJail: withRefresh(stopJail), - setIdle: (name, on) => setJailIdle(name, on).then(() => { - load(); - }), - reloadJail: withRefresh(reloadJail), - reloadAll: () => reloadAllJails().then(() => { - load(); - }), + startJail: startJailMemo, + stopJail: stopJailMemo, + setIdle: setIdleMemo, + reloadJail: reloadJailMemo, + reloadAll: reloadAllMemo, }; }