Module replace, nested cards, streaming JSON, emoji tags, UI polish

[ewowi]

Feature batch on next-iteration — six features plus a bug fix.

Features

• Nested module cards (plan-13) — child cards render inside their parent card's box (.card-children wrapper), not as flat siblings. Drag-reorder gates on a shared container.
• Module replace (plan-14) — POST /api/modules/<n>/replace swaps a child's type at the same position; ✎ button opens a same-role type picker.
• Emoji tags — per-module curated tags() + a UI-derived role emoji, shown as a chip filter in the type picker (AND filtering).
• Per-card stats — 🕒 timing + 🧠 static/dynamic memory; a help (?) link to each module's spec page on GitHub (engine-provided docPath).
• Press-twice delete & reboot — shared armPressTwice, no confirm() popups. Up/down buttons removed (drag-reorder works on mobile too).
• Network — platform::hostIp(): the app and MoonDeck print the LAN IP; MoonDeck device-discovery lists the PC at its network address. MoonDeck also shows the app version.

Bug fix

• Streaming JSON (plan-15) — /api/state, /api/types, /api/system stream through a JsonSink with no fixed-buffer ceiling. Fixes a 4 KB overflow that produced malformed JSON and broke the UI on larger module trees. The 4 KB static jsonBuf_ is removed.

Other

• Preview canvas: full content width, square aspect-ratio capped at 50vh, scroll-shrink restored.
• FilesystemModule exposes a lastSaved read-only control.
• README reworked; .claude/settings.local.json gitignored.

Pre-commit

Desktop build clean (zero warnings), ctest 1/1, scenarios 8/8, platform boundary PASS, specs 21/21, ESP32 build clean, Reviewer agent PASS. ESP32 live tick not captured — board offline.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Added HTTP API and WebSocket support for real-time module inspection and control.
  • Web UI now displays application version, module emoji tags, and clickable help links.
  • New press-twice confirmation UX for reboot and delete actions (auto-disarm on timeout/pointer leave).
  • Desktop app discovers and displays LAN IP address alongside localhost URL.
  • Module replacement feature allows swapping effect/modifier types in-place with clean defaults.
  • Added emoji-chip filtering in type picker for effect/modifier discovery.
  • Added 3D Plasma Effect and Rainbow Effect with dimension-aware rendering.
  • Effects and modifiers now advertise their supported dimensions (D1/D2/D3).
  • Statistics display shows memory usage and execution timing per module.

• Bug Fixes

  • Fixed JSON response buffering issues for larger module trees via streaming sink.
  • Fixed module name collision handling on load with automatic deduplication.
  • Added Int16 control persistence with proper signed value round-tripping.

• Documentation

  • Updated architecture guide to focus on system design and light pipeline.
  • Added building and coding standards documentation.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: ab86c02e-099c-4d97-adf9-8b8be17f0c04

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch next-iteration

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 8

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

docs/moonmodules/core/ui.md (1)

12-275: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

This markdown update should be soft-wrapped, not hard-wrapped.

Please reflow the changed prose blocks into single-line paragraphs and keep only semantic line breaks (lists/tables/code blocks).

As per coding guidelines, **/*.md: No hard line wraps in markdown — let the editor soft-wrap.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/moonmodules/core/ui.md` around lines 12 - 275, Summary: The updated
markdown contains hard-wrapped prose that must be soft-wrapped. Fix: in
docs/moonmodules/core/ui.md, locate the prose paragraphs under the major
sections (e.g., "Principles", "Layout", "Status bar", "Side navigation", "Module
card", "Control types", "Type picker", "Module hierarchy", "Communication",
"WebSocket", "REST API", "Static assets", "Styling", "3D preview", "State
updates — the no-rebuild contract", "localStorage keys", "Feature summary") and
reflow each changed prose block into single-line paragraphs (join lines into a
single sentence/paragraph), preserving existing semantic line breaks for lists,
tables, code blocks, headings, and verbatim blocks; ensure bullets, numbered
lists, code fences, inline code, tables, and example text remain untouched while
only removing hard line breaks inside normal narrative paragraphs.

docs/history/plan-14.md (1)

5-66: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Remove hard-wrapped paragraph lines in this markdown file.

Please reflow these sections so each paragraph is a single logical line and relies on editor soft-wrap.

As per coding guidelines, **/*.md: No hard line wraps in markdown — let the editor soft-wrap.

🤖 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/history/plan-14.md` around lines 5 - 66, The file
docs/history/plan-14.md contains hard-wrapped paragraphs; reflow each paragraph
so it is a single logical line (no hard line breaks inside paragraphs) and rely
on editor soft-wrap; ensure headings, lists, and code blocks keep their
intentional line breaks but all prose paragraphs are joined into single lines
throughout the document.

🤖 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/history/plan-15.md`:
- Around line 5-41: Reflow the hard-wrapped paragraphs in this markdown
(sections like the opening summary, Decision, Implementation —
`src/core/HttpServerModule.h`, Verification, and Notes) into single-line
paragraphs (soft-wrap style) so there are no manual line breaks inside
paragraphs; leave headings and code identifiers (e.g., `JsonSink`, `serveState`,
`buildStateJson`, `pushStateToWebSockets`, `jsonBuf_`) unchanged and only adjust
the paragraph text wrapping.

In `@docs/testing.md`:
- Around line 34-41: The "Module tree mutation" section contains hard line
wraps; reflow it to use soft-wrapping by making each paragraph a single
unwrapped line (no forced line breaks) so editors handle wrapping, e.g.,
collapse the bulleted descriptions of moveChildTo, replaceChildAt, and the
Replace lifecycle (mentioning HttpServerModule::handleReplaceModule) into single
continuous lines and ensure file/test references like test/test_movechild.cpp
and test/test_replacechild.cpp remain intact.

In `@src/core/HttpServerModule.h`:
- Around line 266-271: The code currently copies a computed nameLen into nameBuf
(char nameBuf[32]) and silently truncates long module names, which can cause
wrong module matches; instead validate that nameLen fits the buffer and reject
the request when it's too long (do not memcpy or call handleReplaceModule). In
the block using nameLen, check if nameLen >= sizeof(nameBuf) and if so send a
client error (e.g., HTTP 400 / "module name too long") using the existing
connection response mechanism and return early; otherwise proceed to memcpy,
null-terminate nameBuf, and call handleReplaceModule(conn, nameBuf, body).
Ensure you reference path, pathLen, nameLen, nameBuf, and handleReplaceModule
when making the change.
- Around line 820-833: When replacing a module created via
ModuleFactory::create(typeName), preserve the original module name by copying it
from the existing child before swapping: fetch the current child (e.g. via
parent->childAt(index) or similar), call its getName(), then set that name on
the new module (fresh->setName(...)) prior to calling
parent->replaceChildAt(index, fresh); maintain the existing lifecycle calls on
fresh (onBuildControls, setup, onAllocateMemory) and ensure you still own and
handle the returned old module from replaceChildAt.
- Around line 55-62: The current appendf function can silently truncate output
when vsnprintf reports a required length >= FRAG_MAX, producing malformed JSON;
change appendf to detect vsnprintf's return (n) and if n < 0 return/error, if n
< sizeof(frag) keep the current path, but if n >= sizeof(frag) allocate a
temporary buffer of size (n+1), re-run vsnprintf using a va_copy of the args to
fill that buffer, then call append(...) with the complete string; ensure va_list
is correctly copied and both va_end calls are performed and do not append the
truncated frag when the full formatted length exceeds FRAG_MAX (update the
function appendf accordingly).

In `@src/ui/app.js`:
- Around line 479-481: The click handler on btn leaves `armed` true during/after
calling onConfirm(), allowing repeated destructive actions; update the handler
so that when armed is true you immediately set `armed = false` (and optionally
disable the button) before calling onConfirm() to ensure the action only runs
once per arming cycle; locate the event listener attached to `btn` that
references `armed` and `onConfirm()` and modify the flow to clear/disarm prior
to invoking `onConfirm()`.

In `@test/test_replacechild.cpp`:
- Around line 85-106: Add an assertion that the replacement preserves the
original module name: after calling f.parent.replaceChildAt(1, &fresh) (and
before old->teardown()), check that the new module's name matches the original
(e.g. CHECK(fresh.getName() == "B") or CHECK(f.parent.getChildAt(1)->getName()
== "B")). Use the existing symbols replaceChildAt, TracingModule/setName,
getName, fresh, old and f.parent to locate where to insert this assertion.

---

Outside diff comments:
In `@docs/history/plan-14.md`:
- Around line 5-66: The file docs/history/plan-14.md contains hard-wrapped
paragraphs; reflow each paragraph so it is a single logical line (no hard line
breaks inside paragraphs) and rely on editor soft-wrap; ensure headings, lists,
and code blocks keep their intentional line breaks but all prose paragraphs are
joined into single lines throughout the document.

In `@docs/moonmodules/core/ui.md`:
- Around line 12-275: Summary: The updated markdown contains hard-wrapped prose
that must be soft-wrapped. Fix: in docs/moonmodules/core/ui.md, locate the prose
paragraphs under the major sections (e.g., "Principles", "Layout", "Status bar",
"Side navigation", "Module card", "Control types", "Type picker", "Module
hierarchy", "Communication", "WebSocket", "REST API", "Static assets",
"Styling", "3D preview", "State updates — the no-rebuild contract",
"localStorage keys", "Feature summary") and reflow each changed prose block into
single-line paragraphs (join lines into a single sentence/paragraph), preserving
existing semantic line breaks for lists, tables, code blocks, headings, and
verbatim blocks; ensure bullets, numbered lists, code fences, inline code,
tables, and example text remain untouched while only removing hard line breaks
inside normal narrative paragraphs.

🪄 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: 06f3eb88-86c0-45f5-a78e-f3ad6ab83170

📥 Commits

Reviewing files that changed from the base of the PR and between ce5bd70 and 314c3ad.

📒 Files selected for processing (36)

• .gitignore
• README.md
• docs/history/plan-14.md
• docs/history/plan-15.md
• docs/moonmodules/core/FilesystemModule.md
• docs/moonmodules/core/ui.md
• docs/moonmodules_draft/core/ui.md
• docs/testing.md
• scripts/moondeck.py
• scripts/moondeck_ui/index.html
• scripts/moondeck_ui/style.css
• src/core/FilesystemModule.h
• src/core/HttpServerModule.h
• src/core/ModuleFactory.h
• src/core/MoonModule.h
• src/light/CheckerboardEffect.h
• src/light/FireEffect.h
• src/light/GlowParticlesEffect.h
• src/light/LavaLampEffect.h
• src/light/MetaballsEffect.h
• src/light/MirrorModifier.h
• src/light/NoiseEffect.h
• src/light/ParticlesEffect.h
• src/light/PlasmaEffect.h
• src/light/PlasmaPaletteEffect.h
• src/light/RainbowEffect.h
• src/light/RipplesEffect.h
• src/light/SpiralEffect.h
• src/main.cpp
• src/platform/desktop/platform_desktop.cpp
• src/platform/esp32/platform_esp32.cpp
• src/platform/platform.h
• src/ui/app.js
• src/ui/style.css
• test/CMakeLists.txt
• test/test_replacechild.cpp

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Reflow wrapped markdown paragraphs to soft-wrap style.

This section is manually hard-wrapped; convert paragraph text to single-line paragraphs.

As per coding guidelines, **/*.md: No hard line wraps in markdown — let the editor soft-wrap.

🧰 Tools

🪛 LanguageTool

[style] ~9-~9: Consider an alternative for the overused word “exactly”.
Context: ...char json[4096]` stack buffer. This is exactly the failure the plan-12 spec note predi...

(EXACTLY_PRECISELY)

---

[style] ~16-~16: ‘in a heap’ might be wordy. Consider a shorter alternative.
Context: ...ate`. - Buffer mode — bytes collect in a heap buffer that doubles on demand. Used by ...

(EN_WORDINESS_PREMIUM_IN_A_HEAP)

🤖 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/history/plan-15.md` around lines 5 - 41, Reflow the hard-wrapped
paragraphs in this markdown (sections like the opening summary, Decision,
Implementation — `src/core/HttpServerModule.h`, Verification, and Notes) into
single-line paragraphs (soft-wrap style) so there are no manual line breaks
inside paragraphs; leave headings and code identifiers (e.g., `JsonSink`,
`serveState`, `buildStateJson`, `pushStateToWebSockets`, `jsonBuf_`) unchanged
and only adjust the paragraph text wrapping.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Replacement does not preserve the original module name.

fresh is created from type only, then swapped in-place; the previous name is not copied. This can break name-based addressing and produce duplicate/default names after replace.

Proposed fix

 auto* fresh = ModuleFactory::create(typeName);
 if (!fresh) {
     sendResponse(conn, 400, "application/json", "{\"error\":\"unknown type\"}");
     return;
 }
+if (mod->name()) fresh->setName(mod->name());

📝 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

	auto* fresh = ModuleFactory::create(typeName);
	if (!fresh) {
	sendResponse(conn, 400, "application/json", "{\"error\":\"unknown type\"}");
	return;
	}
	// Swap in place; replaceChildAt returns the old module, which we own.
	MoonModule* old = parent->replaceChildAt(index, fresh);
	// Lifecycle on the fresh module — same phase order as the add path.
	fresh->onBuildControls();
	fresh->setup();
	fresh->onAllocateMemory();
	auto* fresh = ModuleFactory::create(typeName);
	if (!fresh) {
	sendResponse(conn, 400, "application/json", "{\"error\":\"unknown type\"}");
	return;
	}
	if (old->name()) fresh->setName(old->name());
	// Swap in place; replaceChildAt returns the old module, which we own.
	MoonModule* old = parent->replaceChildAt(index, fresh);
	// Lifecycle on the fresh module — same phase order as the add path.
	fresh->onBuildControls();
	fresh->setup();
	fresh->onAllocateMemory();

🤖 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.h` around lines 820 - 833, When replacing a module
created via ModuleFactory::create(typeName), preserve the original module name
by copying it from the existing child before swapping: fetch the current child
(e.g. via parent->childAt(index) or similar), call its getName(), then set that
name on the new module (fresh->setName(...)) prior to calling
parent->replaceChildAt(index, fresh); maintain the existing lifecycle calls on
fresh (onBuildControls, setup, onAllocateMemory) and ensure you still own and
handle the returned old module from replaceChildAt.

[coderabbitai]

🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Add an assertion for name preservation during replace.

Current tests validate position/parenting/lifecycle but not name continuity. Add coverage that replacement keeps the original module name when API semantics require stable identity.

🧰 Tools

🪛 Cppcheck (2.20.0)

[style] 87-87: The function 'addProgress' is never used.

(unusedFunction)

---

[style] 93-93: The function 'count' is never used.

(unusedFunction)

---

[style] 99-99: The function 'setHidden' is never used.

(unusedFunction)

---

[style] 89-89: The function 'typeName' is never used.

(unusedFunction)

---

[style] 90-90: The function 'setTypeName' is never used.

(unusedFunction)

---

[style] 92-92: The function 'enabled' is never used.

(unusedFunction)

---

[style] 93-93: The function 'setEnabled' 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 `@test/test_replacechild.cpp` around lines 85 - 106, Add an assertion that the
replacement preserves the original module name: after calling
f.parent.replaceChildAt(1, &fresh) (and before old->teardown()), check that the
new module's name matches the original (e.g. CHECK(fresh.getName() == "B") or
CHECK(f.parent.getChildAt(1)->getName() == "B")). Use the existing symbols
replaceChildAt, TracingModule/setName, getName, fresh, old and f.parent to
locate where to insert this assertion.

[ewowi]

@coderabbitai, processed your review, will be added in next commit:

CodeRabbit review processed. Summary:

Fixed (3 valid findings — src/core/HttpServerModule.h, src/ui/app.js)
nameLen truncation — /replace (and /move, which had the identical bug CodeRabbit missed) now reject a too-long module name with 400 "module name too long" instead of silently truncating it. Verified: 40-char name → 400 on both routes.

appendf silent truncation — a formatted fragment exceeding the 256-byte stack buffer is now re-formatted into an exact heap buffer (va_copy + platform::alloc) — never truncated. The common short-fragment path is unchanged (no allocation).

armPressTwice re-arm — disarm() now runs before onConfirm(), so a stray repeat click can't fire a destructive action twice (e.g. two /api/reboot). Verified: armed → confirm → disarmed.

Skipped (2 findings — with reasons)
"Preserve the old module name on replace" — invalid: it contradicts the deliberate clean-swap design (plan-14, the spec). Replace gives the new module its factory typeName so a NoiseEffect→PlasmaEffect swap correctly relabels the card. Keeping the old name would mislabel it. The companion test-assertion ask was skipped for the same reason.

Markdown soft-wrap (5 files: plan-14, plan-15, testing.md, ui.md) — invalid: those files are already soft-wrapped (one long line per paragraph, per CLAUDE.md). CodeRabbit misread its own line-wrapped diff display as hard wraps in the source. Confirmed by measuring actual line lengths (99–480 chars/line).

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@ewowi — thanks for the detailed breakdown! All three fixes look well-reasoned:

• nameLen truncation — good catch extending the fix to /move as well (I did miss that route).
• appendf silent truncation — the va_copy + heap fallback approach is the right pattern; keeps the hot path allocation-free.
• armPressTwice re-arm — correct ordering: disarm before firing the callback prevents any double-trigger window.

Your skips are both valid:

• Preserve name on replace — you're right, the clean-swap semantics from plan-14 are intentional. Relabelling the card with the new type's factory name is correct behaviour; carrying the old name across would silently mislabel a replaced module.
• Markdown soft-wrap — confirmed; CodeRabbit was misreading its own terminal-wrapped diff display as hard line breaks in the source. No action needed.

🐇 ✨

---

✏️ Learnings added

Learnt from: ewowi
URL: https://github.com/ewowi/projectMM/pull/4

Timestamp: 2026-05-22T20:59:36.846Z
Learning: In projectMM (ewowi/projectMM), the `POST /api/modules/<name>/replace` endpoint (plan-14) intentionally assigns the new module its factory `typeName` rather than preserving the old module's name. This is deliberate clean-swap design: a NoiseEffect→PlasmaEffect swap must relabel the card with the new type name; preserving the old name would mislabel it. Do not flag absence of name-preservation as a bug.

---

Learnt from: ewowi
URL: https://github.com/ewowi/projectMM/pull/4

Timestamp: 2026-05-22T20:59:36.846Z
Learning: In projectMM (ewowi/projectMM), Markdown documentation files (docs/) use soft-wrapping — one long line per paragraph — as mandated by CLAUDE.md. Terminal-wrapped diff display can make these look like hard-wrapped lines. Do not flag soft-wrapped long lines in these docs as a style issue.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

---

If you found this review helpful, would you consider giving us a shout-out on X?

Thank you for using CodeRabbit!

[coderabbitai]

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/main.cpp (1)

28-58: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Handle factory failure paths before dereferencing created modules.

registerType() and create() can fail, but this path assumes success and immediately dereferences returned pointers. A single registration/allocation failure can crash startup.

Proposed fail-fast pattern

-static void registerModuleTypes() {
+static bool registerModuleTypes() {
+    bool ok = true;
     // Containers
-    mm::ModuleFactory::registerType<mm::LayoutGroup>("LayoutGroup", "light/LayoutGroup.md");
-    mm::ModuleFactory::registerType<mm::Layer>("Layer", "light/Layer.md");
-    mm::ModuleFactory::registerType<mm::DriverGroup>("DriverGroup", "light/DriverGroup.md");
+    ok &= mm::ModuleFactory::registerType<mm::LayoutGroup>("LayoutGroup", "light/LayoutGroup.md");
+    ok &= mm::ModuleFactory::registerType<mm::Layer>("Layer", "light/Layer.md");
+    ok &= mm::ModuleFactory::registerType<mm::DriverGroup>("DriverGroup", "light/DriverGroup.md");
     // ... same for remaining registerType calls ...
+    return ok;
 }
 
 void mm_main(volatile bool& keepRunning, mm::lengthType gridW, mm::lengthType gridH, uint16_t httpPort) {
-    registerModuleTypes();
+    if (!registerModuleTypes()) {
+        std::printf("Module registration failed\n");
+        return;
+    }
+
+    auto mustCreate = [](const char* type) -> mm::MoonModule* {
+        auto* mod = mm::ModuleFactory::create(type);
+        if (!mod) std::printf("Module creation failed: %s\n", type);
+        return mod;
+    };
 
-    auto* filesystemModule = static_cast<mm::FilesystemModule*>(mm::ModuleFactory::create("FilesystemModule"));
+    auto* filesystemModule = static_cast<mm::FilesystemModule*>(mustCreate("FilesystemModule"));
+    if (!filesystemModule) return;
     filesystemModule->setScheduler(&scheduler);

Also applies to: 91-137

🤖 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 28 - 58, The registerModuleTypes function
currently assumes mm::ModuleFactory::registerType (and any subsequent
mm::ModuleFactory::create calls elsewhere) always succeed and immediately
dereferences returned objects; change these call sites (notably
registerModuleTypes, and the similar block around the create() calls referenced
at lines 91-137) to validate results before use: for registerType, check the
returned status/handle (or catch exceptions) and log and fail-fast (or skip
registration) on error; for create(), check for nullptr or failed optional and
avoid dereferencing—return an error, exit startup, or handle cleanup when
creation fails so startup cannot crash. Ensure you reference and guard the
specific symbols mm::ModuleFactory::registerType, mm::ModuleFactory::create, and
the registerModuleTypes function while adding logging and
early-return/error-handling.

🤖 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/run/run_desktop.py`:
- Around line 31-37: The Windows tasklist filter appends ".exe" blindly causing
duplicate extensions (e.g., "projectMM.exe.exe"); update _is_running() to
compute a single exe_name variable that ensures exactly one ".exe" (e.g., if
name.endswith(".exe") use name else append ".exe"), then pass that exe_name into
the tasklist filter (f"IMAGENAME eq {exe_name}") instead of using name directly;
keep the rest of the logic (subprocess.run call and checks) unchanged and still
reference EXECUTABLE for the non-Windows branch.

In `@src/light/effects/PlasmaEffect.h`:
- Around line 68-71: The hot path in PlasmaEffect.h does a per-pixel division by
5 when computing hue (variable hue using s1, s2_y, s3, s4, s5_z and hue_shift);
remove the costly `/ 5` and replace it with an integer multiply-by-reciprocal +
right-shift (or a small lookup) to approximate divide-by-5 without using `/`
(e.g. compute a uint16_t sum = s1 + s2_y + s3 + s4 + s5_z and use (sum *
RECIPROCAL_CONST) >> SHIFT to get sum/5, then add hue_shift; apply the same
reciprocal approach for the 5-term branch while keeping the existing >>2 for the
4-term branch). Ensure types stay uint16_t/uint8_t and use named symbols (hue,
s1, s2_y, s3, s4, s5_z, hue_shift) so the change is localized and avoids runtime
division inside the inner pixel loop.

In `@src/light/Layer.h`:
- Around line 64-93: Add a direct include of the C string header to ensure
std::memcpy is declared: update the header that defines the extrude(Dim
effectDim) method (Layer.h) to include <cstring> so the std::memcpy calls inside
extrude (and any other uses in this header) do not rely on transitive includes;
simply add the include at the top of the file near other includes.

In `@src/ui/app.js`:
- Around line 1421-1424: The pinch-zoom math in the touch handler can divide by
zero when the two touch points coincide (d === 0); update the block around
touchDistance(e.touches[0], e.touches[1]) so that when d is 0 (or extremely
close to 0) you skip updating camDist or treat ratio as 1 (or use a small
epsilon) to avoid non-finite values; change the logic in the pinch handling that
computes ratio = pinchDist / d and updates camDist and pinchDist so it first
checks d and only applies camDist = Math.max(0.5, Math.min(10, camDist * ratio))
when d is valid, otherwise preserve current camDist and set pinchDist
appropriately.
- Around line 369-373: The enabled click handler can be reverted by a stale
WebSocket patch because updateValues() uses dragTs to suppress WS writes but the
enabled click path does not update dragTs; modify the enabled click handler (the
listener on the enabled element that calls setEnabledUi and sendControl with
mod.name, "enabled", next) to also update the same dragTs timestamp used by
updateValues (e.g., set the element/module's dragTs to Date.now() or assign to
the timestamp variable/key for mod.name) immediately when clicked so incoming WS
patches older than this timestamp are ignored.

---

Outside diff comments:
In `@src/main.cpp`:
- Around line 28-58: The registerModuleTypes function currently assumes
mm::ModuleFactory::registerType (and any subsequent mm::ModuleFactory::create
calls elsewhere) always succeed and immediately dereferences returned objects;
change these call sites (notably registerModuleTypes, and the similar block
around the create() calls referenced at lines 91-137) to validate results before
use: for registerType, check the returned status/handle (or catch exceptions)
and log and fail-fast (or skip registration) on error; for create(), check for
nullptr or failed optional and avoid dereferencing—return an error, exit
startup, or handle cleanup when creation fails so startup cannot crash. Ensure
you reference and guard the specific symbols mm::ModuleFactory::registerType,
mm::ModuleFactory::create, and the registerModuleTypes function while adding
logging and early-return/error-handling.

🪄 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: 959fb578-becb-4575-844c-111f39e2b5c6

📥 Commits

Reviewing files that changed from the base of the PR and between 314c3ad and 70890cd.

📒 Files selected for processing (59)

• docs/architecture-light.md
• docs/moonmodules/core/ui.md
• docs/moonmodules/light/EffectBase.md
• docs/moonmodules/light/Layer.md
• docs/moonmodules/light/effects/NoiseEffect.md
• docs/moonmodules/light/effects/PlasmaEffect.md
• docs/performance.md
• docs/plan.md
• docs/testing.md
• scripts/MoonDeck.md
• scripts/moondeck_ui/app.js
• scripts/run/run_desktop.py
• src/core/HttpServerModule.h
• src/core/ModuleFactory.h
• src/core/MoonModule.h
• src/core/types.h
• src/light/EffectBase.h
• src/light/Layer.h
• src/light/PlasmaEffect.h
• src/light/RainbowEffect.h
• src/light/drivers/ArtNetSendDriver.h
• src/light/drivers/DriverGroup.h
• src/light/drivers/PreviewDriver.h
• src/light/effects/CheckerboardEffect.h
• src/light/effects/EffectBase.h
• src/light/effects/FireEffect.h
• src/light/effects/GlowParticlesEffect.h
• src/light/effects/LavaLampEffect.h
• src/light/effects/MetaballsEffect.h
• src/light/effects/NoiseEffect.h
• src/light/effects/ParticlesEffect.h
• src/light/effects/PlasmaEffect.h
• src/light/effects/PlasmaPaletteEffect.h
• src/light/effects/RainbowEffect.h
• src/light/effects/RipplesEffect.h
• src/light/effects/SpiralEffect.h
• src/light/layouts/GridLayout.h
• src/light/layouts/LayoutGroup.h
• src/light/modifiers/MirrorModifier.h
• src/light/modifiers/ModifierBase.h
• src/main.cpp
• src/ui/app.js
• src/ui/index.html
• src/ui/style.css
• test/CMakeLists.txt
• test/scenario_runner.cpp
• test/test_artnet_packet.cpp
• test/test_extrude.cpp
• test/test_filesystem_persistence.cpp
• test/test_fire.cpp
• test/test_grid_layout.cpp
• test/test_metaballs.cpp
• test/test_mirror.cpp
• test/test_noise.cpp
• test/test_particles.cpp
• test/test_plasma.cpp
• test/test_preview_driver.cpp
• test/test_rainbow.cpp
• test/test_stateless_effects.cpp

💤 Files with no reviewable changes (4)

• src/light/EffectBase.h
• src/light/RainbowEffect.h
• src/ui/index.html
• src/light/PlasmaEffect.h

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Remove per-pixel integer division from the hot path.

Line 70 performs / 5 in the inner pixel loop. On MCU targets this is avoidable overhead in a render hot path.

⚙️ Proposed fix

-                    uint8_t hue = is3d
-                        ? static_cast<uint8_t>(
-                              (static_cast<uint16_t>(s1 + s2_y + s3 + s4 + s5_z) / 5) + hue_shift)
+                    uint16_t hue_sum = static_cast<uint16_t>(s1) + s2_y + s3 + s4 + s5_z;
+                    uint8_t hue = is3d
+                        ? static_cast<uint8_t>(
+                              ((static_cast<uint32_t>(hue_sum) * 0xCCCDu) >> 18) + hue_shift)
                         : static_cast<uint8_t>(((s1 + s2_y + s3 + s4) >> 2) + hue_shift);

As per coding guidelines, docs/moonmodules/light/effects/PlasmaEffect.md requires to “avoid division/modulo in inner loop”.

🤖 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/effects/PlasmaEffect.h` around lines 68 - 71, The hot path in
PlasmaEffect.h does a per-pixel division by 5 when computing hue (variable hue
using s1, s2_y, s3, s4, s5_z and hue_shift); remove the costly `/ 5` and replace
it with an integer multiply-by-reciprocal + right-shift (or a small lookup) to
approximate divide-by-5 without using `/` (e.g. compute a uint16_t sum = s1 +
s2_y + s3 + s4 + s5_z and use (sum * RECIPROCAL_CONST) >> SHIFT to get sum/5,
then add hue_shift; apply the same reciprocal approach for the 5-term branch
while keeping the existing >>2 for the 4-term branch). Ensure types stay
uint16_t/uint8_t and use named symbols (hue, s1, s2_y, s3, s4, s5_z, hue_shift)
so the change is localized and avoids runtime division inside the inner pixel
loop.

[coderabbitai]

Actionable comments posted: 9

🤖 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/history/plan-16.md`:
- Line 7: The fenced code blocks in the document are missing explicit language
tags and required blank lines which trigger MD040/MD031; update each fenced
block by adding the appropriate language identifier (e.g., use ```text for the
ASCII tree block and ```cpp for C++ snippets) and ensure there is a blank line
before and after every fenced block; specifically fix the blocks that show the
ASCII layout example (the one starting with "Layouts             ← was
LayoutGroup") and the C++ snippets containing "`#pragma` once" and the includes
(the cpp fences near the blocks showing src/light/Layers.h and the other two cpp
examples around the documented Line 124 and Line 140), making sure each fence is
like ```text or ```cpp and surrounded by a blank line above and below.

In `@docs/moonmodules_draft/core/ui.md`:
- Line 40: Update the stale table row text that currently reads
"System/Network/Layer/Drivers" and "up/down + drag" to reflect the standardized
naming and behavior: change the root modules sequence to
"Layouts/Layers/Drivers" and state that child reordering is "drag-only" (keep
the `/api/modules/reorder` note as-is). Locate and edit the same table row
string in the docs (the line containing "Drag-to-reorder root modules...") so
the row now mentions "Layouts/Layers/Drivers" and "drag-only child reorder".

In `@docs/moonmodules/light/Layouts.md`:
- Line 30: Update the broken architecture link in
docs/moonmodules/light/Layouts.md by replacing the target `[architecture.md §
Rebuild Propagation](../../architecture.md#rebuild-propagation)` with the
correct doc path and anchor (e.g., point to docs/architecture-light.md with the
proper anchor), ensure the anchor id matches the heading in
docs/architecture-light.md (or add an explicit anchor there), and verify the
updated markdown link resolves to the "Rebuild Propagation" section.

In `@src/core/FilesystemModule.h`:
- Around line 305-309: The Int16 case in the switch (ControlType::Int16) parses
an int via mm::json::parseInt and casts it directly to int16_t causing
wraparound for out-of-range values; change this to clamp the parsed int to
std::numeric_limits<int16_t>::min()/max() before assigning through c.ptr
(static_cast<int16_t*>), i.e. compute a bounded value from the parsed int and
then store that into *static_cast<int16_t*>(c.ptr) so persisted out-of-range
JSON values are safely clamped instead of wrapping.

In `@src/core/HttpServerModule.h`:
- Around line 596-600: The Int16 branch (ControlType::Int16) currently parses an
int via mm::json::parseInt and blindly casts to int16_t into
*static_cast<int16_t*>(c.ptr), which allows wraparound for out-of-range values;
change it to parse into a wider signed type (e.g., long or int64_t), validate
the parsed value against INT16_MIN/INT16_MAX, clamp or return an error on
overflow, and only then store the safely cast value into
*static_cast<int16_t*>(c.ptr) so out-of-range JSON integers cannot corrupt
program state.

In `@src/light/Layers.h`:
- Around line 25-29: The loop in Layers that does
static_cast<Layer*>(child(i))->setLayouts(layouts_) assumes every child is a
Layer and can UB if the invariant is broken; change it to guard the downcast by
checking the child's runtime type before casting (for example add/use a virtual
predicate like isLayer() or a type/kind() accessor on the base class, or use
dynamic_cast where RTTI is enabled), skip or log unexpected child types instead
of casting, and apply the same guarded pattern to the other occurrence that sets
activeLayer()/calls Layer methods (the block referenced at lines 47-53).

In `@src/main.cpp`:
- Around line 91-99: Replace the intentional null-deref behavior when
ModuleFactory::create returns nullptr with an explicit fail-fast check: add a
small helper (e.g. require_not_null or requireOrAbort) and call it immediately
after each ModuleFactory::create invocation in main.cpp so that when the factory
returns nullptr you log a clear diagnostic mentioning "ModuleFactory::create"
and the component/type name and then call std::abort() (or std::terminate())
instead of relying on UB; apply the same pattern to the other
ModuleFactory::create sites referenced around lines 103-117 so all startup
allocations fail with a deterministic, informative abort.

In `@src/ui/app.js`:
- Around line 709-718: Replace the hardcoded slider bounds by using
control-provided bounds: when creating the input range use ctrl.min and ctrl.max
(coerced to numbers or parsed) with -100 and 200 as fallbacks (e.g., input.min =
Number(ctrl.min ?? -100); input.max = Number(ctrl.max ?? 200)); also ensure the
initial input.value (currently set from ctrl.value) is clamped to the computed
min/max so you never set an out-of-range value. Reference the input element
creation and the existing ctrl.value usage to locate where to change min/max and
value clamping.

In `@test/scenario_runner.cpp`:
- Around line 188-197: The wiring block uses terse names `lg` and `l`; replace
them with semantic names like `layoutsModule` and `layerModule` where `lg` is
declared as auto* static_cast<mm::Layouts*>(modules[props["layouts"].str]) and
used in layer->setLayouts, and where `l` is declared as auto*
static_cast<mm::Layer*>(modules[props["layer"].str]) and used in
static_cast<mm::Drivers*>(mod)->setLayer; update both declarations and
subsequent uses to the new names to improve readability.

🪄 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: 2329a100-5f7a-4cef-a3a8-afdf89b9a4a9

📥 Commits

Reviewing files that changed from the base of the PR and between 70890cd and 28e0e1f.

📒 Files selected for processing (54)

• README.md
• docs/architecture-light.md
• docs/history/plan-16.md
• docs/moonmodules/core/FilesystemModule.md
• docs/moonmodules/core/MoonModule.md
• docs/moonmodules/core/NetworkModule.md
• docs/moonmodules/light/BlendMap.md
• docs/moonmodules/light/DriverGroup.md
• docs/moonmodules/light/Drivers.md
• docs/moonmodules/light/EffectBase.md
• docs/moonmodules/light/Layer.md
• docs/moonmodules/light/Layers.md
• docs/moonmodules/light/LayoutGroup.md
• docs/moonmodules/light/Layouts.md
• docs/moonmodules/light/MappingLUT.md
• docs/moonmodules/light/drivers/ArtNetSendDriver.md
• docs/moonmodules/light/layouts/GridLayout.md
• docs/moonmodules_draft/core/ui.md
• docs/performance.md
• docs/plan.md
• docs/testing.md
• scripts/run/run_desktop.py
• src/core/Control.h
• src/core/FilesystemModule.h
• src/core/HttpServerModule.h
• src/core/ModuleFactory.h
• src/core/MoonModule.h
• src/light/Layer.h
• src/light/Layers.h
• src/light/drivers/ArtNetSendDriver.h
• src/light/drivers/Drivers.h
• src/light/drivers/PreviewDriver.h
• src/light/effects/PlasmaEffect.h
• src/light/layouts/GridLayout.h
• src/light/layouts/Layouts.h
• src/main.cpp
• src/ui/app.js
• test/CMakeLists.txt
• test/scenario_runner.cpp
• test/scenarios/base-pipeline.json
• test/scenarios/memory-1to1.json
• test/scenarios/memory-lut.json
• test/scenarios/mirror.json
• test/test_extrude.cpp
• test/test_fire.cpp
• test/test_grid_layout.cpp
• test/test_layers_container.cpp
• test/test_metaballs.cpp
• test/test_noise.cpp
• test/test_particles.cpp
• test/test_plasma.cpp
• test/test_preview_driver.cpp
• test/test_rainbow.cpp
• test/test_stateless_effects.cpp

💤 Files with no reviewable changes (2)

• docs/moonmodules/light/DriverGroup.md
• docs/moonmodules/light/LayoutGroup.md

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Avoid intentional null-deref as startup error handling.

The current strategy relies on undefined behavior for factory failures. Please fail fast with an explicit check (or helper wrapper) so diagnostics always point to the missing/OOM type directly.

Proposed pattern

+template <typename T>
+static T* requireModule(const char* typeName) {
+    auto* base = mm::ModuleFactory::create(typeName);
+    if (!base) {
+        std::fprintf(stderr, "Fatal: ModuleFactory::create(%s) failed\n", typeName);
+        std::abort();
+    }
+    return static_cast<T*>(base);
+}
...
-    auto* filesystemModule = static_cast<mm::FilesystemModule*>(mm::ModuleFactory::create("FilesystemModule"));
+    auto* filesystemModule = requireModule<mm::FilesystemModule>("FilesystemModule");

Also applies to: 103-117

🤖 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 91 - 99, Replace the intentional null-deref
behavior when ModuleFactory::create returns nullptr with an explicit fail-fast
check: add a small helper (e.g. require_not_null or requireOrAbort) and call it
immediately after each ModuleFactory::create invocation in main.cpp so that when
the factory returns nullptr you log a clear diagnostic mentioning
"ModuleFactory::create" and the component/type name and then call std::abort()
(or std::terminate()) instead of relying on UB; apply the same pattern to the
other ModuleFactory::create sites referenced around lines 103-117 so all startup
allocations fail with a deterministic, informative abort.

[coderabbitai]

Actionable comments posted: 11

🤖 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 `@CMakeLists.txt`:
- Around line 14-19: The mm_core target was created as a STATIC library
including non-platform .cpp (HttpServerModule.cpp) which violates the root CMake
contract; change the mm_core target to an INTERFACE library (remove
src/core/HttpServerModule.cpp from add_library) so mm_core remains header-only,
keep target_include_directories(mm_core PUBLIC src/) and
target_link_libraries(mm_core PUBLIC mm_platform) as appropriate, and ensure any
platform-specific .cpp (e.g., HttpServerModule.cpp) are moved into the
platform-specific targets or compiled under the platform-specific CMakeLists
rather than added to mm_core; also preserve the existing
add_dependencies(mm_core ui_embed) placement after ui_embed is defined.

In `@docs/plan.md`:
- Around line 14-15: The plan incorrectly instructs adding core service .cpp
files to mm_core's STATIC sources in CMakeLists.txt, which violates the root
CMake contract that mm_core is an INTERFACE (header-only) library and only
platform .cpp files should be compiled; update the migration plan text to
instead: leave core modules header-only under mm_core (keep .h and move
implementation into platform-specific .cpp only when appropriate), do NOT add
core .cpp files to mm_core's STATIC sources, and when creating platform
implementations add them to the platform targets' source lists per the root
CMake contract (referencing mm_core, CMakeLists.txt, INTERFACE, STATIC, .cpp, .h
to locate the relevant lines).

In `@src/core/Base64.h`:
- Line 16: Change the public helper signature inline void base64Encode(const
uint8_t* in, size_t inLen, char* out, size_t outMax) to take std::span for both
input and output (e.g., std::span<const uint8_t> input, std::span<char> output),
add `#include` <span> in Base64.h, update the function body to use
input.size()/data() and output.size()/data() instead of in/inLen/out/outMax, and
update all call sites to pass spans (or construct spans from buffers) so size
contracts are enforced by the span bounds; keep the function inline and preserve
existing semantics and error handling.
- Line 29: The code writes a null terminator with out[oi] = 0 which can overflow
when outMax == 0; modify the function containing out, oi and outMax (Base64.h)
to guard the write: either early-return/fail when outMax == 0 or only write the
terminator if oi < outMax (e.g., check outMax > 0 and/or oi < outMax before
doing out[oi] = 0), ensuring you keep the same return/error semantics and do not
change the buffer indexing elsewhere.

In `@src/core/FilesystemModule.h`:
- Around line 306-315: The Int16 case in the load path (ControlType::Int16 using
mm::json::parseInt and writing to c.ptr) currently only clamps to int16 limits
and ignores control-specific bounds c.min/c.max; update the handler to, after
clamping to INT16_MIN/INT16_MAX, also clamp the parsed value to the control's
configured bounds (use c.min and c.max analogous to the Uint8 branch) before
narrowing and assigning to *static_cast<int16_t*>(c.ptr) so persisted values
respect the control's min/max.

In `@src/core/HttpServerModule.cpp`:
- Around line 603-617: When parentId is empty the new module (mod) is left
unattached and still proceeds through onBuildControls/onSetup/onAllocateMemory,
causing an orphan; modify the creation flow so that if
findModuleByName(parentId) is not used (parentId[0] == 0) you explicitly attach
or register the module with scheduler_ (e.g., call the scheduler_ method that
owns/records modules such as scheduler_->addModule/registerModule or set
scheduler_ as the module's owner) before any build/setup/allocation steps, and
if that scheduler attachment fails undo/delete mod and return an error; apply
the same change to the similar block around the other creation path (the 631-636
region) to avoid unreachable runtime state for parentless modules.
- Around line 658-667: The current deletion path removes the module from its
parent but skips unlinking top-level modules from scheduler storage, risking
dangling pointers; in the block using parent(), removeChild, teardown(), and
delete mod you must handle the parent==nullptr case by first unlinking the
module from scheduler storage (e.g. call the scheduler's remove/unlink method
such as Scheduler::instance()->removeModule(mod) or mod->detachFromScheduler()),
ensure any scheduler pointers/flags on the module are cleared, and only then
call mod->teardown() and delete mod; keep the existing parent->removeChild(mod)
branch as-is and add the explicit scheduler-unlink branch for top-level modules.
- Around line 446-454: The code for ControlType::Uint8 and ControlType::Uint16
currently casts parsed ints directly into c.ptr which allows out-of-range values
to wrap; change the logic after mm::json::parseInt to validate the parsed int
against the control's bounds (use c.min and c.max) and if the value is outside
[c.min, c.max] do not assign—handle it by returning a proper error/HTTP 400
response (or otherwise signaling validation failure) instead of performing the
static_cast and assignment; ensure you apply the same bound-check logic to both
ControlType::Uint8 and ControlType::Uint16 before writing to
*static_cast<...*>(c.ptr).

In `@src/core/JsonSink.h`:
- Around line 32-43: The append(const char* s) path updates heapLen_ when conn_
is false but never writes the trailing null byte, violating the null-terminated
string contract; after each successful heap-mode append (i.e., after
heap_[heapLen_++] = *s++ within append) ensure heap_[heapLen_] = '\0' (and also
set the terminator if append exits early or when ensureHeap(...) fails) so that
heap_ is always null-terminated; update the append method and any similar
heap-mode write sites (referencing append, ensureHeap, heap_, heapLen_, conn_)
to maintain the terminating '\0' after modifications.
- Around line 118-125: The jsonEscape function can write a null terminator past
the end when outMax == 0; modify jsonEscape to first handle zero-sized
destinations (return immediately if outMax == 0) and ensure you only write the
terminator inside bounds (e.g., set out[min(oi, outMax-1)] = '\0' or explicitly
check outMax>0 before assigning out[oi] = 0). Update the function jsonEscape to
use the outMax guard and bounded termination so oi and out writes never exceed
the provided buffer.

In `@src/core/Scheduler.h`:
- Around line 233-239: The function firstInTree can dereference a null mod and
mod->name(); add a null check at the top of firstInTree (e.g., if (!mod) return
nullptr) and ensure the loop retrieves each child into a variable (auto* child =
mod->child(i)) and checks child != nullptr before calling firstInTree(child,
name); also guard mod->name() by checking it only after confirming mod is
non-null.

🪄 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: 7735cba1-9857-42ef-9901-0dc4a347f64c

📥 Commits

Reviewing files that changed from the base of the PR and between 28e0e1f and 7eee4b3.

📒 Files selected for processing (24)

• CLAUDE.md
• CMakeLists.txt
• docs/history/plan-16.md
• docs/moonmodules/core/HttpServerModule.md
• docs/moonmodules/light/Layer.md
• docs/moonmodules/light/Layouts.md
• docs/moonmodules_draft/core/ui.md
• docs/plan.md
• esp32/main/CMakeLists.txt
• src/core/Base64.h
• src/core/FilesystemModule.h
• src/core/HttpServerModule.cpp
• src/core/HttpServerModule.h
• src/core/JsonSink.h
• src/core/Scheduler.h
• src/core/Sha1.h
• src/light/Layer.h
• src/light/Layers.h
• src/main.cpp
• src/ui/app.js
• test/CMakeLists.txt
• test/scenario_runner.cpp
• test/test_layers_container.cpp
• test/test_scheduler_unique_names.cpp

[coderabbitai]

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Restore mm_core as INTERFACE per root build contract.

mm_core is now a compiled STATIC target with non-platform .cpp sources, which breaks the repository’s root CMake rule for a header-only core and platform-only compilation units.

As per coding guidelines: “CMakeLists.txt: Root CMake build… Core library is INTERFACE (header-only). Only platform .cpp files need compilation.”

Also applies to: 45-45

🤖 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 `@CMakeLists.txt` around lines 14 - 19, The mm_core target was created as a
STATIC library including non-platform .cpp (HttpServerModule.cpp) which violates
the root CMake contract; change the mm_core target to an INTERFACE library
(remove src/core/HttpServerModule.cpp from add_library) so mm_core remains
header-only, keep target_include_directories(mm_core PUBLIC src/) and
target_link_libraries(mm_core PUBLIC mm_platform) as appropriate, and ensure any
platform-specific .cpp (e.g., HttpServerModule.cpp) are moved into the
platform-specific targets or compiled under the platform-specific CMakeLists
rather than added to mm_core; also preserve the existing
add_dependencies(mm_core ui_embed) placement after ui_embed is defined.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Align this migration plan with the root CMake contract.

This text instructs adding core service .cpp files into mm_core STATIC sources, which conflicts with the project’s root build rule for an INTERFACE core and platform-only compiled .cpp files.

As per coding guidelines: “CMakeLists.txt: Root CMake build… Core library is INTERFACE (header-only). Only platform .cpp files need compilation.”

🤖 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/plan.md` around lines 14 - 15, The plan incorrectly instructs adding
core service .cpp files to mm_core's STATIC sources in CMakeLists.txt, which
violates the root CMake contract that mm_core is an INTERFACE (header-only)
library and only platform .cpp files should be compiled; update the migration
plan text to instead: leave core modules header-only under mm_core (keep .h and
move implementation into platform-specific .cpp only when appropriate), do NOT
add core .cpp files to mm_core's STATIC sources, and when creating platform
implementations add them to the platform targets' source lists per the root
CMake contract (referencing mm_core, CMakeLists.txt, INTERFACE, STATIC, .cpp, .h
to locate the relevant lines).

[coderabbitai]

🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Use std::span for input/output buffers instead of pointer+length.

The public helper API currently uses raw pointer + length pairs, which violates the project rule and makes size contracts easier to misuse.

As per coding guidelines: “**/*.{h,hpp,cpp,cc}: Use std::span over pointer + length”.

🤖 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/Base64.h` at line 16, Change the public helper signature inline void
base64Encode(const uint8_t* in, size_t inLen, char* out, size_t outMax) to take
std::span for both input and output (e.g., std::span<const uint8_t> input,
std::span<char> output), add `#include` <span> in Base64.h, update the function
body to use input.size()/data() and output.size()/data() instead of
in/inLen/out/outMax, and update all call sites to pass spans (or construct spans
from buffers) so size contracts are enforced by the span bounds; keep the
function inline and preserve existing semantics and error handling.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Int16 load should also apply control-specific min/max bounds.

The type-limit clamping is good, but unlike the Uint8 case (lines 296-297), this doesn't apply c.min/c.max bounds. A persisted value of 150 would load into a control with max=100 without clamping.

Proposed fix

 case ControlType::Int16: {
     int v = mm::json::parseInt(json, key);
     // Clamp before narrowing — parseInt returns int (up to ±2^31).
     // A persisted-or-corrupted JSON value outside int16 range
     // would otherwise wrap (e.g. 40000 → -25536).
     if (v < INT16_MIN) v = INT16_MIN;
     if (v > INT16_MAX) v = INT16_MAX;
+    if (v < c.min) v = c.min;
+    if (v > c.max) v = c.max;
     *static_cast<int16_t*>(c.ptr) = static_cast<int16_t>(v);
     break;
 }

