feat(sync): multi-display wireless sync — extend scrolling across two LED matrices - #330
Merged
ChuckBuilds merged 7 commits intoMay 14, 2026
Merged
Conversation
… LED matrices Adds a leader/follower sync system that extends Vegas scroll mode content continuously across two physically adjacent LED matrix units over WiFi. Architecture: - Leader broadcasts scroll position via UDP at ~90fps; follower renders the offset slice of the same image at 60fps using dead reckoning to absorb UDP jitter (smooth, stutter-free motion) - At each cycle transition the leader sends the composed scroll image via TCP (PNG-compressed ~15–40KB) so both displays render pixel-identical content regardless of plugin data timing differences - Auto-discovery via UDP subnet broadcast — no IP configuration required - Heartbeat watchdog (6s timeout) falls back to standalone if peer goes offline Key files: - src/common/sync_manager.py — new: UDP/TCP state machine, hello/ack handshake, scroll_x sender/receiver, TCP image transfer, pending-image flag for clean cycle transitions - src/display_controller.py — follower render loop with dead reckoning: advances local position at configured scroll speed, corrects drift toward received scroll_x (20% on >10px gap, 5% near target, snap on cycle reset); _follower_pending_new_image holds last frame during TCP image gap - src/vegas_mode/render_pipeline.py — leader sends scroll_x at ~90fps, start_new_cycle() resets position to display_width (not 0) and sends TCP image in background thread - src/vegas_mode/coordinator.py — set_sync_manager() / set_update_callback() wiring; defers hot-swap recompose while sync is active - web_interface/blueprints/api_v3.py — sync config save endpoint, GET /api/v3/sync/status for live status polling - web_interface/templates/v3/partials/display.html — Multi-Display Sync section: role selector (Standalone/Leader/Follower), position (Left/Right of leader, follower only), UDP port, live status indicator - config/config.template.json — sync block: role, port, follower_position Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a leader/follower DisplaySyncManager (UDP/TCP), integrates it into the Vegas render pipeline and coordinator, wires a follower render path and helpers in DisplayController, and exposes sync config/status in the config template and web UI. ChangesMulti-Display Synchronization System
Sequence Diagram(s)sequenceDiagram
participant Follower
participant Leader
participant TCP as LeaderTCP
Follower->>Leader: UDP broadcast "hello"/"hb"
Leader->>Follower: UDP "hello_ack" (compat + width)
loop during cycle
Leader->>Follower: UDP "sx" (scroll_x)
alt new cycle
Leader->>Follower: UDP "nc" (new cycle)
Leader->>TCP: TCP send_scroll_image (PNG)
TCP-->>Follower: TCP deliver image
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
web_interface/templates/v3/partials/display.html (1)
795-803: ⚡ Quick winGuard polling interval creation to avoid duplicate timers on partial re-mounts.
If this template script runs more than once, multiple 5s pollers accumulate and spam
/api/v3/sync/status.♻️ Proposed fix
- setInterval(pollSyncStatus, 5000);+ if (window.__syncStatusPoller) clearInterval(window.__syncStatusPoller);+ window.__syncStatusPoller = setInterval(pollSyncStatus, 5000); }); } else { updateSyncUI(); - setInterval(pollSyncStatus, 5000);+ if (window.__syncStatusPoller) clearInterval(window.__syncStatusPoller);+ window.__syncStatusPoller = setInterval(pollSyncStatus, 5000); }🤖 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 795 - 803, The script creates a new 5s poller each time it runs, causing duplicate timers; modify the logic around setInterval/pollSyncStatus to guard against existing timers by storing the interval id on a persistent scope (e.g., window.syncStatusInterval) and only call setInterval if that variable is not already set, or clearInterval(window.syncStatusInterval) before creating a new one; update references to updateSyncUI and pollSyncStatus accordingly so the interval id is reused/cleared to prevent multiple concurrent pollers.src/common/sync_manager.py (1)
111-117: ⚡ Quick winUse
Callablefrom typing instead of the builtincallablein type annotations.
callableis Python's builtin function (returns bool), not a type.Optional[callable]is invalid as an annotation — static type checkers (mypy, pyright) will flag it, and it doesn't document the expected signature. Usetyping.Callable.♻️ Proposed fix
-from typing import Optional+from typing import Any, Callable, Optional- self._on_new_cycle: Optional[callable] = None # called when leader starts new cycle- self._on_scroll_image: Optional[callable] = None # called with Image when received+ self._on_new_cycle: Optional[Callable[[], None]] = None # called when leader starts new cycle+ self._on_scroll_image: Optional[Callable[[Image.Image], None]] = None # called with Image when received self._pending_scroll_image: Optional[Image.Image] = None # image received before callback set self._img_server_sock = None # TCP server for scroll image transfer # Leader state additions - self._on_follower_connected: Optional[callable] = None # called when follower connects+ self._on_follower_connected: Optional[Callable[[], None]] = None # called when follower connectsAlso tighten the public-API signatures:
- def set_on_new_cycle(self, callback) -> None:+ def set_on_new_cycle(self, callback: Callable[[], None]) -> None:- def set_on_follower_connected(self, callback) -> None:+ def set_on_follower_connected(self, callback: Callable[[], None]) -> None:- def set_on_scroll_image(self, callback) -> None:+ def set_on_scroll_image(self, callback: Callable[[Image.Image], None]) -> None:As per coding guidelines: "Use type hints for function parameters and return values".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/common/sync_manager.py` around lines 111 - 117, Replace the incorrect Optional[callable] annotations with typing.Callable-based annotations and import Callable from typing; update _on_new_cycle, _on_scroll_image, and _on_follower_connected to use Optional[Callable[..., Any]] (or a more specific Callable[[arg_types], ReturnType] if you know the exact callback signatures) and keep _pending_scroll_image as Optional[Image.Image]; ensure you add "from typing import Optional, Callable, Any" at the top and adjust any docstrings or usages to reflect the tighter callback signatures (e.g., _on_scroll_image should be Callable[[Image.Image], None] if it takes an Image).
🤖 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 `@src/common/sync_manager.py`:
- Around line 356-373: send_frame currently drops datagrams silently when
len(data) > 65000; add a one-shot warning to surface this condition: detect the
oversized packet just after building data (the header/arr.tobytes() step used
with _RAW_MAGIC and _RAW_HEADER in send_frame), and if it exceeds the UDP cap
log a single warning via self.logger.warn/self.logger.warning including the
computed data size, the maximum allowed (≈65000) and the image dimensions
(image.width/height); store a boolean flag on the instance (e.g.
self._oversized_frame_warned) so the warning is emitted only once to avoid
spamming, and keep the existing behavior of not sending the packet. Ensure
references to send_frame, _RAW_MAGIC, _RAW_HEADER, self._send_sock and self.port
remain unchanged.
In `@src/display_controller.py`:
- Line 758: The class-level comment and docstring for _send_follower_frame are
stale: _FOLLOWER_SEND_INTERVAL is 1.0/90 and sync_manager.send_frame transmits
raw RGB, not PNG at 30fps; update the docstring/comment to reflect the current
behavior by describing that frames are throttled to ~90fps via
_FOLLOWER_SEND_INTERVAL and that send_frame sends raw RGB bytes (no PNG
encode/decode), and remove or correct any mention of 30fps or PNG in
_send_follower_frame and nearby comments (referencing the
_FOLLOWER_SEND_INTERVAL constant and the _send_follower_frame method and calls
to sync_manager.send_frame).
- Line 1926: Replace wall-clock timing with monotonic timing: wherever the code
uses start_time = time.time() and computes elapsed = time.time() - start_time
for elapsed-duration math (the start_time/elapsed pairs that feed
_should_exit_dynamic, target_duration comparisons, and FPS/logging), change
those to use time.monotonic() (or time.perf_counter() consistently) instead of
time.time(); update each occurrence (including the start_time assignments and
the corresponding elapsed calculations) so elapsed is derived from monotonic
time to avoid NTP/DST clock jumps.
- Line 520: DisplayController currently caches a snapshot of config in
self.config and never updates it, so schedule and brightness decisions (e.g.,
where schedule_config is read and in methods handling
timezone/schedule/brightness around schedule_config, decision paths at lines
~520, ~537, ~636, ~643, ~651) use stale data; fix by subscribing
DisplayController to ConfigService updates (use
ConfigService.subscribe(callback)) and in the callback rebind self.config =
self.config_service.get_config() (or alternatively always call
self.config_service.get_config() at decision points instead of using the cached
self.config) so schedule, timezone, dim schedule and brightness changes take
effect without restart.
In `@web_interface/templates/v3/partials/display.html`:
- Around line 751-792: Replace the unsafe content.innerHTML assignment with
building DOM nodes and using textContent to insert dynamic values: keep setting
content.className as is, then create a span for the icon (set className
"font-bold text-lg leading-none" and icon via textContent) and a separate span
for the status text (set its textContent to the variable text which already
contains API-derived values like d.peer_ip/d.leader_ip); append those spans to
content and leave errorDetail/errorText handling unchanged so that no
user-supplied value is ever inserted via innerHTML.
---
Nitpick comments:
In `@src/common/sync_manager.py`:
- Around line 111-117: Replace the incorrect Optional[callable] annotations with
typing.Callable-based annotations and import Callable from typing; update
_on_new_cycle, _on_scroll_image, and _on_follower_connected to use
Optional[Callable[..., Any]] (or a more specific Callable[[arg_types],
ReturnType] if you know the exact callback signatures) and keep
_pending_scroll_image as Optional[Image.Image]; ensure you add "from typing
import Optional, Callable, Any" at the top and adjust any docstrings or usages
to reflect the tighter callback signatures (e.g., _on_scroll_image should be
Callable[[Image.Image], None] if it takes an Image).
In `@web_interface/templates/v3/partials/display.html`:
- Around line 795-803: The script creates a new 5s poller each time it runs,
causing duplicate timers; modify the logic around setInterval/pollSyncStatus to
guard against existing timers by storing the interval id on a persistent scope
(e.g., window.syncStatusInterval) and only call setInterval if that variable is
not already set, or clearInterval(window.syncStatusInterval) before creating a
new one; update references to updateSyncUI and pollSyncStatus accordingly so the
interval id is reused/cleared to prevent multiple concurrent pollers.
🪄 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: 3f5a3d05-facf-4987-9269-4ede3f51c0e9
📒 Files selected for processing (7)
config/config.template.jsonsrc/common/sync_manager.pysrc/display_controller.pysrc/vegas_mode/coordinator.pysrc/vegas_mode/render_pipeline.pyweb_interface/blueprints/api_v3.pyweb_interface/templates/v3/partials/display.html
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
- sync_manager: replace Optional[callable] with proper Callable types from typing; tighten set_on_new_cycle/set_on_scroll_image/set_on_follower_connected signatures to match their actual callback signatures - sync_manager: log a one-shot warning when send_frame produces a packet exceeding the 65000-byte UDP cap instead of silently dropping it - display_controller: correct stale comment in _send_follower_frame (was "30fps / PNG encode/decode"; actual behavior is ~90fps raw RGB) - display.html: guard setInterval with window.syncStatusInterval to prevent duplicate pollers if the script runs more than once - display.html: replace innerHTML with DOM node creation + textContent for status icon/text to avoid inserting API-derived values via innerHTML Skip: time.time() → monotonic and self.config staleness are pre-existing issues not introduced by this PR. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
sarjent
commented
May 13, 2026
ContributorAuthor
@coderabbitai review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/display_controller.py (1)
2338-2346:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStop the sync manager during cleanup.
cleanup()tears down the display and config service but leaves the sync sockets and threads running. That can keep the sync ports bound and leaves the status writer alive if this controller is recreated in-process.🧹 Proposed fix
def cleanup(self): """Clean up resources.""" + if hasattr(self, 'sync_manager'):+ self.sync_manager.stop()+ # Shutdown config service if it exists if hasattr(self, 'config_service'): try: self.config_service.shutdown()🤖 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 2338 - 2346, The cleanup block currently tears down config_service and display_manager but leaves sync sockets/threads running; update cleanup() to also stop the sync manager and status writer: after the config_service shutdown and before/after display_manager.cleanup(), check hasattr(self, 'sync_manager') and attempt to stop it by calling self.sync_manager.shutdown() and fall back to self.sync_manager.stop() or self.sync_manager.close() inside a try/except that logs warnings on failure, and likewise check hasattr(self, 'status_writer') and call self.status_writer.shutdown() or self.status_writer.stop() (with try/except logging) so sockets/threads are properly terminated when the controller is cleaned up.
♻️ Duplicate comments (2)
src/display_controller.py (2)
1926-1926:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse a monotonic clock for duration math.
These elapsed calculations are back on
time.time(), so NTP or manual clock changes can make display durations and dynamic-cycle exit checks jump backward or forward unexpectedly. Keep wall-clock time for timestamps only; usetime.monotonic()ortime.perf_counter()forstart_time/elapsed.#!/bin/bash rg -nP 'start_time = time\.time\(\)|elapsed(?:_total)? = time\.time\(\) - start_time' src/display_controller.pyAlso applies to: 1997-1997, 2027-2027, 2078-2078, 2084-2084
🤖 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` at line 1926, Replace uses of time.time() for measuring durations with a monotonic clock: set start_time = time.monotonic() and compute elapsed or elapsed_total as time.monotonic() - start_time (instead of time.time() - start_time) so dynamic-cycle exit checks and display durations aren’t affected by wall-clock changes; update all occurrences of the symbols start_time, elapsed, and elapsed_total in this module (including the other instances flagged) to use time.monotonic() (or time.perf_counter()) consistently while keeping wall-clock timestamps as time.time().
520-521:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThese schedule reads are still using the startup config snapshot.
self.configis captured once in__init__, so hot-reloaded schedule, timezone, and brightness changes still will not apply here until process restart. Pull fresh config in these checks or subscribe the controller toConfigServiceupdates before switching these paths toself.config.Also applies to: 537-537, 636-643, 651-651
🤖 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 520 - 521, The code reads schedule/timezone/brightness from the snapshot stored in self.config (e.g., the schedule_config assignment and the other occurrences noted) which prevents hot-reloads; fix by fetching the live config at each check or by subscribing DisplayController to ConfigService updates and updating self.config on change: either replace self.config.get(...) calls with the runtime fetch (e.g., self.config_service.get('schedule') / get('timezone') / get('brightness')) where schedule_config is computed, or add a registration in __init__ to listen to ConfigService change events and refresh self.config before these checks so the logic in the methods that reference schedule_config and the other occurrences uses the latest configuration.
🤖 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 `@config/config.template.json`:
- Around line 129-133: Update the "sync" configuration block by adding concise
inline comments for each key: document valid values for "role" (e.g.,
"standalone", "master", "follower") and their behavior, note "port" requirements
(default 5765, required for network discovery and any firewall/port-range
constraints), and explain when "follower_position" applies (only relevant when
role is "follower" and how values like "left"/"right"/"top"/"bottom" affect
layout); place these comments next to the "sync" block and keys so users can
quickly understand expected values and usage.
In `@src/common/sync_manager.py`:
- Around line 245-264: The code reads a 4-byte length and then allocates a
buffer of that size before opening the image, allowing a malicious peer to
trigger OOM; update the receive path that starts at the accept()/hdr/length
handling to validate the length immediately against a configured MAX_IMAGE_SIZE
(and non-positive values) and reject by closing the connection and continuing if
it exceeds limits, so you never allocate or read unbounded data into `data`;
also ensure the subsequent receive loop and the `Image.open(io.BytesIO(...))`
usage remain unchanged but are only reached when length is validated, and add
brief logging on rejects for observability.
In `@src/display_controller.py`:
- Around line 85-93: The wrapper _follower_gated_update currently blocks all
update_display() calls unconditionally because it only checks
_sync_render_allowed; change the gating logic so it permits hardware writes when
not actually a follower: inside the wrapper (which replaces
display_manager.update_display and captures _real_update and _dm), call
_real_update() if self.sync_manager.role != SyncRole.FOLLOWER OR getattr(_dm,
'_sync_render_allowed', False) is True; alternatively restore
display_manager.update_display to _real_update when role transitions away from
SyncRole.FOLLOWER—update the code around _follower_gated_update, _real_update
and the sync_manager.role check to implement this behavior.
In `@src/vegas_mode/coordinator.py`:
- Around line 140-152: In set_sync_manager, normalize any "standalone"
DisplaySyncManager to None before assigning it to the pipeline: if the passed
sync_manager exists but represents a standalone/no-op controller (e.g. has a
truthy attribute like standalone or a similar sentinel), set sync_manager = None
so render_pipeline.sync_manager only receives a real enabled manager; then
proceed to set render_pipeline.sync_manager and
render_pipeline.sync_follower_left as before. This change should be made in the
set_sync_manager method to avoid treating a constructed-but-standalone
DisplaySyncManager as an enabled manager downstream.
- Around line 401-404: The hardcoded _UPDATE_TICK_FRAMES = 500 causes the update
cadence to vary with FPS; compute the frame interval from a desired time
interval and the current configured FPS instead. Replace the constant with
something like: determine an interval_seconds (e.g. 4s) and compute
_update_tick_frames = max(1, int(self._target_fps * interval_seconds)) (or use
self.config.target_fps/self.target_fps if that name is used in this class)
before the conditional that checks frame_count % _update_tick_frames, keeping
the existing checks for self._update_callback, frame_count and
self._update_tick_running; ensure the computed value is at least 1 so the modulo
stays valid.
In `@src/vegas_mode/render_pipeline.py`:
- Around line 239-245: The current try/except around creating and writing a
blank frame swallows all errors (involving _Image.new,
self.display_manager.image and self.display_manager.update_display) — replace
the bare except/pass with logging of the exception (e.g., catch Exception as e
and call logging.exception or self.logger.exception with a clear message like
"Failed to write blank frame to display" and include the exception) so failures
when creating/updating the blank frame are recorded for remote debugging.
---
Outside diff comments:
In `@src/display_controller.py`:
- Around line 2338-2346: The cleanup block currently tears down config_service
and display_manager but leaves sync sockets/threads running; update cleanup() to
also stop the sync manager and status writer: after the config_service shutdown
and before/after display_manager.cleanup(), check hasattr(self, 'sync_manager')
and attempt to stop it by calling self.sync_manager.shutdown() and fall back to
self.sync_manager.stop() or self.sync_manager.close() inside a try/except that
logs warnings on failure, and likewise check hasattr(self, 'status_writer') and
call self.status_writer.shutdown() or self.status_writer.stop() (with try/except
logging) so sockets/threads are properly terminated when the controller is
cleaned up.
---
Duplicate comments:
In `@src/display_controller.py`:
- Line 1926: Replace uses of time.time() for measuring durations with a
monotonic clock: set start_time = time.monotonic() and compute elapsed or
elapsed_total as time.monotonic() - start_time (instead of time.time() -
start_time) so dynamic-cycle exit checks and display durations aren’t affected
by wall-clock changes; update all occurrences of the symbols start_time,
elapsed, and elapsed_total in this module (including the other instances
flagged) to use time.monotonic() (or time.perf_counter()) consistently while
keeping wall-clock timestamps as time.time().
- Around line 520-521: The code reads schedule/timezone/brightness from the
snapshot stored in self.config (e.g., the schedule_config assignment and the
other occurrences noted) which prevents hot-reloads; fix by fetching the live
config at each check or by subscribing DisplayController to ConfigService
updates and updating self.config on change: either replace self.config.get(...)
calls with the runtime fetch (e.g., self.config_service.get('schedule') /
get('timezone') / get('brightness')) where schedule_config is computed, or add a
registration in __init__ to listen to ConfigService change events and refresh
self.config before these checks so the logic in the methods that reference
schedule_config and the other occurrences uses the latest configuration.
🪄 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: de00afe6-1ff1-48db-b807-cc6654488a5b
📒 Files selected for processing (7)
config/config.template.jsonsrc/common/sync_manager.pysrc/display_controller.pysrc/vegas_mode/coordinator.pysrc/vegas_mode/render_pipeline.pyweb_interface/blueprints/api_v3.pyweb_interface/templates/v3/partials/display.html
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
- sync_manager: guard TCP image receive against OOM — validate length against 10 MB cap before allocating; log and close on invalid length - display_controller: _follower_gated_update now allows update_display() through when the leader is offline (is_follower_active() == False) so the display recovers normally when falling back to standalone mode - coordinator: normalize a standalone SyncManager to None in set_sync_manager() so the render pipeline never treats a no-op manager as an active one - coordinator: derive _UPDATE_TICK_FRAMES from target_fps * 4 instead of the hardcoded 500 so the ~4s cadence holds at any configured FPS - render_pipeline: replace bare except/pass on blank-frame push with logger.exception() so failures are visible in logs Skip: config.template.json comments — JSON does not support inline comments. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
sarjent
commented
May 13, 2026
ContributorAuthor
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/common/sync_manager.py (4)
624-632: 💤 Low value
stop()swallows close errors silently.Ruff flags S110 (try/except/pass) here. A debug-level log keeps the shutdown path noise-free while still surfacing the cause if a socket misbehaves during teardown.
♻️ Proposed fix
for sock in (self._recv_sock, self._send_sock, self._img_server_sock): if sock: try: sock.close() - except Exception:- pass+ except OSError as exc:+ self.logger.debug("Sync: socket close error during stop: %s", exc)🤖 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/common/sync_manager.py` around lines 624 - 632, The stop() method currently swallows socket.close() exceptions; update it to log those exceptions at debug level instead of silently passing. In the loop over self._recv_sock, self._send_sock, self._img_server_sock (inside stop()), catch Exception as e and call the module/class logger (e.g. self._logger.debug or logging.getLogger(__name__).debug) with a short message and include the exception details (use exc_info=True or include str(e)) so close failures are visible in debug logs but do not raise.
44-44: 💤 Low valueUse
tempfile.gettempdir()instead of a hardcoded/tmppath.Hardcoding
/tmp/led_matrix_sync_status.jsonis flagged by Ruff (S108) and is a predictable path on a shared filesystem. The.tmpsibling used inwrite_status_file()is opened withopen(tmp, "w"), which follows symlinks — a pre-staged symlink at/tmp/led_matrix_sync_status.json.tmpcould redirect writes. Use a per-user/runtime directory instead.♻️ Proposed fix
-STATUS_FILE = "/tmp/led_matrix_sync_status.json"+import tempfile+STATUS_FILE = os.path.join(tempfile.gettempdir(), "led_matrix_sync_status.json")🤖 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/common/sync_manager.py` at line 44, Replace the hardcoded STATUS_FILE with a path built from tempfile.gettempdir() (e.g. os.path.join(tempfile.gettempdir(), "led_matrix_sync_status.json")) and update write_status_file() to perform an atomic, symlink-safe write: create a temp file in that same tempdir (use tempfile.NamedTemporaryFile or tempfile.mkstemp with dir=tmpdir), write to it, fsync, then os.replace() to move it to STATUS_FILE; reference STATUS_FILE and write_status_file() when applying these changes.
441-468: 💤 Low valueBrittle frame-vs-control discrimination via
len(data) > 512.Routing by datagram size means a small raw frame (e.g. very narrow chain, < ~500 bytes raw RGB) is mis-routed into the JSON branch and silently dropped, and any future control message that grows past 512 bytes is mis-routed into the frame decoder. For current panel sizes this is benign, but the
_RAW_MAGICbyte pattern is exactly the disambiguator you already have — check it first regardless of size, and uselen(data) > 512only as a hint for legacy PNG fallback.♻️ Proposed fix
- if len(data) > 512:- # Raw RGB frame: magic(8) + width/height(4) + pixels- try:- if data[:8] == _RAW_MAGIC:+ if data[:8] == _RAW_MAGIC or len(data) > 512:+ # Raw RGB frame (magic-prefixed) or legacy PNG payload+ try:+ if data[:8] == _RAW_MAGIC: w, h = _RAW_HEADER.unpack(data[8:12]) raw = data[12:] img = Image.frombuffer( "RGB", (w, h), raw, "raw", "RGB", 0, 1 ) else: # Fallback: try legacy PNG img = Image.open(io.BytesIO(data)) img.load()🤖 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/common/sync_manager.py` around lines 441 - 468, The code currently decides frame vs control by `len(data) > 512`, which misroutes small raw frames or large control messages; change the logic in the frame handling block to first check the magic token `_RAW_MAGIC` (i.e. test `data[:8] == _RAW_MAGIC`) and treat it as a raw RGB frame (use `_RAW_HEADER.unpack` and `Image.frombuffer` as already implemented), only use `len(data) > 512` as a secondary hint to attempt legacy PNG fallback (i.e. if not `_RAW_MAGIC` and length is large try `Image.open`), and keep the existing exception handling and follower-state updates (references: `_RAW_MAGIC`, `_RAW_HEADER`, Image.frombuffer, Image.open, `_latest_frame`, `_frame_lock`, `_follower_state`).
241-289: 💤 Low valueGuard against PIL decompression-bomb / huge canvas allocations.
The 10 MB cap bounds the compressed payload, but a well-crafted PNG can still expand to a multi-gigapixel canvas during
img.load()on line 273, which on a Pi will OOM the process well before Pillow's defaultMAX_IMAGE_PIXELSwarning fires. Since the leader's scroll image dimensions are predictable (≈ panel rows × scroll width), validate dimensions beforeload().🛡️ Proposed fix
- img = Image.open(io.BytesIO(data))- img.load()+ img = Image.open(io.BytesIO(data))+ # Sanity-check dimensions before allocating the pixel buffer+ max_w, max_h = 20000, 256+ if img.width > max_w or img.height > max_h:+ self.logger.warning(+ "Sync: rejected scroll image with implausible dims %dx%d",+ img.width, img.height,+ )+ continue+ img.load()As per coding guidelines, "Optimize code for Raspberry Pi's limited RAM and CPU capabilities" and "Validate inputs and handle errors early (Fail Fast principle)".
🤖 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/common/sync_manager.py` around lines 241 - 289, The image server currently opens the uploaded image and calls img.load() without validating its pixel dimensions, which allows a crafted PNG to decompress to a massive canvas and OOM the Pi; update _image_server_loop so after creating img = Image.open(io.BytesIO(data)) but before img.load() you validate img.size (width, height) against a safe threshold (e.g. compute from expected scroll dimensions like panel rows × scroll width or a MAX_SCROLL_PIXELS constant) and reject/log and close the connection if width*height or either dimension exceeds that threshold; also handle PIL.Image.DecompressionBombError and ValueError around img.load() and optionally set/temporarily enforce Image.MAX_IMAGE_PIXELS to a lower safe value, making sure to still call the existing self._on_scroll_image and set self._pending_scroll_image only for accepted images.
🤖 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 `@src/common/sync_manager.py`:
- Around line 300-314: The send_scroll_image code leaks the socket if
connect()/sendall() raises; update the send logic in send_scroll_image to ensure
the socket is always closed by using a context manager (use "with
socket.socket(...)" or try/finally closing the socket) around the socket
creation/usage, keep the same timeout/sends, and retain the existing logging
(self.logger.info on success and self.logger.debug on exception) so resources
are cleaned even when connect/send fails.
- Around line 278-282: The race occurs because _image_server_loop and
set_on_scroll_image access _on_scroll_image and _pending_scroll_image without
synchronization; fix it by introducing a dedicated lock (e.g.,
self._scroll_image_lock) and use it to serialize all reads/writes to
_on_scroll_image and _pending_scroll_image inside both _image_server_loop and
set_on_scroll_image so the check/assign sequence is atomic (acquire lock, check
_on_scroll_image, deliver or set _pending_scroll_image, release lock); ensure
any cached pending image is handed to the newly registered callback while still
holding the lock to avoid the lost-interleaving scenario.
---
Nitpick comments:
In `@src/common/sync_manager.py`:
- Around line 624-632: The stop() method currently swallows socket.close()
exceptions; update it to log those exceptions at debug level instead of silently
passing. In the loop over self._recv_sock, self._send_sock,
self._img_server_sock (inside stop()), catch Exception as e and call the
module/class logger (e.g. self._logger.debug or
logging.getLogger(__name__).debug) with a short message and include the
exception details (use exc_info=True or include str(e)) so close failures are
visible in debug logs but do not raise.
- Line 44: Replace the hardcoded STATUS_FILE with a path built from
tempfile.gettempdir() (e.g. os.path.join(tempfile.gettempdir(),
"led_matrix_sync_status.json")) and update write_status_file() to perform an
atomic, symlink-safe write: create a temp file in that same tempdir (use
tempfile.NamedTemporaryFile or tempfile.mkstemp with dir=tmpdir), write to it,
fsync, then os.replace() to move it to STATUS_FILE; reference STATUS_FILE and
write_status_file() when applying these changes.
- Around line 441-468: The code currently decides frame vs control by `len(data)
> 512`, which misroutes small raw frames or large control messages; change the
logic in the frame handling block to first check the magic token `_RAW_MAGIC`
(i.e. test `data[:8] == _RAW_MAGIC`) and treat it as a raw RGB frame (use
`_RAW_HEADER.unpack` and `Image.frombuffer` as already implemented), only use
`len(data) > 512` as a secondary hint to attempt legacy PNG fallback (i.e. if
not `_RAW_MAGIC` and length is large try `Image.open`), and keep the existing
exception handling and follower-state updates (references: `_RAW_MAGIC`,
`_RAW_HEADER`, Image.frombuffer, Image.open, `_latest_frame`, `_frame_lock`,
`_follower_state`).
- Around line 241-289: The image server currently opens the uploaded image and
calls img.load() without validating its pixel dimensions, which allows a crafted
PNG to decompress to a massive canvas and OOM the Pi; update _image_server_loop
so after creating img = Image.open(io.BytesIO(data)) but before img.load() you
validate img.size (width, height) against a safe threshold (e.g. compute from
expected scroll dimensions like panel rows × scroll width or a MAX_SCROLL_PIXELS
constant) and reject/log and close the connection if width*height or either
dimension exceeds that threshold; also handle PIL.Image.DecompressionBombError
and ValueError around img.load() and optionally set/temporarily enforce
Image.MAX_IMAGE_PIXELS to a lower safe value, making sure to still call the
existing self._on_scroll_image and set self._pending_scroll_image only for
accepted images.
🪄 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: 03d9eb26-a0c5-4869-8197-a717f8cdc32f
📒 Files selected for processing (4)
src/common/sync_manager.pysrc/display_controller.pysrc/vegas_mode/coordinator.pysrc/vegas_mode/render_pipeline.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/vegas_mode/render_pipeline.py
- src/vegas_mode/coordinator.py
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
- sync_manager: use 'with socket.socket(...)' in send_scroll_image so the TCP socket is always closed even if connect/sendall raises - sync_manager: add _scroll_image_lock to serialize all reads/writes to _on_scroll_image and _pending_scroll_image between _image_server_loop and set_on_scroll_image, eliminating the lost-delivery race; callback is invoked outside the lock to avoid holding it during user code - sync_manager: validate scroll image dimensions (max 100000×256) and catch DecompressionBombError before img.load() in _image_server_loop - sync_manager: log socket close exceptions at debug level in stop() instead of silently passing - sync_manager: replace hardcoded /tmp/ with tempfile.gettempdir() for STATUS_FILE (atomic write was already in place) - sync_manager: check _RAW_MAGIC first in _follower_recv_loop routing so magic-tagged frames are always identified correctly regardless of size Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
sarjent
commented
May 13, 2026
ContributorAuthor
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
sarjent
commented
May 14, 2026
ContributorAuthor
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/common/sync_manager.py (1)
268-273: ⚡ Quick winUse
bytearrayinstead of repeatedbytesconcat for the TCP image buffer.With
_MAX_IMAGE_BYTES = 10 MBand 64 KB chunks,data = b""+data += chunkis O(n²) — each+=allocates a freshbytesand copies the running total (cumulative ~tens of GB of copying for a worst-case payload). Switching tobytearray.extendis O(n) and noticeably gentler on the Pi's RAM/CPU;Image.open(io.BytesIO(data))accepts abytearrayunchanged.♻️ Proposed refactor
- data = b""+ data = bytearray() while len(data) < length: chunk = conn.recv(min(65536, length - len(data))) if not chunk: break - data += chunk+ data.extend(chunk) img = Image.open(io.BytesIO(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 `@src/common/sync_manager.py` around lines 268 - 273, The loop that accumulates TCP image bytes uses immutable bytes and repeated concatenation (data = b""; data += chunk) which is O(n²); change the buffer to a mutable bytearray (initialize data = bytearray()) and append each recv() using data.extend(chunk) inside the same loop that reads from conn with length, then pass the resulting bytearray to Image.open/io.BytesIO as before (io.BytesIO accepts a bytearray) — update the variables referenced in this block (data, chunk, conn, length) accordingly.
🤖 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 `@src/common/sync_manager.py`:
- Around line 207-214: The code sets self._leader_state =
LeaderState.INCOMPATIBLE and assigns self._error_message but always calls
self.logger.error(...) on every hello; move the logging call into the same
state-transition guard that checks prev_state != self._leader_state (the same
place used for CONNECTED logging) so that self.logger.error(self._error_message)
runs only when the leader state actually changes; update references to
self._leader_state, self._error_message and prev_state accordingly to ensure the
error is emitted only on state transitions.
---
Nitpick comments:
In `@src/common/sync_manager.py`:
- Around line 268-273: The loop that accumulates TCP image bytes uses immutable
bytes and repeated concatenation (data = b""; data += chunk) which is O(n²);
change the buffer to a mutable bytearray (initialize data = bytearray()) and
append each recv() using data.extend(chunk) inside the same loop that reads from
conn with length, then pass the resulting bytearray to Image.open/io.BytesIO as
before (io.BytesIO accepts a bytearray) — update the variables referenced in
this block (data, chunk, conn, length) accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Uh oh!
There was an error while loading. Please reload this page.
- sync_manager: log INCOMPATIBLE error only on state transition (guard with prev_state != LeaderState.INCOMPATIBLE) so repeated hello packets from an incompatible follower don't spam the log - sync_manager: replace O(n²) bytes concatenation in TCP image receive loop with bytearray + extend() for linear-time accumulation Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- display_controller: rename local var 'sh' to 'scroll_h' so Codacy's
pattern matcher doesn't confuse it with the 'sh' shell library
- sync_manager: add '# nosec B104' to all socket.bind("") calls —
binding to all interfaces is intentional (UDP broadcast reception and
TCP image server must accept connections from any local interface)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>Codacy attributes the bind-to-all-interfaces finding to the socket.socket() creation lines (140, 439) rather than the .bind() calls. Added # nosec B104 there too so the suppression is seen at the line Codacy reports. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Uh oh!
There was an error while loading. Please reload this page.
5 tasks
2 tasks
ChuckBuilds added a commit
that referenced
this pull request
Jun 10, 2026
…367) When the display loop breaks early because current_display_mode changed (on-demand activation, live priority, etc.), it would fall through to the "honour minimum duration" sleep for the *previous* mode — blocking for up to that mode's full display_duration (default 30s) without polling on-demand requests or re-checking the mode. New modes could sit unrendered for up to 30s, or get clobbered by a queued stop request before ever displaying. This guard was added in #298 to fix#196 (live priority not interrupting long display durations) and was accidentally dropped in #330 as collateral damage of an unrelated time.monotonic() -> time.time() cleanup in the same diff hunk. Restoring it fixes both the original #196 regression and a new symptom found via the on-air MQTT plugin, where ON/OFF toggles could be delayed by up to 30s or missed entirely depending on timing within the previous mode's display cycle. Co-authored-by: Chuck <chuck@example.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
15 tasks
This was referenced Jul 11, 2026
ChuckBuilds added a commit
that referenced
this pull request
Jul 12, 2026
…or (#395) Investigating a user report that Vegas scroll mode doesn't update scores or game status. Root cause: PR #299 (Mar 28) added a mechanism so a live score change reached the ticker within a few seconds instead of waiting for a full scroll cycle -- _tick_plugin_updates_for_vegas() diffed plugin_last_update timestamps to detect which plugins got fresh data and called coordinator.mark_plugin_updated() for each, and should_recompose() checked has_pending_updates_for_visible_segments() to trigger an immediate hot-swap. PR #330 (May 14, multi-display wireless sync) refactored both call sites while adding sync support and silently deleted this entire mechanism -- not just gated it behind the new sync-mode deferral it legitimately needed, but removed it outright. The result: VegasModeCoordinator. mark_plugin_updated() and StreamManager.has_pending_updates_for_visible_ segments() have been fully implemented but never called from anywhere since. Vegas mode's only remaining freshness sources are a 5s content cache TTL (fine) and full recompose at cycle boundaries, which depending on min/max_cycle_duration can be minutes away -- so live scores/status can sit stale far longer than a user would expect from a "live" ticker. Fix: - Restored _tick_plugin_updates_for_vegas() in display_controller.py, wired as the Vegas coordinator's update callback in place of the plain _tick_plugin_updates(). Diffs plugin_last_update before/after the tick and calls vegas_coordinator.mark_plugin_updated(plugin_id) for each plugin that actually got new data (rather than returning the list, since the callback interface no longer consumes a return value). - Restored the has_pending_updates_for_visible_segments() check in render_pipeline.should_recompose(), positioned after (not instead of) the sync-mode early return PR #330 added, so standalone installations regain immediate refresh while synced leader/follower pairs correctly keep deferring hot-swaps to cycle boundaries as PR #330 intended. Test plan: - Added test_display_controller_vegas_tick.py and test_vegas_render_pipeline_recompose.py -- neither area had any prior test coverage, which is very likely why this regression went unnoticed for ~2.5 months. - Verified both new test files fail against the pre-fix code (swapped in the current main versions of both files) with exactly the expected errors -- AttributeError for the deleted method, and the recompose assertion returning False instead of True -- then pass against the fix. - Confirmed the sync-mode deferral this restoration must not break still holds: test_sync_active_defers_pending_updates_to_cycle_boundary. - Full related suite (test_vegas_plugin_adapter, test_vegas_config, test_display_controller_plugin_toggle, test_display_controller_ optimizations, test_plugin_system): 108 passed, 1 pre-existing failure unrelated to this change (test_circuit_breaker, stale mock signature). - Full CI plugin-safety suite (test_harness, test_visual_rendering, test_plugin_matrix): 52 passed, 2 pre-existing skips.
ChuckBuilds added a commit
that referenced
this pull request
Jul 12, 2026
…mutation (#398) * fix(vegas): restore live plugin-update refresh dropped by sync refactor Investigating a user report that Vegas scroll mode doesn't update scores or game status. Root cause: PR #299 (Mar 28) added a mechanism so a live score change reached the ticker within a few seconds instead of waiting for a full scroll cycle -- _tick_plugin_updates_for_vegas() diffed plugin_last_update timestamps to detect which plugins got fresh data and called coordinator.mark_plugin_updated() for each, and should_recompose() checked has_pending_updates_for_visible_segments() to trigger an immediate hot-swap. PR #330 (May 14, multi-display wireless sync) refactored both call sites while adding sync support and silently deleted this entire mechanism -- not just gated it behind the new sync-mode deferral it legitimately needed, but removed it outright. The result: VegasModeCoordinator. mark_plugin_updated() and StreamManager.has_pending_updates_for_visible_ segments() have been fully implemented but never called from anywhere since. Vegas mode's only remaining freshness sources are a 5s content cache TTL (fine) and full recompose at cycle boundaries, which depending on min/max_cycle_duration can be minutes away -- so live scores/status can sit stale far longer than a user would expect from a "live" ticker. Fix: - Restored _tick_plugin_updates_for_vegas() in display_controller.py, wired as the Vegas coordinator's update callback in place of the plain _tick_plugin_updates(). Diffs plugin_last_update before/after the tick and calls vegas_coordinator.mark_plugin_updated(plugin_id) for each plugin that actually got new data (rather than returning the list, since the callback interface no longer consumes a return value). - Restored the has_pending_updates_for_visible_segments() check in render_pipeline.should_recompose(), positioned after (not instead of) the sync-mode early return PR #330 added, so standalone installations regain immediate refresh while synced leader/follower pairs correctly keep deferring hot-swaps to cycle boundaries as PR #330 intended. Test plan: - Added test_display_controller_vegas_tick.py and test_vegas_render_pipeline_recompose.py -- neither area had any prior test coverage, which is very likely why this regression went unnoticed for ~2.5 months. - Verified both new test files fail against the pre-fix code (swapped in the current main versions of both files) with exactly the expected errors -- AttributeError for the deleted method, and the recompose assertion returning False instead of True -- then pass against the fix. - Confirmed the sync-mode deferral this restoration must not break still holds: test_sync_active_defers_pending_updates_to_cycle_boundary. - Full related suite (test_vegas_plugin_adapter, test_vegas_config, test_display_controller_plugin_toggle, test_display_controller_ optimizations, test_plugin_system): 108 passed, 1 pre-existing failure unrelated to this change (test_circuit_breaker, stale mock signature). - Full CI plugin-safety suite (test_harness, test_visual_rendering, test_plugin_matrix): 52 passed, 2 pre-existing skips. * fix(vegas): lock plugin_last_update snapshot/diff against concurrent mutation _tick_plugin_updates_for_vegas() snapshotted and later re-iterated plugin_manager.plugin_last_update from the Vegas background update-tick thread while the main render loop (or other callers) could mutate the same dict concurrently — a real race (unprotected dict iteration/mutation across threads), not just a style nit. Move the snapshot/update/diff into a new locked PluginManager.run_scheduled_updates_with_changes() so all reads and mutations of plugin_last_update happen under one lock, and update DisplayController to use it. The lock is only held around the dict accesses, not the update pass itself, so slow plugin update() calls don't serialize against other callers. Also add a regression test covering that the Vegas coordinator is wired to the Vegas-aware tick callback rather than the plain one. Skipped as not worth the change: - Narrowing the broad `except Exception` around vc.mark_plugin_updated(plugin_id) to specific types: it's a deliberate per-plugin isolation boundary (matches the same pattern used elsewhere in this file for plugin/coordinator calls) and there's no documented, stable set of exceptions that call can raise to narrow to. - Adding an inactive-DisplaySyncManager test to test_vegas_render_pipeline_recompose.py: verified VegasModeCoordinator.set_sync_manager() already normalizes a SyncRole.STANDALONE manager to None before handing it to the render pipeline (src/vegas_mode/coordinator.py:152-156), so should_recompose()'s `is not None` check is correct in practice; the suggested case is already covered by that normalization. --------- Co-authored-by: Claude <noreply@anthropic.com>
ChuckBuilds added a commit
that referenced
this pull request
Sep 7, 2026
…works The Pixlet button was the reported symptom; the app store is dead the same way. #330 dropped all thirteen Starlark routes, and browse, categories and install are what the store page is built on -- each answering the generic 404, which from the UI is indistinguishable from an empty store. Restores the other eleven from 1c4d5c5^: apps list and detail, delete, per-app config get/put, toggle, render, manual .star upload, and the Tronbyte repository browse/categories/install. Their dependency closure came with them (8 helpers, resolved by walking the handlers' references rather than by eye), plus the Tuple and Type imports the old file had. Two changes rather than a straight revert. The restored handlers used `request.get_json()` where the file has since standardised on `silent=True`: without it Werkzeug raises on a bodyless POST before the handler's own `if not data` guard runs, so the caller gets a framework error instead of the declared 400. test_api_v3_optional_body.py already checks for exactly that and caught all three. test_every_endpoint_the_frontend_calls_is_registered reads the URLs out of plugins_manager.js and matches each through the URL map, so a rewrite of this file cannot quietly drop the set again -- one assertion over the frontend's own list is what would have caught #330. 26 tests, 17 of which fail against main. Full core suite: 3973 passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9
ChuckBuilds added a commit
that referenced
this pull request
Sep 7, 2026
…em toggle An app installed from the store appeared nowhere and could not be enabled or disabled. Same cause as the 404s: #253 surfaced installed apps in /plugins/installed as `starlark:<app_id>` entries and routed `starlark:` toggles to the Starlark manifest, and #330 removed both along with the routes. Without the first, the app store installs successfully into a list nothing renders. Without the second, toggling one falls through to the plugin_manager lookup and answers "Plugin not found" -- a Starlark app is an entry in starlark-apps' own manifest, not a plugin in that sense. Restored as two named helpers rather than the original inline blocks: _starlark_virtual_plugins() reads the loaded plugin when there is one and the on-disk manifest otherwise, so the list is right before starlark-apps loads _toggle_starlark_app() updates through the plugin's own _update_manifest_safe when loaded, or the manifest directly when not Two changes on the original. The toggle now runs app_id through _validate_and_sanitize_app_id first -- it reaches a filesystem manifest and had no validation where the other Starlark routes all have it. And _starlark_virtual_plugins swallows its own failures: the entries are appended to the real plugin list, and a broken Starlark manifest should cost the Starlark rows, not empty the plugins page. 6 new tests covering listing, the fields the UI keys on, toggle persistence, the unknown-app 404, traversal rejection, and that a Starlark failure leaves the rest of the list intact. Full core suite: 3979 passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9
ChuckBuilds added a commit
that referenced
this pull request
Sep 7, 2026
The Pixlet install button reported "Pixlet install failed: Resource not found" -- Flask's 404 handler, because the route did not exist. #253 added thirteen Starlark routes; #330 rewrote api_v3.py and dropped all of them, along with the `starlark:<app_id>` entries that surface installed apps in the plugins list and the toggle branch that enables them. Restores all thirteen routes, the plugin-list entries and the toggle path, so Pixlet installs, the app store browses and installs, and an installed app can be managed like any other plugin. Not a straight revert. Three error paths stopped returning exception text to the caller; the manifest write moved off a shared temp filename that two concurrent writers could interleave; both dynamic importers stopped leaving half-initialised modules in sys.modules; the config update rolls back when the save fails; the toggle checks that persistence succeeded; and the path check returns the validated path instead of a boolean so callers stop re-joining the raw value. New tests no longer reach GitHub. Verified on a 256x64 Pi: Pixlet installs and runs (v0.53.1), the store lists 1000 apps, install/toggle/uninstall round-trip, and traversal and command-injection probes are rejected at every entry. 25 CodeQL alerts dismissed as verified false positives -- path-injection where traversal is blocked, and one list-form subprocess with no shell. Both classes already present on main. Full core suite: 3981 passed.
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 freeto 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
Architecture
Both Pis must have identical
rowsandcols.chain_lengthmay differ.Configuration
Set in Display Settings → Multi-Display Sync, or directly in
config.json:Files changed
src/common/sync_manager.pysrc/display_controller.pysrc/vegas_mode/render_pipeline.pydisplay_width, TCP image sendsrc/vegas_mode/coordinator.pyset_sync_manager(),set_update_callback(), hot-swap deferral during syncweb_interface/blueprints/api_v3.pyGET /api/v3/sync/statusendpointweb_interface/templates/v3/partials/display.htmlconfig/config.template.jsonsyncconfig blockTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Behavior
UX