Web UI: mobile navigation, guided onboarding, basic/advanced config tiering + performance - #417
Merged
Merged
Conversation
…2,300 lines) Plugin config forms have been rendered server-side (plugin_config.html via GET /partials/plugin-config/<id>) since the HTMX migration; the old client-side generator survived as unreachable code. Verified dead by call graph, not by naming: showPluginConfigModal and showGithubTokenInstructions have zero callers anywhere in templates or JS, and everything removed here is reachable only from those two roots. Removed: - plugins_manager.js: showPluginConfigModal, generatePluginConfigForm, generateFormFromSchema, generateFieldHtml, generateSimpleConfigForm, handlePluginConfigSubmit, the modal's JSON-editor view (initJsonEditor, switchPluginConfigView, syncFormToJson/JsonToForm, saveConfigFromJsonEditor, resetPluginConfigToDefaults, displayValidationErrors, closePluginConfigModal, savePluginConfiguration, currentPluginConfigState), their exclusive helpers (getSchemaPropertyType, escapeCssSelector, dotToNested, collectBooleanFields, normalizeFormDataForConfig, flattenConfig, loadCustomHtmlWidget), the orphaned-modal cleanup block, the modal's listener wiring, and the never-invoked showGithubTokenInstructions/closeInstructionsModal pair. - plugins.html: the #plugin-config-modal markup those functions drove. - base.html: the deprecated pluginConfigData() component and the window.PluginConfigHelpers shim (only ever called by pluginConfigData). Deliberately kept, verified still live: - renderArrayObjectItem, getSchemaProperty, escapeHtml/escapeAttribute (window-exposed for the top-level array-of-objects handlers the server-rendered form uses), toggleNestedSection, addKeyValuePair/ addArrayObjectItem families, executePluginAction, and window.currentPluginConfig = null init (file-upload.js and executePluginAction read it, optional-chained). - app()'s internal generateConfigForm/generateSimpleConfigForm methods in base.html: unreachable now but embedded in the live Alpine component; excising methods from a live object is deferred to keep this change zero-risk. Validation: every deletion seam inspected line-by-line; Jinja parse of both templates passes; repo-wide sweep confirms zero remaining references to any deleted function or element id (deleted ranges contained no Jinja tags). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
Phones previously got the desktop layout squeezed: ~12 system tabs plus one tab per installed plugin wrapped into many rows of small pill buttons, and the header's settings-search and system-stats widgets were dropped entirely (hidden below their breakpoints, never relocated). - Off-canvas nav drawer below md: the existing nav markup (system tab row + #plugin-tabs-row, including dynamically injected plugin tabs) is wrapped in a #site-nav container that CSS repositions into a slide-in drawer on small screens. Same DOM nodes, same @click handlers, nothing duplicated. Tabs become full-width rows with 44px+ touch targets. A hamburger button (md:hidden) in the header and a backdrop toggle the new mobileNavOpen Alpine state (added to both app() definitions, mirroring activeTab). Clicking any tab, a search result, or the backdrop closes the drawer. At md+ hard CSS guards make all drawer styles inert - desktop renders exactly as before. - Header widgets relocated, not hidden: placeHeaderWidgets() in app.js moves the #settings-search-wrap and #system-stats nodes (same elements, listeners intact - both are looked up by id from SSE/search code, so they must never be duplicated) into the drawer below md and back into the header above it, via a matchMedia listener. - Fixed 13 breakpoint utility classes that templates referenced but app.css never defined (sm:block, sm:grid-cols-2, sm:text-sm, md:block, md:w-auto, lg:block, lg:flex, lg:w-64, xl:grid-cols-2/3, 2xl:grid-cols-2/3/4). This was a live bug: 'hidden sm:block' on the search box and 'hidden lg:flex' on the stats meant BOTH were invisible at every screen width. Audit method (repeatable): diff classes used in templates vs defined in app.css. - Mobile modal sizing: one global rule caps .modal-content at 95vw/90vh with internal scroll below 640px - covers every modal without per-template changes. - Horizontal-scroll affordance: pure-CSS edge-fade shadows on .overflow-x-auto containers (scrolling-shadows technique), plus larger in-table touch targets below md. Validation: breakpoint used-vs-defined audit now returns zero gaps; Jinja parse of base.html passes; all changes to desktop behavior are additive (new utilities) or scoped inside max-width media queries. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
… collapsed Advanced Settings section Plugin config pages show every schema property at equal visual priority, which overwhelms first-time users. Plugin authors can now add "x-advanced": true to any flat (non-object) property in config_schema.json to move it into one collapsed "Advanced Settings (N)" section rendered after the basic fields - progressive disclosure with zero loss of control. Implementation: the main render loop in plugin_config.html splits ordered properties into basic/advanced tiers; the advanced group reuses the exact .nested-section/.nested-content/toggleSection() shell that nested object sections already use, so the settings search's expand-on-match behavior works on advanced fields with no JS changes. Object-type properties ignore the flag (they already render as their own collapsible sections). No backend change needed: jsonschema ignores unknown x-* keywords exactly as it does for x-widget/x-propertyOrder. Documented in docs/widget-guide.md alongside the other x-* extensions. Validation (rendered with real Jinja, not just parsed): - synthetic schema with 2 advanced fields: basic fields render before the section, advanced inside the collapsed shell, count badge correct, x-advanced on an object property correctly ignored - schema without any x-advanced: output is identical to the pre-change template (whitespace-normalized diff against git HEAD's version) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
…ion readout The Hardware Configuration card showed ~17 fields at equal priority; a new user only needs 7 of them to get a correctly-sized, correctly-colored image (rows, cols, chain_length, parallel, brightness, hardware_mapping, led_rgb_sequence). The other 10 (multiplexing, panel_type, row_address_type, gpio_slowdown, rp1_rio, scan_mode, pwm_bits, pwm_dither_bits, pwm_lsb_nanoseconds, limit_refresh_rate_hz) now live in a collapsed "Advanced Hardware Settings" section using the same nested-section shell as plugin config forms, so toggleSection() and settings-search auto-expand work unchanged. led_rgb_sequence moved up beside brightness/hardware_mapping (2-col grid became 3-col). No field was removed or renamed; the form still posts the same names to /api/v3/config/main. Also adds a live "Your display: W x H pixels" readout under the four sizing fields (width = cols x chain_length, height = rows x parallel - the exact math the chain-length tooltip describes in prose), recomputed client-side on every input event, no round-trip. Deviation from plan, deliberate: disable_hardware_pulsing / inverse_colors / show_refresh_rate stay in their separate "Display Options" card rather than moving across cards - relocating fields between form sections risks regressions for no decluttering gain in the card users complained about. Validation (real Jinja render): all 17 hardware fields present exactly once, basic fields render before the advanced section and the 10 advanced fields inside it, div count balanced (71/71), readout + recompute script present. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
Getting a plugin onto the display used to take three disconnected manual
steps: install from the store, flip its enable toggle, then restart the
display service - with no in-UI hint that steps 2 and 3 were needed (only
docs/GETTING_STARTED.md mentions it).
- installPlugin() now enables the plugin immediately on successful install
(owner-confirmed behavior change: always auto-enable, no opt-out; users
who don't want it running toggle it off as before), then shows a
persistent toast ("... restart the display to show it") with an inline
"Restart Now" button wired to the existing restartDisplay() - the same
function the three existing Restart Display buttons call.
- notification.js: show() accepts optional { actionLabel, onAction } to
render one inline action button per toast. Callbacks are stored per
notification id and cleaned up on dismiss; a new triggerAction() public
method runs the callback and dismisses. The global showNotification()
shorthand now forwards a full options object as its second argument
(legacy type-string calls unchanged).
Scope note: applies to the plugin store's install path (window.installPlugin).
The custom-registry install path keeps its existing behavior.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
New users land on a dense multi-tab dashboard with no suggested order of
operations (the only guided flow is the WiFi captive portal). This adds a
non-gating checklist card at the top of Overview with five steps, each a
deep link that switches to the right tab (and closes the mobile nav drawer):
1. Set panel size -> Display tab (done: rows/cols/chain_length > 0)
2. Set timezone/location -> General tab (done: differs from template
defaults America/New_York / Tampa)
3. Install a plugin -> Plugins tab (done: /api/v3/plugins/installed
non-empty)
4. Enable a plugin -> Plugins tab (done: any installed plugin enabled)
5. Configure it -> Plugins tab (done: first enabled plugin has >=1
saved value differing from its
schema defaults)
Steps 1-2 are computed server-side in Jinja from main_config (already in the
partial's context); 3-5 client-side from existing endpoints. No new backend
state: dismissal persists in localStorage (mirroring the reconciliation
banner's sessionStorage pattern one section up); deep links use the same
_x_dataStack app-data access as settings-search.js. Disclosed heuristic
limit: values left at legitimate defaults (a user actually in Tampa) read
as "not done".
Validation: real Jinja render across 3 config variants confirms the
server-side done-flags flip correctly; div balance intact; /plugins/config
response shape (config dict directly in .data) verified against api_v3.py.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
…splay mode The primary rotation's order was invisible and unconfigurable: modes are registered in parallel-load COMPLETION order, so rotation order actually varied between restarts. Only the niche Vegas Scroll mode had a working order UI. This adds real, persisted ordering end to end: Backend: - config.template.json: new display.plugin_rotation_order (default [], fully backward compatible). - display_controller.py: _apply_plugin_rotation_order() rebuilds available_modes grouped by plugin per the configured list (each plugin's modes keep their declared order; unlisted plugins follow in existing relative order; empty config = exact no-op). Applied at startup after parallel load and after live enable/disable reconcile (before the existing _resync_mode_index_after_change, which preserves the current mode). Mirrors vegas_mode get_ordered_plugins() semantics. - api_v3.py save_main_config: accepts plugin_rotation_order as a JSON array (same parse/guard pattern as vegas_plugin_order). Frontend: - New shared widget static/v3/js/widgets/plugin-order-list.js: the Vegas section's drag-and-drop list factored out verbatim (native HTML5 drag events, saved-order-first rendering, hidden-input JSON sync), parameterized by container/order-input/optional exclude-checkbox/badge. - display.html: Vegas section now calls the shared module; its ~130-line inline copy of the same logic is deleted. - durations.html: new "Rotation Order" card above the durations grid using the same module, posting plugin_rotation_order with the existing form. Deviation from plan, deliberate: durations stay as their own mode-keyed grid rather than inline in the drag rows - verified display_durations keys are MODE names (display_controller.py resolves duration per mode_key), not plugin ids, and one plugin can own several modes, so the planned 1:1 inline pairing was wrong. Validation: py_compile on both Python files; _apply_plugin_rotation_order unit-tested standalone (configured order applied, empty-config no-op, unknown ids skipped - 3/3); both templates render with balanced divs, the hidden input carries the saved order, and the old inline implementation is confirmed gone; config.template.json parses. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
The user-visible URL no longer carries the interface version: the pages
blueprint is now registered un-prefixed (primary) AND at /v3 (second
registration, name='pages_v3_legacy'), so:
- http://<device>/ serves the interface directly (the old @app.route('/')
redirect is removed — the blueprint's own index takes its place)
- every existing /v3/... bookmark and all the hardcoded /v3/partials/...
fetches in templates/JS keep working verbatim through the alias mount —
zero template/JS churn, zero broken links
- url_for('pages_v3.*') resolves against the primary registration, so all
server-side redirects (captive portal detection endpoints) now emit
un-prefixed URLs
- the AP-mode captive-portal allowlist learned the un-prefixed page paths
(/setup, /partials/, /settings/, /plugin-ui/) so setup-mode requests
don't redirect-loop
- /api/v3 and the templates/v3, static/v3 directories are deliberately
untouched (internal, invisible to users; owner-confirmed scope)
Validation: dual registration mechanics tested against real Flask (test
client): /, /v3, /v3/ redirect, partials and /setup reachable on both
mounts, url_for yields un-prefixed paths; py_compile passes.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
display_preview_generator() opened each changed snapshot with PIL and re-encoded it to PNG just to base64 it — but /tmp/led_matrix_preview.png already IS a PNG, written atomically by the display service (tmp file + os.replace in display_manager.py), so a partially-written file can never be observed. Read the bytes and base64 them directly: identical payload (front-end consumes data:image/png;base64 — verified in base.html), one full image decode+encode per frame less on the same Pi that's driving the matrix. The existing mtime skip and viewer-marker throttling are unchanged (they already covered the "skip unchanged frames" concern). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
The interface ships a ~5,000-line HTML shell and >20k lines of JS uncompressed; on phone/WiFi that dominates load time. Flask-Compress gzips/brotlis compressible responses transparently. - Optional dependency, same graceful pattern as flask-limiter: missing package = uncompressed responses, no crash. - SSE safety verified empirically against the real package (1.24): an actual streamed text/event-stream response comes back with no Content-Encoding while a large HTML response gzips — the display preview / stats / logs streams are unaffected. - Added flask-compress>=1.14 to web_interface/requirements.txt. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
…ug switch plugins_manager.js, base.html's inline scripts, and app.js emitted 198 console.log calls in production - including per-interaction [DEBUG] dumps - costing main-thread time and drowning real errors in noise. - New window.debugLog() gate defined in base.html's first inline script (before any other script runs): forwards to console.log only when localStorage.pluginDebug === 'true' - the SAME switch plugins_manager.js already used for its _PLUGIN_DEBUG_EARLY logs, so existing debug workflow docs stay valid. Exposed as window.LEDMATRIX_DEBUG for other scripts. - Mechanically rewrote console.log( -> debugLog( in plugins_manager.js (127), base.html (64), app.js (7). Verified no occurrences lived inside string literals before rewriting; console.error/console.warn untouched. - app.js's no-Alpine showNotification fallback restored to console.info - it's a user-facing last resort, not debug output. Both load paths are safe: the gate is the first inline <script> in <head>, and every rewritten file loads deferred after it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
…-capable Font Awesome, CodeMirror, and htmx were fetched from cdnjs/unpkg on every fresh page load, adding third-party round-trips on a device that often lives on a local network (and htmx was local-only in AP mode, meaning two different loading behaviors to reason about). - Vendored pinned copies under static/v3/vendor/: Font Awesome 6.0.0 (css/all.min.css + the 8 webfonts it references relatively) and CodeMirror 5.65.2 (core, javascript mode, closebrackets/matchbrackets addons, base + monokai css) - ~1.1 MB total, exact versions the CDN tags pinned. - htmx + sse + json-enc extensions now load from the existing local copies (verified 1.9.10, matching the CDN pin) on EVERY network, not just AP mode; the pinned CDN copies remain as a one-shot rescue fallback, mirroring the pattern Alpine already used. The convoluted isAPMode source-flipping logic collapses away. - Dropped the CDN preconnect/dns-prefetch hints (no longer on the critical path). - Fixed a latent bug while relinking CodeMirror: the loader requested mode/json/json.min.js, which does not exist on cdnjs (HTTP 404 verified) - it 404'd on every JSON-editor open. JSON highlighting comes from the javascript mode; the phantom entry is removed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
…le static files
base.html shipped ~4,200 lines of inline JavaScript inside the HTML
document, re-downloaded and re-parsed on every page load (gzip helps the
transfer, but inline scripts can never be browser-cached). The four
largest blocks - none containing any Jinja syntax, verified by scanning
every inline block for {{ }} / {% %} - now live as static files served
with the app's existing mtime-versioned immutable caching:
- js/htmx-config.js (246 lines): HTMX swap/script-execution config,
toggleSection helpers
- js/app-early.js (346 lines): early helpers + the app() stub that must
precede Alpine init
- js/app-shell.js (2,997 lines): SSE wiring + the full Alpine app()
implementation and tab logic
- js/custom-feeds-helpers.js (262 lines): custom-feeds table helpers
Each replacement <script src> is CLASSIC (no defer/async) at the exact
position of the inline block it replaces - identical execution timing and
DOM visibility to inline scripts, so relative ordering with the deferred
scripts and with each other is unchanged. base.html drops from ~4,940 to
1,079 lines.
Validation: extraction proven lossless by programmatically reassembling
the four files back into the template and comparing against git HEAD -
byte-for-byte identical. Jinja parse passes; script open/close tags
balanced (53/53, after excluding a literal "<script>" inside an HTML
comment).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds configurable plugin rotation ordering, modularizes the v3 web application, introduces responsive navigation and live preview behavior, expands plugin configuration UI, adds optional compression and local asset loading, and adds web smoke and static-audit tests. ChangesPlugin rotation ordering
Modular application bootstrap
Plugin management and notifications
Responsive navigation and delivery
Settings and supporting UI
Web validation and maintenance
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| ErrorProne | 6 high |
🟢 Metrics 232 complexity · 4 duplication
Metric Results Complexity 232 Duplication 4
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
…onfig The initial SSE render sized the preview image (and both overlay canvases) from the server-reported config dimensions (cols x chain_length, rows x parallel), while the scale slider's re-render path sized from img.naturalWidth/naturalHeight. Whenever the snapshot PNG's actual size disagrees with the config (stale config, display service not restarted after a hardware change), the initial render stretched the image at a fractional ratio - blurry despite image-rendering: pixelated - and touching the scale slider "fixed" it. Reported live on the devpi test rig. Both paths now size from the loaded image's natural dimensions inside img.onload (which also removes a transient wrong-size flash between src assignment and load). The meta label now reports the true snapshot size. The preview card also gets overflow-x-auto so on narrow screens a wide preview scrolls at its exact pixel-perfect size instead of being squeezed into the viewport (fractional downscaling of pixel art also reads as blur). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
…ve Double-Sided Owner-requested layout refinement of the Display Settings tab: - The "Display Options" card (disable_hardware_pulsing, inverse_colors, show_refresh_rate, use_short_date_format, Dynamic Duration) moves inside the collapsed advanced section, now titled "Advanced Hardware & Display Options (15)". Hidden form fields still submit with the form, and settings search still auto-expands the section on match, so nothing is lost - the tab just leads with the essentials. - The "Vegas Scroll Mode" section moves above "Double-Sided Display". New section order: Hardware (+ advanced dropdown) > Vegas Scroll > Double-Sided > Multi-Display Sync. Validation (real Jinja render): all 23 field names present exactly once, divs balanced (70/70), the five Display Options fields render inside the advanced section's bounds, and section markers confirm the new order. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
…des when complete Two gaps reported from live testing on the devpi rig: 1. The timezone/location step never showed done for a user whose real timezone IS the shipped default (America/New_York) - the heuristic can only detect difference-from-default, not "user saved this". Clicking an item's checkbox now toggles it done manually (persisted per browser in localStorage), so any heuristic false-negative is one tap to clear. Clicking the item text still deep-links to its tab. 2. The card now hides itself automatically once every step is done (auto-detected or manually checked) - previously it stayed until the X was clicked. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (9)
web_interface/static/v3/js/custom-feeds-helpers.js-95-104 (1)
95-104: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGive the icon-only remove button an accessible name.
const removeButton = document.createElement('button'); removeButton.type = 'button'; +removeButton.setAttribute('aria-label', 'Remove feed');🤖 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/static/v3/js/custom-feeds-helpers.js` around lines 95 - 104, Add an accessible name to the icon-only removeButton created in the custom feed row generation flow, using an appropriate aria-label or equivalent button text while preserving its existing click handler and visual icon.web_interface/app.py-685-686 (1)
685-686: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winCatch only expected snapshot I/O errors.
Catching
Exceptionsilently hides programming failures. Limit this to transient filesystem errors and retain debug context.- except Exception: # nosec B110 - transient read error (e.g. file rotated away); skip this update - pass + except OSError: + app.logger.debug( + "[Display Preview] Snapshot read failed; retrying", + exc_info=True, + )As per coding guidelines, catch specific exceptions and include contextual logging with stack traces.
🤖 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/app.py` around lines 685 - 686, Update the exception handler around the snapshot read to catch only expected transient filesystem/I/O exceptions instead of broad Exception. Replace the silent pass with contextual debug logging that includes the exception and stack trace, while preserving the behavior of skipping the failed update.Sources: Coding guidelines, Linters/SAST tools
web_interface/app.py-69-70 (1)
69-70: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winLog when response compression is unavailable.
The silent fallback makes degraded Pi deployments difficult to diagnose. Emit one structured warning with the installation remedy.
except ImportError: - pass + app.logger.warning( + "[Web Interface] Response compression disabled; install flask-compress" + )As per coding guidelines, implement comprehensive structured logging and provide clear troubleshooting messages.
🤖 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/app.py` around lines 69 - 70, Update the ImportError handler around the response-compression import in web_interface/app.py to emit one structured warning instead of silently passing. Include that compression is unavailable and provide the installation remedy, while preserving the fallback behavior.Source: Coding guidelines
web_interface/static/v3/js/custom-feeds-helpers.js-252-261 (1)
252-261: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReset the file input after failed uploads too.
After a network/server failure, selecting the same file again will not fire
change. Move the reset intofinally().- event.target.value = ''; } else { alert('Upload failed: ' + (data.message || 'Unknown error')); } }) .catch(error => { console.error('Upload error:', error); alert('Upload failed: ' + error.message); + }) + .finally(() => { + event.target.value = ''; });🤖 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/static/v3/js/custom-feeds-helpers.js` around lines 252 - 261, Move the file-input reset from the successful upload branch into a finally() handler on the upload promise chain, using the existing event.target reference so it runs after both success and failure while preserving the current error logging and alerts.web_interface/templates/v3/partials/durations.html-65-74 (1)
65-74: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winBound the widget-loading retry and surface failure.
If
PluginOrderListnever loads, this schedules ten callbacks per second indefinitely while leaving “Loading plugins…” visible. Stop after a short timeout and show a refresh/troubleshooting message; move this bootstrap into the extracted cacheable JavaScript bundle.🤖 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/templates/v3/partials/durations.html` around lines 65 - 74, Update initRotationOrderList so PluginOrderList loading retries are bounded by a short timeout, then replace the “Loading plugins…” state with a refresh/troubleshooting message when loading fails. Move this bootstrap logic into the extracted cacheable JavaScript bundle while preserving initialization once window.PluginOrderList becomes available.web_interface/static/v3/js/widgets/plugin-order-list.js-116-123 (1)
116-123: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate parsed values are arrays.
Valid JSON such as
{}ornullbypasses the catch, thencurrentOrder.forEach()throws and replaces the widget with an error. Normalize both values withArray.isArray().🤖 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/static/v3/js/widgets/plugin-order-list.js` around lines 116 - 123, Update the saved-order parsing in the plugin order initialization block to validate both parsed values with Array.isArray(). Normalize currentOrder and excluded to empty arrays when JSON parses successfully but returns null, an object, or any non-array value, while preserving valid arrays and the existing parse-error handling.web_interface/templates/v3/partials/display.html-644-650 (1)
644-650: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winBound the widget-loading retry.
If the script fails to load, this schedules callbacks at 10 Hz indefinitely. Cap the attempts and replace the loading message with a clear reload/error action.
🤖 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/templates/v3/partials/display.html` around lines 644 - 650, Update initPluginOrderList to cap deferred retries when window.PluginOrderList remains unavailable, tracking attempts and stopping after a reasonable limit instead of scheduling indefinitely. Once retries are exhausted, replace the loading message with a clear error state and reload action while preserving normal initialization when the widget loads.web_interface/templates/v3/partials/display.html-138-149 (1)
138-149: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExpose the expanded state to assistive technology.
Add
aria-controlsandaria-expandedto this button, then updatearia-expandedinsidetoggleSection()when the region opens or closes.🤖 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/templates/v3/partials/display.html` around lines 138 - 149, Update the advanced hardware section toggle button and toggleSection() to expose state to assistive technology: add aria-controls referencing display-section-advanced-hardware and initialize aria-expanded, then update aria-expanded on the button whenever that section opens or closes. Use the existing toggleSection() behavior and the button’s section relationship without changing visual behavior.web_interface/templates/v3/base.html-293-301 (1)
293-301: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep the hamburger’s accessible name in sync.
Bind the label to state, for example
mobileNavOpen ? 'Close navigation menu' : 'Open navigation menu'.🤖 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/templates/v3/base.html` around lines 293 - 301, The mobile navigation toggle’s accessible label is always “Open navigation menu.” Update the button in the mobile nav toggle markup to bind aria-label to mobileNavOpen, using “Close navigation menu” when open and “Open navigation menu” when closed.
🧹 Nitpick comments (3)
web_interface/templates/v3/partials/overview.html (1)
116-118: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse a more specific selector to resolve Alpine app state.
Using a generic
[x-data]selector might grab the wrong element if there are other Alpine components (e.g., a header or sidebar component) earlier in the DOM, causing tab navigation links to fail. Consider matching the more robust resolution logic utilized insettings-search.js, which prefers the mainapp()scope and gracefully handles Alpine data extraction.♻️ Proposed refactor for robust state resolution
- var appEl = document.querySelector('[x-data]'); - var data = appEl && appEl._x_dataStack && appEl._x_dataStack[0]; + var appEl = document.querySelector('[x-data="app()"]') || document.querySelector('[x-data]'); + var data = null; + if (appEl) { + if (appEl._x_dataStack && appEl._x_dataStack[0]) data = appEl._x_dataStack[0]; + else if (appEl.__x && appEl.__x.$data) data = appEl.__x.$data; + }🤖 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/templates/v3/partials/overview.html` around lines 116 - 118, Update the Alpine state resolution near the appEl/data initialization to target the main app() scope instead of the generic [x-data] selector, following the robust resolution approach used by settings-search.js. Preserve graceful handling when Alpine state cannot be extracted so tab navigation continues to work with other components present.src/display_controller.py (1)
2854-2855: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefix the new rotation logs with
[DisplayController].This keeps remote Raspberry Pi diagnostics attributable when multiple managers share the same log.
Proposed fix
-logger.info("Plugin reconcile complete: +%s -%s (%d modes)", +logger.info("[DisplayController] Plugin reconcile complete: +%s -%s (%d modes)", sorted(to_add), sorted(to_remove), len(self.available_modes)) -logger.info("Applied plugin rotation order %s -> modes: %s", +logger.info("[DisplayController] Applied plugin rotation order %s -> modes: %s", configured, self.available_modes)As per coding guidelines,
src/*.pymust use structured logging prefixes for consistent identification of log sources.Also applies to: 2885-2886
🤖 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 `@src/display_controller.py` around lines 2854 - 2855, Update the rotation-related log messages in the display controller, including the “Plugin reconcile complete” log and the corresponding log near the second referenced location, to begin with the structured “[DisplayController]” prefix. Preserve the existing messages, arguments, and logging levels.Source: Coding guidelines
web_interface/static/v3/js/app-shell.js (1)
1937-1975: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBatch the canvas pixel readback.
When LED mode is enabled, a 128×64 preview performs 8,192 synchronous
getImageData()calls per frame. Read one fullImageDatabuffer and index it, ideally reusing the offscreen canvas.🤖 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/static/v3/js/app-shell.js` around lines 1937 - 1975, Batch pixel readback in the LED rendering loop around the offscreen canvas setup: call offCtx.getImageData once for the full logicalWidth × logicalHeight buffer, then index its data array using each pixel’s RGBA offset inside the existing x/y loop. Reuse the existing offscreen canvas where possible, while preserving transparency/black filtering and dot rendering behavior.
🤖 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/blueprints/api_v3.py`:
- Around line 966-978: The plugin_rotation_order handling in
web_interface/blueprints/api_v3.py lines 966-978 must enforce a nested display
value containing only plugin-ID strings: remove the handled key before generic
merging, validate that the parsed value is a list of strings, and return a
descriptive 400 response for invalid input. In src/display_controller.py lines
2868-2872, defensively validate manually edited or migrated configuration and
retain the existing rotation order when plugin_rotation_order is invalid; update
the relevant consumption logic without changing valid-order behavior.
In `@web_interface/static/v3/js/app-shell.js`:
- Around line 1590-1602: The dotToNested function must prevent prototype
pollution while converting dotted keys. Reject any path segment named __proto__,
prototype, or constructor, and create result and intermediate nested objects
with Object.create(null); apply the same protections to the corresponding
implementation around the additional referenced location.
- Around line 2728-2731: Update window.executePluginAction to accept arguments
in the caller order (actionId, index, pluginId), remove the unused index from
its signature, and use actionId and pluginId when constructing the fetch
request. Ensure callers and the handler consistently map each identifier to the
correct query parameter.
- Around line 1476-1489: Remove the manual HTML entity replacement block in the
JSON parsing flow and call JSON.parse directly with the original value. Apply
the same change to both occurrences, including the corresponding logic near the
other reported location, preserving the existing parsing behavior without
additional decoding.
- Around line 2867-2872: Replace uses of the `app` factory in the uninstall
success and related paths with the Alpine component instance returned by
`getAppComponent()`. Update calls and property access for
`loadInstalledPlugins`, `activeTab`, and `showNotification` across the
referenced branches, preserving their existing behavior while ensuring they
operate on the component instance.
- Around line 344-356: Update the installedPlugins setter in the
window.installedPlugins definition to always replace _installedPluginsValue and
the component’s installedPlugins state with the incoming plugin list, including
same-ID updates to metadata or enabled state. Keep the old/new ID comparison
only for conditionally calling updatePluginTabs and related change logging,
rather than gating state assignment.
- Around line 788-942: Update generateFieldHtml and the additional
form-generation blocks to prevent plugin-controlled schema data, keys, values,
enum entries, and action metadata from being interpolated into HTML or inline
event handlers. Build elements and attributes through DOM APIs with
addEventListener, or apply a vetted context-aware sanitizer/escaping strategy
that safely handles text, attribute, and JavaScript contexts while preserving
current form behavior.
In `@web_interface/static/v3/js/custom-feeds-helpers.js`:
- Around line 158-177: The asset upload flow must match the backend contract:
update the FormData field in the upload handler to use files instead of file,
and read the successful response from uploaded_files instead of data.files.
Preserve the existing status validation and first-uploaded-file handling.
In `@web_interface/static/v3/js/htmx-config.js`:
- Around line 123-152: Update the form-submission logging in the HTMX response
error handler to stop emitting complete form values. Replace formPayload
construction and logging with field names plus redacted metadata only, ensuring
sensitive values such as API keys, passwords, and credentials are never written
to the console.
- Around line 55-115: Remove the global console.error and console.warn overrides
around originalError/originalWarn. Replace broad substring-based suppression
with narrowly scoped handling tied to HTMX event/error hooks, suppressing only
confirmed HTMX-generated messages while preserving console diagnostics for
unrelated DOM, malformed-HTML, and browser warnings.
- Around line 163-190: Remove the htmx:afterSwap event listener that clones and
reinserts scripts, including its nested script-processing logic. Rely on HTMX’s
default execution of swapped script tags and leave unrelated HTMX configuration
unchanged.
In `@web_interface/static/v3/js/widgets/plugin-order-list.js`:
- Around line 59-103: The setupDragAndDrop function only supports native
dragging, so add accessible touch and keyboard reordering for each
.plugin-order-item, such as move-up and move-down controls or equivalent
pointer/keyboard handlers. Ensure each reorder updates the container order and
calls syncInputs() immediately, while preserving the existing drag-and-drop
behavior.
In `@web_interface/static/v3/plugins_manager.js`:
- Around line 3891-3903: Update togglePlugin to return its asynchronous request
promise, then chain the post-install success notification from that promise so
it is shown only after enablement resolves successfully. Keep the installation
flow’s immediate togglePlugin(pluginId, true) call, but move the enabled message
and restartDisplay action into the resolved-success path; preserve failure
handling without offering restart when enablement fails.
---
Minor comments:
In `@web_interface/app.py`:
- Around line 685-686: Update the exception handler around the snapshot read to
catch only expected transient filesystem/I/O exceptions instead of broad
Exception. Replace the silent pass with contextual debug logging that includes
the exception and stack trace, while preserving the behavior of skipping the
failed update.
- Around line 69-70: Update the ImportError handler around the
response-compression import in web_interface/app.py to emit one structured
warning instead of silently passing. Include that compression is unavailable and
provide the installation remedy, while preserving the fallback behavior.
In `@web_interface/static/v3/js/custom-feeds-helpers.js`:
- Around line 95-104: Add an accessible name to the icon-only removeButton
created in the custom feed row generation flow, using an appropriate aria-label
or equivalent button text while preserving its existing click handler and visual
icon.
- Around line 252-261: Move the file-input reset from the successful upload
branch into a finally() handler on the upload promise chain, using the existing
event.target reference so it runs after both success and failure while
preserving the current error logging and alerts.
In `@web_interface/static/v3/js/widgets/plugin-order-list.js`:
- Around line 116-123: Update the saved-order parsing in the plugin order
initialization block to validate both parsed values with Array.isArray().
Normalize currentOrder and excluded to empty arrays when JSON parses
successfully but returns null, an object, or any non-array value, while
preserving valid arrays and the existing parse-error handling.
In `@web_interface/templates/v3/base.html`:
- Around line 293-301: The mobile navigation toggle’s accessible label is always
“Open navigation menu.” Update the button in the mobile nav toggle markup to
bind aria-label to mobileNavOpen, using “Close navigation menu” when open and
“Open navigation menu” when closed.
In `@web_interface/templates/v3/partials/display.html`:
- Around line 644-650: Update initPluginOrderList to cap deferred retries when
window.PluginOrderList remains unavailable, tracking attempts and stopping after
a reasonable limit instead of scheduling indefinitely. Once retries are
exhausted, replace the loading message with a clear error state and reload
action while preserving normal initialization when the widget loads.
- Around line 138-149: Update the advanced hardware section toggle button and
toggleSection() to expose state to assistive technology: add aria-controls
referencing display-section-advanced-hardware and initialize aria-expanded, then
update aria-expanded on the button whenever that section opens or closes. Use
the existing toggleSection() behavior and the button’s section relationship
without changing visual behavior.
In `@web_interface/templates/v3/partials/durations.html`:
- Around line 65-74: Update initRotationOrderList so PluginOrderList loading
retries are bounded by a short timeout, then replace the “Loading plugins…”
state with a refresh/troubleshooting message when loading fails. Move this
bootstrap logic into the extracted cacheable JavaScript bundle while preserving
initialization once window.PluginOrderList becomes available.
---
Nitpick comments:
In `@src/display_controller.py`:
- Around line 2854-2855: Update the rotation-related log messages in the display
controller, including the “Plugin reconcile complete” log and the corresponding
log near the second referenced location, to begin with the structured
“[DisplayController]” prefix. Preserve the existing messages, arguments, and
logging levels.
In `@web_interface/static/v3/js/app-shell.js`:
- Around line 1937-1975: Batch pixel readback in the LED rendering loop around
the offscreen canvas setup: call offCtx.getImageData once for the full
logicalWidth × logicalHeight buffer, then index its data array using each
pixel’s RGBA offset inside the existing x/y loop. Reuse the existing offscreen
canvas where possible, while preserving transparency/black filtering and dot
rendering behavior.
In `@web_interface/templates/v3/partials/overview.html`:
- Around line 116-118: Update the Alpine state resolution near the appEl/data
initialization to target the main app() scope instead of the generic [x-data]
selector, following the robust resolution approach used by settings-search.js.
Preserve graceful handling when Alpine state cannot be extracted so tab
navigation continues to work with other components present.
🪄 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
Run ID: 0b852b3e-c895-4725-87dd-0e8debfc7ec6
⛔ Files ignored due to path filters (15)
web_interface/static/v3/vendor/codemirror/addon/edit/closebrackets.min.jsis excluded by!**/*.min.jsweb_interface/static/v3/vendor/codemirror/addon/edit/matchbrackets.min.jsis excluded by!**/*.min.jsweb_interface/static/v3/vendor/codemirror/codemirror.min.cssis excluded by!**/*.min.cssweb_interface/static/v3/vendor/codemirror/codemirror.min.jsis excluded by!**/*.min.jsweb_interface/static/v3/vendor/codemirror/mode/javascript/javascript.min.jsis excluded by!**/*.min.jsweb_interface/static/v3/vendor/codemirror/theme/monokai.min.cssis excluded by!**/*.min.cssweb_interface/static/v3/vendor/fontawesome/css/all.min.cssis excluded by!**/*.min.cssweb_interface/static/v3/vendor/fontawesome/webfonts/fa-brands-400.ttfis excluded by!**/*.ttfweb_interface/static/v3/vendor/fontawesome/webfonts/fa-brands-400.woff2is excluded by!**/*.woff2web_interface/static/v3/vendor/fontawesome/webfonts/fa-regular-400.ttfis excluded by!**/*.ttfweb_interface/static/v3/vendor/fontawesome/webfonts/fa-regular-400.woff2is excluded by!**/*.woff2web_interface/static/v3/vendor/fontawesome/webfonts/fa-solid-900.ttfis excluded by!**/*.ttfweb_interface/static/v3/vendor/fontawesome/webfonts/fa-solid-900.woff2is excluded by!**/*.woff2web_interface/static/v3/vendor/fontawesome/webfonts/fa-v4compatibility.ttfis excluded by!**/*.ttfweb_interface/static/v3/vendor/fontawesome/webfonts/fa-v4compatibility.woff2is excluded by!**/*.woff2
📒 Files selected for processing (23)
config/config.template.jsondocs/widget-guide.mdsrc/display_controller.pyweb_interface/app.pyweb_interface/blueprints/api_v3.pyweb_interface/requirements.txtweb_interface/static/v3/app.cssweb_interface/static/v3/app.jsweb_interface/static/v3/js/app-early.jsweb_interface/static/v3/js/app-shell.jsweb_interface/static/v3/js/custom-feeds-helpers.jsweb_interface/static/v3/js/htmx-config.jsweb_interface/static/v3/js/settings-search.jsweb_interface/static/v3/js/widgets/notification.jsweb_interface/static/v3/js/widgets/plugin-order-list.jsweb_interface/static/v3/plugins_manager.jsweb_interface/templates/v3/base.htmlweb_interface/templates/v3/captive_setup.htmlweb_interface/templates/v3/partials/display.htmlweb_interface/templates/v3/partials/durations.htmlweb_interface/templates/v3/partials/overview.htmlweb_interface/templates/v3/partials/plugin_config.htmlweb_interface/templates/v3/partials/plugins.html
💤 Files with no reviewable changes (1)
- web_interface/templates/v3/partials/plugins.html
… order, drop v3 from UI branding - Add debugLog to the /* global */ headers of the six JS files that call it (defined in base.html's first inline script) — resolves the wall of "'debugLog' is not defined" ESLint errors failing the Codacy check. - Fix the two js/double-escaping CodeQL alerts in app-shell.js: the entity-unescape chains decoded & before </>, so a value containing a pre-escaped "&lt;" wrongly double-decoded to "<". & now decodes last (standard order). Pre-existing bug, made visible when the inline scripts moved into scannable .js files. - Page title / header drop the "- v3" suffix, matching the de-versioned user-facing URL. The remaining 7 CodeQL alerts are pre-existing patterns newly visible to scanning (CodeQL doesn't see inline template JS): 4 github.com/htmx.org URL-substring checks (the htmx ones match error-message text, not URLs — false positives in context) and 1 innerHTML XSS-through-DOM in the GitHub install flow. Triage/fix deferred to a focused follow-up rather than expanding this PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
The durations partial (/partials/durations) has existed as a route with no nav tab and no content panel referencing it - an orphaned page. That made the new rotation-order UI unreachable through the interface (caught by the owner testing on the rig; my endpoint-level tests fetched the partial by URL and never noticed the missing entry point). - New "Rotation" tab (fa-rotate icon, verified present in the vendored FA 6.0.0 css) between Display and Backup & Restore, wired exactly like the other tabs (#durations-content + hx-get + loadtab; loadTabContent() is fully generic, so no JS changes needed). - Page heading updated from "Display Durations" to "Rotation & Durations" to match its content since the rotation-order card landed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
The durations grid looped over display.display_durations, which nothing has
ever populated (verified {} on a real production install) - so the page
rendered no duration fields at all. Worse, its inputs posted bare mode
names, which save_main_config's endswith('_duration') filter silently
dropped: the page was broken in both directions, unnoticed because it was
also unreachable (previous commit).
- pages_v3._load_durations_partial now builds one entry per display mode of
every ENABLED plugin via plugin_manager.get_plugin_display_modes()
(falling back to the plugin id), overlaid with saved values, defaulting
to the display controller's 30s. Grouped per plugin, sorted by name.
Saved keys not owned by any enabled plugin stay visible under "Other
saved entries" instead of vanishing.
- durations.html renders the grouped inputs, named duration__<mode_key>
(mode keys are arbitrary, so they can't use the *_duration suffix
convention), with an explanatory empty state when no plugins are enabled.
- api_v3.save_main_config accepts the new duration__<mode> fields and
writes them into display.display_durations under the bare mode key -
exactly what the display controller reads
(display_durations.get(mode_key, 30)).
Validation: py_compile both blueprints; Jinja render with 3 groups asserts
grouped inputs, saved-value overlay, stale-entry group, empty state, and
div balance.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web_interface/static/v3/js/app-shell.js (1)
1229-1236: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAlign the plugin-action handler signature with its callers.
This handler calls
executePluginAction('${action.id}', ${index}, '${safePluginId}'). If the global function expects(pluginId, actionId), these identifiers are passed in the wrong order. Ensure callers and the handler consistently map each identifier to the correct parameter.🤖 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/static/v3/js/app-shell.js` around lines 1229 - 1236, Update the plugin-action button’s executePluginAction call and its global handler signature so pluginId and actionId are passed and interpreted consistently. Ensure the action identifier and plugin identifier map to the correct parameters in every caller without changing the index behavior.
♻️ Duplicate comments (4)
web_interface/static/v3/js/app-shell.js (4)
1481-1487: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winParse the DOM value without decoding entities again.
The HTML parser already decodes attribute entities before
FormDatareads the value. Replacing these entities mutates legitimate strings (such as literal&), which can corrupt stored data. Remove the entity replacement blocks and use the original value directly.
web_interface/static/v3/js/app-shell.js#L1481-L1487: Remove the manual HTML entity replacement block and use thevaluedirectly inJSON.parse().web_interface/static/v3/js/app-shell.js#L2253-L2259: Remove the duplicate HTML entity replacement block here as well.🤖 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/static/v3/js/app-shell.js` around lines 1481 - 1487, Remove the manual HTML entity replacement blocks at web_interface/static/v3/js/app-shell.js#L1481-L1487 and `#L2253-L2259`, and pass the original value directly to JSON.parse() in both sites. Preserve all surrounding parsing behavior without decoding entities a second time.
1591-1605: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReject prototype keys while nesting fields.
A field path such as
__proto__.pollutedtraverses intoObject.prototypeand modifies it globally. Reject__proto__,prototype, andconstructorwhen iterating path segments, and ideally construct nested maps withObject.create(null).
web_interface/static/v3/js/app-shell.js#L1591-L1605: Add a check inside thedotToNestedloop to ignore these reserved keys.web_interface/static/v3/js/app-shell.js#L2340-L2354: Apply the identical prototype pollution protection to this duplicatedotToNestedimplementation.🛠️ Proposed fix
const dotToNested = (obj) => { const result = {}; for (const key in obj) { const parts = key.split('.'); let current = result; for (let i = 0; i < parts.length - 1; i++) { + if (['__proto__', 'constructor', 'prototype'].includes(parts[i])) continue; if (!current[parts[i]]) { current[parts[i]] = {}; } current = current[parts[i]]; } - current[parts[parts.length - 1]] = obj[key]; + const lastPart = parts[parts.length - 1]; + if (!['__proto__', 'constructor', 'prototype'].includes(lastPart)) { + current[lastPart] = obj[key]; + } } return result; };🤖 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/static/v3/js/app-shell.js` around lines 1591 - 1605, Update both duplicate dotToNested implementations at web_interface/static/v3/js/app-shell.js lines 1591-1605 and 2340-2354 to skip path segments named __proto__, prototype, or constructor before traversing or assigning properties. Prefer null-prototype objects for the root and nested maps while preserving normal dot-path nesting behavior.
351-357: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not discard same-ID plugin updates.
The setter ignores updates to
installedPluginsunless the sorted ID list changes. Freshenabledstate and metadata (like commit info) remain stale if a plugin is toggled or updated. Always replace the stored value and component state; only gate the UI tab rerender.🛠️ Proposed fix
- // Only update if plugin list actually changed - if (oldIds !== newIds) { - debugLog('window.installedPlugins changed:', newPlugins.length, 'plugins'); - _installedPluginsValue = newPlugins; - this.installedPlugins = newPlugins; - this.updatePluginTabs(); - } + // Always update state to capture metadata/enabled changes + _installedPluginsValue = newPlugins; + this.installedPlugins = newPlugins; + + // Only re-render tabs if the list of plugins (by ID) changed + if (oldIds !== newIds) { + debugLog('window.installedPlugins changed:', newPlugins.length, 'plugins'); + this.updatePluginTabs(); + }🤖 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/static/v3/js/app-shell.js` around lines 351 - 357, Update the installedPlugins setter around the oldIds/newIds comparison so _installedPluginsValue and this.installedPlugins are always replaced with newPlugins, including same-ID changes to enabled state or metadata. Restrict the oldIds !== newIds condition to calling updatePluginTabs() and its related change-only logging.
789-833: 🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy liftStop interpolating plugin data directly into HTML.
Schema titles, descriptions, keys, config values, enum values, and action metadata enter text, attributes, and inline handlers without context-safe encoding. This can corrupt controls or produce stored DOM XSS.
Sanitize these values before interpolation, for instance by using the component's existing
this.escapeHtml()function for text content. Be especially careful with attributes (whichescapeHtmldoes not fully protect if not quoted properly). Ideally, build these forms with DOM APIs andaddEventListener.🤖 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/static/v3/js/app-shell.js` around lines 789 - 833, Update generateFieldHtml and all related form-rendering paths to context-safely encode plugin-controlled schema titles, descriptions, keys, values, enum labels, and action metadata before inserting them into HTML text, attributes, or inline handlers. Reuse the existing escapeHtml method for text and properly quote and attribute-encode attribute values; avoid direct interpolation into event-handler attributes, preferably replacing them with DOM construction and addEventListener bindings. Ensure generated controls retain their current behavior without allowing stored DOM XSS.
🤖 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`:
- Line 145: Update the description paragraph in the display template to remove
the duplicated wording and restore the malformed em-dash entity to a valid
— entity, leaving one clear, non-repeated description of multiplexing,
panel variants, PWM tuning, and display options.
---
Outside diff comments:
In `@web_interface/static/v3/js/app-shell.js`:
- Around line 1229-1236: Update the plugin-action button’s executePluginAction
call and its global handler signature so pluginId and actionId are passed and
interpreted consistently. Ensure the action identifier and plugin identifier map
to the correct parameters in every caller without changing the index behavior.
---
Duplicate comments:
In `@web_interface/static/v3/js/app-shell.js`:
- Around line 1481-1487: Remove the manual HTML entity replacement blocks at
web_interface/static/v3/js/app-shell.js#L1481-L1487 and `#L2253-L2259`, and pass
the original value directly to JSON.parse() in both sites. Preserve all
surrounding parsing behavior without decoding entities a second time.
- Around line 1591-1605: Update both duplicate dotToNested implementations at
web_interface/static/v3/js/app-shell.js lines 1591-1605 and 2340-2354 to skip
path segments named __proto__, prototype, or constructor before traversing or
assigning properties. Prefer null-prototype objects for the root and nested maps
while preserving normal dot-path nesting behavior.
- Around line 351-357: Update the installedPlugins setter around the
oldIds/newIds comparison so _installedPluginsValue and this.installedPlugins are
always replaced with newPlugins, including same-ID changes to enabled state or
metadata. Restrict the oldIds !== newIds condition to calling updatePluginTabs()
and its related change-only logging.
- Around line 789-833: Update generateFieldHtml and all related form-rendering
paths to context-safely encode plugin-controlled schema titles, descriptions,
keys, values, enum labels, and action metadata before inserting them into HTML
text, attributes, or inline handlers. Reuse the existing escapeHtml method for
text and properly quote and attribute-encode attribute values; avoid direct
interpolation into event-handler attributes, preferably replacing them with DOM
construction and addEventListener bindings. Ensure generated controls retain
their current behavior without allowing stored DOM XSS.
🪄 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
Run ID: b75e220d-0cfb-4d02-bf2d-1da7636a5088
📒 Files selected for processing (12)
web_interface/blueprints/api_v3.pyweb_interface/blueprints/pages_v3.pyweb_interface/static/v3/app.jsweb_interface/static/v3/js/app-early.jsweb_interface/static/v3/js/app-shell.jsweb_interface/static/v3/js/custom-feeds-helpers.jsweb_interface/static/v3/js/htmx-config.jsweb_interface/static/v3/plugins_manager.jsweb_interface/templates/v3/base.htmlweb_interface/templates/v3/partials/display.htmlweb_interface/templates/v3/partials/durations.htmlweb_interface/templates/v3/partials/overview.html
🚧 Files skipped from review as they are similar to previous changes (7)
- web_interface/templates/v3/partials/overview.html
- web_interface/static/v3/js/custom-feeds-helpers.js
- web_interface/static/v3/app.js
- web_interface/blueprints/api_v3.py
- web_interface/static/v3/js/app-early.js
- web_interface/static/v3/js/htmx-config.js
- web_interface/templates/v3/base.html
… web app Three usability improvements from live testing feedback: - Restart-pending banner: every successful POST to /api/v3/config/main (display hardware, rotation/durations, general settings) now raises a persistent banner - "Configuration saved, restart the display to apply" - with a Restart Now button that posts restart_display_service directly. Backed by sessionStorage so it survives tab switches and reloads until restarted or dismissed. Plugin config saves are deliberately excluded: they apply live via the display process's config watcher. - Unsaved-changes guard: plugin config panels are Alpine x-if templates, so navigating away destroys the panel and revisiting re-fetches it - edits were silently discarded. Forms now mark themselves dirty on input (cleared on successful submit), a capture-phase click handler confirms before a lossy tab switch, and beforeunload guards full page unloads. System tabs (x-show, persistent DOM) are exempt - no false prompts. - Installable web app: manifest.json (standalone display, dark theme) + generated LED-grid icons (192/512 maskable + 180 apple-touch), linked from base.html. "Add to Home Screen" now yields an app-like fullscreen experience; no service worker, so zero behavioral risk. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
Guardrails so this branch's fix classes can't regress silently:
- test_web_smoke.py (24 tests): boots the pages blueprint with the same
dual registration app.py uses and asserts every page/partial returns 200
with its load-bearing markers (nav wiring, getting-started card, advanced
section, rotation order card, per-mode duration inputs), the /v3 legacy
alias serves everything, all critical static assets (incl. vendored
fontawesome/codemirror, PWA manifest/icons) are served, durations group
per plugin with the leftover bucket, and the advanced-hardware section
really contains the tuning fields. Would have caught this session's
unreachable-durations-page and orphaned-tab bugs instantly.
- test_web_static_audit.py (3 tests): (1) every responsive utility class
referenced in templates is actually defined in app.css - the
silently-no-op class bug that left the header search box invisible at
every width; (2) every url_for('static', ...) reference points to a real
file; (3) any JS file calling the debugLog global declares it in a
/* global */ header.
All 40 web tests pass (24 + 3 new, 13 existing) under pytest + Flask.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
…rawer a11y Preview-while-configuring: - Floating mini preview (fixed, bottom-right) available on every tab except Overview, fed from the same SSE display stream by updateDisplayPreview - no new connections. Collapses to a round toggle button; open/closed state persists in localStorage; hides on Overview where the full preview lives. - "Preview on display" button on every plugin config page header: runs that plugin on the real display for 60 seconds via the existing /display/on-demand/start API and opens the floating preview, closing the configure -> see-the-result loop. Drawer/nav accessibility: - aria-current="page" tracks the active tab (system + dynamic plugin tabs, matched via their Alpine @click expression), updated from the activeTab watcher so search deep-links and checklist navigation are covered too. - Escape closes the mobile drawer and returns focus to the hamburger; opening the drawer moves focus to its first tab. Validation: all 40 web tests pass; Jinja parse + div balance on both touched templates. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
Owner
Author
|
CI workflow addition (requires push access with Add to - name: Run web interface tests
run: |
pytest --no-cov \
test/test_web_smoke.py \
test/test_web_static_audit.py \
test/test_web_settings_ui.pyNo new dependencies needed — Flask is already installed by the existing steps. Verified green locally and on the devpi rig. 🤖 Generated with Claude Code |
…callbacks, misc lint Verified each of the 27 reported findings against current code; all fixed except one rule class skipped with reason below. - app-early.js: plugin tab buttons are now built with createElement/ createTextNode instead of innerHTML template strings (icon class and name come from plugin manifests - semi-trusted input; the old code escaped the name but interpolated the icon class). Both construction sites. Also the forEach arrow no longer returns tab.remove()'s value. - plugin-order-list.js: rows, empty state, and error state all built with DOM APIs - the file no longer contains innerHTML at all (the now-unneeded escapeHtml/escapeAttr helpers are removed); MODE_LABELS is a Map so the vegas-mode lookup can't hit prototype properties. - notification.js: actionCallbacks is a Map (get/set/delete) instead of a plain object - resolves the object-injection-sink and dynamic-delete findings; triggerAction also type-checks the callback. - htmx-config.js: unused catch binding dropped; var -> const in the afterSettle handler; the swapped-<script> re-execution reads/writes textContent instead of innerHTML; the diagnostic form payload uses a null-prototype object so a field named __proto__ can't pollute. - custom-feeds-helpers.js DELETED (with its script tag): all three of its functions (addCustomFeedRow, removeCustomFeedRow, handleCustomFeedLogoUpload) are shadowed by the deferred widgets/custom-feeds.js window assignments, which always win at call time - the copies were dead even when they lived inline in base.html. This also resolves the unused-function and unused-variable findings there. Skipped: 4x "Non-serializable expression must be wrapped with $(...)" in app-early.js - that rule targets code crossing a browser-automation serialization boundary (e.g. page.evaluate); these are ordinary arrow functions in plain browser code with no such boundary. Validation: all 40 web tests pass (incl. the static-asset reference audit, which confirms no template still points at the deleted file); Jinja parse OK. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/test_web_smoke.py`:
- Around line 85-102: Update the test client fixture around
pv.pages_v3.config_manager and pv.pages_v3.plugin_manager to save both original
attributes, yield the configured test client instead of returning it, and
restore the originals in teardown so later tests are unaffected.
🪄 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
Run ID: fea3eb31-f085-4232-9577-387482e128d5
⛔ Files ignored due to path filters (3)
web_interface/static/v3/icons/apple-touch-icon.pngis excluded by!**/*.pngweb_interface/static/v3/icons/icon-192.pngis excluded by!**/*.pngweb_interface/static/v3/icons/icon-512.pngis excluded by!**/*.png
📒 Files selected for processing (12)
test/test_web_smoke.pytest/test_web_static_audit.pyweb_interface/static/v3/app.cssweb_interface/static/v3/app.jsweb_interface/static/v3/js/app-early.jsweb_interface/static/v3/js/app-shell.jsweb_interface/static/v3/js/htmx-config.jsweb_interface/static/v3/js/widgets/notification.jsweb_interface/static/v3/js/widgets/plugin-order-list.jsweb_interface/static/v3/manifest.jsonweb_interface/templates/v3/base.htmlweb_interface/templates/v3/partials/plugin_config.html
🚧 Files skipped from review as they are similar to previous changes (7)
- web_interface/templates/v3/partials/plugin_config.html
- web_interface/static/v3/js/widgets/notification.js
- web_interface/static/v3/js/widgets/plugin-order-list.js
- web_interface/static/v3/js/htmx-config.js
- web_interface/templates/v3/base.html
- web_interface/static/v3/js/app-early.js
- web_interface/static/v3/js/app-shell.js
…ally The in-app updater (Update Now banner -> git_pull action) stashed, pulled, and purged plugins - but never touched Python dependencies. Any release adding a package (e.g. this branch's flask-compress, which lives in web_interface/requirements.txt) silently required an SSH session and a manual pip install that most users will never do. - git_pull now records HEAD before pulling; after a successful pull it diffs old..new and, if requirements.txt or web_interface/requirements.txt changed, installs exactly those via _pip_install_requirements - the same vetted root-visible sudo path the Tools-tab buttons use (with its existing graceful fallback when the sudo wrapper isn't configured). Results are appended to the update toast; a failure points the user at the Tools-tab button instead of failing the whole update. - install_base_requirements (Tools tab) now also installs web_interface/requirements.txt - previously it only covered the root file, so web-only dependencies were unreachable from the UI entirely. No install happens when the pull was already-up-to-date or when no requirements file changed, so routine updates stay fast. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
…vacy fixes Verified each finding against current code. Fixed: - api_v3: plugin_rotation_order is now strictly validated (JSON list of strings, 400 with a descriptive message otherwise) and popped from the payload before any further handling. - display_controller: _apply_plugin_rotation_order defensively ignores a non-list value (keeps the existing rotation, logs a warning) and drops non-string entries; new logs carry the [DisplayController] prefix. Unit-tested both defensive paths. - app.py: snapshot-read handler narrowed to OSError with debug logging; flask-compress ImportError now emits one structured warning with the install remedy. - htmx-config: the response-error logger prints form FIELD NAMES only - values (API keys, passwords) never reach the console. - plugin-order-list: saved order/exclusions normalized with Array.isArray (a saved "null" previously crashed .forEach); each row gained keyboard/touch-accessible move-up/move-down buttons (HTML5 drag events don't fire on most mobile browsers) that reorder and syncInputs() immediately alongside native drag. - app-shell: window.installedPlugins setter always takes the new list (same-ID metadata/enabled updates were silently dropped); tab rebuild stays gated on ID changes. LED dot renderer reads the frame with ONE getImageData call instead of one per pixel (~9,200/frame at 192x48). - plugins_manager: togglePlugin returns its request promise resolving the API outcome; the install flow now shows the "installed and enabled" toast (with Restart Now) only after enablement succeeds, and a warning without a restart offer when it fails. - a11y: hamburger aria-label flips Open/Close with drawer state; both Advanced-section toggle buttons declare aria-controls/aria-expanded and the shared toggleSection() keeps aria-expanded in sync; move buttons have per-plugin aria-labels. - Rotation/Vegas order-list bootstraps cap their retries (~5s) and show a reload hint instead of spinning forever; Alpine app-state lookups prefer [x-data="app()"] with a generic fallback. Skipped, with reasons: - executePluginAction arg order: caller (plugin_config.html) already passes (actionId, index, pluginId) matching the signature exactly. - generateFieldHtml XSS, entity-unescape blocks, dotToNested pollution, and "app.loadInstalledPlugins" in app-shell: all inside the legacy client-side config cluster whose entry points are shadowed by plugins_manager.js / replaced by server-rendered forms (zero live callers, verified) - queued for wholesale deletion in the follow-up rather than patching dead code. - custom-feeds-helpers.js findings (3): file was deleted in a prior commit. - console.error/warn override removal and afterSwap script re-execution removal: deliberate pre-existing workarounds every partial's inline init currently depends on; reworking them safely needs isolated testing (follow-up), and the error suppression is already double-gated (insertBefore AND htmx match). - "move durations bootstrap into a bundle": inline partial-scoped init is the established pattern for HTMX partials in this codebase. Validation: all 40 web tests pass; py_compile on all touched Python; all touched templates parse; rotation-order defensive paths unit-tested. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web_interface/blueprints/api_v3.py (1)
993-1010: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winConsume per-mode duration fields before the generic configuration merge.
These keys remain in
data, so Lines 1133-1158 also persist entries such asduration__clockat the configuration root. Invalid values are likewise reported as successfully saved and later persisted there.Pop each processed field and return a descriptive 400 for invalid values.
Proposed fix
for field in mode_duration_fields: mode_key = field[len('duration__'):] if not mode_key: continue + raw_duration = data.pop(field) try: - current_config['display']['display_durations'][mode_key] = int(data[field]) + current_config['display']['display_durations'][mode_key] = int(raw_duration) except (ValueError, TypeError): - logger.warning("Ignoring non-integer duration for %s", mode_key) + return jsonify({ + 'status': 'error', + 'message': f"Duration for '{mode_key}' must be an integer" + }), 400As per coding guidelines, “Validate inputs and handle errors early (Fail Fast principle)” and “Provide clear error messages for troubleshooting.”
🤖 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 993 - 1010, Update the per-mode duration handling in the configuration update flow to consume each processed duration__ field from data before the generic merge at the later configuration-processing block. Validate each value before updating current_config['display']['display_durations']; on the first invalid or non-integer value, return HTTP 400 with a descriptive error identifying the mode and invalid input instead of logging and continuing.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/blueprints/api_v3.py`:
- Around line 1720-1733: Update the dependency-install loops in
web_interface/blueprints/api_v3.py at lines 1720-1733 and 1775-1779 so each
requirements-file installation handles its own TimeoutExpired and OSError
failures, records a clear labeled message in dep_notes or outputs respectively,
marks all_ok false in the outputs path, and continues processing subsequent
changed files.
---
Outside diff comments:
In `@web_interface/blueprints/api_v3.py`:
- Around line 993-1010: Update the per-mode duration handling in the
configuration update flow to consume each processed duration__ field from data
before the generic merge at the later configuration-processing block. Validate
each value before updating current_config['display']['display_durations']; on
the first invalid or non-integer value, return HTTP 400 with a descriptive error
identifying the mode and invalid input instead of logging and continuing.
🪄 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
Run ID: 3cff2f5a-bb62-49a1-9ca4-838c9201d7dc
📒 Files selected for processing (13)
src/display_controller.pyweb_interface/app.pyweb_interface/blueprints/api_v3.pyweb_interface/static/v3/app.jsweb_interface/static/v3/js/app-shell.jsweb_interface/static/v3/js/htmx-config.jsweb_interface/static/v3/js/widgets/plugin-order-list.jsweb_interface/static/v3/plugins_manager.jsweb_interface/templates/v3/base.htmlweb_interface/templates/v3/partials/display.htmlweb_interface/templates/v3/partials/durations.htmlweb_interface/templates/v3/partials/overview.htmlweb_interface/templates/v3/partials/plugin_config.html
🚧 Files skipped from review as they are similar to previous changes (10)
- web_interface/templates/v3/partials/plugin_config.html
- web_interface/static/v3/js/widgets/plugin-order-list.js
- src/display_controller.py
- web_interface/static/v3/js/htmx-config.js
- web_interface/templates/v3/partials/overview.html
- web_interface/app.py
- web_interface/templates/v3/base.html
- web_interface/static/v3/app.js
- web_interface/static/v3/js/app-shell.js
- web_interface/static/v3/plugins_manager.js
- htmx-config.js: two more unused catch bindings dropped (optional catch
binding), matching the earlier fix.
- app-early.js: second forEach arrow (the stub updatePluginTabs copy)
braced so the callback no longer returns tab.remove()'s value.
The other 4 findings ("Non-serializable expression must be wrapped with
$(...)") are deliberately NOT "fixed": that rule belongs to a
browser-automation (WebdriverIO-style) lint context and is misfiring on
ordinary arrow-function constants. Converting them to function
declarations would look compliant but BREAK the code - all four arrows
intentionally capture the enclosing Alpine component's `this` for the
stub-to-full enhancement logic. The right remedy is disabling that
pattern for this repo in Codacy's Code Patterns settings (or dismissing
the four findings), not a code change.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
…izable The floating preview opened empty and stayed empty until the display next CHANGED - the SSE stream only pushes frames on change, and the panel only consumed frames while already open, so the connection's initial frame (sent while the panel was closed) was dropped. Reported from mobile testing as "the button doesn't work". - updateDisplayPreview now caches the latest frame globally regardless of panel state; opening the panel populates the image from that cache instantly, then live frames take over. - Resizable: a size button cycles 192/256/384/512px presets (persisted per browser; works on touch), and desktop additionally gets a native drag handle (CSS resize: both). The image is fluid within the panel; on phones the panel is capped to the viewport width. The size icon (fa-up-right-and-down-left-from-center) is verified present in the vendored FA 6.0.0. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
…file dependency installs; fix test fixture leak Verified each finding against current code. - api_v3 save_main_config: both duration blocks (*_duration suffix fields and the newer duration__<mode> fields) only READ from `data`, never removed the keys. The generic "remaining keys" merge later in the same function has no skip-list entry for either pattern, so every duration field was ALSO written a second time as a bogus top-level config key (e.g. "clock_duration": 30 and "duration__mlb_live": 42 sitting at config root, alongside the correct nested display.display_durations.<key>). Confirmed by tracing the full function. Fixed by popping each handled key from `data` (same pattern already used for plugin_rotation_order) and validating strictly: a non-integer duration now returns 400 with a message naming the offending field/mode instead of silently logging and moving on (for the *_duration fields, which previously had zero validation at all). - api_v3 dependency-install loops (git_pull's post-update sync and install_base_requirements): _pip_install_requirements can raise subprocess.TimeoutExpired or OSError (confirmed: install_requirements_file in permission_utils.py never catches either internally, despite its docstring's "never raises on non-zero exit" only covering return codes). Both loops previously let one file's exception either abort the whole try block (skipping the second requirements file entirely) or propagate uncaught. Each file's install is now in its own try/except, so a timeout or OSError on one file is recorded as a labeled failure and the loop continues to the next file. - test_web_smoke.py: the `client` fixture mutated the module-level pages_v3 Blueprint singleton's config_manager/plugin_manager directly with no teardown - since pages_v3 is shared across the whole pytest process (test_web_settings_ui.py touches the same attributes), this fixture's mocks could leak into whichever test ran next. Now saves the originals, yields the client, and restores them in a finally block. Validation: py_compile passes; all 40 web tests pass with the now-generator fixture. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
An earlier sed-based text update concatenated the old and new copies of this description instead of replacing one with the other, leaving a duplicated sentence with the — entity broken into ".mdash;" (visible as literal "mdash;" text on the page). Restored to one clean sentence. Other findings from this review were already fixed in a prior commit (installedPlugins setter) or are confirmed dead code with zero live callers (executePluginAction/dotToNested/entity-unescape/generateFieldHtml, all reachable only from the two unused savePluginConfig copies in app-shell.js - grepped every template, no references) - same legacy cluster flagged in earlier review passes, still queued for a dedicated deletion follow-up rather than patched in place here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
There was a problem hiding this comment.
🧹 Nitpick comments (1)
web_interface/blueprints/api_v3.py (1)
987-988: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the duration field check.
The strings
'default_duration'and'transition_duration'both end with'_duration', making the explicit tuple check redundant.♻️ Proposed refactor
- duration_fields = [k for k in list(data.keys()) - if k.endswith('_duration') or k in ('default_duration', 'transition_duration')] + duration_fields = [k for k in list(data.keys()) if k.endswith('_duration')]🤖 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 987 - 988, Update the duration_fields comprehension to identify duration keys solely with the existing endswith('_duration') check, removing the redundant explicit default_duration and transition_duration membership test.
🤖 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.
Nitpick comments:
In `@web_interface/blueprints/api_v3.py`:
- Around line 987-988: Update the duration_fields comprehension to identify
duration keys solely with the existing endswith('_duration') check, removing the
redundant explicit default_duration and transition_duration membership test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ac08e4aa-18c7-4df9-af6e-a3eeccad31e0
📒 Files selected for processing (8)
test/test_web_smoke.pyweb_interface/blueprints/api_v3.pyweb_interface/static/v3/app.cssweb_interface/static/v3/app.jsweb_interface/static/v3/js/app-early.jsweb_interface/static/v3/js/app-shell.jsweb_interface/static/v3/js/htmx-config.jsweb_interface/templates/v3/base.html
🚧 Files skipped from review as they are similar to previous changes (6)
- test/test_web_smoke.py
- web_interface/static/v3/js/htmx-config.js
- web_interface/static/v3/js/app-early.js
- web_interface/static/v3/app.js
- web_interface/templates/v3/base.html
- web_interface/static/v3/js/app-shell.js
…uble-executing every partial's inline script) Re-verified this CodeRabbit finding, previously deferred as "needs isolated testing" - traced it to a confirmed, active bug rather than a style concern: htmx 1.9.10's own config defaults to allowScriptTags: true (confirmed in the vendored htmx.min.js, which itself contains the same clone-and-reinsert <script> mechanism internally). This means htmx ALREADY re-executes every <script> tag in swapped content by default, exactly like a browser navigating to a new page. The custom htmx:afterSwap listener in htmx-config.js did the identical clone-and-reinsert a SECOND time on top of htmx's own handling - so every inline <script> block in every HTMX-loaded partial (overview, display, durations, plugin config, etc. - most partials have one) executed twice per load. Confirmed safe to delete outright, not just narrow: grepped every hand-written JS file for a manual `dispatchEvent(... 'htmx:afterSwap' ...)` that might have relied on this handler for a non-htmx code path (e.g. the direct-fetch fallbacks like loadOverviewDirect) - none exists, so nothing depended on this listener specifically; htmx's native handling covers every real htmx-driven swap on its own. Left in place, unchanged: the console.error/console.warn global override a few lines up in the same file, which suppresses known-noisy HTMX-timing-race messages. That one is a legitimate anti-pattern too (broad substring matching can mask unrelated errors) but redesigning it needs care to preserve real diagnostics while still hiding the specific harmless races it targets - a scoped follow-up, not a same-day deletion like this confirmed-duplicate handler. Validation: all 27 fast web tests pass; JS brace/paren balance sanity checked (no local Node/browser available in this sandbox to execute the file directly - verify manually in-browser before merge). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
…le-complete log Re-verifying the full CodeRabbit findings list against current code surfaced one still-open item: the nitpick asked for the prefix on BOTH rotation-related log lines, but only "Applied plugin rotation order" got it in the earlier pass - "Plugin reconcile complete" was missed. No message/argument/level change, matching the finding's own scope. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
4 tasks
The "Logs tab" link in the display-settings simulation-mode banner was a real <a href> to /v3/logs, but no such route has ever existed (log content is loaded client-side via activeTab, not a dedicated page route) - the link 404'd regardless of the /v3 prefix change. Switch it to the same activeTab-switching pattern the real nav uses.
Cols already allowed up to 128; Rows was capped at 64, which rejects valid larger panel configurations (e.g. 128-row tile chains). No server-side schema enforces a rows max, so this was purely an overly-strict HTML input attribute.
CodeQL flags .includes('htmx.org') as "incomplete URL substring
sanitization" - a false positive here, since this string is only ever
matched against console.error/warn message text to decide whether to
suppress a known-harmless HTMX timing-race log line, not used for any
URL-trust/redirect decision. The check was also redundant: 'htmx' is
already a substring of 'htmx.org', so the plain .includes('htmx') check
right next to it already covers every case the removed check did.
…depends on
Removing the custom htmx:afterSwap script-reexecution handler (in a prior
commit, as a "duplicate execution" cleanup) broke every partial whose Alpine
x-data component function is defined by an inline <script> in that same
partial (e.g. wifi.html's wifiSetup()) - confirmed live via browser console:
"Alpine Expression Error: wifiSetup is not defined" on every field in the
WiFi tab.
Root cause: htmx's own native script execution runs during its "settle"
phase (~20ms after swap, per htmx's own defaultSettleDelay), but Alpine's
MutationObserver evaluates x-data on newly-inserted elements synchronously,
right as the swap lands - before settle. So the inline <script> defining
wifiSetup() was still un-run when Alpine tried to call it, and Alpine does
not retry a failed x-data evaluation later once the function does become
defined.
Fix: re-execute swapped <script> tags ourselves on htmx:afterSwap (which
fires synchronously, before settle, beating Alpine's observer), and disable
htmx's own native script re-execution (htmx.config.allowScriptTags = false)
so the same script doesn't also run a second time during settle - restoring
correct timing without reintroducing the original double-execution bug.
Also in this commit:
- fix XSS: unescaped repoUrl in a title attribute in renderSavedRepositories
- replace .includes('github.com') substring checks with real URL hostname
validation (CodeQL: incomplete URL substring sanitization)
…g it Confirmed live: installing hockey-scoreboard logged "installation queued" (success) immediately followed by "enabling it failed" with a 404 "Plugin not found" from /api/v3/plugins/toggle. /api/v3/plugins/install runs the actual clone + plugin-manager discovery asynchronously via an operation queue when one is configured - the response installPlugin() was checking only means the operation was queued, not that the plugin is installed yet. Calling togglePlugin() right after that response 404s because plugin_manager hasn't discovered the new plugin. Fix: reuse the same operation-polling mechanism uninstallPlugin() already has (generalized pollOperationStatus() to take onComplete/onFailed/onTimeout callbacks instead of hardcoding uninstall behavior) so installPlugin() waits for the operation to actually complete before enabling it. Falls back to enabling immediately when no operation_id is returned (direct/synchronous install path, no queue configured).
- custom-feeds.js: fix asset-upload contract mismatch (field name "file" -> "files", response read from top-level "uploaded_files" not "data.files") - same bug already fixed in this file on a separate branch/PR (#420), which this branch never received since they're independent PRs off main - custom-feeds.js: add aria-label to the two icon-only "remove feed" buttons - custom-feeds.js: move file-input reset into .finally() so a failed upload doesn't leave the input stuck holding the file, blocking retry of the same file - app-shell.js: fix executePluginAction(pluginId, actionId) parameter order/count mismatch vs. its callers' (actionId, index, pluginId) - currently masked by plugins_manager.js's correct version overwriting this one at load time (classic vs. deferred script order), but worth fixing outright since it's an isolated, self-contained reassignment (not inside the Alpine app() object literal) and removes a latent footgun - overview.html: align Alpine-state resolution with settings-search.js's two-tier getAppData() (also check appEl.__x.$data, not just _x_dataStack) Verified already addressed by earlier passes (no change needed): plugin_rotation_order validation, DisplayController log prefixes, togglePlugin returning its promise for install-flow chaining, installedPlugins setter always updating state, mobile-nav aria-label, toggleSection aria-expanded sync, PluginOrderList bounded init retries (both display.html and durations.html), plugin-order-list.js Array.isArray validation, batched getImageData in the LED-dot preview renderer, app.py exception narrowing/ logging, form-submission log redaction. Confirmed dead code, skipped (unreachable - zero template/JS callers, verified via full-repo grep): dotToNested prototype-pollution hardening, generateFieldHtml HTML-injection hardening, and the HTML-entity-unescape block in JSON parsing - all three live only inside app-shell.js's two legacy savePluginConfig implementations (one Alpine-method, one standalone), neither of which any template or script calls. The real, live plugin-config path is server-rendered via GET /partials/plugin-config/<id>. Explicitly NOT reverted: the htmx:afterSwap script-execution listener. An earlier finding batch asked to remove it as "duplicate" htmx behavior; that was tried and reverted this session after live testing on hardware proved it broke every partial whose Alpine x-data depends on an inline <script> in the same partial (confirmed: WiFi tab hard-failed with "wifiSetup is not defined"). Removing it again would reintroduce that regression.
ChuckBuilds
added a commit
that referenced
this pull request
Jul 18, 2026
…tract (#420) Every custom-feed logo upload has been failing: handleCustomFeedLogoUpload posts the file under field name "file" and reads the response from data.data.files, but the backend endpoint it calls (api_v3.upload_plugin_asset, /api/v3/plugins/assets/upload) requires the field name "files" (checks 'files' not in request.files, 400s "No files provided" otherwise) and returns the result in a top-level "uploaded_files" key - there is no nested "data" wrapper in the response at all. Confirmed by reading the endpoint directly, and cross-checked against file-upload-single.js, a sibling widget that uses the correct contract against the same endpoint. - formData.append('file', file) -> formData.append('files', file) - data.data.files / data.data.files[0] -> data.uploaded_files / data.uploaded_files[0] No other call sites in this file used the stale contract (grepped for both patterns after the fix - zero remaining). The response entries' 'path' and 'id' fields (both read further down in the same handler) are unaffected - only the wrapper shape was wrong. Found incidentally while re-verifying a CodeRabbit review on an unrelated PR (#417) that had deleted a differently-named dead file (custom-feeds-helpers.js) with the same bug; this widget (custom-feeds.js) is the live code path and was never touched by that PR. Validation: brace/paren balance check; explicit assertions that the old field name and response shape no longer appear anywhere in the file. No Python changed, no existing tests cover this endpoint's client flow. Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ Signed-off-by: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Users reported the web interface is hard to use on phones and overwhelming to configure. This branch addresses both without removing any capability, then layers on owner-requested refinements from live testing on the devpi rig. 22 commits, each independently documented and validated.
Mobile
aria-currenttracks the active tab.sm:block/lg:flexwere never defined in app.css, so both widgets were invisible at every width. 13 missing breakpoint utilities filled.Faster setup, granular control kept
x-advanced: trueschema flag -> collapsed Advanced Settings section on any plugin config page (search auto-expands into it). Documented in docs/widget-guide.md.display.plugin_rotation_order, applied at startup and live reconcile; order was previously non-deterministic parallel-load completion order), plus per-screen duration fields for every enabled plugin's display modes. This page was previously orphaned (no tab) and doubly broken (empty dict + save filter dropped its fields); now reachable and round-trips correctly.Feedback-loop & safety UX
URL & performance
/;/v3kept as a working legacy alias; "- v3" branding dropped.localStorage.pluginDebug.Tests
Test plan
plugin_rotation_orderand observe the order on the panel🤖 Generated with Claude Code
https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
Summary by CodeRabbit
New Features
plugin_rotation_order) plus a “Rotation & Durations” page with per-mode duration inputs.Improvements
/(with/v3as a legacy alias), and mobile navigation/responsive layout was refreshed.Documentation
x-advanced.Tests