fix(web): don't validate double-sided settings when the feature is disabled - #422
Conversation
…sabled
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
|
Warning Review limit reached
Next review available in: 54 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe API now validates double-sided copies and axis only when enabled, while preserving stored values for invalid disabled-mode inputs. The display form conditionally shows dependent settings, uses improved save-result handling, and adds tests for the updated behavior. ChangesDouble-Sided Configuration
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant save_main_config
participant ConfigManager
Browser->>save_main_config: Submit display settings
save_main_config->>save_main_config: Coerce enabled state
save_main_config->>save_main_config: Validate copies and axis if enabled
save_main_config->>ConfigManager: Save accepted configuration
ConfigManager-->>save_main_config: Save result
save_main_config-->>Browser: HTTP response
Browser->>Browser: Show save notification
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
web_interface/blueprints/api_v3.py (1)
874-888: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd type annotations to the validation helper.
Annotate
copiesand the optional error-string return type to satisfy the project’s Python typing requirement. As per coding guidelines, “Use type hints for function parameters and return values.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web_interface/blueprints/api_v3.py` around lines 874 - 888, Add type annotations to the nested _copies_fits_hardware helper: annotate copies as an integer and its return value as an optional string, preserving the existing validation logic and None/error-message behavior.Source: Coding guidelines
test/test_web_api.py (1)
208-226: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a vertical-axis divisibility regression test.
This only proves horizontal validation uses
chain_length. Add a case withchain_length=2,parallel=3,copies=2, andaxis=vertical, expecting HTTP 400 and an error mentioningparallel; otherwise a dimension mix-up in the vertical branch would pass coverage. As per coding guidelines, “Test edge cases including empty data, API failures, and configuration errors.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/test_web_api.py` around lines 208 - 226, Add a regression test alongside test_save_double_sided_enabled_enforces_divisibility using chain_length=2, parallel=3, double_sided_copies=2, and double_sided_axis=vertical. Post to /api/v3/config/main and assert HTTP 400, an error message mentioning parallel, and that mock_config_manager.save_config_atomic was not called.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web_interface/templates/v3/partials/display.html`:
- Around line 617-627: Update window.showDisplaySaveResult so status is
successful only when xhr.status is within the 2xx range; treat status 0 and all
other non-2xx responses as errors. Prevent data.status from overriding this
error verdict, while preserving JSON message handling and allowing valid 2xx
responses to use the server-provided status.
---
Nitpick comments:
In `@test/test_web_api.py`:
- Around line 208-226: Add a regression test alongside
test_save_double_sided_enabled_enforces_divisibility using chain_length=2,
parallel=3, double_sided_copies=2, and double_sided_axis=vertical. Post to
/api/v3/config/main and assert HTTP 400, an error message mentioning parallel,
and that mock_config_manager.save_config_atomic was not called.
In `@web_interface/blueprints/api_v3.py`:
- Around line 874-888: Add type annotations to the nested _copies_fits_hardware
helper: annotate copies as an integer and its return value as an optional
string, preserving the existing validation logic and None/error-message
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b40098fe-3e76-4aa7-87a5-60199e429295
📒 Files selected for processing (3)
test/test_web_api.pyweb_interface/blueprints/api_v3.pyweb_interface/templates/v3/partials/display.html
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
Up to standards ✅🟢 Issues
|
2 participants
Pull Request
Summary
Saving anything on the Display tab failed with a
400 Double-sided copies (2) must divide chain length (3) evenlywhile double-sided mode was off. The Display form posts every field in one request — includingdouble_sided_copies(default2) anddouble_sided_axis— regardless of the Enabled checkbox, and the server block was gated only on "is any double-sided field present in the payload": it wroteds_config['enabled']but never read it. A user withchain_length: 3and the untouched defaultcopies: 2was locked out of saving any display setting — brightness, GPIO slowdown, Vegas, sync. This gates the checks on the enabled flag and tidies up two related UI problems visible in the same console log.Type of change
Related issues
None filed — reported directly with the browser console output showing the 400 alongside a green "Display settings saved" toast.
What changed
web_interface/blueprints/api_v3.py— readenabledfirst, then branch:chain_length/parallelis a hardware-relational check and now only runs when the feature is on. This is the check that produced the reported error.intin 2–8, axis in the whitelist) still return 400 when enabled. When disabled they drop the offending value and leave the stored one untouched, so the save succeeds and the config stays well-formed.effective_axisresolution are unchanged; the two divisibility branches moved into a small local helper returning the message orNone.The runtime was already correct —
_resolve_double_sided()returnsNoneimmediately when disabled (src/display_manager.py:109), so nothing is composited or wrapped. No change there.web_interface/templates/v3/partials/display.htmldisabledon purpose:disabledinputs aren't submitted, and if all three keys vanished from the payload the server block would be skipped entirely and a previously-enabled config would never be flipped tofalse.xhr.responseJSON— a jQuery property that does not exist on a nativeXMLHttpRequest— so it was alwaysundefinedand every save fired a green "Display settings saved", including the 400s. It now parsesresponseTextand uses the real status code.Test plan
EMULATOR=true python3 run.py)scripts/dev_server.py)pytest)pytest test/ --ignore=test/plugins→ 1071 passed, 1 skipped. Two failures intest/web_interface/test_state_reconciliation.pyare pre-existing — they fail identically on a clean checkout ofmain.Two existing double-sided tests were asserting
200on payloads that the divisibility check (added later, in #373) turns into400s, so they were failing before this change:test_save_double_sided_settingspostedcopies=2, axis=verticalagainst a fixture with nodisplay.hardware, soparallelfell back to1and1 % 2 != 0. It now supplies matching hardware values in the test rather than editing the shared fixture.test_save_double_sided_unchecked_disablespostedcopies=4withchain_lengthdefaulting to2. It passes as written now that a disabled save skips the check.Added three tests: the reported regression (disabled +
copies=2+chain_length=3→ 200), unparseable/unknown values while disabled (→ 200, stored values untouched), and the check still firing when enabled (→ 400).Not automated, worth a manual pass: toggling the Enabled checkbox shows/hides the two fields, and a failed save now shows a single red toast with no accompanying green one.
Documentation
README.mdif user-facing behavior changeddocs/if developer behavior changedPlugin compatibility
Plugins see the logical per-screen size through
_LogicalMatrix, and that path is unchanged.Checklist
CONTRIBUTING.mdCONTRIBUTING.mdandCODE_OF_CONDUCT.mddisplay.double_sidedis unchanged in shapeNotes for reviewer
Two divisibility rules disagree, and I left them that way. The API validates against panel counts (
chain_length/parallel) while the runtime validates against physical pixels (cols*chain_length,rows*parallel,display_manager.py:128-143). Sochain_length=3, copies=2is rejected by the API but the runtime would accept it — 192 px / 2 gives a 96 px logical screen spanning 1.5 panels. The API's rule is the stricter and more physically meaningful one, so I kept both as they are, but it's worth a second opinion on whether they should be unified.The duplicate toast is still there.
web_interface/static/v3/app.js:41-51has a globalhtmx:afterRequestlistener that already parsesresponseTextcorrectly, so any response carrying amessagenow produces two toasts. Repairing the form handler in place rather than deleting it was a deliberate call — it's still the only thing that reports 2xx responses whose body has nomessage— but deleting it would collapse both toasts into one if you'd prefer that.The same
responseJSONbug lives elsewhere.web_interface/templates/v3/partials/durations.html:14has the identical handler, andweb_interface/static/v3/js/app-shell.jsreadsxhr.responseJSONin four places. Left alone to keep this diff scoped to the reported issue.Generated by Claude Code
Summary by CodeRabbit
New Features
Bug Fixes