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

48 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

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.

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.

3. Remove Empty helpers/ Directory

Where: backend/app/helpers/ — contains only __init__.py and __pycache__/.

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.

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.

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.

5. Add Concurrency Comment / Guard to RuntimeState

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.).

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).

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.

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.

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.

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.

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.

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.

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 36–43): 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 (~100–200): 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").

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.

Docs changes needed: None.

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).

13. Split Multi-Component Files to One Component Per File

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.

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.

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 ~23–43) 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.

Docs changes needed: None.

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.

15. Replace Inline Styles and Hardcoded Values with makeStyles and Design Tokens

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.

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.

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.

Docs changes needed: None.

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.

16. Deduplicate TimeRange Type

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";.

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.

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.

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."

17. Move Runtime Constants Out of types/

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.

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.

Docs changes needed: None.

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.

18. Fix Type-Unsafe Casts and Missing import type Qualifiers

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.

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.

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.

Docs changes needed: None.

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.

19. Move Utility Functions Out of Page Modules

Where: frontend/src/pages/HistoryPage.tsx — the areHistoryQueriesEqual() function (lines ~135–146) 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.

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.

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.

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.

20. Relocate Misplaced Files

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.

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.

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.

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.

22. Surface Error State Instead of console.warn in useSetup

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.

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.

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.

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.

Reference in New Issue View Git Blame Copy Permalink