lukas.pupkalipinski/BanGUI
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity
Files
1ba82d56e7600f28d577c4a3d718f030f6d38795
BanGUI/Docs/Tasks.md

46 KiB
Raw Blame History

BanGUI — Task List

This document breaks the entire BanGUI project into development stages, ordered so that each stage builds on the previous one. Every task is described in prose with enough detail for a developer to begin work. References point to the relevant documentation.

Reference: Docs/Refactoring.md for full analysis of each issue.

Open Issues

Issues are grouped by category and ordered roughly by severity. Each entry describes the current state, the desired end state, pitfalls to watch for, and which documentation needs updating when the task is done.

Bug Fixes / Correctness

TASK-001 — Race condition in useJailBannedIps: missing AbortController (done)

Where fixed: frontend/src/hooks/useJailBannedIps.ts, frontend/src/api/jails.ts

Summary: Added an AbortController ref to cancel stale fetches, passed the signal into fetchJailBannedIps, and abort on unmount.

Where found: frontend/src/hooks/useJailBannedIps.ts — the load callback is async and calls fetchJailBannedIps with no AbortSignal.

Goal: Add a useRef<AbortController | null> to useJailBannedIps. Before each fetch, abort the previous controller and create a new one. Pass signal to fetchJailBannedIps. In the useEffect cleanup return, abort the controller. After every await, check signal.aborted before calling any state setter.

Possible traps:

  • fetchJailBannedIps in api/jails.ts does not yet accept a signal — that parameter must be added first (see TASK-005).
  • The hook uses a debounced search value; the debounce timer itself does not need to be cancelled, only the in-flight request.
  • Setting state after abort causes a React warning in some versions; always guard with if (!ctrl.signal.aborted).

Docs changes needed: None.

Why: Without cancellation, rapidly changing the search or page can cause an older, slower response to overwrite the result of a newer request. The user sees stale data with no indication it is stale.

TASK-002 — HistoryPage filter effect has a stale appliedQuery dependency (done)

Where found: frontend/src/pages/HistoryPage.tsx lines 214–230. The useEffect lists appliedQuery as a dependency, reads it inside the effect, and then calls setAppliedQuery — which triggers the same effect again immediately.

Goal: Remove appliedQuery from the effect dependency array and use a useRef to hold the last-applied query for comparison. The ref must be updated synchronously when the query changes so the comparison is always accurate without creating a circular dependency.

Possible traps:

  • The areHistoryQueriesEqual utility must still be called to avoid redundant fetches; moving it to a ref pattern requires care not to skip the equality check.
  • Removing a dependency while keeping // eslint-disable-next-line comments is tempting but wrong; restructure so the lint rule is satisfied naturally.

Docs changes needed: None.

Why: The current code runs the effect one extra time on every filter change — once when filters change, and once because appliedQuery just changed. This causes two API calls per user interaction instead of one.

TASK-003 — BanUnbanForm floating promises and no double-submit guard (done)

Where fixed: frontend/src/pages/jails/BanUnbanForm.tsx, frontend/src/pages/jails/__tests__/BanUnbanForm.test.tsx

Summary: Converted ban and unban handlers to async functions with separate submit states and disabled submit buttons while requests are in flight.

Where found: frontend/src/pages/jails/BanUnbanForm.tsx — handleBan and handleUnban are synchronous functions that call onBan(…).then(…).catch(…). The returned promise is not awaited and is not assigned to anything.

Goal: Convert handleBan and handleUnban to async functions. Add an isSubmitting state variable. Set it to true before the API call and false in a finally block. Disable both submit buttons while isSubmitting is true. The promise chain can then become a simple try/catch/finally.

Possible traps:

  • If the outer handler is passed to an onClick that is typed as () => void, TypeScript will not warn about the lost promise; ESLint rule @typescript-eslint/no-misused-promises (see TASK-030) would catch this.
  • Two separate actions (ban and unban) share the same isSubmitting flag; use two flags (isBanning, isUnbanning) to allow one to proceed while the other is loading.

Docs changes needed: None.

Why: Without a double-submit guard, clicking "Ban" quickly fires multiple identical API requests. If the .catch() branch itself throws, the error is an unhandled rejection that surfaces as a console warning and is invisible to the user.

TASK-004 — KVEditor key rename silently overwrites duplicate keys (done)

Where fixed: frontend/src/components/config/KVEditor.tsx, frontend/src/components/config/configStyles.ts, frontend/src/components/config/__tests__/KVEditor.test.tsx

Goal: Before applying a key rename, check whether newKey already exists in entries. If it does, show a validation error inline (a MessageBar beneath the affected input row or a red border via validationState="error" on the Fluent UI Input). Block the onChange call until the conflict is resolved.

Possible traps:

  • The component is fully controlled (parent owns the entries object), so the error state must be local to KVEditor — adding it to the parent's state would be over-engineering.
  • While the user is mid-edit (typing the new key name), partial names that match existing keys should not be flagged until the input loses focus, to avoid premature errors.

Docs changes needed: None.

