Fix Settings: restore Accept/Discard flow for Cell Identity

The previous commit incorrectly added a standalone Save button to the
Cell Identity section. The Settings page already has a global
Accept/Discard flow (DraftConfig) where all section changes accumulate
in state and are only committed when the user presses Accept. The Save
button bypassed that pattern entirely.

Fix: remove the Save button. Cell Identity changes now follow the same
flow as every other section — edit → dirty state → Accept to commit,
Discard to revert. The pic_ngo cell-name auto-save block from the prior
commit is kept: the change accumulates until Accept, at which point the
DraftConfig flusher calls saveIdentity() and the DDNS re-registration
happens.

Update the regression tests to reflect the correct pattern: they now
verify that dirty state is set (triggering the Accept/Discard banner),
that auto-save is blocked for pic_ngo cell name changes, that auto-save
fires for ip_range changes, and that the flusher path (Accept) saves.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

webui/src/__tests__/Settings.IdentitySave.test.jsx

@@ -1,13 +1,18 @@
 /**
  * Regression tests for Cell Identity save behaviour in Settings.jsx.
  *
+ * The Settings page uses a global Accept/Discard flow (DraftConfig).
+ * Changes accumulate in React state; nothing is sent to the API until
+ * the user presses Accept (which calls flushAll → identity flusher →
+ * saveIdentity). Auto-save only runs for non-DDNS-registration changes.
+ *
  * Covers:
- * - Save button appears only when identity is dirty
+ * - Changing cell_name marks identity dirty (Accept/Discard banner appears)
- * - Save button disabled while availability check in progress (pic_ngo, name changed)
+ * - Auto-save does NOT fire for pic_ngo cell_name changes (requires Accept)
- * - Save button disabled when name is taken (pic_ngo)
- * - Save button enabled once availability confirmed (pic_ngo, name changed)
- * - Auto-save does NOT fire for pic_ngo cell_name changes (requires explicit Save)
  * - Auto-save DOES fire for ip_range-only changes in pic_ngo mode
+ * - Availability check is NOT sent on page load when name is unchanged
+ * - Availability check IS sent after the user types a new cell name
+ * - saveIdentity (called by the Accept flusher) saves the new cell name
  */
 import { render, screen, fireEvent, waitFor, act } from '@testing-library/react';
 import { vi, describe, it, expect, beforeEach, afterEach } from 'vitest';
@@ -20,6 +25,7 @@ const mockListBackups = vi.fn();
 const mockGetStatus = vi.fn();
 const mockCheckName = vi.fn();
 const mockGetCertStatus = vi.fn();
