Validate controls in the backend; drop the SET_DEVICE_MODEL Improv RPC#24
Open
ewowi wants to merge 3 commits into
Open
Validate controls in the backend; drop the SET_DEVICE_MODEL Improv RPC#24ewowi wants to merge 3 commits into
ewowi wants to merge 3 commits into
Conversation
deviceModel is now set like any other catalog default — an APPLY_OP `set System.deviceModel` op (installer over serial) or POST /api/control (MoonDeck) — instead of a bespoke SET_DEVICE_MODEL Improv RPC. Its printable-ASCII rule moves onto the control as a per-control validator on ControlDescriptor, so every write path (HTTP, serial APPLY_OP, persistence load) runs the same check in one backend place. This is architecturally cleaner — any control that needs input rules declares them on itself — and it removes a whole vendor RPC + its device-side handler, buffers, and JS frame builder. SET_TX_POWER stays a dedicated RPC because it genuinely must land before the radio associates; deviceModel has no such ordering constraint, so it rides the generic transport (like deviceName already does).
KPI: 16384lights | PC:365KB | tick:112/87/111/9/1/313/37/15/18/111/11us(FPS:8928/11494/9009/111111/1000000/3194/27027/66666/55555/9009/90909) | src:97(19660) | test:68(10232) | lizard:76w
(No ESP32 tick line: this change touches only Improv provisioning + the validator hook + boot wiring — no render-path code — so the device tick is unchanged from the last committed ESP32 value, tick:5155us(FPS:193). ESP32 esp32 + esp32p4-eth both build clean under -Werror.)
Core:
- Control: ControlDescriptor gains an optional per-control validator (`bool (*validate)(const char*)`, Text/Password only). applyControlValue runs it on the incoming value BEFORE the write and returns Malformed on reject (prior value preserved, no partial write), so the check covers HTTP, APPLY_OP-over-serial, and persistence load in one place. addText takes an optional validator arg.
- SystemModule: deviceModel's printable-ASCII rule (1..31, 0x20-0x7E, no NUL) is now that validator (validateDeviceModel), declared on the control via addText. Removed setDeviceModel().
- ImprovProvisioningModule: dropped the SET_DEVICE_MODEL pending buffer + per-tick poll + the deviceModelOut init args; deviceModel arrives via the APPLY_OP poll like any other control. systemModule wiring stays (GET_DEVICE_INFO device name).
Platform:
- platform_esp32_improv.cpp: removed the SET_DEVICE_MODEL (0xFE) handler, its dispatch case, and the deviceModelOut/deviceModelReady g_improv fields + init args.
- platform.h + platform_desktop.cpp: improvProvisioningInit drops the deviceModelOut/deviceModelOutLen/deviceModelReady params.
- main.cpp: comment-only — deviceModel injection now goes through the apply-core + validator, not a dedicated RPC.
UI:
- improv-frame.js: removed the IMPROV_CMD_SET_DEVICE_MODEL export.
- install-orchestrator.js: removed encodeSetDeviceModelPayload + sendSetBoardFrame; pushDefaultsOverSerial sends only APPLY_OP ops now (the deviceModel name is one of the catalog `set` ops). The TX-power frame still precedes provisioning.
Tests:
- unit_Control_apply_absent_key: a per-control validator accepts valid input, rejects a raw control byte / empty value (Malformed), preserves the prior value on reject; a no-validator Text control accepts anything that fits.
- unit_HttpServerModule_apply: a validated Text control (Tag) is enforced THROUGH the apply-core — valid value applies via both applySetControl (HTTP) and applyOp (APPLY_OP-over-serial), bad bytes / empty rejected with the prior value kept. Proves the serial path is guarded with no per-transport special-casing.
Docs / CI:
- CLAUDE.md: extended "Present tense only" to ban absence-narration ("no longer", "anymore", "formerly", "X was removed") — state what is, not what stopped being; decisions.md lessons exempt (the before/after contrast is the lesson).
- ImprovProvisioningModule.md + SystemModule.md: deviceModel arrives via APPLY_OP set + the per-control validator; removed the SET_DEVICE_MODEL RPC from the wire contract.
- install/README.md, index.html, landing/index.html: APPLY_OP-only wording; landing page links to the getting-started guide; installer credit line drops the inaccurate "secure handshake" phrasing.
- backlog: removed the per-control-validator-hook item (shipped here).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@docs/moonmodules/core/ImprovProvisioningModule.md`:
- Line 30: The documentation states that the installer awaits each ack for
APPLY_OP operations, but the actual implementation in install-orchestrator.js
uses open-loop pacing without reading ack responses. Update the APPLY_OP
description in the ImprovProvisioningModule.md file to accurately reflect that
the installer does not wait for acks and instead uses open-loop pacing for
sending operations, removing or correcting the phrase "the installer awaits each
ack" to match the actual behavior documented in install-orchestrator.js lines
144-153.
In `@docs/moonmodules/core/SystemModule.md`:
- Line 19: The deviceModel bullet point in SystemModule.md contains historical
narration at the end that violates the documentation's present-state-only rule.
Remove the trailing sentence beginning with "Was its own `BoardModule` child
until folded into System" from the deviceModel bullet description, keeping all
the current operational details about how it functions but trimming the
historical context about its past organizational structure.
In `@src/core/Control.cpp`:
- Around line 275-279: The scratch buffer in the Text/Password control
validation path is fixed at 64 bytes, causing silent truncation when maxLen
exceeds 64. Instead of using a fixed char scratch[64] array, allocate the
scratch buffer dynamically to accommodate the actual maxLen value. The
mm::json::parseString call should parse into a buffer sized to hold the maximum
length specified by maxLen, not hardcoded to sizeof(scratch). This ensures
validated text values larger than 63 characters are properly preserved rather
than silently truncated.
In `@src/core/Control.h`:
- Around line 181-187: The validate function pointer is being added directly to
the ControlDescriptor structure, which unnecessarily increases memory overhead
for every control descriptor instance even when validation is not needed,
conflicting with the <16 bytes per descriptor guideline. Instead of adding this
pointer to the ControlDescriptor structure itself, implement a separate
validation registry or lookup mechanism that only stores validators for controls
that actually require them, keeping the descriptor structure lean and only
allocating validation storage when necessary.
In `@test/unit/core/unit_Control_apply_absent_key.cpp`:
- Around line 119-142: The test for applyControlValue with the
acceptPrintableAscii validator is missing boundary checks for the maximum
allowed string length. The validator accepts 1 to 31 characters maximum (31
chars + 1 null terminator in the 32-byte buffer), but the current test only
validates empty strings and control-byte rejection. Add two additional CHECK
statements after the existing tests: one that verifies a 31-character valid
string is accepted and applied successfully, and another that verifies a
32-character string is rejected as Malformed with the prior value preserved.
These boundary tests should use the same applyControlValue and applyPolicy calls
as the existing test cases to ensure length validation is properly exercised.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ae6f1c93-96c6-4caa-aa8c-ba80793ce62f
📒 Files selected for processing (19)
CLAUDE.mddocs/backlog/backlog.mddocs/install/README.mddocs/install/improv-frame.jsdocs/install/index.htmldocs/install/install-orchestrator.jsdocs/landing/index.htmldocs/moonmodules/core/ImprovProvisioningModule.mddocs/moonmodules/core/SystemModule.mdsrc/core/Control.cppsrc/core/Control.hsrc/core/ImprovProvisioningModule.hsrc/core/SystemModule.hsrc/main.cppsrc/platform/desktop/platform_desktop.cppsrc/platform/esp32/platform_esp32_improv.cppsrc/platform/platform.htest/unit/core/unit_Control_apply_absent_key.cpptest/unit/core/unit_HttpServerModule_apply.cpp
💤 Files with no reviewable changes (3)
- docs/install/improv-frame.js
- docs/backlog/backlog.md
- src/platform/desktop/platform_desktop.cpp
Comment on lines
+181
to
+187
| // Optional per-control input validator (Text/Password only; nullptr = accept anything | ||
| // that fits the buffer). applyControlValue calls it on the incoming string BEFORE the | ||
| // write and returns ApplyResult::Malformed on reject, so the check covers EVERY write | ||
| // path — HTTP /api/control, APPLY_OP over serial, persistence load — in one place. | ||
| // A control with a wire-format constraint (e.g. deviceModel's printable-ASCII rule) | ||
| // declares it here, so the rule lives with the control, not with any one transport. | ||
| bool (*validate)(const char* value) = nullptr; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Avoid widening ControlDescriptor for every control entry.
Adding a raw function pointer to every descriptor increases per-control memory permanently, even for controls that never use validation. This pushes the structure farther from the ESP32 descriptor-size target and scales poorly with control count.
As per coding guidelines: "Target <16 bytes per control descriptor on ESP32."
🤖 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/core/Control.h` around lines 181 - 187, The validate function pointer is
being added directly to the ControlDescriptor structure, which unnecessarily
increases memory overhead for every control descriptor instance even when
validation is not needed, conflicting with the <16 bytes per descriptor
guideline. Instead of adding this pointer to the ControlDescriptor structure
itself, implement a separate validation registry or lookup mechanism that only
stores validators for controls that actually require them, keeping the
descriptor structure lean and only allocating validation storage when necessary.
Source: Coding guidelines
Reworks the UI layout so the 3D preview no longer crowds the module cards, and processes the CodeRabbit review of the per-control-validator commit. On a wide screen the preview and cards sit side by side; on a narrow screen (or when popped out) the preview becomes a small draggable picture-in-picture and the cards take the full width, so configuration always has room. KPI: 16384lights | PC:365KB | tick:111/88/112/9/1/310/36/15/18/111/11us(FPS:9009/11363/8928/111111/1000000/3225/27777/66666/55555/9009/90909) | ESP32:tick:3661us(FPS:273) | heap:8329KB | src:97(19663) | test:68(10251) | lizard:76w (ESP32 tick captured live from the S3 running this firmware. The preview rework is front-end only; the device tick reflects the testbench render config, unchanged by it.) UI: - index.html / style.css: .content's main area is a .workspace with two modes. .mode-docked (wide ≥960px) lays the preview pane beside a fixed 480px card column that scrolls on its own — the preview is sticky and stable, replacing the old scroll-shrink stack that ate vertical space above the cards. .mode-pip (narrow, or popped out) detaches the preview into a fixed, draggable, corner-snapping picture-in-picture (a drag bar with dock/close), cards full-width; a re-show pill brings a dismissed PiP back. - preview3d.js: setupShrink → setupLayout — width-driven mode toggle (matchMedia + rAF-throttled resize), pointer-drag with viewport clamp + corner snap, dock/close/show wiring, localStorage-persisted corner/dismissed/forcePip. The single WebGL canvas is restyled in place across modes (never duplicated), and the drag handle is a separate element so it doesn't fight the camera-orbit pointer handler. The existing resize re-fit keeps the canvas correct through dock↔PiP. - app.js: setupShrink() → setupLayout() call site. Core: - Control.cpp: the validator scratch buffer is sized to the control's full buffer (maxLen ≤ 255), not a fixed 64, so a long-but-valid Text value reaches the validator intact rather than being silently truncated first. (🐇 CodeRabbit) Tests: - unit_Control_apply_absent_key: added the validator length boundary — a 31-char value is accepted, a 32-char value is rejected (Malformed) with the prior value preserved, using a 64-byte buffer so the validator's own length check (not parse truncation) is what rejects. (🐇 CodeRabbit) Docs / CI: - ImprovProvisioningModule.md: APPLY_OP pacing is open-loop (a fixed inter-op delay, not an ack read-back) with idempotent ops; corrected the inaccurate "awaits each ack". (🐇 CodeRabbit) - SystemModule.md: dropped the trailing "Was its own BoardModule" history (present-tense rule). (🐇 CodeRabbit) - docs/history/plans: saved the responsive-preview plan. Reviews: - 🐇 scratch buffer truncation (Control.cpp) — fixed. - 🐇 "awaits each ack" inaccuracy (ImprovProvisioningModule.md) — fixed (open-loop pacing). - 🐇 BoardModule history narration (SystemModule.md) — fixed (removed). - 🐇 validator length-boundary test gap — fixed (31-accept / 32-reject). - 🐇 validate pointer on ControlDescriptor (>16B) — accepted: the descriptor is already ~28B on ESP32 (int32 min/max + flags, predating the 16B target); a validator side-table to reclaim 4 bytes on a fixed-capacity array is a bespoke lookup that fails Common-patterns-first. The pointer-on-descriptor is the recognizable, minimal choice. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@docs/moonmodules/core/ImprovProvisioningModule.md`:
- Line 30: The APPLY_OP description in the ImprovProvisioningModule.md
documentation contains backward-looking language that narrates the absence of
old features. Remove or rewrite the final parenthetical statement that begins
with "(The device-side catalog fetch + the old `?deviceModel=` handoff are
removed..." to focus only on present-state functionality. Either delete this
sentence entirely or rephrase it to describe current capabilities (such as using
MoonDeck on the LAN) without referencing what has been removed, as per the
documentation style guidelines requiring present-state wording only.
In `@src/ui/index.html`:
- Around line 37-42: The preview control buttons with IDs preview-dock,
preview-close, and preview-reset use only symbol icons and title attributes,
which is insufficient for screen reader accessibility. Add aria-label attributes
to each of these buttons to provide explicit accessible names that screen
readers can reliably announce, ensuring the accessible names clearly describe
the actions (e.g., "Pop preview out or dock", "Hide preview", "Reset camera").
In `@src/ui/preview3d.js`:
- Around line 205-214: The preview-close button currently persists the dismissed
state even when the preview is in docked mode, causing confusing behavior when
the narrow mode auto-PiP is triggered later. Find the click handler for the
preview-close element (referenced at lines 234-238 according to the comment) and
add a condition to only set dismissed=true when the preview is actually in PiP
mode (check the pip variable state or similar condition that indicates PiP
mode). This ensures that closing the preview in docked mode does not persist a
hidden state that will affect the appearance when auto-PiP is activated in
narrow mode.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 652e242d-56e4-4cda-8856-24ae9ca7e1e7
📒 Files selected for processing (10)
docs/history/plans/Plan-20260622 - Responsive split-pane preview with draggable PiP.mddocs/moonmodules/core/ImprovProvisioningModule.mddocs/moonmodules/core/SystemModule.mdsrc/core/Control.cppsrc/ui/app.jssrc/ui/index.htmlsrc/ui/preview3d.jssrc/ui/style.csstest/scenarios/light/scenario_Driver_mutation.jsontest/unit/core/unit_Control_apply_absent_key.cpp
Fixes a crash where a UI refresh could reboot the device, sharpens the 3D preview rendering (crisp LEDs, off-LEDs now visible, a bounding box), and adds an explicit "inject defaults" button to MoonDeck. The preview no longer holds the device's mDNS browse handle across ticks, which was the crash; discovery still works via a throttled synchronous query. KPI: 16384lights | PC:365KB | tick:118/90/134/9/2/379/45/24/22/124/15us(FPS:8474/11111/7462/111111/500000/2638/22222/41666/45454/8064/66666) | ESP32(S3,REST):tick:3215us(FPS:311) | src:97(19677) | test:68(10283) | lizard:76w (ESP32 tick read from the live S3's REST, not the serial KPI capture — the S3's USB serial dropped mid-session; the device stayed up on the network, uptime 4135s, confirming the mDNS fix holds. P4 also stable: 2452us/407fps, uptime 2715s.) Core: - platform_esp32.cpp + platform.h: replaced the async mDNS browse (mdnsBrowseStart/Poll/Stop — held a query handle across ticks and polled it) with one synchronous mdnsBrowse(service, proto, timeoutMs, cb). The async handle's internal queue is owned + freed by the mDNS component's own task when the query window expires; a poll landing after that expiry asserted on a null queue (xQueueSemaphoreTake queue.c:1709), rebooting the device — intermittently, on a UI refresh, grid-size-sensitive (a longer tick widened the window). A self-contained synchronous call holds no handle across ticks, so the race can't exist. mDNS advertising (<deviceName>.local) is untouched — only the peer-discovery browse changed. - DevicesModule: stepMdns calls the synchronous mdnsBrowse, throttled to one ~60ms query every ~8th tick (mdns_query_ptr blocks its full timeout and loop1s is charged to the tick, so an unthrottled per-tick query would tank FPS; one brief hiccup every ~8s is invisible for discovery). Removed the mdnsQuerying_ state. - BinaryBroadcaster + HttpServerModule: clientGeneration() bumps on each new WS client so PreviewDriver re-sends the coordinate table the moment a page connects (a refresh shows the preview at once). - PreviewDriver: send the coordinate table only when geometry changes (onBuildState) or a new client connects — never on a timer (the old ~1Hz rebuild was a hot-path waste). Light domain: - PreviewDriver: frame builder now sends EVERY light (dark ones too) so the shader can draw off LEDs as faint placeholder rings — the grid shape stays visible and an all-off scene isn't a black screen. UI: - preview3d.js: crisp-disc shader (a ~1px AA rim instead of a half-radius fade that read as blurry at small grids); off/near-black LEDs render as a dim placeholder ring; a faint wireframe bounding box around the light volume (a second line program). setupShrink → setupLayout already shipped; this is the render-quality pass. - index.html: preview button aria-labels. Scripts / MoonDeck: - moondeck_ui: explicit "inject defaults" button per discovered device — re-pushes the SELECTED device-model's full deviceModels.json config on demand (reuses POST /api/push-board → _push_board_to_device), distinct from the implicit on-pick push. Inline injecting…/injected ✓/failed ✗ feedback. Tests: - unit_PreviewDriver: a new client (clientGeneration bump) forces a coord-table re-send; advancing the clock several seconds with no client change asserts NO re-send (guards against re-introducing a timer-based rebuild). - scenario JSONs: live observed.* write-backs from running perf_light / perf_full / GridLayout_resize on the S3 and P4. Docs: - decisions.md: the mDNS async-handle-lifecycle-race lesson (don't hold a vendor library's async handle across your event loop; it races the library's internal timers) and the preview coord-cadence keepalive-timer lesson. - ImprovProvisioningModule.md: present-tense wording fix. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
docs/moonmodules/core/ImprovProvisioningModule.md (1)
30-32:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep this paragraph in present tense.
The
no HTTP,no browser handoff, andthere's no STAphrasing still reads as absence narration. This is the same doc-style cleanup already called out above; please rephrase those clauses to describe the current flow directly.🤖 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 `@docs/moonmodules/core/ImprovProvisioningModule.md` around lines 30 - 32, Rephrase the negative absence constructions in this documentation paragraph to describe the actual present-tense flows directly. Specifically, convert phrases like "no HTTP and no browser handoff", "there's no STA to provision", and "aren't linked" into active descriptions of what actually happens or what is available on eth-only builds. Replace absence narration with direct statements of the current behavior and capabilities, keeping the entire paragraph in present tense throughout.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/moondeck_ui/app.js`:
- Around line 893-899: The pushBoard function lacks timeout protection, which
can wedge the button indefinitely if a network request stalls. Add an
AbortController to the fetch call in pushBoard with a timeout (e.g., 10 seconds)
that aborts the request if it exceeds the duration, ensuring the abort triggers
the existing catch handler to call onDone(false) and allow the button to be
re-enabled. Pass the abort signal to the fetch request options.
- Around line 893-899: The pushBoard function is incorrectly using r.ok (HTTP
status) to determine operation success, but the /api/push-board endpoint returns
the actual success status in the JSON response body as {"ok": ...}. Since HTTP
200 can be returned even when the operation failed, this causes false success
reports. Fix this by parsing the response as JSON using r.json() and checking
the ok field from the parsed JSON object, then passing that value to onDone.
Apply the same fix to the other location mentioned at lines 926-928.
In `@src/platform/esp32/platform_esp32.cpp`:
- Around line 1005-1007: Update the documentation comments in the mDNS service
browse section to reflect the current synchronous implementation instead of the
removed async pattern. Remove or update the comments at lines 1005–1006 that
reference the old async behavior (mentions of "poll a few ms each tick",
"mdnsSearch_", and "cancelMdnsBrowse"). Additionally, update the section header
at line 1002 that labels this section as "(async, non-blocking)" to accurately
describe the current synchronous behavior, ensuring the documentation matches
the actual implementation.
In `@src/ui/preview3d.js`:
- Around line 480-483: The comment block describes adding "plus half a cell" to
the half-extents calculation to enclose the outermost LED centres, but the
actual code computing hx = previewBox_.x / 2 / md does not implement this
offset. Update the comment to accurately describe what the code actually
does—remove the mention of "plus half a cell" since that adjustment is not
present in the hx calculation, making the documentation match the actual
implementation.
---
Duplicate comments:
In `@docs/moonmodules/core/ImprovProvisioningModule.md`:
- Around line 30-32: Rephrase the negative absence constructions in this
documentation paragraph to describe the actual present-tense flows directly.
Specifically, convert phrases like "no HTTP and no browser handoff", "there's no
STA to provision", and "aren't linked" into active descriptions of what actually
happens or what is available on eth-only builds. Replace absence narration with
direct statements of the current behavior and capabilities, keeping the entire
paragraph in present tense throughout.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3aee67fe-270c-4cd5-9813-6759c2a6be24
📒 Files selected for processing (18)
docs/history/decisions.mddocs/moonmodules/core/ImprovProvisioningModule.mdscripts/moondeck_ui/app.jsscripts/moondeck_ui/style.csssrc/core/BinaryBroadcaster.hsrc/core/DevicesModule.hsrc/core/HttpServerModule.cppsrc/core/HttpServerModule.hsrc/light/drivers/PreviewDriver.hsrc/platform/desktop/platform_desktop.cppsrc/platform/esp32/platform_esp32.cppsrc/platform/platform.hsrc/ui/index.htmlsrc/ui/preview3d.jstest/scenarios/light/scenario_GridLayout_resize.jsontest/scenarios/light/scenario_perf_full.jsontest/scenarios/light/scenario_perf_light.jsontest/unit/light/unit_PreviewDriver.cpp
Comment on lines
+893
to
+899
| const pushBoard = (board, onDone) => { | ||
| fetch("/api/push-board", { | ||
| method: "POST", | ||
| headers: {"Content-Type": "application/json"}, | ||
| body: JSON.stringify({ip: device.ip, board}), | ||
| }).then(r => onDone && onDone(r.ok)).catch(() => onDone && onDone(false)); | ||
| }; |
There was a problem hiding this comment.
Bound pushBoard with a timeout to prevent wedged inject button.
Line 924 disables the button, and recovery depends on callback execution. A stalled network request can keep the button disabled indefinitely. Add an AbortController timeout and route timeout to onDone(false).
Proposed fix
- const pushBoard = async (board, onDone) => {
+ const pushBoard = async (board, onDone) => {
+ const controller = new AbortController();
+ const timeoutId = setTimeout(() => controller.abort(), 8000);
try {
const r = await fetch("/api/push-board", {
method: "POST",
headers: {"Content-Type": "application/json"},
body: JSON.stringify({ip: device.ip, board}),
+ signal: controller.signal,
});
+ clearTimeout(timeoutId);
let ok = r.ok;
if (r.ok) {
const j = await r.json().catch(() => null);
ok = !!(j && j.ok);
}
if (onDone) onDone(ok);
} catch (_) {
+ clearTimeout(timeoutId);
if (onDone) onDone(false);
}
};Also applies to: 924-929
🤖 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 `@scripts/moondeck_ui/app.js` around lines 893 - 899, The pushBoard function
lacks timeout protection, which can wedge the button indefinitely if a network
request stalls. Add an AbortController to the fetch call in pushBoard with a
timeout (e.g., 10 seconds) that aborts the request if it exceeds the duration,
ensuring the abort triggers the existing catch handler to call onDone(false) and
allow the button to be re-enabled. Pass the abort signal to the fetch request
options.
Source: Learnings
Use API {"ok": ...} instead of HTTP status for inject result.
Line 898 currently reports success from r.ok, but /api/push-board reports operation success in JSON ({"ok": false} can still be HTTP 200). This can display “injected ✓” when the push actually failed.
Proposed fix
- const pushBoard = (board, onDone) => {
- fetch("/api/push-board", {
+ const pushBoard = async (board, onDone) => {
+ try {
+ const r = await fetch("/api/push-board", {
method: "POST",
headers: {"Content-Type": "application/json"},
body: JSON.stringify({ip: device.ip, board}),
- }).then(r => onDone && onDone(r.ok)).catch(() => onDone && onDone(false));
+ });
+ let ok = r.ok;
+ if (r.ok) {
+ const j = await r.json().catch(() => null);
+ ok = !!(j && j.ok);
+ }
+ if (onDone) onDone(ok);
+ } catch (_) {
+ if (onDone) onDone(false);
+ }
};Also applies to: 926-928
🤖 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 `@scripts/moondeck_ui/app.js` around lines 893 - 899, The pushBoard function is
incorrectly using r.ok (HTTP status) to determine operation success, but the
/api/push-board endpoint returns the actual success status in the JSON response
body as {"ok": ...}. Since HTTP 200 can be returned even when the operation
failed, this causes false success reports. Fix this by parsing the response as
JSON using r.json() and checking the ok field from the parsed JSON object, then
passing that value to onDone. Apply the same fix to the other location mentioned
at lines 926-928.
Comment on lines
+1005
to
+1007
| // handle lets us poll a few ms each tick instead. (mdnsSearch_ is forward-declared above, | ||
| // next to cancelMdnsBrowse, so mdnsInit/mdnsStop can cancel an in-flight query.) | ||
|
|
There was a problem hiding this comment.
Stale documentation describes the removed async pattern.
Lines 1005–1006 reference the old async API ("poll a few ms each tick", "mdnsSearch_", "cancelMdnsBrowse") that no longer exists. Since these lines are changed in this PR, they should be updated or removed to match the synchronous implementation documented below.
Additionally, the section header at line 1002 ("mDNS service browse (async, non-blocking)") now contradicts the synchronous behavior—consider updating it as part of this cleanup.
📝 Suggested fix
-// --- mDNS service browse (async, non-blocking) ---
-// One in-flight async query at a time (DevicesModule serialises service types). The
-// synchronous mdns_query_ptr would block the full timeout on the render task; the async
-// handle lets us poll a few ms each tick instead. (mdnsSearch_ is forward-declared above,
-// next to cancelMdnsBrowse, so mdnsInit/mdnsStop can cancel an in-flight query.)
-
+// --- mDNS service browse (synchronous, bounded-blocking) ---
+//
// One synchronous PTR browse for `service`/`proto`, blocking up to `timeoutMs`, then it🤖 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/platform/esp32/platform_esp32.cpp` around lines 1005 - 1007, Update the
documentation comments in the mDNS service browse section to reflect the current
synchronous implementation instead of the removed async pattern. Remove or
update the comments at lines 1005–1006 that reference the old async behavior
(mentions of "poll a few ms each tick", "mdnsSearch_", and "cancelMdnsBrowse").
Additionally, update the section header at line 1002 that labels this section as
"(async, non-blocking)" to accurately describe the current synchronous behavior,
ensuring the documentation matches the actual implementation.
Source: Coding guidelines
Comment on lines
+480
to
+483
| // Faint wireframe cuboid around the light volume. Rebuilt only when the box extent | ||
| // changes (cached by boxKey). Half-extents match the normalised, box-centred point | ||
| // coords (pos/maxDim - 0.5*box/maxDim), plus half a cell so the box encloses the | ||
| // outermost LED centres rather than bisecting them. |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Comment mentions "plus half a cell" but code doesn't add it.
The comment says the half-extents include "plus half a cell so the box encloses the outermost LED centres," but line 489 computes hx = previewBox_.x / 2 / md without any cell offset. The box thus bisects the outermost LEDs rather than enclosing them. This is visually fine—just a documentation mismatch.
Suggested comment fix
-// Half-extents match the normalised, box-centred point
-// coords (pos/maxDim - 0.5*box/maxDim), plus half a cell so the box encloses the
-// outermost LED centres rather than bisecting them.
+// Half-extents match the normalised, box-centred point
+// coords (pos/maxDim - 0.5*box/maxDim), so the box passes through the outermost
+// LED centres.🤖 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/ui/preview3d.js` around lines 480 - 483, The comment block describes
adding "plus half a cell" to the half-extents calculation to enclose the
outermost LED centres, but the actual code computing hx = previewBox_.x / 2 / md
does not implement this offset. Update the comment to accurately describe what
the code actually does—remove the mention of "plus half a cell" since that
adjustment is not present in the hx calculation, making the documentation match
the actual implementation.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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
Validation moves from a bespoke per-transport Improv RPC onto the control itself, and the
SET_DEVICE_MODELRPC is removed.deviceModelis now set like any other catalog default — anAPPLY_OP set System.deviceModelop (installer over serial) orPOST /api/control(MoonDeck) — and its printable-ASCII rule is a per-control validator onControlDescriptor, so every write path (HTTP, serial APPLY_OP, persistence load) runs the same check in one backend place.Why
SET_DEVICE_MODELexisted only to carry validation that the generic write path lacked — the code itself flagged this as a "known asymmetry" and named the fix: "a per-control validator hook onControlDescriptor, not a one-off dispatch exception." This PR does exactly that. The result is architecturally cleaner (any control that needs input rules declares them on itself, checked wherever the write comes from) and a net deletion (−48 lines): a whole vendor RPC + device handler + buffers + JS frame builder go away.SET_TX_POWERstays a dedicated RPC because it genuinely must land before the radio associates;deviceModelhas no such ordering constraint, so it rides the generic transport — exactly likedeviceNamealready does (a plain Text control, re-applied live each tick for mDNS/AP/hostname).What changed
ControlDescriptor::validate(new) — optionalbool (*)(const char*)for Text/Password controls.applyControlValueruns it before the write, returnsMalformedon reject (prior value preserved, no partial write).SystemModule—deviceModel's rule becomesvalidateDeviceModel, declared viaaddText.setDeviceModel()removed.SET_DEVICE_MODEL(0xFE) removed — device handler, dispatch case,g_improvfields,improvProvisioningInitparams (all backends), theImprovProvisioningModulepending buffer + poll, the JSencodeSetDeviceModelPayload/sendSetBoardFrame+ theIMPROV_CMD_SET_DEVICE_MODELexport.unit_Control_apply_absent_key) and through the apply-core / APPLY_OP path (unit_HttpServerModule_apply, a validatedTagmodule): valid applies via both HTTP and serial; bad bytes / empty rejected with the prior value kept — proving the serial path is guarded with no per-transport special-casing.Verification
-Werror), 436 unit tests (8350 assertions), scenarios, platform boundary,check_devices, JS (6/6) + Python (4/4) host tests.-Werror:esp32,esp32p4-eth.tick:5155us / FPS:193).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation