BanGUI/Docs/Tasks.md

TASK-BUG-04 — autoSavePayload Silently Drops Intentional Zero Values

Where found frontend/src/components/config/JailsTab.tsx lines 213–215:

ban_time: Number(banTime) || jail.ban_time,
find_time: Number(findTime) || jail.find_time,
max_retry: Number(maxRetry) || jail.max_retry,

The || operator treats 0 as falsy, so when a user sets ban_time to 0 (which in fail2ban means "ban permanently") the payload silently falls back to the server's current value. The user's intent is discarded without any error.

Goal Replace the || fallback with an explicit NaN guard:

ban_time: Number.isNaN(Number(banTime)) ? jail.ban_time : Number(banTime),
find_time: Number.isNaN(Number(findTime)) ? jail.find_time : Number(findTime),
max_retry: Number.isNaN(Number(maxRetry)) ? jail.max_retry : Number(maxRetry),

This preserves 0 and other valid numeric values while still falling back when the field contains non-numeric text.

Possible traps and issues

• An empty string converts to 0 via Number(""), which would then be sent to the API. If empty input is invalid, add a separate guard: only fall back if the trimmed string is empty or NaN.
• max_retry of 0 is meaningless in fail2ban (would never ban). Consider adding UI validation that shows a warning for max_retry < 1 rather than silently correcting it.

Docs changes needed None required.

Why this is needed ban_time = 0 in fail2ban sets a permanent ban — a common use case for admin hardening. The current code makes it impossible to save this value through the UI, with no error shown to the user.

---

TASK-BUG-05 — InactiveJailDetail.handleValidate Swallows Network Failures

Where found frontend/src/components/config/JailsTab.tsx inside InactiveJailDetail, the handleValidate callback:

onValidate()
  .then((result) => { setValidationResult(result); })
  .catch(() => { /* validation call failed — ignore */ })
  .finally(() => { setValidating(false); });

If the API call fails (network error, 500, timeout), the spinner stops, validationResult remains null, and there is zero user feedback. The user cannot distinguish a clean "no issues" result from a silent failure.

Goal Add error state to InactiveJailDetail and render a MessageBar intent="error" when validation fails:

.catch((err: unknown) => {
  setValidationError(err instanceof Error ? err.message : "Validation request failed.");
})

Clear validationError when the user clicks Validate again.

Possible traps and issues

• The existing validationResult display logic handles the empty-issues case with a success banner. Ensure the new error state renders instead of the success banner when set.
• handleFetchError should be used (or replicated) so that auth errors don't show a generic error banner — they should trigger the session-expiry flow instead.

Docs changes needed None required.

Why this is needed Silent failure is worse than a visible error. A user who validates a jail config and sees nothing has no idea whether validation passed or the server is unreachable.

---

TASK-BUG-06 — JailConfigDetail Form State Never Re-syncs After Background Refresh

Where found frontend/src/components/config/JailsTab.tsx. JailConfigDetail initialises all 20+ form fields from jail prop in useState calls (lines 126–161). The component uses key={selectedActiveJail.name}, which forces remount only when the selected jail changes, not when the data for the already-selected jail is refreshed by the parent. If useJailConfigs does a background refresh and delivers updated server data for the currently-selected jail, the form continues displaying the stale locally-edited values.

Goal Add a useEffect that resets form fields when the incoming jail prop changes identity (i.e. when a server refresh delivers a new object for the same jail name). The effect must only run when the user is not mid-edit. The cleanest approach is to track a lastSavedJail ref and compare it to the incoming jail; if the auto-save has no pending changes and jail has changed, reset the fields.

Alternatively, expose a resetToServer button that lets the user explicitly pull the latest server state without relying on automatic detection.

Possible traps and issues

• Automatically resetting a form a user is actively editing is hostile. The reset must only happen when autoSave reports no pending changes and no dirty state.
• Comparing the full jail object on every render is expensive; use a ref to track the last-applied server version by comparing a stable property like jail.name + JSON.stringify(jail) (hashed or shallow-compared field by field).
• This issue is partially mitigated by key={selectedActiveJail.name} forcing remount on jail selection change.

