wled-screen-controller-mixed/REVIEW.md

Codebase Review Report

Generated 2026-03-09

---

1. Bugs (Critical)

Thread Safety / Race Conditions

Issue	Location	Description
Dict mutation during iteration	composite_stream.py:121, mapped_stream.py:102	update_source() calls _sub_streams.clear() from the API thread while _processing_loop iterates the dict on a background thread. Will crash with RuntimeError: dictionary changed size during iteration.
Clock ref-count corruption	color_strip_stream_manager.py:286-304	On clock hot-swap, _release_clock reads the new clock_id from the store (already updated), so it releases the newly acquired clock instead of the old one. Leaks the old runtime, destroys the new one.
SyncClockRuntime race	sync_clock_runtime.py:42-49	get_time() reads _running, _offset, _epoch without _lock, while pause()/resume()/reset() modify them under _lock. Compound read can double-count elapsed time.
SyncClockManager unprotected dicts	sync_clock_manager.py:26-54	_runtimes and _ref_counts are plain dicts mutated from both the async event loop and background threads with no lock.

Silent Failures

Issue	Location	Description
Crashed streams go undetected	mapped_stream.py:214, composite_stream.py	When the processing loop dies, get_latest_colors() permanently returns stale data. The target keeps sending frozen colors to LEDs with no indicator anything is wrong.
Crash doesn't fire state_change event	wled_target_processor.py:900	Fatal exception path sets _is_running = False without firing state_change event (only stop() fires it). Dashboard doesn't learn about crashes via WebSocket.
WebSocket broadcast client mismatch	kc_target_processor.py:481-485	zip(self._ws_clients, results) pairs results with the live list, but clients can be removed between scheduling gather and collecting results, causing wrong clients to be dropped.

Security

Issue	Location	Description
Incomplete path traversal guard	auto_backup.py	Filename validation uses string checks (".." in filename) instead of Path.resolve().is_relative_to().

---

2. Performance

High Impact (Hot Path)

Issue	Location	Impact
Per-frame np.array() from list	ddp_client.py:195	Allocates a new numpy array from a Python list every frame. Should use pre-allocated buffer.
Triple FFT for mono audio	analysis.py:168-174	When audio is mono (common for system loopback), runs 3 identical FFTs. 2x wasted CPU.
frame_time = 1.0/fps in every loop iteration	8 stream files	Recomputed every frame despite _fps only changing on consumer subscribe. Should be cached.
4x deque traversals per frame for metrics	kc_target_processor.py:413-416	Full traversal of metrics deques every frame to compute avg/min/max.
3x spectrum .copy() per audio chunk	analysis.py:195-201	~258 array allocations/sec for read-only consumers. Could use non-writable views.

Medium Impact

Issue	Location
getattr + dict lookup per composite layer per frame	composite_stream.py:299-304
Unconditional self.*= attribute writes every frame in audio stream	audio_stream.py:255-261
JSON.parse(localStorage) on every collapsed-section call	dashboard.js _getCollapsedSections
Effect/composite/mapped streams hardcoded to 30 FPS	effect_stream.py, composite_stream.py:37, mapped_stream.py:33
Double querySelectorAll on card reconcile	card-sections.js:229-232
Module import inside per-second sampling function	metrics_history.py:21,35
datetime.utcnow() twice per frame	kc_target_processor.py:420,464
Redundant bytes() copy of bytes slice	ddp_client.py:222
Unnecessary .copy() of temp interp result	audio_stream.py:331,342
Multiple intermediate numpy allocs for luminance	value_stream.py:486-494

---

3. Code Quality

Architecture