Why: Silently dropping the value of an existing key when the user renames another key to the same name is destructive data loss with no warning. Config settings that share a key name are semantically invalid in fail2ban anyway.

TASK-005 — SetupGuard redirects to /setup when the backend is temporarily unreachable (done)

Where fixed: frontend/src/components/SetupGuard.tsx

Where found: frontend/src/components/SetupGuard.tsx. When useSetup returns { loading: false, status: null } due to a network error, the guard treats this the same as "setup not completed" and redirects to /setup.

Goal: Destructure error from useSetup in addition to status and loading. Add an error branch: if error is non-null and status is null, render an error card (Fluent UI MessageBar with a Retry button) instead of redirecting. Only redirect to /setup when status is definitively { completed: false }.

Possible traps:

  • useSetup may need to be updated to expose an error field if it does not already.
  • There is a brief window on initial load where both loading and error are false and status is null; this state must not trigger a redirect.

Docs changes needed: None.

Why: A transient network hiccup during app startup causes a setup-complete user to be dropped into the setup wizard and potentially overwrite their configuration. This is a silent data-integrity risk.

Security

TASK-006 — No 401 interceptor: expired sessions show broken pages instead of redirecting (done)

Where found: frontend/src/api/client.ts, request function. All non-2xx responses including 401 are thrown as a generic ApiError. Consumers render "Failed to load…" messages instead of redirecting.

Goal: After the if (!response.ok) check, add a dedicated branch: if response.status === 401, dispatch a custom DOM event (bangui:session-expired) before throwing. In AuthProvider, listen for this event with window.addEventListener and call the existing logout cleanup logic, then navigate to /login. The event approach avoids coupling the API client to React context.

Possible traps:

  • The event listener must be added and removed inside a useEffect in AuthProvider to avoid leaks.
  • Pages that show a brief error flash before the redirect fires should have their error messages suppressed for ApiError with status 401 — wrap this in a shared utility (isAuthError(err)).
  • If the backend returns 403 (forbidden) rather than 401 for an expired token, the interceptor must also handle that status.

Docs changes needed: Update Docs/Backend-Development.md to document that 401 responses trigger client-side logout.

Why: Currently a user with an expired session sees multiple "Failed to load" error boxes and must manually navigate to /login. This is confusing and the broken state can be mistaken for a backend outage.

TASK-007 — Setup page password validation too weak (done)

Where found: frontend/src/pages/SetupPage.tsx, validate() function. Only masterPassword.length < 8 is checked.

