### 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** `frontend/src/hooks/useJailList.ts`. The `withRefresh` helper is defined inside the hook body without `useCallback`. It wraps an operation and calls `refresh()` after it completes. Because it is recreated every render, `startJail`, `stopJail`, `setIdle`, and `reloadJail` — which are all produced by `withRefresh(...)` — are also new references every render. Any child component receiving these as props will re-render even when nothing has changed. **Goal** Wrap `withRefresh` in `useCallback` with `[refresh]` as dependency, or inline `useCallback` directly for each operation: ```ts const startJail = useCallback(async (name: string) => { await apiStartJail(name); refresh(); }, [refresh]); ``` This ensures the function references are stable between renders. **Possible traps and issues** - If `withRefresh` is used to produce many operations, it may be cleaner to keep `withRefresh` as a `useCallback` that captures `refresh`, rather than wrapping each call individually. - After stabilising these references, `JailOverviewSection` or any child wrapped in `React.memo` will correctly skip re-renders when only unrelated state changes. **Docs changes needed** None required. **Why this is needed** Unstable function references passed as props defeat `React.memo` optimisations and cause unnecessary child re-renders. On the Jails page, which renders a list of potentially many jails, this compounds with every parent state change. --- ### TASK-STATE-03 — `DashboardFilterBar` Has Dual State Source **Where found** `frontend/src/components/DashboardFilterBar.tsx`. The component reads from both `DashboardFilterProvider` context and from props using `??` as a fallback. `HistoryPage` passes filter values via props without mounting a `DashboardFilterProvider`, so the context values are `undefined` and the `??` fallback silently provides context defaults. `DashboardPage` uses context, so props are `undefined` and context values apply. Both pages render the same component but through different code paths. **Goal** Choose one data source and use it consistently. The recommended approach is to use props everywhere: `DashboardPage` should read from context via `useDashboardFilter()` and pass the values explicitly to `DashboardFilterBar` as props, exactly like `HistoryPage` does. This makes the component's behaviour predictable — it always reads from props, never from context. **Possible traps and issues** - `DashboardFilterProvider` may be used by other components on the dashboard. Audit all consumers of `useDashboardFilter()` before removing the context read from `DashboardFilterBar`. - The `??` fallback chain must be fully removed; otherwise the dual-source behaviour can creep back. **Docs changes needed** None required. **Why this is needed** Silent dual-source components are a debugging hazard. A developer adding a new consumer of `DashboardFilterBar` has no obvious signal about which data source is active, leading to subtle bugs when one source overrides the other. --- ### TASK-STATE-04 — Token in `sessionStorage` Is Never Sent (Misleading Auth Model) **Where found** `frontend/src/providers/AuthProvider.tsx`. After login, a JWT token and `expires_at` timestamp are stored in `sessionStorage`. `isAuthenticated` is derived entirely from the `expires_at` check against the local clock. However, `frontend/src/api/client.ts` uses `credentials: "include"` (cookie auth) for every request and never reads the token from `sessionStorage`. The stored token is purely decorative. **Goal** Remove the `sessionStorage` token storage entirely. `isAuthenticated` should instead be determined by whether the backend considers the session valid. The practical check is: the user is authenticated when the last request did not return 401/403. A simple approach is to set `isAuthenticated = true` on successful login, set it to `false` when `SESSION_EXPIRED_EVENT` fires, and persist only a boolean (or nothing at all) in `sessionStorage` for page-reload continuity. If the token is stored intentionally for future use (e.g. switching to Bearer token auth), add a clear comment explaining this, and add a `TODO` so it is not silently misleading. **Possible traps and issues** - If `expires_at` is used to proactively redirect the user to login before a request fails, removing it changes the UX: the user will stay on the page until the next request fails with 401. This is generally acceptable since the server is the authority on session validity. - Test that `SetupGuard` and `RequireAuth` still work correctly after removing the `sessionStorage` check. **Docs changes needed** Update `Docs/Web-Development.md` auth section to accurately describe the cookie-based auth model. **Why this is needed** The misleading code makes the auth model appear to be token-based when it is actually cookie-based. This causes confusion during development and maintenance, and the local clock `expires_at` check can cause premature logouts if there is clock skew between client and server. --- ### TASK-PERF-01 — `ConfigListDetail` Calls `sortItems()` on Every Render **Where found** `frontend/src/components/config/ConfigListDetail.tsx`. The `sortItems()` function is called directly in the render path without `useMemo`. For config lists with many items (e.g. all filters in a system with many services) this performs an O(n log n) sort on every render, including renders triggered by unrelated state changes. **Goal** Wrap the `sortItems()` call in `useMemo` with the items array as dependency: ```ts const sortedItems = useMemo(() => sortItems(items), [items]); ``` Replace all uses of the raw `items` in the render with `sortedItems`. **Possible traps and issues** - Verify that `items` array reference is stable (not recreated on every parent render). If the parent passes a new array each time, the `useMemo` will re-sort every render anyway. Trace `items` back to its source hook to confirm stability. - `sortItems` must be a pure function with no side effects for `useMemo` to be safe. Verify this. **Docs changes needed** None required. **Why this is needed** The Config page is already re-rendering due to multiple concurrent hooks. Adding an O(n log n) sort to each render cycle adds unnecessary CPU work, visible as jank when navigating long filter or action lists. --- ### TASK-PERF-02 — `useSchedule` Exposes No `refresh` Function **Where found** `frontend/src/hooks/useSchedule.ts`. The hook fetches the import schedule on mount and exposes no way to re-fetch. After a `PUT /api/blocklists/schedule` from a different component or tab, the displayed schedule data stays stale until the user navigates away and back. **Goal** Expose a `refresh` callback from `useSchedule`, following the same pattern as `useListData` and other hooks. The `BlocklistsPage` (or whichever component saves schedule changes) should call `refresh()` after a successful save, and after `runImportNow()` completes. **Possible traps and issues** - `useSchedule` currently uses a simple `useEffect` on mount. Adding `refresh` means converting the internal fetch into a `useCallback` and calling it from the effect. - Ensure the `AbortController` pattern is applied correctly when adding `refresh`. **Docs changes needed** None required. **Why this is needed** After saving a new schedule the user sees the old schedule until they reload the page. This makes the save feel broken even when it succeeded. --- ### TASK-QUALITY-01 — `KVEditor` Uses `entryKeys.join(",")` as Effect Dependency **Where found** `frontend/src/components/config/KVEditor.tsx`. An effect dependency is computed as `entryKeys.join(",")`. This works for most key values but produces incorrect results (false equality) when any key contains a comma character — two different key sets could produce the same joined string. **Goal** Replace the join-based comparison with a stable serialisation that cannot produce false equality. The simplest correct option is `JSON.stringify(entryKeys)`, which handles commas, empty strings, and special characters correctly. Alternatively, use `useDeepCompareEffect` from a utility library, or maintain a counter that increments whenever keys change. **Possible traps and issues** - `JSON.stringify` on a large array is marginally more expensive than `join`. For a config editor with typically fewer than 50 keys this cost is negligible. - Ensure the dependency is the full keys array (not the joined string) and let React's referential equality handle the common case; only reach for `JSON.stringify` if the array reference itself is not stable. **Docs changes needed** None required. **Why this is needed** A KV entry key containing a comma (e.g. `"a,b"` vs separate keys `"a"` and `"b"`) would cause the effect to not fire when it should, silently failing to update derived state. --- ### TASK-QUALITY-02 — `useConfigItem.save()` Briefly Shows Session-Expiry as Save Error **Where found** `frontend/src/hooks/useConfigItem.ts` lines 70–80. The `save()` function's catch block calls `setSaveError(err.message)` for all errors including `ApiError(401)` and `ApiError(403)`. The HTTP client layer dispatches `SESSION_EXPIRED_EVENT` on those status codes, which triggers auth handling, but `setSaveError` still runs first and may briefly display an "Unauthorized" or similar message before the navigation occurs. **Goal** Check for auth errors before setting save error state: ```ts } catch (err: unknown) { if (isAuthError(err)) throw err; // let auth handler deal with it const message = err instanceof Error ? err.message : "Failed to save data"; setSaveError(message); throw err; } ``` **Possible traps and issues** - Rethrowing auth errors is correct here since the caller might also have error handling. Confirm that all callers of `save()` handle the re-thrown auth error gracefully (typically by not doing anything — the session expiry flow handles navigation). - Import `isAuthError` from `../api/client`. **Docs changes needed** None required. **Why this is needed** Briefly flashing "Unauthorized" in a form's save-error field is confusing UX when the correct outcome is a redirect to the login page. --- ### TASK-QUALITY-03 — `useHistory` Object Identity Dependency Footgun **Where found** `frontend/src/hooks/useHistory.ts`. The hook accepts a `query` object as a parameter and lists it directly in the `useCallback` dependency array for the internal `load` function. If a caller passes an inline object literal on every render (e.g. `useHistory({ page: 1, jail: selectedJail })`), `query` is a new reference every render, causing a new `load` callback, which causes `useEffect([load])` to fire, triggering an infinite re-fetch. **Goal** Document this constraint prominently in the hook's JSDoc and in `Docs/Web-Development.md`. Alternatively, change the hook to accept individual primitive parameters instead of an object, eliminating the reference-stability requirement: ```ts export function useHistory(page: number, pageSize: number, jail?: string, ...): UseHistoryResult ``` This is the safest fix because it makes incorrect usage a compile-time error. **Possible traps and issues** - Changing the signature is a breaking change for all callers. Audit all call sites before changing the signature. - The interim documentation fix (a clear JSDoc warning) is a lower-risk option if refactoring callers is out of scope. **Docs changes needed** Add a note to `Docs/Web-Development.md`: "Hooks that accept objects as parameters must either destructure to primitives internally or require the caller to provide a stable reference (e.g. via `useMemo`)." **Why this is needed** This is a silent footgun. The hook works correctly in all current call sites only because callers happen to use `useMemo` or stable state references. A future caller passing an inline literal will introduce an infinite re-fetch with no obvious diagnostic. --- ### TASK-QUALITY-04 — `pendingSaveRef as boolean` Redundant Cast in `useAutoSave` **Where found** `frontend/src/hooks/useAutoSave.ts`. The code contains `if (pendingSaveRef.current as boolean)` where `pendingSaveRef` is already typed as `React.MutableRefObject`. The `as boolean` cast is redundant and suggests the author was uncertain about the type. **Goal** Remove the cast: `if (pendingSaveRef.current)`. Run TypeScript type-check to confirm no error is introduced. **Possible traps and issues** - None. This is a one-line cleanup. **Docs changes needed** None required. **Why this is needed** Redundant type assertions are noise that makes reviewers second-guess the type system. They also suppress TypeScript errors in cases where the cast is actually incorrect. --- ### TASK-QUALITY-05 — `console.warn` in `MapPage` Provides No User Feedback for Threshold Errors **Where found** `frontend/src/pages/MapPage.tsx` lines ~148–151: ```ts useEffect(() => { if (mapThresholdError) { console.warn("Failed to load map color thresholds:", mapThresholdError); } }, [mapThresholdError]); ``` When the threshold fetch fails the map silently falls back to hardcoded defaults. The user has no indication that their custom thresholds are not being applied. **Goal** Replace the `console.warn` with a small inline `MessageBar` or tooltip near the map legend that indicates thresholds could not be loaded and defaults are in use. The `console.warn` should be removed from production-facing code. **Possible traps and issues** - The fallback behaviour (using hardcoded defaults) is correct and the map should still render. The notification should be non-blocking (not a modal or full-page error). - If the threshold fetch failing is expected in certain deployment configurations (e.g. feature not configured), an info-level message rather than a warning may be more appropriate. **Docs changes needed** None required. **Why this is needed** `console.warn` is invisible to end users. If a custom threshold configuration is silently not applied, the map colour coding may be misleading with no indication of why. --- ### TASK-QUALITY-06 — `console.log` Leaked in `HistoryPage.test.tsx` **Where found** `frontend/src/pages/__tests__/HistoryPage.test.tsx` line 8. A `console.log` statement was left in the test file, likely from a debugging session. **Goal** Remove the `console.log` call. **Possible traps and issues** - None. **Docs changes needed** None required. **Why this is needed** Debug logs in test files pollute the test runner output and make it harder to spot real failures or warnings.