Issue	Description
12 store classes with duplicated boilerplate	All JSON stores repeat the same load/save/CRUD pattern with no base class. A BaseJsonStore[T] would eliminate ~60% of each store file.
DeviceStore.save() uses unsafe temp file	Fixed-path temp file instead of atomic_write_json used by all other stores.
scene_activator.py accesses ProcessorManager._processors directly	Lines 33, 68, 90, 110 — bypasses public API, breaks encapsulation.
Route code directly mutates ProcessorManager internals	devices.py accesses manager._devices and manager._color_strip_stream_manager in 13+ places.
color-strips.js is 1900+ lines	Handles 11 CSS source types, gradient editor, composite layers, mapped zones, card rendering, overlay control — should be split.
No DataCache for color strip sources	Every other entity uses DataCache. CSS sources are fetched with raw fetchWithAuth in 5+ places with no deduplication.

Consistency / Hygiene

Issue	Location
Dict[str, any] (lowercase any) — invalid type annotation	template_store.py:138,187, audio_template_store.py:126,155
datetime.utcnow() deprecated — 88 call sites in 42 files	Project-wide
_icon SVG helper duplicated verbatim in 3 JS files	color-strips.js:293, automations.js:41, kc-targets.js:49
hexToRgbArray private to one file, pattern inlined elsewhere	color-strips.js:471 vs line 1403
Hardcoded English fallback in showToast	color-strips.js:1593
ColorStripStore.create_source silently creates wrong type for unknown source_type	color_strip_store.py:92-332
update_source clock_id clearing uses undocumented empty-string sentinel	color_strip_store.py:394-395
DeviceStore._load lacks per-item error isolation (unlike all other stores)	device_store.py:122-138
No unit tests	Zero test files. Highest-risk: CalibrationConfig/PixelMapper geometry, DDP packets, automation conditions.

---

4. Features & Suggestions

High Impact / Low Effort

Suggestion	Details
Auto-restart crashed processing loops	Add backoff-based restart when _processing_loop dies. Currently crashes are permanent until manual intervention.
Fire state_change on crash	Add finally block in _processing_loop to notify the dashboard immediately.
POST /system/auto-backup/trigger	~5 lines of Python. Manual backup trigger before risky config changes.
is_healthy property on streams	Let target processors detect when their color source has died.
Rotate webhook token endpoint	POST /automations/{id}/rotate-webhook-token — regenerate without recreating automation.
"Start All" targets button	"Stop All" exists but "Start All" (the more common operation after restart) is missing.
Include auto-backup settings in backup	Currently lost on restore.
Distinguish "crashed" vs "stopped" in dashboard	metrics.last_error is already populated — just surface it.

High Impact / Moderate Effort

Suggestion	Details
Home Assistant MQTT discovery	Publish auto-discovery payloads so devices appear in HA automatically. MQTT infra already exists.
Device health WebSocket events	Eliminates 5-30s poll latency for online/offline detection.
GET /system/store-errors	Surface startup deserialization failures to the user. Currently only in logs.
Scene snapshot should capture device brightness	software_brightness is not saved/restored by scenes.
Exponential backoff on events WebSocket reconnect	Currently fixed 3s retry, generates constant logs during outages.
CSS source import/export	Share individual sources without full config backup.
Per-target error ring buffer via API	GET /targets/{id}/logs for remote debugging.
DDP socket reconnection	UDP socket invalidated on network changes; no reconnect path exists.
Adalight serial reconnection	COM port disconnect crashes the target permanently.
MQTT-controlled brightness and scene activation	Direct command handler without requiring API key management.

Nice-to-Have

Suggestion	Details
Configurable metrics history window (currently hardcoded 120 samples / 2 min)
Replace window.prompt() API key entry with proper modal
Pattern template live preview (SVG/Canvas)
Keyboard shortcuts for start/stop targets and scene activation
FPS chart auto-scaling y-axis (Math.max(target*1.15, maxSeen*1.1))
WLED native preset target type (send {"ps": id} instead of pixels)
Configurable DDP max packet size per device
GET /system/active-streams unified runtime snapshot
OpenMetrics / Prometheus endpoint for Grafana integration
Configurable health check intervals (currently hardcoded 10s/60s)
Configurable backup directory path
GET /system/logs?tail=100&level=ERROR for in-app log viewing
Device card "currently streaming" badge