fix(web): don't validate double-sided settings when the feature is disabled (#422)

* fix(web): don't validate double-sided settings when the feature is disabled Saving anything on the Display tab failed with a 400 when double-sided mode was off: Double-sided copies (2) must divide chain length (3) evenly The Display form posts every field in one request, including double_sided_copies (default 2) and double_sided_axis, whether or not the Enabled checkbox is ticked. The server block was gated only on "is any double-sided field present in the payload" — it wrote ds_config['enabled'] but never read it. A user with chain_length: 3 and the untouched default copies: 2 was locked out of saving any display setting at all: brightness, GPIO slowdown, Vegas, sync. Gate the checks on the enabled flag: - Divisibility against chain_length/parallel is hardware-relational and only runs when the feature is on. - Structural checks (copies parses as an int in 2..8, axis in the whitelist) still 400 when enabled; when disabled they drop the value and leave the stored one untouched rather than rejecting the save. The runtime already gated correctly (_resolve_double_sided returns None when disabled), so nothing there changes. Also in the Display tab: - Hide Copies / Split Axis until Enabled is ticked, mirroring the Vegas Scroll pattern. Hidden rather than disabled, so the fields keep submitting and the server still sees an 'off' state to persist. - Fix the save toast: the form's handler read xhr.responseJSON, a jQuery property that doesn't exist on a native XMLHttpRequest, so it was always undefined and every save reported a green "Display settings saved" — even the 400s. Parse responseText and use the real status. Tests: two existing double-sided tests asserted 200 on payloads that the divisibility check (added later, in #373) turns into 400s; the first now supplies matching hardware values and the second passes as written now that a disabled save skips the check. Added coverage for the reported regression, for bad values while disabled, and for the check still firing when enabled. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016aUvzTXpEYhNEWYQvFqQU9 * fix(web): treat only 2xx as a successful display save Review feedback on #422. - showDisplaySaveResult tested `xhr.status >= 400` for failure, so a network error — which reports status 0 — was waved through as "Display settings saved". That's the same class of false-success bug this branch set out to fix. Test the 2xx range instead, and let a response body refine a successful verdict without overturning a failed one. - Annotate the _copies_fits_hardware helper, matching the annotated helpers already in api_v3.py. - Cover the vertical divisibility branch: chain_length 2 would divide evenly, so only parallel 3 can produce the rejection, which pins the branch to the right hardware dimension. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016aUvzTXpEYhNEWYQvFqQU9 --------- Co-authored-by: Claude <noreply@anthropic.com>
2026-08-01 08:48:05 +00:00 · 2026-07-28 16:54:42 -04:00
parent 3872a68ff7
commit e2acbfb566
3 changed files with 180 additions and 30 deletions
@@ -146,6 +146,11 @@ class TestConfigAPI:

    def test_save_double_sided_settings(self, client, mock_config_manager):
        """Double-sided form fields are persisted under display.double_sided."""
