Crash-safe module ops, modifier overhaul, Windows desktop port

[ewowi]

This branch spans three themes that grew together over the iteration. All commit + pre-merge gates pass; both review rounds (👾 Reviewer over the whole branch, 🐇 CodeRabbit) are processed.

1. Crash-safe module lifecycle + scenario tooling

• Delete/rebuild never crashes. Deleting a Layer used to leave drivers holding a dangling pointer (→ LoadProhibited boot loop). Drivers now re-resolve the active Layer from the Layers container at every buildState and tolerate null; Layers re-propagates Layouts to children. Regression-tested.
• clear_children scenario op — a scenario clears a container and rebuilds its own canvas from any starting tree; replace_module over the live API; remove_module/delete_module aliased so the in-process and live runners can't diverge. Non-editable submodules (Board, Preview, Improv) are refused by delete/replace, enforced at the API.
• scenario_AllEffects_grid_sizes — every effect (no modifier) across 16/32/64/128, measuring tick/FPS/heap/block.

2. Modifier overhaul

• MultiplyModifier replaces MirrorModifier — per-axis tile (multiplyX/Y/Z, 1–64) + per-axis mirrorX/Y/Z. The kaleidoscope superset: a pure mirror is multiply-2-with-mirror. Effective multiplier clamps to the axis extent, so multiplyZ on a 2D layout is a no-op (not a blank).
• CheckerboardModifier — a mask (size + invert) that drops lights via outCount=0, no ModifierBase contract change.
• Fan-out limited only by memory — the LUT scratch buffer is sized dynamically to the modifier's fan-out (clamped to the box), not a fixed cap; OOM degrades to the identity LUT.
• Layout status lines — Layouts shows physical lights + bounding box, Layer shows the logical render box, via the MoonModule status slot (no new controls).
• UI — "+ add module" rename; drag-reorder stopPropagation fix (a drop no longer bubbles to ancestor cards and undoes itself); documented insert-not-swap reorder semantics.

3. Windows desktop port

• Windows desktop build + ESP-IDF-on-Windows path; CI Windows/macOS build fixes; MoonDeck port enum + installer hints. (Implemented + tested on a Windows machine.)

Notable bug fixes from hardware testing

• uint16 overflow black-screen (no-PSRAM): a high fan-out logicalCount × maxMultiplier (and maxMultiplier itself) overflowed uint16, wrapping the LUT size to ~0 → black display. Both now compute in uint64 + saturate. Invisible on the uint32 desktop build — caught on the Olimex.
• Device-created names with spaces (ensureUniqueName "Grid 2") were API-unaddressable. Root-caused: the disambiguating separator is now - (URL-safe), and the live runner URL-encodes names.

Verification

Commit gates green on every commit; pre-merge: Reviewer (1 blocker fixed), CodeRabbit (7 fixed / 3 skipped-with-reason), docs sync, carry-forward lesson, permission review. Modifier fixes verified on the Olimex (esp32-eth) hardware. ESP32 KPI captured; one live-perf snapshot deferred during a session where the board's Ethernet PHY was intermittently flapping (a documented hardware issue — firmware ruled out; backlog).

🤖 Generated with Claude Code

[coderabbitai]

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

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

[coderabbitai]

Actionable comments posted: 2

Caution

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

⚠️ Outside diff range comments (4)

src/core/HttpServerModule.cpp (1)

727-738: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Enforce userEditable() in replace endpoint as well.

Line 727+ validates top-level replacement, but there is no userEditable() check in handleReplaceModule. That means protected apparatus modules can still be replaced through API even though the editability contract covers delete and replace.

