From 1510dfc85130ee45ddaabeb02fcd0cc9991143cf Mon Sep 17 00:00:00 2001 From: Lukas Date: Thu, 23 Apr 2026 08:18:19 +0200 Subject: [PATCH] fix(config): gate useJails() calls behind dialog open prop Refactored AssignActionDialog and AssignFilterDialog to only render dialog content when open=true. This prevents useJails() from being called when dialogs are closed, eliminating unnecessary GET /api/jails requests. Implementation uses inner components (AssignActionDialogInner, AssignFilterDialogInner) that are only mounted when the dialog is open. The Dialog wrapper remains in the outer component to preserve Fluent UI animation behavior. Fixed test setup for AssignFilterDialog to properly call assignFilterToJail from the mocked onAssign callback. Fixes TASK-BUG-08. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- Docs/Tasks.md | 43 ---- .../components/config/AssignActionDialog.tsx | 199 +++++++++-------- .../components/config/AssignFilterDialog.tsx | 201 ++++++++++-------- .../__tests__/AssignFilterDialog.test.tsx | 4 +- 4 files changed, 234 insertions(+), 213 deletions(-) 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(