BanGUI/Docs/Refactoring.md

BanGUI — Architecture Issues & Refactoring Plan

This document catalogues architecture violations, code smells, and structural issues found during a full project review. Issues are grouped by category and prioritised.

---

1. Service-to-Service Coupling (Backend)

The architecture mandates that dependencies flow routers → services → repositories, yet 15 service-to-service imports exist, with 7 using lazy imports to work around circular dependencies.

Source Service	Imports From	Line	Mechanism
history_service	ban_service	L29	Direct import of 3 private functions: _get_fail2ban_db_path, _parse_data_json, _ts_to_iso
auth_service	setup_service	L23	Top-level import
config_service	setup_service	L47	Top-level import
config_service	health_service	L891	Lazy import inside function
config_file_service	jail_service	L57–58	Top-level import + re-export of JailNotFoundError
blocklist_service	jail_service	L299	Lazy import
blocklist_service	geo_service	L343	Lazy import
jail_service	geo_service	L860, L1047	Lazy import (2 sites)
ban_service	geo_service	L251, L392	Lazy import (2 sites)
history_service	geo_service	L19	TYPE_CHECKING import

Impact: Circular dependency risk; lazy imports hide coupling; private function imports create fragile links between services.

Recommendation:

• Extract _get_fail2ban_db_path(), _parse_data_json(), _ts_to_iso() from ban_service to app/utils/fail2ban_db_utils.py (shared utility).
• Pass geo-enrichment as a callback parameter instead of each service importing geo_service directly. The router or dependency layer should wire this.
• Where services depend on another service's domain exceptions (e.g., JailNotFoundError), move exceptions to app/models/ or a shared app/exceptions.py.

---

2. God Modules (Backend)

Several service files far exceed a reasonable size for a single-domain module:

File	Lines	Functions	Issue
config_file_service.py	3105	73	Handles jails, filters, and actions — three distinct domains crammed into one file
jail_service.py	1382	34	Manages jail listing, status, controls, banned-IP queries, and geo enrichment
config_service.py	921	~20	Socket-based config, log preview, regex testing, and service status
file_config_service.py	1011	~20	Raw file I/O for jails, filters, and actions

Recommendation:

• Split config_file_service.py into filter_config_service.py, action_config_service.py, and a slimmed-down jail_config_service.py.
• Extract log-preview / regex-test functionality from config_service.py into a dedicated log_service.py.

---

3. Confusing Config Service Naming (Backend)

Three services with overlapping names handle different aspects of configuration, causing developer confusion:

Current Name	Purpose
config_service	Read/write via the fail2ban socket
config_file_service	Parse/activate/deactivate jails from files on disk
file_config_service	Raw file I/O for jail/filter/action .conf files

config_file_service vs file_config_service differ only by word order, making it easy to import the wrong one.

Recommendation: Rename for clarity:

• config_service → keep (socket-based)
• config_file_service → jail_activation_service (its main job is activating/deactivating jails)
• file_config_service → raw_config_io_service or merge into config_file_service

---

4. Architecture Doc Drift

The architecture doc does not fully reflect the current codebase:

Category	In Architecture Doc	Actually Exists	Notes
Repositories	4 listed	6 files	fail2ban_db_repo.py and geo_cache_repo.py are missing from the doc
Utils	4 listed	8 files	conffile_parser.py, config_parser.py, config_writer.py, jail_config.py are undocumented
Tasks	3 listed	4 files	geo_re_resolve.py is missing from the doc
Services	conffile_parser listed as a service	Actually in app/utils/	Doc says "Services" but the file is in utils/
Routers	file_config.py not listed	Exists	Missing from router table

Recommendation: Update the Architecture doc to reflect the actual file inventory.

---

5. Shared Private Functions Cross Service Boundary (Backend)

history_service.py imports three underscore-prefixed ("private") functions from ban_service.py:

from app.services.ban_service import _get_fail2ban_db_path, _parse_data_json, _ts_to_iso

