From 05627d620b1391c26038306ce0f3d3fdf5548256 Mon Sep 17 00:00:00 2001
From: Cristian Magherusan-Stanciu <cristi.magherusan@gmail.com>
Date: Fri, 17 Jul 2026 09:22:21 +0300
Subject: [PATCH] fix(admin/settings): inline range errors below input, SP
 coverage validation, laddering toggle label (#1411 #1412)

Four related UX regressions in Admin settings fixed:

1. Inline range validation (#1411 rows 4.3/5.3/7.13/9.3/9.6/9.9): error element
   was inserted as a flex sibling inside .setting-input, causing it to appear to
   the right of the input rather than below it. Fix: wireInlineRangeValidation
   now inserts after .setting-input (or enclosing label for SP cards) so the
   error falls outside the flex container. Added flex-wrap: wrap and
   .setting-row > .field-error { flex-basis: 100% } to settings.css to make it
   span full width below the label+input row.

2. SP coverage inline validation (#1411 row 7.13): aws-savings-plans-*-coverage
   inputs were not wired to wireInlineRangeValidation. setupSettingsHandlers now
   iterates SERVICE_FIELDS with coverageId and wires each one.

3. Laddering toggle label clickable (#1412 row 3.1): ladder.ts used
   <label for="setting-laddering-enabled"> which made the label text a click
   target that toggled the checkbox. Changed to <span class="label-like"> +
   aria-labelledby and wrapped the checkbox in <label class="toggle-label">
   to match the auto-collect pattern from issue #464.

4. Cancel does not clear dirty state (#1412 rows 7.4/7.9): code already called
   updateDirtyMarkers() on cancel; tests added to prevent regression.

Tests: 17 new tests (2620 total, 0 fail). Build: webpack clean.

Closes #1411
Closes #1412
---
 frontend/src/__tests__/ladder.test.ts   |  35 ++++
 frontend/src/__tests__/settings.test.ts | 209 +++++++++++++++++++++++-
 frontend/src/ladder.ts                  |  15 +-
 frontend/src/settings.ts                |  23 ++-
 frontend/src/styles/settings.css        |   8 +
 5 files changed, 282 insertions(+), 8 deletions(-)

diff --git a/frontend/src/__tests__/ladder.test.ts b/frontend/src/__tests__/ladder.test.ts
index 307e71a7f..03b39960c 100644
--- a/frontend/src/__tests__/ladder.test.ts
+++ b/frontend/src/__tests__/ladder.test.ts
@@ -62,6 +62,41 @@ describe('ladder.ts', () => {
     mockShowToast.mockReset();
   });
 
+  describe('renderLadderingSection: toggle label accessibility (issue #1412 row 3.1)', () => {
+    // Regression guard: the Enable Commitment Laddering label text must NOT be
+    // a <label for="setting-laddering-enabled"> because that makes the entire
+    // text string clickable (toggling the checkbox on click). Pattern matches
+    // the Auto-collect row fix from issue #464: use a <span> + aria-labelledby
+    // instead.
+    test('enable-laddering label is a span, not a <label for> element', async () => {
+      await renderSection();
+      // A <label for="setting-laddering-enabled"> would make the text clickable.
+      const badLabel = document.querySelector('label[for="setting-laddering-enabled"]');
+      expect(badLabel).toBeNull();
+    });
+
+    test('enable-laddering input carries aria-labelledby referencing the span', async () => {
+      await renderSection();
+      const input = document.getElementById('setting-laddering-enabled') as HTMLInputElement | null;
+      expect(input).not.toBeNull();
+      const labelledById = input!.getAttribute('aria-labelledby');
+      expect(labelledById).toBe('setting-laddering-enabled-label');
+      // The referenced element must exist and contain the label text.
+      const labelSpan = document.getElementById('setting-laddering-enabled-label');
+      expect(labelSpan).not.toBeNull();
+      expect(labelSpan!.textContent).toMatch(/Enable Commitment Laddering/);
+    });
+
+    test('enable-laddering checkbox is wrapped in a .toggle-label element', async () => {
+      await renderSection();
+      const input = document.getElementById('setting-laddering-enabled') as HTMLInputElement | null;
+      expect(input).not.toBeNull();
+      // The input must be inside a .toggle-label so the toggle switch is the
+      // only click target (not the surrounding descriptive text).
+      expect(input!.closest('.toggle-label')).not.toBeNull();
+    });
+  });
+
   describe('renderConfigTable XSS escaping', () => {
     test('renders a malicious cloud_account_id/provider as inert text, not markup', async () => {
       await renderSection();
diff --git a/frontend/src/__tests__/settings.test.ts b/frontend/src/__tests__/settings.test.ts
index 2a5dade9e..3b753301d 100644
--- a/frontend/src/__tests__/settings.test.ts
+++ b/frontend/src/__tests__/settings.test.ts
@@ -19,7 +19,8 @@ jest.mock('../api', () => ({
   updateAccount: jest.fn(),
   deleteAccount: jest.fn(),
   testAccountCredentials: jest.fn(),
-  saveAccountCredentials: jest.fn()
+  saveAccountCredentials: jest.fn(),
+  getLadderConfigs: jest.fn().mockResolvedValue([]),
 }));
 
 // Mock federation module — loadGlobalSettings fires initFederationPanel void-style,
@@ -73,9 +74,9 @@ describe('Settings Module', () => {
           <option value="partial-upfront">Partial Upfront</option>
           <option value="all-upfront">All Upfront</option>
         </select>
-        <input type="number" id="setting-default-coverage">
-        <input type="number" id="setting-notification-days">
-        <input type="number" id="setting-recs-stale-hours" value="24">
+        <input type="number" id="setting-default-coverage" min="0" max="100">
+        <input type="number" id="setting-notification-days" min="1" max="30">
+        <input type="number" id="setting-recs-stale-hours" value="24" min="0" max="8760">
         <select id="setting-recs-lookback-days">
           <option value="7" selected>7 days</option>
           <option value="30">30 days</option>
@@ -1234,4 +1235,204 @@ describe('Settings Module', () => {
       expect(rdsPayment.value).not.toBe('no-upfront');
     });
   });
+
+  // -----------------------------------------------------------------------
+  // Inline range validation — top-level numeric settings (issue #1411)
+  // -----------------------------------------------------------------------
+  // These inputs have min/max in the test DOM (added alongside the fix) so
+  // wireInlineRangeValidation creates the error element and wires the
+  // 'input'/'blur' events. Tests confirm the regression is gone: inline
+  // error appears immediately as the user types, not only after Save.
+  describe('inline range validation on top-level numeric settings (#1411)', () => {
+    function fire(el: HTMLInputElement, type: 'input' | 'blur'): void {
+      el.dispatchEvent(new Event(type));
+    }
+
+    beforeEach(() => {
+      setupSettingsHandlers();
+    });
+
+    it.each([
+      ['setting-notification-days', '0',     'Must be a whole number between 1 and 30'],
+      ['setting-notification-days', '31',    'Must be a whole number between 1 and 30'],
+      ['setting-recs-stale-hours',  '-1',    'Must be a whole number between 0 and 8760'],
+      ['setting-default-coverage',  '101',   'Must be a whole number between 0 and 100'],
+    ])('typing %s=%s shows inline error with message %s', (id, val, msg) => {
+      const input = document.getElementById(id) as HTMLInputElement;
+      input.value = val;
+      fire(input, 'input');
+
+      expect(input.getAttribute('aria-invalid')).toBe('true');
+      const errEl = document.getElementById(`${id}-range-error`);
+      expect(errEl).not.toBeNull();
+      expect(errEl!.classList.contains('hidden')).toBe(false);
+      expect(errEl!.textContent).toBe(msg);
+    });
+
+    it('typing a fractional value into notification-days is rejected inline', () => {
+      const input = document.getElementById('setting-notification-days') as HTMLInputElement;
+      input.value = '1.5';
+      fire(input, 'input');
+
+      expect(input.getAttribute('aria-invalid')).toBe('true');
+      const errEl = document.getElementById('setting-notification-days-range-error');
+      expect(errEl!.classList.contains('hidden')).toBe(false);
+    });
+
+    it('restoring a valid value clears the inline error', () => {
+      const input = document.getElementById('setting-notification-days') as HTMLInputElement;
+      input.value = '0';
+      fire(input, 'input');
+      expect(input.getAttribute('aria-invalid')).toBe('true');
+
+      input.value = '5';
+      fire(input, 'input');
+      expect(input.getAttribute('aria-invalid')).toBeNull();
+      const errEl = document.getElementById('setting-notification-days-range-error');
+      expect(errEl!.classList.contains('hidden')).toBe(true);
+    });
+  });
+
+  // -----------------------------------------------------------------------
+  // Inline range validation — SP coverage inputs (issue #1411 row 7.13)
+  // -----------------------------------------------------------------------
+  describe('inline range validation wired for SP coverage inputs (#1411 row 7.13)', () => {
+    function fire(el: HTMLInputElement, type: 'input' | 'blur'): void {
+      el.dispatchEvent(new Event(type));
+    }
+
+    beforeEach(() => {
+      setupSettingsHandlers();
+    });
+
+    it.each([
+      'aws-savings-plans-compute-coverage',
+      'aws-savings-plans-ec2instance-coverage',
+      'aws-savings-plans-sagemaker-coverage',
+      'aws-savings-plans-database-coverage',
+    ])('typing 101 into %s shows inline error', (id) => {
+      const input = document.getElementById(id) as HTMLInputElement;
+      input.value = '101';
+      fire(input, 'input');
+
+      expect(input.getAttribute('aria-invalid')).toBe('true');
+      const errEl = document.getElementById(`${id}-range-error`);
+      expect(errEl).not.toBeNull();
+      expect(errEl!.classList.contains('hidden')).toBe(false);
+      expect(errEl!.textContent).toBe('Must be a whole number between 0 and 100');
+    });
+
+    it('valid SP coverage value clears any prior error', () => {
+      const id = 'aws-savings-plans-compute-coverage';
+      const input = document.getElementById(id) as HTMLInputElement;
+      input.value = '200';
+      fire(input, 'input');
+      expect(input.getAttribute('aria-invalid')).toBe('true');
+
+      input.value = '80';
+      fire(input, 'input');
+      expect(input.getAttribute('aria-invalid')).toBeNull();
+      const errEl = document.getElementById(`${id}-range-error`);
+      expect(errEl!.classList.contains('hidden')).toBe(true);
+    });
+  });
+
+  // -----------------------------------------------------------------------
+  // Cancel on Global Defaults clears dirty state (issue #1412 rows 7.4/7.9)
+  // -----------------------------------------------------------------------
+  // These tests use loadGlobalSettings() to establish a proper dirty-tracking
+  // snapshot (identical to the real app flow where settings are loaded before
+  // the user interacts). Without the snapshot every field looks dirty regardless
+  // of its value, so the test cannot distinguish "cancel cleared the marker"
+  // from "marker was never set".
+  describe('cancel on Global Defaults propagation clears dirty state (#1412 7.4/7.9)', () => {
+    // Minimal getConfig response that satisfies loadGlobalSettings without errors.
+    const baseConfigResponse = {
+      global: {
+        enabled_providers: ['aws'] as string[],
+        notification_email: '',
+        auto_collect: true,
+        default_term: 3,
+        default_payment: 'all-upfront',
+        default_coverage: 80,
+        notification_days_before: 3,
+        recommendations_cache_stale_hours: 24,
+        recommendations_lookback_days: 7,
+        grace_period_days: { aws: 7, azure: 7, gcp: 7 },
+        laddering_enabled: false,
+      },
+      services: [],
+      credentials: {},
+    };
+
+    test('cancelling term cascade clears has-unsaved and dirty markers', async () => {
+      // loadGlobalSettings populates the form AND calls snapshotAllFields() so
+      // dirty tracking has a clean baseline (term=3 in the snapshot).
+      (api.getConfig as jest.Mock).mockResolvedValue(baseConfigResponse);
+      await loadGlobalSettings();
+      setupSettingsHandlers();
+      mockConfirmDialog.mockResolvedValueOnce(false);
+
+      // Change term to 1 (differs from snapshot=3) -> confirm appears -> cancel.
+      const defaultTerm = document.getElementById('setting-default-term') as HTMLSelectElement;
+      defaultTerm.value = '1';
+      defaultTerm.dispatchEvent(new Event('change'));
+      await new Promise((r) => setTimeout(r, 0));
+
+      // Cancel must restore the previous value.
+      expect(defaultTerm.value).toBe('3');
+
+      // With snapshot=3 and restored=3 the diff is clean, so markers must clear.
+      const tabBtn = document.getElementById('admin-tab-btn');
+      const saveBar = document.querySelector('.settings-buttons');
+      expect(tabBtn?.classList.contains('has-unsaved')).toBe(false);
+      expect(saveBar?.classList.contains('dirty')).toBe(false);
+    });
+
+    test('cancelling payment cascade clears has-unsaved and dirty markers', async () => {
+      (api.getConfig as jest.Mock).mockResolvedValue(baseConfigResponse);
+      await loadGlobalSettings();
+      setupSettingsHandlers();
+      mockConfirmDialog.mockResolvedValueOnce(false);
+
+      const defaultPayment = document.getElementById('setting-default-payment') as HTMLSelectElement;
+      defaultPayment.value = 'no-upfront';
+      defaultPayment.dispatchEvent(new Event('change'));
+      await new Promise((r) => setTimeout(r, 0));
+
+      expect(defaultPayment.value).toBe('all-upfront');
+
+      const tabBtn = document.getElementById('admin-tab-btn');
+      const saveBar = document.querySelector('.settings-buttons');
+      expect(tabBtn?.classList.contains('has-unsaved')).toBe(false);
+      expect(saveBar?.classList.contains('dirty')).toBe(false);
+    });
+  });
+
+  // -----------------------------------------------------------------------
+  // Global Defaults popup uses display names (issue #1412 rows 7.3/7.8)
+  // -----------------------------------------------------------------------
+  describe('Global Defaults popup shows display names not backend IDs (#1412 7.3/7.8)', () => {
+    test('confirmAndPropagateTerm passes display names to confirmDialog', async () => {
+      setupSettingsHandlers();
+      mockConfirmDialog.mockResolvedValueOnce(false);
+
+      const defaultTerm = document.getElementById('setting-default-term') as HTMLSelectElement;
+      defaultTerm.dataset['previous'] = '3';
+      // Set one service to a different value so the diff is non-empty.
+      (document.getElementById('aws-ec2-term') as HTMLSelectElement).value = '3';
+      defaultTerm.value = '1';
+      defaultTerm.dispatchEvent(new Event('change'));
+      await new Promise((r) => setTimeout(r, 0));
+      await new Promise((r) => setTimeout(r, 0));
+
+      expect(mockConfirmDialog).toHaveBeenCalledTimes(1);
+      const callArg = mockConfirmDialog.mock.calls[0]![0] as { body: Node };
+      // body is the HTMLDivElement from buildAffectedList; serialize to check text.
+      const bodyText = (callArg.body as HTMLElement).textContent ?? '';
+      // Must include display name, not raw backend ID.
+      expect(bodyText).toMatch(/EC2 Reserved Instances/);
+      expect(bodyText).not.toMatch(/^aws$/m);
+    });
+  });
 });
diff --git a/frontend/src/ladder.ts b/frontend/src/ladder.ts
index 833a3dce0..0cfccc47a 100644
--- a/frontend/src/ladder.ts
+++ b/frontend/src/ladder.ts
@@ -90,12 +90,21 @@ function renderLadderingSection(globalEnabled: boolean): string {
 
   <div class="setting-row">
     <div class="setting-info">
-      <label for="setting-laddering-enabled">Enable Commitment Laddering</label>
+      <!-- Plain span (not <label for=>) so clicking the descriptive text
+           does not flip the toggle. The toggle's .toggle-label wraps the input
+           and provides the click target. aria-labelledby on the input keeps the
+           descriptive text as the toggle's accessible name. Same pattern as the
+           Auto-collect row (issue #464 / #1412). -->
+      <span class="label-like" id="setting-laddering-enabled-label">Enable Commitment Laddering</span>
       <span class="setting-hint">Global kill-switch. Must be on before any per-account config can fire.</span>
     </div>
     <div class="setting-input">
-      <input type="checkbox" id="setting-laddering-enabled"
-             ${globalEnabled ? 'checked' : ''}${disabledAttr}>
+      <label class="toggle-label">
+        <input type="checkbox" id="setting-laddering-enabled"
+               aria-labelledby="setting-laddering-enabled-label"
+               ${globalEnabled ? 'checked' : ''}${disabledAttr}>
+        <span class="slider"></span>
+      </label>
     </div>
   </div>
 
diff --git a/frontend/src/settings.ts b/frontend/src/settings.ts
index 8c1376f2b..0fe20b974 100644
--- a/frontend/src/settings.ts
+++ b/frontend/src/settings.ts
@@ -260,7 +260,17 @@ function wireInlineRangeValidation(
     // while the value is still in flux (WCAG SCR32). CodeRabbit on #471.
     errorEl.setAttribute('role', 'status');
     errorEl.setAttribute('aria-live', 'polite');
-    input.insertAdjacentElement('afterend', errorEl);
+    // Insert the error element outside the nearest flex container so it
+    // renders below the input row rather than as a flex item to its right.
+    // Priority: after the .setting-input wrapper (settings form rows) ->
+    // after the enclosing <label> (SP-coverage cards) -> directly after the
+    // input as a final fallback (test DOM / modal forms without that wrapper).
+    // Issue #1411.
+    const insertionTarget =
+      input.closest('.setting-input') ??
+      (input.closest('label') as HTMLLabelElement | null) ??
+      input;
+    insertionTarget.insertAdjacentElement('afterend', errorEl);
     // Append (don't overwrite) any pre-existing aria-describedby so the
     // input's existing help text (e.g. unit hints) stays announced.
     const existingDescribedBy = input.getAttribute('aria-describedby');
@@ -2649,6 +2659,17 @@ export function setupSettingsHandlers(signal?: AbortSignal): void {
   wireInlineRangeValidation('setting-grace-aws', { signal, requireInteger: true });
   wireInlineRangeValidation('setting-grace-azure', { signal, requireInteger: true });
   wireInlineRangeValidation('setting-grace-gcp', { signal, requireInteger: true });
+  // Per-SP-plan coverage percentage inputs — must carry the same inline
+  // validation as the global default-coverage field so users see range errors
+  // on the service cards without waiting for Save. Issue #1411 row 7.13.
+  SERVICE_FIELDS
+    .filter(f => 'coverageId' in f)
+    .forEach(f =>
+      wireInlineRangeValidation(
+        (f as { coverageId: string }).coverageId,
+        { signal, requireInteger: true },
+      )
+    );
 
   // Set up dirty-field tracking
   setupDirtyTracking(signal);
diff --git a/frontend/src/styles/settings.css b/frontend/src/styles/settings.css
index 062b14950..85e573c0b 100644
--- a/frontend/src/styles/settings.css
+++ b/frontend/src/styles/settings.css
@@ -81,6 +81,7 @@
 
 .setting-row {
     display: flex;
+    flex-wrap: wrap; /* allow inline range-error to drop below the label+input row */
     justify-content: space-between;
     align-items: center;
     padding: 1rem 0;
@@ -91,6 +92,13 @@
     border-bottom: none;
 }
 
+/* Inline range-error inserted after .setting-input (outside the flex row) must
+   span the full width so it appears below the label+input pair, not beside it.
+   Issue #1411. */
+.setting-row > .field-error {
+    flex-basis: 100%;
+}
+
 .setting-info {
     flex: 1;
     display: flex;