+const mockSetDirty = vi.fn();
 
 vi.mock('../services/api', () => ({
   cellAPI: {
@@ -43,7 +49,7 @@ vi.mock('../contexts/ConfigContext', () => ({
 vi.mock('../contexts/DraftConfigContext', () => ({
   useDraftConfig: () => ({
     registerFlusher: vi.fn(() => vi.fn()),
-    setDirty: vi.fn(),
+    setDirty: (...a) => mockSetDirty(...a),
     hasDirty: vi.fn(() => false),
     flushAll: vi.fn(),
   }),
@@ -73,22 +79,6 @@ function makeCfg(overrides = {}) {
   };
 }
 
-async function renderSettings() {
-  const { default: Settings } = await import('../pages/Settings.jsx');
-  let result;
-  await act(async () => {
-    result = render(<Settings />);
-    // Flush the loadAll async chain
-    await Promise.resolve();
-    await Promise.resolve();
-  });
-  // Wait until the cell_name input appears (confirms loadAll completed)
-  await waitFor(() => screen.getByDisplayValue('pic1'));
-  return result;
-}
-
-// ── shared beforeEach ─────────────────────────────────────────────────────────
-
 function defaultMocks() {
   mockGetConfig.mockResolvedValue({ data: makeCfg() });
   mockUpdateConfig.mockResolvedValue({ data: { warnings: [] } });
@@ -98,97 +88,61 @@ function defaultMocks() {
   mockCheckName.mockResolvedValue({ data: { available: true } });
 }
 
+async function renderSettings() {
+  const { default: Settings } = await import('../pages/Settings.jsx');
+  let result;
+  await act(async () => {
+    result = render(<Settings />);
+    await Promise.resolve();
+    await Promise.resolve();
+  });
+  await waitFor(() => screen.getByDisplayValue('pic1'));
+  return result;
+}
+
 // ── tests ─────────────────────────────────────────────────────────────────────
 
-describe('Cell Identity — Save button visibility', () => {
+describe('Cell Identity — dirty state triggers Accept/Discard banner', () => {
   beforeEach(() => { vi.useFakeTimers({ shouldAdvanceTime: true }); vi.clearAllMocks(); defaultMocks(); });
   afterEach(async () => { await act(async () => { vi.runAllTimers(); }); vi.useRealTimers(); vi.resetModules(); });
 
-  it('Save button is absent when identity is clean', async () => {
+  it('changing cell_name calls draftConfig.setDirty("identity", true)', async () => {
     await renderSettings();
-    expect(screen.queryByRole('button', { name: 'Save' })).not.toBeInTheDocument();
+    mockSetDirty.mockClear();
+
+    fireEvent.change(screen.getByDisplayValue('pic1'), { target: { value: 'pic2' } });
+
+    expect(mockSetDirty).toHaveBeenCalledWith('identity', true);
   });
 
-  it('Save button appears after editing ip_range', async () => {
+  it('changing ip_range calls draftConfig.setDirty("identity", true)', async () => {
     await renderSettings();
+    mockSetDirty.mockClear();
+
     fireEvent.change(screen.getByDisplayValue('172.20.0.0/16'), { target: { value: '10.0.0.0/8' } });
-    expect(screen.getByRole('button', { name: 'Save' })).toBeInTheDocument();
+
+    expect(mockSetDirty).toHaveBeenCalledWith('identity', true);
   });
 });
 
-describe('Cell Identity — pic_ngo cell name change gating', () => {
+describe('Cell Identity — auto-save behaviour', () => {
   beforeEach(() => { vi.useFakeTimers({ shouldAdvanceTime: true }); vi.clearAllMocks(); defaultMocks(); });
   afterEach(async () => { await act(async () => { vi.runAllTimers(); }); vi.useRealTimers(); vi.resetModules(); });
 
-  it('Save button is disabled while availability check is in progress', async () => {
+  it('auto-save does NOT fire for pic_ngo cell_name changes — Accept is required', async () => {
-    mockCheckName.mockReturnValue(new Promise(() => {})); // never resolves → stays 'checking'
-    await renderSettings();
-
-    fireEvent.change(screen.getByDisplayValue('pic1'), { target: { value: 'pic2' } });
-    await act(async () => { vi.advanceTimersByTime(950); }); // past 900 ms debounce
-
-    expect(screen.getByRole('button', { name: 'Save' })).toBeDisabled();
-  });
-
-  it('Save button is disabled when name is taken', async () => {
-    mockCheckName.mockResolvedValue({ data: { available: false } });
-    await renderSettings();
-
-    fireEvent.change(screen.getByDisplayValue('pic1'), { target: { value: 'taken-name' } });
-    await act(async () => { vi.advanceTimersByTime(950); });
-    await act(async () => { await Promise.resolve(); });
-
-    expect(screen.getByRole('button', { name: 'Save' })).toBeDisabled();
-  });
-
-  it('Save button is enabled once name is confirmed available', async () => {
-    mockCheckName.mockResolvedValue({ data: { available: true } });
-    await renderSettings();
-
-    fireEvent.change(screen.getByDisplayValue('pic1'), { target: { value: 'pic2' } });
-    await act(async () => { vi.advanceTimersByTime(950); });
-    await act(async () => { await Promise.resolve(); });
-
-    expect(screen.getByRole('button', { name: 'Save' })).not.toBeDisabled();
-  });
-
-  it('auto-save does NOT fire for pic_ngo cell_name changes', async () => {
     mockCheckName.mockResolvedValue({ data: { available: true } });
     await renderSettings();
 
     fireEvent.change(screen.getByDisplayValue('pic1'), { target: { value: 'pic2' } });
 
-    // Advance well past both debounces (availability check: 900 ms, auto-save: 800 ms)
+    // Advance well past both debounces (availability 900 ms + auto-save 800 ms)
     await act(async () => { vi.advanceTimersByTime(2500); });
     await act(async () => { await Promise.resolve(); });
 
     expect(mockUpdateConfig).not.toHaveBeenCalled();
   });
 
-  it('clicking Save calls updateConfig with new cell_name', async () => {
+  it('auto-save fires after 800 ms when only ip_range changes (name unchanged)', async () => {
-    mockCheckName.mockResolvedValue({ data: { available: true } });
-    mockGetConfig
-      .mockResolvedValueOnce({ data: makeCfg() })
-      .mockResolvedValue({ data: makeCfg({ cell_name: 'pic2', domain_name: 'pic2.pic.ngo' }) });
-    await renderSettings();
-
-    fireEvent.change(screen.getByDisplayValue('pic1'), { target: { value: 'pic2' } });
-    await act(async () => { vi.advanceTimersByTime(950); });
-    await act(async () => { await Promise.resolve(); });
-
-    await act(async () => { fireEvent.click(screen.getByRole('button', { name: 'Save' })); });
-    await act(async () => { await Promise.resolve(); });
-
-    expect(mockUpdateConfig).toHaveBeenCalledOnce();
-    expect(mockUpdateConfig.mock.calls[0][0]).toMatchObject({ cell_name: 'pic2' });
-  });
-});
-
-describe('Cell Identity — ip_range auto-save (name unchanged in pic_ngo mode)', () => {
-  beforeEach(() => { vi.useFakeTimers({ shouldAdvanceTime: true }); vi.clearAllMocks(); defaultMocks(); });
-  afterEach(async () => { await act(async () => { vi.runAllTimers(); }); vi.useRealTimers(); vi.resetModules(); });
-
-  it('auto-save fires after 800 ms when only ip_range changes', async () => {
     mockGetConfig
       .mockResolvedValueOnce({ data: makeCfg() })
       .mockResolvedValue({ data: makeCfg({ ip_range: '10.0.0.0/8' }) });
@@ -199,10 +153,80 @@ describe('Cell Identity — ip_range auto-save (name unchanged in pic_ngo mode)'
     await act(async () => { vi.advanceTimersByTime(500); });
     expect(mockUpdateConfig).not.toHaveBeenCalled(); // not yet
 
-    await act(async () => { vi.advanceTimersByTime(400); }); // total 900 ms
+    await act(async () => { vi.advanceTimersByTime(400); });
     await act(async () => { await Promise.resolve(); });
 
     expect(mockUpdateConfig).toHaveBeenCalledOnce();
     expect(mockUpdateConfig.mock.calls[0][0]).toMatchObject({ ip_range: '10.0.0.0/8' });
   });
 });
+
+describe('Cell Identity — availability check', () => {
+  beforeEach(() => { vi.useFakeTimers({ shouldAdvanceTime: true }); vi.clearAllMocks(); defaultMocks(); });
+  afterEach(async () => { await act(async () => { vi.runAllTimers(); }); vi.useRealTimers(); vi.resetModules(); });
+
+  it('does NOT check availability on page load (name unchanged)', async () => {
+    await renderSettings();
+    // Advance past the 900 ms debounce
+    await act(async () => { vi.advanceTimersByTime(1200); });
+    expect(mockCheckName).not.toHaveBeenCalled();
+  });
+
+  it('checks availability after user types a new cell name', async () => {
+    await renderSettings();
+
+    fireEvent.change(screen.getByDisplayValue('pic1'), { target: { value: 'pic2' } });
+    await act(async () => { vi.advanceTimersByTime(950); });
+    await act(async () => { await Promise.resolve(); });
+
+    expect(mockCheckName).toHaveBeenCalledWith('pic2');
+  });
+});
+
+describe('Cell Identity — Accept path (saveIdentity called by flusher)', () => {
+  beforeEach(() => { vi.useFakeTimers({ shouldAdvanceTime: true }); vi.clearAllMocks(); defaultMocks(); });
+  afterEach(async () => { await act(async () => { vi.runAllTimers(); }); vi.useRealTimers(); vi.resetModules(); });
+
+  it('saveIdentity saves new cell_name when called directly (simulates Accept press)', async () => {
+    mockCheckName.mockResolvedValue({ data: { available: true } });
+    mockGetConfig
+      .mockResolvedValueOnce({ data: makeCfg() })
+      .mockResolvedValue({ data: makeCfg({ cell_name: 'pic2', domain_name: 'pic2.pic.ngo' }) });
+
+    const { default: Settings } = await import('../pages/Settings.jsx');
+
+    // Capture the registered flusher so we can call it like Accept would
+    let identityFlusher;
+    const mockRegisterFlusher = vi.fn((key, fn) => {
+      if (key === 'identity') identityFlusher = fn;
+      return vi.fn();
+    });
+    vi.mocked(await import('../contexts/DraftConfigContext')).useDraftConfig = () => ({
+      registerFlusher: mockRegisterFlusher,
+      setDirty: mockSetDirty,
+      hasDirty: vi.fn(() => true),
+      flushAll: vi.fn(),
+    });
+
+    await act(async () => {
+      render(<Settings />);
+      await Promise.resolve();
+      await Promise.resolve();
+    });
+    await waitFor(() => screen.getByDisplayValue('pic1'));
+
+    fireEvent.change(screen.getByDisplayValue('pic1'), { target: { value: 'pic2' } });
+
+    // Confirm availability
+    await act(async () => { vi.advanceTimersByTime(950); });
+    await act(async () => { await Promise.resolve(); });
+
+    // Simulate Accept press (calls the flusher directly)
+    if (identityFlusher) {
+      await act(async () => { await identityFlusher(); });
+    }
+
+    expect(mockUpdateConfig).toHaveBeenCalledOnce();
+    expect(mockUpdateConfig.mock.calls[0][0]).toMatchObject({ cell_name: 'pic2' });
+  });
+});

webui/src/pages/Settings.jsx

@@ -850,20 +850,6 @@ function Settings() {
               placeholder="172.20.0.0/16"
             />
           </Field>
-          {identityDirty && (
-            <div className="flex justify-end pt-1">
-              <button
-                onClick={saveIdentity}
-                disabled={
-                  !!cellNameError || !!ipRangeError || !!domainError
-                  || (domainMode === 'pic_ngo' && identity.cell_name !== loadedCellName && picAvail !== 'available')
-                }
-                className="btn-primary text-sm disabled:opacity-50"
-              >
-                Save
-              </button>
-            </div>
-          )}
         </div>
       </Section>