Add File Manager, MQTT/Homebridge control, file-upload OTA, Ethernet opt-in#36
Merged
Conversation
Adds a File Manager: a boot-wired system tool to browse and manage the device filesystem from the web UI — a lazy expand/collapse folder tree with breadcrumb navigation, per-file size, and an inline text editor (pretty-prints JSON, guards binary files read-only). Create/delete files and folders on real LittleFS; verified on desktop and an ESP32-S3.
tick:159us(FPS:6289) [PC] / tick:1069us(FPS:935) [ESP32]
Core:
- FileManagerModule: new .h+.cpp core service module (~50 lines) exposing only the filesystem operations (path/new folder/delete, show hidden) — all controls UI-hidden; browsing lives entirely in the UI over /api/dir. safePath rejects `..` traversal and roots at the mount.
- HttpServerModule: added GET /api/dir (single-level listing → JSON [{name,isDir,size}], hidden-filter param, streamed to the socket) as the tree's lazy-load source, alongside the existing /api/file read/write. fileQueryPath guards the HTTP entry path against traversal.
- platform fsList: callback now carries each entry's size (desktop + ESP32).
- platform_esp32 fsRemove: route a directory to rmdir() (POSIX ::remove maps to unlink and fails on dirs) so an empty-folder delete works on LittleFS, matching the desktop std::filesystem::remove contract.
- main.cpp: register + boot-wire FileManagerModule; setName("File Manager").
UI:
- app.js: renderFileManager — lazy tree (📁/📄, size, indent), clickable breadcrumb on its own row with reveal-and-tidy (collapse siblings on jump), ＋folder/＋file/delete(press-twice)/refresh toolbar, panel-owned show-hidden pill that re-lists live, native <dialog> editor with pretty-JSON on open + binary read-only guard. ＋file reuses the /api/file write path (empty-body POST). Fixed a panel-stacking bug (render into one stable panel) and armPressTwice blanking a button's label on mouseleave-before-arm.
- style.css: fm-* tree / breadcrumb / toolbar / editor classes, theme-aware.
Tests:
- unit_FileManagerModule: create/delete/nested/non-empty-reject/file-delete/`..`-traversal/empty-path coverage against the real fs seam via fsSetRoot.
Docs / CI:
- ui.md: File Manager entry; restored the accidentally-orphaned Audio heading.
- backlog-core: File Manager follow-ups (drag-drop tiers, dates/NTP, .ml highlighting) + a flash-budget investigation item (4MB-classic ceiling, -Os win, MM_MINIMAL / repartition levers).
- Plan-20260704 - FileManagerModule (shipped).md.
Reviews:
- 👾 ui.md deleted the `### Audio` heading, orphaning the Audio section under File Manager — fixed (restored the heading).
- 👾 editor Save used strlen() and would truncate a file with an embedded NUL — fixed (binary content loads read-only, Save disabled; the Content-Length write fix stays backlogged with drag-drop).
- 👾 two path-traversal guards both claimed to be "the ONE place" — comments corrected to state two guarded filesystem entry points; no shared helper extracted (a one-line strstr idiom, extracting nets more indirection than it removes).
- 👾 "loads on first expand" over-claimed (re-fetches per render) — comment corrected to "when expanded"; a cache isn't worth the state at LittleFS scale.
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:
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/HttpServerModule.cpp`:
- Around line 403-418: The HttpServerModule::serveFileContents file endpoint is
returning raw contents for paths under /.config/, which can expose plaintext
secrets from module JSON files. Update the serveFileContents path handling
(and/or fileQueryPath guard) to deny reads for /.config/*.json or redact
ControlType::Password fields before calling sendResponse, so the unauthenticated
/api/file route cannot return sensitive config verbatim.
- Around line 420-436: The `/api/file` write path in
HttpServerModule::handleWriteFile can accept bodies larger than the request
buffer handled by handleConnection, which can lead to truncated writes. Update
the size check to reject bodies beyond the actual request buffer limit (or
increase the buffer to match kFileApiCap) before calling
platform::fsWriteAtomic, and return an appropriate 413/400 response when
oversized input is received.
In `@src/ui/app.js`:
- Around line 2905-2924: Update the file-loading logic in the app’s
fetch/preview path so binary detection in the `try` block is not based only on
`text.indexOf("\0")`; also treat content as unsafe if `res.text()` introduced
replacement-character corruption (for example, the Unicode replacement symbol),
and in that case follow the same read-only/disabled-save flow used for binary
files. Keep the change localized around the `fetch("/api/file?path=" +
encodeURIComponent(relPath))` handling and the `body`/`saveBtn` status updates.
- Around line 2884-2943: openFileEditor() currently ignores the passed-in size,
so truncated loads from /api/file can be saved back and overwrite the full file.
In openFileEditor(relPath, size), compare the fetched text length against size
(and any known fetch cap behavior) after loading; if the content appears
truncated, show a clear “edit-capped/truncated” status message, keep the
textarea read-only or otherwise disable save, and prevent the saveBtn handler
from posting data. Use the existing openFileEditor, body, status, and saveBtn
flow to add this guard.
In `@test/unit/core/unit_FileManagerModule.cpp`:
- Around line 25-28: The test fixture root in Rig is timing-dependent because it
uses platform::millis() to build the temp path, which can collide across rapid
or parallel initializations. Update the Rig constructor to use a deterministic
temp prefix combined with a monotonic unique source or existing temp helper
instead of millis(), and keep the cleanup via std::filesystem::remove_all on
that generated root.
- Line 39: The cleanup in Rig::~Rig() is resetting the filesystem root to "."
instead of restoring the default/shared test root, which can affect later tests.
Update the destructor in Rig to restore the previous root explicitly or reset
with the appropriate empty/default path using platform::fsSetRoot, and keep the
std::filesystem::remove_all(root) cleanup unchanged.
🪄 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: 8c290847-b6f8-4ee9-a21a-b9c7ebddbcec
📒 Files selected for processing (17)
CMakeLists.txtdocs/backlog/backlog-core.mddocs/history/plans/Plan-20260704 - FileManagerModule (shipped).mddocs/moonmodules/core/ui/ui.mdesp32/main/CMakeLists.txtsrc/core/FileManagerModule.cppsrc/core/FileManagerModule.hsrc/core/HttpServerModule.cppsrc/core/HttpServerModule.hsrc/main.cppsrc/platform/desktop/platform_desktop.cppsrc/platform/esp32/platform_esp32_fs.cppsrc/platform/platform.hsrc/ui/app.jssrc/ui/style.csstest/CMakeLists.txttest/unit/core/unit_FileManagerModule.cpp
Comment on lines
+403
to
+418
| void HttpServerModule::serveFileContents(platform::TcpConnection& conn, const char* query) { | ||
| char path[160]; | ||
| if (!fileQueryPath(query, path, sizeof(path))) { | ||
| sendResponse(conn, 400, "application/json", "{\"error\":\"bad path\"}"); | ||
| return; | ||
| } | ||
| // A fixed-size read buffer (kFileApiCap) — the size cap is the point; a larger file is | ||
| // truncated to the cap (the UI shows what fits and flags the file as edit-capped elsewhere). | ||
| static char fileBuf[kFileApiCap + 1]; | ||
| const int n = platform::fsRead(path, fileBuf, sizeof(fileBuf)); | ||
| if (n < 0) { sendResponse(conn, 404, "application/json", "{\"error\":\"not found\"}"); return; } | ||
| fileBuf[n] = 0; | ||
| // Serve the raw bytes as text/plain — the UI renders it in the editor. (Content type is | ||
| // deliberately generic; the manager edits text/config, and a binary preview is the UI's call.) | ||
| sendResponse(conn, 200, "text/plain", fileBuf); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP -C3 'writeValue|Password' src/core/FilesystemModule.cppRepository: MoonModules/projectMM
Length of output: 1159
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== HttpServerModule serveFileContents =="
ast-grep outline src/core/HttpServerModule.cpp --view expanded | sed -n '1,220p'
echo
echo "== Search for /api/file, /api/state, and password obfuscation =="
rg -n -C 3 'serveFileContents|/api/file|/api/state|Password|obfusc|xor|writeControls|writeControlValue|FilesystemModule::writeValue|fileQueryPath' src/core
echo
echo "== Relevant slice of FilesystemModule.cpp =="
sed -n '300,420p' src/core/FilesystemModule.cpp
echo
echo "== Relevant slice of Control.cpp if present =="
if [ -f src/core/Control.cpp ]; then
rg -n -C 4 'Password|writeControlValue|JsonSink' src/core/Control.cpp
fiRepository: MoonModules/projectMM
Length of output: 49042
Block /api/file from serving /.config/*.json verbatim
/.config/<Module>.json stores ControlType::Password values in plaintext, while /api/state only obfuscates the API view. Serving those files through the unauthenticated file endpoint leaks Wi‑Fi credentials and other secrets; deny reads under /.config/ or redact password fields before returning file contents.
🧰 Tools
🪛 Clang (14.0.6)
[warning] 404-404: do not declare C-style arrays, use std::array<> instead
(modernize-avoid-c-arrays)
[warning] 406-406: escaped string literal can be written as a raw string literal
(modernize-raw-string-literal)
[warning] 411-411: do not declare C-style arrays, use std::array<> instead
(modernize-avoid-c-arrays)
[warning] 412-412: variable name 'n' is too short, expected at least 3 characters
(readability-identifier-length)
[warning] 413-413: escaped string literal can be written as a raw string literal
(modernize-raw-string-literal)
🪛 Cppcheck (2.21.0)
[style] 403-403: The function 'readString' is never used.
(unusedFunction)
[style] 413-413: The function 'readInt' is never used.
(unusedFunction)
🤖 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/HttpServerModule.cpp` around lines 403 - 418, The
HttpServerModule::serveFileContents file endpoint is returning raw contents for
paths under /.config/, which can expose plaintext secrets from module JSON
files. Update the serveFileContents path handling (and/or fileQueryPath guard)
to deny reads for /.config/*.json or redact ControlType::Password fields before
calling sendResponse, so the unauthenticated /api/file route cannot return
sensitive config verbatim.
Streams file up/downloads of any size in the File Manager (was 8 KB-capped), reclaims per-instance memory in SystemModule by binding read-only controls straight to platform static strings, and makes MoonDeck track devices by their MAC — a stable identity that survives DHCP IP changes — so it can remember each board's flash port, firmware and deviceModel and offer them as a one-click "select for flashing".
tick:143us(FPS:6993) [PC] / tick:915us(FPS:1092) [ESP32]
Core:
- FileManagerModule: filesystem-usage gauge (used/total) is now this module's own control, shown in its panel; setup() forces `show hidden` off each boot (a view preference, not persisted config).
- HttpServerModule: /api/file up/download stream (fsWriteStream pulls the body → file, fsReadAt streams the file → socket) so any-size, binary-safe transfers work with a fixed buffer; kUploadMax + free-space (507) guards; byte-exact writes (real Content-Length, not strlen).
- SystemModule: mac/chip/sdk/bootReason/wifiCoproc read-only controls bind directly to the platform's static strings — removed the per-instance copy buffers (~98 bytes/instance); new `mac` control exposes the stable per-chip identity.
- FilesystemModule: dropped the filesystem-usage control + its members (moved to FileManagerModule).
- platform: new fsWriteStream (pull-based streamed atomic write), fsReadAt (pread-style), fsSize, macString (format-once static) — reusable primitives; fsList callback carries entry size; fsRemove routes a directory to rmdir on ESP32 (POSIX ::remove fails on dirs) to match the desktop contract.
UI:
- app.js: File Manager renders a lazy expand/collapse tree over /api/dir with breadcrumb reveal-and-tidy, drag-drop upload + per-file download, an inline editor (pretty-JSON on open, binary/non-text read-only guard), and the usage bar below the tree; a shared controlRendersGenerically() predicate keeps the two render paths from duplicating a control.
Scripts / MoonDeck:
- moondeck.py: devices are keyed by MAC (the only persistent identity — a MAC-less device is shown live but not stored); renamed the per-device `board` field/functions/endpoint to `deviceModel` (board is the bare PCB per the architecture; this field holds a catalog entry); firmware now read from FirmwareUpdateModule (was wrongly read from SystemModule → always empty); last_port linked to the flashed board by MAC (shared _link_last_flash from discover + refresh); list_serial_ports returns cu.* on macOS so the port dropdown matches the flash port; devices sorted by name + IPs stored bare (:80 stripped).
- flash_esp32.py: tees the flash output to parse esptool's MAC line, recording {port, mac, firmware} so MoonDeck attributes last_port to the exact board even when two share a firmware.
- moondeck_ui: each device shows a clickable last_port chip that selects the board for flashing — sets the network port, firmware and deviceModel to what the board actually runs, in sync.
Tests:
- unit_FileManagerModule: fsWriteStream multi-chunk + NUL, fsReadAt positional read, atomic-commit (no torn file); the Rig temp root uses a monotonic counter, not millis().
Docs / CI:
- CLAUDE.md: core-grows-slower calibration — the bar is a kind test (an industry-standard, reusable primitive), not a line-count race, stated present-tense.
- ui.md: File Manager tree/upload/download/usage-bar + streaming; Filesystem entry trimmed to lastSaved.
- backlog-core: streamed any-size up/down shipped; folder-upload + folder-zip-download remain.
- Plans: File Manager drag-drop + download + usage bar; Streamed file upload+download.
Reviews:
- 🐇 /.config plaintext-secret exposure via /api/file: initially fixed with a 403 guard, then removed at the product owner's direction (LAN API is already unauthenticated; `show hidden` off-by-default is the accepted weak protection). Net subtraction: the guard + helpers are gone.
- 🐇 write could exceed the request buffer → truncation: resolved by the streaming rewrite (fsWriteStream), not a size check.
- 🐇 binary detection only checked NUL: also treat a UTF-8 replacement char (U+FFFD) as non-text → editor read-only.
- 🐇 openFileEditor ignored the size arg (truncated-save risk): obsolete now downloads stream the whole file; dropped the unused arg + fixed the stale comment.
- 🐇 Rig temp path used millis() (collision risk): switched to a monotonic counter.
- 🐇 Rig dtor reset root to ".": accepted — that is the codebase's established default-restore, matching the sibling FilesystemModule test.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/core/HttpServerModule.cpp (1)
339-365: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject overlong
pathvalues instead of truncating them.When the decoded query path fills
out, the loop exits and the truncated prefix is accepted. That can route a create/write/delete to a different filesystem entry than the client requested.Proposed fix
while (*p && *p != '&' && i + 1 < cap) { char c = *p; if (c == '%' && p[1] && p[2]) { // %XX → byte @@ out[i++] = c; p++; } + if (*p && *p != '&') return false; // path did not fit; reject instead of truncating out[i] = 0;🤖 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/HttpServerModule.cpp` around lines 339 - 365, The path-decoding helper that trims query `path` into `out` currently accepts truncated values when the buffer fills up, which can misroute filesystem actions. Update the loop and postાવ checks so an overlong decoded path is rejected outright instead of treating the truncated prefix as valid, and make sure the existing normalization logic around `std::strstr(out, "..")` and the rooted-path handling only runs for fully captured input.src/ui/app.js (2)
3043-3046: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDisable Save after a failed load.
After a GET failure, the textarea is emptied but Save remains enabled, so clicking it can POST an empty body to the same path and create/truncate data.
Proposed fix
} catch (err) { body.value = ""; + body.readOnly = true; + saveBtn.disabled = true; status.textContent = "load failed: " + err.message; }🤖 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/app.js` around lines 3043 - 3046, After the GET failure in the load handler, the UI clears body.value but leaves the Save action enabled, which can post an empty payload; update the failure path in the catch block around the load logic to also disable Save (or otherwise prevent submission) until a successful load restores it. Use the existing load/save UI state handling in src/ui/app.js near the body.value and status.textContent updates to locate the relevant save control.
2813-2869: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake file tree rows keyboard-operable.
The file/folder rows are clickable
<div>s only, so keyboard users cannot select, expand, or open entries. Add focus and Enter/Space activation, with ARIA state for folders.Proposed fix
const rowEl = document.createElement("div"); rowEl.className = "fm-row" + (childPath === st.selected ? " fm-row--sel" : ""); rowEl.style.paddingLeft = (depth * 16 + 4) + "px"; + rowEl.tabIndex = 0; + rowEl.setAttribute("role", "treeitem"); + rowEl.setAttribute("aria-selected", childPath === st.selected ? "true" : "false"); + if (entry.isDir) rowEl.setAttribute("aria-expanded", isOpen ? "true" : "false"); @@ - rowEl.addEventListener("click", (ev) => { - ev.stopPropagation(); + const activateRow = () => { const wasSelected = st.selected === childPath; st.selected = childPath; st.selectedIsDir = entry.isDir; if (entry.isDir) { if (isOpen) st.expanded.delete(childPath); else st.expanded.add(childPath); renderFileManager(mod, host); } else if (wasSelected) { openFileEditor(childPath); } else { renderFileManager(mod, host); // reflect selection; click again to open } + }; + rowEl.addEventListener("click", (ev) => { + ev.stopPropagation(); + activateRow(); + }); + rowEl.addEventListener("keydown", (ev) => { + if (ev.target !== rowEl || (ev.key !== "Enter" && ev.key !== " ")) return; + ev.preventDefault(); + ev.stopPropagation(); + activateRow(); });🤖 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/app.js` around lines 2813 - 2869, The file tree rows created in renderFileManager are mouse-only today, so users can’t select, expand, or open entries from the keyboard. Update the row element setup in this block to make each fm-row focusable and operable via Enter/Space, and wire the same click behavior for keyboard activation. Also add appropriate ARIA semantics and state for folders (using the existing entry.isDir and isOpen values) so assistive tech can understand expand/collapse.
🤖 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 1038-1039: The save/apply request path in app.js currently sends
the device payload even when mac is blank, which can make save-profile appear
successful without persisting anything. Add a guard in the save/apply flow
around the device request construction so both save and apply are blocked when
device.mac is missing, using the existing request-building logic near the body:
JSON.stringify call and the save-profile/apply actions to locate it.
- Around line 929-935: The push-device request currently calls
AbortSignal.timeout() directly, which can throw on browsers that do not support
it and prevent the fetch flow from starting. Add a small timeout-signal helper
near the fetch("/api/push-device") call that uses AbortSignal.timeout when
available and falls back to an AbortController with setTimeout otherwise, then
use that helper in the fetch options so onDone still runs and the inject button
is not left disabled.
In `@scripts/moondeck.py`:
- Around line 193-201: The device model is being deduced too early in the data
returned from the device, which causes the later push scheduling logic to
compare against an already-merged value. Update the discovery flow in
moondeck.py so the raw device-reported model and the deduced fallback model are
tracked separately, and only apply the fallback when building the value used for
catalog/push fan-out. Then adjust the compare-and-schedule logic around the push
check so it uses the raw reported model versus the merged model, ensuring the
first discover push is not skipped.
- Around line 193-201: The device identity payload currently stores port-80
devices as a bare IP in the discovery return dict, which causes
refresh_devices.probe to re-probe them with _probe_device(ip) using the default
8080 port. Update the refresh flow so it preserves or reconstructs the correct
target port for bare IPs from the discovery result, and adjust the logic around
_probe_device and the ip field handling in the discovery code so ESP32 devices
discovered on port 80 are rechecked on port 80 during refresh.
- Around line 1070-1078: Require an explicit mac in the save/apply profile flow
and return a failure when no matching active-network device is found instead of
silently succeeding. Update the `/api/save-profile` and `/api/apply-profile`
handling in `moondeck.py` around the `_store` logic and the profile-target
lookup so they validate `mac` up front, search only the active network’s
devices, and report an error when the target device cannot be matched. Keep the
behavior consistent between the two endpoints by reusing the same mac validation
and “device not found” handling paths.
- Around line 1150-1153: The discover flow in the device loop currently skips
entries with no MAC via _device_key(fresh), which removes them from
/api/discover entirely. Update the logic around the devices iteration so
MAC-less peers are still included in discover results, while only
persistence/identity-dependent paths rely on the key. If needed, split the
handling in this section into a persisted-device path and a transient-only path
so the discover response keeps showing online anonymous devices.
In `@src/core/HttpServerModule.cpp`:
- Around line 461-469: The upload draining logic in
`handleConnection()`/`fsWriteStream()` is using a per-call retry loop in
`uploadPull()` that resets the 50 ms budget each time, so a slow client can keep
the request open indefinitely. Change the implementation to enforce one total
deadline for the entire upload request (or move to a tick-driven upload state)
and abort the transfer without committing once that deadline is exceeded, while
preserving the existing bounded-read behavior in `uploadPull()`.
- Around line 202-208: Clamp the buffered upload prefix to the declared
Content-Length in HttpServerModule.cpp: in the POST file upload path that
computes initialLen and calls handleWriteFile, ensure the bytes passed from the
already-read body never exceed contentLen. Update the logic around the
body/initialLen calculation so any extra bytes read past the request body are
excluded before streaming, preventing uploadPull() from writing beyond the
upload and avoiding remaining underflow.
- Around line 451-472: The upload path currently treats a 0 return from
uploadPull() as success, so truncated bodies can still be committed by
fsWriteStream() and renamed into place. Update uploadPull() to distinguish clean
EOF from early close/timeout using an explicit error/abort signal in the
UploadSource/stream callback state, and have fsWriteStream() propagate that
failure instead of committing when the body is incomplete. Use the existing
uploadPull(), UploadSource, and fsWriteStream() symbols to wire the abort path
through the upload flow.
In `@src/platform/platform.h`:
- Around line 115-129: `fsWriteStream` currently cannot distinguish clean EOF
from an aborted upload because `FsWriteSrc` only returns a byte count, so a 0
from `uploadPull` can incorrectly commit a truncated temp file. Update the
streaming contract in `platform.h` by adding an explicit abort/error signal or
separate status return, then propagate that through `uploadPull` and both
platform implementations of `fsWriteStream`. Ensure the temp file is only
renamed on true success, and is discarded on abort or error.
In `@src/ui/app.js`:
- Around line 891-894: The generic control filter in controlRendersGenerically
is still allowing FileManager’s panel-owned “show hidden” toggle to render,
which duplicates the control and skips the immediate tree refresh behavior.
Update the predicate so it explicitly excludes that FileManager control
alongside the existing hidden/filesystem checks, and keep renderFileManager as
the single owner of this toggle so the live-reconfiguration path stays intact.
- Around line 2967-2968: The skipped-upload alert message in the drag-drop flow
is stale and still says “≤ 8 KB,” which conflicts with the actual upload cap.
Update the message in the upload handling logic around the skipped-file alert to
reflect FM_UPLOAD_CAP = 256 * 1024 (or derive the displayed limit from that
constant) so the text matches the real size threshold.
---
Outside diff comments:
In `@src/core/HttpServerModule.cpp`:
- Around line 339-365: The path-decoding helper that trims query `path` into
`out` currently accepts truncated values when the buffer fills up, which can
misroute filesystem actions. Update the loop and postાવ checks so an overlong
decoded path is rejected outright instead of treating the truncated prefix as
valid, and make sure the existing normalization logic around `std::strstr(out,
"..")` and the rooted-path handling only runs for fully captured input.
In `@src/ui/app.js`:
- Around line 3043-3046: After the GET failure in the load handler, the UI
clears body.value but leaves the Save action enabled, which can post an empty
payload; update the failure path in the catch block around the load logic to
also disable Save (or otherwise prevent submission) until a successful load
restores it. Use the existing load/save UI state handling in src/ui/app.js near
the body.value and status.textContent updates to locate the relevant save
control.
- Around line 2813-2869: The file tree rows created in renderFileManager are
mouse-only today, so users can’t select, expand, or open entries from the
keyboard. Update the row element setup in this block to make each fm-row
focusable and operable via Enter/Space, and wire the same click behavior for
keyboard activation. Also add appropriate ARIA semantics and state for folders
(using the existing entry.isDir and isOpen values) so assistive tech can
understand expand/collapse.
🪄 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: e9ac3fa6-a693-450e-b91e-c611e771824f
⛔ Files ignored due to path filters (1)
scripts/build/flash_esp32.pyis excluded by!**/build/**
📒 Files selected for processing (22)
CLAUDE.mddocs/backlog/backlog-core.mddocs/history/plans/Plan-20260704 - File Manager drag-drop + download + usage bar.mddocs/history/plans/Plan-20260704 - Streamed file upload+download (any size).mddocs/moonmodules/core/ui/ui.mdscripts/moondeck.pyscripts/moondeck_ui/app.jsscripts/moondeck_ui/style.csssrc/core/FileManagerModule.cppsrc/core/FileManagerModule.hsrc/core/FilesystemModule.cppsrc/core/FilesystemModule.hsrc/core/HttpServerModule.cppsrc/core/HttpServerModule.hsrc/core/SystemModule.hsrc/platform/desktop/platform_desktop.cppsrc/platform/esp32/platform_esp32.cppsrc/platform/esp32/platform_esp32_fs.cppsrc/platform/platform.hsrc/ui/app.jssrc/ui/style.csstest/unit/core/unit_FileManagerModule.cpp
💤 Files with no reviewable changes (1)
- src/core/FilesystemModule.h
Comment on lines
+1070
to
+1078
| # Store under the device with this MAC (the identity — not the IP, a DHCP lease), in the | ||
| # ACTIVE network only (scoping avoids a collision when two networks share a private subnet). | ||
| mac = (params.get("mac") or "").strip().lower() | ||
| def _store(state) -> None: | ||
| net = _active_network(state) | ||
| if not net: | ||
| return | ||
| for d in net.get("devices") or []: | ||
| if d.get("ip") == ip: | ||
| if mac and (d.get("mac") or "").strip().lower() == mac: |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Require mac and fail when the profile target is not found.
/api/save-profile can now return {"ok": true} even when mac is missing or no active-network device matches it, so the captured profile is silently discarded. Apply the same explicit mac validation to /api/apply-profile too.
Proposed fix
ip = params.get("ip", "")
name = (params.get("name") or "").strip()
- if not ip or not name:
- self._send_json({"error": "ip and name required"}, 400)
+ mac = (params.get("mac") or "").strip().lower()
+ if not ip or not name or not mac:
+ self._send_json({"error": "ip, mac and name required"}, 400)
return
@@
- mac = (params.get("mac") or "").strip().lower()
+ stored = False
def _store(state) -> None:
+ nonlocal stored
net = _active_network(state)
if not net:
return
for d in net.get("devices") or []:
- if mac and (d.get("mac") or "").strip().lower() == mac:
+ if (d.get("mac") or "").strip().lower() == mac:
profiles = [p for p in d.get("profiles", [])
if p.get("name") != name]
profiles.append({"name": name, "modules": modules})
d["profiles"] = profiles
+ stored = True
mutate_state(_store)
+ if not stored:
+ self._send_json({"error": "device not found"}, 404)
+ return
self._send_json({"ok": True, "moduleCount": len(modules)})
@@
ip = params.get("ip", "")
name = (params.get("name") or "").strip()
- if not ip or not name:
- self._send_json({"error": "ip and name required"}, 400)
+ mac = (params.get("mac") or "").strip().lower()
+ if not ip or not name or not mac:
+ self._send_json({"error": "ip, mac and name required"}, 400)
return
@@
- mac = (params.get("mac") or "").strip().lower()Also applies to: 1096-1104
🤖 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.py` around lines 1070 - 1078, Require an explicit mac in the
save/apply profile flow and return a failure when no matching active-network
device is found instead of silently succeeding. Update the `/api/save-profile`
and `/api/apply-profile` handling in `moondeck.py` around the `_store` logic and
the profile-target lookup so they validate `mac` up front, search only the
active network’s devices, and report an error when the target device cannot be
matched. Keep the behavior consistent between the two endpoints by reusing the
same mac validation and “device not found” handling paths.
Comment on lines
+1150
to
+1153
| for fresh in devices: | ||
| ip = fresh.get("ip", "") | ||
| keep = by_ip.get(ip, {}) | ||
| key = _device_key(fresh) | ||
| if not key: | ||
| continue # no MAC → not a persistent device; skip |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the script and inspect the relevant area
ast-grep outline scripts/moondeck.py --view expanded | sed -n '1,220p'
printf '\n--- around the cited lines ---\n'
sed -n '1120,1185p' scripts/moondeck.py
printf '\n--- search for MAC-less / identity contract references ---\n'
rg -n "MAC-less|persistent device|discover response|identity contract|no MAC|fresh in devices|_device_key|merged" scripts/moondeck.pyRepository: MoonModules/projectMM
Length of output: 9380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "MAC-less|persistent device|discover response|device identity|discover.*response|online but not persisted|not persisted" .Repository: MoonModules/projectMM
Length of output: 2862
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- _device_key and discover/refresh helpers ---'
sed -n '129,170p' scripts/moondeck.py
printf '\n--- discover_devices / refresh_devices / link_last_flash ---\n'
sed -n '403,535p' scripts/moondeck.py
printf '\n--- request handlers around discover/refresh endpoints ---\n'
sed -n '948,1048p' scripts/moondeck.pyRepository: MoonModules/projectMM
Length of output: 13200
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "discover_devices\(" scripts/moondeck.py
sed -n '1048,1125p' scripts/moondeck.pyRepository: MoonModules/projectMM
Length of output: 4447
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n '"online"|selected|last_port|deviceModel|devices": \[' scripts/moondeck.py
sed -n '1185,1260p' scripts/moondeck.pyRepository: MoonModules/projectMM
Length of output: 9330
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "discover_devices\(" scripts/moondeck.py
rg -n "refresh_devices\(" scripts/moondeck.py
rg -n "mutate_state\(_merge_discover\)|_merge_discover|_send_json\(state\)|/api/state" scripts/moondeck.pyRepository: MoonModules/projectMM
Length of output: 1555
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "discover_devices\(|refresh_devices\(|_merge_discover|/api/state|active_network|net\[\"devices\"\]" scripts/moondeck.pyRepository: MoonModules/projectMM
Length of output: 2414
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '612,710p' scripts/moondeck.pyRepository: MoonModules/projectMM
Length of output: 4700
Keep MAC-less devices visible in discover results. Line 1153 drops probe results with no mac, so those peers never reach the /api/discover response and disappear entirely instead of staying visible while online. If they should be transient-only, this needs a separate non-persisted path.
🤖 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.py` around lines 1150 - 1153, The discover flow in the
device loop currently skips entries with no MAC via _device_key(fresh), which
removes them from /api/discover entirely. Update the logic around the devices
iteration so MAC-less peers are still included in discover results, while only
persistence/identity-dependent paths rely on the key. If needed, split the
handling in this section into a persisted-device path and a transient-only path
so the discover response keeps showing online anonymous devices.
Bench-tooling and driver-safety pass: MoonDeck now flashes fast by default and resolves baud per exact board, links serial ports across the S31's derived MAC, makes the device name the prominent line, and drops the unused pin-profiles feature. On the device, a WS2812 driver clamps an over-long per-pin strand to a drivable length (with a warning) instead of choking the whole grid onto one pin. File uploads stream with an idle-timeout guard. KPI: 16384lights | PC:465KB | tick:143/111/147/19/2/346/75/22/26/206/148/21/1/45us(FPS:6993/9009/6802/52631/500000/2890/13333/45454/38461/4854/6756/47619/1000000/22222) | ESP32:1358KB | tick:8332us(FPS:120) | heap:126KB | src:171(33102) | test:119(17139) | lizard:117w Core: - HttpServerModule: replaced the whole-upload wall-clock deadline with a per-read idle timeout — bounds a stall (defeats a slowloris trickler) without aborting a legitimate large upload over slow LittleFS/weak WiFi; the compare is uint32-wraparound-safe. - DriverBase: added setConfigWarn() beside setConfigErr() — a borrowed-literal warning with the same "clear only MY status" retract rule, so the WS2812 drivers stop inlining it (No-duplication). Light domain: - PinList: assignCounts() gained a per-pin maxPerPin clamp + a kClampedWarning out-param — a pin over the ceiling drives the first maxPerPin (output stays lit) rather than idling, and signals so the driver can warn. The ceiling is a per-protocol value (WS2812 2048/pin ≈ 16 FPS; a clocked SPI type passes far higher). - RmtLedDriver / ParallelLedDriver: pass kMaxWs2812LedsPerPin and route the clamp signal through DriverBase::setConfigWarn (warns on clamp, clears on recovery). Fixes a 128×128 grid on one pin pinning the tick at ~490 ms (FPS 1). UI: - app.js (MoonDeck): device name is the prominent line, IP demoted to a clickable info-row link; fixed the row wrapper from <label> to <div> so clicking the IP/name opens the device UI in the view pane instead of toggling the selection checkbox; removed the pin-profiles row. - app.js (device): File Manager upload cap message uses the real cap; editor Save disabled on load failure; binary-content detection. Scripts / MoonDeck: - flash_esp32.py: default baud is now 921600 (DIY-bench assumption), resolved by the exact deviceModel (--device-model) so a board's opt-down (the LOLIN's CH340 → 460800) can't leak to a firmware-sibling with a fine bridge; firmware-only flashes take the lowest sibling baud; explicit --baud always wins. - moondeck.py: _mac_matches() links a flash breadcrumb to the ESP32-S31, whose esp_efuse_mac_get_default() returns the EUI-64-truncated MAC (30:ED:A0:FF:FE:F3) rather than the raw efuse the breadcrumb stores; _device_model_for_port() maps a port to its device's model for baud resolution; _link_last_flash clears a port from other devices before linking it (a port maps to one device); _strip_probe_transient() unifies the _reportedModel drop; removed /api/save-profile + /api/apply-profile + _capture_device_profile. - moondeck_config.json: flash card passes pass_port_device_model. - improv_provision.py / build_esp32_ethonly.py: --device-model rename; --firmware esp32-eth fix. Tests: - test_flash_baud.py: pins the fast default, the exact-model opt-down, and the sibling-leak fix. - test_moondeck_mac_match.py: pins the S31 EUI-64-truncated MAC match, directional (no over-acceptance). - installer-flash-baud.test.mjs: pins the installer's catalog-driven baud + safe 460800 default. - unit_RmtLedDriver_pins.cpp: the per-pin clamp + warn signal. - unit_FileManagerModule.cpp: fsWriteStream abort discards the temp file. Docs / CI: - CLAUDE.md: Reviewer runs pre-commit on large commits (≥ ~10 files across areas) or on demand; model fixed to Fable in the agent table (stated once). - deviceModels.json: LOLIN D32 flashBaud 460800 (CH340 opt-down); S31 renamed to fit the 31-char deviceModel buffer; removed the S31's now-redundant flashBaud. - README / RmtLedDriver.md / MoonDeck.md: flashBaud schema row, the WS2812 per-pin ceiling, the fast-default + port→deviceModel baud mapping. - check_devices.py: DEVICE_MODEL_MAX guard + flashBaud validation. Reviews: - 🐇 File Manager upload: stale "≤8KB" alert → real cap; editor Save gated on load; binary detection — fixed. - 🐇 HttpServerModule upload: overlong-path reject, pipelining guard (initial = min(initialLen, contentLen)) — fixed. - 👾 HttpServerModule upload deadline could abort a legit max-size upload — fixed (idle timeout, wraparound-safe). - 👾 Multi-pin over-ceiling clamp shifts later slices — accepted: degrades not crashes (Robustness), pathological config, documented in the warning + RmtLedDriver.md. - 👾 Docs didn't carry flashBaud / the per-pin ceiling / the baud mapping — fixed (three docs). - 👾 _link_last_flash didn't clear a port from other devices — fixed. - 👾 FLASH_BAUDS + orchestrator baud comments were factually stale — fixed. - 👾 warn/clear-on-recovery duplicated in two drivers — fixed (DriverBase::setConfigWarn). - 👾 _reportedModel stripped by two idioms — fixed (_strip_probe_transient). - 👾 S31 catalog-rename re-inject — no-op: the bench S31 already reports the current name. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web-installer/install-orchestrator.js (1)
619-680: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winPreserve board-specific flash baud on reused detect() connections. The
_detectedbranch reuses theESPLoadercreated at 460800 indetect(), so anydeviceModels.jsonflashBaudoverride is skipped on that fast path. Rebuild the loader with the resolved baud, or stash that baud in_detected, before flashing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web-installer/install-orchestrator.js` around lines 619 - 680, The _detected fast path in install-orchestrator.js reuses the ESPLoader created by detect(), so any board-specific flash baud from catalogFlashBaud/deviceModels.json is skipped. Update the _detected branch to carry the resolved flash baud along with port/transport/esploader, or rebuild ESPLoader there using the resolved baud before flashing, so reused detect() connections still honor the board override.
♻️ Duplicate comments (2)
src/core/HttpServerModule.cpp (2)
462-487: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftUpload idle-timeout resets per successful byte — a slow client can wedge the device indefinitely.
kUploadIdleMsbounds the gap between reads, not the total upload duration; a client trickling one byte just under 5s can keephandleConnection()(invoked synchronously fromloop20ms()) blocked with no overall deadline, stalling the scheduler tick for as long as the attacker chooses. This is the same trade-off flagged previously — the fix needs either a whole-request deadline or moving upload draining to a tick-driven state machine.🤖 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/HttpServerModule.cpp` around lines 462 - 487, The upload path in uploadPull currently only enforces kUploadIdleMs between reads, so a client can keep handleConnection() blocked indefinitely by trickling bytes. Update the HttpServerModule.cpp upload flow around uploadPull and its caller to enforce a whole-request deadline as well as the per-read idle timeout, or move the body-drain logic out of the synchronous path into a tick-driven state machine. Make sure the fix is tied to the existing UploadSource/handleConnection flow so a slow upload cannot wedge loop20ms().
418-443: 🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
/api/filecan still serve/overwrite/.config/*.json, leaking plaintext secrets.
serveFileContents/handleWriteFileonly reject..traversal viafileQueryPath; nothing stopsGET /api/file?path=/.config/Network.jsonfrom returning the raw config file. Per this file's ownwriteControlscomment,ControlType::Passwordvalues are stored in plaintext on disk (only the/api/stateAPI view XOR-obfuscates them), so this unauthenticated endpoint can leak WiFi credentials and other secrets verbatim. Thehidden=1dir-listing flag only affects the tree view — it doesn't gate direct-path reads. The write path has the mirrored risk of letting a client silently corrupt persisted module config.Also applies to: 490-524
🤖 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/HttpServerModule.cpp` around lines 418 - 443, The direct file APIs in serveFileContents and handleWriteFile are allowing access to sensitive persisted config files under hidden locations like .config, since fileQueryPath only blocks traversal and not secret-bearing paths. Update the path validation in fileQueryPath or in the read/write handlers to reject access to protected config directories and sensitive JSON files, and make the same check apply to both the GET and write flow so /api/file cannot read or overwrite plaintext secrets.
🤖 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/backlog/backlog-core.md`:
- Around line 314-317: The device model catalog path is stale in the backlog
note: update the reference in the three-level device model section so it matches
the actual shipped catalog location used elsewhere in this PR. Use the same
`web-installer/deviceModels.json` path that appears in the flash-baud contract
test and keep the rest of the provenance note unchanged.
In `@scripts/MoonDeck.md`:
- Around line 344-348: The documentation text under the Board dropdown section
is stale and uses the old “board” wording instead of the renamed device-model
terminology. Update the heading/content in the affected MoonDeck.md section so
it matches the rest of the document’s “Device-model picker” language, using the
same terminology consistently alongside the device-model flow described in the
provisioning instructions.
In `@test/unit/light/unit_RmtLedDriver_pins.cpp`:
- Around line 127-154: The current tests cover auto-distributed counts, but they
do not exercise an explicit oversized ledsPerPin path in assignCounts. Add a
case in unit_RmtLedDriver_pins.cpp that calls assignCounts with a large explicit
string value (for example via the assignCounts helper) and verify it clamps to
mm::kMaxWs2812LedsPerPin, sets warn to mm::kClampedWarning, and still leaves the
remaining pins receiving the correct auto-distributed remainder. Use
assignCounts and the existing clamp-warning assertions as the reference point.
In `@web-installer/install-orchestrator.js`:
- Around line 204-220: `catalogFlashBaud` duplicates the same
`./deviceModels.json` fetch and `catalog.find(b => b && b.name === board)`
lookup already performed in `sendConfigOverSerial`, so consolidate this into a
shared helper such as `fetchCatalogEntry(board)` or equivalent. Update both
`catalogFlashBaud` and `sendConfigOverSerial` to call the shared helper, keeping
the fallback behavior and `DEFAULT_FLASH_BAUD` logic unchanged while eliminating
the extra network round-trip and duplicate lookup code.
---
Outside diff comments:
In `@web-installer/install-orchestrator.js`:
- Around line 619-680: The _detected fast path in install-orchestrator.js reuses
the ESPLoader created by detect(), so any board-specific flash baud from
catalogFlashBaud/deviceModels.json is skipped. Update the _detected branch to
carry the resolved flash baud along with port/transport/esploader, or rebuild
ESPLoader there using the resolved baud before flashing, so reused detect()
connections still honor the board override.
---
Duplicate comments:
In `@src/core/HttpServerModule.cpp`:
- Around line 462-487: The upload path in uploadPull currently only enforces
kUploadIdleMs between reads, so a client can keep handleConnection() blocked
indefinitely by trickling bytes. Update the HttpServerModule.cpp upload flow
around uploadPull and its caller to enforce a whole-request deadline as well as
the per-read idle timeout, or move the body-drain logic out of the synchronous
path into a tick-driven state machine. Make sure the fix is tied to the existing
UploadSource/handleConnection flow so a slow upload cannot wedge loop20ms().
- Around line 418-443: The direct file APIs in serveFileContents and
handleWriteFile are allowing access to sensitive persisted config files under
hidden locations like .config, since fileQueryPath only blocks traversal and not
secret-bearing paths. Update the path validation in fileQueryPath or in the
read/write handlers to reject access to protected config directories and
sensitive JSON files, and make the same check apply to both the GET and write
flow so /api/file cannot read or overwrite plaintext secrets.
🪄 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: 7510865d-5f0b-4908-bbeb-557d0eb3e2a9
⛔ Files ignored due to path filters (3)
scripts/build/build_esp32_ethonly.pyis excluded by!**/build/**scripts/build/flash_esp32.pyis excluded by!**/build/**scripts/build/improv_provision.pyis excluded by!**/build/**
📒 Files selected for processing (28)
CLAUDE.mddocs/backlog/backlog-core.mddocs/moonmodules/light/drivers/RmtLedDriver.mddocs/moonmodules/light/effects/effects.mdscripts/MoonDeck.mdscripts/check/check_devices.pyscripts/moondeck.pyscripts/moondeck_config.jsonscripts/moondeck_ui/app.jsscripts/moondeck_ui/index.htmlscripts/moondeck_ui/style.csssrc/core/HttpServerModule.cppsrc/light/drivers/Drivers.hsrc/light/drivers/ParallelLedDriver.hsrc/light/drivers/PinList.hsrc/light/drivers/RmtLedDriver.hsrc/platform/desktop/platform_desktop.cppsrc/platform/esp32/platform_esp32_fs.cppsrc/platform/platform.hsrc/ui/app.jstest/js/installer-flash-baud.test.mjstest/python/test_flash_baud.pytest/python/test_moondeck_mac_match.pytest/unit/core/unit_FileManagerModule.cpptest/unit/light/unit_RmtLedDriver_pins.cppweb-installer/README.mdweb-installer/deviceModels.jsonweb-installer/install-orchestrator.js
Comment on lines
+344
to
+348
| **Board dropdown (pre-association injection)**: pick your device model next to the Firmware dropdown and the flow forwards `--device-model` — the script then resolves the deviceModel's `deviceModels.json` settings and pushes the TX-power cap over the `SET_TX_POWER` vendor RPC **before** the credentials, plus `SET_DEVICE_MODEL` after success. This matters for brown-out-prone weak-powered device models (cap 8 dBm): at full TX power they fail their very first WiFi association, so the cap can't wait for the post-online HTTP injection. Leave the dropdown on "(any model)" for device models without special settings. | ||
|
|
||
| ```bash | ||
| # Equivalent CLI for a weak-powered board (cap resolved from deviceModels.json): | ||
| uv run scripts/build/improv_provision.py --port /dev/cu.usbmodem-XXX --board "ESP32-S3 N16R8 Dev" | ||
| uv run scripts/build/improv_provision.py --port /dev/cu.usbmodem-XXX --device-model "ESP32-S3 N16R8 Dev" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Stale "Board dropdown" heading — inconsistent with the device-model rename.
Line 344 still reads "Board dropdown (pre-association injection): pick your device model next to the Firmware dropdown", but the rest of the document (e.g. line 18's "Device-model picker") has been renamed away from "board." Update the heading to "Device-model dropdown" for consistency.
🧰 Tools
🪛 LanguageTool
[style] ~344-~344: To elevate your writing, try using an alternative expression here.
Context: ... SET_DEVICE_MODEL after success. This matters for brown-out-prone weak-powered device...
(MATTERS_RELEVANT)
🤖 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.md` around lines 344 - 348, The documentation text under the
Board dropdown section is stale and uses the old “board” wording instead of the
renamed device-model terminology. Update the heading/content in the affected
MoonDeck.md section so it matches the rest of the document’s “Device-model
picker” language, using the same terminology consistently alongside the
device-model flow described in the provisioning instructions.
… fixes
Renames the top-level scripts/ folder to moondeck/ (its home is the MoonDeck dev console) with a moondeck/ci/ split for the two CI-only release scripts, then makes MoonDeck stream card output live instead of dumping it at the end, and fixes a batch of smaller MoonDeck/installer issues found while shaking out the rename.
KPI: 16384lights | PC:646KB | tick:150/115/155/20/2/369/78/23/28/214/155/22/1/48us(FPS:6666/8695/6451/50000/500000/2710/12820/43478/35714/4672/6451/45454/1000000/20833) | ESP32:1388KB | tick:925us(FPS:1081) | heap:8385KB | src:171(33116) | test:119(17150) | lizard:117w
Core:
- HttpServerModule: the whole-request upload cap is now checked on every uploadPull entry, not only while the socket is dry — a paced trickler that always keeps a byte ready could previously evade it (the idle-only timeout still resets on progress; both compares stay wraparound-safe).
Scripts / MoonDeck:
- scripts/ → moondeck/ (wholesale rename): the folder is MoonDeck's home, holding the console (moondeck.py, moondeck_ui/, moondeck_config.json, MoonDeck.md) plus the build/check/scenario/run tooling it invokes. Function-based subfolders kept (not reorganised by UI tab — the tab a card shows on is config data, not directory structure). The two truly-CI-only scripts (package_desktop, verify_version) move to moondeck/ci/. All references updated: CI workflows, CMake (root + esp32), mkdocs, .gitignore, .coderabbit, the 10 test-path constructions, and every card path.
- moondeck.py: cards now run under a pseudo-terminal (Unix) / PYTHONUNBUFFERED (Windows) so a long ESP32 build/flash streams progress live to the SSE log instead of dumping at exit — the standard unbuffer/script trick, one change at the single spawn point so every card benefits. The pty master fd is closed on all paths (spawn failure, kill, stream end); the stream teardown pops the registry entry only if it still owns the proc (a fast re-run under the same id no longer breaks the new proc's Stop).
- moondeck.py: Live Scenarios / scenario_pipeline no longer require a scenario or module selection — both scripts treat empty/"all" as "run everything", so requiring them wrongly blocked the "all" case ("Select a scenario first" with a scenario picked). Only port + firmware stay mandatory.
- flash_esp32.py: the post-flash .last_flash.json breadcrumb writes to moondeck/ (a stale scripts/ path the rename sweep missed was raising FileNotFoundError after an otherwise-successful flash).
- check_specs gains a MoonDeck card (the always-run spec gate joins its sibling check cards); the legacy build_esp32_ethonly.py wrapper is deleted (use build_esp32.py --firmware esp32-eth).
UI:
- app.js (MoonDeck): the port dropdown shows a "last flashed: <device> · <model>" hint under it, looked up from the last_port breadcrumb — so a /dev/cu.* port isn't opaque; the hint refreshes after Discover and Refresh (both can re-attribute last_port).
Web installer:
- install-orchestrator.js: fetchCatalogEntry() shared by the flash-baud lookup and the serial-config push (dedup, one catalog fetch); a reused detect() connection re-negotiates to the board's catalog flashBaud via changeBaud() so the baud override isn't skipped on that path.
Tests:
- unit_RmtLedDriver_pins.cpp: an explicit oversized-ledsPerPin case pinning the clamp + warn + remainder-from-requested-count semantics.
Docs / CI:
- CLAUDE.md: the Reviewer runs pre-commit on a large commit (or on demand), stated once with the model fixed to Fable in the agent table.
- .gitignore: MoonDeck state (moondeck.json, .last_flash.json) ignored at the renamed path — a stale scripts/ rule had left the state file (WiFi creds + device list) stageable.
- mkdocs: broken directory-links repointed at their index pages, the frozen-plan cross-repo links delinked, and the /install/ nav made a full external URL — the site builds warning-free.
- Plan-20260705 - Rename scripts to moondeck.md saved.
Reviews:
- 👾 Upload hard-cap only checked while the socket was dry (paced-trickler slowloris gap) — fixed (checked every pull entry).
- 👾 Pty master/slave fd leaked on spawn failure and on kill-before-stream — fixed (closed on both paths; verified 0 fd growth over repeated runs).
- 👾 Stream teardown could pop a re-run proc under the same id — fixed (pop only if still the registered proc).
- 👾 Port hint went stale after Refresh (a last_port re-attribution consumer) — fixed (hint refreshes after Refresh too).
- 👾 changeBaud fallback comment overclaimed "still open at the prior baud" — softened to best-effort.
- 👾 SSE disconnect now tears down the child on Unix (browser reload aborts a build) — accepted: dead-and-visible beats the old wedged-and-invisible pipe behaviour.
- 👾 Present-tense comment nits, a bespoke fd-stash pattern, and a pre-existing _serve_static traversal gap — deferred: pre-existing / Low, and some fixes would trigger the build gates for a comment; backlogged as a sweep, not this commit.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
moondeck/moondeck_ui/app.js (1)
936-963: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCatalog-shape doc comment is stale relative to the actual entry format.
The top-of-file state doc (see
deviceModelscomment near init) documents catalog entries as{ key, label, firmwares }, but the code here explicitly notes "the single-name catalog (no separate key/label)" and consumesb.namefor both value and label (line 957). Worth updating the earlier top-of-file comment so the documented shape matches what the code actually consumes — avoids confusing a future reader who trusts the header comment.🤖 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 `@moondeck/moondeck_ui/app.js` around lines 936 - 963, The device-model catalog documentation is stale and still describes entries as having separate key/label fields, while the picker logic in device-model handling uses a single-name shape via b.name. Update the top-of-file deviceModels state comment to match the actual catalog format consumed by deviceModelPicker and the fwModels mapping, so the documented schema aligns with what the code expects.moondeck/moondeck.py (2)
1031-1038: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale "board" wording left over from the deviceModel rename.
Several do_POST comments still describe the renamed fields/tuples as "board" even though the code now uniformly uses
deviceModel:"Push a single (ip, board) to a device"/"picks a board from the per-device dropdown"(1032-1033),"(ip, board) tuples"(1059, 1145),"carrying the user fields (board, last_port, selected) forward"(1080), and"Schedule a board push for every online device with a non-empty board"(1173-1174). This directly contradicts the PR's intent of migrating identity fromboardtodeviceModeland will confuse future readers into thinking there's a separateboardfield still in play.♻️ Proposed wording fix
- # Push a single (ip, board) to a device. Called by the JS when the - # user picks a board from the per-device dropdown — saveState + # Push a single (ip, deviceModel) to a device. Called by the JS when the + # user picks a deviceModel from the per-device dropdown — saveState @@ - pushes = [] # (ip, board) tuples populated by _merge_discover + pushes = [] # (ip, deviceModel) tuples populated by _merge_discover @@ - # user fields (board, last_port, selected) forward. A found device WITHOUT a MAC is + # user fields (deviceModel, last_port, selected) forward. A found device WITHOUT a MAC is @@ - pushes = [] # (ip, board) tuples populated by _merge_refresh + pushes = [] # (ip, deviceModel) tuples populated by _merge_refresh @@ - # Schedule a board push for every online device with a - # non-empty board. Redundant writes are cheap on the device + # Schedule a deviceModel push for every online device with a + # non-empty deviceModel. Redundant writes are cheap on the deviceAlso applies to: 1059-1059, 1078-1081, 1145-1145, 1173-1177
🤖 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 `@moondeck/moondeck.py` around lines 1031 - 1038, Update the stale do_POST comments in moondeck.py so they consistently refer to deviceModel instead of board. In the /api/push-device handler and the related bulk-push comments, replace wording like “(ip, board)”, “picks a board”, “board push”, and “user fields (board, last_port, selected)” with the renamed deviceModel terminology. Use the nearby symbols do_POST, /api/push-device, saveState, and the device persistence flow to ensure all affected comments match the current data model.
1244-1253: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the same device-model key on both sides.
moondeck/moondeck_ui/app.js:525sendsparams.device_model, butmoondeck/moondeck.py:1251readsparams.get("deviceModel"), sopass_device_modelnever forwards--device-modelfrom provisioning. Rename one side or accept both keys.🤖 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 `@moondeck/moondeck.py` around lines 1244 - 1253, The pass_device_model plumbing is using mismatched parameter keys, so the provisioning path never forwards the device model to improv_provision.py. Update the parameter handling in moondeck.py’s script execution block to read the same key sent by moondeck_ui/app.js (or accept both the camelCase and snake_case forms) before extending cmd with --device-model, ensuring the deviceModel value reaches the pass_device_model branch.
♻️ Duplicate comments (1)
src/core/HttpServerModule.cpp (1)
333-369: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winStill unresolved:
/api/fileGET serves.config/*.json(plaintext secrets) with no restriction.
fileQueryPathonly rejects".."traversal; it has no denylist for/.config/. Per the codebase's own comment onwriteControls(Password fields are "sent XOR-obfuscated... not in plaintext" only through/api/state), the underlying.config/<Module>.jsonfiles persistControlType::Passwordin plaintext. SinceserveFileContentsstreams any resolved path without a hidden/.configcheck,GET /api/file?path=/.config/Network.json(or whichever module holds Wi-Fi/API secrets) returns the raw plaintext value over an unauthenticated LAN endpoint. This is the same concern raised on a previous revision of this range and does not appear to carry an "Addressed" marker, unlike four sibling findings in this file that do.🔒 Suggested fix: deny `.config/` reads at the shared path guard
if (i == 0 || std::strstr(out, "..")) return false; // empty or traversal → reject + if (std::strstr(out, "/.config/") || std::strncmp(out, "/.config", 8) == 0) return false; if (out[0] != '/') { // relative → root at the mountAlso applies to: 418-443
🤖 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/HttpServerModule.cpp` around lines 333 - 369, The shared path guard in fileQueryPath allows resolved reads of hidden config files, which lets serveFileContents return plaintext secrets from .config JSON files. Update fileQueryPath (and the GET /api/file path flow it feeds) to explicitly reject any request targeting .config or hidden config files before routing, while keeping the existing traversal and overlong-path checks intact. Use the existing fileQueryPath and serveFileContents symbols to place the denylist at the centralized path validation point.
🤖 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/backlog/docs-system-overhaul.md`:
- Line 92: The decision note wording in the backlog doc is misleading because
the rename label in the referenced note no longer reflects the actual directory
change. Update the note to clearly state the real source and destination names,
keeping the reference to the deferred rename item
(`rename-scripts-to-moondeck.md`) intact so the record is unambiguous.
In `@docs/backlog/leddriver-analysis-top-down.md`:
- Line 395: The KPI collector hyperlink in the docs points to the wrong target
and will 404. Update the markdown link in the fps/jitter KPI line so it
references the actual collector location under moondeck/check/collect_kpi.py
instead of moondeck/collect/collect_kpi.py, keeping the surrounding prose
unchanged.
In `@docs/backlog/rename-scripts-to-moondeck.md`:
- Around line 1-9: The backlog note has a stale rename description and an
inconsistent example path. Update the heading/title text in the markdown note so
the source and target folder names match the actual top-level rename being
documented, and fix the MoonDeck example so it includes the folder prefix
consistently. Keep the change limited to the backlog doc and adjust the wording
in the section that describes the separate commit/sweep.
In `@docs/backlog/rename-to-moonlight.md`:
- Around line 65-69: The blockquote in this rename doc contains an empty line,
which triggers markdownlint MD028. Remove the blank line inside the quoted
section in the affected block so the Markdown stays a single contiguous
blockquote while preserving the existing text and structure.
In `@docs/coding-standards.md`:
- Around line 119-125: The summary-page prose in coding-standards currently uses
a fenced heading token with a trailing space, which triggers markdownlint MD038.
Update the affected `### ` prose block in the summary-page guidance to use `###`
with no trailing space, keeping the rest of the table-generation and
`mkdocs_hooks.py` guidance unchanged.
In `@docs/history/plans/Plan-20260624` - Semver version + update-available
badge.md:
- Around line 45-46: The Markdown heading for the Verification section is
missing the required blank line before the following list, which triggers MD022.
Update the plan document so `## Verification` is followed by an empty line
before the verification bullets, keeping the rest of the content unchanged.
In `@docs/history/plans/Plan-20260702` - Docs site Phase 0 (MkDocs Material).md:
- Line 50: The deferred rename note is incorrect because it now says moondeck/ →
moondeck/, which hides the remaining work. Update the note in the plan doc to
reflect the actual pending rename from scripts/ to moondeck/, keeping the
reference to the deferred next commit and the rename-scripts-to-moondeck backlog
item.
In `@docs/history/plans/Plan-20260705` - Rename scripts to moondeck.md:
- Around line 1-10: The plan title and decision summary in this document have
the rename direction reversed, making the migration look like a no-op. Update
the main heading and the first decision item to reflect the actual move from
scripts to moondeck, and ensure the wording stays consistent with the rest of
the plan; use the plan title and the “Decision (what ships)” section as the
primary anchors.
In `@moondeck/diag/preview_health.py`:
- Line 238: The config load in preview_health.py uses an անմanaged open() call,
so update the JSON read to use a context manager around the moondeck.json file
handle. Keep the behavior the same, but ensure the file is opened and closed via
a with block in the code that assigns cfg, so the cleanup is localized to the
config-loading logic.
In `@moondeck/docs/generate_test_docs.py`:
- Around line 391-392: The regeneration hint in generate_test_docs should match
the documented command by including uv run. Update the failure message in the
args.check and not ok branch so it tells users to rerun
moondeck/docs/generate_test_docs.py via uv run, keeping the instruction
copy-pasteable and consistent with the documented invocation.
In `@moondeck/docs/screenshot_modules.py`:
- Line 379: The string in the screenshot generation output is an unused f-string
and will trigger Ruff F541. Update the print statement in the documentation
helper so it is a plain string literal without the f prefix, keeping the
displayed command text unchanged. Use the surrounding screenshot_modules output
logic to locate the exact print call.
In `@moondeck/run/preview_installer.py`:
- Line 307: Remove the unnecessary f-string prefix from the print statement in
the preview installer flow, since the message in the run/preview_installer.py
output has no interpolated values and Ruff flags this as F541. Update the print
call in the code path that emits the “run uv run moondeck/build/build_esp32.py
--firmware <variant> first to enable end-to-end flash” message to use a plain
string literal instead of an f-string.
---
Outside diff comments:
In `@moondeck/moondeck_ui/app.js`:
- Around line 936-963: The device-model catalog documentation is stale and still
describes entries as having separate key/label fields, while the picker logic in
device-model handling uses a single-name shape via b.name. Update the
top-of-file deviceModels state comment to match the actual catalog format
consumed by deviceModelPicker and the fwModels mapping, so the documented schema
aligns with what the code expects.
In `@moondeck/moondeck.py`:
- Around line 1031-1038: Update the stale do_POST comments in moondeck.py so
they consistently refer to deviceModel instead of board. In the /api/push-device
handler and the related bulk-push comments, replace wording like “(ip, board)”,
“picks a board”, “board push”, and “user fields (board, last_port, selected)”
with the renamed deviceModel terminology. Use the nearby symbols do_POST,
/api/push-device, saveState, and the device persistence flow to ensure all
affected comments match the current data model.
- Around line 1244-1253: The pass_device_model plumbing is using mismatched
parameter keys, so the provisioning path never forwards the device model to
improv_provision.py. Update the parameter handling in moondeck.py’s script
execution block to read the same key sent by moondeck_ui/app.js (or accept both
the camelCase and snake_case forms) before extending cmd with --device-model,
ensuring the deviceModel value reaches the pass_device_model branch.
---
Duplicate comments:
In `@src/core/HttpServerModule.cpp`:
- Around line 333-369: The shared path guard in fileQueryPath allows resolved
reads of hidden config files, which lets serveFileContents return plaintext
secrets from .config JSON files. Update fileQueryPath (and the GET /api/file
path flow it feeds) to explicitly reject any request targeting .config or hidden
config files before routing, while keeping the existing traversal and
overlong-path checks intact. Use the existing fileQueryPath and
serveFileContents symbols to place the denylist at the centralized path
validation point.
🪄 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: a214aced-d165-4a7f-98ea-425697f9e526
⛔ Files ignored due to path filters (16)
moondeck/build/_idf_win_shim.pyis excluded by!**/build/**moondeck/build/build_desktop.pyis excluded by!**/build/**moondeck/build/build_esp32.pyis excluded by!**/build/**moondeck/build/clean_esp32.pyis excluded by!**/build/**moondeck/build/compute_version.pyis excluded by!**/build/**moondeck/build/erase_flash_esp32.pyis excluded by!**/build/**moondeck/build/flash_esp32.pyis excluded by!**/build/**moondeck/build/generate_build_info.pyis excluded by!**/build/**moondeck/build/generate_firmwares.pyis excluded by!**/build/**moondeck/build/generate_manifest.pyis excluded by!**/build/**moondeck/build/host_wifi.pyis excluded by!**/build/**moondeck/build/improv_probe.pyis excluded by!**/build/**moondeck/build/improv_provision.pyis excluded by!**/build/**moondeck/build/improv_smoke_test.pyis excluded by!**/build/**moondeck/build/setup_esp_idf.pyis excluded by!**/build/**scripts/build/build_esp32_ethonly.pyis excluded by!**/build/**
📒 Files selected for processing (114)
.coderabbit.yaml.github/workflows/release.yml.github/workflows/test.yml.gitignoreCLAUDE.mdCMakeLists.txtREADME.mddocs/architecture.mddocs/backlog/README.mddocs/backlog/backlog-core.mddocs/backlog/docs-system-overhaul.mddocs/backlog/folder-structure-proposal.mddocs/backlog/leddriver-analysis-top-down.mddocs/backlog/rename-scripts-to-moondeck.mddocs/backlog/rename-to-moonlight.mddocs/building.mddocs/coding-standards.mddocs/gettingstarted.mddocs/history/README.mddocs/history/decisions.mddocs/history/plans/Plan-20260519 - ESP32 Deployment.mddocs/history/plans/Plan-20260520 - Adaptive Memory Allocation & Memory Scenario Testing.mddocs/history/plans/Plan-20260520 - Live Scenario Testing (Item 8).mddocs/history/plans/Plan-20260521 - Control-list-driven JSON persistence (item 11).mddocs/history/plans/Plan-20260522 - Nest child module cards inside their parent card's box.mddocs/history/plans/Plan-20260522 - UI rewrite to ui-spec.md baseline (item 12).mddocs/history/plans/Plan-20260523 - Top-level shape change toLayouts,Layers,Drivers.mddocs/history/plans/Plan-20260524 - Release 1.0 distribution - web installer + GitHub Releases.mddocs/history/plans/Plan-20260525 - Release-channel picker + first-boot WiFi provisioning.mddocs/history/plans/Plan-20260620 - Improv-as-REST - push device-model config over serial.mddocs/history/plans/Plan-20260621 - Improv frame-contract unit tests (pytest + node-test).mddocs/history/plans/Plan-20260622 - Non-blocking preview send (shipped).mddocs/history/plans/Plan-20260624 - Dev-channel update badge.mddocs/history/plans/Plan-20260624 - Semver version + update-available badge.mddocs/history/plans/Plan-20260626 - Composable modifiers (chain the whole stack).mddocs/history/plans/Plan-20260626 - MoonLive Stage 0 (native codegen spike) (shipped).mddocs/history/plans/Plan-20260627 - MoonLive Stage 1 controls (shipped).mddocs/history/plans/Plan-20260627 - MoonLive Stage 3 (IR seam + assembler, second statement).mddocs/history/plans/Plan-20260628 - ESP32-S31 board + IDF pin + System sdkDate (shipped).mddocs/history/plans/Plan-20260630 - Doc consolidation - per-type compact pages (effects-modifiers-layouts).mddocs/history/plans/Plan-20260702 - Docs Phase 1+2 (nav fold + tests in build).mddocs/history/plans/Plan-20260702 - Docs Phase 3 (drift validation, not snippet de-dup).mddocs/history/plans/Plan-20260702 - Docs Phase 4b (Doxide pilot, attempted, abandoned).mddocs/history/plans/Plan-20260702 - Docs Phase 4b (source-generated technical docs, superseded by Docs v2).mddocs/history/plans/Plan-20260702 - Docs site Phase 0 (MkDocs Material).mddocs/history/plans/Plan-20260702 - Docs v2 (two-surface module docs).mddocs/history/plans/Plan-20260705 - Rename scripts to moondeck.mddocs/history/release-notes-v1.0.0.mddocs/moonmodules/core/archive/FirmwareUpdateModule.mddocs/moonmodules/core/archive/ImprovProvisioningModule.mddocs/moonmodules/light/drivers/NetworkSendDriver.mddocs/moonmodules/light/layouts/layouts.mddocs/performance.mddocs/testing.mdesp32/main/CMakeLists.txtmkdocs.ymlmoondeck/MoonDeck.mdmoondeck/check/check_devices.pymoondeck/check/check_firmwares.pymoondeck/check/check_platform_boundary.pymoondeck/check/check_specs.pymoondeck/check/collect_kpi.pymoondeck/ci/package_desktop.pymoondeck/ci/verify_version.pymoondeck/diag/preview_health.pymoondeck/docs/_test_metadata.pymoondeck/docs/build_docs.pymoondeck/docs/gen_api.pymoondeck/docs/generate_test_docs.pymoondeck/docs/install_playwright.pymoondeck/docs/mkdocs_hooks.pymoondeck/docs/moxygen-templates/cpp/class.mdmoondeck/docs/moxygen-templates/cpp/index.mdmoondeck/docs/moxygen-templates/cpp/namespace.mdmoondeck/docs/moxygen-templates/cpp/page.mdmoondeck/docs/screenshot_modules.pymoondeck/docs/update_module_docs.pymoondeck/moondeck.pymoondeck/moondeck_config.jsonmoondeck/moondeck_ui/app.jsmoondeck/moondeck_ui/index.htmlmoondeck/moondeck_ui/style.cssmoondeck/rename/rename_to_moonlight.pymoondeck/report/history_report.pymoondeck/run/monitor_esp32.pymoondeck/run/preview_installer.pymoondeck/run/run_desktop.pymoondeck/run/show_crash_log.pymoondeck/scenario/_net_probe.pymoondeck/scenario/_observed.pymoondeck/scenario/_preview_ws.pymoondeck/scenario/run_live_scenario.pymoondeck/scenario/run_network_live.pymoondeck/scenario/run_network_roundtrip.pymoondeck/scenario/run_scenario.pymoondeck/test/test_desktop.pysrc/core/HttpServerModule.cpptest/js/improv-frame.test.mjstest/python/test_build_esp32_s31.pytest/python/test_build_esp32_venv_select.pytest/python/test_check_specs_drift.pytest/python/test_compute_version.pytest/python/test_flash_baud.pytest/python/test_improv_frame.pytest/python/test_installer_manifests.pytest/python/test_mkdocs_slug.pytest/python/test_moondeck_mac_match.pytest/python/test_verify_version.pytest/scenarios/core/scenario_MoonModule_control_change.jsontest/scenarios/core/scenario_NetworkModule_mdns_toggle.jsontest/unit/light/unit_RmtLedDriver_pins.cppweb-installer/README.mdweb-installer/improv-frame.jsweb-installer/install-orchestrator.js
💤 Files with no reviewable changes (1)
- moondeck/check/check_devices.py
Comment on lines
+65
to
+69
| > **Could we reuse `library.json`'s `name` now where a literal sits (subtraction, not a new constant)?** Surveyed `moondeck/` for it — verdict: **no genuine low-hanging fruit.** ~95% of `projectMM` literals there are the **binary name** (`build/…/projectMM`, `.bin`, `.exe`, `.log`, `pkill projectMM`, crash `.ips`) which must track the **CMake target**, not `library.json` (wiring them to the product name would break the path to the file on disk); plus one **wire literal** (`_net_probe.py` ArtNet source-name, must byte-match the device) and ~15 prose/docstrings. The only product-name candidates — `generate_manifest.py`'s manifest `name`/`home_assistant_domain` — must *stay* `projectMM` today (Step 2), don't currently read `library.json`, and flip alongside `library.json` in the sweep anyway, so wiring them is new plumbing for zero present benefit. The principle (reuse an existing source of truth over a hardcoded literal) is right; it just has no payoff here because the literals are either binary-coupled or static-until-the-switch. (The real home for product-identity reuse is still the library API — see the box above.) | ||
|
|
||
| > **The constant has a real future home: projectMM/MoonLight as a library.** When the project is offered as an embeddable library, a consumer will want one runtime identity to read (an "About"/banner string, the protocol source-name they can query) — *that* is the ongoing, widely-referenced use a `kProjectName` constant genuinely earns (the test the rename failed). But build it **then**, against a real library API surface (it may want to be a small `ProjectInfo` — name + version + url — not a bare string), per *Concrete first, abstract later* — not speculatively now. Tracked as a seed in [backlog-core](backlog-core.md); when the library work starts, introduce the identity constant as part of its public API and let the wire-strings + UI derive from it. | ||
| 4. **Author the mechanical sweep script** — ✅ **Done:** [`scripts/rename/rename_to_moonlight.py`](../../scripts/rename/rename_to_moonlight.py), dry-run by default (`--apply` writes; reserved for switch-day Phase 3.3, *after* the repo rename). What the dry-run against today's tree established: replaces two tokens (`ProjectMM` the enum, then `projectMM`) — a plain token swap is correct for *every* form (repo URL, host path, `projectMM.bin`, product name, `deviceName` slug) since `projectMM` is never a substring of another token; file list comes from `git ls-files` so build output (`build/`, `esp32/build/`) is excluded without a brittle blocklist; `docs/history` (era record) + the rename doc itself are content-excluded. Verified: **542 hits across 113 files**, and `MoonLive` / predecessor `MoonLight` / `namespace mm` are provably never touched (0 files where their count changes). The enum rename is safe — device classification keys on the `"modules"` marker, not the label string. The script de-risks switch-day; it is NOT run with `--apply` until then. | ||
| 5. **Prep MoonDeck / `moondeck.json` / bench registry** — ✅ **investigated; nothing to change now, two things flagged for switch-day.** (a) **The functional chain stays `projectMM` until the switch (and flips together in the sweep):** `moondeck_config.json`'s `process_name: "projectMM"` ↔ the CMake binary `projectMM` ↔ the `build/<host>/projectMM` run/log path ↔ `pkill projectMM`. These are tracked files the sweep rewrites in one pass, so they stay consistent — changing `process_name` early would break MoonDeck's process detection against today's binary, so don't. (b) **The sweep cannot reach the gitignored bench registry** `scripts/moondeck.json` (it's private, per [[bench-setup]]; the sweep uses `git ls-files`). Its `"board": "projectMM testbench …"` values reference catalog `name`s that *do* flip — so after the switch they'd mismatch only on your bench. **Switch-day local-tooling note: hand-update `scripts/moondeck.json` board names** (and re-provision bench devices if you want the new mDNS identity) — the sweep covers tracked files only. The MoonDeck prose (`MoonDeck.md`, code comments) flips in the normal sweep. | ||
| 4. **Author the mechanical sweep script** — ✅ **Done:** [`moondeck/rename/rename_to_moonlight.py`](../../moondeck/rename/rename_to_moonlight.py), dry-run by default (`--apply` writes; reserved for switch-day Phase 3.3, *after* the repo rename). What the dry-run against today's tree established: replaces two tokens (`ProjectMM` the enum, then `projectMM`) — a plain token swap is correct for *every* form (repo URL, host path, `projectMM.bin`, product name, `deviceName` slug) since `projectMM` is never a substring of another token; file list comes from `git ls-files` so build output (`build/`, `esp32/build/`) is excluded without a brittle blocklist; `docs/history` (era record) + the rename doc itself are content-excluded. Verified: **542 hits across 113 files**, and `MoonLive` / predecessor `MoonLight` / `namespace mm` are provably never touched (0 files where their count changes). The enum rename is safe — device classification keys on the `"modules"` marker, not the label string. The script de-risks switch-day; it is NOT run with `--apply` until then. | ||
| 5. **Prep MoonDeck / `moondeck.json` / bench registry** — ✅ **investigated; nothing to change now, two things flagged for switch-day.** (a) **The functional chain stays `projectMM` until the switch (and flips together in the sweep):** `moondeck_config.json`'s `process_name: "projectMM"` ↔ the CMake binary `projectMM` ↔ the `build/<host>/projectMM` run/log path ↔ `pkill projectMM`. These are tracked files the sweep rewrites in one pass, so they stay consistent — changing `process_name` early would break MoonDeck's process detection against today's binary, so don't. (b) **The sweep cannot reach the gitignored bench registry** `moondeck/moondeck.json` (it's private, per [[bench-setup]]; the sweep uses `git ls-files`). Its `"board": "projectMM testbench …"` values reference catalog `name`s that *do* flip — so after the switch they'd mismatch only on your bench. **Switch-day local-tooling note: hand-update `moondeck/moondeck.json` board names** (and re-provision bench devices if you want the new mDNS identity) — the sweep covers tracked files only. The MoonDeck prose (`MoonDeck.md`, code comments) flips in the normal sweep. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the blank line inside the blockquote.
That empty line will trip markdownlint MD028 and can fail the docs lint step.
Proposed fix
- > **Could we reuse `library.json`'s `name` now where a literal sits (subtraction, not a new constant)?** Surveyed `moondeck/` for it — verdict: **no genuine low-hanging fruit.** ~95% of `projectMM` literals there are the **binary name** (`build/…/projectMM`, `.bin`, `.exe`, `.log`, `pkill projectMM`, crash `.ips`) which must track the **CMake target**, not `library.json` (wiring them to the product name would break the path to the file on disk); plus one **wire literal** (`_net_probe.py` ArtNet source-name, must byte-match the device) and ~15 prose/docstrings. The only product-name candidates — `generate_manifest.py`'s manifest `name`/`home_assistant_domain` — must *stay* `projectMM` today (Step 2), don't currently read `library.json`, and flip alongside `library.json` in the sweep anyway, so wiring them to the product name is new plumbing for zero present benefit. The principle (reuse an existing source of truth over a hardcoded literal) is right; it just has no payoff here because the literals are either binary-coupled or static-until-the-switch. (The real home for product-identity reuse is still the library API — see the box above.)
->
+ > **Could we reuse `library.json`'s `name` now where a literal sits (subtraction, not a new constant)?** Surveyed `moondeck/` for it — verdict: **no genuine low-hanging fruit.** ~95% of `projectMM` literals there are the **binary name** (`build/…/projectMM`, `.bin`, `.exe`, `.log`, `pkill projectMM`, crash `.ips`) which must track the **CMake target**, not `library.json` (wiring them to the product name would break the path to the file on disk); plus one **wire literal** (`_net_probe.py` ArtNet source-name, must byte-match the device) and ~15 prose/docstrings. The only product-name candidates — `generate_manifest.py`'s manifest `name`/`home_assistant_domain` — must *stay* `projectMM` today (Step 2), don't currently read `library.json`, and flip alongside `library.json` in the sweep anyway, so wiring them to the product name is new plumbing for zero present benefit. The principle (reuse an existing source of truth over a hardcoded literal) is right; it just has no payoff here because the literals are either binary-coupled or static-until-the-switch. (The real home for product-identity reuse is still the library API — see the box above.)📝 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.
Suggested change
| > **Could we reuse `library.json`'s `name` now where a literal sits (subtraction, not a new constant)?** Surveyed `moondeck/` for it — verdict: **no genuine low-hanging fruit.** ~95% of `projectMM` literals there are the **binary name** (`build/…/projectMM`, `.bin`, `.exe`, `.log`, `pkill projectMM`, crash `.ips`) which must track the **CMake target**, not `library.json` (wiring them to the product name would break the path to the file on disk); plus one **wire literal** (`_net_probe.py` ArtNet source-name, must byte-match the device) and ~15 prose/docstrings. The only product-name candidates — `generate_manifest.py`'s manifest `name`/`home_assistant_domain` — must *stay* `projectMM` today (Step 2), don't currently read `library.json`, and flip alongside `library.json` in the sweep anyway, so wiring them is new plumbing for zero present benefit. The principle (reuse an existing source of truth over a hardcoded literal) is right; it just has no payoff here because the literals are either binary-coupled or static-until-the-switch. (The real home for product-identity reuse is still the library API — see the box above.) | |
| > **The constant has a real future home: projectMM/MoonLight as a library.** When the project is offered as an embeddable library, a consumer will want one runtime identity to read (an "About"/banner string, the protocol source-name they can query) — *that* is the ongoing, widely-referenced use a `kProjectName` constant genuinely earns (the test the rename failed). But build it **then**, against a real library API surface (it may want to be a small `ProjectInfo` — name + version + url — not a bare string), per *Concrete first, abstract later* — not speculatively now. Tracked as a seed in [backlog-core](backlog-core.md); when the library work starts, introduce the identity constant as part of its public API and let the wire-strings + UI derive from it. | |
| 4. **Author the mechanical sweep script** — ✅ **Done:** [`scripts/rename/rename_to_moonlight.py`](../../scripts/rename/rename_to_moonlight.py), dry-run by default (`--apply` writes; reserved for switch-day Phase 3.3, *after* the repo rename). What the dry-run against today's tree established: replaces two tokens (`ProjectMM` the enum, then `projectMM`) — a plain token swap is correct for *every* form (repo URL, host path, `projectMM.bin`, product name, `deviceName` slug) since `projectMM` is never a substring of another token; file list comes from `git ls-files` so build output (`build/`, `esp32/build/`) is excluded without a brittle blocklist; `docs/history` (era record) + the rename doc itself are content-excluded. Verified: **542 hits across 113 files**, and `MoonLive` / predecessor `MoonLight` / `namespace mm` are provably never touched (0 files where their count changes). The enum rename is safe — device classification keys on the `"modules"` marker, not the label string. The script de-risks switch-day; it is NOT run with `--apply` until then. | |
| 5. **Prep MoonDeck / `moondeck.json` / bench registry** — ✅ **investigated; nothing to change now, two things flagged for switch-day.** (a) **The functional chain stays `projectMM` until the switch (and flips together in the sweep):** `moondeck_config.json`'s `process_name: "projectMM"` ↔ the CMake binary `projectMM` ↔ the `build/<host>/projectMM` run/log path ↔ `pkill projectMM`. These are tracked files the sweep rewrites in one pass, so they stay consistent — changing `process_name` early would break MoonDeck's process detection against today's binary, so don't. (b) **The sweep cannot reach the gitignored bench registry** `scripts/moondeck.json` (it's private, per [[bench-setup]]; the sweep uses `git ls-files`). Its `"board": "projectMM testbench …"` values reference catalog `name`s that *do* flip — so after the switch they'd mismatch only on your bench. **Switch-day local-tooling note: hand-update `scripts/moondeck.json` board names** (and re-provision bench devices if you want the new mDNS identity) — the sweep covers tracked files only. The MoonDeck prose (`MoonDeck.md`, code comments) flips in the normal sweep. | |
| 4. **Author the mechanical sweep script** — ✅ **Done:** [`moondeck/rename/rename_to_moonlight.py`](../../moondeck/rename/rename_to_moonlight.py), dry-run by default (`--apply` writes; reserved for switch-day Phase 3.3, *after* the repo rename). What the dry-run against today's tree established: replaces two tokens (`ProjectMM` the enum, then `projectMM`) — a plain token swap is correct for *every* form (repo URL, host path, `projectMM.bin`, product name, `deviceName` slug) since `projectMM` is never a substring of another token; file list comes from `git ls-files` so build output (`build/`, `esp32/build/`) is excluded without a brittle blocklist; `docs/history` (era record) + the rename doc itself are content-excluded. Verified: **542 hits across 113 files**, and `MoonLive` / predecessor `MoonLight` / `namespace mm` are provably never touched (0 files where their count changes). The enum rename is safe — device classification keys on the `"modules"` marker, not the label string. The script de-risks switch-day; it is NOT run with `--apply` until then. | |
| 5. **Prep MoonDeck / `moondeck.json` / bench registry** — ✅ **investigated; nothing to change now, two things flagged for switch-day.** (a) **The functional chain stays `projectMM` until the switch (and flips together in the sweep):** `moondeck_config.json`'s `process_name: "projectMM"` ↔ the CMake binary `projectMM` ↔ the `build/<host>/projectMM` run/log path ↔ `pkill projectMM`. These are tracked files the sweep rewrites in one pass, so they stay consistent — changing `process_name` early would break MoonDeck's process detection against today's binary, so don't. (b) **The sweep cannot reach the gitignored bench registry** `moondeck/moondeck.json` (it's private, per [[bench-setup]]; the sweep uses `git ls-files`). Its `"board": "projectMM testbench …"` values reference catalog `name`s that *do* flip — so after the switch they'd mismatch only on your bench. **Switch-day local-tooling note: hand-update `moondeck/moondeck.json` board names** (and re-provision bench devices if you want the new mDNS identity) — the sweep covers tracked files only. The MoonDeck prose (`MoonDeck.md`, code comments) flips in the normal sweep. | |
| > **Could we reuse `library.json`'s `name` now where a literal sits (subtraction, not a new constant)?** Surveyed `moondeck/` for it — verdict: **no genuine low-hanging fruit.** ~95% of `projectMM` literals there are the **binary name** (`build/…/projectMM`, `.bin`, `.exe`, `.log`, `pkill projectMM`, crash `.ips`) which must track the **CMake target**, not `library.json` (wiring them to the product name would break the path to the file on disk); plus one **wire literal** (`_net_probe.py` ArtNet source-name, must byte-match the device) and ~15 prose/docstrings. The only product-name candidates — `generate_manifest.py`'s manifest `name`/`home_assistant_domain` — must *stay* `projectMM` today (Step 2), don't currently read `library.json`, and flip alongside `library.json` in the sweep anyway, so wiring them to the product name is new plumbing for zero present benefit. The principle (reuse an existing source of truth over a hardcoded literal) is right; it just has no payoff here because the literals are either binary-coupled or static-until-the-switch. (The real home for product-identity reuse is still the library API — see the box above.) | |
| > **The constant has a real future home: projectMM/MoonLight as a library.** When the project is offered as an embeddable library, a consumer will want one runtime identity to read (an "About"/banner string, the protocol source-name they can query) — *that* is the ongoing, widely-referenced use a `kProjectName` constant genuinely earns (the test the rename failed). But build it **then**, against a real library API surface (it may want to be a small `ProjectInfo` — name + version + url — not a bare string), per *Concrete first, abstract later* — not speculatively now. Tracked as a seed in [backlog-core](backlog-core.md); when the library work starts, introduce the identity constant as part of its public API and let the wire-strings + UI derive from it. | |
| 4. **Author the mechanical sweep script** — ✅ **Done:** [`moondeck/rename/rename_to_moonlight.py`](../../moondeck/rename/rename_to_moonlight.py), dry-run by default (`--apply` writes; reserved for switch-day Phase 3.3, *after* the repo rename). What the dry-run against today's tree established: replaces two tokens (`ProjectMM` the enum, then `projectMM`) — a plain token swap is correct for *every* form (repo URL, host path, `projectMM.bin`, product name, `deviceName` slug) since `projectMM` is never a substring of another token; file list comes from `git ls-files` so build output (`build/`, `esp32/build/`) is excluded without a brittle blocklist; `docs/history` (era record) + the rename doc itself are content-excluded. Verified: **542 hits across 113 files**, and `MoonLive` / predecessor `MoonLight` / `namespace mm` are provably never touched (0 files where their count changes). The enum rename is safe — device classification keys on the `"modules"` marker, not the label string. The script de-risks switch-day; it is NOT run with `--apply` until then. |
🧰 Tools
🪛 LanguageTool
[style] ~65-~65: “low-hanging fruit” can be a clichéd phrase in professional communication. Consider an alternative expression to make your writing more engaging and original.
Context: ...ondeck/for it — verdict: **no genuine low-hanging fruit.** ~95% ofprojectMM` literals there a...
(LOW_HANGING_FRUIT)
🪛 markdownlint-cli2 (0.22.1)
[warning] 66-66: Blank line inside blockquote
(MD028, no-blanks-blockquote)
🤖 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/backlog/rename-to-moonlight.md` around lines 65 - 69, The blockquote in
this rename doc contains an empty line, which triggers markdownlint MD028.
Remove the blank line inside the quoted section in the affected block so the
Markdown stays a single contiguous blockquote while preserving the existing text
and structure.
Source: Linters/SAST tools
…from UI Firmware can now be flashed by uploading a .bin straight to the device over the network (no URL host needed), and the File Manager gains an upload button plus a compact icon-only toolbar with double-click-to-open. FilesystemModule and HttpServerModule no longer render as empty UI cards. All hardware-verified on an ESP32-S3. KPI: PC tick:150/116/148/19/2/348/74/22/26/208/155/22/1/44us | ESP32 tick:900us(FPS:1111) | heap:8378KB Core - MoonModule: added appearsInUi() virtual (default true) so a pure engine module can opt out of the UI card list; the state serializer skips any module where it's false. - FilesystemModule, HttpServerModule: return appearsInUi()=false — they're infrastructure, not user cards. - FileManagerModule: dropped the stored path_/statusBuf_/lastSaved_ buffers; the lastSaved readout now binds zero-copy to the FilesystemModule engine's live buffer, and mkdir/delete moved to HTTP endpoints so the module holds no op state. - HttpServerModule: added POST /api/firmware/upload (handleFirmwareUpload) — streams the request body into the OTA partition via the same UploadSource/uploadPull the file-upload path uses; 409 concurrency guard + truncated-upload abort; on success sends a clean 200, then reboots into the flashed image. Extracted the shared filesystem-path guard to a public static parseFilePath (was a file-local static) so it's unit-testable. - HttpServerModule: added POST/DELETE /api/dir?path= (mkdir/remove), the path riding the query so no persisted control holds it. Platform - otaWriteStream (esp32): commits the image + flips the boot pointer and returns, letting the caller answer before rebooting (esp_ota_begin/write/end/set_boot_partition). Desktop stub returns false (unsupported). - Reverted the earlier URL-OTA cert-bundle split back to the unconditional esp_crt_bundle_attach — that already satisfies esp_https_ota_begin's verification check for plain-http LAN OTA; the split was self-inflicted and unnecessary. Light domain - MultiplyModifier: multiplyZ now defaults to 2 (was 1). - RubiksCubeEffect: added a usePalette control — colour the six faces from the system-wide palette instead of the classic Rubik's colours. UI - app.js: File Manager files open on double-click (single-click selects) — the VS Code / Finder / Explorer norm; icon-only toolbar (folder/file/upload/delete/refresh) with labels in the tooltip; an upload button reusing fmDropUpload; Firmware card gains an icon-only install-from-file (.bin) button. - style.css: .fm-tool--icon square icon buttons + .fw-upload-row. Tests - unit_MoonModule: appearsInUi defaults true, overridable false, dispatches through the base pointer. - unit_FileManagerModule: added direct tests for HttpServerModule::parseFilePath (accept/root/%XX+'+' decode; reject raw + percent-encoded '..', empty, missing, overlong). Scripts / MoonDeck - moondeck.py: OTA-serve handler + local firmware .bin server; PTY-based live SSE streaming; port-to-deviceModel resolution. - moondeck_ui: per-device OTA button; port-device hint; prominent device name. - Lint: f-string-without-placeholder fixes across several scripts; one open()-in-context-manager fix in preview_health.py. Docs - ui.md: File Manager doc synced to the /api/dir HTTP create/delete + lastSaved readout. - effects.md: RubiksCube usePalette control documented. - coding-standards + backlog + plans: stale scripts/ path references and a markdown code-span fix. Reviews - 👾 OTA success path sent no HTTP response (dead JS success branch): fixed — device answers 200 then reboots; hardware-verified the 200 arrives. - 👾 parseFilePath traversal guard was untested: fixed — extracted to a public static + added accept/reject unit tests. - 👾 wrong comment (serializer "skips this"), duplicated cert comment, three absence-narration comments: fixed — all rewritten present-tense / de-duplicated. - 👾 g_otaBytesRead not reset at upload start (stale progress after a prior OTA): fixed. - 👾 usePalette missing from the effects catalog: fixed. - 👾 DELETE /api/dir deletes files + returns 500 on non-empty dir (REST-shape nits): accepted — non-blocking, changing the REST shape is out of scope for this change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/ui/app.js (1)
543-543: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
hasVisibleControlsdoesn't use the newcontrolRendersGenericallyfilter.Line 543 still computes visibility via
mod.controls.some(c => !c.hidden), while the actual row loop andsyncVisibleControlsnow filter throughcontrolRendersGenerically(mod, ctrl). ForFileManagerModulethis is currently harmless (it doesn't accept children, sowrapInDetailsis false regardless), but the two predicates can now diverge: a future child-accepting module with module-specific exclusions (like FileManagerModule'sfilesystem/lastSaved) could computehasVisibleControls = truewhile every actually-rendered row is filtered out, producing an empty "controls" disclosure.♻️ Proposed fix
- const hasVisibleControls = mod.controls && mod.controls.some(c => !c.hidden); + const hasVisibleControls = mod.controls && mod.controls.some(c => controlRendersGenerically(mod, c));Also applies to: 940-946
🤖 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/app.js` at line 543, The hasVisibleControls check in app.js is still using mod.controls.some(c => !c.hidden), which can disagree with the new controlRendersGenerically(mod, ctrl) filtering used by the row rendering and syncVisibleControls. Update the hasVisibleControls computation to use the same predicate as the control loop so the disclosure only opens when at least one actually renderable control exists. Keep the change aligned with the logic in the app.js control rendering path and syncVisibleControls.src/core/HttpServerModule.cpp (1)
104-129: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate
Content-Lengthbefore trusting it downstream.
contentLenis parsed withstd::atoi(line 108) with no bounds check, then cast tosize_tat lines 208/252 to drive two independent streaming paths. A malformed/negative header underflows to a hugesize_t:
/api/file(handleWriteFile) is saved by its ownkUploadMaxcheck (line 556‑559), which correctly rejects the huge value with 413./api/firmware/upload(handleFirmwareUpload) has no analogous ceiling — a hugeremainingproceeds intoUploadSource, and the connection can be held open for the fullkUploadHardMs(60s) before the pull loop's hard deadline aborts it, turning a single malformed header into a much longer, attacker-triggerable stall of the connection handler than any legitimate use needs.Separately,
std::atoioverflow and theheaderSize + contentLensigned addition are both undefined behavior per the C/C++ standard when the header claims a value outsideintrange.🛡️ Proposed fix
auto* clh = std::strstr(req, "Content-Length:"); if (clh) { - contentLen = std::atoi(clh + 15); + char* endp = nullptr; + long parsed = std::strtol(clh + 15, &endp, 10); + if (endp == clh + 15 || parsed < 0 || parsed > static_cast<long>(std::numeric_limits<int>::max() / 2)) { + sendResponse(conn, 400, "application/json", "{\"error\":\"bad content length\"}"); + return; + } + contentLen = static_cast<int>(parsed); int headerSize = static_cast<int>(headerEnd + 4 - req); int bodyNeeded = headerSize + contentLen;🤖 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/HttpServerModule.cpp` around lines 104 - 129, Validate and bound the parsed Content-Length before it is used in HttpServerModule’s request handling, especially in the streaming/upload paths. Replace the unsafe std::atoi-based parse in the header processing logic with a checked conversion in the request parser, reject negative/overflowing/non-numeric values early, and ensure the computed body size cannot underflow or exceed a reasonable upload limit before it reaches handleWriteFile or handleFirmwareUpload. Add a ceiling similar to the existing file-upload guard so UploadSource never receives a huge remaining value from a malformed header.
♻️ Duplicate comments (1)
test/unit/core/unit_FileManagerModule.cpp (1)
44-44: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRestore the shared filesystem root instead of pinning it to
..
fsSetRoot(".")still leaves later tests (in the same binary) pointed at the CWD instead of the platform default ("build").fsSetRoot("")restores the documented default perfsSetRoot's own contract. This was flagged in a previous review round and is still present.🛠️ Proposed fix
- ~Rig() { platform::fsSetRoot("."); std::filesystem::remove_all(root); } + ~Rig() { platform::fsSetRoot(""); std::filesystem::remove_all(root); }🤖 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 `@test/unit/core/unit_FileManagerModule.cpp` at line 44, The Rig teardown is restoring the shared filesystem root to "." instead of the platform default, which leaves later tests pointed at the CWD. Update the Rig destructor to call platform::fsSetRoot with the documented default restore value so the FileManagerModule test fixture returns the shared root to its original state; use Rig and fsSetRoot to locate the change.
🤖 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 `@moondeck/moondeck_ui/index.html`:
- Around line 104-108: The iframe#view-frame in index.html is missing an
accessible name, so add a descriptive title or aria-label to the view-frame
iframe to make it identifiable to assistive technologies. Keep the existing
sandbox settings and allow-modals rationale unchanged; just update the iframe
element so screen readers can announce its purpose when navigating frames.
In `@moondeck/moondeck.py`:
- Around line 1034-1069: The /api/ota handler in MoonDeckWebServer accepts
unsanitized ip and firmware values, which lets callers trigger server-side POSTs
to arbitrary hosts and pass path-like firmware values into the build path/URL.
Add input validation in this handler before constructing bin_path, bin_url, or
the urllib.request.Request: reject non-host/IP inputs for ip with a strict
format or allowlist, and apply the same firmware guard used by
_serve_firmware_bin to block "/" and "..". Keep the existing OTA flow, but only
proceed when both values are validated and safe.
In `@src/core/FileManagerModule.h`:
- Around line 31-34: The doc comment references the wrong path-guard method
name; update the mention of HttpServerModule::fileQueryPath to match the actual
declaration, parseFilePath, so the comment aligns with the real symbol used in
HttpServerModule and does not mislead readers. Keep the rest of the robustness
wording intact while renaming the referenced method consistently wherever it
appears in this comment block.
In `@src/platform/platform.h`:
- Around line 230-241: The header comment for otaWriteStream is incorrect
because it says the function reboots on success, but the ESP32 implementation
only marks the status as rebooting and returns true; update the documentation to
say the reboot is performed by the caller after a successful otaWriteStream, and
keep the behavior description aligned with the actual flow in otaWriteStream and
HttpServerModule::handleFirmwareUpload.
---
Outside diff comments:
In `@src/core/HttpServerModule.cpp`:
- Around line 104-129: Validate and bound the parsed Content-Length before it is
used in HttpServerModule’s request handling, especially in the streaming/upload
paths. Replace the unsafe std::atoi-based parse in the header processing logic
with a checked conversion in the request parser, reject
negative/overflowing/non-numeric values early, and ensure the computed body size
cannot underflow or exceed a reasonable upload limit before it reaches
handleWriteFile or handleFirmwareUpload. Add a ceiling similar to the existing
file-upload guard so UploadSource never receives a huge remaining value from a
malformed header.
In `@src/ui/app.js`:
- Line 543: The hasVisibleControls check in app.js is still using
mod.controls.some(c => !c.hidden), which can disagree with the new
controlRendersGenerically(mod, ctrl) filtering used by the row rendering and
syncVisibleControls. Update the hasVisibleControls computation to use the same
predicate as the control loop so the disclosure only opens when at least one
actually renderable control exists. Keep the change aligned with the logic in
the app.js control rendering path and syncVisibleControls.
---
Duplicate comments:
In `@test/unit/core/unit_FileManagerModule.cpp`:
- Line 44: The Rig teardown is restoring the shared filesystem root to "."
instead of the platform default, which leaves later tests pointed at the CWD.
Update the Rig destructor to call platform::fsSetRoot with the documented
default restore value so the FileManagerModule test fixture returns the shared
root to its original state; use Rig and fsSetRoot to locate the change.
🪄 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: 7fade00c-5720-409c-ba2e-4332cd094c49
⛔ Files ignored due to path filters (3)
moondeck/build/improv_probe.pyis excluded by!**/build/**moondeck/build/improv_smoke_test.pyis excluded by!**/build/**moondeck/build/setup_esp_idf.pyis excluded by!**/build/**
📒 Files selected for processing (35)
docs/backlog/docs-system-overhaul.mddocs/backlog/leddriver-analysis-top-down.mddocs/backlog/rename-scripts-to-moondeck.mddocs/backlog/rename-to-moonlight.mddocs/coding-standards.mddocs/history/plans/Plan-20260624 - Semver version + update-available badge.mddocs/history/plans/Plan-20260702 - Docs site Phase 0 (MkDocs Material).mddocs/history/plans/Plan-20260705 - Rename scripts to moondeck.mddocs/moonmodules/core/ui/ui.mddocs/moonmodules/light/effects/effects.mdmoondeck/diag/preview_health.pymoondeck/docs/build_docs.pymoondeck/docs/generate_test_docs.pymoondeck/docs/screenshot_modules.pymoondeck/moondeck.pymoondeck/moondeck_ui/app.jsmoondeck/moondeck_ui/index.htmlmoondeck/run/preview_installer.pymoondeck/scenario/run_live_scenario.pysrc/core/FileManagerModule.cppsrc/core/FileManagerModule.hsrc/core/FilesystemModule.cppsrc/core/FilesystemModule.hsrc/core/HttpServerModule.cppsrc/core/HttpServerModule.hsrc/core/MoonModule.hsrc/light/effects/RubiksCubeEffect.hsrc/light/modifiers/MultiplyModifier.hsrc/platform/desktop/platform_desktop.cppsrc/platform/esp32/platform_esp32_ota.cppsrc/platform/platform.hsrc/ui/app.jssrc/ui/style.csstest/unit/core/unit_FileManagerModule.cpptest/unit/core/unit_MoonModule.cpp
A device can now be controlled from Homebridge (and other MQTT hubs) over a hand-rolled MQTT 3.1.1 client, driving a new master on/off plus brightness and a HomeKit colour-wheel-to-palette map. Ethernet becomes opt-in per board so a WiFi-only board no longer tries to bring up an Ethernet PHY it lacks. A new "Shelly" board (LOLIN D32 + 24-LED ring) ships in the installer catalog, verified end-to-end on hardware. KPI: PC tick:121/92/118/…us | ESP32 tick:1547us(FPS:646) | heap:153KB Core - MqttModule (new): a Network sub-service that bridges the light controls to an MQTT broker. The topic identity is a STABLE `projectMM/<last6-of-MAC>` (rename-proof, the WLED/Tasmota/HA convention); the friendly deviceName rides a separate retained `<prefix>/name` topic. Connect/keepalive/reconnect run entirely non-blocking on loop1s via the platform's connectStart/connectPoll — no stall in the render loop. Live reconfig on a broker/credential change (onUpdate), clean DISCONNECT on disable (onEnabled), bounded connect + CONNACK timeouts, resets on any protocol violation. - MqttPacket.h (new): MQTT 3.1.1 wire format (varint + build/parse + a byte-at-a-time inbound parser), dependency-free and golden-vector tested like ImprovFrame.h. No library. - MoonModule: added readBool/readUint8 — one generic control-reader so HttpServerModule and MqttModule read Drivers' on/brightness/palette with a single, consistent absent-control default. - HttpServerModule: the WLED shim now drives the real Drivers `on` control (deleted the bri=0 on/off fudge); its driversOn/driversBrightness use the shared reader. - IrModule: added a learnable on/off Toggle action (a new ActionKind) driving Drivers.on. - NetworkModule: `ethType` defaults to 0/None — Ethernet is opt-in per board, set explicitly in the catalog, so a WiFi-only board wastes no PHY-init. Pins still seed from the per-chip default for a board that opts in. Platform - TcpConnection::connect (blocking, for httpRequest) plus connectStart/connectPoll (non-blocking outbound connect with getaddrinfo DNS, desktop + esp32/lwip) — the first outbound-TCP client + DNS in the platform layer. Light domain - Drivers: new `on` bool (master power) that gates the correction LUT to zero without touching the brightness value; every consumer (UI, IR, WLED app, MQTT) drives this one control. - Palette: nearestForHue/representativeHueSat — a HomeKit colour wheel's hue+saturation pick the nearest palette, computed from each palette's average RGB (no hand table); folds any out-of-range hue. UI / Docs - ui.md: MQTT card + the MAC-stable topics + retained name topic + a paste-in Homebridge mqttthing config. Catalog - Shelly (new): LOLIN D32, esp32 firmware, 24-LED grid (1×24), RMT pin 2, Hue driver, MQTT, flashBaud 460800, TX-power cap 8. - Ethernet made explicit per board: S31 (ethType 4 + RGMII pins), testbench-P4 (ethType 2 + NANO pins) now declare their wiring; QuinLED Dig-Octa gets ethRstGpio -1 (GPIO5 is an LED on that board, not an eth reset). check_devices.py enforces "ethType set → board-wiring pins present" and rejects a non-int ethType. Scripts / MoonDeck - moondeck.py: /api/ota validates ip (host/IP form) + firmware (no "/" / "..") before use. - moondeck_ui/index.html: iframe accessible title. Tests - unit_MqttPacket, unit_MqttModule, unit_TcpConnect, unit_Drivers_container (on/off), unit_IrModule (toggle), unit_Palette (nearestForHue), unit_NetworkModule_ethernet (opt-in), unit_MoonModule (readBool/readUint8), unit_HttpServerModule_apply (real-on WLED behaviour): new/updated to pin each behaviour, including the reviewer-found bugs (hue overflow, password-without-username, retain flag, MAC-stable-across-rename, non-blocking connect). Reviews - 👾 Personal LAN IPs shipped in the Shelly catalog entry: fixed — bench Hue/broker IPs stripped; the user sets them on their own network. - 👾 Blocking connect + unbounded blocking write inside Scheduler::tick (an unreachable/zero-window broker froze the render loop): fixed — non-blocking connectStart/connectPoll + an all-or-reset sendPacket over writeSome. - 👾 Silent-broker wedge (Connecting never timed out): fixed — a connect + CONNACK timeout bounds both wait states. - 👾 Recurring synchronous DNS every 5s on a bad hostname: fixed — a 30s backoff after a failed attempt keeps the per-attempt resolve rare (accepted residual: the DNS call itself is synchronous, per the plan; async DNS is a future option). - 👾 int32 overflow on hue ≥ 360: fixed at the source (nearestForHue folds any hue). - 👾 A broker/credential change never re-homed, and an unrelated buildState dropped MQTT: fixed — onUpdate scoped to the module's own controls. - 👾 Dead disable path / dangling socket: fixed — onEnabled sends a clean DISCONNECT. - 👾 Ignored Malformed inbound (stream desync) + a short CONNACK treated as accepted: fixed — reset on both. - 👾 driversOn/driversBrightness duplicated with contradictory absent-defaults: fixed — one shared MoonModule reader. - 👾 core→light include against the plan's boundary line: kept as a stated PO-accepted trade-off (a pure hue→palette conversion), not precedent-defence. - 👾 Windows connectPoll missed a refused connect (writefds only): fixed — also selects the exception set. - 👾 Unused parseMqttConnack/parseMqttSuback: trimmed (the streaming parser is what the module uses). - 🐇 (prior PR) Content-Length bounded parse, /api/ota input validation, iframe title, hasVisibleControls predicate, parseFilePath doc, otaWriteStream comment: fixed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/light/drivers/Drivers.h (1)
67-67: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
teardown()doesn't clearconfigWarn_, breaking the documented symmetry withconfigErr_.
clearConfigErr()is called fromteardown()(Line 67), but there's no equivalentconfigWarn_clear, even though the comment at Line 147 states this follows the "Same 'clear only MY status' rule as configErr_." A driver torn down while showing an active warning leaves that warning displayed indefinitely instead of being retracted like the error/fail-buffer counterparts.🐛 Proposed fix
- void teardown() override { clearFailBuf(); clearConfigErr(); } + void teardown() override { clearFailBuf(); clearConfigErr(); setConfigWarn(nullptr); }Also applies to: 143-158
🤖 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/light/drivers/Drivers.h` at line 67, teardown() currently clears fail and error state but leaves configWarn_ behind, which breaks the same “clear only MY status” behavior used by configErr_ and the warning/status helpers in Drivers.h. Update teardown() and the related warning-clear path in the driver status handling methods (such as the config warning logic around configWarn_ / clearConfigErr()) so teardown also retracts any active warning, keeping the status cleanup symmetric with the existing fail-buffer and config error cleanup.src/ui/app.js (1)
2792-2798: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winWhitespace-only names bypass the client-side guard.
if (!name) return;doesn't catch a whitespace-only prompt input;name.trim()is only applied afterward. Check the trimmed value instead.🐛 Proposed fix
- const name = prompt("New folder name in " + base + ":"); - if (!name) return; + const name = prompt("New folder name in " + base + ":"); + if (!name || !name.trim()) return; st.expanded.add(base); // reveal the new child await runOp("new folder", joinFsPath(base, name.trim()));(apply the same check to the
newFileBtnhandler)Also applies to: 2807-2824
🤖 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/app.js` around lines 2792 - 2798, The new folder and new file handlers in the app still allow whitespace-only prompt input because they only check the raw prompt value before trimming. In the `newBtn` click handler and the corresponding `newFileBtn` handler, trim the prompt result first and return early if the trimmed name is empty, then use that trimmed value when calling `runOp` and `joinFsPath` so the client-side guard blocks blank names consistently.docs/backlog/rename-scripts-to-moondeck.md (1)
9-40: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winThis backlog note looks stale and self-corrupted — the rename appears to have already shipped.
The actual repository already has
moondeck/…paths throughout (e.g.moondeck/moondeck.py,moondeck/scenario/run_live_scenario.py,moondeck/check/check_devices.pyin this same review stack), yet this note still frames the rename as "to be done… in the next cycle" and describes CLAUDE.md/CI/CMake as already usingmoondeck/…(should bescripts/…if truly pre-rename). Line 24's own worked regex,s{moondeck/}{moondeck/}, is a no-op — ironically the exact "naive replace" bug this note warns about, applied to itself.Per this doc's own Decision section ("Delete this backlog note once the rename ships") and the backlog guideline, this file should be deleted rather than kept in this contradictory state.
As per coding guidelines,
docs/backlog/**/*.md: "Treat backlog items as temporary working narrative: delete them once shipped, and allow backlog content to shrink as well as grow."🤖 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/backlog/rename-scripts-to-moondeck.md` around lines 9 - 40, This backlog note is stale and internally inconsistent because the rename it describes has already landed, so the file should be removed. Delete the backlog entry itself rather than editing the wording, and make sure no remaining references in the note are preserved; the unique target here is the docs/backlog rename-scripts-to-moondeck item and its decision to delete once shipped.Source: Coding guidelines
src/core/FileManagerModule.cpp (1)
38-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win"Persisted bool" doc claim vs. force-reset-every-boot behavior.
FileManagerModule.h's class doc describesshow hiddenas "a persisted bool the tree reads", butsetup()here unconditionally resets it tofalseon every boot regardless of the value FilesystemModule just loaded from flash — so any savedtruevalue is silently discarded at boot. The behavior is well-reasoned and documented in this file's own comment ("transient view preference... re-toggles per session"), but the header's "persisted bool" phrasing is now misleading, and the control still round-trips through the flash-write debounce for no functional benefit (extra write cycles on every toggle since the value never survives a reboot anyway).Worth reconciling: either update the header docstring's wording, or reconsider not persisting this flag at all (e.g. keep it purely client-side in the UI/local-storage layer via the
hidden=query param) if boot behavior is meant to always start fresh.🤖 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/FileManagerModule.cpp` around lines 38 - 44, The mismatch is between the `FileManagerModule` class doc in `FileManagerModule.h` and the boot-time reset behavior in `FileManagerModule::setup()`: the doc says `show hidden` is a persisted bool, but `setup()` always forces `showHidden_` to false after `MoonModule::setup()` loads persisted state. Either update the class documentation to describe it as a transient per-session setting, or change the design so `showHidden_` is not persisted at all and is handled only in the UI/client flow (for example via the `hidden=` query param); keep the wording and behavior consistent.moondeck/moondeck_ui/app.js (1)
1014-1024: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMisleading inline message: "pick a device first" should reference the device model.
The guard fires when
deviceModelPicker.valueis empty, but the button acts on the device-model picker (not device selection, which already happened via the row itself) — the message will confuse users into thinking they need to select/check a device.✏️ Proposed fix
- if (!deviceModel) { injectBtn.textContent = "pick a device first"; setTimeout(() => injectBtn.textContent = "inject", 1500); return; } + if (!deviceModel) { injectBtn.textContent = "pick a device model first"; setTimeout(() => injectBtn.textContent = "inject", 1500); return; }🤖 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 `@moondeck/moondeck_ui/app.js` around lines 1014 - 1024, The inline guard message in the `injectBtn` click handler is misleading because it tells users to “pick a device first” even though the action depends on `deviceModelPicker.value`. Update the empty-state text in this handler to reference selecting a device model instead, and keep the existing reset/timeout behavior unchanged so the user-facing feedback matches the `deviceModelPicker` flow.src/main.cpp (1)
418-430: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOrdering-rationale comment wasn't updated for the new modules.
This comment explains the
scheduler.addModuleordering (filesystem → system → firmwareUpdate → network → light pipeline → HTTP) but doesn't mention wherefileManagerModule(inserted betweensystemModuleandfirmwareUpdateModule) or the newmqttModulechild-attach fit into that rationale.🤖 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/main.cpp` around lines 418 - 430, Update the ordering-rationale comment near scheduler.addModule and networkModule->addChild to reflect the new module placement: mention fileManagerModule in the top-level sequence between systemModule and firmwareUpdateModule, and explain that mqttModule is attached as a child of networkModule alongside improvModule. Keep the rationale aligned with the current setup in main.cpp so the documented order matches the actual module registration and child wiring.
♻️ Duplicate comments (2)
src/ui/app.js (1)
2954-2959: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
openFileEditoris called without the file's size — truncation guard from prior review cannot work.
entry.sizeis already known here (used at line 2922 for the size column) but isn't forwarded toopenFileEditor(childPath). This is the root cause behind the still-open prior review finding on the truncation guard: even ifopenFileEditorinternally compares fetched-content length against asizeparameter, this call site never supplies one, so a file exceeding the server's read cap would silently load truncated and Save would overwrite it with the truncated content — a real data-loss path.🛡️ Proposed fix
rowEl.addEventListener("dblclick", (ev) => { ev.stopPropagation(); - openFileEditor(childPath); + openFileEditor(childPath, entry.size || 0); });🤖 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/app.js` around lines 2954 - 2959, The dblclick handler in the file row is calling openFileEditor without the known file size, so the truncation guard cannot run. Update the rowEl listener to pass entry.size along with childPath, using the existing openFileEditor call site and the same entry data already used for the size column. Make sure openFileEditor receives the size parameter consistently so it can detect truncated reads before editing/saving.test/unit/core/unit_FileManagerModule.cpp (1)
44-44: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
~Rig()still resets the shared fs root to.instead of restoring the default.
fsSetRoot(".")points the shared filesystem root at the repo root for any test that runs afterward in the same binary;fsSetRoot("")falls back to the default"build"root perfsSetRoot's own implementation. This matches a previously-raised comment that doesn't appear to have been addressed in this commit.🛠️ Proposed fix
- ~Rig() { platform::fsSetRoot("."); std::filesystem::remove_all(root); } + ~Rig() { platform::fsSetRoot(""); std::filesystem::remove_all(root); }🤖 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 `@test/unit/core/unit_FileManagerModule.cpp` at line 44, The Rig destructor still resets the shared filesystem root to "." instead of restoring the default build root. Update Rig::~Rig() to call platform::fsSetRoot("") so it falls back to the default behavior defined by fsSetRoot, and keep the cleanup of root unchanged. Locate the fix in the Rig struct’s destructor in the FileManagerModule unit test.
🤖 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/ui/ui.md`:
- Line 52: The UI MQTT description uses future-tense roadmap wording (“and later
Home Assistant”), which conflicts with the present-tense documentation
guideline. Update the affected sentence in the UI doc to describe only current
behavior in present tense, and remove any implied future support wording while
keeping the existing symbols/terms like MQTT broker, Homebridge,
Scheduler::setControl, and MqttPacket.h intact.
- Around line 173-176: The `learn` action list in `ui.md` still includes a
standalone `off` entry that no longer matches the rest of the documented
actions. Update the `learn` description to use the consolidated `on/off` action
only, and keep it consistent with the existing `code on/off` read-only entry and
the `Drivers.on` behavior described below.
In `@moondeck/check/check_devices.py`:
- Around line 212-214: The ethType validation in check_devices.py is too
permissive because isinstance(et, int) accepts bool values, so a JSON true can
slip through and be treated as a valid LAN8720 value. Update the ethType guard
in the same validation block that uses errors.append and the {1: ...}.get(et,
...) mapping to use an exact-type integer check like the flashBaud validation
does, so only real ints are accepted and booleans are rejected consistently.
In `@moondeck/moondeck_ui/app.js`:
- Around line 1037-1056: The OTA click handler in otaBtn.addEventListener is
using a blocking alert() on failure, which breaks the inline-feedback pattern
used by pushDevice and injectBtn in this file. Update the /api/ota flow to
report errors only through the button text (for example, “failed ✗” plus the
existing status reset) and remove the alert() calls from both the fetch error
path and the non-ok response path, keeping the UX consistent with the other
controls.
In `@src/core/FilesystemModule.cpp`:
- Around line 43-47: Reword the comment in FilesystemModule so it uses present
tense and describes the current design directly instead of framing it as the
absence of an override. Update the nearby explanatory text around
FilesystemModule and FileManagerModule to say that FilesystemModule is a non-UI
persistence engine that renders no card in the module tree, while
FileManagerModule displays the “last saved” status and filesystem-usage gauge.
In `@src/core/MqttPacket.h`:
- Around line 109-116: mqttWriteFixedHeader currently calls
encodeRemainingLength with out + 1 before confirming the remaining-length
encoding fits, which can write past the buffer when bodyLen requires more than
one byte. Update mqttWriteFixedHeader to determine the encoded remaining-length
size first or otherwise validate available space before writing, and only emit
the fixed header when 1 + the encoded length is within outLen. Make sure the fix
covers all callers that use this helper, including buildMqttConnect,
buildMqttPublish, and buildMqttSubscribe.
In `@test/unit/core/unit_TcpConnect.cpp`:
- Around line 18-39: The TcpConnection/TcpServer unit tests are still using real
loopback sockets and polling loops, which violates the test guideline against
timing- and network-dependent behavior. Update the TcpConnection::connect and
TcpServer::open/accept coverage to avoid OS socket usage in test/**, ideally by
moving this behavior behind a mockable transport abstraction and testing the
logic with fakes instead of waiting for real accepts. Keep the test cases
focused on deterministic behavior and remove the bounded sleep/poll loops from
the loopback connect scenarios.
---
Outside diff comments:
In `@docs/backlog/rename-scripts-to-moondeck.md`:
- Around line 9-40: This backlog note is stale and internally inconsistent
because the rename it describes has already landed, so the file should be
removed. Delete the backlog entry itself rather than editing the wording, and
make sure no remaining references in the note are preserved; the unique target
here is the docs/backlog rename-scripts-to-moondeck item and its decision to
delete once shipped.
In `@moondeck/moondeck_ui/app.js`:
- Around line 1014-1024: The inline guard message in the `injectBtn` click
handler is misleading because it tells users to “pick a device first” even
though the action depends on `deviceModelPicker.value`. Update the empty-state
text in this handler to reference selecting a device model instead, and keep the
existing reset/timeout behavior unchanged so the user-facing feedback matches
the `deviceModelPicker` flow.
In `@src/core/FileManagerModule.cpp`:
- Around line 38-44: The mismatch is between the `FileManagerModule` class doc
in `FileManagerModule.h` and the boot-time reset behavior in
`FileManagerModule::setup()`: the doc says `show hidden` is a persisted bool,
but `setup()` always forces `showHidden_` to false after `MoonModule::setup()`
loads persisted state. Either update the class documentation to describe it as a
transient per-session setting, or change the design so `showHidden_` is not
persisted at all and is handled only in the UI/client flow (for example via the
`hidden=` query param); keep the wording and behavior consistent.
In `@src/light/drivers/Drivers.h`:
- Line 67: teardown() currently clears fail and error state but leaves
configWarn_ behind, which breaks the same “clear only MY status” behavior used
by configErr_ and the warning/status helpers in Drivers.h. Update teardown() and
the related warning-clear path in the driver status handling methods (such as
the config warning logic around configWarn_ / clearConfigErr()) so teardown also
retracts any active warning, keeping the status cleanup symmetric with the
existing fail-buffer and config error cleanup.
In `@src/main.cpp`:
- Around line 418-430: Update the ordering-rationale comment near
scheduler.addModule and networkModule->addChild to reflect the new module
placement: mention fileManagerModule in the top-level sequence between
systemModule and firmwareUpdateModule, and explain that mqttModule is attached
as a child of networkModule alongside improvModule. Keep the rationale aligned
with the current setup in main.cpp so the documented order matches the actual
module registration and child wiring.
In `@src/ui/app.js`:
- Around line 2792-2798: The new folder and new file handlers in the app still
allow whitespace-only prompt input because they only check the raw prompt value
before trimming. In the `newBtn` click handler and the corresponding
`newFileBtn` handler, trim the prompt result first and return early if the
trimmed name is empty, then use that trimmed value when calling `runOp` and
`joinFsPath` so the client-side guard blocks blank names consistently.
---
Duplicate comments:
In `@src/ui/app.js`:
- Around line 2954-2959: The dblclick handler in the file row is calling
openFileEditor without the known file size, so the truncation guard cannot run.
Update the rowEl listener to pass entry.size along with childPath, using the
existing openFileEditor call site and the same entry data already used for the
size column. Make sure openFileEditor receives the size parameter consistently
so it can detect truncated reads before editing/saving.
In `@test/unit/core/unit_FileManagerModule.cpp`:
- Line 44: The Rig destructor still resets the shared filesystem root to "."
instead of restoring the default build root. Update Rig::~Rig() to call
platform::fsSetRoot("") so it falls back to the default behavior defined by
fsSetRoot, and keep the cleanup of root unchanged. Locate the fix in the Rig
struct’s destructor in the FileManagerModule unit test.
🪄 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: 6bd0d1af-6764-4cf9-8761-b17f87be7976
⛔ Files ignored due to path filters (4)
docs/assets/boards/shelly.jpgis excluded by!**/*.jpgmoondeck/build/improv_probe.pyis excluded by!**/build/**moondeck/build/improv_smoke_test.pyis excluded by!**/build/**moondeck/build/setup_esp_idf.pyis excluded by!**/build/**
📒 Files selected for processing (59)
CMakeLists.txtdocs/backlog/docs-system-overhaul.mddocs/backlog/leddriver-analysis-top-down.mddocs/backlog/rename-scripts-to-moondeck.mddocs/backlog/rename-to-moonlight.mddocs/coding-standards.mddocs/history/plans/Plan-20260624 - Semver version + update-available badge.mddocs/history/plans/Plan-20260702 - Docs site Phase 0 (MkDocs Material).mddocs/history/plans/Plan-20260705 - Homebridge MQTT control.mddocs/history/plans/Plan-20260705 - Rename scripts to moondeck.mddocs/moonmodules/core/ui/ui.mddocs/moonmodules/light/effects/effects.mdesp32/main/CMakeLists.txtmoondeck/check/check_devices.pymoondeck/diag/preview_health.pymoondeck/docs/build_docs.pymoondeck/docs/generate_test_docs.pymoondeck/docs/screenshot_modules.pymoondeck/moondeck.pymoondeck/moondeck_ui/app.jsmoondeck/moondeck_ui/index.htmlmoondeck/run/preview_installer.pymoondeck/scenario/run_live_scenario.pysrc/core/FileManagerModule.cppsrc/core/FileManagerModule.hsrc/core/FilesystemModule.cppsrc/core/FilesystemModule.hsrc/core/HttpServerModule.cppsrc/core/HttpServerModule.hsrc/core/IrModule.hsrc/core/MoonModule.hsrc/core/MqttModule.cppsrc/core/MqttModule.hsrc/core/MqttPacket.hsrc/core/NetworkModule.hsrc/light/Palette.hsrc/light/drivers/Drivers.hsrc/light/effects/RubiksCubeEffect.hsrc/light/modifiers/MultiplyModifier.hsrc/main.cppsrc/platform/desktop/platform_desktop.cppsrc/platform/esp32/platform_esp32.cppsrc/platform/esp32/platform_esp32_ota.cppsrc/platform/platform.hsrc/ui/app.jssrc/ui/style.csstest/CMakeLists.txttest/scenarios/light/scenario_Audio_mutation.jsontest/unit/core/unit_FileManagerModule.cpptest/unit/core/unit_HttpServerModule_apply.cpptest/unit/core/unit_IrModule.cpptest/unit/core/unit_MoonModule.cpptest/unit/core/unit_MqttModule.cpptest/unit/core/unit_MqttPacket.cpptest/unit/core/unit_NetworkModule_ethernet.cpptest/unit/core/unit_TcpConnect.cpptest/unit/light/unit_Drivers_container.cpptest/unit/light/unit_Palette.cppweb-installer/deviceModels.json
ewowi
changed the title
Processes the whole-branch Fable Reviewer pass and the CodeRabbit PR review. Fixes one real correctness bug (an MQTT fixed-header buffer overrun) plus a data-integrity guard (editor refuses to save a truncated read), and a large round of dedup/subtraction: one canonical module-lookup, shared Drivers-state readers, one OTA-in-flight helper, one app.js error-message helper. Net −166 lines.
KPI: 16384lights | PC:646KB | tick:121/92/119/15/2/271/57/17/20/156/114/16/1/34us(FPS:8264/10869/8403/66666/500000/3690/17543/58823/50000/6410/8771/62500/1000000/29411) | ESP32:1368KB | tick:903us(FPS:1107) | heap:156KB
Core
- MqttPacket.h: mqttWriteFixedHeader validated the remaining-length varint size BEFORE writing the header, into a local scratch — a bodyLen needing a 2–4-byte varint into a 1–2-byte buffer no longer overruns (encodeRemainingLength is unbounded). Pinned by a new regression test.
- HttpServerModule + Scheduler: findModuleByName now delegates to Scheduler::firstByName (one canonical tree-walk; deleted the duplicate findInTree recursion). firstByName gained a null/empty-name guard so the delegating path is UB-safe. The Drivers master-state reads (on/brightness/palette) are now shared free functions mm::driversOn/Brightness/Palette(scheduler) with one absent-control default, replacing the duplicated per-module driversOn/driversBrightness that had drifted.
- FirmwareUpdateModule: one otaInFlight() helper replaces the 4-way strcmp(g_otaStatus,…) chain duplicated across the URL and upload OTA guards.
- FilesystemModule: reworded the engine/tool-split comment to present tense (was absence-narration).
- Platform: deleted the dead blocking TcpConnection::connect(host,port,timeout) (desktop + esp32 impls, declaration, and its 3 loopback test cases) — MqttModule uses the non-blocking connectStart/connectPoll. Corrected the false "off the render hot path" comments on the OTA/upload drain (it runs on loop20ms inside Scheduler::tick).
- main.cpp: every core module's registerType docPath repointed to a real published target — user-facing modules to core/ui/ui.md#anchor, the 6 source-comment rationale cross-refs to core/moxygen/<Name>.md; the stale per-module core/<Name>.md paths (removed by the moxygen migration) produced broken help links. Updated the module-registration ordering comment to include fileManager and the mqtt-as-network-child wiring.
Light domain
- Drivers: teardown() now clears every shared status string (fail buffer, config error, AND config warning) symmetrically, so a stopped driver leaves no stale warning.
UI
- app.js: one errorMessage(res) helper extracts the server's {"error":…} body (else HTTP <status>) — replaced ~8 copies of the idiom. FileManager: filesystem/lastSaved controls carry the generic hidden flag (set C++-side) so controlRendersGenerically drops its bespoke per-module special-case. New file/folder prompts trim before the empty-guard so whitespace-only names are rejected. The editor takes the listing's size and loads read-only on a truncated read (a short server-side read would otherwise be saved back, corrupting the file).
Scripts / MoonDeck
- check_devices.py: ethType uses an exact-type int check (type(et) is int) like flashBaud, so a JSON bool no longer passes as ethType 1.
- moondeck_ui/app.js: OTA failures report through the button text only (removed the alert()s, matching inject/push); the inject empty-state says "pick a device model" (it depends on the model picker).
Docs
- ui.md: dropped the future-tense "and later Home Assistant" (present-tense rule); clarified that the IR learn control's first option `off` is the disarmed state, not a light action. FileManagerModule.h documents `show hidden` as a transient per-session preference (setup() forces it off each boot). Carried the branch's MQTT/Ethernet lessons into decisions.md.
Tests
- unit_MqttPacket: fixed-header-refuses-a-too-small-buffer regression guard. unit_TcpConnect: trimmed to the connectStart/connectPoll surface. unit_FileManagerModule: Rig dtor restores the DEFAULT fs root (fsSetRoot("")) instead of ".".
Reviews
- 👾 Dead blocking connect(), false hot-path comments, bespoke control-hide, stale docPaths (12 registerType + 6 cross-refs), duplicated OTA strcmp chain, duplicated module-lookup + Drivers-reads, corrupt rename backlog, stale scripts/ refs, drift comments, ~8× app.js error idiom: all fixed.
- 🐇 MQTT fixed-header overrun (+ test), ethType bool-accepts-int, moondeck OTA alert→button text, injectBtn message, present-tense comments, transient show-hidden doc, teardown configWarn_, main.cpp ordering comment, whitespace-name guard, editor truncated-read guard, Rig dtor default root, ui.md future-tense + learn-off clarify: all fixed.
- 🐇 Rewrite unit_TcpConnect off real loopback sockets: not done — the test pins the platform socket primitive itself (getaddrinfo + non-blocking connect + select); a mock would test the fake. Loopback is deterministic on desktop, the poll loop is a bounded ceiling not a sleep, and real-loopback is the established pattern (unit_AudioModule_sync, the NetworkReceive/driver tests).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
moondeck/moondeck_ui/app.js (2)
886-903: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse keyboard-accessible controls for device links.
spanelements with click handlers are not focusable or keyboard-activatable, so keyboard users cannot open a device from the name/IP controls. Use buttons/anchors or add equivalent focus and key handling.🤖 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 `@moondeck/moondeck_ui/app.js` around lines 886 - 903, The device name/IP controls in app.js use clickable span elements, which are not keyboard-accessible. Update the nameText and ipLink interaction in the device row rendering to use semantic interactive elements such as buttons or anchors, or add proper tabindex, focus styling, and keyboard handlers for Enter/Space so the existing showInView behavior can be triggered without a mouse.
925-930: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winClear stale firmware/model when selecting a last-flashed port.
If this device has no known firmware or
deviceModel, theseifstatements leave the previous device’s selections instate. The next flash/inject can pair this device’s port with another device’s firmware/model.Proposed fix
- if (device.firmware && firmwares.includes(device.firmware)) state.firmware = device.firmware; - if (device.deviceModel) state.provisionDeviceModel = device.deviceModel; + state.firmware = device.firmware && firmwares.includes(device.firmware) ? device.firmware : ""; + state.provisionDeviceModel = device.deviceModel || "";🤖 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 `@moondeck/moondeck_ui/app.js` around lines 925 - 930, The portChip click handler is leaving stale flash settings in state when the selected device does not have a known firmware or deviceModel. Update the portChip.addEventListener("click", ...) logic to explicitly clear state.firmware and state.provisionDeviceModel (and any related model selection) when device.firmware is missing/not in firmwares or device.deviceModel is absent, while still setting a.port from device.last_port via getActiveNetwork(). Keep the existing per-device assignment path, but make the fallback reset behavior unconditional so the next flash/inject cannot reuse another device’s firmware/model.test/unit/core/unit_FileManagerModule.cpp (2)
32-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a portable temp directory instead of hardcoded
/tmp.Hardcoding
/tmpassumes a POSIX filesystem layout.std::filesystem::temp_directory_path()is the standard, portable way to obtain the OS temp dir.♻️ Optional refactor
- static unsigned counter = 0; - std::snprintf(root, sizeof(root), "/tmp/mm_fm_test_%u", counter++); + static unsigned counter = 0; + const auto tmp = std::filesystem::temp_directory_path().string(); + std::snprintf(root, sizeof(root), "%s/mm_fm_test_%u", tmp.c_str(), counter++);🤖 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 `@test/unit/core/unit_FileManagerModule.cpp` at line 32, The test setup in the FileManager module is hardcoding a POSIX temp path, which breaks portability. Update the temp root initialization in the FileManager test helper to use std::filesystem::temp_directory_path() instead of "/tmp", while keeping the unique suffix logic (counter/root) intact so the generated directory remains isolated per test run.
35-38: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueUnchecked
fopenresults.If either
fopencall fails (e.g. no space, permission denied), the subsequentfputs/fclosedereference a nullFILE*and crash the test binary instead of failing the assertion cleanly.<
🛡️ Optional guard
{ FILE* f = std::fopen((std::string(root) + "/.config/Drivers.json").c_str(), "w"); - std::fputs("{\"brightness\":20}", f); std::fclose(f); } + REQUIRE(f); std::fputs("{\"brightness\":20}", f); std::fclose(f); }🤖 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 `@test/unit/core/unit_FileManagerModule.cpp` around lines 35 - 38, The test setup in unit_FileManagerModule.cpp is using std::fopen without checking the returned FILE* before calling std::fputs and std::fclose, which can crash the test binary if file creation fails. Update the fixture code around the two std::fopen usages to validate the handle first and fail the test cleanly if opening either file does not succeed, so the helpers in this setup block never dereference a null FILE*.src/core/HttpServerModule.cpp (2)
107-121: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject malformed
Content-Lengthvalues, not just negative/oversized ones.
strtol(..., nullptr, 10)acceptsContent-Length: abcas0andContent-Length: 12junkas12, despite the comment saying malformed values are rejected. Use an end pointer and overflow check before trusting the parsed length.Proposed fix
+#include <cerrno> `#include` <cstdlib>- const long parsed = std::strtol(clh + 15, nullptr, 10); - if (parsed < 0 || parsed > kContentLenMax) { + const char* value = clh + 15; + char* end = nullptr; + errno = 0; + const long parsed = std::strtol(value, &end, 10); + while (*end == ' ' || *end == '\t') end++; + if (errno == ERANGE || end == value || + (*end && *end != '\r' && *end != '\n') || + parsed < 0 || parsed > kContentLenMax) { sendResponse(conn, 400, "application/json", "{\"error\":\"invalid content-length\"}"); return;🤖 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/HttpServerModule.cpp` around lines 107 - 121, The Content-Length parsing in HttpServerModule::handleFirmwareUpload currently trusts strtol with a null end pointer, so malformed values like non-numeric text or trailing junk can be accepted as valid lengths. Update the parsing logic to use an end pointer, verify the entire header value is consumed, and check for overflow/underflow before assigning to contentLen; keep rejecting negative or oversized values with the existing 400 response.
123-128: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject oversized bodies on non-streaming POST routes.
The buffer cap is correct for
/api/fileand/api/firmware/upload, but every other POST route only sees the buffered prefix. A large/api/controlor/api/modulesbody can therefore be parsed from a truncated JSON prefix instead of being rejected.Proposed fix
// Read POST body if present // Body pointer (headerEnd already found above) char* body = headerEnd ? const_cast<char*>(headerEnd) + 4 : nullptr; + const size_t bufferedBodyLen = + body ? static_cast<size_t>(totalRead) - static_cast<size_t>(body - req) : 0; + const bool streamingPost = + std::strcmp(method, "POST") == 0 && + (std::strcmp(path, "/api/file") == 0 || + std::strcmp(path, "/api/firmware/upload") == 0); + if (std::strcmp(method, "POST") == 0 && !streamingPost && + static_cast<size_t>(contentLen) > bufferedBodyLen) { + sendResponse(conn, 400, "application/json", "{\"error\":\"incomplete request body\"}"); + return; + } // RouteAlso applies to: 165-167
🤖 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/HttpServerModule.cpp` around lines 123 - 128, The body buffering logic in HttpServerModule should not silently cap request bodies for non-streaming POST routes like handleRequest/handleWriteFile-related parsing paths, because that can truncate JSON and allow oversized payloads to be parsed as a prefix. Update the request-size handling so only streaming upload routes (/api/file and /api/firmware/upload) may exceed the buffer, and all other POST routes explicitly reject bodies larger than the allowed buffered size before parsing. Apply the same fix to both places in the request handling flow where bodyNeeded is computed.src/ui/app.js (1)
2937-2950: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon’t re-render the file row before its double-click can fire.
The first click on a file immediately rebuilds the File Manager panel, removing
rowEl; that makes the intendeddblclickopen action unreliable. Defer the file-selection re-render or update file selection in place so the second click still targets the same row.Proposed fix
rowEl.addEventListener("click", (ev) => { ev.stopPropagation(); st.selected = childPath; st.selectedIsDir = entry.isDir; if (entry.isDir) { if (isOpen) st.expanded.delete(childPath); else st.expanded.add(childPath); + renderFileManager(mod, host); // reflect the selection highlight / expand + } else { + window.setTimeout(() => { + if (st.selected === childPath) renderFileManager(mod, host); + }, 250); } - renderFileManager(mod, host); // reflect the selection highlight / expand });🤖 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/app.js` around lines 2937 - 2950, The file row click handler in rowEl is re-rendering the File Manager immediately, which destroys the same row before the double-click handler on non-directories can fire. Update the selection behavior in the rowEl click/dblclick flow so file clicks do not call renderFileManager(mod, host) right away; instead defer the re-render or update the selection state in place for files while keeping the existing dblclick openFileEditor(childPath, entry.size) path intact.src/core/MqttModule.cpp (2)
336-373: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winpublishState() ignores
sendPacket()failures, unlike every other send site in this file.
publishName()only commitsnameSig_on send success, and the SUBSCRIBE/PINGREQ/CONNECT paths callresetConnection()on a failed send.publishState()breaks that pattern: it fires all threesendPacket()calls without checking their return value, then unconditionally updateslastOn_/lastBri_/lastPalette_/havePublished_at the end. A failedwriteSome()(broker backpressure, half-closed socket) leaves the module believing the broker has the current state, so the stalegettopics won't be retried until the underlying value changes again or the keepalive timeout eventually forces a reconnect.🛠️ Proposed fix — track send success and gate the cache update
char topic[128]; uint8_t buf[kSendBufLen]; + bool ok = true; // on/get buildTopic(topic, sizeof(topic), "on/get"); const char* onStr = on ? "true" : "false"; size_t n = buildMqttPublish(topic, reinterpret_cast<const uint8_t*>(onStr), std::strlen(onStr), buf, sizeof(buf)); - if (n) sendPacket(buf, n); + ok &= n && sendPacket(buf, n); // brightness/get (0..100 for mqttthing) buildTopic(topic, sizeof(topic), "brightness/get"); char briStr[8]; std::snprintf(briStr, sizeof(briStr), "%d", (bri * 100) / 255); n = buildMqttPublish(topic, reinterpret_cast<const uint8_t*>(briStr), std::strlen(briStr), buf, sizeof(buf)); - if (n) sendPacket(buf, n); + ok &= n && sendPacket(buf, n); // hsv/get — the chosen palette's representative hue, full sat, value = brightness%. buildTopic(topic, sizeof(topic), "hsv/get"); char hsvStr[16]; std::snprintf(hsvStr, sizeof(hsvStr), "%u,100,%d", static_cast<unsigned>(Palettes::representativeHue(pal)), (bri * 100) / 255); n = buildMqttPublish(topic, reinterpret_cast<const uint8_t*>(hsvStr), std::strlen(hsvStr), buf, sizeof(buf)); - if (n) sendPacket(buf, n); + ok &= n && sendPacket(buf, n); - lastOn_ = on; lastBri_ = bri; lastPalette_ = pal; - havePublished_ = true; + if (!ok) { resetConnection("error: publish failed"); return; } + lastOn_ = on; lastBri_ = bri; lastPalette_ = pal; + havePublished_ = true; }🤖 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/MqttModule.cpp` around lines 336 - 373, publishState() currently ignores sendPacket() failures and still marks the state as published. Update MqttModule::publishState to check the return value of each sendPacket call for the on/get, brightness/get, and hsv/get publishes, following the same failure-handling pattern used by publishName() and the SUBSCRIBE/PINGREQ/CONNECT paths. If any publish fails, stop committing lastOn_, lastBri_, lastPalette_, and havePublished_, and trigger the same connection-reset behavior used elsewhere in MqttModule so the state can be retried.
161-168: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueConsider using a slash-free CONNECT client ID.
topicPrefix()producesprojectMM/<mac>and reuses it as the MQTT ClientId. The spec only guarantees acceptance of alphanumeric IDs; a MAC-only value would keep the ID stable while avoiding broker compatibility issues.🤖 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/MqttModule.cpp` around lines 161 - 168, The CONNECT client ID in MqttModule::sendConnectPacket() is currently derived from topicPrefix() and may include a slash, which can cause broker compatibility issues. Update the clientId generation so it uses a stable slash-free identifier for buildMqttConnect(), ideally based on the MAC address but restricted to alphanumeric characters, while keeping the rest of sendConnectPacket() behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/ui/app.js`:
- Around line 2768-2772: The filesystem operation helper in runOp only handles
non-OK responses, so fetch rejections can escape as unhandled promise
rejections. Update runOp to wrap the fetch call and the existing response
handling in try/catch, show an alert with the same operation context when a
network error occurs, and keep renderFileManager(mod, host) running after both
success and handled failures.
---
Outside diff comments:
In `@moondeck/moondeck_ui/app.js`:
- Around line 886-903: The device name/IP controls in app.js use clickable span
elements, which are not keyboard-accessible. Update the nameText and ipLink
interaction in the device row rendering to use semantic interactive elements
such as buttons or anchors, or add proper tabindex, focus styling, and keyboard
handlers for Enter/Space so the existing showInView behavior can be triggered
without a mouse.
- Around line 925-930: The portChip click handler is leaving stale flash
settings in state when the selected device does not have a known firmware or
deviceModel. Update the portChip.addEventListener("click", ...) logic to
explicitly clear state.firmware and state.provisionDeviceModel (and any related
model selection) when device.firmware is missing/not in firmwares or
device.deviceModel is absent, while still setting a.port from device.last_port
via getActiveNetwork(). Keep the existing per-device assignment path, but make
the fallback reset behavior unconditional so the next flash/inject cannot reuse
another device’s firmware/model.
In `@src/core/HttpServerModule.cpp`:
- Around line 107-121: The Content-Length parsing in
HttpServerModule::handleFirmwareUpload currently trusts strtol with a null end
pointer, so malformed values like non-numeric text or trailing junk can be
accepted as valid lengths. Update the parsing logic to use an end pointer,
verify the entire header value is consumed, and check for overflow/underflow
before assigning to contentLen; keep rejecting negative or oversized values with
the existing 400 response.
- Around line 123-128: The body buffering logic in HttpServerModule should not
silently cap request bodies for non-streaming POST routes like
handleRequest/handleWriteFile-related parsing paths, because that can truncate
JSON and allow oversized payloads to be parsed as a prefix. Update the
request-size handling so only streaming upload routes (/api/file and
/api/firmware/upload) may exceed the buffer, and all other POST routes
explicitly reject bodies larger than the allowed buffered size before parsing.
Apply the same fix to both places in the request handling flow where bodyNeeded
is computed.
In `@src/core/MqttModule.cpp`:
- Around line 336-373: publishState() currently ignores sendPacket() failures
and still marks the state as published. Update MqttModule::publishState to check
the return value of each sendPacket call for the on/get, brightness/get, and
hsv/get publishes, following the same failure-handling pattern used by
publishName() and the SUBSCRIBE/PINGREQ/CONNECT paths. If any publish fails,
stop committing lastOn_, lastBri_, lastPalette_, and havePublished_, and trigger
the same connection-reset behavior used elsewhere in MqttModule so the state can
be retried.
- Around line 161-168: The CONNECT client ID in MqttModule::sendConnectPacket()
is currently derived from topicPrefix() and may include a slash, which can cause
broker compatibility issues. Update the clientId generation so it uses a stable
slash-free identifier for buildMqttConnect(), ideally based on the MAC address
but restricted to alphanumeric characters, while keeping the rest of
sendConnectPacket() behavior unchanged.
In `@src/ui/app.js`:
- Around line 2937-2950: The file row click handler in rowEl is re-rendering the
File Manager immediately, which destroys the same row before the double-click
handler on non-directories can fire. Update the selection behavior in the rowEl
click/dblclick flow so file clicks do not call renderFileManager(mod, host)
right away; instead defer the re-render or update the selection state in place
for files while keeping the existing dblclick openFileEditor(childPath,
entry.size) path intact.
In `@test/unit/core/unit_FileManagerModule.cpp`:
- Line 32: The test setup in the FileManager module is hardcoding a POSIX temp
path, which breaks portability. Update the temp root initialization in the
FileManager test helper to use std::filesystem::temp_directory_path() instead of
"/tmp", while keeping the unique suffix logic (counter/root) intact so the
generated directory remains isolated per test run.
- Around line 35-38: The test setup in unit_FileManagerModule.cpp is using
std::fopen without checking the returned FILE* before calling std::fputs and
std::fclose, which can crash the test binary if file creation fails. Update the
fixture code around the two std::fopen usages to validate the handle first and
fail the test cleanly if opening either file does not succeed, so the helpers in
this setup block never dereference a null FILE*.
🪄 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: e795021d-84ca-4adf-9608-bd871b59196e
📒 Files selected for processing (36)
docs/assets/extra.cssdocs/backlog/rename-scripts-to-moondeck.mddocs/history/decisions.mddocs/moonmodules/core/ui/ui.mdesp32/sdkconfig.defaults.esp32p4-ethesp32/sdkconfig.defaults.esp32p4-eth-wifiesp32/sdkconfig.defaults.esp32s31esp32/sdkconfig.defaults.ethesp32/sdkconfig.defaults.eth-spimoondeck/check/check_devices.pymoondeck/moondeck_ui/app.jssrc/core/AudioModule.hsrc/core/DevicesModule.hsrc/core/FileManagerModule.cppsrc/core/FileManagerModule.hsrc/core/FilesystemModule.cppsrc/core/FirmwareUpdateModule.hsrc/core/HttpServerModule.cppsrc/core/HttpServerModule.hsrc/core/ImprovFrame.hsrc/core/MqttModule.cppsrc/core/MqttModule.hsrc/core/MqttPacket.hsrc/core/Scheduler.cppsrc/core/Scheduler.hsrc/light/drivers/Drivers.hsrc/main.cppsrc/platform/desktop/platform_desktop.cppsrc/platform/esp32/platform_esp32.cppsrc/platform/platform.hsrc/ui/app.jstest/scenario_runner.cpptest/unit/core/unit_FileManagerModule.cpptest/unit/core/unit_MqttPacket.cpptest/unit/core/unit_TcpConnect.cppweb-installer/install-orchestrator.js
💤 Files with no reviewable changes (3)
- src/platform/esp32/platform_esp32.cpp
- docs/backlog/rename-scripts-to-moondeck.md
- src/platform/desktop/platform_desktop.cpp
Comment on lines
+2768
to
+2772
| const runOp = async (op, targetPath) => { | ||
| const method = op === "delete" ? "DELETE" : "POST"; | ||
| const res = await fetch("/api/dir?path=" + encodeURIComponent(targetPath), { method }); | ||
| if (!res.ok) alert(`${op} failed: ${await errorMessage(res)}`); | ||
| renderFileManager(mod, host); // rebuild the tree from /api/dir (fresh listing) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Surface network failures from filesystem operations.
fetch() rejections bypass the existing alert path, so create/delete can fail as an unhandled promise rejection. Wrap the request in try/catch and keep the same refresh behavior.
Proposed fix
const runOp = async (op, targetPath) => {
const method = op === "delete" ? "DELETE" : "POST";
- const res = await fetch("/api/dir?path=" + encodeURIComponent(targetPath), { method });
- if (!res.ok) alert(`${op} failed: ${await errorMessage(res)}`);
- renderFileManager(mod, host); // rebuild the tree from /api/dir (fresh listing)
+ try {
+ const res = await fetch("/api/dir?path=" + encodeURIComponent(targetPath), { method });
+ if (!res.ok) alert(`${op} failed: ${await errorMessage(res)}`);
+ } catch (err) {
+ alert(`${op} failed: ${err.message}`);
+ }
+ renderFileManager(mod, host); // rebuild the tree from /api/dir (fresh listing)
};📝 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.
Suggested change
| const runOp = async (op, targetPath) => { | |
| const method = op === "delete" ? "DELETE" : "POST"; | |
| const res = await fetch("/api/dir?path=" + encodeURIComponent(targetPath), { method }); | |
| if (!res.ok) alert(`${op} failed: ${await errorMessage(res)}`); | |
| renderFileManager(mod, host); // rebuild the tree from /api/dir (fresh listing) | |
| const runOp = async (op, targetPath) => { | |
| const method = op === "delete" ? "DELETE" : "POST"; | |
| try { | |
| const res = await fetch("/api/dir?path=" + encodeURIComponent(targetPath), { method }); | |
| if (!res.ok) alert(`${op} failed: ${await errorMessage(res)}`); | |
| } catch (err) { | |
| alert(`${op} failed: ${err.message}`); | |
| } | |
| renderFileManager(mod, host); // rebuild the tree from /api/dir (fresh listing) |
🤖 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/app.js` around lines 2768 - 2772, The filesystem operation helper in
runOp only handles non-OK responses, so fetch rejections can escape as unhandled
promise rejections. Update runOp to wrap the fetch call and the existing
response handling in try/catch, show an alert with the same operation context
when a network error occurs, and keep renderFileManager(mod, host) running after
both success and handled failures.
Processes the second CodeRabbit pass on PR #36 (all 10 findings valid, all fixed) plus a README refresh from the pre-merge gates. Hardens the HTTP body-length handling, makes MQTT state-publish failures retry instead of going stale, and fixes a File Manager double-click race. KPI: 16384lights | PC:646KB | tick:125/99/121/15/2/284/61/18/21/162/120/17/1/36us(FPS:8000/10101/8264/66666/500000/3521/16393/55555/47619/6172/8333/58823/1000000/27777) | ESP32:1368KB | tick:1879us(FPS:532) | heap:106KB Core - HttpServerModule: Content-Length now parses with an end pointer — rejects trailing junk ("123abc"), catches ERANGE overflow, and requires the value to end cleanly (CR/LF/space/EOS), returning 400. A non-streaming POST body larger than the 2 KB buffer now returns 413 instead of being silently truncated and parsed as a JSON prefix; only the streaming routes (/api/file, /api/firmware/upload) may exceed the buffer (they take the buffered prefix and drain the remainder off the socket). - MqttModule: publishState checks every sendPacket return — on any failure it resets the connection and does NOT commit last on/brightness/palette or havePublished_, so the change republishes after reconnect rather than leaving the hub showing stale values (the same "stamp only on success" rule publishName uses). The CONNECT clientId is now a slash-free "projectMM-<last6-of-MAC>" (MQTT-3.1.3-5 allows only [0-9a-zA-Z]; a broker may reject the '/' that topicPrefix carries) — same MAC identity as the topics. UI - app.js: the File Manager filesystem-op helper (runOp) wraps its fetch in try/catch so a network error surfaces as an alert instead of an unhandled rejection. A FILE row's single-click now updates the selection IN PLACE (move the highlight, refresh the breadcrumb + delete button) instead of re-rendering the tree — a re-render destroyed the row mid-gesture, so the dblclick after a double-click could miss and openFileEditor wouldn't fire. Folder clicks still re-render (rows appear/disappear). Scripts / MoonDeck - moondeck_ui/app.js: the device name and IP are now <button class="link-button"> (keyboard-focusable, Enter/Space activate) instead of clickable spans. The port chip unconditionally sets state.firmware/provisionDeviceModel — clearing them when the device has none — so the next Build/Flash can't reuse the previous device's firmware/model. - moondeck_ui/style.css: .link-button strips the native button chrome (reads as inline text) + a focus-visible ring. Docs - README: added File Manager / MQTT-Homebridge / firmware-upload-OTA feature bullets (linking to ui.md), so a README reader knows the branch's new capabilities exist. Tests - unit_FileManagerModule: the Rig temp root uses std::filesystem::temp_directory_path() (portable, not a hardcoded /tmp), and a writeFile helper REQUIREs a non-null FILE* before writing so a failed open fails the test cleanly instead of crashing the binary. Reviews - 🐇 Content-Length endptr/overflow, non-streaming body-cap → 413, publishState send-failure retry, slash-free clientId, runOp unhandled rejection, file-click dblclick race, moondeck device-row a11y buttons, portChip stale flash-state reset, portable test temp path, unchecked fopen: all fixed. (10 of 10 findings valid; none skipped this round.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.
What
This branch (6 commits) adds four capabilities and a rename, all hardware-verified:
projectMM/<last6-of-MAC>(rename-proof, the WLED/Tasmota/HA convention); the friendly deviceName rides a separate retained<prefix>/nametopic..binstraight to the device over the network (no URL host needed), streamed into the OTA partition.ethTypedefaults to None; a WiFi-only board no longer tries to bring up an Ethernet PHY it lacks. Boards that use Ethernet declare their wiring explicitly in the installer catalog.Plus a Shelly board (LOLIN D32 + 24-LED ring) in the installer catalog, and the
scripts/→moondeck/rename (with live MoonDeck output streaming).How
Core
FileManagerModule(new): exposes only the filesystem operations; browsing lives entirely in the UI over/api/dir. The engine-vs-tool split is explicit —FilesystemModule(the persistence engine) renders no card; the File Manager displays its "last saved" + filesystem-usage gauge.MqttModule(new): a Network sub-service bridging the light controls to a broker. Connect/keepalive/reconnect run entirely non-blocking onloop1svia the platform'sconnectStart/connectPoll— no render-loop stall. Live reconfig on a broker/credential change, clean DISCONNECT on disable, bounded connect + CONNACK timeouts, reset on any protocol violation.MqttPacket.h(new): MQTT 3.1.1 wire format (varint + build/parse + a byte-at-a-time inbound parser), golden-vector tested likeImprovFrame.h. No library.MoonModule::readBool/readUint8— one generic control-reader; the sharedmm::driversOn/Brightness/Palette(scheduler)free functions read the Drivers master state with a single absent-control default, used by both the WLED shim and MQTT.HttpServerModule: newGET /api/dir(lazy-load tree source),POST/DELETE /api/dir(mkdir/remove),POST /api/firmware/upload(streamed file-upload OTA, 409 guard). The WLED shim drives the real Driversoncontrol (deleted thebri=0on/off fudge).findModuleByNamedelegates toScheduler::firstByName(one canonical tree-walk).parseFilePathextracted public + unit-tested.IrModule: a learnable on/off Toggle action (newActionKind) drivingDrivers.on.NetworkModule:ethTypeopt-in per board; pins still seed from the per-chip default for a board that opts in.Platform
TcpConnection::connectStart/connectPoll— non-blocking outbound connect withgetaddrinfoDNS (desktop + esp32/lwip), the first outbound-TCP client + DNS in the platform layer.otaWriteStream(esp32): commits the image + flips the boot pointer and returns, so the caller answers before rebooting.Light domain
Drivers: newonbool (master power) that gates the correction LUT to zero without touching the brightness value; every consumer (UI, IR, WLED app, MQTT) drives this one control.teardown()clears every shared status string (fail/error/warning) symmetrically.Palette:nearestForHue/representativeHueSat— a HomeKit colour wheel's hue+saturation pick the nearest palette, computed from each palette's average RGB (no hand table).UI / Docs
app.js: lazy File Manager tree, double-click-to-open editor with pretty-JSON, binary + truncated-read read-only guards, icon toolbar, upload. One sharederrorMessage(res)helper for/api/*failures. MQTT card + MAC-stable topics + paste-in Homebridgemqttthingconfig inui.md.Catalog
check_devices.pyenforces "ethType set → board-wiring pins present" and rejects a non-int (incl. bool) ethType.Tests
nearestForHue, Ethernet opt-in,readBool/readUint8, real-onWLED behaviour, File Manager fs seam +parseFilePath, and the MQTT fixed-header overrun regression.Review
An internal 👾 Reviewer pass ran on the whole branch diff (
main...HEAD); CodeRabbit 🐇 reviewed the PR. All findings fixed on the branch, except one skipped with reason:onUpdate/ clean disable / stream-desync / Windows refused-connect / dead blockingconnect()/ stale docPaths (12 registerType + 6 cross-refs →ui.md/moxygen) / duplicated module-lookup + Drivers-read wrappers (converged toScheduler::firstByName+ sharedmm::drivers*) / oneotaInFlight()helper / generichidden-flag control-hide / app.jserrorMessagededup.alert()→ button text; injectBtn "pick a device model"; present-tense doc comments; File Managershow hiddendocumented transient; whitespace-only new-name guard; editor truncated-read guard (passentry.size); Rig dtor restores default fs root;ui.mdfuture-tense "later Home Assistant" removed +learnoff-sentinel clarified.unit_TcpConnectoff real loopback sockets. Reason: that test pins the platform socket primitive itself (getaddrinfo+ non-blockingconnect+select); a mock transport would mock away the code under test. Loopback is deterministic on desktop, the poll loop is a bounded ceiling (not a sleep), and real-loopback is the established pattern (unit_AudioModule_sync, the NetworkReceive/driver tests).Follow-ups (backlogged)
.mlhighlighting; async DNS; the upload/OTA drain-per-tick zero-freeze fix.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes