BanGUI/Docs/Tasks.md

TASK-ABORT-03 — Stale abortRef Read in .finally() in Three Hooks

Where found frontend/src/hooks/useGlobalConfig.ts, frontend/src/hooks/useServerSettings.ts, and frontend/src/hooks/useJailConfigDetail.ts. In each hook the .finally() block reads:

.finally(() => {
  if (!abortRef.current?.signal.aborted) {
    setLoading(false);
  }
});

abortRef.current is a mutable ref. If load() is called a second time before the first fetch completes, abortRef.current is replaced with a new controller. The first fetch's .finally() then reads the new controller's signal (which is not aborted) and calls setLoading(false) while the second fetch is still in flight.

Goal Capture the local controller reference before the async operation and use that in .finally():

const ctrl = new AbortController();
abortRef.current = ctrl;
// ...
.finally(() => {
  if (!ctrl.signal.aborted) {   // use the captured local variable
    setLoading(false);
  }
});

This is the pattern already used correctly in useListData, useBanTrend, and most other hooks.

Possible traps and issues

• This is a low-frequency race condition (only observable if load() is called in rapid succession), but it produces a spinner that disappears prematurely while a fetch is still in flight.
• After this fix verify that the existing checks in .then() and .catch() also use the locally-captured ctrl (not abortRef.current) — they typically already do, but confirm.

Docs changes needed Add a code convention note in Docs/Web-Development.md: "Always capture the AbortController in a local const and use that local variable in all callbacks — never read abortRef.current from inside an async callback."

Why this is needed The loading spinner is the primary feedback mechanism for data fetches. Prematurely clearing it while a request is still in flight misleads the user and can cause partial renders.

---

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:

const abortRef = useRef<AbortController | null>(null);
const lookup = useCallback(async (ip: string): Promise<void> => {
  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

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:

} 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:

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:

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:

} 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:

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<boolean>. 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:

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.