From 10d516e93ef9fecfc9bfdb1630d57c144068597b Mon Sep 17 00:00:00 2001
From: Cristian Magherusan-Stanciu <cristi.magherusan@gmail.com>
Date: Thu, 1 Oct 2026 20:39:23 +0200
Subject: [PATCH] fix(frontend): preserve ladder form selections when editing

Clone the form before populating its controls so editing retains the
provider, approval mode and cadence in the submitted configuration.

Cover the Edit and submit path in Jest and the production browser,
including repeated opens, defaults, Cancel/Close and mobile behavior.
---
 frontend/src/__tests__/ladder.test.ts    |  29 +++++-
 frontend/src/ladder.ts                   |  32 +++---
 frontend/tests-e2e/ladder-config.spec.ts | 125 +++++++++++++++++++++++
 3 files changed, 166 insertions(+), 20 deletions(-)
 create mode 100644 frontend/tests-e2e/ladder-config.spec.ts

diff --git a/frontend/src/__tests__/ladder.test.ts b/frontend/src/__tests__/ladder.test.ts
index 03b39960..cb1aca3a 100644
--- a/frontend/src/__tests__/ladder.test.ts
+++ b/frontend/src/__tests__/ladder.test.ts
@@ -9,6 +9,7 @@
  */
 import { initLadderingSettings, renderConfigTable, saveLadderConfig } from '../ladder';
 import type { LadderConfig } from '../api';