These are implementation details of ban_service that should not be consumed externally. Their _ prefix signals they are not part of the public API.

Recommendation: Move these to app/utils/fail2ban_db_utils.py as public functions and import from there in both services.

---

6. Missing Error Boundaries (Frontend)

No React Error Boundary component exists anywhere in the frontend. A single unhandled exception in any component will crash the entire application with a white screen.

Recommendation: Add an <ErrorBoundary> wrapper in MainLayout.tsx or App.tsx with a fallback UI that shows the error and offers a retry/reload.

---

7. Duplicated Formatting Functions (Frontend)

Several formatting functions are independently defined in multiple files instead of being shared:

Function	Defined In	Also In
formatTimestamp()	BanTable.tsx (L103)	— (but fmtTime() in BannedIpsSection.tsx does the same thing)
fmtSeconds()	JailDetailPage.tsx (L152)	JailsPage.tsx (L147) — identical
fmtTime()	BannedIpsSection.tsx (L139)	—

Recommendation: Consolidate into src/utils/formatDate.ts and import from there.

---

8. Duplicated Hook Logic (Frontend)

Three hooks follow an identical fetch-then-save pattern with near-identical code:

Hook	Lines	Pattern
useFilterConfig.ts	91	Load item → expose save → handle abort
useActionConfig.ts	89	Load item → expose save → handle abort
useJailFileConfig.ts	76	Load item → expose save → handle abort

Recommendation: Create a generic useConfigItem<T>() hook that takes fetchFn and saveFn parameters and eliminates the triplication.

---

9. Inconsistent Error Handling in Hooks (Frontend)

Hooks handle errors differently:

• Some filter out AbortError (e.g., useHistory, useMapData)
• Others catch all errors indiscriminately (e.g., useBans, useBlocklist)

This means some hooks surface spurious "request aborted" errors to the UI while others don't.

Recommendation: Standardise a shared error-catching pattern, e.g. a handleFetchError(err, setError) utility that always filters AbortError.

---

10. No Global Request State / Caching (Frontend)

Each hook manages its own loading/error/data state independently. There is:

• No request deduplication (two components fetching the same data trigger two requests)
• No stale-while-revalidate caching
• No automatic background refetching

Recommendation: Consider adopting React Query (TanStack Query) or SWR for data-fetching hooks. This would eliminate boilerplate in every hook (abort handling, loading state, error state, caching) and provide automatic deduplication.

---

11. Large Frontend Components

Component	Lines	Issue
BlocklistsPage.tsx	968	Page does a lot: source list, add/edit dialogs, import log, schedule config
JailsTab.tsx	939	Combines jail list, config editing, raw config, validation, activate/deactivate
JailsPage.tsx	691	Mixes jail table, detail drawer, ban/unban forms
JailDetailPage.tsx	663	Full detail view with multiple sections

Recommendation: Extract sub-sections into focused child components. For example, JailsTab.tsx could delegate to <JailConfigEditor>, <JailValidation>, and <JailActivateDialog>.

---

12. Duplicated Section Styles (Frontend)

The same card/section styling pattern (backgroundColor, borderRadius, border, padding using Fluent UI tokens) is repeated across 13+ files. Each page recreates it in its own makeStyles block.

Recommendation: Define a shared useCardStyles() or export a sectionStyle in src/theme/commonStyles.ts and import it.

---

Summary by Priority

Priority	Issue	Section
High	Service-to-service coupling / circular deps	§1
High	God module config_file_service.py (3105 lines, 73 functions)	§2
High	Shared private function imports across services	§5
Medium	Confusing config service naming	§3
Medium	Architecture doc drift	§4
Medium	Missing error boundaries (frontend)	§6
Medium	No global request state / caching (frontend)	§10
Low	Duplicated formatting functions (frontend)	§7
Low	Duplicated hook logic (frontend)	§8
Low	Inconsistent error handling in hooks (frontend)	§9
Low	Large frontend components	§11
Low	Duplicated section styles (frontend)	§12