+        # 2 copies on the vertical axis needs parallel to be a multiple of 2.
+        mock_config_manager.load_config.return_value['display']['hardware'] = {
+            'chain_length': 2, 'parallel': 2,
+        }
+
        response = client.post(
            '/api/v3/config/main',
            data={
@@ -175,6 +180,91 @@ class TestConfigAPI:
        assert ds['enabled'] is False
        assert ds['copies'] == 4

+    def test_save_double_sided_disabled_skips_divisibility_check(self, client, mock_config_manager):
+        """A copies/chain_length mismatch must not block saves while disabled.
+
+        The Display form posts copies/axis on every save, so validating them
+        with the feature off locked users out of every other display setting.
+        """
+        mock_config_manager.load_config.return_value['display']['hardware'] = {
+            'chain_length': 3, 'parallel': 1,
+        }
+
+        response = client.post(
+            '/api/v3/config/main',
+            data={
+                'double_sided_copies': '2',
+                'double_sided_axis': 'horizontal',
+                'brightness': '75',
+            },
+            content_type='application/x-www-form-urlencoded',
+        )
+
+        assert response.status_code == 200
+        ds = mock_config_manager.save_config_atomic.call_args[0][0]['display']['double_sided']
+        assert ds['enabled'] is False
+        assert ds['copies'] == 2
+
+    def test_save_double_sided_enabled_enforces_divisibility(self, client, mock_config_manager):
+        """The same mismatch is still rejected once the feature is turned on."""
+        mock_config_manager.load_config.return_value['display']['hardware'] = {
+            'chain_length': 3, 'parallel': 1,
+        }
+
+        response = client.post(
+            '/api/v3/config/main',
+            data={
+                'double_sided_enabled': 'true',
+                'double_sided_copies': '2',
+                'double_sided_axis': 'horizontal',
+            },
+            content_type='application/x-www-form-urlencoded',
+        )
+
+        assert response.status_code == 400
+        assert 'chain length' in response.get_json()['message']
+        mock_config_manager.save_config_atomic.assert_not_called()
+
+    def test_save_double_sided_vertical_checks_parallel(self, client, mock_config_manager):
+        """The vertical axis is checked against parallel, not chain_length."""
+        mock_config_manager.load_config.return_value['display']['hardware'] = {
+            'chain_length': 2, 'parallel': 3,
+        }
+
+        response = client.post(
+            '/api/v3/config/main',
+            data={
+                'double_sided_enabled': 'true',
+                'double_sided_copies': '2',
+                'double_sided_axis': 'vertical',
+            },
+            content_type='application/x-www-form-urlencoded',
+        )
+
+        # chain_length 2 would divide evenly — only parallel 3 rejects this.
+        assert response.status_code == 400
+        assert 'parallel' in response.get_json()['message']
+        mock_config_manager.save_config_atomic.assert_not_called()
+
+    def test_save_double_sided_disabled_ignores_bad_values(self, client, mock_config_manager):
+        """While disabled, unusable copies/axis are dropped rather than rejected."""
+        mock_config_manager.load_config.return_value['display']['double_sided'] = {
+            'enabled': True, 'copies': 2, 'axis': 'horizontal',
+        }
+
+        response = client.post(
+            '/api/v3/config/main',
+            data={'double_sided_copies': 'abc', 'double_sided_axis': 'diagonal'},
+            content_type='application/x-www-form-urlencoded',
+        )
+
+        assert response.status_code == 200
+        ds = mock_config_manager.save_config_atomic.call_args[0][0]['display']['double_sided']
+        assert ds['enabled'] is False
+        # Stored values left untouched rather than overwritten with junk.
+        assert ds['copies'] == 2
+        assert ds['axis'] == 'horizontal'
+
    def test_save_double_sided_invalid_copies_rejected(self, client, mock_config_manager):
        """copies < 2 is rejected with a 400 before any save."""
        response = client.post(
@@ -13,7 +13,7 @@ import uuid
 import logging
 from datetime import datetime
 from pathlib import Path
-from typing import Dict, Any
+from typing import Dict, Any, Optional
 from urllib.parse import urlparse, urlunparse

 logger = logging.getLogger(__name__)
@@ -864,16 +864,15 @@ def save_main_config():
            ds_config = current_config['display']['double_sided']

            # Enabled checkbox: omitted from the form when unchecked.
-            ds_config['enabled'] = _coerce_to_bool(data.get('double_sided_enabled'))
+            # The Display form posts copies/axis on every save regardless of this
+            # checkbox, so when the feature is off we accept the values without
+            # rejecting the whole save — otherwise a stale copies/chain_length
+            # mismatch locks the user out of every other display setting.
+            enabled = _coerce_to_bool(data.get('double_sided_enabled'))
+            ds_config['enabled'] = enabled

-            if 'double_sided_copies' in data and data['double_sided_copies'] not in ('', None):
-                try:
-                    copies = int(data['double_sided_copies'])
-                except (ValueError, TypeError):
-                    return jsonify({'status': 'error', 'message': "Double-sided copies must be an integer"}), 400
-                if not (2 <= copies <= 8):
-                    return jsonify({'status': 'error', 'message': "Double-sided copies must be between 2 and 8"}), 400
-                # Validate divisibility against the relevant hardware dimension.
+            def _copies_fits_hardware(copies: int) -> Optional[str]:
+                """Error message if copies doesn't divide the panel evenly, else None."""
                # Use axis from this request if provided, else from stored config.
                hw = current_config.get('display', {}).get('hardware', {})
                effective_axis = (data.get('double_sided_axis')
@@ -881,17 +880,40 @@ def save_main_config():
                if effective_axis == 'horizontal':
                    chain_length = int(hw.get('chain_length', 2) or 2)
                    if chain_length % copies != 0:
-                        return jsonify({'status': 'error', 'message': f"Double-sided copies ({copies}) must divide chain length ({chain_length}) evenly"}), 400
+                        return f"Double-sided copies ({copies}) must divide chain length ({chain_length}) evenly"
                elif effective_axis == 'vertical':
                    parallel = int(hw.get('parallel', 1) or 1)
                    if parallel % copies != 0:
-                        return jsonify({'status': 'error', 'message': f"Double-sided copies ({copies}) must divide parallel ({parallel}) evenly"}), 400
+                        return f"Double-sided copies ({copies}) must divide parallel ({parallel}) evenly"
+                return None
+
+            if 'double_sided_copies' in data and data['double_sided_copies'] not in ('', None):
+                copies = None
+                try:
+                    copies = int(data['double_sided_copies'])
+                except (ValueError, TypeError):
+                    if enabled:
+                        return jsonify({'status': 'error', 'message': "Double-sided copies must be an integer"}), 400
+                if copies is not None and not (2 <= copies <= 8):
+                    if enabled:
+                        return jsonify({'status': 'error', 'message': "Double-sided copies must be between 2 and 8"}), 400
+                    # Disabled: leave the stored value alone rather than writing junk.
+                    copies = None
+                if copies is not None:
+                    # Divisibility is a hardware-relational check — only meaningful
+                    # when the feature is actually on.
+                    if enabled:
+                        fit_error = _copies_fits_hardware(copies)
+                        if fit_error:
+                            return jsonify({'status': 'error', 'message': fit_error}), 400
                    ds_config['copies'] = copies

            if 'double_sided_axis' in data:
                axis = data['double_sided_axis']
                if axis not in ('horizontal', 'vertical'):
+                    if enabled:
                        return jsonify({'status': 'error', 'message': "Double-sided axis must be 'horizontal' or 'vertical'"}), 400
+                else:
                    ds_config['axis'] = axis

        # Handle Vegas scroll mode settings
@@ -30,7 +30,7 @@
          hx-ext="json-enc"
          hx-headers='{"Content-Type": "application/json"}'
          hx-swap="none"
-          hx-on:htmx:after-request="showNotification(event.detail.xhr.responseJSON?.message || 'Display settings saved', event.detail.xhr.responseJSON?.status || 'success')"
+          hx-on:htmx:after-request="showDisplaySaveResult(event.detail.xhr)"
          class="space-y-6"
          novalidate
          onsubmit="fixInvalidNumberInputs(this); return true;">
@@ -475,8 +475,7 @@
            <h3 class="text-md font-medium text-gray-900 mb-1">Double-Sided Display</h3>
            <p class="text-sm text-gray-600 mb-4">Show the same content on every panel in the chain &mdash; e.g. two 64&times;32 panels mirrored, or four panels as two identical screens. Rendered once and duplicated, so it adds no extra CPU. Takes effect after a display restart.</p>

-            <div class="grid grid-cols-1 md:grid-cols-3 gap-4">
-                <div class="form-group" id="setting-display-double_sided_enabled" data-setting-key="display.double_sided.enabled">
+            <div class="form-group mb-4" id="setting-display-double_sided_enabled" data-setting-key="display.double_sided.enabled">
                <label class="flex items-center gap-2">
                    <input type="checkbox"
                           id="double_sided_enabled"
@@ -489,8 +488,13 @@
                </label>
            </div>

+            <!-- Hidden (not disabled) when the feature is off, so the values are
+                 still submitted and round-trip through a save. -->
+            <div id="double_sided_settings"
+                 class="grid grid-cols-1 md:grid-cols-2 gap-4"
+                 {% if not main_config.display.get('double_sided', {}).get('enabled') %}style="display: none;"{% endif %}>
                <div class="form-group" id="setting-display-double_sided_copies" data-setting-key="display.double_sided.copies">
-                    <label for="double_sided_copies" class="block text-sm font-medium text-gray-700">Copies{{ ui.help_tip('How many identical screens to split the panel area into (2–8).\nMust divide the panel evenly — e.g. 2 for a two-sided cube.', 'Copies') }}</label>
+                    <label for="double_sided_copies" class="block text-sm font-medium text-gray-700">Copies{{ ui.help_tip('How many identical screens to split the panel area into (2–8).\nWhen enabled, this must divide the panel evenly — e.g. 2 for a two-sided cube.', 'Copies') }}</label>
                    <input type="number"
                           id="double_sided_copies"
                           name="double_sided_copies"
@@ -607,6 +611,28 @@ if (typeof window.fixInvalidNumberInputs !== 'function') {
    };
 }

+// Report the outcome of a display-settings save. XMLHttpRequest has no
+// `responseJSON` (that's a jQuery property) — read `responseText` and the real
+// status code, otherwise a failed save reports success.
+window.showDisplaySaveResult = function(xhr) {
+    // Only 2xx counts as saved. A network failure reports status 0, which any
+    // `>= 400` test would wave through as success.
+    const httpSuccess = xhr.status >= 200 && xhr.status < 300;
+    let message = httpSuccess
+        ? 'Display settings saved'
+        : 'Display settings were not saved. Check your connection and try again.';
+    let status = httpSuccess ? 'success' : 'error';
+    try {
+        const data = JSON.parse(xhr.responseText);
+        if (data.message) message = data.message;
+        // A body can refine a successful verdict but never overturn a failed one.
+        if (httpSuccess && data.status) status = data.status;
+    } catch {
+        // Non-JSON body — fall back to the status-code verdict above.
+    }
+    showNotification(message, status);
+};
+
 // Vegas Scroll Mode Settings
 (function() {
    // Escape HTML to prevent XSS
@@ -631,6 +657,18 @@ if (typeof window.fixInvalidNumberInputs !== 'function') {
        });
    }

+    // Double-sided: copies/axis only mean anything while the feature is on.
+    // Hidden rather than disabled so the fields keep submitting and the server
+    // still sees an 'off' state to persist.
+    const doubleSidedCheckbox = document.getElementById('double_sided_enabled');
+    const doubleSidedSettings = document.getElementById('double_sided_settings');
+
+    if (doubleSidedCheckbox && doubleSidedSettings) {
+        doubleSidedCheckbox.addEventListener('change', function() {
+            doubleSidedSettings.style.display = this.checked ? 'grid' : 'none';
+        });
+    }
+
    // Update scroll speed display
    const scrollSpeedSlider = document.getElementById('vegas_scroll_speed');
    const scrollSpeedValue = document.getElementById('vegas_scroll_speed_value');