Lukas
f84aeef249
- Update AuthProvider with improved error handling and token management - Enhance API client with better request/response handling - Add comprehensive test coverage for auth flows - Update documentation with current tasks Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
150 lines
8.9 KiB
Markdown
150 lines
8.9 KiB
Markdown
### T-15 · Replace `window` event bus for session expiry with React context callback
|
|
|
|
**Where found:** `frontend/src/api/client.ts` — `window.dispatchEvent(new Event(SESSION_EXPIRED_EVENT))`; `frontend/src/providers/AuthProvider.tsx` — `window.addEventListener(SESSION_EXPIRED_EVENT, ...)`
|
|
|
|
**Why this is needed:** Using `window` as a side-channel bypasses React's component tree, breaks in non-browser environments (SSR, test environments without full JSDOM), and creates an invisible coupling between the API client and the auth provider. It also fires globally — any code anywhere that dispatches `bangui:session-expired` on window will trigger a logout, with no tracing possible.
|
|
|
|
**Goal:** The API client receives an `onUnauthorized` callback injected at the provider level, called directly instead of via a DOM event.
|
|
|
|
**What to do:**
|
|
1. Add an `onUnauthorized` callback to the API client (e.g. a module-level setter `setUnauthorizedHandler(fn)`).
|
|
2. In `AuthProvider`, call `setUnauthorizedHandler(handleSessionExpired)` on mount and reset it on unmount.
|
|
3. In `client.ts`, call the handler directly instead of `window.dispatchEvent`.
|
|
4. Remove `SESSION_EXPIRED_EVENT` string constant and the `window.addEventListener` in `AuthProvider`.
|
|
|
|
**Possible traps and issues:**
|
|
- The module-level handler setter is still global state — an alternative is to pass the handler as a parameter to `request()` via a context object, but that changes the API signature more significantly.
|
|
- Tests that mock `window.dispatchEvent` need updating.
|
|
- SSR / Vitest environments that already mock `window` may need adjustment.
|
|
|
|
**Docs changes needed:** None.
|
|
|
|
**Doc references:** `frontend/src/api/client.ts`, `frontend/src/providers/AuthProvider.tsx`
|
|
|
|
---
|
|
|
|
### T-16 · Centralise `PAGE_SIZE` frontend constants
|
|
|
|
**Where found:** `frontend/src/hooks/useBans.ts:14` (`PAGE_SIZE = 100`); `frontend/src/pages/HistoryPage.tsx:45` (`PAGE_SIZE = 50`)
|
|
|
|
**Why this is needed:** Page sizes can silently diverge from backend defaults. If the backend changes `_DEFAULT_PAGE_SIZE`, the frontend won't know. Having multiple files define the same concept differently is also misleading.
|
|
|
|
**Goal:** All pagination constants in `frontend/src/utils/constants.ts`.
|
|
|
|
**What to do:**
|
|
1. Add `BAN_PAGE_SIZE = 100`, `HISTORY_PAGE_SIZE = 50` to `frontend/src/utils/constants.ts` (create it if it doesn't exist).
|
|
2. Replace local `const PAGE_SIZE = ...` in each hook/page with imports.
|
|
|
|
**Possible traps and issues:** Trivial. Verify test snapshots don't hard-code the old inline constant.
|
|
|
|
**Docs changes needed:** None.
|
|
|
|
**Doc references:** `frontend/src/utils/constants.ts`
|
|
|
|
---
|
|
|
|
### T-17 · `useHistory` is missing abort-signal guards — stale state update bug
|
|
|
|
**Where found:** `frontend/src/hooks/useHistory.ts` — `.then()`, `.catch()`, `.finally()` callbacks update state without checking `abortRef.current.signal.aborted`
|
|
|
|
**Why this is needed:** Every other data-fetching hook in the codebase guards all state-update callbacks against aborted signals. `useHistory` does not. If the component unmounts mid-request, `setItems`, `setTotal`, `setLoading` will all fire on an unmounted component. In React 18 this is a no-op but it still indicates a broken invariant and `handleFetchError` could misclassify the abort as a real error (depends on whether `fetch` threw `AbortError` or the API module swallowed it).
|
|
|
|
**Goal:** All callbacks in `useHistory` check the abort signal before mutating state.
|
|
|
|
**What to do:**
|
|
1. Capture the controller in a local variable inside `load()` (already done: `abortRef.current = new AbortController()`).
|
|
2. In `.then()`: add `if (abortRef.current.signal.aborted) return;` before `setItems(...)`.
|
|
3. In `.catch()`: add the same guard before `handleFetchError(...)`.
|
|
4. In `.finally()`: add `if (!abortRef.current.signal.aborted)` before `setLoading(false)`.
|
|
|
|
**Possible traps and issues:**
|
|
- `abortRef.current` may have been replaced by a new controller before the callback fires. Capture the controller in a closure variable at the top of `load()`: `const controller = abortRef.current`.
|
|
|
|
**Docs changes needed:** None.
|
|
|
|
**Doc references:** `frontend/src/hooks/useHistory.ts`
|
|
|
|
---
|
|
|
|
### T-18 · Merge `useDashboardCountryData` and `useMapData` — near-identical hooks
|
|
|
|
**Where found:** `frontend/src/hooks/useDashboardCountryData.ts` and `frontend/src/hooks/useMapData.ts`
|
|
|
|
**Why this is needed:** Both hooks call `fetchBansByCountry`, maintain the same state shape (`countries`, `countryNames`, `bans`, `total`, `loading`, `error`), and implement the same abort-controller pattern. The only behavioural difference is that `useMapData` adds a 300ms debounce. Any bug fix must be applied to both.
|
|
|
|
**Goal:** A single `useBansByCountry` base hook; `useMapData` adds the debounce on top.
|
|
|
|
**What to do:**
|
|
1. Create `useBansByCountry(range, origin, source, countryCode?)` — the shared fetch logic without debounce.
|
|
2. Refactor `useDashboardCountryData` to wrap `useBansByCountry`.
|
|
3. Refactor `useMapData` to wrap `useBansByCountry` and add the debounce layer.
|
|
4. Keep the existing hook names as thin wrappers to preserve call sites.
|
|
|
|
**Possible traps and issues:**
|
|
- `useMapData` returns `{ data }` (the full response object) whereas `useDashboardCountryData` unpacks `countries`, `countryNames`, `bans`, `total`. Normalise the return shape before collapsing.
|
|
- Tests for each hook test them independently — update or merge.
|
|
|
|
**Docs changes needed:** None.
|
|
|
|
**Doc references:** `frontend/src/hooks/useDashboardCountryData.ts`, `frontend/src/hooks/useMapData.ts`
|
|
|
|
---
|
|
|
|
### T-19 · Move `DashboardFilterProvider` — page-scoped provider in wrong directory
|
|
|
|
**Where found:** `frontend/src/providers/DashboardFilterProvider.tsx` — instantiated only inside `DashboardPage.tsx`
|
|
|
|
**Why this is needed:** The `providers/` directory implies app-wide providers (alongside `AuthProvider`, `ThemeProvider`, `TimezoneProvider`). `DashboardFilterProvider` wraps only `DashboardPageContent` and is not used anywhere else. Its placement implies reuse that doesn't exist, misleading future contributors about its scope.
|
|
|
|
**Goal:** Co-located with its only consumer.
|
|
|
|
**What to do:**
|
|
1. Move `DashboardFilterProvider.tsx` to `frontend/src/pages/` (alongside `DashboardPage.tsx`) or to `frontend/src/pages/dashboard/` if the page is split into a subdirectory.
|
|
2. Update imports in `DashboardPage.tsx` and any tests.
|
|
|
|
**Possible traps and issues:** Only `DashboardPage.tsx` imports it — confirm with grep before moving.
|
|
|
|
**Docs changes needed:** `Docs/Web-Development.md` — document what belongs in `providers/` (app-wide) vs co-located.
|
|
|
|
**Doc references:** `Docs/Web-Development.md`
|
|
|
|
---
|
|
|
|
### T-20 · Replace inline `style={{}}` objects with `makeStyles` classes
|
|
|
|
**Where found:** `frontend/src/pages/map/MapBansTable.tsx` (multiple), `pages/JailDetailPage.tsx`, `pages/HistoryPage.tsx`, `pages/history/IpDetailView.tsx`, `components/WorldMap.tsx`, `components/TopCountriesPieChart.tsx`, `components/TopCountriesBarChart.tsx`
|
|
|
|
**Why this is needed:** The project uses Fluent UI's `makeStyles` with atomic CSS caching. Inline `style={{}}` objects are allocated on every render, bypass the atomic CSS cache, and are inconsistent with the established pattern. Exceptions are acceptable only for truly dynamic values (e.g. tooltip `left`/`top` that change on mouse move) — static layout values must use `makeStyles`.
|
|
|
|
**Goal:** All static layout properties moved to `makeStyles`. Inline styles only for genuinely dynamic values.
|
|
|
|
**What to do:**
|
|
1. Audit each file listed above.
|
|
2. For static `display: flex`, `gap`, `margin`, `padding` — move to `makeStyles` in that component's style block.
|
|
3. Keep inline `style` only where the value is truly dynamic at runtime (e.g. `WorldMap` tooltip position, `TopCountriesBarChart` chart height).
|
|
|
|
**Possible traps and issues:**
|
|
- `MapBansTable.tsx` has several consecutive inline `style` objects for its pagination row — these can be collapsed into one named class.
|
|
- Some components use both `tokens.*` values and inline styles — ensure `makeStyles` is imported where it isn't already.
|
|
|
|
**Docs changes needed:** `Docs/Web-Development.md` — add styling rule: use `makeStyles` for all static styles; inline `style` only for runtime-dynamic values.
|
|
|
|
**Doc references:** `Docs/Web-Development.md`, Fluent UI v9 docs on `makeStyles`
|
|
|
|
---
|
|
|
|
### T-21 · `Fail2BanMetadataService` has inline socket timeout hardcoded
|
|
|
|
**Where found:** `backend/app/services/fail2ban_metadata_service.py:64` — `socket_timeout: float = 5.0`
|
|
|
|
**Why this is needed:** Part of the same constant duplication as T-08. This file doesn't use `_SOCKET_TIMEOUT` as a module constant — it hardcodes `5.0` inline as a local variable. Should use the constant from `constants.py` once T-08 is done.
|
|
|
|
**Goal:** After T-08, replace with `FAIL2BAN_SOCKET_TIMEOUT_FAST` import.
|
|
|
|
**What to do:** Covered by T-08 sweep.
|
|
|
|
**Possible traps and issues:** None — dependent on T-08.
|
|
|
|
**Docs changes needed:** None.
|
|
|
|
**Doc references:** `backend/app/services/fail2ban_metadata_service.py` |