Stabilize function references in useJails with useCallback
Previously, the withRefresh helper and all operations (startJail, stopJail, setIdle, reloadJail, reloadAll) were recreated on every render because they were defined in the hook body without useCallback. This caused unnecessary re-renders of child components using React.memo when parent state changed. Now each operation is wrapped in useCallback with [load] as its dependency. This ensures function references remain stable between renders, allowing React.memo optimizations to work correctly in JailOverviewSection. Tests confirm that function references are now stable between consecutive renders. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
@@ -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**
|
||||
|
||||
54
frontend/src/hooks/__tests__/useJailList.test.ts
Normal file
54
frontend/src/hooks/__tests__/useJailList.test.ts
Normal file
@@ -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);
|
||||
});
|
||||
});
|
||||
@@ -70,12 +70,45 @@ export function useJails(): UseJailsResult {
|
||||
};
|
||||
}, [load]);
|
||||
|
||||
const withRefresh =
|
||||
(fn: (name: string) => Promise<unknown>) =>
|
||||
const startJailMemo = useCallback(
|
||||
async (name: string): Promise<void> => {
|
||||
await fn(name);
|
||||
await startJail(name);
|
||||
load();
|
||||
};
|
||||
},
|
||||
[load],
|
||||
);
|
||||
|
||||
const stopJailMemo = useCallback(
|
||||
async (name: string): Promise<void> => {
|
||||
await stopJail(name);
|
||||
load();
|
||||
},
|
||||
[load],
|
||||
);
|
||||
|
||||
const reloadJailMemo = useCallback(
|
||||
async (name: string): Promise<void> => {
|
||||
await reloadJail(name);
|
||||
load();
|
||||
},
|
||||
[load],
|
||||
);
|
||||
|
||||
const setIdleMemo = useCallback(
|
||||
(name: string, on: boolean): Promise<void> =>
|
||||
setJailIdle(name, on).then(() => {
|
||||
load();
|
||||
}),
|
||||
[load],
|
||||
);
|
||||
|
||||
const reloadAllMemo = useCallback(
|
||||
(): Promise<void> =>
|
||||
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,
|
||||
};
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user