alexei.dolgolyov
30fa107ef7
- Add `tags: List[str]` field to all 13 entity types (devices, output targets, CSS sources, picture sources, audio sources, value sources, sync clocks, automations, scene presets, capture/audio/PP/pattern templates) - Update all stores, schemas, and route handlers for tag CRUD - Add GET /api/v1/tags endpoint aggregating unique tags across all stores - Create TagInput component with chip display, autocomplete dropdown, keyboard navigation, and API-backed suggestions - Display tag chips on all entity cards (searchable via existing text filter) - Add tag input to all 14 editor modals with dirty check support - Add CSS styles and i18n keys (en/ru/zh) for tag UI - Also includes code review fixes: thread safety, perf, store dedup Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
139 lines
9.2 KiB
Markdown
139 lines
9.2 KiB
Markdown
# 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 | |
|