Docs changes needed None required.

Why this is needed If fail2ban reloads externally (e.g. another admin makes a change), the GUI background-refreshes the config but the currently-open form silently shows stale data. A save action would overwrite the external change.

---

TASK-BUG-07 — useJails() Called Twice on JailsPage (Double HTTP Request)

Where found frontend/src/pages/JailsPage.tsx line 11: const { jails } = useJails(); — used only to extract jailNames for useIpLookup. frontend/src/pages/jails/JailOverviewSection.tsx line 55: const { jails, ... } = useJails(); — the full feature hook. Both components are rendered simultaneously on JailsPage, causing two parallel GET /api/jails requests on every page load.

Goal Remove the useJails() call from JailsPage. Pass jailNames to JailsPage's children as a prop from JailOverviewSection, or lift useJails() to JailsPage and thread jails down as a prop to JailOverviewSection. Since JailOverviewSection already owns all the jail operations, the simplest fix is to accept an optional onJailNamesLoaded callback or have JailsPage access jail names directly from JailOverviewSection via a ref or by reading from the single hook call.

Possible traps and issues

• JailsPage currently passes jailNames to a separate IpLookupSection or similar. After consolidating to one useJails() call the prop-drilling path needs to be updated.
• JailOverviewSection is the authoritative consumer of useJails; making JailsPage the single call site and passing the result down as props is the cleanest structural change.

Docs changes needed None required.

Why this is needed Every visit to the Jails page sends two identical requests to the backend. At scale with many jails this doubles the serialization and deserialization cost for no benefit.

---

TASK-BUG-08 — AssignActionDialog and AssignFilterDialog Call useJails() When Closed

Where found frontend/src/components/config/AssignActionDialog.tsx line 71 and frontend/src/components/config/AssignFilterDialog.tsx line 71. Both components call useJails() unconditionally at the top of the component body. The parent mounts these dialogs regardless of their open prop (so they can animate in), meaning GET /api/jails is fired every time the Config page tab containing these dialogs renders, even when the dialogs are never opened.

Goal Gate the useJails() call behind the open prop. Because React hooks cannot be called conditionally, the fix is to extract the dialog body into a separate inner component that is only rendered when open is true:

export function AssignActionDialog({ open, ... }) {
  return open ? <AssignActionDialogInner ... /> : null;
}
function AssignActionDialogInner({ ... }) {
  const { jails, ... } = useJails();
  ...
}

This way useJails() only mounts (and fetches) when the dialog is actually open.

Possible traps and issues

• Fluent UI Dialog animations may require the wrapper element to always exist for the open/close animation to work. In that case keep the <Dialog open={open}> wrapper in the outer component and only render the dialog content conditionally.
• Since useJails() is already called on JailsPage and JailOverviewSection, ideally jail data would be passed as a prop rather than fetched again in the dialog. Longer term, a jail context or shared data store would eliminate redundant fetches.

Docs changes needed None required.

Why this is needed Config tab loads currently trigger GET /api/jails from up to four independent call sites simultaneously: JailsPage, JailOverviewSection, AssignActionDialog, and AssignFilterDialog. This creates unnecessary backend load.

---

TASK-BUG-09 — linesCount Input in ServerHealthSection Fires Fetch on Every Keystroke

Where found frontend/src/components/config/ServerHealthSection.tsx line 410: onChange={(_e, d) => { setLinesCount(Number(d.value)); }}. The linesCount value is passed directly to useServerHealth(linesCount, filterValue). useServerHealth re-creates its refresh callback when linesCount changes, which rebuilds fetchData, which triggers useEffect([fetchData]), firing a GET /api/config/server/log for every digit typed. Unlike filterValue (which is debounced at 500ms), linesCount has no debounce.

Goal Introduce a debouncedLinesCount state mirroring the existing filterValue / filterRaw pattern already in the component. Update the onChange handler to set a raw state, and apply a debounce (300–500ms) before committing to linesCount passed to useServerHealth.

Possible traps and issues

