From 49f1b49ac6c5f7bdb2986ef285c91b48f3ece7ad Mon Sep 17 00:00:00 2001 From: Anso Date: Sun, 14 Jun 2026 13:42:01 -0400 Subject: [PATCH] fix(settings): clear stale pending and unsaved indicators after save (#1370) * fix(settings): clear stale pending and unsaved indicators after save Settings sections held their saved baseline in a mutable ref and computed the dirty count with useMemo keyed on the live values, so updating the ref on a successful save never re-ran the calculation. The masthead pending count and the sidebar unsaved dot stayed stale until the section remounted, making operators think the save had failed. Move the baseline into state behind a shared useSettingsDirty hook with separate load (reset) and save-success (markSaved) operations. markSaved adopts the submitted snapshot as the baseline only, so an edit made while a save is in flight survives and a failed save stays dirty and retryable. Migrate the five sections that used the pattern. * test(settings): await the save-failure retry assertion to avoid a race In the failed-save reconcile test, wait for the Save button to re-enable after the PATCH settles instead of asserting synchronously, so the retry check cannot race the isSaving reset. --- .../settings/DataRetentionSection.tsx | 32 +-- .../components/settings/DeveloperSection.tsx | 16 +- .../settings/DockerStorageSection.tsx | 25 +- .../components/settings/FleetMeshSection.tsx | 24 +- .../components/settings/HostAlertsSection.tsx | 29 +-- .../__tests__/SettingsDirtyReconcile.test.tsx | 235 ++++++++++++++++++ .../__tests__/useSettingsDirty.test.ts | 62 +++++ .../components/settings/useSettingsDirty.ts | 53 ++++ 8 files changed, 388 insertions(+), 88 deletions(-) create mode 100644 frontend/src/components/settings/__tests__/SettingsDirtyReconcile.test.tsx create mode 100644 frontend/src/components/settings/__tests__/useSettingsDirty.test.ts create mode 100644 frontend/src/components/settings/useSettingsDirty.ts diff --git a/frontend/src/components/settings/DataRetentionSection.tsx b/frontend/src/components/settings/DataRetentionSection.tsx index dc02d485..ad6a1346 100644 --- a/frontend/src/components/settings/DataRetentionSection.tsx +++ b/frontend/src/components/settings/DataRetentionSection.tsx @@ -1,4 +1,4 @@ -import { useState, useRef, useEffect, useMemo } from 'react'; +import { useState, useEffect } from 'react'; import { Input } from '@/components/ui/input'; import { Skeleton } from '@/components/ui/skeleton'; import { RefreshCw } from 'lucide-react'; @@ -15,6 +15,7 @@ import { SettingsSection } from './SettingsSection'; import { SettingsField } from './SettingsField'; import { SettingsActions, SettingsPrimaryButton } from './SettingsActions'; import { useMastheadStats } from './MastheadStatsContext'; +import { useSettingsDirty } from './useSettingsDirty'; interface DataRetentionSectionProps { onDirtyChange?: (dirty: boolean) => void; @@ -44,23 +45,10 @@ export function DataRetentionSection({ onDirtyChange }: DataRetentionSectionProp const { isPaid } = useLicense(); const { activeNode } = useNodes(); const readOnly = !isAdmin; - const [settings, setSettings] = useState({ ...DEFAULT_DATA_RETENTION }); - const serverSettingsRef = useRef({ ...DEFAULT_DATA_RETENTION }); + const { settings, setSettings, dirtyCount, hasChanges, reset, markSaved } = useSettingsDirty({ ...DEFAULT_DATA_RETENTION }); const [isLoading, setIsLoading] = useState(false); const [isSaving, setIsSaving] = useState(false); - const dirtyCount = useMemo(() => { - const baseline = serverSettingsRef.current; - let n = 0; - if (settings.metrics_retention_hours !== baseline.metrics_retention_hours) n++; - if (settings.log_retention_days !== baseline.log_retention_days) n++; - if (settings.audit_retention_days !== baseline.audit_retention_days) n++; - if (settings.scan_history_per_image_limit !== baseline.scan_history_per_image_limit) n++; - return n; - }, [settings]); - - const hasChanges = dirtyCount > 0; - useEffect(() => { onDirtyChange?.(hasChanges); }, [hasChanges, onDirtyChange]); @@ -89,8 +77,7 @@ export function DataRetentionSection({ onDirtyChange }: DataRetentionSectionProp audit_retention_days: nodeData.audit_retention_days ?? DEFAULT_SETTINGS.audit_retention_days, scan_history_per_image_limit: nodeData.scan_history_per_image_limit ?? DEFAULT_SETTINGS.scan_history_per_image_limit, }; - setSettings(safe); - serverSettingsRef.current = { ...safe }; + reset(safe); } catch (e) { console.error('Failed to fetch data retention settings', e); } finally { @@ -106,17 +93,18 @@ export function DataRetentionSection({ onDirtyChange }: DataRetentionSectionProp }; const saveSettings = async () => { + const submitted = { ...settings }; const payload: DataRetentionFields = { - metrics_retention_hours: settings.metrics_retention_hours, - log_retention_days: settings.log_retention_days, - scan_history_per_image_limit: settings.scan_history_per_image_limit, + metrics_retention_hours: submitted.metrics_retention_hours, + log_retention_days: submitted.log_retention_days, + scan_history_per_image_limit: submitted.scan_history_per_image_limit, }; // audit_retention_days is a paid-only key the backend rejects from a // Community operator. The field renders only when isPaid, so include it // in the save only then; otherwise a Community save would 403 on a key // the operator cannot edit and never sees. if (isPaid) { - payload.audit_retention_days = settings.audit_retention_days; + payload.audit_retention_days = submitted.audit_retention_days; } setIsSaving(true); try { @@ -129,7 +117,7 @@ export function DataRetentionSection({ onDirtyChange }: DataRetentionSectionProp toast.error(err?.error || err?.message || 'Failed to save settings.'); return; } - serverSettingsRef.current = { ...settings }; + markSaved(submitted); toast.success('Data retention saved.'); window.dispatchEvent(new CustomEvent(SENCHO_SETTINGS_CHANGED, { detail: { changedKeys: Object.keys(payload) }, diff --git a/frontend/src/components/settings/DeveloperSection.tsx b/frontend/src/components/settings/DeveloperSection.tsx index 09fe37e3..235a9f26 100644 --- a/frontend/src/components/settings/DeveloperSection.tsx +++ b/frontend/src/components/settings/DeveloperSection.tsx @@ -1,4 +1,4 @@ -import { useState, useRef, useEffect } from 'react'; +import { useState, useEffect } from 'react'; import { TogglePill } from '@/components/ui/toggle-pill'; import { Skeleton } from '@/components/ui/skeleton'; import { useAuth } from '@/context/AuthContext'; @@ -14,6 +14,7 @@ import { SettingsSection } from './SettingsSection'; import { SettingsField } from './SettingsField'; import { SettingsActions, SettingsPrimaryButton } from './SettingsActions'; import { useMastheadStats } from './MastheadStatsContext'; +import { useSettingsDirty } from './useSettingsDirty'; interface DeveloperSectionProps { onDirtyChange?: (dirty: boolean) => void; @@ -37,13 +38,10 @@ export function DeveloperSection({ onDirtyChange }: DeveloperSectionProps) { const { isAdmin } = useAuth(); const { activeNode } = useNodes(); const readOnly = !isAdmin; - const [settings, setSettings] = useState({ ...DEFAULT_DEVELOPER }); - const serverSettingsRef = useRef({ ...DEFAULT_DEVELOPER }); + const { settings, setSettings, hasChanges, reset, markSaved } = useSettingsDirty({ ...DEFAULT_DEVELOPER }); const [isLoading, setIsLoading] = useState(false); const [isSaving, setIsSaving] = useState(false); - const hasChanges = settings.developer_mode !== serverSettingsRef.current.developer_mode; - useEffect(() => { onDirtyChange?.(hasChanges); }, [hasChanges, onDirtyChange]); @@ -69,8 +67,7 @@ export function DeveloperSection({ onDirtyChange }: DeveloperSectionProps) { const safe: DeveloperFields = { developer_mode: (nodeData.developer_mode as '0' | '1') ?? DEFAULT_SETTINGS.developer_mode, }; - setSettings(safe); - serverSettingsRef.current = { ...safe }; + reset(safe); } catch (e) { console.error('Failed to fetch developer settings', e); } finally { @@ -86,8 +83,9 @@ export function DeveloperSection({ onDirtyChange }: DeveloperSectionProps) { }; const saveSettings = async () => { + const submitted = { ...settings }; const payload = { - developer_mode: settings.developer_mode, + developer_mode: submitted.developer_mode, }; setIsSaving(true); try { @@ -100,7 +98,7 @@ export function DeveloperSection({ onDirtyChange }: DeveloperSectionProps) { toast.error(err?.error || err?.message || 'Failed to save settings.'); return; } - serverSettingsRef.current = { ...settings }; + markSaved(submitted); toast.success('Developer settings saved.'); window.dispatchEvent(new CustomEvent(SENCHO_SETTINGS_CHANGED, { detail: { changedKeys: Object.keys(payload) }, diff --git a/frontend/src/components/settings/DockerStorageSection.tsx b/frontend/src/components/settings/DockerStorageSection.tsx index 36937bdf..0a5547b5 100644 --- a/frontend/src/components/settings/DockerStorageSection.tsx +++ b/frontend/src/components/settings/DockerStorageSection.tsx @@ -1,4 +1,4 @@ -import { useState, useRef, useEffect, useMemo } from 'react'; +import { useState, useEffect } from 'react'; import { Skeleton } from '@/components/ui/skeleton'; import { RefreshCw } from 'lucide-react'; import { apiFetch } from '@/lib/api'; @@ -11,6 +11,7 @@ import { SettingsSection } from './SettingsSection'; import { SettingsField } from './SettingsField'; import { SettingsActions, SettingsPrimaryButton } from './SettingsActions'; import { useMastheadStats } from './MastheadStatsContext'; +import { useSettingsDirty } from './useSettingsDirty'; import { TogglePill } from '@/components/ui/toggle-pill'; import { NumberChip } from './SystemControls'; @@ -40,22 +41,10 @@ export function DockerStorageSection({ onDirtyChange }: DockerStorageSectionProp const { activeNode } = useNodes(); const { isAdmin } = useAuth(); const readOnly = !isAdmin; - const [settings, setSettings] = useState({ ...DEFAULT_DOCKER_STORAGE }); - const serverSettingsRef = useRef({ ...DEFAULT_DOCKER_STORAGE }); + const { settings, setSettings, dirtyCount, hasChanges, reset, markSaved } = useSettingsDirty({ ...DEFAULT_DOCKER_STORAGE }); const [isLoading, setIsLoading] = useState(false); const [isSaving, setIsSaving] = useState(false); - const dirtyCount = useMemo(() => { - const baseline = serverSettingsRef.current; - let n = 0; - if (settings.docker_janitor_gb !== baseline.docker_janitor_gb) n++; - if (settings.prune_on_update !== baseline.prune_on_update) n++; - if (settings.reclaim_hero !== baseline.reclaim_hero) n++; - return n; - }, [settings]); - - const hasChanges = dirtyCount > 0; - useEffect(() => { onDirtyChange?.(hasChanges); }, [hasChanges, onDirtyChange]); @@ -83,8 +72,7 @@ export function DockerStorageSection({ onDirtyChange }: DockerStorageSectionProp prune_on_update: (nodeData.prune_on_update as '0' | '1') ?? DEFAULT_SETTINGS.prune_on_update, reclaim_hero: (nodeData.reclaim_hero as '0' | '1') ?? DEFAULT_SETTINGS.reclaim_hero, }; - setSettings(safe); - serverSettingsRef.current = { ...safe }; + reset(safe); } catch (e) { console.error('Failed to fetch Docker & storage settings', e); } finally { @@ -100,18 +88,19 @@ export function DockerStorageSection({ onDirtyChange }: DockerStorageSectionProp }; const saveSettings = async () => { + const submitted = { ...settings }; setIsSaving(true); try { const res = await apiFetch('/settings', { method: 'PATCH', - body: JSON.stringify(settings), + body: JSON.stringify(submitted), }); if (!res.ok) { const err = await res.json().catch(() => ({})); toast.error(err?.error || err?.message || 'Failed to save settings.'); return; } - serverSettingsRef.current = { ...settings }; + markSaved(submitted); toast.success('Docker & storage settings saved.'); } catch (e: unknown) { toast.error((e as Error)?.message || 'Something went wrong.'); diff --git a/frontend/src/components/settings/FleetMeshSection.tsx b/frontend/src/components/settings/FleetMeshSection.tsx index 6beb3c1a..bb6d4739 100644 --- a/frontend/src/components/settings/FleetMeshSection.tsx +++ b/frontend/src/components/settings/FleetMeshSection.tsx @@ -1,4 +1,4 @@ -import { useState, useRef, useEffect, useMemo } from 'react'; +import { useState, useEffect } from 'react'; import { Skeleton } from '@/components/ui/skeleton'; import { RefreshCw } from 'lucide-react'; import { apiFetch } from '@/lib/api'; @@ -11,6 +11,7 @@ import { SettingsSection } from './SettingsSection'; import { SettingsField } from './SettingsField'; import { SettingsActions, SettingsPrimaryButton } from './SettingsActions'; import { useMastheadStats } from './MastheadStatsContext'; +import { useSettingsDirty } from './useSettingsDirty'; import { TogglePill } from '@/components/ui/toggle-pill'; interface FleetMeshSectionProps { @@ -36,21 +37,10 @@ export function FleetMeshSection({ onDirtyChange }: FleetMeshSectionProps) { const { activeNode } = useNodes(); const { isAdmin } = useAuth(); const readOnly = !isAdmin; - const [settings, setSettings] = useState({ ...DEFAULT_FLEET_MESH }); - const serverSettingsRef = useRef({ ...DEFAULT_FLEET_MESH }); + const { settings, setSettings, dirtyCount, hasChanges, reset, markSaved } = useSettingsDirty({ ...DEFAULT_FLEET_MESH }); const [isLoading, setIsLoading] = useState(false); const [isSaving, setIsSaving] = useState(false); - const dirtyCount = useMemo(() => { - const baseline = serverSettingsRef.current; - let n = 0; - if (settings.mesh_auto_recreate !== baseline.mesh_auto_recreate) n++; - if (settings.snapshot_documentation !== baseline.snapshot_documentation) n++; - return n; - }, [settings]); - - const hasChanges = dirtyCount > 0; - useEffect(() => { onDirtyChange?.(hasChanges); }, [hasChanges, onDirtyChange]); @@ -77,8 +67,7 @@ export function FleetMeshSection({ onDirtyChange }: FleetMeshSectionProps) { mesh_auto_recreate: (nodeData.mesh_auto_recreate as '0' | '1') ?? DEFAULT_SETTINGS.mesh_auto_recreate, snapshot_documentation: (nodeData.snapshot_documentation as '0' | '1') ?? DEFAULT_SETTINGS.snapshot_documentation, }; - setSettings(safe); - serverSettingsRef.current = { ...safe }; + reset(safe); } catch (e) { console.error('Failed to fetch fleet mesh settings', e); } finally { @@ -94,18 +83,19 @@ export function FleetMeshSection({ onDirtyChange }: FleetMeshSectionProps) { }; const saveSettings = async () => { + const submitted = { ...settings }; setIsSaving(true); try { const res = await apiFetch('/settings', { method: 'PATCH', - body: JSON.stringify(settings), + body: JSON.stringify(submitted), }); if (!res.ok) { const err = await res.json().catch(() => ({})); toast.error(err?.error || err?.message || 'Failed to save settings.'); return; } - serverSettingsRef.current = { ...settings }; + markSaved(submitted); toast.success('Fleet settings saved.'); } catch (e: unknown) { toast.error((e as Error)?.message || 'Something went wrong.'); diff --git a/frontend/src/components/settings/HostAlertsSection.tsx b/frontend/src/components/settings/HostAlertsSection.tsx index ce8dde23..ec9e8fc9 100644 --- a/frontend/src/components/settings/HostAlertsSection.tsx +++ b/frontend/src/components/settings/HostAlertsSection.tsx @@ -1,4 +1,4 @@ -import { useState, useRef, useEffect, useMemo } from 'react'; +import { useState, useEffect } from 'react'; import { Skeleton } from '@/components/ui/skeleton'; import { RefreshCw } from 'lucide-react'; import { apiFetch } from '@/lib/api'; @@ -11,6 +11,7 @@ import { SettingsSection } from './SettingsSection'; import { SettingsField } from './SettingsField'; import { SettingsActions, SettingsPrimaryButton } from './SettingsActions'; import { useMastheadStats } from './MastheadStatsContext'; +import { useSettingsDirty } from './useSettingsDirty'; import { TogglePill } from '@/components/ui/toggle-pill'; import { NumberChip } from './SystemControls'; @@ -45,26 +46,10 @@ export function HostAlertsSection({ onDirtyChange }: HostAlertsSectionProps) { const { activeNode } = useNodes(); const { isAdmin } = useAuth(); const readOnly = !isAdmin; - const [settings, setSettings] = useState({ ...DEFAULT_HOST_ALERTS }); - const serverSettingsRef = useRef({ ...DEFAULT_HOST_ALERTS }); + const { settings, setSettings, dirtyCount, hasChanges, reset, markSaved } = useSettingsDirty({ ...DEFAULT_HOST_ALERTS }); const [isLoading, setIsLoading] = useState(false); const [isSaving, setIsSaving] = useState(false); - const dirtyCount = useMemo(() => { - const baseline = serverSettingsRef.current; - let n = 0; - if (settings.host_cpu_limit !== baseline.host_cpu_limit) n++; - if (settings.host_ram_limit !== baseline.host_ram_limit) n++; - if (settings.host_disk_limit !== baseline.host_disk_limit) n++; - if (settings.host_alert_suppression_mins !== baseline.host_alert_suppression_mins) n++; - if (settings.global_crash !== baseline.global_crash) n++; - if (settings.health_gate_enabled !== baseline.health_gate_enabled) n++; - if (settings.health_gate_window_seconds !== baseline.health_gate_window_seconds) n++; - return n; - }, [settings]); - - const hasChanges = dirtyCount > 0; - useEffect(() => { onDirtyChange?.(hasChanges); }, [hasChanges, onDirtyChange]); @@ -96,8 +81,7 @@ export function HostAlertsSection({ onDirtyChange }: HostAlertsSectionProps) { health_gate_enabled: (nodeData.health_gate_enabled as '0' | '1') ?? DEFAULT_SETTINGS.health_gate_enabled, health_gate_window_seconds: nodeData.health_gate_window_seconds ?? DEFAULT_SETTINGS.health_gate_window_seconds, }; - setSettings(safe); - serverSettingsRef.current = { ...safe }; + reset(safe); } catch (e) { console.error('Failed to fetch host alert settings', e); } finally { @@ -113,18 +97,19 @@ export function HostAlertsSection({ onDirtyChange }: HostAlertsSectionProps) { }; const saveSettings = async () => { + const submitted = { ...settings }; setIsSaving(true); try { const res = await apiFetch('/settings', { method: 'PATCH', - body: JSON.stringify(settings), + body: JSON.stringify(submitted), }); if (!res.ok) { const err = await res.json().catch(() => ({})); toast.error(err?.error || err?.message || 'Failed to save settings.'); return; } - serverSettingsRef.current = { ...settings }; + markSaved(submitted); toast.success('Host alerts saved.'); } catch (e: unknown) { toast.error((e as Error)?.message || 'Something went wrong.'); diff --git a/frontend/src/components/settings/__tests__/SettingsDirtyReconcile.test.tsx b/frontend/src/components/settings/__tests__/SettingsDirtyReconcile.test.tsx new file mode 100644 index 00000000..b6211b81 --- /dev/null +++ b/frontend/src/components/settings/__tests__/SettingsDirtyReconcile.test.tsx @@ -0,0 +1,235 @@ +/** + * After a successful save the section must reconcile its dirty state immediately, + * with no remount: the parent dirty flag (onDirtyChange) clears, the masthead + * EDITED stat flips from pending to saved, and the Save button disables. A failed + * save must keep the section dirty and retryable, and an edit made while a save is + * in flight must survive (the submitted snapshot, not the live state, becomes the + * new baseline). + * + * The onDirtyChange boolean asserted here is exactly the value SettingsPage stores + * in its dirtyFlags map and the sidebar renders the unsaved dot from, so a clean + * transition here proves the parent indicators clear. + */ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { render, screen, fireEvent, waitFor, act, cleanup } from '@testing-library/react'; +import type { MastheadMetadataItem } from '@/components/ui/PageMasthead'; + +const { masthead } = vi.hoisted(() => ({ + masthead: { last: null as MastheadMetadataItem[] | null }, +})); + +vi.mock('@/lib/api', () => ({ apiFetch: vi.fn() })); +vi.mock('@/components/ui/toast-store', () => ({ + toast: { error: vi.fn(), success: vi.fn(), warning: vi.fn(), info: vi.fn(), loading: vi.fn(), dismiss: vi.fn() }, +})); +vi.mock('@/context/AuthContext', () => ({ useAuth: () => ({ isAdmin: true }) })); +vi.mock('@/context/NodeContext', () => ({ useNodes: () => ({ activeNode: { id: 'local' } }) })); +vi.mock('@/context/LicenseContext', () => ({ useLicense: vi.fn(() => ({ isPaid: true })) })); +vi.mock('../MastheadStatsContext', () => ({ + useMastheadStats: (stats: MastheadMetadataItem[] | null) => { masthead.last = stats; }, +})); + +import { apiFetch } from '@/lib/api'; +import { useLicense } from '@/context/LicenseContext'; +import { HostAlertsSection } from '../HostAlertsSection'; +import { DockerStorageSection } from '../DockerStorageSection'; +import { FleetMeshSection } from '../FleetMeshSection'; +import { DataRetentionSection } from '../DataRetentionSection'; +import { DeveloperSection } from '../DeveloperSection'; + +const mockedFetch = apiFetch as unknown as ReturnType; +const mockedLicense = useLicense as unknown as ReturnType; + +const FULL_SETTINGS: Record = { + host_cpu_limit: '90', + host_ram_limit: '90', + host_disk_limit: '90', + host_alert_suppression_mins: '60', + global_crash: '1', + docker_janitor_gb: '5', + prune_on_update: '1', + reclaim_hero: '1', + mesh_auto_recreate: '0', + metrics_retention_hours: '24', + log_retention_days: '30', + audit_retention_days: '90', + scan_history_per_image_limit: '50', + developer_mode: '0', +}; + +/** GET /settings resolves load data; PATCH resolves ok unless overridden per-test. */ +function wireDefaultFetch() { + mockedFetch.mockImplementation((_url: string, opts?: { method?: string }) => { + if (opts?.method === 'PATCH') return Promise.resolve({ ok: true, json: async () => ({}) }); + return Promise.resolve({ ok: true, json: async () => ({ ...FULL_SETTINGS }) }); + }); +} + +function lastDirty(spy: ReturnType): boolean | undefined { + const calls = spy.mock.calls; + return calls.length ? (calls[calls.length - 1][0] as boolean) : undefined; +} + +beforeEach(() => { + mockedFetch.mockReset(); + mockedLicense.mockReturnValue({ isPaid: true }); + masthead.last = null; + wireDefaultFetch(); +}); + +describe('settings dirty reconcile on save', () => { + it('clears the masthead, parent dirty flag, and Save button after a successful save (no remount)', async () => { + const onDirty = vi.fn(); + render(); + const save = await screen.findByRole('button', { name: /save alerts/i }); + + // Edit: section becomes dirty. + fireEvent.click(screen.getAllByRole('switch')[0]); // global_crash + await waitFor(() => expect(lastDirty(onDirty)).toBe(true)); + expect(masthead.last?.[0]).toMatchObject({ label: 'EDITED', value: '1 pending', tone: 'warn' }); + expect(save).not.toBeDisabled(); + + // Save succeeds: everything reconciles without a remount. + fireEvent.click(save); + await waitFor(() => expect(lastDirty(onDirty)).toBe(false)); + expect(masthead.last?.[0]).toMatchObject({ label: 'EDITED', value: 'saved', tone: 'value' }); + expect(save).toBeDisabled(); + // A remount would re-run the load effect; the section loads exactly once. + const getCalls = mockedFetch.mock.calls.filter(c => c[1]?.method !== 'PATCH'); + expect(getCalls).toHaveLength(1); + }); + + it('keeps the section dirty and retryable when the save fails', async () => { + mockedFetch.mockImplementation((_url: string, opts?: { method?: string }) => { + if (opts?.method === 'PATCH') return Promise.resolve({ ok: false, json: async () => ({ error: 'nope' }) }); + return Promise.resolve({ ok: true, json: async () => ({ ...FULL_SETTINGS }) }); + }); + const onDirty = vi.fn(); + render(); + const save = await screen.findByRole('button', { name: /save alerts/i }); + + fireEvent.click(screen.getAllByRole('switch')[0]); + await waitFor(() => expect(lastDirty(onDirty)).toBe(true)); + + fireEvent.click(save); + await waitFor(() => expect(mockedFetch.mock.calls.some(c => c[1]?.method === 'PATCH')).toBe(true)); + // Still dirty, still enabled, masthead still pending: the operator can retry. + // Wait for the save to settle so the assertion does not race the isSaving reset. + await waitFor(() => expect(save).not.toBeDisabled()); + expect(lastDirty(onDirty)).toBe(true); + expect(masthead.last?.[0]).toMatchObject({ label: 'EDITED', value: '1 pending', tone: 'warn' }); + }); + + it('preserves an edit made while the PATCH is in flight', async () => { + let resolvePatch: ((v: { ok: boolean; json: () => Promise }) => void) | undefined; + mockedFetch.mockImplementation((_url: string, opts?: { method?: string }) => { + if (opts?.method === 'PATCH') { + return new Promise<{ ok: boolean; json: () => Promise }>(res => { resolvePatch = res; }); + } + return Promise.resolve({ ok: true, json: async () => ({ ...FULL_SETTINGS }) }); + }); + const onDirty = vi.fn(); + render(); + const save = await screen.findByRole('button', { name: /save alerts/i }); + + // Change field A and submit (PATCH now pending). + fireEvent.click(screen.getAllByRole('switch')[0]); // global_crash + await waitFor(() => expect(lastDirty(onDirty)).toBe(true)); + fireEvent.click(save); + await waitFor(() => expect(mockedFetch.mock.calls.some(c => c[1]?.method === 'PATCH')).toBe(true)); + + // Change field B while the save is still in flight (fieldset stays editable). + const healthGate = screen.getAllByRole('switch')[1]; // health_gate_enabled + const healthBefore = healthGate.getAttribute('aria-checked'); + fireEvent.click(healthGate); + expect(healthGate.getAttribute('aria-checked')).not.toBe(healthBefore); + + // Resolve the save: only the submitted snapshot becomes the baseline. + await act(async () => { + resolvePatch?.({ ok: true, json: async () => ({}) }); + }); + + // The later edit survives and keeps the section dirty/retryable. + expect(healthGate.getAttribute('aria-checked')).not.toBe(healthBefore); + await waitFor(() => expect(lastDirty(onDirty)).toBe(true)); + expect(save).not.toBeDisabled(); + }); +}); + +describe('every migrated section clears its dirty flag on save', () => { + interface Case { + name: string; + render: (onDirty: (d: boolean) => void) => void; + saveName: RegExp; + edit: () => void; + } + + const cases: Case[] = [ + { + name: 'HostAlertsSection', + render: onDirty => render(), + saveName: /save alerts/i, + edit: () => fireEvent.click(screen.getAllByRole('switch')[0]), + }, + { + name: 'DockerStorageSection', + render: onDirty => render(), + saveName: /save settings/i, + edit: () => fireEvent.click(screen.getAllByRole('switch')[0]), + }, + { + name: 'FleetMeshSection', + render: onDirty => render(), + saveName: /save settings/i, + edit: () => fireEvent.click(screen.getAllByRole('switch')[0]), + }, + { + name: 'DataRetentionSection', + render: onDirty => render(), + saveName: /save settings/i, + edit: () => fireEvent.change(screen.getAllByRole('spinbutton')[0], { target: { value: '48' } }), + }, + { + name: 'DeveloperSection', + render: onDirty => render(), + saveName: /save settings/i, + edit: () => fireEvent.click(screen.getByRole('switch')), + }, + ]; + + for (const c of cases) { + it(`${c.name} reports clean after a successful save`, async () => { + const onDirty = vi.fn(); + c.render(onDirty); + const save = await screen.findByRole('button', { name: c.saveName }); + c.edit(); + await waitFor(() => expect(lastDirty(onDirty)).toBe(true)); + fireEvent.click(save); + await waitFor(() => expect(lastDirty(onDirty)).toBe(false)); + cleanup(); + }); + } + + it('DataRetentionSection reconciles for a Community operator even though the paid key is omitted from the payload', async () => { + // markSaved adopts the full submitted snapshot (incl. audit_retention_days), + // not the trimmed Community payload, so the section re-baselines and clears. + mockedLicense.mockReturnValue({ isPaid: false }); + const onDirty = vi.fn(); + render(); + const save = await screen.findByRole('button', { name: /save settings/i }); + fireEvent.change(screen.getAllByRole('spinbutton')[0], { target: { value: '48' } }); + await waitFor(() => expect(lastDirty(onDirty)).toBe(true)); + fireEvent.click(save); + await waitFor(() => expect(lastDirty(onDirty)).toBe(false)); + expect(save).toBeDisabled(); + }); + + it('DeveloperSection masthead stays a DEV MODE stat, not an EDITED pending count', async () => { + const onDirty = vi.fn(); + render(); + await screen.findByRole('button', { name: /save settings/i }); + fireEvent.click(screen.getByRole('switch')); // developer_mode 0 -> 1 + await waitFor(() => expect(lastDirty(onDirty)).toBe(true)); + expect(masthead.last?.[0]).toMatchObject({ label: 'DEV MODE', value: 'on' }); + }); +}); diff --git a/frontend/src/components/settings/__tests__/useSettingsDirty.test.ts b/frontend/src/components/settings/__tests__/useSettingsDirty.test.ts new file mode 100644 index 00000000..feca4b22 --- /dev/null +++ b/frontend/src/components/settings/__tests__/useSettingsDirty.test.ts @@ -0,0 +1,62 @@ +/** + * Covers dirty counting, the load (reset) vs save-success (markSaved) split, and + * the in-flight case: markSaved moves only the baseline, so an edit made while a + * PATCH is in flight is preserved. + */ +import { describe, it, expect } from 'vitest'; +import { act, renderHook } from '@testing-library/react'; +import { useSettingsDirty } from '../useSettingsDirty'; + +type Fields = Record<'a' | 'b', string>; + +const INITIAL: Fields = { a: '1', b: '1' }; + +describe('useSettingsDirty', () => { + it('starts clean', () => { + const { result } = renderHook(() => useSettingsDirty(INITIAL)); + expect(result.current.dirtyCount).toBe(0); + expect(result.current.hasChanges).toBe(false); + }); + + it('counts changed fields against the baseline', () => { + const { result } = renderHook(() => useSettingsDirty(INITIAL)); + act(() => result.current.setSettings(prev => ({ ...prev, a: '2' }))); + expect(result.current.dirtyCount).toBe(1); + expect(result.current.hasChanges).toBe(true); + act(() => result.current.setSettings(prev => ({ ...prev, b: '2' }))); + expect(result.current.dirtyCount).toBe(2); + }); + + it('reset adopts both current values and baseline, and edits count against the new baseline', () => { + const { result } = renderHook(() => useSettingsDirty(INITIAL)); + act(() => result.current.reset({ a: '9', b: '9' })); + expect(result.current.settings).toEqual({ a: '9', b: '9' }); + expect(result.current.hasChanges).toBe(false); + // An edit after reset is measured against the freshly adopted baseline. + act(() => result.current.setSettings(prev => ({ ...prev, a: '8' }))); + expect(result.current.dirtyCount).toBe(1); + }); + + it('markSaved clears dirty without touching current settings', () => { + const { result } = renderHook(() => useSettingsDirty(INITIAL)); + act(() => result.current.setSettings(prev => ({ ...prev, a: '2' }))); + const submitted = result.current.settings; + act(() => result.current.markSaved(submitted)); + expect(result.current.settings).toEqual({ a: '2', b: '1' }); + expect(result.current.hasChanges).toBe(false); + }); + + it('keeps an edit made after the submitted snapshot dirty (in-flight save)', () => { + const { result } = renderHook(() => useSettingsDirty(INITIAL)); + // User changes a, "submits", then changes b before the save resolves. + act(() => result.current.setSettings(prev => ({ ...prev, a: '2' }))); + const submitted = result.current.settings; // { a: '2', b: '1' } + act(() => result.current.setSettings(prev => ({ ...prev, b: '2' }))); + // Save resolves: baseline adopts the submitted snapshot only. + act(() => result.current.markSaved(submitted)); + // b was changed after submitting, so the section stays dirty on b. + expect(result.current.settings).toEqual({ a: '2', b: '2' }); + expect(result.current.dirtyCount).toBe(1); + expect(result.current.hasChanges).toBe(true); + }); +}); diff --git a/frontend/src/components/settings/useSettingsDirty.ts b/frontend/src/components/settings/useSettingsDirty.ts new file mode 100644 index 00000000..211828ed --- /dev/null +++ b/frontend/src/components/settings/useSettingsDirty.ts @@ -0,0 +1,53 @@ +import { useCallback, useMemo, useState } from 'react'; +import type { Dispatch, SetStateAction } from 'react'; + +/** Settings field values are flat primitives, compared shallowly with `!==`. */ +type SettingsValue = string | number | boolean | null | undefined; +type SettingsFields = Record; + +/** + * Tracks dirty state for a flat settings-fields object against a saved baseline. + * + * The baseline lives in state (not a ref) so adopting a new baseline re-renders + * and re-runs the dirty calculation. Load and save-success are deliberately + * separate operations: a section's fields stay editable while a save is in + * flight, so the save path must move only the baseline and leave the current + * values alone, or an edit made between clicking Save and the PATCH resolving + * would be discarded. + */ +export interface SettingsDirty { + settings: T; + setSettings: Dispatch>; + dirtyCount: number; + hasChanges: boolean; + /** Load / node-switch: adopt `next` as both the current values and the saved baseline. */ + reset: (next: T) => void; + /** Save success: adopt the submitted snapshot as the saved baseline only, + * preserving edits made while the PATCH was in flight. Pass a snapshot + * captured before the request, not live settings read after the await. */ + markSaved: (submitted: T) => void; +} + +export function useSettingsDirty(initial: T): SettingsDirty { + const [settings, setSettings] = useState(initial); + const [baseline, setBaseline] = useState(initial); + + const dirtyCount = useMemo(() => { + let n = 0; + for (const key of Object.keys(settings) as (keyof T)[]) { + if (settings[key] !== baseline[key]) n += 1; + } + return n; + }, [settings, baseline]); + + const reset = useCallback((next: T) => { + setSettings({ ...next }); + setBaseline({ ...next }); + }, []); + + const markSaved = useCallback((submitted: T) => { + setBaseline({ ...submitted }); + }, []); + + return { settings, setSettings, dirtyCount, hasChanges: dirtyCount > 0, reset, markSaved }; +}