refactoring-backend #3

Merged
lukas.pupkalipinski merged 403 commits from refactoring-backend into main 2026-05-20 20:23:46 +02:00
Showing only changes of commit 9e7f881a8a - Show all commits

View File

@@ -8,488 +8,558 @@ Reference: `Docs/Refactoring.md` for full analysis of each issue.
## Open Issues
### 1. Arbitrary File Read in `preview_log()`
**Where:** `backend/app/services/log_service.py` — the `preview_log()` function (around line 212). Called from `backend/app/routers/config_misc.py` via `POST /api/config/preview-log`.
**Goal:** The `preview_log()` function must validate the incoming `log_path` against an allowlist of safe directory prefixes before reading the file, exactly the way `read_fail2ban_log()` already does at line 131 using `_SAFE_LOG_PREFIXES`. After the fix, any path outside the allowed directories must be rejected with a `ConfigOperationError`.
**Status:** Completed.
**Possible traps and issues:**
- The allowlist may need to be broader than `_SAFE_LOG_PREFIXES` (`/var/log`, `/config/log`) because `preview_log` is used with arbitrary log files that fail2ban monitors (e.g. `/var/log/auth.log`, `/var/log/nginx/access.log`). Restricting too tightly could break the regex tester feature.
- The path must be resolved (`.resolve()`) before checking prefixes, otherwise symlink-based bypasses are possible.
- Needs a test that confirms a path like `/etc/shadow` or `/proc/self/environ` is rejected.
**Docs changes needed:** None. This is an internal security hardening; the API contract does not change (invalid paths return an error response instead of file contents).
**Why this is needed:** An authenticated user can currently read the tail of any file readable by the backend process by sending a crafted `log_path` in the request body. This is an OWASP path traversal / arbitrary file read vulnerability. Even though it requires authentication, it violates the principle of least privilege — the endpoint should only read log files, not arbitrary system files.
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.
---
### 2. Missing Repository Protocols for `settings_repo` and `history_archive_repo`
**Status:** Completed.
**Where:** `backend/app/repositories/protocols.py` defines protocols for 5 of the 7 repositories. The two missing are `settings_repo.py` and `history_archive_repo.py`.
**Goal:** Define `SettingsRepository` and `HistoryArchiveRepository` protocols in `protocols.py` that match the public functions of `settings_repo` (`get_setting`, `set_setting`, `delete_setting`, `get_all_settings`) and `history_archive_repo` (`archive_ban_event`, `get_max_timeofban`, `get_archived_history`). Add corresponding dependency providers in `dependencies.py` and typed aliases so these repositories can be injected the same way as the other five.
**Possible traps and issues:**
- `settings_repo` is imported directly by `setup_service` and `auth_service` — these call sites need to be updated to accept the protocol type, or at minimum the direct import must remain compatible.
- `history_archive_repo` is imported inline in `history_service.py` and `ban_service.py` via `from app.repositories.history_archive_repo import ...`. Converting these to dependency injection requires threading the repo through service function signatures, which touches multiple call sites in routers and tasks.
- Changing function signatures is a breaking change for tests that call services directly.
**Docs changes needed:** Update `Docs/Architekture.md` section 2.2 (Repositories table) to list all 7 repositories as having protocols.
**Why this is needed:** The other 5 repositories all have protocol definitions, enabling clean test mocking via dependency injection. Without protocols, `settings_repo` and `history_archive_repo` are tightly coupled — tests must monkeypatch the module import rather than injecting a fake. This inconsistency makes the codebase harder to test and violates the project's own dependency inversion principle documented in section 10 of the architecture doc.
## Bug Fixes / Correctness
---
### 3. Remove Empty `helpers/` Directory
### TASK-001 — Race condition in `useJailBannedIps`: missing AbortController
**Where:** `backend/app/helpers/` — contains only `__init__.py` and `__pycache__/`.
**Where found:** `frontend/src/hooks/useJailBannedIps.ts` — the `load` callback is `async` and calls `fetchJailBannedIps` with no AbortSignal.
**Goal:** Delete the `backend/app/helpers/` directory entirely. All utility code already lives in `backend/app/utils/`, which is the documented location for helper modules.
**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 and issues:**
- Verify no import references `app.helpers` anywhere in the codebase before deleting.
- If any generated tooling or IDE configuration references `helpers/`, that needs cleanup too.
**Docs changes needed:** The architecture doc (`Docs/Architekture.md`) section 2.1 lists `helpers/` in the project structure tree. Remove that line.
**Status:** Completed.
**Why this is needed:** An empty package directory with no module inside it is confusing for developers. It suggests functionality should live there, when the project convention is to use `utils/`. Removing it eliminates ambiguity about where new helper code should go.
---
### 4. Add Path Validation to `_geoip_reader` Initialization
**Status:** Completed.
**Where:** `backend/app/services/geo_service.py` — the `init_geoip()` function (around line 249) sets the module-level `_geoip_reader` without acquiring `_cache_lock`.
**Goal:** Add a clear code comment documenting the startup-only assumption: `init_geoip()` must only be called during application startup (from `startup.py`) before request handling begins. Optionally, add an assertion or guard that prevents double-initialization.
**Possible traps and issues:**
- Wrapping `init_geoip()` in `_cache_lock` would require making it `async`, which changes the call site in `startup.py`. Since `geoip2.database.Reader()` does blocking file I/O, a lock alone is not sufficient — it would also need `run_blocking`.
- The simplest safe approach is a module-level `bool` flag (`_geoip_initialized`) checked at the top of `init_geoip()`, with a `RuntimeError` if called twice.
- Over-engineering this (full lock + async) is not worth it for a function called exactly once.
**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 this is needed:** A future developer unfamiliar with the startup sequence could call `init_geoip()` from a request handler or background task, introducing a data race on `_geoip_reader`. A guard and comment make the contract explicit and prevent silent bugs.
**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.
---
### 5. Add Concurrency Comment / Guard to `RuntimeState`
### TASK-002 — `HistoryPage` filter effect has a stale `appliedQuery` dependency
**Where:** `backend/app/utils/runtime_state.py` — the `RuntimeState` dataclass (line 47) and its mutation functions (`process_health_probe_result`, `record_activation`, `clear_pending_recovery`, etc.).
**Where found:** `frontend/src/pages/HistoryPage.tsx` lines 214230. The `useEffect` lists `appliedQuery` as a dependency, reads it inside the effect, and then calls `setAppliedQuery` — which triggers the same effect again immediately.
**Goal:** Add a module-level docstring section explicitly documenting the concurrency model: `RuntimeState` is safe only under single-worker asyncio with cooperative scheduling. Mutations must not span `await` points (no read-modify-write across suspension). If multi-worker deployment is ever needed, `RuntimeState` must be replaced with a shared backend (Redis, shared memory).
**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 and issues:**
- Adding locks to `RuntimeState` would be overkill for the current single-worker design and would complicate every read path.
- The real risk is a future developer adding an `await` between reading and writing a `RuntimeState` attribute in a new function, introducing a TOCTOU race. Documentation is the correct mitigation here, not locks.
- `process_health_probe_result()` already does read-then-write on `server_status` and `pending_recovery`, but since it runs in a background task that completes without yielding between those operations, it is safe today.
**Docs changes needed:** Update `Docs/Architekture.md` section 6 (Authentication & Session Management) where the session cache is already documented as process-local. Add a parallel note about `RuntimeState` in the same section or in a new "Concurrency Model" subsection.
**Why this is needed:** The safety of `RuntimeState` depends on an implicit assumption (single-worker, no `await` between read and write) that is not written down anywhere. If the deployment model changes or a developer modifies state-mutating functions, silent data races could appear with no warning. Explicit documentation prevents this.
**Status:** Completed.
---
### 6. `geo_service` Calls `db.commit()` Directly Instead of Deferring to Repository
**Where:** `backend/app/services/geo_service.py` — approximately 5 locations (lines 417, 443, 455, 637, 813) where the service calls `await db.commit()` directly after repository operations.
**Goal:** Move transaction commit calls into the repository layer or into a unit-of-work pattern so that the service layer does not manage database transaction boundaries. The repository functions (`upsert_entry`, `bulk_upsert_entries`, etc.) should commit internally, or the caller should use an explicit context manager that commits on exit.
**Possible traps and issues:**
- The current pattern exists because `geo_service` writes to the DB from both request handlers and background tasks, and commit timing differs between the two contexts. A blanket "always commit in the repo" approach could cause excessive commits during batch operations (e.g., `lookup_batch` resolves 100 IPs — committing after each `upsert_entry` instead of once at the end would be 100x more disk I/O).
- A pragmatic middle ground: keep `db.commit()` in the service for batch operations, but document why this deviates from the general pattern. Alternatively, add a `bulk_upsert_and_commit` repo method that handles both.
- Changing commit semantics can cause data loss if the commit is moved but an exception prevents it from running.
**Docs changes needed:** None required, but a comment in the architecture doc section 2.2 (Repositories) noting that geo cache bulk operations are an intentional exception to the "repositories own transactions" rule would help.
**Status:** Completed.
**Why this is needed:** Every other service delegates full DB lifecycle to the repository. `geo_service` is the only one calling `db.commit()` directly. This inconsistency makes it harder to reason about transaction boundaries and could confuse developers who expect the repository to handle commits.
---
### 7. Consider Splitting Large Service and Router Files
**Where:**
- `backend/app/services/jail_service.py` — 1287 lines
- `backend/app/services/ban_service.py` — 1068 lines
- `backend/app/services/action_config_service.py` — 949 lines
- `backend/app/services/raw_config_io_service.py` — 981 lines
- `backend/app/routers/file_config.py` — 814 lines
- `backend/app/routers/jail_config.py` — 763 lines
**Goal:** These files are cohesive today but trending toward sizes where navigation becomes difficult. `raw_config_io_service.py` in particular contains near-identical CRUD logic for three config file types (jail, filter, action). The goal is not to split them now, but to establish a size threshold (e.g., 800 lines) and plan extraction points:
- `raw_config_io_service.py` → extract shared `_list_conf_files`, `_read_conf_file`, `_write_conf_file`, `_create_conf_file` into a base helper, reducing the per-type functions to thin wrappers.
- `file_config.py` router → split into `jail_file_config.py`, `filter_file_config.py`, `action_file_config.py` (matching the existing `jail_config.py` / `filter_config.py` / `action_config.py` split).
- `ban_service.py` → extract aggregation/statistics functions (`ban_trend`, `bans_by_country`, `bans_by_jail`) into a `ban_stats_service.py`.
**Possible traps and issues:**
- Splitting services changes import paths, which breaks any test that imports from the old location.
- Moving functions between modules can introduce circular imports if the extracted module needs to call back into the original.
- Splitting too early (before the file is genuinely hard to navigate) adds indirection without benefit.
- `raw_config_io_service.py` deduplication looks tempting but the per-type functions have subtle differences in path resolution and validation that a generic helper must account for.
**Docs changes needed:** Update `Docs/Architekture.md` section 2.1 (Project Structure) and section 2.2 (Module Purposes) to reflect any new modules created.
**Status:** Completed.
**Why this is needed:** Large files slow down code review, increase merge conflict probability, and make it harder to find relevant code. Proactively identifying split points before the files grow further ensures the architecture stays navigable. This is a maintenance concern, not a correctness issue.
---
### 8. Split `api/config.ts` into Domain-Specific API Modules
**Where:** `frontend/src/api/config.ts` — a single file that currently bundles socket-based jail config, file-based config (jail.d / filter.d / action.d), server settings, log preview, regex testing, and jail activation/deactivation. The `endpoints.ts` file references `ENDPOINTS.health` but no API module calls it.
**Goal:** Break `api/config.ts` into the modules the architecture specifies: `api/file_config.ts` (all file-based read/write for jail.d, filter.d, action.d), `api/server.ts` (server settings, log level, syslog socket, DB path, purge age), and `api/health.ts` (the health check call using `ENDPOINTS.health`). The socket-based jail/filter config functions can remain in `api/config.ts` but should be limited to that domain. Additionally, ban-related functions currently spread across `api/dashboard.ts` and `api/jails.ts` should be consolidated into `api/bans.ts`, and IP geolocation calls from `api/jails.ts` should move to `api/geo.ts`.
**Possible traps and issues:**
- Every hook that imports from `api/config.ts` needs its import path updated. `useConfig.ts` is the primary consumer; its six internal hooks already map to the split domains, making the update mechanical once the modules are created.
- `api/health.ts` currently has no consumer — the `ENDPOINTS.health` constant exists but is unused. Adding the module is straightforward but a corresponding `useHealth` hook (or integration into `useServerStatus`) is needed for it to be called.
- Moving functions between modules changes the public surface; any test that imports API functions by path will fail and must be updated.
- Do not create stub files just to satisfy the architecture map — only create a module when there is at least one function to put in it.
**Docs changes needed:** `Docs/Architekture.md` section 3.2 (API Layer table) already lists the target modules; no text changes needed beyond verifying the table matches once the split is done.
**Status:** Completed.
**Why this is needed:** `api/config.ts` currently serves five conceptually unrelated domains. Developers looking for server settings functions or health check calls have no obvious place to find them, and the file will keep growing as new endpoints are added. Splitting it into focused modules makes the API layer match the documented architecture and makes new additions predictable.
---
### 9. Move Hooks Out of Provider Files
**Where:** `frontend/src/providers/AuthProvider.tsx` — exports `useAuth()` (defined at the bottom of the file, around line 111). `frontend/src/providers/TimezoneProvider.tsx` — exports `useTimezone()` (defined at the bottom of the file, around line 70).
**Goal:** Move `useAuth()` into its own file `frontend/src/hooks/useAuth.ts` and `useTimezone()` into `frontend/src/hooks/useTimezone.ts`. Each provider file should only define and export the context object and the provider component. The hooks should import the context from the provider file and re-export it from the `hooks/` directory.
**Status:** Completed.
**Possible traps and issues:**
- Every file that currently imports `useAuth` from `providers/AuthProvider` must update its import path. Run a global search for `from "../providers/AuthProvider"` and `from "../../providers/AuthProvider"` to find all consumers before moving.
- The context object (`AuthContext`, `TimezoneContext`) must remain exported from the provider file so the hook can import it. Do not move the context — only the hook function.
- The architecture rule is "each hook in its own file under `hooks/`". The hook file should be a thin wrapper that calls `useContext(AuthContext)` and throws if outside the provider — identical logic to what exists today, just at a different path.
**Docs changes needed:** None. The architecture doc already lists `hooks/useAuth.ts` as the canonical location.
**Why this is needed:** The rule is that all custom hooks live under `hooks/`, each in their own file. Hooks buried at the bottom of provider files are hard to discover and violate the separation between "provider component" (what wraps the app) and "consumer hook" (what components call). Developers looking for `useAuth` will search `hooks/` and not find it.
---
### 10. Split Multi-Hook Files into Single-Hook Files
**Where:**
- `frontend/src/hooks/useBlocklist.ts` — defines `useBlocklists`, `useSchedule`, `useImportLog`, `useRunImport` (four hooks in one file).
- `frontend/src/hooks/useConfig.ts` — defines `useJailConfigs`, `useJailConfigDetail`, `useGlobalConfig`, `useServerSettings`, `useRegexTester`, `useLogPreview` (six hooks in one file).
- `frontend/src/hooks/useJails.ts` — defines multiple hooks in one file.
**Goal:** Split each multi-hook file so that every hook lives in its own file following the `hooks/<hookName>.ts` naming convention. Create: `useBlocklists.ts`, `useSchedule.ts`, `useImportLog.ts`, `useRunImport.ts`, `useJailConfigs.ts`, `useJailConfigDetail.ts`, `useGlobalConfig.ts`, `useServerSettings.ts`, `useRegexTester.ts`, `useLogPreview.ts`, and whatever hooks are in `useJails.ts`. If hooks share internal utilities or types, extract those to a helper module — do not inline them in each new file.
**Status:** Completed.
**Possible traps and issues:**
- All existing import sites must be updated. Each page and component imports specific hooks by name from these files; the import path changes but the imported name stays the same.
- Hooks within the same file may share local helper functions or state logic that is not currently exported. Those helpers must be extracted into a shared internal module (e.g., `hooks/_configHelpers.ts`) or duplicated if they are truly trivial.
- After splitting, `useConfig.ts` and `useBlocklist.ts` themselves might become barrel re-export files. Decide upfront whether to keep them as barrels (for backward-compatible imports) or delete them entirely and update all import sites.
- `useConfig.ts` is the most complex — `useJailConfigDetail` may depend on state set up by `useJailConfigs`. If they share state, they must either be kept in the same file or refactored to communicate via props/context.
**Docs changes needed:** `Docs/Architekture.md` section 3.2 (Hooks table) already lists individual hooks; no text update needed, but verify the table is complete after the split.
**Why this is needed:** The rule is one hook per file. When six hooks are crammed into one file, the file grows to hundreds of lines and becomes a grab-bag. A developer looking for `useLogPreview` has no indication it lives inside `useConfig.ts`. Single-file hooks are immediately discoverable and separately testable.
---
### 11. Remove Direct API Calls from Components
**Where:** Eleven component files call the API layer directly, bypassing the required hooks layer:
- `frontend/src/components/config/JailsTab.tsx` (~lines 3643): imports and calls `addLogPath`, `deactivateJail`, `deleteJailLocalOverride`, `deleteLogPath`, `fetchInactiveJails`, `fetchJailConfigFileContent`, `updateJailConfigFile`, `validateJailConfig`.
- `frontend/src/components/config/FiltersTab.tsx` (~74, ~82, ~130): `fetchFilterFile`, `updateFilterFile`, `fetchFilters`.
- `frontend/src/components/config/ActionsTab.tsx` (~100200): `fetchActionFile`, `updateActionFile`, `removeActionFromJail`, `fetchActions`.
- `frontend/src/components/config/ConfFilesTab.tsx` (~60): `fetchList` inside a `useEffect`.
- `frontend/src/components/config/ActivateJailDialog.tsx` (~45, ~80): `validateJailConfig`, `activateJail`.
- `frontend/src/components/config/AssignActionDialog.tsx` (~30, ~70): `fetchJails`, `assignActionToJail`.
- `frontend/src/components/config/AssignFilterDialog.tsx`: `fetchJails`, `assignFilterToJail`.
- `frontend/src/components/config/CreateActionDialog.tsx` (~40): `createAction`.
- `frontend/src/components/config/CreateFilterDialog.tsx`: `createFilter`.
- `frontend/src/components/config/CreateJailDialog.tsx`: `createJailConfigFile`.
- `frontend/src/components/config/ServerHealthSection.tsx` (~45, ~80): `fetchServiceStatus`, `fetchFail2BanLog`.
- `frontend/src/components/blocklist/BlocklistSourcesSection.tsx` (~90): `fetchPreview` inside `PreviewDialog`.
**Goal:** Each component must receive its data and mutation callbacks via props or via a hook — it must never import from `api/` directly. For each component above: identify which operations it performs, create or extend the appropriate hook in `hooks/`, and replace the inline API call with a hook call or a callback prop. Dialog components (`ActivateJailDialog`, `AssignActionDialog`, etc.) are the trickiest — they typically need a single async action; these can accept an `onConfirm: () => Promise<void>` callback prop where the caller (the parent tab) provides the hook-backed implementation.
**Possible traps and issues:**
- `ServerHealthSection` combines polling, log streaming, and a one-shot reload into a single component body. The best split is a dedicated `useServerHealth` hook that owns the polling state and exposes `{ status, log, refresh, lineCount, setLineCount }`.
- Dialog components that call `validateJailConfig` during `useEffect` on open (like `ActivateJailDialog`) need the validation result as a prop or via a passed-in async callback — not fetched internally.
- Converting `fetchJails` calls inside dialogs to a prop means the parent tab must pass the jail list down. This is the correct data-flow direction per the architecture.
- `ConfFilesTab` does a `useEffect` fetch that is essentially a custom hook. Extracting it into `useConfFiles` is the minimal fix.
- After this change, many components will become purely presentational, which is the goal.
**Docs changes needed:** None. This task brings the code into conformance with the documented rules ("Components receive data via props only — they never call the API directly").
**Status:** Completed.
**Why this is needed:** Components that call the API directly are impossible to unit-test without mocking the network, cannot be reused with alternate data sources, and violate the layered architecture. The hook layer exists precisely to own data fetching so that the component layer stays declarative and testable.
---
### 12. Add AbortController Cleanup to All async `useEffect` Calls
**Where:** Multiple hooks and components create async fetch calls inside `useEffect` without an `AbortController`:
- `frontend/src/hooks/useBans.ts`: `doFetch` has no abort, allowing stale in-flight requests to race.
- `frontend/src/hooks/useActionConfig.ts`, `useFilterConfig.ts`, `useJailFileConfig.ts`: each passes an `AbortSignal` placeholder to `useConfigItem` but the underlying `fetchFn` callback ignores it — abort is silently dropped.
- `frontend/src/hooks/useBlocklist.ts` (`useSchedule`): `fetchSchedule` called with no abort.
- `frontend/src/hooks/useMapColorThresholds.ts`: `load` called with no abort.
- `frontend/src/hooks/useTimezoneData.ts`: `fetchTimezone()` with no cleanup returned from `useEffect`.
- `frontend/src/components/config/ConfFilesTab.tsx` (~60): `useEffect` fetch, no cleanup.
- `frontend/src/components/config/ActivateJailDialog.tsx` (~45): `validateJailConfig` in `useEffect`, no cleanup.
- `frontend/src/components/config/AssignActionDialog.tsx` (~30) and `AssignFilterDialog.tsx`: `fetchJails` in `useEffect`, no cleanup.
- `frontend/src/components/blocklist/BlocklistScheduleSection.tsx` (~60): `setTimeout` return not stored in a ref — cannot be cancelled on unmount.
**Goal:** Every `useEffect` that initiates an async operation must create an `AbortController`, pass its `signal` to the fetch call, and return a cleanup function that calls `controller.abort()`. For `setTimeout` usage in components, store the timeout ID in a `useRef` and clear it in the cleanup. For the `useConfigItem` pattern, the `fetchFn` callback type must accept a `signal: AbortSignal` parameter and pass it through to the underlying API call.
**Possible traps and issues:**
- `useConfigItem` is a shared internal hook consumed by `useActionConfig`, `useFilterConfig`, and `useJailFileConfig`. Changing its `fetchFn` signature to require a `signal` parameter means updating all three consumers simultaneously — make the change in one commit to avoid half-broken states.
- `AbortError` must be caught and silently ignored (it is not a real error — it is expected on unmount). Use `if (err instanceof DOMException && err.name === "AbortError") return;` in the catch block.
- Fetch calls inside dialog components that are currently being fixed (Task 11) will migrate to hooks anyway — those abort fixes can be done as part of Task 11 rather than separately.
- `BlocklistScheduleSection`'s `setTimeout` replacement with `useRef` is a one-liner but easy to forget to clear in the cleanup function.
**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.
**Status:** Completed.
**Why this is needed:** Without abort cleanup, a component that unmounts mid-fetch will still call `setState` on the unmounted component, producing React "can't perform state update on unmounted component" warnings and, more critically, stale responses from a fast-then-slow request sequence can silently overwrite fresher data. This is a reliability issue in any view that mounts and unmounts frequently (dialogs, paginated tabs).
**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.
---
### 13. Split Multi-Component Files to One Component Per File
### TASK-003 — `BanUnbanForm` floating promises and no double-submit guard
**Where:**
- `frontend/src/pages/JailsPage.tsx`: defines `JailOverviewSection`, `BanUnbanForm`, `IpLookupSection`, and `JailsPage` in one file.
- `frontend/src/pages/JailDetailPage.tsx`: defines `CodeList`, `JailInfoSection`, `PatternsSection`, `BantimeEscalationSection`, and `JailDetailPage` — five components in one file.
- `frontend/src/pages/BlocklistsPage.tsx`: defines `ImportResultDialog` and `BlocklistsPage` in one file.
- `frontend/src/components/config/FiltersTab.tsx`: defines `FilterDetail` and `FiltersTab`.
- `frontend/src/components/config/ActionsTab.tsx`: defines `ActionDetail` and `ActionsTab`.
- `frontend/src/components/config/JailFileForm.tsx`: defines `StringListEditor`, `KVEditor`, `JailSectionPanel`, `JailFileFormInner`, and the outer form component.
- `frontend/src/components/config/FilterForm.tsx`: defines `KVEditor` and `FilterFormEditor`.
- `frontend/src/components/config/ActionForm.tsx`: defines `KVEditor`, `CommandField`, and `ActionFormEditor`.
- `frontend/src/components/blocklist/BlocklistSourcesSection.tsx`: defines `SourceFormDialog`, `PreviewDialog`, and `BlocklistSourcesSection`.
**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:** Move each sub-component to its own file following the rule "one component per file, filename matches component name". Sub-components that are only used by one parent and have no reuse value outside of it can live in a co-located subdirectory alongside the parent (e.g., `pages/jails/BanUnbanForm.tsx`). Dialog sub-components extracted from blocklists should go into `components/blocklist/`. `KVEditor` is duplicated in `FilterForm.tsx` and `ActionForm.tsx` — after extraction it should become a single shared `components/config/KVEditor.tsx`.
**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 and issues:**
- `JailFileForm.tsx` is the most complex with five components sharing local type definitions. Extract common prop types to a `types/` file or the parent's file before splitting.
- `ImportResultDialog` in `BlocklistsPage.tsx` must be replaced with a Fluent UI `<Dialog>` (see Task 14) — do Tasks 13 and 14 for that file in the same pass to avoid touching the file twice.
- `KVEditor` appears identically in both `FilterForm.tsx` and `ActionForm.tsx`. After extracting it to a shared file, delete both inlined copies and import from the shared location.
- `CodeList`, `JailInfoSection`, etc. in `JailDetailPage.tsx` are not reused elsewhere. They can live in a `pages/jail/` subfolder. Ensure the main page import path stays valid.
**Docs changes needed:** `Docs/Architekture.md` section 3.1 project structure tree would eventually need updating if new subdirectories are created under `pages/` or `components/`. Update section 3.2 Components table to list any newly extracted reusable components.
**Why this is needed:** Files with multiple components grow unbounded as features are added. A developer looking for `BanUnbanForm` finds no file by that name — they have to search and discover it is buried hundreds of lines into `JailsPage.tsx`. The single-component-per-file rule exists so that every component is immediately discoverable by name in the file tree.
---
### 14. Replace Custom Modal Overlay with Fluent UI `<Dialog>`
**Where:** `frontend/src/pages/BlocklistsPage.tsx` — the `ImportResultDialog` component (lines ~2343) is a hand-built modal using `position: fixed`, a `rgba(0,0,0,0.5)` backdrop `<div>`, and custom inline styles. `frontend/src/components/ErrorBoundary.tsx` (line ~50) uses a raw `<button type="button">` element with inline styles for the reload action in the error fallback UI.
**Goal:** Replace `ImportResultDialog` with a standard Fluent UI `<Dialog>` from `@fluentui/react-components`, using `<DialogSurface>`, `<DialogTitle>`, `<DialogBody>`, and `<DialogActions>` slots. For `ErrorBoundary`, replace the raw `<button>` with a Fluent UI `<Button>` and remove all inline `style` props from the fallback UI, replacing them with `makeStyles` rules.
**Possible traps and issues:**
- `ImportResultDialog` uses `position: fixed` scroll-locking that Fluent UI `<Dialog>` handles natively. After the switch, verify that the dialog renders above the navigation sidebar (check the z-index layer order configured in `theme/tokens.ts`).
- The `ErrorBoundary` is a class component (required because `componentDidCatch` is a class lifecycle method). Fluent UI `<Button>` and `makeStyles` work fine from class components — `makeStyles` returns a hook but it can be called in a wrapper functional component that renders the fallback instead of inline in the class `render()`.
- After `ImportResultDialog` is replaced, the parent `BlocklistsPage` should pass `open` and `onDismiss` props to the Fluent dialog in the same way it currently controls the hand-built overlay.
- This fix should be done in the same pass as Task 13 for `BlocklistsPage` to avoid redundant edits to the same file.
**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.
**Status:** Completed.
**Why this is needed:** The architecture explicitly forbids building custom modal overlays ("Use Fluent UI `<Dialog>` for modals and confirmations — never build custom modal overlays"). Hand-built modals bypass Fluent UI's accessibility tree (focus trap, ARIA roles, portal rendering) and duplicate boilerplate that the library provides. A Fluent `<Dialog>` is keyboard-accessible, screen-reader-compatible, and inherits the theme automatically.
**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.
---
### 15. Replace Inline Styles and Hardcoded Values with `makeStyles` and Design Tokens
### TASK-004 — `KVEditor` key rename silently overwrites duplicate keys
**Where:** Inline `style={...}` props and hardcoded pixel/rem values appear throughout the codebase. Major concentrations:
- `frontend/src/pages/JailsPage.tsx`: `fontFamily: "Consolas, 'Courier New', monospace"`, `fontSize: "0.85rem"`, `marginLeft: "8px"`, `gap: "6px"`.
- `frontend/src/pages/JailDetailPage.tsx`: multiple `style={{ color: tokens.colorNeutralForeground3 }}`, `fontFamily: "Consolas, ..."`, and `marginTop: tokens.spacingVerticalS` as inline props.
- `frontend/src/pages/HistoryPage.tsx`: hardcoded `fontSize` strings in `makeStyles` (`"0.85rem"`, `"0.75rem"`, `"0.9rem"`, `"0.9em"`), inline styles on timeline items.
- `frontend/src/pages/MapPage.tsx`: loading spinner, filter bar, pagination footer — all inline.
- `frontend/src/components/config/RawConfigSection.tsx`: `<textarea style={{ fontFamily:"monospace", fontSize:"0.85rem", borderLeft:"3px solid…" }}>`.
- `frontend/src/components/config/AutoSaveIndicator.tsx`: `style={{ minWidth: 80 }}`, `style={{ opacity, transform, transition }}`.
- `frontend/src/components/config/JailFileForm.tsx`, `FilterForm.tsx`, `ActionForm.tsx`: `style={{ gap: 4 }}`, `style={{ marginBottom: 8 }}`, `style={{ width: 140 }}`, etc.
- `frontend/src/components/config/blocklistStyles.ts`: `fontSize: "12px"` (×2), `gap: "2px"`.
- `frontend/src/components/blocklist/BlocklistScheduleSection.tsx` and `BlocklistSourcesSection.tsx`: multiple inline style props with hardcoded pixel strings.
- `frontend/src/components/jail/BannedIpsSection.tsx`: `style={{ color: … }}`, `style={{ minWidth:"80px" }}`.
- `frontend/src/components/BanTrendChart.tsx`, `JailDistributionChart.tsx`, `TopCountriesBarChart.tsx`, `TopCountriesPieChart.tsx`: Recharts tooltip divs with `fontSize: "13px"`, `borderRadius: "4px"`, `padding: "8px 12px"` duplicated across all four files.
- `frontend/src/components/WorldMap.tsx`: `strokeWidth: 0.75`, `fontSize: "9px"` in `makeStyles`, and tooltip positioned with inline `style={{ left: tooltip.x + 12, top: tooltip.y + 12 }}`.
- `frontend/src/components/SetupGuard.tsx`: spinner container with inline flex styles.
**Where found:** `frontend/src/components/config/KVEditor.tsx`, `handleKeyChange` function (lines 1420).
**Goal:** All custom styling must use `makeStyles` from `@fluentui/react-components`. All size and colour values must use design tokens (`tokens.spacingVerticalM`, `tokens.fontSizeBase200`, `tokens.fontFamilyMonospace`, etc.) rather than literal pixel or rem strings. For the four chart tooltip components, extract a shared `ChartTooltip` wrapper component (or a `chartTooltipStyle` constant in `utils/chartTheme.ts`) to eliminate the duplication. Hardcoded values inside existing `makeStyles` calls (`"0.85rem"`, `"12px"`, `"9px"`) must be replaced with the appropriate token.
**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 and issues:**
- `tokens.fontFamilyMonospace` exists in Fluent UI v9 and should replace every `fontFamily: "Consolas, 'Courier New', monospace"` and `fontFamily: "monospace"` occurrence.
- For Recharts chart elements (`tick`, `activeDot`, `outerRadius`, `margin`), Recharts does not accept Griffel class names — those numeric props must use named constants defined once in `utils/chartTheme.ts` (e.g., `CHART_TICK_FONT_SIZE = 12`). This is the correct approach because Recharts bypasses the DOM styling entirely.
- The `WorldMap` tooltip position (`left: x + 12, top: y + 12`) is a dynamic inline style driven by runtime coordinates — this one is acceptable as a genuine dynamic value and does not need `makeStyles`. Document the exemption with a comment.
- `AutoSaveIndicator`'s `opacity`, `transform`, and `transition` for the save-state animation may be better expressed as Griffel `selectors` or via `mergeClasses` with conditional class names.
- `minWidth: 80` on `AutoSaveIndicator` and `minWidth: "80px"` on `BannedIpsSection` `<Dropdown>` should use a named `makeStyles` constant rather than a raw number, since there is no exact Fluent token for these values.
**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.
**Status:** Completed.
**Why this is needed:** Inline styles bypass Griffel's atomic CSS engine, produce non-deduplicatable style nodes, and cannot respond to theme changes. Hardcoded pixel values break when the user switches to a different Fluent UI theme with different spacing scales. The project rule states that `makeStyles` with design tokens is the only permitted styling mechanism.
**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.
---
### 16. Deduplicate `TimeRange` Type
### TASK-005 — `SetupGuard` redirects to `/setup` when the backend is temporarily unreachable
**Where:** `types/ban.ts` line ~13, `types/map.ts` line ~6, and `types/history.ts` line ~7 each contain an identical type alias: `export type TimeRange = "24h" | "7d" | "30d" | "365d";`.
**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:** Keep the single definition in `types/ban.ts` (it is the most general domain) and replace the duplicate definitions in `types/map.ts` and `types/history.ts` with `import type { TimeRange } from "./ban"` followed by a re-export if consumers import `TimeRange` from those files. Verify that all import sites resolve correctly after the change.
**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 and issues:**
- Search for all `import { TimeRange }` and `import type { TimeRange }` occurrences to identify every consumer. If a consumer imports from `types/history` or `types/map`, either update that import to point to `types/ban`, or re-export from the domain file with `export type { TimeRange } from "./ban"` to maintain backward-compatible imports without duplication.
- `TIME_RANGE_LABELS` in `types/ban.ts` (a runtime object mapping `TimeRange` keys to display strings) must also move to `utils/` as part of Task 17 — coordinate these two changes to avoid touching `types/ban.ts` twice.
**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 this is needed:** Three identical type definitions will diverge the moment someone changes the set of allowed time ranges in one file but forgets the others. The project rule is "define it once, import everywhere."
**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.
---
### 17. Move Runtime Constants Out of `types/`
## Security
**Where:** `frontend/src/types/ban.ts` — exports `BAN_ORIGIN_FILTER_LABELS` (a `Record<string, string>` mapping ban origin keys to display strings) and `TIME_RANGE_LABELS` (a `Record<TimeRange, string>` mapping time-range keys to display strings), both declared with `as const`. These are runtime JavaScript objects, not type declarations.
---
**Goal:** Move `BAN_ORIGIN_FILTER_LABELS` and `TIME_RANGE_LABELS` to `frontend/src/utils/constants.ts` (create the file if it does not exist, or add to an existing constants utility). Update all import sites. The `types/ban.ts` file should contain only `interface`, `type`, and `enum` declarations with no runtime-evaluatable code.
### TASK-006 — No 401 interceptor: expired sessions show broken pages instead of redirecting
**Possible traps and issues:**
- Search for every file that imports `BAN_ORIGIN_FILTER_LABELS` or `TIME_RANGE_LABELS` from `types/ban` and update the import path to `utils/constants`.
- If `utils/constants.ts` does not yet exist, create it. If it already exists, append the constants to it — do not create a second constants file.
- After the move, `types/ban.ts` should not need to import from `utils/` — the dependency direction is `utils/``types/` (utils import types), never the reverse.
**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
**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
**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.
**Status:** Completed.
**Why this is needed:** The `types/` directory is meant to contain zero runtime code — only TypeScript type declarations that are erased at compile time. A runtime `const` object in a types file is confusing, breaks the "no runtime code in types/" rule, and pollutes the bundle with a lookup table alongside pure type transpile artifacts.
**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.
---
### 18. Fix Type-Unsafe Casts and Missing `import type` Qualifiers
### TASK-009 — `resolveFluentToken` calls `getComputedStyle` on every render
**Where:**
- `frontend/src/pages/BlocklistsPage.tsx` (~line 50): `const safeUseBlocklistStyles = useBlocklistStyles as unknown as () => { root: string }` — a double cast that discards the real return type.
- `frontend/src/hooks/useDashboardCountryData.ts`: `setBans(data.bans as DashboardBanItem[])` — an unchecked cast on an API response.
- `frontend/src/pages/HistoryPage.tsx` (~line 31): `TableColumnDefinition` is a TypeScript-only type imported without the `type` modifier in a mixed import from `@fluentui/react-components`.
- `frontend/src/components/jail/BannedIpsSection.tsx` (~line 5): `TableColumnDefinition` inline-typed in a value import.
- `frontend/src/components/BanTable.tsx` (~line 4): same pattern.
**Where found:** `frontend/src/utils/chartTheme.ts`, `resolveFluentToken` function. Called 23 times per render in `BanTrendChart`, `TopCountriesPieChart`, `TopCountriesBarChart`, and `JailDistributionChart`.
**Goal:** Remove the `as unknown as` cast in `BlocklistsPage.tsx` — if `useBlocklistStyles` has the wrong return type signature, fix the `makeStyles` definition rather than casting around it. Replace `data.bans as DashboardBanItem[]` in `useDashboardCountryData.ts` with a type guard function that validates the shape at runtime (or remove the cast if TypeScript already infers the correct type from a typed API call). Convert all mixed value+type imports that include pure types to either use the inline `type` modifier (`import { type TableColumnDefinition, createTableColumn }`) or a separate `import type { TableColumnDefinition }` statement.
**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 and issues:**
- The `as unknown as` cast in `BlocklistsPage.tsx` likely exists because `blocklistStyles.ts` exports `useBlocklistStyles` with a type signature that does not match how it is consumed. The fix is to correct the `makeStyles` definitions in `blocklistStyles.ts` so the return type is accurate — then the cast is unnecessary.
- A runtime type guard for `DashboardBanItem[]` does not need to be exhaustive — checking that `data.bans` is an array and that the first element has an `ip` string field is sufficient to catch serialization bugs without adding heavy validation overhead.
- The `import type` change is a zero-risk change that only affects the TypeScript compiler output (unused import elimination); it never changes runtime behavior.
**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.
**Status:** Completed.
**Why this is needed:** `as unknown as T` is the TypeScript equivalent of `any` — it silences the type checker without any safety guarantee. If the actual runtime value does not match `T`, the error is deferred to a cryptic downstream failure. The `import type` issue is a code hygiene violation — it prevents tree-shaking tools from safely dropping type-only imports and misleads readers into thinking a type has a runtime value.
**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.
---
### 19. Move Utility Functions Out of Page Modules
### TASK-010 — No code splitting: all pages bundled into the main chunk
**Where:** `frontend/src/pages/HistoryPage.tsx` — the `areHistoryQueriesEqual()` function (lines ~135146) is a pure comparison utility defined inside the page module. It takes two query objects and returns a boolean — it has no React dependency and no side effects.
**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:** Move `areHistoryQueriesEqual` to `frontend/src/utils/` (either into a new `queryUtils.ts` or an existing utility file if a suitable one exists). Import it back into `HistoryPage.tsx` from that location. Verify the function has no implicit dependency on page-local types — if it relies on `HistoryQuery` from `types/history.ts`, it can still live in utils by importing that type.
**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.
**Status:** Completed.
**Possible traps and issues:**
- This is a mechanical move with no behavioral change. The only risk is a stale import if the function is used in more than one place (confirm with a search before moving).
- If similar utility functions exist in other page files (a search for `function` declarations inside page files is worthwhile), extract them in the same pass.
**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 this is needed:** The architecture rule is that pages contain "no business logic — only orchestration." A pure utility function with no JSX and no React dependency does not belong in a page module. It is invisible to other developers who might need the same comparison, cannot be independently unit-tested, and adds noise to the page's module surface.
**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.
---
### 20. Relocate Misplaced Files
### TASK-011 — No `React.memo` on any heavy component
**Where:**
- `frontend/src/data/isoNumericToAlpha2.ts` — a pure static lookup table (`Record<number, string>`) with no React dependency. The `data/` directory is not in the architecture specification.
- `frontend/src/api/map.test.ts` — a test file sitting directly in `api/` among source modules.
- `frontend/src/theme/commonStyles.ts` — exports `useCommonSectionStyles` and `useCardStyles`, which are `makeStyles`-based component style hooks, not theme token definitions.
- `frontend/src/components/config/configStyles.ts` — a standalone `makeStyles` file for config components instead of styles being co-located in each component file.
- `frontend/src/components/blocklist/blocklistStyles.ts` — same issue.
**Where found:** Every component in `frontend/src/components/` — zero uses of `React.memo` exist in the codebase.
**Goal:**
- Move `data/isoNumericToAlpha2.ts` to `utils/isoNumericToAlpha2.ts`. Delete the `data/` directory.
- Move `api/map.test.ts` to `api/__tests__/map.test.ts` (create the `__tests__` directory if needed).
- Move `theme/commonStyles.ts`: if the styles in it are shared across multiple components with no better home, move them to the component directory that uses them most, or rename the file to `components/commonStyles.ts`. The `theme/` directory must contain only `customTheme.ts` and token-related files.
- For `configStyles.ts` and `blocklistStyles.ts`: inline each exported `makeStyles` hook into the component file that uses it, or keep the file in the same directory as the components but document that it is a style helper rather than a theme definition. The architecture rule is "co-locate styles in the same file as the component" — a shared styles file is acceptable only if documented as an explicit exception for styles used by multiple components in the same subdirectory.
**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).
**Status:** Completed.
**Possible traps and issues:**
- Moving `isoNumericToAlpha2.ts` changes its import path in `WorldMap.tsx` and any other consumer — update all import sites.
- Moving `api/map.test.ts` to a `__tests__` subdirectory requires the test runner config (`vitest.config.ts`) to include `api/__tests__/` if it only scans `__tests__/` top-level directories — verify the glob pattern first.
- `theme/commonStyles.ts` is imported by multiple components. Moving it requires updating all consumers. Verify by grepping for `commonStyles` before touching it.
- `configStyles.ts` is imported by several files in `components/config/`. Moving its contents inline into each component file means duplicating styles that happen to be identical — evaluate whether a shared file is a justified exception before breaking it up.
**Docs changes needed:** `Docs/Architekture.md` section 3.1 (Project Structure) shows `theme/` with two files; update to reflect the correct contents after the move.
**Why this is needed:** Files placed in the wrong directory mislead developers about where to find things and where to put new related code. A `data/` directory that is not in the specification is a dead-end — future developers may put unrelated lookup tables there, or avoid it entirely because its purpose is unclear. Test files mixed alongside source modules are harder to exclude from coverage and harder to find.
---
### 21. Remove Test Scaffolding from Production Hooks
**Where:** `frontend/src/hooks/useMapData.ts` — exports `getLastArgs()` and `setMapData()` as named exports. These functions are test-only scaffolding (they expose internal state for assertion in tests) and have no legitimate use in production code.
**Goal:** Remove `getLastArgs` and `setMapData` from `useMapData.ts`. If the tests that use them cannot function without this scaffolding, refactor those tests to test the hook's public return value and side effects via `renderHook` from React Testing Library instead of reading internal state directly.
**Possible traps and issues:**
- Find every test that imports `getLastArgs` or `setMapData` from `useMapData` and rewrite them before removing the exports. Removing the exports first will cause a type error that points to the test files.
- `renderHook` from `@testing-library/react` is the correct tool for testing hook return values without exposing internals.
- If `setMapData` is used to inject test data, the test should instead mock the API layer (`api/map.ts`) so that when `useMapData` calls the API, it gets controlled data — no internal exposure required.
**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 this is needed:** Test scaffolding exported from production hooks ships in the production bundle, pollutes the module's public API, and creates a false dependency between the hook's internal implementation and its tests. If the internal structure of `useMapData` changes, tests that rely on `getLastArgs`/`setMapData` will break even if the observable behavior is unchanged. Testing via the public interface (return values, rendered output) makes refactoring safe.
**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.
---
### 22. Surface Error State Instead of `console.warn` in `useSetup`
### TASK-012 — `useMapData` sets `loading=true` before the debounce fires
**Where:** `frontend/src/hooks/useSetup.ts` (~line 60) — a `console.warn("Setup status check failed:", ...)` is the only response to a failed setup status fetch. The `error` state variable is not set.
**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:** Replace the `console.warn` with `setError(err instanceof Error ? err.message : "Unknown error")` so that the error is surfaced to the consuming component via the hook's public interface. Remove the `console.warn` entirely — the hook already has an `error` state for this purpose.
**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.
**Status:** Completed.
**Possible traps and issues:**
- Verify that `SetupPage.tsx` (or whatever consumes `useSetup`) already renders the `error` state. If it does not, add an error message display (a Fluent UI `<MessageBar intent="error">`) so the surfaced error is actually visible to the user.
- `console.warn` in hooks that ship to production will produce console noise for end users and may appear in error monitoring tools as unexpected warnings. The rule is to handle errors in state, not in the console.
**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 this is needed:** A silent `console.warn` means the first sign of a setup check failure is a spinner that never resolves — the user has no feedback and no actionable message. Error state in hooks exists precisely to enable the UI layer to display a meaningful error to the user.
**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
**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
**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.
**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
**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
**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:
```ts
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
**Where found:** `frontend/src/pages/DashboardPage.tsx` and `frontend/src/pages/MapPage.tsx` both contain the identical line:
```ts
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`
**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
**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
**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`
**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
**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
**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:
```ts
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`
**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.