Conversation
📝 WalkthroughWalkthroughThis PR introduces end-to-end Art-Net DMX support, including a UDP server for receiving and mapping Art-Net packets to runtime parameters, persistent configuration management, API endpoints for status and control, frontend UI for configuration, and event streaming for real-time parameter updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (Browser)
participant Server as FastAPI Server
participant DMX as DMX Server
participant UDP as UDP Socket
participant WebRTC as WebRTC Manager
participant Params as Runtime Parameters
UDP->>DMX: Art-Net packets (universe, channels)
DMX->>DMX: Parse packet & extract channels
DMX->>DMX: Apply channel→parameter mappings
DMX->>DMX: Scale DMX (0-255) to param min/max
DMX->>DMX: Throttle (~60Hz) & deduplicate
DMX->>Params: Update parameter values
DMX->>WebRTC: Broadcast parameter updates
DMX->>Client: Send SSE event (dmx/stream)
Client->>Server: GET /api/v1/dmx/status
Server->>DMX: Get status (port, listening, config)
DMX-->>Server: Status response
Server-->>Client: JSON response
Client->>Server: PUT /api/v1/dmx/config
Server->>DMX: Update mappings & settings
DMX->>DMX: Apply new mappings to running server
DMX-->>Server: Config confirmed
Server-->>Client: Updated config response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
🚀 fal.ai Preview Deployment
TestingConnect to this preview deployment by running this on your branch: 🧪 E2E tests will run automatically against this deployment. |
✅ E2E Tests passed
Test ArtifactsCheck the workflow run for screenshots. |
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/settings/DmxTab.tsx`:
- Around line 81-106: fetchAll currently overwrites in-progress edits by
unconditionally calling setLocalMappings, setLocalPort, and setDirty when the
tab becomes active; change fetchAll so it only updates localMappings and
localPort (from c.mappings and String(c.preferred_port)) if the user has no
unsaved edits (i.e., dirty is false). To do this, read the latest dirty state
inside fetchAll (either add dirty to fetchAll's dependency array or access a
dirtyRef that mirrors dirty) and wrap the setLocalMappings/setLocalPort/setDirty
calls in a conditional (if (!dirty) { ... }) so switching back to the tab
doesn’t discard in-progress edits. Ensure references to fetchAll, isActive, and
dirty remain consistent in the useEffect dependencies.
- Around line 131-159: handleToggleLogging currently updates runtime state via
updateDmxSettings and setStatus but never marks the form dirty; change
handleToggleLogging to call setDirty(true) when the toggle succeeds (use the
same updated response) so the logging change is persistable. In handleApplyPort,
don't unconditionally clear dirty or toast success: after calling applyDmxPort
check updated.listening (and use updated.port) before calling setDirty(false)
and toast.success; if updated.listening is false or missing, show a toast.error
and leave dirty true so mapping/logging edits aren’t hidden; keep
setIsApplyingPort finally to false and continue using setStatus(prev => (prev ?
{ ...prev, ...updated } : prev)) to merge state.
- Around line 182-226: The import path currently ignores the exported
log_all_messages value; update handleImport to read imported.log_all_messages
and, if it's a boolean, restore it into the same state you read from in
handleExport (the status.log_all_messages state) by calling the appropriate
setter used in this component (e.g., setStatus or the local setter that controls
the DMX logging flag); keep existing behavior for preferred_port/mappings, mark
form dirty with setDirty(true), and show the same success/error toasts.
In `@src/scope/server/app.py`:
- Around line 889-913: Validate and sanitize the incoming DMX config in
dmx_put_config before calling save_config: verify request.preferred_port is
either None or an int in the valid port range (1–65535) and raise
HTTPException(400) on bad input; normalize request.mappings by passing
request.mappings through mappings_to_dict() (or a dedicated sanitizer) and use
the normalized result when assigning cfg["mappings"] and when calling
srv.set_mappings; update cfg["preferred_port"] with the validated/normalized
value, save_config(cfg) with the cleaned config, and return the cleaned config
(import HTTPException from fastapi if needed).
In `@src/scope/server/dmx_config.py`:
- Around line 76-86: The save_config function currently swallows all exceptions
from writing the config (save_config and _config_path are the relevant symbols);
remove the broad try/except or at minimum re-raise after logging so write
failures propagate to callers instead of being silently ignored. Specifically,
eliminate the catch-all except block (or keep logger.exception(...) but follow
it with a raise) so that any error from path.write_text(...) surfaces to the API
handlers.
In `@src/scope/server/dmx_server.py`:
- Around line 184-186: Avoid rebuilding the full path inventory on every DMX
packet by caching the result of _get_known_paths() (e.g., store as
self._cached_known_paths and a small version/dirty flag like
self._known_paths_version). Change the hot-path call in dmx_server.py to return
the cached map and only call _get_known_paths() to refresh the cache when the
version is stale; add cache invalidation where pipeline/mapping state changes
(the functions that add/remove mappings or load/unload pipelines — the code
paths that currently trigger schema rebuilds in src/scope/server/dmx_docs.py) to
increment the version or set the dirty flag so the cache is recomputed once
rather than per-packet.
- Around line 187-205: The code treats configured channel "ch" as a zero-based
index when accessing dmx_data; change it to use 1-based DMX channel numbers by
computing an index = ch - 1 and use that for bounds-checking and access.
Specifically, in the loop over self._mappings (symbols: _mappings, uni, ch,
dmx_data, known_paths, param_key, raw_value) replace the current bounds check
and raw_value assignment with logic that skips if ch <= 0 or index >=
len(dmx_data), then set raw_value = dmx_data[index]; keep the rest of the
scaling logic unchanged.
- Around line 269-305: The _bound_port value is left set after stop() or after a
failed restart, causing status() to report a stale port; update stop() to clear
self._bound_port (set to None) when closing the socket and also ensure start()
resets self._bound_port to None before attempting binds (and after the bind loop
fails) so the cached port is never left pointing at an old/closed socket; adjust
references to self._bound_port in start, stop, and any status() method
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e7612cea-b81d-4feb-a581-e10c6ec7bf6f
📒 Files selected for processing (9)
app/src/services/pythonProcess.tsfrontend/src/components/SettingsDialog.tsxfrontend/src/components/settings/DmxTab.tsxfrontend/src/lib/api.tsfrontend/src/pages/StreamPage.tsxsrc/scope/server/app.pysrc/scope/server/dmx_config.pysrc/scope/server/dmx_docs.pysrc/scope/server/dmx_server.py
| const handleToggleLogging = async (checked: boolean) => { | ||
| try { | ||
| const updated = await updateDmxSettings({ log_all_messages: checked }); | ||
| setStatus(prev => (prev ? { ...prev, ...updated } : prev)); | ||
| } catch (err) { | ||
| toast.error("Failed to update DMX logging"); | ||
| console.error(err); | ||
| } | ||
| }; | ||
|
|
||
| const handleApplyPort = async () => { | ||
| const port = parseInt(localPort, 10); | ||
| if (isNaN(port) || port < 1024 || port > 65535) { | ||
| toast.error("Enter a valid port (1024–65535)"); | ||
| return; | ||
| } | ||
| setIsApplyingPort(true); | ||
| try { | ||
| const updated = await applyDmxPort(port); | ||
| setStatus(prev => (prev ? { ...prev, ...updated } : prev)); | ||
| setDirty(false); | ||
| toast.success(`DMX now listening on port ${updated.port ?? port}`); | ||
| } catch (err) { | ||
| toast.error("Failed to apply port"); | ||
| console.error(err); | ||
| } finally { | ||
| setIsApplyingPort(false); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Logging changes aren't persistable, and port apply can report false success.
handleToggleLogging() updates runtime state but never marks the form dirty, so that setting can't be saved on its own. handleApplyPort() then clears dirty and toasts success without checking updated.listening, which can hide unsaved mapping/logging edits and claim the port is live after a failed rebind.
Possible fix
const handleToggleLogging = async (checked: boolean) => {
try {
const updated = await updateDmxSettings({ log_all_messages: checked });
setStatus(prev => (prev ? { ...prev, ...updated } : prev));
+ setDirty(true);
} catch (err) {
toast.error("Failed to update DMX logging");
console.error(err);
}
};
@@
setIsApplyingPort(true);
try {
const updated = await applyDmxPort(port);
setStatus(prev => (prev ? { ...prev, ...updated } : prev));
- setDirty(false);
- toast.success(`DMX now listening on port ${updated.port ?? port}`);
+ if (!updated.listening || updated.port == null) {
+ toast.error("Failed to bind the DMX port");
+ return;
+ }
+ toast.success(`DMX now listening on port ${updated.port}`);
} catch (err) {
toast.error("Failed to apply port");
console.error(err);
} finally {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/settings/DmxTab.tsx` around lines 131 - 159,
handleToggleLogging currently updates runtime state via updateDmxSettings and
setStatus but never marks the form dirty; change handleToggleLogging to call
setDirty(true) when the toggle succeeds (use the same updated response) so the
logging change is persistable. In handleApplyPort, don't unconditionally clear
dirty or toast success: after calling applyDmxPort check updated.listening (and
use updated.port) before calling setDirty(false) and toast.success; if
updated.listening is false or missing, show a toast.error and leave dirty true
so mapping/logging edits aren’t hidden; keep setIsApplyingPort finally to false
and continue using setStatus(prev => (prev ? { ...prev, ...updated } : prev)) to
merge state.
| const handleExport = () => { | ||
| const exportData: DmxConfigResponse = { | ||
| preferred_port: parseInt(localPort, 10) || 6454, | ||
| log_all_messages: status?.log_all_messages ?? false, | ||
| mappings: localMappings, | ||
| }; | ||
| const blob = new Blob([JSON.stringify(exportData, null, 2)], { | ||
| type: "application/json", | ||
| }); | ||
| const url = URL.createObjectURL(blob); | ||
| const link = document.createElement("a"); | ||
| link.href = url; | ||
| link.download = "scope-dmx-config.json"; | ||
| document.body.appendChild(link); | ||
| link.click(); | ||
| document.body.removeChild(link); | ||
| URL.revokeObjectURL(url); | ||
| toast.success("DMX config exported"); | ||
| }; | ||
|
|
||
| const handleImport = async (file: File) => { | ||
| try { | ||
| const text = await file.text(); | ||
| const imported = JSON.parse(text); | ||
| if (!Array.isArray(imported.mappings)) { | ||
| toast.error("Invalid DMX config file"); | ||
| return; | ||
| } | ||
| const mappings: DmxMapping[] = imported.mappings.filter( | ||
| (m: DmxMapping) => | ||
| typeof m.universe === "number" && | ||
| typeof m.channel === "number" && | ||
| typeof m.key === "string" && | ||
| m.key.length > 0 | ||
| ); | ||
| setLocalMappings(mappings); | ||
| if (typeof imported.preferred_port === "number") { | ||
| setLocalPort(String(imported.preferred_port)); | ||
| } | ||
| setDirty(true); | ||
| toast.success(`Imported ${mappings.length} mapping(s)`); | ||
| } catch { | ||
| toast.error("Failed to parse DMX config file"); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Import/export doesn't round-trip log_all_messages.
The exported JSON includes log_all_messages, but the import path ignores it and only restores preferred_port/mappings. Re-importing a saved config silently drops the user's logging preference.
Possible fix
setLocalMappings(mappings);
if (typeof imported.preferred_port === "number") {
setLocalPort(String(imported.preferred_port));
}
+ if (typeof imported.log_all_messages === "boolean") {
+ setStatus(prev =>
+ prev
+ ? { ...prev, log_all_messages: imported.log_all_messages }
+ : prev
+ );
+ }
setDirty(true);
toast.success(`Imported ${mappings.length} mapping(s)`);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleExport = () => { | |
| const exportData: DmxConfigResponse = { | |
| preferred_port: parseInt(localPort, 10) || 6454, | |
| log_all_messages: status?.log_all_messages ?? false, | |
| mappings: localMappings, | |
| }; | |
| const blob = new Blob([JSON.stringify(exportData, null, 2)], { | |
| type: "application/json", | |
| }); | |
| const url = URL.createObjectURL(blob); | |
| const link = document.createElement("a"); | |
| link.href = url; | |
| link.download = "scope-dmx-config.json"; | |
| document.body.appendChild(link); | |
| link.click(); | |
| document.body.removeChild(link); | |
| URL.revokeObjectURL(url); | |
| toast.success("DMX config exported"); | |
| }; | |
| const handleImport = async (file: File) => { | |
| try { | |
| const text = await file.text(); | |
| const imported = JSON.parse(text); | |
| if (!Array.isArray(imported.mappings)) { | |
| toast.error("Invalid DMX config file"); | |
| return; | |
| } | |
| const mappings: DmxMapping[] = imported.mappings.filter( | |
| (m: DmxMapping) => | |
| typeof m.universe === "number" && | |
| typeof m.channel === "number" && | |
| typeof m.key === "string" && | |
| m.key.length > 0 | |
| ); | |
| setLocalMappings(mappings); | |
| if (typeof imported.preferred_port === "number") { | |
| setLocalPort(String(imported.preferred_port)); | |
| } | |
| setDirty(true); | |
| toast.success(`Imported ${mappings.length} mapping(s)`); | |
| } catch { | |
| toast.error("Failed to parse DMX config file"); | |
| } | |
| }; | |
| const handleExport = () => { | |
| const exportData: DmxConfigResponse = { | |
| preferred_port: parseInt(localPort, 10) || 6454, | |
| log_all_messages: status?.log_all_messages ?? false, | |
| mappings: localMappings, | |
| }; | |
| const blob = new Blob([JSON.stringify(exportData, null, 2)], { | |
| type: "application/json", | |
| }); | |
| const url = URL.createObjectURL(blob); | |
| const link = document.createElement("a"); | |
| link.href = url; | |
| link.download = "scope-dmx-config.json"; | |
| document.body.appendChild(link); | |
| link.click(); | |
| document.body.removeChild(link); | |
| URL.revokeObjectURL(url); | |
| toast.success("DMX config exported"); | |
| }; | |
| const handleImport = async (file: File) => { | |
| try { | |
| const text = await file.text(); | |
| const imported = JSON.parse(text); | |
| if (!Array.isArray(imported.mappings)) { | |
| toast.error("Invalid DMX config file"); | |
| return; | |
| } | |
| const mappings: DmxMapping[] = imported.mappings.filter( | |
| (m: DmxMapping) => | |
| typeof m.universe === "number" && | |
| typeof m.channel === "number" && | |
| typeof m.key === "string" && | |
| m.key.length > 0 | |
| ); | |
| setLocalMappings(mappings); | |
| if (typeof imported.preferred_port === "number") { | |
| setLocalPort(String(imported.preferred_port)); | |
| } | |
| if (typeof imported.log_all_messages === "boolean") { | |
| setStatus(prev => | |
| prev | |
| ? { ...prev, log_all_messages: imported.log_all_messages } | |
| : prev | |
| ); | |
| } | |
| setDirty(true); | |
| toast.success(`Imported ${mappings.length} mapping(s)`); | |
| } catch { | |
| toast.error("Failed to parse DMX config file"); | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/settings/DmxTab.tsx` around lines 182 - 226, The
import path currently ignores the exported log_all_messages value; update
handleImport to read imported.log_all_messages and, if it's a boolean, restore
it into the same state you read from in handleExport (the
status.log_all_messages state) by calling the appropriate setter used in this
component (e.g., setStatus or the local setter that controls the DMX logging
flag); keep existing behavior for preferred_port/mappings, mark form dirty with
setDirty(true), and show the same success/error toasts.
| @app.put("/api/v1/dmx/config") | ||
| async def dmx_put_config(request: DmxConfigRequest): | ||
| """Save / import a full DMX mapping configuration.""" | ||
| from .dmx_config import ( | ||
| load_config, | ||
| mappings_to_dict, | ||
| save_config, | ||
| ) | ||
|
|
||
| cfg = load_config() | ||
| if request.preferred_port is not None: | ||
| cfg["preferred_port"] = request.preferred_port | ||
| if request.log_all_messages is not None: | ||
| cfg["log_all_messages"] = request.log_all_messages | ||
| if request.mappings is not None: | ||
| cfg["mappings"] = request.mappings | ||
| save_config(cfg) | ||
|
|
||
| # Hot-reload into the running server | ||
| srv = get_dmx_server() | ||
| if srv is not None: | ||
| srv.log_all_messages = cfg.get("log_all_messages", False) | ||
| srv.set_mappings(mappings_to_dict(cfg.get("mappings", []))) | ||
|
|
||
| return cfg |
There was a problem hiding this comment.
Sanitize the DMX config before persisting and echoing it back.
This endpoint writes raw preferred_port and mappings to disk, but the runtime immediately normalizes mappings through mappings_to_dict(). That means blank keys/out-of-range values are reported as “saved” even though they will be ignored, and a bad port is only discovered on the next restart/startup. Validate here and either return the cleaned config or a 400.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/scope/server/app.py` around lines 889 - 913, Validate and sanitize the
incoming DMX config in dmx_put_config before calling save_config: verify
request.preferred_port is either None or an int in the valid port range
(1–65535) and raise HTTPException(400) on bad input; normalize request.mappings
by passing request.mappings through mappings_to_dict() (or a dedicated
sanitizer) and use the normalized result when assigning cfg["mappings"] and when
calling srv.set_mappings; update cfg["preferred_port"] with the
validated/normalized value, save_config(cfg) with the cleaned config, and return
the cleaned config (import HTTPException from fastapi if needed).
6ecc767 to
0c2ed8b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
src/scope/server/dmx_config.py (1)
37-41:⚠️ Potential issue | 🟠 MajorValidate DMX channels as 1-512 here.
DMXServer._process_mappings()now reads slots asch - 1, but this validator still accepts0and rejects512. That makes channel 0 persistable-but-dead at runtime, and the last DMX slot impossible to save or import. Please align the frontend defaults/bounds to the same range too.Possible fix
- if not key or channel < 0 or channel > 511 or universe < 0: + if not key or channel < 1 or channel > 512 or universe < 0: return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scope/server/dmx_config.py` around lines 37 - 41, The validator in this block accepts channel 0 and rejects 512, causing a mismatch with DMXServer._process_mappings() which treats slots as ch - 1; update the check on the parsed channel to require 1 <= channel <= 512 (keep universe >= 0 and key non-empty) so the last DMX slot can be saved and channel 0 is rejected, and ensure any front-end/defaults that set channel bounds are aligned to the same 1–512 range.src/scope/server/app.py (1)
829-850:⚠️ Potential issue | 🟠 MajorValidate and normalize DMX config before persisting or restarting it.
These handlers still accept any integer
preferred_port, anddmx_put_config()writes rawmappingsback to disk/response even though the live server immediately normalizes them throughmappings_to_dict(). A bad port can take the listener down on restart, and invalid or duplicate mappings can come back as “saved” even though the runtime mapping table drops invalid entries and collapses duplicate slots.Also applies to: 857-874, 895-925
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scope/server/app.py` around lines 829 - 850, The handlers (e.g., update_dmx_settings and dmx_put_config) must validate and normalize incoming DMX config before writing or applying it: check preferred_port is a valid TCP/UDP port (e.g., integer 1–65535) and reject or clamp invalid values before persisting or applying; for mappings, run the runtime normalization routine (use mappings_to_dict or the server's normalizer) to remove invalid/duplicate entries and persist the normalized mappings rather than the raw payload; update the code paths that call load_config/save_config to save the normalized config and only restart/apply the server with the validated values (reference update_dmx_settings, dmx_put_config, mappings_to_dict, load_config, save_config, and the DMX server instance methods).frontend/src/components/settings/DmxTab.tsx (2)
154-159:⚠️ Potential issue | 🟠 MajorSeparate port-apply state from mapping dirty state.
Lines 157-159 treat
applyDmxPort()as if the entire form were saved. That hides unsaved mapping/import edits, and it still reports success when the restart response can come back withlistening: falseandport: null.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/settings/DmxTab.tsx` around lines 154 - 159, The code currently treats applyDmxPort() as if the entire form was saved—remove setting the global mapping dirty flag here and make the port-apply flow independent: keep setIsApplyingPort(true)/false around applyDmxPort(port), call setStatus(prev => (prev ? { ...prev, ...updated } : prev)) to update status from the response, but do NOT call setDirty(false) here; instead only clear mapping/import dirty state when the actual mapping/import save handler succeeds. Also branch the toast based on updated.listening (show success with updated.port when listening===true, otherwise show an error/warning and do not clear any dirty state). Ensure you reference the apply handler (applyDmxPort), the status updater (setStatus), the mapping dirty setter (setDirty) and the existing port-applying flag (setIsApplyingPort) when making these changes.
224-229:⚠️ Potential issue | 🟡 MinorImport should restore
log_all_messagestoo.The exported config includes that flag, but the import path only restores mappings and port. Re-importing a saved file silently drops the logging preference.
Possible fix
setLocalMappings(mappings); if (typeof imported.preferred_port === "number") { setLocalPort(String(imported.preferred_port)); } + if (typeof imported.log_all_messages === "boolean") { + setStatus(prev => + prev + ? { ...prev, log_all_messages: imported.log_all_messages } + : prev + ); + } setDirty(true); toast.success(`Imported ${mappings.length} mapping(s)`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/settings/DmxTab.tsx` around lines 224 - 229, The import currently only restores mappings and preferred_port; modify the import handler to also restore the exported log_all_messages flag by setting the component's log-all-message state from imported.log_all_messages (coerce to boolean). Locate the state updater that controls this flag (e.g., setLogAllMessages or setLocalLogAllMessages) and call it after setLocalPort, then keep the existing setDirty(true) and toast.success call so the UI reflects the change.src/scope/server/dmx_server.py (1)
261-272:⚠️ Potential issue | 🟡 MinorKeep verbose unmapped-channel logs 1-based.
This loop still enumerates from 0, so slot 1 is logged as
ch=0, and a mapped first channel will be reported as unmapped when verbose logging is enabled.Possible fix
- for ch, raw in enumerate(dmx_data): + for ch, raw in enumerate(dmx_data, start=1): if raw == 0: continue if (universe, ch) not in self._mappings: logger.info(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scope/server/dmx_server.py` around lines 261 - 272, The verbose unmapped-channel logging uses zero-based enumeration which causes off-by-one mismatches with mappings; change the loop to compute a one-based slot (e.g., slot = ch + 1) and use that slot when checking self._mappings (replace (universe, ch) with (universe, slot)) and when calling logger.info (log ch=slot) so logs and mapping checks are consistent and 1-based; keep using self._log_all_messages, dmx_data, raw, universe, self._mappings, and logger.info as the referenced symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/services/pythonProcess.ts`:
- Around line 129-147: The logServerLines helper currently treats each chunk as
full lines; fix it by keeping a per-stream remainder buffer for stdout and
stderr (e.g., stdoutBuffer and stderrBuffer) and updating the on('data')
handlers to prepend the existing buffer, split on /\r?\n/, log all complete
lines via logServerLines, and store the final partial segment back into the
respective buffer instead of logging it; also ensure you flush any remaining
buffer contents in the child.on('close'|'exit') handler so trailing partial
lines are logged. Use the existing symbols logServerLines,
child.stdout?.on('data', ...), child.stderr?.on('data', ...), and stderrBuffer
(add stdoutBuffer analog) to locate and implement the change.
---
Duplicate comments:
In `@frontend/src/components/settings/DmxTab.tsx`:
- Around line 154-159: The code currently treats applyDmxPort() as if the entire
form was saved—remove setting the global mapping dirty flag here and make the
port-apply flow independent: keep setIsApplyingPort(true)/false around
applyDmxPort(port), call setStatus(prev => (prev ? { ...prev, ...updated } :
prev)) to update status from the response, but do NOT call setDirty(false) here;
instead only clear mapping/import dirty state when the actual mapping/import
save handler succeeds. Also branch the toast based on updated.listening (show
success with updated.port when listening===true, otherwise show an error/warning
and do not clear any dirty state). Ensure you reference the apply handler
(applyDmxPort), the status updater (setStatus), the mapping dirty setter
(setDirty) and the existing port-applying flag (setIsApplyingPort) when making
these changes.
- Around line 224-229: The import currently only restores mappings and
preferred_port; modify the import handler to also restore the exported
log_all_messages flag by setting the component's log-all-message state from
imported.log_all_messages (coerce to boolean). Locate the state updater that
controls this flag (e.g., setLogAllMessages or setLocalLogAllMessages) and call
it after setLocalPort, then keep the existing setDirty(true) and toast.success
call so the UI reflects the change.
In `@src/scope/server/app.py`:
- Around line 829-850: The handlers (e.g., update_dmx_settings and
dmx_put_config) must validate and normalize incoming DMX config before writing
or applying it: check preferred_port is a valid TCP/UDP port (e.g., integer
1–65535) and reject or clamp invalid values before persisting or applying; for
mappings, run the runtime normalization routine (use mappings_to_dict or the
server's normalizer) to remove invalid/duplicate entries and persist the
normalized mappings rather than the raw payload; update the code paths that call
load_config/save_config to save the normalized config and only restart/apply the
server with the validated values (reference update_dmx_settings, dmx_put_config,
mappings_to_dict, load_config, save_config, and the DMX server instance
methods).
In `@src/scope/server/dmx_config.py`:
- Around line 37-41: The validator in this block accepts channel 0 and rejects
512, causing a mismatch with DMXServer._process_mappings() which treats slots as
ch - 1; update the check on the parsed channel to require 1 <= channel <= 512
(keep universe >= 0 and key non-empty) so the last DMX slot can be saved and
channel 0 is rejected, and ensure any front-end/defaults that set channel bounds
are aligned to the same 1–512 range.
In `@src/scope/server/dmx_server.py`:
- Around line 261-272: The verbose unmapped-channel logging uses zero-based
enumeration which causes off-by-one mismatches with mappings; change the loop to
compute a one-based slot (e.g., slot = ch + 1) and use that slot when checking
self._mappings (replace (universe, ch) with (universe, slot)) and when calling
logger.info (log ch=slot) so logs and mapping checks are consistent and 1-based;
keep using self._log_all_messages, dmx_data, raw, universe, self._mappings, and
logger.info as the referenced symbols.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 75ba7c7e-17dc-48fe-a06b-3d7841038692
📒 Files selected for processing (9)
app/src/services/pythonProcess.tsfrontend/src/components/SettingsDialog.tsxfrontend/src/components/settings/DmxTab.tsxfrontend/src/lib/api.tsfrontend/src/pages/StreamPage.tsxsrc/scope/server/app.pysrc/scope/server/dmx_config.pysrc/scope/server/dmx_docs.pysrc/scope/server/dmx_server.py
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/pages/StreamPage.tsx
- frontend/src/lib/api.ts
app/src/services/pythonProcess.ts
Outdated
| const logServerLines = (prefix: 'info' | 'error', output: string) => { | ||
| const lines = output.trim().split(/\r?\n/).filter(Boolean); | ||
| for (const line of lines) { | ||
| if (prefix === 'info') { | ||
| logger.info('[SERVER]', line); | ||
| } else { | ||
| logger.error('[SERVER]', line); | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| child.stdout?.on('data', (data) => { | ||
| logger.info('[SERVER]', data.toString().trim()); | ||
| logServerLines('info', data.toString()); | ||
| }); | ||
|
|
||
| child.stderr?.on('data', (data) => { | ||
| const output = data.toString(); | ||
| stderrBuffer += output; | ||
| logger.error('[SERVER]', output.trim()); | ||
| logServerLines('error', output); |
There was a problem hiding this comment.
Keep a per-stream remainder when normalizing server output.
This helper still logs whatever is left in the current chunk as a full line. If a traceback or log line spans multiple chunks, the output is fragmented instead of normalized.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/services/pythonProcess.ts` around lines 129 - 147, The logServerLines
helper currently treats each chunk as full lines; fix it by keeping a per-stream
remainder buffer for stdout and stderr (e.g., stdoutBuffer and stderrBuffer) and
updating the on('data') handlers to prepend the existing buffer, split on
/\r?\n/, log all complete lines via logServerLines, and store the final partial
segment back into the respective buffer instead of logging it; also ensure you
flush any remaining buffer contents in the child.on('close'|'exit') handler so
trailing partial lines are logged. Use the existing symbols logServerLines,
child.stdout?.on('data', ...), child.stderr?.on('data', ...), and stderrBuffer
(add stdoutBuffer analog) to locate and implement the change.
Signed-off-by: Thom Shutt <[email protected]>
Signed-off-by: Thom Shutt <[email protected]>
Signed-off-by: Thom Shutt <[email protected]>
9612ead to
1717271
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
frontend/src/components/settings/DmxTab.tsx (3)
148-166:⚠️ Potential issue | 🟡 MinorPort apply can report false success.
handleApplyPortclearsdirtyand toasts success without checking if the server actually bound to the port (updated.listening). If the bind fails, the user sees a success message while the server isn't listening.Suggested fix
try { const updated = await applyDmxPort(port); setStatus(prev => (prev ? { ...prev, ...updated } : prev)); - setDirty(false); - toast.success(`DMX now listening on port ${updated.port ?? port}`); + if (updated.listening && updated.port != null) { + toast.success(`DMX now listening on port ${updated.port}`); + } else { + toast.error("Failed to bind the DMX port"); + } } catch (err) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/settings/DmxTab.tsx` around lines 148 - 166, handleApplyPort currently treats any response from applyDmxPort as success; change it to inspect the returned updated.listening flag and only clear setDirty(false), show toast.success, and update status (via setStatus) when updated.listening is true; if updated.listening is false (or missing) treat it as a failure: do not clear dirty, keep status unchanged or merge the returned state but surface an error via toast.error and console.error, and ensure setIsApplyingPort(false) still runs in the finally block. Use the existing function names (handleApplyPort, applyDmxPort, setStatus, setDirty, setIsApplyingPort) and the updated object returned by applyDmxPort to decide success.
209-233:⚠️ Potential issue | 🟡 MinorImport doesn't restore
log_all_messages.The exported JSON includes
log_all_messages, buthandleImportonly restorespreferred_portandmappings. Re-importing a saved config silently drops the logging preference.Suggested fix
setLocalMappings(mappings); if (typeof imported.preferred_port === "number") { setLocalPort(String(imported.preferred_port)); } + if (typeof imported.log_all_messages === "boolean") { + setStatus(prev => + prev ? { ...prev, log_all_messages: imported.log_all_messages } : prev + ); + } setDirty(true); toast.success(`Imported ${mappings.length} mapping(s)`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/settings/DmxTab.tsx` around lines 209 - 233, The import handler (handleImport) restores only mappings and preferred_port but ignores the exported log_all_messages flag; update handleImport to check if typeof imported.log_all_messages === "boolean" and, when present, call the local state setter (e.g., setLocalLogAllMessages(imported.log_all_messages)) alongside setLocalPort and setLocalMappings, then keep setting setDirty(true) and the success toast so the logging preference is preserved on re-import.
138-146:⚠️ Potential issue | 🟡 MinorLogging toggle changes aren't persistable.
handleToggleLoggingupdates runtime state viaupdateDmxSettingsbut never marks the form dirty, so the logging preference won't be included when the user clicks Save unless they also modify a mapping or port.Suggested fix
const handleToggleLogging = async (checked: boolean) => { try { const updated = await updateDmxSettings({ log_all_messages: checked }); setStatus(prev => (prev ? { ...prev, ...updated } : prev)); + setDirty(true); } catch (err) { toast.error("Failed to update DMX logging"); console.error(err); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/settings/DmxTab.tsx` around lines 138 - 146, handleToggleLogging updates DMX runtime state via updateDmxSettings but doesn’t mark the settings form dirty, so the change isn’t included on Save; after the await updateDmxSettings(...) and setStatus(...) call, mark the settings form as dirty so Save will persist the new log_all_messages value (e.g., call your form dirty setter or Formik methods: formik.setFieldValue('log_all_messages', checked) and formik.setFieldTouched('log_all_messages', true) or call setIsDirty(true) / markSettingsDirty()), ensuring the saved payload reads the updated status/log_all_messages from the same state that Save uses.
🧹 Nitpick comments (2)
frontend/src/components/settings/DmxTab.tsx (1)
68-68: Unused state setter pattern.
setConfigis extracted butconfigis never read. Consider removing the state entirely or using it to display/compare saved vs. local state.- const [, setConfig] = useState<DmxConfigResponse | null>(null); + const [config, setConfig] = useState<DmxConfigResponse | null>(null);Or remove if truly unused:
- const [, setConfig] = useState<DmxConfigResponse | null>(null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/settings/DmxTab.tsx` at line 68, The current line declares useState for DmxConfigResponse but only extracts the setter (const [, setConfig]) and never reads the state, so remove the unused React state if you don't need to track saved config; specifically remove the useState import/usage for DmxConfigResponse and the setConfig reference (or alternatively change to const [config, setConfig] and use config where appropriate to display/compare saved vs local state). Ensure any calls that currently call setConfig are either removed or adjusted to update a real state variable named config if you want to keep the saved-config behavior.src/scope/server/dmx_paths.py (1)
131-141: Potential key collision inget_all_numeric_paths.If the same parameter key exists in multiple pipelines (e.g., a custom plugin reuses a common key name), the flat dict will silently overwrite earlier entries. This is likely acceptable since the metadata (min/max/type) should be consistent for the same key, but worth noting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scope/server/dmx_paths.py` around lines 131 - 141, get_all_numeric_paths currently flattens paths into result and silently overwrites duplicate keys; update the function (get_all_numeric_paths) to detect when result already contains p["key"], compare the existing metadata vs the new p, and handle collisions instead of blindly overwriting: if metadata matches, skip or keep first; if metadata differs, emit a clear warning (use the module logger) including the conflicting key and the two source entries (e.g., include a source/pipeline identifier from p or add one), or aggregate source info into the stored entry so callers can see duplicates. Ensure you reference the local variables data, groups, paths, p, and result when implementing the check and logging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/settings/DmxTab.tsx`:
- Around line 261-272: The unmapped-channel log in the backend (dmx_server.py)
prints the variable ch as a 0-based index, causing mismatch with the UI's
1-based DMX channel numbering; locate the log statement that references ch (the
"unmapped channel" / unmapped log inside the packet/handler routine) and change
it to log ch + 1 (and adjust any adjacent range prints similarly) so all
user-facing logs report channels using 1–512 numbering consistent with the
DmxTab UI.
In `@src/scope/server/app.py`:
- Around line 829-831: preferred_port is currently unvalidated in
DmxSettingsRequest, DmxRestartRequest and DmxConfigRequest which allows invalid
port numbers; update each Pydantic model (DmxSettingsRequest, DmxRestartRequest,
DmxConfigRequest) to validate preferred_port by declaring it with a Field
constraint (e.g., Field(..., ge=1, le=65535) or Field(None, ge=1, le=65535) for
optional) so values <=0 or >65535 are rejected, or alternatively add a
`@validator` for preferred_port enforcing 1–65535 and raising ValueError on
invalid input; ensure the change is applied to every occurrence of
preferred_port in those model classes.
In `@src/scope/server/dmx_server.py`:
- Around line 261-272: The unmapped-channel logging uses 0-based indices from
enumerate(dmx_data) but mappings in self._mappings are 1-based; update the check
and log to use a 1-based channel index (e.g., ch_1 = ch + 1) so both the mapping
lookup (use (universe, ch_1) instead of (universe, ch)) and the logger.info
message print ch_1 (and raw) — keep the rest of the loop logic the same and
reference self._log_all_messages, dmx_data, self._mappings, universe, and logger
when making the change.
---
Duplicate comments:
In `@frontend/src/components/settings/DmxTab.tsx`:
- Around line 148-166: handleApplyPort currently treats any response from
applyDmxPort as success; change it to inspect the returned updated.listening
flag and only clear setDirty(false), show toast.success, and update status (via
setStatus) when updated.listening is true; if updated.listening is false (or
missing) treat it as a failure: do not clear dirty, keep status unchanged or
merge the returned state but surface an error via toast.error and console.error,
and ensure setIsApplyingPort(false) still runs in the finally block. Use the
existing function names (handleApplyPort, applyDmxPort, setStatus, setDirty,
setIsApplyingPort) and the updated object returned by applyDmxPort to decide
success.
- Around line 209-233: The import handler (handleImport) restores only mappings
and preferred_port but ignores the exported log_all_messages flag; update
handleImport to check if typeof imported.log_all_messages === "boolean" and,
when present, call the local state setter (e.g.,
setLocalLogAllMessages(imported.log_all_messages)) alongside setLocalPort and
setLocalMappings, then keep setting setDirty(true) and the success toast so the
logging preference is preserved on re-import.
- Around line 138-146: handleToggleLogging updates DMX runtime state via
updateDmxSettings but doesn’t mark the settings form dirty, so the change isn’t
included on Save; after the await updateDmxSettings(...) and setStatus(...)
call, mark the settings form as dirty so Save will persist the new
log_all_messages value (e.g., call your form dirty setter or Formik methods:
formik.setFieldValue('log_all_messages', checked) and
formik.setFieldTouched('log_all_messages', true) or call setIsDirty(true) /
markSettingsDirty()), ensuring the saved payload reads the updated
status/log_all_messages from the same state that Save uses.
---
Nitpick comments:
In `@frontend/src/components/settings/DmxTab.tsx`:
- Line 68: The current line declares useState for DmxConfigResponse but only
extracts the setter (const [, setConfig]) and never reads the state, so remove
the unused React state if you don't need to track saved config; specifically
remove the useState import/usage for DmxConfigResponse and the setConfig
reference (or alternatively change to const [config, setConfig] and use config
where appropriate to display/compare saved vs local state). Ensure any calls
that currently call setConfig are either removed or adjusted to update a real
state variable named config if you want to keep the saved-config behavior.
In `@src/scope/server/dmx_paths.py`:
- Around line 131-141: get_all_numeric_paths currently flattens paths into
result and silently overwrites duplicate keys; update the function
(get_all_numeric_paths) to detect when result already contains p["key"], compare
the existing metadata vs the new p, and handle collisions instead of blindly
overwriting: if metadata matches, skip or keep first; if metadata differs, emit
a clear warning (use the module logger) including the conflicting key and the
two source entries (e.g., include a source/pipeline identifier from p or add
one), or aggregate source info into the stored entry so callers can see
duplicates. Ensure you reference the local variables data, groups, paths, p, and
result when implementing the check and logging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d1071a5f-0a92-4fe1-ac18-709e2b0bb252
📒 Files selected for processing (8)
frontend/src/components/SettingsDialog.tsxfrontend/src/components/settings/DmxTab.tsxfrontend/src/lib/api.tsfrontend/src/pages/StreamPage.tsxsrc/scope/server/app.pysrc/scope/server/dmx_config.pysrc/scope/server/dmx_paths.pysrc/scope/server/dmx_server.py
| Listening on UDP port {status.port} | ||
| </span> | ||
| ) : ( | ||
| <span className="text-sm text-muted-foreground"> | ||
| Not listening | ||
| </span> | ||
| )} | ||
| </div> | ||
| </div> | ||
|
|
||
| {/* Preferred Port */} | ||
| <div className="flex items-center gap-4"> |
There was a problem hiding this comment.
Unmapped channel log uses 0-based index inconsistently.
The enumeration at line 263 yields 0-based indices, but DMX operators expect 1-based channels (1–512). The log message displays ch as 0-based, which may confuse users comparing against their DMX console.
Suggested fix (if aligning with 1-based DMX convention)
Note: This is in the backend file dmx_server.py, lines 261-272, but since the DmxTab UI displays channel ranges and this relates to user expectations, flagging here for awareness. The backend should log ch + 1 for consistency with the 1-based UI.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/settings/DmxTab.tsx` around lines 261 - 272, The
unmapped-channel log in the backend (dmx_server.py) prints the variable ch as a
0-based index, causing mismatch with the UI's 1-based DMX channel numbering;
locate the log statement that references ch (the "unmapped channel" / unmapped
log inside the packet/handler routine) and change it to log ch + 1 (and adjust
any adjacent range prints similarly) so all user-facing logs report channels
using 1–512 numbering consistent with the DmxTab UI.
src/scope/server/app.py
Outdated
| class DmxSettingsRequest(BaseModel): | ||
| log_all_messages: bool | None = None | ||
| preferred_port: int | None = None |
There was a problem hiding this comment.
Add port validation to Pydantic models.
preferred_port is accepted without validation across DmxSettingsRequest, DmxRestartRequest, and DmxConfigRequest. Invalid port values (≤0 or >65535) will be silently accepted and could cause the server to fail on restart.
🛡️ Proposed fix using Pydantic Field validation
+from pydantic import Field
+
class DmxSettingsRequest(BaseModel):
log_all_messages: bool | None = None
- preferred_port: int | None = None
+ preferred_port: int | None = Field(default=None, ge=1, le=65535)
class DmxRestartRequest(BaseModel):
- preferred_port: int | None = None
+ preferred_port: int | None = Field(default=None, ge=1, le=65535)
class DmxConfigRequest(BaseModel):
- preferred_port: int | None = None
+ preferred_port: int | None = Field(default=None, ge=1, le=65535)
log_all_messages: bool | None = None
mappings: list[dict] | None = NoneAlso applies to: 853-854, 895-898
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/scope/server/app.py` around lines 829 - 831, preferred_port is currently
unvalidated in DmxSettingsRequest, DmxRestartRequest and DmxConfigRequest which
allows invalid port numbers; update each Pydantic model (DmxSettingsRequest,
DmxRestartRequest, DmxConfigRequest) to validate preferred_port by declaring it
with a Field constraint (e.g., Field(..., ge=1, le=65535) or Field(None, ge=1,
le=65535) for optional) so values <=0 or >65535 are rejected, or alternatively
add a `@validator` for preferred_port enforcing 1–65535 and raising ValueError on
invalid input; ensure the change is applied to every occurrence of
preferred_port in those model classes.
| # Log unmapped channels with non-zero values when verbose logging is on | ||
| if self._log_all_messages: | ||
| for ch, raw in enumerate(dmx_data): | ||
| if raw == 0: | ||
| continue | ||
| if (universe, ch) not in self._mappings: | ||
| logger.info( | ||
| "DMX UNMAPPED uni=%d ch=%d raw=%d (no mapping)", | ||
| universe, | ||
| ch, | ||
| raw, | ||
| ) |
There was a problem hiding this comment.
Unmapped channel log uses 0-based index.
The enumerate(dmx_data) loop yields 0-based indices, but the mapping check and log message use ch directly. Since mappings use 1-based channels (per the fix at line 202-206), the unmapped channel log at line 268 will display 0-based values, which may confuse operators comparing against their DMX console.
Suggested fix
if self._log_all_messages:
- for ch, raw in enumerate(dmx_data):
+ for ch_index, raw in enumerate(dmx_data):
if raw == 0:
continue
- if (universe, ch) not in self._mappings:
+ ch = ch_index + 1 # 1-based for consistency
+ if (universe, ch) not in self._mappings:
logger.info(
"DMX UNMAPPED uni=%d ch=%d raw=%d (no mapping)",
universe,
ch,
raw,
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/scope/server/dmx_server.py` around lines 261 - 272, The unmapped-channel
logging uses 0-based indices from enumerate(dmx_data) but mappings in
self._mappings are 1-based; update the check and log to use a 1-based channel
index (e.g., ch_1 = ch + 1) so both the mapping lookup (use (universe, ch_1)
instead of (universe, ch)) and the logger.info message print ch_1 (and raw) —
keep the rest of the loop logic the same and reference self._log_all_messages,
dmx_data, self._mappings, universe, and logger when making the change.
Prevent same param from multiple pipelines appearing multiple times (e.g. 'zoomzoomzoom' in trigger) by tracking seen keys when grouping. Signed-off-by: Rafał Leszko <[email protected]> Made-with: Cursor
… on listening state handleToggleLogging now sets dirty so the logging change is persistable. handleApplyPort checks updated.listening before clearing dirty/toasting success; shows an error toast if the server didn't start listening. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> Signed-off-by: Rafał Leszko <[email protected]>
…dexing Adds preferred_port bounds check, normalizes mappings through mappings_to_dict before saving, and corrects off-by-one in unmapped channel logging to use 1-based indexing. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> Signed-off-by: Rafał Leszko <[email protected]>
DMX server was always started and the frontend always opened an SSE connection, even when users don't use Art-Net. Now DMX is disabled by default and can be enabled via a toggle in Settings > DMX. The server only binds the UDP port and the frontend only opens the SSE stream when DMX is explicitly enabled. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> Signed-off-by: Rafał Leszko <[email protected]>
Resolve conflict in src/scope/server/app.py: keep both DMX cache invalidation from this branch and plugin cache invalidation from main. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> Signed-off-by: Rafał Leszko <[email protected]>
Log the first UDP packet unconditionally to confirm connectivity, and log every packet (with source address) when verbose logging is enabled. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> Signed-off-by: Rafał Leszko <[email protected]>
…tion Use Pydantic Field(ge=1024, le=65535) on all DMX port fields instead of ad-hoc manual checks. Align frontend channel inputs to 1-512 to match the server 1-based convention. Remove misleading dirty flag from the logging toggle since it already persists server-side. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> Signed-off-by: Rafał Leszko <[email protected]>
Adds basic DMX (Art‑Net) support: real‑time DMX input, configurable port binding, mappings, and live SSE updates.
demo_dmx.mp4
Summary by CodeRabbit
Release Notes
New Features