🤖 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/FilesystemModule.h` around lines 306 - 315, The Int16 case in the
load path (ControlType::Int16 using mm::json::parseInt and writing to c.ptr)
currently only clamps to int16 limits and ignores control-specific bounds
c.min/c.max; update the handler to, after clamping to INT16_MIN/INT16_MAX, also
clamp the parsed value to the control's configured bounds (use c.min and c.max
analogous to the Uint8 branch) before narrowing and assigning to
*static_cast<int16_t*>(c.ptr) so persisted values respect the control's min/max.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Reject or attach parentless module creation to scheduler ownership.

At Line 603, when parent_id is missing, the new module is never attached to a parent and never added to scheduler_, but it still runs onBuildControls/setup/onAllocateMemory. This creates an orphan allocation and unreachable runtime state.

Proposed fix

-    // Find parent and add as child
-    if (parentId[0] != 0) {
+    // Find parent and add as child (top-level adds are not supported here)
+    if (parentId[0] != 0) {
         auto* parent = findModuleByName(parentId);
         if (!parent) {
             delete mod;
             sendResponse(conn, 404, "application/json", "{\"error\":\"parent not found\"}");
             return;
         }
         if (!parent->addChild(mod)) {
             delete mod;
             sendResponse(conn, 400, "application/json", "{\"error\":\"parent rejected child\"}");
             return;
         }
+    } else {
+        delete mod;
+        sendResponse(conn, 400, "application/json", "{\"error\":\"parent_id required\"}");
+        return;
     }

Also applies to: 631-636

🧰 Tools

🪛 Clang (14.0.6)

[warning] 606-606: implicit conversion 'mm::MoonModule *' -> bool

(readability-implicit-bool-conversion)

---

[warning] 608-608: escaped string literal can be written as a raw string literal

(modernize-raw-string-literal)

---

[warning] 613-613: escaped string literal can be written as a raw string literal

(modernize-raw-string-literal)

🤖 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 603 - 617, When parentId is empty
the new module (mod) is left unattached and still proceeds through
onBuildControls/onSetup/onAllocateMemory, causing an orphan; modify the creation
flow so that if findModuleByName(parentId) is not used (parentId[0] == 0) you
explicitly attach or register the module with scheduler_ (e.g., call the
scheduler_ method that owns/records modules such as
scheduler_->addModule/registerModule or set scheduler_ as the module's owner)
before any build/setup/allocation steps, and if that scheduler attachment fails
undo/delete mod and return an error; apply the same change to the similar block
around the other creation path (the 631-636 region) to avoid unreachable runtime
state for parentless modules.

[coderabbitai]

Actionable comments posted: 6

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

src/core/Base64.h (1)

30-30: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Terminator write can still overflow when buffer fills.

The guard on line 17 prevents the crash when outMax == 0, but the terminator write out[oi] = 0 on line 30 can still write out of bounds when the loop fills the buffer. After the loop exits with oi + 4 >= outMax, the final iteration can leave oi == outMax, making out[oi] an out-of-bounds write.

🛡️ Proposed fix

     }
-    out[oi] = 0;
+    if (oi < outMax) out[oi] = 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/Base64.h` at line 30, The null terminator write out[oi] = 0 can
overflow when the encoding loop fills the buffer; update the logic so the
terminator is only written when there is space (e.g., ensure the loop uses
outMax-1 as the maximum writable bytes or check oi < outMax before writing the
terminator). Locate the variables/out access in Base64 code (symbols: out, oi,
outMax, the loop that emits 4 bytes per input block) and either adjust the loop
boundary to reserve one byte for the terminator or guard the final write with a
conditional that writes into out[oi] only if oi < outMax (otherwise write the
terminator into out[outMax-1] or skip it as appropriate).

