BanGUI/Docs/Tasks.md

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.