Goal: Add minimum complexity rules: at least one uppercase letter, one number, and one special character (e.g. !@#$%^&*()). Show a password-strength indicator (a simple four-step progress bar is sufficient) that updates as the user types. Add a validation message per unmet rule rather than a single generic string.

Possible traps:

  • The complexity rules must match whatever the backend enforces; check backend/app/routers/setup.py for server-side validation and keep the two in sync.
  • The "confirm password" field comparison must run after the strength check, not instead of it, so both errors can be shown simultaneously.

Docs changes needed: Document the password policy in Docs/Instructions.md.

Why: An 8-character minimum allows trivially weak passwords such as 12345678. Because this is the single master credential for the entire application, it warrants stronger client-side guidance.

Performance

TASK-008 — buildBanColumns and HISTORY_COLUMNS recreated on every render (done)

Where fixed: frontend/src/components/BanTable.tsx, frontend/src/pages/HistoryPage.tsx

Where found:

  • frontend/src/components/BanTable.tsx — buildBanColumns(styles) called unconditionally in the render body.
  • frontend/src/pages/HistoryPage.tsx — HISTORY_COLUMNS(onClickIp, styles) called unconditionally in the render body.

Goal: Wrap both column-definition arrays in useMemo. In BanTable, the dependency is [styles]. In HistoryPage, the dependency is [styles] — the onClickIp callback should be wrapped in useCallback first so it has a stable reference.

Possible traps:

  • Fluent UI createTableColumn creates objects that are compared by reference inside DataGrid; if the array is recreated on every render, DataGrid will re-render all cells even when data has not changed.
  • The styles object from makeStyles is stable across renders (Fluent UI guarantees this), so [styles] as a dependency is effectively [] in practice.

Docs changes needed: None.

Why: Recreating column definitions every render triggers unnecessary cell re-renders in DataGrid, which is one of the heaviest Fluent UI components. On a page with 100 rows and 7 columns, this means 700 unnecessary cell renders per parent re-render.

TASK-009 — resolveFluentToken calls getComputedStyle on every render (done)

Where fixed: frontend/src/components/BanTrendChart.tsx, frontend/src/components/TopCountriesPieChart.tsx, frontend/src/components/TopCountriesBarChart.tsx, frontend/src/components/JailDistributionChart.tsx

Summary: Wrapped Fluent token resolution calls in useMemo([]) in each chart component and added tests verifying token resolution is memoized across rerenders.

Where found: frontend/src/utils/chartTheme.ts, resolveFluentToken function. Called 2–3 times per render in BanTrendChart, TopCountriesPieChart, TopCountriesBarChart, and JailDistributionChart.

Goal: In each chart component that calls resolveFluentToken, wrap the calls in useMemo with an empty dependency array [] (the theme never changes at runtime in the current implementation). When dark mode support is added (TASK-015), the dependency should change to the active theme object.

Possible traps:

  • resolveFluentToken reads a CSS custom property from a DOM element; calling it at module level (outside a component) would read before the FluentProvider has injected its tokens, returning an empty string.
  • The function itself should not be memoized globally — only the results per component, since each component may be mounted in a different theming context in tests.

Docs changes needed: None.

Why: getComputedStyle on every render is a forced style recalculation. On low-end hardware or when many charts are visible simultaneously, this measurably degrades frame rate.

TASK-010 — No code splitting: all pages bundled into the main chunk (done)

Where found: frontend/src/App.tsx — all page imports are static (import { DashboardPage } from "./pages/DashboardPage"). frontend/vite.config.ts has no build.rollupOptions.manualChunks.

Goal: Convert all seven page imports in App.tsx to React.lazy(() => import(…)). Wrap the <Routes> block in <React.Suspense fallback={<Spinner size="large" />}>. In vite.config.ts, add manualChunks to split react/react-dom, Fluent UI, Recharts, and the D3/TopoJSON geo stack into separate vendor chunks.

Possible traps:

  • React.lazy requires the module to have a default export; all current pages use named exports. Either add a re-export default in each page file, or use import(…).then(m => ({ default: m.PageName })) in the lazy call.
  • The <Suspense> fallback renders during navigation; place it inside <BrowserRouter> but outside <AuthProvider> so the spinner is not blocked by auth context loading.
  • The D3/TopoJSON data (world-atlas/countries-110m.json) is a large JSON file (~100 KB gzipped) and should be placed in the geo chunk alongside d3-geo and topojson-client.

Docs changes needed: None.

Why: Every user who opens the dashboard must download, parse, and execute JavaScript for the map, config editor, and blocklist pages — even if they never visit those routes. Code splitting allows the initial load to be significantly smaller.

TASK-011 — No React.memo on any heavy component (done)

Where found: Every component in frontend/src/components/ — zero uses of React.memo exist in the codebase.

Goal: Apply React.memo to the components whose props rarely change but whose render is expensive: WorldMap, BanTrendChart, JailDistributionChart, TopCountriesPieChart, TopCountriesBarChart, BanTable, and ServerStatusBar. Use the default shallow-equality comparator for props that are primitives. For WorldMap, provide a custom comparator that compares the countries record by key count and values (shallow object comparison).

Possible traps:

  • Inline function props (e.g. onSelectCountry={() => …}) will always be a new reference and defeat React.memo. Ensure all callback props passed to memoized components are wrapped in useCallback at the call site.
  • Inline style={{…}} objects passed as props also defeat memoization; these must be moved to makeStyles first (see TASK-018).
  • React.memo does not help if the component itself calls an expensive hook that always returns a new object.

Docs changes needed: None.

Why: DashboardPage re-renders whenever the 30-second server status poll fires, causing all five chart components and the ban table to re-render even though the filter state has not changed.

TASK-012 — useMapData sets loading=true before the debounce fires (done)

Where fixed: frontend/src/hooks/useMapData.ts, frontend/src/hooks/__tests__/useMapData.test.ts

Where found: frontend/src/hooks/useMapData.ts, load callback — setLoading(true) is called at the top of load, but the actual fetch is deferred inside a setTimeout of 300 ms.

Goal: Move setLoading(true) inside the setTimeout callback, immediately before the abortRef.current?.abort() call. This ensures the spinner only appears when a real network request is about to start.

Possible traps:

  • The caller (MapPage) uses loading to fade the map table with opacity: 0.5. There will now be a 300 ms window where filters have changed but the table is still showing old data at full opacity; this is acceptable and more correct than showing a spinner immediately.
  • If the component unmounts during the debounce window, the clearTimeout in the cleanup must run; verify the existing useEffect cleanup correctly returns clearTimeout.

Docs changes needed: None.

Why: Users see a loading spinner for 300 ms before any network activity has started, which makes the UI feel slower than it is.

Architecture / Code Quality

TASK-013 — Consolidate api/config.ts and api/file_config.ts duplicate functions (done)

Where fixed: frontend/src/api/config.ts, frontend/src/hooks/useFilterRawFile.ts, frontend/src/hooks/useActionRawFile.ts, frontend/src/components/config/JailFilesTab.tsx, frontend/src/components/config/ExportTab.tsx

Where found: Both frontend/src/api/config.ts and frontend/src/api/file_config.ts export identical functions: fetchJailConfigFiles, createJailConfigFile, fetchJailConfigFileContent, updateJailConfigFile, setJailConfigFileEnabled, fetchFilterFiles, fetchFilterFile, updateFilterFile, createFilterFile, fetchActionFiles, fetchActionFile, updateActionFile, createActionFile.

Goal: Delete api/file_config.ts. Update the four files that import from it (hooks/useFilterRawFile.ts, hooks/useActionRawFile.ts, components/config/JailFilesTab.tsx, components/config/ExportTab.tsx) to import the same functions from api/config.ts instead. Verify that both modules currently export functions with identical signatures before the merge; resolve any differences first.

Possible traps:

  • file_config.ts lacks AbortSignal parameters on all its functions, while the corresponding functions in config.ts may have them. After migration, all callers must be updated to pass the signal.
  • Running a global search for from.*file_config after the migration should return zero results; add this as a CI check via ESLint no-restricted-imports if desired.

Docs changes needed: None.

Why: Two modules exporting the same API functions will drift apart over time. Any bug fix or new feature applied to one will silently not apply to the other.

TASK-014 — Add AbortSignal to all API functions missing it (done)

Where found:

  • frontend/src/api/dashboard.ts — fetchServerStatus, fetchBansByJail, fetchBanTrend have no signal parameter.
  • frontend/src/api/jails.ts — fetchJailBannedIps (and others) have no signal parameter.
  • frontend/src/api/blocklist.ts — fetchSchedule (used by the polling hook) has no signal parameter.

Summary: Added optional signal parameters to dashboard API functions and updated useServerStatus, useBanTrend, useJailDistribution, and useBlocklistStatus to pass abort signals from refs. Added tests to verify server status and blocklist schedule polling use AbortSignal.

Goal: Add signal?: AbortSignal as the last parameter to every get/post/put/del wrapper call in these modules. The pattern is already established in api/config.ts and api/map.ts and should be replicated uniformly. After adding the parameters, update the consuming hooks to pass their abortRef.current.signal.

Possible traps:

  • Functions used by polling hooks (fetchServerStatus in useServerStatus, fetchSchedule in useBlocklistStatus) must pass the signal from a useRef<AbortController> rather than from a useEffect-local controller, because the poll fires outside the effect's lifecycle.
  • Adding a signal parameter is non-breaking (it is optional), so no consumers need to change immediately; they can be updated incrementally.

Docs changes needed: None.

Why: Without a signal, requests fired by polling hooks cannot be cancelled when the component unmounts, leaking network activity and potentially updating state on unmounted components (React warning).

TASK-015 — Standardise AbortController pattern across all hooks (done)

Where fixed: frontend/src/hooks/useSetup.ts, frontend/src/hooks/useServerHealth.ts, frontend/src/api/setup.ts, frontend/src/api/config.ts, frontend/src/hooks/README.md, frontend/src/hooks/__tests__/useSetupAndServerHealth.test.ts

Where found: Three different patterns exist in frontend/src/hooks/:

  1. useRef<AbortController | null> with manual abort before each fetch (correct — used in useActiveBans, useActionList).
  2. Inline AbortController created inside useEffect and aborted in cleanup (acceptable for single-fetch effects — used in useSchedule).
  3. No AbortController at all (incorrect — used in useJailBannedIps, useBlocklistStatus).

Goal: Hooks that expose a manual refresh() function must use pattern 1 (the useRef approach), because the abort must survive across effect re-runs. Hooks that only fetch once on mount and never need manual refresh may use pattern 2. Remove all instances of pattern 3. Document the chosen conventions in a comment block at the top of a new file frontend/src/hooks/README.md (or inline in fetchError.ts).

Possible traps:

  • When migrating useBlocklistStatus from a boolean cancelled flag to an AbortController, the underlying fetchSchedule call must first accept a signal (TASK-014).
  • Some hooks use both a debounce timer and an abort controller; the abort must cancel the in-flight request, while the timer cancellation prevents a new request from starting. These are independent concerns and should not be conflated.

Docs changes needed: Add an ADR (Architecture Decision Record) or hook convention note to Docs/Backend-Development.md or a new Docs/Frontend-Development.md.

Why: Inconsistent patterns make it hard for a new developer to know which approach to follow. The presence of pattern 3 causes memory leaks and stale-state React warnings in production.

TASK-016 — Extract generic useListData hook to eliminate duplicated fetch-list pattern (done)

Where fixed: frontend/src/hooks/useListData.ts, frontend/src/hooks/useActionList.ts, frontend/src/hooks/useFilterList.ts, frontend/src/hooks/useJailConfigs.ts, frontend/src/hooks/useBlocklists.ts, frontend/src/api/config.ts, frontend/src/api/blocklist.ts, frontend/src/hooks/__tests__/useListData.test.ts

Where found: frontend/src/hooks/useActionList.ts, useFilterList.ts, useJailConfigs.ts, and useBlocklists.ts each contain ~40 lines of nearly identical code: useState for data/loading/error, a useRef<AbortController>, a refresh callback with abort-create-set-loading-fetch-set logic, and a useEffect that calls refresh on mount.

Goal: Create frontend/src/hooks/useListData.ts that exports a generic hook:

function useListData<TResponse, TItem>(options: {
  fetcher: (signal: AbortSignal) => Promise<TResponse>;
  selector: (response: TResponse) => TItem[];
  errorMessage: string;
}): { items: TItem[]; loading: boolean; error: string | null; refresh: () => void }

Rewrite the four hooks listed above as thin wrappers around useListData.

Possible traps:

  • Some hooks (e.g. useJailConfigs) expose additional operations like reload and update beyond just listing. These should remain in their own hook and call useListData only for the list-loading portion.
  • The fetcher function must accept a signal; ensure all underlying API functions have been updated (TASK-014) before replacing the hook internals.
  • Generic hooks with complex type parameters can confuse TypeScript's inference; provide explicit type arguments at each call site to avoid unknown leaking out.

Docs changes needed: None.

Why: Four copies of identical logic means bug fixes must be applied four times. The pattern is stable enough to abstract — every list hook has the same contract.

TASK-017 — Move source derivation out of page components (done)

Where fixed: frontend/src/utils/queryUtils.ts, frontend/src/pages/DashboardPage.tsx, frontend/src/pages/MapPage.tsx

Summary: Added getDataSource to frontend/src/utils/queryUtils.ts and replaced inline timeRange === "24h" ternaries with the shared function in both dashboard and map pages.

Where found: frontend/src/pages/DashboardPage.tsx and frontend/src/pages/MapPage.tsx both contain the identical line:

const source = timeRange === "24h" ? "fail2ban" : "archive";

Goal: Create a utility function getDataSource(timeRange: TimeRange): "fail2ban" | "archive" in frontend/src/utils/constants.ts (or a new queryUtils.ts entry). Import and call it in both pages. Remove the inline ternary.

Possible traps:

  • This rule ("24h uses live fail2ban data, everything else uses the archive") is a backend contract, not a UI preference. If the backend ever adds a new time range or changes the source mapping, both places must be updated in sync; centralising it makes that change a one-liner.
  • The function name should make the business rule obvious — getLiveDataSource is more descriptive than getDataSource.

Docs changes needed: Document the time range → source mapping in Docs/Backend-Development.md if not already described.

Why: Duplicated business logic in UI components is fragile. If the rule changes or a new time range is added, it is easy to update one page and miss the other.

TASK-018 — Move inline style={{…}} objects to makeStyles (done)

Where fixed: frontend/src/pages/MapPage.tsx, frontend/src/pages/DashboardPage.tsx, frontend/src/pages/jail/PatternsSection.tsx, frontend/src/pages/jails/BanUnbanForm.tsx, frontend/src/layouts/MainLayout.tsx, frontend/src/pages/jails/jailsPageStyles.ts

Summary: Moved constant inline style objects into makeStyles classes and used mergeClasses for dynamic button layout in MainLayout. Kept runtime-dependent opacity and collapse state as conditional class application.

Where found: 30+ instances across frontend/src/pages/MapPage.tsx, frontend/src/pages/map/MapBansTable.tsx, frontend/src/pages/DashboardPage.tsx, frontend/src/pages/jail/PatternsSection.tsx, frontend/src/pages/jails/BanUnbanForm.tsx, frontend/src/layouts/MainLayout.tsx, and others.

Goal: For each instance of style={{…}} that is not dynamic (i.e. the style values are constants or token references, not computed from props), move the declaration into the nearest makeStyles call. Dynamic styles that depend on runtime values (e.g. style={{ opacity: loading ? 0.5 : 1 }}) should remain inline or be converted to conditional mergeClasses with two separate class definitions.

Possible traps:

  • The MainLayout logout button uses style={{ width: "100%", justifyContent: collapsed ? "center" : "flex-start" }} — this is dynamic and must become two makeStyles entries toggled with mergeClasses.
  • The WorldMap country <path> uses CSS custom properties (--country-fill) as inline styles for CSS variable injection; this is intentional and should not be changed.
  • Running ESLint with @typescript-eslint/no-unsafe-assignment can catch some object literals, but a manual pass is more reliable.

Docs changes needed: None.

Why: Inline style objects create a new object reference on every render, defeating React.memo (TASK-011) and preventing the browser from reusing cached styles. Fluent UI's makeStyles uses atomic CSS which is far more cache-efficient.

TASK-019 — Replace index keys with stable keys in editable lists (done)

Where fixed: frontend/src/components/config/StringListEditor.tsx, frontend/src/components/config/RegexList.tsx, frontend/src/components/config/JailsTab.tsx, frontend/src/components/config/stableListEntries.ts

Summary: Added stable internal IDs for editable string lists, replaced index-based React keys with stable entry IDs, and updated the jail log path list to use the path value as its key.

Where found: 19 instances identified, including:

  • frontend/src/components/config/StringListEditor.tsx line 34 — key={index} on editable Input rows.
  • frontend/src/components/config/RegexList.tsx line 66 — key={i} on editable regex rows.
  • frontend/src/components/config/JailsTab.tsx lines 384, 714, 726 — key={i} and key={idx}.
  • frontend/src/components/blocklist/BlocklistScheduleSection.tsx line 147 — key={i} on <option> elements.

Goal: For editable list components (StringListEditor, RegexList), each item needs a stable identity. Since the items are plain strings with no natural ID, generate a stable ID when an item is added (e.g. a running counter stored in a useRef, or a crypto.randomUUID() call). Store the list as { id: string; value: string }[] internally and emit string[] via onChange. For static read-only lists (<option> elements, skeleton items), index keys are acceptable and do not need to be changed.

Possible traps:

  • Changing the internal representation of StringListEditor from string[] to { id, value }[] changes the component's internal state shape but not its public interface (items: string[], onChange: (next: string[]) => void), so callers are unaffected.
  • If two items have the same string value (which is valid in some config fields), using the value itself as the key will cause the same de-duplication problem as using indices. Only a generated ID is reliably stable.

Docs changes needed: None.

Why: When React sees key={index} on input elements in an editable list and an item is deleted from the middle, it reuses the DOM node at that index rather than the one associated with the deleted item. The result is that the wrong input field appears to clear or change value.

TASK-020 — Standardise loading vs isLoading naming across hooks (done)

Where found: Dashboard and country-data hooks (useBans, useBanTrend, useDashboardCountryData) return isLoading. All other hooks (useActiveBans, useJails, useBlocklists, useFilterList, useActionList, etc.) return loading.

Goal: Standardise on loading: boolean throughout. Update the three hooks that use isLoading to rename the field. Update all call sites that destructure isLoading from those hooks to use loading.

Possible traps:

  • A global search-and-replace on isLoading will also match any variable named isLoading that is locally derived (e.g. const isLoading = loading && …); review each replacement manually.
  • The rename affects destructuring at the call site, not just the hook's return object; both must be updated atomically.

Docs changes needed: None.

Why: Inconsistent naming forces developers to check the hook signature every time they use one, and increases the chance of misreading state (treating loading as isLoading when they have different semantics in some codebase convention).

TASK-021 — Persist sidebar collapse preference to localStorage (done)

Where fixed: frontend/src/layouts/MainLayout.tsx

Where found: frontend/src/layouts/MainLayout.tsx — useState(() => window.innerWidth < 640) initialises collapsed state but never saves the user's toggle preference.

Goal: Replace the useState initialiser with one that reads from localStorage first (localStorage.getItem("bangui_sidebar_collapsed")), falling back to the window.innerWidth < 640 check. Add a useEffect that writes to localStorage whenever collapsed changes.

Possible traps:

  • localStorage access can throw in private-browsing mode in some browsers; wrap it in a try/catch and fall back silently to the width-based default.
  • The existing useEffect that watches matchMedia for viewport changes should continue to work but should not override a persisted preference; only auto-collapse/expand when there is no saved preference.

Docs changes needed: None.

Why: Users who prefer the expanded sidebar on a wide monitor should not have to re-expand it on every page refresh.

TASK-022 — Add dark mode support and OS preference detection (done)

Where fixed: frontend/src/App.tsx, frontend/src/providers/ThemeProvider.tsx, frontend/src/layouts/MainLayout.tsx, frontend/src/theme/customTheme.ts, frontend/src/components/BanTrendChart.tsx, frontend/src/components/TopCountriesPieChart.tsx, frontend/src/components/TopCountriesBarChart.tsx, frontend/src/components/JailDistributionChart.tsx, Docs/Web-Design.md

Where found: frontend/src/App.tsx line 25 — <FluentProvider theme={lightTheme}> is hardcoded. frontend/src/theme/customTheme.ts already exports darkTheme but it is never used.

Goal: In App.tsx (or a new ThemeProvider wrapper), initialise theme from localStorage.getItem("bangui_theme"). If no preference is stored, fall back to window.matchMedia("(prefers-color-scheme: dark)").matches. Add a toggle button in MainLayout's sidebar footer that switches between light and dark and persists the choice. Pass the active theme to <FluentProvider>.

Possible traps:

  • resolveFluentToken (used by chart components) reads CSS custom properties injected by FluentProvider. When the theme switches, the memoized color values from TASK-009 must be invalidated; add theme as a dependency to the useMemo calls in chart components.
  • The matchMedia listener for OS-level theme changes should update app state only when no explicit user preference has been saved.
  • The toggle button icon should be accessible — use SunRegular / WeatherMoonRegular from @fluentui/react-icons with an aria-label.

Docs changes needed: Update Docs/Web-Design.md to mention dark theme support.

Why: Dark mode is a standard accessibility and user-comfort feature. The infrastructure (darkTheme) already exists in the codebase; wiring it up is the only remaining work.

TASK-023 — Replace loose string types with union types in config models (done)

Where fixed: frontend/src/types/config.ts, frontend/src/types/jail.ts, frontend/src/components/config/JailsTab.tsx, frontend/src/components/config/JailSectionPanel.tsx, backend/app/models/config.py

Summary: Added explicit union types for DNSMode, LogEncoding, and BackendType in frontend config models and backend Pydantic models. Updated the affected jail config form state and select handlers to use the narrowed types.

Where found: frontend/src/types/config.ts — JailConfig.use_dns, JailConfig.log_encoding, JailConfig.backend, and several action/filter fields are typed as plain string despite having a fixed set of valid values defined by fail2ban.

Goal: Define union types for these fields:

type DNSMode = "yes" | "warn" | "no" | "raw";
type LogEncoding = "auto" | "ascii" | "utf-8" | "latin-1";
type BackendType = "auto" | "polling" | "pyinotify" | "systemd" | "gamin";

Update JailConfig (and the corresponding JailConfigUpdate patch type) to use these. Update any <Select> or <Input> components that render these fields to use the narrowed types.

Possible traps:

  • The backend Pydantic models should be the source of truth. Check backend/app/models/ to ensure the union type lists match the actual validation constraints before committing the frontend types.
  • If the backend returns a value not in the union (e.g. a custom backend plugin), TypeScript will flag it as a type error. A string & {} escape hatch or a broader union including string for unknown values may be needed.

Docs changes needed: None.

Why: Plain string types allow invalid values to be passed to API endpoints and rendered in select dropdowns without compile-time errors. Union types shift error detection from runtime to build time.

TASK-024 — Add useReducer to ServerTab and ConfFilesTab (done)

Status: done

Summary: Replaced the local useState clusters in ServerTab.tsx and ConfFilesTab.tsx with reducer-based state management, ensuring compound updates such as flush/reload/restart transitions and file content edits update atomically.

Where found:

  • frontend/src/components/config/ServerTab.tsx — 11 useState calls managing logLevel, logTarget, dbPurgeAge, dbMaxMatches, flushing, msg, isReloading, isRestarting, and three map threshold fields.
  • frontend/src/components/config/ConfFilesTab.tsx — 10 useState calls with a subtle synchronisation dependency between contents and editedContents.

Goal: For ServerTab, define a ServerTabState type and a serverTabReducer. All action buttons (flush, reload, restart) dispatch { type: "FLUSH_START" } / { type: "FLUSH_SUCCESS", result } / { type: "FLUSH_ERROR", message } actions that update related state atomically. For ConfFilesTab, use useReducer to ensure contents and editedContents are always updated in the same dispatch to prevent one being stale relative to the other.

Possible traps:

  • The useAutoSave hook in ServerTab subscribes to the updatePayload memo, which derives values from state. If the reducer replaces the individual useState calls, the memoisation dependencies must be updated to read from state.logLevel etc.
  • useReducer does not replace useRef for the abortRef pattern; keep abort controllers in refs, not in reducer state.

Docs changes needed: None.

Why: When multiple state values must change in response to a single action (e.g. isFlushing = true and msg = null both change when flush starts), useState requires two separate set calls that fire two renders. useReducer batches them into one.

TASK-025 — Consolidate barrel re-export files or replace with a single index.ts

Where found: frontend/src/hooks/useJails.ts, hooks/useConfig.ts, and hooks/useBlocklist.ts are pure re-export files that do nothing except forward exports from other hook files.

Goal: Either (a) delete the three barrel files and update all 20+ import sites to use direct imports (e.g. from "../hooks/useJailList" instead of from "../hooks/useJails"), or (b) consolidate them into a single frontend/src/hooks/index.ts that re-exports everything from the hooks folder, and update all imports to from "../hooks". Option (a) is preferred because it makes the import graph explicit and avoids accidentally importing unused hook modules.

Possible traps:

  • Some consumers import both useJails (the list hook) and the useJails name from useJails.ts (the barrel); after refactoring, the hook function formerly exported as useJails from the barrel now lives in useJailList.ts as useJails — the function name is the same, only the file path changes.
  • A global find-replace from from "../hooks/useJails" to from "../hooks/useJailList" should cover most cases, but verify by running the TypeScript compiler with --noEmit afterwards.

Docs changes needed: None.

Why: Barrel files that just re-export create an extra indirection layer that obscures the real source of a function. IDEs sometimes fail to navigate through barrel files correctly, breaking "Go to Definition".

TASK-026 — Props drilling: introduce a Dashboard filter context

Where found: frontend/src/pages/DashboardPage.tsx passes timeRange, originFilter, and source individually as props to BanTrendChart, BanTable, TopCountriesPieChart, TopCountriesBarChart, and JailDistributionChart.

Goal: Create frontend/src/providers/DashboardFilterProvider.tsx that wraps the dashboard content and exposes { timeRange, originFilter, source, setTimeRange, setOriginFilter } via context. Individual chart components read the filter values from the context instead of receiving them as props. DashboardFilterBar writes to the context rather than receiving callback props.

Possible traps:

  • Introducing a context for the dashboard is only justified if at least one deeply-nested component (more than one level) needs access; currently all consumers are direct children of DashboardPage, making this a marginal improvement. Evaluate whether the added complexity is worth it before starting.
  • If any chart is used outside the dashboard (e.g. in a modal or embedded view), removing the props would break that usage; keep props as optional overrides.

Docs changes needed: None.

Why: Passing the same three values to five sibling components is repetitive and makes each component's prop signature larger than necessary. A context also makes it easier to add new filter dimensions in the future.

Developer Experience / Tooling

TASK-027 — Add missing ESLint rules

Where found: frontend/eslint.config.ts — only react-hooks/recommended, no-explicit-any, explicit-function-return-type, and no-unused-vars are enabled. Several important rules are absent.

Goal: Add the following rules to eslint.config.ts:

  • @typescript-eslint/no-floating-promises: "error" — catches unawaited promises like those in BanUnbanForm (TASK-003).
  • @typescript-eslint/no-misused-promises: "error" — catches async functions passed as event handlers without void.
  • react/no-array-index-key: "warn" — requires adding eslint-plugin-react to devDependencies.
  • react/no-unstable-nested-components: "error" — prevents component definitions inside render functions.
  • @typescript-eslint/switch-exhaustiveness-check: "error" — ensures switch on a union type covers all cases.

Install eslint-plugin-react if not already present. Run npm run lint after each rule addition and fix all violations before enabling the rule at "error" level.

Possible traps:

  • no-floating-promises will flag every void someAsync() call; add void as an explicit acknowledgement only where the promise is intentionally fire-and-forget (e.g. the handleLogout onClick).
  • no-misused-promises has a known false-positive with Fluent UI's onCheckedChange prop, which passes a React synthetic event; configure the rule to ignore checkAsync patterns if needed.
  • Adding eslint-plugin-react requires configuring the plugin with { version: "detect" } in settings.

Docs changes needed: None.

Why: Several of the bugs identified in this task list (floating promises, index keys, unstable nested components) would be caught automatically at lint time with these rules. Prevention is cheaper than code review.

TASK-028 — Add security and SEO meta tags to index.html

Where found: frontend/index.html — the file contains only charset, viewport, and title meta tags.

Goal: Add the following:

  • <link rel="icon" href="/favicon.ico" /> (create a simple icon asset in frontend/public/).
  • <meta name="description" content="BanGUI — fail2ban management interface." />
  • <meta name="theme-color" content="#0F6CBD" /> (matches the brand primary colour).
  • <meta name="robots" content="noindex, nofollow" /> — this is a private admin interface; it should not be indexed by search engines.
  • <noscript>BanGUI requires JavaScript to run.</noscript>

Do not add a Content-Security-Policy meta tag; CSP should be set as an HTTP response header from the nginx configuration (Docker/nginx.conf) to prevent it being stripped by proxies.

Possible traps:

  • The theme-color meta tag is a hint to mobile browsers for the browser chrome colour; it should match lightTheme brand primary. When dark mode (TASK-022) is implemented, it can be updated dynamically via JavaScript.
  • Adding noindex is important because BanGUI runs on a local network and may be accidentally accessible via a public IP; preventing search engine indexing adds a minor layer of security through obscurity.

Docs changes needed: None.

Why: Missing noindex on an admin interface risks it being discovered via search engine crawlers if accidentally exposed. Missing <noscript> leaves users with JS disabled seeing a completely blank page with no explanation.

TASK-029 — Configure Vite proxy target via environment variable

Where found: frontend/vite.config.ts line 31 — target: "http://backend:8000" is a hardcoded Docker service hostname.

Goal: Change the proxy target to process.env["VITE_BACKEND_URL"] ?? "http://backend:8000". Document the variable in a frontend/.env.example file with the value VITE_BACKEND_URL=http://localhost:8000 for local development without Docker.

Possible traps:

  • Vite's loadEnv helper can be used inside defineConfig to read .env files, but process.env also works for variables set in the shell or Docker Compose environment. Either approach is acceptable.
  • The fallback "http://backend:8000" ensures the Docker compose stack continues to work without any changes.

Docs changes needed: Add VITE_BACKEND_URL to Docs/Backend-Development.md or a new Docs/Frontend-Development.md setup guide.

Why: Developers running the backend locally on a different port or hostname cannot run npm run dev without modifying the committed vite.config.ts.

TASK-030 — Add manual chunk splitting to vite.config.ts

Where found: frontend/vite.config.ts — build section is absent entirely; Rollup uses its default single-chunk strategy.

Goal: Add a build.rollupOptions.output.manualChunks function that groups modules into the following chunks: react-vendor (react, react-dom, react-router-dom), ui-vendor (@fluentui/react-components, @fluentui/react-icons), chart-vendor (recharts), geo-vendor (d3-geo, topojson-client, world-atlas). This is complementary to TASK-010 (lazy loading) but independent — chunk splitting improves cache efficiency even without lazy loading.

Possible traps:

  • manualChunks can be a function (id: string) => string | undefined; use id.includes("node_modules/react") style checks rather than exact package names to handle monorepo sub-packages.
  • Fluent UI has many sub-packages (@fluentui/react-components, @fluentui/merge-styles, @griffel/react, etc.); group all @fluentui and @griffel packages into the ui-vendor chunk.
  • The geo-vendor chunk containing the world atlas JSON will be large (~100 KB gzipped) and only needed on the /map route; this makes TASK-010 (lazy loading) particularly important for the map page.

Docs changes needed: None.

Why: Without chunk splitting, all vendor code is in a single bundle. When any vendor dependency is updated, the user must re-download the entire bundle. Splitting by vendor allows browsers to cache stable chunks across deployments.

Reference in New Issue View Git Blame Copy Permalink