+import { waitFor } from '@testing-library/dom';
 
 jest.mock('../api', () => ({
   getLadderConfigs: jest.fn(),
@@ -50,9 +51,9 @@ function baseConfig(overrides: Partial<LadderConfig> = {}): LadderConfig {
 
 // Render the full section (incl. the modal form) into the DOM so the exported
 // helpers have the elements they read.
-async function renderSection(): Promise<void> {
+async function renderSection(configs: LadderConfig[] = []): Promise<void> {
   document.body.innerHTML = '<div id="commitment-laddering-settings"></div>';
-  (api.getLadderConfigs as jest.Mock).mockResolvedValue([]);
+  (api.getLadderConfigs as jest.Mock).mockResolvedValue(configs);
   await initLadderingSettings(false);
 }
 
@@ -62,6 +63,30 @@ describe('ladder.ts', () => {
     mockShowToast.mockReset();
   });
 
+  test.each([
+    ['aws', 'email_approval', 'daily'],
+    ['azure', 'auto_approve', 'weekly'],
+    ['gcp', 'auto_approve', 'weekly'],
+  ] as const)('editing %s preserves the selected values in the submitted config', async (provider, mode, cadence) => {
+    const config = baseConfig({ id: 'config-1', provider, mode, cadence, target_coverage: 80 });
+    await renderSection([config]);
+    (api.upsertLadderConfig as jest.Mock).mockImplementation(async (cfg: LadderConfig) => cfg);
+
+    document.querySelector<HTMLButtonElement>('.ladder-edit-btn')!.click();
+    expect(document.getElementById('ladder-cfg-provider')).toHaveValue(provider);
+    expect(document.getElementById('ladder-cfg-mode')).toHaveValue(mode);
+    expect(document.getElementById('ladder-cfg-cadence')).toHaveValue(cadence);
+    expect(document.getElementById('ladder-cfg-provider')).toBeDisabled();
+    expect(document.getElementById('ladder-cfg-account')).toHaveAttribute('readonly');
+
+    (document.getElementById('ladder-cfg-target-coverage') as HTMLInputElement).value = '75';
+    document.getElementById('ladder-config-form')!.dispatchEvent(new Event('submit', { cancelable: true }));
+    await waitFor(() => expect(document.getElementById('ladder-config-modal')).toHaveClass('hidden'));
+    expect(api.upsertLadderConfig).toHaveBeenCalledTimes(1);
+    const { updated_at: _updatedAt, ...expected } = config;
+    expect(api.upsertLadderConfig).toHaveBeenCalledWith({ ...expected, target_coverage: 75 });
+  });
+
   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
diff --git a/frontend/src/ladder.ts b/frontend/src/ladder.ts
index 0cfccc47..ee92a36e 100644
--- a/frontend/src/ladder.ts
+++ b/frontend/src/ladder.ts
@@ -358,6 +358,20 @@ function openLadderConfigModal(existing?: api.LadderConfig): void {
   const modal = document.getElementById('ladder-config-modal');
   if (!modal) return;
 
+  // Clone before populating: select values do not survive cloneNode.
+  const form = document.getElementById('ladder-config-form');
+  if (form) {
+    const newForm = form.cloneNode(true) as HTMLElement;
+    form.replaceWith(newForm);
+    newForm.addEventListener('submit', (e) => {
+      e.preventDefault();
+      saveLadderConfig();
+    });
+    // Cancel is inside the cloned form; the outside Close button keeps its listener.
+    newForm.querySelector('#ladder-modal-cancel-btn')
+      ?.addEventListener('click', closeLadderModal);
+  }
+
   // Populate modal fields.
   setValue('ladder-cfg-id', existing?.id ?? '');
   setValue('ladder-cfg-account', existing?.cloud_account_id ?? '');
@@ -386,24 +400,6 @@ function openLadderConfigModal(existing?: api.LadderConfig): void {
   if (accountInput) accountInput.readOnly = !!existing;
   if (providerSelect) providerSelect.disabled = !!existing;
 
-  // Wire the save button (remove previous listener by replacing the element
-  // clone so duplicate-listener accumulation cannot occur).
-  const form = document.getElementById('ladder-config-form');
-  if (form) {
-    const newForm = form.cloneNode(true) as HTMLElement;
-    form.replaceWith(newForm);
-    newForm.addEventListener('submit', (e) => {
-      e.preventDefault();
-      saveLadderConfig();
-    });
-    // The Cancel button lives inside the form, so cloneNode drops its
-    // listener (cloneNode does not copy event handlers). Re-wire it on the
-    // fresh node. The × close button sits outside the form and keeps the
-    // listener attached once in wireModalCloseButtons.
-    newForm.querySelector('#ladder-modal-cancel-btn')
-      ?.addEventListener('click', closeLadderModal);
-  }
-
   modal.classList.remove('hidden');
 }
 
diff --git a/frontend/tests-e2e/ladder-config.spec.ts b/frontend/tests-e2e/ladder-config.spec.ts
new file mode 100644
index 00000000..876c358f
--- /dev/null
+++ b/frontend/tests-e2e/ladder-config.spec.ts
@@ -0,0 +1,125 @@
+import { test, expect } from '@playwright/test';
+import { mockApi, seedAuth } from './fixtures/recs';
+import type { LadderConfig } from '../src/api/ladder';
+
+for (const viewport of [{ width: 1280, height: 900 }, { width: 390, height: 844 }]) {
+  test(`ladder editor preserves selections and resets new configs at ${viewport.width}px`, async ({ page }) => {
+    await page.setViewportSize(viewport);
+    const errors: string[] = [];
+    const failedRequests: string[] = [];
+    page.on('pageerror', error => errors.push(error.message));
+    page.on('response', response => { if (response.status() >= 400) failedRequests.push(response.url()); });
+    page.on('requestfailed', request => failedRequests.push(request.url()));
+    await page.route('**/*', route => new URL(route.request().url()).hostname === '127.0.0.1'
+      ? route.fallback() : route.abort());
+    await seedAuth(page);
+    await mockApi(page);
+    await page.route('**/api/commitment-options', route => route.fulfill({ json: { status: 'ok', aws: {} } }));
+    await page.route('**/api/ri-exchange/config', route => route.fulfill({ json: {
+      mode: 'manual', auto_exchange_enabled: false, utilization_threshold: 80,
+      max_payment_per_exchange_usd: 0, max_payment_daily_usd: 0, lookback_days: 30,
+    } }));
+
+    const configs: LadderConfig[] = ['aws', 'azure', 'gcp'].map((provider, index) => ({
+      id: `config-${index}`, cloud_account_id: `account-${index}`, provider, enabled: true,
+      mode: provider === 'aws' ? 'email_approval' : 'auto_approve',
+      cadence: provider === 'aws' ? 'daily' : 'weekly',
+      target_coverage: 80, buffer_fraction: 0.2, baseline_percentile: 10,
+      lookback_days: 14, buffer_utilization_threshold: 85,
+      max_hourly_commit_per_run: 5, max_actions_per_run: 3,
+      ramp_schedule: { steps: [{ after_days: 0, fraction: 0.5 }, { after_days: 7, fraction: 0.5 }] },
+    }));
+    const saves: LadderConfig[] = [];
+    await page.route('**/api/ladder/configs', async route => {
+      if (route.request().method() === 'GET') {
+        await route.fulfill({ json: { configs } });
+        return;
+      }
+      expect(route.request().method()).toBe('PUT');
+      const saved = route.request().postDataJSON() as LadderConfig;
+      saves.push(saved);
+      const index = configs.findIndex(config => config.cloud_account_id === saved.cloud_account_id && config.provider === saved.provider);
+      if (index < 0) configs.push(saved);
+      else configs[index] = saved;
+      await route.fulfill({ json: saved });
+    });
+
+    await page.goto('/admin/purchasing');
+    const modal = page.locator('#ladder-config-modal');
+    const field = (name: string) => page.locator(`#ladder-cfg-${name}`);
+    const edit = (provider: string) => page.locator(`.ladder-edit-btn[data-provider="${provider}"]`).first();
+
+    for (const provider of ['azure', 'gcp', 'aws']) {
+      const config = configs.find(row => row.provider === provider)!;
+      await edit(provider).click();
+      await expect(modal).toBeVisible();
+      await expect(field('provider')).toHaveValue(config.provider);
+      await expect(field('provider')).toBeDisabled();
+      await expect(field('account')).toHaveValue(config.cloud_account_id);
+      await expect(field('account')).toHaveAttribute('readonly');
+      await expect(field('id')).toHaveValue(config.id!);
+      await expect(field('mode')).toHaveValue(config.mode);
+      await expect(field('cadence')).toHaveValue(config.cadence);
+      await expect(field('enabled')).toBeChecked();
+      for (const [name, value] of Object.entries({
+        'target-coverage': 80, 'buffer-fraction': 0.2, 'baseline-percentile': 10,
+        'lookback-days': 14, 'buf-util-threshold': 85, 'max-hourly': 5, 'max-actions': 3,
+      })) await expect(field(name)).toHaveValue(String(value));
+      expect(JSON.parse(await field('ramp-schedule').inputValue())).toEqual(config.ramp_schedule);
+      await page.locator(provider === 'gcp' ? '#ladder-modal-close-btn' : '#ladder-modal-cancel-btn').click();
+      await expect(modal).toBeHidden();
+      expect(saves).toEqual([]);
+    }
+
+    for (const [index, target] of [75, 70].entries()) {
+      await edit('azure').click();
+      const expected = { ...configs.find(row => row.provider === 'azure')!, target_coverage: target };
+      await field('target-coverage').fill(String(target));
+      await page.locator('#ladder-config-save-btn').click();
+      await expect(modal).toBeHidden();
+      expect(saves).toHaveLength(index + 1);
+      expect(saves[index]).toEqual(expected);
+      await expect(page.locator('#ladder-configs-table-container tbody tr').filter({ has: edit('azure') })).toContainText(`${target.toFixed(1)}%`);
+      await page.getByRole('button', { name: 'Dismiss notification' }).click();
+    }
+
+    await page.locator('#ladder-add-config-btn').click();
+    await expect(field('id')).toHaveValue('');
+    await expect(field('account')).toHaveValue('');
+    await expect(field('account')).not.toHaveAttribute('readonly');
+    await expect(field('provider')).toBeEnabled();
+    await expect(field('provider')).toHaveValue('aws');
+    await expect(field('mode')).toHaveValue('email_approval');
+    await expect(field('cadence')).toHaveValue('daily');
+    await expect(field('enabled')).not.toBeChecked();
+    for (const [name, value] of Object.entries({
+      'target-coverage': '100', 'buffer-fraction': '0.1', 'baseline-percentile': '5',
+      'lookback-days': '30', 'buf-util-threshold': '90', 'max-hourly': '', 'max-actions': '10',
+    })) await expect(field(name)).toHaveValue(value);
+    expect(JSON.parse(await field('ramp-schedule').inputValue())).toEqual({ steps: [{ after_days: 0, fraction: 1 }] });
+    await field('account').fill('new-account');
+    await field('provider').selectOption('gcp');
+    await field('mode').selectOption('auto_approve');
+    await field('cadence').selectOption('weekly');
+    await page.locator('#ladder-config-save-btn').click();
+    await expect(modal).toBeHidden();
+    expect(saves).toHaveLength(3);
+    expect(saves[2]).toEqual({
+      cloud_account_id: 'new-account', provider: 'gcp', mode: 'auto_approve', cadence: 'weekly',
+      enabled: false, target_coverage: 100, buffer_fraction: 0.1, baseline_percentile: 5,
+      lookback_days: 30, buffer_utilization_threshold: 90, max_hourly_commit_per_run: null,
+      max_actions_per_run: 10, ramp_schedule: { steps: [{ after_days: 0, fraction: 1 }] },
+    });
+    await page.getByRole('button', { name: 'Dismiss notification' }).click();
+    await page.goto('/admin/general');
+    await expect(page).toHaveURL(/\/admin\/general$/);
+    await page.goBack();
+    await expect(page).toHaveURL(/\/admin\/purchasing$/);
+    await expect(edit('azure')).toBeVisible();
+    await page.goForward();
+    await expect(page).toHaveURL(/\/admin\/general$/);
+    expect(saves).toHaveLength(3);
+    expect(errors).toEqual([]);
+    expect(failedRequests).toEqual([]);
+  });
+}