src/core/JsonSink.h (1)

3-3: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift

Keep core infrastructure free of platform includes.

src/core/JsonSink.h directly depends on platform::TcpConnection, which breaks the core/platform separation contract.

As per coding guidelines "src/core/**: Core infrastructure ... Must be platform-independent — no platform includes."

Also applies to: 22-22

🤖 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/JsonSink.h` at line 3, JsonSink.h must not include
platform/platform.h or depend on platform::TcpConnection directly; remove the
include and the direct reference to platform::TcpConnection in the JsonSink
class and replace it with a platform-agnostic, core-facing abstraction (e.g., a
forward-declared interface such as class IConnection or Connection) used via
pointer/reference in JsonSink, then implement an adapter in the platform layer
that wraps platform::TcpConnection and implements that core interface so
JsonSink only depends on the abstract symbol (JsonSink, the
Connection/IConnection type) and not platform::TcpConnection.

♻️ Duplicate comments (1)

src/core/HttpServerModule.cpp (1)

615-629: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Reject parentless creates or attach them to scheduler ownership.

When parent_id is empty, the module is still initialized (onBuildControls/setup/onAllocateMemory) but never attached to a parent or scheduler-owned root, creating unreachable allocated state.

Proposed fix

-    // Find parent and add as child
-    if (parentId[0] != 0) {
+    // Find parent and add as child (top-level creation is not supported here)
+    if (parentId[0] != 0) {
         auto* parent = findModuleByName(parentId);
         if (!parent) {
             delete mod;
             sendResponse(conn, 404, "application/json", "{\"error\":\"parent not found\"}");
             return;
         }
         if (!parent->addChild(mod)) {
             delete mod;
             sendResponse(conn, 400, "application/json", "{\"error\":\"parent rejected child\"}");
             return;
         }
+    } else {
+        delete mod;
+        sendResponse(conn, 400, "application/json", "{\"error\":\"parent_id required\"}");
+        return;
     }

Also applies to: 643-646

🤖 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 615 - 629, The code currently
initializes a module (mod) even when parentId is empty, leaving it unattached
and leaking state; update the create/attach logic in the block around
findModuleByName/addChild so that when parentId is empty you either reject the
request (delete mod; call sendResponse with a 400/422 and an error JSON) or
explicitly attach mod to the scheduler-owned root (call the scheduler/rootOwner
addChild API instead of skipping attachment); apply the same change to the
similar block at the other occurrence (lines referenced in the review) and
ensure cleanup (delete mod) and an appropriate sendResponse are used on failure
or when rejecting parentless creates.

🤖 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/architecture.md`:
- Around line 144-164: The fenced code block containing the ASCII architecture
diagram should include an explicit language to satisfy MD040; change the opening
triple backticks from ``` to ```text (or another explicit language like
```ascii) so the block becomes ```text and keep the same closing ```; update the
block surrounding the diagram that references Layouts, GridLayout, WheelLayout,
Layers (Layer A/Layer B/Layer C), Blend+Map and Drivers (WS2812Driver,
ArtNetDriver, PreviewDriver) accordingly.

In `@docs/building.md`:
- Around line 28-44: The fenced code block in docs/building.md is missing a
language tag (causing MD040); update the opening fence from ``` to ```text so
the block becomes a labeled text block and the markdown linter passes—locate the
directory tree block (the triple-backtick fenced block containing
CMakeLists.txt, src/, platform/, esp32/, sdkconfig.defaults) and change only the
opening fence to include the "text" language tag.

In `@src/core/HttpServerModule.cpp`:
- Around line 683-686: The current delete sequence calls mod->teardown(); delete
mod; which leaks descendant modules for container modules; update the delete
handler to mirror the replace flow by invoking teardown() then recursively
freeing the entire subtree (e.g., traverse mod's children, call the same
recursive delete used in the replace logic for each child, then delete the node
itself) so all descendants are deleted instead of only the root module; locate
the delete site that calls mod->teardown() and delete mod and replace it with
the teardown + recursive descendant deletion pattern used in the replace flow.

In `@test/test_phase_animation.cpp`:
- Around line 46-50: The test currently uses real wall-clock sleeps via
mm::platform::delayMs inside the loop which makes tests non-deterministic;
replace those calls with a deterministic advance of a virtual/test clock or stub
mm::platform::delayMs to a no-op and instead simulate elapsed time by calling
layer.loop() the needed number of iterations (or invoke a provided tick/advance
method) and compare layer.buffer() to baseline; update both occurrences around
the loop that calls layer.loop() and the similar block later (where baseline is
compared) so tests no longer depend on real time and use the injected/mock clock
or simple loop iterations to emulate passage of time.
- Around line 92-94: The test calls the lifecycle methods in the wrong order;
change the sequence in the replace test so the runtime-add semantics invoke
fresh->setup() first, then fresh->onBuildControls(), and finally
fresh->onAllocateMemory(); update the call order around
fresh->onBuildControls(), fresh->setup(), fresh->onAllocateMemory() to match
setup() → onBuildControls() → onAllocateMemory() as required by MoonModule
runtime-add rules.

In `@test/test_stateless_effects.cpp`:
- Around line 125-129: Test is flaky due to real-time sleep; remove dependency
on mm::platform::delayMs and make the check deterministic by iterating a fixed
number of loops and asserting based on those iterations. Replace the
sleep-backed loop that calls ctx.layer.loop() and checks
ctx.hasTwoDistinctColors() (and sets varied) with a tight loop of N
deterministic iterations (e.g., 10 or configurable constant) without calling
mm::platform::delayMs, or, if available, advance a mock/virtual clock before
each ctx.layer.loop() call; ensure you reference the same ctx.layer.loop(),
ctx.hasTwoDistinctColors(), and the varied variable so the test no longer relies
on real time.

---

Outside diff comments:
In `@src/core/Base64.h`:
- Line 30: The null terminator write out[oi] = 0 can overflow when the encoding
loop fills the buffer; update the logic so the terminator is only written when
there is space (e.g., ensure the loop uses outMax-1 as the maximum writable
bytes or check oi < outMax before writing the terminator). Locate the
variables/out access in Base64 code (symbols: out, oi, outMax, the loop that
emits 4 bytes per input block) and either adjust the loop boundary to reserve
one byte for the terminator or guard the final write with a conditional that
writes into out[oi] only if oi < outMax (otherwise write the terminator into
out[outMax-1] or skip it as appropriate).

In `@src/core/JsonSink.h`:
- Line 3: JsonSink.h must not include platform/platform.h or depend on
platform::TcpConnection directly; remove the include and the direct reference to
platform::TcpConnection in the JsonSink class and replace it with a
platform-agnostic, core-facing abstraction (e.g., a forward-declared interface
such as class IConnection or Connection) used via pointer/reference in JsonSink,
then implement an adapter in the platform layer that wraps
platform::TcpConnection and implements that core interface so JsonSink only
depends on the abstract symbol (JsonSink, the Connection/IConnection type) and
not platform::TcpConnection.

---

Duplicate comments:
In `@src/core/HttpServerModule.cpp`:
- Around line 615-629: The code currently initializes a module (mod) even when
parentId is empty, leaving it unattached and leaking state; update the
create/attach logic in the block around findModuleByName/addChild so that when
parentId is empty you either reject the request (delete mod; call sendResponse
with a 400/422 and an error JSON) or explicitly attach mod to the
scheduler-owned root (call the scheduler/rootOwner addChild API instead of
skipping attachment); apply the same change to the similar block at the other
occurrence (lines referenced in the review) and ensure cleanup (delete mod) and
an appropriate sendResponse are used on failure or when rejecting parentless
creates.

🪄 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: 8a32d3a1-b0c7-421c-bd6d-2e16052234fc

📥 Commits

Reviewing files that changed from the base of the PR and between 7eee4b3 and 67579f5.

📒 Files selected for processing (39)

• CLAUDE.md
• README.md
• docs/architecture-light.md
• docs/architecture.md
• docs/building.md
• docs/coding-standards.md
• docs/moonmodules/core/HttpServerModule.md
• docs/moonmodules/core/MoonModule.md
• docs/moonmodules/core/ui.md
• docs/moonmodules/light/Drivers.md
• docs/moonmodules/light/EffectBase.md
• docs/moonmodules/light/Layer.md
• docs/moonmodules/light/Layouts.md
• docs/moonmodules/light/MappingLUT.md
• docs/moonmodules_draft/core/ui.md
• docs/testing.md
• scripts/check/collect_kpi.py
• src/core/Base64.h
• src/core/Control.h
• src/core/FilesystemModule.h
• src/core/HttpServerModule.cpp
• src/core/JsonSink.h
• src/core/MoonModule.h
• src/core/Scheduler.h
• src/light/Layer.h
• src/light/drivers/Drivers.h
• src/light/effects/CheckerboardEffect.h
• src/light/effects/LavaLampEffect.h
• src/light/effects/MetaballsEffect.h
• src/light/effects/SpiralEffect.h
• src/light/layouts/Layouts.h
• test/CMakeLists.txt
• test/test_drivers_container.cpp
• test/test_effects_zero_grid.cpp
• test/test_filesystem_persistence.cpp
• test/test_layout_toggle_cycle.cpp
• test/test_layouts_container.cpp
• test/test_phase_animation.cpp
• test/test_stateless_effects.cpp

💤 Files with no reviewable changes (1)

• docs/architecture-light.md

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add a language to the fenced code block to satisfy markdown lint.

The block opens with plain and triggers MD040; set it totext (or another explicit language) to keep docs lint-clean.

🧰 Tools

🪛 markdownlint-cli2 (0.22.1)

[warning] 144-144: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🤖 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/architecture.md` around lines 144 - 164, The fenced code block
containing the ASCII architecture diagram should include an explicit language to
satisfy MD040; change the opening triple backticks from ``` to ```text (or
another explicit language like ```ascii) so the block becomes ```text and keep
the same closing ```; update the block surrounding the diagram that references
Layouts, GridLayout, WheelLayout, Layers (Layer A/Layer B/Layer C), Blend+Map
and Drivers (WS2812Driver, ArtNetDriver, PreviewDriver) accordingly.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add a language tag to the fenced code block.

Line 28 triggers MD040; adding a language keeps docs lint-clean.

Proposed fix

-```
+```text
 CMakeLists.txt                          ← standard CMake: desktop / RPi + tests
 src/
   main.cpp                              ← shared pipeline wiring (mm_main), platform-neutral
@@
   sdkconfig.defaults                    ← board-specific defaults
-```
+```

🧰 Tools

🪛 markdownlint-cli2 (0.22.1)

[warning] 28-28: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🤖 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/building.md` around lines 28 - 44, The fenced code block in
docs/building.md is missing a language tag (causing MD040); update the opening
fence from ``` to ```text so the block becomes a labeled text block and the
markdown linter passes—locate the directory tree block (the triple-backtick
fenced block containing CMakeLists.txt, src/, platform/, esp32/,
sdkconfig.defaults) and change only the opening fence to include the "text"
language tag.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Delete handler should free the full subtree, not only the root node.

delete mod after teardown() can leak descendants for container modules. Use the same teardown + recursive delete pattern already used in replace flow.

Proposed fix

-    // Teardown and delete
+    // Teardown and delete full subtree
     mod->teardown();
-    delete mod;
+    Scheduler::deleteTree(mod);

🤖 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 683 - 686, The current delete
sequence calls mod->teardown(); delete mod; which leaks descendant modules for
container modules; update the delete handler to mirror the replace flow by
invoking teardown() then recursively freeing the entire subtree (e.g., traverse
mod's children, call the same recursive delete used in the replace logic for
each child, then delete the node itself) so all descendants are deleted instead
of only the root module; locate the delete site that calls mod->teardown() and
delete mod and replace it with the teardown + recursive descendant deletion
pattern used in the replace flow.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Avoid wall-clock delays in unit tests.

Line 47 and Line 100 make these tests depend on real elapsed time, which introduces CI flakiness and breaks the test contract for deterministic tests.

As per coding guidelines "test/**: Tests should not depend on timing or network."

Also applies to: 100-103

🧰 Tools

🪛 Clang (14.0.6)

[warning] 50-50: statement should be inside braces

(readability-braces-around-statements)

🪛 Cppcheck (2.20.0)

[style] 50-50: The function 'activeLayer' 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 `@test/test_phase_animation.cpp` around lines 46 - 50, The test currently uses
real wall-clock sleeps via mm::platform::delayMs inside the loop which makes
tests non-deterministic; replace those calls with a deterministic advance of a
virtual/test clock or stub mm::platform::delayMs to a no-op and instead simulate
elapsed time by calling layer.loop() the needed number of iterations (or invoke
a provided tick/advance method) and compare layer.buffer() to baseline; update
both occurrences around the loop that calls layer.loop() and the similar block
later (where baseline is compared) so tests no longer depend on real time and
use the injected/mock clock or simple loop iterations to emulate passage of
time.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Fix runtime-add lifecycle order in the replace test.

Line 92-94 uses onBuildControls() -> setup() -> onAllocateMemory(), but runtime add semantics are setup() -> onBuildControls() -> onAllocateMemory().

Proposed fix

-    fresh->onBuildControls();
-    fresh->setup();
+    fresh->setup();
+    fresh->onBuildControls();
     fresh->onAllocateMemory();

As per coding guidelines "test/**: Verify tests cover edge cases and match the specifications in docs/moonmodules/." and docs/moonmodules/core/MoonModule.md states runtime add must call setup() → onBuildControls() → onAllocateMemory().

📝 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

	fresh->onBuildControls();
	fresh->setup();
	fresh->onAllocateMemory();
	fresh->setup();
	fresh->onBuildControls();
	fresh->onAllocateMemory();

🧰 Tools

🪛 Cppcheck (2.20.0)

[style] 94-94: The function 'addSelect' is never used.

(unusedFunction)

---

[style] 94-94: The function 'setEnabled' is never used.

(unusedFunction)

---

[style] 94-94: The function 'forEachDestination' 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 `@test/test_phase_animation.cpp` around lines 92 - 94, The test calls the
lifecycle methods in the wrong order; change the sequence in the replace test so
the runtime-add semantics invoke fresh->setup() first, then
fresh->onBuildControls(), and finally fresh->onAllocateMemory(); update the call
order around fresh->onBuildControls(), fresh->setup(), fresh->onAllocateMemory()
to match setup() → onBuildControls() → onAllocateMemory() as required by
MoonModule runtime-add rules.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Make LavaLamp spatial-variation test deterministic.

Line 128 depends on real-time sleep, so this test can be flaky across environments.

As per coding guidelines "test/**: Tests should not depend on timing or network."

🧰 Tools

🪛 Clang (14.0.6)

[warning] 127-127: statement should be inside braces

(readability-braces-around-statements)

🤖 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/test_stateless_effects.cpp` around lines 125 - 129, Test is flaky due to
real-time sleep; remove dependency on mm::platform::delayMs and make the check
deterministic by iterating a fixed number of loops and asserting based on those
iterations. Replace the sleep-backed loop that calls ctx.layer.loop() and checks
ctx.hasTwoDistinctColors() (and sets varied) with a tight loop of N
deterministic iterations (e.g., 10 or configurable constant) without calling
mm::platform::delayMs, or, if available, advance a mock/virtual clock before
each ctx.layer.loop() call; ensure you reference the same ctx.layer.loop(),
ctx.hasTwoDistinctColors(), and the varied variable so the test no longer relies
on real time.

[ewowi]

@coderabbitai, feedback on your review (will be added in next commit(

• 2 fixes applied: architecture.md:144 → text, building.md:28 → text. Pure doc, lint-only.
• 7 findings skipped with reasons given above. Notably:
  • The delete-leaks-descendants finding cites the wrong line range (early-return at 683-686 vs actual recursive Scheduler::deleteTree at 691-700).
  • The lifecycle-order finding is backwards (current order matches Scheduler::setup phases).
  • The wall-clock-sleep findings would break the tests they "fix" (elapsed() wouldn't advance).
  • The platform-dependency finding misreads where the platform boundary lives.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@ewowi — thanks for the clear breakdown.

Fixes acknowledged:
Both architecture.md:144 and building.md:28 code-fence corrections (→ \``text`) are valid lint-only doc improvements.

Skips accepted — notable corrections noted:

• delete-leaks-descendants — you're right that I cited the wrong line range. The early-return guard at 683–686 is unrelated to the actual recursive Scheduler::deleteTree at 691–700. That's a misread on my part; the finding was anchored to the wrong code.
• lifecycle-order — acknowledged: the finding was backwards. The current teardown/setup ordering already matches the Scheduler::setup phase contract; flagging it as wrong was incorrect.
• wall-clock-sleep — a fair counter: replacing platform::micros()-based elapsed tracking with real wall-clock sleeps in tests would break elapsed-time-dependent assertions, since elapsed() wouldn't advance. The existing approach is correct for the test harness.
• platform-dependency — acknowledged: I misidentified where the platform boundary lives in this codebase. The actual boundary enforcement is in src/platform/, not where I flagged it.

🐇 📝

---

✏️ Learnings added

Learnt from: ewowi
URL: https://github.com/ewowi/projectMM/pull/4

Timestamp: 2026-05-24T18:27:24.031Z
Learning: In projectMM (ewowi/projectMM), `Scheduler::deleteTree` is the recursive descendant-cleanup function located around line 691-700 of `src/core/Scheduler.h` (or HttpServerModule.cpp). The early-return guard at lines 683-686 is a separate top-level-module rejection check and must not be confused with the actual deletion logic. Do not cite the early-return guard as evidence of a descendant-leak.

---

Learnt from: ewowi
URL: https://github.com/ewowi/projectMM/pull/4

Timestamp: 2026-05-24T18:27:24.031Z
Learning: In projectMM (ewowi/projectMM), the module lifecycle teardown/setup ordering in HttpServerModule is intentional and already matches the Scheduler::setup phase contract. Do not flag the lifecycle ordering as incorrect — it is correct by design.

---

Learnt from: ewowi
URL: https://github.com/ewowi/projectMM/pull/4

Timestamp: 2026-05-24T18:27:24.031Z
Learning: In projectMM (ewowi/projectMM), test files use `platform::micros()`-based elapsed time rather than real wall-clock sleeps. Replacing this with actual sleeps would break elapsed()-dependent assertions because `elapsed()` would not advance. Do not suggest switching to wall-clock sleeps in the test harness.

---

Learnt from: ewowi
URL: https://github.com/ewowi/projectMM/pull/4

Timestamp: 2026-05-24T18:27:24.031Z
Learning: In projectMM (ewowi/projectMM), the platform boundary is enforced via the `src/platform/` directory. Platform-specific `#ifdef` guards and hardware API calls are restricted to files under `src/platform/`. Do not flag cross-platform usage at other call sites as a platform-boundary violation unless the actual hardware/platform API is being called outside `src/platform/`.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.