• The debounce ref pattern (filterDebounceRef) is already present in the component; the linesCount debounce should reuse the same approach to avoid introducing a second debounce timer ref.
• Number(d.value) on an empty field produces 0. Guard against 0 or negative values before passing to the API, or the backend may reject them.

Docs changes needed None required.

Why this is needed Typing "500" in the Lines field currently fires three HTTP requests ("5", "50", "500"). Each request fetches potentially hundreds of log lines and serializes them, adding unnecessary backend load.

---

TASK-ABORT-01 — Missing signal Parameter on Multiple API Functions

Where found The following API functions accept no signal parameter and therefore cannot be cancelled regardless of what the calling hook does:

File	Function
frontend/src/api/history.ts	fetchHistory, fetchIpHistory
frontend/src/api/map.ts	fetchBansByCountry
frontend/src/api/jails.ts	fetchActiveBans, fetchJails
frontend/src/api/config.ts	fetchJailConfig, fetchInactiveJails, fetchJailConfigFiles, fetchFilterFiles (partially — calls fetchFilters which accepts signal but discards it), fetchActionFiles, fetchFilterFile, fetchActionFile
frontend/src/api/blocklist.ts	fetchImportLog, previewBlocklist

Goal Add an optional signal?: AbortSignal parameter to each function and forward it to the underlying get() call. Example:

export async function fetchHistory(query: HistoryQuery, signal?: AbortSignal): Promise<HistoryResponse> {
  return get<HistoryResponse>(`${ENDPOINTS.history}?${buildQuery(query)}`, signal);
}

Then update every calling hook to forward its controller's signal.

Possible traps and issues

• fetchFilterFiles is an adapter that calls fetchFilters() internally. The correct fix is to accept signal and thread it into fetchFilters(signal), then map the result.
• previewBlocklist is called from useBlocklists.previewSource which has no abort machinery. That hook should also be updated to add an AbortController if unmount cancellation is desired.
• Search for all call sites after adding the parameter; TypeScript's optional ? means existing callers will not break but they should be updated to pass the signal where available.

Docs changes needed Add a coding convention to Docs/Web-Development.md: "All API functions that perform a GET request must accept an optional signal?: AbortSignal parameter and forward it to the HTTP client."

Why this is needed Without signals, navigating away from a page mid-fetch does not cancel the in-flight HTTP request. The response arrives, the callback fires, and React attempts a state update on an unmounted component. In development mode React warns about this; in production it causes silent no-ops and wastes server resources.

---

TASK-ABORT-02 — Hooks Create AbortController but Never Forward Signal to API

Where found Several hooks correctly create a controller and check signal.aborted in callbacks, but then call the API function without passing the signal — so the fetch is never actually cancelled:

Hook	API call missing signal
frontend/src/hooks/useConfigActiveStatus.ts	fetchJails(), fetchJailConfigs()
frontend/src/hooks/useMapData.ts	fetchBansByCountry(range, origin, source, countryCode)
frontend/src/hooks/useDashboardCountryData.ts	fetchBansByCountry(timeRange, origin, source)
frontend/src/hooks/useImportLog.ts	fetchImportLog(page, pageSize, sourceId)
frontend/src/hooks/useJailAdmin.ts	fetchInactiveJails()

Goal After completing TASK-ABORT-01, update each hook listed above to pass controller.signal (or ctrl.signal) to the API call. For example in useMapData:

fetchBansByCountry(range, origin, source, countryCode, controller.signal)

Possible traps and issues

• This task is a dependency of TASK-ABORT-01: the API functions must accept signal first.
• useConfigActiveStatus makes two sequential fetches; both need the same signal forwarded.
• After adding the signal parameter, ensure the abort check in .then() / .catch() callbacks uses controller.signal.aborted (the local capture), not abortRef.current?.signal.aborted (the mutable ref).

Docs changes needed None beyond TASK-ABORT-01.

Why this is needed The abort guard (if (controller.signal.aborted) return;) is present but pointless when the HTTP request itself was never aborted. The network request still completes and consumes server resources; only the state update is skipped.

---

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.