Suggested fix

 void HttpServerModule::handleReplaceModule(platform::TcpConnection& conn, const char* moduleName, const char* body) {
     auto* mod = findModuleByName(moduleName);
     if (!mod) {
         sendResponse(conn, 404, "application/json", "{\"error\":\"module not found\"}");
         return;
     }
     auto* parent = mod->parent();
     if (!parent) {
         sendResponse(conn, 400, "application/json", "{\"error\":\"top-level modules cannot be replaced\"}");
         return;
     }
+    if (!mod->userEditable()) {
+        sendResponse(conn, 400, "application/json", "{\"error\":\"module not replaceable\"}");
+        return;
+    }
     char typeName[32] = {};
     mm::json::parseString(body, "type", typeName, sizeof(typeName));

Also applies to: 758-763

🤖 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 727 - 738, The replace endpoint
currently skips the userEditable() guard; update handleReplaceModule to call
mod->userEditable() (or the module object's userEditable method) after resolving
mod/parent and before proceeding with replacement, and if it returns false send
a 403 JSON response (e.g. "{\"error\":\"module not editable\"}") and return;
apply the same check in the second replace path referenced around the 758-763
region so both code paths enforce the editability contract.

scripts/scenario/run_live_scenario.py (1)

600-604: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

--update-contract path uses unhashable dict keys and can crash.

At Line 600 and Line 687, (step, target) is used as a dict key, but step is a Python dict (unhashable). This raises TypeError: unhashable type: 'dict' during contract update/rollback.

Suggested fix (key by step index instead of step dict)

-    for step in scenario.get("steps", []):
+    for step_idx, step in enumerate(scenario.get("steps", [])):
         step_name = step.get("name", "?")
         op = step.get("op", "")
         step_result = {"name": step_name, "op": op}
@@
             if update_contract:
                 # Preserve any per-step tolerance overrides already in place.
                 existing = step.get("contract", {}).get(target, {})
-                if (step, target) not in pending_contract_originals:
+                key = (step_idx, target)
+                if key not in pending_contract_originals:
                     # Deep enough copy: existing is a flat dict of scalars.
-                    pending_contract_originals[(step, target)] = (
+                    pending_contract_originals[key] = (
                         dict(existing) if existing else None
                     )
@@
-        for step in scenario.get("steps", []):
+        for step_idx, step in enumerate(scenario.get("steps", [])):
             contract = step.get("contract")
             if contract and target in contract:
-                if (step, target) in pending_contract_originals:
-                    orig = pending_contract_originals[(step, target)]
+                key = (step_idx, target)
+                if key in pending_contract_originals:
+                    orig = pending_contract_originals[key]
                     if orig is None:
                         del contract[target]
                     else:
                         contract[target] = orig

Also applies to: 687-688

🤖 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/scenario/run_live_scenario.py` around lines 600 - 604, The bug is
that pending_contract_originals is keyed by (step, target) where step is an
unhashable dict; change the key to use the step index instead (e.g., use the
loop index or an explicit step_index variable) so keys are hashable; update all
references that create or access pending_contract_originals (the lines that set
pending_contract_originals[(step, target)] and the rollback/access at lines
corresponding to 687-688) to use (step_index, target) consistently, ensuring any
places that previously passed the step dict now use the index or retrieve the
step dict from the steps list by that index.

src/main.cpp (1)

197-212: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Update the PreviewDriver wiring comment.

The code now binds Drivers through setLayers(layersContainer), so the nearby setLayer wording is no longer true.

📝 Suggested fix

-    // PreviewDriver reads the active Layer (via Drivers' setLayer wiring) for the
-    // light positions and the sparse driver buffer it streams; it owns its own
+    // PreviewDriver reads the active Layer resolved by Drivers from the bound
+    // Layers container for the light positions and sparse driver buffer it
+    // streams; it owns its own

As per coding guidelines, comments in src/**/*.{h,hpp,cpp,js,ts,tsx} should only be changed when they are outdated or factually wrong, and this one is now factually wrong after the wiring change.

🤖 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 197 - 212, The comment near PreviewDriver is
outdated: it still says "via Drivers' setLayer wiring" but the code now calls
drivers->setLayers(layersContainer); update the comment to reference the actual
wiring (e.g., "via the Drivers container's setLayers(layersContainer) wiring")
and ensure any other phrasing that implied setLayer or an externally-owned frame
is corrected to match PreviewDriver and Drivers (PreviewDriver,
Drivers::setLayers, layersContainer).

Source: Coding guidelines

test/scenario_runner.cpp (1)

528-583: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Purge deleted subtrees from ctx.modules, not just the direct child.

Both delete paths remove only the root entry before calling deleteTree(). If the removed module owns descendants, those descendant ids stay in ctx.modules pointing at freed objects, so a later set_control, remove_module, or re-add can operate on dangling memory instead of the rebuilt subtree.

🤖 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/scenario_runner.cpp` around lines 528 - 583, The delete paths
(remove_module handling around symbols target, parent->removeChild,
target->teardown, mm::Scheduler::deleteTree and the clear_children loop for
childMod) only erase the root id from ctx.modules leaving descendant ids
pointing at freed memory; fix by purging all ctx.modules entries whose stored
pointer lies inside the deleted subtree before calling
mm::Scheduler::deleteTree. Implement this by iterating ctx.modules and erasing
any entry where the value is equal to or is a descendant of the subtree root
(e.g., add a helper isDescendant(subtreeRoot, node) or collect pointers from the
subtree and remove matching map entries) in both the remove_module branch (use
target as subtreeRoot) and the clear_children branch (use childMod as
subtreeRoot) prior to deleteTree().

🤖 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 `@CLAUDE.md`:
- Line 118: The ordered list prefix on the "Commit message format" item uses
"8." which triggers MD029; update the numeric prefix (the leading "8." in that
list item) to the "1." style expected by the markdownlint rule (or change all
ordered-list items in that block to use the "1." prefix) so the list follows the
1/1/1 ordered-list-prefix style; ensure the rest of the line text remains
unchanged (look for the "Commit message format" list entry and the leading
numeric prefix).

In `@docs/backlog/backlog.md`:
- Line 20: The doc comment incorrectly states the code uses writev in
TcpConnection::writeChunks; update the documentation to say the implementation
calls sendmsg(fd_, &msg, MSG_DONTWAIT) with an iovec[] in platform_desktop.cpp
(rather than writev) and explain the correct Windows mapping is WSASend with
WSABUF[] or a coalesced send while preserving the non-blocking partial-write
semantics.

---

Outside diff comments:
In `@scripts/scenario/run_live_scenario.py`:
- Around line 600-604: The bug is that pending_contract_originals is keyed by
(step, target) where step is an unhashable dict; change the key to use the step
index instead (e.g., use the loop index or an explicit step_index variable) so
keys are hashable; update all references that create or access
pending_contract_originals (the lines that set pending_contract_originals[(step,
target)] and the rollback/access at lines corresponding to 687-688) to use
(step_index, target) consistently, ensuring any places that previously passed
the step dict now use the index or retrieve the step dict from the steps list by
that index.

In `@src/core/HttpServerModule.cpp`:
- Around line 727-738: The replace endpoint currently skips the userEditable()
guard; update handleReplaceModule to call mod->userEditable() (or the module
object's userEditable method) after resolving mod/parent and before proceeding
with replacement, and if it returns false send a 403 JSON response (e.g.
"{\"error\":\"module not editable\"}") and return; apply the same check in the
second replace path referenced around the 758-763 region so both code paths
enforce the editability contract.

In `@src/main.cpp`:
- Around line 197-212: The comment near PreviewDriver is outdated: it still says
"via Drivers' setLayer wiring" but the code now calls
drivers->setLayers(layersContainer); update the comment to reference the actual
wiring (e.g., "via the Drivers container's setLayers(layersContainer) wiring")
and ensure any other phrasing that implied setLayer or an externally-owned frame
is corrected to match PreviewDriver and Drivers (PreviewDriver,
Drivers::setLayers, layersContainer).

In `@test/scenario_runner.cpp`:
- Around line 528-583: The delete paths (remove_module handling around symbols
target, parent->removeChild, target->teardown, mm::Scheduler::deleteTree and the
clear_children loop for childMod) only erase the root id from ctx.modules
leaving descendant ids pointing at freed memory; fix by purging all ctx.modules
entries whose stored pointer lies inside the deleted subtree before calling
mm::Scheduler::deleteTree. Implement this by iterating ctx.modules and erasing
any entry where the value is equal to or is a descendant of the subtree root
(e.g., add a helper isDescendant(subtreeRoot, node) or collect pointers from the
subtree and remove matching map entries) in both the remove_module branch (use
target as subtreeRoot) and the clear_children branch (use childMod as
subtreeRoot) prior to deleteTree().

🪄 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: 9e54edc3-5303-46af-bf87-b9ac98bcfd74

📥 Commits

Reviewing files that changed from the base of the PR and between b23664d and ecd47ce.

📒 Files selected for processing (20)

• CLAUDE.md
• README.md
• docs/architecture.md
• docs/backlog/backlog.md
• docs/history/FastLED.md
• docs/history/NightDriverStrip.md
• docs/history/README.md
• docs/history/WLED-MM.md
• docs/history/WLED.md
• docs/performance.md
• scripts/scenario/run_live_scenario.py
• src/core/HttpServerModule.cpp
• src/core/ImprovProvisioningModule.h
• src/light/drivers/Drivers.h
• src/light/layers/Layers.h
• src/main.cpp
• test/scenario_runner.cpp
• test/scenarios/light/scenario_AllEffects_grid_sizes.json
• test/scenarios/light/scenario_Layouts_mutation.json
• test/unit/light/unit_PreviewDriver.cpp

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Fix ordered-list prefix style to satisfy MD029.

Line 118 uses 8. while your markdownlint configuration expects 1/1/1 list-prefix style, so this keeps the warning active.

Suggested fix

-8. Commit message format (the structure below uses hard newlines *between* its parts — title, summary, KPI line, bullets are each their own line. But do **not** hard-wrap *within* a part: the summary paragraph and each bullet are a single unbroken line that the viewer soft-wraps — same reasoning as the no-hard-wraps-in-markdown rule in [coding-standards.md](docs/coding-standards.md), keeps diffs clean and renders correctly on GitHub. Only the title obeys a length cap; everything else runs as long as it needs to on one line):
+1. Commit message format (the structure below uses hard newlines *between* its parts — title, summary, KPI line, bullets are each their own line. But do **not** hard-wrap *within* a part: the summary paragraph and each bullet are a single unbroken line that the viewer soft-wraps — same reasoning as the no-hard-wraps-in-markdown rule in [coding-standards.md](docs/coding-standards.md), keeps diffs clean and renders correctly on GitHub. Only the title obeys a length cap; everything else runs as long as it needs to on one line):

📝 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

8. Commit message format (the structure below uses hard newlines *between* its parts — title, summary, KPI line, bullets are each their own line. But do **not** hard-wrap *within* a part: the summary paragraph and each bullet are a single unbroken line that the viewer soft-wraps — same reasoning as the no-hard-wraps-in-markdown rule in [coding-standards.md](docs/coding-standards.md), keeps diffs clean and renders correctly on GitHub. Only the title obeys a length cap; everything else runs as long as it needs to on one line):

1. Commit message format (the structure below uses hard newlines *between* its parts — title, summary, KPI line, bullets are each their own line. But do **not** hard-wrap *within* a part: the summary paragraph and each bullet are a single unbroken line that the viewer soft-wraps — same reasoning as the no-hard-wraps-in-markdown rule in [coding-standards.md](docs/coding-standards.md), keeps diffs clean and renders correctly on GitHub. Only the title obeys a length cap; everything else runs as long as it needs to on one line):

🧰 Tools

🪛 markdownlint-cli2 (0.22.1)

[warning] 118-118: Ordered list item prefix
Expected: 1; Actual: 8; Style: 1/1/1

(MD029, ol-prefix)

🤖 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 `@CLAUDE.md` at line 118, The ordered list prefix on the "Commit message
format" item uses "8." which triggers MD029; update the numeric prefix (the
leading "8." in that list item) to the "1." style expected by the markdownlint
rule (or change all ordered-list items in that block to use the "1." prefix) so
the list follows the 1/1/1 ordered-list-prefix style; ensure the rest of the
line text remains unchanged (look for the "Commit message format" list entry and
the leading numeric prefix).

Source: Linters/SAST tools

[coderabbitai]

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 (1)

test/scenario_runner.cpp (1)

594-624: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Purge old descendant ids during replace_module to avoid dangling pointers.

Line 622 deletes old, but descendant ids from old can remain in ctx.modules. Later set_control/delete steps can dereference freed pointers.

Suggested fix

             mm::MoonModule* old = parent->replaceChildAt(index, fresh);
             fresh->onBuildControls();
             fresh->setup();
             fresh->onBuildState();
-            if (old) { old->teardown(); mm::Scheduler::deleteTree(old); }
-            ctx.modules[targetId] = fresh;
+            if (old) {
+                old->teardown();
+                ctx.purgeSubtree(old);  // drop old + descendant id mappings before free
+                mm::Scheduler::deleteTree(old);
+            }
+            ctx.modules[targetId] = 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 `@test/scenario_runner.cpp` around lines 594 - 624, When replacing a module in
the replace_module branch, you must remove any ctx.modules entries that point to
the old module or any of its descendants before calling
mm::Scheduler::deleteTree(old); update the handler that uses
parent->replaceChildAt(...) / old to traverse the subtree rooted at old, collect
all scenario ids stored in ctx.modules whose pointer equals old or any
descendant, erase those keys from ctx.modules, then proceed with old->teardown()
and mm::Scheduler::deleteTree(old); this prevents dangling pointers when later
set_control/remove steps look up those ids.

🤖 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/building.md`:
- Around line 91-94: The fenced code blocks under the "**macOS / Linux:**" and
the "**Windows** (PowerShell — run once with admin...)" headings are missing a
blank line before the opening ``` and need a blank line after the closing ``` to
satisfy markdownlint MD031; edit the docs/building.md content around the
"**macOS / Linux:**" and "**Windows**" sections (and the other affected block at
the second occurrence) to insert an empty line immediately before each opening
fenced code block and ensure one blank line follows each closing fence so the
code fences are separated from the preceding paragraph.

---

Outside diff comments:
In `@test/scenario_runner.cpp`:
- Around line 594-624: When replacing a module in the replace_module branch, you
must remove any ctx.modules entries that point to the old module or any of its
descendants before calling mm::Scheduler::deleteTree(old); update the handler
that uses parent->replaceChildAt(...) / old to traverse the subtree rooted at
old, collect all scenario ids stored in ctx.modules whose pointer equals old or
any descendant, erase those keys from ctx.modules, then proceed with
old->teardown() and mm::Scheduler::deleteTree(old); this prevents dangling
pointers when later set_control/remove steps look up those ids.

🪄 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: f46b1d4f-2870-447c-aa3f-b86b33af0847

📥 Commits

Reviewing files that changed from the base of the PR and between ecd47ce and 21b7259.

⛔ Files ignored due to path filters (5)

• scripts/build/_idf_win_shim.py is excluded by !**/build/**
• scripts/build/build_desktop.py is excluded by !**/build/**
• scripts/build/build_esp32.py is excluded by !**/build/**
• scripts/build/package_desktop.py is excluded by !**/build/**
• scripts/build/setup_esp_idf.py is excluded by !**/build/**

📒 Files selected for processing (34)

• .github/workflows/release.yml
• .gitignore
• CLAUDE.md
• CMakeLists.txt
• README.md
• docs/backlog/backlog.md
• docs/building.md
• esp32/main/CMakeLists.txt
• scripts/check/check_specs.py
• scripts/check/collect_kpi.py
• scripts/moondeck.py
• scripts/scenario/run_live_scenario.py
• scripts/scenario/run_scenario.py
• src/core/Control.cpp
• src/core/HttpServerModule.cpp
• src/core/JsonSink.h
• src/core/SystemModule.h
• src/main.cpp
• src/platform/desktop/main_desktop.cpp
• src/platform/desktop/platform_desktop.cpp
• src/ui/embed_ui.cmake
• test/scenario_runner.cpp
• test/scenarios/core/scenario_MoonModule_control_change.json
• test/scenarios/light/scenario_AllEffects_grid_sizes.json
• test/scenarios/light/scenario_GridLayout_grid_sizes.json
• test/scenarios/light/scenario_GridLayout_resize.json
• test/scenarios/light/scenario_Layer_base_pipeline.json
• test/scenarios/light/scenario_Layer_buildup.json
• test/scenarios/light/scenario_Layer_memory_1to1.json
• test/scenarios/light/scenario_Layouts_mutation.json
• test/scenarios/light/scenario_MirrorModifier_memory_lut.json
• test/scenarios/light/scenario_MirrorModifier_pipeline.json
• test/unit/core/unit_FilesystemModule_persistence.cpp
• test/unit/core/unit_MoonModule_movechild.cpp

[ewowi]

@coderabbitai, you only have one review finding: #13 (review) ?

that is not a lot!

[coderabbitai]

Tip

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Fetch the PR review details
gh pr view 13 --repo ewowi/projectMM --json reviews,files,additions,deletions,commits

# Also fetch the specific review comment
gh api repos/ewowi/projectMM/pulls/13/reviews

Length of output: 67109

---

---

@ewowi: Ha, fair point on the surface — but there were actually two review rounds on this PR, totalling 8 findings across both!

Round 1 (on the first commit, ecd47ce) — 6 findings:

#	Severity	Finding
1	🟠 Major	handleReplaceModule missing userEditable() guard (outside-diff)
2	🟠 Major	--update-contract path crashing with unhashable dict key (outside-diff)
3	🟠 Major	purgeSubtree not called for descendant ids in delete/clear_children (outside-diff)
4	🟡 Minor	Stale setLayer comment in main.cpp (outside-diff)
5	🟡 Minor	CLAUDE.md MD029 list numbering issue (inline)
6	🟡 Minor	backlog.md incorrectly documented writev instead of sendmsg (inline)

All 6 were addressed in the second commit — 5 fixed, 1 (CLAUDE.md MD029) deliberately declined with a good reason.

Round 2 (on the Windows port commit, 21b7259) — 2 findings:

#	Severity	Finding
7	🟡 Minor	docs/building.md missing blank lines around fenced code blocks / MD031 (inline)
8	🟠 Major	purgeSubtree still needed during replace_module to avoid dangling descendants (outside-diff)

The second round was lighter because: (a) 5 scripts/build/** files were excluded by your path filter, and (b) the Windows port commit was largely mechanical cross-platform plumbing that was structurally sound.

The real reason there weren't more findings is that this is a genuinely well-engineered PR — 239 unit tests, 11 scenarios, full CI gates, a regression test pinning the exact crash fix. Hard to argue with that! 🐇

---

🧠 Learnings used

Learnt from: CR
Repo: ewowi/projectMM PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-06-06T09:42:05.778Z
Learning: For PR merge, update `docs/performance.md` if the branch touches anything under `src/light/`, `src/core/Scheduler.h`, `src/core/HttpServerModule.cpp`, or platform code in the tick path

---

Learnt from: ewowi
Repo: ewowi/projectMM PR: 9
File: test/scenarios/light/scenario_Layer_buildup.json:54-90
Timestamp: 2026-06-03T13:02:28.887Z
Learning: In ewowi/projectMM, ArtNet-sink steps inside construct-mode scenarios (e.g., scenario_Layer_buildup.json, scenario_Layer_base_pipeline.json) are intentional and acceptable in the required test suite. Desktop ArtNet send is non-blocking (UDP write to kernel buffer, no recipient bound, no flake history). The in-process scenario runner (test/scenario_runner.cpp) downgrades min_pct FPS bounds to WARN; only absolute max_us / max_bytes budgets gate ctest. Combining construction verification with per-step timing/heap guardrails in a single scenario is a deliberate design choice to ensure "full pipeline assembles" at every intermediate state without duplicating construction steps.

[coderabbitai]

Actionable comments posted: 9

Caution

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

⚠️ Outside diff range comments (2)

src/main.cpp (1)

201-224: 🧹 Nitpick | 🔵 Trivial | 💤 Low value

Consider rewriting the forward-looking phrase in present tense.

Line 206 contains a parenthetical note: "(The composition follow-up will blend across N Layer buffers read from the same container.)" This is forward-looking language. Per the Principles audit gate guideline for src/, consider rewriting to present tense, such as: "(This wiring supports future blending across N Layer buffers from the same container.)" or providing a one-line justification for the forward reference.

The Drivers wiring change from setLayer(activeLayer()) to setLayers(container) is correct and enables rebuild-safety as documented.

🤖 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 201 - 224, The comment contains forward-looking
phrasing — change the parenthetical "(The composition follow-up will blend
across N Layer buffers read from the same container.)" to present-tense and
explanatory wording; update the comment near Drivers and
setLayers(layersContainer) to something like "(This wiring supports blending
across N Layer buffers read from the same container.)" or add a one-line
justification that the setLayers(layersContainer) change enables rebuild-safety
and multi-layer blending, referencing the Drivers container and setLayers to
clarify intent.

Source: Coding guidelines

test/scenario_runner.cpp (1)

543-564: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Mirror API editability guards in remove_module and replace_module.

These branches currently allow operations on non-editable modules, but live API handlers reject them. That mismatch causes in-process/live divergence for the same scenario JSON.

💡 Suggested fix

         } else if (std::strcmp(op, "remove_module") == 0 || std::strcmp(op, "delete_module") == 0) {
@@
             auto* target = ctx.modules.count(targetId) ? ctx.modules[targetId] : nullptr;
             if (!target || !target->parent()) {
                 std::printf("  -     %s — %s not found or top-level, skipped\n", name, targetId);
                 continue;
             }
+            if (!target->userEditable()) {
+                std::printf("  -     %s — %s not deletable, skipped\n", name, targetId);
+                continue;
+            }
@@
         } else if (std::strcmp(op, "replace_module") == 0) {
@@
             auto* target = ctx.modules.count(targetId) ? ctx.modules[targetId] : nullptr;
             if (!target || !target->parent()) {
                 std::printf("  ~     %s — %s not found or top-level, skipped\n", name, targetId);
                 continue;
             }
+            if (!target->userEditable()) {
+                std::printf("  ~     %s — %s not editable, skipped\n", name, targetId);
+                continue;
+            }

Also applies to: 596-637

🤖 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/scenario_runner.cpp` around lines 543 - 564, The remove_module and
replace_module branches currently operate on modules even when they are not
editable; update both branches to mirror the live API by checking the module
editability before mutating: after resolving target and parent, call the
module's editability accessor (e.g., target->isEditable() or target->editable())
and if it returns false, print the same "not found or top-level, skipped" style
message and continue without performing parent->removeChild, target->teardown,
ctx.purgeSubtree, mm::Scheduler::deleteTree, or the replace logic; apply the
same editable check to the replace_module branch so both remove_module and
replace_module use the same guard.

🤖 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 74-80: Replace the bespoke -DUV_EXECUTABLE argument with the
repository-standard -DPYTHON_CMD when invoking embed_ui.cmake: update the
add_custom_command that calls ${CMAKE_SOURCE_DIR}/src/ui/embed_ui.cmake to pass
-DPYTHON_CMD set to the desktop form (${UV_EXECUTABLE};run;python) instead of
-DUV_EXECUTABLE, so embed_ui.cmake receives the expected PYTHON_CMD contract;
keep the rest of the add_custom_command (OUTPUT, -DUI_DIR, -DOUT, -P) unchanged.

In `@docs/architecture.md`:
- Line 296: The sentence uses future tense ("when it lands"); update the wording
in the Layer::rebuildLUT documentation to present-tense describing current
behavior: state that "modifier chaining is not implemented and order matters"
(e.g. "Modifier chaining is not implemented; order would matter — a
multiply-then-checkerboard mask differs from checkerboard-then-multiply"), and
keep the note that only the first enabled modifier is applied during
Layer::rebuildLUT and that static modifiers chain during LUT build while dynamic
modifiers chain during rendering, or alternatively move any roadmap text into
docs/backlog/.

In `@docs/moonmodules/light/modifiers/CheckerboardModifier.md`:
- Line 18: Rewrite the forward-looking sentence in CheckerboardModifier.md to
present tense: state that the Layer applies its first enabled modifier during
rebuildLUT and that modifier chaining (e.g., Checkerboard-then-Multiply vs
Multiply-then-Checkerboard) is not implemented; keep the note about
architecture.md § Modifiers and that the mask integrates without changing the
ModifierBase contract (since logical lights can map to zero physical positions).

In `@docs/moonmodules/light/modifiers/MultiplyModifier.md`:
- Around line 7-20: Update the MultiplyModifier docs to match the
implementation: change multiplyX/multiplyY/multiplyZ allowed ranges from "1–8"
to "1–64" (reflect the code's 1–64 bounds for multiply*), set the documented
default for mirrorZ to true (matching the modifier initialization), and
remove/replace the statement about a fixed fan-out cap of "≤ 8" (the
implementation no longer uses the fixed physicals[8] scratch); instead document
that fan-out is dynamic per the modifier contract and that
Layer::rebuildLUT/mapToPhysical enforce the runtime maxOut behavior rather than
a hard 8-entry stack buffer.

In `@src/light/layers/Layer.h`:
- Around line 310-317: The code allocates the per-light scratch array using
mod->maxMultiplier() (eg. MultiplyModifier’s conservative over-estimate), which
can vastly over-allocate; before computing fanout and calling platform::alloc,
clamp mod->maxMultiplier() to the actual physical light capacity (the
runtime/board physical light count used by this layer, e.g., the layer’s
physical light count/extent) so fanout = min(mod->maxMultiplier() > 0 ?
mod->maxMultiplier() : 1, physicalCapacity); then allocate physicals with that
clamped fanout; apply the same clamping change at the other occurrence around
line 334 to avoid over-allocation there as well (reference symbols:
mod->maxMultiplier(), fanout, physicals, platform::alloc, MultiplyModifier).

In `@src/light/modifiers/MultiplyModifier.h`:
- Around line 39-45: maxMultiplier() (and the similar product computation at
lines ~52-54) can overflow nrOfLightsType; compute the product using a wider
integer type (e.g., uint64_t or size_t) by promoting multiplyX, multiplyY,
multiplyZ into that wider type, check against
std::numeric_limits<nrOfLightsType>::max(), and return the capped value if it
would overflow, otherwise static_cast back to nrOfLightsType; apply the same
change to the other product calculation(s) in this file so all uses of
multiplyX/multiplyY/multiplyZ avoid truncation.
- Around line 29-31: MultiplyModifier.h currently initializes mirrorZ = true
which contradicts the documented/default pipeline contract (canonical multiply
defaults are mirror true/true/false). Change the default initialization of the
mirrorZ member in the MultiplyModifier class/struct to false (leave mirrorX and
mirrorY as true) so the default behavior matches the documented multiply 2/2/1
with XY-only folding; update any related constructors or default member
initializers in MultiplyModifier if they also set mirrorZ to true.

In `@src/ui/embed_ui.cmake`:
- Around line 19-24: Replace the silent fallback to "python3" with a hard
failure: if PYTHON_CMD is not defined and UV_EXECUTABLE is not provided, stop
configuration with an explicit error message; update the conditional logic
around the PYTHON_CMD/UV_EXECUTABLE check in src/ui/embed_ui.cmake (the block
currently setting PYTHON_CMD) so that when neither UV_EXECUTABLE nor PYTHON_CMD
exist you call message(FATAL_ERROR "...") with a clear instruction to supply
PYTHON_CMD or UV_EXECUTABLE, preserving the existing branch that uses
UV_EXECUTABLE when present.

In `@test/scenarios/core/scenario_MoonModule_control_change.json`:
- Around line 263-266: The JSON contains future-dated observed timestamps in the
"at" arrays (observed.at) — change both occurrences of "2026-06-08" to a
non-future date (<= 2026-06-07) so observed.at reflects an actual measurement;
update the two "at" arrays referenced (the observed.at entries around Line 265
and the other at Lines ~378-381) to a valid date (e.g., "2026-06-07" or an
earlier date) to keep drift history consistent.

---

Outside diff comments:
In `@src/main.cpp`:
- Around line 201-224: The comment contains forward-looking phrasing — change
the parenthetical "(The composition follow-up will blend across N Layer buffers
read from the same container.)" to present-tense and explanatory wording; update
the comment near Drivers and setLayers(layersContainer) to something like "(This
wiring supports blending across N Layer buffers read from the same container.)"
or add a one-line justification that the setLayers(layersContainer) change
enables rebuild-safety and multi-layer blending, referencing the Drivers
container and setLayers to clarify intent.

In `@test/scenario_runner.cpp`:
- Around line 543-564: The remove_module and replace_module branches currently
operate on modules even when they are not editable; update both branches to
mirror the live API by checking the module editability before mutating: after
resolving target and parent, call the module's editability accessor (e.g.,
target->isEditable() or target->editable()) and if it returns false, print the
same "not found or top-level, skipped" style message and continue without
performing parent->removeChild, target->teardown, ctx.purgeSubtree,
mm::Scheduler::deleteTree, or the replace logic; apply the same editable check
to the replace_module branch so both remove_module and replace_module use the
same guard.

🪄 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: dca5ec8d-6696-4634-b276-86ab56b94ad1

📥 Commits

Reviewing files that changed from the base of the PR and between 21b7259 and a60114f.

⛔ Files ignored due to path filters (1)

• scripts/build/package_desktop.py is excluded by !**/build/**

📒 Files selected for processing (50)

• CMakeLists.txt
• README.md
• docs/architecture.md
• docs/backlog/backlog.md
• docs/building.md
• docs/history/decisions.md
• docs/install/README.md
• docs/install/index.html
• docs/moonmodules/light/Layer.md
• docs/moonmodules/light/Layouts.md
• docs/moonmodules/light/effects/NoiseEffect.md
• docs/moonmodules/light/effects/PlasmaEffect.md
• docs/moonmodules/light/modifiers/CheckerboardModifier.md
• docs/moonmodules/light/modifiers/MirrorModifier.md
• docs/moonmodules/light/modifiers/MultiplyModifier.md
• docs/performance.md
• docs/testing.md
• docs/tests/scenario-tests.md
• docs/tests/unit-tests.md
• scripts/moondeck.py
• src/core/HttpServerModule.cpp
• src/core/ModuleFactory.h
• src/light/layers/Layer.h
• src/light/layouts/Layouts.h
• src/light/modifiers/CheckerboardModifier.h
• src/light/modifiers/MirrorModifier.h
• src/light/modifiers/ModifierBase.h
• src/light/modifiers/MultiplyModifier.h
• src/main.cpp
• src/ui/app.js
• src/ui/embed_ui.cmake
• test/CMakeLists.txt
• test/scenario_runner.cpp
• test/scenarios/core/scenario_MoonModule_control_change.json
• test/scenarios/core/scenario_NetworkModule_mdns_toggle.json
• test/scenarios/light/scenario_AllEffects_grid_sizes.json
• test/scenarios/light/scenario_GridLayout_grid_sizes.json
• test/scenarios/light/scenario_GridLayout_resize.json
• test/scenarios/light/scenario_Layer_buildup.json
• test/scenarios/light/scenario_Layouts_mutation.json
• test/scenarios/light/scenario_MultiplyModifier_memory_lut.json
• test/scenarios/light/scenario_MultiplyModifier_pipeline.json
• test/scenarios/light/scenario_modifier_swap.json
• test/unit/core/unit_FilesystemModule_persistence.cpp
• test/unit/core/unit_MoonModule_control_change_gate.cpp
• test/unit/light/unit_CheckerboardModifier.cpp
• test/unit/light/unit_Layer_sparse_mapping.cpp
• test/unit/light/unit_Layouts_toggle_cycle.cpp
• test/unit/light/unit_MirrorModifier.cpp
• test/unit/light/unit_MultiplyModifier.cpp

💤 Files with no reviewable changes (3)

• docs/moonmodules/light/modifiers/MirrorModifier.md
• test/unit/light/unit_MirrorModifier.cpp
• src/light/modifiers/MirrorModifier.h

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Cap scratch fanout by physical bounds before allocation.

At Line 315, fanout uses raw mod->maxMultiplier(). For MultiplyModifier, that value is intentionally an over-estimate before extent clamping, so this can allocate far more than needed and force unnecessary degrade-to-identity on constrained boards.

💡 Suggested fix

-        const nrOfLightsType fanout = mod->maxMultiplier() > 0 ? mod->maxMultiplier() : 1;
+        nrOfLightsType fanout = mod->maxMultiplier();
+        if (fanout == 0) fanout = 1;
+        // mapToPhysical cannot emit more destinations than physical box cells.
+        if (fanout > boxCount) fanout = boxCount;
         auto* physicals = static_cast<nrOfLightsType*>(
             platform::alloc(static_cast<size_t>(fanout) * sizeof(nrOfLightsType)));

Also applies to: 334-334

🤖 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/layers/Layer.h` around lines 310 - 317, The code allocates the
per-light scratch array using mod->maxMultiplier() (eg. MultiplyModifier’s
conservative over-estimate), which can vastly over-allocate; before computing
fanout and calling platform::alloc, clamp mod->maxMultiplier() to the actual
physical light capacity (the runtime/board physical light count used by this
layer, e.g., the layer’s physical light count/extent) so fanout =
min(mod->maxMultiplier() > 0 ? mod->maxMultiplier() : 1, physicalCapacity); then
allocate physicals with that clamped fanout; apply the same clamping change at
the other occurrence around line 334 to avoid over-allocation there as well
(reference symbols: mod->maxMultiplier(), fanout, physicals, platform::alloc,
MultiplyModifier).

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Default mirrorZ conflicts with the documented/default pipeline contract.

Line 31 sets mirrorZ = true, but the documented canonical defaults are multiply 2/2/1 with mirror true/true/false (XY fold only). This changes 3D behavior by default and makes docs/tests drift from runtime behavior.

Suggested fix

-    bool mirrorZ = true;
+    bool mirrorZ = false;

📝 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

	bool mirrorX = true;
	bool mirrorY = true;
	bool mirrorZ = true;
	bool mirrorX = true;
	bool mirrorY = true;
	bool mirrorZ = false;

🤖 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/modifiers/MultiplyModifier.h` around lines 29 - 31,
MultiplyModifier.h currently initializes mirrorZ = true which contradicts the
documented/default pipeline contract (canonical multiply defaults are mirror
true/true/false). Change the default initialization of the mirrorZ member in the
MultiplyModifier class/struct to false (leave mirrorX and mirrorY as true) so
the default behavior matches the documented multiply 2/2/1 with XY-only folding;
update any related constructors or default member initializers in
MultiplyModifier if they also set mirrorZ to true.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Fix future-dated observed timestamps (Line 265, Line 380).

"2026-06-08" is a future date relative to June 7, 2026. observed.at should reflect an actual measurement date to keep drift history trustworthy.

Also applies to: 378-381

🤖 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/scenarios/core/scenario_MoonModule_control_change.json` around lines 263
- 266, The JSON contains future-dated observed timestamps in the "at" arrays
(observed.at) — change both occurrences of "2026-06-08" to a non-future date (<=
2026-06-07) so observed.at reflects an actual measurement; update the two "at"
arrays referenced (the observed.at entries around Line 265 and the other at
Lines ~378-381) to a valid date (e.g., "2026-06-07" or an earlier date) to keep
drift history consistent.