195 lines
9.1 KiB
Markdown
195 lines
9.1 KiB
Markdown
# 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`:
|
||
|
||
```python
|
||
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 | |