# 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 |