diff --git a/Docs/Tasks.md b/Docs/Tasks.md index 0095b99..f229737 100644 --- a/Docs/Tasks.md +++ b/Docs/Tasks.md @@ -1,46 +1,3 @@ -### TASK-BUG-06 — `JailConfigDetail` Form State Never Re-syncs After Background Refresh - -**Where found** -`frontend/src/components/config/JailsTab.tsx`. `JailConfigDetail` initialises all 20+ form fields from `jail` prop in `useState` calls (lines 126–161). The component uses `key={selectedActiveJail.name}`, which forces remount only when the *selected jail changes*, not when the data for the already-selected jail is refreshed by the parent. If `useJailConfigs` does a background refresh and delivers updated server data for the currently-selected jail, the form continues displaying the stale locally-edited values. - -**Goal** -Add a `useEffect` that resets form fields when the incoming `jail` prop changes identity (i.e. when a server refresh delivers a new object for the same jail name). The effect must only run when the user is not mid-edit. The cleanest approach is to track a `lastSavedJail` ref and compare it to the incoming `jail`; if the auto-save has no pending changes and `jail` has changed, reset the fields. - -Alternatively, expose a `resetToServer` button that lets the user explicitly pull the latest server state without relying on automatic detection. - -**Possible traps and issues** -- Automatically resetting a form a user is actively editing is hostile. The reset must only happen when `autoSave` reports no pending changes and no dirty state. -- Comparing the full `jail` object on every render is expensive; use a ref to track the last-applied server version by comparing a stable property like `jail.name + JSON.stringify(jail)` (hashed or shallow-compared field by field). -- This issue is partially mitigated by `key={selectedActiveJail.name}` forcing remount on jail selection change. - -**Docs changes needed** -None required. - -**Why this is needed** -If fail2ban reloads externally (e.g. another admin makes a change), the GUI background-refreshes the config but the currently-open form silently shows stale data. A save action would overwrite the external change. - ---- - -### TASK-BUG-07 — `useJails()` Called Twice on `JailsPage` (Double HTTP Request) - -**Where found** -`frontend/src/pages/JailsPage.tsx` line 11: `const { jails } = useJails();` — used only to extract `jailNames` for `useIpLookup`. `frontend/src/pages/jails/JailOverviewSection.tsx` line 55: `const { jails, ... } = useJails();` — the full feature hook. Both components are rendered simultaneously on `JailsPage`, causing two parallel `GET /api/jails` requests on every page load. - -**Goal** -Remove the `useJails()` call from `JailsPage`. Pass `jailNames` to `JailsPage`'s children as a prop from `JailOverviewSection`, or lift `useJails()` to `JailsPage` and thread `jails` down as a prop to `JailOverviewSection`. Since `JailOverviewSection` already owns all the jail operations, the simplest fix is to accept an optional `onJailNamesLoaded` callback or have `JailsPage` access jail names directly from `JailOverviewSection` via a ref or by reading from the single hook call. - -**Possible traps and issues** -- `JailsPage` currently passes `jailNames` to a separate `IpLookupSection` or similar. After consolidating to one `useJails()` call the prop-drilling path needs to be updated. -- `JailOverviewSection` is the authoritative consumer of `useJails`; making `JailsPage` the single call site and passing the result down as props is the cleanest structural change. - -**Docs changes needed** -None required. - -**Why this is needed** -Every visit to the Jails page sends two identical requests to the backend. At scale with many jails this doubles the serialization and deserialization cost for no benefit. - ---- - ### TASK-BUG-08 — `AssignActionDialog` and `AssignFilterDialog` Call `useJails()` When Closed **Where found** diff --git a/frontend/src/components/config/AssignActionDialog.tsx b/frontend/src/components/config/AssignActionDialog.tsx index 1220417..8a6c6a9 100644 --- a/frontend/src/components/config/AssignActionDialog.tsx +++ b/frontend/src/components/config/AssignActionDialog.tsx @@ -54,9 +54,8 @@ export interface AssignActionDialogProps { /** * Confirmation dialog for assigning an action to a jail. * - * Fetches running jails from the API for the jail selector. The user can - * optionally request a fail2ban reload immediately after the assignment is - * written. + * Wraps AssignActionDialogInner and only renders the inner component when + * the dialog is open, preventing unnecessary API calls when closed. * * @param props - Component props. * @returns JSX element. @@ -68,6 +67,43 @@ export function AssignActionDialog({ onAssigned, onAssign, }: AssignActionDialogProps): React.JSX.Element { + const handleClose = useCallback((): void => { + onClose(); + }, [onClose]); + + return ( + { if (!data.open) handleClose(); }}> + + {open && ( + + )} + + + ); +} + +interface AssignActionDialogInnerProps { + actionName: string | null; + onClose: () => void; + onAssigned: () => void; + onAssign: (jailName: string, payload: AssignActionRequest, reload: boolean) => Promise; +} + +/** + * Inner component that contains the dialog body and only renders when open. + * This ensures useJails() is only called when the dialog is actually visible. + */ +function AssignActionDialogInner({ + actionName, + onClose, + onAssigned, + onAssign, +}: AssignActionDialogInnerProps): React.JSX.Element { const { jails, loading: jailsLoading, error: jailsError } = useJails(); const [selectedJail, setSelectedJail] = useState(""); const [reload, setReload] = useState(false); @@ -77,17 +113,16 @@ export function AssignActionDialog({ const activeJails = jails.filter((j) => j.enabled); useEffect(() => { - if (!open) return; setError(null); setSelectedJail(""); setReload(false); - }, [open]); + }, []); useEffect(() => { - if (jailsError && open) { + if (jailsError) { setError(jailsError); } - }, [jailsError, open]); + }, [jailsError]); const handleClose = useCallback((): void => { if (submitting) return; @@ -119,86 +154,82 @@ export function AssignActionDialog({ const canConfirm = selectedJail !== "" && !submitting && !jailsLoading; return ( - { if (!data.open) handleClose(); }}> - - - Assign Action to Jail - - {actionName !== null && ( - - Assign action{" "} - - {actionName} - {" "} - to a jail. This appends the action to the jail's{" "} - action list in its .local override file. - - )} + + Assign Action to Jail + + {actionName !== null && ( + + Assign action{" "} + + {actionName} + {" "} + to a jail. This appends the action to the jail's{" "} + action list in its .local override file. + + )} - {error !== null && ( - - {error} - - )} + {error !== null && ( + + {error} + + )} - + {jailsLoading ? ( + + ) : ( + { setSelectedJail(d.value); }} - aria-label="Target jail" - > - - {activeJails.map((j) => ( - - ))} - - )} - + + {activeJails.map((j) => ( + + ))} + + )} + - { setReload(Boolean(d.checked)); }} - style={{ marginTop: tokens.spacingVerticalS }} - /> - - - - - - - - + { setReload(Boolean(d.checked)); }} + style={{ marginTop: tokens.spacingVerticalS }} + /> + + + + + + ); } diff --git a/frontend/src/components/config/AssignFilterDialog.tsx b/frontend/src/components/config/AssignFilterDialog.tsx index 328ab64..8eab8cf 100644 --- a/frontend/src/components/config/AssignFilterDialog.tsx +++ b/frontend/src/components/config/AssignFilterDialog.tsx @@ -54,9 +54,8 @@ export interface AssignFilterDialogProps { /** * Confirmation dialog for assigning a filter to a jail. * - * Fetches running jails from the API for the jail selector. The user can - * optionally request a fail2ban reload immediately after the assignment is - * written. + * Wraps AssignFilterDialogInner and only renders the inner component when + * the dialog is open, preventing unnecessary API calls when closed. * * @param props - Component props. * @returns JSX element. @@ -68,6 +67,43 @@ export function AssignFilterDialog({ onAssigned, onAssign, }: AssignFilterDialogProps): React.JSX.Element { + const handleClose = useCallback((): void => { + onClose(); + }, [onClose]); + + return ( + { if (!data.open) handleClose(); }}> + + {open && ( + + )} + + + ); +} + +interface AssignFilterDialogInnerProps { + filterName: string | null; + onClose: () => void; + onAssigned: () => void; + onAssign: (jailName: string, payload: AssignFilterRequest, reload: boolean) => Promise; +} + +/** + * Inner component that contains the dialog body and only renders when open. + * This ensures useJails() is only called when the dialog is actually visible. + */ +function AssignFilterDialogInner({ + filterName, + onClose, + onAssigned, + onAssign, +}: AssignFilterDialogInnerProps): React.JSX.Element { const { jails, loading: jailsLoading, error: jailsError } = useJails(); const [selectedJail, setSelectedJail] = useState(""); const [reload, setReload] = useState(false); @@ -77,17 +113,16 @@ export function AssignFilterDialog({ const activeJails = jails.filter((j) => j.enabled); useEffect(() => { - if (!open) return; setError(null); setSelectedJail(""); setReload(false); - }, [open]); + }, []); useEffect(() => { - if (jailsError && open) { + if (jailsError) { setError(jailsError); } - }, [jailsError, open]); + }, [jailsError]); const handleClose = useCallback((): void => { if (submitting) return; @@ -119,87 +154,83 @@ export function AssignFilterDialog({ const canConfirm = selectedJail !== "" && !submitting && !jailsLoading; return ( - { if (!data.open) handleClose(); }}> - - - Assign Filter to Jail - - {filterName !== null && ( - - Assign filter{" "} - - {filterName} - {" "} - to a jail. This writes{" "} - filter = {filterName}{" "} - into the jail's .local override file. - - )} + + Assign Filter to Jail + + {filterName !== null && ( + + Assign filter{" "} + + {filterName} + {" "} + to a jail. This writes{" "} + filter = {filterName}{" "} + into the jail's .local override file. + + )} - {error !== null && ( - - {error} - - )} + {error !== null && ( + + {error} + + )} - + {jailsLoading ? ( + + ) : ( + { setSelectedJail(d.value); }} - aria-label="Target jail" - > - - {activeJails.map((j) => ( - - ))} - - )} - + + {activeJails.map((j) => ( + + ))} + + )} + - { setReload(Boolean(d.checked)); }} - style={{ marginTop: tokens.spacingVerticalS }} - /> - - - - - - - - + { setReload(Boolean(d.checked)); }} + style={{ marginTop: tokens.spacingVerticalS }} + /> + + + + + + ); } diff --git a/frontend/src/components/config/__tests__/AssignFilterDialog.test.tsx b/frontend/src/components/config/__tests__/AssignFilterDialog.test.tsx index 358eef1..ddf7bc9 100644 --- a/frontend/src/components/config/__tests__/AssignFilterDialog.test.tsx +++ b/frontend/src/components/config/__tests__/AssignFilterDialog.test.tsx @@ -73,7 +73,9 @@ function renderDialog(overrides: Partial undefined), + onAssign: vi.fn(async (jailName, payload, reload) => { + await assignFilterToJail(jailName, payload, reload); + }), ...overrides, }; return render(