From b7f5f8483ad703e8b517564f847de1b0092d3c1d Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 28 Jul 2026 20:17:29 +0000 Subject: [PATCH] fix(web): don't validate double-sided settings when the feature is disabled MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_016aUvzTXpEYhNEWYQvFqQU9 --- test/test_web_api.py | 69 +++++++++++++++++++ web_interface/blueprints/api_v3.py | 50 ++++++++++---- .../templates/v3/partials/display.html | 62 +++++++++++++---- 3 files changed, 152 insertions(+), 29 deletions(-) diff --git a/test/test_web_api.py b/test/test_web_api.py index e8f57bea..2bfe46bb 100644 --- a/test/test_web_api.py +++ b/test/test_web_api.py @@ -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,70 @@ 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_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( diff --git a/web_interface/blueprints/api_v3.py b/web_interface/blueprints/api_v3.py index 41631ef1..ae1a8c38 100644 --- a/web_interface/blueprints/api_v3.py +++ b/web_interface/blueprints/api_v3.py @@ -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): + """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,18 +880,41 @@ 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 - ds_config['copies'] = copies + 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'): - return jsonify({'status': 'error', 'message': "Double-sided axis must be 'horizontal' or 'vertical'"}), 400 - ds_config['axis'] = axis + 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 vegas_fields = ['vegas_scroll_enabled', 'vegas_scroll_speed', 'vegas_separator_width', diff --git a/web_interface/templates/v3/partials/display.html b/web_interface/templates/v3/partials/display.html index b577010f..0add70c7 100644 --- a/web_interface/templates/v3/partials/display.html +++ b/web_interface/templates/v3/partials/display.html @@ -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,22 +475,26 @@

Double-Sided Display

Show the same content on every panel in the chain — e.g. two 64×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.

-
-
- -
+
+ +
+ +
- + = 400 ? 'error' : 'success'; + try { + const data = JSON.parse(xhr.responseText); + if (data.message) message = data.message; + if (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 +651,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');